a34fc8b11a wallet: handle disabled startup settings (Robert Hamilton)
b7113e6f42 test: characterize disabled wallet settings (Robert Hamilton)
Pull request description:
I hit a crash while creating a new wallet in Bitcoin-Qt 31.1 on an Apple silicon Mac with `nosettings=1`. After looking through the crash report and code, I traced it to saving the wallet's load-on-startup setting: the settings writer throws when dynamic settings are disabled.
Wallet RPCs report errors with `-nosettings` after changing wallet state. In Qt, the same settings write causes an uncaught exception.
Return a persistence failure when dynamic settings are disabled so wallet operations finish with their existing startup-setting warning. This avoids an uncaught exception in Qt and RPC errors after the wallet state has already changed. Keep in-memory and no-op updates unchanged.
The first commit adds functional coverage for the current behavior. The second adds the fix, updates the assertions to expect success with warnings, and documents that failed settings writes keep the in-memory changes.
### Manual Reproduction
Run on the parent commit and the fixed commit, using a fresh temporary regtest data directory each time:
```sh
{ cmake -B build-wallet-review -DBUILD_GUI=ON && cmake --build build-wallet-review -j --target bitcoin-qt; } >/dev/null 2>&1
build-wallet-review/bin/bitcoin-qt -regtest -datadir="$(mktemp -d)" -nosettings -noconnect
```
Choose `File` > `Create Wallet...`, enter `repro`, leave the defaults unchanged, and click `Create`.
Before the fix, the application terminates with:
```text
libc++abi: terminating due to uncaught exception of type std::logic_error: Attempt to write settings file when dynamic settings are disabled.
```
After the fix, the wallet is created and the application displays:
```text
Wallet load on startup setting could not be updated, so wallet may not be loaded next node startup.
```
ACKs for top commit:
l0rinc:
tested ACK a34fc8b11a
kevkevinpal:
tACK a34fc8b11a
achow101:
ACK a34fc8b11a
jeanpablojp:
tACK a34fc8b11a
Tree-SHA512: 5e43028200478f89e71ebe7e0fc28c559f15e713226124899a69eb90d413d8ecaaaca02267d5a848068d70555b3e4334993f414de2debf6a22d73a51a71d1acd
e014e5bb61 miner: Enforce murch-zawy rule (BIP54) (Fabian Jahr)
Pull request description:
Opened separate from #35793 as [requested by darosior](https://github.com/bitcoin/bitcoin/pull/35793#discussion_r3704817804). This makes the miner enforce the murch-zawy rule for which #35793 adds the validation part.
A node whose clock is behind the first block of the difficulty period currently reports a mintime below the consensus floor in getblocktemplate and fails to build a valid template for the last block of the period so it can't mine until its clock catches up. This is mostly a theoretical concern on mainnet because it would require a huge system clock misconfiguration. It might be a bigger concern on test networks with volatile hashrates. But generally, I think our miner should be able to create valid templates in any situation.
ACKs for top commit:
kevkevinpal:
crACK [e014e5b](e014e5bb61)
darosior:
ACK e014e5bb61
sedited:
ACK e014e5bb61
Tree-SHA512: 299f83459e92654e028ce1c27470e1dcaae4a58de10687659860cdb425d3af330d3b8da6d8ca8f727c9eaebb2f0a3f3af5185f9daa57e92133435cb80a9e365c
Return a persistence failure when dynamic settings are disabled so
wallet operations finish with their existing startup-setting warning.
This avoids an uncaught exception in Qt and RPC errors after the wallet
state has already changed. Keep in-memory and no-op updates unchanged.
TxDownloadManagerImpl retains a reference to PeerManagerImpl::m_rng,
which is non-thread-safe and guarded by g_msgproc_mutex.
BlockConnected runs on the validation background thread while holding
only m_tx_download_mutex. Reconsidering an orphan with multiple
announcers could therefore use m_rng concurrently with message
processing.
Regression introduced in 9cc7dc50bd
5be248341a bugfix: compare real chunk weight against block weight limit (ismaelsadeeq)
fc98790869 test: `TestChunkBlockLimits` uses incorrect weight for comparison (ismaelsadeeq)
Pull request description:
Partially fixes#35596
When assembling a block template, `BlockAssembler::addChunks()` adds chunks of transactions until the block is close to being full. For each chunk, `TestChunkBlockLimits()` checks both the weight and the sigop-cost limits before the chunk is included.
The weight check compared the chunk's **sigops-adjusted** weight against `block_max_weight`:
```cpp
if (nBlockWeight + chunk_feerate.size >= m_options.block_max_weight) {
return false;
}
```
Whereas `nBlockWeight` accumulates the actual chunk weight.
A chunk whose sigop-adjusted weight exceeds the actual weight can be wrongly skipped even though the block sigop limit is enforced independently on the next line, and that could pass. Those chunks pay higher fees, so this could potentially cause miners to needlessly forfeit some fees revenue.
This PR fixes this by passing the chunk's real weight (sum of `GetTxWeight()`, accumulated in the same loop that already sums sigop cost) to `TestChunkBlockLimits()`. The separate sigop-cost check is unchanged.
- The first commit adds `TestSigOpsAdjustedWeightChunkLimit`: it builds one sigop-dense transaction sized to fit by real weight but not by adjusted weight, and asserts that the tx is skipped and only the coinbase is mined.
- The second commit applies the fix and flips the assertion to show the transaction is now included.
ACKs for top commit:
pablomartin4btc:
Code Review ACK 5be248341a
sedited:
ACK 5be248341a
Tree-SHA512: b3fe9bfaa6d83d713d0243c0fc0d0fb8e68e1060bf6d606e43d9a52bd1ec07e42c561a1ba3426f180903726826c4a664bb131ddc5160354a96e3454d538fbf6c
7f9c4e2928 doc: add release notes (ismaelsadeeq)
e18d392689 test: add mempool estimator i/o fuzz test (ismaelsadeeq)
970f02096d fees: persist mempool policy estimator data (ismaelsadeeq)
7dcb37989d fees: move fee_estimates.dat into fees directory (ismaelsadeeq)
0db2b69e6d rpc: add verbosity option to estimatesmartfee options (ismaelsadeeq)
06bb65730e fees: gate mempool estimates on recent block coverage (ismaelsadeeq)
cfe585df25 validation: emit block mempool removal signal from ConnectTip (ismaelsadeeq)
0d88558f95 fees: return mempool estimates when it's lower than block policy (ismaelsadeeq)
693b1351af fees: add caching to MemPoolFeeRateEstimator (ismaelsadeeq)
c9bb3df29f fees: add MemPoolFeeRateEstimator class (ismaelsadeeq)
9cacf677a9 rpc: add fee_rate_estimator option to estimatesmartfee (ismaelsadeeq)
ba6c61bbdd fees: add FeeRateEstimatorManager class (ismaelsadeeq)
2cb6b831e0 fees: add EstimateFeeRate and MaximumTarget to CBlockPolicyEstimator (ismaelsadeeq)
5adb2ab084 refactor: test block policy estimator directly (ismaelsadeeq)
9c8309a890 test: rename policy estimator tests to block policy estimator tests (ismaelsadeeq)
e3d5ef1b5f fees: move StringForBlockPolicyEstimateReason to block policy estimator (ismaelsadeeq)
74245c20e0 fees: split wallet and estimator fee reasons (ismaelsadeeq)
Pull request description:
This PR is another attempt to fix#27995 using a better approach.
For background and motivation, see #27995 and the discussion in the Delving Bitcoin post [Mempool Based Fee Estimation on Bitcoin Core](https://delvingbitcoin.org/t/mempool-based-fee-estimation-on-bitcoin-core/703).
This PR is currently limited to using the mempool only to lower what is recommended by the Block Policy Estimator.
Accurate and safe fee estimation using the mempool is challenging. There are open questions about how to prevent mempool games that are theoretically possible for miners [(a variant of the Finney attack)](https://delvingbitcoin.org/t/mempool-based-fee-estimation-on-bitcoin-core/703/6).
This is one reason this PR uses the mempool only to lower the Block Policy Estimator result. The Block Policy Estimator itself is not gameable in this way, so the combined estimate is not susceptible to this attack increasing the returned feerate.
The underlying assumption is that, with the current tools and work done to make RBF and CPFP feasible and reliable (TRUC transaction relay, ephemeral anchors, cluster size 2 package RBF), underestimation is safer than overestimation. We now assume it is relatively easy to fee-bump later if a transaction does not confirm, whereas once a fee is overestimated there is no way to recover from that.
Another open question when using the mempool for fee estimation is how to account for incoming transaction inflow.
[Bitcoin Augur](https://github.com/block/bitcoin-augur) does this by using past inflow plus a constant expected inflow to predict future inflow. I find this unconvincing for fee estimation and potentially prone to more overestimation, as past conditions are not always representative of the future. See my [review of the Augur fee rate estimator and open questions](https://github.com/block/bitcoin-augur/issues/3).
This PR uses a much simpler approach based on current user behavior, similar to the widely used method employed by mempool.space: looking at the top block of the mempool and selecting a percentile feerate depending on whether the user is economical or conservative.
Empirical data from both myself and Clara Shikhelman shows that the 75th percentile feerate for economical users and the 50th percentile feerate for conservative users provide positive confirmation guarantees, hence this is what is used in this PR.
Parallel research by Rene Pickhardt and his student suggests that using the [average fee per byte of the block template performs well](https://delvingbitcoin.org/t/mempool-based-fee-estimation-on-bitcoin-core/703/12).
All of these are constants that can be adjusted. There is parallel work exploring these constants and running benchmarks across fee estimators to find a sweet spot.
See also work in LND, the [LND Budget Sweeper](https://delvingbitcoin.org/t/lnds-deadline-aware-budget-sweeper/1512), which applies this idea successfully. Their approach is to estimate fees initially with bitcoind, then increment gradually as the confirmation deadline approaches, using a fixed fee budget.
Historical data indicates that this PR's approach can [reduce overestimation quite significantly (~29%)](https://delvingbitcoin.org/t/mempool-based-fee-estimation-on-bitcoin-core/703/8).
This is particularly useful in scenarios where the Block Policy Estimator recommends a high feerate while the mempool is empty.
<img width="1800" height="1090" alt="56f3ba26c0184521c42bb82ec9d8c9f2224d4f8e" src="https://github.com/user-attachments/assets/c035c40c-8ece-42a7-b290-d29f1ac9bf4d" />
As seen in the image above, there is only one remaining unfixed case: when there is a sudden inflow of transactions and the feerate rises, the Block Policy Estimator takes time to reflect this. In that case, users will continue to see a low feerate estimate until it slowly updates. From the historical data linked above, [this occurs about ~26% of the time](https://delvingbitcoin.org/t/mempool-based-fee-estimation-on-bitcoin-core/703/8).
Overall, we observe a **73% success rate with 0% overestimation, and 26% underestimation** with this approach.
See https://bitcoincorefeerate.com/stats for recent running stats that have almost identical data.
This PR also includes refactors that enable this work. Rather than splitting the PR and implementing changes incrementally, I opted for an end-to-end implementation:
### 1. Refactors
* Split the mixed fee reason enum into separate wallet and block policy concepts. The wallet now has a `FeeReason` enum for why the wallet selected a fee rate (`FEE_RATE_ESTIMATOR`, `MEMPOOL_MIN`, `USER_SPECIFIED`, `FALLBACK`, `REQUIRED`), while the Block Policy Estimator uses `BlockPolicyEstimateReason` for its internal threshold details.
* Move `StringForBlockPolicyEstimateReason` to the Block Policy Estimator code, keeping the estimator-specific strings with the estimator.
* Move detailed Block Policy Estimator logging out of wallet transaction creation and into the estimator path. Wallet transaction creation now logs the selected fee and wallet fee reason instead of leaking estimator internals.
* Keep the wallet RPC `fee_reason` field name for compatibility, but update its meaning to report the wallet fee reason instead of the Block Policy Estimator's internal threshold reason.
* Rename policy estimator tests and files to block-policy-specific names where appropriate.
* Update Block Policy Estimator unit tests to be independent of the mempool and validation interface.
### 2. Introduce Mempool-Based Fee Estimator and Fee Estimator Manager
* Introduce `FeeRateEstimation` and `FeeRateEstimationError` as common estimator result types, avoiding new out-parameters for fee estimation results.
* Add `FeeRateEstimatorType` to identify the estimator that produced a result.
* Add `FeeRateEstimatorManager`, responsible for owning the Block Policy Estimator and Mempool Fee Rate Estimator.
* Update the node context to store a `std::unique_ptr` to `FeeRateEstimatorManager` instead of `CBlockPolicyEstimator`.
* Update `CBlockPolicyEstimator` to no longer subscribe directly to the validation interface; instead, `FeeRateEstimatorManager` subscribes and forwards relevant notifications.
* Add a mempool fee estimator that generates a block template when called, calculates a percentile feerate, and returns the 75th percentile for economical mode or the 50th percentile for conservative mode.
* When the selected estimate is below the node's fee floor, `estimatesmartfee` still returns at least the max of `mempoolminfee` and `minrelaytxfee`.
* Add caching to the mempool estimator so new estimates are generated at most every 7 seconds while the chain tip is unchanged, assuming enough [transactions have propagated](https://bitcoin.stackexchange.com/questions/125776/how-long-does-it-take-for-a-transaction-to-propagate-through-the-network/125777#125777) to make a meaningful difference.
This heuristic will likely be replaced by requesting block templates via the general-purpose block template cache proposed here: https://github.com/bitcoin/bitcoin/issues/33389
* Update `MempoolTransactionsRemovedForBlock` to receive the connected block as well as the transactions removed from the mempool.
* Track the weight of block transactions and mempool transactions removed due to block connection after each block connection.
This data is tracked for the last 6 mined blocks. A mempool feerate estimate is returned only when the ratio of mempool transaction weight removed due to block connection to block transaction weight is greater than 75% across the tracked window. This heuristic provides rough confidence that the node's mempool matches that of the majority of the hashrate. The 75% threshold is arbitrary and can be adjusted.
There is a caveat when transactions in the local mempool are consistently not mined by the network, as described in #27995 (e.g. due to filtering).
Accounting for these transactions during fee estimation is not necessary, as they should be evicted from the mempool itself (see #33510). Handling this again within fee estimation would be redundant.
* Persist statistics for the 6 most recent mined blocks to `fees/mempool_policy_estimator.dat` during periodic flushes and shutdown, so this data is available after restarts.
* Move Block Policy Estimator data from `fee_estimates.dat` to `fees/block_policy_estimates.dat`, migrating the legacy file during startup when needed.
* Add `fee_rate_estimator` to the `estimatesmartfee` options object. Supported values are `"none"` (default combined behavior), `"block_policy"` (use only the Block Policy Estimator), and `"mempool_policy"` (use only the Mempool Fee Rate Estimator). Unknown values are treated as `"none"`.
* Add `verbosity` to the `estimatesmartfee` options object. With `verbosity >= 2`, the RPC returns recent mempool health statistics.
* Expose the selected fee rate estimator in `estimatesmartfee` results when `fee_rate_estimator` is `"none"` and the estimate succeeds.
* Add unit, functional, and fuzz test coverage for the new estimator behavior, persistence, RPC options, and estimator I/O.
<details>
<summary>see example output</summary>
```bash
bitcoin-cli estimatesmartfee 1 economical '{"verbosity": 2, "fee_rate_estimator": "none"}'
```
```json
{
"feerate": 0.00002133,
"estimator": "mempool_policy",
"blocks": 2,
"mempool_health_statistics": [
{
"block_height": 927953,
"block_weight": 3991729,
"mempool_txs_weight": 3942409
}
]
}
```
</details>
ACKs for top commit:
willcl-ark:
reACK 7f9c4e2928
jsarenik:
Approach ACK 7f9c4e2
Tree-SHA512: c35b423eea0eb34524cf5ad07822c0ab8d53e2ab78965b58c8738044c61c77352184822360ed077a51bfbf83d0226d221e988f7156b1948023707c7e1fb31495
Introduce FeeRateEstimatorManager to wrap CBlockPolicyEstimator and
act as the single point of contact for fee rate estimation in the node.
It inherits CValidationInterface so it can register directly with the
validation signals and receive mempool/block events.
Wire it into NodeContext (fee_estimator_man), init, shutdown, the RPC
server utility helpers (EnsureAnyFeeEstimatorMan), and the wallet-facing
interfaces::Chain API.
The Chain method estimateSmartFee is renamed to getFeeRateEstimate and
now returns util::Expected<FeeRateEstimation, FeeRateEstimationError>
instead of CFeeRate, so callers get the full estimation context without
needing FeeCalculation. estimateMaxBlocks is renamed to
maximumFeeEstimationTargetBlocks (still returns the max target).
CBlockPolicyEstimator no longer inherits CValidationInterface; the
manager now receives the mempool/block validation events and forwards
them to the CBlockPolicyEstimator.
Co-authored-by: willcl-ark <will@256k1.dev>
Introduce that common interface:
- FeeRateEstimatorType identifies the source estimator in a result.
- FeeRateEstimation carries the feerate and returned target of a
successful estimate; FeeRateEstimationError carries the error
message alongside a zero-value estimation.
- EstimateFeeRate wraps estimateSmartFee and returns
util::Expected<FeeRateEstimation, FeeRateEstimationError>.
- MaximumTarget delegates to HighestTargetTracked(LONG_HALFLIFE) so
callers do not need to know about block policy horizons.
Update call sites in rpc/fees.cpp and node/interfaces.cpp.
A later commit introduces FeeRateEstimatorManager, which selects
between multiple fee rate estimators. To compare estimates and
report which estimator produced them, the manager needs each fee
rate estimator to expose a uniform output, whereas
estimateSmartFee's CFeeRate/FeeCalculation output is specific to
the block policy fee rate estimator.
Co-authored-by: willcl-ark <will@256k1.dev>
25bed560be test: add forward-compat functional test for txindex (sedited)
703304ed8c doc: add release notes for txindex disk usage and downgrading (Andrew Toth)
8e5320a2d2 tests: cover txindex hash prefix collisions and legacy fallback (Andrew Toth)
b75efa19ba txindex: skip bloom filters and legacy lookups for new databases (Andrew Toth)
004d7c098c txindex: hash key prefixes and pack block positions (Andrew Toth)
5a255970fd refactor: move txindex db constants and legacy key to txindex_key.h (Andrew Toth)
327660134c txindex: pass the full block to DB::WriteTxs (Andrew Toth)
42771e7998 txindex: use a new block locator for downgrade safety (Andrew Toth)
4b08baed72 txindex: return optional tx and block hash from FindTx (Andrew Toth)
Pull request description:
The current txindex uses the full 32-byte txid as keys, which takes up about 66 GB of disk space today on mainnet. Using a 5-byte key prefix instead drops the disk usage to 26 GB - cutting the size to less than half.
Using the full 32-bytes is unnecessary since a 5-byte salted siphash will produce collisions in about 1 in 1.1 trillion. Some collisions will occur, but the penalty is just an extra disk read, deserialization and hash.
The tx position can be appended to the key instead of used as a value, and a LevelDB iterator can seek to the prefix and then scan for the correct tx. This is an almost identical approach to `txospenderindex`.
Also instead of storing the file position of the block, we can store only the sequence of the connected block and offset of the transaction in the block. This can be packed into a 6-byte key suffix using 3-byte representations of the sequence and offset in the block. The block file can be recovered by the CBlockIndex that is already in memory. The sequence is mapped to the block hash in the db, so we can lookup the block hash to find the CBlockIndex during reads.
If a tx is not found with this method, we fallback to looking up the legacy entry. With this method a user with an existing db can opt to erase the `indexes/txindex` folder and reindex, or keep the current index and new entries will be appended with the smaller footprint.
The time to index was faster on my machine with this method, 1h19m vs current 1h50m.
Lookups are roughly the same, around 0.2ms per lookup with `getrawtransaction`.
When testing on mainnet, I got 894,549 2-way collisions, 395 3-way collision, and 1 4-way collision that worst case could cause an extra 3 false positives when reading.
ACKs for top commit:
l0rinc:
diff reACK 25bed560be
sedited:
ACK 25bed560be
ajtowns:
ACK 25bed560be
Tree-SHA512: a25c79ca7e722e2f372b65f5fc11c8b194ad49f2240b4881c7e606306aabbd3604aede3f1c33606b467486affac3a3f503638f513c896935cebbc02709cb60d8
Both are identical since C++17 and this refactor shouldn't change any
behavior. The benefits are consistency and to be explicit, to avoid
confusion with the C++11/14 constexpr.
Co-Authored-By: l0rinc <pap.lorinc@gmail.com>
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's/^constexpr \S+ \w+(\[\])? ?[={]/inline &/' $( \
git grep -l '^constexpr ' -- \
'*.h' \
':(exclude)src/minisketch' \
)
-END VERIFY SCRIPT-
Both are fine and this refactor shouldn't change any behavior.
However, inline const will ensure each symbol has a single address
across all TU, making the release binary smaller.
-BEGIN VERIFY SCRIPT-
# Replace `static const`
sed -i "s/^static const /inline const /" $( \
git grep -l "^static const " -- \
'*.h' \
':(exclude)src/leveldb' \
':(exclude)src/secp256k1' \
)
# Replace plain `const`
sed -i --regexp-extended 's/^const (\S+ \w+(\[\])? ?[={])/inline &/' $( \
git grep -l '^const ' -- \
'*.h' \
':(exclude)src/leveldb' \
':(exclude)src/secp256k1' \
)
-END VERIFY SCRIPT-
Both are fine and this refactor shouldn't change any behavior.
However, inline constexpr will ensure each symbol has a single address
across all TU, making the release binary smaller.
Note, a follow-up commit will deal with string literals (const char*)
and other static const, which can not be constexpr (e.g. std::vector).
-BEGIN VERIFY SCRIPT-
# Limit to types that can be constexpr
type='bool|CAmount|size_t|((signed|unsigned) )?int|u?int[0-9]+_t|std::array|DatabaseFormat|CFeeRate|std::streamsize'
sed -i --regexp-extended "s/^(static )?const (${type})\>/inline constexpr \2/" $( \
git grep -l --extended-regexp "^(static )?const " -- \
'*.h' \
':(exclude)src/leveldb' \
':(exclude)src/secp256k1' \
)
-END VERIFY SCRIPT-
Both are fine and this refactor shouldn't change any behavior.
However, inline constexpr will ensure each symbol has a single address
across all TU, making the release binary smaller.
Review note: In theory the script may also cover functions, but they
were handled in the prior commit, to remove the redundant inline for
them.
-BEGIN VERIFY SCRIPT-
sed --regexp-extended -i 's/^(static constexpr|constexpr static)\>/inline constexpr/g' $( \
git grep --extended-regexp -l '^(static constexpr|constexpr static)' -- \
'*.h' \
':(exclude)src/crc32c' \
':(exclude)src/ipc/libmultiprocess' \
':(exclude)src/minisketch' \
)
-END VERIFY SCRIPT-
TestChunkBlockLimits() compared the chunk's sigops-adjusted weight against
block_max_weight, while nBlockWeight tracks real transaction weight. This
over-counted sigop-dense chunks and could skip ones that actually fit,
losing fees; the block sigop limit is enforced separately on the next line.
Pass the chunk's real weight (sum of GetTxWeight()) instead, and update the
test to show the chunk is now included.
349c72ee00 net_processing: Drop unnecessary txid arg from InitiateTxBroadcastToAll (Anthony Towns)
12b0dc33c4 doc: Add release note for -txsendrate etc (Anthony Towns)
5cde66341a tests: basic functional test for tx rate limiting (Anthony Towns)
4842903ac1 rpc: report -txsendrate and bucket info via getnetworkinfo (Anthony Towns)
74a47a5207 init: add -txsendrate configuration parameter (Anthony Towns)
6307bd034b net_processing: Provide a 30bpm heartbeat log while inv backlog is in use (Anthony Towns)
df31ee57aa net_processing: add a global delay queue for sending txs (Anthony Towns)
7927650e56 util/tokenbucket.h: Provide a generic TokenBucket class (Anthony Towns)
749bb447f8 txmempool: Drop CompareMiningScoreWithTopology (Anthony Towns)
e1b7490fbc net_processing: Replace CompareInvMempoolOrder (Anthony Towns)
6cfc65d210 txmempool: Add ExtractBestByMiningScoreWithTopology (Anthony Towns)
026f70e05f net_processing: Remove per-peer rate-limiting (Anthony Towns)
46c8c471dc net_processing: bump last_inv_sequence for bip35 messages explicitly (Anthony Towns)
Pull request description:
Per-peer `m_tx_inventory_to_send` queues have CPU and memory costs that scale with both queue size and peer count. Under high transaction volume, this has previously caused severe issues ([May 2023 disclosure][1]) and still can cause measurable delays ([Feb 2026 Runestone surge][2], with the msghand thread observed hitting 100% CPU and queue memory reaching ~95MB).
This PR replaces the per-peer rate limiting with a global queue using dual token buckets (limiting transaction by both count and serialized size). Transactions that arrive within the bucket capacity still relay nearly immediately, but excess transactions queue in a global backlog and drain as the token buckets refill.
Key parameters:
- Count bucket: 14 tx/s, 420 capacity (30s buffer)
- Size bucket: 20 kB/s (~12 MB/600s), 50 MB capacity
- Outbound peers refill faster by a factor of 2.5
Per-peer queues are retained solely for privacy batching and are always fully emptied, removing the old `INVENTORY_BROADCAST_MAX` cap.
This reduces the memory and CPU burden during transaction spikes when the queuing logic is engaged from O(queue * peers) to O(queue), as the queued transactions no longer need to be retained per-peer or re-sorted per-peer.
Design discussion: https://gist.github.com/ajtowns/d61bea974a07190fa6c6c8eaef3638b9
[1]: https://bitcoincore.org/en/2024/10/08/disclose-large-inv-to-send/
[2]: https://bnoc.xyz/t/increased-b-msghand-thread-utilization-due-to-runestone-transactions-on-2026-02-17/81
ACKs for top commit:
sipa:
Code review ACK 349c72ee00. I haven't tested it myself yet (though switched my well-connected node to it now), but the posted benchmarks and analyses look convincing.
instagibbs:
reACK 349c72ee00
mzumsande:
ACK 349c72ee00
Tree-SHA512: 2196a23308cb7fe36738cf638edf5c5b0e9ba32b11c083609fd8b50291e05bb33484f9921f8beab28d94c58d1adddea4c8ae1182a60a7f53f54be7370e2a0e47
If a `static class` member is not inlined or `constexpr`, the linker
will fail when attempting to ODR-use the constant (passing as `const
T&`). These can be fixed by finding all member variables that are
`const` qualified and inlining them with `constexpr`. There is a
clang-tidy pull request that would lint these callsites: https://github.com/llvm/llvm-project/pull/162741
A script was used to modify these sites, however it cannot run as a
scripted-diff because it uses clang-query and a build folder.
The script only queries for integer and enumeration types, as other data
members would have to be marked `constexpr` or `inline` from what I
understand: https://en.cppreference.com/cpp/language/static#Constant_static_members
Removing the ZMQ forward declaration was a clang-tidy lint.
The script used to find these sites, LLM assisted:
```
set -uxo pipefail
cd "$(git rev-parse --show-toplevel)"
BUILD=${BUILD:-build}
if [ ! -f "${BUILD}/compile_commands.json" ]; then
echo "error: ${BUILD}/compile_commands.json not found. Run cmake -B ${BUILD} first." >&2
exit 1
fi
if ! command -v clang-query >/dev/null; then
echo "error: clang-query not on PATH. Install clang-tools." >&2
exit 1
fi
if ! git diff --quiet || ! git diff --cached --quiet; then
echo "error: working tree has uncommitted changes. Commit or stash first." >&2
exit 1
fi
MATCHER='match varDecl(hasParent(cxxRecordDecl()),
hasType(qualType(isConstQualified(),
anyOf(hasCanonicalType(isInteger()),
hasDeclaration(enumDecl())))),
hasInitializer(expr()),
unless(isConstexpr()),
isExpansionInFileMatching("/src/"))'
RAW=$(mktemp)
trap 'rm -f "$RAW"' EXIT
echo "Sweeping TUs (batched, may take a few minutes)..." >&2
find src -type d \( -name secp256k1 -o -name leveldb -o -name crc32c \
-o -name minisketch -o -name libmultiprocess -o -name ctaes \) -prune -o \
-name '*.cpp' -print0 \
| xargs -0 -n 50 clang-query -p "${BUILD}" \
-c 'set output diag' \
-c "${MATCHER}" \
>>"$RAW" || true
ROOT=$(pwd)
LOCS=$(grep -oE "${ROOT}/src/[^:]+:[0-9]+:[0-9]+:" "$RAW" \
| sed -E "s|^${ROOT}/||; s|:[0-9]+:$||" \
| sort -u)
if [ -z "$LOCS" ]; then
echo "no matches" >&2
exit 0
fi
FILTERED=""
while IFS=: read -r file line; do
case "$file" in
src/secp256k1/*|src/leveldb/*|src/crc32c/*|src/minisketch/*|src/ipc/libmultiprocess/*|src/crypto/ctaes/*) continue ;;
src/tinyformat.h) continue ;;
esac
src=$(sed -n "${line}p" "$file")
case "$src" in *inline*) continue ;; esac
FILTERED+="${file}:${line}"$'\n'
done <<<"$LOCS"
FILTERED=$(printf '%s' "$FILTERED" | sed '/^$/d')
if [ -z "$FILTERED" ]; then
echo "no matches after filtering" >&2
exit 0
fi
echo "Sites to rewrite ($(echo "$FILTERED" | wc -l)):" >&2
echo "$FILTERED" >&2
declare -A LINES
while IFS=: read -r file line; do
LINES[$file]+="${line} "
done <<<"$FILTERED"
for file in "${!LINES[@]}"; do
args=()
for line in ${LINES[$file]}; do
args+=(-e "${line}s/static const /static constexpr /")
done
sed -i "${args[@]}" "$file"
done
echo >&2
echo "===== proposed diff =====" >&2
git --no-pager diff
```
75929b11ed doc: add release note for submitSolution IPC changes (woltx)
ed75d70fdb refactor: centralize SubmitBlock result handling (w0xlt)
cbaa1696f3 mining: add reason and debug output to submitSolution (w0xlt)
83f3bc002d mining: clarify SubmitBlock result handling (w0xlt)
Pull request description:
`BlockTemplate.submitSolution` currently returns only a boolean, so IPC mining clients cannot determine why a submission failed without inspecting Bitcoin Core's debug log.
Returning `reason` and `debug`, as `Mining.submitBlock` already does, lets callers distinguish a concrete block rejection from a duplicate or inconclusive result. Here, `inconclusive` means the method returns failure, but validation did not determine that the submitted block is invalid.
This follow-up was suggested during the review of #34644:
https://github.com/bitcoin/bitcoin/pull/34644#discussion_r2853758006
This PR:
- Extracts a shared `SubmitBlock` helper that wraps `ProcessNewBlock` with `SubmitBlockStateCatcher` to capture `BlockValidationState`
- Adds `reason` and `debug` output parameters to `submitSolution`, matching `submitBlock`
- Makes both methods delegate to the same helper, eliminating duplicated logic
ACKs for top commit:
optout21:
ACK 75929b11ed
achow101:
light ACK 75929b11ed
Sjors:
ACK 75929b11ed
enirox001:
ACK 75929b11ed
sedited:
ACK 75929b11ed
Tree-SHA512: 31b1c305c20aaebdfa2d887665d9927830d0f97ba3c3469e2792148ad799d5a400a000cc0ca0b9add071d314e27c9da44d55228c442533a32a7c031678b78a55
Adds a debug-only configuration option to set the target
transaction/second rate for relay to inbound connections. This is mostly
intended to be set to artificially low values to aid in testing behaviour
when a backlog occurs, but is also available in case the default 14tx/s
target is somehow too low in practice.
The oversized `-dbcache` warning currently switches from a fixed `450 MiB` threshold below `2 GiB` of RAM to `75%` of total RAM at `2 GiB`.
This creates a cliff where a small increase in RAM can raise the warning threshold to about `1536 MiB`.
Apply the `75%` factor only to RAM above a `2 GiB` reserve while keeping `DEFAULT_DB_CACHE` as the minimum threshold.
This removes the cliff: the threshold stays at the default until the percentage term exceeds it, then grows by `0.75 MiB` per additional MiB of RAM.
This also aligns better with the recently merged parallel input prevout fetcher which performs better with slightly lower dbcache memory.
Co-authored-by: Bortlesboat <Bortlesboat@users.noreply.github.com>
fabafd91f1 refactor: Use u64 over size_t for all cache sizes to fix a 32-bit overflow (MarcoFalke)
Pull request description:
This is a refactor on 64-bit systems, because size_t is equal to u64.
However, on 32-bit systems, it fixes an integer overflow while calculating the cache sizes:
```
src/node/caches.cpp:71:49: runtime error: unsigned integer overflow: 471859200 * 10 cannot be represented in type size_t (aka "unsigned int")
```
This happens while multiplying the default cache size (450MiB) by 10:
```
index_sizes.tx_index = std::min(total_cache * 10 / 100, ...)
^^^^^^^^^^^^^^^^
```
The issue was introduced in commit d06dabf26b.
----
This change follows similar changed one in the past, like 3789215f73, ac76d94117, or 28a523fb94.
Generally, using fixed sized integer types for calculations is beneficial, because all platforms behave exactly the same way. With platform-dependent types there is a risk that the same calculation yields different results. This has several resulting benefits:
* Easier review, because there is no need to review the same code several times for each supported platform.
* Easier quality assurance, because there is less need to run the same code several times in sanitizers for each supported platform, which is [tedious](https://github.com/bitcoin/bitcoin/issues/32375#issuecomment-4825318068).
There are also no downsides, because there is no measurable overhead on 32-bit for u64 calculations that are done only once in the lifetime of the program. Also, there is no measurable memory overhead when a few fields on 32-bit store some extra zero bytes.
----
As said, testing is only possible by picking one of the tedious options:
* Apply a diff on 64-bit arch and compile with `-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DSANITIZERS=integer`
```diff
diff --git a/src/node/caches.cpp b/src/node/caches.cpp
index c98b8ce604..cfd49b60d3 100644
--- a/src/node/caches.cpp
+++ b/src/node/caches.cpp
@@ -58,3 +58,3 @@ CacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
{
- size_t total_cache{CalculateDbCacheBytes(args)};
+ uint32_t total_cache(CalculateDbCacheBytes(args));
@@ -72,6 +72,6 @@ CacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
IndexCacheSizes index_sizes;
- index_sizes.tx_index = std::min(total_cache * 10 / 100, args.GetBoolArg("-txindex", DEFAULT_TXINDEX) ? MAX_TX_INDEX_CACHE : 0);
- index_sizes.txospender_index = std::min(total_cache * 5 / 100, args.GetBoolArg("-txospenderindex", DEFAULT_TXOSPENDERINDEX) ? MAX_TXOSPENDER_INDEX_CACHE : 0);
+ index_sizes.tx_index = std::min<uint32_t>(total_cache * 10 / 100, args.GetBoolArg("-txindex", DEFAULT_TXINDEX) ? MAX_TX_INDEX_CACHE : 0);
+ index_sizes.txospender_index = std::min<uint32_t>(total_cache * 5 / 100, args.GetBoolArg("-txospenderindex", DEFAULT_TXOSPENDERINDEX) ? MAX_TXOSPENDER_INDEX_CACHE : 0);
if (n_indexes > 0) {
- size_t max_cache = std::min(total_cache * 5 / 100, MAX_FILTER_INDEX_CACHE);
+ size_t max_cache = std::min<uint32_t>(total_cache * 5 / 100, MAX_FILTER_INDEX_CACHE);
index_sizes.filter_index = max_cache / n_indexes;
```
This will give a roughly similar error:
```
sh-5.3$ echo 'Bw==' | base64 -d > /tmp/blob
sh-5.3$ UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=block_index_tree ./bld-cmake/bin/fuzz /tmp/blob
./src/node/caches.cpp:73:59: runtime error: unsigned integer overflow: 1073741824 * 10 cannot be represented in type 'uint32_t' (aka 'unsigned int')
#0 0x55ca78da5c47 in node::CalculateCacheSizes(ArgsManager const&, unsigned long) ./src/node/caches.cpp:73:59
```
* Alternatively, to reproduce in a fresh `podman run -it --rm --platform linux/i386 debian:unstable`:
```
export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git ./b-c && cd b-c && apt install build-essential cmake pkg-config python3-zmq libzmq3-dev libevent-dev libboost-dev libsqlite3-dev systemtap-sdt-dev libcapnp-dev capnproto libqrencode-dev qt6-tools-dev qt6-l10n-tools qt6-base-dev clang llvm libc++-dev libc++abi-dev mold -y && cmake -B ./bld-cmake -DAPPEND_CXXFLAGS='-O3 -g2' -DAPPEND_CFLAGS='-O3 -g2' -DCMAKE_BUILD_TYPE=Debug -DCMAKE_EXE_LINKER_FLAGS=-fuse-ld=mold -DCMAKE_C_COMPILER='clang;-ftrivial-auto-var-init=pattern' -DCMAKE_CXX_COMPILER='clang++;-ftrivial-auto-var-init=pattern' -DSANITIZERS=address,float-divide-by-zero,integer,undefined --preset=dev-mode && cmake --build ./bld-cmake --parallel $(nproc)
echo 'Bw==' | base64 -d > /tmp/blob
UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=block_index_tree ./bld-cmake/bin/fuzz /tmp/blob
# or:
UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ASAN_OPTIONS="detect_leaks=0" ./bld-cmake/bin/test_bitcoin-qt
```
ACKs for top commit:
l0rinc:
ACK fabafd91f1
sedited:
Re-ACK fabafd91f1
theStack:
re-ACK fabafd91f1
Tree-SHA512: cebbc29636b4074917c96cf3af8fcc176dcd328821d5032dc8609475e18778dfda4e287ded889c024ab29b79d81004bc9a8e57a88eaa0a5eaefe5f8bba1462ab
dc1c17c085 doc: add release notes (Andrew Toth)
0e10937184 fuzz: add coins_view_stacked fuzz harness to test concurrent leveldb reads (Andrew Toth)
ce610a6ff4 fuzz: update harnesses to cover CoinsViewOverlay::StartFetching (Andrew Toth)
760fb22dc3 test: add unit tests for CoinsViewOverlay::StartFetching (Andrew Toth)
d69a3b20de doc: update CoinsViewOverlay docstring to describe parallel fetching (Andrew Toth)
ab2a379237 coins: fetch inputs in parallel (Andrew Toth)
fdf283036a coins: add ready flag to InputToFetch (Andrew Toth)
ede11b8314 validation: collect block inputs in CoinsViewOverlay before ConnectBlock (Andrew Toth)
f82043af50 coins: introduce thread pool in CoinsViewOverlay (Andrew Toth)
5bf1c32008 validation: add -prevoutfetchthreads configuration option (Andrew Toth)
Pull request description:
This PR is a continuation of https://github.com/bitcoin/bitcoin/pull/31132. All outstanding issues raised there have been resolved, but the volume of stale comments can make that change difficult to review.
Currently, when connecting a block, each input prevout is looked up one at a time. For every input we first check the in-memory coins cache, and on a miss we make a synchronous round-trip to the chainstate LevelDB to read the coin from disk. Because these lookups happen serially as the block is being validated, the disk read latency stacks up and dominates the time spent in `ConnectBlock` whenever many inputs are not already in the cache.
This PR moves those disk reads onto a pool of worker threads that run in parallel with block connection. Before entering `ConnectBlock` the block is handed to a `CoinsViewOverlay`, which kicks off the workers to begin fetching all of the block's prevouts from disk and warming the cache. The main validation thread continues to do exactly the same work it does today, hitting the cache for each input in order. The only difference is that by the time it asks, the coin is much more likely to already be there. There are no validation logic or consensus behavior changes. This is purely a parallelization of an existing read pattern.
The number of fetcher threads is configurable via `-prevoutfetchthreads=<n>`, defaulting to 8 and capped at 16. Setting it to 0 disables input fetching entirely and reverts to the previous serial behavior.
We have measured large performance gains for IBD and `-reindex-chainstate`, as well as worst-case steady-state block connection at the tip. l0rinc ran many thorough benchmarking passes on the original PR across multiple machines, storage types, dbcache sizes[^1], operating systems[^2], and fetcher thread counts[^3]. Many other contributors also posted their benchmark results in the original PR. IBD speedups range from 1.18× to over 3× faster[^4]. Worst-case block connection time for network-attached storage was over 2× faster[^5]. Flamegraph comparisons before and after this change are available[^6].
On safety: `ConnectBlock` runs while holding `cs_main`, so nothing else in the node can mutate the chainstate while the fetchers are reading it.
On LevelDB: [concurrent reads are fully supported](https://github.com/bitcoin/bitcoin/blob/master/src/leveldb/include/leveldb/db.h#L44) and [documented as such](https://github.com/bitcoin/bitcoin/blob/master/src/leveldb/doc/index.md#concurrency). We already rely on this in production today against our other LevelDB-backed databases. The `txindex` DB is read by multiple simultaneous HTTP RPC worker threads via the `getrawtransaction` RPC. The `blockfilterindex` DB is called concurrently from both the P2P `cfilters` / `cfheaders` / `cfcheckpt` message handlers on the `msghand` thread, and from the `getblockfilter` RPC on the HTTP RPC worker threads. We have not yet been issuing concurrent reads against the chainstate DB, but there is no LevelDB-side reason we can't. In fact, the chainstate DB is already being touched by more than one thread on master, because LevelDB schedules its own background compaction work.
For reviewers:
The main change is `CoinsViewOverlay` gets 1 new public and 2 new private methods.
- `StartFetching`: public method called in lieu of `CreateResetGuard` before we enter `ConnectBlock`. It still returns a `ResetGuard` so the view is `Reset` before the block it is working on leaves scope. This kicks off worker threads who each just run `while (ProcessInput()) {}` and then return.
- `StopFetching`: private method called on `Reset` whenever the guard leaves scope or `Flush`. Stops all threads and clears multi threaded state.
- `ProcessInput`: private method that fetches a single input prevout. Returns `true` if an input was fetched and `false` otherwise. This is the only method on `CoinsViewOverlay` that is called concurrently by multiple threads. Every other method on the overlay is still called synchronously on the main thread.
The `CoinsViewOverlay::FetchCoinFromBase` method is also extended to lookup the coins fetched from `ProcessInput` first before falling back to `base->PeekCoin`.
Mutating methods `Reset` and `Flush` are overridden in `CoinsViewOverlay` to call `StopFetching` first.
[^1]: https://github.com/bitcoin/bitcoin/pull/31132#pullrequestreview-3515011880
[^2]: https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3767758819
[^3]: https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3617721711
[^4]: https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3678847806
[^5]: https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-4071032270
[^6]: https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3617315125
ACKs for top commit:
l0rinc:
reACK dc1c17c085
willcl-ark:
ACK dc1c17c085
theStack:
re-ACK dc1c17c085
ryanofsky:
Code review ACK dc1c17c085 with changes to StopFetching and AllInputsConsumed checking behavior since last review.
Tree-SHA512: 89c1c2890f65aac5cd546edc44504956c47b6fada256d3b86ced47e6dd8c72f633a4357753b3b9805b9ba6ed02790822090d70578aba2964baf50d7eb956864c
This is a refactor on 64-bit systems, because size_t is equal to u64.
However, on 32-bit systems, it fixes an integer overflow while calculating the cache sizes:
src/node/caches.cpp:71:49: runtime error: unsigned integer overflow: 471859200 * 10 cannot be represented in type size_t (aka "unsigned int")
This happens while multiplying the default cache size (450MiB) by 10:
index_sizes.tx_index = std::min(total_cache * 10 / 100, ...)
^^^^^^^^^^^^^^^^
The issue was introduced in commit d06dabf26b.
====
Also, add missing includes in touched files, according to IWYU.
fa615bd163 refactor: Move LoadGenesisBlock to ChainstateManager (MarcoFalke)
Pull request description:
The function does not need anything from any chainstate, so it should not sit in the Chainstate class.
ACKs for top commit:
l0rinc:
Tested ACK fa615bd163
janb84:
reACK fa615bd163
sedited:
ACK fa615bd163
Tree-SHA512: 482b5c140faa35944a890c941fd185a896a3c04fec46d0cc6dd56830f62ee8fe5200fd13d25f7a80a5da0eb4fb42717f42c8bce837d933668432f5786f8380e3
9784818442 mining: add getTransactionsByWitnessID() IPC method (Sjors Provoost)
d282ae6883 mining: add getTransactionsByTxID() IPC method (Sjors Provoost)
0d5e4d4712 test: restart node after IPC option override test (Sjors Provoost)
f16b3613cd ipc: Serialize null CTransactionRef as empty Data (Sjors Provoost)
0f466e1094 mempool: add lookup by witness hash (Sjors Provoost)
Pull request description:
For Stratum v2 custom job declaration to be bandwidth efficient, the pool can request[^0] only the transactions that it doesn't know about.
The spec doesn't specify how this is achieved, but one method is to call the `getrawtransaction` RPC on each transaction id listed in [DeclareMiningJob](https://stratumprotocol.org/specification/06-Job-Declaration-Protocol?query=DeclareMiningJob#644-declareminingjob-client-server) (or a subset if the pool software maintains a cache). Using RPC is inefficient, made worse by the need to make multiple calls. It also doesn't support queuing by witness id (yet, see #34013).
This PR introduces two new IPC methods:
- `getTransactionsById()`: takes a list of `Txid`'s
- `getTransactionsByWitnessID()`: : takes a list of `Wtxid`'s
Both return a list of serialised transactions. An empty element is returned for transactions that were not found.
Unlike the RPC counterpart, the IPC methods do not take advantage of `-txindex`. This could be done in a followup. For `Wtxid` that would involve adding a `-witnesstxindex`.
I thought about having a single (or overloaded) `getTransactions()` that works with both `Txid` and `Wtxid`, but I prefer that clients are intentional about which one they want.
A unit and functional test cover the new functionality.
Sv2 probably only needs `getTransactionsByWitnessID()`, but it's easy enough to just add both.
To rest with Rust use:
- https://github.com/2140-dev/bitcoin-capnp-types/pull/11
[^0]: there's two reasons the pool requests these transactions: to approve the template and to broadcast the block if a solution is found (the miner will also broadcast via their template provider). See also https://github.com/stratum-mining/sv2-spec/issues/170
ACKs for top commit:
achow101:
ACK 9784818442
sedited:
Re-ACK 9784818442
ViniciusCestarii:
Re-ACK 9784818442
ismaelsadeeq:
Code review ACK 9784818442
Tree-SHA512: 3c6ceb572ab7d8bd090a8f31b5e331304a7a19a3d1f1551c9c2e1ee41339d76f96ca6c41bd634c87fca0a969e7d9bfa6a16c26fb06c0dd2315f6ca1c76a16a31
The function does not need anything from any chainstate, so it should
not sit in the Chainstate class.
Also, mark it [[nodiscard]], and the one place that ignores the return
value with (void).
Also, change the error log strings to not include the __func__, which is
redundant with -logsourcelocations. This is not a refactor, but this log
is only for debugging extremely rare errors.
Add a belt-and-suspenders feature, limit the amount of
memory and cpu possible when unlucky or simply misconfigured.
The worst case limit is roughly 400kB * 10,000 = 4GB, regardless
of usage pattern.
Before this change, sheer volume of broadcasts, mismatches in
standardness rules, or simply fee mismatches may result in unbounded
growth of memory usage. As the feature may be expanded in
the future, explicit bounds helps reasoning going forward.
Add a configuration option for the number of worker threads used for
parallel UTXO prevout prefetching during block connection.
Default is 8 threads, max is 16, 0 disables parallel fetching.
b847626562 test: refresh MiniWallet after node restart (Sjors Provoost)
f4e643cb15 test: merge mining options in package feerate check (Sjors Provoost)
280ce6a0ae miner: ensure block_max_weight is flattened before limit checks (Sjors Provoost)
65bd3164fb mining: clarify test_block_validity comment (Sjors Provoost)
978e7216e6 test: use shared default_ipc_timeout (Sjors Provoost)
Pull request description:
This implement the suggested followups from #33966. Each commit links to the original comment.
The most important change is the extra asserts added in `miner: ensure block_max_weight is flattened before limit checks`.
ACKs for top commit:
achow101:
ACK b847626562
enirox001:
tACK b847626562
sedited:
ACK b847626562
w0xlt:
ACK b847626562
Tree-SHA512: 47678eaed604228269bd892ccf8ff58804745bbc7675b4a93528da9a9292a2eb1e0562cdb8341edac77178563420885b48282bb9e5c2b997b28f2fc64ceeff3d
d6359937bf validation: check invariants when inserting into m_blocks_unlinked (stratospher)
0852925bd8 test/doc: remove misleading comment and improve tests (stratospher)
ca4a380281 test: add coverage for UB caused by FindMostWorkChain (stratospher)
c787b3b99b validation: avoid duplicates in m_blocks_unlinked (stratospher)
Pull request description:
This is joint work with @ mzumsande.
note: this requires a pruned node with deep reorgs to trigger. still it breaks assumptions in the codebase and is good to fix. A similar UB was fixed in https://github.com/bitcoin/bitcoin/pull/34521.
This PR prevents duplicate insertions into `m_blocks_unlinked` in `FindMostWorkChain`. There are 3 ways to insert into `m_blocks_unlinked`:
1. `LoadBlockIndex` - not problematic, as each block index is processed only once.
2. `ReceivedBlockTransactions` - not problematic, as this is usually only called once per block when it is first accepted in `AcceptBlock`. in the rare case it’s triggered again after pruning, the block would have been removed from `m_blocks_unlinked` when it was initially pruned, so duplicates still can’t arise.
3. `FindMostWorkChain` - problematic when multiple candidate tips share common chains of ancestors, traversals from each tip to the fork point may insert duplicate (`pprev`, `pindex`) entries for blocks whose parents have been pruned.
When the missing parent is later received and `ReceivedBlockTransactions` processes `m_blocks_unlinked`, the same entry may be processed multiple times. This can result in the block being re-added to `setBlockIndexCandidates` with a modified `nSequenceId`, violating its ordering invariants and leading to undefined behavior. So avoid duplicate insertions into `m_blocks_unlinked` in `FindMostWorkChain`.
### how to test:
use the updated `feature_pruning.py` which adds coverage for this scenario.
- on master: the test (with the below diff) fails since `nSequenceId` is being modified for an entry in `setBlockIndexCandidates`
- on this branch: the test (with the below diff) passes
```diff --git a/src/validation.cpp b/src/validation.cpp
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3814,6 +3814,12 @@ void ChainstateManager::ReceivedBlockTransactions(const CBlock& block, CBlockInd
pindex->nHeight, pindex->m_chain_tx_count, prev_tx_sum(*pindex), CLIENT_NAME, FormatFullVersion(), CLIENT_BUGREPORT);
}
pindex->m_chain_tx_count = prev_tx_sum(*pindex);
+ for (const auto& c : m_chainstates) {
+ if (c->setBlockIndexCandidates.contains(pindex)) {
+ LogInfo("### pindex UB = %s", pindex);
+ assert(false);
+ }
+ }
pindex->nSequenceId = nBlockSequenceId++;
for (const auto& c : m_chainstates) {
c->TryAddBlockIndexCandidate(pindex);
```
ACKs for top commit:
sedited:
Re-ACK d6359937bf
marcofleon:
crACK d6359937bf
stringintech:
ACK d6359937bf
mzumsande:
Sure - Code Review ACK [d635993](d6359937bf)
Tree-SHA512: bb21adc2d92fe1865bbbcebf775a850ca3eccac6fe83d7bca10b78eee4c0abf782e44fa0ddfec9d9a70f42fa40bdc49b68a1d1b4905cdc371ba29117d3120619
Move the accepted/new-block/reason consistency check into SubmitBlock()
so submitBlock() and submitSolution() use the same success criteria.
This keeps duplicate and inconclusive handling in one place, removes the
new_block output parameter from the helper, and makes the helper return
whether the submitted block was accepted as a new valid block.
Add reason and debug output parameters to submitSolution, matching
submitBlock. This relays the specific failure reason (e.g.
"bad-version(...)", "bad-witness-nonce-size", "duplicate") to callers
instead of just a bool.
Use a new capnp ordinal for the updated method and keep the old @7 method
as a deprecated entry point returning an explicit error, so old clients do
not decode corrupt result fields and are directed to update.
Make the submitBlock return value explicit and check that it stays
consistent with the BIP22 reason string, so future changes do not return
success with a reason or failure without one.
Report "inconclusive" when no specific block rejection reason is
available. This covers blocks accepted without being connected, and
processing failures where ProcessNewBlock returns false without an
invalid BlockChecked result, for example when ActivateBestChain fails
after BlockChecked reported a valid block.
Also document why no validation-interface queue drain is needed before
unregistering: BlockChecked is emitted synchronously by ProcessNewBlock,
unlike most validation signals.
Previously, the signal was using btcsignals::optional_last_value<bool>.
However, this only worked by accident:
The return value was influenced by the order in which the connections
were done. The noui callbacks would always overwrite the return value
with false. This makes the code overall brittle, and confusing.
For example, the following patch that changes the order of connections
would break the only and single place where the return value actually
matters:
```diff
diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
index 0b89c605b9..976549470e 100644
--- a/src/qt/bitcoin.cpp
+++ b/src/qt/bitcoin.cpp
@@ -488,3 +488,2 @@ int GuiMain(int argc, char* argv[])
btcsignals::scoped_connection handler_message_box = ::uiInterface.ThreadSafeMessageBox_connect(noui_ThreadSafeMessageBox);
- btcsignals::scoped_connection handler_question = ::uiInterface.ThreadSafeQuestion_connect(noui_ThreadSafeQuestion);
btcsignals::scoped_connection handler_init_message = ::uiInterface.InitMessage_connect(noui_InitMessage);
@@ -663,2 +662,3 @@ int GuiMain(int argc, char* argv[])
app.createWindow(networkStyle.data());
+ btcsignals::scoped_connection handler_question = ::uiInterface.ThreadSafeQuestion_connect(noui_ThreadSafeQuestion);
// Perform base initialization before spinning up initialization/shutdown thread
```
This can be tested by applying the patch and then calling:
(May have to be started twice to trigger the question)
```
bitcoin-qt -regtest -datadir=/tmp -mocktime=123456789
```
Before the changes in this commit (on current master), pressing `OK`
would not have any effect and would abort the program.
After the changes in this commit, pressing `OK` will correctly trigger a
-reindex and leave the program running.
The message will always return false (a constant) and the return value
is never used.
Also, annotate ThreadSafeMessageBox in the GUI code as [[nodiscard]],
because it may actually return a value, which is handled for questions
(but not for messages).
For an entry A -> B in m_blocks_unlinked, the entry B was added into
m_blocks_unlinked either because:
- some ancestor of B was never received (or)
- some ancestor of B was pruned away.
Every insert must satisfy two invariants:
1. B has BLOCK_HAVE_DATA set.
2. No duplicate A -> B entries in m_blocks_unlinked (this is UB zone if
this entry gets popped twice in ReceivedBlockTransactions and
happens to be in setBlockIndexCandidates)
2 bugs (#35070 and #35168) discovered recently stemmed from the
m_blocks_unlinked insertion sites not enforcing these invariants.
So add a helper which wraps around insertion sites of m_blocks_unlinked
with these invariants.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
remove misleading comment for m_blocks_unlinked since for
pruned nodes:
- usually A is the missing data (just like in non-pruned nodes)
- in PruneOneBlockFile, we remove entries once data for B is missing.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
