deploymentstatus: move g_versionbitscache global to ChainstateManager

ajtowns commented at 2:08 am on March 17, 2022: member

Gives ChainstateManager a reference to the CChainParams its working on, and simplifies some of the functions that would otherwise take that as a parameter. Removes the g_versionbitscache global by moving it into ChainstateManager.

dongcarl commented at 2:53 am on March 17, 2022: member

Concept ACK!

Also wanted to note that in one of my branches for libbitcoinkernel I give ChainstateManager a CChainParams as well, so it definitely makes sense!

DrahtBot added the label Block storage on Mar 17, 2022

DrahtBot added the label Mining on Mar 17, 2022

DrahtBot added the label P2P on Mar 17, 2022

DrahtBot added the label RPC/REST/ZMQ on Mar 17, 2022

DrahtBot added the label Utils/log/libs on Mar 17, 2022

DrahtBot added the label UTXO Db and Indexes on Mar 17, 2022

DrahtBot added the label Validation on Mar 17, 2022

ajtowns force-pushed on Mar 17, 2022

DrahtBot added the label Needs rebase on Mar 17, 2022

ajtowns force-pushed on Mar 17, 2022

ajtowns renamed this:
~~WIP remove g_versionbitscache global~~
WIP: deploymentstatus: move g_versionbitscache global to ChainstateManager
on Mar 17, 2022

DrahtBot removed the label Needs rebase on Mar 17, 2022

DrahtBot commented at 1:20 pm on March 19, 2022: 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:

#25073 (test: Cleanup miner_tests by MarcoFalke)
#25064 ([kernel 2b/n] Add ChainstateManager::m_adjusted_time_callback by dongcarl)
#24737 (Remove taproot chain param by MarcoFalke)
#24571 (p2p: Prevent block index fingerprinting by sending additional getheaders messages by dergoegge)
#24565 (Remove LOCKTIME_MEDIAN_TIME_PAST constant by MarcoFalke)
#24232 (assumeutxo: add init and completion logic by jamesob)
#24008 (assumeutxo: net_processing changes by jamesob)
#20799 (net processing: Only support version 2 compact blocks by jnewbery)
#18933 (rpc: Add submit option to generateblock 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.

ajtowns force-pushed on Mar 25, 2022

ajtowns renamed this:
~~WIP: deploymentstatus: move g_versionbitscache global to ChainstateManager~~
deploymentstatus: move g_versionbitscache global to ChainstateManager
on Mar 25, 2022

ajtowns marked this as ready for review on Mar 25, 2022

ajtowns commented at 2:59 pm on March 25, 2022: member

Should be ready for review now

ajtowns force-pushed on Apr 4, 2022

DrahtBot added the label Needs rebase on Apr 4, 2022

ajtowns force-pushed on Apr 4, 2022

DrahtBot removed the label Needs rebase on Apr 4, 2022

ajtowns force-pushed on Apr 5, 2022

ajtowns force-pushed on Apr 7, 2022

ajtowns force-pushed on Apr 14, 2022

ajtowns commented at 9:00 pm on April 14, 2022: member

Rebased on top of #22564 also no longer introduce a circular dependency between deploymentstatus and validation.

ajtowns force-pushed on Apr 14, 2022

in src/validation.h:944 in 555eb9344c outdated

935@@ -935,6 +936,11 @@ class ChainstateManager
936         return m_blockman.m_block_index;
937     }
938 
939+    /**
940+     * Track versionbit status
941+     */
942+    mutable VersionBitsCache m_versionbitscache;

dongcarl commented at 6:29 pm on April 18, 2022:

Could you explain a bit why m_versionbitscache is marked mutable?

ajtowns commented at 10:40 pm on April 18, 2022:

It’s a cache, so its existence doesn’t impact the logical API of the class – if we didn’t use a cache, you could have a const ChainstateManager and get answers as to whether a deployment is active/enabled and what version to use for new blocks; so adding a cache shouldn’t mean that you can only do that with a non-const ChainstateManager.

ie, I’m treating const ChainstateManager as being “logically” constant, not bitwise constant.

dongcarl commented at 6:47 pm on April 19, 2022:

Sounds good!

in src/validation.h:860 in 16f51f5cc2 outdated

857 public:
858     explicit ChainstateManager(const CChainParams& chainparams) : m_chainparams{chainparams} { }
859 
860+    const CChainParams& GetParams() const { return m_chainparams; }
861+    const Consensus::Params& GetConsensus() const { return m_chainparams.GetConsensus(); }
862+

dongcarl commented at 6:37 pm on April 18, 2022:

Perhaps these additions could be made in the previous commit where we introduce the private m_chainparams member

in src/validation.h:497 in 9fccb380e1 outdated

