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

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

    When pindex_prev is the chain tip, NextSyncBlock() should return nullptr (there’s no next block to sync). Currently this works, but relies on the fork/reorg path to produce the result: chain.Next(tip) returns nullptr, falling through to FindFork(tip) which returns tip itself, then chain.Next(tip) returns nullptr again. Add an explicit early return for this case. This makes NextSyncBlock()’s three cases self-documenting:

    1. next block exists on the active chain — return it
    2. at the chain tip — return nullptr
    3. on a stale branch — find the fork point and return the block after it

    It also makes the existing comment (“Since block is not in the chain”) accurate — the tip is in the chain, so it shouldn’t reach that path.

  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
    ACK optout21, furszy, stickies-v
    Concept ACK 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:

    • #34489 (index: batch db writes during initial sync by furszy)
    • #34440 (Refactor CChain methods to use references, tests by optout21)

    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: 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    }

    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: 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    }

    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: 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: contributor 
    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: 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.

    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: 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.

    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.

    optout21 commented at 6:00 am on February 7, 2026:

    there seems not have any tests for Contains() .

    See #34416

    HowHsu commented at 10:15 am on February 7, 2026:

    there seems not have any tests for Contains() .

    See #34416

    Compared to that, the test coverage has already been included in #34416, so I’ll remove this one. @l0rinc

  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: 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.

    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. HowHsu referenced this in commit 3544cce18a on Feb 6, 2026
  40. HowHsu force-pushed on Feb 6, 2026
  41. DrahtBot added the label CI failed on Feb 6, 2026
  42. DrahtBot removed the label CI failed on Feb 6, 2026
  43. HowHsu marked this as ready for review on Feb 6, 2026
  44. DrahtBot added the label CI failed on Feb 6, 2026
  45. HowHsu force-pushed on Feb 6, 2026
  46. DrahtBot removed the label CI failed on Feb 6, 2026
  47. in src/index/base.cpp:162 in 3544cce18a outdated 
    159-    if (pindex) {
160+    if (pindex_prev == chain.Tip()) {
161+        return nullptr;
162+    }
163+
164+    if (auto* pindex{chain.Next(pindex_prev)}) {
    furszy commented at 5:20 pm on February 7, 2026:
    nit: const auto pindex{chain.Next(pindex_prev)}
    HowHsu commented at 7:14 am on February 9, 2026:
    done
  48. furszy commented at 5:31 pm on February 7, 2026: member

    ACK 7d2a7597af2be8edec7473c8360849a7ab835cc9

    This isn’t really an optimization as the first commit suggests. The reorg and tip paths run rarely, and this adds a Tip() call for every block during initial sync (which translates to an extra vector lookup per block).

    Still, this makes NextSyncBlock()’s behavior explicit, so merging it isn’t that bad either. We have #34489, which improves the situation by avoiding calls to NextSyncBlock() anyway.

  49. optout21 commented at 6:00 am on February 8, 2026: contributor

    This isn’t really an optimization as the first commit suggests. The reorg and tip paths run rarely, and this adds a Tip() call for every block during initial sync (which translates to an extra vector lookup per block).

    Good point to consider the cost of calling Tip() as well! I think the current version here was adapted from my suggestion, so I accept the criticism – it is not optimized for the common case (https://github.com/bitcoin/bitcoin/pull/32875#discussion_r2698656419). The original version (if (pindex || pindex_prev == chain.Tip()) { 9ac0dfa) was different, as the Tip() call would have been optimized out if pindex != null, so in the common case.

    Considering this, the reasonable way to filter out an unneeded call to Next(Fork()) in the tip special case would be to check for tip after the Next() call, but before the FindFork() call. I’m not sure it’s worth if, though

     0--- a/src/index/base.cpp
 1+++ b/src/index/base.cpp
 2@@ -158,10 +158,15 @@ static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev, CChain&
 3     const CBlockIndex* pindex = chain.Next(pindex_prev);
 4     if (pindex) {
 5         return pindex;
 6     }
 7+    // if at the tip, there is no next
 8+    if (pindex_prev == chain.Tip()) {
 9+        return nullptr;
10+    }
11     // Since block is not in the chain, return the next block in the chain AFTER the last common ancestor.
12     // Caller will be responsible for rewinding back to the common ancestor.
13     return chain.Next(chain.FindFork(pindex_prev));
14 }
  50. HowHsu commented at 6:55 am on February 9, 2026: contributor

    ACK 7d2a759

    This isn’t really an optimization as the first commit suggests. The reorg and tip paths run rarely, and this adds a Tip() call for every block during initial sync (which translates to an extra vector lookup per block).

    Thanks, I forgot this totally.

    Still, this makes NextSyncBlock()’s behavior explicit, so merging it isn’t that bad either. We have #34489, which improves the situation by avoiding calls to NextSyncBlock() anyway.

    I read that one, since the batch size is 500 (for now), and the to-be-synced chain can be very long, it may still be worth optimize NextSyncBlock(), I’ll take what @optout21 gives.

  51. HowHsu referenced this in commit b99a3f0852 on Feb 9, 2026
  52. HowHsu force-pushed on Feb 9, 2026
  53. HowHsu referenced this in commit 9fa5cf4cfd on Feb 9, 2026
  54. HowHsu force-pushed on Feb 9, 2026
  55. DrahtBot added the label CI failed on Feb 9, 2026
  56. DrahtBot removed the label CI failed on Feb 9, 2026
  57. sedited requested review from furszy on Mar 16, 2026
  58. in src/index/base.cpp:162 in 9fa5cf4cfd 
    159-    if (pindex) {
160+    if (const auto* pindex{chain.Next(pindex_prev)}) {
161         return pindex;
162     }
163 
164+    // if at the tip, there is no next
    furszy commented at 3:27 pm on March 16, 2026:

    In 9fa5cf4cfdb37d382142cbb7d6d2d17cf22becf1:

    tiny cosmetic nit:

    0    // If there is no next block, we might be synced

    To state we check whether there is a next block first.

    HowHsu commented at 11:47 am on March 17, 2026:
    done
    furszy commented at 12:47 pm on March 17, 2026:
    Already ACKed it, but for next time; no need to add me as a co-author. It was a cosmetic nit.
  59. in src/test/txindex_tests.cpp:35 in 7b24a38a01 
    30@@ -31,6 +31,10 @@ BOOST_FIXTURE_TEST_CASE(txindex_initial_sync, TestChain100Setup)
31 
32     txindex.Sync();
33 
34+    // BlockUntilSyncedToCurrentChain should return true after Sync() completes,
35+    // since all historical blocks have been indexed.
    furszy commented at 3:39 pm on March 16, 2026:

    In 7b24a38a01dd1f5e5391344497c2db6d400e898c:

    This comment is correct only because we do a BlockUntilSyncedToCurrentChain() prior to Sync() in the test, otherwise it wouldn’t be correct as we could have an old signal from a block in the past that triggers a state rewind.

    There is a PR improving this situation and I have a branch that goes even further in terms of improving the misleading BlockUntilSyncedToCurrentChain() method.

    I would suggest dropping this second commit for now. It seems orthogonal to the PR goal anyway. We can move forward with the first one as is.

    HowHsu commented at 11:48 am on March 17, 2026:
    I see, that commit has been removed. Thanks, furszy.
  60. furszy commented at 3:39 pm on March 16, 2026: member
    ACK with a nuance.
  61. HowHsu referenced this in commit e108d4a740 on Mar 17, 2026
  62. HowHsu force-pushed on Mar 17, 2026
  63. index: add explicit early exit in NextSyncBlock() when the input is the chain tip 
    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>
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
    db3c25cfae
  64. HowHsu force-pushed on Mar 17, 2026
  65. DrahtBot added the label CI failed on Mar 17, 2026
  66. DrahtBot removed the label CI failed on Mar 17, 2026
  67. furszy commented at 12:46 pm on March 17, 2026: member
    ACK db3c25cfae1cc70171cc9bd921a2bf9be2c9da7e
  68. optout21 commented at 2:03 pm on March 17, 2026: contributor

    crACK db3c25cfae1cc70171cc9bd921a2bf9be2c9da7e

    This is a slight optimization – early exit handling of a special case. The change is small and localized.

  69. l0rinc commented at 2:13 pm on March 17, 2026: contributor
    My NACK still stands, the author doesn’t understand the change and ignores comments and tests, see #32875 (review)
  70. HowHsu commented at 3:30 pm on March 17, 2026: contributor

    My NACK still stands, the author doesn’t understand the change and ignores comments and tests, see #32875 (comment)

    Hi @l0rinc,

    Regarding the comment that “the author doesn’t understand the change”, I’ve updated the PR description—could you clarify which part is still unclear or missing?

    As for test coverage, I’ve added a test. Also, in this discussion (https://github.com/bitcoin/bitcoin/pull/32875#discussion_r2941200576), furszy suggested dropping the second commit for now, since BlockUntilSyncedToCurrentChain is currently flawed.

    Thanks!

  71. sedited commented at 8:08 am on March 25, 2026: contributor
    @l0rinc can you respond to @HowHsu’s latest comment here? If you still have concerns, maybe re-enumerate them again.
  72. l0rinc commented at 1:28 pm on March 25, 2026: contributor
    Nothing has changed, he’s still ignoring my comments, and claims tests were added, even though this PR doesn’t have any. The author doesn’t understand the basics; I don’t think he should touch this code.
  73. HowHsu commented at 2:35 pm on March 25, 2026: contributor

    Nothing has changed, he’s still ignoring my comments, and claims tests were added, even though this PR doesn’t have any. The author doesn’t understand the basics; I don’t think he should touch this code.

    Please see this conversation: #32875 (review) The first type of tests you mentioned are inclued by @optout21 ’s PR, so I removed my previous added one. Please see this conversation: #32875 (review) the second type of tests you mentioned to add in like txindex_tests was added, but just like what @furszy said, it’s actually orthogonal to the goal of this PR. Let me clarify: this PR is primarily a code cleanup to make the logic clearer, with a small optimization as well—the special case handled here now returns early. How about you providing a test as you described such that if we revert this code the test fails

    Let’s align on a few basic points:

    1. For a bug fix: a unit test is needed to demonstrate that the logic fails without the fix.
    2. For a code cleanup: no new tests are required, as long as all existing tests continue to pass.
    3. For an optimization: a benchmark is typically needed. However, the change in this PR is too minor to justify adding one.

    Correct me if I’m wrong.

  74. sedited commented at 10:30 am on March 26, 2026: contributor
    Reading through this I’m confused now too. This seems like a straight forward readability refactor, basically as suggested by stickies-v in the first comment, that should not require further comment or work.
  75. furszy commented at 2:36 pm on March 26, 2026: member

    The code changes are sound and straightforward to review with no behavioral change.

    When pindex_prev is the chain tip, the existing code falls through to FindFork(), which returns pindex_prev itself, followed by Next() returning nullptr. The new code produces the same result directly.

    The motivation is also clear, NextSyncBlock handles three distinct cases: (1) a next block exists on the active chain, (2) we are at the tip, or (3) we are on a stale branch. Previously the tip case was handled implicitly through the FindFork path, which made the accompanying comment “block is not in the chain” misleading, since the tip is very much in the chain. The explicit early return makes each case self-documenting and the remaining comment accurate. The performance optimization introduced by this change is negligible as we are only skipping a chain vector access inside FindFork.

    reACK https://github.com/bitcoin/bitcoin/commit/db3c25cfae1cc70171cc9bd921a2bf9be2c9da7e

  76. stickies-v approved
  77. stickies-v commented at 9:11 pm on March 26, 2026: contributor

    ACK db3c25cfae1cc70171cc9bd921a2bf9be2c9da7e

    This is a pretty trivial refactor that makes this function much easier to understand. I frankly don’t understand the pushback (on the current version)

    @HowHsu I think it’s great you’re doing everything you can to make this PR easy-to-review. That said, I think you might have taken it a bit further than is productive: the current PR description makes this seem like way bigger of a change/deal than it is. Suggested alternative you’re free to use, if you think it’s good:

    When pindex_prev is the chain tip, NextSyncBlock() should return nullptr (there’s no next block to sync). Currently this works, but relies on the fork/reorg path to produce the result: chain.Next(tip) returns nullptr, falling through to FindFork(tip) which returns tip itself, then chain.Next(tip) returns nullptr again.

    Add an explicit early return for this case. This makes NextSyncBlock()’s three cases self-documenting:

    1. next block exists on the active chain — return it
    2. at the chain tip — return nullptr
    3. on a stale branch — find the fork point and return the block after it

    It also makes the existing comment (“Since block is not in the chain”) accurate — the tip is in the chain, so it shouldn’t reach that path.

  78. l0rinc commented at 0:44 am on March 27, 2026: contributor
    The current version seems mostly okay, but I don’t get this pushback for adding tests, but I will remove my nack and will let others decide: Concept ACK
  79. HowHsu closed this on Mar 27, 2026
  80. HowHsu reopened this on Mar 27, 2026
  81. HowHsu commented at 4:03 am on March 27, 2026: contributor

    ACK db3c25c

    This is a pretty trivial refactor that makes this function much easier to understand. I frankly don’t understand the pushback (on the current version)

    @HowHsu I think it’s great you’re doing everything you can to make this PR easy-to-review. That said, I think you might have taken it a bit further than is productive: the current PR description makes this seem like way bigger of a change/deal than it is. Suggested alternative you’re free to use, if you think it’s good:

    When pindex_prev is the chain tip, NextSyncBlock() should return nullptr (there’s no next block to sync). Currently this works, but relies on the fork/reorg path to produce the result: chain.Next(tip) returns nullptr, falling through to FindFork(tip) which returns tip itself, then chain.Next(tip) returns nullptr again. Add an explicit early return for this case. This makes NextSyncBlock()’s three cases self-documenting:

    1. next block exists on the active chain — return it
    2. at the chain tip — return nullptr
    3. on a stale branch — find the fork point and return the block after it

    It also makes the existing comment (“Since block is not in the chain”) accurate — the tip is in the chain, so it shouldn’t reach that path.

    I’ve taken this. Thanks, stickies-v.

  82. fanquake merged this on Mar 30, 2026
  83. fanquake closed this on Mar 30, 2026


HowHsu DrahtBot stickies-v luke-jr l0rinc optout21 furszy sedited


l0rinc optout21

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-04-12 15:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me