test: Set BIP34Height = 2 for regtest #16333

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:1906-bip34H2 changing 6 files +18 −14
  1. MarcoFalke commented at 1:43 PM on July 3, 2019: member

    BIP34 is active on the current tip of mainnet, so all miners must obey it. It would be nice if it also was active in fresh regtest instances from the earliest time possible.

    I changed the BIP34 height to 2, so that the block at height=1 may be used to mine a duplicate coinbase. (Needed to test mainnet behaviour)

    This pull is done in two commits:

    • test: Create all blocks with version 4 or higher: Now that BIP34 is activated earlier, we need to create blocks with a higher version number. Just bump it to 4 instead of 2 to avoid having to bump it again later.

    • test: Set BIP34Height = 2 for regtest: This fixes the BIP34 implementation in the tests (to match the one of the Core codebase) and updates the tests where needed

  2. fanquake added the label Tests on Jul 3, 2019
  3. in test/functional/test_framework/blocktools.py:99 in faa2e0d668 outdated 
     108 | -    return r
 109 | +
 110 | +def script_BIP34_coinbase_height(height):
 111 | +    if height <= 16:
 112 | +        res = CScriptOp.encode_op_n(height)
 113 | +        # Append dummy to increase scriptSig size above 2
    instagibbs commented at 2:06 PM on July 3, 2019:

    please document why

    MarcoFalke commented at 2:27 PM on July 3, 2019:

    I don't know the reason for this. The rule was added by satoshi in the first commit https://github.com/bitcoin/bitcoin/blame/5b721607b1057df4dfe97f80d235ed372312f398/main.h#L495

    instagibbs commented at 2:53 PM on July 3, 2019:

    Ok then just document it's a check in CheckTransaction, I just couldn't recall this rule.

    MarcoFalke commented at 2:58 PM on July 3, 2019:

    Done

  4. in src/test/miner_tests.cpp:56 in faa2e0d668 outdated 
      79 | -    {2, 0x03e8779a}, {1, 0x98f34d8f}, {1, 0xc07b2b07}, {1, 0xdfe29668},
  80 | -    {1, 0x3141c7c1}, {1, 0xb3b595f4}, {1, 0x735abf08}, {5, 0x623bfbce},
  81 | -    {2, 0xd351e722}, {1, 0xf4ca48c9}, {1, 0x5b19c670}, {1, 0xa164bf0e},
  82 | -    {2, 0xbbbeb305}, {2, 0xfe1c810a},
  83 | +} BLOCKINFO[] = {
  84 | +    {8, 582909131}, {0, 971462344}, {2, 1169481553}, {6, 66147495}, {7, 427785981}, {8, 80538907}, {8, 207348013}, {2,
    laanwj commented at 2:30 PM on July 3, 2019:

    this doesn't look like an improvement in readability, might want to surround this table with // clang-format off and // clang-format on

    MarcoFalke commented at 2:34 PM on July 3, 2019:

    clang format will put them in separate lines, which is wasteful. So I ran the "paragraph formatter" in vim for extra compaction. I don't think this needs to be human-readable, so should be fine as is.

    MarcoFalke commented at 12:36 PM on August 5, 2019:

    Done. Added // clang-format {off,on}

  5. MarcoFalke force-pushed on Jul 3, 2019
  6. MarcoFalke closed this on Jul 3, 2019
  7. MarcoFalke reopened this on Jul 3, 2019
  8. DrahtBot commented at 5:57 PM on July 3, 2019: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  9. promag commented at 3:38 PM on July 4, 2019: member

    Fails with Assertion failed: lock cs_main not held in ./validation.h:411

  10. fanquake added the label Waiting for author on Jul 5, 2019
  11. ajtowns commented at 1:51 AM on July 5, 2019: contributor

    I changed the BIP34 height to 2, so that the block at height=1 may be used to mine a duplicate coinbase. (Needed to test mainnet behaviour)

    This didn't quite make sense to me, but I think it works. I think the two behaviours that warrant testing are:

    • block 1 coinbase txid = X, block 2 spends X, block 3 coinbase txid = X, block 4 spends X again; all okay
    • block 1 coinbase txid = X, block 2 at most partially spends X, block 3 coinbase txid = X, should be prevented by BIP30 enforcement

    I think this adds a test for the second case (with X completely unspent), but not the first? (And duplicates are at block 120 not block 3)

  12. in src/test/miner_tests.cpp:212 in fa1a949bf3 outdated 
     207 | @@ -221,21 +208,19 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
 208 |      BOOST_CHECK(pblocktemplate = AssemblerForTest(chainparams).CreateNewBlock(scriptPubKey));
 209 |  
 210 |      // We can't make transactions until we have inputs
 211 | -    // Therefore, load 100 blocks :)
 212 | +    // Therefore, load 110 blocks :)
 213 | +    static_assert(110 == sizeof(BLOCKINFO) / sizeof(*BLOCKINFO));
    fqlx commented at 1:44 AM on July 6, 2019:

    I'd prefer not to use Yoda conditions

  13. MarcoFalke force-pushed on Jul 8, 2019
  14. MarcoFalke force-pushed on Jul 8, 2019
  15. MarcoFalke force-pushed on Jul 8, 2019
  16. MarcoFalke cross-referenced this on Jul 9, 2019 from issue test: Add test for BIP30 duplicate tx by MarcoFalke
  17. MarcoFalke force-pushed on Jul 9, 2019
  18. MarcoFalke force-pushed on Jul 9, 2019
  19. ajtowns commented at 6:25 AM on July 16, 2019: contributor

    Based on the comments in #14633 our implementation doesn't match BIP34 for blocks at heights lower than 17 (since we do OP_1..OP_16 instead of non-minimal 0x01 [0x01...0x10] as per BIP34); maybe it might be better to only test BIP34 from block 17 rather than block 2? FWIW, tests look like they work unchanged with BIP34Height = 17 instead of 2.

    If so, could be worth adding assert(globalChainParams->GetConsensus().BIP34Height > 16); to SelectParams() along with a comment about divergence from BIP34 if that assertion were violated.

    Alternatively, seems like BIP34 probably should be updated to at least note the divergence to the original specification for bitcoind's regtest (and potentially signet or "tapnet" or similar, which might actually require interoperability with other clients).

    (Edited to add: ACK once the above is dealt with one way or another, fwiw)

  20. MarcoFalke referenced this in commit 62117f9f36 on Aug 5, 2019
  21. MarcoFalke removed the label Waiting for author on Aug 5, 2019
  22. MarcoFalke commented at 12:27 PM on August 5, 2019: member

    maybe it might be better to only test BIP34 from block 17 rather than block 2 If so, could be worth adding assert(globalChainParams->GetConsensus().BIP34Height > 16);

    Adding this assert would break existing test networks such as

    So I will keep this pull request as-is for now.

  23. MarcoFalke force-pushed on Aug 5, 2019
  24. MarcoFalke force-pushed on Aug 5, 2019
  25. sidhujag referenced this in commit f44d5064d6 on Aug 10, 2019
  26. MarcoFalke force-pushed on Aug 15, 2019
  27. DrahtBot added the label Needs rebase on Sep 17, 2019
  28. MarcoFalke force-pushed on Sep 17, 2019
  29. DrahtBot removed the label Needs rebase on Sep 17, 2019
  30. jtimon commented at 10:12 PM on October 10, 2019: contributor

    Concept ACK Just a question, the second commit won't pass the tests until the next commit is applied too, will it? If that's the case, perhaps we should consider a squash for "git bisect" purposes. If all commits pass all tests independently, I said nothing.

  31. MarcoFalke commented at 6:37 PM on October 11, 2019: member

    Huh, the second commit changes nothing other than that the tests mine version 4 blocks instead of version 1 blocks. If version 1 blocks were accepted, then version 4 blocks are accepted as well under our current consensus rules. So the second commit should pass. And I am pretty sure I did check that it passed.

  32. DrahtBot added the label Needs rebase on Oct 30, 2019
  33. MarcoFalke force-pushed on Oct 30, 2019
  34. DrahtBot removed the label Needs rebase on Oct 30, 2019
  35. MarcoFalke force-pushed on Jan 2, 2020
  36. DrahtBot cross-referenced this on Feb 11, 2020 from issue Tests: Chainparams: Make regtest almost fully customizable by jtimon
  37. DrahtBot cross-referenced this on Feb 26, 2020 from issue Multiprocess bitcoin by ryanofsky
  38. DrahtBot cross-referenced this on Mar 2, 2020 from issue test: Bump timeouts to accomodate really slow disks by MarcoFalke
  39. DrahtBot added the label Needs rebase on Mar 5, 2020
  40. MarcoFalke force-pushed on Apr 30, 2020
  41. DrahtBot removed the label Needs rebase on Apr 30, 2020
  42. DrahtBot cross-referenced this on May 1, 2020 from issue Make g_chainman internal to validation by MarcoFalke
  43. DrahtBot added the label Needs rebase on May 23, 2020
  44. MarcoFalke force-pushed on May 23, 2020
  45. DrahtBot removed the label Needs rebase on May 23, 2020
  46. DrahtBot cross-referenced this on Jun 15, 2020 from issue util: Add Assert identity function by MarcoFalke
  47. DrahtBot cross-referenced this on Jun 28, 2020 from issue QA: Use GBT to get block versions correct by luke-jr
  48. DrahtBot added the label Needs rebase on Jul 4, 2020
  49. MarcoFalke force-pushed on Jul 4, 2020
  50. DrahtBot removed the label Needs rebase on Jul 4, 2020
  51. DrahtBot cross-referenced this on Jul 7, 2020 from issue multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky
  52. DrahtBot cross-referenced this on Jul 7, 2020 from issue multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky
  53. DrahtBot cross-referenced this on Jul 24, 2020 from issue Replace unused BIP 9 logic with draft BIP 8 by luke-jr
  54. DrahtBot cross-referenced this on Jul 30, 2020 from issue refactor: replace sizeof(a)/sizeof(a[0]) by ARRAYLEN(a) by theStack
  55. DrahtBot cross-referenced this on Aug 7, 2020 from issue Run clang-tidy -*,performance-* by Warchant
  56. ShengguangXiao referenced this in commit efa1e6e6cc on Aug 28, 2020
  57. DrahtBot cross-referenced this on Oct 15, 2020 from issue tree-wide: De-globalize ChainstateManager by dongcarl
  58. DrahtBot added the label Needs rebase on Oct 16, 2020
  59. MarcoFalke force-pushed on Oct 27, 2020
  60. DrahtBot removed the label Needs rebase on Oct 27, 2020
  61. DrahtBot cross-referenced this on Nov 20, 2020 from issue refactor: replace (sizeof(a)/sizeof(a[0])) with C++17 std::size by theStack
  62. DrahtBot cross-referenced this on Dec 22, 2020 from issue [Bundle 2/n] Prune g_chainman usage in mempool-related validation functions by dongcarl
  63. DrahtBot cross-referenced this on Dec 22, 2020 from issue [Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex by dongcarl
  64. DrahtBot added the label Needs rebase on Feb 1, 2021
  65. MarcoFalke force-pushed on Jun 18, 2021
  66. DrahtBot removed the label Needs rebase on Jun 18, 2021
  67. DrahtBot cross-referenced this on Jun 18, 2021 from issue test: Properly set BIP34 height in CreateNewBlock_validity unit test by MarcoFalke
  68. MarcoFalke force-pushed on Jun 18, 2021
  69. test: Create all blocks with version 4 or higher fac90c55be
  70. MarcoFalke force-pushed on Jun 18, 2021
  71. MarcoFalke force-pushed on Jun 18, 2021
  72. test: Set BIP34Height = 2 for regtest 222290f543
  73. MarcoFalke force-pushed on Jun 18, 2021
  74. MarcoFalke cross-referenced this on Jul 25, 2021 from issue test: Reduce number of blocks generated in rpc_signrawtransaction by laanwj
  75. theStack commented at 11:35 PM on July 25, 2021: contributor

    Concept ACK

  76. in test/functional/test_framework/messages.py:618 in 222290f543 
     614 | @@ -615,7 +615,7 @@ def __init__(self, header=None):
 615 |              self.calc_sha256()
 616 |  
 617 |      def set_null(self):
 618 | -        self.nVersion = 1
 619 | +        self.nVersion = 4
    jonatack commented at 1:20 PM on July 28, 2021:

    fac90c55be478f0323eafa1d560e it seems that using from test_framework.blocktools import VERSIONBITS_LAST_OLD_BLOCK_VERSION in messages.py would be a circular import, but perhaps

            self.nVersion = 4  # VERSIONBITS_LAST_OLD_BLOCK_VERSION in test_framework.blocktools
    MarcoFalke commented at 2:15 PM on July 28, 2021:

    Might do if I have to re-touch

  77. in src/test/validation_block_tests.cpp:101 in 222290f543 
      98 |      }
  99 |  
 100 | +    // submit block header, so that miner can get the block height from the
 101 | +    // global state and the node has the topology of the chain
 102 | +    BlockValidationState ignored;
 103 | +    BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlockHeaders({pblock->GetBlockHeader()}, ignored, Params()));
    jonatack commented at 1:52 PM on July 28, 2021:

    222290f5438827093 clever simplification. nit, feel free to ignore

        BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlockHeaders({pblock->GetBlockHeader()}, /* state */ ignored, Params()));
    MarcoFalke commented at 2:12 PM on July 28, 2021:

    I don't think named args are useful for args whose meaning is already clear. Otherwise we could add the comment to every arg. So leaving as-is.

  78. jonatack commented at 1:56 PM on July 28, 2021: contributor

    ACK 222290f54388270937cb6c174195717e2214ec0d tested and reviewed rebased to current master 5e213822f86d

    Feel free to ignore the comments.

  79. theStack commented at 1:16 AM on August 1, 2021: contributor
    • test: Create all blocks with version 4 or higher: Now that BIP34 is activated earlier, we need to create blocks with a higher version number from where it activated. Just bump it up to at least 4.

    This confused me a little. The block version required after BIP34 activation is fixed and hard-coded to >= 2, I don't think it depends on anything related to what version was used prior the activation (maybe I misunderstood your explanation though?):

    https://github.com/bitcoin/bitcoin/blob/6499928bfb19674f98724b4aa5238d874e6738e4/src/validation.cpp#L3159

    All tests still pass with nVersion=2 instead of 4 in the first commit. That said, bumping already to 4 for future similar PRs to activate soft-forks in regtest earlier can't hurt. Will code-review tomorrow, just wanted to get sure that I understand the motivation for choosing 4 in the first commit.

  80. MarcoFalke commented at 3:05 PM on August 1, 2021: member

    This confused me a little.

    Edited OP comment

  81. theStack approved
  82. theStack commented at 5:21 PM on August 1, 2021: contributor

    This confused me a little.

    Edited OP comment

    Thanks for clarifying!

    Tested ACK 222290f54388270937cb6c174195717e2214ec0d

  83. MarcoFalke commented at 10:40 AM on August 2, 2021: member

    @ajtowns Mind to re-ACK/NACK based on your

    (Edited to add: ACK once the above is dealt with one way or another, fwiw) (https://github.com/bitcoin/bitcoin/pull/16333#issuecomment-511683294)

    ?

  84. ajtowns commented at 2:13 PM on August 2, 2021: contributor

    ACK 222290f54388270937cb6c174195717e2214ec0d

    Still seems sad this isn't documented accurately outside the source, but bip34's final and a new bip just for the first ~16 blocks on signet or regtest seems weak too.

  85. MarcoFalke merged this on Aug 3, 2021
  86. MarcoFalke closed this on Aug 3, 2021
  87. MarcoFalke deleted the branch on Aug 3, 2021
  88. sidhujag referenced this in commit c0cf3a4c0b on Aug 4, 2021
  89. theStack cross-referenced this on Nov 17, 2021 from issue test: refactor: dedup code by taking use of `create_block` parameters by theStack
  90. MarcoFalke referenced this in commit 3a36ec83d0 on Nov 22, 2021
  91. Munkybooty referenced this in commit 2c0929fa2f on Nov 25, 2021
  92. Munkybooty referenced this in commit 0b1215a5fc on Nov 25, 2021
  93. Munkybooty referenced this in commit d0f1663305 on Nov 30, 2021
  94. bitcoin locked this on Aug 18, 2022
Contributors
MarcoFalkeinstagibbslaanwjDrahtBotpromagajtownsfqlxjtimontheStackjonatack
Labels
Tests
Linked (view graph)
#14633 Fix height serialization inside script of coinbase input#16060 Bury bip9 deployments#16363 test: Add test for BIP30 duplicate tx#22542 test: Reduce number of blocks generated in rpc_signrawtransaction#23521 test: refactor: dedup code by taking use of `create_block` parameters
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:54 UTC