indexes: Read the locator's top block during init, allow interaction with reindex-chainstate #25193

mzumsande · 2022-05-23T21:27:22Z

This makes two improvements to the index init phase:

1) Prevent index corruption in case a reorg happens when the index was switched off:
This is done by reading in the top block stored in the locator instead of looking for a fork point already in BaseIndex::Init().
Before, we'd just go back to the fork point by calling FindForkInGlobalIndex(), which would have corrupted the coinstatsindex because its saved muhash needs to be reverted step by step by un-applying all blocks in between, which wasn't done before. This is now being done a bit later in ThreadSync(), which has existing logic to call the custom Rewind() method when going back along the chain to the forking point (thanks ryanofsky for pointing this out to me!).

2) Allow using the -reindex-chainstate option without needing to disabling indexes:
With BaseIndex::Init() not calling FindForkInGlobalIndex() anymore, we can allow reindex-chainstate with active indexes. reindex-chainstate deletes the chain and rebuilds it later in ThreadImport, so there is no chain available during BaseIndex::Init(), which would lead to problems (see #24789).
But now we'll only need the chain a bit later in BaseIndex::ThreadSync, which will wait for the reindex-chainstate in ThreadImport to finish and will continue syncing after that.

mzumsande · 2022-05-23T21:28:26Z

One thing I'm unsure about is that part 1) of this PR will now make it impossible to go back if we don't know the top block of the locator for some reason - although I don't know how this would be possible because I don't know of a process that would prune stale blocks from the Block Index (except the contrib/linearize script).

The old code could recover an index with a no-longer existing best block from that (at the cost of corrupting the coinstatsindex), but I wonder if it would be good to still call FindForkInGlobalIndex(), maybe as a fallback, for the other indexes.

DrahtBot · 2022-05-23T21:45:07Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky
Concept ACK	jonatack
Stale ACK	willcl-ark, pinheadmz

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:

#27607 (init: verify blocks data existence only once for all the indexers by furszy)
#27596 (assumeutxo (2) by jamesob)
#27125 (refactor, kernel: Decouple ArgsManager from blockstorage by TheCharlatan)
#25302 (build: Check usages of #if defined(...) by brokenprogrammer)
#24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)
#19792 (rpc: Add dumpcoinstats by fjahr)

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.

jonatack · 2022-05-23T22:42:06Z

Concept ACK

src/index/base.cpp

ryanofsky

Light code review ACK 1e598cf. First commit looks good. I had some questions and suggestions about some things in the second and third commits, but all the code looked ok and seemed like it should work

ryanofsky · 2022-06-09T20:11:56Z

src/node/blockstorage.cpp

@@ -885,6 +886,7 @@ void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFile
                StartShutdown();
                return;
            }
+            fReindexChainState = false;


In commit "node: add fReindexChainState flag to node" (8898261)

I understand that what this is doing with the new fReindexChainState global is similar to what happens with the existing fReindex global, but I think what happens with the fReindex global is not ideal. I think it's unnecessarily fragile how the value starts off false, then switches to true, then switches back to false again, and think a one way latch would be better. I think it's bad the the variable doesn't have any straightforward meaning but is some combination "was reindexing requested?" and "is reindexing in progress?". Also in this commit, fReindexChainState is different local variables subtly replaced by a global so you can't tell just looking at a diff whether all the existing fReindexChainState references were updated correctly.

I'd suggest leaving fReindexChainState alone, and adding a simpler atomic_bool g_indexes_ready_to_sync = false global variable, and setting it to true here, and in the normal code path. I think this would make the diff and the overall initialization sequence simpler and easier to understand.

That makes sense. I dropped the commit that made fReindexChainState a global and added g_indexes_ready_to_sync as suggested.

Making g_indexes_ready_to_sync a one-way switch requires to manually set it to true in the unit tests though. I did that in TestingSetup::TestingSetup() to avoid setting it in each unit tests that uses indexes separately.

ryanofsky · 2022-06-09T20:18:59Z

src/index/base.cpp

