refactor: move methods under CChainState (pt. 1)

jamesob commented at 6:50 pm on May 7, 2019: member

Parent PR: #15606 Issue: #15605 Specification: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal

This changeset starts moving functionality intimately related to CChainState into methods. Parameterizing these functions by a particular CChainState is necessary for the use of multiple chainstates simultaneously (e.g. for asynchronous background validation).

In this change, we

make the CChainState interface public - since other units will start to invoke its methods directly,
introduce ::ChainstateActive(), the CChainState equivalent for ::ChainActive(),
and move IsInitialBlockDownload() and FlushStateToDisk() into methods on CChainState.

Independent of assumeutxo, these changes better encapsulate chainstate behavior and allow easier use from a testing context.

There are more methods that we’ll move in the future, but they require other substantial changes (i.e. moving ownership of the CCoinsView* hierarchy into CChainState) so we’ll save them for future PRs.

The first move-only commit is most easily reviewed with git diff ... --color-moved=dimmed_zebra.

in src/validation.cpp:37 in 0f7a07c4a4 outdated

34@@ -34,7 +35,6 @@
35 #include <timedata.h>
36 #include <tinyformat.h>
37 #include <txdb.h>
38-#include <txmempool.h>

MarcoFalke commented at 6:53 pm on May 7, 2019:

style-nit: Why shuffle the includes for no reason?

jamesob commented at 6:54 pm on May 7, 2019:

Oops, will fix.

jamesob force-pushed on May 7, 2019

DrahtBot added the label Mining on May 7, 2019

DrahtBot added the label P2P on May 7, 2019

DrahtBot added the label Refactoring on May 7, 2019

DrahtBot added the label RPC/REST/ZMQ on May 7, 2019

DrahtBot added the label Validation on May 7, 2019

DrahtBot commented at 10:50 pm on May 7, 2019: 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:

#16092 (Don’t use global (external) symbols for symbols that are used in only one translation unit by practicalswift)
#15981 (Use func for log messages by Bushstar)
#15921 (Tidy up ValidationState interface by jnewbery)
#15759 ([p2p] Add 2 outbound blocks-only connections by sdaftuar)
#15192 (Add missing cs_main locks in ThreadImport(…)/Shutdown(…)/gettxoutsetinfo(…)/InitScriptExecutionCache(). Add missing locking annotations. by practicalswift)
#15191 (validation: Add missing cs_{LastBlockFile,nBlockSequenceId} locks in PruneAndFlush() and UnloadBlockIndex(). Add missing locking annotations. by practicalswift)
#14856 (net: remove more CConnman globals (theuni) by dongcarl)
#14193 (validation: Add missing mempool locks 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.

fanquake added this to the "In progress" column in a project

fanquake removed the label Mining on May 8, 2019

fanquake removed the label P2P on May 8, 2019

fanquake removed the label RPC/REST/ZMQ on May 8, 2019

DrahtBot added the label Needs rebase on May 8, 2019

in src/validation.h:444 in 65e2268428 outdated

