Support having SegWit always active in regtest (sipa, ajtowns, jnewbery)

sipa commented at 6:07 am on September 23, 2017: member

Most tests shouldn’t have to deal with the now-historical SegWit activation transition (and other deployments, but SegWit is certainly the hardest one to accomodate).

This PR makes a versionbits starttime of -1 equal to “always active”, and enables it by default for SegWit on regtest. Individual tests can override this by using the existing -vbparams option.

A few unit tests and functional tests are adapted to indeed override vbparams, as they specifically test the transition.

This is in preparation for wallet SegWit support, but I thought having earlier eyes on it would be useful.

fanquake added the label Tests on Sep 23, 2017

laanwj commented at 11:15 am on September 25, 2017: member

Concept ACK

in test/functional/sendheaders.py:216 in 2902cec680 outdated

214@@ -215,7 +215,7 @@ def run_test(self):
215         connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], inv_node))
216         # Set nServices to 0 for test_node, so no block download will occur outside of

jl2012 commented at 3:11 pm on September 25, 2017:

Please fix the comment

MarcoFalke commented at 8:49 pm on November 7, 2017:

This was not fixed

TheBlueMatt commented at 9:22 pm on September 26, 2017: member

Concept ACK @sdaftuar may be interested.

jonasschnelli commented at 2:47 am on September 28, 2017: contributor

Yes. Please.

utACK 2902cec68031d61b5707f24edaebb4c26ee1712b

laanwj requested review from sdaftuar on Oct 4, 2017

laanwj commented at 10:35 am on October 5, 2017: member

I’m adding this as “high priority for review” because it’s a dependency for #11403.

MarcoFalke added this to the milestone 0.15.1 on Oct 5, 2017

MarcoFalke added the label Needs backport on Oct 5, 2017

jl2012 commented at 12:57 pm on October 5, 2017: contributor

utACK 2902cec, but please fix comment in sendheaders.py to “Set nServices to NODE_WITNESS for test_node” or something that matches the code

in src/test/test_bitcoin.cpp:109 in 2902cec680 outdated