494@@ -495,6 +495,7 @@ class CChainState
495     node::BlockManager& m_blockman;
496 
497     /** Chain parameters for this chainstate */
498+    /* TODO: replace with m_chainman.GetParams() */

dongcarl commented at 6:39 pm on April 18, 2022:

By this, do you mean all references to (CChainState::)m_params by CChainState member functions should instead be references to m_chainman.GetParams()?

ajtowns commented at 10:41 pm on April 18, 2022:

Yeah. Or perhaps add a GetParams() and GetConsensus() to CChainState that just call m_chainman.GetParams() or m_chainman.GetConsensus().

ajtowns commented at 5:53 am on April 19, 2022:

(I didn’t start on this here since I imagine it would have a bunch of conflicts with other PRs)

dongcarl commented at 6:47 pm on April 19, 2022:

Sounds good!

in src/validation.cpp:3402 in e49e7cca6e outdated

3360@@ -3361,19 +3361,19 @@ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensu
3361     return true;
3362 }
3363 
3364-void UpdateUncommittedBlockStructures(CBlock& block, const CBlockIndex* pindexPrev, const Consensus::Params& consensusParams)
3365+void ChainstateManager::UpdateUncommittedBlockStructures(CBlock& block, const CBlockIndex* pindexPrev) const

dongcarl commented at 6:44 pm on April 18, 2022:

nit: I’m not sure that this change is 100% necessary. The change seems to trade one reference for another, and since ChainstateManager is much larger in scope, it is harder to see after the change that this is a function that is limited in scope.

However, this is not at all a bit deal and up to subjective taste so feel free to ignore!

ajtowns commented at 10:48 pm on April 18, 2022:

It needs to call DeploymentActive(.., SEGWIT), and that’s something that the ChainstateManager is responsible for being able to answer. At least that was my logic.

Note that it’s a const member function, so it’s not doing anything to change the chain state, if that counts as reducing the scope?

dongcarl commented at 6:48 pm on April 19, 2022:

Sounds good!

in src/validation.cpp:1910 in 555eb9344c outdated

