refactor: remove incorrect lifetimebounds

andrewtoth commented at 2:47 pm on November 13, 2025: contributor

You can use the attribute by adding a LIFETIMEBOUND annotation defined in src/attributes.h; please grep the codebase for examples.

While grepping, I found an incorrect usage of the LIFETIMEBOUND annotation on BlockManager::CheckBlockDataAvailability. This could be misleading about usage for other greppers. As I was looking, I also noticed a missing LIFETIMEBOUND on BlockManager::GetFirstBlock. While looking more closely at that method, it should return a reference instead of a pointer. The only reason to return a pointer is if it can be null.

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.

141117f5e8

DrahtBot commented at 2:47 pm on November 13, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33870.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, stickies-v, optout21, l0rinc, vasild

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

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.

f743e6c5dd

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.

99d012ec80

andrewtoth force-pushed on Nov 13, 2025

DrahtBot added the label CI failed on Nov 13, 2025

DrahtBot commented at 3:07 pm on November 13, 2025: contributor

🚧 At least one of the CI tasks failed. Task ASan + LSan + UBSan + integer: https://github.com/bitcoin/bitcoin/actions/runs/19335549548/job/55309378834 LLM reason (✨ experimental): Compiler error: invalid use of [[clang::lifetimebound]] via LIFETIMEBOUND macro causing build to fail in transactions.cpp.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

maflcko commented at 3:10 pm on November 13, 2025: member

missing refactor prefix in title?

maflcko commented at 3:14 pm on November 13, 2025: member

review ACK 99d012ec80a4415e1a37218fb4933550276b9a0a 💧

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 99d012ec80a4415e1a37218fb4933550276b9a0a 💧
3xyGpVDtVHHyyLs7KsRFPpe0aUkigeleeXEzzZvZti86AX7JgaUjoGNMayUzL2meLegXdcJZCG7xrzFkf1GPFDw==

andrewtoth renamed this:
~~Remove incorrect lifetimebounds~~
refactor: remove incorrect lifetimebounds
on Nov 13, 2025

DrahtBot removed the label CI failed on Nov 13, 2025

DrahtBot added the label Refactoring on Nov 13, 2025

stickies-v commented at 6:31 pm on November 13, 2025: contributor

Concept ACK

stickies-v approved

stickies-v commented at 3:14 pm on November 14, 2025: contributor

ACK 99d012ec80a4415e1a37218fb4933550276b9a0a

Fixes lifetime attributes, improves clarity of interface by excluding the possibility of null result.

in src/rpc/blockchain.cpp:3119 in 99d012ec80

3115@@ -3116,8 +3116,8 @@ static RPCHelpMan dumptxoutset()
3116         if (node.chainman->m_blockman.IsPruneMode()) {
3117             LOCK(node.chainman->GetMutex());
3118             const CBlockIndex* current_tip{node.chainman->ActiveChain().Tip()};
3119-            const CBlockIndex* first_block{node.chainman->m_blockman.GetFirstBlock(*current_tip, /*status_mask=*/BLOCK_HAVE_MASK)};
3120-            if (first_block->nHeight > target_index->nHeight) {
3121+            const CBlockIndex& first_block{node.chainman->m_blockman.GetFirstBlock(*current_tip, /*status_mask=*/BLOCK_HAVE_MASK)};

optout21 commented at 4:07 pm on November 15, 2025:

An off-topic nit: the current_tip value technically can be nullptr, and this is not checked here and not handled inside GetFirstBlock. However, this can happen only on an empty chain, and the behavior is not touched by this PR.

l0rinc commented at 6:05 pm on November 17, 2025:

You mean similarly to https://github.com/bitcoin/bitcoin/blob/99d012ec80a4415e1a37218fb4933550276b9a0a/src/rpc/blockchain.cpp#L867-L868 ?

optout21 commented at 7:56 am on November 18, 2025:

Yes, a similar check could be done in the beginning, when tip is retrieved. But prob. outside of the scope of this PR.

maflcko commented at 10:18 am on November 20, 2025:

I don’t think the tip can be nullptr in rpc code. This can just be a CHECK_NONFATAL, but this seems unrelated to lifetimebound stuff?

l0rinc commented at 10:27 am on November 20, 2025:

this seems unrelated to lifetimebound stuff

Yes, he started with:

An off-topic nit

optout21 commented at 4:12 pm on November 15, 2025: none

ACK 99d012ec80a4415e1a37218fb4933550276b9a0a Review, local build, unit tests. The change has two small code-health improvements; first concerns compiler lifetime hints and can be validated by code review. The other change – change of return value from pointer to reference reduces chance of null pointer mishandling. The affected method is used in a few places, all are checked, so the impact is minimal.

in src/node/blockstorage.h:405 in 141117f5e8 outdated

401@@ -402,7 +402,7 @@ class BlockManager
402     //! Check if all blocks in the [upper_block, lower_block] range have data available.
403     //! The caller is responsible for ensuring that lower_block is an ancestor of upper_block
404     //! (part of the same chain).
405-    bool CheckBlockDataAvailability(const CBlockIndex& upper_block LIFETIMEBOUND, const CBlockIndex& lower_block LIFETIMEBOUND) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
406+    bool CheckBlockDataAvailability(const CBlockIndex& upper_block, const CBlockIndex& lower_block) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);

l0rinc commented at 5:13 pm on November 17, 2025:

nor does BlockManager instance retain references to the parameters

First I though we might as well make it const to add more weight to the claim that LIFETIMEBOUND isn’t needed here:

0    bool CheckBlockDataAvailability(const CBlockIndex& upper_block, const CBlockIndex& lower_block) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);

