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/29770.

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #26966 (index: initial sync speedup, parallelize process by furszy)

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.

Concept ACK

@stickies-v Thanks for the feedback, I will leave this unaddressed for now until #29668 has been merged. Then I will get back to it when I take this out of draft mode.

Rebased with updates that resulted from the changes in #29668 before merge plus an additional commit that addresses left-over comments in #29668.

It would be nice to implement 81638f5 differently, without adding the <chain.h> dependency to all index headers. This dependency provides access to node internal types that indexes running on other processes (in the future) will not know about.

Should not be too complicated to get rid of the dependency. I will draft something, maybe I'll just give @maflcko s suggestion a try.

It seems we all implemented the same "index X requires block data" and "index Y requires block undo data" in slightly different ways. I did it in #26966 some time ago, and ryanofsky also did it in #24230.

My preference would be to follow #24230 and use the custom options class. It is more flexible than introducing a new method to override on all/most classes every time a new index customization is added.

I'm wouldn't call having helper functions returning bools for each case more flexible than what we have here. At least this is what I saw in #24230 and I think you mean:

virtual bool RequiresBlockUndoData() const { return false; }

It may be more readable in the short term but if we build more complicated stuff beyond just checking undo data I think this will be complicated. And we already use the mask everywhere so why not build on this? I like being able to grep for the blockstatus enum values too in order to see where block data is checked in various ways, that would be also lost.

maflcko's suggestion wouldn't remove the dependency. Unless you place the new enum somewhere else?. Which I don't think it worth it.

Yes, I was thinking of putting it in a file for types which we have been doing in the code base in several places. And @ryanofsky suggests introducing a kernel/types.h here already. Moving code isn't hard to review and chain.h is big enough to be broken up a bit more IMO.

If we agree that the final goal of the indexes is to run in isolation, in a standalone process, and be the first consumers of the kernel library, then linking them to chain internal fields like the CBlockIndex status mask isn't the way to go. These objects will live only within the kernel process.

Yes, I hope they can run in isolation in the future. But the index still needs to have an understanding of what block data is and what it needs in order to decide if it can handle what the the kernel gives it. So I don't think it can be avoided that the index has knowledge of the block status so this would need to be shared. That doesn't mean that they can't run isolation, but the kernel needs to share this knowledge with them. We can only avoid this if we forget about the concept suggested here and let the index ask for data from the kernel until it hits something unexpected and fails or we basically reimplement the block status as a list of bools in the index which will be much harder to reason about and maintain. I don't see how an options object prevents that.

just rebased

@fjahr can you rebase this, I'm still interested in this change.

Rebased and addressed @furszy 's comment here, sorry for the long delay!

Rebased and adapted the changes that #32694 introduced.

I think you could implement this with a single chain traversal and without too many changes if you make CheckBlockDataAvailability aware of the lack of undo data in the genesis block. See furszy@927c13d and furszy@be23f17

It was a bit hard for me to relate the changes to the changes here but it seems to me that the major reason this is simpler is because there is no real distinction between indexes that need undo data and indexes that don't need it. This is similar to the suggestion by @mzumsande here previously: #29770 (review). I think the handling of it isn't ideal because it's not consistent. Image two indices are present, one (A) is caught up close to the tip and requires undo data, the other (B) was just started and hasn't synced at all but doesn't need undo data. If A is before B in the list of indexes, then the full chain will be checked for undo data because A latched the undo check to true and then B required the full chain to be checked. If B is before A in the list of indexes no blocks are checked for undo data, not even the few needed for A, because the undo check never latches to true.

Does that seem accurate to you? I didn't test this, just looked at your code. Please also note that I try to do a single traversal here in the code too, it is just broken into two different steps. To stay with the example above, my code here should check the few needed undo blocks for A in the first traversal, and then the second picks up where undo traversal ended and checks all the rest of the blocks for B without undo data. It's still a single traversal as in no block should be checked twice.

EDIT: No, that last part isn't true anymore. I had that implemented but it was making this even more complicated so I simplified it to that we don't check for block data if we have already checked all the necessary for block data + undo data. I can add this back though.

It seems fine to me to move the genesis handling from StartIndexBackgroundSync to CheckBlockDataAvailability, I just don't think that in itself simplifies the code. I can apply this but first let me know if we are alinged about the other changes first. Maybe after all this is too much premature optimization and we should just do something simpler like just check all blocks for undo data.

The test doesn’t appear to exercise the issue. I cherry-picked 155803b onto master, and it passes without failure. Was expecting some crash (or a not-common failure) due to the lack of undo data and the missing check for it.

Good catch! There was a race condition in the test which I fixed in the latest push. With that the test fails consistently on current master for me.

