index: handle case where pindex_prev equals chain tip in NextSyncBlock() #32875

pull HowHsu wants to merge 3 commits into bitcoin:master from HowHsu:fix-comment changing 3 files +27 −2
  1. HowHsu commented at 10:03 am on July 4, 2025: none

    Background

    NextSyncBlock() is used in BaseIndex::Sync() to get the next block index to be synced, so that BaseIndex::Sync() can build the index for that block. https://github.com/bitcoin/bitcoin/blob/1d3243806da6db8bd13e6b20906628ed434362b6/src/index/base.cpp#L150-L151

    It normally calls chain.Next(pindex_prev) to get the next block index on the active chain. If that fails, pindex_prev is on a fork. In that case, chain.FindFork(pindex_prev) is called to return the fork point, and then chain.Next(fork point) is returned for the caller to handle. The latter logic is referred to as the fork code below.

    Issue:

    The current master implementation of NextSyncBlock() handles a specific case implicitly in the fork code.

    The special case is pindex_prev == chain tip (simplified as the special case below). This means the input block is already the tip of the active chain, and there is no next block to be indexed anymore. Thus, NextSyncBlock() should return nullptr. This expectation is also implied in BaseIndex::Sync(): https://github.com/bitcoin/bitcoin/blob/1d3243806da6db8bd13e6b20906628ed434362b6/src/index/base.cpp#L220-L221

    Although NextSyncBlock() does return nullptr in this case, it is handled in a confusing code flow: https://github.com/bitcoin/bitcoin/blob/1d3243806da6db8bd13e6b20906628ed434362b6/src/index/base.cpp#L150-L166

    calls line 158: chain.Next(pindex_prev) returns nullptr because the below code flow: https://github.com/bitcoin/bitcoin/blob/1d3243806da6db8bd13e6b20906628ed434362b6/src/chain.h#L417-L423

    calls line 419: chain.Contains(): https://github.com/bitcoin/bitcoin/blob/1d3243806da6db8bd13e6b20906628ed434362b6/src/chain.h#L411-L414

    calls line 405: https://github.com/bitcoin/bitcoin/blob/1d3243806da6db8bd13e6b20906628ed434362b6/src/chain.h#L403-L408

    Thus in NextSyncBlock line 159, because pindex is nullptr, it finally falls into the fork code. it is handled only coincidentally by the fork code: https://github.com/bitcoin/bitcoin/blob/1d3243806da6db8bd13e6b20906628ed434362b6/src/index/base.cpp#L163-L165

    in this code, FindFork() is called, https://github.com/bitcoin/bitcoin/blob/1d3243806da6db8bd13e6b20906628ed434362b6/src/chain.cpp#L50-L59 and because pindex is the active chain tip, the while loop on line 56 is not iterated. It finally returns pindex itself. So at last chain.Next(pindex_prev) is called again to return nullptr.

    However, the fork code is designed to handle reorgs, not this special case. This seems to be an oversight rather than something by design.

    How to test it

    Because NextSyncBlock() returns nullptr in the special case anyway, some debug code like assert() or std::cout or log sematics has to be added into this method to see the code flow in this method. The caller can be this unit test:

    https://github.com/bitcoin/bitcoin/blob/1d3243806da6db8bd13e6b20906628ed434362b6/src/test/txindex_tests.cpp#L16-L33

    Line 32 calls Sync() and it calls NextSyncBlock()

    Solution:

    Rather than modifying the comments on lines 163–164 to make them consistent with the fork code behavior, it would be better to handle this special case explicitly and early in NextSyncBlock().

  2. DrahtBot added the label UTXO Db and Indexes on Jul 4, 2025
  3. DrahtBot commented at 10:03 am on July 4, 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/32875.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept NACK l0rinc

    If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #34440 (Refactor CChain methods to use references, tests by optout21)
    • #24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  4. stickies-v commented at 2:41 pm on July 4, 2025: contributor

    You’re correct that the current docstring is incorrect. I personally don’t find your suggestion very clear either, as the two cases talk about different parts of the return statement.

    An alternative would be to keep the docstring as-is and just make the chaintip case more explicit in code:

    0    const CBlockIndex* pindex = chain.Next(pindex_prev);