439+    ALWAYS
440+};
441+
442+struct CBlockIndexWorkComparator
443+{
444+    bool operator()(const CBlockIndex *pa, const CBlockIndex *pb) const {

Sjors commented at 3:09 pm on May 8, 2019:

Does this implementation really need to be in the header?

ryanofsky commented at 5:11 pm on May 8, 2019:

The method implementation doesn’t, but the struct declaration does because it affects the layout of CChainState objects.

MarcoFalke commented at 6:23 pm on May 13, 2019:

in commit de39209d0c move-only: make the CChainState interface public

Could keep this implementation in the cpp file? Only put the struct and declaration in the header?

Also, the commit description mentions a “namespace”, which is no longer true, so should be removed?

MarcoFalke commented at 4:30 pm on May 8, 2019: member

I dislike that member functions that previously could (and should) only be called in the validation module are now callable from anywhere. To make it worse, they are easily confused e.g. ::ActivateBestChain vs CChainState::ActivateBestChain. The only solution I could think of is to mark the public methods that are not supposed to be called outside of validation with an underscore or something (similar to python).

MarcoFalke commented at 4:36 pm on May 8, 2019: member

An alternative could be to remove the global methods (e.g. ::ActivateBestChain) and inline them into the members?

Sjors commented at 5:06 pm on May 8, 2019: member

Concept ACK

Can confirm 802ae945b4b0e87be78ab84e2954e69a4abc2f22 is move-only, but see inline comment about CBlockIndexWorkComparator.

I did an IBD on testnet with 65e2268, stopping it a few times (which I assume covers both functions you moved) and checking getblockchaininfo.

Sjors commented at 5:15 pm on May 8, 2019: member

could (and should) only be called in the validation module are now callable from anywhere

Would you say the root cause of that is that ::ChainstateActive is global? Maybe we can make that only available through interfaces? There’s not that many places we access member functions from.

MarcoFalke commented at 5:26 pm on May 8, 2019: member

Would you say the root cause of that is that ::ChainstateActive is global?

Also, chainActive, mapBlockIndex, …. I fail to see how an interface could help here. Shouldn’t CChainState be the interface?

jamesob commented at 5:43 pm on May 8, 2019: member

An alternative could be to remove the global methods (e.g. ::ActivateBestChain) and inline them into the members?

I like this idea but left it on the table in the interest of minimizing the diff.

In general, my preference would be to have anything outside of validation deal with the CChainState interface (::ChainstateActive() or other instances), and have anything only to be used by validation in a static function outside of the header file. I didn’t initially do this (to keep the diff small), but I’m happy to add commits if that seems preferable.

jamesob force-pushed on May 8, 2019

DrahtBot removed the label Needs rebase on May 8, 2019

in src/validation.h:531 in 9147f28ec0 outdated

540+     * or always and in all cases if we're in prune mode and are deleting files.
541+     *
542+     * If FlushStateMode::NONE is used, then FlushStateToDisk(...) won't do anything
543+     * besides checking if we need to prune.
544+     */
545+    bool FlushStateToDisk(

MarcoFalke commented at 6:34 pm on May 13, 2019:

In commit 9147f28ec0 refactoring: FlushStateToDisk -> CChainState

It seems weird to move a method that was static in validation.cpp to the header file as a public member function. At the very least it should be private. (However, I’d prefer if in the same commit the FlushStateToDisk and PruneAndFlush were moved to a public method in CChainState.)

jamesob commented at 2:58 am on May 15, 2019:

Agree with your general concern, but usage of FlushStateToDisk happens in places that won’t work with private like AcceptToMemoryPoolWithTime and so if we made it private, we’d have to attach a bunch of friend statements which seems kind of gross.

MarcoFalke commented at 12:04 pm on May 15, 2019:

Hmm, makes sense

in src/validation.h:525 in 61fd34e244 outdated

520@@ -523,6 +521,14 @@ class CChainState {
521      */
522     CCriticalSection m_cs_chainstate;
523 
524+    /**
525+     * Is this chainstate undergoing initial block download?

MarcoFalke commented at 6:39 pm on May 13, 2019:

in commit 61fd34e244 refactoring: IsInitialBlockDownload -> CChainState

Instead of a question, say “Whether this c…”?

in src/validation.h:528 in 61fd34e244 outdated

520@@ -523,6 +521,14 @@ class CChainState {
521      */
522     CCriticalSection m_cs_chainstate;
523 
524+    /**
525+     * Is this chainstate undergoing initial block download?
526+     *
527+     * Mutable because we need to be able to mark IsInitialBlockDownload()
528+     * const, which toggles this for caching purposes.

MarcoFalke commented at 6:40 pm on May 13, 2019:

in commit 61fd34e refactoring: IsInitialBlockDownload -> CChainState

This is only toggled once (aka latched)? So should keep that nomenclature?

MarcoFalke commented at 6:41 pm on May 13, 2019: member

utACK 61fd34e244

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3utACK 61fd34e244
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUihkgv/aDDUmSAn14AENjmciWELRSosOaTj/t0ws0/y09T+/f8wcmv/kCBx6tnR
 8JJkdcyteDAIlxJADIzT3TnhhXg93zwozotbv9hYGt/wTij4bLJIO96mBIWqJc/9V
 9ivfScok+S1CwLVRp8nT3mzJMWMQLFFp7CyfZmKFnrCWCI6tuK2GSqCuy5dKkDjgk
101hPhYTjk1OsYUhn2OjFVyfK4C1I6U8qfTFh6H6iSTAX4lA5/x9Jx/yINw93PDaH2
11mMX8II9Pns7YY+zEXfNhw23IqomDe6GBTMBFw+6VAKAiGV5mIQeoabRcEdeqDT+4
12xn6CzXaID4ENm+tqDkiaFTrSP7JW3BIt7Bnz3+D1NbPZzRow2rEwaF9pH8/DPmVk
13WWU7ZziPywF2Q3BTCPq1SrkYgBSDYHYlA98XyZ4OAdhiT/tPV1gM5kihyy2LufpE
14lec41niuCaOn8zDJeUF4KkhtVfMTdXE8f7od+i6ziBpCJO4UZ6fxxZWQFiDJQIJQ
151DFdLFPI
16=enPL
17-----END PGP SIGNATURE-----

Timestamp of file with hash 2470d8f5b5b09630c89a5fc8a331144aed1939344367664ad30f7e38bc00a911 -

in src/validation.h:284 in 9147f28ec0 outdated

278@@ -278,9 +279,9 @@ void PruneOneBlockFile(const int fileNumber) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
279 void UnlinkPrunedFiles(const std::set<int>& setFilesToPrune);
280 
281 /** Flush all state, indexes and buffers to disk. */
282-void FlushStateToDisk();
283+void FlushStateToDisk(CChainState& chainstate);
284 /** Prune block files and flush state to disk. */
285-void PruneAndFlush();
286+void PruneAndFlush(CChainState& chainstate);

ryanofsky commented at 9:55 pm on May 14, 2019:

Is there any particular reason why FlushStateToDisk PruneAndFlush should not be ChainState members and FlushStateToDisk IsInitialBlockDownload should be? Curious how your comment in #15976 (comment) applies to these individual cases. Not that it really matters, but it’d be nice to have consistency, and maybe explicit rationale for why things are or aren’t members in the commit messages.

jamesob commented at 2:55 am on May 15, 2019:

No good reason beyond the fact that FlushStateToDisk affects on-disk block files (which are shared across chainstates) - but that’s a symptom of a more general issue (that FSTD handles pruning, which seems like a separate concern) so probably not worth worrying about here.

ryanofsky approved

ryanofsky commented at 10:13 pm on May 14, 2019: member

utACK 61fd34e24400b4cee07a379630331f37d2847588. Change is simple and looks good, though it might be nice to follow up on some of Marco’s suggestions.

jamesob force-pushed on May 15, 2019

jamesob commented at 3:04 am on May 15, 2019: member

Thanks for the good feedback @Sjors @MarcoFalke @ryanofsky. I agree with the comments so far and have incorporated all of them aside from those noted above. Initially I hadn’t touched the global Flush functions because I thought it’d create an unnecessarily large diff, but it turns out that it’s negligible.

I took the liberty of renaming the global function equivalents for clarity, and in the process I did notice something sort of interesting. PruneAndFlush() doesn’t seem to actually do any flushing, just pruning, and so I’m calling it PruneBlockFiles() accordingly. This might be worth more general investigation because other parts of the code seem to assume that call flushes chainstate (which doesn’t seem correct).

in src/validation.cpp:4761 in 5fe4eb3a1a outdated

4759@@ -4916,3 +4760,21 @@ class CMainCleanup
4760         mapBlockIndex.clear();
4761     }
4762 } instance_of_cmaincleanup;
4763+
4764+bool CBlockIndexWorkComparator::operator()(const CBlockIndex *pa, const CBlockIndex *pb) const {

MarcoFalke commented at 12:12 pm on May 15, 2019:

in commit a8930a66b1 move-only: make the CChainState interface public

Why is this moved to the end of the cpp file instead of keeping it at the top? This breaks both git blame and git diff with --color-moved=dimmed-zebra --ignore-all-space

jamesob commented at 1:52 pm on May 15, 2019:

It’d be in an awkward place at the top of the file - right below the “global state” comment. Happy to move it back if you think the diff convenience is worth it though.

MarcoFalke commented at 2:38 pm on May 15, 2019:

I don’t understand what the “global state” comment should convey. If a symbol is global and not const, we prefix it with g_. Might want to remove it?

in src/validation.h:613 in 5fe4eb3a1a outdated

609@@ -435,6 +610,9 @@ bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, C
610 /** Remove invalidity status from a block and its descendants. */
611 void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
612 
613+/** @returns the most-work chainstate. */

MarcoFalke commented at 12:15 pm on May 15, 2019:

0/** [@returns](/bitcoin-bitcoin/contributor/returns/) the most-work valid chainstate. */

in commit 7794d41a58 refactoring: introduce ChainstateActive()

Could clarify that this the chainstate is fully validated (as opposed to a pow header chain)

jamesob commented at 1:52 pm on May 15, 2019:

Ah good point! Thanks.

in src/validation.cpp:2063 in 5fe4eb3a1a outdated

2047-void FlushStateToDisk() {
2048+void CChainState::ForceFlushStateToDisk() {
2049     CValidationState state;
2050     const CChainParams& chainparams = Params();
2051-    if (!FlushStateToDisk(chainparams, state, FlushStateMode::ALWAYS)) {
2052+    if (!this->FlushStateToDisk(chainparams, state, FlushStateMode::ALWAYS)) {

MarcoFalke commented at 12:23 pm on May 15, 2019:

in commit 7ab9e339a8 refactoring: FlushStateToDisk -> CChainState

style-question: You remove the global with the same name in this commit, so the this-> is not required?

jamesob commented at 1:48 pm on May 15, 2019:

I like prefixing with this-> for methods because it makes it obvious we’re calling a method, sort of like m_. See the IRC discussion: http://www.erisian.com.au/bitcoin-core-dev/log-2019-05-06.html#l-572

Happy to undo it if it’s controversial though.

MarcoFalke commented at 2:39 pm on May 15, 2019:

Asked because we don’t do this elsewhere. You can pick whatever you prefer ofc

ryanofsky commented at 3:56 pm on May 15, 2019:

I like prefixing with this-> for methods because it makes it obvious we’re calling a method, sort of like m_

FWIW, I still think #14635 is a good solution that would improve readability in these cases

in src/validation.cpp:2061 in 5fe4eb3a1a outdated

2061     const CChainParams& chainparams = Params();
2062-    if (!FlushStateToDisk(chainparams, state, FlushStateMode::NONE)) {
2063+
2064+    // Confusingly, nothing is actually flushed to disk due to
2065+    // FlushStateMode::NONE; the only effect of this call is to prune
2066+    // block files if necessary.

MarcoFalke commented at 12:28 pm on May 15, 2019:

in commit 7ab9e339a8 refactoring: FlushStateToDisk -> CChainState

I’d rather not add a comment that starts with “confusingly”. You renamed the function and removed the confusion.

MarcoFalke commented at 12:30 pm on May 15, 2019: member

utACK 5fe4eb3a1a

jamesob force-pushed on May 15, 2019

jamesob commented at 1:57 pm on May 15, 2019: member

PruneAndFlush() doesn’t seem to actually do any flushing

My mistake: flushes do happen when calling PruneAndFlush(). I’ve undone that particular rename and removed the comment I added.

ryanofsky approved

ryanofsky commented at 3:58 pm on May 15, 2019: member

utACK 64d0f872fcf6b61de9e4e62ce13dbbe3b00cbb6c. Since previous: ForceFlushStateToDisk and PruneAndFlush are now class members, CBlockIndexWorkComparator compare method is moved to the cpp file, some comments and commit messages are updated

move-only: make the CChainState interface public

along with DisconnectResult, and CBlockIndexWorkComparator.

The CChainState interface needs to be known to the rest of the system because
many global functions will move to CChainState methods. This is to allow
other parts of the system to be parameterized per chainstate instance
instead of assuming a single global.

d7c97edeea

refactoring: introduce ChainstateActive()

To be used once we move global functions (e.g. FlushStateToDisk()) into
CChainState methods.

Thanks to Marco Falke for suggestions

4d6688603b

refactoring: FlushStateToDisk -> CChainState

Also renames global methods for clarity:

- ::FlushStateToDisk() -> CChainState::ForceFlushStateToDisk()
  - This performs an unconditional flush.

- ::PruneAndFlush() -> CChainState::PruneAndFlush()

3ccbc376dd

refactoring: IsInitialBlockDownload -> CChainState

We introduce CChainState.m_cached_finished_ibd because the static state it
replaces would've been shared across all CChainState instances.

403e677c9e

in src/validation.cpp:64 in 64d0f872fc outdated

62@@ -63,162 +63,10 @@
63 /**
64  * Global state

MarcoFalke commented at 8:41 pm on May 15, 2019:

nit, in commit a8930a66b1 move-only: make the CChainState interface public

This comment wasn’t accurate before the commit, nor is it accurate after the commit. And it made you move the code block below all the way down in the file.

Could instead remove this comment and keep the code block below in place? This also makes git blame and git diff work on the commit without workarounds.

jamesob commented at 1:08 pm on May 16, 2019:

Yep, agree. Not sure how much it helps the diff, but that comment should be removed. I’ve pushed the change.

jamesob force-pushed on May 16, 2019

in src/validation.h:518 in 3ccbc376dd outdated

512@@ -508,6 +513,28 @@ class CChainState {
513 
514     bool LoadBlockIndex(const Consensus::Params& consensus_params, CBlockTreeDB& blocktree) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
515 
516+    /**
517+     * Update the on-disk chain state.
518+     * The caches and indexes are flushed depending on the mode we're called with

Sjors commented at 1:49 pm on May 16, 2019:

Nit: might as well improve this comment while you’re moving it, by using more sentences…

0* Consider flushing caches and indexes for the on-disk chain state.
1* If we're in prune mode and about to delete block files, we flush.
2* Otherwise we check FlushStateMode:
3* IF_NEEDED: if the chainstate is too large compared to -dbcache (plus -mempoolsize in IBD)
4* PERIODIC: it's been a while since the last write
5* ALWAYS: e.g. during shutdown or RPC queries about the chain state
6* NONE: only flush if a manual prune height provided and it is above the current prune height

This clarification also makes it pretty obvious that this method needs to be split, but that can wait.

jamesob commented at 6:41 pm on May 30, 2019:

I very much like the idea of clarifying this comment, but in my experience doc changes sometimes generate a surprising amount of feedback. I don’t want to expand this PR beyond its current size (or delay it more than necessary), but will definitely be happy to review follow-up PRs making the change you suggest.

in src/validation.h:534 in 3ccbc376dd outdated

529+        int nManualPruneHeight = 0);
530+
531+    //! Unconditionally flush all changes to disk.
532+    void ForceFlushStateToDisk();
533+
534+    //! Prune blockfiles from the disk if necessary and then flush chainstate changes

Sjors commented at 2:22 pm on May 16, 2019:

Nit: flushing is done before pruning. Rename to FlushAndPrune()?

jamesob commented at 2:41 pm on May 16, 2019:

Ehh, well it depends on what you’re talking about being flushed - i.e. pcoinsTip get flushed after the prune happens.

Sjors commented at 3:01 pm on May 16, 2019:

I was talking about the chain state, which is the biggest thing :-)

jamesob commented at 3:05 pm on May 16, 2019:

Yeah, pcoinsTip (chainstate) is flushed after pruning.

Sjors commented at 3:42 pm on May 16, 2019:

Ah wait, never mind, you’re right. Why do we flush the chainstate cache after pruning? Is that in order to minimize the chances of running out of disk space?

in src/validation.cpp:2067 in 3ccbc376dd outdated

2066 }
2067 
2068-void PruneAndFlush() {
2069+void CChainState::PruneAndFlush() {
2070     CValidationState state;
2071     fCheckForPruning = true;

Sjors commented at 2:24 pm on May 16, 2019:

Yuck (but out of scope)

Sjors approved

Sjors commented at 2:30 pm on May 16, 2019: member

utACK 403e677

ryanofsky approved

ryanofsky commented at 10:10 pm on May 16, 2019: member

utACK 403e677c9ebbf9744733010e6b0c2d1b182ee850. Only change since previous review is removing global state comment as suggested.

in src/validation.cpp:4272 in 403e677c9e

4268@@ -4414,7 +4269,7 @@ bool CChainState::LoadGenesisBlock(const CChainParams& chainparams)
4269 
4270 bool LoadGenesisBlock(const CChainParams& chainparams)
4271 {
4272-    return g_chainstate.LoadGenesisBlock(chainparams);
4273+    return ::ChainstateActive().LoadGenesisBlock(chainparams);

MarcoFalke commented at 2:37 pm on May 17, 2019:

in commit 4d6688603b refactoring: introduce ChainstateActive()

Why is this changed? It seems this method will go away anyway. See https://github.com/bitcoin/bitcoin/pull/15606/files#diff-24efdb00bfbe56b140fb006b562cc70bL4415

in src/validation.cpp:4170 in 403e677c9e

4166@@ -4312,7 +4167,7 @@ bool CChainState::RewindBlockIndex(const CChainParams& params)
4167 }
4168 
4169 bool RewindBlockIndex(const CChainParams& params) {
4170-    if (!g_chainstate.RewindBlockIndex(params)) {
4171+    if (!::ChainstateActive().RewindBlockIndex(params)) {

MarcoFalke commented at 2:38 pm on May 17, 2019:

in commit 4d66886 refactoring: introduce ChainstateActive()

Why is this changed? It seems that eventually you’ll pass in the chainstate, see https://github.com/bitcoin/bitcoin/pull/15606/files#diff-24efdb00bfbe56b140fb006b562cc70bR4324

So why would you change the same line twice?

jamesob commented at 7:23 pm on May 23, 2019:

Even before that change, we remove the g_chainstate symbol (https://github.com/bitcoin/bitcoin/pull/15606/commits/4c4b9ce2f310a519f1579f6a2892c590c1b96c6a) which means that this change would have to happen at some point anyway.

in src/validation.cpp:4037 in 403e677c9e

4033@@ -4179,7 +4034,7 @@ bool CChainState::ReplayBlocks(const CChainParams& params, CCoinsView* view)
4034 }
4035 
4036 bool ReplayBlocks(const CChainParams& params, CCoinsView* view) {
4037-    return g_chainstate.ReplayBlocks(params, view);
4038+    return ::ChainstateActive().ReplayBlocks(params, view);

MarcoFalke commented at 2:39 pm on May 17, 2019:

in commit 4d66886 refactoring: introduce ChainstateActive()

Same here (and all other changes in this commit)

in src/validation.cpp:70 in 403e677c9e

65- */
66-namespace {
67-    struct CBlockIndexWorkComparator
68-    {
69-        bool operator()(const CBlockIndex *pa, const CBlockIndex *pb) const {
70-            // First sort by most total work, ...

MarcoFalke commented at 2:40 pm on May 17, 2019:

git-style-nit in commit d7c97edeea move-only: make the CChainState interface public

Could keep the previous indentation to preserve the git blame and minimize the diff line count? No strong opinion, though. Please don’t address my nit if you don’t have to touch the patches for other reasons.

jamesob commented at 7:16 pm on May 23, 2019:

I don’t know that we should maintain a weird indentation just for ease-of-diff, but if other reviewers disagree, I’ll make the change.

promag commented at 9:15 pm on June 2, 2019:

I don’t mind the indentation git show --color-moved=dimmed_zebra --color-moved-ws=ignore-all-space d7c97edeea8cee10ad9da1f940d39d5073ac142d.

I think this operator was inlined? Does it make sense to declare it inline?

MarcoFalke commented at 2:46 pm on May 17, 2019: member

utACK 403e677c9e, though the diff still seems a bit bloated with some unnecessary changes in the second commit.

fanquake added this to the "Blockers" column in a project

MarcoFalke commented at 7:46 pm on May 23, 2019: member

My comments are mostly meant to minimize the changed lines, so purely stylistic. I guess you shouldn’t address them unless you need to invalidate the reviews for other reasons.

in src/validation.h:438 in d7c97edeea outdated

433+class ConnectTrace;
434+
435+struct CBlockIndexWorkComparator
436+{
437+    bool operator()(const CBlockIndex *pa, const CBlockIndex *pb) const;
438+};

Empact commented at 3:13 am on May 29, 2019:

nit in commit d7c97ed move-only: make the CChainState interface public

How about defining CBlockIndexWorkComparator privately within CChainState, given it is not used outside the class?

in src/validation.cpp:83 in 4d6688603b outdated

79@@ -80,6 +80,8 @@ bool CBlockIndexWorkComparator::operator()(const CBlockIndex *pa, const CBlockIn
80 
81 CChainState g_chainstate;
82 
83+CChainState& ChainstateActive() { return g_chainstate; }

Empact commented at 3:17 am on May 29, 2019:

nit in commit 4d66886 refactoring: introduce ChainstateActive()

How about making g_chainstate static?

Empact commented at 3:27 am on May 29, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/15976/commits/403e677c9ebbf9744733010e6b0c2d1b182ee850 no need to address my nits herein

in src/validation.h:538 in 403e677c9e

533+        CValidationState &state,
534+        FlushStateMode mode,
535+        int nManualPruneHeight = 0);
536+
537+    //! Unconditionally flush all changes to disk.
538+    void ForceFlushStateToDisk();

Empact commented at 3:28 am on May 29, 2019:

nit: given context, this could be renamed ForceFlushToDisk

in src/validation.h:531 in 403e677c9e

526+     * or always and in all cases if we're in prune mode and are deleting files.
527+     *
528+     * If FlushStateMode::NONE is used, then FlushStateToDisk(...) won't do anything
529+     * besides checking if we need to prune.
530+     */
531+    bool FlushStateToDisk(

Empact commented at 3:29 am on May 29, 2019:

nit: given context, this could be renamed FlushToDisk

in src/validation.h:436 in 403e677c9e

431+enum class FlushStateMode {
432+    NONE,
433+    IF_NEEDED,
434+    PERIODIC,
435+    ALWAYS
436+};

Empact commented at 3:30 am on May 29, 2019:

nit: this could be moved to CChainState::FlushMode

jamesob commented at 6:02 pm on May 30, 2019: member

4 utACKs on this - is there anyone else who wants to give it a look before merge?

promag commented at 9:17 pm on June 2, 2019: member

utACK 403e677 and rebased with current master.

laanwj commented at 9:54 am on June 5, 2019: member

utACK 403e677c9ebbf9744733010e6b0c2d1b182ee850

laanwj merged this on Jun 5, 2019

laanwj closed this on Jun 5, 2019

laanwj referenced this in commit 5d37c1bde0 on Jun 5, 2019

fanquake removed this from the "Blockers" column in a project

sidhujag referenced this in commit 252b7cf94b on Jun 6, 2019

laanwj referenced this in commit c6e42f1ca9 on Jul 2, 2019

sidhujag referenced this in commit 4408967f33 on Jul 3, 2019

fanquake moved this from the "In progress" to the "Done" column in a project

fanquake moved this from the "Done" to the "In progress" column in a project

fanquake moved this from the "In progress" to the "Done" column in a project

deadalnix referenced this in commit 7487045167 on May 7, 2020

deadalnix referenced this in commit 570f1a6731 on May 7, 2020

deadalnix referenced this in commit 86da53b158 on May 7, 2020

deadalnix referenced this in commit ea63f129f9 on May 11, 2020

kittywhiskers referenced this in commit 676d29d27f on Oct 9, 2021

kittywhiskers referenced this in commit ab09ba87b7 on Oct 10, 2021

kittywhiskers referenced this in commit 8b7572ef37 on Oct 16, 2021

kittywhiskers referenced this in commit 706154b5c4 on Oct 16, 2021

kittywhiskers referenced this in commit 17b222d094 on Oct 16, 2021

kittywhiskers referenced this in commit fd22e01621 on Oct 21, 2021

kittywhiskers referenced this in commit 1bb1cea5c9 on Oct 22, 2021

PastaPastaPasta referenced this in commit 29dbe98ee2 on Oct 23, 2021

Munkybooty referenced this in commit 5fdbb0b6de on Nov 4, 2021

pravblockc referenced this in commit ca0a23d71c on Nov 18, 2021

DrahtBot locked this on Dec 16, 2021

refactor: move methods under CChainState (pt. 1) #15976

Conflicts