From 99d012ec80a4415e1a37218fb4933550276b9a0a Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Thu, 13 Nov 2025 09:43:51 -0500 Subject: [PATCH] refactor: return reference instead of pointer The return value of BlockManager::GetFirstBlock must always be non-null. This can be inferred by the implementation, which has an assertion that the return value is not null. A raw pointer should only be returned if the result may be null. In this case a reference is more appropriate. --- src/node/blockstorage.cpp | 8 ++++---- src/node/blockstorage.h | 4 ++-- src/rpc/blockchain.cpp | 6 +++--- src/test/blockmanager_tests.cpp | 4 ++-- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index a9b9b9a6360..b6e1b8b0b53 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -588,7 +588,7 @@ bool BlockManager::IsBlockPruned(const CBlockIndex& block) const return m_have_pruned && !(block.nStatus & BLOCK_HAVE_DATA) && (block.nTx > 0); } -const CBlockIndex* BlockManager::GetFirstBlock(const CBlockIndex& upper_block, uint32_t status_mask, const CBlockIndex* lower_block) const +const CBlockIndex& BlockManager::GetFirstBlock(const CBlockIndex& upper_block, uint32_t status_mask, const CBlockIndex* lower_block) const { AssertLockHeld(::cs_main); const CBlockIndex* last_block = &upper_block; @@ -596,7 +596,7 @@ const CBlockIndex* BlockManager::GetFirstBlock(const CBlockIndex& upper_block, u while (last_block->pprev && ((last_block->pprev->nStatus & status_mask) == status_mask)) { 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); @@ -604,13 +604,13 @@ const CBlockIndex* BlockManager::GetFirstBlock(const CBlockIndex& upper_block, 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) { if (!(upper_block.nStatus & BLOCK_HAVE_DATA)) return false; - return GetFirstBlock(upper_block, BLOCK_HAVE_DATA, &lower_block) == &lower_block; + return &GetFirstBlock(upper_block, BLOCK_HAVE_DATA, &lower_block) == &lower_block; } // If we're using -prune with -reindex, then delete block files that will be ignored by the diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 4ee75f071ce..9ba7873cdca 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -422,11 +422,11 @@ public: * @param lower_block The earliest possible block to return. If null, the * search can extend to the genesis block. * - * @return A non-null pointer to the earliest block between `upper_block` + * @return A reference to the earliest block between `upper_block` * and `lower_block`, inclusive, such that every block between the * returned block and `upper_block` has `status_mask` flags set. */ - const CBlockIndex* GetFirstBlock( + const CBlockIndex& GetFirstBlock( const CBlockIndex& upper_block LIFETIMEBOUND, uint32_t status_mask, const CBlockIndex* lower_block LIFETIMEBOUND = nullptr diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 4ef7bc96a4f..ff8cb119183 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -870,7 +870,7 @@ std::optional GetPruneHeight(const BlockManager& blockman, const CChain& ch // If the chain tip is pruned, everything is pruned. if (!((chain_tip->nStatus & BLOCK_HAVE_MASK) == BLOCK_HAVE_MASK)) return chain_tip->nHeight; - const auto& first_unpruned{*CHECK_NONFATAL(blockman.GetFirstBlock(*chain_tip, /*status_mask=*/BLOCK_HAVE_MASK, first_block))}; + const auto& first_unpruned{blockman.GetFirstBlock(*chain_tip, /*status_mask=*/BLOCK_HAVE_MASK, first_block)}; if (&first_unpruned == first_block) { // All blocks between first_block and chain_tip have data, so nothing is pruned. return std::nullopt; @@ -3116,8 +3116,8 @@ static RPCHelpMan dumptxoutset() if (node.chainman->m_blockman.IsPruneMode()) { LOCK(node.chainman->GetMutex()); const CBlockIndex* current_tip{node.chainman->ActiveChain().Tip()}; - const CBlockIndex* first_block{node.chainman->m_blockman.GetFirstBlock(*current_tip, /*status_mask=*/BLOCK_HAVE_MASK)}; - if (first_block->nHeight > target_index->nHeight) { + const CBlockIndex& first_block{node.chainman->m_blockman.GetFirstBlock(*current_tip, /*status_mask=*/BLOCK_HAVE_MASK)}; + if (first_block.nHeight > target_index->nHeight) { throw JSONRPCError(RPC_MISC_ERROR, "Could not roll back to requested height since necessary block data is already pruned."); } } diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp index a3504da6f79..a155199437b 100644 --- a/src/test/blockmanager_tests.cpp +++ b/src/test/blockmanager_tests.cpp @@ -119,7 +119,7 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup) }; // 1) Return genesis block when all blocks are available - BOOST_CHECK_EQUAL(blockman.GetFirstBlock(tip, BLOCK_HAVE_DATA), chainman->ActiveChain()[0]); + BOOST_CHECK_EQUAL(&blockman.GetFirstBlock(tip, BLOCK_HAVE_DATA), chainman->ActiveChain()[0]); BOOST_CHECK(blockman.CheckBlockDataAvailability(tip, *chainman->ActiveChain()[0])); // 2) Check lower_block when all blocks are available @@ -133,7 +133,7 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup) func_prune_blocks(last_pruned_block); // 3) The last block not pruned is in-between upper-block and the genesis block - BOOST_CHECK_EQUAL(blockman.GetFirstBlock(tip, BLOCK_HAVE_DATA), first_available_block); + BOOST_CHECK_EQUAL(&blockman.GetFirstBlock(tip, BLOCK_HAVE_DATA), first_available_block); BOOST_CHECK(blockman.CheckBlockDataAvailability(tip, *first_available_block)); BOOST_CHECK(!blockman.CheckBlockDataAvailability(tip, *last_pruned_block)); }