5af4d6e test: add test to ensure indexes dont commit too early:
This seems like a good example for #35260 - it would help me with the review to know what the state was before the change and which commit changes the assumption and how and where it changes it.
Maybe something like this documenting the state before the fix:
BOOST_FIXTURE_TEST_CASE(coinstatsindex_commits_ahead_of_chainstate_flush, TestChain100Setup)
{
Chainstate& chainstate = Assert(m_node.chainman)->ActiveChainstate();
BlockValidationState state;
auto current_tip{[&] { return Assert(WITH_LOCK(::cs_main, return chainstate.m_chain.Tip())); }};
auto coinsdb_best_is{[&](const CBlockIndex& block) {
LOCK(::cs_main);
BOOST_CHECK(chainstate.CoinsDB().GetBestBlock() == block.GetBlockHash());
}};
auto sync_fresh_index{[&](int expected_height) {
CoinStatsIndex index{interfaces::MakeChain(m_node), /*n_cache_size=*/1_MiB, /*f_memory=*/false, /*f_wipe=*/true};
BOOST_REQUIRE(index.Init());
index.Sync();
BOOST_CHECK_EQUAL(index.GetSummary().best_block_height, expected_height);
index.Stop();
}};
auto read_persisted_index_height{[&] {
CoinStatsIndex index{interfaces::MakeChain(m_node), /*n_cache_size=*/1_MiB};
BOOST_REQUIRE(index.Init());
const int height{index.GetSummary().best_block_height};
index.Stop();
return height;
}};
auto flush_and_commit_index{[&] {
CoinStatsIndex index{interfaces::MakeChain(m_node), /*n_cache_size=*/1_MiB};
BOOST_REQUIRE(index.Init());
index.Sync();
BOOST_REQUIRE(chainstate.FlushStateToDisk(state, FlushStateMode::FORCE_FLUSH));
m_node.validation_signals->SyncWithValidationInterfaceQueue();
index.Stop();
}};
// For one unflushed index tip, check the durable height before and after the chainstate catches up.
auto check_unflushed_index_commit{[&](const CBlockIndex& index_tip, int expected_persisted_height) {
sync_fresh_index(index_tip.nHeight);
BOOST_CHECK_EQUAL(read_persisted_index_height(), expected_persisted_height);
flush_and_commit_index();
BOOST_CHECK_EQUAL(read_persisted_index_height(), index_tip.nHeight);
}};
// Start from a known durable chainstate boundary.
BOOST_REQUIRE(chainstate.FlushStateToDisk(state, FlushStateMode::FORCE_FLUSH));
auto* flushed_tip{current_tip()};
coinsdb_best_is(*flushed_tip);
// Straight-line case: connect one more block without flushing the chainstate.
CreateAndProcessBlock({}, CScript() << OP_TRUE);
auto* unflushed_tip{current_tip()};
BOOST_REQUIRE_GT(unflushed_tip->nHeight, flushed_tip->nHeight);
coinsdb_best_is(*flushed_tip);
check_unflushed_index_commit(*unflushed_tip, /*expected_persisted_height=*/unflushed_tip->nHeight); // TODO: Persists ahead of the flushed chainstate.
auto* reorg_flushed_tip{current_tip()};
coinsdb_best_is(*reorg_flushed_tip);
// Fork case: leave the flushed chainstate behind, then index a competing unflushed tip.
BOOST_REQUIRE(chainstate.InvalidateBlock(state, reorg_flushed_tip));
NodeClockContext clock_ctx{};
clock_ctx += 1s;
CreateAndProcessBlock({}, CScript() << OP_TRUE);
clock_ctx += 1s;
CreateAndProcessBlock({}, CScript() << OP_TRUE);
auto* fork_tip{current_tip()};
BOOST_REQUIRE_EQUAL(fork_tip->nHeight, reorg_flushed_tip->nHeight + 1);
BOOST_CHECK(fork_tip->GetAncestor(reorg_flushed_tip->nHeight) != reorg_flushed_tip);
coinsdb_best_is(*reorg_flushed_tip);
check_unflushed_index_commit(*fork_tip, /*expected_persisted_height=*/fork_tip->nHeight); // TODO: Persists a fork from the flushed chainstate.
}
so that the commit fixing it would have a minimal diff of what it affects exactly:
diff --git a/src/test/coinstatsindex_tests.cpp b/src/test/coinstatsindex_tests.cpp
--- a/src/test/coinstatsindex_tests.cpp (revision 7177975c1d50fd4e0164fa6093a06b68302f0115)
+++ b/src/test/coinstatsindex_tests.cpp (revision e8197142a66c98e72d2f8c6f138f4e29a9d83b94)
@@ -124,7 +124,7 @@
}
}
-BOOST_FIXTURE_TEST_CASE(coinstatsindex_commits_ahead_of_chainstate_flush, TestChain100Setup)
+BOOST_FIXTURE_TEST_CASE(coinstatsindex_commit_is_limited_by_chainstate_flush, TestChain100Setup)
{
Chainstate& chainstate = Assert(m_node.chainman)->ActiveChainstate();
BlockValidationState state;
@@ -175,7 +175,7 @@
BOOST_REQUIRE_GT(unflushed_tip->nHeight, flushed_tip->nHeight);
coinsdb_best_is(*flushed_tip);
- check_unflushed_index_commit(*unflushed_tip, /*expected_persisted_height=*/unflushed_tip->nHeight); // TODO: Persists ahead of the flushed chainstate.
+ check_unflushed_index_commit(*unflushed_tip, /*expected_persisted_height=*/0);
auto* reorg_flushed_tip{current_tip()};
coinsdb_best_is(*reorg_flushed_tip);
@@ -193,7 +193,7 @@
BOOST_CHECK(fork_tip->GetAncestor(reorg_flushed_tip->nHeight) != reorg_flushed_tip);
coinsdb_best_is(*reorg_flushed_tip);
- check_unflushed_index_commit(*fork_tip, /*expected_persisted_height=*/fork_tip->nHeight); // TODO: Persists a fork from the flushed chainstate.
+ check_unflushed_index_commit(*fork_tip, /*expected_persisted_height=*/0);
}
BOOST_AUTO_TEST_SUITE_END()