1893@@ -1894,10 +1894,11 @@ void StopScriptCheckWorkerThreads()
1894 class WarningBitsConditionChecker : public AbstractThresholdConditionChecker
1895 {
1896 private:
1897-    int bit;
1898+    const ChainstateManager& m_chainman;

dongcarl commented at 6:49 pm on April 18, 2022:

Perhaps we can use a const VersionBitsCache& here?

ajtowns commented at 10:51 pm on April 18, 2022:

Could, but it should all get optimised away since WarningBitsConditionChecker is only briefly instantiated while it’s being checked.

dongcarl commented at 6:49 pm on April 19, 2022:

Sure!

dongcarl commented at 6:51 pm on April 18, 2022: member

Medium Code-Review ACK 555eb9344ca83f772fa810570e1aa07881d33a41

💥 🔫 Another mutable global bites the dust!

ajtowns force-pushed on Apr 18, 2022

DrahtBot added the label Needs rebase on Apr 20, 2022

ajtowns force-pushed on Apr 20, 2022

DrahtBot removed the label Needs rebase on Apr 20, 2022

MarcoFalke commented at 6:48 am on April 25, 2022: member

Are you sure this is ready for review? Looks like it is based on #22564 for now, which might change its commits. So maybe leave it in draft until #22564 is merged? (Reviewers can still ACK your commits here, but I don’t think it can be merged before #22564).

Also, I am wondering what to do with #24917. Should that go before/after #22564 or should it be included there? cc @dongcarl @ajtowns

ajtowns commented at 8:28 am on April 25, 2022: member

Are you sure this is ready for review? Looks like it is based on #22564 for now,

Yeah, there’s a trivial conflict between #22564 and these patches in that they both change the call to g_versionbitscache.Clear(); seemed more sensible to review the changes in #22564 first, so I rebased the changes here on top of that.

Also, I am wondering what to do with #24917. Should that go before/after #22564 or should it be included there?

Seems fine to keep it separate, and 22564 seems higher priority and not blocked on anything; so I’d say 22564 first and keep 24917 separate.

DrahtBot added the label Needs rebase on Apr 26, 2022

ajtowns force-pushed on Apr 27, 2022

DrahtBot removed the label Needs rebase on Apr 27, 2022

ajtowns force-pushed on Apr 28, 2022

DrahtBot added the label Needs rebase on Apr 28, 2022

ajtowns force-pushed on Apr 29, 2022

DrahtBot removed the label Needs rebase on Apr 29, 2022

in src/validation.h:859 in 4cbc4e94b7 outdated

853@@ -851,6 +854,11 @@ class ChainstateManager
854     friend CChainState;
855 
856 public:
857+    explicit ChainstateManager(const CChainParams& chainparams) : m_chainparams{chainparams} { }
858+
859+    const CChainParams& GetParams() const { return m_chainparams; }
860+    const Consensus::Params& GetConsensus() const { return m_chainparams.GetConsensus(); }

MarcoFalke commented at 2:46 pm on April 29, 2022:

4cbc4e94b764ba2b4a79610fadbb092a7bdd076e: What about a public m_params and m_consensus? There shouldn’t be any risk associated with read-only references. And a public member removes the need for a function wrapper that only returns the member.

ajtowns commented at 4:13 pm on April 29, 2022:

6 to one, half a dozen the other? I thought accessors were more common than extra variables, and make it a bit clearer that they can’t get out of sync? shrug

in src/validation.h:18 in 0fc617a406 outdated

14@@ -15,6 +15,7 @@
15 #include <chain.h>
16 #include <chainparams.h>
17 #include <consensus/amount.h>
18+#include <consensus/params.h>

MarcoFalke commented at 2:48 pm on April 29, 2022:

0fc617a406232e21bdc8b39b2f8308f965775825 Any reason to add this include when the existing fwd-decl is enough?

ajtowns commented at 4:20 pm on April 29, 2022:

Adding #include <chainparams.h> in the previous commit already brings consensus/params.h in so the fwd declaration isn’t saving anything. Later, DeploymentActive* use info from consensus/params.h too.

MarcoFalke commented at 8:23 am on May 3, 2022:

I think for passing references or pointers, a fwd-decl is enough.

iwyu seems to agree:

 0validation.h should remove these lines:
 1- #include <config/bitcoin-config.h>  // lines 10-10
 2- #include <consensus/params.h>  // lines 18-18
 3- #include <stdint.h>  // lines 39-39
 4- #include <util/hasher.h>  // lines 30-30
 5- class CBlockTreeDB;  // lines 46-46
 6- class CTxMemPool;  // lines 47-47
 7- struct AssumeutxoData;  // lines 53-53
 8- struct ChainTxData;  // lines 49-49
 9- struct DisconnectedBlockTransactions;  // lines 50-50
10- struct LockPoints;  // lines 52-52

(Not saying you should fix all includes in this pull here, but I think not making it worse is a good compromise)

ajtowns commented at 2:11 pm on May 3, 2022:

Ah, iwyu treats the DeploymentActive stuff as recommending that deploymentstatus.h add the #include <consensus/params.h> not validation.h. I’ve got some later commits that move those functions into validation.h which updates iwyu’s recommendations to a #include.

Since it’s already included indirectly by chainparams.h I don’t really see the include as making it worse, but it’s not really better either, so will change it back if there are other fixes to bundle it with.

ajtowns commented at 2:13 am on May 10, 2022:

Done now

MarcoFalke approved

MarcoFalke commented at 2:56 pm on April 29, 2022: member

ACK

MarcoFalke removed the label UTXO Db and Indexes on Apr 29, 2022

MarcoFalke removed the label RPC/REST/ZMQ on Apr 29, 2022

MarcoFalke removed the label P2P on Apr 29, 2022

MarcoFalke removed the label Mining on Apr 29, 2022

MarcoFalke removed the label Validation on Apr 29, 2022

MarcoFalke removed the label Block storage on Apr 29, 2022

MarcoFalke removed the label Utils/log/libs on Apr 29, 2022

MarcoFalke added the label Refactoring on Apr 29, 2022

dongcarl commented at 7:13 pm on May 2, 2022: member

ACK d490648b2591f523a096f926e3f29e34d72da55a

Since last review @ 555eb9344ca83f772fa810570e1aa07881d33a41

Rebased over merge of #22564
Addressed #24595 (review)

DrahtBot added the label Needs rebase on May 6, 2022

ajtowns force-pushed on May 9, 2022

DrahtBot removed the label Needs rebase on May 9, 2022

ajtowns commented at 8:42 pm on May 9, 2022: member

Rebased past #24538

EDIT: also tweaked validation.h to not directly include consensus/params.h as per #24595 (review)

validation: add CChainParams to ChainstateManager 69675ea4e7

validation: remove redundant CChainParams params from ChainstateManager methods 38860f93b6

validation: replace ::Params() calls with chainstate/chainman member 5c67e84d37

validation: move UpdateUncommittedBlockStructures and GenerateCoinbaseCommitment into ChainstateManager eaa2e3f25c

deploymentstatus: allow chainman in place of consensusParams deffe0df6c

refactor: use chainman instead of chainParams for DeploymentActive* 78adef1753

deploymentstatus: make versionbitscache a parameter d603f1d8a7

test/versionbits: make versionbitscache a parameter eca22c726a

validation: move g_versionbitscache into ChainstateManager bb5c24b120

ajtowns force-pushed on May 10, 2022

dongcarl commented at 8:11 pm on May 12, 2022: member

reACK bb5c24b120a3ac7df367a1c5d9b075ca564efb5f

in src/validation.cpp:1460 in 5c67e84d37 outdated

1456@@ -1457,7 +1457,7 @@ PackageMempoolAcceptResult ProcessNewPackage(CChainState& active_chainstate, CTx
1457     assert(std::all_of(package.cbegin(), package.cend(), [](const auto& tx){return tx != nullptr;}));
1458 
1459     std::vector<COutPoint> coins_to_uncache;
1460-    const CChainParams& chainparams = Params();
1461+    const CChainParams& chainparams = active_chainstate.m_params;

MarcoFalke commented at 6:55 am on May 13, 2022:

5c67e84d37d452e9186a6357e5405fabeff241c7 nit: .m_chainman.GetParams()

Seems odd to add a TODO and violate it in the same commit ( /* TODO: replace with m_chainman.GetParams() */)

in src/test/versionbits_tests.cpp:432 in eca22c726a outdated

428@@ -429,7 +429,7 @@ BOOST_AUTO_TEST_CASE(versionbits_computeblockversion)
429             const uint32_t dep_mask{g_versionbitscache.Mask(chainParams->GetConsensus(), dep)};
430             BOOST_CHECK(!(chain_all_vbits & dep_mask));
431             chain_all_vbits |= dep_mask;
432-            check_computeblockversion(chainParams->GetConsensus(), dep);
433+            check_computeblockversion(g_versionbitscache, chainParams->GetConsensus(), dep);

MarcoFalke commented at 6:58 am on May 13, 2022:

eca22c726ac48b4216bb68cc0f0bbd655c43ac12 nit: Later diff could be smaller by a few lines if you added a VersionBitsCache& vbcache{g_versionbitscache}; in this commit already.

MarcoFalke approved

MarcoFalke commented at 6:59 am on May 13, 2022: member

review ACK bb5c24b120a3ac7df367a1c5d9b075ca564efb5f 📙

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK bb5c24b120a3ac7df367a1c5d9b075ca564efb5f 📙
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUi6+Qv8Dw67I3SBVnRjV/M10aNNkHAduz39CLI+zz2TwT/0IyAbgOEB9QAwzWS1
 8RFEQdpklN4Vxu+4qwkpEUWtl0cpJV8buVab33eUgDI1WuN/jo8nzF0GWpFwkHUJp
 94VEYDP+9zWssKUGgtsV0LPadXFsk74f9KW0q1FN+UljnEBBksPtRfGok1TQSzesH
10m0JlsdZcOBMFjNA+k7cQ3xyZi4b7tERRe8NZ1dewvHMBGpeh5J/n/TwHVO/4NO4Z
11S7qAENfXCP8PWblW5LgvUSlxpyKbwma5ygHe7M1/RnmzMQO1BRkcq0x78ktyxWwU
12Nq1hh4/ETvJgy3j3FIIm2AZeRRo+I1HhhJ1DuqKsqs3KNsM/+jToHrkel7Qi4WtT
13nzyK66L2Tb/MKGsXq2lyU9UC2U4REW3FTi0btmEnRl35RCUyhWbNppl2eZDaheHk
14NKnJV/wgZvgJUVra4Gt6pverD2dPL9/PA6V/M27tF1R9/buUMeEyQYVcBPBw5zSB
15ySIzD/fI
16=er/p
17-----END PGP SIGNATURE-----

MarcoFalke merged this on May 13, 2022

MarcoFalke closed this on May 13, 2022

sidhujag referenced this in commit 16e302dcf4 on May 13, 2022

in src/test/versionbits_tests.cpp:416 in bb5c24b120

416+    BOOST_CHECK_EQUAL(versionbitscache.ComputeBlockVersion(lastBlock, params) & (1 << bit), 0);
417 }
418 
419 BOOST_AUTO_TEST_CASE(versionbits_computeblockversion)
420 {
421+    VersionBitsCache vbcache; // don't use chainman versionbitscache since we want custom chain params

MarcoFalke commented at 12:21 pm on May 16, 2022:

Removed chainman completely from this test in https://github.com/bitcoin/bitcoin/pull/25125

aureleoules referenced this in commit 83efe632d7 on Aug 12, 2022

aureleoules referenced this in commit 672fdf35b0 on Sep 16, 2022

aureleoules referenced this in commit 5d3f98d278 on Oct 10, 2022

MarcoFalke referenced this in commit a97791d9fb on Oct 19, 2022

adam2k referenced this in commit 993d6f7c40 on Oct 19, 2022

satsie referenced this in commit 2484cba86a on Oct 31, 2022

DrahtBot locked this on May 16, 2023

deploymentstatus: move g_versionbitscache global to ChainstateManager #24595

Conflicts