The ::Params() global is verbose and confusing. Also it makes tests a bit harder to write because they’d have to mock a global.
Fix all issues by simply using a member variable that points to the right params.
(Can be reviewed with --word-diff-regex=.)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
563@@ -564,6 +564,8 @@ class CChainState
564 //! mempool that is kept in sync with the chain
565 CTxMemPool& m_mempool;
566
567+ const CChainParams& m_params;
nit: This member variable looks like the only one without a Doxygen comment in this section. Maybe add one?
Is protected better here than private? It compiled for me even when the variable is private.
private didn’t compile, so I left this as-is for now.
const CChainParams& m_params; to private section and it did compile for me. It confuses me but ok.
private and protected are the same anyway, unless the class is derived
Concept ACK.
It looks like a very nice simplification which makes it harder to make a mistake.
181@@ -182,7 +182,7 @@ class CBlockIndex
182 //!
183 //! Note: this value is modified to show BLOCK_OPT_WITNESS during UTXO snapshot
184 //! load to avoid the block index being spuriously rewound.
185- //! @sa RewindBlockIndex
186+ //! @sa NeedsRedownload
The main change in the first commit (fa216cc7) seems to be: “refactor: Add CChainState member to CChainState class” rather than “refactor: Remove ::Params() global from CChainState”.
Also, the changes like this https://github.com/bitcoin/bitcoin/pull/21789/commits/fa216cc79267ba25fa3709982626def61f4abe2a#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R2284 in the first commit (fa216cc7) can probably be moved to the second commit (fa06aaacf3b9c35d9e30c883bcd7780dcf0bceda) so that the first commit would only introduce m_params.
Changes in fa216cc79267ba25fa3709982626def61f4abe2a & fa06aaacf3b9c35d9e30c883bcd7780dcf0bceda LGTM.
184@@ -185,12 +185,12 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
185 assert(!::ChainstateActive().CanFlushToDisk());
186 ::ChainstateActive().InitCoinsCache(1 << 23);
187 assert(::ChainstateActive().CanFlushToDisk());
188- if (!::ChainstateActive().LoadGenesisBlock(chainparams)) {
189+ if (!::ChainstateActive().LoadGenesisBlock()) {
chainparams reference and just calling Params() in the call to PeerManager::make() below.
::Params(), so I’ll leave it as is for now.
It is confusing and verbose to repeatedly access the global when a
member variable can simply refer to it.
Passing this is confusing and redundant with the m_params member.
97@@ -98,7 +98,7 @@ BOOST_AUTO_TEST_CASE(findCommonAncestor)
98 auto* orig_tip = active.Tip();
99 for (int i = 0; i < 10; ++i) {
100 BlockValidationState state;
101- m_node.chainman->ActiveChainstate().InvalidateBlock(state, Params(), active.Tip());
102+ m_node.chainman->ActiveChainstate().InvalidateBlock(state, active.Tip());
#include <chainparams.h> still needed?
