init: Coalesce Chainstate loading sequence between {,non-}unittest codepaths

dongcarl commented at 4:18 pm on October 14, 2021: member

This PR:

Coalesce the Chainstate loading sequence between AppInitMain and *TestingSetup (which makes it more tested)
Makes the Chainstate loading sequence reusable in preparation for future work extracting out our consensus engine.

Code-wise, this PR:

Extracts AppInitMain’s Chainstate loading sequence into a ::LoadChainstateSequence function
Makes this ::LoadChainstateSequence function reusable by
1. Decoupling it from various concepts (ArgsManager, uiInterface, etc)
2. Making it report errors using an enum rather than by setting a bilingual_str
Makes *TestingSetup use this new ::LoadChainstateSequence

Reviewers: Aside from commentary, I’ve also included git diff flags of interest in the commit messages which I hope will aid review!

in src/init.cpp:28 in 29c5c2b470 outdated

23@@ -24,6 +24,8 @@
24 #include <index/blockfilterindex.h>
25 #include <index/coinstatsindex.h>
26 #include <index/txindex.h>
27+#include <init/caches.h> // for CalculateCacheSizes
28+#include <init/chainstate.h> // for LoadChainstateSequence

Empact commented at 4:25 pm on October 14, 2021:

nit: FYI @fanquake is discouraging “for” comments: #15294#pullrequestreview-779241491

jnewbery commented at 1:06 pm on November 5, 2021:

I agree. These comments are unmaintainable.

jamesob commented at 4:30 pm on October 14, 2021: member

Concept ACK! I’ve already done a preliminary review of this code but will formally test and review very soon.

DrahtBot added the label Needs rebase on Oct 14, 2021

dongcarl force-pushed on Oct 14, 2021

dongcarl commented at 5:04 pm on October 14, 2021: member

Pushed 29c5c2b4700aa3f537e7291971dbd763469fce1c -> 3d5aa4b8503ed834e4e14651cd64f8a5d3d47f46

Rebased over master
Changed some commit messages

DrahtBot removed the label Needs rebase on Oct 14, 2021

DrahtBot added the label Build system on Oct 14, 2021

DrahtBot added the label RPC/REST/ZMQ on Oct 14, 2021

DrahtBot added the label Validation on Oct 14, 2021

DrahtBot commented at 8:00 pm on October 14, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21763 (test: Run AppInitSanityChecks before all tests by MarcoFalke)

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.

theuni commented at 9:57 pm on October 14, 2021: member

Concept ACK. Will review.

DrahtBot added the label Needs rebase on Oct 15, 2021

dongcarl force-pushed on Oct 15, 2021

dongcarl commented at 3:50 pm on October 15, 2021: member

Pushed 3d5aa4b850…7dd898c0df:

Rebased over master
Tweaked last commit to minimize use of gArgs in setup_common

DrahtBot removed the label Needs rebase on Oct 15, 2021

MarcoFalke removed the label Build system on Oct 26, 2021

MarcoFalke removed the label RPC/REST/ZMQ on Oct 26, 2021

MarcoFalke removed the label Validation on Oct 26, 2021

MarcoFalke added the label Refactoring on Oct 26, 2021

in src/init.cpp:1415 in 7dd898c0df outdated

