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

mzumsande commented at 9:27 pm on May 23, 2022: contributor

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 commented at 9:28 pm on May 23, 2022: contributor

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 commented at 9:45 pm on May 23, 2022: contributor

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 commented at 10:42 pm on May 23, 2022: contributor

Concept ACK

DrahtBot added the label Refactoring on May 23, 2022

DrahtBot added the label Needs rebase on May 25, 2022

mzumsande force-pushed on May 25, 2022

DrahtBot removed the label Needs rebase on May 25, 2022

in src/index/base.cpp:72 in 20bd221989 outdated

66@@ -67,7 +67,13 @@ bool BaseIndex::Init()
67     if (locator.IsNull()) {
68         SetBestBlockIndex(nullptr);
69     } else {
70-        SetBestBlockIndex(m_chainstate->FindForkInGlobalIndex(locator));
71+        // Setting the best block to the locator's top block. If it is not part of the
72+        // best chain, we will rewind to the fork point during index sync
73+        const CBlockIndex* locator_index{m_chainstate->m_blockman.LookupBlockIndex(locator.vHave.front())};

ryanofsky commented at 8:00 pm on June 9, 2022:

In commit “index: Use first block from locator instead of looking for fork point” (20bd2219894e53ab39d2b11edf9a7f3c7bda489b)

Might be better to call vHave.at(0) or check explicitly for an empty vector because vHave.front() behavior is undefined if the vector is empty.

mzumsande commented at 7:27 pm on June 23, 2022:

Changed to vHave.at(0)

in src/node/blockstorage.cpp:889 in 8898261ac0 outdated

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

ryanofsky commented at 8:11 pm on June 9, 2022:

In commit “node: add fReindexChainState flag to node” (8898261ac03945e559c98a0abcb9455942ab4775)

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.

mzumsande commented at 7:26 pm on June 23, 2022:

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

mzumsande commented at 8:07 pm on June 23, 2022:

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.

in src/index/base.cpp:81 in 1e598cfb2b outdated

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

ryanofsky commented at 8:18 pm on June 9, 2022:

In commit “index: Enable reindex-chainstate with active indexes” (1e598cfb2b3d0e56b1fb301bcf6870f42453c7cc)

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.

mzumsande commented at 7:25 pm on June 23, 2022:

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?

ryanofsky approved

ryanofsky commented at 8:21 pm on June 9, 2022: contributor

Light code review ACK 1e598cfb2b3d0e56b1fb301bcf6870f42453c7cc. 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

in src/index/base.cpp:74 in 1e598cfb2b outdated

68@@ -67,10 +69,16 @@ bool BaseIndex::Init()
69     if (locator.IsNull()) {
70         SetBestBlockIndex(nullptr);
71     } else {
72-        SetBestBlockIndex(m_chainstate->FindForkInGlobalIndex(locator));
73+        // Setting the best block to the locator's top block. If it is not part of the
74+        // best chain, we will rewind to the fork point during index sync
75+        const CBlockIndex* locator_index{m_chainstate->m_blockman.LookupBlockIndex(locator.vHave.front())};

Crypt-iQ commented at 6:15 pm on June 22, 2022:

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.

ryanofsky commented at 0:04 am on June 23, 2022:

re: #25193 (review)

In commit “index: Use first block from locator instead of looking for fork point” (20bd2219894e53ab39d2b11edf9a7f3c7bda489b)

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.

mzumsande commented at 7:34 pm on June 23, 2022:

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.

Crypt-iQ commented at 4:25 pm on June 27, 2022:

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

ryanofsky commented at 6:56 pm on June 27, 2022:

re: #25193 (review)

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

Crypt-iQ commented at 8:41 pm on June 27, 2022:

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

ryanofsky commented at 10:39 pm on June 27, 2022:

re: #25193 (review)

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 force-pushed on Jun 23, 2022

mzumsande commented at 7:35 pm on June 23, 2022: contributor

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

mzumsande force-pushed on Jun 23, 2022

in src/init.cpp:1556 in 91947e4387 outdated

1550@@ -1563,6 +1551,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1551     RegisterValidationInterface(node.peerman.get());
1552 
1553     // ********************************************************* Step 8: start indexers
1554+
1555+    // If reindex-chainstate was specified, delay syncing indexes until ThreadImport has reindexed the chain
1556+    g_indexes_ready_to_sync = !fReindexChainState;

ryanofsky commented at 6:41 pm on June 27, 2022:

In commit “index: Enable reindex-chainstate with active indexes” (91947e4387d98290d39c9201f85a9aefa235a4d1)

This is a pedantic, academic suggestion, but should consider writing this as:

0if (!fReindexChainState) g_indexes_ready_to_sync = true;

To make this part of the init function idempotent, and also make it clearer that index code only needs to handle g_indexes_ready_to_sync transitioning from false to true, never the other way around.

mzumsande commented at 3:16 pm on July 1, 2022:

Done with the latest push.

ryanofsky approved

ryanofsky commented at 6:59 pm on June 27, 2022: contributor

Code review ACK 91947e4387d98290d39c9201f85a9aefa235a4d1. 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

DrahtBot added the label Needs rebase on Jun 29, 2022

MarcoFalke removed the label Refactoring on Jul 1, 2022

MarcoFalke removed the label Needs rebase on Jul 1, 2022

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

mzumsande force-pushed on Jul 1, 2022

MarcoFalke added the label Needs rebase on Jul 1, 2022

mzumsande commented at 3:19 pm on July 1, 2022: contributor

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

DrahtBot removed the label Needs rebase on Jul 1, 2022

ryanofsky approved

ryanofsky commented at 5:20 pm on July 7, 2022: contributor

Code review ACK 702f481f7e97ebfd438f27464de3a2d4d77497bc. Only changes since last review were rebasing and making suggested code clarification (thanks!)

DrahtBot added the label Needs rebase on Jul 18, 2022

mzumsande force-pushed on Jul 19, 2022

mzumsande commented at 10:24 pm on July 19, 2022: contributor

702f481 to 6a85526: rebased due to conflict with #25487

DrahtBot removed the label Needs rebase on Jul 19, 2022

DrahtBot added the label Needs rebase on Jul 20, 2022

mzumsande force-pushed on Jul 20, 2022

mzumsande commented at 3:15 pm on July 20, 2022: contributor

6a85526 to 53ff637: rebased due to conflict with #25308

DrahtBot removed the label Needs rebase on Jul 20, 2022

ryanofsky approved

ryanofsky commented at 7:29 pm on August 18, 2022: contributor

Code review ACK 53ff63799404e6828779c5e4f321acf256fe9018. Only changes since last review were rebasing.

I do think it would be good if PR description described benefits of these changes, which are (1) avoiding possible coinstatsindex corruption in the case of a reorg and (2) allowing the -reindex-chainstate option to be used without disabling indexes. You could also consider including this information in release notes.

mzumsande commented at 5:49 pm on August 21, 2022: contributor

I do think it would be good if PR description described benefits of these changes

Done, thanks.

You could also consider including this information in release notes.

Do you mean a sentence like “The -reindex-chainstate option can now be used without the necessity to disable custom indexes”? I’m not sure if the first change changes user experience enough to be part of the release notes.

DrahtBot added the label Needs rebase on Sep 16, 2022

mzumsande force-pushed on Sep 19, 2022

mzumsande commented at 5:41 pm on September 19, 2022: contributor

53ff637 to 2d0acc2: Rebased

DrahtBot removed the label Needs rebase on Sep 19, 2022

ryanofsky approved

ryanofsky commented at 5:57 pm on October 6, 2022: contributor

Code review ACK 2d0acc259836e5cb7f40579e76375420a942b86c. This is a simple, helpful PR. Would encourage others to review.

Only changes since last review are mentioning benefits of the change in the PR description (I added a few formatting and wording tweaks on top of this) and rebasing.

Do you mean a sentence like “The -reindex-chainstate option can now be used without the necessity to disable custom indexes”? I’m not sure if the first change changes user experience enough to be part of the release notes.

Fair enough. I think it’s a nice feature, but no need to mention it

DrahtBot added the label Needs rebase on Oct 26, 2022

mzumsande force-pushed on Nov 3, 2022

mzumsande commented at 1:20 am on November 3, 2022: contributor

2d0acc2 to 0d00d69: rebased due to conflict with #25704

DrahtBot removed the label Needs rebase on Nov 3, 2022

DrahtBot added the label Needs rebase on Jan 27, 2023

mzumsande force-pushed on Jan 31, 2023

DrahtBot removed the label Needs rebase on Jan 31, 2023

DrahtBot added the label Needs rebase on Mar 15, 2023

mzumsande force-pushed on Apr 5, 2023

mzumsande commented at 5:11 pm on April 5, 2023: contributor

0577bd6d1bcbbc808fd20d7a368a562799ceccc3 to 974140f9e721740f857b45d10d7dbab62fdbbe53: rebased due to conflict with #25781

DrahtBot removed the label Needs rebase on Apr 5, 2023

furszy commented at 2:43 pm on April 6, 2023: member

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. https://github.com/furszy/bitcoin-core/commit/1525e0ae360d10cbc8bd5d8f15bc27112254dbef.

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

willcl-ark approved

willcl-ark commented at 12:44 pm on May 4, 2023: contributor

ACK 974140f9e7

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 5fafeec471 and checked that it did indeed fail without the corresponding changes to BaseIndex::Init().

DrahtBot requested review from ryanofsky on May 4, 2023

DrahtBot added the label Needs rebase on May 11, 2023

in src/index/base.cpp:103 in 974140f9e7 outdated

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

pinheadmz commented at 5:37 pm on May 11, 2023:

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

mzumsande commented at 5:59 pm on May 11, 2023:

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

pinheadmz commented at 5:48 pm on May 11, 2023: member

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:

https://github.com/bitcoin/bitcoin/blob/974140f9e721740f857b45d10d7dbab62fdbbe53/src/index/base.cpp#L100

pinheadmz approved

pinheadmz commented at 5:54 pm on May 11, 2023: member

ACK 974140f9e721740f857b45d10d7dbab62fdbbe53

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.

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 974140f9e721740f857b45d10d7dbab62fdbbe53
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmRdKuQACgkQ5+KYS2KJ
 7yTrWzRAAi1TzWndTKGoM6WwPvnXacA3OkluidZqIoa6zkeEnYzE7voAkRs2ZVmZp
 8T9FJ90ouWfkIfoaC1SewYGpJ2dykVSP41dmCgo0F06ceigiOAHTENDqJxkjFNL3e
 9+3sau/iPaGSOw8LU863kfABCI0KSqffnW9drQPySCt6lhCqSEhUJBpdBHHtaXS2z
10/pF7nTUbvhV7hKbUKWB6bdLLkDO8w54yZXQkt3iNQDFWATFmutqxc7nhpdNe1fQW
11vOqAOdw3ZdmPVNJB5H9nzmx46HDJemEr/w/8jy+6Hl6RAaxlhIs2a4gR64jLEeg6
128OD69S4XpCxu4FosZwoJyp7FmHL0ZINh4tR6bAPDO9Oevg5SFtsRP3zy01NI0PRW
13wS3drLRG7PVkuFuwUHcExhPQyQIUoGnhLopfPjtngRTjfEuNUlUoYaZHzzO7Yjl6
14eR6WG16vgtCHilnV1vypBX06U5gnq2Lu5VHoNTFuh9cveaIxXBKXtnJVLPhgaAmI
15mMTfl+2A/2rvS3PgVXRBa+16e42h8y6rJB+6oHghhAD4Arz6sfn48K1blWk1Sp1j
16NgNtREdWeqMmRVPTutn7Aoxa3/k7NeBC5qeipZdviSOqTusvuzt2mYZsIzVw7Aie
17bwoX0quWu8tftAI4oEfGnRjXmJ9+XTJourAfZ9RDCJWzdYG6PXo=
18=HWSL
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

mzumsande commented at 6:00 pm on May 11, 2023: contributor

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

ryanofsky commented at 1:40 pm on May 17, 2023: contributor

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 approved

ryanofsky commented at 1:42 pm on May 17, 2023: contributor

Code review ACK 974140f9e721740f857b45d10d7dbab62fdbbe53. 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.

index: Use first block from locator instead of looking for fork point

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.

60bec3c82d

index: Enable reindex-chainstate with active indexes

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.

97844d9268

mzumsande force-pushed on May 17, 2023

mzumsande commented at 3:38 pm on May 17, 2023: contributor

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.

DrahtBot removed the label Needs rebase on May 17, 2023

ryanofsky approved

ryanofsky commented at 5:21 pm on May 17, 2023: contributor

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

DrahtBot requested review from pinheadmz on May 17, 2023

DrahtBot requested review from willcl-ark on May 17, 2023

ryanofsky merged this on May 17, 2023

ryanofsky closed this on May 17, 2023

mzumsande deleted the branch on May 17, 2023

sidhujag referenced this in commit 8ca195ecf1 on May 18, 2023

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

Reviews

Conflicts