3333bae9b2 tidy: modernize-use-equals-default (MarcoFalke)
Pull request description:
Prior to C++20, `modernize-use-equals-default` could have been problematic because it could turn a non-aggregate into an aggregate. The risk would be that aggregate initialization would be enabled where the author did not intend to enable it.
With C++20, aggregate for those is forbidden either way. (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1008r1.pdf)
So enabled it for code clarity, consistency, and possibly unlocking compiler optimizations. See https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html
ACKs for top commit:
stickies-v:
ACK 3333bae9b2
Tree-SHA512: ab42ff01be7ca7e7d8b4c6a485e68426f59627d83dd827cf292304829562348dc17a52ee009f5f6f3c1c2081d7166ffac4baef23197ebeba8de7767c6ddfe255
9e13ccc50e psbt: Check non witness utxo outpoint early (Ava Chow)
Pull request description:
A common issue that our fuzzers keep finding is that outpoints don't exist in the non witness utxos. Instead of trying to track this down and checking in various individual places, do the check early during deserialization. This also unifies the error message returned for this class of problems.
ACKs for top commit:
maflcko:
lgtm ACK 9e13ccc50e
S3RK:
tACK 9e13ccc50e
dergoegge:
utACK 9e13ccc50e
Tree-SHA512: 81b8055b146c6358052226578ddfec0ae5bd877968c7f4f62dc3d6a684545ea568f37c7f1bd619918441af9e453ba8b26531a2280d218da37fa15480f1b45d0e
c7376babd1 doc: Clarify distinction between util and common libraries in libraries.md (Ryan Ofsky)
4f74c59334 util: Move util/string.h functions to util namespace (Ryan Ofsky)
4d05d3f3b4 util: add TransactionError includes and namespace declarations (Ryan Ofsky)
680eafdc74 util: move fees.h and error.h to common/messages.h (Ryan Ofsky)
02e62c6c9a common: Add PSBTError enum (Ryan Ofsky)
0d44c44ae3 util: move error.h TransactionError enum to node/types.h (Ryan Ofsky)
9bcce2608d util: move spanparsing.h to script/parsing.h (Ryan Ofsky)
6dd2ad4792 util: move spanparsing.h Split functions to string.h (Ryan Ofsky)
23cc8ddff4 util: move HexStr and HexDigit from util to crypto (TheCharlatan)
6861f954f8 util: move util/message to common/signmessage (Ryan Ofsky)
cc5f29fbea build: move memory_cleanse from util to crypto (Ryan Ofsky)
5b9309420c build: move chainparamsbase from util to common (Ryan Ofsky)
ffa27af24d test: Add check-deps.sh script to check for unexpected library dependencies (Ryan Ofsky)
Pull request description:
Remove `fees.h`, `errors.h`, and `spanparsing.h` from the util library. Specifically:
- Move `Split` functions from `util/spanparsing.h` to `util/string.h`, using `util` namespace for clarity.
- Move remaining spanparsing functions to `script/parsing.h` since they are used for descriptor and miniscript parsing.
- Combine `util/fees.h` and `util/errors.h` into `common/messages.h` so there is a place for simple functions that generate user messages to live, and these functions are not part of the util library.
Motivation for this change is that the util library is a dependency of the kernel, and we should remove functionality from util that shouldn't be called by kernel code or kernel applications. These changes should also improve code organization and make functions easier to discover. Some of these same moves are (or were) part of #28690, but did not help with code organization, or made it worse, so it is better to move them and clean them up in the same PR so code only has to change one time.
ACKs for top commit:
achow101:
ACK c7376babd1
TheCharlatan:
Re-ACK c7376babd1
hebasto:
re-ACK c7376babd1.
Tree-SHA512: 5bcef16c1255463b1b69270548711e7ff78ca0dd34e300b95e3ca1ce52ceb34f83d9ddb2839e83800ba36b200de30396e504bbb04fa02c6d0c24a16d06ae523d
Add TransactionError to node namespace and include it directly instead of
relying on indirect include through common/messages.h
This is a followup to a previous commit which moved the TransactionError enum.
These changes were done in a separate followup just to keep the previous commit
more minimal and easy to review.
Add separate PSBTError enum instead of reusing TransactionError enum for PSBT
operations, and drop unused error codes. The error codes returned by PSBT
operations and transaction broadcast functions mostly do not overlap, so using
an unified enum makes it harder to call any of these functions and know which
errors actually need to be handled.
Define PSBTError in the common library because PSBT functionality is
implemented in the common library and used by both the node (for rawtransaction
RPCs) and the wallet.
A common issue that our fuzzers keep finding is that outpoints don't
exist in the non witness utxos. Instead of trying to track this down and
checking in various individual places, do the check early during
deserialization.
6e9f8bb050 rpc, tests: in `utxoupdatepsbt` also look for the transaction in the txindex (ishaanam)
a5b4883fb4 rpc: extract psbt updating logic into ProcessPSBT (ishaanam)
Pull request description:
Previously the `utxoupdatepsbt` RPC, added in #13932, only updated the inputs spending segwit outputs with the `witness_utxo`, because the full transaction is needed for legacy inputs. Before this RPC looked for just the utxos in the utxo set and the mempool. This PR makes it so that if the user has txindex enabled, then the full transaction is looked for there for all inputs. If it is not found in the txindex or txindex isn't enabled, then the mempool is checked for the full transaction. If the transaction an input is spending from is still not found at that point, then the utxo set is searched and the inputs spending segwit outputs are updated with just the utxo.
ACKs for top commit:
achow101:
ACK 6e9f8bb050
Xekyo:
ACK 6e9f8bb050
Tree-SHA512: 078db3c37a1ecd5816d80a42e8bd1341e416d661f508fa5fce0f4e1249fefd7b92a0d45f44957781cb69d0953145ef096ecdd4545ada39062be27742402dac6f
Previously only the segwit utxos being spent by the psbt were looked for and
added to the psbt. Now, the full transaction corresponding to each of these
utxos (legacy and segwit) is looked for in the txindex and mempool and added
to the psbt. If txindex is disabled and the transaction is not in the mempool,
then we fall back to getting just the utxo (if segwit) from the utxo set.
906631450d s/transcation/transaction/ (Greg Sanders)
Pull request description:
ACKs for top commit:
fanquake:
ACK 906631450d - looks like other comments are being addressed elsewhere.
Tree-SHA512: c835a14db2e0cf5e0317c95c8c7441df1f7c6cb14be7809fd947e07ea9d23f1f171f111429aabd0509b7f17601bc742041316b18e1135e547a966961f2c65038
935acdcc79 refactor: modernize the implementation of uint256.* (pasta)
Pull request description:
- Constructors of uint256 to utilize Span instead of requiring a std::vector
- converts m_data into a std::array
- Prefers using `WIDTH` instead of `sizeof(m_data)`
- make all the things constexpr
- replace C style functions with c++ equivalents
- memset -> std::fill
This may also be replaced by std::memset, but I think that std::fill is more idiomatic of modern c++ and readable.
- memcpy -> std::copy
Note: In practice, implementations of std::copy avoid multiple assignments and use bulk copy functions such as std::memmove if the value type is TriviallyCopyable and the iterator types satisfy LegacyContiguousIterator. (https://en.cppreference.com/w/cpp/algorithm/copy)
This could also likely be replaced by std::memcpy, but as said above, I believe the using std::copy is the more c++ way to do anything and is almost guaranteed to compile to the same asm
- memcmp -> std::memcmp
ACKs for top commit:
achow101:
ACK 935acdcc79
hebasto:
Approach ACK 935acdcc79.
aureleoules:
reACK 935acdcc79
john-moffett:
ACK 935acdcc79
stickies-v:
Approach ACK 935acdcc7
Tree-SHA512: 4f1ba54ff2198eea0e505d41e73d552c84c60f6878d5c85a94a8ab57f39afc94ef8d79258e7afd01fa84ec2a99f4404bb877eecd671f65e1ee9273f3129fc650
dee89438b8 Abstract out ComputeTapbranchHash (Russell O'Connor)
8e3fc99427 Do not use CScript for tapleaf scripts until the tapleaf version is known (Russell O'Connor)
Pull request description:
While BIP-341 calls the contents of tapleaf a "script", only in the case that the tapleaf version is `0xc0` is this script known to be a tapscript. Otherwise the tapleaf "script" is simply an uninterpreted string of bytes.
This PR corrects the issue where the type `CScript` is used prior to the tapleaf version being known to be a tapscript. This prevents `CScript` methods from erroneously being called on non-tapscript data.
A second commit abstracts out the TapBranch hash computation in the same manner that the TapLeaf computation is already abstracted. These two abstractions ensure that the TapLeaf and TapBranch tagged hashes are always constructed properly.
ACKs for top commit:
ajtowns:
ACK dee89438b8
instagibbs:
ACK dee89438b8
achow101:
ACK dee89438b8
sipa:
ACK dee89438b8
aureleoules:
reACK dee89438b8 - I verified that there is no behavior change.
Tree-SHA512: 4a1d37f3e9a1890e7f5eadcf65562688cc451389581fe6e2da0feb2368708edacdd95392578d8afff05270d88fc61dce732d83d1063d84d12cf47b5f4633ec7e
- Constructors of uint256 to utilize Span instead of requiring a std::vector
- converts m_data into a std::array
- Prefers using `WIDTH` instead of `sizeof(m_data)`
- make all the things constexpr
- replace C style functions with c++ equivalents
- memset -> std::fill
- memcpy -> std::copy
Note: In practice, implementations of std::copy avoid multiple assignments and use bulk copy functions such as std::memmove if the value type is TriviallyCopyable and the iterator types satisfy LegacyContiguousIterator. (https://en.cppreference.com/w/cpp/algorithm/copy)
- memcmp -> std::memcmp
Instead of having an entire TaprootBuilder which may or may not be
complete, and could potentially have future changes that interact oddly
with taproot tree tuples, have m_tap_tree be just the tuples.
When needed in other a TaprootBuilder for actual use, the tuples will be
added to a a TaprootBuilder that, in the future, can take in whatever
other data is needed as well.
Since TaprootBuilder has assertions for the depth and leaf versions, the
PSBT decoder should check these values before calling
TaprootBuilder::Add so that the assertions are not triggered on
malformed taproot trees.
To make it easier to de/serialize individual KeyOriginInfo for PSBTs,
separate the actual de/serialization of KeyOriginInfo to its own
function.
This is an additional separation where any length prefix is processed by
the caller.
Since the removal of NODISCARD in 81d5af42f4,
the only attributes def is LIFETIMEBOUND, and it's included in many more
places that it is used.
This removes all includes which do not have an associated use of LIFETIMEBOUND,
and adds it to the following files, due to their use of the same:
* src/validationinterface.h
* src/script/standard.h
Base32/base64 are mechanisms for encoding binary data. That they'd
decode to a string is just bizarre. The fact that they'd do that
based on the type of input arguments even more so.
SerializeToVector, UnserializeFromVector, DeserializeHDKeypaths, and SerializeHDKeypaths
were in sign.h where PSBT was originally implemented. Since all of the PSBT serialization
has moved to its own file, these functions should follow.
