indexes: Don't wipe indexes again when continuing a prior reindex

TheCharlatan commented at 8:08 pm on May 17, 2024: contributor

When restarting bitcoind during an ongoing reindex without setting the -reindex flag again, the block and coins db is left intact, but any data from the optional indexes is discarded. While not a bug per se, wiping the data again is wasteful, both in terms of having to write it again, as well as potentially leading to longer startup times. So keep the index data instead when continuing a prior reindex.

Also includes a bugfix and smaller code cleanups around the reindexing code. The bug was introduced in b47bd959207e82555f07e028cc2246943d32d4c3: “kernel: De-globalize fReindex”.

DrahtBot commented at 8:08 pm on May 17, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, fjahr, furszy, ryanofsky
Concept ACK	luke-jr
Stale ACK	theStack

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:

#30214 (refactor: Improve assumeutxo state representation by ryanofsky)
#30111 (locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip by glozow)
#30110 (refactor: TxDownloadManager by glozow)
#29678 (Bugfix: Correct first-run free space checks by luke-jr)
#29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)

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.

DrahtBot added the label UTXO Db and Indexes on May 17, 2024

DrahtBot added the label CI failed on May 17, 2024

DrahtBot removed the label CI failed on May 18, 2024

TheCharlatan force-pushed on May 18, 2024

stickies-v commented at 10:22 am on May 20, 2024: contributor

Concept ACK

in src/init.cpp:1642 in 991f50acad outdated

