refactor: Avoid passing params where not needed #25168

pull MarcoFalke wants to merge 5 commits into bitcoin:master from MarcoFalke:2205-remove-bloat-🏮 changing 21 files +66 −92

MarcoFalke commented at 4:51 PM on May 18, 2022: member
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.
Do not pass time getter to Chainstate helpers fa4ee53dca
Do not pass Consensus::Params& to Chainstate helpers faf012b438
Do not pass CChainParams& to BlockAssembler constructor fafe5c0ca2
Do not pass CChainParams& to PeerManager::make fa30234be8
Do not call global Params() when chainman is in scope fa1b76aeb0
DrahtBot added the label Refactoring on May 18, 2022
vincenzopalazzo approved
vincenzopalazzo commented at 6:41 PM on May 18, 2022: none

ACK https://github.com/bitcoin/bitcoin/pull/25168/commits/fa1b76aeb064b315a3767a8f59836ca18aeb117e
fanquake commented at 8:58 AM on May 19, 2022: member

Concept ACK
DrahtBot commented at 3:16 PM on May 19, 2022: contributor


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)
- #24474 (Additional thread safety annotations for CNode/Peer by ajtowns)
- #24232 (assumeutxo: add init and completion logic by jamesob)
- #23443 (p2p: Erlay support signaling by naumenkogs)
- #21527 (net_processing: lock clean up by ajtowns)
- #21006 (rpc: reduce LOCK(cs_min) scope in rest_block: ~5 times as many requests per second by martinus)
- #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.
DrahtBot cross-referenced this on May 19, 2022 from issue test: Cleanup miner_tests by MarcoFalke
DrahtBot cross-referenced this on May 19, 2022 from issue [kernel 2b/n] Add `ChainstateManager::m_adjusted_time_callback` by dongcarl
DrahtBot cross-referenced this on May 19, 2022 from issue assumeutxo: add init and completion logic by jamesob
DrahtBot cross-referenced this on May 20, 2022 from issue p2p: Erlay support signaling by naumenkogs
DrahtBot cross-referenced this on May 20, 2022 from issue rpc: reduce LOCK(cs_min) scope in rest_block: ~5 times as many requests per second by martinus
DrahtBot cross-referenced this on May 20, 2022 from issue rpc: Add submit option to generateblock by MarcoFalke
promag commented at 11:01 AM on May 20, 2022: member

Code review ACK fa1b76aeb064b315a3767a8f59836ca18aeb117e.
MarcoFalke merged this on May 20, 2022
MarcoFalke closed this on May 20, 2022
MarcoFalke deleted the branch on May 20, 2022
DrahtBot cross-referenced this on May 20, 2022 from issue Additional thread safety annotations for CNode/Peer members accessed via the message processing thread by ajtowns
DrahtBot cross-referenced this on May 20, 2022 from issue net_processing: lock clean up by ajtowns
dongcarl commented at 10:38 PM on May 25, 2022: contributor

post-merge ACK. This synergizes well with some of the upcoming libbitcoinkernel changes. We might actually be able to remove all references to a global CChainParams :astonished: (no promises tho)
sidhujag referenced this in commit 3bb5318bd1 on May 28, 2022
fanquake cross-referenced this on Jun 1, 2022 from issue kernel: pass params to BlockManager rather than using a global by theuni
bitcoin locked this on May 25, 2023

Contributors

Labels

Linked (view graph)

#25264 kernel: pass params to BlockManager rather than using a global

refactor: Avoid passing params where not needed #25168

Conflicts