highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-05-18 18:04:44 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#34440: refactor: Change CChain methods to use references, add tests

Browse Source 
7c75244ade Change pindexMostWork parameter of ActivateBestChainStep() to reference (optout)
c5eb283bca Change CChain::FindFork() to take ref (optout)
20b58e281a Change CChain::Next() to take reference (optout)
fe2d6e25e0 Change CChain::Contains() to take reference (optout)
db56bcd692 test: Add CChain::FindFork() tests (optout)
8333abdd91 test: Add CChain basic tests (optout)

Pull request description:

  Refactor `CChain` methods (`Contains()`, `Next()`, `FindFork()`) to use references instead of pointers, to minimize the risk of accidental `nullptr` dereference (memory access violation). Also add missing unit tests to the `CChain` class.

  The `CChain::Contains()` method (in `src/chain.h`) dereferences its input without checking. The `Next()` method also calls into this with a `nullptr` if invoked with `nullptr`. While most call sites have indirect guarantee that the input is not `nullptr`, it's not easy to establish this to all call sites with high confidence. These methods are publicly available. There is no known high-level use case to trigger this error, but the fix is easy, and makes the code safer.

  Changes:

  - Add basic unit tests for `CChain` class methods
  - Add unit tests for `CChain::FindFork()`
  - Change `CChain::Contains()` to take reference
  - Change `CChain::Next()` to take reference
  - Change `CChain::FindFork()` to take reference
  - Change `pindexMostWork` parameter of `ActivateBestChainStep()` to reference
  - Rename changed parameters (`* pindex` --> `& index`)

  Alternative. A simpler change is to stick with pointers, with extra checks where needed, see #34416 .

  This change is remotely related to and indirectly triggered by #32875 .

  Further ideas, not considered in this PR:

  - Change `InvalidateBlock()` and `PreciousBlock()` to take references.
  - Change `CChain` internals to store references instead of pointers
  - Change CChain to always have at least one element (genesis), that way there is always genesis and tip.
  - Check related methods to return reference (guaranteed non-null) -- `FindFork`, `FindEarliestAtLeast`, `FindForkInGlobalIndex`, `blockman.AddToBlockIndex`, etc.

ACKs for top commit:
  l0rinc:
    reACK 7c75244ade
  maflcko:
    re-review ACK 7c75244ade 🌅
  achow101:
    ACK 7c75244ade
  hodlinator:
    re-ACK 7c75244ade

Tree-SHA512: 122f40120058f7e1f0273b3afed9c54966c05f06b6f2fee45bc48430617f24a5e4320a9bb7bb0ac986f2accfa22fabae5cc941b949758ddca2e9fcd472b46c33
This commit is contained in:
Ava Chow
2026-04-22 15:49:51 -07:00
parent bb90899955 7c75244ade
commit 0cbd220294
16 changed files with 174 additions and 67 deletions
Download Patch File Download Diff File Expand all files Collapse all files

46
src/validation.cpp
View File

