test: Activate all regtest softforks at height 1, unless overridden #22818

maflcko · 2021-08-27T13:39:46Z

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.

laanwj · 2021-08-27T19:57:49Z

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>
…

DrahtBot · 2021-08-28T07:28:13Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22976 (scripted-diff: Rename overloaded int GetArg to GetIntArg by ryanofsky)
#22711 (test: check for specific block reject reasons in p2p_segwit.py by theStack)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
#10102 ([experimental] Multiprocess bitcoin by ryanofsky)

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.

theStack · 2021-08-28T18:11:11Z

Concept ACK

kristapsk · 2021-08-28T18:19:27Z

Concept ACK

maflcko · 2021-08-30T14:52:41Z

-testactivationheight=bip34,

Done

practicalswift · 2021-09-04T13:51:02Z

Strong Concept ACK

theStack

LGTM overall, left two small improvement suggestions.

src/chainparams.cpp

test/functional/feature_dersig.py

theStack

ACK fa32518 🍴

theStack

re-ACK fa5b518

Checked via git range-diff fa32518a...fa5b518f that changes since my last ACK are only rebase-related.

maflcko · 2021-09-10T10:21:40Z

Force pushed (trivial, without rebase)

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.

laanwj · 2021-09-20T21:06:14Z

test/functional/rpc_blockchain.py

-            'bip65': {'type': 'buried', 'active': True, 'height': CLTV_HEIGHT},
-            'csv': {'type': 'buried', 'active': False, 'height': 432},
-            'segwit': {'type': 'buried', 'active': True, 'height': 0},
+            '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)

laanwj · 2021-09-20T21:06:59Z

Code review ACK fa4db86

theStack

re-ACK fa4db86

…, unles…

… rpc_blockchain fa4ca8d test: Add -testactivationheight tests to rpc_blockchain (MarcoFalke) Pull request description: Suggested: bitcoin/bitcoin#22818 (comment) ACKs for top commit: laanwj: Code review ACK fa4ca8d theStack: Concept and code-review ACK fa4ca8d Tree-SHA512: 41304db1a15c0c705a9cc2808c9f1d7831a321a8a7948a28ec5d3ee1ed3da6a0ce67cd50c99a33aaed86830c59608eb6ffadbeaba67d95245c490f9b6c277912

…ckchain fa4ca8d test: Add -testactivationheight tests to rpc_blockchain (MarcoFalke) Pull request description: Suggested: bitcoin#22818 (comment) ACKs for top commit: laanwj: Code review ACK fa4ca8d theStack: Concept and code-review ACK fa4ca8d Tree-SHA512: 41304db1a15c0c705a9cc2808c9f1d7831a321a8a7948a28ec5d3ee1ed3da6a0ce67cd50c99a33aaed86830c59608eb6ffadbeaba67d95245c490f9b6c277912

mzumsande

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?

maflcko · 2022-03-10T18:51:36Z

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.

This was changed in bitcoin#22818 from 0 to 1. Since it changes BLOCK_OPT_WIT of the genesis block, older versions of bitcoin core would not read regtest directories created with newer versions without a reindex.

mzumsande · 2022-03-10T19:48:50Z

see #24527

5ce3057 test: set segwit height back to 0 on regtest (Martin Zumsande) Pull request description: The change of `consensus.SegwitHeight` from 0 to 1 for regtest in bitcoin#22818 had the effect that if I create a regtest enviroment with current master (or 23.x), and then try to load this chain with an older version (22.x), I get an InitError `Witness data for blocks after height 0 requires validation. Please restart with -reindex` and have to reindex because `BLOCK_OPT_WITNESS` is no longer set for the Genesis block and `NeedsRedownload()` in validation returns `true` with an older version. That might be a bit annoying for tests that use a shared regtest dir with different versions. If people think this is enough of an issue to be worth fixing, I think it should also make it into 23.x ACKs for top commit: theStack: Concept and code-review ACK 5ce3057 Tree-SHA512: b0e89ff7fc953bc0ae929d2da44cde7149321d987fb4763934f6c9635d00d807129a50b459cc5e69e86bb1819e4b063b969486e8016a1cb8db8f905fa315653d

This was changed in bitcoin#22818 from 0 to 1. Since it changes BLOCK_OPT_WIT of the genesis block, older versions of bitcoin core would not read regtest directories created with newer versions without a reindex. Github-Pull: bitcoin#24527 Rebased-From: 5ce3057

5ce3057 test: set segwit height back to 0 on regtest (Martin Zumsande) Pull request description: The change of `consensus.SegwitHeight` from 0 to 1 for regtest in bitcoin#22818 had the effect that if I create a regtest enviroment with current master (or 23.x), and then try to load this chain with an older version (22.x), I get an InitError `Witness data for blocks after height 0 requires validation. Please restart with -reindex` and have to reindex because `BLOCK_OPT_WITNESS` is no longer set for the Genesis block and `NeedsRedownload()` in validation returns `true` with an older version. That might be a bit annoying for tests that use a shared regtest dir with different versions. If people think this is enough of an issue to be worth fixing, I think it should also make it into 23.x ACKs for top commit: theStack: Concept and code-review ACK 5ce3057 Tree-SHA512: b0e89ff7fc953bc0ae929d2da44cde7149321d987fb4763934f6c9635d00d807129a50b459cc5e69e86bb1819e4b063b969486e8016a1cb8db8f905fa315653d

