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

dongcarl · 2021-10-14T16:18:06Z

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!

Empact · 2021-10-14T16:25:36Z

src/init.cpp

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


nit: FYI @fanquake is discouraging "for" comments: #15294 (review)

I agree. These comments are unmaintainable.

jamesob · 2021-10-14T16:30:55Z

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

dongcarl · 2021-10-14T17:04:42Z

Pushed 29c5c2b -> 3d5aa4b

Rebased over master
Changed some commit messages

DrahtBot · 2021-10-14T20:00:33Z

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 · 2021-10-14T21:57:10Z

Concept ACK. Will review.

dongcarl · 2021-10-15T15:50:14Z

Pushed 3d5aa4b...7dd898c:

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

jamesob

ACK 7dd898c (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!

Show signature data

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 7dd898c0df678859ca92773b6990e4e28a83ab24 ([`jamesob/ackr/23280.2.dongcarl.init_coalesce_chainstate`](https://github.com/jamesob/bitcoin/tree/ackr/23280.2.dongcarl.init_coalesce_chainstate))

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmF8OJcACgkQepNdrbLE
TwW1uxAAjTIPIZ4ddUhSlAax8GQR43wEE3Qvk5+I51RHNGyIS8QoPb0lgmBYgqJW
bqvVyMC0cKg9QNDLELrQQ/ySVf2Vl9H/6va4P18ZAKLg8cNuyeVr3cl4Swe0ZH9F
clRa8b75NTRUtOZlt34O2eTfigyKbnQ5L1+G8HJWNoBvWEPt5JSV7G2UU1tyLEh3
3lvS5vHivSt+8WqIhB/lBIERJmI7vHjFAhN8P6SgLyQr7NxKA8Lw6cFUFeJIWKDP
pkza4uabu1cDqAurxO2jgIq4GDL7FXz3fkXcFzGTPgY1JlXI26dEkuWBSEA42zGs
ubizjy/JIyRUbzRv5jBia5woDJgR40eFfsb7GkZOHfZtoQsDN85xwvZpRCzl/YnY
y1exK8fGSmCuP6cIP2mbbz9TrUlrGfyZm0vwIiqrRpGtO7q1CdTyiV7gnOwRUCjY
LqUI2WXMuJGSD/WaPIghpllFBejadETVkfjMUHBtaTO8WdpPchdgnLZFtKlyjvLV
x1UkNk3Xqkzzk75+XPEK+GBSDPnaD9fDnfypu7f18pin2O6gGFVErQto67xM1+HY
0eRlQOhM/T+SdsDWcl9z4fUS1pA+zxxpr3BRNHR4aNbuxXj326VIVnQUatEotncy
ilPh7NYFFVMWpoOGG5HbucjlazQLf/Z4Q9FxUqdDglXRSqK+kQM=
=XedH
-----END PGP SIGNATURE-----

Show platform data

Tested on Linux-5.10.0-9-amd64-x86_64-with-glibc2.28

Configured 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

Compiled 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

Compiler version: Debian clang version 13.0.1-++20211019122913+8a93745a7121-1~exp1~20211019003538.10

src/init.cpp

jamesob · 2021-10-29T17:41:51Z

src/validation.cpp

@@ -3764,6 +3764,7 @@ bool CChainState::LoadChainTip()
    PruneBlockIndexCandidates();

    tip = m_chain.Tip();
+    uiInterface.NotifyBlockTip(GetSynchronizationState(IsInitialBlockDownload()), tip);


b229024

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.

jamesob · 2021-10-29T18:00:43Z

src/init/caches.cpp

+#include <util/system.h>
+#include <validation.h>
+
+void CalculateCacheSizes(const ArgsManager& args, CacheSizes& cache_sizes, size_t n_indexes)


1f966b8

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.

dongcarl · 2021-10-29T20:28:03Z

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.

src/init/chainstate.cpp

jnewbery · 2021-11-05T13:06:57Z

src/init.cpp

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


I agree. These comments are unmaintainable.

jnewbery

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:

bitcoin/src/init.cpp

Line 1842 in 77a2f5d

SetRPCWarmupFinished();

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?

src/Makefile.am

jnewbery · 2021-11-05T13:29:12Z

src/init/chainstate.h

+    ERROR_CORRUPTED_BLOCK_DB,
+};
+
+std::optional<ChainstateLoadingError> LoadChainstateSequence(bool fReset,


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

src/init.cpp

jnewbery · 2021-11-05T14:29:09Z

src/init.cpp

+        }
+        if (rv.has_value()) {
+            switch (rv.value()) {
+            case ChainstateLoadingError::ERROR_LOADING_BLOCK_DB:


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")

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

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.

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.

In commit "init/chainstate: Decouple from stringy errors" (cd29d33)

re: #23280 (comment)

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.

src/init.cpp

jnewbery · 2021-11-05T15:35:13Z

src/init/caches.h

+    int64_t tx_index_cache_size;
+    int64_t filter_index_cache_size;
+};
+void CalculateCacheSizes(const ArgsManager& args, CacheSizes& cache_sizes, size_t n_indexes = 0);


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?

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

#23280 (comment)

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

src/init/caches.h

jnewbery · 2021-11-05T15:40:57Z

src/init.cpp

+                                        false,
+                                        false,


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

Suggested change

false,

false,

/*block_tree_db_in_memory=*/false,

/*coins_db_in_memory=*/false,

src/init/chainstate.cpp

ryanofsky

This looks very promising! Mostly reviewed this

ryanofsky · 2021-11-04T19:15:28Z

src/init/chainstate.h

+class ChainstateManager;
+struct NodeContext;
+
+bool LoadChainstateSequence(bool& fLoaded,


In commit "init: Extract chainstate loading sequence" (83ed7db)

The sequence can have 4 types of outcomes: 1. Success 2. Shutdown requested - nothing failed but a shutdown was triggered in the middle of the sequence 3. Soft failure - a failure that might be recovered from with a reindex 4. Hard failure - a failure that definitively cannot be recovered from with a reindex Currently, LoadChainstateSequence returns a bool which: - if false - Definitely a "Hard failure" - if true - if fLoaded -> "Success" - if ShutdownRequested() -> "Shutdown requested" - 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.

src/init.cpp

ryanofsky · 2021-11-05T12:55:51Z

src/init/chainstate.cpp

-                            int64_t nCoinCacheUsage,
-                            unsigned int check_blocks,
-                            unsigned int check_level) {
+std::optional<ChainstateLoadingError> LoadChainstateSequence(bool fReset,


In commit "init/chainstate: Decouple from stringy errors" (cd29d33)

1. Success 2. Shutdown requested - nothing failed but a shutdown was triggered in the middle of the sequence 3. Soft failure - a failure that might be recovered from with a reindex 4. Hard failure - a failure that definitively cannot be recovered from with a reindex Previously, LoadChainstateSequence returns a bool which: - if false - Definitely a "Hard failure" - if true - if fLoaded -> "Success" - if ShutdownRequested() -> "Shutdown requested" - else -> "Soft failure" After this change, LoadChainstateSequence returns a std::optional<ChainstateLoadingError> which: - if has_value() - Either "Soft failure" or "Hard failure", caller decides what to do based on the specific ChainstateLoadingError enum member - else - Either "Success" or "Shutdown requested", caller checks 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.

ryanofsky · 2021-11-05T17:25:28Z

src/init/chainstate.h

+class ChainstateManager;
+struct NodeContext;
+
+bool LoadChainstateSequence(bool& fLoaded,


In commit "init: Extract chainstate loading sequence" (83ed7db)

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

bitcoin/src/init/common.cpp

Line 25 in 77a2f5d

namespace init {

bitcoin/src/init/bitcoind.cpp

Line 15 in 77a2f5d

namespace init {

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,

src/init/chainstate.h

src/validation.cpp

src/init/chainstate.h

ryanofsky · 2021-11-05T18:49:05Z

src/init/chainstate.h

+                                                             std::optional<std::function<bool()>> shutdown_requested = std::nullopt,
+                                                             std::optional<std::function<void()>> coins_error_cb = std::nullopt,
+                                                             std::optional<std::function<void()>> verifying_blocks_cb = std::nullopt);


In commit "init/chainstate: Add options for in-memory DBs" (f45b885)

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:

struct LoadChainStatesOptions { std::optional<std::function<bool()>> shutdown_requested_cb; std::optional<std::function<void()>> coins_error_cb; std::optional<std::function<void()>> verifying_blocks_cb; };

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:

bool block_tree_db_in_memory = false; bool coins_db_in_memory = false;

maflcko · 2021-12-23T07:23:40Z

@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 · 2021-12-23T22:50:23Z

@MarcoFalke Thanks for the reminder! Opened #23855.

…stingSetup 826e12b test: call VerifyLoadedChainstate during ChainTestingSetup (James O'Beirne) Pull request description: for additional coverage and similarity to actual init process. Followup to bitcoin#23280. ACKs for top commit: dongcarl: Code Review ACK 826e12b ryanofsky: Code review ACK 826e12b Tree-SHA512: a4e7fd25e5d7a08b1e154ae6daf67c3048260a2684b0e569b544dd826693b7b969db9923b191e499cb8d8d0a2a73eb9330ff45909313145a9abb6052eb8c3ad9

…" fixups e3544c8 init: Use clang-tidy named args syntax (Carl Dong) 3401630 style-only: Rename *Chainstate return values (Carl Dong) 1dd5827 docs: Make LoadChainstate comment more accurate (Carl Dong) 6b83576 node/chainstate: Use MAX_FUTURE_BLOCK_TIME (Carl Dong) Pull request description: There are 2 proposed fixups in discussions in #23280 which I have not implemented: 1. An overhaul to return types and an option type for the two `*Chainstate` functions: #23280 (comment) - The change reintroduces stringy return types and is quite involved. It could be discussed in a separate PR. 2. Passing in the unix time to `VerifyChainstate` instead of a callback to get the time: #23280 (comment) - I'm not sure it matters much whether it's a callback or just the actual unix time. Also, I think `VerifyDB` can take quite a while, and I don't want to impose that the function have to "run quickly" in order to have it be correct. If reviewers feel strongly about either of the two fixups listed above, please feel free to open a PR based on mine and I'll close this one! ACKs for top commit: ryanofsky: Code review ACK e3544c8 MarcoFalke: ACK e3544c8 🐸 Tree-SHA512: dd1de0265b6785eef306e724b678ce03d7c54ea9f4b5ea0ccd7af59cce2ea3aba73fd4af0c15e2dca9265807dc4075f9afa2ec103672677b6638b1a4fc090904

Summary: ``` 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 ``` Partial backport of [[bitcoin/bitcoin#23280 | core#23280]]: bitcoin/bitcoin@cb64af9 Depends on D12554. Note: fPruneMode has been renamed in chainstate.cpp to prevent shadowing the global variable. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Subscribers: PiRK Differential Revision: https://reviews.bitcoinabc.org/D12555

Summary: ``` ...instead just move it out ``` Partial backport of [[bitcoin/bitcoin#23280 | core#23280]]: bitcoin/bitcoin@cbac28b Depends on D12555. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12556

Summary: ``` This allows us to separate the initialization code from translations and error reporting. This change changes the caller semantics of LoadChainstate quitendrastically. 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. ``` Partial backport of [[bitcoin/bitcoin#23280 | core#23280]]: bitcoin/bitcoin@ae9121f Depends on D12556. Test Plan: ninja all check-extended Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Subscribers: PiRK Differential Revision: https://reviews.bitcoinabc.org/D12557

Summary: ``` ...instead pass in only the necessary information ``` Partial backport of [[bitcoin/bitcoin#23280 | core#23280]]: bitcoin/bitcoin@c7a5c46 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12565

Summary: ``` ...instead pass in only the necessary information Also allow mempool to be a nullptr ``` Partial backport of [[bitcoin/bitcoin#23280 | core#23280]]: bitcoin/bitcoin@9162a4f Depends on D12565. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12566

Summary: Partial backport of [[bitcoin/bitcoin#23280 | core#23280]]: bitcoin/bitcoin@8715658 Depends on D12566. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12567

Summary: Partial backport of [[bitcoin/bitcoin#23280 | core#23280]]: bitcoin/bitcoin@975235c Depends on D12567. Test Plan: Read the comment. Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12568

Summary: Partial backport of [[bitcoin/bitcoin#23280 | core#23280]]: bitcoin/bitcoin@adf4912?diff=split&w=1 Depends on D12568. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12569

Summary: Partial backport of [[bitcoin/bitcoin#23280 | core#23280]]: bitcoin/bitcoin@ca7c0b9 Depends on D12569. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12570

Summary: ``` ...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. ``` Partial backport of [[bitcoin/bitcoin#23280 | core#23280]]: bitcoin/bitcoin@b345979 Depends on D12570. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12571

Summary: ``` ...by moving the try/catch out of LoadChainstate ``` Partial backport of [[bitcoin/bitcoin#23280 | core#23280]]: bitcoin/bitcoin@aad8d59 Depends on D12571. Note that the extra scope will be removed in a follow up. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12572

Summary: Partial backport of [[bitcoin/bitcoin#23280 | core#23280]]: bitcoin/bitcoin@8d466a8 Depends on D12572. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12573

Summary: Partial backport of [[bitcoin/bitcoin#23280 | core#23280]]: bitcoin/bitcoin@2414ebc Test Plan: ninja all check-all Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D12576

Summary: ``` ...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. ``` Partial backport of [[bitcoin/bitcoin#23280 | core#23280]]: bitcoin/bitcoin@05441c2 Depends on D12576. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D12577

Summary: ``` ...instead allow optionally passing in a std::function<bool()> ``` Partial backport of [[bitcoin/bitcoin#23280 | core#23280]]: bitcoin/bitcoin@4da9c07 Depends on D12577. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D12578

Summary: We used to pass Config before the verification was split. The original commit is about changing the signature of VerifyDB, but this is not applicable to our codebase because we need the config to get the max block size. Partial backport of [[bitcoin/bitcoin#23280 | core#23280]]: bitcoin/bitcoin@15f2e33 Depends on D12578. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D12579

Summary: Partial backport of [[bitcoin/bitcoin#23280 | core#23280]]: bitcoin/bitcoin@ac4bf13 Depends on D12579. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D12580

Summary: Partial backport of [[bitcoin/bitcoin#23280 | core#23280]]: bitcoin/bitcoin@ceb9790 Depends on D12580. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D12581

Summary: Partial backport of [[bitcoin/bitcoin#23280 | core#23280]]: bitcoin/bitcoin@c541da0 Depends on D12581. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D12582

Summary: ``` This commit coalesces the chainstate loading sequence between our unit test and non-unit test init codepaths. ``` Partial backport of [[bitcoin/bitcoin#23280 | core#23280]]: bitcoin/bitcoin@9a5a5a3 Depends on D12582. Test Plan: ninja all check Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12583

Summary: Most of them have been removed along the backport already. Partial backport of [[bitcoin/bitcoin#23280 | core#23280]]: bitcoin/bitcoin@3b1584b Depends on D12583. Test Plan: Read the code. Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12584

Summary: Completes backport of [[bitcoin/bitcoin#23280 | core#23280]]: bitcoin/bitcoin@7f15eff Depends on D12584. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12585

Empact reviewed Oct 14, 2021

View reviewed changes

DrahtBot added the Needs rebase label Oct 14, 2021

dongcarl force-pushed the 2021-09-kernel-lib-v3 branch from 29c5c2b to 3d5aa4b Compare October 14, 2021 17:04

DrahtBot added Build system RPC/REST/ZMQ Validation and removed Needs rebase labels Oct 14, 2021

DrahtBot added the Needs rebase label Oct 15, 2021

dongcarl force-pushed the 2021-09-kernel-lib-v3 branch from 3d5aa4b to 7dd898c Compare October 15, 2021 15:49

DrahtBot removed the Needs rebase label Oct 15, 2021

jamesob mentioned this pull request Oct 15, 2021

test: add stress tests for initialization #23289

Merged

maflcko added Refactoring and removed Build system RPC/REST/ZMQ Validation labels Oct 26, 2021

DrahtBot mentioned this pull request Oct 27, 2021

test: Parse command line arguments from unit and fuzz tests, make addrman consistency check ratio easier to change #23373

Merged

jamesob approved these changes Oct 29, 2021

View reviewed changes

jnewbery reviewed Nov 5, 2021

View reviewed changes

src/init/chainstate.cpp Outdated Show resolved Hide resolved

ryanofsky reviewed Nov 5, 2021

View reviewed changes

dongcarl mentioned this pull request Dec 23, 2021

refactor: Post-"Chainstate loading sequence coalescence" fixups #23855

Merged

jamesob mentioned this pull request Jan 3, 2022

bug: Reindex started via ThreadSafeQuestion will always fail #22964

Closed

ryanofsky mentioned this pull request Feb 11, 2022

[kernel 0/n] Introduce bitcoin-chainstate #24304

Merged

bitcoin locked and limited conversation to collaborators Dec 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dongcarl commented Oct 14, 2021 •

edited

Loading

Empact Oct 14, 2021

jnewbery Nov 5, 2021

jamesob commented Oct 14, 2021

dongcarl commented Oct 14, 2021 •

edited

Loading

DrahtBot commented Oct 14, 2021 •

edited

Loading

theuni commented Oct 14, 2021

dongcarl commented Oct 15, 2021

jamesob left a comment

jamesob Oct 29, 2021

jamesob Oct 29, 2021

dongcarl commented Oct 29, 2021 •

edited

Loading

jnewbery Nov 5, 2021

jnewbery left a comment

jnewbery Nov 5, 2021

jnewbery Nov 5, 2021

theuni Nov 5, 2021

jamesob Nov 9, 2021

jnewbery Nov 9, 2021

ryanofsky Nov 12, 2021

jnewbery Nov 5, 2021

jamesob Nov 9, 2021

dongcarl Nov 15, 2021

jnewbery Nov 5, 2021

ryanofsky left a comment

ryanofsky Nov 4, 2021

ryanofsky Nov 5, 2021

ryanofsky Nov 5, 2021

ryanofsky Nov 5, 2021

maflcko commented Dec 23, 2021

dongcarl commented Dec 23, 2021

-                                        false,
-                                        false,
+                                        /*block_tree_db_in_memory=*/false,
+                                        /*coins_db_in_memory=*/false,

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

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

Conversation

dongcarl commented Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesob commented Oct 14, 2021

dongcarl commented Oct 14, 2021 • edited Loading

DrahtBot commented Oct 14, 2021 • edited Loading

Conflicts

theuni commented Oct 14, 2021

dongcarl commented Oct 15, 2021

jamesob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongcarl commented Oct 29, 2021 • edited Loading

Choose a reason for hiding this comment

jnewbery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko commented Dec 23, 2021

dongcarl commented Dec 23, 2021

dongcarl commented Oct 14, 2021 •

edited

Loading

dongcarl commented Oct 14, 2021 •

edited

Loading

DrahtBot commented Oct 14, 2021 •

edited

Loading

dongcarl commented Oct 29, 2021 •

edited

Loading