assumeutxo: indexing changes #24006

jamesob · 2022-01-07T16:56:57Z

This is part of the assumeutxo project (parent PR: #15606)

This PR includes the necessary changes for indexing to work with the operation of a background validation chainstate. In short, it

only emits existing ValidationInterface events for the active chain,
introduces a BackgroundBlockConnected event for the background chain to support indexation, and
removes assumptions in the indexing framework that indexation will happen sequentially.

More details below.

Adds BackgroundBlockConnected to the validationinterface and uses it in
index maintenance. Ensures that index building will work when part of
the chain is validated asynchronously in the background by a second
chainstate.

This changeset removes the guarantee that indexes will be built
sequentially, as none of the current indexes require this and it is no
longer easy to offer this guarantee when multiple chainstates are in
use.

Within BaseIndex, we only update m_best_block_index (which is
essentially the progress marker for how far along the indexing process
is) for chains which we can be certain that all blocks under a given
block on that chain have had indexing performed on them. In other
words, we will not update m_best_block_index during a BlockConnected
event that comes from an active snapshot chain, i.e. a chain for which
some of the blocks underneath Tip() are not indexed.

Once background validation is completed and no background chainstates
are in use, the indexer will happily use BlockConnected events from the
active chain to update m_best_block_index as usual. No blocks from the
active chainstate will have been missed for indexing (despite not
previously updating m_best_block_index), so it is safe to perform this
transition without any extra behavior.

Some unnecessary logic from BaseIndex::ChainStateFlushed() is removed,
since the locator can no longer be assumed to be an ancestor of the
best_block_index (since the "best block" may be on a background chainstate
and the locator may have come from an assumed-valid tip well ahead of
it).

For use in later commits.

To be used in subsequent commits.

Adds BackgroundBlockConnected to the validationinterface and uses it in index maintenance. Ensures that index building will work when part of the chain is validated asynchronously in the background by a second chainstate. This changeset removes the guarantee that indexes will be built sequentially, as none of the current indexes require this and it is no longer easy to offer this guarantee when multiple chainstates are in use. Within BaseIndex, we only update `m_best_block_index` (which is essentially the progress marker for how far along the indexing process is) for chains which we can be certain that all blocks under a given block on that chain have had indexing performed on them. In other words, we will not update `m_best_block_index` during a BlockConnected event that comes from an active snapshot chain, i.e. a chain for which some of the blocks underneath Tip() are not indexed. Once background validation is completed and no background chainstates are in use, the indexer will happily use BlockConnected events from the active chain to update `m_best_block_index` as usual. No blocks from the active chainstate will have been missed for indexing (despite not previously updating m_best_block_index), so it is safe to perform this transition without any extra behavior. Some unnecessary logic from BaseIndex::ChainStateFlushed() is removed, since the locator can no longer be assumed to be an ancestor of the best_block_index (since the "best block" may be on a background chainstate and the locator may have come from an assumed-valid tip well ahead of it).

DrahtBot · 2022-01-07T20:29:42Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24008 (assumeutxo: net_processing changes by jamesob)
#22485 (index: doc/log BaseIndex sync behavior with empty datadir by jamesob)
#19888 (rpc, test: Improve getblockstats for unspendables by fjahr)
#15606 (assumeutxo by jamesob)

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.

Sjors

This changeset removes the guarantee that indexes will be built
sequentially, as none of the current indexes require this and it is no
longer easy to offer this guarantee when multiple chainstates are in
use.

I'm confused. IIUC when a new index starts building while IBD is in progress it will:

While only synching from assume valid to the tip: do nothing
While syncing the background chain state: index, from genesis up
When background chain state is finish: index assumed valid blocks

If so, then it's still building sequentially. Or is it indexing assumed valid blocks already in (1) and (2)?

Also, doesn't MuHash in coinstatsindex require the index to be in sequence, because the hash for each block is an increment of that of the previous block? Or do we store the increment and infer the value for each block on the fly? cc @fjahr

Regarding ValidationInterface callbacks for active chain, do you have any hint for how to reason about what the consumers of these callbacks expect?

Sjors · 2022-01-10T18:16:56Z

src/validation.h

@@ -822,6 +822,10 @@ class ChainstateManager
        CBlockIndex** ppindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
    friend CChainState;

+    // Returns nullptr if no snapshot ahs been loaded.


nit: ahs -> has

Sjors · 2022-01-10T18:22:07Z

src/validation.h

+    //!   In other words, give us the chainstate for which we can reasonably expect
+    //!   that all blocks beneath the tip have been indexed. In practice this means
+    //!   when using an assumed-valid chainstate based upon a snapshot, return only the
+    //!   fully validated chain.


This is a bit cryptically worded, because "fully validated chain" could be read as "the background chain, but only once it's fully validated".

fjahr · 2022-01-10T20:57:09Z

Also, doesn't MuHash in coinstatsindex require the index to be in sequence, because the hash for each block is an increment of that of the previous block? Or do we store the increment and infer the value for each block on the fly?

It works like this in the index but since the MuHash is just a representation of a UTXO set and does not commit to a previous state you can "jump in" at any point in the chain and calculate the MuHash at any height by looking only at the UTXO set (inefficient because the full set has to be processed) and then from this point on work with the deltas (=blocks) as usual (efficient). Because of this it is also possible to calculate the MuHash at the tip without the index, it just takes a while. I haven't looked at the code in detail yet but just skimming I think the code for this starting of the coinstatsindex at any height is not included yet, right? But since assumeutxo supplies the UTXO set and its corresponding height this should be possible to build this in a follow-up.

But I agree with Sjors' question on the conceptual level: do we really need the indices to sync already right away on the blocks between the au height and tip or is it not maybe simpler to have the indices only sync with the background chain? Syncing the blocks between au and tip first we could serve blockfilters at the tip right away, which is nice, but if there is ever an issue with an incorrect snapshot then we make it other people's problem too because we are serving incorrect blockfilters.

jamesob · 2022-01-10T21:05:39Z

I'm confused.

The way this works as-written is that if the user starts with an empty datadir and then proceeds to load a snapshot, indexes will be built on the basis of incoming (Background)BlockConnected events that will come from both chainstates (due to the behavior documented in the unmerged #22485) - hence the lack of ordering.

But now that we're discussing it, maybe it's preferable to just defer index building until we're done with the assumed-valid chain...

Also, doesn't MuHash in coinstatsindex require the index to be in sequence, because the hash for each block is an increment of that of the previous block? Or do we store the increment and infer the value for each block on the fly? cc @fjahr

Good catch! Thought I'd checked that but apparently not: https://github.com/jamesob/bitcoin/blob/e84248814296b82d926f5018dd62d917ddefe626/src/index/coinstatsindex.cpp#L118-L121

A few options that come to mind:

we can defer indexation until there is are no assumed-valid chainstates in use (as @fjahr suggested as I was typing this), or
we can introduce an m_requires_sequential member variable to BaseIndex that does the above, but only for indexes that require it. This is kind of an optimized version of the first option.

Sjors · 2022-01-11T09:33:51Z

I think we can introduce m_requires_sequential in the future if we really want to build an index earlier. I can't see an obvious use case for that atm.

jamesob · 2022-01-12T16:24:53Z

Closing this while I work on a simpler implementation that preserves sequential indexing at the cost of deferring indexing on the assumed-valid chainstate.

jamesob and others added 4 commits January 7, 2022 10:48

validation: only send ValidationInterface callbacks for active chain

9ce8364

add ChainstateManager.getSnapshot{Height,BaseBlock}()

e50aa36

For use in later commits.

validation: index: add methods for snapshot-aware indexing

0975c1f

To be used in subsequent commits.

DrahtBot added UTXO Db and Indexes Validation labels Jan 7, 2022

jamesob mentioned this pull request Jan 7, 2022

assumeutxo: net_processing changes #24008

Closed

jamesob mentioned this pull request Jan 7, 2022

assumeutxo #15606

Closed

18 tasks

This was referenced Jan 8, 2022

refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager #22564

Merged

rpc, test: Improve getblockstats for unspendables #19888

Merged

Sjors reviewed Jan 10, 2022

View reviewed changes

DrahtBot mentioned this pull request Jan 11, 2022

doc: BaseIndex sync behavior with empty datadir #22485

Merged

jamesob closed this Jan 12, 2022

bitcoin locked and limited conversation to collaborators Jan 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

assumeutxo: indexing changes #24006

assumeutxo: indexing changes #24006

Uh oh!

jamesob commented Jan 7, 2022

Uh oh!

DrahtBot commented Jan 7, 2022 •

edited

Loading

Uh oh!

Sjors left a comment •

edited

Loading

Uh oh!

Sjors Jan 10, 2022

Uh oh!

Sjors Jan 10, 2022

Uh oh!

fjahr commented Jan 10, 2022 •

edited

Loading

Uh oh!

jamesob commented Jan 10, 2022

Uh oh!

Sjors commented Jan 11, 2022

Uh oh!

jamesob commented Jan 12, 2022

Uh oh!

Uh oh!

assumeutxo: indexing changes #24006

assumeutxo: indexing changes #24006

Uh oh!

Conversation

jamesob commented Jan 7, 2022

Uh oh!

DrahtBot commented Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Sjors left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Jan 10, 2022

Choose a reason for hiding this comment

Uh oh!

Sjors Jan 10, 2022

Choose a reason for hiding this comment

Uh oh!

fjahr commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesob commented Jan 10, 2022

Uh oh!

Sjors commented Jan 11, 2022

Uh oh!

jamesob commented Jan 12, 2022

Uh oh!

Uh oh!

DrahtBot commented Jan 7, 2022 •

edited

Loading

Sjors left a comment •

edited

Loading

fjahr commented Jan 10, 2022 •

edited

Loading