1    if (pindex || pindex_prev == chain.Tip()) {
2        return pindex;
3    }

     0diff --git a/src/index/base.cpp b/src/index/base.cpp
 1index 8c958ac5f6..506ee73d2e 100644
 2--- a/src/index/base.cpp
 3+++ b/src/index/base.cpp
 4@@ -140,15 +140,12 @@ static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev, CChain&
 5     }
 6 
 7     const CBlockIndex* pindex = chain.Next(pindex_prev);
 8-    if (pindex) {
 9+    if (pindex || pindex_prev == chain.Tip()) {
10         return pindex;
11     }
12 
13-    // Two cases goes here:
14-    // 1. pindex_prev is the tip: chain.FindFork(pindex_pre) returns pindex_prev itself
15-    // 2. pindex_prev is not in m_chain: since block is not in the chain, return the next
16-    // block in the chain AFTER the last common ancestor. Caller will be responsible for
17-    // rewinding back to the common ancestor.
18+    // If block is not in the chain, return the next block in the chain AFTER the last common ancestor.
19+    // Caller will be responsible for rewinding back to the common ancestor.
20     return chain.Next(chain.FindFork(pindex_prev));
21 }
22
  5. HowHsu commented at 3:15 pm on July 4, 2025: none

    You’re correct that the current docstring is incorrect. I personally don’t find your suggestion very clear either, as the two cases talk about different parts of the return statement.

    An alternative would be to keep the docstring as-is and just make the chaintip case more explicit in code:

    0    const CBlockIndex* pindex = chain.Next(pindex_prev);
1    if (pindex || pindex_prev == chain.Tip()) {
2        return pindex;
3    }

    git diff on d6bf3b1

    True, I’ll update it.

  6. HowHsu force-pushed on Jul 4, 2025
  7. HowHsu commented at 3:41 pm on July 4, 2025: none

    You’re correct that the current docstring is incorrect. I personally don’t find your suggestion very clear either, as the two cases talk about different parts of the return statement.

    An alternative would be to keep the docstring as-is and just make the chaintip case more explicit in code:

    0    const CBlockIndex* pindex = chain.Next(pindex_prev);
1    if (pindex || pindex_prev == chain.Tip()) {
2        return pindex;
3    }

    git diff on d6bf3b1

    Hey stickies-v, do you mind me add Suggested-by tag of you?

  8. HowHsu force-pushed on Jul 4, 2025
  9. luke-jr commented at 8:57 pm on July 7, 2025: member 
    0    CBlockIndex* NextInclRewind(const CBlockIndex* pindex) const
1    {
2        if (!Contains(Assert(pindex))) {
3            pindex = Assert(FindFork(pindex));
4        }
5        return (*this)[pindex->nHeight + 1];
6    }
  10. in src/index/base.cpp:143 in cf2551883b outdated 
    139@@ -140,7 +140,7 @@ static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev, CChain&
