indexes: Stop using node internal types and locking cs_main, improve sync logic #24230

dc4675ddaca8d0d1d40847191cb21ccd116a6a3c : why bother with a pointer here? To make it consistent with phashBlock in CBlockIndex? I assume it's not just to shave 31 bytes of the BlockInfo size?

re: #24230 (review)

dc4675d : why bother with a pointer here? To make it consistent with phashBlock in CBlockIndex? I assume it's not just to shave 31 bytes of the BlockInfo size?

Yes, hash is a reference and prev_hash is a pointer to minimize copying and make the struct small. C++ doesn't provide named arguments, so I'm defining the struct as a substitute for named arguments, to make block connected and disconnected declarations more readable and the argument list easier to change in the future.

The field types are the same as the argument types that would be used otherwise. const uint256& hash could be replaced by uint256 hash, and const uint256* prev_hash could be std::optional<uint256>, and const CBlock* could be shared_ptr<CBlock>, but the various block connected and disconnected method implementations don't benefit from getting copies instead of references, or optional instead of pointers, or smart pointers instead of plain pointers, so I'm opting for the cheapest and simplest passing style in each case.

ae26ae5d3a1d32ce212d66477b71a5ac03354338: you're using context() here, but src/interfaces/chain.h suggests that should only be done in a test. Does this need a TODO? Is this resolved in one of the followup PRs?

re: #24230 (review)

ae26ae5: you're using context() here, but src/interfaces/chain.h suggests that should only be done in a test. Does this need a TODO? Is this resolved in one of the followup PRs?

Good catch. Now removed this in the last commit and added comment about it being temporary

515a1dbce29078a1e90dc3f97bf25b309face9bb: this might warrant a TODO (or note in the commit message) that this CBlockIndex is going away in a later commit.

re: #24230 (review)

515a1db: this might warrant a TODO (or note in the commit message) that this CBlockIndex is going away in a later commit.

Good suggestion, added comments to clarify

15398b1677362f4d98ffc786f1ae9976ad595ea8: comment above needs update

re: #24230 (review)

15398b1: comment above needs update

Good catch, updated

