fReindex is one of the last remaining globals exposed by the kernel library, so move it into the blockstorage class to reduce the amount of global mutable state and make the kernel library a bit less awkward to use.
This pull request is part of the libbitcoinkernel project.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | ryanofsky, mzumsande, stickies-v, achow101 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
🚧 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.
32@@ -33,6 +33,8 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, BlockManager::Op
33
34 if (auto value{args.GetBoolArg("-fastprune")}) opts.fast_prune = *value;
35
36+ opts.reindex = args.GetBoolArg("-reindex", false);
To avoid defining the default value twice (default member init in BlockManagerOpts), perhaps better to use:
0 if (auto value{args.GetBoolArg("-reindex")}) opts.reindex = *value;
Approach ACK d9bcbbf2293ef427b37eefca30f074be5eeeca26 and code LGTM.
Some thoughts I have from reviewing this PR, but that are orthogonal to it:
It seems like we have 4 places where we store information about reindexing being required or being in progress:
BlockTreeDB (through WriteReindexing() and ReadReindexing())BlockManager::m_reindex (previously fReindex)BlockManagerOpts::reindexChainstateLoadOptions::reindex1. and 2. are updated based on reindex progress, and should stay in sync with each other most of the time. 3. and 4. are set up to one time after initialization, and then never updated again, and thus will stay in sync with each other but may fall out of sync with 1. and 2.. Perhaps this is intentional?
2. (BlockManager::m_reindex) seems to be indicate both whether a reindex is requested or in progress, whereas 1. (BlockTreeDB) is exclusively used to indicate if the reindex is in progress, and 3. and 4. (BlockManagerOpts::reindex and ChainstateLoadOptions::reindex) is exclusively used to indicate if the reindex is requested.
I wonder if this could and should be cleaned up? For example, replacing all uses of ChainstateLoadOptions::reindex with BlockManager::m_reindex is a trivial code change and passes unit and functional tests:
0diff --git a/src/init.cpp b/src/init.cpp
1index 0550eec970..c9183b5c7b 100644
2--- a/src/init.cpp
3+++ b/src/init.cpp
4@@ -1560,7 +1560,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
5
6 node::ChainstateLoadOptions options;
7 options.mempool = Assert(node.mempool.get());
8- options.reindex = chainman.m_blockman.m_reindex;
9 options.reindex_chainstate = fReindexChainState;
10 options.prune = chainman.m_blockman.IsPruneMode();
11 options.check_blocks = args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS);
12@@ -1602,7 +1601,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
13
14 if (!fLoaded && !ShutdownRequested(node)) {
15 // first suggest a reindex
16- if (!options.reindex) {
17+ if (!chainman.m_blockman.m_reindex) {
18 bool fRet = uiInterface.ThreadSafeQuestion(
19 error + Untranslated(".\n\n") + _("Do you want to rebuild the block database now?"),
20 error.original + ".\nPlease restart with -reindex or -reindex-chainstate to recover.",
21diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
22index 3ee712ab7e..70d4c18c1d 100644
23--- a/src/node/chainstate.cpp
24+++ b/src/node/chainstate.cpp
25@@ -45,10 +45,10 @@ static ChainstateLoadResult CompleteChainstateInitialization(
26 .path = chainman.m_options.datadir / "blocks" / "index",
27 .cache_bytes = static_cast<size_t>(cache_sizes.block_tree_db),
28 .memory_only = options.block_tree_db_in_memory,
29- .wipe_data = options.reindex,
30+ .wipe_data = chainman.m_blockman.m_reindex,
31 .options = chainman.m_options.block_tree_db});
32
33- if (options.reindex) {
34+ if (chainman.m_blockman.m_reindex) {
35 pblocktree->WriteReindexing(true);
36 //If we're reindexing in prune mode, wipe away unusable block files and all undo data files
37 if (options.prune) {
38@@ -89,7 +89,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
39 }
40
41 auto is_coinsview_empty = [&](Chainstate* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
42- return options.reindex || options.reindex_chainstate || chainstate->CoinsTip().GetBestBlock().IsNull();
43+ return chainman.m_blockman.m_reindex || options.reindex_chainstate || chainstate->CoinsTip().GetBestBlock().IsNull();
44 };
45
46 assert(chainman.m_total_coinstip_cache > 0);
47@@ -110,7 +110,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
48 chainstate->InitCoinsDB(
49 /*cache_size_bytes=*/chainman.m_total_coinsdb_cache * init_cache_fraction,
50 /*in_memory=*/options.coins_db_in_memory,
51- /*should_wipe=*/options.reindex || options.reindex_chainstate);
52+ /*should_wipe=*/chainman.m_blockman.m_reindex || options.reindex_chainstate);
53
54 if (options.coins_error_cb) {
55 chainstate->CoinsErrorCatcher().AddReadErrCallback(options.coins_error_cb);
56@@ -142,7 +142,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
57 }
58 }
59
60- if (!options.reindex) {
61+ if (!chainman.m_blockman.m_reindex) {
62 auto chainstates{chainman.GetAll()};
63 if (std::any_of(chainstates.begin(), chainstates.end(),
64 [](const Chainstate* cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->NeedsRedownload(); })) {
65@@ -188,7 +188,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
66 // Load a chain created from a UTXO snapshot, if any exist.
67 bool has_snapshot = chainman.DetectSnapshotChainstate();
68
69- if (has_snapshot && (options.reindex || options.reindex_chainstate)) {
70+ if (has_snapshot && (chainman.m_blockman.m_reindex || options.reindex_chainstate)) {
71 LogPrintf("[snapshot] deleting snapshot chainstate due to reindexing\n");
72 if (!chainman.DeleteSnapshotChainstate()) {
73 return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated("Couldn't remove snapshot chainstate.")};
74@@ -247,7 +247,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
75 ChainstateLoadResult VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options)
76 {
77 auto is_coinsview_empty = [&](Chainstate* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
78- return options.reindex || options.reindex_chainstate || chainstate->CoinsTip().GetBestBlock().IsNull();
79+ return chainman.m_blockman.m_reindex || options.reindex_chainstate || chainstate->CoinsTip().GetBestBlock().IsNull();
80 };
81
82 LOCK(cs_main);
83diff --git a/src/node/chainstate.h b/src/node/chainstate.h
84index a6e9a0331b..ebcff861ce 100644
85--- a/src/node/chainstate.h
86+++ b/src/node/chainstate.h
87@@ -22,7 +22,6 @@ struct ChainstateLoadOptions {
88 CTxMemPool* mempool{nullptr};
89 bool block_tree_db_in_memory{false};
90 bool coins_db_in_memory{false};
91- bool reindex{false};
92 bool reindex_chainstate{false};
93 bool prune{false};
94 //! Setting require_full_verification to true will require all checks at
95diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
96index 3140f43dcf..1b960ad266 100644
97--- a/src/test/util/setup_common.cpp
98+++ b/src/test/util/setup_common.cpp
99@@ -278,7 +278,6 @@ void ChainTestingSetup::LoadVerifyActivateChainstate()
100 options.mempool = Assert(m_node.mempool.get());
101 options.block_tree_db_in_memory = m_block_tree_db_in_memory;
102 options.coins_db_in_memory = m_coins_db_in_memory;
103- options.reindex = chainman.m_blockman.m_reindex;
104 options.reindex_chainstate = m_args.GetBoolArg("-reindex-chainstate", false);
105 options.prune = chainman.m_blockman.IsPruneMode();
106 options.check_blocks = m_args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS);
Most of these concerns are quite orthogonal to this PR, since it’s the same situation on master with fReindex, so we don’t need to discuss this further here but I thought I’d raise it.
1483@@ -1485,7 +1484,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1484
1485 node.notifications = std::make_unique<KernelNotifications>(*Assert(node.shutdown), node.exit_status);
1486 ReadNotificationArgs(args, *node.notifications);
1487- fReindex = args.GetBoolArg("-reindex", false);
In commit “kernel: De-globalize fReindex” (d9bcbbf2293ef427b37eefca30f074be5eeeca26)
Note: removing this line does not change behavior, because GetBoolArg("-reindex") is now called a few lines below on line 1501 in ApplyArgsManOptions(args, blockman_opts)
256+ m_interrupt{interrupt},
257+ m_reindex{m_opts.reindex} {};
258
259 const util::SignalInterrupt& m_interrupt;
260 std::atomic<bool> m_importing{false};
261+ std::atomic_bool m_reindex;
In commit “kernel: De-globalize fReindex” (d9bcbbf2293ef427b37eefca30f074be5eeeca26)
I don’t think m_reindex is good name for this variable because it is easy to get confused with the reindex option which reflects whether reindexing was requested, not whether it is currently happening. Would suggest calling this m_reindexing and adding a comment that this reflects whether reindexing is currently happening, and is set to true when reindexing is requested, and false when reindexing completes. Could also mention that it is serialized to BlockTreeDB so it persists across restarts.
3248@@ -3250,10 +3249,10 @@ bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex*
3249 return true;
3250 }
3251
3252-static SynchronizationState GetSynchronizationState(bool init)
3253+static SynchronizationState GetSynchronizationState(bool init, std::atomic<bool>& reindex)
In commit “kernel: De-globalize fReindex” (d9bcbbf2293ef427b37eefca30f074be5eeeca26)
I think std::atomic<bool>& reindex should be replaced by bool reindexing here. Calling it reindexing would make the name consistent with the block manager m_reindexing member (suggested above) which this is referencing. Avoiding std::atomic would make the function more deterministic, and avoiding the non-const reference would make it clear the value will not be modified.
Code review ACK d9bcbbf2293ef427b37eefca30f074be5eeeca26, but I think reindex /m_reindex naming is a little confusing so I left some suggestions below to clean it up.
I also really like stickies suggestion from #29817#pullrequestreview-2001088868 to delete the ChainstateLoadOptions::reindex variable and think it would be great to add it as a second commit. If you do add it, I also think it would also be good idea to add a third commit renaming ChainstateLoadOptions::reindex_chainstate ChainstateLoadOptions::should_wipe as a small scripted diff so that name is more descriptive and internally consistent.
Stickies other suggestion #29817 (review) to avoid duplicating the default value also seems good.
Re #29817#pullrequestreview-2006662114 and #29817#pullrequestreview-2001088868
For example, replacing all uses of ChainstateLoadOptions::reindex with BlockManager::m_reindex is a trivial code change and passes unit and functional tests … I also really like stickies suggestion from #29817#pullrequestreview-2001088868 to delete the ChainstateLoadOptions::reindex variable and think it would be great to add it as a second commi
I thought abour this initially as well, but the way I read the code, this is actually a change in behaviour. The persisted reindexing value is read from the database with a call to LoadBlockIndex. Further down, this would lead to the coins being wiped again after a restart. Meaning if the user restarts without setting reindex, but with reindexing set in the database, it would not wipe the block tree db, but would wipe the coins, while with the current behaviour it would not wipe anything and continue from where it left off.
Updated d9bcbbf2293ef427b37eefca30f074be5eeeca26 -> 9b2c9c2fce32fe858d1361e863c72108a384a5c8 (noGlobalReindex_0 -> noGlobalReindex_1, compare)
GetSynchronizationState to a bool.m_reindex to m_reindexing and added a comment describing its purpose.1559@@ -1562,7 +1560,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1560
1561 node::ChainstateLoadOptions options;
1562 options.mempool = Assert(node.mempool.get());
1563- options.reindex = node::fReindex;
1564+ options.reindex = chainman.m_blockman.m_reindexing;
In commit “kernel: De-globalize fReindex” (9b2c9c2fce32fe858d1361e863c72108a384a5c8)
I think it would be less fragile to assign blockman_opts.reindex instead of chainman.m_blockman.m_reindex.
Assigning blockman_opts.reindex should ensure and make it more obvious that this is only affected by the value of the -reindex setting, not the value of a previous setting saved with WriteReindexing.
In commit “kernel: De-globalize fReindex” (b47bd959207e82555f07e028cc2246943d32d4c3)
I think it would be less fragile to assign
blockman_opts.reindexinstead ofchainman.m_blockman.m_reindex.
This suggestion was not implemented in this PR, but is implemented in followup PR #30132.
1607@@ -1610,7 +1608,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1608 error.original + ".\nPlease restart with -reindex or -reindex-chainstate to recover.",
1609 "", CClientUIInterface::MSG_ERROR | CClientUIInterface::BTN_ABORT);
1610 if (fRet) {
1611- fReindex = true;
1612+ chainman.m_blockman.m_reindexing = true;
In commit “kernel: De-globalize fReindex” (9b2c9c2fce32fe858d1361e863c72108a384a5c8)
Note: I initially thought there was a minor bug in existing code (independent of this PR) on line 1605 above which checks if (!options.reindex) instead of if (!chainman.m_blockman.m_reindexing). It seemed wrong because it could trigger an error suggesting using reindex flag even if reindexing was already happening from a previous request. This behavior is surprising, but on second thought, could actually make sense if something has gone wrong during reindexing and there is a reason to restart it.
In commit “kernel: De-globalize fReindex” (b47bd959207e82555f07e028cc2246943d32d4c3)
It turns out there was a bug introduced here. Setting chainman.m_blockman.m_reindexing = true has no effect because chainman is destroyed and recreated on the next loop iteration on line 1537 above. So after this change, reindexing no longer occurs when the user answers “Yes” above. This is fixed in #30132
1640@@ -1643,17 +1641,17 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1641 // ********************************************************* Step 8: start indexers
1642
1643 if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) {
1644- g_txindex = std::make_unique<TxIndex>(interfaces::MakeChain(node), cache_sizes.tx_index, false, fReindex);
1645+ g_txindex = std::make_unique<TxIndex>(interfaces::MakeChain(node), cache_sizes.tx_index, false, chainman.m_blockman.m_reindexing);
In commit “kernel: De-globalize fReindex” (9b2c9c2fce32fe858d1361e863c72108a384a5c8)
It seems like there is a prexisting bug here, and the f_wipe argument passed here should be blockman_opts.reindex instead of chainman.m_blockman.m_reindexing or fReindex. Otherwise if the node is restarted before reindexing completes the TxIndex will be wiped a second time?
Same for BlockFilterIndex and CoinStatsIndex below
reindexing atomic is true do not issue any validation signals, so they don’t have an effect on the indexes. However recreating them is redundant and I don’t think it is particularly useful. I think it would also be good to make the behavior between the coins and the other indexes consistent, so I’ll take your suggestion here.
AFAICT the operations executed while the
reindexingatomic is true do not issue any validation signals, so they don’t have an effect on the indexes.
I need to test this, but this would be surprising to me. I think the idea behind having a separate StartIndexBackgroundSync function that runs after index initialization is that indexes should be able to receive signals during reindexing when -reindex is used so they do not need to completely resync when indexing is finished.
Will check on this, but I think I might recommend just keeping behavior unchanged in this PR so it is a more straightforward refactoring. Or this behavior is worth changing, it seems like it could be moved to a separate commit.
I need to test this, but this would be surprising to me.
Indeed, my impression was wrong. When we restart after a previous reindex without passing in -reindex, the first step is activating the chain again.
Code review ACK 9b2c9c2fce32fe858d1361e863c72108a384a5c8. This looks good in its current form. I just had one suggestion below (not important) and some questions about possible minor pre-existing bugs.
re: #29817 (comment)
I thought abour this initially as well, but the way I read the code, this is actually a change in behaviour […]
Thanks for the explanation, that makes sense.
Updated 9b2c9c2fce32fe858d1361e863c72108a384a5c8 -> b0594f6cf157e9bd71b2062cdf0aa65936fd2282 (noGlobalReindex_1 -> noGlobalReindex_2, compare)
blockman_opts.reindex instead of chainman.m_blockman.m_reindexing.
Rebased b0594f6cf157e9bd71b2062cdf0aa65936fd2282 -> 46b9ec10dd238c5c1191bcdfcf06d1361cde15f1 (noGlobalReindex_2 -> noGlobalReindex_3, compare)
fReindex is one of the last remaining globals exposed by the kernel
library, so move it into the blockstorage class to reduce the amount of
global mutable state and make the kernel library a bit less awkward to
use.
Updated 46b9ec10dd238c5c1191bcdfcf06d1361cde15f1 -> b47bd959207e82555f07e028cc2246943d32d4c3 (noGlobalReindex_3 -> noGlobalReindex_4, compare)
-reindex flag.
Code review ACK b47bd959207e82555f07e028cc2246943d32d4c3. I rereviewed the whole PR, but the only change since last review was reverting the bugfix #29817 (review) and make the change a pure refactoring.
I think this change makes the code a lot less confusing by using separate names for the -reindex option and reindexing state, and could avoid bugs like the one mentioned in the future.
4821@@ -4823,8 +4822,8 @@ bool ChainstateManager::LoadBlockIndex()
4822
4823 if (needs_init) {
4824 // Everything here is for *new* reindex/DBs. Thus, though
needs_init variable are confusing. Originally (https://github.com/bitcoin/bitcoin/commit/eda888e57352037ab2e60f6ef90098b3ce23a157) it may have made sense, but now there is just a log message left which doesn’t even seem to belong here (no db initialization is happening here). So I think this could be cleaned up, but no need to do it here.
3253@@ -3255,10 +3254,10 @@ bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex*
3254 return true;
3255 }
3256
3257-static SynchronizationState GetSynchronizationState(bool init)
3258+static SynchronizationState GetSynchronizationState(bool init, bool reindexing)
static SynchronizationState GetSynchronizationState(const ChainstateManager& chainman) makes for a more logical function signature now, but it doesn’t really seem to make for much of a clean-up either, so… not sure.
