test: Fix broken chainstatemanager_snapshot_init check
What the developer wrote
Authored by Ryan Ofsky
test: Fix broken chainstatemanager_snapshot_init check
The following test code never checked anything because the if statement was
always false:
if (cs != &chainman_restarted.ActiveChainstate()) {
BOOST_CHECK_EQUAL(cs->m_chain.Height(), 109);
}
Also, the height of the background chainstate it was intending to check is 110,
not 109. Fix both problems by rewriting the check.
What changed, and why it matters
This is a fix to a broken test in Bitcoin Core's own test suite. The test was supposed to check the heights of two chainstates after a simulated node restart, but an `if` condition was always false, so the check never actually ran. The commit also corrects the expected height value. It does not change any production code that runs on real Bitcoin nodes, so it cannot directly affect live network security.
No security action required. Treat as a normal test-quality improvement. Reviewers may optionally verify that the corrected expected heights (109/210 pre-activation, 110/220 post-activation) match the intended test scenario described in the new comments.
Security signals we found
Test-only change with no production code modifications
Fixes an ineffective assertion (always-false condition) in a unit test
Corrects expected chainstate height in test assertion
Evidence from the diff
The commit modifies src/test/validation_chainstatemanager_tests.cpp, a unit test file. It fixes chainstatemanager_snapshot_init: a loop intended to verify the background chainstate height used if (cs != &chainman_restarted.ActiveChainstate()), but ActiveChainstate() at that point was the background chainstate itself, making the condition always false and the BOOST_CHECK_EQUAL never executed. The patch captures the chainstate pointers before activation, checks their heights explicitly (109 and 210), then after activation checks updated heights (110 and 220) and that only one chainstate remains. No consensus, validation, P2P, wallet, or RPC code is changed.
Changed components
src/test/validation_chainstatemanager_tests.cppInspect captured patch +17 / −10
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index bf440ca6..c6ae6088 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/src/test/validation_chainstatemanager_tests.cpp
@@ -557,7 +557,8 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup)
BOOST_CHECK_EQUAL(cs2.setBlockIndexCandidates.size(), num_indexes - last_assumed_valid_idx + 1);
}
-//! Ensure that snapshot chainstates initialize properly when found on disk.
+//! 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)
{
ChainstateManager& chainman = *Assert(m_node.chainman);
@@ -591,8 +592,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup)
BOOST_CHECK_EQUAL(bg_chainstate.m_chain.Height(), 109);
// Test that simulating a shutdown (resetting ChainstateManager) and then performing
- // chainstate reinitializing successfully cleans up the background-validation
- // chainstate data, and we end up with a single chainstate that is at tip.
+ // chainstate reinitializing successfully reloads both chainstates.
ChainstateManager& chainman_restarted = this->SimulateNodeRestart();
BOOST_TEST_MESSAGE("Performing Load/Verify/Activate of chainstate");
@@ -600,9 +600,18 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup)
// This call reinitializes the chainstates.
this->LoadVerifyActivateChainstate();
+ std::vector<Chainstate*> chainstates;
{
LOCK(chainman_restarted.GetMutex());
- BOOST_CHECK_EQUAL(chainman_restarted.GetAll().size(), 2);
+ chainstates = chainman_restarted.GetAll();
+ BOOST_CHECK_EQUAL(chainstates.size(), 2);
+ // Background chainstate has height of 109 not 110 here due to a quirk
+ // of the LoadVerifyActivate only calling ActivateBestChain on one
+ // chainstate. The height would be 110 after a real restart, but it's
+ // fine for this test which is focused on the snapshot chainstate.
+ BOOST_CHECK_EQUAL(chainstates[0]->m_chain.Height(), 109);
+ BOOST_CHECK_EQUAL(chainstates[1]->m_chain.Height(), 210);
+
BOOST_CHECK(chainman_restarted.IsSnapshotActive());
BOOST_CHECK(!chainman_restarted.IsSnapshotValidated());
@@ -618,12 +627,10 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup)
BOOST_CHECK_EQUAL(chainman_restarted.ActiveHeight(), 220);
// Background chainstate should be unaware of new blocks on the snapshot
- // chainstate.
- for (Chainstate* cs : chainman_restarted.GetAll()) {
- if (cs != &chainman_restarted.ActiveChainstate()) {
- BOOST_CHECK_EQUAL(cs->m_chain.Height(), 109);
- }
- }
+ // chainstate, but the block disconnected above is now reattached.
+ BOOST_CHECK_EQUAL(chainstates[0]->m_chain.Height(), 110);
+ BOOST_CHECK_EQUAL(chainstates[1]->m_chain.Height(), 220);
+ BOOST_CHECK_EQUAL(chainman_restarted.GetAll().size(), 1);
}
}
Why this scored 15/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.