BLOCKING: Consensus: Move blocksize and related parameters to consensusparams ...without removing consensus/consensus.h [#6526 alternative] #6625

jtimon · 2015-09-02T23:26:24Z

This is a replacement for #6526 with my nits integrated in the commits where they were more relevant.
It has a smaller diff (138 additions and 65 deletions VS +116 −95) is more amenable to any block size change proposal (many of them will need a function), to #6382 (adding new test chains for block size tests) and to libconsenusus-related #6591 (which is also part of the process of making chainparams less global #5970).
I will rebase #6382, #6591 and #5970 on top of this. Please, @theuni close #6526 in favor of this if it passes your review.

Blocking:

Consensus: Adapt declarations of most obviously consensus functions Consensus: Adapt declarations of most obviously consensus functions #6591 [jtimon/consensus-params-0.12.99]
Chainparams cleanup DEPENDENT: Globals: Avoid calling Params() #5970 [chainparams_cleanup](depends on Chainparams: Explicit Consensus::Params arg for almost all remaining functions #6163)
Chainparams: Introduce std::numeric_limits<uint64_t>::max() new testnet chains to test different block sizes DEPENDENT: Chainparams: Introduce N testnet chains to test different block sizes #6382 [chainparams-sizetest-0.11.99]

dcousens · 2015-09-03T01:37:26Z

is more amenable to any block size change proposal

@jtimon can you summarize why? #6526 was relatively concise already, and manually diffing that against this doesn't sound like a great use of time.

jtimon · 2015-09-03T02:37:03Z

@jtimon can you summarize why? #6526 was relatively concise already, and manually diffing that against this doesn't sound like a great use of time.

In summary, this doesn't destroy consensus/consensus.h (doesn't undo costly refactor work #5970), this doesn't s/Params().GetConsensus()/consensusParams/ when it's not necessary (because the function depends on CChainParams, see #XXXX), and it goes further in relation to facilitate implementations of consensus block size changes by creating MaxBlockSize() and MaxBlockSigops() (which is diff-free when you're replacing MAX_BLOCK_SIZE with consensusParams.nMaxBlockSize anyway).

jtimon · 2015-09-04T20:18:15Z

Force pushed with a fix in diff --git a/src/test/test_bitcoin.cpp b/src/test/test_bitcoin.cpp (for those who found the time to fetch a). CBaseChainParams::REGTEST was being used more than once in src/test/test_bitcoin.cpp and that caused problems in #6382 .

dcousens · 2015-09-05T01:19:27Z

src/main.cpp

-    if (block.vtx.empty() || block.vtx.size() > MAX_BLOCK_SIZE || ::GetSerializeSize(block, SER_NETWORK, PROTOCOL_VERSION) > MAX_BLOCK_SIZE)
+    uint64_t nMaxBlockSize = MaxBlockSize(consensusParams);
+    uint64_t nMaxBlockSigops = MaxBlockSigops(consensusParams);
+    uint64_t nMaxTxSize = consensusParams.nMaxTxSize;


Why was this pulled out as a variable?

No particular reason, ask @theuni

dcousens · 2015-09-05T01:22:27Z

utACK

jtimon · 2015-09-06T03:44:56Z

Added a squashme commit that should solve @dcousens' nits (ie Consensus::CheckTx() gets const Consensus::Params& consensusParams instead of uint64_t nMaxTxSize).
EDIT: Here's the squashed version: 0cac6d2 (also declaring Consensus::CheckTxInputs in consensus.h and adding some documentation to the function).

jtimon · 2015-09-12T15:00:14Z

Updated with the squashed version.

jtimon · 2015-09-14T20:11:15Z

Closing in favor of #6672.

jtimon · 2015-10-20T16:01:22Z

Rebased and reopened.

dcousens · 2015-10-21T01:45:18Z

re-ACK, but the constant shifting of code to different [but almost the same] PRs is getting insane.
I'd rather not have to ACK this again.

maflcko · 2015-11-03T12:20:01Z

src/txmempool.cpp

@@ -484,7 +486,7 @@ void CTxMemPool::removeCoinbaseSpends(const CCoinsViewCache *pcoins, unsigned in
                continue;
            const CCoins *coins = pcoins->AccessCoins(txin.prevout.hash);
            if (fSanityCheck) assert(coins);


@jtimon Needs trivial rebase due to e06c14f.

jtimon · 2015-11-03T13:52:37Z

@MarcoFalke Thanks, rebased.

jtimon · 2015-11-03T14:49:56Z

New uses of MAX_BLOCK_SIZE/MaxBlockSize() were introduced in #6622.

jtimon · 2015-11-11T14:45:49Z

Rebased with a tiny fix that would have break the build (if this has been merged without the fix).
I haven't squashed the new commits (from what was contained and reviewed in #6672) to facilitate re-review.

jtimon · 2015-11-17T15:57:57Z

Great, I was thinking about leaving CheckTransaction/Consensus::CheckTx declared in main.h if we were going for the 2/3 commits option and that was a problem.

btcdrak · 2015-11-17T15:59:33Z

I agree with @sipa, it doesn't look too disruptive as-is.

jtimon · 2015-11-27T13:28:58Z

Needed trivial rebase.

maflcko · 2015-11-28T22:15:25Z

utACK 82b2443

btcdrak · 2015-11-28T22:42:05Z

utACK

jtimon · 2015-12-01T21:45:02Z

Rebased(6)

dcousens · 2015-12-02T02:04:30Z

Re-ACK, @laanwj anything holding this back?

jtimon · 2015-12-03T11:57:57Z

Rebased(7)
Like I said the only disruptive commit is the last one and is the one that is requiring rebase all the time. Prediction: Next time this PR requires rebase, it will probably be due to the last commit and master...jtimon:incomplete-consensus-blocksize-0.12.99 will remain without conflicts. The third commit is somewhat disruptive too, so I may be wrong, but if I am right, I will just drop the last commit, sorry @theuni

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

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.

…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 · 2015-12-08T22:43:40Z

@gmaxwell

Bitcoin will be able to move forward with these increases when improvements and understanding render their risks widely acceptable relative to the risks of not deploying them. In Bitcoin Core we should keep patches ready to implement them as the need and the will arises, to keep the basic software engineering from being the limiting factor.

This would make it much easier to maintain those ready "patches" (even the simple bip102 has rapidly bitrot). If ithad been merged for 0.12 like was supposed to be the plan (according to sipa and btcdrak comments), that would have also make those pat hes easier to backport to 0.12.
Many people including you reviewed this as part of the closed #6672 , only a few small changes required for rebase has been added.
I'm sorry if I sound repetitive but I can't really understand what is holding this for so, so long.

dcousens · 2015-12-09T00:03:53Z

This was ACK'd for 0.12 a while ago, it should still be able to make it in right?

maflcko · 2015-12-09T00:12:35Z

Yes, if this get's merged in master, at least the first three commits should be ~~backported~~ (can be merged as is).

devrandom · 2015-12-10T19:05:12Z

ACK

jgarzik · 2015-12-18T05:25:56Z

Structure seems reasonable, with the caveat that MaxBlockSize() and MaxBlockSigOps() will inevitably be modified depending on what state they examine.

jtimon · 2015-12-18T11:57:19Z

Yeah, when I changed the variable in the Consensus::Params struct (what @cfields had initially) to a function I wasn't sure on whether to use block.nHeight or header.mediaTime(), certainly not header.nTime. If we can finally agree on that somewhere I don't mind to change it.

jgarzik · 2015-12-18T13:31:52Z

ut ACK, for the record

jtimon · 2015-12-21T07:06:13Z

Replaced by #7238 (a subset of this) with @rustyrussell 's nit fixed.

This was referenced Sep 2, 2015

Consensus: Adapt declarations of most obviously consensus functions #6591

Closed

Move blocksize and related parameters to consensusparams #6526

Closed

laanwj added the Refactoring label Sep 4, 2015

jtimon mentioned this pull request Sep 4, 2015

DEPENDENT: Chainparams: Introduce N testnet chains to test different block sizes #6382

Closed

12 tasks

jtimon force-pushed the consensus-blocksize-0.12.99 branch from 989e374 to 4a96d92 Compare September 4, 2015 20:12

jtimon mentioned this pull request Sep 4, 2015

DEPENDENT: Globals: Avoid calling Params() #5970

Closed

14 tasks

dcousens reviewed Sep 5, 2015
View reviewed changes

jtimon mentioned this pull request Sep 6, 2015

Optimizations: Consensus: In AcceptToMemoryPool, ConnectBlock, and CreateNewBlock #6445

Closed

jtimon force-pushed the consensus-blocksize-0.12.99 branch from 820c84c to 0cac6d2 Compare September 12, 2015 14:59

jtimon mentioned this pull request Sep 14, 2015

Consensus: Separate most consensus functions to consensus.cpp #6672

Closed

jtimon closed this Sep 14, 2015

jtimon reopened this Oct 20, 2015

jtimon force-pushed the consensus-blocksize-0.12.99 branch from 0cac6d2 to dc23326 Compare October 20, 2015 16:01

jtimon mentioned this pull request Oct 30, 2015

BIP 102: Increase block size limit to 2MB #6451

Closed

maflcko reviewed Nov 3, 2015
View reviewed changes

jtimon force-pushed the consensus-blocksize-0.12.99 branch from dc23326 to f45ede4 Compare November 3, 2015 13:52

jtimon force-pushed the consensus-blocksize-0.12.99 branch from a2f24e0 to b89e8bb Compare November 11, 2015 14:42

jtimon force-pushed the consensus-blocksize-0.12.99 branch from 20ceb05 to 82b2443 Compare November 27, 2015 13:28

jtimon force-pushed the consensus-blocksize-0.12.99 branch 2 times, most recently from e9a9115 to e66e102 Compare December 1, 2015 22:13

jtimon force-pushed the consensus-blocksize-0.12.99 branch from e66e102 to 4334ace Compare December 3, 2015 11:52

theuni added 4 commits December 3, 2015 14:47

consensus: don't define MAX_STANDARD_TX_SIGOPS in terms of block size

ccae215

consensus: teach CheckTransaction to check for an arbitrary max tx size

654c7c6

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.

jtimon force-pushed the consensus-blocksize-0.12.99 branch from 4334ace to 6111e5b Compare December 3, 2015 13:48

jtimon closed this Dec 18, 2015

jtimon reopened this Dec 18, 2015

jtimon mentioned this pull request Dec 21, 2015

Blocksize: Some small preparations for a blocksize hardfork #7238

Closed

jtimon closed this Dec 21, 2015

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

BLOCKING: Consensus: Move blocksize and related parameters to consensusparams ...without removing consensus/consensus.h [#6526 alternative] #6625

BLOCKING: Consensus: Move blocksize and related parameters to consensusparams ...without removing consensus/consensus.h [#6526 alternative] #6625

Uh oh!

Conversation

jtimon commented Sep 2, 2015

Uh oh!

dcousens commented Sep 3, 2015

Uh oh!

jtimon commented Sep 3, 2015

Uh oh!

jtimon commented Sep 4, 2015

Uh oh!

dcousens Sep 5, 2015

Choose a reason for hiding this comment

Uh oh!

jtimon Sep 5, 2015

Choose a reason for hiding this comment

Uh oh!

dcousens commented Sep 5, 2015

Uh oh!

jtimon commented Sep 6, 2015

Uh oh!

jtimon commented Sep 12, 2015

Uh oh!

jtimon commented Sep 14, 2015

Uh oh!

jtimon commented Oct 20, 2015

Uh oh!

dcousens commented Oct 21, 2015

Uh oh!

maflcko Nov 3, 2015

Choose a reason for hiding this comment

Uh oh!

jtimon commented Nov 3, 2015

Uh oh!

jtimon commented Nov 3, 2015

Uh oh!

jtimon commented Nov 11, 2015

Uh oh!

jtimon commented Nov 17, 2015

Uh oh!

btcdrak commented Nov 17, 2015

Uh oh!

jtimon commented Nov 27, 2015

Uh oh!

maflcko commented Nov 28, 2015

Uh oh!

btcdrak commented Nov 28, 2015

Uh oh!

jtimon commented Dec 1, 2015

Uh oh!

dcousens commented Dec 2, 2015

Uh oh!

jtimon commented Dec 3, 2015

Uh oh!

jtimon commented Dec 8, 2015

Uh oh!

dcousens commented Dec 9, 2015

Uh oh!

maflcko commented Dec 9, 2015

Uh oh!

devrandom commented Dec 10, 2015

Uh oh!

jgarzik commented Dec 18, 2015

Uh oh!

jtimon commented Dec 18, 2015

Uh oh!

jgarzik commented Dec 18, 2015

Uh oh!

jtimon commented Dec 21, 2015

Uh oh!

Uh oh!