Skip to content

Commit 4d0c00d

Browse files
author
MacroFake
committed
Merge bitcoin/bitcoin#25168: refactor: Avoid passing params where not needed
fa1b76a Do not call global Params() when chainman is in scope (MacroFake) fa30234 Do not pass CChainParams& to PeerManager::make (MacroFake) fafe5c0 Do not pass CChainParams& to BlockAssembler constructor (MacroFake) faf012b Do not pass Consensus::Params& to Chainstate helpers (MacroFake) fa4ee53 Do not pass time getter to Chainstate helpers (MacroFake) Pull request description: It seems confusing to pass chain params, consensus params, or a time function around when it is not needed. Fix this by: * Inlining the passed time getter function. I don't see a use case why this should be mockable. * Using `chainman.GetConsensus()` or `chainman.GetParams()`, where possible. ACKs for top commit: promag: Code review ACK fa1b76a. vincenzopalazzo: ACK bitcoin/bitcoin@fa1b76a Tree-SHA512: 1abff5cba4b4871d97f17dbcdf67bc9255ff21fa4150a79a74e39b28f0610eab3e7dee24d56872dd6e111f003b55e288958cdd467e6218368d896f191e4ec9cd
2 parents 8c61374 + fa1b76a commit 4d0c00d

21 files changed

+66
-92
lines changed

