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

Merge bitcoin/bitcoin#34884: validation: remove unused code in FindMostWorkChain

Browse Source 
ba01b00d45 refactor: use for loops in FindMostWorkChain (stratospher)
aa0eef735b test: add InvalidateBlock/ReconsiderBlock asymmetry test (stratospher)
1b0b3e2c2c validation: remove redundant marking in FindMostWorkChain (stratospher)

Pull request description:

  recent PRs like #31405, #30666 mark all `m_block_index` descendants as invalid immediately whenever an invalid block is encountered in `SetBlockFailureFlags`. so by the time we reach `FindMostWorkChain`, the block in `setBlockIndexCandidates` already has `BLOCK_FAILED_VALID` set on it - not just on its ancestor. this means `pindexTest = pindexFailed` whenever `fFailedChain` fires, and the inner `while (pindexTest != pindexFailed)` loop body is never reached!

  I think we can remove it but I've just replaced it with `Assume` in this PR for safety + good to document this invariant in case the code changes in future. (noticed by @ stickies-v in https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2815053885)

  the second commit is unrelated and adds a unit test for the situation in https://github.com/bitcoin/bitcoin/issues/32173

ACKs for top commit:
  fjahr:
    re-ACK ba01b00d45
  optout21:
    crACK ba01b00d45
  w0xlt:
    ACK ba01b00d45
  ryanofsky:
    Code review ACK ba01b00d45, just tweaking comment and for loop condition since last review.

Tree-SHA512: a8be3c30b1c41b76690d16d850e87e9e71fa6a1ecaa8b90ec997ffee1aace48b336a7009a480cd016103759d79c964b3d761a13ae936523808b2930beb68dae5
This commit is contained in:
Ryan Ofsky
2026-04-09 08:31:05 -04:00
parent 2b541eeb36 ba01b00d45
commit 141fbe4d53
2 changed files with 94 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

88
src/test/validation_chainstatemanager_tests.cpp
View File

@@ -618,6 +618,94 @@ BOOST_FIXTURE_TEST_CASE(loadblockindex_invalid_descendants, TestChain100Setup)
BOOST_CHECK(child->nStatus & BLOCK_FAILED_VALID);
}
//! Verify that ReconsiderBlock clears failure flags for the target block, its ancestors, and descendants,
//! but not for sibling forks that diverge from a shared ancestor.
BOOST_FIXTURE_TEST_CASE(invalidate_block_and_reconsider_fork, TestChain100Setup)
{
ChainstateManager& chainman = *Assert(m_node.chainman);
Chainstate& chainstate = chainman.ActiveChainstate();
// we have a chain of 100 blocks: genesis(0) <- ... <- block98 <- block99 <- block100
CBlockIndex* block98;
CBlockIndex* block99;
CBlockIndex* block100;
{
LOCK(chainman.GetMutex());
block98 = chainman.ActiveChain()[98];
block99 = chainman.ActiveChain()[99];
block100 = chainman.ActiveChain()[100];
}
// create the following block constellation:
// genesis(0) <- ... <- block98 <- block99 <- block100
// <- block99' <- block100'
// by temporarily invalidating block99. the chain tip now falls to block98,
// mine 2 new blocks on top of block 98 (block99' and block100') and then restore block99 and block 100.
BlockValidationState state;
BOOST_REQUIRE(chainstate.InvalidateBlock(state, block99));
BOOST_REQUIRE(WITH_LOCK(cs_main, return chainman.ActiveChain().Tip()) == block98);
CScript coinbase_script = CScript() << ToByteVector(coinbaseKey.GetPubKey()) << OP_CHECKSIG;
for (int i = 0; i < 2; ++i) {
CreateAndProcessBlock({}, coinbase_script);
}
const CBlockIndex* fork_block99;
const CBlockIndex* fork_block100;
{
LOCK(chainman.GetMutex());
fork_block99 = chainman.ActiveChain()[99];
BOOST_REQUIRE(fork_block99->pprev == block98);
fork_block100 = chainman.ActiveChain()[100];
BOOST_REQUIRE(fork_block100->pprev == fork_block99);
}
// Restore original block99 and block100
{
LOCK(chainman.GetMutex());
chainstate.ResetBlockFailureFlags(block99);
chainman.RecalculateBestHeader();
}
chainstate.ActivateBestChain(state);
BOOST_REQUIRE(WITH_LOCK(cs_main, return chainman.ActiveChain().Tip()) == block100);
{
LOCK(chainman.GetMutex());
BOOST_CHECK(!(block100->nStatus & BLOCK_FAILED_VALID));
BOOST_CHECK(!(block99->nStatus & BLOCK_FAILED_VALID));
BOOST_CHECK(!(fork_block100->nStatus & BLOCK_FAILED_VALID));
BOOST_CHECK(!(fork_block99->nStatus & BLOCK_FAILED_VALID));
}
// Invalidate block98
BOOST_REQUIRE(chainstate.InvalidateBlock(state, block98));
{
LOCK(chainman.GetMutex());
// block98 and all descendants of block98 are marked BLOCK_FAILED_VALID
BOOST_CHECK(block98->nStatus & BLOCK_FAILED_VALID);
BOOST_CHECK(block99->nStatus & BLOCK_FAILED_VALID);
BOOST_CHECK(block100->nStatus & BLOCK_FAILED_VALID);
BOOST_CHECK(fork_block99->nStatus & BLOCK_FAILED_VALID);
BOOST_CHECK(fork_block100->nStatus & BLOCK_FAILED_VALID);
}
// Reconsider block99. ResetBlockFailureFlags clears BLOCK_FAILED_VALID from
// block99 and its ancestors (block98) and descendants (block100)
// but NOT from block99' and block100' (not a direct ancestor/descendant)
{
LOCK(chainman.GetMutex());
chainstate.ResetBlockFailureFlags(block99);
chainman.RecalculateBestHeader();
}
chainstate.ActivateBestChain(state);
{
LOCK(chainman.GetMutex());
BOOST_CHECK(!(block98->nStatus & BLOCK_FAILED_VALID));
BOOST_CHECK(!(block99->nStatus & BLOCK_FAILED_VALID));
BOOST_CHECK(!(block100->nStatus & BLOCK_FAILED_VALID));
BOOST_CHECK(fork_block99->nStatus & BLOCK_FAILED_VALID);
BOOST_CHECK(fork_block100->nStatus & BLOCK_FAILED_VALID);
}
}
//! Ensure that snapshot chainstate can be loaded when found on disk after a
//! restart, and that new blocks can be connected to both chainstates.
BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup)