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

jamesob · 2019-05-07T18:50:11Z

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.

src/validation.cpp

DrahtBot · 2019-05-07T22:50:22Z

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.

maflcko · 2019-05-08T16:30:42Z

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

maflcko · 2019-05-08T16:36:38Z

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

Sjors

Concept ACK

Can confirm 802ae94 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.

src/validation.h

Sjors · 2019-05-08T17:15:01Z

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.

maflcko · 2019-05-08T17:26:31Z

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 · 2019-05-08T17:43:23Z

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.

maflcko

utACK 61fd34e

Show signature and timestamp

Signature:

Timestamp of file with hash 2470d8f5b5b09630c89a5fc8a331144aed1939344367664ad30f7e38bc00a911 -

src/validation.h

ryanofsky

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

src/validation.h

jamesob · 2019-05-15T03:04:22Z

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

maflcko

utACK 5fe4eb3

src/validation.cpp

src/validation.h

src/validation.cpp

jamesob · 2019-05-15T13:57:09Z

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

utACK 64d0f87. 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

src/validation.cpp

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.

Empact · 2019-05-29T03:27:20Z

utACK 403e677 no need to address my nits herein

Empact · 2019-05-29T03:28:48Z

src/validation.h

+        int nManualPruneHeight = 0);
+
+    //! Unconditionally flush all changes to disk.
+    void ForceFlushStateToDisk();


nit: given context, this could be renamed ForceFlushToDisk

Empact · 2019-05-29T03:29:24Z

src/validation.h

+     * If FlushStateMode::NONE is used, then FlushStateToDisk(...) won't do anything
+     * besides checking if we need to prune.
+     */
+    bool FlushStateToDisk(


nit: given context, this could be renamed FlushToDisk

Empact · 2019-05-29T03:30:35Z

src/validation.h

+    IF_NEEDED,
+    PERIODIC,
+    ALWAYS
+};


nit: this could be moved to CChainState::FlushMode

jamesob · 2019-05-30T18:02:57Z

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

promag

utACK 403e677 and rebased with current master.

promag · 2019-06-02T21:15:58Z

src/validation.cpp

-    struct CBlockIndexWorkComparator
-    {
-        bool operator()(const CBlockIndex *pa, const CBlockIndex *pb) const {
-            // First sort by most total work, ...


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?

laanwj · 2019-06-05T09:54:55Z

utACK 403e677

403e677 refactoring: IsInitialBlockDownload -> CChainState (James O'Beirne) 3ccbc37 refactoring: FlushStateToDisk -> CChainState (James O'Beirne) 4d66886 refactoring: introduce ChainstateActive() (James O'Beirne) d7c97ed move-only: make the CChainState interface public (James O'Beirne) Pull request description: This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11): 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`. ACKs for commit 403e67: Empact: utACK 403e677 no need to address my nits herein Sjors: utACK 403e677 ryanofsky: utACK 403e677. Only change since previous review is removing global state comment as suggested. MarcoFalke: utACK 403e677, though the diff still seems a bit bloated with some unnecessary changes in the second commit. promag: utACK 403e677 and rebased with current [master](c7cfd20). Tree-SHA512: 6fcf260bb2dc201361170c0b4547405366f5f331fcc3a2bac29b24442814b7b244ca1b58aac5af716885f9a130c343b544590dff780da0bf835c7c5b3ccb2257

403e677 refactoring: IsInitialBlockDownload -> CChainState (James O'Beirne) 3ccbc37 refactoring: FlushStateToDisk -> CChainState (James O'Beirne) 4d66886 refactoring: introduce ChainstateActive() (James O'Beirne) d7c97ed move-only: make the CChainState interface public (James O'Beirne) Pull request description: This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11): Parent PR: bitcoin#15606 Issue: bitcoin#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`. ACKs for commit 403e67: Empact: utACK bitcoin@403e677 no need to address my nits herein Sjors: utACK 403e677 ryanofsky: utACK 403e677. Only change since previous review is removing global state comment as suggested. MarcoFalke: utACK 403e677, though the diff still seems a bit bloated with some unnecessary changes in the second commit. promag: utACK 403e677 and rebased with current [master](c7cfd20). Tree-SHA512: 6fcf260bb2dc201361170c0b4547405366f5f331fcc3a2bac29b24442814b7b244ca1b58aac5af716885f9a130c343b544590dff780da0bf835c7c5b3ccb2257

fa2b083 [test] Add test to check mempool consistency in case of reorgs (MarcoFalke) fabeb1f validation: Add missing mempool locks (MarcoFalke) fa0c9db txpool: Make nTransactionsUpdated atomic (MarcoFalke) Pull request description: Take the mempool read lock during reorgs, so that we don't accidentally read an inconsistent mempool. ACKs for top commit: laanwj: code review ACK fa2b083 ryanofsky: utACK fa2b083 [EDIT: was ~e284e422e75189794e24fe482819d8b1407857c3~, from bad copy and paste]. Changes since last review: rebase after #15976, adding vTxHashes lock annotation, adding new commit dropping mempool lock for nTransactionsUpdated and making it atomic to avoid deadlock between mempool lock and g_best_block_mutex Tree-SHA512: cfe7777993589087753e000e3736d79d320dca412383fb77b56bef8946a04049722bf888c11b6f722adf677165185c7e58b4a269f7c5fa25e84dda375f6c8a7d

fa2b083 [test] Add test to check mempool consistency in case of reorgs (MarcoFalke) fabeb1f validation: Add missing mempool locks (MarcoFalke) fa0c9db txpool: Make nTransactionsUpdated atomic (MarcoFalke) Pull request description: Take the mempool read lock during reorgs, so that we don't accidentally read an inconsistent mempool. ACKs for top commit: laanwj: code review ACK fa2b083 ryanofsky: utACK fa2b083 [EDIT: was ~e284e422e75189794e24fe482819d8b1407857c3~, from bad copy and paste]. Changes since last review: rebase after bitcoin#15976, adding vTxHashes lock annotation, adding new commit dropping mempool lock for nTransactionsUpdated and making it atomic to avoid deadlock between mempool lock and g_best_block_mutex Tree-SHA512: cfe7777993589087753e000e3736d79d320dca412383fb77b56bef8946a04049722bf888c11b6f722adf677165185c7e58b4a269f7c5fa25e84dda375f6c8a7d

Summary: 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. This is a partial backport of Core [[bitcoin/bitcoin#15976 | PR15976]] : bitcoin/bitcoin@d7c97ed Test Plan: ninja all check-all Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D5996

Summary: To be used once we move global functions (e.g. FlushStateToDisk()) into CChainState methods. Thanks to Marco Falke for suggestions This is a partial backport of Core [[bitcoin/bitcoin#15976 | PR15976]] : bitcoin/bitcoin@4d66886 Depends on D5996 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6001

Summary: Also renames global methods for clarity: - ::FlushStateToDisk() -> CChainState::ForceFlushStateToDisk() - This performs an unconditional flush. - ::PruneAndFlush() -> CChainState::PruneAndFlush() This is a partial backport of Core [[bitcoin/bitcoin#15976 | PR15976]] : bitcoin/bitcoin@3ccbc37 Depends on D6001 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6002

Summary: We introduce CChainState.m_cached_finished_ibd because the static state it replaces would've been shared across all CChainState instances. This is a partial backport of Core [[bitcoin/bitcoin#15976 | PR15976]] : bitcoin/bitcoin@403e677 Depends on D6002 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6035

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

merge bitcoin#15948, bitcoin#15976, bitcoin#16194...: assumeutxo project backports (part 1)

fa2b083 [test] Add test to check mempool consistency in case of reorgs (MarcoFalke) fabeb1f validation: Add missing mempool locks (MarcoFalke) fa0c9db txpool: Make nTransactionsUpdated atomic (MarcoFalke) Pull request description: Take the mempool read lock during reorgs, so that we don't accidentally read an inconsistent mempool. ACKs for top commit: laanwj: code review ACK fa2b083 ryanofsky: utACK fa2b083 [EDIT: was ~e284e422e75189794e24fe482819d8b1407857c3~, from bad copy and paste]. Changes since last review: rebase after bitcoin#15976, adding vTxHashes lock annotation, adding new commit dropping mempool lock for nTransactionsUpdated and making it atomic to avoid deadlock between mempool lock and g_best_block_mutex Tree-SHA512: cfe7777993589087753e000e3736d79d320dca412383fb77b56bef8946a04049722bf888c11b6f722adf677165185c7e58b4a269f7c5fa25e84dda375f6c8a7d

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

maflcko reviewed May 7, 2019

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

jamesob force-pushed the 2019-05-au-cchainstate branch from 0f7a07c to 65e2268 Compare May 7, 2019 18:56

DrahtBot added Mining P2P Refactoring RPC/REST/ZMQ Validation labels May 7, 2019

fanquake removed Mining P2P RPC/REST/ZMQ labels May 8, 2019

DrahtBot added the Needs rebase label May 8, 2019

Sjors reviewed May 8, 2019

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

jamesob force-pushed the 2019-05-au-cchainstate branch from 65e2268 to 61fd34e Compare May 8, 2019 20:14

DrahtBot removed the Needs rebase label May 8, 2019

maflcko reviewed May 13, 2019

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

src/validation.h Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

ryanofsky approved these changes May 14, 2019

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

jamesob force-pushed the 2019-05-au-cchainstate branch from 61fd34e to 5fe4eb3 Compare May 15, 2019 02:57

maflcko reviewed May 15, 2019

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

src/validation.cpp Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

jamesob force-pushed the 2019-05-au-cchainstate branch from 5fe4eb3 to 64d0f87 Compare May 15, 2019 13:54

ryanofsky approved these changes May 15, 2019

View reviewed changes

src/validation.cpp Show resolved Hide resolved

maflcko reviewed May 15, 2019

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

Empact reviewed May 29, 2019

View reviewed changes

src/validation.h

IF_NEEDED,

PERIODIC,

ALWAYS

};

Copy link

Contributor

Empact May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this could be moved to CChainState::FlushMode

promag reviewed Jun 2, 2019

View reviewed changes

laanwj merged commit 403e677 into bitcoin:master Jun 5, 2019

maflcko mentioned this pull request Sep 8, 2019

[WIP] [FEATURE] RPC: Add command for pruning past ChainTip #16827

Closed

jamesob mentioned this pull request Aug 5, 2021

assumeutxo #15606

Closed

18 tasks

kwvg added a commit to kwvg/dash that referenced this pull request Oct 9, 2021

merge bitcoin#15976: move methods under CChainState

676d29d

kwvg mentioned this pull request Oct 9, 2021

merge bitcoin#15948, #15976, #16194...: assumeutxo project backports (part 1) dashpay/dash#4498

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Oct 10, 2021

merge bitcoin#15976: move methods under CChainState

ab09ba8

kwvg added a commit to kwvg/dash that referenced this pull request Oct 16, 2021

merge bitcoin#15976: move methods under CChainState

8b7572e

kwvg added a commit to kwvg/dash that referenced this pull request Oct 16, 2021

merge bitcoin#15976: move methods under CChainState

706154b

kwvg added a commit to kwvg/dash that referenced this pull request Oct 16, 2021

merge bitcoin#15976: move methods under CChainState

17b222d

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

kwvg added a commit to kwvg/dash that referenced this pull request Oct 21, 2021

merge bitcoin#15976: move methods under CChainState

fd22e01

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

kwvg added a commit to kwvg/dash that referenced this pull request Oct 22, 2021

merge bitcoin#15976: move methods under CChainState

1bb1cea

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Oct 23, 2021

Merge pull request #4498 from kittywhiskers/assumeutxo

29dbe98

merge bitcoin#15948, bitcoin#15976, bitcoin#16194...: assumeutxo project backports (part 1)

pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021

merge bitcoin#15976: move methods under CChainState

ca0a23d

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

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

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

Uh oh!

Conversation

jamesob commented May 7, 2019

Uh oh!

Uh oh!

DrahtBot commented May 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented May 8, 2019

Uh oh!

maflcko commented May 8, 2019

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented May 8, 2019

Uh oh!

maflcko commented May 8, 2019

Uh oh!

jamesob commented May 8, 2019

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamesob commented May 15, 2019

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jamesob commented May 15, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Empact commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact May 29, 2019

Choose a reason for hiding this comment

Uh oh!

Empact May 29, 2019

Choose a reason for hiding this comment

Uh oh!

Empact May 29, 2019

Choose a reason for hiding this comment

Uh oh!

jamesob commented May 30, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Jun 2, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jun 5, 2019

Uh oh!

Uh oh!

DrahtBot commented May 7, 2019 •

edited

Loading

Empact commented May 29, 2019 •

edited

Loading