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:
0--- a/src/index/base.cpp
1+++ b/src/index/base.cpp
2@@ -111,6 +111,14 @@ bool BaseIndex::Init()
3 if (!synced && g_indexes_ready_to_sync) {
4 const CBlockIndex* block_to_test = start_block;
5 if (!block_to_test) block_to_test = active_chain.Genesis();
6+
7+ // Assert block_to_test is not null here. It can't be null because the
8+ // genesis block can't be null here. The genesis block will be null
9+ // during this BaseIndex::Init() call if the node is being started for
10+ // the first time, or if -reindex is used. But in both of these cases
11+ // m_best_block_index is also null so this branch is not reached.
12+ assert(block_to_test);
13+
14 if (!active_chain.Contains(block_to_test)) {
15 // if the bestblock is not part of the mainchain, find the fork
16 // so we can make sure we have all data down to the fork
17@@ -118,7 +126,7 @@ bool BaseIndex::Init()
18 }
19
20 // make sure we have all block data back to the start block
21- if (m_chainstate->m_blockman.CheckBlockDataAvailability(*active_chain.Tip(), block_to_test) != block_to_test) {
22+ if (!m_chainstate->m_blockman.CheckBlockDataAvailability(*active_chain.Tip(), *Assert(block_to_test))) {
23 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()));
24 }
25 }
26--- a/src/node/blockstorage.cpp
27+++ b/src/node/blockstorage.cpp
28@@ -402,7 +402,7 @@ bool BlockManager::IsBlockPruned(const CBlockIndex* pblockindex)
29 return (m_have_pruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0);
30 }
31
32-const CBlockIndex* BlockManager::CheckBlockDataAvailability(const CBlockIndex& upper_block, const CBlockIndex* lower_block)
33+const CBlockIndex& FindFirstStored(const CBlockIndex& upper_block, const CBlockIndex* lower_block) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
34 {
35 AssertLockHeld(::cs_main);
36 const CBlockIndex* last_block = &upper_block;
37@@ -410,7 +410,7 @@ const CBlockIndex* BlockManager::CheckBlockDataAvailability(const CBlockIndex& u
38 while (last_block->pprev && (last_block->pprev->nStatus & BLOCK_HAVE_DATA)) {
39 if (lower_block) {
40 // Return if we reached the lower_block
41- if (last_block == lower_block) return lower_block;
42+ if (last_block == lower_block) return *lower_block;
43 // if range was surpassed, means that 'lower_block' is not part of the 'upper_block' chain
44 // and so far this is not allowed.
45 assert(last_block->nHeight >= lower_block->nHeight);
46@@ -418,12 +418,17 @@ const CBlockIndex* BlockManager::CheckBlockDataAvailability(const CBlockIndex& u
47 last_block = last_block->pprev;
48 }
49 assert(last_block != nullptr);
50- return last_block;
51+ return *last_block;
52+}
53+
54+bool BlockManager::CheckBlockDataAvailability(const CBlockIndex& upper_block, const CBlockIndex& lower_block)
55+{
56+ return &FindFirstStored(upper_block, &lower_block) == &lower_block;
57 }
58
59 const CBlockIndex* BlockManager::GetFirstStoredBlock(const CBlockIndex& start_block)
60 {
61- return CheckBlockDataAvailability(start_block);
62+ return &FindFirstStored(start_block, nullptr);
63 }
64
65 // If we're using -prune with -reindex, then delete block files that will be ignored by the
66--- a/src/node/blockstorage.h
67+++ b/src/node/blockstorage.h
68@@ -217,15 +217,15 @@ public:
69 //! Returns last CBlockIndex* that is a checkpoint
70 const CBlockIndex* GetLastCheckpoint(const CCheckpointData& data) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
71
72- //! Find the oldest block that is not pruned in the [upper_block, lower_block] range.
73- //! If all blocks down to lower_block are available, returns lower_block.
74- //! If 'lower_block=nullptr': the function verifies all blocks down to the genesis block.
75+ //! Check if all blocks in the [upper_block, lower_block] range have data available.
76 //! Preconditions:
77 //! 1) The caller must ensure that upper_block has block data available.
78 //! 2) The caller must ensure that lower_block is an ancestor of upper_block (part of the same chain).
79- const CBlockIndex* CheckBlockDataAvailability(const CBlockIndex& upper_block LIFETIMEBOUND, const CBlockIndex* lower_block=nullptr) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
80+ bool CheckBlockDataAvailability(const CBlockIndex& upper_block LIFETIMEBOUND, const CBlockIndex& lower_block LIFETIMEBOUND) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
81
82- //! Find the first block that is not pruned
83+ //! Find the first stored ancestor of start_block immediately after the last
84+ //! pruned ancestor. Return value will never be null. Caller is responsible
85+ //! for ensuring that start_block has data is not pruned.
86 const CBlockIndex* GetFirstStoredBlock(const CBlockIndex& start_block LIFETIMEBOUND) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
87
88 /** True if any block files have ever been pruned. */
89diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp
90index fadf1c2026c0..a7248935f394 100644
91--- a/src/test/blockmanager_tests.cpp
92+++ b/src/test/blockmanager_tests.cpp
93@@ -107,11 +107,11 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup)
94
95 // 1) Return genesis block when all blocks are available
96 BOOST_CHECK_EQUAL(blockman.GetFirstStoredBlock(tip), chainman->ActiveChain()[0]);
97- BOOST_CHECK_EQUAL(blockman.CheckBlockDataAvailability(tip), chainman->ActiveChain()[0]);
98+ BOOST_CHECK(blockman.CheckBlockDataAvailability(tip, *chainman->ActiveChain()[0]));
99
100- // 2) Return lower_block when all blocks are available
101+ // 2) Check lower_block when all blocks are available
102 CBlockIndex* lower_block = chainman->ActiveChain()[tip.nHeight / 2];
103- BOOST_CHECK_EQUAL(blockman.CheckBlockDataAvailability(tip, lower_block), lower_block);
104+ BOOST_CHECK(blockman.CheckBlockDataAvailability(tip, *lower_block));
105
106 // Prune half of the blocks
107 int height_to_prune = tip.nHeight / 2;
108@@ -121,10 +121,8 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup)
109
110 // 3) The last block not pruned is in-between upper-block and the genesis block
111 BOOST_CHECK_EQUAL(blockman.GetFirstStoredBlock(tip), first_available_block);
112- BOOST_CHECK_EQUAL(blockman.CheckBlockDataAvailability(tip), first_available_block);
113-
114- // 4) The last block not pruned in the [tip, last_pruned_block] range is the lower_block + 1
115- BOOST_CHECK_EQUAL(blockman.CheckBlockDataAvailability(tip, last_pruned_block), first_available_block);
116+ BOOST_CHECK(blockman.CheckBlockDataAvailability(tip, *first_available_block));
117+ BOOST_CHECK(!blockman.CheckBlockDataAvailability(tip, *last_pruned_block));
118 }
119
120 BOOST_AUTO_TEST_SUITE_END()
