test: Set BIP34Height = 2 for regtest

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

fanquake added the label Tests on Jul 3, 2019

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

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}

MarcoFalke force-pushed on Jul 3, 2019

MarcoFalke closed this on Jul 3, 2019

MarcoFalke reopened this on Jul 3, 2019

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.

promag commented at 3:38 pm on July 4, 2019: member

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

fanquake added the label Waiting for author on Jul 5, 2019

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)

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

MarcoFalke force-pushed on Jul 8, 2019

MarcoFalke force-pushed on Jul 9, 2019

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)

MarcoFalke referenced this in commit 62117f9f36 on Aug 5, 2019

MarcoFalke removed the label Waiting for author on Aug 5, 2019

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.

MarcoFalke force-pushed on Aug 5, 2019

sidhujag referenced this in commit f44d5064d6 on Aug 10, 2019

MarcoFalke force-pushed on Aug 15, 2019

DrahtBot added the label Needs rebase on Sep 17, 2019

MarcoFalke force-pushed on Sep 17, 2019

DrahtBot removed the label Needs rebase on Sep 17, 2019

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.

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.

DrahtBot added the label Needs rebase on Oct 30, 2019

MarcoFalke force-pushed on Oct 30, 2019

DrahtBot removed the label Needs rebase on Oct 30, 2019

MarcoFalke force-pushed on Jan 2, 2020

DrahtBot added the label Needs rebase on Mar 5, 2020

MarcoFalke force-pushed on Apr 30, 2020

DrahtBot removed the label Needs rebase on Apr 30, 2020

DrahtBot added the label Needs rebase on May 23, 2020

MarcoFalke force-pushed on May 23, 2020

DrahtBot removed the label Needs rebase on May 23, 2020

DrahtBot added the label Needs rebase on Jul 4, 2020

MarcoFalke force-pushed on Jul 4, 2020

DrahtBot removed the label Needs rebase on Jul 4, 2020

ShengguangXiao referenced this in commit efa1e6e6cc on Aug 28, 2020

DrahtBot added the label Needs rebase on Oct 16, 2020

MarcoFalke force-pushed on Oct 27, 2020

DrahtBot removed the label Needs rebase on Oct 27, 2020

DrahtBot added the label Needs rebase on Feb 1, 2021

MarcoFalke force-pushed on Jun 18, 2021

DrahtBot removed the label Needs rebase on Jun 18, 2021

MarcoFalke force-pushed on Jun 18, 2021

test: Create all blocks with version 4 or higher fac90c55be

MarcoFalke force-pushed on Jun 18, 2021

test: Set BIP34Height = 2 for regtest 222290f543

MarcoFalke force-pushed on Jun 18, 2021

theStack commented at 11:35 pm on July 25, 2021: member

Concept ACK

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

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.

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.

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.

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

This confused me a little.

Edited OP comment

theStack approved

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

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)

?

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.

MarcoFalke merged this on Aug 3, 2021

MarcoFalke closed this on Aug 3, 2021

MarcoFalke deleted the branch on Aug 3, 2021

sidhujag referenced this in commit c0cf3a4c0b on Aug 4, 2021

MarcoFalke referenced this in commit 3a36ec83d0 on Nov 22, 2021

Munkybooty referenced this in commit 2c0929fa2f on Nov 25, 2021

Munkybooty referenced this in commit 0b1215a5fc on Nov 25, 2021

Munkybooty referenced this in commit d0f1663305 on Nov 30, 2021

DrahtBot locked this on Aug 18, 2022

test: Set BIP34Height = 2 for regtest #16333

Conflicts