This was previously changed in 9ba1fff29e4794615c599e59ef453848a9bdb880,
without updating the documentation.
Co-authored-by: stringintech <stringintech@gmail.com>
1d9f1cb4bd6b119e1d56cbdd7f6ce4d4521fffa3 kernel: improve BlockChecked ownership semantics (stickies-v)
9ba1fff29e4794615c599e59ef453848a9bdb880 kernel: refactor: ConnectTip to pass block pointer by value (stickies-v)
Pull request description:
Subscribers to the BlockChecked validation interface event may need access to the block outside of the callback scope. Currently, this is only possible by copying the block, which makes exposing this validation interface event publicly either cumbersome or with significant copy overhead.
By using shared_ptr, we make the shared ownership explicit and allow users to safely use the block outside of the callback scope. By using a const-ref shared_ptr, no atomic reference count cost is incurred if a subscriber does not require block ownership.
For example: in #30595, this would allow us to drop the `kernel_BlockPointer` handle entirely, and generalize everything into `kernel_Block`. This PoC is implemented in https://github.com/stickies-v/bitcoin/commits/kernel/remove-blockpointer/.
---
### Performance
I have added a benchmark in a [separate branch](https://github.com/stickies-v/bitcoin/commits/2025-07/validation-interface-ownership-benched/), to ensure this change does not lead to a problematic performance regression. Since most of the overhead comes from the subscribers, I have added scenarios for `One`, `Two`, and `Ten` subscribers. From these results, it appears there is no meaningful performance difference on my machine.
When `BlockChecked()` takes a `const CBlock&` reference _(master)_:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 170.09 | 5,879,308.26 | 0.3% | 0.01 | `BlockCheckedOne`
| 1,603.95 | 623,460.10 | 0.5% | 0.01 | `BlockCheckedTen`
| 336.00 | 2,976,173.37 | 1.1% | 0.01 | `BlockCheckedTwo`
When `BlockChecked()` takes a `const std::shared_ptr<const CBlock>&` _(this PR)_:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 172.20 | 5,807,155.33 | 0.1% | 0.01 | `BlockCheckedOne`
| 1,596.79 | 626,254.52 | 0.0% | 0.01 | `BlockCheckedTen`
| 333.38 | 2,999,603.17 | 0.3% | 0.01 | `BlockCheckedTwo`
ACKs for top commit:
achow101:
ACK 1d9f1cb4bd6b119e1d56cbdd7f6ce4d4521fffa3
w0xlt:
reACK 1d9f1cb4bd
ryanofsky:
Code review ACK 1d9f1cb4bd6b119e1d56cbdd7f6ce4d4521fffa3. These all seem like simple changes that make sense
TheCharlatan:
ACK 1d9f1cb4bd6b119e1d56cbdd7f6ce4d4521fffa3
yuvicc:
Code Review ACK 1d9f1cb4bd6b119e1d56cbdd7f6ce4d4521fffa3
Tree-SHA512: 7ed0cccb7883dbb1885917ef749ab7cae5d60ee803b7e3145b2954d885e81ba8c9d5ab1edb9694ce6b308235c621094c029024eaf99f1aab1b47311c40958095
fab2980bdc55b5c77f574f879a6ab62db5eda427 assumevalid: log every script validation state change (Lőrinc)
Pull request description:
The `-assumevalid` option skips script verification for a specified block and all its ancestors during Initial Block Download.
Many new [users are surprised](https://github.com/bitcoin/bitcoin/issues/32832) when this suddenly slows their node to a halt.
This commit adds a log message to clearly indicate when this optimization ends and full validation begins (and vice versa).
<details>
<summary>Testing instructions</summary>
The behavior can easily be tested by adding this before the new log:
```C++
// TODO hack to enable/disable script checks based on block height for testing purposes
if (pindex->nHeight < 100) fScriptChecks = false;
else if (pindex->nHeight < 200) fScriptChecks = true;
else if (pindex->nHeight < 300) fScriptChecks = false;
else if (pindex->nHeight < 400) fScriptChecks = true;
```
and exercise the new code with:
```bash
cmake -B build && cmake --build build && mkdir -p demo && build/bin/bitcoind -datadir=demo -stopatheight=500 | grep 'signature validation'
```
showing something like:
* Disabling signature validations at block #1 (00000000839a8e6886ab5951d76f411475428afc90947ee320161bbf18eb6048).
* Enabling signature validations at block #100 (000000007bc154e0fa7ea32218a72fe2c1bb9f86cf8c9ebf9a715ed27fdb229a).
* Disabling signature validations at block #200 (000000008f1a7008320c16b8402b7f11e82951f44ca2663caf6860ab2eeef320).
* Enabling signature validations at block #300 (0000000062b69e4a2c3312a5782d7798b0711e9ebac065cd5d19f946439f8609).
</details>
ACKs for top commit:
achow101:
ACK fab2980bdc55b5c77f574f879a6ab62db5eda427
ajtowns:
crACK fab2980bdc55b5c77f574f879a6ab62db5eda427
davidgumberg:
untested crACK fab2980bdc
Tree-SHA512: e90b66f7423b639356daace476942ce83e65e70466544394cbe2f15738bdbf716163eaf590c64c5448f9b41aeeaafe3342c48c6a7a478678a70b0310ca94e11d
c0d91fc69c67e6f7123326d4f3caeac069d2637b Add release note for #33050 and #33183 error string changes (Antoine Poinsot)
b3f781a0ef4b763ef7ba8b5b20871a7707ec090e contrib: adapt max reject string size in tracing demo (Antoine Poinsot)
9a04635432183c437829339dbf10e7d702581010 scripted-diff: validation: rename mandatory errors into block errors (Antoine Poinsot)
Pull request description:
This is a followup to #33050 now that it's merged. Using "block"/"mempool" as the error reason is clearer to a user than "mandatory"/"non-mandatory". The "non-mandatory" errors got renamed to "mempool" in #33050 (see https://github.com/bitcoin/bitcoin/pull/33050#discussion_r2230103371). This takes care of the second part of the renaming.
ACKs for top commit:
fjahr:
utACK c0d91fc69c67e6f7123326d4f3caeac069d2637b
davidgumberg:
lgtm ACK c0d91fc69c
ajtowns:
utACK c0d91fc69c67e6f7123326d4f3caeac069d2637b
Crypt-iQ:
utACK c0d91fc69c67e6f7123326d4f3caeac069d2637b
janb84:
utACK c0d91fc69c67e6f7123326d4f3caeac069d2637b
instagibbs:
ACK c0d91fc69c67e6f7123326d4f3caeac069d2637b
Tree-SHA512: b463e633c57dd1eae7c49d23239a59066a672f355142ec194982eddc927a7646bc5cde583dc8d6f45075bf5cbb96dbe73f7e339e728929b0eff356b674d1b68c
2b00030af84efd50cfc60125db4ae49da6b7aa9a interfaces, chain, refactor: Remove inaccurate getActiveChainLocator (pablomartin4btc)
110a0f405cd696ebfd6edce07c1b347723e84a0f interfaces, chain, refactor: Remove unused getTipLocator (pablomartin4btc)
Pull request description:
Remove `Chain::getTipLocator`, `Chain::GetLocator()`, and `Chain::getActiveChainLocator`:
- `Chain::getTipLocator` is no longer used.
- `Chain::GetLocator`, replaced its call by `GetLocator()`, which uses `LocatorEntries`, avoiding direct access to the chain itself (change suggested by l0rinc while reviewing this PR to maintain consistency with the overall refactoring).
- `Chain::getActiveChainLocator`, whose name was misleading, has functionality redundant with Chain::findBlock.
- Additionally, the comment for getActiveChainLocator became inaccurate following changes in commit ed470940cd (from PR #25717).
This is a [follow-up](https://github.com/bitcoin/bitcoin/pull/29652#issuecomment-3151665095) to #29652.
ACKs for top commit:
achow101:
ACK 2b00030af84efd50cfc60125db4ae49da6b7aa9a
furszy:
ACK 2b00030af84efd50cfc60125db4ae49da6b7aa9a
stickies-v:
ACK 2b00030af84efd50cfc60125db4ae49da6b7aa9a
w0xlt:
ACK 2b00030af8
Tree-SHA512: b12ba6a15feeaeec692d69204a6e155e3af43edfac25597dabf14cacca1e4a2152574816e58dc544f39043c5721f5e707acf544f4541d3b9c0f8c0c40069215e
de0675f9de5feae1f070840ad7218b1378fb880b refactor: Move `transaction_identifier.h` to primitives (marcofleon)
6f068f65de17951dc459bc8637e5de15b84ca445 Remove implicit uint256 conversion and comparison (marcofleon)
9c24cda72edb2085edfa75296d6b42fab34433d9 refactor: Convert remaining instances from uint256 to Txid (marcofleon)
d2ecd6815d89c9b089b55bc96fdf93b023be8dda policy, refactor: Convert uint256 to Txid (marcofleon)
f6c0d1d23128f742dfdda253752cba7db9bb0679 mempool, refactor: Convert uint256 to Txid (marcofleon)
aeb0f783305c923ee7667c46ca0ff7e1b96ed45c refactor: Convert `mini_miner` from uint256 to Txid (marcofleon)
326f24472487dc7f447839136db2ccf60833e9a2 refactor: Convert RPCs and `merkleblock` from uint256 to Txid (marcofleon)
49b3d3a92a7250e80c56ff8c351cf1670e32c1a2 Clean up `FindTxForGetData` (marcofleon)
Pull request description:
This is the final leg of the [type safety refactor](https://github.com/bitcoin/bitcoin/pull/32189).
All of these changes are straightforward `uint256` --> `Txid` along with any necessary explicit conversions. Also, `transaction_identifier.h` is moved to primitives in the last commit, as `Txid` and `Wtxid` become fundamental types after this PR.
ACKs for top commit:
stickies-v:
re-ACK de0675f9de5feae1f070840ad7218b1378fb880b, no changes since a20724d926d5844168c6a13fa8293df8c8927efe except address review nits.
janb84:
re ACK de0675f9de5feae1f070840ad7218b1378fb880b
dergoegge:
re-ACK de0675f9de5feae1f070840ad7218b1378fb880b
theStack:
Code-review ACK de0675f9de5feae1f070840ad7218b1378fb880b
Tree-SHA512: 2413160fca7ab146a8d79d18ce3afcf7384cacc73c513d41928904aa453b4dd7a350064cee71e9c5d015da5904c7c81ac17603e50a47441ebc5b0c653235dd08
Using "block" or "mempool" as the prefix in place of "mandatory" or "non-mandatory" is clearer
to a user. "non-mandatory" was renamed into "mempool" as part of #33050. This takes care of the
other half of this renaming as a scripted diff.
-BEGIN VERIFY SCRIPT-
sed -i 's/mandatory-script-verify/block-script-verify/g' $(git grep -l mandatory-script-verify)
-END VERIFY SCRIPT-
Also removed CChain::GetLocator() and replaced its call
with GetLocator() which uses LocatorEntries instead.
Co-authored-by: ryanofsky <ryan@ofsky.org>
Co-authored-by: l0rinc <l0rinc@users.noreply.github.com>
876dbdfb4702410dfd4037614dc9298a0c09c63e tests: drop expect_disconnect behaviour for tx relay (Anthony Towns)
b29ae9efdfeeff774e32ee433ce67d8ed8ecd49f validation: only check input scripts once (Anthony Towns)
266dd0e10d08c0bfde63205db15d6c210a021b90 net_processing: drop MaybePunishNodeForTx (Anthony Towns)
Pull request description:
Because we do not discourage nodes for transactions we consider non-standard, we don't get any DoS protection from this check in adversarial scenarios, so remove the check entirely both to simplify the code and reduce the risk of splitting the network due to changes in tx relay policy.
Then, because we no longer make use of the distinction between consensus and standardness failures during script validation, don't re-validate each script with only-consensus rules, reducing the cost to us of transactions that we won't relay.
ACKs for top commit:
achow101:
ACK 876dbdfb4702410dfd4037614dc9298a0c09c63e
darosior:
re-ACK 876dbdfb4702410dfd4037614dc9298a0c09c63e
sipa:
re-ACK 876dbdfb4702410dfd4037614dc9298a0c09c63e
glozow:
ACK 876dbdfb4702410dfd4037614dc9298a0c09c63e
Tree-SHA512: 8bb0395766dde54fc48f7077b80b88e35581aa6e3054d6d65735965147abefffa7348f0850bb3d46f6c2541fd384ecd40a00a57fa653adabff8a35582e2d1811
554befd8738ea993b3b555e7366558a9c32c915c test: revive `getcoinscachesizestate` (Lőrinc)
64ed0fa6b7a2b637f236c3abf2f045adf6a067cf refactor: modernize `LargeCoinsCacheThreshold` (Lőrinc)
1b40dc02a6a292239037ac5a44e0d0c9506d5fa2 refactor: extract `LargeCoinsCacheThreshold` from `GetCoinsCacheSizeState` (Lőrinc)
Pull request description:
After the changes in https://github.com/bitcoin/bitcoin/pull/25325 `getcoinscachesizestate` [always ended the test early](https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/test/validation_flush_tests.cpp.gcov.html#L65):
| File | Line Rate | Line Total | Line Hit | Branch Rate | Branch Total | Branch Hit |
|------------------------------|---------:|-----------:|---------:|------------:|-------------:|-----------:|
| validation_flush_tests.cpp | **31.5 %** | 54 | 17 | 22.3 % | 242 | 54 |
The test revival was [extracted from a related PR](https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2109417797) where it was [discovered](https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2044004503).
ACKs for top commit:
achow101:
ACK 554befd8738ea993b3b555e7366558a9c32c915c
LarryRuane:
ACK 554befd8738ea993b3b555e7366558a9c32c915c
w0xlt:
ACK 554befd873
Tree-SHA512: f5057254de8fb3fa627dd20fee6818cfadeb2e9f629f9972059ad7b32e01fcd7dc9922eff9da2d363b36a9f0954d9bc1c4131d47b2a9c6cc348d9864953b91be
These remaining miscellaneous changes were identified by commenting out
the `operator const uint256&` conversion and the `Compare(const uint256&)`
method from `transaction_identifier.h`.
d3b8a54a81209420ef6447dd4581e1b6b8550647 Refactor CFeeRate to use FeeFrac internally (Pol Espinasa)
Pull request description:
The `FeeFrac` type represents a fraction, intended to be used for `sats/vbyte` or `sats/WU`. It was added to improve accuracy when evaluating fee rates in cluster mempool. [1]
But it can also be used to fix the precision issues that the current `CFeeRate` class has now.
At the moment, `CFeeRate` handles the fee rate as satoshis per kilovirtualbyte: `CAmount / kvB` using an integer.
This PR fix `CFeeRate` precision issues by encapsulating `FeeFrac` internally keeping backwards compatibility.
This PR can also be used as a based to use multiple units on RPC calls as detailed in this issue [2].
Some previous discussions:
[1] https://github.com/bitcoin/bitcoin/pull/30535
[2] https://github.com/bitcoin/bitcoin/issues/32093
ACKs for top commit:
achow101:
ACK d3b8a54a81209420ef6447dd4581e1b6b8550647
murchandamus:
code review, lightly tested ACK d3b8a54a81209420ef6447dd4581e1b6b8550647
ismaelsadeeq:
re-ACK d3b8a54a81209420ef6447dd4581e1b6b8550647 📦
theStack:
Code-review ACK d3b8a54a81209420ef6447dd4581e1b6b8550647
Tree-SHA512: 5a8149d81e82ad4e60a0e76ff6a82a5b1c4e212cf5156c1cdd16bf9acbb351e7be458eac3f0a2ae89107f331062b299c1d9ca649d3b820ad0b68e6d1a14292e5
The `-assumevalid` option skips script verification for a specified block and all its ancestors during Initial Block Download.
Many new users are surprised when this suddenly slows their node to a halt.
This commit adds a log message to clearly indicate when this optimization ends and full validation begins (and vice versa).
When using `-assumeutxo`, logging is suppressed for the active assumed-valid chainstate and for the background validation chainstate to avoid the confusing toggles.
-------
> cmake -B build && cmake --build build && mkdir -p demo && build/bin/bitcoind -datadir=demo -stopatheight=500 | grep 'signature validation'
```
2025-08-08T20:59:21Z Disabling signature validations at block #1 (00000000839a8e6886ab5951d76f411475428afc90947ee320161bbf18eb6048).
2025-08-08T20:59:21Z Enabling signature validations at block #100 (000000007bc154e0fa7ea32218a72fe2c1bb9f86cf8c9ebf9a715ed27fdb229a).
2025-08-08T20:59:21Z Disabling signature validations at block #200 (000000008f1a7008320c16b8402b7f11e82951f44ca2663caf6860ab2eeef320).
2025-08-08T20:59:21Z Enabling signature validations at block #300 (0000000062b69e4a2c3312a5782d7798b0711e9ebac065cd5d19f946439f8609).
```
Previously, we would check failing input scripts twice when considering
a transaction for the mempool, in order to distinguish policy failures
from consensus failures. This allowed us both to provide a different
error message and to discourage peers for consensus failures. Because we
are no longer discouraging peers for consensus failures during tx relay,
and because checking a script can be expensive, only do this once.
Also renames non-mandatory-script-verify-flag error to
mempool-script-verify-flag-failed.
27aefac42505e9c083fa131d3d7edbec7803f3c0 validation: detect witness stripping without re-running Script checks (Antoine Poinsot)
2907b58834ab011f7dd0c42d323e440abd227c25 policy: introduce a helper to detect whether a transaction spends Segwit outputs (Antoine Poinsot)
eb073209db9efdbc2c94bc1f535a27ec6b20d954 qa: test witness stripping in p2p_segwit (Antoine Poinsot)
Pull request description:
Since it was introduced in 4eb515574e1012bc8ea5dafc3042dcdf4c766f26 (#18044), the detection of a stripped witness relies on running the Script checks 3 times. In the worst case, this consists in running Script validation for every single input 3 times.
Detection of a stripped witness is necessary because in this case wtxid==txid, and the transaction's wtxid must not be added to the reject filter or it could allow a malicious peer to interfere with txid-based orphan resolution as used in 1p1c package relay.
However it is not necessary to run Script validation to detect a stripped witness (much less so doing it 3 times in a row). There are 3 types of witness program: defined program types (Taproot, P2WPKH and P2WSH), undefined types, and the Pay-to-anchor carve-out.
For defined program types, Script validation with an empty witness will always fail (by consensus). For undefined program types, Script validation is always going to fail regardless of the witness (by standardness). For P2A, an empty witness is never going to lead to a failure.
Therefore it holds that we can always detect a stripped witness without re-running Script validation. However this might lead to more "false positives" (cases where we return witness stripping for an otherwise invalid transaction) than the existing implementation. For instance a transaction with one P2PKH input with an invalid signature and one P2WPKH input with its witness stripped. The existing implementation would treat it as consensus invalid while the implementation in this PR would always consider it witness stripped.
h/t AJ: this essentially implements a variant of https://github.com/bitcoin/bitcoin/pull/33066#issuecomment-3135258539.
ACKs for top commit:
sipa:
re-ACK 27aefac42505e9c083fa131d3d7edbec7803f3c0
Crypt-iQ:
re-ACK 27aefac42505e9c083fa131d3d7edbec7803f3c0
glozow:
reACK 27aefac42505e9c083fa131d3d7edbec7803f3c0
Tree-SHA512: 70cf76b655b52bc8fa2759133315a3f11140844b6b80d9de3c95f592050978cc01a87bd2446e3a9c25cc872efea7659d6da3337b1a709511771fece206e9f149
Since it was introduced in 4eb515574e1012bc8ea5dafc3042dcdf4c766f26 (#18044), the detection of a
stripped witness relies on running the Script checks 3 times. In the worst case, this consists in
running Script validation 3 times for every single input.
Detection of a stripped witness is necessary because in this case wtxid==txid, and the transaction's
wtxid must not be added to the reject filter or it could allow a malicious peer to interfere with
txid-based orphan resolution as used in 1p1c package relay.
However it is not necessary to run Script validation to detect a stripped witness (much less so
doing it 3 times in a row). There are 3 types of witness program: defined program types (Taproot,
P2WPKH, P2WSH), undefined types, and the Pay-to-anchor carve-out.
For defined program types, Script validation with an empty witness will always fail (by consensus).
For undefined program types, Script validation is always going to fail regardless of the witness (by
standardness). For P2A, an empty witness is never going to lead to a failure.
Therefore it holds that we can always detect a stripped witness without re-running Script validation.
However this might lead to more "false positives" (cases where we return witness stripping for an
otherwise invalid transaction) than the existing implementation. For instance a transaction with one
P2PKH input with an invalid signature and one P2WPKH input with its witness stripped. The existing
implementation would treat it as consensus invalid while the implementation in this commit would
always consider it witness stripped.
ea17a9423fb431a86d36927b02d3624f654fd867 [doc] release note for relaxing requirement of all unconfirmed parents present (glozow)
12f48d5ed302e92a334dbe971c66df467d402655 test: add chained 1p1c propagation test (Greg Sanders)
525be56741cff5f89d596b8a0c44df00f2209bcb [unit test] package submission 2p1c with 1 parent missing (glozow)
f24771af0581e8117bd638227469e37cc69c5103 relax child-with-unconfirmed-parents rule (glozow)
Pull request description:
Broadens the package validation interface, see #27463 for wider context.
On master, package rules include that (1) the package topology must be child-wth-parents (2) all of the child's unconfirmed parents must be present. This PR relaxes the second rule, leaving the first rule untouched (there are plans to change that as well, but not here).
Original motivation for this rule was based on the idea that we would have a child-with-unconfirmed-parents package relay protocol, and this would verify that the peer provided the "correct" package. For various reasons, we're not planning on doing this. We could potentially do this for ancestor packages (with a similar definition that all UTXOs to make the tx valid are available in this package), but it's also questionable whether it's useful to enforce this.
This rule gets in the way of certain usage of 1p1c package relay currently. If a transaction has multiple parents, of which only 1 requires a package CPFP, this rule blocks the package from relaying. Even if all the non-low-feerate parents are already in mempool, when the p2p logic submits the 1p1c package, it gets rejected for not meeting this rule.
ACKs for top commit:
ishaanam:
re-utACK ea17a9423fb431a86d36927b02d3624f654fd867
instagibbs:
ACK ea17a9423fb431a86d36927b02d3624f654fd867
Tree-SHA512: c2231761ae7b2acea10a96735e7a36c646f517964d0acb59bacbae1c5a1950e0223458b84c6d5ce008f0c1d53c1605df0fb3cd0064ee535ead006eb7c0fa625b
Subscribers to the BlockChecked validation interface event may need
access to the block outside of the callback scope. Currently, this
is only possible by copying the block, which makes exposing this
validation interface event publicly either cumbersome or with significant
copy overhead.
By using shared_ptr, we make the shared ownership explicit and allow
users to safely use the block outside of the callback scope.
b6d4688f77df9e31fd64e2be300f55bb8e944bd0 [doc] reword comments in test_mid_package_replacement (glozow)
f3a613aa5bb780f5d85821fa00742f5a99df81b5 [cleanup] delete brittle test_mid_package_eviction (glozow)
c3cd7fcb2cd9f9911f8251a6f29120f65c63ea37 [doc] remove references to now-nonexistent Finalize() function (glozow)
d8140f5f050076bfa129169c1223cab94b243a6e don't make a copy of m_non_base_coins (glozow)
98ba2b1db2eb81e08b550ba0d5069a9289f30e13 [doc] MemPoolAccept coins views (glozow)
ba02c30b8a635db88100916a1bb9c40fb0b47ab6 [doc] always CleanupTemporaryCoins after a mempool trim (glozow)
Pull request description:
Deletes `test_mid_package_eviction` that is brittle and already covered in other places. It was introduced in #28251 addressing 2 issues: (1) calling `LimitMempoolSize()` in the middle of package validation and (2) not updating coins view cache when the mempool contents change, leading to "disappearing coins."
(1) If you let `AcceptSingleTransaction` call `LimitMempoolSize` in the middle of package validation, you should get a failure in `test_mid_package_eviction_success` (the package is rejected):
```
diff --git a/src/validation.cpp b/src/validation.cpp
index f2f6098e214..4bd6f059849 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1485,7 +1485,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
FinalizeSubpackage(args);
// Limit the mempool, if appropriate.
- if (!args.m_package_submission && !args.m_bypass_limits) {
+ if (!args.m_bypass_limits) {
LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip());
// If mempool contents change, then the m_view cache is dirty. Given this isn't a package
// submission, we won't be using the cache anymore, but clear it anyway for clarity.
```
Mempool modifications have a pretty narrow interface since #31122 and `TrimToSize()` cannot be called while there is an outstanding mempool changeset. So I think there is a low likelihood of accidentally reintroducing this problem and not immediately hitting e.g. a fuzzer crash on this line b53fab1467/src/txmempool.cpp (L1143)
(2) If you remove the `CleanupTemporaryCoins()` call from `ClearSubPackageState()` you should get a failure from `test_mid_package_replacement`:
```
diff --git a/src/validation.cpp b/src/validation.cpp
index f2f6098e214..01b904b69ef 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -779,7 +779,7 @@ private:
m_subpackage = SubPackageState{};
// And clean coins while at it
- CleanupTemporaryCoins();
+ // CleanupTemporaryCoins();
}
};
```
I also added/cleaned up the documentation about coins views to hopefully make it extremely clear when people should `CleanupTemporaryCoins`.
ACKs for top commit:
instagibbs:
reACK b6d4688f77
sdaftuar:
utACK b6d4688f77df9e31fd64e2be300f55bb8e944bd0
marcofleon:
ACK b6d4688f77df9e31fd64e2be300f55bb8e944bd0
Tree-SHA512: 79c68e263013b1153520f5453e6b579b8fe7e1d6a9952b1ac2c3c3c017034e6d21d7000a140bba4cc9d2ce50ea3a84cc6f91fd5febc52d7b3fa4f797955d987d
Move-only commit, enabled reusing the large cache size calculation logic later. The only difference is the removal of the `static` keyword (since in a constexpr function it's a C++23 extension)
It is redundant with -logsourcelocations and the log messages are
clearer without it.
Also, remove a double-space.
Also, add braces around `if` touched in the next commit.
This tiny behavior change requires a test fixup.
This rule was originally introduced along with a very early proposal for
package relay as a way to verify that the "correct"
child-with-unconfirmed-parents package was provided for a transaction,
where correctness was defined as all of the transactions unconfirmed
parents. However, we are not planning to introduce a protocol where
peers would be asked to send these packages.
This rule has downsides: if a transaction has multiple parents but only
1 that requires package CPFP to be accepted, the current rule prevents
us from accepting that package. Even if the other parents are already in
mempool, the p2p logic will only submit the 1p1c package, which fails
this check. See the test in p2p_1p1c_network.py
a60f863d3e276534444571282f432b913d3967db scripted-diff: Replace GenTxidVariant with GenTxid (marcofleon)
c8ba1995986323cd9e76097acc1f15eed7c60943 Remove old GenTxid class (marcofleon)
072a198ea4bc9f1e8449cd31e55d397b75ce4ad5 Convert remaining instances of GenTxid to GenTxidVariant (marcofleon)
1b528391c79497ae19f7e481439e350533c7cd1a Convert `txrequest` to GenTxidVariant (marcofleon)
bde4579b0780aa3754af35beffbcfeb31f28045b Convert `txdownloadman_impl` to GenTxidVariant (marcofleon)
c876a892ec0b04851bea0a688d7681b6aaca4cb7 Replace GenTxid with Txid/Wtxid overloads in `txmempool` (marcofleon)
de858ce2bea83c53635dee9a49c8c273a12440dd move-only: make GetInfo a private CTxMemPool member (stickies-v)
eee473d9f3019a0ea4ebbc9c41781813ad574a86 Convert `CompareInvMempoolOrder` to GenTxidVariant (marcofleon)
243553d59071f3e43a42f3809706790495b17ffc refactor: replace get_iter_from_wtxid with GetIter(const Wtxid&) (stickies-v)
fcf92fd640eae60d1f601136a4e1c9de8ccb68b5 refactor: make CTxMemPool::GetIter strongly typed (marcofleon)
11d28f21bb8f0c3094934b3fef45871f73bb216a Implement GenTxid as a variant (marcofleon)
Pull request description:
Part of the [type safety refactor](https://github.com/bitcoin/bitcoin/pull/32189).
This PR changes the GenTxid class to a variant, which holds both Txids and Wtxids. This provides compile-time type safety and eliminates the manual type check (bool m_is_wtxid). Variables that can be either a Txid or a Wtxid are now using the new GenTxid variant, instead of uint256.
ACKs for top commit:
w0xlt:
ACK a60f863d3e
dergoegge:
Code review ACK a60f863d3e276534444571282f432b913d3967db
maflcko:
review ACK a60f863d3e276534444571282f432b913d3967db 🎽
theStack:
Code-review ACK a60f863d3e276534444571282f432b913d3967db
Tree-SHA512: da9b73b7bdffee2eb9281a409205519ac330d3336094d17681896703fbca8099608782c9c85801e388e4d90af5af8abf1f34931f57bbbe6e9674d802d6066047
8cc3ac6c2328873e57c31f237d194c5f22b6748a validation: Don't use IsValid() to filter for invalid blocks (Martin Zumsande)
86d98b94e5469e6476dc87f54b8ac25074603f0c test: verify that ancestors of a reconsidered block can become the chain tip (stratospher)
3c39a55e64bedfc384f6a7b9de1410bc8b46a9bb validation: Add ancestors of reconsiderblock to setBlockIndexCandidates (Martin Zumsande)
Pull request description:
When we call `reconsiderblock` for some block, `Chainstate::ResetBlockFailureFlags` puts the descendants of that block into `setBlockIndexCandidates` (if they meet the criteria, i.e. have more work than the tip etc.), but never put any ancestors into the set even though we do clear their failure flags.
I think that this is wrong, because `setBlockIndexCandidates` should always contain all eligible indexes that have at least as much work as the current tip, which can include ancestors of the reconsidered block. This is being checked by `CheckBlockIndex()`, which could fail if it was invoked after `ActivateBestChain` connects a block and releases `cs_main`:
``` diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 7b04bd9a5b..ff0c3c9f58 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3551,6 +3551,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
}
}
// When we reach this point, we switched to a new tip (stored in pindexNewTip).
+ m_chainman.CheckBlockIndex();
if (exited_ibd) {
// If a background chainstate is in use, we may need to rebalance our
```
makes `rpc_invalidateblock.py` fail on master.
Even though we don't currently have a `CheckBlockIndex()` in that place, after `cs_main` is released other threads could invoke it, which is happening in the rare failures of #16444 where an invalid header received from another peer could trigger a `CheckBlockIndex()` call that would fail.
Fix this by adding eligible ancestors to `setBlockIndexCandidates` in `Chainstate::ResetBlockFailureFlags` (also simplifying that function a bit).
Fixes#16444
ACKs for top commit:
achow101:
ACK 8cc3ac6c2328873e57c31f237d194c5f22b6748a
TheCharlatan:
Re-ACK 8cc3ac6c2328873e57c31f237d194c5f22b6748a
stratospher:
reACK 8cc3ac6.
Tree-SHA512: 53f27591916246be4093d64b86a0494e55094abd8c586026b1247e4a36747bc3d6dbe46dc26ee4a22f47b8eb0d9699d13e577dee0e7198145f3c9b11ab2a30b7
To mitigate disk-filling attacks caused by unsafe usages of LogPrintf and
friends, we rate-limit them by passing a should_ratelimit bool that
eventually makes its way to LogPrintStr which may call
LogRateLimiter::Consume. The rate limiting is accomplished by
adding a LogRateLimiter member to BCLog::Logger which tracks source
code locations for the given logging window.
Every hour, a source location can log up to 1MiB of data. Source
locations that exceed the limit will have their logs suppressed for the
rest of the window determined by m_limiter.
This change affects the public LogPrintLevel function if called with
a level >= BCLog::Level::Info.
The UpdateTipLog function has been changed to use the private LogPrintLevel_
macro with should_ratelimit set to false. This allows UpdateTipLog to log
during IBD without hitting the rate limit.
Note that on restart, a source location that was rate limited before the
restart will be able to log until it hits the rate limit again.
Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
9341b5333ad54ccdb7c16802ff06c51b956948e7 blockstorage: make block read hash checks explicit (Lőrinc)
2371b9f4ee0b108ebbb8afedc47d73ce0f97d272 test/bench: verify hash in `ComputeFilter` reads (Lőrinc)
5d235d50d6dd0cc23175a1484e8ebb6cdc6e2183 net: assert block hash in `ProcessGetBlockData` and `ProcessMessage` (Lőrinc)
Pull request description:
A follow-up to https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2094072165, after which validating the hash of a read block from disk doesn't incur the cost of calculating its hash anymore.
### Summary
This PR adds explicit checks that the read block header's hash matches the one we were expecting.
### Context
After the previous PR, validating a block's hash during read operations became essentially free. This PR leverages that by requiring callers to provide a block's expected hash (or `std::nullopt`), preventing silent failures caused by corrupted or mismatched data. Most `ReadBlock` usages were updated with expected hashes and now fail on mismatch.
### Changes
* added hash assertions in `ProcessGetBlockData` and `ProcessMessage` to validate that the block read from disk matches the expected hash;
* updated tests and benchmark to pass the correct block hash to `ReadBlock()`, ensuring the hash validation is tested - or none if we already expect PoW failure;
* removed the default value for `expected_hash`, requiring an explicit hash for all block reads.
### Why is the hash still optional (but no longer has a default value)
* for header-error tests, where the goal is to trigger failures early in the parsing process;
* for out-of-order orphan blocks, where the child hash isn't available before the initial disk read.
ACKs for top commit:
maflcko:
review ACK 9341b5333ad54ccdb7c16802ff06c51b956948e7 🕙
achow101:
ACK 9341b5333ad54ccdb7c16802ff06c51b956948e7
hodlinator:
ACK 9341b5333ad54ccdb7c16802ff06c51b956948e7
janb84:
re ACK 9341b5333ad54ccdb7c16802ff06c51b956948e7
Tree-SHA512: cf1d4fff4c15e3f8898ec284929cb83d7e747125d4ee759e77d369f1716728e843ef98030be32c8d608956a96ae2fbefa0e801200c333b9eefd6c086ec032e1f
a18e57232867d946bc35769632fed49e1bf1464f test: more template verification tests (Sjors Provoost)
10c908808fb80cd4fbde9d377079951b91944755 test: move gbt proposal mode tests to new file (Sjors Provoost)
94959b8deedcff98a55c87b5e473890b2e7a3b16 Add checkBlock to Mining interface (Sjors Provoost)
6077157531c1cec6dea8e6f90b4df8ef7b5cec4e ipc: drop BlockValidationState special handling (Sjors Provoost)
74690f4ed82b1584abb07c0387db0d924c4c0cab validation: refactor TestBlockValidity (Sjors Provoost)
Pull request description:
This PR adds the IPC equivalent of the `getblocktemplate` RPC in `proposal` mode.
In order to do so it has `TestBlockValidity` return error reasons as a string instead of `BlockValidationState`. This avoids complexity in IPC code for handling the latter struct.
The new Mining interface method is used in `miner_tests`.
It's not used by the `getblocktemplate` and `generateblock` RPC calls, see https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2096473337
The `inconclusive-not-best-prevblk` check is moved from RPC
code to `TestBlockValidity`.
Test coverage is increased by `mining_template_verification.py`.
Superseedes #31564
## Background
### Verifying block templates (no PoW)
Stratum v2 allows miners to generate their own block template. Pools may wish (or need) to verify these templates. This typically involves comparing mempools, asking miners to providing missing transactions and then reconstructing the proposed block.[^0] This is not sufficient to ensure a proposed block is actually valid. In some schemes miners could take advantage of incomplete validation[^1].
The Stratum Reference Implementation (SRI), currently the only Stratum v2 implementation, collects all missing mempool transactions, but does not yet fully verify the block.[^2]. It could use the `getblocktemplate` RPC in `proposal` mode, but using IPC is more performant, as it avoids serialising up to 4 MB of transaction data as JSON.
(although SRI could use this PR, the Template Provider role doesn't need it, so this is _not_ part of #31098)
[^0]: https://github.com/stratum-mining/sv2-spec/blob/main/06-Job-Declaration-Protocol.md
[^1]: https://delvingbitcoin.org/t/pplns-with-job-declaration/1099/45?u=sjors
[^2]: https://github.com/stratum-mining/stratum/blob/v1.1.0/roles/jd-server/src/lib/job_declarator/message_handler.rs#L196
ACKs for top commit:
davidgumberg:
reACK a18e572328
achow101:
ACK a18e57232867d946bc35769632fed49e1bf1464f
TheCharlatan:
ACK a18e57232867d946bc35769632fed49e1bf1464f
ryanofsky:
Code review ACK a18e57232867d946bc35769632fed49e1bf1464f just adding another NONFATAL_UNREACHABLE since last review
Tree-SHA512: 1a6c29f45a1666114f10f55aed155980b90104db27761c78aada4727ce3129e6ae7a522d90a56314bd767bd7944dfa46e85fb9f714370fc83e6a585be7b044f1
Comments are expanded.
Return BlockValidationState instead of passing a reference.
Lock Chainman mutex instead of cs_main.
Remove redundant chainparams and pindexPrev arguments.
Drop defaults for checking proof-of-work and merkle root.
The ContextualCheckBlockHeader check is moved to after CheckBlock,
which is more similar to normal validation where context-free checks
are done first.
Validation failure reasons are no longer printed through LogError(),
since it depends on the caller whether this implies an actual bug
in the node, or an externally sourced block that happens to be invalid.
When called from getblocktemplate, via BlockAssembler::CreateNewBlock(),
this method already throws an std::runtime_error if validation fails.
Additionally it moves the inconclusive-not-best-prevblk check from RPC
code to TestBlockValidity.
There is no behavior change when callling getblocktemplate with proposal.
Previously this would return a BIP22ValidationResult which can throw for
state.IsError(). But CheckBlock() and the functions it calls only use
state.IsValid().
The final assert is changed into Assume, with a LogError.
Co-authored-by: <Ryan Ofsky <ryan@ofsky.org>
Dropped the default expected_hash parameter from `ReadBlock()`.
In `blockmanager_flush_block_file` tests, we pass {} since the tests would already fail at PoW validation for corrupted blocks.
In `ChainstateManager::LoadExternalBlockFile`, we pass {} when processing child blocks because their hashes aren't known beforehand.
029ba1a21d570f7db6c4366ec9a30a381b56d6fb index: remove CBlockIndex access from CustomAppend() (furszy)
91b7ab6c69264a46f70825546a1574478d9e824a refactor: index, simplify CopyHeightIndexToHashIndex to process single block (furszy)
6f1392cc42cde638773f2b697d7d2c58abcdc860 indexes, refactor: Remove remaining CBlockIndex* uses in index Rewind methods (Ryan Ofsky)
0a248708dc9d465db09168c39b3f12cb4c9465b7 indexes, refactor: Stop requiring CBlockIndex type to call IsBIP30Unspendable (Ryan Ofsky)
331a25cb16632042dd6782a9b62fcc5c8aa6da3b test: indexes, avoid creating threads when sync runs synchronously (furszy)
Pull request description:
Combining common refactors from #24230 and #26966, aiming to move both efforts forward while reducing their size and review burden.
Broadly, #24230 focuses on enabling indexes to run in a separate process, and #26966 aims to parallelize the indexes initial synchronization process. A shared prerequisite for both is ensuring that only the base index class interacts with the node’s chain internals - child index classes should instead operate solely through chain events.
This PR moves disk read lookups from child index classes to the base index class. It also includes a few documentation improvements and a test-only code cleanup.
ACKs for top commit:
maflcko:
review ACK 029ba1a21d570f7db6c4366ec9a30a381b56d6fb 👡
achow101:
ACK 029ba1a21d570f7db6c4366ec9a30a381b56d6fb
TheCharlatan:
Re-ACK 029ba1a21d570f7db6c4366ec9a30a381b56d6fb
davidgumberg:
ACK 029ba1a21d570f7db6c
mzumsande:
Code Review ACK 029ba1a21d570f7db6c4366ec9a30a381b56d6fb
Tree-SHA512: f073af407fc86f228cb47a32c7bcf2241551cc89ff32059317eb81d5b86fd5fda35f228d2567e0aedbc9fd6826291f5fee05619db35ba44108421ae04d11e6fb
IsValid() also returns false for blocks that have not been
validated yet up to the default validity level of BLOCK_VALID_TRANSACTIONS but
are not marked as invalid - e.g. if we only know the header.
Here, we specifically want to filter for invalid blocks.
Also removes the default arg from IsValid() which is now unused outside
of tests, to prevent this kind of misuse for the future.
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
When we call reconsiderblock for some block, ResetBlockFailureFlags puts the descendants of that block
into setBlockIndexCandidates (if they meet the criteria, i.e. have more work than the tip etc.)
We also clear the failure flags of the ancestors, but we never put any of those into setBlockIndexCandidates
this is wrong and could lead to failures in CheckBlockIndex().
f6b782f3aad4a6bcf823a9a0fabb4418bca1eea1 doc: Improve m_best_header documentation (Martin Zumsande)
ee673b9aa0157d94220722491f135aef23142521 validation: remove m_failed_blocks (Martin Zumsande)
ed764ea2b4edb3cf1925a4bff5f39567a8be54ac validation: Add more checks to CheckBlockIndex() (Martin Zumsande)
9a70883002e1fee76c24810808af4fb43f2c8cf5 validation: in invalidateblock, calculate m_best_header right away (Martin Zumsande)
8e39f2d20d09592035ae048d0cfe955c733310d9 validation: in invalidateblock, mark children as invalid right away (Martin Zumsande)
4c29326183ba3c9d0b198cb2cec37c3119862c19 validation: cache all headers with enough PoW in invalidateblock (Martin Zumsande)
15fa5b5a908d1019fc1b3042901b42bee0a1bd95 validation: call InvalidBlockFound also from AcceptBlock (Martin Zumsande)
Pull request description:
Some fields in validation are set opportunistically by "best effort":
- The `BLOCK_FAILED_CHILD` status (which means that the block index has an invalid predecessor)
- `m_best_header` (the most-work header not known to be invalid).
This means that there are known situations in which these fields are not set when they should be, or set to wrong values. This is tolerated because the fields are not used for anything consensus-critical and triggering these situations involved creating invalid blocks with valid PoW header, so would have a cost attached. Also, having stricter guarantees for these fields requires iterating over the entire block index, which has some DoS potential, especially with any header above the checkpoint being accepted int he past (see e.g. #11531).
However, there are reasons to change this now:
- RPCs use these fields and can report wrong results
- There is the constant possibility that someone could add code that expects these fields to be correct, especially because it is not well documented that these fields cannot always be relied upon.
- DoS concerns have become less of an issue after #25717 - now an attacker would need to invest much more work because they can't fork off the last checkpoint anymore
This PR continues the work from #30666 to ensure that `BLOCK_FAILED_CHILD` status and `m_best_header` are always correct:
- it adds a call to `InvalidChainFound()` in `AcceptBlock()`.
- it adds checks for `BLOCK_FAILED_CHILD` and `m_best_header` to `CheckBlockIndex()`. In order to be able to do this, the existing cache in the RPC-only `InvalidateBlock()` is adjusted to handle these as well. These are performance optimizations with the goal of avoiding having a call of `InvalidChainFound()` / looping over the block index after each disconnected block.
I also wrote a fuzz test to find possible edge cases violating `CheckBlockIndex`, which I will PR separately soon.
- it removes the `m_failed_blocks` set, which was a heuristic necessary when we couldn't be sure if a given block index had an invalid predecessor or not. Now that we have that guarantee, the set is no longer needed.
ACKs for top commit:
stickies-v:
re-ACK f6b782f3aad4a6bcf823a9a0fabb4418bca1eea1
achow101:
reACK f6b782f3aad4a6bcf823a9a0fabb4418bca1eea1
ryanofsky:
Code review ACK f6b782f3aad4a6bcf823a9a0fabb4418bca1eea1 with only minor code & comment updates
TheCharlatan:
Re-ACK f6b782f3aad4a6bcf823a9a0fabb4418bca1eea1
Tree-SHA512: 1bee324216eeee6af401abdb683abd098b18212833f9600dbc0a46244e634cb0e6f2a320c937a5675a12af7ec4a7d10fabc1db9e9bc0d9d0712e6e6ca72d084f
After changes in previous commits, we now mark all blocks that descend from an invalid block
immediately as the block is found invalid. This happens both in the AcceptBlock
and ConnectBlock stages of block validation.
As a result, the pindexPrev->nStatus check in AcceptBlockHeader is now sufficient to detect
invalid blocks and checking m_failed_blocks there is no longer necessary.
This adds checks that
1) Descendants of invalid block indexes are also marked invalid
2) m_best_header cannot be invalid, and there can be no valid
block with more work than it.
Before, m_best_header would be calculated only after disconnecting
multiple blocks, letting go of cs_main in the meantime.
This is in preparation for adding checks to CheckBlockIndex()
requiring that m_best_header is the most-work header not known to be invalid.
Co-authored-by: stringintech <stringintech@gmail.com>
Before, they would be marked as invalid only after disconnecting
multiple blocks, letting go of cs_main in the meantime.
This is in preparation for adding a check to
CheckBlockIndex() requiring that descendants of invalid block indexes
are always marked as invalid.
Entries from highpow_outofchain_headers are now only removed if made invalid,
no longer after inserting into setBlockIndexCandidates, because they
might still become invalid later in the second case.
This means that blocks could be inserted multiple times now into
setBlockIndexCandidates - this won't have any effect, so
behavior isn't changed.
We now include blocks without HaveNumChainTxs() / lower validation status
than BLOCK_VALID_TRANSACTIONS. These checks are still performed at the
spot where we use the cache to insert into setBlockIndexCandidates.
This is in preparation for using the cache for more things than
just setBlockIndexCandidates candidates in the following commits.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
When a block it found invalid during acceptance (but before connection)
we now mark its descendants with BLOCK_FAILED_CHILD and update
m_best_header when these things weren't done reliably before.
This does not fix a serious bug because the flags and m_best_header were being set on a best-effort basis before
and not used for anything critical.
Setting these reliably has a slight performance cost (iterating over the
entire block index) but leads to more consistency in validation and allows removing m_failed_blocks in a later commit.
This can only be triggered by providing a block with sufficient PoW
that is otherwise invalid, so it is not a DoS vector.
