All softforks that are active at the tip of mainnet, should also be active from genesis in regtest. Otherwise their rules might not be enforced in user testing, thus making their testing less useful.
To still allow tests to check pre-softfork rules, a runtime argument can change the activation height.
Concept ACK. But I would personally prefer to overload one argument for the different activation heights, instead of adding a new argument for every single thing, for now and in the future. Either e,g,:
0-testactivationheights=<bip34height>,<dersigheight>,<cltvheight>,<csvheight>
(this will work fine for the tests which “know” about everything anyway, even if new things get added) or, alternatively
0-testactivationheight=bip34,<height>
1-testactivationheight=dersig,<height>
2…
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
-testactivationheight=bip34,
Done
397- consensus.BIP66Height = 102; // BIP66 activated on regtest (Block at height 101 and earlier not enforced for testing purposes)
398- consensus.CSVHeight = 432; // CSV activated on regtest (Used in rpc activation tests)
399+ consensus.BIP65Height = 1; // Always active unless overridden
400+ consensus.BIP66Height = 1; // Always active unless overridden
401+ consensus.CSVHeight = 1; // Always active unless overridden
402 consensus.SegwitHeight = 0; // SEGWIT is always activated on regtest unless overridden
SegwitHeight to 1? Otherwise this implies to the reader that there’s a subtle difference. (Maybe there is one that I’m not aware of, but at least the tests still seem to pass with a value of 1).
108@@ -110,7 +109,7 @@ def run_test(self):
109 peer.sync_with_ping()
110
111 self.log.info("Test that transactions with non-DER signatures cannot appear in a block")
112- block.nVersion = 3
113+ block.nVersion = 4
re-ACK fa5b518f165dd220dce2f46ad590b8ce8bdd26d8
Checked via git range-diff fa32518a...fa5b518f that changes since my last ACK are only rebase-related.
D’oh, didn’t see the “Needs rebase” label when I did the previous review. At least the re-review is trivial now.
ACK fae5b05486ae90f630cb26226f2e656507b391c9 1️⃣
segwitheight is already 0 for regtest
The block version does not have any effect on the segwit consensus rules
or block relay logic.
Same for feature_dersig.
This will be needed in a later commit.
139@@ -142,11 +140,11 @@ def _test_getblockchaininfo(self):
140 assert_greater_than(res['size_on_disk'], 0)
141
142 assert_equal(res['softforks'], {
143- 'bip34': {'type': 'buried', 'active': True, 'height': 2},
144- 'bip66': {'type': 'buried', 'active': True, 'height': DERSIG_HEIGHT},
145- 'bip65': {'type': 'buried', 'active': True, 'height': CLTV_HEIGHT},
146- 'csv': {'type': 'buried', 'active': False, 'height': 432},
147- 'segwit': {'type': 'buried', 'active': True, 'height': 0},
148+ 'bip34': {'type': 'buried', 'active': True, 'height': 1},
consensus.SegwitHeight from 0 to 1 has the effect that if I create a regtest enviroment with a current master (or 23.0), and then try to load the chain with an older version (e.g. 22.0), I get an InitError
Witness data for blocks after height 0 requires validation. Please restart with -reindex
because BLOCK_OPT_WITNESS is no longer set for the Genesis block and NeedsRedownload() in validation returns true.
That might be a bit annoying for some tests - @MarcoFalke @theStack what do you think?
0. An alternative might be to just use -reindex with 22.x, which will make the datadir compatible for 22.x and 23.x at the same time.