refactor: simplify pruning violation check

By generalizing 'GetFirstStoredBlock' and implementing
'CheckBlockDataAvailability' we can dedup code and
avoid repeating work when multiple indexes are enabled.
E.g. get the oldest block across all indexes and
perform the pruning violation check from that point
up to the tip only once (this feature is being introduced
in a follow-up commit).

This commit shouldn't change behavior in any way.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

4 files changed, 48 insertions, 35 deletions

diff --git a/src/index/base.cpp b/src/index/base.cpp
index ef3ba72cad..8accc2a6a4 100644
--- a/src/index/base.cpp
+++ b/src/index/base.cpp
@@ -111,37 +111,23 @@ bool BaseIndex::Init()
     const CBlockIndex* start_block = m_best_block_index.load();
     bool synced = start_block == active_chain.Tip();
     if (!synced && g_indexes_ready_to_sync) {
-        bool prune_violation = false;
-        if (!start_block) {
-            // index is not built yet
-            // make sure we have all block data back to the genesis
-            bool has_tip_data = active_chain.Tip()->nStatus & BLOCK_HAVE_DATA;
-            prune_violation = !has_tip_data || m_chainstate->m_blockman.GetFirstStoredBlock(*active_chain.Tip()) != active_chain.Genesis();
+        const CBlockIndex* block_to_test = start_block ? start_block : 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
+            block_to_test = active_chain.FindFork(block_to_test);
         }
-        // in case the index has a best block set and is not fully synced
-        // check if we have the required blocks to continue building the index
-        else {
-            const CBlockIndex* block_to_test = start_block;
-            if (!active_chain.Contains(block_to_test)) {
-                // if the bestblock is not part of the mainchain, find the fork
-                // and make sure we have all data down to the fork
-                block_to_test = active_chain.FindFork(block_to_test);
-            }
-            const CBlockIndex* block = active_chain.Tip();
-            prune_violation = true;
-            // check backwards from the tip if we have all block data until we reach the indexes bestblock
-            while (block_to_test && block && (block->nStatus & BLOCK_HAVE_DATA)) {
-                if (block_to_test == block) {
-                    prune_violation = false;
-                    break;
-                }
-                // block->pprev must exist at this point, since block_to_test is part of the chain
-                // and thus must be encountered when going backwards from the tip
-                assert(block->pprev);
-                block = block->pprev;
-            }
-        }
-        if (prune_violation) {
+
+        // make sure we have all block data back to the start block
+        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()));
         }
     }
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
index cd057dd708..e3c6b062b2 100644
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -403,17 +403,31 @@ bool BlockManager::IsBlockPruned(const CBlockIndex* pblockindex)
     return (m_have_pruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0);
 }
 
-const CBlockIndex* BlockManager::GetFirstStoredBlock(const CBlockIndex& start_block)
+const CBlockIndex* BlockManager::GetFirstStoredBlock(const CBlockIndex& upper_block, const CBlockIndex* lower_block)
 {
     AssertLockHeld(::cs_main);
-    const CBlockIndex* last_block = &start_block;
-    assert(last_block->nStatus & BLOCK_HAVE_DATA);
+    const CBlockIndex* last_block = &upper_block;
+    assert(last_block->nStatus & BLOCK_HAVE_DATA); // 'upper_block' must have data
     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 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);
+        }
         last_block = last_block->pprev;
     }
+    assert(last_block != nullptr);
     return last_block;
 }
 
+bool BlockManager::CheckBlockDataAvailability(const CBlockIndex& upper_block, const CBlockIndex& lower_block)
+{
+    if (!(upper_block.nStatus & BLOCK_HAVE_DATA)) return false;
+    return GetFirstStoredBlock(upper_block, &lower_block) == &lower_block;
+}
+
 // If we're using -prune with -reindex, then delete block files that will be ignored by the
 // reindex.  Since reindexing works by starting at block file 0 and looping until a blockfile
 // is missing, do the same here to delete any later block files after a gap.  Also delete all
diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h
index a963cad358..07ae223cbf 100644
--- a/src/node/blockstorage.h
+++ b/src/node/blockstorage.h
@@ -222,10 +222,15 @@ public:
     //! Returns last CBlockIndex* that is a checkpoint
     const CBlockIndex* GetLastCheckpoint(const CCheckpointData& data) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
+    //! Check if all blocks in the [upper_block, lower_block] range have data available.
+    //! The caller is responsible for ensuring that lower_block is an ancestor of upper_block
+    //! (part of the same chain).
+    bool CheckBlockDataAvailability(const CBlockIndex& upper_block LIFETIMEBOUND, const CBlockIndex& lower_block LIFETIMEBOUND) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+
     //! 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);
+    const CBlockIndex* GetFirstStoredBlock(const CBlockIndex& start_block LIFETIMEBOUND, const CBlockIndex* lower_block=nullptr) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
     /** True if any block files have ever been pruned. */
     bool m_have_pruned = false;
diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp
index 58ab49a329..e4ed861b12 100644
--- a/src/test/blockmanager_tests.cpp
+++ b/src/test/blockmanager_tests.cpp
@@ -92,6 +92,7 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_scan_unlink_already_pruned_files, TestChain
 
 BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup)
 {
+    // The goal of the function is to return the first not pruned block in the range [upper_block, lower_block].
     LOCK(::cs_main);
     auto& chainman = m_node.chainman;
     auto& blockman = chainman->m_blockman;
@@ -110,6 +111,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(blockman.CheckBlockDataAvailability(tip, *chainman->ActiveChain()[0]));
+
+    // 2) Check lower_block when all blocks are available
+    CBlockIndex* lower_block = chainman->ActiveChain()[tip.nHeight / 2];
+    BOOST_CHECK(blockman.CheckBlockDataAvailability(tip, *lower_block));
 
     // Prune half of the blocks
     int height_to_prune = tip.nHeight / 2;
@@ -117,8 +123,10 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup)
     CBlockIndex* last_pruned_block = first_available_block->pprev;
     func_prune_blocks(last_pruned_block);
 
-    // 2) The last block not pruned is in-between upper-block and the genesis block
+    // 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(blockman.CheckBlockDataAvailability(tip, *first_available_block));
+    BOOST_CHECK(!blockman.CheckBlockDataAvailability(tip, *last_pruned_block));
 }
 
 BOOST_AUTO_TEST_SUITE_END()