15398b1677362f4d98ffc786f1ae9976ad595ea8 nit: "and reindex" (I haven't checked if re-index nukes all the indexes, or if it leaves them alone until the previous best block is reached again)

re: #24230 (review)

15398b1 nit: "and reindex" (I haven't checked if re-index nukes all the indexes, or if it leaves them alone until the previous best block is reached again)

Rephrased the comment to avoid the word sync. This isn't referring to IBD or reindexing globally. It's just referring to notifications from the attachChain / ThreadSync code that runs when the index is loaded. For example if we added an RPC method to start and stop indexes. chain_tip would be false for the blocks read after the index was first started, until the sync thread caught up. This would be independent of whether node itself was syncing to the network.

11310351345e844ae22f9bc00bed00eff7197500 : this constraint of BLOCK_HAVE_DATA | BLOCK_HAVE_UNDO makes sense in the context of indexes, because an index is updated after a block is connected, at which point it will always have undo data. But if we ever need it in other places, it seems a bit odd. Maybe add a comment? Alternatively maybe we should just add nStatus or bools for the individual flags we care about?

1131035 : this constraint of BLOCK_HAVE_DATA | BLOCK_HAVE_UNDO makes sense in the context of indexes, because an index is updated after a block is connected, at which point it will always have undo data. But if we ever need it in other places, it seems a bit odd. Maybe add a comment? Alternatively maybe we should just add nStatus or bools for the individual flags we care about?

I guess I'll drop this. I assumed one wouldn't be present without the other, and was basing it on the thing CDiskBlockIndex is doing. Most intermediate code which uses this is deleted in the end anyway.

ade3f4306bba6e40197d645774097c78a5e0681a: this is a nice addition, e.g. block->hash is more readable than before when block would be a hash.

2504811223597b211aa52b86a6386f4c02286d50 : IIUC this makes it so the Handler that's in m_handler (before its moved) is destroyed when this function is done. Why is it insufficient to just call m_handler->disconnect() and have it be destroyed along with BaseIndex?

I guess that's what "because ... disconnect code may need to lock the mutex" refers to, but it's not super clear to me why this is.

Related to the next d148f9e2abc05c83b60f1d92bcd7884174fa9ae3 where you assert that m_handler really needs to be gone by the time destructor on BaseIndex is called.

re: #24230 (review)

2504811 : IIUC this makes it so the Handler that's in m_handler (before its moved) is destroyed when this function is done. Why is it insufficient to just call m_handler->disconnect() and have it be destroyed along with BaseIndex?

I guess that's what "because ... disconnect code may need to lock the mutex" refers to, but it's not super clear to me why this is.

Thanks, wrote a more descriptive comment. The disconnect code waits for the last notification to be processed, so if the last notification need to lock m_mutex for any reason there would be a deadlock if this code wasn't releasing it.

re: #24230 (review)

Thanks, wrote a more descriptive comment.

Actually fixed now. I messed up the last push and did not include this change.

0cefaaab906e3de84a4e2c5a15e27e56255e17ac: I guess you can now defer this until after commit()?

re: #24230 (review)

0cefaaa: I guess you can now defer this until after commit()?

I guess you're suggesting this code could commit first and update m_best_block_index later. I think this may be true currently, but I don't think it's necessarily safe to assume that other indexing code (particularly in CustomInit methods) doesn't care about the m_best_block_index value or won't depend on it in the future.

Anyway, the indexing commit code is pretty low quality--it's bad about creating multiple batches, and sharing state inappropriately, and not being atomic--but I'm changing it as little as possible because it's mostly orthogonal to this PR. I do think the PR should make it a little easier to improve committing in the future since at least now it's passing the locator explicitly and it changes the base Commit method to call the custom Commit method instead of the other way around, so the control flow is simpler.

d6ebc941f641d68eabe4c3243eb6769dd2d13cf5: this refactor of CustomRewind from iterating over blocks to CustomRemove which just takes a single block might be worth doing in a separate commit.

re: #24230 (review)

d6ebc94: this refactor of CustomRewind from iterating over blocks to CustomRemove which just takes a single block might be worth doing in a separate commit.

~Good suggestion, split this commit~

re: #24230 (comment)

d6ebc94: this refactor of CustomRewind from iterating over blocks to CustomRemove which just takes a single block might be worth doing in a separate commit.

I tried this but couldn't actually figure out how how to do it in a good way. I updated the commit description and I want the commit to reflect fact that block read and block undo read is moving from custom indexes up to the BaseIndex::Rewind. If I add the loop in base class without removing it from the custom indexes, it's less clear that code is just moving.

b8be01a7ef20672fd5faf4f6b82cf5d3fa2a717b : Why this extra condition?

re: #24230 (review)

b8be01a : Why this extra condition?

Good catch. This change makes more sense in the earlier commit adding this code, so I moved it. (It wasn't needed before this commit just because IgnoreBlockConnected was always returning true in this case before this commit.)

b8be01a7ef20672fd5faf4f6b82cf5d3fa2a717b This seems new...

re: #24230 (review)

b8be01a This seems new...

This is not actually new, it's moved from BlockFilterIndex::CustomAppend and CoinStatsIndex::CustomAppend below. The different UndoReadFromDisk calls are being consolidated here

b8be01a7ef20672fd5faf4f6b82cf5d3fa2a717b : this also seems new. Maybe belongs in the next commit?

re: #24230 (review)

b8be01a : this also seems new. Maybe belongs in the next commit?

This if (m_synced) condition just preserves existing behavior. Now that ThreadSync is calling blockConnected, m_synced is no longer always true at this point. For some reason I don't know (but don't want to change) ThreadSync code didn't previously update m_best_block_index while it was reading and connecting blocks, and I didn't want to change that. This behavior might have been intended for performance reasons, or error handling reasons, or maybe so RPC indexing status would only report best block of indexing data that had actually been flushed to disk. But in any case nothing should be changing here.

re: #24230 (review)

b8be01a : this also seems new. Maybe belongs in the next commit?

Maybe I should add a comment here like "Don't update m_best_block_index between flushes if not synced. Unclear why best block is not updated in this case, but this has been longstanding behavior since syncing was introduced in #13033 so care should be taken if changing this."

re: #24230 (review)

Maybe I should add a comment

Added now

My impression is that in the old code, the ThreadSync the strategy was to update m_best_block_index exactly before calling Commit() (and using pindex instead of m_best_block_index for everything else). So updating it after each WriteBlock call was just unnecessary.

re: #24230 (review)

My impression is that in the old code, the ThreadSync the strategy was to update m_best_block_index exactly before calling Commit() (and using pindex instead of m_best_block_index for everything else). So updating it after each WriteBlock call was just unnecessary.

I don't think this implies it's unnecessary unless you are assuming ChainStateFlushed (which calls Commit) is always called immediately after BlockConnected, which I think may not always be the case (i.e. during IBD). I'd actually be more worried about removing the m_best_block_index assignment here than I would be about assigning it unconditionally. I'd be happy to review another PR which changes this (and happy to rebase this PR on top) but in general I would like to not change m_best_block_index semantics in this PR unless it really helps things and we can be very sure it's safe.

I meant "updating it after each WriteBlock in the ThreadSync phase" is unnecessary. During the Sync phase, validation callbacks are ignored, and ThreadSync does the work - considering that the ThreadSync code in master makes sure that m_best_block_index is updated right before each possible Commit() call, and never used otherwise (pindex is used for that), I think that an earlier update of m_best_block_index after the WriteBlock call within ThreadSync is just not necessary. So I was just trying to explain why the longstanding behavior in master makes sense to me. I'm not suggesting to change anything here.

Ok. Current behavior in master (which should not be changed here) still does not make sense to me. I don't think it's wrong (though it could be wrong), I just mean it seems inconsistent and strange.

e359c2666cfd205c2ea84c241bccf565670365c0: I'm a bit puzzled about what chainStateFlushed is for in the context of indexing. Right now it doesn't do much other than call Commit(). But we weren't calling it here before.

The commit title says "... to chainStateFlushed" but in reality that function is untouched and only blockConnected is changed.

re: #24230 (review)

e359c26: I'm a bit puzzled about what chainStateFlushed is for in the context of indexing.

It just calls Commit() to update the best block locator on disk and flush whatever custom state the index may want to save in CustomCommit().

Right now it doesn't do much other than call Commit(). But we weren't calling it here before.

In commit e359c2666cfd205c2ea84c241bccf565670365c0, previous line 201 Commit(...); is changing to notifications->chainStateFlushed(...); so there should be preserving behavior

The commit title says "... to chainStateFlushed" but in reality that function is untouched and only blockConnected is changed.

I'm happy to change commit subject and I made some tweaks, but it would help to have more specific suggestions to know what to change. The format of these commits subject lines generally is "index: Remove [part of indexing code that doesn't belong in indexing]" or "index: Move [part of indexing code out of the code that will be moved to the node later]", so if I rename this subject line I might need to rename the other "index: Move" subject lines too.

It is often hard to describe changes commits are making in just a few words. The subject lines here are trying to describe the goal of each commit, and the commit bodies are trying to itemize the actual changes. A lot of the changes in different commits are pretty similar to each other, so the subject lines are also trying focus on what makes commits distinct from each other.

I guess this line could technically result in a double Commit() call at some point (based on blockConnected -> m_last_locator_write_time conditional), but even if that's true it would only happen once per locator-sync-interval AFAICT. So not a big deal.

In commit "indexes, refactor: Move Commit logic out of ThreadSync to notification handlers" (7add4807d84e89daec20042559b076f9941b9c1a)

re: #24230 (review)

I guess this line could technically result in a double Commit() call at some point (based on blockConnected -> m_last_locator_write_time conditional), but even if that's true it would only happen once per locator-sync-interval AFAICT. So not a big deal.

This shouldn't happen because the new notifications->blockConnected call is not passing any block data (block.data == nullptr, so the blockConnected implementation just updates m_synced and m_best_block_index and doesn't do anything else with the block).

