Skip to content

DEPENDENT: Globals: Avoid calling Params()#5970

Closed
jtimon wants to merge 8 commits intobitcoin:masterfrom
jtimon:chainparams_cleanup
Closed

DEPENDENT: Globals: Avoid calling Params()#5970
jtimon wants to merge 8 commits intobitcoin:masterfrom
jtimon:chainparams_cleanup

Conversation

@jtimon
Copy link
Contributor

@jtimon jtimon commented Apr 4, 2015

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

Dependencies:

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.

@jtimon jtimon changed the title Chainparams cleanup Dependent: Chainparams cleanup Apr 4, 2015
This was referenced Apr 8, 2015
Merged
Merged
@jtimon jtimon force-pushed the chainparams_cleanup branch 2 times, most recently from 0b4d429 to 9a2b7d3 Compare April 8, 2015 15:24
@jtimon
Copy link
Contributor Author

jtimon commented Apr 8, 2015

Note that AcceptToMemoryPool is ignored because the calls to Params() there will be replaced with policy initialization in init.cpp.

@jtimon jtimon force-pushed the chainparams_cleanup branch 2 times, most recently from 018f16e to 69dcb20 Compare April 9, 2015 16:52
@jtimon jtimon changed the title Dependent: Chainparams cleanup DEPENDENT: Chainparams cleanup Apr 10, 2015
@jtimon jtimon force-pushed the chainparams_cleanup branch 3 times, most recently from bce9d09 to 426b8a8 Compare April 10, 2015 17:49
This was referenced Apr 10, 2015
Merged
Merged
Merged
@jtimon jtimon force-pushed the chainparams_cleanup branch 2 times, most recently from 0fa8e89 to 80b1eb5 Compare April 17, 2015 00:41
@jtimon jtimon mentioned this pull request Apr 17, 2015
Closed
@jtimon jtimon force-pushed the chainparams_cleanup branch 3 times, most recently from e0e2102 to b8e6dc3 Compare April 19, 2015 22:36
@jtimon jtimon mentioned this pull request Apr 19, 2015
Merged
@jtimon
Copy link
Contributor Author

jtimon commented Apr 23, 2015

Closing for now, although I will keep updating the dependencies and rebasing.

@jtimon jtimon closed this Apr 23, 2015
@jtimon jtimon reopened this May 19, 2015
@jtimon jtimon force-pushed the chainparams_cleanup branch from b8e6dc3 to 66b91b2 Compare May 19, 2015 23:07
@jtimon jtimon closed this May 19, 2015
@jtimon jtimon mentioned this pull request Jul 1, 2015
Closed
@jtimon jtimon mentioned this pull request Jul 23, 2015
Merged
@dcousens
Copy link
Contributor

dcousens commented Aug 27, 2015

Comparison URL to review this PR independently of its dependent: jtimon/bitcoin@consensus-params-0.12.99...jtimon:chainparams_cleanup

@jtimon jtimon force-pushed the chainparams_cleanup branch 2 times, most recently from a1f7b3a to a8fe228 Compare August 29, 2015 02:40
@jtimon jtimon mentioned this pull request Sep 2, 2015
Closed
@jtimon jtimon force-pushed the chainparams_cleanup branch from a8fe228 to 0e1d15b Compare September 3, 2015 00:13
@jtimon
Copy link
Contributor Author

jtimon commented Sep 3, 2015

Rebased on top of #6591

@jtimon jtimon force-pushed the chainparams_cleanup branch from 0e1d15b to 9298359 Compare September 12, 2015 16:54
@jtimon jtimon mentioned this pull request Sep 15, 2015
Closed
@jtimon jtimon force-pushed the chainparams_cleanup branch 2 times, most recently from 2b3dd9a to 0664e23 Compare November 11, 2015 00:31
@jtimon jtimon changed the title DEPENDENT: Chainparams cleanup DEPENDENT: Globals: Avoid calling Params() Nov 11, 2015
@jtimon jtimon force-pushed the chainparams_cleanup branch from 0664e23 to 2bcce4e Compare November 11, 2015 12:22
@jtimon jtimon mentioned this pull request Nov 11, 2015
Merged
@jtimon
Copy link
Contributor Author

jtimon commented Nov 11, 2015

Rebased after #6163 and #6982 have been merged, opened #6986.

@jtimon jtimon force-pushed the chainparams_cleanup branch from 2bcce4e to 3de575a Compare November 11, 2015 14:18
@jtimon jtimon mentioned this pull request Nov 17, 2015
Closed
@theuni @jtimon
consensus: don't define MAX_STANDARD_TX_SIGOPS in terms of block size
25d5800
@theuni @jtimon
consensus: teach ExtractMatches to check for an arbitrary max transac…
0b92e2f 
…tion 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.
@theuni @jtimon
consensus: teach CheckTransaction to check for an arbitrary max tx size
5f0f204 
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.
@theuni @jtimon
consensus: Move consensus constants into Consensus::Params and consen…
20ceb05 
…sus.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
@jtimon jtimon force-pushed the chainparams_cleanup branch from 3de575a to e3e49f6 Compare November 17, 2015 15:59
@jtimon
Chainparams: Explicit CChainParams arg for main:
8550a27 
-AcceptBlock
-AcceptBlockHeader
-ActivateBestChain
-ConnectTip
-InitBlockIndex
-LoadExternalBlockFile
-VerifyDB parametric constructor
@jtimon
Consensus: MOVEONLY: Most important consensus function declarations t…
7129375 
…o consensus/consensus.h
@jtimon
Consensus: Chainparams: Explicit Consensus::Params for consensus func…
b716f03 
…tions:

-CheckBlockHeader
-ContextualCheckBlockHeader
-CheckBlock
-ContextualCheckBlock

Also add nTime parameter to CheckBlockHeader and CheckBlock.
Also use the oportunity to rename the functions inside the Consensus namespace.
@jtimon
Chainparams: Explicit CChainParams arg for ProcessMessage and Connect…
11bcb7b 
…Block
@jtimon jtimon force-pushed the chainparams_cleanup branch from e3e49f6 to 11bcb7b Compare November 17, 2015 16:32
@jtimon jtimon mentioned this pull request Nov 18, 2015
Merged
@jtimon
Copy link
Contributor Author

jtimon commented Nov 18, 2015

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).

@jtimon jtimon closed this Nov 18, 2015
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jtimon @dcousens @laanwj @theuni