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

HowHsu commented at 10:03 am on July 4, 2025: none

When pindex_prev is the chain tip, return early rather than mixing it with the reorg case.

DrahtBot added the label UTXO Db and Indexes on Jul 4, 2025

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. A summary of reviews will appear here.

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

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.

HowHsu force-pushed on Jul 4, 2025

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?

HowHsu force-pushed on Jul 4, 2025

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    }

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.

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

When pindex_prev is the chain tip, return early rather than mixing it
with the reorg case.

Suggested-by: stickies-v

9ac0dfa974

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?

HowHsu requested review from l0rinc on Sep 18, 2025

HowHsu requested review from stickies-v on Sep 18, 2025

HowHsu force-pushed on Sep 18, 2025

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

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

Code Coverage & Benchmarks

Reviews