I added more documentation about different types of blockConnected calls in interfaces/chain.h but the idea is that a blockConnected call with block.data != nullptr provides information about a new block that is being connected and needs to be processed, and a call with block.data == nullptr just tells you the last block that was already connected, which is useful at the beginning or end of a sync.

In commit 94fa723893a5acd2bdb69e859b76675f2d615b61: This still feels a bit convoluted. The way I read this, this now adds a call to IgnoreChainstateFlushed. Why not move the commit call to the new blockConnected code block as well?

re: #24230 (review)

In commit 94fa723: This still feels a bit convoluted. The way I read this, this now adds a call to IgnoreChainstateFlushed. Why not move the commit call to the new blockConnected code block as well?

The point of index blockConnected method is to add new data from blocks to the index, and the point of the chainStateFlushed method is to commit updated state to the database. In this one specific place, blockConnected and chainStateFlushed method are both called together, but normally they are called separately, so committing index data in blockConnected would be change in behavior and bad for performance.

It is true that adding a call to chainStateFlushed here goes through IgnoreChainstateFlushed logic instead of committing unconditionally, and this adds some temporary complexity. But the ignore method is dropped later, and ultimately indexing code becomes simpler because it no longer has to have special case code dealing with syncing. It can handle block connected and flush events the same whether it is in the sync phase or not.

commit dc4675ddaca8d0d1d40847191cb21ccd116a6a3c: Looks like undo_pos is never being used.

re: #24230 (review)

commit dc4675d: Looks like undo_pos is never being used.

Great catch! Removed

f631a1b16ac2f58f47942c7cf51fbe8445694f65: I think that this might be a slight change in behavior: If m_best_block_index is nullptr here (which is possible if we get an interrupt early in init), this will segfault now. Though the old behavior (that a locator to the tip was returned) was not ideal either, see #24117.

re: #24230 (review)

f631a1b: I think that this might be a slight change in behavior: If m_best_block_index is nullptr here (which is possible if we get an interrupt early in init), this will segfault now. Though the old behavior (that a locator to the tip was returned) was not ideal either, see #24117.

Absolutely. Rebasing on your fix from #24117, I believe this should be fixed here as well.