1638@@ -1639,17 +1639,17 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1639     // ********************************************************* Step 8: start indexers
1640 
1641     if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) {
1642-        g_txindex = std::make_unique<TxIndex>(interfaces::MakeChain(node), cache_sizes.tx_index, false, chainman.m_blockman.m_reindexing);
1643+        g_txindex = std::make_unique<TxIndex>(interfaces::MakeChain(node), cache_sizes.tx_index, false, blockman_opts.reindex);

stickies-v commented at 1:58 pm on May 20, 2024:

As discussed offline, I think this change means we’re ignoring the GUI-based reindex from https://github.com/bitcoin/bitcoin/pull/30132/files#diff-b1e19192258d83199d8adaa5ac31f067af98f63554bfdd679bd8e8073815e69dR1609, both for these optional indices but also for the block index

TheCharlatan force-pushed on May 20, 2024

TheCharlatan commented at 2:05 pm on May 20, 2024: contributor

Updated 991f50acad6809c83c0c44cf3c55fb9d2c0107e3 -> 9de8b263dabd6dd2f86f1f0253c6ee3fac7a4407 (preserveIndexOnRestart_0 -> preserveIndexOnRestart_1, compare)

Addressed @stickies-v’s comment, fixing a bug introduced in b47bd959207e82555f07e028cc2246943d32d4c3: “kernel: De-globalize fReindex”.

ryanofsky commented at 3:58 pm on May 20, 2024: contributor

fixing a bug introduced in b47bd959207e82555f07e028cc2246943d32d4c3: “kernel: De-globalize fReindex”.

~~Is this true? That commit, which was part of #29817, should not have changed any previous behavior~~

EDIT: Never mind, I see the problem now after reading f27290c39d63df36a1e1baa7f9c1609ebb65ca97 commit description. The bug happens because the BlockManager is destroyed each loop iteration in AppInitMain so the value of the chainman.m_blockman.m_reindexing variable gets reset.

furszy commented at 9:21 pm on May 20, 2024: member

Looking good in a first glance. It would be nice to add some coverage for it just so it doesn’t happen again. Maybe assert that certain logs are not present during init? Like the “Wiping LevelDB in <index_path>” one.

maflcko added this to the milestone 28.0 on May 21, 2024

stickies-v commented at 2:36 pm on May 21, 2024: contributor

Approach ACK. First 2 commits (0d04433149324616e838a30512bee9a04397855f) LGTM but the third one I’m going to need to spend a lot more time wrapping my head around the implications.

TheCharlatan commented at 3:49 pm on May 23, 2024: contributor

Updated 9de8b263dabd6dd2f86f1f0253c6ee3fac7a4407 -> dd290b3b58a1f9f9e182803866b8e2b9f72ecb3b (preserveIndexOnRestart_1 -> preserveIndexOnRestart_2, compare)

Added a commit for testing that the indexes are still there when continuing a reindex.

in test/functional/feature_reindex.py:85 in dd290b3b58 outdated

80+        with node.busy_wait_for_debug_log([b'basic block filter index is enabled at height']):
81+            self.restart_node(0, ['-blockfilterindex'])
82+
83+        # Restart node with reindex and stop reindex as soon as it starts reindexing
84+        self.log.info("Restarting node while reindexing..")
85+        with node.busy_wait_for_debug_log([b'initload thread start'], timeout=3):

furszy commented at 4:01 pm on May 23, 2024:

thinking more about this, let’s remove this timeout=3. It could fail on slow system or when running valgrind.

in test/functional/feature_reindex.py:80 in dd290b3b58 outdated

72@@ -73,13 +73,36 @@ def find_block(b, start):
73         # All blocks should be accepted and processed.
74         assert_equal(self.nodes[0].getblockcount(), 12)
75 
76+    def continue_reindex_after_shutdown(self):
77+        node = self.nodes[0]
78+        self.generate(node, 1500)
79+        self.log.info("Wait until block filter index is synced..")
80+        with node.busy_wait_for_debug_log([b'basic block filter index is enabled at height']):

maflcko commented at 4:04 pm on May 23, 2024:

any reason to use the busy loop here?

in test/functional/feature_reindex.py:86 in dd290b3b58 outdated

81+            self.restart_node(0, ['-blockfilterindex'])
82+
83+        # Restart node with reindex and stop reindex as soon as it starts reindexing
84+        self.log.info("Restarting node while reindexing..")
85+        with node.busy_wait_for_debug_log([b'initload thread start'], timeout=3):
86+            node.stop_node()

maflcko commented at 4:05 pm on May 23, 2024:

Any reason to put the stop under the busy loop here?

in test/functional/feature_reindex.py:88 in dd290b3b58 outdated

83+        # Restart node with reindex and stop reindex as soon as it starts reindexing
84+        self.log.info("Restarting node while reindexing..")
85+        with node.busy_wait_for_debug_log([b'initload thread start'], timeout=3):
86+            node.stop_node()
87+            node.start(['-blockfilterindex', '-reindex'])
88+            node.wait_for_rpc_connection(False)

maflcko commented at 4:05 pm on May 23, 2024:

nit: Should use named args for integral literal arguments

luke-jr commented at 4:50 pm on May 23, 2024: member

Concept ACK

TheCharlatan force-pushed on May 24, 2024

TheCharlatan commented at 7:37 am on May 24, 2024: contributor

Updated dd290b3b58a1f9f9e182803866b8e2b9f72ecb3b -> 891784ce73a1d911d33b125b66f3856fe6cda56b (preserveIndexOnRestart_2 -> preserveIndexOnRestart_3, compare)

Addressed @furszy’s comment, removed the timeout on the initload busy loop.
Addressed @maflcko’s comment, removed the busyloop waiting for the block filter index. I initially thought it might be useful to wait for the index to load completely, but I don’t think this is strictly required for this test.
Addressed @maflcko’s comment, moved stop_node out of the busy loop.
Addressed @maflcko’s comment, using named args for literal arguments now.

in test/functional/test_framework/test_node.py:244 in 891784ce73 outdated

240@@ -241,7 +241,7 @@ def start(self, extra_args=None, *, cwd=None, stdout=None, stderr=None, env=None
241         if self.start_perf:
242             self._start_perf()
243 
244-    def wait_for_rpc_connection(self):
245+    def wait_for_rpc_connection(self, wait_for_import=True):

maflcko commented at 7:56 am on May 24, 2024:

nit: Could force named args for options?

0    def wait_for_rpc_connection(self, *, wait_for_import=True):

in test/functional/feature_reindex.py:80 in 891784ce73 outdated

72@@ -73,13 +73,35 @@ def find_block(b, start):
73         # All blocks should be accepted and processed.
74         assert_equal(self.nodes[0].getblockcount(), 12)
75 
76+    def continue_reindex_after_shutdown(self):
77+        node = self.nodes[0]
78+        self.generate(node, 1500)
79+        self.log.info("Wait until block filter index is synced..")
80+        self.restart_node(0, ['-blockfilterindex'])

maflcko commented at 7:57 am on May 24, 2024:

nit: Is this needed? IIUC the index will be thrown away in the next line anyway?

TheCharlatan commented at 12:39 pm on May 24, 2024:

Yeah, I was thinking this might be used for some more regresssion tests, but as it is right now, it is useless. Will remove.

DrahtBot commented at 8:36 am on May 24, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/25367317004

DrahtBot added the label CI failed on May 24, 2024

TheCharlatan force-pushed on May 24, 2024

TheCharlatan commented at 12:46 pm on May 24, 2024: contributor

Thanks for the reviews @maflcko,

891784ce73a1d911d33b125b66f3856fe6cda56b -> eeea0818c1a20adc5225b98b185953d386c033e0 (preserveIndexOnRestart_3 -> preserveIndexOnRestart_4, compare)

Addressed @maflcko’s comment, removed redundant initial syncing of the blockfilterindex.
Addressed @maflcko’s comment, enforce using named argument.

DrahtBot removed the label CI failed on May 24, 2024

in src/init.cpp:1561 in eeea0818c1 outdated

1557@@ -1558,7 +1558,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1558 
1559         node::ChainstateLoadOptions options;
1560         options.mempool = Assert(node.mempool.get());
1561-        options.reindex = chainman.m_blockman.m_reindexing;
1562+        options.reindex = blockman_opts.reindex;

cbergqvist commented at 9:08 am on May 30, 2024:

nit: How about taking the opportunity to nail down the mutability of ChainstateLoadOptions for improved readability?

 0        const node::ChainstateLoadOptions options{
 1            .mempool = Assert(node.mempool.get()),
 2            .reindex = blockman_opts.reindex,
 3            .reindex_chainstate = fReindexChainState,
 4            .prune = chainman.m_blockman.IsPruneMode(),
 5            .require_full_verification = args.IsArgSet("-checkblocks") || args.IsArgSet("-checklevel"),
 6            .check_blocks = args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS),
 7            .check_level = args.GetIntArg("-checklevel", DEFAULT_CHECKLEVEL),
 8            .coins_error_cb = [] {
 9                uiInterface.ThreadSafeMessageBox(
10                    _("Error reading from database, shutting down."),
11                    "", CClientUIInterface::MSG_ERROR);
12            },
13        };

cbergqvist commented at 9:53 am on May 30, 2024: contributor

Tried cherry-picking only eeea0818c1a20adc5225b98b185953d386c033e0 on top of the base of the PR (058af75874ffa2b4064e3d6d30cc50f0ec754ba8) and ran the test and it succeeds without the other C++ changes?

Sorry for the false alarm, didn’t rebuild before running the test. 🤦 Fails as expected now.

furszy commented at 2:59 pm on May 30, 2024: member

Code review ACK eeea0818c1

DrahtBot requested review from stickies-v on May 30, 2024

DrahtBot requested review from luke-jr on May 30, 2024

ryanofsky approved

ryanofsky commented at 9:19 pm on June 3, 2024: contributor

Code review ACK eeea0818c1a20adc5225b98b185953d386c033e0. All the changes here look good.

But I also think it was a mistake in #29817 to add the BlockManagerOpts::reindex option, and think it would be better to have a less confusing set of options. The following change built on top of this PR could provide a simpler alternative: 9c643e7ddd82523b84f65b614d3e6c1f640b23c7. Feel free to use any of these changes, or they could be a separate followup.

TheCharlatan commented at 10:10 am on June 4, 2024: contributor

Re #30132#pullrequestreview-2094875458

But I also think it was a mistake in #29817 to add the BlockManagerOpts::reindex option, and think it would be better to have a less confusing set of options. The following change built on top of this PR could provide a simpler alternative: https://github.com/bitcoin/bitcoin/commit/9c643e7ddd82523b84f65b614d3e6c1f640b23c7. Feel free to use any of these changes, or they could be a separate followup.

Removing the blockman option looks like a nice improvement, but I am not sure yet about the renaming. Going from do_reindex to wipe_block_tree_db back to m_reindexing seems a bit convoluted. I looked at adding a do_reindex argument to ImportBlocks instead so we don’t have to set m_reindexing when loading the chainstate, but not sure if that is really better either.

In the commit message it also says:

Also the previous set of options did not allow rebuilding the block database without also rebuilding the chainstate database, when it should be possible to do those independently.

This might be nice too. I was thinking a bit if this would really be safe, and I think it should be if the assumption anyway is that the chainstate is pristine.

stickies-v commented at 12:11 pm on June 4, 2024: contributor

The following change built on top of this PR could provide a simpler alternative: 9c643e7.

I like this suggestion. I think the naming is more clear in terms of the actual effect each variable has. I also think not having two reindex options is an improvement, the flow is more clear to me now.

I don’t think wipe_block_tree_db (and a couple of other blockman options) should ultimately be a ChainstateLoadOptions member, but fixing that is probably beyond the scope of this PR (and I’m looking into doing that in a separate pull).

ryanofsky commented at 2:39 pm on June 4, 2024: contributor

Removing the blockman option looks like a nice improvement, but I am not sure yet about the renaming. Going from do_reindex to wipe_block_tree_db back to m_reindexing seems a bit convoluted.

That’s a good point. One possible way to address it could be to rename m_reindexing as well like 38cc045b6966dda009332695ed55c86bd5a9fea3. If we move away from having multiple variables called “reindex” and instead name variables individually based on what they do, I think it would be an improvement overall.

The renames suggested in 9c643e7ddd82523b84f65b614d3e6c1f640b23c7 and 38cc045b6966dda009332695ed55c86bd5a9fea3 don’t directly relate to this PR, so might make more sense as followups to avoid complicating things. On the other hand, if you took the portion of 9c643e7ddd82523b84f65b614d3e6c1f640b23c7 replacing BlockManagerOpts::reindex with do_reindex it would might make the third commit 9de8b263dabd6dd2f86f1f0253c6ee3fac7a4407 here easier to understand. Just feel free to ignore these suggestions for now or use any parts that seem useful.

Also the previous set of options did not allow rebuilding the block database without also rebuilding the chainstate database, when it should be possible to do those independently.

Looking at the code more, I think setting wipe_block_tree_db = true and wipe_chainstate_db = false should probably work, even though we never do it. But if we add these options to the kernel API, the documentation could warn this combination is currently untested / unsupported. One problem with this combination is that it might not handle some reorgs, because LoadExternalBlockFile only currently scans block files and populates CBlockIndex::nDataPos. It doesn’t currently scan undo files and populate CBlockIndex::nUndoPos, though it could.

in test/functional/feature_reindex.py:89 in eeea0818c1 outdated

84+            node.start(['-blockfilterindex', '-reindex'])
85+            node.wait_for_rpc_connection(wait_for_import=False)
86+        node.stop_node()
87+
88+        # Start node without the reindex flag and verify it does not wipe the indexes data again
89+        db_path = node.datadir_path / 'regtest'/ 'indexes' / 'blockfilter' / 'basic' / 'db'

theStack commented at 10:25 pm on June 6, 2024:

nit:

0        db_path = node.chain_path / 'indexes' / 'blockfilter' / 'basic' / 'db'

theStack approved

theStack commented at 10:59 pm on June 6, 2024: contributor

ACK eeea0818c1a20adc5225b98b185953d386c033e0

Nice catch. Took me a while to see where b47bd959207e82555f07e028cc2246943d32d4c3 introduced the bug (the retry logic is confusing indeed!), but both the detailed commit messages and yesterday’s PR review club notes/log were very helpful to grok it.

TheCharlatan commented at 6:14 am on June 7, 2024: contributor

Thanks for the review and ACKs, I will address the left over nits here shortly, I think re-ACKing should be easy enough.

bugfix: Streamline setting reindex option

Reverts a bug introduced in b47bd959207e82555f07e028cc2246943d32d4c3
"kernel: De-globalize fReindex". The change leads to a GUI user being
prompted to re-index on a chainstate loading failure more than once as
well as the node actually not reindexing if the user chooses to. Fix
this by setting the reindexing option instead of the atomic, which can
be safely re-used to indicate that a reindex should be attempted.

The bug specifically is caused by the chainman, and thus the blockman
and its m_reindexing atomic being destroyed on every iteration of
the for loop.

The reindex option for ChainstateLoadOptions is currently also set in a
confusing way. By using the reindex atomic, it is not obvious in which
scenario it is true or false.

The atomic is controlled by both the user passing the -reindex option,
the user chosing to reindex if something went wrong during chainstate
loading when running the gui, and by reading the reindexing flag from
the block tree database in LoadBlockIndexDB. In practice this read is
done through the chainstate module's CompleteChainstateInitialization's
call to LoadBlockIndex. Since this is only done after the reindex option
is set already, it does not have an effect on it.

Make this clear by using the reindex option from the blockman opts which
is only controlled by the user.

533eab7d67

validation: Remove needs_init from LoadBlockIndex

It does not control any actual logic and the log message as well as the
comment are obsolete, since no database initialization takes place there
anymore. Log messages indicating when indexes and chainstate databases
are loaded exist in other places.

e172553223

TheCharlatan force-pushed on Jun 7, 2024

TheCharlatan commented at 12:00 pm on June 7, 2024: contributor

Thank you for your suggestions @ryanofsky! I applied both of your suggested patches.

eeea0818c1a20adc5225b98b185953d386c033e0 -> 682f1f1827dff78208bc1272f82d217c42a2cd69 (preserveIndexOnRestart_4 -> preserveIndexOnRestart_5, compare)

Addressed @theStack’s comment, using chain_path to construct the db path in the functional tests.
Added @ryanofsky’s suggestion in this patch, removing the blockmanager reindex option as well as renaming reindex to wipe_block_tree_db and reindex_chainstate to wipe_chainstate_db in the ChainstateLoadOptions.
Added @ryanofsky’s suggestion in this patch, renaming m_reindexing to m_blockfiles_indexed.

in src/test/util/setup_common.cpp:280 in 682f1f1827 outdated

275@@ -276,8 +276,8 @@ void ChainTestingSetup::LoadVerifyActivateChainstate()
276     options.mempool = Assert(m_node.mempool.get());
277     options.block_tree_db_in_memory = m_block_tree_db_in_memory;
278     options.coins_db_in_memory = m_coins_db_in_memory;
279-    options.reindex = chainman.m_blockman.m_reindexing;
280-    options.reindex_chainstate = m_args.GetBoolArg("-reindex-chainstate", false);
281+    options.wipe_block_tree_db = m_args.GetBoolArg("-reindex", false);
282+    options.wipe_chainstate_db = options.wipe_block_tree_db || m_args.GetBoolArg("-reindex-chainstate", false);

stickies-v commented at 3:32 pm on June 7, 2024:

nit: the commit message says both wipes should be possible independent of each other, so I think coupling them here is not great (i’m also open to adding a do_reindex variable to avoid duplication).

0    options.wipe_chainstate_db = m_args.GetBoolArg("-reindex", false) || m_args.GetBoolArg("-reindex-chainstate", false);

in src/node/blockstorage.h:276 in 682f1f1827 outdated

276 
277     /**
278-     * Tracks if a reindex is currently in progress. Set to true when a reindex
279-     * is requested and false when reindexing completes. Its value is persisted
280-     * in the BlockTreeDB across restarts.
281+     * Whether blockfiles have been added to the block tree database. Normally

stickies-v commented at 3:55 pm on June 7, 2024:

nit: could be interpreted as “any blockfiles have been added”, so perhaps this is less ambiguous:

0     * Whether all blockfiles have been added to the block tree database. Normally

in src/bitcoin-chainstate.cpp:154 in 682f1f1827 outdated

150@@ -151,7 +151,7 @@ int main(int argc, char* argv[])
151     {
152         LOCK(chainman.GetMutex());
153         std::cout
154-        << "\t" << "Reindexing: " << std::boolalpha << chainman.m_blockman.m_reindexing.load() << std::noboolalpha << std::endl
155+        << "\t" << "Reindexing: " << std::boolalpha << !chainman.m_blockman.m_blockfiles_indexed.load() << std::noboolalpha << std::endl

stickies-v commented at 3:57 pm on June 7, 2024:

nit

0        << "\t" << "Blockfiles Indexed: " << std::boolalpha << chainman.m_blockman.m_blockfiles_indexed.load() << std::noboolalpha << std::endl

in src/init.cpp:1699 in 682f1f1827 outdated

1695@@ -1694,7 +1696,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1696     int chain_active_height = WITH_LOCK(cs_main, return chainman.ActiveChain().Height());
1697 
1698     // On first startup, warn on low block storage space
1699-    if (!chainman.m_blockman.m_reindexing && !fReindexChainState && chain_active_height <= 1) {
1700+    if (!chainman.m_blockman.m_blockfiles_indexed && !do_reindex_chainstate && chain_active_height <= 1) {

stickies-v commented at 3:58 pm on June 7, 2024:

I think this has to be the inverse, but just “reverting back” to !do_reindex is probably more clear:

0    if (!do_reindex && !do_reindex_chainstate && chain_active_height <= 1) {

stickies-v commented at 5:27 pm on June 7, 2024:

I actually don’t understand why we also need to check for !do_reindex_chainstate here, when CChain::vChain is populated by the blockindex building process, so my intuition tells me chain_active_height should only be affected by -reindex (but lldb tells me otherwise, so it does indeed seem necessary).

ryanofsky commented at 7:35 pm on June 7, 2024:

(but lldb tells me otherwise, so it does indeed seem necessary).

This happens because the initial chain tip is set based on the best block in the chainstate database, so if the database is being wiped, the tip is not set and the chain height is 0:

https://github.com/bitcoin/bitcoin/blob/6e4d18f37f42894fe9a0d7948c1da3f061fc6b60/src/node/chainstate.cpp#L136-L138

stickies-v commented at 4:46 pm on June 7, 2024: contributor

Code LGTM 682f1f1827dff78208bc1272f82d217c42a2cd69 modulo one small bug in comments.

I couldn’t find or think of any occasions where the new reduced wiping behaviour introduces problematic behaviour. The new variable names introduced make things significantly more clear and is a very welcome change.

kernel: Add less confusing reindex options

Drop confusing kernel options:

  BlockManagerOpts::reindex
  ChainstateLoadOptions::reindex
  ChainstateLoadOptions::reindex_chainstate

Replacing them with more straightforward options:

  ChainstateLoadOptions::wipe_block_tree_db
  ChainstateLoadOptions::wipe_chainstate_db

Having two options called "reindex" which did slightly different things
was needlessly confusing (one option wiped the block tree database, and
the other caused block files to be rescanned). Also the previous set of
options did not allow rebuilding the block database without also
rebuilding the chainstate database, when it should be possible to do
those independently.

804f09dfa1

indexes: Don't wipe indexes again when already reindexing

Before this change continuing a reindex without the -reindex flag set
would leave the block and coins db intact, but discard the data of the
optional indexes. While not a bug per se, wiping the data again is
wasteful, both in terms of having to write it again, and potentially
leading to longer startup times.

When initially running a reindex, both the block index and any further
activated indexes are wiped. On an index's Init(), both the best block
stored by the index and the chain's tip are null. An index's m_synced
member is therefore true. This means that it will process blocks through
validation events while the reindex is running.

Currently, if the reindex is continued without the user re-specifying
the reindex flag, the block index is preserved but further index data is
wiped. This leads to the stored best block being null, but the chain tip
existing. The m_synced member will be set to false. The index will not
process blocks through the validation interface, but instead use the
background sync once the reindex is completed.

If the index is preserved (this change) after a restart its best block
may potentially match the chain tip. The m_synced member will be set to
true and the index can process validation events during the rest of the
reindex.

201c1a9282

test: Add functional test for continuing a reindex

Co-authored-by: furszy <matiasfurszyfer@protonmail.com>

1b1c6dcca0

blockman: Replace m_reindexing with m_blockfiles_indexed

This is a just a mechanical change, renaming and inverting the meaning
of the indexing variable.

"m_blockfiles_indexed" is a more straightforward name for this variable
because this variable just indicates whether or not
<datadir>/blocks/blk?????.dat files have been indexed in the
<datadir>/blocks/index LevelDB database. The name "m_reindexing" was
more confusing, it could be true even if -reindex was not specified, and
false when it was specified. Also, the previous name unnecessarily
required thinking about the whole reindexing process just to understand
simple checks in validation code about whether blocks were indexed.

The motivation for this change is to follow up on previous commits,
moving away from having multiple variables called "reindex" internally,
and instead naming variables individually after what they do and
represent.

f68cba29b3

TheCharlatan force-pushed on Jun 7, 2024

TheCharlatan commented at 5:38 pm on June 7, 2024: contributor

Thanks for the review @stickies-v,

Updated f68cba29b3be0dec7877022b18a193a3b78c1099 -> f68cba29b3be0dec7877022b18a193a3b78c1099 (preserveIndexOnRestart_5 -> preserveIndexOnRestart_6, compare)

Addressed @stickies-v’s comment, slightly changing the way the wipe_chainstate_db is set in setup_common.
Addressed @stickies-v’s comment, clarified m_blockfiles_indexed docstring.
Addressed @stickies-v’s comment, changing bitcoin-chainstate log lines.
Addressed @stickies-v’s comment, fixed missed renaming for the block storage check. The check should be done if a chainstate is present.

stickies-v approved

stickies-v commented at 5:46 pm on June 7, 2024: contributor

ACK f68cba29b3be0dec7877022b18a193a3b78c1099

DrahtBot requested review from furszy on Jun 7, 2024

DrahtBot requested review from ryanofsky on Jun 7, 2024

DrahtBot requested review from theStack on Jun 7, 2024

in test/functional/feature_reindex.py:84 in 1b1c6dcca0 outdated

79+
80+        # Restart node with reindex and stop reindex as soon as it starts reindexing
81+        self.log.info("Restarting node while reindexing..")
82+        node.stop_node()
83+        with node.busy_wait_for_debug_log([b'initload thread start']):
84+            node.start(['-blockfilterindex', '-reindex'])

fjahr commented at 12:18 pm on June 8, 2024:

nit: Why was the blockfilterindex chosen here? I am assuming because it’s slow? Would be good to add a comment because it may be confusing for others in the future what this choice has to do with the test.

TheCharlatan commented at 1:19 pm on June 8, 2024:

I just picked the one which furszy picked and I’m guessing he just picked one too. I’ll check if there is any signifcant effect when picking a different one.

in test/functional/feature_reindex.py:78 in 1b1c6dcca0 outdated

72@@ -73,13 +73,33 @@ def find_block(b, start):
73         # All blocks should be accepted and processed.
74         assert_equal(self.nodes[0].getblockcount(), 12)
75 
76+    def continue_reindex_after_shutdown(self):
77+        node = self.nodes[0]
78+        self.generate(node, 1500)

fjahr commented at 12:27 pm on June 8, 2024:

nit: I guess this is needed so the node can be stopped fast enough. It’s still a race and could turn out to be flakey in the CI, right? I don’t have a better idea to fix this right now but a comment might be good to make this explicit and make future debugging easier if this turns out to be the case.

fjahr commented at 12:52 pm on June 8, 2024: contributor

Code review ACK f68cba29b3be0dec7877022b18a193a3b78c1099

I also confirmed that the new test fails before the changes here are applied.

I think the test could use a bit more explanation on its reasoning but this shouldn’t block a merge of this as-is.

in src/test/util/setup_common.cpp:280 in 804f09dfa1 outdated

275@@ -276,8 +276,8 @@ void ChainTestingSetup::LoadVerifyActivateChainstate()
276     options.mempool = Assert(m_node.mempool.get());
277     options.block_tree_db_in_memory = m_block_tree_db_in_memory;
278     options.coins_db_in_memory = m_coins_db_in_memory;
279-    options.reindex = chainman.m_blockman.m_reindexing;
280-    options.reindex_chainstate = m_args.GetBoolArg("-reindex-chainstate", false);
281+    options.wipe_block_tree_db = m_args.GetBoolArg("-reindex", false);
282+    options.wipe_chainstate_db = m_args.GetBoolArg("-reindex", false) || m_args.GetBoolArg("-reindex-chainstate", false);

furszy commented at 2:10 pm on June 9, 2024:

tiny nit

0    options.wipe_block_tree_db = m_args.GetBoolArg("-reindex", false);
1    options.wipe_chainstate_db = options.wipe_block_tree_db || m_args.GetBoolArg("-reindex-chainstate", false);

Side note: I don’t think this is used anywhere.

stickies-v commented at 11:28 pm on June 9, 2024:

I suggested the current approach, see #30132 (review)

I don’t think this is used anywhere.

What do you mean?

furszy commented at 2:50 am on June 10, 2024:

I suggested the current approach, see #30132 (review)

Hmm ok. We should probably go further and deduplicate the init.cpp / setup_commons.cpp code somewhere in the future.

I don’t think this is used anywhere.

What do you mean?

This is part of the unit test framework and no unit test, benchmark or fuzz test seems to make use of it. “-reindex” and “-reindex-chainstate” are always unset.

furszy commented at 9:43 pm on June 9, 2024: member

Code review ACK f68cba29b3be0dec7877022b18a193a3b78c1099

Great last commit.

Question about 804f09dfa116300914e2aeef05ed9710dd504e8c commit description:

Also the previous set of options did not allow rebuilding the block database without also rebuilding the chainstate database, when it should be possible to do those independently.

is this tested anywhere?

TheCharlatan commented at 8:51 am on June 10, 2024: contributor

Re #30132#pullrequestreview-2106239011

is this tested anywhere?

No, and I’m not sure it should be given we don’t support this. Maybe we can add a comment and assert that just wiping the block index db is not supported for now?

ryanofsky approved

ryanofsky commented at 1:16 pm on June 10, 2024: contributor

Code review ACK f68cba29b3be0dec7877022b18a193a3b78c1099. Only changes since last review were cherry-picking suggested commits that rename variables, improving comments, and making some tweaks to test code.

ryanofsky commented at 1:26 pm on June 10, 2024: contributor

re: #30132 (comment)

No, and I’m not sure it should be given we don’t support this. Maybe we can add a comment and assert that just wiping the block index db is not supported for now?

FWIW, my original draft of 804f09dfa116300914e2aeef05ed9710dd504e8c added this code to LoadChainstate:

0// For now, don't allow wiping block tree db without also wiping chainstate
1// db. There's no reason this could not work in theory, but in practice the
2// code path is untested, and to be really robust, the
3// LoadExternalBlockFile function should to be updated to scan undo files,
4// not just block files, and to populate CBlockIndex::nUndoPos, not just
5// CBlockIndex::nDataPos.
6assert(!options.wipe_block_tree_db || options.wipe_chainstate_db);

I decided to drop it to keep things simpler, since inevitably the kernel API will support combinations of options bitcoin core doesn’t exercise or test, and it might be cumbersome to try to warn about all of them. But I could understand wanting to do it in some cases like this.

I think the PR is ready to merge, so you can let me know if you want to add an assert or just merge it in its current form.

TheCharlatan commented at 2:00 pm on June 10, 2024: contributor

#30132 (comment)

I think the PR is ready to merge, so you can let me know if you want to add an assert or just merge it in its current form.

I think this is rfm.

ryanofsky merged this on Jun 10, 2024

ryanofsky closed this on Jun 10, 2024

indexes: Don’t wipe indexes again when continuing a prior reindex #30132

Code Coverage

Reviews

Conflicts