But it turns out the internally called GetFirstBlock can actually be static (which would document LIFETIMEBOUND a lot better, since we know the bound is in the return value, not the instance):

0static const CBlockIndex& GetFirstBlock(
1    const CBlockIndex& upper_block LIFETIMEBOUND,
2    uint32_t status_mask,
3    const CBlockIndex* lower_block LIFETIMEBOUND = nullptr
4) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);

which would allow making this static, too:

0    static bool CheckBlockDataAvailability(const CBlockIndex& upper_block, const CBlockIndex& lower_block) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);

Changing it to static reveals that the call stack seems to pass blockman needlessly, e.g. https://github.com/bitcoin/bitcoin/blob/141117f5e8b41eb27539d217aa4e6c407c067d90/src/rpc/blockchain.cpp#L858 so changing the references shouldn’t be done in this PR - but we might as well mark them as static here for consistency.

in src/node/blockstorage.h:432 in f743e6c5dd outdated

428@@ -429,7 +429,7 @@ class BlockManager
429     const CBlockIndex* GetFirstBlock(
430         const CBlockIndex& upper_block LIFETIMEBOUND,
431         uint32_t status_mask,
432-        const CBlockIndex* lower_block = nullptr
433+        const CBlockIndex* lower_block LIFETIMEBOUND = nullptr

l0rinc commented at 5:25 pm on November 17, 2025:

Good find, this parameter was added in https://github.com/bitcoin/bitcoin/pull/27607/files#diff-ed3f90693a242b38b9719af171de8f55183576957676dfa358945bea22276bd5R232 where lower_block was already a possible return value (cc: @furszy, @ryanofsky, @TheCharlatan, @mzumsande)

in src/node/blockstorage.cpp:607 in 99d012ec80

605         }
606         last_block = last_block->pprev;
607     }
608     assert(last_block != nullptr);
609-    return last_block;
610+    return *last_block;

l0rinc commented at 5:37 pm on November 17, 2025:

given that call-site CHECK_NONFATAL, can we simplify this to:

0    return *Assert(last_block);

in src/test/blockmanager_tests.cpp:136 in 99d012ec80

132@@ -133,7 +133,7 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup)
133     func_prune_blocks(last_pruned_block);
134 
135     // 3) The last block not pruned is in-between upper-block and the genesis block
136-    BOOST_CHECK_EQUAL(blockman.GetFirstBlock(tip, BLOCK_HAVE_DATA), first_available_block);
137+    BOOST_CHECK_EQUAL(&blockman.GetFirstBlock(tip, BLOCK_HAVE_DATA), first_available_block);

l0rinc commented at 5:46 pm on November 17, 2025:

nit: first_available_block is also a non-nullable reference, please consider this alternative if you touch again:

0CBlockIndex& first_available_block = *chainman->ActiveChain()[height_to_prune + 1];
1CBlockIndex* last_pruned_block = first_available_block.pprev;
2func_prune_blocks(last_pruned_block);
3
4// 3) The last block not pruned is in-between upper-block and the genesis block
5BOOST_CHECK_EQUAL(&BlockManager::GetFirstBlock(tip, BLOCK_HAVE_DATA), &first_available_block);
6BOOST_CHECK(BlockManager::CheckBlockDataAvailability(tip, first_available_block));
7BOOST_CHECK(!BlockManager::CheckBlockDataAvailability(tip, *last_pruned_block));

(used the static references, but I think those migrations should be done in a follow-up instead)

in src/test/blockmanager_tests.cpp:122 in 99d012ec80

118@@ -119,7 +119,7 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup)
119     };
120 
121     // 1) Return genesis block when all blocks are available
122-    BOOST_CHECK_EQUAL(blockman.GetFirstBlock(tip, BLOCK_HAVE_DATA), chainman->ActiveChain()[0]);
123+    BOOST_CHECK_EQUAL(&blockman.GetFirstBlock(tip, BLOCK_HAVE_DATA), chainman->ActiveChain()[0]);

l0rinc commented at 5:48 pm on November 17, 2025:

While there are no direct unit tests for BlockManager::GetFirstBlock setting a lower_block, https://github.com/bitcoin/bitcoin/blob/3789215f73466606eb111714f596a2a5e9bb1933/src/test/blockmanager_tests.cpp#L127 exercises that path implicitly.

in src/node/blockstorage.cpp:612 in 99d012ec80

610+    return *last_block;
611 }
612 
613 bool BlockManager::CheckBlockDataAvailability(const CBlockIndex& upper_block, const CBlockIndex& lower_block)
614 {
615     if (!(upper_block.nStatus & BLOCK_HAVE_DATA)) return false;

l0rinc commented at 5:55 pm on November 17, 2025:

nit: this path is never exercised in the unit tests

l0rinc commented at 6:27 pm on November 17, 2025: contributor

ACK 99d012ec80a4415e1a37218fb4933550276b9a0a

I left a few nits and follow-up ideas, but the code is correct as it is, every suggestion can be applied in a later PR. Especially since making the methods static enables eliminating a few parameters along the call chain - which are orthogonal to this change.

l0rinc approved

vasild approved

vasild commented at 10:02 am on November 20, 2025: contributor

ACK 99d012ec80a4415e1a37218fb4933550276b9a0a

Reviewing is learning, I love it! Thanks!

fanquake merged this on Nov 20, 2025

fanquake closed this on Nov 20, 2025

refactor: remove incorrect lifetimebounds #33870

Code Coverage & Benchmarks

Reviews