Rewrite CreateNewBlock #6898

pull morcos wants to merge 6 commits into bitcoin:master from morcos:fasterCNB changing 10 files +293 −228
  1. morcos commented at 8:58 PM on October 28, 2015: member

    WARNING: This hasn't been used for mainnet mining yet as far as I know.

    NOTE: This includes a commit which changes the default block priority size to 0. This code has been optimized for not including transactions in a priority space. If block priority space > 0 then the code does not offer too much of a performance improvement over the existing code. (EDIT: now its fast regardless)

    The mempool is explicitly assumed to be responsible for maintaining consistency of transactions with respect to not spending non-existent outputs, not double spending, script validity and coinbase maturity. Only finality of transactions is checked before assembling a block.

    A final call to TestBlockValidity is still performed. If an invalid block has been constructed, then an error is thrown logged and a NULL pointer is returned. (EDIT: reverted to original behavior with more informative error)

    This code produces the same blocks as the code it replaces with the following exceptions:

    • The fee rate sort tie breaker is hash instead of priority
    • The priority sort has a secondary tie breaker of hash
    • The priority block is not limited to transactons above AllowFree() (EDIT: fixed)
    • The fee rate sorted part of the block is not limited to transactions above minRelayTxFee (EDIT: fixed)
    • (EDIT: The blocks are the same if you keep scanning to the end to try to fill up the last remaining space, but this code stops after trying 50 times to fill the last 1000 bytes)

    Comparing this to the original code over 2000 calls to CreateNewBlock over the last 2 days. (blockprioritysize=0, maxmempool=300, dbcache=1000, maxsigcachesize=1000000) Time in ms

    Time to assemble block Additional time to perform final TestBlockValidity
    master 2602 240
    this pull 14 438

    The extra slowness in TestBlockValidity is because the cache used to be warmed up in the assembly code.

  2. laanwj added the label Mining on Oct 31, 2015
  3. in src/txmempool.h:None in 8f84ec4230 outdated 
      79 | @@ -78,7 +80,8 @@ class CTxMemPoolEntry
  80 |  
  81 |  public:
  82 |      CTxMemPoolEntry(const CTransaction& _tx, const CAmount& _nFee,
  83 | -                    int64_t _nTime, double _dPriority, unsigned int _nHeight, bool poolHasNoInputsOf = false);
  84 | +                    int64_t _nTime, double _dPriority, unsigned int _nHeight, bool poolHasNoInputsOf = false,
  85 | +                    unsigned int nSigOps = 0);
    luke-jr commented at 6:30 PM on October 31, 2015:

    NACK defaulting nSigOps to 0 when we depend on it being accurate.

    morcos commented at 1:22 AM on November 1, 2015:

    ok i agree, it makes more sense to default it high.

    luke-jr commented at 1:38 AM on November 1, 2015:

    It does not make sense to default it period. Note the exact sigop count is part of the GBT response.

    morcos commented at 4:16 PM on November 3, 2015:

    OK agreed. The default is just for the tests, I think for now it makes sense to remove all defaults from the constructor and just make the tests fill them in. Unfortunately there are 2 other pulls open which also add arguments to the constructor and require changing the tests. I'm going to hold off updating the tests in this pull until we're a bit closer assuming those might be merged first.

  4. in src/miner.cpp:None in 8f84ec4230 outdated 
     295 | @@ -351,8 +296,10 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)
 296 |          pblocktemplate->vTxSigOps[0] = GetLegacySigOpCount(pblock->vtx[0]);
 297 |  
 298 |          CValidationState state;
 299 | -        if (!TestBlockValidity(state, *pblock, pindexPrev, false, false))
 300 | -            throw std::runtime_error("CreateNewBlock(): TestBlockValidity failed");
 301 | +        if (!TestBlockValidity(state, *pblock, pindexPrev, false, false)) {
 302 | +            LogPrintf("ERROR: CNB TestBlockValidity failed: %s\n",FormatStateMessage(state));
 303 | +            return NULL;
    luke-jr commented at 6:30 PM on October 31, 2015:

    Why change exception to returning NULL here? IMO this should be an independent PR.

    morcos commented at 4:17 PM on November 3, 2015:

    The idea was this is now more likely to happen, and it seemed to meet that the JSON error from GetBlockTemplate would still be sufficient notification, and it would be better to not bring the node down. However, I'm happy to leave the existing behavior if people think crashing the node is the right outcome.

    luke-jr commented at 5:33 PM on November 3, 2015:

    Exceptions here should not crash the node... it just gets returned as a nicer JSON-RPC error.

  5. morcos force-pushed on Nov 3, 2015
  6. morcos commented at 4:27 PM on November 3, 2015: member

    OK, this has been rewritten to use CTxMemPoolEntry::GetPriority for the priority sort. This will make it very fast even when maintaining a priority space in the block. So the commit changing the default blockprioritysize has been removed.

    Of course until #6357 is merged or some other decision is taken regarding priority, it will not sort by an accurate measure of priority.

  7. morcos force-pushed on Nov 3, 2015
  8. morcos cross-referenced this on Nov 3, 2015 from issue [WIP] Keep pcoinsTip cache warm by morcos
  9. gmaxwell added this to the milestone 0.12.0 on Nov 5, 2015
  10. morcos cross-referenced this on Nov 9, 2015 from issue Summary of potential performance improvements by morcos
  11. morcos commented at 2:02 PM on November 12, 2015: member

    I addressed @luke-jr 's comment about changing the method of failure return. I still modified it to give some extra state info. That commit and other fixes will get squashed.

    Among the remaining outstanding items is to fix the constructor for CTxMemPoolEntry to not have any default arguments. It would be nice to know a merge order since this will conflict with #6357 and #6915 and involves tedious updating of a bunch of tests.

  12. sipa cross-referenced this on Nov 12, 2015 from issue the RPC should have commands to specify the side of a fork considered 'current' by kanoi
  13. morcos force-pushed on Nov 12, 2015
  14. morcos cross-referenced this on Nov 13, 2015 from issue Lower bound priority by morcos
  15. morcos force-pushed on Nov 15, 2015
  16. morcos cross-referenced this on Nov 17, 2015 from issue Implement helper class for CTxMemPoolEntry constructor by morcos
  17. morcos force-pushed on Nov 18, 2015
  18. morcos commented at 4:53 AM on November 18, 2015: member

    I rebased this and squashed fixes for comments so far.

    I rebased it on top of #7008, because I needed to pick some path forward on how priority would be addressed in this PR. The first 3 commits are therefore part of that pull and not this one. If we decide to merge #6357 as well, then it should be an easy change.

    I will continue testing, but I don't have any other changes planned right now.

  19. morcos renamed this:
    [WIP] Rewrite CreateNewBlock
    Rewrite CreateNewBlock
    on Nov 18, 2015
  20. morcos force-pushed on Nov 18, 2015
  21. morcos commented at 8:10 PM on November 18, 2015: member

    Thanks @sdaftuar for finding a bug in the way I was doing PrioritiseTransaction. There should probably be RPC tests for those functions.

  22. morcos force-pushed on Nov 18, 2015
  23. sdaftuar cross-referenced this on Nov 19, 2015 from issue [Mempool] Fix mempool limiting and replace-by-fee for PrioritiseTransaction by sdaftuar
  24. morcos force-pushed on Nov 19, 2015
  25. morcos commented at 6:33 PM on November 19, 2015: member

    Apologies for all the churn. Squashed again and rebased off a slightly modified index @sdaftuar created for #7062. This introduces one more minor difference from the original code in that fee rates are compared as doubles now.

    If I correct for these differences:

    • using a score index which compares fee rates not doubles
    • make the original code use the hash tie breakers
    • search up 5000 txs to fill the final 1000 bytes
    • implement #6357 to correctly compute priority

    Then I generate the exact same blocks as the old code modulo a few rare cases where double imprecision in the different priority calculation causes tx reordering in the priority space.

  26. sdaftuar cross-referenced this on Nov 19, 2015 from issue [Tests] Add prioritisetransaction RPC test by sdaftuar
  27. morcos force-pushed on Nov 19, 2015
  28. in src/miner.cpp:None in b0fb264e84 outdated 
     286 | @@ -350,8 +287,9 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const CScript& s
 287 |          pblocktemplate->vTxSigOps[0] = GetLegacySigOpCount(pblock->vtx[0]);
 288 |  
 289 |          CValidationState state;
 290 | -        if (!TestBlockValidity(state, chainparams, *pblock, pindexPrev, false, false))
 291 | -            throw std::runtime_error("CreateNewBlock(): TestBlockValidity failed");
 292 | +        if (!TestBlockValidity(state, chainparams, *pblock, pindexPrev, false, false)) {
 293 | +            throw std::runtime_error("CreateNewBlock(): TestBlockValidity failed: " + FormatStateMessage(state));
    jtimon commented at 1:06 PM on November 23, 2015:

    can you strprintf("%: TestBlockValidity failed: %", __func__, ... instead?

  29. sdaftuar commented at 1:43 PM on November 24, 2015: member

    Addressed @jtimon's nit.

  30. btcdrak commented at 8:20 AM on November 29, 2015: contributor

    Needs rebase.

  31. jameshilliard commented at 10:00 AM on November 29, 2015: contributor

    TestBlockValidity should probably be done in a background thread so that it does not delay GBT since TestBlockValidity is just a sanity check.

  32. morcos force-pushed on Nov 30, 2015
  33. morcos commented at 1:27 PM on November 30, 2015: member

    rebased

  34. gmaxwell commented at 10:09 AM on December 1, 2015: contributor

    Concept ACK. Some tested-ack. Will test more now.

    Edit: Hm. needs some not totally trivial rebasing.

  35. jgarzik commented at 12:06 PM on December 1, 2015: contributor

    ut ACK

  36. laanwj commented at 12:34 PM on December 1, 2015: member

    Needs rebase again...

  37. morcos commented at 3:00 PM on December 1, 2015: member

    that was an expected merge conflict, will rebase shortly @laanwj my warning about not having mined, was just a literal warning because i don't have sufficient hashpower to mine on mainnet. I have created 1000s upon 1000s of block templates that all pass TestBlockValidity both in code running this pull live for the past 2 weeks (and prior iterations for weeks before that) and in historical simulation over 6 weeks of data.

  38. Store the total sig op count of a tx.
    Store sum of legacy and P2SH sig op counts.  This is calculated in AcceptToMemory pool and storing it saves redoing the expensive calculation in block template creation.
    c49d5bc9e6
  39. Add a score index to the mempool.
    The score index is meant to represent the order of priority for being included in a block for miners.  Initially this is set to the transactions modified (by any feeDelta) fee rate.  Index improvements and unit tests by sdaftuar.
    f3fe83673e
  40. Add TxPriority class and comparator 7230187b1d
  41. Make accessing mempool parents and children public 1f09287c66
  42. Expose FormatStateMessage 5f12263302
  43. sipa commented at 4:26 PM on December 1, 2015: member

    ACK

  44. Rewrite CreateNewBlock
    Use the score index on the mempool to only add sorted txs in order.  Remove much of the validation while building the block, relying on mempool to be consistent and only contain txs that can be mined.
The mempool is assumed to be consistent as far as not containing txs which spend non-existent outputs or double spends, and scripts are valid.  Finality of txs is still checked (except not coinbase maturity, assumed in mempool).
Still TestBlockValidity in case mempool consistency breaks and return error state if an invalid block was created.
Unit tests are modified to realize that invalid blocks can now be constructed if the mempool breaks its consistency assumptions and also updated to have the right fees, since the cached value is now used for block construction.

Conflicts:
	src/miner.cpp
    553cad94e2
  45. morcos force-pushed on Dec 1, 2015
  46. morcos commented at 5:10 PM on December 1, 2015: member

    rebased

  47. sipa merged this on Dec 1, 2015
  48. sipa closed this on Dec 1, 2015
  49. sipa referenced this in commit 4077ad20d0 on Dec 1, 2015
  50. luke-jr commented at 8:18 PM on December 1, 2015: member

    This was not ready for merging. It needs #6357 first.

  51. luke-jr referenced this in commit e4d7271457 on Dec 1, 2015
  52. luke-jr cross-referenced this on Dec 1, 2015 from issue Revert default block priority size to 50k by luke-jr
  53. laanwj commented at 9:26 AM on December 2, 2015: member

    This was not ready for merging. It needs #6357 first.

    #7008, which took the place of #6537 was merged

  54. NicolasDorier cross-referenced this on Dec 2, 2015 from issue BIP-68: Mempool-only sequence number constraint verification by maaku
  55. NicolasDorier cross-referenced this on Dec 6, 2015 from issue Performance fix propositions for CNB in case of #6312 merging by NicolasDorier
  56. dgenr8 cross-referenced this on Dec 29, 2016 from issue Rewrite CreateNewBlock by dgenr8
  57. Sjors cross-referenced this on Sep 3, 2017 from issue Check specific validation error in miner tests by Sjors
  58. MarkLTZ referenced this in commit 7425415619 on Apr 27, 2019
  59. random-zebra cross-referenced this on Jun 18, 2020 from issue [Wallet] Abandon tx in CommitTransaction if ATMP fails by random-zebra
  60. random-zebra referenced this in commit 98cf582557 on Jun 27, 2020
  61. furszy cross-referenced this on Jul 3, 2020 from issue [Backport] mempool score index. by furszy
  62. furszy cross-referenced this on Jul 3, 2020 from issue Up to CTransactionRef point back ports to do list. by furszy
  63. furszy cross-referenced this on Jul 4, 2020 from issue [WIP] Miner code reworked from upstream 6898 by furszy
  64. random-zebra referenced this in commit e59d8e59fa on Aug 18, 2020
  65. furszy cross-referenced this on Jan 25, 2021 from issue [Miner] Rewrite miner code with proper encapsulation + test coverage by furszy
  66. Fuzzbawls referenced this in commit a88b6ea83f on Feb 3, 2021
  67. str4d cross-referenced this on Aug 10, 2021 from issue ZIP 239 preparations 2 by str4d
  68. zkbot referenced this in commit 12af999d42 on Aug 10, 2021
  69. zkbot referenced this in commit 44f7d7e4ea on Aug 11, 2021
  70. zkbot referenced this in commit 7fa7d5700b on Aug 12, 2021
  71. zkbot referenced this in commit fd462fd8c4 on Aug 13, 2021
  72. str4d cross-referenced this on Aug 13, 2021 from issue ZIP 239 preparations 3 by str4d
  73. bitcoin locked this on Sep 8, 2021
Contributors
morcosluke-jrjtimonsdaftuarbtcdrakjameshilliardgmaxwelljgarziklaanwjsipa
Labels
Mining

Milestone
0.12.0

Linked (view graph)
#6312 BIP-68: Mempool-only sequence number constraint verification#6357 Dynamic priority calculation#6537 [DOC] Document files#6915 [Mempool] Improve removal of invalid transactions after reorgs#6936 [WIP] Keep pcoinsTip cache warm#6976 Summary of potential performance improvements#6995 the RPC should have commands to specify the side of a fork considered 'current'#7008 Lower bound priority#7020 Implement helper class for CTxMemPoolEntry constructor#7062 [Mempool] Fix mempool limiting and replace-by-fee for PrioritiseTransaction#7063 [Tests] Add prioritisetransaction RPC test#7151 Revert default block priority size to 50k#7176 Performance fix propositions for CNB in case of #6312 merging#11220 Check specific validation error in miner tests
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: 2026-05-20 06:55 UTC