`Consensus::CheckTxInputs` calls `tx.GetValueOut()` and assumes output-range checks already ran.
Document the mempool and block validation call paths where this is guaranteed.
Co-authored-by: Antoine Poinsot <darosior@protonmail.com>
After the previous conditions were validated, document why `bad-txns-fee-outofrange` is unreachable once `nValueIn` and `value_out` are `MoneyRange` and `nValueIn >= value_out`.
Although unreachable, keep the check itself in place (instead of removing) as it's part of consensus-critical code; the comment serves as a proof for future refactors.
Inspired by b-c-cov coverage reports:
* "bad-txns-fee-outofrange" - https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/consensus/tx_verify.cpp.gcov.html#L200
3dd815f048c80c9a35f01972e0537eb42531aec7 validation: pre-reserve leaves to prevent reallocs with odd vtx count (Lőrinc)
7fd47e0e56087cd3b5fd76a532bdc3ac331e832e bench: make `MerkleRoot` benchmark more representative (Lőrinc)
f0a21831087410687c4ca31ac00e44f380b859be test: adjust `ComputeMerkleRoot` tests (Lőrinc)
Pull request description:
#### Summary
`ComputeMerkleRoot` [duplicates the last hash](39b6c139bd/src/consensus/merkle.cpp (L54-L56)) when the input size is odd. If the caller provides a `std::vector` whose capacity equals its size, that extra `push_back` forces a reallocation, doubling its capacity (causing peak memory usage of 3x the necessary size).
This affects roughly half of the created blocks (those with odd transaction counts), causing unnecessary memory fragmentation during every block validation.
#### Fix
* Pre-reserves vector capacity to account for the odd-count duplication using `(size + 1) & ~1ULL`.
* This syntax produces [optimal assembly](https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2553107836) across x86/ARM and 32/64-bit platforms for GCC & Clang.
* Eliminates default construction of `uint256` objects that are immediately overwritten by switching from `resize` to `reserve` + `push_back`.
#### Memory Impact
[Memory profiling](https://github.com/bitcoin/bitcoin/pull/32497#issuecomment-3563724551) shows **50% reduction in peak allocation** (576KB → 288KB) and elimination of reallocation overhead.
#### Validation
The benchmark was updated to use an odd leaf count to demonstrate the real-world scenario where the reallocation occurs.
A full `-reindex-chainstate` up to block **896 408** ran without triggering the asserts.
<details>
<summary>Validation asserts</summary>
Temporary asserts (not included in this PR) confirm that `push_back` never reallocates and that the coinbase witness hash remains null:
```cpp
if (hashes.size() & 1) {
assert(hashes.size() < hashes.capacity()); // TODO remove
hashes.push_back(hashes.back());
}
leaves.reserve((block.vtx.size() + 1) & ~1ULL); // capacity rounded up to even
leaves.emplace_back();
assert(leaves.back().IsNull()); // TODO remove
```
</details>
#### Benchmark Performance
While the main purpose is to improve predictability, the reduced memory operations also improve hashing throughput slightly.
ACKs for top commit:
achow101:
ACK 3dd815f048c80c9a35f01972e0537eb42531aec7
optout21:
reACK 3dd815f048c80c9a35f01972e0537eb42531aec7
hodlinator:
re-ACK 3dd815f048c80c9a35f01972e0537eb42531aec7
vasild:
ACK 3dd815f048c80c9a35f01972e0537eb42531aec7
w0xlt:
ACK 3dd815f048c80c9a35f01972e0537eb42531aec7 with minor nits.
danielabrozzoni:
Code review ACK 3dd815f048
Tree-SHA512: e7b578f9deadc0de7d61c062c7f65c5e1d347548ead4a4bb74b056396ad7df3f1c564327edc219670e6e2b2cb51f4e1ccfd4f58dd414aeadf2008d427065c11f
24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94 merkle: remove unused `mutated` arg from `BlockWitnessMerkleRoot` (Lőrinc)
63d640fa6a7090b4e615153b42ebf2e48d909db0 merkle: remove unused `proot` and `pmutated` args from `MerkleComputation` (Lőrinc)
be270551df30dd42b4f1b664234d0c22c09be625 merkle: migrate `path` arg of `MerkleComputation` to a reference (Lőrinc)
Pull request description:
### Summary
Simplifies merkle tree computation by removing dead code found through coverage analysis (following up on #33768 and #33786).
### History
#### BlockWitnessMerkleRoot
Original `MerkleComputation` was added in ee60e5625b (diff-706988c23877f8a557484053887f932b2cafb3b5998b50497ce7ff8118ac85a3R131) where it was called for either `&hash, mutated` or `position, &ret` args.
In 1f0e7ca09c (diff-706988c23877f8a557484053887f932b2cafb3b5998b50497ce7ff8118ac85a3L135-L165) the first usage was inlined in `ComputeMerkleRoot`, leaving the `proot` and , `pmutated` values unused in `MerkleComputation`.
Later in 4defdfab94 the method was moved to test and in 63d6ad7c89 (diff-706988c23877f8a557484053887f932b2cafb3b5998b50497ce7ff8118ac85a3R87-R95) was restored to the code, though with unused parameters again.
#### BlockWitnessMerkleRoot
`BlockWitnessMerkleRoot` was introduced in 8b49040854 where it was already called with `NULL` 8b49040854 (diff-34d21af3c614ea3cee120df276c9c4ae95053830d7f1d3deaf009a4625409ad2R3509) or an unused dummy 8b49040854 (diff-34d21af3c614ea3cee120df276c9c4ae95053830d7f1d3deaf009a4625409ad2R3598-R3599) for the `mutated` parameter.
### Fixes
#### BlockWitnessMerkleRoot
- Converts `path` parameter from pointer to reference (always non-null at call site)
- Removes `proot` and `pmutated` parameters (always `nullptr` at call site)
#### BlockWitnessMerkleRoot
- Removes unused `mutated` output parameter (always passed as `nullptr`)
The change is a refactor that shouldn't introduce *any* behavioral change, only remove dead code, leftovers from previous refactors.
### Coverage proof
https://maflcko.github.io/b-c-cov/total.coverage/src/consensus/merkle.cpp.gcov.html
ACKs for top commit:
optout21:
utACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94
Sjors:
utACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94
achow101:
ACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94
sedited:
ACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94
hodlinator:
ACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94
Tree-SHA512: 6960411304631bc381a3db7a682f6b6ba51bd58936ca85aa237c69a9109265b736b22ec4d891875bddfcbe8517bd3f014c44a4b387942eee4b01029c91ec93e1
The `mutated` parameter is never used at any call site - all callers pass `nullptr`.
The explicit comment in `validation.cpp` explains the reason:
// The malleation check is ignored; as the transaction tree itself
// already does not permit it, it is impossible to trigger in the
// witness tree.
There's a single call to the methods from `ComputeMerklePath` where the last parameter is always provided.
This simplifies the implementation by not having to check for missing parameter.
Previously the SCRIPT_VERIFY_* flags were specified as either uint32_t,
unsigned int, or unsigned. This converts them to a common type alias in
preparation for changing the underlying type.
These remaining miscellaneous changes were identified by commenting out
the `operator const uint256&` conversion and the `Compare(const uint256&)`
method from `transaction_identifier.h`.
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~0 )
-END VERIFY SCRIPT-
This can be reproduced according to the developer notes with something
like
( cd ./src/ && ../contrib/devtools/run-clang-tidy.py -p ../bld-cmake -fix -j $(nproc) )
Also, the header related changes were done manually.
e3014017bacff42d8d69f3061ce1ee621aaa450a test: add IsActiveAfter tests for versionbits (Anthony Towns)
60950f77c35e54e2884cfc14ab67623f3e325099 versionbits: docstrings for BIP9Info (Anthony Towns)
7565563bc7a5bb98ebf03a7d6881912a74d3f302 tests: refactor versionbits fuzz test (Anthony Towns)
2e4e9b9608c722aaf767638e9dba498d8dc3e772 tests: refactor versionbits unit test (Anthony Towns)
525c00f91bb27d0f2a1b2e5532aebec7fac97d3a versionbits: Expose VersionBitsConditionChecker via impl header (Anthony Towns)
e74a7049b477d1853191ded75fdf25024a6e233f versionbits: Expose StateName function (Anthony Towns)
d00d1ed52c8ee95eeed665d68d6715a694bd4c1f versionbits: Split out internal details into impl header (Anthony Towns)
37b9b67a39554465104c9cf1a74690f40019dbad versionbits: Simplify VersionBitsCache API (Anthony Towns)
1198e7d2fd665bf2bc49fd26773d4fd5fbc2b716 versionbits: Move BIP9 status logic for getblocktemplate to versionbits (Anthony Towns)
b1e967c3ec92738affb22d3b58483ebcdd8dfea2 versionbits: Move getdeploymentinfo logic to versionbits (Anthony Towns)
3bd32c20550e69688a4ff02409fb34b9a637b9c4 versionbits: Move WarningBits logic from validation to versionbits (Anthony Towns)
5da119e5d0e61f0b583f0fe21b9a00ee815a3e46 versionbits: Change BIP9Stats to uint32_t types (Anthony Towns)
a679040ec19ef17f3f03988a52207f1c03af701e consensus/params: Move version bits period/threshold to bip9 param (Anthony Towns)
e9d617095d4ce9525a4337d33624cac9d6b4abe6 versionbits: Remove params from AbstractThresholdConditionChecker (Anthony Towns)
9bc41f1b48b2e0cc6abf9714e860a29989d7809c versionbits: Use std::array instead of C-style arrays (Anthony Towns)
Pull request description:
Increases the encapsulation/modularity of the versionbits code, moving more of the logic into the versionbits module rather than having it scattered across validation and rpc code. Updates unit/fuzz tests to test the actual code used rather than just a close approximation of it.
ACKs for top commit:
achow101:
ACK e3014017bacff42d8d69f3061ce1ee621aaa450a
TheCharlatan:
Re-ACK e3014017bacff42d8d69f3061ce1ee621aaa450a
darosior:
ACK e3014017bacff42d8d69f3061ce1ee621aaa450a
Tree-SHA512: 2978db5038354b56fa1dd6aafd511099e9c16504d6a88daeac2ff2702c87bcf3e55a32e2f0a7697e3de76963b68b9d5ede7976ee007e45862fa306911194496d
Rather than having the rule change period/threshold be constant for all
potential deployments on a chain, have it be specific to the deployment
itself. This both matches history (BIP 9 specified a 2016 block period
and 1916 block threshold; BIP 91 specified a 336 block period and 269
block threshold; and BIP 341 specified a 2016 block period and 1815
block threshold), and allows the code to be simplified, as only the
BIP9Deployment structure is needed, not the full Consensus::Params
structure.
Belt and suspenders for future code changes.
Currently this function is only called from TransactionMerklePath() which sets leaves to the block transactions, so the Assume always holds.
The *_RECENT_CONSENSUS_CHANGE variants in the validation result
enumerations were always unused. They seem to have been kept around
speculatively for a soft fork after segwit, however they were never used
for taproot either. This points at them not having a clear purpose.
Based on the original pull requests' comments their usage was never
entirely clear:
https://github.com/bitcoin/bitcoin/pull/11639#issuecomment-370234133https://github.com/bitcoin/bitcoin/pull/15141#discussion_r271039747
Since they are part of the validation interface and need to exposed by
the kernel library keeping them around may also be confusing to future
users of the library.
The Mining interface uses this function in the next commit
to calculate the coinbase merkle path. Stratum v2 uses
this to send a compact work template.
This partially undoes the change in 4defdfab94504018f822dc34a313ad26cedc8255,
but is not a revert, because the implementation changed in the meantime.
This commit also documents the function.
In order to ensure that the change of nVersion to a uint32_t in the
previous commit has no effect, rename nVersion to version in this commit
so that reviewers can easily spot if a spot was missed or if there is a
check somewhere whose semantics have changed.
With package validation rules, transactions that fail individually may
sometimes be eligible for reconsideration if submitted as part of a
(different) package. For now, that includes trasactions that failed for
being too low feerate. Add a new TxValidationResult type to distinguish
these failures from others. In the next commits, we will abort package
validation if a tx fails for any other reason. In the future, we will
also decide whether to cache failures in recent_rejects based on this
result (we won't want to reject a package containing a transaction that
was rejected previously for being low feerate).
Package validation also sometimes elects to skip some transactions when
it knows the package will not be submitted in order to quit sooner. Add
a result to specify this situation; we also don't want to cache these
as rejections.
Instead of using the dummysigner to compute a placeholder satisfaction,
infer a descriptor on the scriptPubKey of the coin being spent and use
the estimation of the satisfaction size given by the descriptor
directly.
Note this (almost, see next paragraph) exactly conserves the previous
behaviour. For instance CalculateMaximumSignedInputSize was previously
assuming the input to be spent in a transaction that spends at least one
Segwit coin, since it was always accounting for the serialization of the
number of witness elements.
In this commit we use a placeholder for the size of the serialization of
the witness stack size (1 byte). Since the logic in this commit is
already tricky enough to review, and that it is only a very tiny
approximation not observable through the existing tests, it is addressed
in the next commit.
935acdcc79d1dc5ac04a83b92e5919ddbfa29329 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 935acdcc79d1dc5ac04a83b92e5919ddbfa29329
hebasto:
Approach ACK 935acdcc79d1dc5ac04a83b92e5919ddbfa29329.
aureleoules:
reACK 935acdcc79d1dc5ac04a83b92e5919ddbfa29329
john-moffett:
ACK 935acdcc79d1dc5ac04a83b92e5919ddbfa29329
stickies-v:
Approach ACK 935acdcc7
Tree-SHA512: 4f1ba54ff2198eea0e505d41e73d552c84c60f6878d5c85a94a8ab57f39afc94ef8d79258e7afd01fa84ec2a99f4404bb877eecd671f65e1ee9273f3129fc650
- 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
In order to prevent memory DoS, we must ensure that we don't accept a new
header into memory until we've performed anti-DoS checks, such as verifying
that the header is part of a sufficiently high work chain. This commit adds a
new argument to AcceptBlockHeader() so that we can ensure that all call-sites
which might cause a new header to be accepted into memory have to grapple with
the question of whether the header is safe to accept, or needs further
validation.
This patch also fixes two places where low-difficulty-headers could have been
processed without such validation (processing an unrequested block from the
network, and processing a compact block).
Credit to Niklas Gögge for noticing this issue, and thanks to Sjors Provoost
for test code.
c4c5b9ca6e98cf44309af73edf5559940a04d00f consensus/params: set default values for BIP9Deployment (Anthony Towns)
Pull request description:
Adds default values for `vDeployments` in `consensus/params.h` so that undefined behaviour is avoided if a deployment is not initialized. Also adds a check in the unit tests to alert if this is happening, since even if it doesn't invoke undefined behaviour it's probably a mistake.
ACKs for top commit:
laanwj:
Code review ACK c4c5b9ca6e98cf44309af73edf5559940a04d00f
Tree-SHA512: 22d7ff86a817d9e9e67c47301fc3b7e9d5821c13565d7706711f113dea220eea29b413a7c8d029691009159cebc85a108d77cb52418734091c196bafb2b12423
Add sanity asserts for return value of `CBlockIndex::GetAncestor()` where appropriate.
In validation.cpp `CheckSequenceLocks`, check the return value of `tip->GetAncestor(maxInputHeight)` stored into `lp->maxInputBlock`. If it ever returns `nullptr` because the ancestor isn't found, it's going to be a bad bug to keep going, since a `LockPoints` object with the `maxInputBlock` member set to `nullptr` signifies no relative lock time.
In the other places, the added asserts would prevent accidental dereferencing of a null pointer which is undefined behavior.
Co-Authored-By: Aurèle Oulès <aurele@oules.com>
Co-Authored-By: danra <danra@users.noreply.github.com>
While chainparams should explicilty set values for each possible
entry in vDeployments, in the past that has been missed resulting
in potential undefined behaviour due to accessing unitinitialized
data. Reduce the severity of future bugs of that nature by providing
benign default values. Adds a unit test to alert if the default value
is not overwritten for the real chains (NEVER_ACTIVE/NEVER_ACTIVE rather
than NEVER_ACTIVE/NO_TIMEOUT).
cccc1e70b8a14430cc94143da97936a60d6c83d3 Enforce Taproot script flags whenever WITNESS is set (MarcoFalke)
fa422994116a7a053789304d56159760081479eb Remove nullptr check in GetBlockScriptFlags (MarcoFalke)
faadc606c7644f2934de390e261d9d65a81a7592 refactor: Pass const reference instead of pointer to GetBlockScriptFlags (MarcoFalke)
Pull request description:
Now that Taproot is active, it makes sense to enforce its rules on all blocks, even historic ones, regardless of the deployment status.
### Benefits:
(With "script flags" I mean "taproot script verification flags".)
* Script flags are known ahead for all blocks (even blocks not yet created) and do not change. This may benefit static analysis, code review, and development of new script features that build on Taproot.
* Any future bugs introduced in the deployment code won't have any effect on the script flags, as they are independent of deployment.
* Enforcing the taproot rules regardless of the deployment status makes testing easier because invalid blocks after activation are also invalid before activation. So there is no need to differentiate the two cases.
* It gives belt-and-suspenders protection against a practically expensive and theoretically impossible IBD reorg attack where the node is eclipsed. While `nMinimumChainWork` already protects against this, the cost for a few months worth of POW might be lowered until a major version release of Bitcoin Core reaches EOL. The needed work for the attack is the difference between `nMinimumChainWork` and the work at block 709632.
For reference, previously the same was done for P2SH and WITNESS in commit 0a8b7b4b33c9d78574627fc606267e2d8955cd1c.
### Implementation:
I found one block which fails verification with the flags applied, so I added a `TaprootException`, similar to the `BIP16Exception`.
For reference, the debug log:
```
ERROR: ConnectBlock(): CheckInputScripts on b10c007c60e14f9d087e0291d4d0c7869697c6681d979c6639dbd960792b4d41 failed with non-mandatory-script-verify-flag (Witness program was passed an empty witness)
BlockChecked: block hash=0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad state=non-mandatory-script-verify-flag (Witness program was passed an empty witness)
InvalidChainFound: invalid block=0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad height=692261 log2_work=92.988459 date=2021-07-23T08:24:20Z
InvalidChainFound: current best=0000000000000000000067b17a4c0ffd77c29941b15ad356ca8f980af137a25d height=692260 log2_work=92.988450 date=2021-07-23T07:47:31Z
ERROR: ConnectTip: ConnectBlock 0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad failed, non-mandatory-script-verify-flag (Witness program was passed an empty witness)
```
Hint for testing, make sure to set `-noassumevalid`.
### Considerations
Obviously this change can lead to consensus splits on the network in light of massive reorgs. Currently the last block before Taproot activation, that is the last block without the Taproot script flags set, is only buried by a few days of POW. However, when and if this patch is included in the next major release, it will be buried by a few months of POW. BIP90 considerations apply when looking at reorgs this large.
ACKs for top commit:
Sjors:
tACK cccc1e70b8a14430cc94143da97936a60d6c83d3
achow101:
ACK cccc1e70b8a14430cc94143da97936a60d6c83d3
laanwj:
Code review ACK cccc1e70b8a14430cc94143da97936a60d6c83d3
ajtowns:
ACK cccc1e70b8a14430cc94143da97936a60d6c83d3 ; code review; wrote a "getblockscriptflags" rpc to quickly check that blocks just had bit 17 (taproot) added; review of earlier revisions had established non-exception blocks do validate with taproot rules enabled.
jamesob:
ACK cccc1e70b8a14430cc94143da97936a60d6c83d3 ([`jamesob/ackr/23536.1.MarcoFalke.enforce_taproot_script_f`](https://github.com/jamesob/bitcoin/tree/ackr/23536.1.MarcoFalke.enforce_taproot_script_f))
Tree-SHA512: 00044de68939caef6420ffd588c1291c041a8b397c80a3df1e3e3487fbeae1821d23975c51c95e44e774558db76f943b00b4e27cbd0213f64a9253116dc6edde