This was changed in bitcoin#22818 from 0 to 1. Since it changes BLOCK_OPT_WIT of the genesis block, older versions of bitcoin core would not read regtest directories created with newer versions without a reindex. Github-Pull: bitcoin#24527 Rebased-From: 5ce3057

DrahtBot added the Validation label Aug 27, 2021

DrahtBot mentioned this pull request Aug 28, 2021

refactor: Clarify and disable unused ArgsManager flags #22766

Merged

This was referenced Aug 28, 2021

multiprocess: Add bitcoin-gui -ipcconnect option #19461

Draft

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

Draft

refactor: Implement missing error checking for ArgsManager flags #16545

Draft

Multiprocess bitcoin #10102

Draft

maflcko force-pushed the 2108-regtestSoft branch 4 times, most recently from cb12298 to facb4bc Compare August 30, 2021 12:42

DrahtBot mentioned this pull request Aug 30, 2021

test: check for specific block reject reasons in p2p_segwit.py #22711

Merged

maflcko force-pushed the 2108-regtestSoft branch from facb4bc to fae8d47 Compare September 3, 2021 15:59

theStack reviewed Sep 5, 2021

View reviewed changes

src/chainparams.cpp Outdated Show resolved Hide resolved

test/functional/feature_dersig.py Show resolved Hide resolved

maflcko force-pushed the 2108-regtestSoft branch 2 times, most recently from fa89e40 to fa32518 Compare September 6, 2021 09:19

theStack approved these changes Sep 6, 2021

View reviewed changes

DrahtBot mentioned this pull request Sep 7, 2021

test: Avoid intermittent test failure in feature_csv_activation.py #22907

Merged

DrahtBot added the Needs rebase label Sep 9, 2021

maflcko force-pushed the 2108-regtestSoft branch from fa32518 to fa5b518 Compare September 9, 2021 07:59

DrahtBot added Needs rebase and removed Needs rebase labels Sep 9, 2021

theStack approved these changes Sep 10, 2021

View reviewed changes

maflcko force-pushed the 2108-regtestSoft branch from fa5b518 to fae5b05 Compare September 10, 2021 10:20

MarcoFalke added 4 commits September 16, 2021 18:52

test: Remove version argument from build_next_block in p2p_segwit test

faa4698

The block version does not have any effect on the segwit consensus rules or block relay logic. Same for feature_dersig.

test: Add extra_args argument to TestChain100Setup constructor

fadb2ef

This will be needed in a later commit.

Introduce -testactivationheight=name@height setting

faad1e5

test: Activate all regtest softforks at height 1, unless overridden

fa4db86

maflcko force-pushed the 2108-regtestSoft branch from fae5b05 to fa4db86 Compare September 16, 2021 16:54

DrahtBot removed the Needs rebase label Sep 16, 2021

laanwj reviewed Sep 20, 2021

View reviewed changes

theStack approved these changes Sep 22, 2021

View reviewed changes

maflcko merged commit 8e9801b into bitcoin:master Sep 24, 2021

maflcko deleted the 2108-regtestSoft branch September 24, 2021 12:07

maflcko mentioned this pull request Sep 24, 2021

test: Add -testactivationheight tests to rpc_blockchain #23086

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 24, 2021

Merge bitcoin#22818: test: Activate all regtest softforks at height 1…

2d58c2f

…, unles…

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 24, 2021

Merge bitcoin#22818: test: Activate all regtest softforks at height 1…

da9da68

…, unles…

maflcko mentioned this pull request Nov 13, 2021

Bury taproot deployment #23505

Closed

mzumsande reviewed Mar 10, 2022

View reviewed changes

mzumsande mentioned this pull request Mar 10, 2022

test: set segwit height back to 0 on regtest #24527

Merged

delta1 added a commit to delta1/elements that referenced this pull request Apr 23, 2023

Merge 8e9801b into merged_master (Bitcoin PR bitcoin/bitcoin#22818)

9bdd001

delta1 added a commit to delta1/elements that referenced this pull request Apr 24, 2023

Merge 8e9801b into merged_master (Bitcoin PR bitcoin/bitcoin#22818)

13fb374

delta1 added a commit to delta1/elements that referenced this pull request Apr 24, 2023

test: elements fixes for Bitcoin PR bitcoin/bitcoin#22818

7856df2

bitcoin locked and limited conversation to collaborators Jun 24, 2023

test: Activate all regtest softforks at height 1, unless overridden #22818

test: Activate all regtest softforks at height 1, unless overridden #22818

Uh oh!

Conversation

maflcko commented Aug 27, 2021

Uh oh!

laanwj commented Aug 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

theStack commented Aug 28, 2021

Uh oh!

kristapsk commented Aug 28, 2021

Uh oh!

maflcko commented Aug 30, 2021

Uh oh!

practicalswift commented Sep 4, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Sep 10, 2021

Uh oh!

laanwj Sep 20, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 21, 2021

Choose a reason for hiding this comment

Uh oh!

laanwj commented Sep 20, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 10, 2022

Uh oh!

mzumsande commented Mar 10, 2022

Uh oh!

Uh oh!

laanwj commented Aug 27, 2021 •

edited

Loading

DrahtBot commented Aug 28, 2021 •

edited

Loading