cryptogera/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-03-16 16:32:47 +00:00
Code Issues Packages Projects Releases Wiki Activity

validation: fix UB in LoadChainTip

Browse Source 
The removal of the chain tip from setBlockIndexCandidates was
happening after nSequenceId was modified. Since the set uses
nSequenceId as a sort key, modifying it while the element is in the
set is undefined behavior, which can cause the erase to fail.

With assumeutxo, a second form of UB exists: two chainstates each
have their own candidate set, but share the same CBlockIndex
objects. Calling LoadChainTip on one chainstate mutates nSequenceIds
that are also in the other chainstate's set.

Fix by populating setBlockIndexCandidates after all changes to
nSequenceId.
This commit is contained in:
marcofleon 2026-03-04 19:39:20 +00:00
parent 9249e6089e
commit 854a6d5a9a
5 changed files with 45 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
src/node/chainstate.cpp
View File

@ -126,6 +126,13 @@ static ChainstateLoadResult CompleteChainstateInitialization(
}
}
// Populate setBlockIndexCandidates in a separate loop, after all LoadChainTip()
// calls have finished modifying nSequenceId. Because nSequenceId is used in the
// set's comparator, changing it while blocks are in the set would be UB.
for (const auto& chainstate : chainman.m_chainstates) {
chainstate->PopulateBlockIndexCandidates();
}
const auto& chainstates{chainman.m_chainstates};
if (std::any_of(chainstates.begin(), chainstates.end(),
[](const auto& cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->NeedsRedownload(); })) {

2
src/test/util/chainstate.h
View File

@ -84,7 +84,6 @@ CreateAndActivateUTXOSnapshot(
chain.InitCoinsDB(1 << 20, /*in_memory=*/true, /*should_wipe=*/false);
chain.InitCoinsCache(1 << 20);
chain.CoinsTip().SetBestBlock(gen_hash);
chain.setBlockIndexCandidates.insert(node.chainman->m_blockman.LookupBlockIndex(gen_hash));
chain.LoadChainTip();
node.chainman->MaybeRebalanceCaches();
@ -106,6 +105,7 @@ CreateAndActivateUTXOSnapshot(
pindex->nSequenceId = 0;
pindex = pindex->pprev;
}
chain.PopulateBlockIndexCandidates();
}
BlockValidationState state;
if (!node.chainman->ActiveChainstate().ActivateBestChain(state)) {

10
src/test/validation_chainstatemanager_tests.cpp
View File

@ -76,8 +76,13 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager, TestChain100Setup)
LOCK(::cs_main);
c2.InitCoinsCache(1 << 23);
c2.CoinsTip().SetBestBlock(active_tip->GetBlockHash());
c2.setBlockIndexCandidates.insert(manager.m_blockman.LookupBlockIndex(active_tip->GetBlockHash()));
for (const auto& cs : manager.m_chainstates) {
cs->ClearBlockIndexCandidates();
}
c2.LoadChainTip();
for (const auto& cs : manager.m_chainstates) {
cs->PopulateBlockIndexCandidates();
}
}
BlockValidationState _;
BOOST_CHECK(c2.ActivateBestChain(_, nullptr));
@ -492,6 +497,9 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup)
BOOST_CHECK(cs->setBlockIndexCandidates.empty());
}
chainman.LoadBlockIndex();
for (const auto& cs : chainman.m_chainstates) {
cs->PopulateBlockIndexCandidates();
}
};
// Ensure that without any assumed-valid BlockIndex entries, only the current tip is

45
src/validation.cpp
View File

