DEPENDENT: Globals: Avoid calling Params() #5970

1. 
   jtimon commented at 1:15 am on April 4, 2015: contributor

   Passing CChainParams explicitly as parameter when possible facilitates testing and refactoring.

   Dependencies:

   • Chainparams: Refactor: Decouple IsSuperMajority from Params() #5968
   • Chainparams: Remove redundant getter CChainParams::SubsidyHalvingInterval() #5996
   • Chainparams: Refactor: Remove redundant HashGenesisBlock() getter #5997
   • Chainparams: Refactor: Remove redundant AllowMinDifficultyBlocks() getter #5998
   • Cleanup: Delete CChainParams getters to attributes from Consensus::Params #5999
   • Chainparams: Explicit CMessageHeader::MessageStartChars to functions in main: #6173
   • Chainparams: Explicit Consensus::Params arg for almost all remaining functions #6163
   • Globals: Make AcceptBlockHeader static (Fix #6163) (AcceptBlockHeader) #6982
   • Globals: Don’t call Params() from miner.cpp #6986 [global-chainparams-miner]
   • Blocksize: Turn MAX_BLOCK_SIZE and MAX_BLOCK_SIGOPS into functions #6625
   • [ ] Chainparams: Explicit Consensus::Params arg in consensus functions #6024
   • Consensus: Adapt declarations of most obviously consensus functions [closed for now] #6591 [consensus-params-0.12.99]
   • Globals: Explicit CChainParams in main.o (not touching the interface with net.o)
   • Globals: Explicit CChainParams in net.o (voluntaries required, ideally someone heavily touching net.o for any other reason)
   • Globals: Remove SelectParams()

   EDIT Initial description:

   After the struct Consensus::Params was created in #5812, there are some redundant getters in CChainParams. This depends on all the PRs that help remove them and continues passing CChainParams and Consensus::Params more explicitly, mainly in main.o, miner.o and init.cpp. The rpc’s, net.o, rest.o and base58.o are left with direct Params() calls. In the tests, Params(CBaseChainParams::MAIN) is preferred.

2. jtimon renamed this:
   Chainparams cleanup
   Dependent: Chainparams cleanup
   on Apr 4, 2015
3. laanwj added the label Improvement on Apr 8, 2015
4. jtimon force-pushed on Apr 8, 2015
5. jtimon force-pushed on Apr 8, 2015
6. 
   jtimon commented at 3:26 pm on April 8, 2015: contributor

   Note that AcceptToMemoryPool is ignored because the calls to Params() there will be replaced with policy initialization in init.cpp.
7. jtimon force-pushed on Apr 9, 2015
8. jtimon force-pushed on Apr 9, 2015
9. jtimon renamed this:
   Dependent: Chainparams cleanup
   DEPENDENT: Chainparams cleanup
   on Apr 10, 2015
10. jtimon force-pushed on Apr 10, 2015
11. jtimon force-pushed on Apr 10, 2015
12. jtimon force-pushed on Apr 10, 2015
13. jtimon force-pushed on Apr 17, 2015
14. jtimon force-pushed on Apr 17, 2015
15. jtimon force-pushed on Apr 17, 2015
16. jtimon force-pushed on Apr 19, 2015
17. jtimon force-pushed on Apr 19, 2015
18. 
    jtimon commented at 1:00 pm on April 23, 2015: contributor

    Closing for now, although I will keep updating the dependencies and rebasing.
19. jtimon closed this on Apr 23, 2015

    ---

20. jtimon reopened this on May 19, 2015

    ---

21. jtimon force-pushed on May 19, 2015
22. jtimon closed this on May 19, 2015

    ---

23. jtimon reopened this on Aug 27, 2015

    ---

24. jtimon force-pushed on Aug 27, 2015
25. 
    jtimon commented at 6:30 am on August 27, 2015: contributor

    Rebased and reopened. #6024 replaced with #6591. voluntaries required for the step “Chainparams: Explicit CChainParams in net.o” which is not coded here. Hopefully someone heavily touching net.o for any other reason (please tell anyone you know doing that about this PR) doesn’t mind to take that part. The final step “Chainparams: Remove SelectParams()” is not coded either, but I’m more than happy to do that myself once it’s possible.
26. 
    dcousens commented at 7:46 am on August 27, 2015: contributor

    Comparison URL to review this PR independently of its dependent: https://github.com/jtimon/bitcoin/compare/consensus-params-0.12.99...jtimon:chainparams_cleanup
27. jtimon force-pushed on Aug 27, 2015
28. jtimon force-pushed on Aug 29, 2015
29. jtimon force-pushed on Sep 3, 2015
30. 
    jtimon commented at 0:14 am on September 3, 2015: contributor

    Rebased on top of #6591
31. jtimon force-pushed on Sep 12, 2015
32. jtimon force-pushed on Nov 10, 2015
33. jtimon force-pushed on Nov 11, 2015
34. jtimon renamed this:
    DEPENDENT: Chainparams cleanup
    DEPENDENT: Globals: Avoid calling Params()
    on Nov 11, 2015
35. jtimon force-pushed on Nov 11, 2015
36. 
    jtimon commented at 12:31 pm on November 11, 2015: contributor

    Rebased after #6163 and #6982 have been merged, opened #6986.
37. jtimon force-pushed on Nov 11, 2015
38. consensus: don't define MAX_STANDARD_TX_SIGOPS in terms of block size 25d5800edf
39. consensus: teach ExtractMatches to check for an arbitrary max transaction number

    This is a no-op change. For now, everything passes MAX_BLOCK_SIZE / 60, so the
    result matches what it would've before.
    
    Tests use a number equal to the number of transactions where necessary,
    to ensure that they're never rejected when blocksizesize isn't being tested.

    0b92e2f29f
40. consensus: teach CheckTransaction to check for an arbitrary max tx size

    This is a no-op change.
    
    Tests use a value of std::numeric_limits<uint64_t>::max() where necessary, to ensure that they're never
    rejected when size isn't being tested.

    5f0f204b74
41. consensus: Move consensus constants into Consensus::Params and consensus.h (as functions)

    The following are now tied to a chain rather than being defined as global
    constants. Their values have not changed.
    
    nMinTxSize
    nMaxBlockSize
    nMaxTxSize
    nMaxBlockSigops
    nCoinbaseMaturity
    
    Also, for free (diff-wise):
    
    Blocksize: Turn MAX_BLOCK_SIZE (nMaxBlockSize) and MAX_BLOCK_SIGOPS (nMaxBlockSigops) into functions
    
    ...which take Consensus::Params as parameter
    This will be convenient to reduce the diff of any proposal that changes the blocksize as a hardfork

    20ceb05e23
42. jtimon force-pushed on Nov 17, 2015
43. Chainparams: Explicit CChainParams arg for main:

    -AcceptBlock
    -AcceptBlockHeader
    -ActivateBestChain
    -ConnectTip
    -InitBlockIndex
    -LoadExternalBlockFile
    -VerifyDB parametric constructor

    8550a273ad
44. Consensus: MOVEONLY: Most important consensus function declarations to consensus/consensus.h 7129375f76
45. Consensus: Chainparams: Explicit Consensus::Params for consensus functions:

    -CheckBlockHeader
    -ContextualCheckBlockHeader
    -CheckBlock
    -ContextualCheckBlock
    
    Also add nTime parameter to CheckBlockHeader and CheckBlock.
    Also use the oportunity to rename the functions inside the Consensus namespace.

    b716f03dba
46. Chainparams: Explicit CChainParams arg for ProcessMessage and ConnectBlock 11bcb7b3e8
47. jtimon force-pushed on Nov 17, 2015
48. 
    jtimon commented at 11:52 am on November 18, 2015: contributor

    Replaced by #7053. Since I think it was extremely useful to group many related PRs under one big “DEPENDENT” one and coordinate how many of them are opened at the same time; at some point after 0.12 is forked I will open and analogous PR to continue this one. But even if I believe that Params() is a great example of an apparently-hard-to-remove global, the next PR will be more generic: there’s more globals to pass down explicitly not just Params() (mempool, mapBlockIndex, chainActive, pindexBestHeader…). So ideally we would make this work per-function rather than per-global. That’s also more parallelizable in terms of people. Explicitly passing all its variables to a function is in principle a simple task, but I think it would be a good opportunity for new developers to get familiarized with the development process (rebases, nits, utACKs, re-utACKs, etc) while the change they’re doing is (while not a priority) something generally desired in the long term, so they can see their PR eventually merged. In any case, the main point of doing it with globals in general is touching function declarations less total times to make the process less disruptive for other unrelated PRs.

    In addition, this was never complete but as new functions are created in main it becomes more incomplete as time passes. Without going function by function, one that I thought only needed Consensus::Params, may need now the full CChainParams while I miss the change in blind rebase. It is not an unsolvable problem, but it would make the refactor more disruptive than needed.

    I will post here to link to that new DEPENDENT PR. In the meantime, if you were interested in this PR, you can review #7053, which is this the last part of this PR that will be opened independently (apart from #6625 which is more part of #6382).

49. jtimon closed this on Nov 18, 2015

    ---

50. MarcoFalke locked this on Sep 8, 2021

Contributors
jtimon dcousens

Labels
Refactoring