Move blocksize and related parameters to consensusparams

theuni commented at 3:07 am on August 6, 2015: member

Firstly, this does not change any consensus parameters or deviate from current behavior.

Instead, hard-coded values are moved to chainparams so that all chains can specify values independently. Further, some values are handled by an interface class so that they may be dynamic based on block time. Each chain is free to implement MaxBlockSize/MaxTxSize/MaxBlockSigops as they wish. This helps to facilitate testing without interrupting mainnet/testnet.

For initial testing, I’ve added a secondary regtest chain with a dynamic blocksize: https://github.com/theuni/bitcoin/blob/blocksize-consensus/src/chainparams.cpp#L90

It’s very simple, but it allows for experimentation. There’s also a test that steps through a few size increases.

My only concern is with the storage of transactions in the wallet: https://github.com/theuni/bitcoin/commit/3872ccf3beff24204ff293db0e81ceb5c7740455#diff-e506682270036e4c8e0b8159a34b652d. We may want to be smarter about considering wallet transactions as valid by differentiating between those known to be in a block and those that aren’t. @jonasschnelli any opinion?

This should not be considered as change to allow for bigger blocks (several pieces are missing in that regard, including p2p changes and blockfile handling), but rather as a logical move of the parameters to a place that makes more sense for them.

in src/wallet/walletdb.cpp: in 13cfdd0b43 outdated

371@@ -371,7 +372,8 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
372             CWalletTx wtx;
373             ssValue >> wtx;
374             CValidationState state;
375-            if (!(CheckTransaction(wtx, state) && (wtx.GetHash() == hash) && state.IsValid()))
376+            const uint64_t nMaxTxSize = consensusParams.MaxTxSize(wtx.GetTxTime());

jonasschnelli commented at 6:59 am on August 6, 2015:

Not sure about that. The wtx.GetTxTime() can response with the stored smartTime and maybe there are cases where not-yet-broadcasted transactions might end up in a return false; and therefore are not added tho the in-memory wtx map.

I’m also not so sure if the CheckTransaction() call makes sense when reading already stored wtx from the disk. A simple check could help to make sure we don’t load wtx from a different net (testnet3 as example),…

in src/chainparamsbase.cpp: in 13cfdd0b43 outdated