1548+                                        },
1549+                                        []() {
1550+                                            uiInterface.InitMessage(_("Verifying blocks…").translated);
1551+                                        });
1552+        } catch (const std::exception& e) {
1553+            LogPrintf("%s\n", e.what()); // XXX

jamesob commented at 5:22 pm on October 29, 2021:

cd29d334a3

XXX to be removed/addressed?

dongcarl commented at 6:13 pm on November 15, 2021:

Fixed as of 6736e24a490d756e0e1e70b9e212775cf305ebe6

in src/validation.cpp:3767 in 7dd898c0df outdated

3763@@ -3764,6 +3764,7 @@ bool CChainState::LoadChainTip()
3764     PruneBlockIndexCandidates();
3765 
3766     tip = m_chain.Tip();
3767+    uiInterface.NotifyBlockTip(GetSynchronizationState(IsInitialBlockDownload()), tip);

jamesob commented at 5:41 pm on October 29, 2021:

b229024932

Note for other reviewers: NotifyBlockTip -> RPCNotifyBlockChange(tip) is performed here: https://github.com/jamesob/bitcoin/blob/master/src/init.cpp#L343-L347

Also note that uiInterface.NotifyBlockTip() is already called from within other CChainState methods (e.g. ActivateBestChain()), so there’s no concern about introducing exposure to a global in CChainState code.

ryanofsky commented at 6:34 pm on November 5, 2021:

In commit “validation: Call NotifyBlockTip in CCS::LoadChainTip” (b229024932ebd4775070149ff0f27d0fd329bf0a)

This seems like a good change, but two questions come to mind here that might be good to address in commit message:

1-Previously this code would work regardless of whether OnRPCStarted was called before NotifyBlockTip. Now it must be called before or the notification will be lost. I’m assuming the correct thing does happen, but it would be good to say if it does, or if something guarantees it.

2-It looks like there are some other callers of NotifyBlockTip_connect in the node code and handleNotifyBlockTip in GUI code that may now receive new notifications. Is this the case, and are all these callers ok receiving it it? Are they are also OK being called with cs_main held?

EDIT: The newer version of b229024932ebd4775070149ff0f27d0fd329bf0a in ca13555758e6d868d3cfa5b55e679fca74381b03 in just moves the OnRPCStarted call instead of replacing it, to avoid potential problems above.

in src/init/caches.cpp:11 in 7dd898c0df outdated

 6+
 7+#include <txdb.h>
 8+#include <util/system.h>
 9+#include <validation.h>
10+
11+void CalculateCacheSizes(const ArgsManager& args, CacheSizes& cache_sizes, size_t n_indexes)

jamesob commented at 6:00 pm on October 29, 2021:

1f966b8d9f

One naive question here might be: why are we using a mutable arg for cache_sizes vs. just returning a constructed CacheSizes instance to make the function pure? Answer seems to be that this is used within a unittest context to modify an existing m_cache_sizes instance.

jamesob approved

jamesob commented at 6:16 pm on October 29, 2021: member

ACK 7dd898c0df678859ca92773b6990e4e28a83ab24 (jamesob/ackr/23280.2.dongcarl.init_coalesce_chainstate)

This is an excellent change in terms of cleaning up and modularizing our messy (and previously un-unittested) initialization code. I had actually taken a run at doing this previously as a part of #15606 but gave up because I thought it’d be too big a change. Carl has done a much better job of implementing it here than I had anyway, but for what it’s worth the design I had sketched is very similar to this one.

This patch paves the way for further libbitcoinkernel work - but even aside from that, it ensures that we’re reusing more (all?) of our chain initialization logic from within the unittests which has quite a bit of value on its own.

I wrote #23289 specifically to test this change, and I’ve run it ~25 times on HEAD. I’ve also tested this branch with a pre-existing mainnet datadir, booting and interrupting init a number of times.

Great job, @dongcarl!

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 7dd898c0df678859ca92773b6990e4e28a83ab24 ([`jamesob/ackr/23280.2.dongcarl.init_coalesce_chainstate`](https://github.com/jamesob/bitcoin/tree/ackr/23280.2.dongcarl.init_coalesce_chainstate))
 4
 5-----BEGIN PGP SIGNATURE-----
 6
 7iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmF8OJcACgkQepNdrbLE
 8TwW1uxAAjTIPIZ4ddUhSlAax8GQR43wEE3Qvk5+I51RHNGyIS8QoPb0lgmBYgqJW
 9bqvVyMC0cKg9QNDLELrQQ/ySVf2Vl9H/6va4P18ZAKLg8cNuyeVr3cl4Swe0ZH9F
10clRa8b75NTRUtOZlt34O2eTfigyKbnQ5L1+G8HJWNoBvWEPt5JSV7G2UU1tyLEh3
113lvS5vHivSt+8WqIhB/lBIERJmI7vHjFAhN8P6SgLyQr7NxKA8Lw6cFUFeJIWKDP
12pkza4uabu1cDqAurxO2jgIq4GDL7FXz3fkXcFzGTPgY1JlXI26dEkuWBSEA42zGs
13ubizjy/JIyRUbzRv5jBia5woDJgR40eFfsb7GkZOHfZtoQsDN85xwvZpRCzl/YnY
14y1exK8fGSmCuP6cIP2mbbz9TrUlrGfyZm0vwIiqrRpGtO7q1CdTyiV7gnOwRUCjY
15LqUI2WXMuJGSD/WaPIghpllFBejadETVkfjMUHBtaTO8WdpPchdgnLZFtKlyjvLV
16x1UkNk3Xqkzzk75+XPEK+GBSDPnaD9fDnfypu7f18pin2O6gGFVErQto67xM1+HY
170eRlQOhM/T+SdsDWcl9z4fUS1pA+zxxpr3BRNHR4aNbuxXj326VIVnQUatEotncy
18ilPh7NYFFVMWpoOGG5HbucjlazQLf/Z4Q9FxUqdDglXRSqK+kQM=
19=XedH
20-----END PGP SIGNATURE-----

0Tested on Linux-5.10.0-9-amd64-x86_64-with-glibc2.28
1
2Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon --enable-natpmp-default CXX=/usr/bin/clang++ CC=/usr/bin/clang PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --disable-openssl-tests --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --no-create --no-recursion
3
4Compiled with /usr/bin/ccache /usr/bin/clang++ -std=c++17 -mavx -mavx2 -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=.  -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4 -msha -msse4.1 -msse4.2  i
5
6Compiler version: Debian clang version 13.0.1-++20211019122913+8a93745a7121-1~exp1~20211019003538.10

dongcarl commented at 8:28 pm on October 29, 2021: member

My sincerest thanks for the thorough review @jamesob, that functional test in #23289 is certainly beyond the call of duty, and the master bug it revealed is leading to some excellent investigation in #23365. 🎉

I’m glad to hear that this sort of cleanup came up for the AssumeUTXO work as well, my hope is that the change will make future work easier involving the init sequence easier to reason about.

reusing more (all?) of our chain initialization logic from within the unittests

One thing to note is that although we load the chainstate, we do not activate it, which is done in ThreadImport with ActivateBestChain, this can be included in followup PRs.

in src/init/chainstate.h:17 in 83ed7db27b outdated

12+class CChainParams;
13+class CClientUIInterface;
14+class ChainstateManager;
15+struct NodeContext;
16+
17+bool LoadChainstateSequence(bool& fLoaded,

ryanofsky commented at 7:15 pm on November 4, 2021:

In commit “init: Extract chainstate loading sequence” (83ed7db27b4c4798666b77d83fc8e7c3eab8c621)

 0The sequence can have 4 types of outcomes:
 1
 21. Success
 32. Shutdown requested
 4 - nothing failed but a shutdown was triggered in the middle of the
 5   sequence
 63. Soft failure
 7 - a failure that might be recovered from with a reindex
 84. Hard failure
 9 - a failure that definitively cannot be recovered from with a reindex
10
11Currently, LoadChainstateSequence returns a bool which:
12   - if false
13       - Definitely a "Hard failure"
14   - if true
15       - if fLoaded -> "Success"
16       - if ShutdownRequested() -> "Shutdown requested"
17       - else -> "Soft failure"

I’d remove this explanation from the commit message and move it to here above this function declaration, or to the .cpp file as a code comment. This is helpful information to know and a shame for it to be lost in a temporary commit message. Also it will make upcoming changes easier to understand if this documentation is updated in sync with the changes.

ryanofsky commented at 5:25 pm on November 5, 2021:

In commit “init: Extract chainstate loading sequence” (83ed7db27b4c4798666b77d83fc8e7c3eab8c621)

Other code in the src/init/ directory is using the `init:: namespace, and I think it’d be good to follow that pattern:

https://github.com/bitcoin/bitcoin/blob/77a2f5d30c5ecb764b8a7c098492e1f5cdec90f0/src/init/common.cpp#L25 https://github.com/bitcoin/bitcoin/blob/77a2f5d30c5ecb764b8a7c098492e1f5cdec90f0/src/init/bitcoind.cpp#L15

In general, I’d like to expand use of pattern putting src/init code in init::, src/util code in util::, src/interfaces code in interfaces::, src/node code in node::, etc,

ryanofsky commented at 6:43 pm on November 5, 2021:

In commit “init: Extract chainstate loading sequence” (83ed7db27b4c4798666b77d83fc8e7c3eab8c621)

“Sequence” here seems like a strange thing to include in a the function name. Most functions are sequences. IMO an ideal name for this function would be something like node::LoadChainStates (assuming this is moved to src/node/ directory and put in node:: namespace).

dongcarl commented at 6:17 pm on November 15, 2021:

Fixed as of 6736e24a490d756e0e1e70b9e212775cf305ebe6

in src/init.cpp:1453 in cd29d334a3 outdated

1450+                break;
1451+            case ChainstateLoadingError::ERROR_CORRUPTED_BLOCK_DB:
1452+                strLoadError = _("Corrupted block database detected");
1453+                break;
1454+            }
1455+        } else if (!ShutdownRequested()) {

ryanofsky commented at 12:35 pm on November 5, 2021:

In commit “init/chainstate: Decouple from stringy errors” (cd29d334a38fdaf0bbbe810a9cbdeeffad394daa)

With this commit introducing and returning a nice status type, I think it is a shame to add a new out-of-band global ShutdownRequested call to check the interrupted status separately from the return value. Would suggest adding a ChainstateLoadingError::INTERRUPTED_SHUTDOWN_REQUESTED or similar return code. And in the future hopefully we can get rid of other global ShutdownRequested calls as well.

In case motivation for this request is unclear, reason I think a return code is better than a global status check is that requiring a global check makes function usage easier to screw up. It is easier to accidentally forget an external check than to forget to handle a return value. Also if there is a race where a shutdown is requested after the function returns, it doesn’t seem right to skip logging the call’s success. The right interruption point is after the function call and the log, not between.

theuni commented at 8:17 pm on November 5, 2021:

I third this idea. It’s what I would’ve expected, and the alternative here seems weird. Additionally, we might want to specify in the future why shutdown was requested (rpc, console, etc).

dongcarl commented at 6:16 pm on November 15, 2021:

Fixed as of 6736e24a490d756e0e1e70b9e212775cf305ebe6

in src/init.cpp:1421 in cd29d334a3 outdated

1418+            switch (rv.value()) {
1419+            case ChainstateLoadingError::ERROR_LOADING_BLOCK_DB:
1420+                strLoadError = _("Error loading block database");
1421+                break;
1422+            case ChainstateLoadingError::ERROR_BAD_GENESIS_BLOCK:
1423+                return false;  // bail immediately!

ryanofsky commented at 12:43 pm on November 5, 2021:

In commit “init/chainstate: Decouple from stringy errors” (cd29d334a38fdaf0bbbe810a9cbdeeffad394daa)

This seems to be losing the “Incorrect or no genesis block found. Wrong datadir for network?” error string. Would be good to fix or say in the commit message if this is intentional.

dongcarl commented at 6:16 pm on November 15, 2021:

Fixed as of 6736e24a490d756e0e1e70b9e212775cf305ebe6

in src/init/chainstate.cpp:27 in 7dd898c0df outdated

22+                                                             bool block_tree_db_in_memory,
23+                                                             bool coins_db_in_memory,
24+                                                             std::function<int64_t()> get_unix_time_seconds,
25+                                                             std::optional<std::function<bool()>> shutdown_requested,
26+                                                             std::optional<std::function<void()>> coins_error_cb,
27+                                                             std::optional<std::function<void()>> verifying_blocks_cb) {

jnewbery commented at 12:54 pm on November 5, 2021:

0                                                             std::optional<std::function<void()>> verifying_blocks_cb)
1{

dongcarl commented at 6:14 pm on November 15, 2021:

Fixed as of 6736e24a490d756e0e1e70b9e212775cf305ebe6

in src/init/chainstate.cpp:15 in cd29d334a3 outdated

25-                            int64_t nBlockTreeDBCache,
26-                            int64_t nCoinDBCache,
27-                            int64_t nCoinCacheUsage,
28-                            unsigned int check_blocks,
29-                            unsigned int check_level) {
30+std::optional<ChainstateLoadingError> LoadChainstateSequence(bool fReset,

ryanofsky commented at 12:55 pm on November 5, 2021:

In commit “init/chainstate: Decouple from stringy errors” (cd29d334a38fdaf0bbbe810a9cbdeeffad394daa)

 01. Success
 12. Shutdown requested
 2 - nothing failed but a shutdown was triggered in the middle of the
 3   sequence
 43. Soft failure
 5 - a failure that might be recovered from with a reindex
 64. Hard failure
 7 - a failure that definitively cannot be recovered from with a reindex
 8
 9Previously, LoadChainstateSequence returns a bool which:
10   - if false
11       - Definitely a "Hard failure"
12   - if true
13       - if fLoaded -> "Success"
14       - if ShutdownRequested() -> "Shutdown requested"
15       - else -> "Soft failure"
16
17After this change, LoadChainstateSequence returns a
18std::optional<ChainstateLoadingError> which:
19   - if has_value()
20       - Either "Soft failure" or "Hard failure", caller decides what
21         to do based on the specific ChainstateLoadingError enum
22         member
23   - else
24       - Either "Success" or "Shutdown requested", caller checks
25         ShutdownRequested() to determine which.

Again I think this information should be removed from the commit message and moved here to the function declaration or the chainstate.cpp file as documentation for the LoadChainstateSequence function. That way instead of having manually compare these two piece of documentation, the differences can be seen with standard diff tools. This will leave the code better documented, let documentation change in sync with the code, and let the commit message succinctly describe the changes in behavior without having to describe the full behavior.

in src/Makefile.am:553 in 7dd898c0df outdated

548@@ -547,6 +549,8 @@ libbitcoin_common_a_SOURCES = \
549   core_write.cpp \
550   deploymentinfo.cpp \
551   external_signer.cpp \
552+  init/caches.cpp \
553+  init/chainstate.cpp \

jnewbery commented at 1:23 pm on November 5, 2021:

I would have guessed that this would be part of libbitcoin_server, since loading a chainstate is something that only bitcoind or bitcoin-qt would do.

dongcarl commented at 6:14 pm on November 15, 2021:

Fixed as of 6736e24a490d756e0e1e70b9e212775cf305ebe6

in src/init/chainstate.h:32 in 7dd898c0df outdated

27+    ERROR_BLOCKS_WITNESS_INSUFFICIENTLY_VALIDATED,
28+    ERROR_BLOCK_FROM_FUTURE,
29+    ERROR_CORRUPTED_BLOCK_DB,
30+};
31+
32+std::optional<ChainstateLoadingError> LoadChainstateSequence(bool fReset,

jnewbery commented at 1:29 pm on November 5, 2021:

It’d be great if there was a final commit that added a doxygen comment for all of these parameters.

in src/init.cpp:1424 in 7dd898c0df outdated

1605-                    }
1606-                }
1607-            } catch (const std::exception& e) {
1608-                LogPrintf("%s\n", e.what());
1609+            case ChainstateLoadingError::ERROR_BAD_GENESIS_BLOCK:
1610+                return false;  // bail immediately!

jnewbery commented at 2:15 pm on November 5, 2021:

I don’t really like the use of “bail” here or in the commit log. It feels a bit colloquial and imprecise.

dongcarl commented at 6:14 pm on November 15, 2021:

Fixed as of 6736e24a490d756e0e1e70b9e212775cf305ebe6, just copied over the existing comment which mostly describes error handling anyway

in src/init.cpp:1456 in 7dd898c0df outdated

1644-            if (!failed_verification) {
1645-                fLoaded = true;
1646-                LogPrintf(" block index %15dms\n", GetTimeMillis() - load_block_index_start_time);
1647-            }
1648-        } while(false);
1649+        } else if (!ShutdownRequested()) {

jnewbery commented at 2:21 pm on November 5, 2021:

What do you think about adding ChainstateLoadingError::SHUTDOWN_REQUESTED as an error reason, and then bringing the shutdown requested logic up into the switch statement above? I think that’d make the logic flow a bit easier to understand.

dongcarl commented at 6:14 pm on November 15, 2021:

Fixed as of 6736e24a490d756e0e1e70b9e212775cf305ebe6

in src/init.cpp:1432 in 7dd898c0df outdated

1553+            LogPrintf("%s\n", e.what()); // XXX
1554+            rv = ChainstateLoadingError::ERROR_GENERIC_BLOCKDB_OPEN_FAILED;
1555+        }
1556+        if (rv.has_value()) {
1557+            switch (rv.value()) {
1558+            case ChainstateLoadingError::ERROR_LOADING_BLOCK_DB:

jnewbery commented at 2:29 pm on November 5, 2021:

I don’t like how the error strings have been separated from the error logic that they’re describing. What do you think about returning an optional std::pair or struct from LoadChainstateSequence() that contains both the enum and error string? That’d simplify this switch statement (all the “soft failure” cases could share the same statement, and there’d be one separate statement for the “hard failure”)

theuni commented at 7:50 pm on November 5, 2021:

Hmm, I rather like the separation of logic. How about a lookup function rather than marrying them? Something like https://github.com/bitcoin/bitcoin/blob/master/src/script/script_error.cpp#L10

jamesob commented at 3:30 pm on November 9, 2021:

I agree with @theuni but in any case this feels like bikeshedding to me. What’s written as-is is a clear improvement and refining this further should be optional or follow-up IMO.

jnewbery commented at 3:37 pm on November 9, 2021:

How about a lookup function rather than marrying them? Something like master/src/script/script_error.cpp#L10

I agree that this would be an improvement, separating the error code:string lookup from the recovery logic.

ryanofsky commented at 5:31 pm on November 12, 2021:

In commit “init/chainstate: Decouple from stringy errors” (cd29d334a38fdaf0bbbe810a9cbdeeffad394daa)

re: #23280 (review)

I agree with John’s original comment, and do not think fine grained error codes are good or that some kind of external error code -> string indirection would be good. I think the ideal thing would be for this to return a straightforward SUCCESS / FAILURE / INTERRUPTED status, with a detailed error string or list of error strings in the case of failure. The problem with fine grained return codes is:

They are maintenance burden making code more verbose than it needs to be and hard to spot inconsistencies easier to introduce.
They are unergnomic for callers (important if this is supposed evolve into a libbitcoin kernel API) because it’s more work for callers to figure out how to handle a long list error codes correctly than a small set of cases.
They are worse for user experience because it’s not possible to include specific information about errors that may be helpful like filenames, timestamps, hashes.

in src/init.cpp:1463 in 7dd898c0df outdated

1449-
1450-                // If the loaded chain has a wrong genesis, bail out immediately
1451-                // (we're likely using a testnet datadir, or the other way around).
1452-                if (!chainman.BlockIndex().empty() &&
1453-                        !chainman.m_blockman.LookupBlockIndex(chainparams.GetConsensus().hashGenesisBlock)) {
1454-                    return InitError(_("Incorrect or no genesis block found. Wrong datadir for network?"));

jnewbery commented at 2:33 pm on November 5, 2021:

It looks like this init error string has been lost. I think that previously this would have been displayed to the user.

dongcarl commented at 6:15 pm on November 15, 2021:

Fixed as of 6736e24a490d756e0e1e70b9e212775cf305ebe6

in src/init/caches.h:20 in 7dd898c0df outdated

15+    int64_t coin_db_cache_size;
16+    int64_t coin_cache_usage_size;
17+    int64_t tx_index_cache_size;
18+    int64_t filter_index_cache_size;
19+};
20+void CalculateCacheSizes(const ArgsManager& args, CacheSizes& cache_sizes, size_t n_indexes = 0);

jnewbery commented at 3:35 pm on November 5, 2021:

Again, it’d be nice to have a doxygen comment here. It’s not immediately obvious what n_indexes represents.

Is there any reason that you made cache_sizes an out-param rather than the return value for this function?

Since this is a new function and you’re adding all the call sites, can we make n_indexes a required argument?

jamesob commented at 3:33 pm on November 9, 2021:

Is there any reason that you made cache_sizes an out-param rather than the return value for this function?

#23280 (review)

dongcarl commented at 6:25 pm on November 15, 2021:

In my latest push (6736e24a490d756e0e1e70b9e212775cf305ebe6), I ended up having CalculateCacheSizes return a CacheSize, which is perhaps a bit more readable.

in src/init/caches.h:14 in 7dd898c0df outdated

 9+#include <cstdint> // for int64_t
10+
11+class ArgsManager;
12+
13+struct CacheSizes {
14+    int64_t block_tree_db_cache_size;

jnewbery commented at 3:36 pm on November 5, 2021:

The _cache_size parts to all these member names seems redundant with the fact they’re members of a struct called CacheSizes

dongcarl commented at 6:15 pm on November 15, 2021:

Fixed as of 6736e24a490d756e0e1e70b9e212775cf305ebe6

in src/init.cpp:1403 in 7dd898c0df outdated

1536+                                        cache_sizes.coin_db_cache_size,
1537+                                        cache_sizes.coin_cache_usage_size,
1538+                                        args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS),
1539+                                        args.GetIntArg("-checklevel", DEFAULT_CHECKLEVEL),
1540+                                        false,
1541+                                        false,

jnewbery commented at 3:40 pm on November 5, 2021:

Can you add inline comments for these arguments (that can be checked by a clang-tidy):

0                                        /*block_tree_db_in_memory=*/false,
1                                        /*coins_db_in_memory=*/false,

jnewbery commented at 3:45 pm on November 5, 2021: member

Concept ACK.

A few questions/observations:

1 Several of the commit logs reference “ACCS” or “ACSS”. Sorry if I’m being dense, but what are those?

2 The commit log for init/chainstate: Decouple from GetTimeMillis reads “…instead require caller to pass in a std::function that returns the current system time in milliseconds as a int64_t”, but that doesn’t match what the code does. Perhaps this is left over from an old version of this branch?

3 Can you explain the benefit of commit init/chainstate: Reduce coupling of LogPrintf? It doesn’t seem a problem to me for LoadChainstateSequence() to call the global logger. Lots of other parts of the codebase do that already. Having the try/catch inside LoadChainstateSequence() means that it’s noexcept and the caller doesn’t need to worry about catching exceptions.

4 In validation: Call NotifyBlockTip in CCS::LoadChainTip, you’re moving the RPC notification into CChainstate::LoadChainTip(). However, that function is also called by ChainstateManager::ActivateSnapshot(), so that’s potentially a behavior change there. You also change the call from RPCNotifyBlockChange() to uiInterface.NotifyBlockTip(). However, uiInterface.NotifyBlockTip has multiple other “slots”, e.g. BlockNotifyGenesisWait. Are we sure that isn’t also a behavior change?

I think that the RPCNotifyBlockChange() call can just be brought down to immediately before RPC leaves warmup:

https://github.com/bitcoin/bitcoin/blob/77a2f5d30c5ecb764b8a7c098492e1f5cdec90f0/src/init.cpp#L1842

There can’t be any pending RPC calls waiting for the blockchange notification (since RPC is still in warmup) so I think the only purpose of this call is to set the latestblock fields.

5 In init/chainstate: Decouple from GetAdjustedTime, why not just pass in the time instead of a function that returns the time?

in src/init/chainstate.cpp:30 in 7dd898c0df outdated

28+    auto is_coinsview_empty = [&](CChainState* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
29+        return fReset || fReindexChainState || chainstate->CoinsTip().GetBestBlock().IsNull();
30+    };
31+
32+    {
33+        LOCK(cs_main);

jnewbery commented at 4:28 pm on November 5, 2021:

This function now consists of three code blocks that each lock cs_main. What do you think about just taking cs_main at the top of the function and holding it throughout?

dongcarl commented at 6:15 pm on November 15, 2021:

Fixed as of 6736e24a490d756e0e1e70b9e212775cf305ebe6

in src/init/chainstate.h:5 in 83ed7db27b outdated

0@@ -0,0 +1,30 @@
1+// Copyright (c) 2021 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#ifndef BITCOIN_INIT_CHAINSTATE_H

ryanofsky commented at 5:36 pm on November 5, 2021:

In commit “init: Extract chainstate loading sequence” (83ed7db27b4c4798666b77d83fc8e7c3eab8c621)

I think the new files here chainstate.h, chainstate.cpp, caches.h, caches.cpp all belong in src/node/ not src/init/.

I maybe I should have given the src/init/ directory a more descriptive name like src/process_init/, but I was intending it to hold common code that various bitcoin executables: bitcoind, bitcoin-qt, bitcoin-node, bitcoin-wallet, and bitcoin-gui all generically need to start up, and to be place for them to customize their individual behavior by overriding interfaces::Init methods.

The new code being added in this PR is all node specific, and would be at home in src/node/. The new code is also not really related to any existing src/init/ code except for being called during startup, and it would be unusual in this case to organize source code based on when it is called, instead of what it does.

I think it should be possible to rename these files pretty painlessly in a rebase and git should be smart enough detect it and not create conflicts. Otherwise a handy command for a git rename over a series of commits is:

0git filter-branch --index-filter 'git mv <old name> <new name>' <base>..<head>

dongcarl commented at 7:00 pm on November 11, 2021:

Ah I see! What do you think about putting the files under src/node/init? And I supposed everything would be under the node::init namespace?

ryanofsky commented at 9:08 pm on November 11, 2021:

Ah I see! What do you think about putting the files under src/node/init? And I supposed everything would be under the node::init namespace?

Probably best to use your own judgement, and other people may have opinions, but IMO just moving things into top level, node, wallet, util directories is ideal for seeing where things come from and making it obvious when you may be doing something bad like calling wallet code from node code. Extra nesting may be good, but I think only if there is some specific justification for it. In this case, I think having an “init” module is not good, because it will encourage people to add startup code from disparate parts of code there instead of closer to the actual database/index/connection structures that are being initialized.

I also think it’s nice when there’s a 1:1 correspondence between directories and namespaces and personally would not suggest extra nesting for namespaces either.

ryanofsky commented at 5:42 pm on November 12, 2021:

re: #23280 (review)

FWIW, followed up with more node:: namespace in #23497

in src/init/chainstate.h:48 in 7dd898c0df outdated

43+                                                             bool block_tree_db_in_memory,
44+                                                             bool coins_db_in_memory,
45+                                                             std::function<int64_t()> get_unix_time_seconds,
46+                                                             std::optional<std::function<bool()>> shutdown_requested = std::nullopt,
47+                                                             std::optional<std::function<void()>> coins_error_cb = std::nullopt,
48+                                                             std::optional<std::function<void()>> verifying_blocks_cb = std::nullopt);

ryanofsky commented at 6:49 pm on November 5, 2021:

In commit “init/chainstate: Add options for in-memory DBs” (f45b885db6636e135d617d915e0ef4ec9e1be8e6)

I think the LoadChainstateSequence function already has so many arguments that if we’re going to add optional ones they should really be consolidated in an options struct like:

0struct LoadChainStatesOptions {
1    std::optional<std::function<bool()>> shutdown_requested_cb;
2    std::optional<std::function<void()>> coins_error_cb;
3    std::optional<std::function<void()>> verifying_blocks_cb;
4};

If it’s possible to eliminate other function arguments that have sensible default values and move them to the options struct as well, that would be great:

0    bool block_tree_db_in_memory = false;
1    bool coins_db_in_memory = false;

in src/init/chainstate.cpp:21 in 83ed7db27b outdated

16+#include <validation.h> // for a lot of things
17+
18+bool LoadChainstateSequence(bool& fLoaded,
19+                            bilingual_str& strLoadError,
20+                            bool fReset,
21+                            CClientUIInterface& uiInterface,

theuni commented at 7:24 pm on November 5, 2021:

In commit “init: Extract chainstate loading sequence”

Could you please use a new variable here rather than shadowing the global? I was very confused about what changed that required AddReadErrCallback([](...)) -> AddReadErrCallback([&](...))

ryanofsky commented at 5:14 pm on November 12, 2021:

re: #23280 (review)

In commit “init: Extract chainstate loading sequence”

Could you please use a new variable here rather than shadowing the global? I was very confused about what changed that required AddReadErrCallback([](...)) -> AddReadErrCallback([&](...))

IMO ideal thing to do here so this commit remains moveonly and easy to review and doesn’t require thinking about renames or new variables or adding [&], is just to keep uiInterface a global instead of a parameter here, since it is already going away later anyway in 83ed7db27b4c4798666b77d83fc8e7c3eab8c621.

dongcarl commented at 6:17 pm on November 15, 2021:

Fixed as of 6736e24a490d756e0e1e70b9e212775cf305ebe6, thanks Russ for the neat idea!

ryanofsky commented at 7:43 pm on November 5, 2021: member

This looks very promising! Mostly reviewed this

83ed7db27b4c4798666b77d83fc8e7c3eab8c621 init: Extract chainstate loading sequence (1/17)
b094ca4fd89b50c7179355f72113d794725f8a9d init/chainstate: Decouple from ArgsManager (2/17)
3ecaf812d4934d63124b640d2aeb008d4b742d89 init/chainstate: Decouple from concept of NodeContext (3/17)
e53e85f8b4ff7642c876950942cadcc819aa50e1 init/chainstate: Decouple from GetTimeMillis (4/17)
cd29d334a38fdaf0bbbe810a9cbdeeffad394daa init/chainstate: Decouple from stringy errors (5/17)
25cea87e6ea4a7cf59f38df1454265b2de8461c9 init/chainstate: Remove do/while loop (6/17)
0e61af06834c9a9331e620343e60e9104e08331e init/chainstate: Decouple from concept of uiInterface (7/17)
38d88e2950afc99caca31388fd75f03c4d06bcc9 init/chainstate: Reduce coupling of LogPrintf (8/17)
10564583429762ddfb6601d1f64ac663406bf8e2 init/chainstate: Move -checkblocks limitation to help (9/17)
b229024932ebd4775070149ff0f27d0fd329bf0a validation: Call NotifyBlockTip in CCS::LoadChainTip (10/17)
8eae069b0015524d877a6fc9f92ab85d2ad6b26b init/chainstate: Decouple from GetAdjustedTime (11/17)
855a86102095003779d63b514728d1f0ed4d3a91 init/chainstate: Decouple from ShutdownRequested (12/17)
391fe919a8321ed8ff9af990797b2cc737968b78 validation: VerifyDB only needs Consensus::Params (13/17)
1f966b8d9fa41073a27a475715cf984b227197de init/caches: Extract cache calculation logic (14/17)
f45b885db6636e135d617d915e0ef4ec9e1be8e6 init/chainstate: Add options for in-memory DBs (15/17)
8e2817a66fb9c00b4392256c700c35380d7f7eb3 test/setup: Use LoadChainstateSequence (16/17)
7dd898c0df678859ca92773b6990e4e28a83ab24 test/setup: Unify m_args and gArgs (17/17)

in src/init/chainstate.cpp:39 in 3ecaf812d4 outdated

35@@ -37,11 +36,11 @@ bool LoadChainstateSequence(bool& fLoaded,
36         const int64_t load_block_index_start_time = GetTimeMillis();
37         try {
38             LOCK(cs_main);
39-            chainman.InitializeChainstate(Assert(node.mempool.get()));
40+            chainman.InitializeChainstate(mempool);

theuni commented at 7:43 pm on November 5, 2021:

In commit “init/chainstate: Decouple from concept of NodeContext”

I think it makes sense to split this into two commits, one for the decoupling and one for the nullable mempool. I think that’s a significant thing to signal for review.

Some justification for why this is safe (and maybe why it wasn’t this way before) would be helpful context for review as well.

dongcarl commented at 6:17 pm on November 15, 2021:

Fixed as of 6736e24a490d756e0e1e70b9e212775cf305ebe6

in src/init.cpp:1406 in e53e85f8b4 outdated

1399@@ -1400,6 +1400,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1400 
1401         uiInterface.InitMessage(_("Loading block index…").translated);
1402 
1403+        const int64_t load_block_index_start_time = GetTimeMillis();

theuni commented at 7:47 pm on November 5, 2021:

In commit “init/chainstate: Decouple from GetTimeMillis”

The commit message mentions passing in a callback for the current time, but that’s not what’s happening here. Fingers crossed that’s not what happens in the next commit, because it sounds ugly :p

ryanofsky commented at 8:54 pm on November 30, 2021:

In commit “node/chainstate: Decouple from GetTimeMillis” (165b30b649923da4ed19749a399c4a4589da326b)

Commit description seems out of date. This commit is not passing a std::function to replace GetTimeMillis, it is just moving the GetTimeMillis call

in src/init/chainstate.cpp:124 in cd29d334a3 outdated

129-            }
130         } catch (const std::exception& e) {
131-            LogPrintf("%s\n", e.what());
132-            strLoadError = _("Error opening block database");
133-            break;
134+            LogPrintf("%s\n", e.what()); // XXX

theuni commented at 8:20 pm on November 5, 2021:

why why why

dongcarl commented at 6:17 pm on November 15, 2021:

Fixed as of 6736e24a490d756e0e1e70b9e212775cf305ebe6

in src/init/chainstate.cpp:90 in 0e61af0683 outdated

94-                uiInterface.ThreadSafeMessageBox(
95-                    _("Error reading from database, shutting down."),
96-                    "", CClientUIInterface::MSG_ERROR);
97-            });
98+            if (coins_error_cb.has_value()) {
99+                chainstate->CoinsErrorCatcher().AddReadErrCallback(coins_error_cb.value());

theuni commented at 8:32 pm on November 5, 2021:

In commit “init/chainstate: Decouple from concept of uiInterface”

Hmm, this seems strange. Before, it printed “Error reading from database, shutting down”. Was that innacurate?

Why not instead return an error type and let the caller print the message and shutdown?

dongcarl commented at 6:18 pm on November 15, 2021:

Not inaccurate, but I think we’re adding a callback here and can’t return immediately?

in src/init/chainstate.cpp:139 in 0e61af0683 outdated

135@@ -138,7 +136,9 @@ std::optional<ChainstateLoadingError> LoadChainstateSequence(bool fReset,
136 
137         for (CChainState* chainstate : chainman.GetAll()) {
138             if (!is_coinsview_empty(chainstate)) {
139-                uiInterface.InitMessage(_("Verifying blocks…").translated);
140+                if (verifying_blocks_cb.has_value()) {

theuni commented at 8:41 pm on November 5, 2021:

Might it make sense to split this function in two rather than requiring a callback in the middle? LoadChainstateSequence / VerifyChainState or so?

Needing to tell that we’ve moved on from one thing to another is a hint to me that maybe those things should be separate functions :)

dongcarl commented at 6:19 pm on November 15, 2021:

Fixed as of 6736e24a490d756e0e1e70b9e212775cf305ebe6

in src/init.cpp:1408 in 38d88e2950 outdated

1418-                                         },
1419-                                         []() {
1420-                                             uiInterface.InitMessage(_("Verifying blocks…").translated);
1421-                                         });
1422+        std::optional<ChainstateLoadingError> rv;
1423+        try {

theuni commented at 8:50 pm on November 5, 2021:

In commit “init/chainstate: Reduce coupling of LogPrintf”

Grouping the two try/catch looks safe to me, but I think it’d be easier to split this into two commits: one to combine, and one to move it out to init. That way the second is pretty much move-only.

Or if you end up going the LoadChainstateSequence/VerifyChainState route, this logic doesn’t need to change.

dongcarl commented at 6:19 pm on November 15, 2021:

Fixed as of 6736e24a490d756e0e1e70b9e212775cf305ebe6

in src/init/chainstate.cpp:139 in 1056458342 outdated

135@@ -136,10 +136,6 @@ std::optional<ChainstateLoadingError> LoadChainstateSequence(bool fReset,
136                 if (verifying_blocks_cb.has_value()) {
137                     verifying_blocks_cb.value()();
138                 }
139-                if (fHavePruned && check_blocks > MIN_BLOCKS_TO_KEEP) {

theuni commented at 8:55 pm on November 5, 2021:

I belive this change also wouldn’t be necessary with this function split in half.

dongcarl commented at 6:20 pm on November 15, 2021:

Fixed as of 6736e24a490d756e0e1e70b9e212775cf305ebe6

in src/init/chainstate.cpp:147 in 8eae069b00 outdated

136@@ -137,7 +137,7 @@ std::optional<ChainstateLoadingError> LoadChainstateSequence(bool fReset,
137                 }
138 
139                 const CBlockIndex* tip = chainstate->m_chain.Tip();
140-                if (tip && tip->nTime > GetAdjustedTime() + 2 * 60 * 60) {
141+                if (tip && tip->nTime > get_unix_time_seconds() + 2 * 60 * 60) {

theuni commented at 9:04 pm on November 5, 2021:

In commit “init/chainstate: Decouple from GetAdjustedTime”

I suppose we can’t just pass a timeout time in here because the current logic includes the runtime of the loading/checking itself?

This seems like yet something else that would be unnecessary if this function were split. That keeps coming up as a potential alternative, but I’m unsure if there’s something that would prevent it, so I’ll stop suggesting it.

in src/init/chainstate.cpp:23 in 855a861020 outdated

19@@ -21,6 +20,7 @@ std::optional<ChainstateLoadingError> LoadChainstateSequence(bool fReset,
20                                                              unsigned int check_blocks,
21                                                              unsigned int check_level,
22                                                              std::function<int64_t()> get_unix_time_seconds,
23+                                                             std::optional<std::function<bool()>> shutdown_requested,

theuni commented at 9:08 pm on November 5, 2021:

In commit “init/chainstate: Decouple from ShutdownRequested”

Without actually trying it.. I think it would be more straightforward here to use a CThreadInterrupt which gets triggered for this.

theuni commented at 9:16 pm on November 5, 2021: member

I like this. LoadChainstateSequence is still quite wonky, but at least it’s better defined. Nice to have that really weird loop removed too.

I’m curious to see what you think about splitting it up. I only reviewed “Decouple from stringy errors” and “Extract cache calculation logic” lightly. They look good but I’d want to scrutinize those two for edge-cases before I could be confident that they’re no-ops.

ryanofsky commented at 5:46 pm on November 12, 2021: member

Just posting few more comments. I do like Cory’s Load/Verify split idea, too.

dongcarl force-pushed on Nov 15, 2021

dongcarl commented at 6:45 pm on November 15, 2021: member

Thanks all for the reviews! I addressed a subset of the concerns in my latest 2 pushes. One thing I’d like some help with:

For the RPC block change notification, I decided to go with jnewbery’s method here:

In validation: Call NotifyBlockTip in CCS::LoadChainTip, you’re moving the RPC notification into CChainstate::LoadChainTip(). However, that function is also called by ChainstateManager::ActivateSnapshot(), so that’s potentially a behavior change there. You also change the call from RPCNotifyBlockChange() to uiInterface.NotifyBlockTip(). However, uiInterface.NotifyBlockTip has multiple other “slots”, e.g. BlockNotifyGenesisWait. Are we sure that isn’t also a behavior change?

I think that the RPCNotifyBlockChange() call can just be brought down to immediately before RPC leaves warmup:

https://github.com/bitcoin/bitcoin/blob/77a2f5d30c5ecb764b8a7c098492e1f5cdec90f0/src/init.cpp#L1842

There can’t be any pending RPC calls waiting for the blockchange notification (since RPC is still in warmup) so I think the only purpose of this call is to set the latestblock fields.

This change is in this commit: dea5766ccaed1db77f8ca335d913ecf1ea4d41d1. This seems quite safe to me, and answers some of the questions ryanofsky posed in #23280 (review). However, I’m not well-versed in RPC and how that interacts with our multiple chainstates. So I’d love some input on that.

jamesob commented at 0:19 am on November 18, 2021: member

reACK https://github.com/bitcoin/bitcoin/pull/23280/commits/2c8f0e948d1ffe2fc729ce51fa42544616b51b5b

I re-reviewed this from scratch (no easier way to do it) after rebasing on master to avoid the peers.dat v3/v4 startup error. Notable changes were LoadChainstate/VerifyLoadedChainstate split, import comment removal, and cs::main lock consolidation, which I think are all fine.

Edit: forgot to mention the RPCNotifyBlockChange() change. While that looks fine to me, in general I would have preferred preserving master’s behavior in this changeset (by, say, parameterizing and injecting that method as we have done with others). But the change seems fine to me; just wish it wasn’t blended in with this larger, ideally-non-behavioral change.

Rerunning my proposed functional tests (#23289) works for the most part, but there is a failure at the end when we intentionally create an init failure by removing block index *.ldb files (https://github.com/bitcoin/bitcoin/pull/23289/commits/d9803f7a0a33688f7429cf10384244f4770851ca#diff-1eb9875f38625ed8a456749c0a96ef72f7a5dc666c7c486f0b585d65680035a0R146). So I guess this change doesn’t maintain strict output backwards compatibility, but personally I don’t think that matters so much if the issue really is just what messages are shown.

 0 node0 2021-11-18T00:06:03.158933Z [init] [dbwrapper.cpp:137] [CDBWrapper] Opening LevelDB in /tmp/bitcoin_func_test_y5h3tzr2/node0/regtest/blocks/index
 1 node0 2021-11-18T00:06:03.159032Z [init] [dbwrapper.cpp:253] [HandleError] Fatal LevelDB error: Corruption: 1 missing files; e.g.: /tmp/bitcoin_func_test_y5h3tzr2/node0/regtest/blocks/index/000035.ldb
 2 node0 2021-11-18T00:06:03.159039Z [init] [dbwrapper.cpp:254] [HandleError] You can use -debug=leveldb to get more complete diagnostic messages
 3 node0 2021-11-18T00:06:03.159101Z [init] [init.cpp:1420] [AppInitMain] Fatal LevelDB error: Corruption: 1 missing files; e.g.: /tmp/bitcoin_func_test_y5h3tzr2/node0/regtest/blocks/index/000035.ldb
 4 node0 2021-11-18T00:06:03.159121Z [init] [noui.cpp:56] [noui_InitMessage] init message: Verifying blocks…
 5 test  2021-11-18T00:06:03.190000Z TestFramework.node0 (DEBUG): [node 0] bitcoind exited with status -11 during initialization
 6 test  2021-11-18T00:06:03.191000Z TestFramework (ERROR): Assertion failed
 7                                   Traceback (most recent call last):
 8                                     File "/home/james/src/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
 9                                       self.run_test()
10                                     File "/home/james/src/bitcoin/./test/functional/feature_init.py", line 163, in run_test
11                                       node.assert_start_raises_init_error(
12                                     File "/home/james/src/bitcoin/test/functional/test_framework/test_node.py", line 523, in assert_start_raises_init_error
13                                       self._raise_assertion_error(
14                                     File "/home/james/src/bitcoin/test/functional/test_framework/test_node.py", line 167, in _raise_assertion_error
15                                       raise AssertionError(self._node_msg(msg))
16                                   AssertionError: [node 0] Expected message "Error opening block database." does not partially match stderr:
17                                   ""

For what it’s worth, the failure is due to this change: https://github.com/bitcoin/bitcoin/pull/23280/commits/18be9fec4967327158ed05f4c3e2531178ba3f86#diff-1767ee60ee6478aaca24348d0fdc0ccdfcf82c35b4543823d9ef39950b50be14L161

Process note

I’d request that we scope future reviews to matters of correctness (vs. style) and leave the latter for follow-up in smaller PRs. ACK invalidation on this PR is painful, and as-written it’s a very substantial improvement (e.g. unittest init reuse) over what we have now.

theuni commented at 2:07 pm on November 18, 2021: member

reACK 2c8f0e9

I re-reviewed this from scratch (no easier way to do it)

Blah, but thanks for confirming the best approach :)

Thanks @jamesob for another thorough review. I’ll re-review from scratch soon as well.

ryanofsky commented at 2:55 pm on November 18, 2021: member

The main thing I’m waiting for is for the new code to be moved out of src/init/ which is for generic process initialization to src/node/ which is for libbitcoin_server node code. Should be easy to do this with a plain git rebase, or in the worst case with git filter-branch mv (https://github.com/bitcoin/bitcoin/pull/23280#discussion_r743858865) if git rename detection chokes.

It would also be nice if LoadChainstate documentation was moved out of commit messages into a code comment for the LoadChainstate declaration so the PR would be easier to understand and review (https://github.com/bitcoin/bitcoin/pull/23280#discussion_r743133511, #23280 (review))

dongcarl force-pushed on Nov 29, 2021

dongcarl commented at 9:45 pm on November 29, 2021: member

Pushed 2c8f0e948d1ffe2fc729ce51fa42544616b51b5b -> fdc07a1d2916a85add9b11130a8c2cf6c02a09eb

Moved some LoadChainstate documentation from commit messages to actual code as suggested by ryanofsky (actually a great idea and made things clearer)
Moved the newly added files to node/ instead of init/, fixed up alphabetical ordering where appropriate
Added some comments about the nuance of the SHUTDOWN_REQUESTED/PROBED enum return value

w/re point number 3, although I’ve implemented the reviewers’ suggestion (returning an enum variant), I think this solution may actually be a bit more complicated than my original one. This is mostly because we can only guarantee that we detect shutdown requests up to our last call to ShutdownRequested, and there may be indirect shutdown requests after that that we won’t catch. So in order to maintain behavior / be correct we still need to check ShutdownRequested() after we exit the sequence, and the enum return value is rendered somewhat useless: https://github.com/bitcoin/bitcoin/pull/23280/commits/64195bf2e4b8c6f74d0df7ba85a1ee0f0b6c4e5f#diff-b1e19192258d83199d8adaa5ac31f067af98f63554bfdd679bd8e8073815e69dR1450-R1451

ryanofsky commented at 10:26 pm on November 29, 2021: member

Thanks for updates! Will look more, but I don’t think the fact that code might need to call ShutdownRequested() later makes having distinct SUCCESS / FAILURE / INTERRUPTED return codes useless. If calling code wants to treat SUCCESS and INTERRUPTED values the same way (by not printing errors, and going on to check whether a shutdown is currently requested), that’s great, but it doesn’t suggest to me that SUCCESS and INTERRUPTED should be merged together. SUCCESS and INTERRUPTED do actually mean different things, and it also would be reasonable for calling code to treat them differently, even if it isn’t now, for example by logging the interruption when INTERRUPTED is returned, or by returning INTERRUPTED to the caller of the caller, and moving Shutdown handling to a higher level.

jamesob commented at 7:10 pm on November 30, 2021: member

ACK fdc07a1d2916a85add9b11130a8c2cf6c02a09eb (jamesob/ackr/23280.5.dongcarl.init_coalesce_chainstate)

Examined interdiff, built, merged & tested recent master and my init functional tests. Changes are as Carl describes.

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK fdc07a1d2916a85add9b11130a8c2cf6c02a09eb ([`jamesob/ackr/23280.5.dongcarl.init_coalesce_chainstate`](https://github.com/jamesob/bitcoin/tree/ackr/23280.5.dongcarl.init_coalesce_chainstate))
 4
 5Examined interdiff, built, merged & tested recent master and my init functional tests. Changes are as Carl describes.
 6-----BEGIN PGP SIGNATURE-----
 7
 8iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmGmduoACgkQepNdrbLE
 9TwUqWRAAmizHsXiSXi+daWs5gX85JJUb/uetyo2r2qoNFsJh2YDCkrYj3VS/EM5D
10JzyscNQKRWSKEhGH7dCP8B1SwK8/7xpcTxw2orOw5Zy4QrbQoZzUy3zKZiV7mrwq
11eiWeUlHq2qdruayNIwuWK0JsCDLkPc24jTmsi9Jr+OtvhHDVqfVWz5hRMzP590JX
12lDpi92cp7EauuLvrort41uir71WXt7id/n1lusqMRwkvjYDhgAFNG3PS8ooO+kNy
13bJRObP55GOizEWoIQ08mIrkbt0wq2ALiQEUFmTvg0ZCFFfuMk4brRrQFoYQwyqc0
14RfQrIfaTblLOefRycdDY+wbmNhEhS0ATmeBFrbikzzEcm5Zd9vaV4IaoD5cBsy5e
15iKA8mQPX5IW1QC0hHxIEm7bJmJ+5dpwJbqxjm9KzVekw/nR/OjHpPIWj5rGG6xFV
16z7Iub6XSjY6w9h/3WSf3hEyTeCnq3kZ5aCUe40tOps1h4jXho70Qtu24l3zA9oza
1795dozmg6G2W6JyGl7nPGLoZE27++z2nkBkScs6zOQL8f16uCvXT5FIIM7xt9/uKg
18voCeCFE4b3dfSSC4Cce1fx4bt4oGIFfhz6V4nlHcXJdRxaaAs+wcepD8uIR+gdZZ
196Q0oWUrYIQHAi/+ZUqDax2YvNcuZJ6S9fewuS2tQIt+BZG88pMc=
20=N1CN
21-----END PGP SIGNATURE-----

0Tested on Linux-5.10.0-9-amd64-x86_64-with-glibc2.28
1
2Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon --enable-natpmp-default CXX=/usr/bin/clang++ CC=/usr/bin/clang PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --disable-openssl-tests --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --no-create --no-recursion
3
4Compiled with /usr/bin/ccache /usr/bin/clang++ -std=c++17 -mavx -mavx2 -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=.  -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4 -msha -msse4.1 -msse4.2  i
5
6Compiler version: Debian clang version 13.0.1-++20211110052940+162f3f18c945-1~exp1~20211110053502.25

MarcoFalke commented at 7:24 pm on November 30, 2021: member

C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\node\caches.cpp(32,9): error C7555: use of designated initializers requires at least '/std:c++20' [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\libbitcoin_server\libbitcoin_server.vcxproj]

laanwj referenced this in commit aef8c7cf82 on Nov 30, 2021

dongcarl force-pushed on Nov 30, 2021

dongcarl commented at 9:50 pm on November 30, 2021: member

Did not realize designated initializers is a GNU extension, my bad.

in src/init.cpp:1412 in 019da3dc84 outdated

1407@@ -1408,9 +1408,14 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1408                                  fReindexChainState,
1409                                  nBlockTreeDBCache,
1410                                  nCoinDBCache,
1411-                                 nCoinCacheUsage,
1412-                                 args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS),
1413-                                 args.GetIntArg("-checklevel", DEFAULT_CHECKLEVEL));
1414+                                 nCoinCacheUsage);
1415+        auto rv2 = VerifyLoadedChainstate(chainman,

ryanofsky commented at 10:36 pm on November 30, 2021:

In commit “Split off VerifyLoadedChainstate” (019da3dc8493054b456222ef23761dd95130b6a7)

Seems like this only be called if rv is nullopt, and it should be skipped if LoadChainstate fails or is interrupted for shutdown. Otherwise this is running verification code that did not run previously.

Would be better to preserve behavior I think. Either way it would also be good to say in commit description whether behavior is changing or not.

in src/init.cpp:1418 in efe9bd2876 outdated

1414+                                     uiInterface.ThreadSafeMessageBox(
1415+                                         _("Error reading from database, shutting down."),
1416+                                         "", CClientUIInterface::MSG_ERROR);
1417+                                 });
1418+
1419+        uiInterface.InitMessage(_("Verifying blocks…").translated);

ryanofsky commented at 10:43 pm on November 30, 2021:

In commit “node/chainstate: Decouple from concept of uiInterface” (efe9bd287656925a3bda50a809edeabe9824e37a)

Seems like there is a minor change in behavior with the verifying blocks message being shown even if is_coinsview_empty is true. This seems ok, but would be good to mention whether behavior change is intentional in commit message.

ryanofsky commented at 10:57 pm on November 30, 2021: member

Mostly reviewed (will update list below with progress).

6345e65d3163f4f5b4545181813b8e8a5ea5a157 node: Extract chainstate loading sequence (1/22)
165b30b649923da4ed19749a399c4a4589da326b node/chainstate: Decouple from GetTimeMillis (2/22)
64195bf2e4b8c6f74d0df7ba85a1ee0f0b6c4e5f node/chainstate: Decouple from stringy errors (3/22)
bb8245bedc18125309ca85e95dfc91eb7c4201bf node/chainstate: Decouple from ArgsManager (4/22)
aa6bc732b148e5e27bfc1df23f42c6c55006185c node/chainstate: Decouple from concept of NodeContext (5/22)
0d2f1a458e5ad8cfbbdc039084ce554daaf41ba9 Move mempool nullptr Assert out of LoadChainstate (6/22)
49d2d11aa5dbe13cfcbb11605b2f60e62f4e848b Move init logistics message for BAD_GENESIS_BLOCK to init.cpp (7/22)
0b340cd58693d5c4956dbf1eda8f81cf0944b02b node/chainstate: Remove do/while loop (8/22)
019da3dc8493054b456222ef23761dd95130b6a7 Split off VerifyLoadedChainstate (9/22)
efe9bd287656925a3bda50a809edeabe9824e37a node/chainstate: Decouple from concept of uiInterface (10/22)
71c78844ee2449e2e8086857bbef57d436cd7b24 node/chainstate: Reduce coupling of LogPrintf (11/22)
02c5d48532e9eadfd4832585e827aea53af7aaf5 Move -checkblocks LogPrintf to AppInitMain (12/22)
ca13555758e6d868d3cfa5b55e679fca74381b03 init: Delay RPC block notif until warmup finished (13/22)
e073634c37f3a1e140920c6e5e3f2c1ae47cd293 node/chainstate: Decouple from GetAdjustedTime (14/22)
2a2a496fe898404876f4ed12cd61aae8600f3e47 node/chainstate: Decouple from ShutdownRequested (15/22)
7d0cf02654214e816734dffe30a466d4d12147f1 validation: VerifyDB only needs Consensus::Params (16/22)
fefd26434cc3bcac443a818903df1c31f40ecff1 node/caches: Extract cache calculation logic (17/22)
4a50ef72d7367c35c52ca8f8e584b5f402ae79df node/chainstate: Add options for in-memory DBs (18/22)
526b218273ead38e5d93198367542c4e97956fd8 test/setup: Use LoadChainstate (19/22)
8b3080f2d662207ee0af2aa48a0a6c3db3a24c56 test/setup: Unify m_args and gArgs (20/22)
97162b171e064ebf745fdee1dd3373c38b7195ed Remove all #include // for * comments (21/22)
fdc07a1d2916a85add9b11130a8c2cf6c02a09eb Collapse the 2 cs_main locks in LoadChainstate (22/22)

sidhujag referenced this in commit f5c617dc72 on Dec 1, 2021

DrahtBot added the label Needs rebase on Dec 1, 2021

MarcoFalke commented at 11:22 am on December 1, 2021: member

 0�[0;36m test  2021-11-30T22:50:44.631000Z TestFramework.node0 (DEBUG): [node 0] bitcoind exited with status -11 during initialization �[0m
 1�[0;36m test  2021-11-30T22:50:44.632000Z TestFramework (ERROR): Assertion failed �[0m
 2�[0;36m                                   Traceback (most recent call last):�[0m
 3�[0;36m                                     File "/private/var/folders/tn/f_9sf1xx5t14qm_6f83q3b840000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin19/test/functional/test_framework/test_framework.py", line 132, in main�[0m
 4�[0;36m                                       self.run_test()�[0m
 5�[0;36m                                     File "/private/var/folders/tn/f_9sf1xx5t14qm_6f83q3b840000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin19/test/functional/feature_init.py", line 168, in run_test�[0m
 6�[0;36m                                       node.assert_start_raises_init_error(�[0m
 7�[0;36m                                     File "/private/var/folders/tn/f_9sf1xx5t14qm_6f83q3b840000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin19/test/functional/test_framework/test_node.py", line 523, in assert_start_raises_init_error�[0m
 8�[0;36m                                       self._raise_assertion_error(�[0m
 9�[0;36m                                     File "/private/var/folders/tn/f_9sf1xx5t14qm_6f83q3b840000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin19/test/functional/test_framework/test_node.py", line 167, in _raise_assertion_error�[0m
10�[0;36m                                       raise AssertionError(self._node_msg(msg))�[0m
11�[0;36m                                   AssertionError: [node 0] Expected message "Error opening block database." does not partially match stderr:�[0m
12�[0;36m                                   ""�[0m

jamesob commented at 3:38 pm on December 1, 2021: member

 0�[0;36m test  2021-11-30T22:50:44.631000Z TestFramework.node0 (DEBUG): [node 0] bitcoind exited with status -11 during initialization �[0m
 1�[0;36m test  2021-11-30T22:50:44.632000Z TestFramework (ERROR): Assertion failed �[0m
 2�[0;36m                                   Traceback (most recent call last):�[0m
 3�[0;36m                                     File "/private/var/folders/tn/f_9sf1xx5t14qm_6f83q3b840000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin19/test/functional/test_framework/test_framework.py", line 132, in main�[0m
 4�[0;36m                                       self.run_test()�[0m
 5�[0;36m                                     File "/private/var/folders/tn/f_9sf1xx5t14qm_6f83q3b840000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin19/test/functional/feature_init.py", line 168, in run_test�[0m
 6�[0;36m                                       node.assert_start_raises_init_error(�[0m
 7�[0;36m                                     File "/private/var/folders/tn/f_9sf1xx5t14qm_6f83q3b840000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin19/test/functional/test_framework/test_node.py", line 523, in assert_start_raises_init_error�[0m
 8�[0;36m                                       self._raise_assertion_error(�[0m
 9�[0;36m                                     File "/private/var/folders/tn/f_9sf1xx5t14qm_6f83q3b840000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin19/test/functional/test_framework/test_node.py", line 167, in _raise_assertion_error�[0m
10�[0;36m                                       raise AssertionError(self._node_msg(msg))�[0m
11�[0;36m                                   AssertionError: [node 0] Expected message "Error opening block database." does not partially match stderr:�[0m
12�[0;36m                                   ""�[0m

Ah yeah, see my comment above (https://github.com/bitcoin/bitcoin/pull/23280#issuecomment-972365657). This should be a simple matter of tacking on a commit to this branch that updates output expectations in the functional test.

in src/init.cpp:1777 in ca13555758 outdated

1758@@ -1759,6 +1759,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1759 
1760     // ********************************************************* Step 13: finished
1761 
1762+    RPCNotifyBlockChange(chainman.ActiveTip());

ryanofsky commented at 7:55 pm on December 1, 2021:

In commit “init: Delay RPC block notif until warmup finished” (9217003327b6849d67dcd5c427e232157854997d)

Would be good to have a code comment saying why this is needed, since no block is changing here.

(Previous commit message b229024932ebd4775070149ff0f27d0fd329bf0a had some information, but I’m not sure how much of it is relevant)

dongcarl force-pushed on Dec 1, 2021

DrahtBot removed the label Needs rebase on Dec 1, 2021

in src/node/caches.cpp:37 in 79ae725d80 outdated

32+        nBlockTreeDBCache,
33+        nCoinDBCache,
34+        nCoinCacheUsage,
35+        nTxIndexCache,
36+        filter_index_cache,
37+    };

ryanofsky commented at 8:51 pm on December 1, 2021:

In commit “node/caches: Extract cache calculation logic” (79ae725d8062761a6650862fd49b7de96ce7616e)

It’s good that this is a moveonly commit, but it would be nice to avoid the intermediate variables in a followup

 0CacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
 1{
 2    int64_t nTotalCache = (args.GetIntArg("-dbcache", nDefaultDbCache) << 20);
 3    nTotalCache = std::max(nTotalCache, nMinDbCache << 20); // total cache cannot be less than nMinDbCache
 4    nTotalCache = std::min(nTotalCache, nMaxDbCache << 20); // total cache cannot be greater than nMaxDbcache
 5    CacheSizes sizes;
 6    sizes.block_tree_db = std::min(nTotalCache / 8, nMaxBlockDBCache << 20);
 7    nTotalCache -= sizes.block_tree_db;
 8    sizes.tx_index = std::min(nTotalCache / 8, args.GetBoolArg("-txindex", DEFAULT_TXINDEX) ? nMaxTxIndexCache << 20 : 0);
 9    nTotalCache -= sizes.tx_index;
10    sizes.filter_index = 0;
11    if (n_indexes > 0) {
12        int64_t max_cache = std::min(nTotalCache / 8, max_filter_index_cache << 20);
13        sizes.filter_index = max_cache / n_indexes;
14        nTotalCache -= sizes.filter_index * n_indexes;
15    }
16    sizes.coins_db = std::min(nTotalCache / 2, (nTotalCache / 4) + (1 << 23)); // use 25%-50% of the remainder for disk cache
17    sizes.coins_db = std::min(sizes.coins_db, nMaxCoinsDBCache << 20); // cap total coins db cache
18    nTotalCache -= sizes.coins_db;
19    sizes.coins = nTotalCache; // the rest goes to in-memory cache
20    return sizes;
21}

in src/test/util/setup_common.cpp:80 in 7ede150299 outdated

76@@ -77,9 +77,9 @@ std::ostream& operator<<(std::ostream& os, const uint256& num)
77 
78 BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::vector<const char*>& extra_args)
79     : m_path_root{fs::temp_directory_path() / "test_common_" PACKAGE_NAME / g_insecure_rand_ctx_temp_path.rand256().ToString()},
80-      m_args{}
81+      m_args{gArgs}

ryanofsky commented at 9:13 pm on December 1, 2021:

In commit “test/setup: Unify m_args and gArgs” (7ede150299ea46d38a0797910c95028e7e948e0d)

I think it would be better to drop this change, and let BasicTestingSetup::m_args keep being a variable instead of a gArgs reference. The reason the BasicTestingSetup::m_args member exists is to provide a way for tests to stop using a shared gArgs instance and let each test have it’s own standalone ArgsManager. This way tests can do m_args.ForceSetArg and other operations without affecting global state and potentially breaking each other.

The other change below which stops using gArgs would be good to keep if it still works:

0-    m_node.args = &gArgs;
1+    m_node.args = &m_args;

If it doesn’t work now, it should work later when @kiminuo gets around to removing more gArgs uses in other code #21244 (review)

jamesob commented at 4:31 pm on December 3, 2021:

Agree that we can (should?) leave this out provided it doesn’t make things too difficult in subsequent commits. I do like the idea of shedding use of gArgs from within this test class, but maybe that should be handled separately if there are non-obvious behavior changes as Russ is mentioning.

ryanofsky approved

ryanofsky commented at 9:29 pm on December 1, 2021: member

Code review ACK 8301c696d0a0be921ce01b6e9f74f7ff6e8c0f2f. PR is a nice improvement. A few things could be tweaked still, but I don’t think any changes are actually needed, and this looks good in its current form.

in src/node/chainstate.cpp:22 in 258ce97cf7 outdated

16@@ -18,6 +17,7 @@ std::optional<ChainstateLoadingError> LoadChainstate(bool fReset,
17                                                      int64_t nBlockTreeDBCache,
18                                                      int64_t nCoinDBCache,
19                                                      int64_t nCoinCacheUsage,
20+                                                     std::optional<std::function<bool()>> shutdown_requested,

ryanofsky commented at 9:52 pm on December 1, 2021:

In commit “node/chainstate: Decouple from ShutdownRequested” (258ce97cf72043ffa69d2b77a4438df723c734a6)

Function objects can be null so here and other places should replace optional<function<X>> with function<X> so there is one null value instead of two

ryanofsky commented at 11:24 pm on December 1, 2021: member

Following up on the return codes discussion #23280 (review), I still think it would be good to reduce the number of status codes the caller needs to handle, and number of function parameters. Experimenting a little, I pushed a change (compiles, but untested) to do this here: b7c7f64dc86f0fcdf07cb1e765f7cccc3a3c8897 (+129/-210). It could be a followup, or feel free to adopt parts of this if they seem useful.

DrahtBot added the label Needs rebase on Dec 2, 2021

in src/node/chainstate.cpp:29 in 8301c696d0 outdated

24+{
25+    auto is_coinsview_empty = [&](CChainState* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
26+        return fReset || fReindexChainState || chainstate->CoinsTip().GetBestBlock().IsNull();
27+    };
28+
29+    {

jamesob commented at 4:27 pm on December 3, 2021:

Nit for follow-up: we can probably just eliminate these braces/indent, eh?

in src/node/chainstate.cpp:141 in 8301c696d0 outdated

136+{
137+    auto is_coinsview_empty = [&](CChainState* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
138+        return fReset || fReindexChainState || chainstate->CoinsTip().GetBestBlock().IsNull();
139+    };
140+
141+    {

jamesob commented at 4:28 pm on December 3, 2021:

Nit for follow-up: same here.

in src/test/util/setup_common.cpp:196 in 8301c696d0 outdated

199+                             m_cache_sizes.block_tree_db,
200+                             m_cache_sizes.coins_db,
201+                             m_cache_sizes.coins,
202+                             true,
203+                             true);
204+    assert(!rv.has_value());

jamesob commented at 4:34 pm on December 3, 2021:

Out of curiosity and not-blocking, is there any reason we shouldn’t also call VerifyLoadedChainstate here just for the sake of coverage?

E.g. this seems to work: https://github.com/jamesob/bitcoin/commit/f8d386913ce8b0dd012b42268926c1501c6b0f03. Feel free to cherry-pick/steal.

fanquake commented at 4:10 am on December 23, 2021:

Followed up with in #23736.

jamesob approved

jamesob commented at 5:02 pm on December 3, 2021: member

reACK 8301c696d0a0be921ce01b6e9f74f7ff6e8c0f2f (jamesob/ackr/23280.6.dongcarl.init_coalesce_chainstate)

Changes since my last ACK include

minor formatting for the LoadChainstate call
break if an error is encountered during LoadChainstate instead of proceeding to VerifyLoadedChainstate (and displaying the “Verifying blocks…” message)
don’t prefix parameter names when returning from CalculateCacheSizes

I’m still singing the same tune: this is a big improvement to init, importantly because it can be reused in unittests.

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3reACK 8301c696d0a0be921ce01b6e9f74f7ff6e8c0f2f ([`jamesob/ackr/23280.6.dongcarl.init_coalesce_chainstate`](https://github.com/jamesob/bitcoin/tree/ackr/23280.6.dongcarl.init_coalesce_chainstate))
 4
 5Changes since my last ACK include
 6- - minor formatting changes for the `LoadChainstate` call
 7- - break if an error is encountered during `LoadChainstate` instead of proceeding to `VerifyLoadedChainstate` (and displaying the "Verifying blocks..." message)
 8- - don't prefix parameter names when returning from CalculateCacheSizes
 9
10I'm still singing the same tune:  this is a big improvement to init, importantly because it can be reused in unittests.
11-----BEGIN PGP SIGNATURE-----
12
13iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmGqTYkACgkQepNdrbLE
14TwWFAg//Suzk9OqS3OCmxCijJEnCWXSv/afG26X5yTqTL/o1DyzQZrFddliL3VJN
15Fx6j3Zln80cq1Jl4yeVRsQ8zU7MWv+hpVEyf4SAjcToJ3qxHan3pZfUalPE8nzFv
161ubvztUag/xhO09/W6yBIFGl7IbUDA4uyayg+7QCM2Plv5IXiP3c01ufiGTpuRxg
171H/VszdnQEjBaRLw1r+cv9pGYBUtzgXUk/i17esfAlZV71C/gUOvfVOwp3wHJB2G
18PGnpwQ7fQ+cQ/4KED/RIuMMQAztHjPqQUcsOFCFWohtGU7KXbQdD3TjlJYRJNIEW
19k5kW+ySVSypJyxuGJVK2wm4Fvu5uzKK2c2tqg714HHMtHxjIz857WTagKpyseKPb
20hhWZOSYdQxyinqXtCEF3r1ufBqxjeEPSjAwl2aDOi0bbZ01rBEFAp2VQzfwBSwoz
21L81/wLLlQg4AG8LRIcDdIi+yz1xHrBbHbvmjN1Yg8X8fQ7qzmnwxqB9MYz74IE1v
227J4pgxgM6wT772fJIZnlNynVR5ZvlX3VcYTpvos4A4KM7BA2n0KFm3AvQB5s0gUi
23omtsbQbysQ5+rqpwUDmhqIhkkSI4pmExmn60Y0UkiLOMrbdKE0AbuGgdQ/p2RvOs
24bktDxlmOwraljitKSJHnD0UQuLmdSxuujFW4IO39dN+RrRwsYMQ=
25=0Nt0
26-----END PGP SIGNATURE-----

0Tested on Linux-5.10.0-9-amd64-x86_64-with-glibc2.28
1
2Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon --enable-natpmp-default CXX=/usr/bin/clang++ CC=/usr/bin/clang PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --disable-openssl-tests --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --disable-openssl-tests --no-create --no-recursion
3
4Compiled with /usr/bin/ccache /usr/bin/clang++ -std=c++17 -mavx -mavx2 -mpclmul -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=.  -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4 -msha -msse4.1 -msse4.2  i
5
6Compiler version: Debian clang version 13.0.1-++20211110052940+162f3f18c945-1~exp1~20211110053502.25

jamesob commented at 5:03 pm on December 3, 2021: member

Needs rebase, though.

jonatack commented at 3:02 pm on December 5, 2021: member

Approach ACK. Debug build clean, needs rebase.

node: Extract chainstate loading sequence

I strongly recommend reviewing with the following git-diff flags:
  --color-moved=dimmed_zebra --color-moved-ws=allow-indentation-change

[META] This commit is intended to be as close to a move-only commit as
       possible, and lingering ugliness will be resolved in subsequent
       commits.

A few variables that are passed in by value instead of by reference
deserve explanation:

- fReset and fReindexChainstate are both local variables in AppInitMain
  and are not modified in the sequence

- fPruneMode, despite being a global, is only modified in
  AppInitParameterInteraction, long before LoadChainstate is called

----

[META] This semantic will change in a future commit named
       "node/chainstate: Decouple from stringy errors"

cb64af9635

node/chainstate: Decouple from GetTimeMillis

...instead just move it out

cbac28b72f

node/chainstate: Decouple from stringy errors

This allows us to separate the initialization code from translations and
error reporting.

This change changes the caller semantics of LoadChainstate quite
drastically.

To see that this change doesn't change behaviour, observe that:

1. Prior to this change, LoadChainstate returned false only in the "bad
   genesis block" failure case (by returning InitError()), indicating
   that the caller should immediately bail. After this change, the
   corresponding ERROR_BAD_GENESIS_BLOCK handler in src/init.cpp
   maintains behavioue by also bailing immediately.

2. The failed_* temporary booleans were only used to break out of the
   outer do/while(false) loop. They can therefore be safely removed.

ae9121f958

node/chainstate: Decouple from ArgsManager

...instead pass in only the necessary information

c7a5c46e6f

node/chainstate: Decouple from concept of NodeContext

...instead pass in only the necessary information

Also allow mempool to be a nullptr

9162a4f93e

Move mempool nullptr Assert out of LoadChainstate 8715658983

Move init logistics message for BAD_GENESIS_BLOCK to init.cpp 975235ca0a

node/chainstate: Remove do/while loop

I strongly recommend reviewing with the following git-diff flags:
  --ignore-space-change

adf4912d77

Split off VerifyLoadedChainstate ca7c0b934d

dongcarl force-pushed on Dec 6, 2021

node/chainstate: Decouple from concept of uiInterface

...instead allow the caller to optionally pass in callbacks which are
triggered for certain events.

Behaviour change: The string "Verifying blocks..." was previously
printed for each chainstate in chainman which did not have an
effectively empty coinsview, now it will be printed once unconditionally
before we call VerifyLoadedChain.

b345979a2b

node/chainstate: Reduce coupling of LogPrintf

...by moving the try/catch out of LoadChainstate

I strongly recommend reviewing with the following git-diff flags:
  --color-moved=dimmed_zebra --color-moved-ws=allow-indentation-change

aad8d59789

Move -checkblocks LogPrintf to AppInitMain 8d466a8504

DrahtBot removed the label Needs rebase on Dec 6, 2021

RandyMcMillan commented at 10:59 pm on December 6, 2021: contributor

0Darwin ₿ 20.5.0 Darwin Kernel Version 20.5.0: Sat May  8 05:10:31 PDT 2021; root:xnu-7195.121.3~9/RELEASE_ARM64_T8101 arm64

Screen Shot 2021-12-06 at 5 32 34 PM

Screen Shot 2021-12-06 at 5 27 26 PM

mzumsande commented at 11:19 pm on December 6, 2021: member

@RandyMcMillan: That happens on current master too when building without bdb, see #23684 - so it’s very likely not related to this PR.

jamesob approved

jamesob commented at 4:45 pm on December 7, 2021: member

reACK c6861476f7e58f7b6b2e5416d2a7b774d1796a0e (jamesob/ackr/23280.7.dongcarl.init_coalesce_chainstate)

Only changes include rebasing on recent master (GetAdjustedTime -> GetTime). Built locally and booted a few times with existing mainnet datadir.

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3reACK c6861476f7e58f7b6b2e5416d2a7b774d1796a0e ([`jamesob/ackr/23280.7.dongcarl.init_coalesce_chainstate`](https://github.com/jamesob/bitcoin/tree/ackr/23280.7.dongcarl.init_coalesce_chainstate))
 4
 5Only changes include rebasing on recent master (GetAdjustedTime -> GetTime). Built locally and booted a few times with existing mainnet datadir. 
 6-----BEGIN PGP SIGNATURE-----
 7
 8iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmGvj4IACgkQepNdrbLE
 9TwW1RBAAnzie/0j3f5h7kn6pH5WLtr4np3ffokobdyfDuPsfhMIp1aqBDX0GQlrs
10sy62TEzoPgcD3EQL4TA29Em0/C+uWQIHuBjwnZXSQvFE38BIh4DfcKS+GE2v1j4H
11rBp7R60pRXjXNPYv7aTGvhkM2VPsOLt/n4xdSzvzBPjXWmSaCb+rJcivEZHciKXX
124VH2djGJiFwmfmXPo0V9UhNLI86HJ3ju+vJTm32bURTuu7+Hm3P6rHotkhNYGgd7
13ZwI35VobxCmU8qd2AD/6H0GwYYr6cketmVi6fwXWpGhzibGvAr7H4nznnNy66AWh
14JoVxIPcbCxJMm8WxzD8+Jn5kVMUeX1c1b7oDUGA6o5r3RLJ7tMD0wg6TXMBi2QQ8
15E8tC3tTF2NC9cgTBv3xQr1hxx6KIE7oT4tShl6G+ryMtq/kNKCAu2nd1qkEzsQW2
16pIcpKUkXBhFrFagUfOri58tKBYEFmohalqjHiEmGAOqqX0FJR7xTEBFGQnlMDUmd
17v7/o6swc+w2dp5T7tDtdCtlNxTG57dPndZKEMzlnue6yBtjV8ZPm7PbqSSWhiLV2
18ZvNDZp1kH1zbeePRf+bzGU8s448RMEG9c77xyNluTJkpCq+4kRFR1aVreWgJvTh+
19OM45ymO4vvYa+0HWqxQUA6CPW/gMr0gkeulQb+MOv/IjpH6dZ7s=
20=uM20
21-----END PGP SIGNATURE-----

0Tested on Linux-5.10.0-9-amd64-x86_64-with-glibc2.28
1
2Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon --enable-natpmp-default CXX=/usr/bin/clang++ CC=/usr/bin/clang PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --disable-openssl-tests --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --disable-openssl-tests --no-create --no-recursion
3
4Compiled with /usr/bin/ccache /usr/bin/clang++ -std=c++17 -mavx -mavx2 -mpclmul -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=.  -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4 -msha -msse4.1 -msse4.2  i
5
6Compiler version: Debian clang version 13.0.1-++20211110052940+162f3f18c945-1~exp1~20211110053502.25

ryanofsky approved

ryanofsky commented at 7:16 pm on December 7, 2021: member

Code review ACK c6861476f7e58f7b6b2e5416d2a7b774d1796a0e. Only change since last review is rebasing after #23636

Maybe this PR is ready for merge. I would still like to drop commit “test/setup: Unify m_args and gArgs” (4e54d903a3b4643cc6d91c899a0ac401fc8f6529) which uses gArgs more in tests, instead off less (https://github.com/bitcoin/bitcoin/pull/23280#discussion_r760574720)

I also think argument and error code code handling here could be simplified, suggested previously in b7c7f64dc86f0fcdf07cb1e765f7cccc3a3c8897 (+129/-210) #23280 (comment)

And there might be some other previous suggestions from reviewers that could use followup, but overall PR is an improvement in its current form, already

init: Delay RPC block notif until warmup finished

See added code comment for more details.

2414ebc18b

node/chainstate: Decouple from GetTime

...instead pass in a std::function<int64_t()>

Note that the static_cast is needed (apparently) for the compiler to
know which overloaded GetTime to choose.

05441c2dc5

node/chainstate: Decouple from ShutdownRequested

...instead allow optionally passing in a std::function<bool()>

4da9c076d1

validation: VerifyDB only needs Consensus::Params

Previously we were passing in CChainParams, when VerifyDB only needed
the Consensus::Params subset.

15f2e33bb3

node/caches: Extract cache calculation logic

I strongly recommend reviewing with the following git-diff flags:
  --color-moved=dimmed_zebra --color-moved-ws=allow-indentation-change

[META] In a future commit, this function will be re-used in TestingSetup
       so that the behaviour matches across test and non-test init
       codepaths.

ac4bf138b8

node/caches: Remove intermediate variables ceb9790341

node/chainstate: Add options for in-memory DBs

[META] In a future commit, these options will be used in TestingSetup to
       ensure that the DBs are in-memory.

c541da0d62

test/setup: Use LoadChainstate

This commit coalesces the chainstate loading sequence between our unit
test and non-unit test init codepaths.

9a5a5a3d08

Remove all #include // for * comments 3b1584b794

Collapse the 2 cs_main locks in LoadChainstate 89bec827fd

style-only: Remove redundant scope in *Chainstate

I strongly recommend reviewing with the following git-diff flags:
  --ignore-space-change

7f15eff2dd

dongcarl force-pushed on Dec 7, 2021

dongcarl commented at 8:03 pm on December 7, 2021: member

Pushed c6861476f7e58f7b6b2e5416d2a7b774d1796a0e -> 7f15eff2ddd86034e84a19413e1a42883987f660

Use std::function’s inherent nullability (https://github.com/bitcoin/bitcoin/pull/23280#discussion_r760598661)
Added code comment in commit “init: Delay RPC block notif until warmup finished” (https://github.com/bitcoin/bitcoin/pull/23280#discussion_r760525288)
Added commit removing intermediate variables in CalculateCacheSizes (https://github.com/bitcoin/bitcoin/pull/23280#discussion_r760560829)
Removed commit “test/setup: Unify m_args and gArgs” (https://github.com/bitcoin/bitcoin/pull/23280#discussion_r760574720)
Added commit “style-only: Remove redundant scope in *Chainstate” (https://github.com/bitcoin/bitcoin/pull/23280#discussion_r762078294, #23280 (review))

dongcarl commented at 8:04 pm on December 7, 2021: member

I also think argument and error code code handling here could be simplified, suggested previously in b7c7f64 (+129/-210) #23280 (comment)

I think we could put that in a followup PR for discussion, perhaps along with #23280 (review)

ryanofsky approved

ryanofsky commented at 8:38 pm on December 7, 2021: member

Code review ACK 7f15eff2ddd86034e84a19413e1a42883987f660. Thanks for updates!

in src/init.cpp:1418 in c541da0d62 outdated

1414@@ -1415,6 +1415,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1415                                 cache_sizes.block_tree_db,
1416                                 cache_sizes.coins_db,
1417                                 cache_sizes.coins,
1418+                                false,

laanwj commented at 3:42 pm on December 8, 2021:

Please use named argument style here

ryanofsky commented at 4:46 pm on December 8, 2021:

Please use named argument style here

This suggestion makes sense. But one way or the other I would also like to incorporate or follow up with b7c7f64dc86f0fcdf07cb1e765f7cccc3a3c8897 (+129/-210) #23280 (comment) which gets rid of these arguments.

laanwj commented at 5:02 pm on December 8, 2021:

Sure, getting rid of them is even better.

in src/node/chainstate.cpp:146 in 05441c2dc5 outdated

142@@ -143,7 +143,7 @@ std::optional<ChainstateLoadVerifyError> VerifyLoadedChainstate(ChainstateManage
143         for (CChainState* chainstate : chainman.GetAll()) {
144             if (!is_coinsview_empty(chainstate)) {
145                 const CBlockIndex* tip = chainstate->m_chain.Tip();
146-                if (tip && tip->nTime > GetTime() + MAX_FUTURE_BLOCK_TIME) {
147+                if (tip && tip->nTime > get_unix_time_seconds() + 2 * 60 * 60) {

laanwj commented at 4:58 pm on December 8, 2021:

As there is no need for precision here, wouldn’t it be simpler to pass in the time here instead of a function that returns the time? (or can this function take that long to run?)

MarcoFalke commented at 5:05 pm on December 8, 2021:

why is this removing MAX_FUTURE_BLOCK_TIME?

This reverts https://github.com/bitcoin/bitcoin/commit/26a1147ce56083d7aa820ac115c16b01e47d911c

MarcoFalke commented at 4:00 pm on December 10, 2021:

Also, agree with laanwj. This only calls is_coinsview_empty, which shouldn’t take more than a second (aka the precision of our clock), so it might be better to just pass in the time.

laanwj commented at 5:16 pm on December 8, 2021: member

Code review ACK This is a big refactor but I think it moves things in the right direction:

Some welcome decoupling, only passing things to functions which they need is great
Much better to use error codes than string-y errors
Nice to have CalculateCacheSizes return a structure rather than the usual way with tons of output argument

jamesob commented at 6:03 pm on December 8, 2021: member

I’ll reACK after the MAX_FUTURE_BLOCK_TIME rebase reversion is fixed

in src/init.cpp:1422 in ae9121f958 outdated

1417@@ -1418,11 +1418,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1418         bilingual_str strLoadError;
1419 
1420         uiInterface.InitMessage(_("Loading block index…").translated);
1421-
1422         const int64_t load_block_index_start_time = GetTimeMillis();
1423-        bool rv = LoadChainstate(fLoaded,
1424-                                 strLoadError,
1425-                                 fReset,
1426+        auto rv = LoadChainstate(fReset,

MarcoFalke commented at 3:24 pm on December 10, 2021:

any reason to call this rv when better names can be chosen? Like err?

in src/init.cpp:1466 in ca7c0b934d outdated

1471                 break;
1472             }
1473         } else {
1474-            fLoaded = true;
1475-            LogPrintf(" block index %15dms\n", GetTimeMillis() - load_block_index_start_time);
1476+            auto rv2 = VerifyLoadedChainstate(chainman,

MarcoFalke commented at 3:59 pm on December 10, 2021:

same

in src/node/chainstate.h:51 in 4da9c076d1 outdated

44@@ -45,10 +45,10 @@ enum class ChainstateLoadingError {
45  *        differentiable by the specific enumerator.
46  *
47  *        Note that a return value of SHUTDOWN_PROBED means ONLY that "during
48- *        this sequence, when we explicitly checked ShutdownRequested() at
49+ *        this sequence, when we explicitly checked shutdown_requested() at
50  *        arbitrary points, one of those calls returned true". Therefore, a
51  *        return value other than SHUTDOWN_PROBED does not guarantee that
52- *        ShutdownRequested() hasn't been called indirectly.
53+ *        shutdown_requested() hasn't been called indirectly.

MarcoFalke commented at 4:03 pm on December 10, 2021:

I think this comment is “wrong”. shutdown_requested is a local symbol to the function, so there is no other place calling it.

Maybe just rephrase as “shutdown hasn’t been requested indirectly”

in src/node/caches.h:20 in ac4bf138b8 outdated

15+    int64_t coins_db;
16+    int64_t coins;
17+    int64_t tx_index;
18+    int64_t filter_index;
19+};
20+CacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes = 0);

MarcoFalke commented at 4:10 pm on December 10, 2021:

seems fragile to provide a default value that is unused, no?

MarcoFalke approved

MarcoFalke commented at 4:16 pm on December 10, 2021: member

review ACK 7f15eff2ddd86034e84a19413e1a42883987f66 💳

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 7f15eff2ddd86034e84a19413e1a42883987f66 💳
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhRtAv+ICYz6n3kVKJ2bJ1tF9yGZ2fyPthEKAjS/ZUsTPpa2PtQaIhogbJWd3JQ
 8BsSdTafJk6P9+ODsSyuzwBYZ1NhBwSDS+PtWjvTOV0i5qE04v3Axd8naRWi2GEj+
 9cai53kzt8vteJdLCq4clcLUsGwfqm1GtWFbZo0gzpZUwBBzR0a4d8w3cEZx8GTL6
10S9kvXOKxvnD07B1s+3BXC9bCksJvpAmAAg5U+mU2X0nKOTvlVU0EypZiZ0Ho2Adg
11NdRFHUAo2ZSaqvL1nccsmNxJIWvDBKowAwoZFOLsGkJ00YjajBGQW6lyfglb3UO3
129YmFJ1h52sMDKJTL+dYIRvqJrAZ5rKaXgT271+UwvGmxe4Tw2pIn+7cAccypVsXr
139MiwMXnoyYwoCa/5qHz6Pb8VOlpbcYAdrQ1KL1DZ64VKZPEgsCCqGjRvVkarn3WD
14j/2UFbANtOkUcorc4A9JgWcAskOXeMF+Bs2ApQWbxVuMsSDXwdNfT3C5C+7tMlhY
15qrz4e3ru
16=BAAV
17-----END PGP SIGNATURE-----

MarcoFalke merged this on Dec 10, 2021

MarcoFalke closed this on Dec 10, 2021

MarcoFalke deleted a comment on Dec 10, 2021

sidhujag referenced this in commit 355097a225 on Dec 10, 2021

RandyMcMillan referenced this in commit daf6dfaddc on Dec 23, 2021

fanquake referenced this in commit f5c678e5c3 on Dec 23, 2021

MarcoFalke commented at 7:23 am on December 23, 2021: member

@dongcarl Are you going to address the feedback or do you want someone else to do it? Either is fine, just let us know.

dongcarl commented at 10:50 pm on December 23, 2021: member

@MarcoFalke Thanks for the reminder! Opened #23855.

sidhujag referenced this in commit 99d9e47d60 on Dec 27, 2021

MarcoFalke referenced this in commit 3917dff732 on Jan 6, 2022

Fabcien referenced this in commit af94e52472 on Nov 21, 2022

Fabcien referenced this in commit d0f1c29cd5 on Nov 21, 2022

Fabcien referenced this in commit b9aeba3d9c on Nov 21, 2022

Fabcien referenced this in commit c899aedf29 on Nov 22, 2022

Fabcien referenced this in commit f57aef1d01 on Nov 22, 2022

Fabcien referenced this in commit aa1cc478e2 on Nov 22, 2022

Fabcien referenced this in commit 6df390479a on Nov 22, 2022

Fabcien referenced this in commit 98e2e28c1a on Nov 22, 2022

Fabcien referenced this in commit 9a6c655473 on Nov 22, 2022

Fabcien referenced this in commit 9d2aaee492 on Nov 22, 2022

Fabcien referenced this in commit 9439a964d2 on Nov 22, 2022

Fabcien referenced this in commit 2e6d9aa12e on Nov 22, 2022

Fabcien referenced this in commit ef89e93b7c on Nov 22, 2022

Fabcien referenced this in commit c333cb502d on Nov 22, 2022

Fabcien referenced this in commit 8d9a8aa4ce on Nov 22, 2022

Fabcien referenced this in commit 21ce559101 on Nov 22, 2022

Fabcien referenced this in commit 39acc8c969 on Nov 22, 2022

Fabcien referenced this in commit 19ca6c8534 on Nov 22, 2022

Fabcien referenced this in commit d4573e2b1d on Nov 22, 2022

Fabcien referenced this in commit 275c7dfdd2 on Nov 22, 2022

Fabcien referenced this in commit 8769f8facb on Nov 22, 2022

Fabcien referenced this in commit b512db11f9 on Nov 22, 2022

DrahtBot locked this on Dec 23, 2022

init: Coalesce Chainstate loading sequence between {,non-}unittest codepaths #23280

Conflicts

Process note