@@ -76,7 +78,7 @@ bool BaseIndex::Init()
        SetBestBlockIndex(locator_index);
    }
    m_synced = m_best_block_index.load() == active_chain.Tip();
-    if (!m_synced) {
+    if (!m_synced && fPruneMode) {


In commit "index: Enable reindex-chainstate with active indexes" (1e598cf)

This is pretty opaque and could use a comment. I'm also not sure it is better to be looking at fPruneMode here, than just checking whether reindexing is happening, when it sounds like reindexing is the real problem, and checking for fPruneMode is just a proxy for avoiding the real source of the problem?

Also it is not obvious to me that just because pruning isn't currently enabled doesn't mean pruning wasn't previously enabled and there couldn't be missing blocks worth checking for here.

Also it is not obvious to me that just because pruning isn't currently enabled doesn't mean pruning wasn't previously enabled and there couldn't be missing blocks worth checking for here.

I think that this shouldn't be possible: We check in Init when loading the chainstate (chainman.LoadBlockIndex()) that we either have all the blocks from genesis, or allow for pruning - otherwise, we abort with an InitError and never get to the point where the indexes are started.

I replaced the check with the indirect g_indexes_ready_to_sync as suggested and added a comment. This way, the pruning check will continue to be executed regardless of pruning status (unless -reindex is specified).

What I don't like is that this would throw a confusing InitError message if it somehow failed on a non-pruning node ("block of the index goes beyond pruned data"). Maybe the check should assert instead if fPrune==false?

Crypt-iQ · 2022-06-22T18:15:22Z

src/index/base.cpp

-        SetBestBlockIndex(m_chainstate->FindForkInGlobalIndex(locator));
+        // Setting the best block to the locator's top block. If it is not part of the
+        // best chain, we will rewind to the fork point during index sync
+        const CBlockIndex* locator_index{m_chainstate->m_blockman.LookupBlockIndex(locator.vHave.front())};


Just leaving as a note, but I think this allows the if (!active_chain.Contains(block_to_test)) { case to be hit. Previously FindForkInGlobalIndex would return a CBlockIndex only in the current chain and so active_chain.Contains would always be true.

re: #25193 (comment)

In commit "index: Use first block from locator instead of looking for fork point" (20bd221)

Yes, this seems to be the case. This is a good indication the original author or this code expecting was expecting to FindForkInGlobalIndex to just return a block based on the locator, not necessarily a block on the current chain. So new code is probably closer to intent of original code.

So new code is probably closer to intent of original code.

True. Although to be fair, this also changes the reading of the best block such that we don't use anything but the top block of the locator, which probably wasn't the original intent because otherwise there would have been no need to store a locator in the first place instead of a single block hash.

Without this patch, is it possible that the old code goes back to genesis in this rare, kind of contrived scenario?:

index best block committed at height h

a reorg occurs starting at h-1 to h+1

notifications are queued

chain state happens to be flushed (say the coins cache size is critical)

node crashes after flush so notifications aren't delivered

on startup, Tip=h+1, so FindForkInGlobalIndex for the index best block returns genesis

re: #25193 (comment)

Without this patch, is it possible that the old code goes back to genesis in this rare, kind of contrived scenario?:

Maybe I need to think about this more, but I don't think it can go back to genesis. It can just go back to the last common block before the reorg. Also, the problem which this patch fixes isn't going backwards in general, but going backwards without rewinding. The problem with the FindForkInGlobalIndex is that it goes backwards without making needed Rewind() call to update indexes

From my reading, if FindForkInGlobalIndex doesn't find pindex in the chain or doesn't find Tip() as an ancestor of pindex, it returns genesis. But yeah a bit orthogonal to the issue at hand

re: #25193 (comment)

From my reading, if FindForkInGlobalIndex doesn't find pindex in the chain or doesn't find Tip() as an ancestor of pindex, it returns genesis. But yeah a bit orthogonal to the issue at hand

Wow, you are right. The FindForkInGlobalIndex behavior is much different than the FindFork behavior. I assumed FindForkInGlobalIndex would try to find the last common ancestor between the locator block and the chain like FindFork does, but it doesn't even try to do this. Instead it will literally only return the exact locator block, or the chain tip, or the genesis block. It doesn't make any sense to me why the function would be implemented this way, but I guess it's good that this PR removes one usage of it...

mzumsande · 2022-06-23T19:35:42Z

1e598cf to 91947e4
addressed feedback by @ryanofsky - thanks!

ryanofsky

Code review ACK 91947e4. In first commit vector front() call is replaced by at(0) call to avoid undefined behavior in case null locator is loaded. In second commit fReindexChainState variable is replaced by simpler g_indexes_ready_to_sync that only changes from false to true

src/init.cpp

ryanofsky · 2022-06-27T18:56:49Z

src/index/base.cpp

-        SetBestBlockIndex(m_chainstate->FindForkInGlobalIndex(locator));
+        // Setting the best block to the locator's top block. If it is not part of the
+        // best chain, we will rewind to the fork point during index sync
+        const CBlockIndex* locator_index{m_chainstate->m_blockman.LookupBlockIndex(locator.vHave.front())};


re: #25193 (comment)

Without this patch, is it possible that the old code goes back to genesis in this rare, kind of contrived scenario?:

Maybe I need to think about this more, but I don't think it can go back to genesis. It can just go back to the last common block before the reorg. Also, the problem which this patch fixes isn't going backwards in general, but going backwards without rewinding. The problem with the FindForkInGlobalIndex is that it goes backwards without making needed Rewind() call to update indexes

mzumsande · 2022-07-01T15:19:28Z

91947e4 to 702f481:
rebased and addressed comment by @ryanofsky

mzumsande · 2023-04-05T17:11:51Z

0577bd6 to 974140f: rebased due to conflict with #25781

furszy

Instead of the global flag that requires manual sets at different locations and the indexes threads active wait, what if we move the indexes threads start after the loading process? e.g. furszy@1525e0a.

It makes code shorter and more robust. Plus, it let us keep the pruning checks as well.

willcl-ark

ACK 974140f

Can confirm via testing that this fixes the majority of #27558, although I was not able to reliably abort my node during an invalidateblock call as OP did in that issue...

I also extracted the feature_coinstatsindex test from 5fafeec and checked that it did indeed fail without the corresponding changes to BaseIndex::Init().

pinheadmz · 2023-05-11T17:37:16Z

src/index/base.cpp

+        // Setting the best block to the locator's top block. If it is not part of the
+        // best chain, we will rewind to the fork point during index sync
+        const CBlockIndex* locator_index{m_chainstate->m_blockman.LookupBlockIndex(locator.vHave.at(0))};
+        if (!locator_index) {
+            return InitError(strprintf(Untranslated("%s: best block of the index not found. Please rebuild the index."), GetName()));
+        }
+        SetBestBlockIndex(locator_index);


Another side effect of this PR is I don't think we use the locator at all anymore besides its tip hash ?!

That's correct! I didn't want to change the db format though to not break compatibility.

pinheadmz · 2023-05-11T17:48:46Z

In first commit vector front() call is replaced by at(0) call to avoid undefined behavior in case null locator is loaded.

This happens again later too:

bitcoin/src/index/base.cpp

Line 100 in 974140f

    
           const CBlockIndex* locator_index{m_chainstate->m_blockman.LookupBlockIndex(locator.vHave.at(0))};

pinheadmz

ACK 974140f

code review and local testing. verified the tests fail without the patches. great bug catch on the rewinding muhash! I also like @furszy idea about dropping the global atomic bool for a rerranged init sequence. I'll be happy to re-review if you included that.

Show Signature

pinheadmz's public key is on keybase

mzumsande · 2023-05-11T18:00:09Z

Thanks! I'll rebase and address furszy's comments next week!

ryanofsky · 2023-05-17T13:40:31Z

Thanks! I'll rebase and address furszy's comments next week!

I think it'd be good to just rebase this and merge it and not try to do the "move the indexes threads start after the loading process" change here. This PR is pretty simple, has had a good amount of review and testing, and I think that change would make it more complicated. furszy also implemented that change separately in #27607, and it should simplify both PRs to base that change on top of this one.

ryanofsky

Code review ACK 974140f. Confirmed this is just a clean rebase since my last review. This needs another rebase now, but after that I would like to merge it.

The index sync code has logic to go back the chain to the forking point, while also updating index-specific state, which is necessary to prevent possible corruption of the coinstatsindex. Also add a test for this (a reorg happens while the index is deactivated) that would not pass before this change.

This is achieved by letting the index sync thread wait until reindex-chainstate is finished. This also disables the pruning check when reindexing the chainstate (which is incompatible with prune mode) because there would be no chain at this point in init.

mzumsande · 2023-05-17T15:38:39Z

974140f to 97844d9: rebased

I think it'd be good to just rebase this and merge it and not try to do the "move the indexes threads start after the loading process" change here.

Ok, I only rebased.

@furszy I like your suggestion and will review/test it when you include them in #27607, which I believe will change init order more anyway.

ryanofsky

Code review ACK 97844d9. Just simple rebase since last review

…t, allo…

DrahtBot mentioned this pull request May 23, 2022

test: add coverage for unknown value to -blockfilterindex #25192

Merged

DrahtBot added the Refactoring label May 23, 2022

This was referenced May 24, 2022

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

Draft

doc: BaseIndex sync behavior with empty datadir #22485

Merged

rpc: Add dumpcoinstats #19792

Closed

DrahtBot added the Needs rebase label May 25, 2022

mzumsande force-pushed the 202205_index_allow_reindex_chainstate branch from c7c3494 to 1e598cf Compare May 25, 2022 22:21

DrahtBot removed the Needs rebase label May 25, 2022

This was referenced Jun 8, 2022

refactor: Reduce number of LoadChainstate parameters and return values #25308

Merged

build: Check usages of #if defined(...) #25302

Closed

ryanofsky reviewed Jun 9, 2022

View reviewed changes

src/index/base.cpp Outdated Show resolved Hide resolved

ryanofsky approved these changes Jun 9, 2022

View reviewed changes

DrahtBot mentioned this pull request Jun 15, 2022

Support ignoring "opt-in" flag for RBF (aka full RBF) #25373

Closed

Crypt-iQ reviewed Jun 22, 2022

View reviewed changes

mzumsande force-pushed the 202205_index_allow_reindex_chainstate branch from 1e598cf to 9bcd930 Compare June 23, 2022 19:31

mzumsande force-pushed the 202205_index_allow_reindex_chainstate branch from 9bcd930 to 91947e4 Compare June 23, 2022 20:08

DrahtBot mentioned this pull request Jun 25, 2022

[kernel 3a/n] Decouple CTxMemPool from ArgsManager #25290

Merged

2 tasks

ryanofsky mentioned this pull request Jun 27, 2022

index: ignore BlockConnected if pindex is in chain #25462

Closed

ryanofsky approved these changes Jun 27, 2022

View reviewed changes

DrahtBot added the Needs rebase label Jun 29, 2022

maflcko removed Refactoring Needs rebase labels Jul 1, 2022

DrahtBot added the UTXO Db and Indexes label Jul 1, 2022

mzumsande force-pushed the 202205_index_allow_reindex_chainstate branch from 91947e4 to 702f481 Compare July 1, 2022 15:16

maflcko added the Needs rebase label Jul 1, 2022

DrahtBot removed the Needs rebase label Apr 5, 2023

furszy reviewed Apr 6, 2023

View reviewed changes

mzumsande mentioned this pull request May 3, 2023

Coinstats index corrupted after invalidateblock and clean shutdown #27558

Closed

1 task

fanquake linked an issue May 4, 2023 that may be closed by this pull request

Coinstats index corrupted after invalidateblock and clean shutdown #27558

Closed

1 task

willcl-ark approved these changes May 4, 2023

View reviewed changes

DrahtBot requested a review from ryanofsky May 4, 2023 12:44

This was referenced May 5, 2023

kernel: Remove args, settings, chainparams, chainparamsbase from kernel library #27576

Merged

assumeutxo (2) #27596

Merged

index: make startup more efficient #27607

Merged

DrahtBot added the Needs rebase label May 11, 2023

pinheadmz reviewed May 11, 2023

View reviewed changes

pinheadmz approved these changes May 11, 2023

View reviewed changes

ryanofsky approved these changes May 17, 2023

View reviewed changes

mzumsande added 2 commits May 17, 2023 11:14

mzumsande force-pushed the 202205_index_allow_reindex_chainstate branch from 974140f to 97844d9 Compare May 17, 2023 15:31

DrahtBot removed the Needs rebase label May 17, 2023

ryanofsky approved these changes May 17, 2023

View reviewed changes

DrahtBot requested review from pinheadmz and willcl-ark May 17, 2023 17:22

ryanofsky merged commit 4e8a765 into bitcoin:master May 17, 2023

TheCharlatan mentioned this pull request May 17, 2023

kernel: Remove util/system from kernel library, interface_ui from validation. #27636

Merged

mzumsande deleted the 202205_index_allow_reindex_chainstate branch May 17, 2023 19:17

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 18, 2023

Merge bitcoin#25193: indexes: Read the locator's top block during ini…

8ca195e

…t, allo…

bitcoin locked and limited conversation to collaborators Sep 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

indexes: Read the locator's top block during init, allow interaction with reindex-chainstate #25193

indexes: Read the locator's top block during init, allow interaction with reindex-chainstate #25193

mzumsande commented May 23, 2022 •

edited by ryanofsky

Loading

mzumsande commented May 23, 2022

DrahtBot commented May 23, 2022 •

edited

Loading

jonatack commented May 23, 2022 •

edited

Loading

ryanofsky left a comment

ryanofsky Jun 9, 2022 •

edited

Loading

mzumsande Jun 23, 2022

mzumsande Jun 23, 2022

ryanofsky Jun 9, 2022

mzumsande Jun 23, 2022

Crypt-iQ Jun 22, 2022

ryanofsky Jun 23, 2022

mzumsande Jun 23, 2022

Crypt-iQ Jun 27, 2022

ryanofsky Jun 27, 2022

Crypt-iQ Jun 27, 2022

ryanofsky Jun 27, 2022

mzumsande commented Jun 23, 2022 •

edited

Loading

ryanofsky left a comment

ryanofsky Jun 27, 2022

mzumsande commented Jul 1, 2022

mzumsande commented Apr 5, 2023

furszy left a comment •

edited

Loading

willcl-ark left a comment

pinheadmz May 11, 2023

mzumsande May 11, 2023

pinheadmz commented May 11, 2023

pinheadmz left a comment

mzumsande commented May 11, 2023

ryanofsky commented May 17, 2023

ryanofsky left a comment

mzumsande commented May 17, 2023

ryanofsky left a comment

indexes: Read the locator's top block during init, allow interaction with reindex-chainstate #25193

indexes: Read the locator's top block during init, allow interaction with reindex-chainstate #25193

Conversation

mzumsande commented May 23, 2022 • edited by ryanofsky Loading

mzumsande commented May 23, 2022

DrahtBot commented May 23, 2022 • edited Loading

Reviews

Conflicts

jonatack commented May 23, 2022 • edited Loading

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzumsande commented Jun 23, 2022 • edited Loading

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzumsande commented Jul 1, 2022

mzumsande commented Apr 5, 2023

furszy left a comment • edited Loading

Choose a reason for hiding this comment

willcl-ark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinheadmz commented May 11, 2023

pinheadmz left a comment

Choose a reason for hiding this comment

mzumsande commented May 11, 2023

ryanofsky commented May 17, 2023

ryanofsky left a comment

Choose a reason for hiding this comment

mzumsande commented May 17, 2023

ryanofsky left a comment

Choose a reason for hiding this comment

mzumsande commented May 23, 2022 •

edited by ryanofsky

Loading

DrahtBot commented May 23, 2022 •

edited

Loading

jonatack commented May 23, 2022 •

edited

Loading

ryanofsky Jun 9, 2022 •

edited

Loading

mzumsande commented Jun 23, 2022 •

edited

Loading

furszy left a comment •

edited

Loading