Chainparams: Explicit Consensus::Params arg for almost all remaining functions #6163

jtimon · 2015-05-19T23:02:13Z

Explicitly pass Consensus::Params to functions:

-CheckBlockIndex
-DisconnectTip
-GetTransaction
-InvalidateBlock
-ProcessGetData
-ReadBlockFromDisk

Without counting the following consensus functions (which is done in #6591):

-CheckBlockHeader
-CheckBlock
-ContextualCheckBlockHeader
-ContextualCheckBlock

These are all the functions that use Consensus::Params but not CChainParams.

laanwj · 2015-05-20T11:12:36Z

utACK

jtimon · 2015-06-02T11:47:51Z

Needed rebase after merging #5669 and #6173 (1)

jtimon · 2015-06-21T12:10:39Z

Needed rebase (2)

jtimon · 2015-07-04T09:15:56Z

ping

theuni · 2015-07-22T21:53:03Z

ut ACK

jgarzik · 2015-07-23T19:29:45Z

Asking a dumb but sincere question: why?

These parameters are global and constant to main, the module being modified. This change appears to add parameters for little apparent gain.

Is this an intermediate step to the code being moved to libconsensus?

jtimon · 2015-07-23T19:43:46Z

Well, these are actually the methods that are not required for libconsensus but still use Consensus::Params.
This is part of a bigger process of making CChainParams being explicitly passed as a const reference instead of using the Params() function all around. Basically that function is a way to hide a global that @theuni and I want to take out of chainparams.

Passing it explicitly as a parameter is also good for testing. Eventually we should be able to remove this line: https://github.com/bitcoin/bitcoin/blob/master/src/test/test_bitcoin.cpp#L36

#5970 is closed but contains this and more of the same, to get a better view of what all this is about.
Some of the changes there are no longer necessary because other PRs have kindly incorporated it when they were changing the function's parameters already.

jtimon · 2015-08-20T23:51:21Z

Needed rebase. (3)

jonasschnelli · 2015-08-27T07:39:10Z

src/main.cpp

@@ -2264,6 +2267,7 @@ static bool ActivateBestChainStep(CValidationState &state, CBlockIndex *pindexMo
 * that is already loaded (to avoid loading it again from disk).
 */
 bool ActivateBestChain(CValidationState &state, const CBlock *pblock) {
+    const CChainParams& chainparams = Params();
    CBlockIndex *pindexNewTip = NULL;
    CBlockIndex *pindexMostWork = NULL;
    const CChainParams& chainParams = Params();


Dup of +L2270?

How did this even compile?

chainparams != chainParams (mind the uppercase P).

@jtimon s/chainparams/chainParams/g might be in order 👍

Oops, lost in rebase.
Thanks @jonasschnelli @dcousens for the extremely fast and helpful review. I will force push this and #5970 with the fix.

I'm going to maintain chainparams instead of chainParams for uniformity across the codebase.
When @theuni asked me #XXXX not to use "params" for both CChainParams and Consensus::Params I used "consensusParams" and "chainparams", but he meant "chainParams" (btw pow.cpp still uses "params" #5812, no big deal).
Now I'm afraid is too late for chainParams, even if he used that in one of his improvements to the checkpoints code #YYYY.
I can dig all the PR numbers if anyone is interested, but I don't have them at hand.
Sorry for the misunderstanding between theuni and me, mostly my fault since I was the listener.
I don't think theuni is married to chainParams, I'm sure he's fine with chainparams, he seemed mostly concerned about using the same variable name for 2 different types, I was just removing "Params()." all around.

jtimon · 2015-08-27T09:06:11Z

@jonasschnelli 's nit solved, faster with the help of @dcousens .

jonasschnelli · 2015-08-27T11:11:42Z

Tested ACK (ran unit tests, did fiddle around with it in regtest).
Gitian build: https://builds.jonasschnelli.ch/pulls/6163/

dcousens · 2015-08-27T11:26:33Z

src/main.cpp

@@ -2063,13 +2064,14 @@ static int64_t nTimePostConnect = 0;
 * corresponding to pindexNew, to bypass loading it again from disk.
 */
 bool static ConnectTip(CValidationState &state, CBlockIndex *pindexNew, const CBlock *pblock) {
+    const CChainParams& chainparams = Params();


Nit, why did you opt for chainparams over chainParams? camelCase is used consistently for most other variables.

Yep, but chainparams is used in more places than chainParams (my fault) so it seems too late. I explained this in more detail here: #6163 (comment)

dcousens · 2015-08-27T11:28:56Z

utACK and concept ACK

jtimon · 2015-10-20T17:42:06Z

Needed rebase (4).

jtimon · 2015-10-30T11:28:30Z

Rebased (4).

maflcko · 2015-10-30T12:08:48Z

Needs to fix https://github.com/bitcoin/bitcoin/blob/master/src/zmq/zmqpublishnotifier.cpp#L149 et al as well.

-CheckBlockIndex -DisconnectTip -GetTransaction -InvalidateBlock -ProcessGetData -ReadBlockFromDisk

jtimon · 2015-10-30T13:08:02Z

@MarcoFalke Yep, that's all I could see in travis, but I wanted to build src/zmq/zmqpublishnotifier.o locally before squashing. Squashing now...

87cbdb8 Globals: Explicit Consensus::Params arg for main: (Jorge Timón)

..and at the same time prevent AcceptBlockHeader() from calling global function Params()

7267843 Globals: Make AcceptBlockHeader static (Fix #6163) (Jorge Timón)

Bitcoin Core refactoring and cleanups 1 Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#5994 - bitcoin/bitcoin#6538 - bitcoin/bitcoin#6163 - bitcoin/bitcoin#6982 - bitcoin/bitcoin#6986 - bitcoin/bitcoin#7053 - bitcoin/bitcoin#7444 - bitcoin/bitcoin#7793 - Excluding some comments in `txmempool.h` on code we haven't yet pulled in. - bitcoin/bitcoin#7916 - bitcoin/bitcoin#7815 Additionally, the Equihash parameters are moved into the consensus parameters. Part of #2074.

Bitcoin Core refactoring and cleanups 1 Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6538 - bitcoin/bitcoin#6163 - bitcoin/bitcoin#6982 - bitcoin/bitcoin#6986 - bitcoin/bitcoin#7053 - bitcoin/bitcoin#7444 - bitcoin/bitcoin#7793 - Excluding some comments in `txmempool.h` on code we haven't yet pulled in. - bitcoin/bitcoin#7916 - bitcoin/bitcoin#7815 Additionally, the Equihash parameters are moved into the consensus parameters, and various other functions in `main.cpp` have `const CChainParams&` arguments added. Part of #2074.

jtimon changed the title ~~Chainparams: Explicit Consensus::Params arg for main:~~ Chainparams: Explicit Consensus::Params arg for almost all remaining functions May 19, 2015

laanwj added the Refactoring label May 20, 2015

This was referenced May 20, 2015

Chainparams: Remove redundant getter CChainParams::SubsidyHalvingInterval() #5996

Merged

DEPENDENT: Globals: Avoid calling Params() #5970

Closed

jtimon force-pushed the chainparams-remaining-consensus branch from 9c92da0 to 273606b Compare June 2, 2015 11:46

jtimon force-pushed the chainparams-remaining-consensus branch from 273606b to 16ccaef Compare June 21, 2015 12:10

jtimon force-pushed the chainparams-remaining-consensus branch from 16ccaef to bf790b2 Compare June 21, 2015 12:15

jtimon mentioned this pull request Jul 1, 2015

Chainparams: Separate globals and depedendent functions #6359

Closed

jtimon mentioned this pull request Jul 24, 2015

Move message processing to new 'procmsg' module. #4646

Closed

jtimon force-pushed the chainparams-remaining-consensus branch from bf790b2 to 48c8e9d Compare August 20, 2015 23:50

jonasschnelli reviewed Aug 27, 2015
View reviewed changes

jtimon force-pushed the chainparams-remaining-consensus branch from 48c8e9d to 99112df Compare August 27, 2015 09:01

jtimon force-pushed the chainparams-remaining-consensus branch from 99112df to 0aac17a Compare August 27, 2015 09:13

dcousens reviewed Aug 27, 2015
View reviewed changes

jtimon force-pushed the chainparams-remaining-consensus branch from 0aac17a to f3ace17 Compare October 20, 2015 17:40

jtimon force-pushed the chainparams-remaining-consensus branch from f3ace17 to 4b8de76 Compare October 30, 2015 11:24

jtimon force-pushed the chainparams-remaining-consensus branch from 4b8de76 to 7d9057a Compare October 30, 2015 11:31

jtimon force-pushed the chainparams-remaining-consensus branch from 7d9057a to b8fb6c2 Compare October 30, 2015 12:24

Globals: Explicit Consensus::Params arg for main:

87cbdb8

-CheckBlockIndex -DisconnectTip -GetTransaction -InvalidateBlock -ProcessGetData -ReadBlockFromDisk

jtimon force-pushed the chainparams-remaining-consensus branch from ad15ff6 to 87cbdb8 Compare October 30, 2015 13:07

laanwj merged commit 87cbdb8 into bitcoin:master Nov 10, 2015

laanwj added a commit that referenced this pull request Nov 10, 2015

Merge pull request #6163

77beab7

87cbdb8 Globals: Explicit Consensus::Params arg for main: (Jorge Timón)

jtimon mentioned this pull request Nov 10, 2015

Globals: Make AcceptBlockHeader static (Fix #6163) (AcceptBlockHeader) #6982

Merged

jtimon added a commit to jtimon/bitcoin that referenced this pull request Nov 11, 2015

Globals: Make AcceptBlockHeader static (Fix bitcoin#6163)

7267843

..and at the same time prevent AcceptBlockHeader() from calling global function Params()

laanwj added a commit that referenced this pull request Nov 11, 2015

Merge pull request #6982

cb841e7

7267843 Globals: Make AcceptBlockHeader static (Fix #6163) (Jorge Timón)

jtimon mentioned this pull request Apr 6, 2016

Globals: TODO: Experiment: Kill "Params()" #7829

Closed

15 tasks

str4d mentioned this pull request Jan 27, 2018

Bitcoin Core refactoring and cleanups 1 zcash/zcash#2912

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Chainparams: Explicit Consensus::Params arg for almost all remaining functions #6163

Chainparams: Explicit Consensus::Params arg for almost all remaining functions #6163

Uh oh!

Conversation

jtimon commented May 19, 2015

Uh oh!

laanwj commented May 20, 2015

Uh oh!

jtimon commented Jun 2, 2015

Uh oh!

jtimon commented Jun 21, 2015

Uh oh!

jtimon commented Jul 4, 2015

Uh oh!

theuni commented Jul 22, 2015

Uh oh!

jgarzik commented Jul 23, 2015

Uh oh!

jtimon commented Jul 23, 2015

Uh oh!

jtimon commented Aug 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Aug 27, 2015

Uh oh!

jonasschnelli commented Aug 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcousens commented Aug 27, 2015

Uh oh!

jtimon commented Oct 20, 2015

Uh oh!

jtimon commented Oct 30, 2015

Uh oh!

maflcko commented Oct 30, 2015

Uh oh!

jtimon commented Oct 30, 2015

Uh oh!

Uh oh!