index: handle case where pindex_prev equals chain tip in NextSyncBlock() #32875
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32875. ReviewsSee the guideline for information on the review process. ConflictsNo conflicts as of last run. |
stickies-v
left a comment
There was a problem hiding this comment.
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:
const CBlockIndex* pindex = chain.Next(pindex_prev);
if (pindex || pindex_prev == chain.Tip()) {
return pindex;
}git diff on d6bf3b1cc2
diff --git a/src/index/base.cpp b/src/index/base.cpp
index 8c958ac5f6..506ee73d2e 100644
--- a/src/index/base.cpp
+++ b/src/index/base.cpp
@@ -140,15 +140,12 @@ static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev, CChain&
}
const CBlockIndex* pindex = chain.Next(pindex_prev);
- if (pindex) {
+ if (pindex || pindex_prev == chain.Tip()) {
return pindex;
}
- // Two cases goes here:
- // 1. pindex_prev is the tip: chain.FindFork(pindex_pre) returns pindex_prev itself
- // 2. pindex_prev is not in m_chain: since block is not in the chain, return the next
- // block in the chain AFTER the last common ancestor. Caller will be responsible for
- // rewinding back to the common ancestor.
+ // If block is not in the chain, return the next block in the chain AFTER the last common ancestor.
+ // Caller will be responsible for rewinding back to the common ancestor.
return chain.Next(chain.FindFork(pindex_prev));
}
True, I'll update it. |
Hey stickies-v, do you mind me add Suggested-by tag of you? |
CBlockIndex* NextInclRewind(const CBlockIndex* pindex) const
{
if (!Contains(Assert(pindex))) {
pindex = Assert(FindFork(pindex));
}
return (*this)[pindex->nHeight + 1];
} |
When pindex_prev is the chain tip, return early rather than mixing it with the reorg case. Suggested-by: stickies-v
Hi Luke, |
|
|
||
| const CBlockIndex* pindex = chain.Next(pindex_prev); | ||
| if (pindex) { | ||
| if (pindex || pindex_prev == chain.Tip()) { |
There was a problem hiding this comment.
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:
const CBlockIndex* pindex = chain.Next(pindex_prev);
if (pindex) {
return pindex;
}
if (pindex_prev == chain.Tip()) {
return nullptr;
}
or even better:
// if at the tip, there is no next
if (pindex_prev == chain.Tip()) {
return nullptr;
}
if (const CBlockIndex* pindex = chain.Next(pindex_prev); pindex) {
return pindex;
}
|
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. |
When pindex_prev is the chain tip, return early rather than mixing it
with the reorg case.