It seems fine to me to move the genesis handling from StartIndexBackgroundSync to CheckBlockDataAvailability, I just don't think that in itself simplifies the code

I think I'd prefer this move too. StartIndexBackgroundSync has too many responsibilities as is. Furszy's commit looks good to me.

think the handling of it isn't ideal because it's not consistent.

I think I agree, but maybe I am still missing something from furszy's patch. In any case the current approach seems fine to me.

Ok, I have picked the commit from @furszy moving the genesis handling from StartIndexBackgroundSync to CheckBlockDataAvailability.

Image two indices are present, one (A) is caught up close to the tip and requires undo data, the other (B) was just started and hasn't synced at all but doesn't need undo data. If A is before B in the list of indexes, then the full chain will be checked for undo data because A latched the undo check to true and then B required the full chain to be checked. If B is before A in the list of indexes no blocks are checked for undo data, not even the few needed for A, because the undo check never latches to true.

Does that seem accurate to you?

I think I understand the rationale behind your code, but the example wasn’t very helpful. Mainly because it seems easily solvable by simply checking whether undo data is required before skipping the index. Some simple code so we are sync in that regards:

start_block = tip;
requires_undo
for index in indexes:
    requires_undo |= index.RequiresUndo()
    // set lowest starting height.

status_flags = check_undo ? undo_flag | data_flag : data_flag
if (!checkBlockAvailability(tip, start_block, status_flags))
    // fail

That being said, as far as I understand it now, the rationale behind splitting the undo data check from the block data check is that their existence on disk might not be synchronized in the future (even though that’s not currently the case).
If we are sync now, I think adding a comment in the code explaining this rationale would be really helpful.

I think I understand the rationale behind your code, but the example wasn’t very helpful. Mainly because it seems easily solvable by simply checking whether undo data is required before skipping the index. Some simple code so we are sync in that regards:

Thanks! But sorry, I don't see how the pseudo-code fixes it. I stared at it for a while but it seems to have the exact same behavior as the code in furszy@be23f17 afaict. Can you point me to the difference and ideally maybe show the patch to the code in the commit. Doesn't have to compile, just so I see where the difference is in the code because I just don't see it.

That being said, as far as I understand it now, the rationale behind splitting the undo data check from the block data check is that their existence on disk might not be synchronized in the future (even though that’s not currently the case).
If we are sync now, I think adding a comment in the code explaining this rationale would be really helpful.

Ok, I can add more documentation, you probably mean within StartIndexBackgroundSync, right?

Adding a bit more context as I reflected on this: for me, I think, when I what playing around with different approaches in StartIndexBackgroundSync I eventually found it easier to reason about the problem in a very general way, i.e. what would we do if we had any number of indexes and they could all be synced to various heights and some of them need undo and some of them don't (like this . As I mentioned before, it's definitely fair to say that that's premature optimization but for me this generalized way was easier to reason about that keeping the current indexes and their constraints in the back of my head while thinking about it. Not sure if that makes sense but that's how I ended up there and I think it's a very rare case where the code does more than we need to right now but that makes it easier to reason about, not harder. But I wouldn't be surprised if others see it differently.

I stared at it for a while but it seems to have the exact same behavior as the code in furszy@be23f17 afaict. Can you point me to the difference and ideally maybe show the patch to the code in the commit. Doesn't have to compile, just so I see where the difference is in the code because I just don't see it.

Hmm, maybe easier to do it offline, but I could try. I'm not even suggesting implementing my code at this point. I ended up understanding why you split the check. I'm ok with it.

But essentially, what my pseudo-code does is extend the current checks to cover undo data if any active index requires it, regardless of its sync height. The idea was to take advantage of the fact that block and undo data are synchronized on disk, and that the bit flag check has no extra cost (checking block_status & data is the same as checking block_status & (data | undo)).

In other words, going back to your example — you mentioned something like:

Tip at 500
indexA at 400 – needs undo
indexB at 10 – no undo

And you said the problem arises when indexB appears first in the loop, right? Because no undo data is checked. But that’s not the case if the flag is just updated at the beginning of the loop.

start_block = tip;
requires_undo
for index in indexes:
    requires_undo |= index.RequiresUndo() --> set to true whenever indexA appears
    // set lowest index height.  --> this will be set to the indexB block

status_flags = check_undo ? undo_flag | data_flag : data_flag
if (!checkBlockAvailability(tip, start_block, status_flags)). --> this will be executed from tip to indexB block, checks block + undo data in this scenario
    // fail

At least this is what I understood from your example.

Ok, I can add more documentation, you probably mean within StartIndexBackgroundSync, right?

Yes. Probably a small: "verify block + undo data existence from tip down to the lowest undo-requiring height, and if no index requires undo data or any block-only index goes lower than the undo index, verify block data existence down to the lowest block-only height".