Move blocksize and related parameters to consensusparams #6526
Conversation
src/wallet/walletdb.cpp
Outdated
There was a problem hiding this comment.
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),...
src/chainparamsbase.cpp
Outdated
There was a problem hiding this comment.
Please don't add yet another non-generic option. See jtimon@acc108a
There was a problem hiding this comment.
Ok. There's no use for this, I only added it because the others are there. I'll kill it.
|
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]). |
|
@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. |
|
Yes, I don't want to debate the concrete activation mechanism here either. It's better to that that in the ml thread. |
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.
…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 makes it easier to explicitly add unit tests for a single chain
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
theuni
force-pushed
the
blocksize-consensus
branch
from
13cfdd0 to
8fc200f
Compare
|
I've trimmed this down to remove the controversial parts, basically only code movement remains. |
|
utACK |
|
utACK |
|
Untested ACK, but I would be really happy if #6591 could be merged before this is. |
|
Please, reviewers, consider the alternative in #6625, I promise it won't take much longer if you have already reviewed this. |
|
Close in favor of #6672 ? |
|
This needs rebase, but the almost-equivalent #6625 is rebased on top of master and reopened. |
|
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) TL;DR Rebase or close for now? |
6 participants
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: theuni@3872ccf#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.