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

pull jtimon wants to merge 4 commits into bitcoin:master from jtimon:consensus-blocksize-0.12.99 changing 23 files +157 −67
  1. jtimon commented at 11:26 pm on September 2, 2015: contributor

    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 #6591 [jtimon/consensus-params-0.12.99]
    • Chainparams cleanup #5970 [chainparams_cleanup](depends on #6163)
    • Chainparams: Introduce std::numeric_limits<uint64_t>::max() new testnet chains to test different block sizes #6382 [chainparams-sizetest-0.11.99]
  2. dcousens commented at 1:37 am on September 3, 2015: contributor

    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.

  3. jtimon commented at 2:37 am on September 3, 2015: contributor

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

  4. laanwj added the label Refactoring on Sep 4, 2015
  5. jtimon force-pushed on Sep 4, 2015
  6. jtimon commented at 8:18 pm on September 4, 2015: contributor
    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 .
  7. in src/main.cpp: in 4a96d92a4a outdated 
    2594@@ -2591,7 +2595,10 @@ bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW, bo
2595     // because we receive the wrong transactions for it.
2596 
2597     // Size limits
2598-    if (block.vtx.empty() || block.vtx.size() > MAX_BLOCK_SIZE || ::GetSerializeSize(block, SER_NETWORK, PROTOCOL_VERSION) > MAX_BLOCK_SIZE)
2599+    uint64_t nMaxBlockSize = MaxBlockSize(consensusParams);
2600+    uint64_t nMaxBlockSigops = MaxBlockSigops(consensusParams);
2601+    uint64_t nMaxTxSize = consensusParams.nMaxTxSize;
    dcousens commented at 1:19 am on September 5, 2015:
    Why was this pulled out as a variable?
    jtimon commented at 2:30 pm on September 5, 2015:
    No particular reason, ask @theuni
  8. in src/consensus/consensus.h: in 4a96d92a4a outdated 
    16+/**
17+ * Consensus validations:
18+ * Check_ means checking everything possible with the data provided.
19+ * Verify_ means all data provided was enough for this level and its "consensus-verified".
20+ */
21+namespace Consensus {
    dcousens commented at 1:22 am on September 5, 2015:
    Why is there no indentation for this namespace block?
    jtimon commented at 2:32 pm on September 5, 2015:
    Because we don’t indent namespaces: https://github.com/bitcoin/bitcoin/blob/master/src/.clang-format#L31
    dcousens commented at 3:58 pm on September 5, 2015:
    OK, thanks for the heads up :+1:
  9. dcousens commented at 1:22 am on September 5, 2015: contributor
    utACK
  10. jtimon commented at 3:44 am on September 6, 2015: contributor
    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: https://github.com/bitcoin/bitcoin/commit/0cac6d221ffd18d2a6af984a78f038023e9a8433 (also declaring Consensus::CheckTxInputs in consensus.h and adding some documentation to the function).
  11. jtimon force-pushed on Sep 12, 2015
  12. jtimon commented at 3:00 pm on September 12, 2015: contributor
    Updated with the squashed version.
  13. jtimon commented at 8:11 pm on September 14, 2015: contributor
    Closing in favor of #6672.
  14. jtimon closed this on Sep 14, 2015
  15. jtimon reopened this on Oct 20, 2015
  16. jtimon force-pushed on Oct 20, 2015
  17. jtimon commented at 4:01 pm on October 20, 2015: contributor
    Rebased and reopened.
  18. dcousens commented at 1:45 am on October 21, 2015: contributor
    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.
  19. in src/txmempool.cpp: in dc23326bd1 outdated 
    485@@ -484,7 +486,7 @@ void CTxMemPool::removeCoinbaseSpends(const CCoinsViewCache *pcoins, unsigned in
486                 continue;
487             const CCoins *coins = pcoins->AccessCoins(txin.prevout.hash);
488             if (fSanityCheck) assert(coins);
    MarcoFalke commented at 12:20 pm on November 3, 2015:
    @jtimon Needs trivial rebase due to e06c14fb59ee493da5283819420d949a14304ca7.
  20. jtimon force-pushed on Nov 3, 2015
  21. jtimon commented at 1:52 pm on November 3, 2015: contributor
    @MarcoFalke Thanks, rebased.
  22. jtimon commented at 2:49 pm on November 3, 2015: contributor
    New uses of MAX_BLOCK_SIZE/MaxBlockSize() were introduced in #6622.
  23. jtimon force-pushed on Nov 11, 2015
  24. jtimon commented at 2:45 pm on November 11, 2015: contributor
    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.
  25. dcousens commented at 9:41 pm on November 11, 2015: contributor
    re-ACK, only did a once over though
  26. jtimon force-pushed on Nov 17, 2015
  27. jtimon commented at 3:29 pm on November 17, 2015: contributor

    Rebased and squashed. @theuni @laanwj @sipa @gmaxwell @jgarzik @gavinandresen Does this have any chance to get into 0.12 as it is? I think it is disruptive enough that if we decide to do so, it would be good to make it happen asap. Otherwise I would like to close it as an independent PR. It is included in both #6382 and #5970 anyway so it won’t be lost, but having it synchronized in both branches plus an independent one is more annoying than it needs to be. BIP102, for example, can be maintained on top of #6382 instead.

    But at the same time, if it is unlikely that it gets merged for 0.12 as it is, It would be very nice to at least create another one with the 2 or 3 commits that are suitable for 0.12 and keep maintaining the rest just as part of #6382 and #5970, at least until 0.12 is forked.

  28. sipa commented at 3:52 pm on November 17, 2015: member

    utACK as-is for 0.12.

    This doesn’t move much code and is unlikely to conflict with other PRs apart from a few simple variable renamings. I do hope that we’ll soon move the functions now declared inside consensus/.h to their respective consensus/.cpp, though.

  29. jtimon commented at 3:57 pm on November 17, 2015: contributor
    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.
  30. btcdrak commented at 3:59 pm on November 17, 2015: contributor
    I agree with @sipa, it doesn’t look too disruptive as-is.
  31. jtimon force-pushed on Nov 27, 2015
  32. jtimon commented at 1:28 pm on November 27, 2015: contributor
    Needed trivial rebase.
  33. MarcoFalke commented at 10:15 pm on November 28, 2015: member
    utACK 82b2443
  34. btcdrak commented at 10:42 pm on November 28, 2015: contributor
    utACK
  35. jtimon commented at 9:45 pm on December 1, 2015: contributor
    Rebased(6)
  36. jtimon force-pushed on Dec 1, 2015
  37. jtimon force-pushed on Dec 1, 2015
  38. dcousens commented at 2:04 am on December 2, 2015: contributor
    Re-ACK, @laanwj anything holding this back?
  39. jtimon force-pushed on Dec 3, 2015
  40. jtimon commented at 11:57 am on December 3, 2015: contributor
    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 https://github.com/bitcoin/bitcoin/compare/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
  41. consensus: don't define MAX_STANDARD_TX_SIGOPS in terms of block size ccae215206
  42. 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.
    b9eac760f0
  43. consensus: teach CheckTransaction to check for an arbitrary max tx size 
    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.
    654c7c6a3b
  44. consensus: Move consensus constants into Consensus::Params and consensus.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
    6111e5bd4c
  45. jtimon force-pushed on Dec 3, 2015
  46. jtimon commented at 10:43 pm on December 8, 2015: contributor

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

  47. dcousens commented at 0:03 am on December 9, 2015: contributor
    This was ACK’d for 0.12 a while ago, it should still be able to make it in right?
  48. MarcoFalke commented at 0:12 am on December 9, 2015: member
    Yes, if this get’s merged in master, at least the first three commits should be backported (can be merged as is).
  49. devrandom commented at 7:05 pm on December 10, 2015: none
    ACK
  50. jtimon closed this on Dec 18, 2015
  51. jgarzik commented at 5:25 am on December 18, 2015: contributor
    Structure seems reasonable, with the caveat that MaxBlockSize() and MaxBlockSigOps() will inevitably be modified depending on what state they examine.
  52. jtimon commented at 11:57 am on December 18, 2015: contributor
    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.
  53. jgarzik commented at 1:31 pm on December 18, 2015: contributor
    ut ACK, for the record
  54. jtimon reopened this on Dec 18, 2015
  55. jtimon commented at 7:06 am on December 21, 2015: contributor
    Replaced by #7238 (a subset of this) with @rustyrussell ’s nit fixed.
  56. jtimon closed this on Dec 21, 2015
  57. MarcoFalke locked this on Sep 8, 2021


jtimon dcousens MarcoFalke sipa btcdrak devrandom jgarzik

Labels
Refactoring

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me