@ -4604,6 +4604,12 @@ bool Chainstate::LoadChainTip()
m_chainman.UpdateIBDStatus();
tip = m_chain.Tip();
// nSequenceId is one of the keys used to sort setBlockIndexCandidates. Ensure all
// candidate sets are empty to avoid UB, as nSequenceId is about to be modified.
for (const auto& cs : m_chainman.m_chainstates) {
assert(cs->setBlockIndexCandidates.empty());
}
// Make sure our chain tip before shutting down scores better than any other candidate
// to maintain a consistent best tip over reboots in case of a tie.
auto target = tip;
@ -4612,13 +4618,6 @@ bool Chainstate::LoadChainTip()
target = target->pprev;
}
// Block index candidates are loaded before the chain tip, so we need to replace this entry
// Otherwise the scoring will be based on the memory address location instead of the nSequenceId
setBlockIndexCandidates.erase(tip);
TryAddBlockIndexCandidate(tip);
PruneBlockIndexCandidates();
tip = m_chain.Tip();
LogInfo("Loaded best chain: hashBestChain=%s height=%d date=%s progress=%f",
tip->GetBlockHash().ToString(),
m_chain.Height(),
@ -4923,6 +4922,22 @@ void Chainstate::ClearBlockIndexCandidates()
setBlockIndexCandidates.clear();
}
void Chainstate::PopulateBlockIndexCandidates()
{
AssertLockHeld(::cs_main);
for (CBlockIndex* pindex : m_blockman.GetAllBlockIndices()) {
// With assumeutxo, the snapshot block is a candidate for the tip, but it
// may not have BLOCK_VALID_TRANSACTIONS (e.g. if we haven't yet downloaded
// the block), so we special-case it here.
if (pindex == SnapshotBase() || pindex == TargetBlock() ||
(pindex->IsValid(BLOCK_VALID_TRANSACTIONS) &&
(pindex->HaveNumChainTxs() || pindex->pprev == nullptr))) {
TryAddBlockIndexCandidate(pindex);
}
}
}
bool ChainstateManager::LoadBlockIndex()
{
AssertLockHeld(cs_main);
@ -4939,19 +4954,6 @@ bool ChainstateManager::LoadBlockIndex()
for (CBlockIndex* pindex : vSortedByHeight) {
if (m_interrupt) return false;
// If we have an assumeutxo-based chainstate, then the snapshot
// block will be a candidate for the tip, but it may not be
// VALID_TRANSACTIONS (eg if we haven't yet downloaded the block),
// so we special-case the snapshot block as a potential candidate
// here.
if (pindex == CurrentChainstate().SnapshotBase() ||
(pindex->IsValid(BLOCK_VALID_TRANSACTIONS) &&
(pindex->HaveNumChainTxs() || pindex->pprev == nullptr))) {
for (const auto& chainstate : m_chainstates) {
chainstate->TryAddBlockIndexCandidate(pindex);
}
}
if (pindex->nStatus & BLOCK_FAILED_VALID && (!m_best_invalid || pindex->nChainWork > m_best_invalid->nChainWork)) {
m_best_invalid = pindex;
}
@ -5743,6 +5745,8 @@ util::Result<CBlockIndex*> ChainstateManager::ActivateSnapshot(
Chainstate& chainstate{AddChainstate(std::move(snapshot_chainstate))};
m_blockman.m_snapshot_height = Assert(chainstate.SnapshotBase())->nHeight;
chainstate.PopulateBlockIndexCandidates();
LogInfo("[snapshot] successfully activated snapshot %s", base_blockhash.ToString());
LogInfo("[snapshot] (%.2f MB)",
chainstate.CoinsTip().DynamicMemoryUsage() / (1000 * 1000));
@ -5971,7 +5975,6 @@ util::Result<void> ChainstateManager::PopulateAndValidateSnapshot(
assert(index);
assert(index == snapshot_start_block);
index->m_chain_tx_count = au_data.m_chain_tx_count;
snapshot_chainstate.setBlockIndexCandidates.insert(snapshot_start_block);
LogInfo("[snapshot] validated snapshot (%.2f MB)",
coins_cache.DynamicMemoryUsage() / (1000 * 1000));

4
src/validation.h
View File

@ -811,12 +811,16 @@ public:
/** Ensures we have a genesis block in the block tree, possibly writing one to disk. */
bool LoadGenesisBlock();
/** Add a block to the candidate set if it has as much work as the current tip. */
void TryAddBlockIndexCandidate(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
void PruneBlockIndexCandidates();
void ClearBlockIndexCandidates() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
/** Populate the candidate set by calling TryAddBlockIndexCandidate on all valid block indices. */
void PopulateBlockIndexCandidates() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
/** Find the last common block of this chain and a locator. */
const CBlockIndex* FindForkInGlobalIndex(const CBlockLocator& locator) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);