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

pull theStack wants to merge 3 commits into bitcoin:master from theStack:202111-test-refactor-use_createblock_parameters changing 14 files +37 −73

theStack commented at 6:06 PM on November 15, 2021: member

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 (review)).
theStack force-pushed on Nov 15, 2021
DrahtBot added the label Tests on Nov 15, 2021
DrahtBot commented at 5:30 AM on November 16, 2021: member



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



Conflicts

No conflicts as of last run.
theStack force-pushed on Nov 17, 2021
theStack commented at 11:40 AM on November 17, 2021: member

Rebased on master.
in test/functional/feature_assumevalid.py:135 in 66e97bd219 outdated
```
 131 | @@ -134,8 +132,7 @@ def run_test(self):
 132 |  
 133 |          # Bury the assumed valid block 2100 deep
 134 |          for _ in range(2100):
 135 | -            block = create_block(self.tip, create_coinbase(height), self.block_time)
 136 | -            block.nVersion = 4
 137 | +            block = create_block(self.tip, create_coinbase(height), self.block_time, version=4)
```
MarcoFalke commented at 12:01 PM on November 17, 2021:

any reason to set 4? This is already the default and shouldn't matter either way?

theStack commented at 1:17 PM on November 17, 2021:

No concrete reason, this was simply planned as pure refactoring (deduplication) PR with minimal review burden. Taking a closer look at it, you are right though, so I removed it and updated the PR description accordingly.
theStack force-pushed on Nov 17, 2021

in test/functional/feature_taproot.py:1270 in 397f6cb4ee outdated

1271 | -        for tx in txs:
1272 | -            tx.rehash()
1273 | -            block.vtx.append(tx)
1274 | -        block.hashMerkleRoot = block.calc_merkle_root()
1275 | +        coinbase_tx = create_coinbase(self.lastblockheight + 1, pubkey=cb_pubkey, extra_output_script=extra_output_script, fees=fees)
1276 | +        block = create_block(self.tip, coinbase_tx, self.lastblocktime + 1, version=4, txlist=txs)

MarcoFalke commented at 1:42 PM on November 17, 2021:

same here

in test/functional/p2p_segwit.py:234 in 397f6cb4ee outdated

 230 | @@ -231,8 +231,7 @@ def build_next_block(self):
 231 |          tip = self.nodes[0].getbestblockhash()
 232 |          height = self.nodes[0].getblockcount() + 1
 233 |          block_time = self.nodes[0].getblockheader(tip)["mediantime"] + 1
 234 | -        block = create_block(int(tip, 16), create_coinbase(height), block_time)
 235 | -        block.nVersion = 4
 236 | +        block = create_block(int(tip, 16), create_coinbase(height), block_time, version=4)

MarcoFalke commented at 1:43 PM on November 17, 2021:

same here

in test/functional/feature_csv_activation.py:176 in 397f6cb4ee outdated

 172 | @@ -173,10 +173,7 @@ def generate_blocks(self, number):
 173 |          return test_blocks
 174 |  
 175 |      def create_test_block(self, txs):
 176 | -        block = create_block(self.tip, create_coinbase(self.tipheight + 1), self.last_block_time + 600)
 177 | -        block.nVersion = 4
 178 | -        block.vtx.extend(txs)
 179 | -        block.hashMerkleRoot = block.calc_merkle_root()
 180 | +        block = create_block(self.tip, create_coinbase(self.tipheight + 1), self.last_block_time + 600, version=4, txlist=txs)

MarcoFalke commented at 1:44 PM on November 17, 2021:

same here

MarcoFalke commented at 1:45 PM on November 17, 2021: member

The other, too?
test: refactor: take use of `create_block` version parameter (or use default) ae9df4ef93

test: refactor: take use of `create_block` txlist parameter

Passing a list of transactions `txlist` to `create_block` appends
them to the block, hence we don't need to do that manually anymore.
The merkle root calculation can also be removed, since this is done
in the end of the helper.

df5d783aef

test: refactor: replace OP_1/OP_TRUE magic numbers by constants e57c0eb865
theStack commented at 3:01 PM on November 17, 2021: member

As suggested by MarcoFalke, I removed now all instances of explicitely setting block version to 4. FWIW as historical context, blocks are created with version 4 by default since #16333 (commit fac90c55be478f0323eafa1d560ea2c56f04fb23 was created in 2019, but the PR was merged only a few months ago).
theStack force-pushed on Nov 17, 2021
stratospher commented at 3:39 PM on November 18, 2021: contributor

tested ACK e57c0eb.
MarcoFalke merged this on Nov 22, 2021
MarcoFalke closed this on Nov 22, 2021
theStack deleted the branch on Nov 22, 2021
sidhujag referenced this in commit 2d78e62f2c on Nov 22, 2021
sidhujag referenced this in commit 0a4d911d8b on Nov 23, 2021
DrahtBot locked this on Nov 22, 2022

Contributors

Labels

Linked (view graph)

#16333 test: Set BIP34Height = 2 for regtest

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

Conflicts