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,:
-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
-testactivationheight=bip34,<height>
-testactivationheight=dersig,<height>
…
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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.
Concept ACK
Concept ACK
-testactivationheight=bip34,<height>
Done
Strong Concept ACK
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
nit: For consistency reasons, could also set 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).
Thanks, done.
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
After changing this, the log message "Test that a version 3 block with..." should also be updated.
Thanks, done.
LGTM overall, left two small improvement suggestions.
ACK fa32518ae3a670f2a964812806d4b07cc488c8a2 🍴
re-ACK fa5b518f165dd220dce2f46ad590b8ce8bdd26d8
Checked via git range-diff fa32518a...fa5b518f that changes since my last ACK are only rebase-related.
Force pushed (trivial, without rebase)
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},
Wouldn't having slightly different activation heights here (even it its 1,2,3,4,5) make for a better test?
Can do on the next force push or another pull request (whichever happens earlier)
Code review ACK fa4db8671bb604e11b43a837f91de8866226f166
re-ACK fa4db8671bb604e11b43a837f91de8866226f166
The change of 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?
No objection setting it back to 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.