test: Set BIP34Height = 2 for regtest #16333

maflcko · 2019-07-03T13:43:42Z

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

test/functional/test_framework/blocktools.py

src/test/miner_tests.cpp

DrahtBot · 2019-07-03T17:57:12Z

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

Conflicts

No conflicts as of last run.

promag · 2019-07-04T15:38:46Z

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

ajtowns · 2019-07-05T01:51:52Z

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)

src/test/miner_tests.cpp

ajtowns · 2019-07-16T06:25:30Z

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)

fa8489a test: Add test for BIP30 duplicate tx (MarcoFalke) 77770d9 test: Properly serialize BIP34 coinbase height (MarcoFalke) Pull request description: This adds a test for BIP30 to check that duplicate txs can exist in the blockchain given the first one was completely spent when the second one is added. (Requested by ajtowns in bitcoin#16333 (comment)) We can not add a test that a later duplicate tx overwrites a previous one, because BIP30 is always enforced on regtest. If someone feels strongly about such a test, some Bitcoin Core code would have to be modified, which can be done in a follow up pull request. Also, add a commit to fix the BIP34 test failures reported in bitcoin#14633 (comment) ACKs for top commit: laanwj: Code review ACK fa8489a Tree-SHA512: c707d0bdc93937263876b603425b53322a2a9f9ec3f50716ae2fa9de8ddc644beb22b26c1bfde7f4aab102633e096b354ef380db919176bd2cb44a2828f884aa

maflcko · 2019-08-05T12:27:09Z

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

signet

bitcoin/src/chainparams.cpp

Line 299 in 542b66b

consensus.BIP34Height = 1;
liquid
https://github.com/ElementsProject/elements/blob/c67ef2938c7f59ff263c13efe5f87fd0f835c34c/src/chainparams.cpp#L642

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

fa8489a test: Add test for BIP30 duplicate tx (MarcoFalke) 77770d9 test: Properly serialize BIP34 coinbase height (MarcoFalke) Pull request description: This adds a test for BIP30 to check that duplicate txs can exist in the blockchain given the first one was completely spent when the second one is added. (Requested by ajtowns in bitcoin#16333 (comment)) We can not add a test that a later duplicate tx overwrites a previous one, because BIP30 is always enforced on regtest. If someone feels strongly about such a test, some Bitcoin Core code would have to be modified, which can be done in a follow up pull request. Also, add a commit to fix the BIP34 test failures reported in bitcoin#14633 (comment) ACKs for top commit: laanwj: Code review ACK fa8489a Tree-SHA512: c707d0bdc93937263876b603425b53322a2a9f9ec3f50716ae2fa9de8ddc644beb22b26c1bfde7f4aab102633e096b354ef380db919176bd2cb44a2828f884aa

jtimon · 2019-10-10T22:12:50Z

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.

maflcko · 2019-10-11T18:37:12Z

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.

theStack · 2021-07-25T23:35:27Z

Concept ACK

jonatack

ACK 222290f tested and reviewed rebased to current master 5e21382

Feel free to ignore the comments.

jonatack · 2021-07-28T13:20:03Z

test/functional/test_framework/messages.py

@@ -615,7 +615,7 @@ def __init__(self, header=None):
            self.calc_sha256()

    def set_null(self):
-        self.nVersion = 1
+        self.nVersion = 4


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

Suggested change

self.nVersion = 4

self.nVersion = 4 # VERSIONBITS_LAST_OLD_BLOCK_VERSION in test_framework.blocktools

Might do if I have to re-touch

src/test/validation_block_tests.cpp

theStack

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?):

bitcoin/src/validation.cpp

Line 3159 in 6499928

    
           if ((block.nVersion < 2 && DeploymentActiveAfter(pindexPrev, consensusParams, Consensus::DEPLOYMENT_HEIGHTINCB)) ||

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.

maflcko · 2021-08-01T15:05:50Z

This confused me a little.

Edited OP comment

theStack

This confused me a little.

Edited OP comment

Thanks for clarifying!

Tested ACK 222290f

maflcko · 2021-08-02T10:40:15Z

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

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

?

ajtowns · 2021-08-02T14:13:40Z

ACK 222290f

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.

…ck` parameters e57c0eb test: refactor: replace OP_1/OP_TRUE magic numbers by constants (Sebastian Falbesoner) df5d783 test: refactor: take use of `create_block` txlist parameter (Sebastian Falbesoner) ae9df4e test: refactor: take use of `create_block` version parameter (or use default) (Sebastian Falbesoner) Pull request description: The helper `create_block` offers two parameters `version` and `txlist` which set the `nVersion` field / extend the `vtx` array of the block, respectively. By taking use of those, we can remove a lot of code, including the recalculation of the merkle root. Both passing txs in string and `CTransaction` format is supported, i.e. we also save potential calls to `tx_from_hex`. The PR also contains another commit which replaces magic numbers for OP_TRUE/OP_1 (0x51) with the proper constant from the `script` module. Instances setting the block version of 4 explicitely after calling `create_block` are removed, as this is the default since #16333 got merged (see #23521 (comment)). ACKs for top commit: stratospher: tested ACK e57c0eb. Tree-SHA512: a56965168d36b40b84e7f334b00472b82c31e8482c9e2651c97a791abd7fee3b40ca15e943a7acafa3acf172066fdace38bb13240084b789fd6ff4f6e510e23a

fa8489a test: Add test for BIP30 duplicate tx (MarcoFalke) 77770d9 test: Properly serialize BIP34 coinbase height (MarcoFalke) Pull request description: This adds a test for BIP30 to check that duplicate txs can exist in the blockchain given the first one was completely spent when the second one is added. (Requested by ajtowns in bitcoin#16333 (comment)) We can not add a test that a later duplicate tx overwrites a previous one, because BIP30 is always enforced on regtest. If someone feels strongly about such a test, some Bitcoin Core code would have to be modified, which can be done in a follow up pull request. Also, add a commit to fix the BIP34 test failures reported in bitcoin#14633 (comment) ACKs for top commit: laanwj: Code review ACK fa8489a Tree-SHA512: c707d0bdc93937263876b603425b53322a2a9f9ec3f50716ae2fa9de8ddc644beb22b26c1bfde7f4aab102633e096b354ef380db919176bd2cb44a2828f884aa

fanquake added the Tests label Jul 3, 2019

instagibbs reviewed Jul 3, 2019

View reviewed changes

test/functional/test_framework/blocktools.py Outdated Show resolved Hide resolved

laanwj reviewed Jul 3, 2019

View reviewed changes

src/test/miner_tests.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the 1906-bip34H2 branch from faa2e0d to fa1a949 Compare July 3, 2019 14:58

maflcko closed this Jul 3, 2019

maflcko reopened this Jul 3, 2019

fanquake added the Waiting for author label Jul 5, 2019

fqlx reviewed Jul 6, 2019

View reviewed changes

src/test/miner_tests.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the 1906-bip34H2 branch 3 times, most recently from 31e927a to da2d9b2 Compare July 8, 2019 21:44

maflcko mentioned this pull request Jul 9, 2019

test: Add test for BIP30 duplicate tx #16363

Merged

maflcko force-pushed the 1906-bip34H2 branch 2 times, most recently from c3de476 to 48ed8fd Compare July 9, 2019 18:55

maflcko removed the Waiting for author label Aug 5, 2019

maflcko force-pushed the 1906-bip34H2 branch 2 times, most recently from faf345f to fa573fd Compare August 5, 2019 12:36

maflcko force-pushed the 1906-bip34H2 branch from fa573fd to fa7284c Compare August 15, 2019 20:20

DrahtBot added the Needs rebase label Sep 17, 2019

maflcko force-pushed the 1906-bip34H2 branch from fa7284c to faf0aac Compare September 17, 2019 18:41

DrahtBot removed the Needs rebase label Sep 17, 2019

DrahtBot mentioned this pull request Nov 20, 2020

refactor: replace (sizeof(a)/sizeof(a[0])) with C++17 std::size #20429

Merged

This was referenced Dec 22, 2020

[Bundle 2/n] Prune g_chainman usage in mempool-related validation functions #20750

Merged

[Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex #20749

Merged

DrahtBot added the Needs rebase label Feb 1, 2021

maflcko force-pushed the 1906-bip34H2 branch from faf558e to ce61675 Compare June 18, 2021 08:16

DrahtBot removed the Needs rebase label Jun 18, 2021

DrahtBot mentioned this pull request Jun 18, 2021

test: Properly set BIP34 height in CreateNewBlock_validity unit test #22277

Merged

maflcko force-pushed the 1906-bip34H2 branch from ce61675 to 0b3b867 Compare June 18, 2021 15:44

test: Create all blocks with version 4 or higher

fac90c5

maflcko force-pushed the 1906-bip34H2 branch 2 times, most recently from fa82ab5 to fa1cd7d Compare June 18, 2021 15:59

test: Set BIP34Height = 2 for regtest

222290f

maflcko force-pushed the 1906-bip34H2 branch from fa1cd7d to 222290f Compare June 18, 2021 18:58

maflcko mentioned this pull request Jul 25, 2021

test: Reduce number of blocks generated in rpc_signrawtransaction #22542

Closed

jonatack reviewed Jul 28, 2021

View reviewed changes

theStack reviewed Aug 1, 2021

View reviewed changes

theStack approved these changes Aug 1, 2021

View reviewed changes

maflcko merged commit ad0fc45 into bitcoin:master Aug 3, 2021

maflcko deleted the 1906-bip34H2 branch August 3, 2021 08:22

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 4, 2021

Merge bitcoin#16333: test: Set BIP34Height = 2 for regtest

c0cf3a4

theStack mentioned this pull request Nov 17, 2021

test: refactor: dedup code by taking use of create_block parameters #23521

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

	self.nVersion = 4
	self.nVersion = 4 # VERSIONBITS_LAST_OLD_BLOCK_VERSION in test_framework.blocktools

test: Set BIP34Height = 2 for regtest #16333

test: Set BIP34Height = 2 for regtest #16333

Uh oh!

Conversation

maflcko commented Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag commented Jul 4, 2019

Uh oh!

ajtowns commented Jul 5, 2019

Uh oh!

Uh oh!

ajtowns commented Jul 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Aug 5, 2019

Uh oh!

jtimon commented Oct 10, 2019

Uh oh!

maflcko commented Oct 11, 2019

Uh oh!

theStack commented Jul 25, 2021

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 1, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 2, 2021

Uh oh!

ajtowns commented Aug 2, 2021

Uh oh!

Uh oh!

maflcko commented Jul 3, 2019 •

edited

Loading

DrahtBot commented Jul 3, 2019 •

edited

Loading

ajtowns commented Jul 16, 2019 •

edited

Loading

jonatack left a comment •

edited

Loading