107@@ -91,6 +108,7 @@ void SelectBaseParams(CBaseChainParams::Network network)
108 
109 CBaseChainParams::Network NetworkIdFromCommandLine()
110 {
111+    bool fBlockRegTest = GetBoolArg("-blockregtest", false);

jtimon commented at 10:32 pm on August 19, 2015:

Please don’t add yet another non-generic option. See https://github.com/jtimon/bitcoin/commit/acc108a61edcccc6832fd6c1b932b762052f1b0d

theuni commented at 11:24 pm on August 19, 2015:

Ok. There’s no use for this, I only added it because the others are there. I’ll kill it.

in src/consensus/params.h: in 13cfdd0b43 outdated

36@@ -25,6 +37,10 @@ struct Params {
37     int64_t nPowTargetSpacing;
38     int64_t nPowTargetTimespan;
39     int64_t DifficultyAdjustmentInterval() const { return nPowTargetTimespan / nPowTargetSpacing; }
40+    uint64_t MaxBlockSize(uint64_t nCurrentTime) const { return blockParams->MaxBlockSize(nCurrentTime); }

jtimon commented at 10:34 pm on August 19, 2015:

We may need to expose Consensus::Params in libconsensus’ C API and thus it cannot have methods. See https://github.com/jtimon/bitcoin/commit/c4cfb5e098d23e017f98aad93698350bb91960b4

theuni commented at 11:27 pm on August 19, 2015:

Point taken. That’s a bit too simplistic though, since we’re going to have to provide parameters. I’ll play around with it.

jtimon commented at 1:22 am on August 20, 2015:

Well, in https://github.com/jtimon/bitcoin/commit/c4cfb5e098d23e017f98aad93698350bb91960b4 the methods are just functions in consensus/consensus.h that take Consensus::Params as one of their parameters (similar to the libconsensus-ready GetBlockSubsidy()).

theuni commented at 1:55 am on August 20, 2015:

But that means that either:

The function will be the same for all chains
MaxBlockSize will have to do a switch/case on the params and behave appropriately

The first way is out, since it means that (for ex) testnet and mainnet must share behavior for parameters The second way gets ugly as additional chains are added.

I added them as virtuals so that each chain can specify its own behavior.

jtimon commented at 2:16 am on August 20, 2015:

In https://github.com/jtimon/bitcoin/commit/c4cfb5e098d23e017f98aad93698350bb91960b4 the functions are equal for all chains (just like any other consensus function), but the values they take in Consensus::Params are different. You don’t need polymorphism for this: you can add as many attributes as you need in Consensus::Params (even bool ones) and sets their values appropriately. For example,

0uint64_t MaxBlockSize(const Consensu::Params& consensusParams, uint64_t nCurrentTime) const
1{
2        // Increases 1000000 bytes/day starting at the regtest Genesis Block's
3        // timestamp.
4        if (nCurrentTime >= consensusParams.nStartTime + consensusParams.nDay)
5            return consensusParams.nBaseSize + consensusParams.nStep * ((nCurrentTime - consensusParams.nStartTime) / consensusParams.nDay);
6        return consensusParams.nBaseSize;
7}

Just set consensusParams.nStartTime to max<uint64_t> for all the chains but the testing one and you are done. See https://github.com/jtimon/bitcoin/commit/06872b568e9b74fdce3d6e8ff0a15ac3095074d4 too.

in src/consensus/params.h: in 13cfdd0b43 outdated

11@@ -12,8 +12,20 @@ namespace Consensus {
12 /**
13  * Parameters that influence chain consensus.
14  */
15+
16+class CDynamicParams

jtimon commented at 10:36 pm on August 19, 2015:

This class should’t be needed: CChainParams::consensus is not const (even though CChainParams::GetConsensus() returns a const reference). See #6382.

in src/main.cpp: in 13cfdd0b43 outdated

690@@ -692,7 +691,7 @@ unsigned int GetP2SHSigOpCount(const CTransaction& tx, const CCoinsViewCache& in
691 
692 
693 
694-bool CheckTransaction(const CTransaction& tx, CValidationState &state)
695+bool CheckTransaction(const CTransaction& tx, CValidationState &state, uint64_t nMaxTxSize)

jtimon commented at 10:39 pm on August 19, 2015:

Another option is to just pass Consensus::Params. It seems more future-proof to me.

theuni commented at 11:29 pm on August 19, 2015:

sounds good.

dcousens commented at 10:06 pm on August 20, 2015:

Is it though? Introducing complex parameters like that make this function harder to test in isolation.

jtimon commented at 11:24 pm on August 20, 2015:

Many other libconsensus-required functions will take Consensus::Params as a parameter anyway, see #6024 for example.

theuni commented at 11:54 pm on August 20, 2015:

I left this as nMaxTxSize for now because it helps with testing (sometimes you want to test a transaction and ignore the size constraint, so you set it to -1). We could always change this to params later if needed.

jtimon commented at 0:13 am on August 21, 2015:

Ok, it’s fine with me. Was just a suggestion but I didn’t really had a strong opinion here.

jtimon commented at 2:40 pm on September 5, 2015:

@dcousens

Why not just pass consensusParams as you have done for the other two functions? Might as well be consistent.

This is my preference as well. Specially given that almost the other Consensus functions are going to take Consensus::Params as well, see https://github.com/bitcoin/bitcoin/pull/6591/files#diff-cbe22f30d7e480617350ef6ceca97d0cR25

I would also use std::numeric_limits<uint64_t>::max() instead of -1 in the tests. I’m happy to change it in #6625 unless someone strongly opposes to it.

in src/main.cpp: in 13cfdd0b43 outdated

2863@@ -2857,6 +2864,7 @@ bool ProcessNewBlock(CValidationState &state, CNode* pfrom, CBlock* pblock, bool
2864 bool TestBlockValidity(CValidationState &state, const CBlock& block, CBlockIndex * const pindexPrev, bool fCheckPOW, bool fCheckMerkleRoot)
2865 {
2866     const CChainParams& chainparams = Params();
2867+    const Consensus::Params& consensusParams = chainparams.GetConsensus();

jtimon commented at 10:41 pm on August 19, 2015:

Having the chainparams variable already (which should eventually beome a parameter of the function), I would just directly use chainparams.GetConsensus() instead of defining a new variable.

theuni commented at 11:29 pm on August 19, 2015:

ok

in src/merkleblock.cpp: in 13cfdd0b43 outdated

145@@ -147,13 +146,13 @@ CPartialMerkleTree::CPartialMerkleTree(const std::vector<uint256> &vTxid, const
146 
147 CPartialMerkleTree::CPartialMerkleTree() : nTransactions(0), fBad(true) {}
148 
149-uint256 CPartialMerkleTree::ExtractMatches(std::vector<uint256> &vMatch) {
150+uint256 CPartialMerkleTree::ExtractMatches(uint64_t nMaxTransactions, std::vector<uint256> &vMatch) {

jtimon commented at 10:44 pm on August 19, 2015:

Good one.

in src/policy/policy.h: in 13cfdd0b43 outdated

22@@ -24,7 +23,7 @@ static const unsigned int MAX_STANDARD_TX_SIZE = 100000;
23 /** Maximum number of signature check operations in an IsStandard() P2SH script */
24 static const unsigned int MAX_P2SH_SIGOPS = 15;
25 /** The maximum number of sigops we're willing to relay/mine in a single tx */
26-static const unsigned int MAX_STANDARD_TX_SIGOPS = MAX_BLOCK_SIGOPS/5;
27+static const unsigned int MAX_STANDARD_TX_SIGOPS = MAX_STANDARD_TX_SIZE/25; // one sigop per 25 bytes

jtimon commented at 10:45 pm on August 19, 2015:

We can also just turn this into a number instead.

in src/consensus/consensus.h: in 13cfdd0b43 outdated

-1@@ -1,16 +0,0 @@
0-// Copyright (c) 2009-2010 Satoshi Nakamoto

jtimon commented at 10:53 pm on August 19, 2015:

consensus/consensus.h is going to be destroyed before I was able to move a single function declaration into it…Sniff sniff

jtimon commented at 9:06 pm on August 26, 2015:

…unless we merge something like #6009 first or we keep the new functions (something like https://github.com/jtimon/bitcoin/commit/c59f170fd15688499f7f6291e2ef0cf18a43fcc0#diff-cbe22f30d7e480617350ef6ceca97d0cR12 ) here. It took very long to create this consensus/consensus.h little step forward, it seems wasteful to remove all those includes (when most of them will have to be reintroduced later).

jtimon commented at 10:56 pm on August 19, 2015: contributor

Apart from the nits, I think always using nHeight is more general as discussed in http://lists.linuxfoundation.org/pipermail/bitcoin-dev/2015-July/009731.html (there are other advantages described there [for example, it will be simpler to do the equivalent of #5966 in the future if we use nHeight from the start]).

theuni commented at 11:36 pm on August 19, 2015: member

@jtimon I’d rather not get into that here. I added the commits such that we could drop 3872ccf..end, then debate the function part later. Looks like I should’ve started with that.

I’ll drop those, so this becomes a relatively simple move into chainparams.

jtimon commented at 1:26 am on August 20, 2015: contributor

Yes, I don’t want to debate the concrete activation mechanism here either. It’s better to that that in the ml thread.

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

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

This is a no-op change. For now, everything passes MAX_BLOCK_SIZE, so the
result matches what it would've before.

Tests use a value of -1 where necessary, to ensure that they're never
rejected when size isn't being tested.

c3763587d3

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.

af428641ac

consensus: make CheckBlock take a Consensus Params arg

This makes it easier to explicitly add unit tests for a single chain

4412d6099a

consensus: Move consensus constants into Consensus::Params

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

5345f2e5ab

consensus: No need for consensus.h anymore 8fc200f1d4

theuni force-pushed on Aug 20, 2015

theuni commented at 9:30 pm on August 20, 2015: member

I’ve trimmed this down to remove the controversial parts, basically only code movement remains.

dcousens commented at 10:10 pm on August 20, 2015: contributor

utACK

laanwj added the label Refactoring on Aug 24, 2015

sipa commented at 11:44 pm on August 25, 2015: member

utACK

jtimon commented at 11:30 pm on August 26, 2015: contributor

Untested ACK, but I would be really happy if #6591 could be merged before this is.

jtimon commented at 0:15 am on September 3, 2015: contributor

Please, reviewers, consider the alternative in #6625, I promise it won’t take much longer if you have already reviewed this.

jtimon commented at 8:11 pm on September 14, 2015: contributor

Close in favor of #6672 ?

jtimon commented at 4:11 pm on October 20, 2015: contributor

This needs rebase, but the almost-equivalent #6625 is rebased on top of master and reopened.

jtimon commented at 6:51 pm on March 16, 2016: contributor

I’m still generally in favor of this. I think the sooner these preparations are done, the sooner it we can stop worrying about them interfering with other changes, or the necessary preparations becoming bigger because new changes are using these constants somewhere else (something that has happened several times after the last rebase of this PR as documented with the required rebases in #6625 and #6672) I just got bored of rebasing the version with my nits squashed (#6625) and the version in #6672 was said to need a document with pictures to be merged.

TL;DR Rebase or close for now?

theuni closed this on Jun 10, 2016

MarcoFalke locked this on Sep 8, 2021

Move blocksize and related parameters to consensusparams #6526