140     }
141 
142     const CBlockIndex* pindex = chain.Next(pindex_prev);
143-    if (pindex) {
144+    if (pindex || pindex_prev == chain.Tip()) {
    l0rinc commented at 11:19 pm on August 4, 2025:
    seems you’ve found a code that isn’t covered by tests - could you add a test case which showcases the need for this addition (such that if we revert this code the test fails)?
    HowHsu commented at 5:39 am on August 5, 2025:

    seems you’ve found a code that isn’t covered by tests - could you add a test case which showcases the need for this addition (such that if we revert this code the test fails)?

    All the cases are actually handled by this function, the thing is just when pindex_prev equals chain.Tip(), it is originally happened to be handled by return chain.Next(chain.FindFork(pindex_prev));, which is not for this case. So I split it out to be explicitly handled. Thus I guess it’s hard to make a test for this.

    l0rinc commented at 5:40 am on August 5, 2025:
    (if you delete and repost instead of editing, we’re getting multiple emails. If we need to modify it, we just cross out old version and add edit: to add the new part)
    HowHsu commented at 5:41 am on August 5, 2025:

    (if you delete and repost instead of editing, we’re getting multiple emails. if we need to modify it, we just cross out old version and add edit: to add the new part)

    Thanks for reminding. Sorry for inconvenience.

    l0rinc commented at 6:23 pm on September 18, 2025:
    I still think this should need a test as well, otherwise it just seems like a random change - especially in a PR that claims to change a comment only
    HowHsu commented at 8:12 am on September 19, 2025:

    I still think this should need a test as well, otherwise it just seems like a random change - especially in a PR that claims to change a comment only

    Hi l0rinc,

    Just like what I said, revert this commit doesn’t cause anything wrong, the work this PR does is just to handle a case explicitly rather than leaving it in return chain.Next(chain.FindFork(pindex_prev)); which is not for that case, and may cause confusion. So this PR is code clean work. I’ve updated the PR title, it isn’t about comment anymore, thanks for pointing this out.

    l0rinc commented at 11:42 pm on September 19, 2025:
    Not sure what you mean, I prefer having a test that I can run with and without the fix to debug both cases to understand the change better. Otherwise this just looks like a random change that we can just revert and the CI wouldn’t even catch it.
    HowHsu commented at 3:46 pm on September 25, 2025:

    Not sure what you mean, I prefer having a test that I can run with and without the fix to debug both cases to understand the change better. Otherwise this just looks like a random change that we can just revert and the CI wouldn’t even catch it.

    Again, why do we write a test for a code clean change?

    l0rinc commented at 4:21 pm on September 25, 2025:
    Not sure what that means, we write tests to tell us when the behavior of the application changed. You have changed the behavior and no test broke, so there’s a lack of test coverage that we should fix to explain why this change is needed. It also helps the reviewers debug the given scenario to understand why this changes is needed - or if there’s a different solution that would also solve it, etc.
    HowHsu commented at 3:24 pm on September 26, 2025:

    Not sure what that means, we write tests to tell us when the behavior of the application changed. You have changed the behavior and no test broke, so there’s a lack of test coverage that we should fix to explain why this change is

    this PR doesn’t change the behavior of NextSyncBlock(), it just tweaks the code to make the function clearer.

    optout21 commented at 1:59 pm on January 16, 2026:
    I think a better way to put it that it’s a slight optimization: in this special case (pindex_prev == chain.Tip()) it returns earlier, without going into the more general but complex handling. Correct?
    optout21 commented at 2:08 pm on January 16, 2026:

    This looks correct, but very hard to read IMHO. In the special case pindex_prev == chain.Tip() the chain.Next() will return nullptr, so in this case pindex will be nullptr. Moreover, in this case calling next could be avoided.

    I’d be happier with this:

    0    const CBlockIndex* pindex = chain.Next(pindex_prev);
1    if (pindex) {
2        return pindex;
3    }
4    if (pindex_prev == chain.Tip()) {
5        return nullptr;
6    }

    or even better:

    0    // if at the tip, there is no next
1    if (pindex_prev == chain.Tip()) {
2        return nullptr;
3    }
4    if (const CBlockIndex* pindex = chain.Next(pindex_prev); pindex) {
5        return pindex;
6    }
    HowHsu commented at 8:19 am on January 28, 2026:
    Yes, your code is better, avoid calling Next() in this case.
    l0rinc commented at 12:27 pm on January 28, 2026:
    If the current code is already covered by tests (I haven’t checked), it should be explained in the PR description (which is already too vague, it’s not obvious what problem we’re solving). If not, we should add a dedicated test. Please unresolve this comment, I don’t consider my concern addressed.
    l0rinc commented at 12:28 pm on January 28, 2026:
    If you took the advice and it contributed to the outcome, consider adding the reviewers as a coauthor, it also incentivises the reviewers.
    HowHsu commented at 1:01 pm on January 28, 2026:

    If you took the advice and it contributed to the outcome, consider adding the reviewers as a coauthor, it also incentivises the reviewers.

    Hi I0rinc, I’ve added Co-developed-by in my newest commit. Let me know if there’s anything I might have missed.

    HowHsu commented at 1:07 pm on January 28, 2026:

    I think a better way to put it that it’s a slight optimization: in this special case (pindex_prev == chain.Tip()) it returns earlier, without going into the more general but complex handling. Correct?

    Yes, optout21, your description of this change is more accurate than mine. Any hints of test this single change? I think it’s hard to do it since the original code also handles this case (but in a wrong way).

    HowHsu commented at 1:13 pm on January 28, 2026:

    If the current code is already covered by tests (I haven’t checked), it should be explained in the PR description (which is already too vague, it’s not obvious what problem we’re solving). If not, we should add a dedicated test. Please unresolve this comment, I don’t consider my concern addressed.

    Sure, I0rinc, I’ll update the description. As @optout21 said, it’s a slight optimization, so it doesn’t change the functionality of NextSyncBlock(), So I don’t have clues how to test the change, but I think we can add tests for the whole method NextSyncBlock() since there is not any unit tests for this method now.

    HowHsu commented at 2:14 pm on January 28, 2026:

    If the current code is already covered by tests (I haven’t checked), it should be explained in the PR description (which is already too vague, it’s not obvious what problem we’re solving). If not, we should add a dedicated test. Please unresolve this comment, I don’t consider my concern addressed.

    Sure, I0rinc, I’ll update the description. As @optout21 said, it’s a slight optimization, so it doesn’t change the functionality of NextSyncBlock(), So I don’t have clues how to test the change, but I think we can add tests for the whole method NextSyncBlock() since there is not any unit tests for this method now. @l0rinc description of PR updated.

    l0rinc commented at 2:18 pm on January 28, 2026:

    you still have Co-Developed-by: stickies-v, optout <optout@nostrplebs.com> in the commit message, see #32875 (review). And please unresolve that thread.

    Note that you can quote a given section from the code by adding the actual link instead which can help reviewers inspect the surrounding, e.g. https://github.com/bitcoin/bitcoin/blob/1d3243806da6db8bd13e6b20906628ed434362b6/src/index/base.cpp#L163-L165

    l0rinc commented at 12:39 pm on February 3, 2026:

    Getting back to the test coverage: I have added:

    0    if (pindex_prev == chain.Tip()) {
1        LogError("pindex_prev == chain.Tip()");
2        std::abort();
3    }

    locally and ran all unit tests to see which one exercises this new branch, revealing:

    0The following tests FAILED:
1         21 - blockfilter_index_tests (Subprocess aborted)
2         35 - coinstatsindex_tests (Subprocess aborted)
3        116 - txindex_tests (Subprocess aborted)

    But they aren’t direct calls (since it’s a static in a cpp), they just call BaseIndex::Sync().

    The simplest flow seems to be https://github.com/bitcoin/bitcoin/blob/fa5f29774872d18febc0df38831a6e45f3de69cc/src/test/txindex_tests.cpp#L30-L32 followed later by https://github.com/bitcoin/bitcoin/blob/fa5f29774872d18febc0df38831a6e45f3de69cc/src/test/txindex_tests.cpp#L56

    But we could make it more explicit in txindex_tests.cpp by:

    0    // BlockUntilSyncedToCurrentChain should return false before txindex is started.
1    BOOST_CHECK(!txindex.BlockUntilSyncedToCurrentChain());
2    txindex.Sync();
3    BOOST_CHECK(txindex.BlockUntilSyncedToCurrentChain()); // Synced to chain tip

    which proves NextSyncBlock returned nullptr for tip, see: https://github.com/bitcoin/bitcoin/blob/d3a479cb077d9a9a4451dc4ecb43fe0daf94f172/src/index/base.cpp#L233-L237

  11. HowHsu commented at 10:46 am on September 18, 2025: none 
    0    CBlockIndex* NextInclRewind(const CBlockIndex* pindex) const
1    {
2        if (!Contains(Assert(pindex))) {
3            pindex = Assert(FindFork(pindex));
4        }
5        return (*this)[pindex->nHeight + 1];
6    }

    Hi Luke, If I’m not getting you wrong, NextInclRewind() will be a method member of CChain, but from my perspective, this function seems tailored to a very specific scenario. To keep the class more general and lightweight, it might be better to set this logic as a separate function just like NextSyncBlock(). And based on that, NextSyncBlock() is already doing well, so how about just tweak if (pindex) to if (pindex || pindex_prev == chain.Tip()) to keep a minimum change?

  12. HowHsu requested review from l0rinc on Sep 18, 2025
  13. HowHsu requested review from stickies-v on Sep 18, 2025
  14. HowHsu force-pushed on Sep 18, 2025
  15. HowHsu renamed this:
    index: Fix missing case in the comment in NextSyncBlock()
    index: handle case where pindex_prev equals chain tip in NextSyncBlock()
    on Sep 19, 2025
  16. optout21 commented at 2:10 pm on January 16, 2026: contributor
    I see this as a slight optimization – early exit handling of a special case. Unit tests would be much welcome to this method! Left some comments regarding the implementation.
  17. HowHsu force-pushed on Jan 28, 2026
  18. HowHsu requested review from optout21 on Jan 28, 2026
  19. HowHsu commented at 12:57 pm on January 28, 2026: none

    I see this as a slight optimization – early exit handling of a special case. Unit tests would be much welcome to this method! Left some comments regarding the implementation.

    Yes, it can be seen as a slight optimization, but since the functionality of NextSyncBlock() is not changed, so adding a unit test for this PR change is not viable? Correct me if I’m wrong. I’ll look into add test for the whole methold if there isn’t one as you mentioned.

  20. HowHsu force-pushed on Jan 28, 2026
  21. HowHsu force-pushed on Jan 29, 2026
  22. HowHsu commented at 12:25 pm on January 30, 2026: none

    I see this as a slight optimization – early exit handling of a special case. Unit tests would be much welcome to this method! Left some comments regarding the implementation.

    Since the test will be for the entire method, not this optimization, I do think it should be in a separate PR.

  23. in src/index/base.cpp:162 in 082083345f 
    159-    if (pindex) {
160+    if (pindex_prev == chain.Tip()) {
161+        return nullptr;
162+    }
163+
164+    if (const CBlockIndex* pindex = chain.Next(pindex_prev); pindex) {
    l0rinc commented at 12:09 pm on February 3, 2026: 
    0    if (auto* pindex{chain.Next(pindex_prev)}) {
  24. in src/index/base.cpp:1 in 082083345f outdated
    l0rinc commented at 12:22 pm on February 3, 2026:

    PR description doesn’t explain what chain.Next or CChain::Contains do in case of chain.Tip() or how we can test this manually or automatically. It could also mention https://github.com/bitcoin/bitcoin/blob/b58eebf1520f503bd4c7c5693546a6859064ce51/src/index/base.cpp#L220-L221 as an existing explanation.

    There’s also no problem description, where is this code used, what are we looking at (to help reviewers and to prove you understood the change), what we’re trying to solve exactly (CChain::FindFork meant for reorgs, it’s walking the ancestor chain which is more expensive for tip than it needs to be). @optout21’s framing is best: “a slight optimization – early exit handling of a special case”

    It’s also stating: “handles a specific case in a wrong way” which is too strong: the behavior was correct, just hacky - it still returned nullptr, it’s just a bit unorthodox to do it via out of bounds in https://github.com/bitcoin/bitcoin/blob/1c2f164d348675382338bd60ea49a4105daa6fef/src/chain.h#L405-L406.

    Instead of explaining with comments, it would be even simpler to explain via a new test, something like:

     0diff --git a/src/test/chain_tests.cpp b/src/test/chain_tests.cpp
 1--- a/src/test/chain_tests.cpp	(revision b58eebf1520f503bd4c7c5693546a6859064ce51)
 2+++ b/src/test/chain_tests.cpp	(date 1770123142310)
 3@@ -82,4 +82,21 @@
 4     }
 5 }
 6 
 7+BOOST_AUTO_TEST_CASE(cchain_contains_next_test)
 8+{
 9+    CBlockIndex block0, block1;
10+    block0.nHeight = 0;
11+    block1.nHeight = 1;
12+    block1.pprev = &block0;
13+
14+    CChain chain;
15+    chain.SetTip(block1);
16+
17+    BOOST_CHECK(chain.Contains(&block0));
18+    BOOST_CHECK_EQUAL(chain.Next(&block0), &block1);
19+
20+    BOOST_CHECK(chain.Contains(&block1));
21+    BOOST_CHECK_EQUAL(chain.Next(&block1), nullptr);  // Next(Tip) == nullptr
22+}
23+
24 BOOST_AUTO_TEST_SUITE_END()
    HowHsu commented at 12:07 pm on February 5, 2026:

    Hey I0rinc, thanks for reviewing,

    PR description doesn’t explain what chain.Next or CChain::Contains do in case of chain.Tip() or how we can test this manually or automatically. It could also mention

    https://github.com/bitcoin/bitcoin/blob/b58eebf1520f503bd4c7c5693546a6859064ce51/src/index/base.cpp#L220-L221

    as an existing explanation. There’s also no problem description, where is this code used, what are we looking at (to help reviewers and to prove you understood the change), what we’re trying to solve exactly (CChain::FindFork meant for reorgs, it’s walking the ancestor chain which is more expensive for tip than it needs to be).

    I agree most what you say, CChain::FindFork meant for reorgs so the special case in this PR shouldn’t be handed by that (which in my word is: handles a specific case in a wrong way, which is not accurate), only one slight point: it’s hard to say it's walking the ancestor chain which is more expensive for tip than it needs to be is a reason, becasue FindFork() handle nullptr at it’s beginning, so yes, for pindex_prev == nullptr, FindFork() is slightly expensive due to a jmp call, but the main reason I create this PR is: FindFork() is not meant for this special case but for reorg, and the comment https://github.com/bitcoin/bitcoin/blob/b58eebf1520f503bd4c7c5693546a6859064ce51/src/index/base.cpp#L163-L164

    also explicitly indicates that FindFork() is not for that, that’s why my first version of this PR is “fix comment”

    @optout21’s framing is best: “a slight optimization – early exit handling of a special case”

    At the first version of this PR, what I did is to fix the comment on line 163 and line 164 which I quote above, I saw this PR as a fix because from the comment on line 163 and line 164, I could see the original author of NextSyncBlock() likely forgot the pindex_prev==nullptr case, it was just a coincidence that FindFork() returns the right result for pindex_prev==nullptr. Fix here means to fix the inconsistency between the comments(line 163, 164) and the code(line 165).

    It’s also stating: “handles a specific case in a wrong way” which is too strong: the behavior was correct, just hacky - it still returned nullptr, it’s just a bit unorthodox to do it via out of bounds in

    HowHsu commented at 2:24 pm on February 5, 2026:

    as an existing explanation. There’s also no problem description, where is this code used, what are we looking at (to help reviewers and to prove you understood the change), what we’re trying to solve exactly (CChain::FindFork meant for reorgs, it’s walking the ancestor chain which is more expensive for tip than it needs to be).

    I agree most what you say, CChain::FindFork meant for reorgs so the special case in this PR shouldn’t be handed by that (which in my word is: handles a specific case in a wrong way, which is not accurate), only one slight point: it’s hard to say it's walking the ancestor chain which is more expensive for tip than it needs to be is a reason, becasue FindFork() handle nullptr at it’s beginning, so yes, for pindex_prev == nullptr, FindFork() is slightly expensive due to a jmp call, but the main reason I create this PR is: FindFork() is not meant for this special case but for reorg, and the comment

    @l0rinc ignore this please.

    HowHsu commented at 8:28 am on February 6, 2026:

    Instead of explaining with comments, it would be even simpler to explain via a new test, something like:

     0diff --git a/src/test/chain_tests.cpp b/src/test/chain_tests.cpp
 1--- a/src/test/chain_tests.cpp	(revision b58eebf1520f503bd4c7c5693546a6859064ce51)
 2+++ b/src/test/chain_tests.cpp	(date 1770123142310)
 3@@ -82,4 +82,21 @@
 4     }
 5 }
 6 
 7+BOOST_AUTO_TEST_CASE(cchain_contains_next_test)
 8+{
 9+    CBlockIndex block0, block1;
10+    block0.nHeight = 0;
11+    block1.nHeight = 1;
12+    block1.pprev = &block0;
13+
14+    CChain chain;
15+    chain.SetTip(block1);
16+
17+    BOOST_CHECK(chain.Contains(&block0));
18+    BOOST_CHECK_EQUAL(chain.Next(&block0), &block1);
19+
20+    BOOST_CHECK(chain.Contains(&block1));
21+    BOOST_CHECK_EQUAL(chain.Next(&block1), nullptr);  // Next(Tip) == nullptr
22+}
23+
24 BOOST_AUTO_TEST_SUITE_END()

    BOOST_CHECK_EQUAL(chain.Next(&block1), nullptr); is actually tested implicitly in below code: https://github.com/bitcoin/bitcoin/blob/1d3243806da6db8bd13e6b20906628ed434362b6/src/test/blockfilter_index_tests.cpp#L131-L139 there seems not have any tests for Contains() . It would certainly be good to add a separate test for these basic chain semantics.

  25. l0rinc changes_requested
  26. l0rinc commented at 1:03 pm on February 3, 2026: contributor

    Concept ACK, we shouldn’t use the hacky out of bounds method to return for a common scenario, adding an explicit tip check documents the code better.

    But approach NACK, I’m not convinced the author understands the problem, so I have added additional info and tests that could be incorporated into the commits and PR description.

  27. HowHsu commented at 1:59 pm on February 3, 2026: none

    Concept ACK, we shouldn’t use the hacky out of bounds method to return for a common scenario, adding an explicit tip check documents the code better.

    But approach NACK, I’m not convinced the author understands the problem, so I have added additional info and tests that could be incorporated into the commits and PR description.

    Got a fever these days, I’ll be back looking into this tomorrow or the day after tomorrow, sorry for the delay

  28. HowHsu requested review from l0rinc on Feb 5, 2026
  29. HowHsu marked this as a draft on Feb 5, 2026
  30. l0rinc commented at 12:55 pm on February 5, 2026: contributor
    NACK, the author doesn’t understand the change or the feedback or how to work with GitHub and Git in general.
  31. HowHsu referenced this in commit 6136f68229 on Feb 5, 2026
  32. HowHsu force-pushed on Feb 5, 2026
  33. HowHsu referenced this in commit 2862022c8f on Feb 5, 2026
  34. HowHsu force-pushed on Feb 5, 2026
  35. DrahtBot added the label CI failed on Feb 5, 2026
  36. DrahtBot removed the label CI failed on Feb 5, 2026
  37. HowHsu referenced this in commit 687056ab13 on Feb 6, 2026
  38. HowHsu force-pushed on Feb 6, 2026
  39. index: add explicit early exit in NextSyncBlock() when the input is the chain tip 
    This is a slight optimization. When pindex_prev is the chain tip, return earlier
and explicitly rather than mixing it with the reorg case. For more detail, please
see PR: https://github.com/bitcoin/bitcoin/pull/32875

Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Co-authored-by: optout <optout@nostrplebs.com>
    3544cce18a
  40. HowHsu force-pushed on Feb 6, 2026
  41. DrahtBot added the label CI failed on Feb 6, 2026
  42. test: verify txindex syncs to chain tip after Sync() call 
    In txindex_initial_sync test, add an explicit check that
BlockUntilSyncedToCurrentChain() returns true immediately after Sync()
completes. This verifies the index has caught up to the chain tip,
rather than relying on later block connection logic to demonstrate this.
This also proves NextSyncBlock() returns nullptr when it reaches the
chain tip.

Co-authored-by: l0rinc <pap.lorinc@gmail.com>
    62b222a7c0
  43. DrahtBot removed the label CI failed on Feb 6, 2026
  44. HowHsu marked this as ready for review on Feb 6, 2026
  45. DrahtBot added the label CI failed on Feb 6, 2026
  46. test: add CChain::Contains and CChain::Next test cases 
    Verify that Contains() returns true for blocks in the chain, and that
Next() returns the following block or nullptr for the chain tip.

Co-authored-by: l0rinc <pap.lorinc@gmail.com>
    7d2a7597af
  47. HowHsu force-pushed on Feb 6, 2026
  48. DrahtBot removed the label CI failed on Feb 6, 2026


HowHsu DrahtBot stickies-v luke-jr l0rinc optout21


l0rinc optout21 stickies-v

Labels
UTXO Db and Indexes

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-02-07 06:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me