From 141117f5e8b41eb27539d217aa4e6c407c067d90 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Thu, 13 Nov 2025 09:37:55 -0500 Subject: [PATCH 1/3] refactor: remove incorrect LIFETIMEBOUND annotations The return value of CheckBlockDataAvailability does not extend the lifetime of the input parameters, nor does BlockManager instance retain references to the parameters. The LIFETIMEBOUND annotations are misleading here since the lifetime of the parameters are not extended past the method call. --- src/node/blockstorage.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index d5da95c7a7d..9a6199193e2 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -402,7 +402,7 @@ public: //! 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); + bool CheckBlockDataAvailability(const CBlockIndex& upper_block, const CBlockIndex& lower_block) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); /** * @brief Returns the earliest block with specified `status_mask` flags set after From f743e6c5dd386b7535e6c9442923a6ee54341994 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Thu, 13 Nov 2025 09:40:45 -0500 Subject: [PATCH 2/3] refactor: add missing LIFETIMEBOUND annotation for parameter The BlockManager::GetFirstBlock lower_block parameter can have its lifetime extended by the return parameter. In the case where lower_block is returned, its lifetime will be bound to the return value. A LIFETIMEBOUND annotation is appropriate here. --- src/node/blockstorage.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 9a6199193e2..4ee75f071ce 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -429,7 +429,7 @@ public: const CBlockIndex* GetFirstBlock( const CBlockIndex& upper_block LIFETIMEBOUND, uint32_t status_mask, - const CBlockIndex* lower_block = nullptr + const CBlockIndex* lower_block LIFETIMEBOUND = nullptr ) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); /** True if any block files have ever been pruned. */ From 99d012ec80a4415e1a37218fb4933550276b9a0a Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Thu, 13 Nov 2025 09:43:51 -0500 Subject: [PATCH 3/3] 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)); }