index: Fix missing case in the comment in NextSyncBlock()

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

The comment here overlooked a specific case where the block is the tip of m_chain, fix it.

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?

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
Signed-off-by: Hao Xu <hao.xu@linux.dev>

cf2551883b

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

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.

index: Fix missing case in the comment in NextSyncBlock() #32875

Code Coverage & Benchmarks

Reviews