3450df4d01608bd29f443aab3df1eed45108b3b2: I think this incremental processing will mean that in case of a deeper reorg, intermediate blocks are processed twice in CopyHeightIndexToHashIndex now, as new_tip and current_tip. Also, there willl be some extra calls to ReadBlockFromDisk also for txindex now (which doesn't implement CustomRemove logic). But this seems fine to me.

re: #24230 (review)

3450df4: I think this incremental processing will mean that in case of a deeper reorg, intermediate blocks are processed twice in CopyHeightIndexToHashIndex now, as new_tip and current_tip.

Yes, exactly. I fixed commit message to describe this instead of saying "This commit does not change behavior in any way"

Also, there willl be some extra calls to ReadBlockFromDisk also for txindex now (which doesn't implement CustomRemove logic). But this seems fine to me.

Oh, this part was not intentional. Changed to remove these block reads. They are wasteful for BlockFilterIndex as well as TxIndex

3450df4d01608bd29f443aab3df1eed45108b3b2: The comment above could be adjusted, CustomRemove now rewinds just one block.

re: #24230 (review)

3450df4d01608bd29f443aab3df1eed45108b3b2: The comment above could be adjusted, CustomRemove now rewinds just one block.

Thanks, now noted in comment

36a9a550debc7324954765e60aefc33ab1456e5c: This should probably be conditional on !m_index.m_synced, otherwise there would be additional log messages after the index is synced (after most new blocks, since SYNC_LOG_INTERVAL=30s).

re: #24230 (review)

36a9a55: This should probably be conditional on !m_index.m_synced, otherwise there would be additional log messages after the index is synced (after most new blocks, since SYNC_LOG_INTERVAL=30s).

Yes, good catch. Added this

f56a851e11321ede9c5bcb43cd067c17f9efab39: Adding the condition if (m_synced) seems unnecessary at this point, IgnoreChainStateFlushed returns right at the beginning if not synced.

re: #24230 (review)

f56a851: Adding the condition if (m_synced) seems unnecessary at this point, IgnoreChainStateFlushed returns right at the beginning if not synced.

Oh, this is actually correct, but the check above should have been removed. The syncing here get simpler after the "Rewrite chain sync logic, remove racy init" commit, but right now chainStateFlushed notification events from the validation interface (not the sync thread) can happen at random times while the sync thread is still syncing, so the check avoids printing in this case while still not committing anything.

f56a851e11321ede9c5bcb43cd067c17f9efab39: why the additional !block.chain_tip condition? I'd assume if we happen to experience a reorg during initial sync and disconnect the tip, block.chain_tip=true and we don't return, but we wouldn't want to process this. Possibly related: If, at this commit, I am in the middle of syncing an index and at the same time call invalidateblock for the tip, my node coredumps with "leveldb/db/db_iter.cc:65: virtual leveldb::Slice leveldb::{anonymous}::DBIter::key() const: Assertion valid_' failed.`"

re: #24230 (review)

f56a851: why the additional !block.chain_tip condition?

Oh no. This is just a bug. It should say if (!m_index.m_synced && block.chain_tip). During sync notifications can come from two different sources;

1 - The sync thread (with chain_tip = false) 2 - The validation interface (with chain_tip = true)

We just want to ignore sync notifications from the validation interface like the comment above says.

I'd assume if we happen to experience a reorg during initial sync and disconnect the tip, block.chain_tip=true and we don't return, but we wouldn't want to process this.

Most of the time we won't want to process this but sometimes we might if the reorg happens right at the end of the sync. We just want to process this whenever being called from the sync thread, and not process it when not called from the sync thread during sync. After sync, we always want to process it.

Possibly related: If, at this commit, I am in the middle of syncing an index and at the same time call invalidateblock for the tip, my node coredumps with "leveldb/db/db_iter.cc:65: virtual leveldb::Slice leveldb::{anonymous}::DBIter::key() const: Assertion valid_' failed.`"

Interesting, it does sound related. I would be curious to know if is fixed now. And maybe I could make this into a test. I'm curious if you made changes to somehow pause the sync to emulate a reorg happening at the same time?

Interesting, it does sound related. I would be curious to know if is fixed now. And maybe I could make this into a test. I'm curious if you made changes to somehow pause the sync to emulate a reorg happening at the same time?

Yes, I don't encounter this anymore now. What I did was manual testing on regtest (just adding a std::this_thread::sleep_for to ThreadSync, giving me the possibility to time my invalidateblock) - I don't know if it is possible to somehow make an automated test for this that is not racy.

fb164ac88e8e5f7b7bb448637a06da6ae299e578 I think getTip() here and in interfaces.cpp could move to the next commit ("Remove SyncWithValidationInterfaceQueue call") where they are used.

re: #24230 (review)

fb164ac I think getTip() here and in interfaces.cpp could move to the next commit

Good catch. Moved this.

cec20142450117a40e64a77b8447e0f6294ec9e9: remove "A"

re: #24230 (review)

cec20142450117a40e64a77b8447e0f6294ec9e9: remove "A"

Thanks, fixed this.

c1bdcdf3388792b7c17e576170167f0be883fa4a: The commit message ("Move Commit and...") doesn't describe what's being done here, maybe something like "Move CustomInit out of Start and error handling code out of ThreadSync to notification handlers"?

re: #24230 (review)

c1bdcdf: The commit message ("Move Commit and...") doesn't describe what's being done here, maybe something like "Move CustomInit out of Start and error handling code out of ThreadSync to notification handlers"?

Thanks, fixed this now.

c1bdcdf3388792b7c17e576170167f0be883fa4a: why both an assert and a conditional? Or did you mean m_init_result.value() in the second line? Maybe also consider being explicit (has_value()) for the std::optional<bool> in the assert to avoid confusion.

re: #24230 (review)

c1bdcdf3388792b7c17e576170167f0be883fa4a: why both an assert and a conditional? Or did you mean m_init_result.value() in the second line? Maybe also consider being explicit (has_value()) for the std::optional<bool> in the assert to avoid confusion.

Good catch. I was intending to use * operator, but I followed your suggestion instead since that seems more readable.

Note: bug here was wasn't actually present in the full PR. Bug was temporarily introduced here in commit c1bdcdf3388792b7c17e576170167f0be883fa4a, but fixed by later commit fb164ac88e8e5f7b7bb448637a06da6ae299e578

cf5d3404484b08939992566cca0f1151d050a921

Do we ever expect that index will be null? Based on the calls to MakeBlockInfo, it doesn't look like it. Make sense to just accept a reference here?

Edit: okay, I see - in a later commit (when we add attachChain), this may be passed a nullptr if we're initializing the handler with an empty datadir.

re: #24230 (review)

Edit: okay, I see - in a later commit (when we add attachChain), this may be passed a nullptr if we're initializing the handler with an empty datadir.

Right, in cases where CChain::Tip() is null this just returns a struct with all default values. Alternative would be to return optional<BlockInfo> and pass around optional<BlockInfo> variables more places. But this would force code to handle nullopt as a special case when in practice the struct values are ignored anyway and the defaults are fine. It's similar to using INT_MAX if you mean to represent infinity. It doesn't capture the meaning perfectly, but the semantics are close enough that it can make sense if it gets rid of special cases and makes code simpler.

15daa47ee06b9f334dd2862856f41a2ad4b34a3d

Sad to see a Russ PR that's introducing a new method with PascalCase, but I guess we're matching existing style... :stuck_out_tongue:

re: #24230 (review)

15daa47

Sad to see a Russ PR that's introducing a new method with PascalCase, but I guess we're matching existing style... stuck_out_tongue

Yeah I like it for new classes mostly. I'm happy you are using this convention in validation code since validation code has lots of standalone functions, and knowing whether you are calling method or a function can really tell you something. In index code there aren't too many standalone functions.

0e80167cc848364a456bbfd4cba12891a5f06b35

Maybe worth doing Assert(block.data)->vtx.size() here? Possibly also with .undo_data below.

re: #24230 (review)

0e80167

Maybe worth doing Assert(block.data)->vtx.size() here? Possibly also with .undo_data below.

Makes sense, added asserts

afce60462f6d4ebc7b470ac2860c5d905e406ec7

Might consider Assert(...) here.

re: #24230 (review)

afce604

Might consider Assert(...) here.

Added asserts above

bf6a0f8ae87a3ea4aec2c5a080f86fc44e0bd19e

Hmm... note to other reviewers, I guess: it wasn't obvious to me that this is equivalent to the original code because it seems like if m_synced is somehow set between the conditional below and the next usage of current_time (line 117 at time of writing), then this code will behave differently than the original code (i.e. current_time == 0 but is used in the second conditional). But I read through and verified that an m_synced set doesn't happen in between.

Would be nice to be able to verify that current_time != 0 in the second usage (line 117), but not mandatory IMO.

re: #24230 (review)

Would be nice to be able to verify that current_time != 0 in the second usage (line 117), but not mandatory IMO.

Thanks. I added a direct assert that m_synced value didn't change. This is an even stricter check, and ensures that the Commit() call still happens and doesn't get skipped.

c4e0a9dd457018d78dc6be29251aec9d649af56c

Edit: this criticism is invalidated by later commit 8c0c231840b6a88e470da69c6885e6b6dd088df5

Now that we've removed the doc here, it'd be nice to have some kind of indication in the attachChain() doc (or even inline in the body of the function) that that function is now responsible for starting and setting m_thread_sync; as it stands in this commit there isn't any external indication of that.

re: #24230 (review)

c4e0a9d

Edit: this criticism is invalidated by later commit 8c0c231

Now that we've removed the doc here, it'd be nice to have some kind of indication in the attachChain() doc (or even inline in the body of the function) that that function is now responsible for starting and setting m_thread_sync; as it stands in this commit there isn't any external indication of that.

To improve this, I added some comments and an assert to this commit.

c4e0a9dd457018d78dc6be29251aec9d649af56c

"sync thread" in the context of Chain is a little ambiguous IMO; maybe say "index sync thread"?

re: #24230 (review)

c4e0a9d

"sync thread" in the context of Chain is a little ambiguous IMO; maybe say "index sync thread"?

Changed comment to "Name to use for attachChain sync thread". I would like to use the new attachChain method for wallets as well as indexes (#15719) so this is avoiding the word "index".

d39c6d00627ddf746e1c37a4d6e07f0a24586034

The naming here given the use is somewhat misleading to me; naively, one possible meaning of "waitForNotifications" is that we're just waiting for a single notification to come in. What this really does is wait for the validation interface queue to clear, i.e. for any pending indexation to complete. I think something like waitForEmptyIndexQueue() or similar might be less confusing.

re: #24230 (review)

d39c6d0

The naming here given the use is somewhat misleading to me; naively, one possible meaning of "waitForNotifications" is that we're just waiting for a single notification to come in. What this really does is wait for the validation interface queue to clear, i.e. for any pending indexation to complete. I think something like waitForEmptyIndexQueue() or similar might be less confusing.

Changed from waitForNotifications to waitForPendingNotifications now. This new name is less consistent with the waitForNotificationsIfTipChanged method name immediately below, but it is less ambiguous.

I think waitForEmptyIndexQueue() would be misleading because the queue doesn't need to be empty for this to return. It just waits for currently pending notifications to be processed, not new notifications. It just marks a point in the stream, so the queue may never need to be empty at any point.

8c0c231840b6a88e470da69c6885e6b6dd088df5

I'm confused about what this is doing - why is connect true here but false in the call above?

re: #24230 (review)

8c0c231

I'm confused about what this is doing - why is connect true here but false in the call above?

Sorry, this logic got messy because the notifications proxy now gets registered and unregistered from different threads, and could disconnected from the handler before the register call could happen, and I was trying to deal with cirrus TSAN errors. I rewrote it so it should understandable now.

To answer the question, connect was true for the older handleNotifications method because it doesn't care about stale notifications and just wants to connect right away, but false for attachChain because it delays registration to avoid sending stale notifications.

c7729d1f0ccb17c39c42ee5c7881d469d150c101

Just so I'm sure I understand, this has the effect of blocking the init thread until the first blockConnected call is completed?

re: #24230 (review)

c7729d1

Just so I'm sure I understand, this has the effect of blocking the init thread until the first blockConnected call is completed?

Yes, added comment. The wallet or index calling attachChain needs be able to find out the sync starting point or last block synced. An alternate approach would be for attachChain to return the starting block. This is approach I took earlier in #15719 https://github.com/bitcoin/bitcoin/blob/2b898a0456d742911c1f74229c9fc64a0012ca42/src/interfaces/chain.h#L260-L261, but it seems like would made things more complicated for callers by adding an output argument, and requiring the caller to update m_synced and m_best_block from multiple places instead of only inside blockConnected.

commit 1819bdb7cb0634dad2e07daccfe7e8a122a943d6: I think that this changed behavior slightly because it changes the order of indexing the block (CustomAppend) and periodically updating the index' best block (Commit) - which is better because the indexed data on disk wouldn't be in an inconsistent state temporarily. I opened #25074 to fix this on master.

re: #24230 (review)

commit 1819bdb: I think that this changed behavior slightly because it changes the order of indexing the block (CustomAppend)

Thanks. Good catch and I reviewed your PR

One general point (if it's too unrelated to this PR, we could also discuss it elsewhere):

I think that the current way of initialising indexes by calling FindForkInGlobalIndex() for the saved locator and setting this as the best block is not compatible with indexes that have state beyond the best block, such as coinstatsindex: For this index, we need to call CustomRemove() (or Rewind on master) to revert the actions of each block and adjust the muhash. This is not done during Init, so the coinstatsindex currently cannot deal with the situation where the saved best block of the index is not part of the best chain.

One solution for this would be not to call FindForkInGlobalIndex but instead read in the first block stored in the locator and then revert blocks step by step up to the fork point - this is necessary for coinstatsindex, but it adds the additional requirement that all blocks between the forkpoint and the saved block must be available, which is something that e.g. txindex doesn't really need.

I did a rough implementation for fixing the coinstatsindex init sequence in this branch, which I think works but (at least on first glance) seems to be really incompatible with the approach here, so I wouldn't just PR it.

What do you think about this problem?

I think that the current way of initialising indexes by calling FindForkInGlobalIndex() for the saved locator and setting this as the best block

You're right but this is easy to fix by just replacing FindForkInGlobalIndex() call with a LookupBlockIndex call. There should be no need to add additional remove/rewind calls, though. Existing code should be aware of forks and how to handle them.

Great, thanks! I missed that NextSyncBlock() goes back to the fork point already in master during sync. If it's that easy to fix, I might open a PR vs master. Here, this can be resolved.

re: #24230 (review)

Great, thanks! I missed that NextSyncBlock() goes back to the fork point already in master during sync. If it's that easy to fix, I might open a PR vs master. Here, this can be resolved.

Yes, this is a good catch and test on your branch looks good, but the fix could be simpler and more generic.

Just to close the thread, the issue described here is pre-existing, and basically orthogonal to this PR even if it conflicts a little. The issue just happens if there is a reorg away from the last block that was indexed while the index is shut down.

bc39508eb83b33e620a81ca89ed3bade90344840 ("Remove index prune_violation code"): I'm not sure if it's helpful to talk about a locator (which is more of a list of blocks) and call it locator_block here, since checkBlocks just accepts a single BlockIndex - it may currently be derived from a locator, but that info isn't really required here - if we stopped using locators and just saved single block hashes, checkBlocks wouldn't be affected at all.

re: #24230 (review)

Renamed from locator_block to start_block

1091e79d7bb57da3b96814fe26101e0bf75e1108 (Move sync thread from index to node)

I think it would be good to abort early and not start the sync process if CustomInit of the initial blockConnected call failed (because the index might be corrupted). This is the behavior on master, and otherwise the syncing logic could commit some work like syncing another block, and while we'd still terminate with an InitError because m_init_result=false after that, this error might no longer occur at the next startup - the index might now be silently corrupted.

re: #24230 (review)

I think it would be good to abort early and not start the sync process if CustomInit of the initial blockConnected call failed (because the index might be corrupted).

Good point. I think I'm missing a if (!*m_init_result) return m_index.Interrupt(); line after initializing m_init_result in blockConnected()

re: #24230 (review)

Added m_index.Interrupt() call on failure

https://github.com/bitcoin/bitcoin/pull/24230/commits/a035b6500f427cb535058c6718ff41edd961fdc3

checkBlocks is pretty vague; maybe hasDataFromTipDown()?

re: #24230 (review)

checkBlocks is pretty vague; maybe hasDataFromTipDown()?

Thanks, renamed

https://github.com/bitcoin/bitcoin/pull/24230/commits/de311f9f9ecb64dc83c070e3824c086b918cb09a

I don't understand what this change does - it isn't obvious to me from the surrounding code that pindex isn't the tip.

re: #24230 (review)

I don't understand what this change does - it isn't obvious to me from the surrounding code that pindex isn't the tip.

Thanks, removed this. Nothing is actually looking at this variable so it doesn't do anything. This code is just reading old blocks which are mostly not from the tip of the chain, so this was setting the variable to avoid seeming like they were.

https://github.com/bitcoin/bitcoin/pull/24230/commits/43e03ec21fc2da2bd76912e3aa6981899030c8c2

Is this comment related to the changes in this commit? It seems like this corresponds to the previous commit.

re: #24230 (review)

Is this comment related to the changes in this commit? It seems like this corresponds to the previous commit.

Thanks, just dropped this. I was looking for someplace to move this comment so I moved it here, but it doesn't really work here and is better to prune

https://github.com/bitcoin/bitcoin/pull/24230/commits/43e03ec21fc2da2bd76912e3aa6981899030c8c2

FWIW, inverting the conditional here makes it harder to verify the diff because now this block of logic isn't move-only.

re: #24230 (review)

FWIW, inverting the conditional here makes it harder to verify the diff because now this block of logic isn't move-only.

Good catch, now avoided this

https://github.com/bitcoin/bitcoin/pull/24230/commits/00b2300f919060a89bca74f0a908602a6a716f6d

This commit fails to compile because MakeBlockInfo() is in the kernel namespace. I guess this code is moved later on, which resolves the compilation failure.

re: #24230 (review)

This commit fails to compile because MakeBlockInfo() is in the kernel namespace. I guess this code is moved later on, which resolves the compilation failure.

Thanks! Updated to fix

https://github.com/bitcoin/bitcoin/pull/24230/commits/5605e4e1310d244892447755c1194195c2e866da#

Just making sure this should actually be a copy and not a reference.

re: #24230 (review)

Just making sure this should actually be a copy and not a reference.

Yes a reference to the shared_ptr wouldn't work here because the shared_ptr could be reset at any time from another thread. A copy of the pointer is actually needed

https://github.com/bitcoin/bitcoin/pull/24230/commits/ac1369650de26f325076340017745c67bdb34900

Is this new behavior or inherited?

re: #24230 (review)

Is this new behavior or inherited?

It's all inherited behavior. The indexes ignored "blockDisconnected" notifications entirely before, so the "During initial sync, ignore validation interface notifications" part of this comment happened implicitly. The "process notifications from sync thread" part of this comment is referring to new blockDisconnected code below, but that code is just moving from the ThreadSync to here (so ThreadSync can move out of the src/index/ to src/node/ later)

https://github.com/bitcoin/bitcoin/pull/24230/commits/2f71f52b9b37b94a03aa203d16eecdca467361ac

Not a big deal, but curious why this was removed since it was introduced in the previous commit.

re: #24230 (review)

2f71f52

Not a big deal, but curious why this was removed since it was introduced in the previous commit.

It's not really removed, just made a internal method instead of a public one so removed from the public interface now that it no longer needs to be called by outside code.

https://github.com/bitcoin/bitcoin/pull/24230/commits/2f71f52b9b37b94a03aa203d16eecdca467361ac

Wouldn't we want to do this check/return before we issue a blockConnected() event, or does it not matter because we know that we at least have data for the current tip?

re: #24230 (review)

2f71f52

Wouldn't we want to do this check/return before we issue a blockConnected() event, or does it not matter because we know that we at least have data for the current tip?

The code is a little different now, and the blockConnected call is replaced by a std::function callback but I think the idea in either case is that it might be useful for the index to know what the chain tip is even if data has been pruned and it might not be possible to sync.

https://github.com/bitcoin/bitcoin/pull/24230/commits/2f71f52b9b37b94a03aa203d16eecdca467361ac

The contents of this function are pretty specific to indexes (e.g. we have logic in here that refuses to set up the notifications handler if there isn't data beneath the given tip) and I think the name attachChain a little bit generic, so might consider renaming this attachIndexer() or something more descriptive.

re: #24230 (review)

2f71f52

The contents of this function are pretty specific to indexes (e.g. we have logic in here that refuses to set up the notifications handler if there isn't data beneath the given tip) and I think the name attachChain a little bit generic, so might consider renaming this attachIndexer() or something more descriptive.

This is intentional because the attachChain method is meant to be usable by the wallet too, and it was actually written originally for the wallet in #15719. I think the behaviors you mentioned which are specific to indexes are things which could be controlled by options or just removed later. But idea is for indexes and wallets to just be passive clients that receive notifications, and do not have to implement their own sync logic.

https://github.com/bitcoin/bitcoin/pull/24230/commits/a6d1c22dc1254cbfedda7add573f0d537aefd36c

Maybe I'm just dense today, but I don't recognize this cpp pattern. Is this some kind of initialization shorthand that's executed once on construction, when we get a valid reference for m_index?

re: #24230 (review)

a6d1c22

Maybe I'm just dense today, but I don't recognize this cpp pattern. Is this some kind of initialization shorthand that's executed once on construction, when we get a valid reference for m_index?

It's more simple than that, it's just declaring NotifyOptions m_options member, where NotifyOptions is an options struct with bool and string fields. The struct is initialized by calling the CustomOptions method whenever the class is constructed.

https://github.com/bitcoin/bitcoin/pull/24230/commits/ecedecac38f231d855e3f663671301d6e1ea7f6b

It would be nice to document what this mutex is for and why it's needed. I gather from below that it's guarding m_notifications and m_handler, but it's not clear to me why that's necessary - my guess is that because the guarded state is accessed by both the sync thread and validationinterface callbacks via Chain.

re: #24230 (review)

ecedeca

It would be nice to document what this mutex is for and why it's needed. I gather from below that it's guarding m_notifications and m_handler, but it's not clear to me why that's necessary - my guess is that because the guarded state is accessed by both the sync thread and validationinterface callbacks via Chain.

This is basically right, it's to allow those variables to be accessed from both the sync thread and the init thread (not the validationinterface thread). Added a comment.

https://github.com/bitcoin/bitcoin/pull/24230/commits/a6d1c22dc1254cbfedda7add573f0d537aefd36c

To add some color here, reviewers can verify that in the pre-PR code

• BlockConnected events are ignored while not m_synced, and
• ThreadSync() calls Commit() which confusingly updates the best block in the index db (GetDB().WriteBestBlock(...)) but doesn't change m_best_block_index.

re: #24230 (review)

a6d1c22

To add some color here, reviewers can verify that in the pre-PR code

• BlockConnected events are ignored while not m_synced, and

• ThreadSync() calls Commit() which confusingly updates the best block in the index db (GetDB().WriteBestBlock(...)) but doesn't change m_best_block_index.

This is right. If it helps I added a separate commit (f552c32de4359cd7dce38ce8c9a340039d5a29c6) to try to document m_best_block_index behavior

https://github.com/bitcoin/bitcoin/pull/24230/commits/15f83492713abefea3f6ffc7e4c100a6177cc462

Would it be worth FatalError()/return from here? I think that is needed to maintain strict behavioral compatibility with the way that ThreadSync() currently behaves, since !Rewind() can be tripped for either a failure to retrieve block data OR failure to retrieve undo data.

re: #24230 (review)

15f8349

Would it be worth FatalError()/return from here? I think that is needed to maintain strict behavioral compatibility with the way that ThreadSync() currently behaves, since !Rewind() can be tripped for either a failure to retrieve block data OR failure to retrieve undo data.

The error still happens on line 99 ("%s: Failed to rewind index %s to a previous chain tip") because rewind_ok will be false. The idea is to keep behavior unchanged, and trigger the error when there is actually an attempt to connect another block.

Maybe though the complexity of having m_rewind_error and m_rewind_start variables is not worth it, and there should be an immediate fatal error.

For now just added a comment about m_rewind_error here

https://github.com/bitcoin/bitcoin/pull/24230/commits/ed79c47c1385dc2e1b25d3958fa83a231770353d

Should the equivalent old code in ThreadSync() be removed here in this commit as well?

re: #24230 (review)

ed79c47

Should the equivalent old code in ThreadSync() be removed here in this commit as well?

Good catch, added that change to this commit!

https://github.com/bitcoin/bitcoin/pull/24230/commits/ed79c47c1385dc2e1b25d3958fa83a231770353d

May be worth a comment that blockDisconnected will interrupt below in this case, triggering the related return?

re: #24230 (review)

ed79c47

May be worth a comment that blockDisconnected will interrupt below in this case, triggering the related return?

This change is implemented a little differently now. The ReadBlockFrom disk call is moved into blockDisconnected so hopefully more straightforward.

https://github.com/bitcoin/bitcoin/pull/24230/commits/ed79c47c1385dc2e1b25d3958fa83a231770353d#diff-8e9fef4d0718d085f31da382899d97e3bbefc8d6a72ede95916a0e2fbd1a532fR92

It might help to add some documentation for how this is used and under what conditions it is set.

re: #24230 (review)

ed79c47#diff-8e9fef4d0718d085f31da382899d97e3bbefc8d6a72ede95916a0e2fbd1a532fR92

It might help to add some documentation for how this is used and under what conditions it is set.

Good idea, added.

index/coinstatsindex.cpp:143:37: error: use of undeclared identifier 'pindex'

re: #24230 (review)

index/coinstatsindex.cpp:143:37: error: use of undeclared identifier 'pindex'

This was probably a rebase conflict. I think this error must have been fixed, or at least I can't reproduce it the commit this line was added (5e2b1ea14823bcc43429a130d8820f7be0f45b4b)

nit in d7eef9f49a827e96337febcf20a22b392dc7b51e: Can use chaiman()?

re: #24230 (review)

nit in d7eef9f: Can use chaiman()?

Thanks, this was done in the meantime, switched to chainman() throughout

In c0d1110d:

Guess that you wrote this because you saw it in a unit test?

I'm not seeing how this could be possible during the regular node initialization process. We aren't signaling any block prior to initializing the indexes.

re: #24230 (review)

Guess that you wrote this because you saw it in a unit test?

No, it I believe it happens pretty reliably when you use -reindex-chainstate. I saw it reliably when I ran the test/functional/feature_coinstatsindex.py "Test that the index works with -reindex-chainstate" test and had a bug in the code that disabled this check. It caused the index to be corrupted because the same blocks would be added to the index twice.

Ok, I think that know how that could happen. It is not related to the Init() call.

The background sync process reads blocks directly from the chain index (calling NextSyncBlock()). So, what if a block gets connected while ThreadSync() is being executed but the event is not dispatched on time.. (before the background sync process finishes)

1. The block connection event is added to the validation queue.
2. The index background sync process reads the block from the chain and processes it.
3. The validation queue dispatches the block connection event added in (1) to the index. Then... boom: the index received an already processed block.

That's accurate, but I don't think ThreadSync needs to even do anything for this to happen, and no new blocks need to be connected after ThreadSync starts.

If -reindex-chainstate indexing completes, and ThreadSync immediately starts and exits because it detects that the index best block is equal to the chain tip, and no new blocks are connected, there could still be stale block connected notifications in the queue which will cause the index to rewind and add the blocks again. The commit "Avoid race, make -reindex-chainstate more efficient" e5f2641137413da5970007a9b932b5993067b550 changes index startup behavior to just ignore the stale block connected notifications while m_ready is false, instead of processing them and pointlessly rewinding and reconnecting blocks.

Yeah ok, that is also a possibility. Great.

Could the fix be decoupled into a standalone PR? Sounds like something that we could rapidly merge independently of this PR. It fixes a bug and also makes this work smaller.

Yeah ok, that is also a possibility. Great.

Could the fix be decoupled into a standalone PR? Sounds like something that we could rapidly merge independently of this PR. It fixes a bug and also makes this work smaller.

Yes, I am definitely looking for suggestions on how many of the first commits it would make sense to split off into a separate PR.

But just to be clear, there isn't a bug here, just suboptimal behavior. If -reindex-chainstate is used and there are still block-connected notifications waiting in the queue when the index is started, it doesn't cause any index corruption. It just causes the last few blocks in the index to be rewound, and then appended again, as if there were a reorg

Yes, I am definitely looking for suggestions on how many of the first commits it would make sense to split off into a separate PR.

Ok cool. I suggested to decouple this commit only because I'm going commit by commit and it seemed like an easy step forward, but will keep moving forward.

But just to be clear, there isn't a bug here, just suboptimal behavior. If -reindex-chainstate is used and there are still block-connected notifications waiting in the queue when the index is started, it doesn't cause any index corruption. It just causes the last few blocks in the index to be rewound, and then appended again, as if there were a reorg

Yeah, we are sync. I said "bug" because of the unexpected behavior. I don't think that it was programmed to do that.

In ba30b60b:

Isn't this conflicting with the StartBackgroundSync() call?

Let's say that there are new blocks on the notifications queue at this point. By not setting the m_synced flag, we allow ThreadSync() to start. This former one might have some work to do if some of the queued blocks notifications were processed in-between Init() and StartBackgroundSync(). Then, while this is being done, the validation queue thread might end up setting m_synced=true. Which would enable the index new block connection signals reception at the same time that ThreadSync() is being executed.

re: #24230 (review)

Isn't this conflicting with the StartBackgroundSync() call?

Let's say that there are new blocks on the notifications queue at this point. By not setting the m_synced flag, we allow ThreadSync() to start.

Yes, that's a good point. I forgot that the m_synced flag is not just used to process notifications, but also to determine whether to start the sync thread.

It's hard to imagine this being a problem in pracice, but I think I should tweak the logic to distinguish between "synced" and "ready" states, where "synced" state means index is in sync with the chainstate and "ready" state means index is ready to start processing blockconnected notifications.

EDIT: Later commit "Move sync thread from index to node" 0048fd1cbc00205c191dae1a96c44d02f4a7aae0 actually already does this so there is not a race condition in the end, but I it would be good to do it earlier.

In b96598d9:

Same as you did for BlockConnected(), what about doing the same cleanup on this method?. Check few lines below, we have the same:

// This checks that ChainStateFlushed callbacks are received after BlockConnected. The check may fail
// immediately after the sync thread catches up and sets m_synced. Consider the case where
// there is a reorg and the blocks on the stale branch are in the ValidationInterface queue
// backlog even after the sync thread has caught up to the new chain tip. In this unlikely
// event, log a warning and let the queue clear

which is no longer possible, thanks to m_ready set in the validation queue thread.

re: #24230 (review)

In b96598d:

Same as you did for BlockConnected(), what about doing the same cleanup on this method?

Great idea, done.

In b96598d9:

Small note, the comment that is above the RegisterValidationInterface() call is not so accurate anymore.

re: #24230 (review)

In b96598d:

Small note, the comment that is above the RegisterValidationInterface() call is not so accurate anymore.

Rewrote the comment. (Comment was also incomplete before, because even before this it was important to register for notifications before releasing cs_main.)