mirror of
https://github.com/bitcoin/bitcoin.git
synced 2026-07-16 09:36:54 +02:00
6b58eb6d51
d6359937bfvalidation: check invariants when inserting into m_blocks_unlinked (stratospher)0852925bd8test/doc: remove misleading comment and improve tests (stratospher)ca4a380281test: add coverage for UB caused by FindMostWorkChain (stratospher)c787b3b99bvalidation: 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-ACKd6359937bfmarcofleon: crACKd6359937bfstringintech: ACKd6359937bfmzumsande: Sure - Code Review ACK [d635993](d6359937bf) Tree-SHA512: bb21adc2d92fe1865bbbcebf775a850ca3eccac6fe83d7bca10b78eee4c0abf782e44fa0ddfec9d9a70f42fa40bdc49b68a1d1b4905cdc371ba29117d3120619
src/node/
The src/node/ directory contains code that needs to access node state
(state in CChain, CBlockIndex, CCoinsView, CTxMemPool, and similar
classes).
Code in src/node/ is meant to be segregated from code in
src/wallet/ and src/qt/, to ensure wallet and GUI
code changes don't interfere with node operation, to allow wallet and GUI code
to run in separate processes, and to perhaps eventually allow wallet and GUI
code to be maintained in separate source repositories.
As a rule of thumb, code in one of the src/node/,
src/wallet/, or src/qt/ directories should avoid
calling code in the other directories directly, and only invoke it indirectly
through the more limited src/interfaces/ classes.
This directory is at the moment
sparsely populated. Eventually more substantial files like
src/validation.cpp and
src/txmempool.cpp might be moved there.