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: member

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

    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: member

    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 force-pushed on Jul 9, 2019
  17. MarcoFalke force-pushed on Jul 9, 2019
  18. ajtowns commented at 6:25 am on July 16, 2019: member

    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)

  19. MarcoFalke referenced this in commit 62117f9f36 on Aug 5, 2019
  20. MarcoFalke removed the label Waiting for author on Aug 5, 2019
  21. 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.

  22. MarcoFalke force-pushed on Aug 5, 2019
  23. MarcoFalke force-pushed on Aug 5, 2019
  24. sidhujag referenced this in commit f44d5064d6 on Aug 10, 2019
  25. MarcoFalke force-pushed on Aug 15, 2019
  26. DrahtBot added the label Needs rebase on Sep 17, 2019
  27. MarcoFalke force-pushed on Sep 17, 2019
  28. DrahtBot removed the label Needs rebase on Sep 17, 2019
  29. 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.
  30. 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.
  31. DrahtBot added the label Needs rebase on Oct 30, 2019
  32. MarcoFalke force-pushed on Oct 30, 2019
  33. DrahtBot removed the label Needs rebase on Oct 30, 2019
  34. MarcoFalke force-pushed on Jan 2, 2020
  35. DrahtBot added the label Needs rebase on Mar 5, 2020
  36. MarcoFalke force-pushed on Apr 30, 2020
  37. DrahtBot removed the label Needs rebase on Apr 30, 2020
  38. DrahtBot added the label Needs rebase on May 23, 2020
  39. MarcoFalke force-pushed on May 23, 2020
  40. DrahtBot removed the label Needs rebase on May 23, 2020
  41. DrahtBot added the label Needs rebase on Jul 4, 2020
  42. MarcoFalke force-pushed on Jul 4, 2020
  43. DrahtBot removed the label Needs rebase on Jul 4, 2020
  44. ShengguangXiao referenced this in commit efa1e6e6cc on Aug 28, 2020
  45. DrahtBot added the label Needs rebase on Oct 16, 2020
  46. MarcoFalke force-pushed on Oct 27, 2020
  47. DrahtBot removed the label Needs rebase on Oct 27, 2020
  48. DrahtBot added the label Needs rebase on Feb 1, 2021
  49. MarcoFalke force-pushed on Jun 18, 2021
  50. DrahtBot removed the label Needs rebase on Jun 18, 2021
  51. MarcoFalke force-pushed on Jun 18, 2021
  52. test: Create all blocks with version 4 or higher fac90c55be
  53. MarcoFalke force-pushed on Jun 18, 2021
  54. MarcoFalke force-pushed on Jun 18, 2021
  55. test: Set BIP34Height = 2 for regtest 222290f543
  56. MarcoFalke force-pushed on Jun 18, 2021
  57. theStack commented at 11:35 pm on July 25, 2021: member
    Concept ACK
  58. 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

    0        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
  59. 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

    0    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.
  60. jonatack commented at 1:56 pm on July 28, 2021: member

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

    Feel free to ignore the comments.

  61. theStack commented at 1:16 am on August 1, 2021: member
    • 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.

  62. MarcoFalke commented at 3:05 pm on August 1, 2021: member

    This confused me a little.

    Edited OP comment

  63. theStack approved
  64. theStack commented at 5:21 pm on August 1, 2021: member

    This confused me a little.

    Edited OP comment

    Thanks for clarifying!

    Tested ACK 222290f54388270937cb6c174195717e2214ec0d

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

    ?

  66. ajtowns commented at 2:13 pm on August 2, 2021: member

    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.

  67. MarcoFalke merged this on Aug 3, 2021
  68. MarcoFalke closed this on Aug 3, 2021

  69. MarcoFalke deleted the branch on Aug 3, 2021
  70. sidhujag referenced this in commit c0cf3a4c0b on Aug 4, 2021
  71. MarcoFalke referenced this in commit 3a36ec83d0 on Nov 22, 2021
  72. Munkybooty referenced this in commit 2c0929fa2f on Nov 25, 2021
  73. Munkybooty referenced this in commit 0b1215a5fc on Nov 25, 2021
  74. Munkybooty referenced this in commit d0f1663305 on Nov 30, 2021
  75. DrahtBot locked this on Aug 18, 2022

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-10-04 13:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me