104@@ -105,6 +105,7 @@ TestingSetup::~TestingSetup()
105 
106 TestChain100Setup::TestChain100Setup() : TestingSetup(CBaseChainParams::REGTEST)
107 {
108+    UpdateVersionBitsParameters(Consensus::DEPLOYMENT_SEGWIT, 0, 999999999999ULL);

jnewbery commented at 5:24 pm on October 5, 2017:

nit: add comment saying “never activate segwit for these tests”

jnewbery commented at 3:30 pm on October 16, 2017:

not addressed in 6998584728e45f77f7f67e65e17fb459558739f2. I think a comment would make it clearer why you’re updating the vb parameter here.

theuni commented at 10:58 pm on October 16, 2017:

For anyone curious, the problem with these tests is that they create a block via CreateAndProcessBlock(), which uses BlockAssembler::CreateNewBlock(), then it adds transactions to the new block. The IncrementExtraNonce corrects for the new merkle root, but not the new witness root. GenerateCoinbaseCommitment wouldn’t work as it only adds a commitment if there’s not one there already.

Updating the params here is fine imo, but if anyone would prefer to avoid it, we’d just need a ForceUpdateWitnessCommitment() function.

sipa commented at 2:54 pm on October 23, 2017:

Added a comment.

in test/functional/nulldummy.py:45 in 2902cec680 outdated

39@@ -40,7 +40,7 @@ class NULLDUMMYTest(BitcoinTestFramework):
40     def set_test_params(self):
41         self.num_nodes = 1
42         self.setup_clean_chain = True
43-        self.extra_args = [['-whitelist=127.0.0.1', '-walletprematurewitness']]
44+        self.extra_args = [['-whitelist=127.0.0.1', '-walletprematurewitness', '-vbparams=segwit:0:999999999999']]

jnewbery commented at 5:38 pm on October 5, 2017:

request: add a comment to say that this script tests nulldummy activation, which is implemented to activate at the same height as SegWit, so we set SegWit to be inactive initially.

sipa commented at 7:17 am on October 12, 2017:

Done.

in test/functional/p2p-fullblocktest.py:56 in 2902cec680 outdated

52@@ -53,6 +53,7 @@ class FullBlockTest(ComparisonTestFramework):
53     # Change the "outcome" variable from each TestInstance object to only do the comparison.
54     def set_test_params(self):
55         self.num_nodes = 1
56+        self.extra_args = [["-vbparams=segwit:0:999999999999", "-whitelist=127.0.0.1"]]

jnewbery commented at 5:41 pm on October 5, 2017:

I don’t think this change is needed. In fact, -whitelist isn’t needed either so we could remove this line entirely.

sipa commented at 7:17 am on October 12, 2017:

Done.

sipa force-pushed on Oct 5, 2017

sipa commented at 5:57 pm on October 5, 2017: member

@jl2012 Done.

laanwj added this to the "Blockers" column in a project

jnewbery commented at 9:55 pm on October 5, 2017: member

I think this is ok, although it does mean that our default test framework environment is now running with segwit active, but p2sh not active, which is impossible on mainnet (and is in fact causes an assert in the script interpreter here: https://github.com/bitcoin/bitcoin/blob/17f2acedbe078f179556f4550eca547726f087e1/src/script/interpreter.cpp#L1552 if a witness block is received)

assumevalid.py fails with this PR, but we can fix that by liberally sprinkling on some "-vbparams=segwit:0:999999999999". I have a commit that does that here: https://github.com/jnewbery/bitcoin/tree/pr11389.

Longer term, I think we should also make P2SH always active in regtest so we’re not running in this impossible environment.

EDIT: also appears to break zmq_test.py (which I haven’t tested locally or fixed)

NicolasDorier commented at 4:07 am on October 6, 2017: contributor

Can we do the same for CSV?

jl2012 commented at 8:02 am on October 6, 2017: contributor

zmq_test.py failed. I think you need to fix it like you did in #11403

jl2012 commented at 8:04 am on October 6, 2017: contributor

@NicolasDorier : why CSV? This one is needed because we need to make segwit addresses as default

JeremyRubin commented at 5:21 pm on October 6, 2017: contributor

I think it might be good to make it such that bit must also be set to -1 (or similarly like 65 or something) if the starttime = -1.

This ensures that there aren’t further changes needed if the bit is reused by another soft fork.

achow101 commented at 9:16 pm on October 8, 2017: member

Concept ACK

jnewbery commented at 7:55 pm on October 9, 2017: member

I have a fix for the failing zmq_test.py in my branch: https://github.com/jnewbery/bitcoin/tree/pr11389

NicolasDorier commented at 4:44 am on October 10, 2017: contributor

@jl2012 well there is no good reason to have a regtest without CSV activated.

sdaftuar commented at 7:41 pm on October 10, 2017: member

@jnewbery’s observation about p2sh should be addressed: if we are setting segwit to always active in regtest, we should also set p2sh to always be active as well.

However if we think it’s likely we’d merge something like what @jl2012 has proposed in #11398, then I think this implementation would immediately go unused, as we’d be replacing the need for the versionbits state machine to have a special hard coded value in favor of putting a hardcoded height in the validation layer above that. I think something like #11398 makes sense, and it supports setting segwit to always being on in regtest as well, so I’d prefer that we do that instead.

theuni commented at 8:03 pm on October 10, 2017: member

utACK modulo @jnewbery’s comments.

sipa force-pushed on Oct 12, 2017

sipa commented at 7:29 am on October 12, 2017: member

I’ve pushed a rebase with some extra changes:

When a versionbits deployment has a negative start time, it is active from height (the absolute value of) that start time (rather than just -1 meaning always).
BIP16 deployment is changed into a versionbits deployment with accurate negative start times. This is certainly an improvement over the existing hardcoded chain-independent timestamp, though perhaps people prefer a BIP16Height approach or an approach like #11426/#11398. I’ve not done that to reduce code churn here, and still easily override start times.

In general, I think we should make deployments buried when they’re completely over, but there is still value in being able to have a mix, where the versionbits deployment applies to some chains, but regtest can have an overridden always-on or scheduled-at-some-height deployment for easy of testing before the feature is actually buried.

jnewbery commented at 6:46 pm on October 12, 2017: member

I much prefer the approach in #11398 . Once a deployment is sufficiently buried, I think we should remove all logic around how that deployment was activated. At that point it doesn’t matter how the deployment was activated. We just want to know that if we’re on a certain chain at a certain height we should start enforcing a rule.

I think inventing a fiction that old softforks were activated using BIP9 is a complication rather than a simplification.

To respond to your points in #11398:

Advantages:

Easier to review the change that buries a deployment.

You’re right that this makes the code delta to bury a BIP9 deployment smaller, but for someone looking at the code for the first time, if (pindex->nHeight >= consensusparams.BIPxxHeight) is much easier to understand than if (VersionBitsState(pindex->pprev, consensusparams, Consensus::DEPLOYMENT_xx, versionbitscache) == THRESHOLD_ACTIVE) where I need to look at and understand the AbstractThresholdConditionChecker class and understand that -ve numbers mean pinned deployments.

Support making a deployment buried in regtest while it isn’t yet in mainnet/testnet (which can simplify functional testing)

We already have control to make a BIP9 deployment active at a certain height in regtest using -vbparams. What advantages do you see for making a deployment buried instead of just activated at a height?

I’m ambivalent on #11426. To me that’s just a syntax change and I don’t think it’s materially better or worse than what we have already.

sipa commented at 8:20 pm on October 12, 2017: member

@jnewbery Good points, but perhaps I’m not making my intent clear.

I’ll start with describing my ideal end goal:

There is an option to configure the height at which (any) consensus rule activated, both for old deployments, and (originally) BIP9 deployments.
When no special activate-at-height is given, the normal activation rules (BIP9 or otherwise) apply.
After a deployment is buried, the height-activation can be made default for that deployment, and its old deployment logic removed. Depending on whether tests need it or historical exceptions, it could be removed entirely and hardcoded.

Having this means that for a new softfork it’s possible to write a test that has the new rule always active, or only active at some particular height (and not needing to go through hoops to activate it by mining blocks manually) - while it’s not even defined on mainnet.

This functionality partially overlaps with the requirements for #11403 (not needing to go through activation for tests that are unrelated to SegWit activation), which indirectly also needs a way to have P2SH always active.

This PR implements effectively what was described above, but with the caveats (1) only for BIP9 deployments and BIP16 and (2) through a pretty hacky way of classifying BIP16 as a VB deployment. All of that can be cleaned up by having explicit buried deployments, though. This is regtest-only functionality that doesn’t need much compatibility between releases IMHO.

I think it’s also orthogonal to the question of actually completely removing the segwit or P2SH deployments - both of those are useful too, but as discussed in the meeting today, that may not be super trivial.

Finally,

We already have control to make a BIP9 deployment active at a certain height in regtest using -vbparams. What advantages do you see for making a deployment buried instead of just activated at a height?

That just doesn’t accomplish the goal, as BIP9 can at the earliest activate a deployment at 3*interval, which is at height 432 for regtest. For SegWit wallet support I really want a way to have its rules always active. It’s certainly possible to go modify all the heights in all the tests, but that’s busywork, and it really shouldn’t be the test’s responsibility to deal with that.

jnewbery commented at 9:53 pm on October 12, 2017: member

I’ll start with describing my ideal end goal:

Good! We have the same ideal end goal.

This PR implements effectively what was described above, but with the caveats (1) only for BIP9 deployments and BIP16 and (2) through a pretty hacky way of classifying BIP16 as a VB deployment

Yes - that’s what I see as a complication rather than a simplification. I prefer #11398 and also pinning SegWit to block 481824. Obviously I won’t NACK this if it’s a blocker for #11403. In Apache voting terms, I’m a -0.

I think it’s also orthogonal to the question of actually completely removing the segwit or P2SH deployments - both of those are useful too, but as discussed in the meeting today, that may not be super trivial.

By that, do you mean pinning the activation to genesis? Yes, agree that it’s orthogonal and can be considered later.

That just doesn’t accomplish the goal, as BIP9 can at the earliest activate a deployment at 3*interval.

Yes - I understand now.

sipa commented at 9:57 pm on October 12, 2017: member

Yes - that’s what I see as a complication rather than a simplification. I prefer #11398 and also pinning SegWit to block 481824.

Sure, that’s a bigger step towards the intended goal, but I don’t see that happening for 0.15.1. #11398 seems pretty invasive.

sipa force-pushed on Oct 13, 2017

sipa commented at 11:18 pm on October 13, 2017: member

Updated to use @jnewbery’s height-pinned-P2SH instead of the P2SH-was-really-a-BIP9-deployment-I-promise approach.

in src/consensus/params.h:30 in b934a2c452 outdated

26@@ -27,7 +27,7 @@ enum DeploymentPos
27 struct BIP9Deployment {
28     /** Bit position to select the particular bit in nVersion. */
29     int bit;
30-    /** Start MedianTime for version bits miner confirmation. Can be a date in the past */
31+    /** Start MedianTime for version bits miner confirmation. Can be a date in the past. Negative numbers mean hardcoded height. */

ryanofsky commented at 1:35 pm on October 16, 2017:

Could expand comment to suggest why a bip9 deployment would have a hardcoded activation height.

ryanofsky commented at 1:58 pm on October 16, 2017:

0
1EDIT: Never mind. I was thinking genesis block had height 1 not height 0 (misremembered it was depths not heights that were counted from 1).

morcos commented at 7:26 pm on October 17, 2017:

I would expand this comment a bit. I think its a bit ugly and counterintuitive that positive numbers are times and negative numbers are heights. I think I’m ok with the hack, but it should be more clearly identified

sipa commented at 2:54 pm on October 23, 2017:

Done.

sipa commented at 2:56 pm on October 23, 2017:

Better now?

in src/versionbits.cpp:31 in b934a2c452 outdated

26@@ -27,6 +27,12 @@ ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex*
27     int64_t nTimeStart = BeginTime(params);
28     int64_t nTimeTimeout = EndTime(params);
29 
30+    // Check if this deployment has a fixed activation height.
31+    if (nTimeStart < 0) {

ryanofsky commented at 2:00 pm on October 16, 2017:

Possible to test this logic in versionbits_tests.cpp?

theuni commented at 7:36 pm on October 16, 2017:

Agree, this really needs a test :(

Might be better to do it in the python framework though, to make sure that negative values are parsed correctly all the way down from the cmdline.

ajtowns commented at 9:06 am on October 17, 2017:

I’ve added some tests to versionbits_tests in https://github.com/sipa/bitcoin/pull/106 fwiw.

sipa commented at 2:55 pm on October 23, 2017:

@ajtowns Thanks, I’ve included some of those commits, but reworked them a bit (I’ve simplied the code to only support always-active, so the tests are easier now).

ryanofsky commented at 2:09 pm on October 16, 2017: member

utACK 6998584728e45f77f7f67e65e17fb459558739f2

in test/functional/p2p-compactblocks.py:98 in 6998584728 outdated

92@@ -93,7 +93,7 @@ def set_test_params(self):
93         self.setup_clean_chain = True
94         # Node0 = pre-segwit, node1 = segwit-aware
95         self.num_nodes = 2
96-        self.extra_args = [["-vbparams=segwit:0:0"], ["-txindex"]]
97+        self.extra_args = [["-vbparams=segwit:0:0"], ["-vbparams=segwit:0:999999999999", "-txindex"]]

jnewbery commented at 4:42 pm on October 16, 2017:

Perhaps add a comment here to say something like:

This test was written assuming segwit is activated using BIP9 at height 432 (3 x nMinerConfirmationWindows of 144). TODO: rewrite this test to have segwit always active.

sipa commented at 2:55 pm on October 23, 2017:

Added comment.

in test/functional/p2p-segwit.py:115 in 6998584728 outdated

110@@ -111,7 +111,7 @@ class SegWitTest(BitcoinTestFramework):
111     def set_test_params(self):
112         self.setup_clean_chain = True
113         self.num_nodes = 3
114-        self.extra_args = [["-whitelist=127.0.0.1"], ["-whitelist=127.0.0.1", "-acceptnonstdtxn=0"], ["-whitelist=127.0.0.1", "-vbparams=segwit:0:0"]]
115+        self.extra_args = [["-whitelist=127.0.0.1", "-vbparams=segwit:0:999999999999"], ["-whitelist=127.0.0.1", "-acceptnonstdtxn=0", "-vbparams=segwit:0:999999999999"], ["-whitelist=127.0.0.1", "-vbparams=segwit:0:0"]]

jnewbery commented at 4:43 pm on October 16, 2017:

suggestion: add comment saying that this test tests segwit pre- and post- activation, so segwit is activated using BIP9.

sipa commented at 2:56 pm on October 23, 2017:

Added comment.

in test/functional/segwit.py:81 in 6998584728 outdated

76@@ -77,9 +77,9 @@ class SegWitTest(BitcoinTestFramework):
77     def set_test_params(self):
78         self.setup_clean_chain = True
79         self.num_nodes = 3
80-        self.extra_args = [["-walletprematurewitness", "-rpcserialversion=0"],
81-                           ["-blockversion=4", "-promiscuousmempoolflags=517", "-prematurewitness", "-walletprematurewitness", "-rpcserialversion=1"],
82-                           ["-blockversion=536870915", "-promiscuousmempoolflags=517", "-prematurewitness", "-walletprematurewitness"]]
83+        self.extra_args = [["-walletprematurewitness", "-rpcserialversion=0", "-vbparams=segwit:0:999999999999"],

jnewbery commented at 4:43 pm on October 16, 2017:

suggestion: add comment saying that this test tests segwit pre- and post- activation, so segwit is activated using BIP9.

sipa commented at 2:56 pm on October 23, 2017:

Added comment.

in test/functional/zmq_test.py:81 in 6998584728 outdated

77+        body.deserialize(BytesIO(msg[1]))
78+        body.calc_sha256()
79         msgSequence = struct.unpack('<I', msg[-1])[-1]
80         assert_equal(msgSequence, 0) # must be sequence 0 on rawtx
81 
82         # Check that the rawtx hashes to the hashtx

jnewbery commented at 4:46 pm on October 16, 2017:

Update comment to say that the txid of the rawtx is the hashtx (it’s not strictly true to say that the rawtx hashes to the hashtx since the rawtx now also includes the witness)

sipa commented at 2:56 pm on October 23, 2017:

This code has recently been refactored, and no change is necessary now.

in src/chainparams.cpp:293 in 6998584728 outdated

291@@ -289,7 +292,7 @@ class CRegTestParams : public CChainParams {
292         consensus.vDeployments[Consensus::DEPLOYMENT_CSV].nStartTime = 0;
293         consensus.vDeployments[Consensus::DEPLOYMENT_CSV].nTimeout = 999999999999ULL;

theuni commented at 7:11 pm on October 16, 2017:

Unrelated nit, 999999999999ULL is a terribly confusing value for these nTimeouts!

sipa commented at 2:55 pm on October 23, 2017:

Used constants from @ajtowns’s commit.

in src/versionbits.cpp:32 in 6998584728 outdated

26@@ -27,6 +27,12 @@ ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex*
27     int64_t nTimeStart = BeginTime(params);
28     int64_t nTimeTimeout = EndTime(params);
29 
30+    // Check if this deployment has a fixed activation height.
31+    if (nTimeStart < 0) {
32+        if (pindexPrev && pindexPrev->nHeight + 1 >= -nTimeStart) return THRESHOLD_ACTIVE;

theuni commented at 7:25 pm on October 16, 2017:

paranoid nit: check for nTimeStart != std::numeric_limits<int64_t>::min()

sipa commented at 2:55 pm on October 23, 2017:

This logic has been simplified, so I don’t think this is necessary anymore.

jnewbery commented at 7:56 pm on October 16, 2017: member

Looks good. I’ve requested a few extra comments for clarity. Tested ACK 6998584728e45f77f7f67e65e17fb459558739f2.

I’d urge reviewers to carefully consider the implications of pinning the P2SH activation to a height instead of a time. This means, for example, that there are potential mainnet chains that were previously invalid but are now valid, namely where the pinned P2SH height is not reached by April 1st 2012, and an invalid P2SH spend is included before activation height but after the old activation time.

In practice, I’m confident that this isn’t a problem:

to make such a chain the most-work chain would require a 5 year re-org
the other way this could be encountered would be an invalid low-difficulty headers chain up to April 2012. We’re protected against this since the most recent checkpoint (height 295,000) is higher than the BIP16 activation height (173805).

In any case, this buried deployment should be written up as a BIP along with the other soft fork deployments we want to bury in v0.16 (CLTV, CSV, SegWit)

theuni commented at 8:40 pm on October 16, 2017: member

@jnewbery Good points. Add to that list: This could invalidate a block on current chains if timestamps went backwards at all after April 1, 2012. I checked a dozen or so blocks on mainnet/testnet for that, but I’d be more comfortable if others checked as well.

jnewbery commented at 10:08 pm on October 16, 2017: member

@theuni - yes, that’s another good reason not to use blocktime to deploy a softfork! I’m fairly certain that block times didn’t go backwards over the P2SH activation, but it was a possibility that P2SH would have been activated and then deactivated.

morcos approved

morcos commented at 7:44 pm on October 17, 2017: member

Alex was here 6998584

looks correct to me

sipa force-pushed on Oct 23, 2017

sipa commented at 2:57 pm on October 23, 2017: member

Rebased, and included some commits from @ajtowns. Instead of supporting negative numbers for any height, the only thing supported now is exactly -1 to mean always active.

in src/consensus/params.h:43 in 7470e86c5d outdated

39+
40+    static constexpr int64_t NO_TIMEOUT = 999999999999ULL;
41+    static constexpr int64_t ALWAYS_ACTIVE = -1;
42 };
43 
44+

jnewbery commented at 4:48 pm on October 23, 2017:

nit: please remove extra line.

sipa commented at 1:03 pm on October 24, 2017:

Done.

in src/consensus/params.h:36 in 7470e86c5d outdated

35+     *  activation cannot not use this. */
36     int64_t nStartTime;
37     /** Timeout/expiry MedianTime for the deployment attempt. */
38     int64_t nTimeout;
39+
40+    static constexpr int64_t NO_TIMEOUT = 999999999999ULL;

jnewbery commented at 4:53 pm on October 23, 2017:

I think these could do with comments, since it’s a bit of an abuse of the BIP9 parameters:

NO_TIMEOUT means that we’re using BIP9 deployment, but that there’s no timeout, ie we never go from DEFINED to FAILED
ALWAYS_ACTIVE means that BIP9 is bypassed and we go straight to NO_TIMEOUT.

(ok, perhaps that’s obvious, but I don’t think a comment would hurt)

jnewbery commented at 4:57 pm on October 23, 2017:

oops. I see that there is a comment above, next to the nStartTime variable. Perhaps move it down so it’s next to the constexpr?

sipa commented at 1:03 pm on October 24, 2017:

Done.

jnewbery commented at 5:08 pm on October 23, 2017: member

Tested ACK 7470e86c5de1a4ee979aedd81fc34671949bc73c.

A couple of very minor nits. Feel free to ignore them.

ajtowns approved

ajtowns commented at 11:41 am on October 24, 2017: member

Tested ACK 7470e86c5de1a4ee979aedd81fc34671949bc73c

+1 on just supporting nStartTime=-1 rather than generic heights; simpler is better.

That does make the commit description for 22daeb8a a little confusing though. Maybe “Unit tests for always-active BIP 9 deployments” ?

Also the dates on the two latest commits are out of order, which causes github to be confusing since it seems to think chronologically ordering commits is useful.

sipa force-pushed on Oct 24, 2017

sipa commented at 1:04 pm on October 24, 2017: member

@ajtowns Done.

jnewbery commented at 2:21 pm on October 24, 2017: member

Tested ACK ccbc90ee485111634cc8941961b491f5a9b0ce85.

Also independently verified that 173805 and 514 are the correct heights for BIP16 activation on mainnet and testnet3, and that no blocks after those have a blocktime before 1333238400.

in src/test/versionbits_tests.cpp:38 in 5d86e970c2 outdated

31@@ -32,6 +32,12 @@ class TestConditionChecker : public AbstractThresholdConditionChecker
32     int GetStateSinceHeightFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateSinceHeightFor(pindexPrev, paramsDummy, cache); }
33 };
34 
35+class TestFixedHeightConditionChecker : public TestConditionChecker
36+{
37+public:
38+    int64_t BeginTime(const Consensus::Params& params) const override { return -1; }

ryanofsky commented at 5:11 pm on October 24, 2017:

In commit “Unit tests for always-active versionbits.”

Could return ALWAYS_ACTIVE instead of -1 literally here.

sipa commented at 3:30 am on November 7, 2017:

Done.

ryanofsky commented at 5:17 pm on October 24, 2017: member

utACK ccbc90ee485111634cc8941961b491f5a9b0ce85. Changes since last review were switching to ALWAYS_ACTIVE/NO_TIMEOUT, rebasing, adding unit test and comments.

in src/consensus/params.h:36 in abae8fb9cf outdated

30@@ -31,6 +31,9 @@ struct BIP9Deployment {
31     int64_t nStartTime;
32     /** Timeout/expiry MedianTime for the deployment attempt. */
33     int64_t nTimeout;
34+
35+    /** Constant for nTimeout very far in the future. */
36+    static constexpr int64_t NO_TIMEOUT = 999999999999ULL;

theuni commented at 8:31 pm on October 24, 2017:

Micro nit: Still confusing value (int64 = unsigned long long). Not worth fixing unless other changes are needed too.

sipa commented at 3:30 am on November 7, 2017:

Done, switched to numeric_limits.

theuni commented at 8:34 pm on October 24, 2017: member

utACK ccbc90ee485111634cc8941961b491f5a9b0ce85

in src/consensus/params.h:40 in 7f1693bd73 outdated

33@@ -34,6 +34,11 @@ struct BIP9Deployment {
34 
35     /** Constant for nTimeout very far in the future. */
36     static constexpr int64_t NO_TIMEOUT = 999999999999ULL;
37+    /** Special value for nStartTime indicating that the deployment is always active.
38+     *  This is useful for testing, as it means tests don't need to deal with the activation
39+     *  process (which takes at least 3 BIP9 intervals). Only tests that specifically test the
40+     *  behaviour during activation cannot not use this. */

sdaftuar commented at 2:02 pm on October 31, 2017:

“cannot not” -> “cannot”

sipa commented at 3:31 am on November 7, 2017:

Done.

in src/test/versionbits_tests.cpp:109 in 5d86e970c2 outdated

105@@ -92,6 +106,7 @@ class VersionBitsTester
106         for (int i = 0; i < CHECKERS; i++) {
107             if (InsecureRandBits(i) == 0) {
108                 BOOST_CHECK_MESSAGE(checker[i].GetStateFor(vpblock.empty() ? nullptr : vpblock.back()) == THRESHOLD_DEFINED, strprintf("Test %i for DEFINED", num));
109+                BOOST_CHECK_MESSAGE(checker_fixed[i].GetStateFor(vpblock.empty() ? nullptr : vpblock.back()) == THRESHOLD_ACTIVE, strprintf("Test %i for ACTIVE (fixed height)", num));

sdaftuar commented at 2:34 pm on October 31, 2017:

This test is called TestDefined, but we’re checking here for THRESHOLD_ACTIVE for the always-active version bit – not really sure this makes sense?

I guess maybe it makes a certain amount of sense that we want to check that the always-on bit is ACTIVE and not DEFINED, but I think it could use a comment here (and in TestStarted/TestLockedIn). Also, I wonder why there’s no similar test for ACTIVE state in TestFailed?

sipa commented at 3:31 am on November 7, 2017:

Added to TestFailed.

in src/test/versionbits_tests.cpp:35 in 5d86e970c2 outdated

31@@ -32,6 +32,12 @@ class TestConditionChecker : public AbstractThresholdConditionChecker
32     int GetStateSinceHeightFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateSinceHeightFor(pindexPrev, paramsDummy, cache); }
33 };
34 
35+class TestFixedHeightConditionChecker : public TestConditionChecker

sdaftuar commented at 2:37 pm on October 31, 2017:

nit: “FixedHeight” should be “AlwaysActive” or similar, everywhere in this file.

sipa commented at 3:31 am on November 7, 2017:

Done, renamed in many places.

in src/test/versionbits_tests.cpp:62 in 5d86e970c2 outdated

58 public:
59-    VersionBitsTester() : num(0) {}
60+    VersionBitsTester() : num(0) {
61+        for (unsigned int  i = 0; i < CHECKERS; i++) {
62+            checker_fixed[i] = TestFixedHeightConditionChecker();
63+        }

sdaftuar commented at 2:41 pm on October 31, 2017:

nit: I believe these 3 lines are unnecessary.

sipa commented at 3:31 am on November 7, 2017:

Fixed.

sdaftuar approved

sdaftuar commented at 2:46 pm on October 31, 2017: member

utACK ccbc90ee485111634cc8941961b491f5a9b0ce85

in test/functional/zmq_test.py:11 in ccbc90ee48 outdated

 7@@ -8,6 +8,7 @@
 8 import struct
 9 
10 from test_framework.test_framework import BitcoinTestFramework, SkipTest
11+from test_framework.mininode import CTransaction, BytesIO

MarcoFalke commented at 4:54 pm on October 31, 2017:

nit: BytesIO is taken from the python stl, so from io import BytesIO should work as well

sipa commented at 3:31 am on November 7, 2017:

Done.

jtimon commented at 7:59 pm on November 2, 2017: contributor

utACK abae8fb9cf719d3a187d103737a3a3139c6f7e71 7f1693bd733862f997980548a0818b9100fb796a d6247b496510a6307cba22db3e29445b4511f861

fast review ack for the other 2 commits

fanquake added this to the "In progress" column in a project

Improve handling of BIP9Deployment limits

Small tweaks by Pieter Wuille.

526023aa7a

[consensus] Pin P2SH activation to block 173805 on mainnet 18e071841e

Always-active versionbits support d07ee77ab9

Unit tests for always-active versionbits. 4bd89210a1

Have SegWit active by default d618458184

sipa force-pushed on Nov 7, 2017

in src/test/test_bitcoin.cpp:111 in d618458184

105@@ -106,6 +106,9 @@ TestingSetup::~TestingSetup()
106 
107 TestChain100Setup::TestChain100Setup() : TestingSetup(CBaseChainParams::REGTEST)
108 {
109+    // CreateAndProcessBlock() does not support building SegWit blocks, so don't activate in these tests.
110+    // TODO: fix the code to support SegWit blocks.
111+    UpdateVersionBitsParameters(Consensus::DEPLOYMENT_SEGWIT, 0, Consensus::BIP9Deployment::NO_TIMEOUT);

MarcoFalke commented at 8:49 pm on November 7, 2017:

µnit: could use named args /* nStartTime */…, maybe?

MarcoFalke commented at 8:52 pm on November 7, 2017: member

utACK d618458184742b15a7ab0349127ede7a2946a182

MarcoFalke renamed this:
~~Support having SegWit always active in regtest~~
Support having SegWit always active in regtest (sipa, ajtowns, jnewbery)
on Nov 7, 2017

in test/functional/test_framework/mininode.py:1652 in d618458184

1648@@ -1649,7 +1649,7 @@ class NodeConn(asyncore.dispatcher):
1649         "regtest": b"\xfa\xbf\xb5\xda",   # regtest
1650     }
1651 
1652-    def __init__(self, dstaddr, dstport, rpc, callback, net="regtest", services=NODE_NETWORK, send_version=True):
1653+    def __init__(self, dstaddr, dstport, rpc, callback, net="regtest", services=NODE_NETWORK|NODE_WITNESS, send_version=True):

MarcoFalke commented at 9:38 pm on November 7, 2017:

µnit: Since you change the default here, no need to overwrite in p2p-segwit.py

ryanofsky commented at 9:53 pm on November 7, 2017: member

utACK d618458184742b15a7ab0349127ede7a2946a182. Changes since last review: numeric limits NO_TIMEOUT value, comment fix, always active comment, fixed -> active rename, python import tweak.

MarcoFalke merged this on Nov 7, 2017

MarcoFalke closed this on Nov 7, 2017

MarcoFalke referenced this in commit dd561667cb on Nov 7, 2017

MarcoFalke referenced this in commit fd676d2ccf on Nov 8, 2017

MarcoFalke referenced this in commit 0d12d41f4d on Nov 8, 2017

MarcoFalke referenced this in commit c9ed435475 on Nov 8, 2017

MarcoFalke referenced this in commit 2a65f1bee7 on Nov 8, 2017

MarcoFalke referenced this in commit 49aa56d6ea on Nov 8, 2017

meshcollider commented at 3:14 am on November 8, 2017: contributor

Post-merge utACK https://github.com/bitcoin/bitcoin/pull/11389/commits/d618458184742b15a7ab0349127ede7a2946a182

fanquake moved this from the "Blockers" to the "For backport" column in a project

fanquake moved this from the "In progress" to the "Done" column in a project

laanwj removed this from the "For backport" column in a project

MarcoFalke removed the label Needs backport on Nov 9, 2017

MarcoFalke removed this from the milestone 0.15.2 on Nov 9, 2017

MarcoFalke commented at 7:45 pm on November 9, 2017: member

Removing from backport

jonasschnelli referenced this in commit d889c036cd on Jan 11, 2018

darwin referenced this in commit c633a020f9 on Apr 28, 2019

D4nte referenced this in commit 7afda0ab29 on Jun 21, 2019

D4nte referenced this in commit c174b3acdc on Jun 23, 2019

D4nte referenced this in commit 6b53e5f609 on Jun 24, 2019

DrahtBot locked this on Sep 8, 2021

Support having SegWit always active in regtest (sipa, ajtowns, jnewbery) #11389