In commit "refactor: simplify pruning violation check" (2412ffdce69ce2b164e5303ae4b610ab0d00fd5c)
Looking at this code, it's not really clear that block_to_test is going to be non-null, and that the CheckBlockDataAvailability check is going to pass. If it were null, the pruning check would fail even if there were no pruned data.
So I think it would be good add an assert with an explanation of why block_to_test can't be null here, and I implemented this below.
While implementing this I also noticed that current CheckBlockDataAvailability interface seems less safe to use because it accepts a second null parameter when there's no reason to pass it one, and it's also awkward because it returns a pointer instead of just a boolean that reflects whether data is available. Would suggest the following changes to address all of this:
--- a/src/index/base.cpp
+++ b/src/index/base.cpp
@@ -111,6 +111,14 @@ bool BaseIndex::Init()
if (!synced && g_indexes_ready_to_sync) {
const CBlockIndex* block_to_test = start_block;
if (!block_to_test) block_to_test = active_chain.Genesis();
+
+ // Assert block_to_test is not null here. It can't be null because the
+ // genesis block can't be null here. The genesis block will be null
+ // during this BaseIndex::Init() call if the node is being started for
+ // the first time, or if -reindex is used. But in both of these cases
+ // m_best_block_index is also null so this branch is not reached.
+ assert(block_to_test);
+
if (!active_chain.Contains(block_to_test)) {
// if the bestblock is not part of the mainchain, find the fork
// so we can make sure we have all data down to the fork
@@ -118,7 +126,7 @@ bool BaseIndex::Init()
}
// make sure we have all block data back to the start block
- if (m_chainstate->m_blockman.CheckBlockDataAvailability(*active_chain.Tip(), block_to_test) != block_to_test) {
+ if (!m_chainstate->m_blockman.CheckBlockDataAvailability(*active_chain.Tip(), *Assert(block_to_test))) {
return InitError(strprintf(Untranslated("%s best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)"), GetName()));
}
}
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -402,7 +402,7 @@ bool BlockManager::IsBlockPruned(const CBlockIndex* pblockindex)
return (m_have_pruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0);
}
-const CBlockIndex* BlockManager::CheckBlockDataAvailability(const CBlockIndex& upper_block, const CBlockIndex* lower_block)
+const CBlockIndex& FindFirstStored(const CBlockIndex& upper_block, const CBlockIndex* lower_block) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
{
AssertLockHeld(::cs_main);
const CBlockIndex* last_block = &upper_block;
@@ -410,7 +410,7 @@ const CBlockIndex* BlockManager::CheckBlockDataAvailability(const CBlockIndex& u
while (last_block->pprev && (last_block->pprev->nStatus & BLOCK_HAVE_DATA)) {
if (lower_block) {
// Return if we reached the lower_block
- if (last_block == lower_block) return lower_block;
+ if (last_block == lower_block) return *lower_block;
// if range was surpassed, means that 'lower_block' is not part of the 'upper_block' chain
// and so far this is not allowed.
assert(last_block->nHeight >= lower_block->nHeight);
@@ -418,12 +418,17 @@ const CBlockIndex* BlockManager::CheckBlockDataAvailability(const CBlockIndex& u
last_block = last_block->pprev;
}
assert(last_block != nullptr);
- return last_block;
+ return *last_block;
+}
+
+bool BlockManager::CheckBlockDataAvailability(const CBlockIndex& upper_block, const CBlockIndex& lower_block)
+{
+ return &FindFirstStored(upper_block, &lower_block) == &lower_block;
}
const CBlockIndex* BlockManager::GetFirstStoredBlock(const CBlockIndex& start_block)
{
- return CheckBlockDataAvailability(start_block);
+ return &FindFirstStored(start_block, nullptr);
}
// If we're using -prune with -reindex, then delete block files that will be ignored by the
--- a/src/node/blockstorage.h
+++ b/src/node/blockstorage.h
@@ -217,15 +217,15 @@ public:
//! Returns last CBlockIndex* that is a checkpoint
const CBlockIndex* GetLastCheckpoint(const CCheckpointData& data) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
- //! Find the oldest block that is not pruned in the [upper_block, lower_block] range.
- //! If all blocks down to lower_block are available, returns lower_block.
- //! If 'lower_block=nullptr': the function verifies all blocks down to the genesis block.
+ //! Check if all blocks in the [upper_block, lower_block] range have data available.
//! Preconditions:
//! 1) The caller must ensure that upper_block has block data available.
//! 2) The caller must ensure that lower_block is an ancestor of upper_block (part of the same chain).
- const CBlockIndex* CheckBlockDataAvailability(const CBlockIndex& upper_block LIFETIMEBOUND, const CBlockIndex* lower_block=nullptr) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+ bool CheckBlockDataAvailability(const CBlockIndex& upper_block LIFETIMEBOUND, const CBlockIndex& lower_block LIFETIMEBOUND) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
- //! Find the first block that is not pruned
+ //! Find the first stored ancestor of start_block immediately after the last
+ //! pruned ancestor. Return value will never be null. Caller is responsible
+ //! for ensuring that start_block has data is not pruned.
const CBlockIndex* GetFirstStoredBlock(const CBlockIndex& start_block LIFETIMEBOUND) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
/** True if any block files have ever been pruned. */
diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp
index fadf1c2026c0..a7248935f394 100644
--- a/src/test/blockmanager_tests.cpp
+++ b/src/test/blockmanager_tests.cpp
@@ -107,11 +107,11 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup)
// 1) Return genesis block when all blocks are available
BOOST_CHECK_EQUAL(blockman.GetFirstStoredBlock(tip), chainman->ActiveChain()[0]);
- BOOST_CHECK_EQUAL(blockman.CheckBlockDataAvailability(tip), chainman->ActiveChain()[0]);
+ BOOST_CHECK(blockman.CheckBlockDataAvailability(tip, *chainman->ActiveChain()[0]));
- // 2) Return lower_block when all blocks are available
+ // 2) Check lower_block when all blocks are available
CBlockIndex* lower_block = chainman->ActiveChain()[tip.nHeight / 2];
- BOOST_CHECK_EQUAL(blockman.CheckBlockDataAvailability(tip, lower_block), lower_block);
+ BOOST_CHECK(blockman.CheckBlockDataAvailability(tip, *lower_block));
// Prune half of the blocks
int height_to_prune = tip.nHeight / 2;
@@ -121,10 +121,8 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup)
// 3) The last block not pruned is in-between upper-block and the genesis block
BOOST_CHECK_EQUAL(blockman.GetFirstStoredBlock(tip), first_available_block);
- BOOST_CHECK_EQUAL(blockman.CheckBlockDataAvailability(tip), first_available_block);
-
- // 4) The last block not pruned in the [tip, last_pruned_block] range is the lower_block + 1
- BOOST_CHECK_EQUAL(blockman.CheckBlockDataAvailability(tip, last_pruned_block), first_available_block);
+ BOOST_CHECK(blockman.CheckBlockDataAvailability(tip, *first_available_block));
+ BOOST_CHECK(!blockman.CheckBlockDataAvailability(tip, *last_pruned_block));
}
BOOST_AUTO_TEST_SUITE_END()