‎src/bitcoin-chainstate.cpp

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,6 @@ int main(int argc, char* argv[])
7676
std::ref(chainman),
7777
nullptr,
7878
false,
79-
chainparams.GetConsensus(),
8079
false,
8180
2 << 20,
8281
2 << 22,
@@ -91,10 +90,8 @@ int main(int argc, char* argv[])
9190
auto maybe_verify_error = node::VerifyLoadedChainstate(std::ref(chainman),
9291
false,
9392
false,
94-
chainparams.GetConsensus(),
9593
DEFAULT_CHECKBLOCKS,
96-
DEFAULT_CHECKLEVEL,
97-
/*get_unix_time_seconds=*/static_cast<int64_t (*)()>(GetTime));
94+
DEFAULT_CHECKLEVEL);
9895
if (maybe_verify_error.has_value()) {
9996
std::cerr << "Failed to verify loaded Chain state from your datadir." << std::endl;
10097
goto epilogue;

‎src/init.cpp

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1435,7 +1435,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
14351435
chainman,
14361436
Assert(node.mempool.get()),
14371437
fPruneMode,
1438-
chainparams.GetConsensus(),
14391438
fReindexChainState,
14401439
cache_sizes.block_tree_db,
14411440
cache_sizes.coins_db,
@@ -1482,7 +1481,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
14821481
break;
14831482
case ChainstateLoadingError::ERROR_BLOCKS_WITNESS_INSUFFICIENTLY_VALIDATED:
14841483
strLoadError = strprintf(_("Witness data for blocks after height %d requires validation. Please restart with -reindex."),
1485-
chainparams.GetConsensus().SegwitHeight);
1484+
chainman.GetConsensus().SegwitHeight);
14861485
break;
14871486
case ChainstateLoadingError::SHUTDOWN_PROBED:
14881487
break;
@@ -1499,10 +1498,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
14991498
maybe_verify_error = VerifyLoadedChainstate(chainman,
15001499
fReset,
15011500
fReindexChainState,
1502-
chainparams.GetConsensus(),
15031501
check_blocks,
1504-
args.GetIntArg("-checklevel", DEFAULT_CHECKLEVEL),
1505-
/*get_unix_time_seconds=*/static_cast<int64_t(*)()>(GetTime));
1502+
args.GetIntArg("-checklevel", DEFAULT_CHECKLEVEL));
15061503
} catch (const std::exception& e) {
15071504
LogPrintf("%s\n", e.what());
15081505
maybe_verify_error = ChainstateLoadVerifyError::ERROR_GENERIC_FAILURE;
@@ -1558,7 +1555,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
15581555
ChainstateManager& chainman = *Assert(node.chainman);
15591556

15601557
assert(!node.peerman);
1561-
node.peerman = PeerManager::make(chainparams, *node.connman, *node.addrman, node.banman.get(),
1558+
node.peerman = PeerManager::make(*node.connman, *node.addrman, node.banman.get(),
15621559
chainman, *node.mempool, ignores_incoming_txs);
15631560
RegisterValidationInterface(node.peerman.get());
15641561

@@ -1680,8 +1677,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
16801677
chain_active_height = chainman.ActiveChain().Height();
16811678
if (tip_info) {
16821679
tip_info->block_height = chain_active_height;
1683-
tip_info->block_time = chainman.ActiveChain().Tip() ? chainman.ActiveChain().Tip()->GetBlockTime() : Params().GenesisBlock().GetBlockTime();
1684-
tip_info->verification_progress = GuessVerificationProgress(Params().TxData(), chainman.ActiveChain().Tip());
1680+
tip_info->block_time = chainman.ActiveChain().Tip() ? chainman.ActiveChain().Tip()->GetBlockTime() : chainman.GetParams().GenesisBlock().GetBlockTime();
1681+
tip_info->verification_progress = GuessVerificationProgress(chainman.GetParams().TxData(), chainman.ActiveChain().Tip());
16851682
}
16861683
if (tip_info && chainman.m_best_header) {
16871684
tip_info->header_height = chainman.m_best_header->nHeight;

‎src/net_processing.cpp

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -459,7 +459,7 @@ struct CNodeState {
459459
class PeerManagerImpl final : public PeerManager
460460
{
461461
public:
462-
PeerManagerImpl(const CChainParams& chainparams, CConnman& connman, AddrMan& addrman,
462+
PeerManagerImpl(CConnman& connman, AddrMan& addrman,
463463
BanMan* banman, ChainstateManager& chainman,
464464
CTxMemPool& pool, bool ignore_incoming_txs);
465465

@@ -1584,17 +1584,17 @@ std::optional<std::string> PeerManagerImpl::FetchBlock(NodeId peer_id, const CBl
15841584
return std::nullopt;
15851585
}
15861586

1587-
std::unique_ptr<PeerManager> PeerManager::make(const CChainParams& chainparams, CConnman& connman, AddrMan& addrman,
1587+
std::unique_ptr<PeerManager> PeerManager::make(CConnman& connman, AddrMan& addrman,
15881588
BanMan* banman, ChainstateManager& chainman,
15891589
CTxMemPool& pool, bool ignore_incoming_txs)
15901590
{
1591-
return std::make_unique<PeerManagerImpl>(chainparams, connman, addrman, banman, chainman, pool, ignore_incoming_txs);
1591+
return std::make_unique<PeerManagerImpl>(connman, addrman, banman, chainman, pool, ignore_incoming_txs);
15921592
}
15931593

1594-
PeerManagerImpl::PeerManagerImpl(const CChainParams& chainparams, CConnman& connman, AddrMan& addrman,
1594+
PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman,
15951595
BanMan* banman, ChainstateManager& chainman,
15961596
CTxMemPool& pool, bool ignore_incoming_txs)
1597-
: m_chainparams(chainparams),
1597+
: m_chainparams(chainman.GetParams()),
15981598
m_connman(connman),
15991599
m_addrman(addrman),
16001600
m_banman(banman),

‎src/net_processing.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ struct CNodeStateStats {
3939
class PeerManager : public CValidationInterface, public NetEventsInterface
4040
{
4141
public:
42-
static std::unique_ptr<PeerManager> make(const CChainParams& chainparams, CConnman& connman, AddrMan& addrman,
42+
static std::unique_ptr<PeerManager> make(CConnman& connman, AddrMan& addrman,
4343
BanMan* banman, ChainstateManager& chainman,
4444
CTxMemPool& pool, bool ignore_incoming_txs);
4545
virtual ~PeerManager() { }

‎src/node/chainstate.cpp

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ std::optional<ChainstateLoadingError> LoadChainstate(bool fReset,
1313
ChainstateManager& chainman,
1414
CTxMemPool* mempool,
1515
bool fPruneMode,
16-
const Consensus::Params& consensus_params,
1716
bool fReindexChainState,
1817
int64_t nBlockTreeDBCache,
1918
int64_t nCoinDBCache,
@@ -57,7 +56,7 @@ std::optional<ChainstateLoadingError> LoadChainstate(bool fReset,
5756
}
5857

5958
if (!chainman.BlockIndex().empty() &&
60-
!chainman.m_blockman.LookupBlockIndex(consensus_params.hashGenesisBlock)) {
59+
!chainman.m_blockman.LookupBlockIndex(chainman.GetConsensus().hashGenesisBlock)) {
6160
return ChainstateLoadingError::ERROR_BAD_GENESIS_BLOCK;
6261
}
6362

@@ -126,10 +125,8 @@ std::optional<ChainstateLoadingError> LoadChainstate(bool fReset,
126125
std::optional<ChainstateLoadVerifyError> VerifyLoadedChainstate(ChainstateManager& chainman,
127126
bool fReset,
128127
bool fReindexChainState,
129-
const Consensus::Params& consensus_params,
130128
int check_blocks,
131-
int check_level,
132-
std::function<int64_t()> get_unix_time_seconds)
129+
int check_level)
133130
{
134131
auto is_coinsview_empty = [&](CChainState* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
135132
return fReset || fReindexChainState || chainstate->CoinsTip().GetBestBlock().IsNull();
@@ -140,12 +137,12 @@ std::optional<ChainstateLoadVerifyError> VerifyLoadedChainstate(ChainstateManage
140137
for (CChainState* chainstate : chainman.GetAll()) {
141138
if (!is_coinsview_empty(chainstate)) {
142139
const CBlockIndex* tip = chainstate->m_chain.Tip();
143-
if (tip && tip->nTime > get_unix_time_seconds() + MAX_FUTURE_BLOCK_TIME) {
140+
if (tip && tip->nTime > GetTime() + MAX_FUTURE_BLOCK_TIME) {
144141
return ChainstateLoadVerifyError::ERROR_BLOCK_FROM_FUTURE;
145142
}
146143

147144
if (!CVerifyDB().VerifyDB(
148-
*chainstate, consensus_params, chainstate->CoinsDB(),
145+
*chainstate, chainman.GetConsensus(), chainstate->CoinsDB(),
149146
check_level,
150147
check_blocks)) {
151148
return ChainstateLoadVerifyError::ERROR_CORRUPTED_BLOCK_DB;

‎src/node/chainstate.h

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,6 @@ std::optional<ChainstateLoadingError> LoadChainstate(bool fReset,
5959
ChainstateManager& chainman,
6060
CTxMemPool* mempool,
6161
bool fPruneMode,
62-
const Consensus::Params& consensus_params,
6362
bool fReindexChainState,
6463
int64_t nBlockTreeDBCache,
6564
int64_t nCoinDBCache,
@@ -78,10 +77,8 @@ enum class ChainstateLoadVerifyError {
7877
std::optional<ChainstateLoadVerifyError> VerifyLoadedChainstate(ChainstateManager& chainman,
7978
bool fReset,
8079
bool fReindexChainState,
81-
const Consensus::Params& consensus_params,
8280
int check_blocks,
83-
int check_level,
84-
std::function<int64_t()> get_unix_time_seconds);
81+
int check_level);
8582
} // namespace node
8683

8784
#endif // BITCOIN_NODE_CHAINSTATE_H

‎src/node/interfaces.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -228,15 +228,15 @@ class NodeImpl : public Node
228228
uint256 getBestBlockHash() override
229229
{
230230
const CBlockIndex* tip = WITH_LOCK(::cs_main, return chainman().ActiveChain().Tip());
231-
return tip ? tip->GetBlockHash() : Params().GenesisBlock().GetHash();
231+
return tip ? tip->GetBlockHash() : chainman().GetParams().GenesisBlock().GetHash();
232232
}
233233
int64_t getLastBlockTime() override
234234
{
235235
LOCK(::cs_main);
236236
if (chainman().ActiveChain().Tip()) {
237237
return chainman().ActiveChain().Tip()->GetBlockTime();
238238
}
239-
return Params().GenesisBlock().GetBlockTime(); // Genesis block's time of current network
239+
return chainman().GetParams().GenesisBlock().GetBlockTime(); // Genesis block's time of current network
240240
}
241241
double getVerificationProgress() override
242242
{
@@ -245,7 +245,7 @@ class NodeImpl : public Node
245245
LOCK(::cs_main);
246246
tip = chainman().ActiveChain().Tip();
247247
}
248-
return GuessVerificationProgress(Params().TxData(), tip);
248+
return GuessVerificationProgress(chainman().GetParams().TxData(), tip);
249249
}
250250
bool isInitialBlockDownload() override {
251251
return chainman().ActiveChainstate().IsInitialBlockDownload();
@@ -546,7 +546,7 @@ class ChainImpl : public Chain
546546
double guessVerificationProgress(const uint256& block_hash) override
547547
{
548548
LOCK(cs_main);
549-
return GuessVerificationProgress(Params().TxData(), chainman().m_blockman.LookupBlockIndex(block_hash));
549+
return GuessVerificationProgress(chainman().GetParams().TxData(), chainman().m_blockman.LookupBlockIndex(block_hash));
550550
}
551551
bool hasBlocks(const uint256& block_hash, int min_height, std::optional<int> max_height) override
552552
{

‎src/node/miner.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,8 @@ BlockAssembler::Options::Options()
6262
nBlockMaxWeight = DEFAULT_BLOCK_MAX_WEIGHT;
6363
}
6464

65-
BlockAssembler::BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params, const Options& options)
66-
: chainparams(params),
65+
BlockAssembler::BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool, const Options& options)
66+
: chainparams{chainstate.m_chainman.GetParams()},
6767
m_mempool(mempool),
6868
m_chainstate(chainstate)
6969
{
@@ -87,8 +87,8 @@ static BlockAssembler::Options DefaultOptions()
8787
return options;
8888
}
8989

90-
BlockAssembler::BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params)
91-
: BlockAssembler(chainstate, mempool, params, DefaultOptions()) {}
90+
BlockAssembler::BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool)
91+
: BlockAssembler(chainstate, mempool, DefaultOptions()) {}
9292

9393
void BlockAssembler::resetBlock()
9494
{

‎src/node/miner.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -157,8 +157,8 @@ class BlockAssembler
157157
CFeeRate blockMinFeeRate;
158158
};
159159

160-
explicit BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params);
161-
explicit BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params, const Options& options);
160+
explicit BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool);
161+
explicit BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool, const Options& options);
162162

163163
/** Construct a new block template with coinbase to scriptPubKeyIn */
164164
std::unique_ptr<CBlockTemplate> CreateNewBlock(const CScript& scriptPubKeyIn);

‎src/rest.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,7 @@ static bool rest_block(const std::any& context,
305305
if (chainman.m_blockman.IsBlockPruned(pblockindex))
306306
return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not available (pruned data)");
307307

308-
if (!ReadBlockFromDisk(block, pblockindex, Params().GetConsensus()))
308+
if (!ReadBlockFromDisk(block, pblockindex, chainman.GetParams().GetConsensus()))
309309
return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
310310
}
311311

0 commit comments

Comments
 (0)