@@ -127,7 +127,7 @@ const CBlockIndex* Chainstate::FindForkInGlobalIndex(const CBlockLocator& locato
for (const uint256& hash : locator.vHave) {
const CBlockIndex* pindex{m_blockman.LookupBlockIndex(hash)};
if (pindex) {
if (m_chain.Contains(pindex)) {
if (m_chain.Contains(*pindex)) {
return pindex;
}
if (pindex->GetAncestor(m_chain.Height()) == m_chain.Tip()) {
@@ -3125,7 +3125,7 @@ CBlockIndex* Chainstate::FindMostWorkChain()
// Check whether all blocks on the path between the currently active chain and the candidate are valid.
// Just going until the active chain is an optimization, as we know all blocks in it are valid already.
bool fInvalidAncestor = false;
for (CBlockIndex *pindexTest = pindexNew; pindexTest && !m_chain.Contains(pindexTest); pindexTest = pindexTest->pprev) {
for (CBlockIndex *pindexTest = pindexNew; pindexTest && !m_chain.Contains(*pindexTest); pindexTest = pindexTest->pprev) {
assert(pindexTest->HaveNumChainTxs() || pindexTest->nHeight == 0);
// Pruned nodes may have entries in setBlockIndexCandidates for
@@ -3173,18 +3173,18 @@ void Chainstate::PruneBlockIndexCandidates() {
}
/**
* Try to make some progress towards making pindexMostWork the active block.
* pblock is either nullptr or a pointer to a CBlock corresponding to pindexMostWork.
* Try to make some progress towards making index_most_work the active block.
* pblock is either nullptr or a pointer to a CBlock corresponding to index_most_work.
*
* @returns true unless a system error occurred
*/
bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, std::vector<ConnectedBlock>& connected_blocks)
bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex& index_most_work, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, std::vector<ConnectedBlock>& connected_blocks)
{
AssertLockHeld(cs_main);
if (m_mempool) AssertLockHeld(m_mempool->cs);
const CBlockIndex* pindexOldTip = m_chain.Tip();
const CBlockIndex* pindexFork = m_chain.FindFork(pindexMostWork);
const CBlockIndex* pindexFork = m_chain.FindFork(index_most_work);
// Disconnect active blocks which are no longer in the best chain.
bool fBlocksDisconnected = false;
@@ -3208,13 +3208,13 @@ bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex*
std::vector<CBlockIndex*> vpindexToConnect;
bool fContinue = true;
int nHeight = pindexFork ? pindexFork->nHeight : -1;
while (fContinue && nHeight != pindexMostWork->nHeight) {
while (fContinue && nHeight != index_most_work.nHeight) {
// Don't iterate the entire list of potential improvements toward the best tip, as we likely only need
// a few blocks along the way.
int nTargetHeight = std::min(nHeight + 32, pindexMostWork->nHeight);
int nTargetHeight = std::min(nHeight + 32, index_most_work.nHeight);
vpindexToConnect.clear();
vpindexToConnect.reserve(nTargetHeight - nHeight);
CBlockIndex* pindexIter = pindexMostWork->GetAncestor(nTargetHeight);
CBlockIndex* pindexIter = index_most_work.GetAncestor(nTargetHeight);
while (pindexIter && pindexIter->nHeight != nHeight) {
vpindexToConnect.push_back(pindexIter);
pindexIter = pindexIter->pprev;
@@ -3223,7 +3223,7 @@ bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex*
// Connect new blocks.
for (CBlockIndex* pindexConnect : vpindexToConnect | std::views::reverse) {
if (!ConnectTip(state, pindexConnect, pindexConnect == pindexMostWork ? pblock : std::shared_ptr<const CBlock>(), connected_blocks, disconnectpool)) {
if (!ConnectTip(state, pindexConnect, pindexConnect == &index_most_work ? pblock : std::shared_ptr<const CBlock>(), connected_blocks, disconnectpool)) {
if (state.IsInvalid()) {
// The block violates a consensus rule.
if (state.GetResult() != BlockValidationResult::BLOCK_MUTATED) {
@@ -3373,7 +3373,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
// in case snapshot validation is completed during ActivateBestChainStep, the
// result of GetRole() changes from BACKGROUND to NORMAL.
const ChainstateRole chainstate_role{this->GetRole()};
if (!ActivateBestChainStep(state, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fInvalidFound, connected_blocks)) {
if (!ActivateBestChainStep(state, *pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fInvalidFound, connected_blocks)) {
// A system error occurred
return false;
}
@@ -3398,7 +3398,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
} while (!m_chain.Tip() || (starting_tip && CBlockIndexWorkComparator()(m_chain.Tip(), starting_tip)));
if (!blocks_connected) return true;
const CBlockIndex* pindexFork = m_chain.FindFork(starting_tip);
const CBlockIndex* pindexFork = starting_tip ? m_chain.FindFork(*starting_tip) : nullptr;
bool still_in_ibd = m_chainman.IsInitialBlockDownload();
if (was_in_ibd && !still_in_ibd) {
@@ -3508,7 +3508,7 @@ bool Chainstate::PreciousBlock(BlockValidationState& state, CBlockIndex* pindex)
return ActivateBestChain(state, std::shared_ptr<const CBlock>());
}
bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pindex)
bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* const pindex)
{
AssertLockNotHeld(m_chainstate_mutex);
AssertLockNotHeld(::cs_main);
@@ -3534,16 +3534,16 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
{
LOCK(cs_main);
for (auto& entry : m_blockman.m_block_index) {
CBlockIndex* candidate = &entry.second;
CBlockIndex& candidate = entry.second;
// We don't need to put anything in our active chain into the
// multimap, because those candidates will be found and considered
// as we disconnect.
// Instead, consider only non-active-chain blocks that score
// at least as good with CBlockIndexWorkComparator as the new tip.
if (!m_chain.Contains(candidate) &&
!CBlockIndexWorkComparator()(candidate, pindex->pprev) &&
!(candidate->nStatus & BLOCK_FAILED_VALID)) {
highpow_outofchain_headers.insert({candidate->nChainWork, candidate});
!CBlockIndexWorkComparator()(&candidate, pindex->pprev) &&
!(candidate.nStatus & BLOCK_FAILED_VALID)) {
highpow_outofchain_headers.insert({candidate.nChainWork, &candidate});
}
}
}
@@ -3563,9 +3563,9 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
// Lock for as long as disconnectpool is in scope to make sure MaybeUpdateMempoolForReorg is
// called after DisconnectTip without unlocking in between
LOCK(MempoolMutex());
if (!m_chain.Contains(pindex)) break;
if (!m_chain.Contains(*pindex)) break;
pindex_was_in_chain = true;
CBlockIndex* disconnected_tip{m_chain.Tip()};
CBlockIndex* const disconnected_tip{m_chain.Tip()};
// ActivateBestChain considers blocks already in m_chain
// unconditionally valid already, so force disconnect away from it.
@@ -3635,7 +3635,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
{
LOCK(cs_main);
if (m_chain.Contains(to_mark_failed)) {
if (m_chain.Contains(*to_mark_failed)) {
// If the to-be-marked invalid block is in the active chain, something is interfering and we can't proceed.
return false;
}
@@ -4712,7 +4712,7 @@ VerifyDBResult CVerifyDB::VerifyDB(
reportDone = percentageDone / 10;
}
m_notifications.progress(_("Verifying blocks…"), percentageDone, false);
pindex = chainstate.m_chain.Next(pindex);
pindex = chainstate.m_chain.Next(*pindex);
CBlock block;
if (!chainstate.m_blockman.ReadBlock(block, *pindex)) {
LogError("Verification error: ReadBlock failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString());
@@ -5149,7 +5149,7 @@ void ChainstateManager::CheckBlockIndex() const
std::multimap<const CBlockIndex*, const CBlockIndex*> forward;
for (auto& [_, block_index] : m_blockman.m_block_index) {
// Only save indexes in forward that are not part of the best header chain.
if (!best_hdr_chain.Contains(&block_index)) {
if (!best_hdr_chain.Contains(block_index)) {
// Only genesis, which must be part of the best header chain, can have a nullptr parent.
assert(block_index.pprev);
forward.emplace(block_index.pprev, &block_index);
@@ -5384,7 +5384,7 @@ void ChainstateManager::CheckBlockIndex() const
pindex = range.first->second;
nHeight++;
continue;
} else if (best_hdr_chain.Contains(pindex)) {
} else if (best_hdr_chain.Contains(*pindex)) {
// Descend further into best header chain.
nHeight++;
pindex = best_hdr_chain[nHeight];