Hardcode CSV and SEGWIT deployment

jl2012 commented at 10:06 am on September 25, 2017: contributor

Similar to BIP90, this hardcode the deployment of CSV softfork, which has been active for over a year.

laanwj added the label Consensus on Sep 25, 2017

jl2012 force-pushed on Sep 25, 2017

gmaxwell approved

gmaxwell commented at 4:51 pm on September 25, 2017: contributor

Concept ACK

dcousens approved

dcousens commented at 0:31 am on September 26, 2017: contributor

concept ACK

in src/rpc/blockchain.cpp:1141 in 947d28a3aa outdated

1135-            "        \"version\": xx,         (numeric) block version\n"
1136-            "        \"reject\": {            (object) progress toward rejecting pre-softfork blocks\n"
1137+            "        \"height\": \"xxxxxx\",  (numeric) height of the first block which the rules are enforced\n"
1138+            "        \"type\": \"xxxx\",      (string) original activation mechanism (\"ism\" or \"bip9\")\n"
1139+            "        \"version\": xx,         (numeric) block version (only for \"ism\" type)\n"
1140+            "        \"reject\": {            (object) progress toward rejecting pre-softfork blocks (only for \"ism\" type)\n"

jnewbery commented at 3:10 pm on September 26, 2017:

why not combine ['reject']['status'] with ['active'] below? Once these buried softforks are pinned, an active ISM softfork is equivalent to an active BIP9 softfork.

jl2012 commented at 5:25 pm on September 26, 2017:

No special reason. I just want to keep existing response untouched

jnewbery commented at 7:32 pm on September 27, 2017:

I honestly think ['reject']['status'] makes almost no sense in a post-ISM world. This PR is already a breaking API change (getblockchaininfo RPC no longer returns bip9_softforks['csv']) so why not use the opportunity to rationalize the way the information is presented to the user?

jl2012 commented at 7:35 am on September 28, 2017:

ok. Then I prefer to just unify softforks and bit_softforks, so we don’t need to break the API everytime we bury a softfork

in test/functional/bip68-sequence.py:376 in 947d28a3aa outdated

374+        min_activation_height = 576
375         height = self.nodes[0].getblockcount()
376         assert(height < min_activation_height)
377         self.nodes[0].generate(min_activation_height-height)
378-        assert(get_bip9_status(self.nodes[0], 'csv')['status'] == 'active')
379+        assert(get_buried_softforks_status(self.nodes[0], 'csv'))

jnewbery commented at 3:11 pm on September 26, 2017:

assert is a statement, not a function. This should be:

assert get_buried_softforks_status(self.nodes[0], 'csv')

in test/functional/test_framework/util.py:311 in 947d28a3aa outdated

307@@ -308,6 +308,15 @@ def get_bip9_status(node, key):
308     info = node.getblockchaininfo()
309     return info['bip9_softforks'][key]
310 
311+def get_buried_softforks_status(node, key):

jnewbery commented at 3:12 pm on September 26, 2017:

nit: prefer a name like buried_softfork_active(), since this is returning a bool (True if the softfork is active)

in test/functional/test_framework/util.py:318 in 947d28a3aa outdated

313+        if fork['id'] == key:
314+            if fork['type'] == "ism":
315+                return fork['reject']['status']
316+            else:
317+                return fork['active']
318+    assert(not "unknown softfork")

jnewbery commented at 3:14 pm on September 26, 2017:

I don’t understand what this is supposed to be doing. This will always assert if we drop down to this line (a string has truthiness True, so not "string" is always False)

jl2012 commented at 5:31 pm on September 26, 2017:

It would drop down to this line only if key is not found in the results of getblockchaininfo(). I want it to exit with the error “unknown softfork” printed

jnewbery commented at 6:26 pm on September 26, 2017:

ok, in that case you can be much more compact:

0def get_buried_softforks_status(node, key):
1    softforks = [sf for sf in node.getblockchaininfo()['softforks'] if sf['id'] == key]
2    assert softforks, "get_buried_softforks_status() called with invalid softfork %s" % key
3    return softforks[0]['active']

(assuming you make the change from ['reject']['status'] with ['active'] suggested above)

jnewbery commented at 3:31 pm on September 26, 2017: member

Concept ACK. I’ve lightly reviewed the code and left a few comments inline.

Some high-level feedback:

I think this needs a BIP or announcement (as per #8391 (comment))
Why set CSV height to 576 in regtest, instead of 432 where it currently activates?
I haven’t tested all the extended tests, but bip9-softforks.py certainly fails. I think we may be able to just remove that test entirely at this point.

jl2012 commented at 5:34 pm on September 26, 2017: contributor

@jnewbery

Yes it needs a BIP. Not sure if it should be added to BIP90, or have a new BIP
It’s because bip68-112-113-p2p.py activates it at 576

in test/functional/bip68-112-113-p2p.py:13 in 947d28a3aa outdated

12@@ -13,11 +13,8 @@
13 activation after a further 144 blocks

jnewbery commented at 6:40 pm on September 26, 2017:

Can remove these two lines

jnewbery commented at 7:50 pm on September 27, 2017:

not addressed in 48e683d18d4d3d995e7192048d4182c4c0eac0be

in src/chainparams.cpp:266 in 947d28a3aa outdated

262@@ -271,6 +263,7 @@ class CRegTestParams : public CChainParams {
263         consensus.BIP34Hash = uint256();
264         consensus.BIP65Height = 1351; // BIP65 activated on regtest (Used in rpc activation tests)
265         consensus.BIP66Height = 1251; // BIP66 activated on regtest (Used in rpc activation tests)
266+        consensus.CSVHeight = 576; // CSV activated on regtest (Used in rpc activation tests)

jnewbery commented at 6:45 pm on September 26, 2017:

I suggest you change this to 432 to avoid having to change bip68-sequence.py

in test/functional/bip68-112-113-p2p.py:210 in 947d28a3aa outdated

205@@ -209,35 +206,10 @@ def get_tests(self):
206         self.tip = int("0x" + self.nodes[0].getbestblockhash(), 0)
207         self.nodeaddress = self.nodes[0].getnewaddress()
208 
209-        assert_equal(get_bip9_status(self.nodes[0], 'csv')['status'], 'defined')
210-        test_blocks = self.generate_blocks(61, 4)
211+        # Activation height is hardcoded and the softfork will active with any block version
212+        test_blocks = self.generate_blocks(489, 4)

jnewbery commented at 6:45 pm on September 26, 2017:

Suggestion: Change this to 345

in test/functional/bip68-112-113-p2p.py:252 in 947d28a3aa outdated

246@@ -275,9 +247,9 @@ def get_tests(self):
247 
248         # 2 more version 4 blocks
249         test_blocks = self.generate_blocks(2, 4)
250-        yield TestInstance(test_blocks, sync_every_block=False) # 5
251+        yield TestInstance(test_blocks, sync_every_block=False) # 2
252         # Not yet advanced to ACTIVE, height = 574 (will activate for block 576, not 575)
253-        assert_equal(get_bip9_status(self.nodes[0], 'csv')['status'], 'locked_in')
254+        assert(not get_buried_softforks_status(self.nodes[0], 'csv'))

jnewbery commented at 6:45 pm on September 26, 2017:

assert is a statement, not a function

gmaxwell commented at 0:56 am on September 27, 2017: contributor

If this is getting a BIP (I agree thats reasonable) it should perhaps be bundled with the ones for BIP143/BIP147.

jl2012 force-pushed on Sep 27, 2017

jl2012 commented at 6:16 pm on September 27, 2017: contributor

As suggested I made another commit to hardcode SEGWIT deployment. To make it compatible with #11403, segwit is always active on regtest, unless overridden

jl2012 force-pushed on Sep 27, 2017

jl2012 renamed this:
~~Hardcode CSV deployment~~
Hardcode CSV and SEGWIT deployment
on Sep 27, 2017

in src/rpc/blockchain.cpp:1185 in 48e683d18d outdated

1185-    BIP9SoftForkDescPushBack(bip9_softforks, "csv", consensusParams, Consensus::DEPLOYMENT_CSV);
1186-    BIP9SoftForkDescPushBack(bip9_softforks, "segwit", consensusParams, Consensus::DEPLOYMENT_SEGWIT);
1187+    softforks.push_back(BuriedSoftForkDesc("bip34", 2, tip, consensusParams.BIP34Height));
1188+    softforks.push_back(BuriedSoftForkDesc("bip66", 3, tip, consensusParams.BIP66Height));
1189+    softforks.push_back(BuriedSoftForkDesc("bip65", 4, tip, consensusParams.BIP65Height));
1190+    softforks.push_back(BuriedSoftForkDesc("csv", 9, tip, consensusParams.CSVHeight));

jnewbery commented at 7:45 pm on September 27, 2017:

9 seems like an abuse of the version field here

in src/rpc/blockchain.cpp:1052 in 48e683d18d outdated

1069-    return rv;
1070-}
1071-
1072-static UniValue SoftForkDesc(const std::string &name, int version, CBlockIndex* pindex, const Consensus::Params& consensusParams)
1073+/** For buried softforks */
1074+static UniValue BuriedSoftForkDesc(const std::string &name, int version, CBlockIndex* pindex, const int &height)

jnewbery commented at 7:47 pm on September 27, 2017:

just pass height by value

in test/functional/bip68-112-113-p2p.py:17 in 48e683d18d outdated

20-mine 3 blocks and verify still at LOCKED_IN and test that enforcement has not triggered
21+mine 345 blocks and seed block chain with the 82 inputs will use for our tests at height 428
22+mine 3 blocks and verify still not ACTIVE and test that enforcement has not triggered
23 mine 1 block and test that enforcement has triggered (which triggers ACTIVE)
24 Test BIP 113 is enforced
25 Mine 4 blocks so next height is 580 and test BIP 68 is enforced for time and height

jnewbery commented at 7:52 pm on September 27, 2017:

This (and other comments in this test) need to be changed to reflect CSV activation at height 432

in src/init.cpp:445 in 48e683d18d outdated

441@@ -442,6 +442,7 @@ std::string HelpMessage(HelpMessageMode mode)
442         strUsage += HelpMessageOpt("-limitdescendantcount=<n>", strprintf("Do not accept transactions if any ancestor would have <n> or more in-mempool descendants (default: %u)", DEFAULT_DESCENDANT_LIMIT));
443         strUsage += HelpMessageOpt("-limitdescendantsize=<n>", strprintf("Do not accept transactions if any ancestor would have more than <n> kilobytes of in-mempool descendants (default: %u).", DEFAULT_DESCENDANT_SIZE_LIMIT));
444         strUsage += HelpMessageOpt("-vbparams=deployment:start:end", "Use given start/end times for specified version bits deployment (regtest-only)");
445+        strUsage += HelpMessageOpt("-segwitheight=<n>", "Set the activation height of segwit. -1 to disable. (regtest-only)");

jnewbery commented at 7:58 pm on September 27, 2017:

sort alphabetically?

in src/init.cpp:1153 in 48e683d18d outdated

1144@@ -1144,6 +1145,21 @@ bool AppInitParameterInteraction()
1145             }
1146         }
1147     }
1148+
1149+    if (gArgs.IsArgSet("-segwitheight")) {
1150+        if (!chainparams.MineBlocksOnDemand()) {
1151+            return InitError("Segwit activation parameters may only be overridden on regtest.");
1152+        }
1153+        int nHeight = gArgs.GetArg("-segwitheight", chainparams.GetConsensus().SEGWITHeight);

jnewbery commented at 7:59 pm on September 27, 2017:

nit: no hungarian notation please. segwit_height or height should do.

in src/validation.cpp:2847 in 48e683d18d outdated

2843@@ -2844,8 +2844,8 @@ bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::P
2844 
2845 bool IsWitnessEnabled(const CBlockIndex* pindexPrev, const Consensus::Params& params)
2846 {
2847-    LOCK(cs_main);
2848-    return (VersionBitsState(pindexPrev, params, Consensus::DEPLOYMENT_SEGWIT, versionbitscache) == THRESHOLD_ACTIVE);
2849+    int nHeight = pindexPrev == nullptr ? 0 : pindexPrev->nHeight + 1;

jnewbery commented at 8:06 pm on September 27, 2017:

nit: no hungarian notation.

You could just do away with the local variable entirely:

0    return ((int)chainActive.Height() >= params.SEGWITHeight);

jl2012 commented at 7:15 pm on September 29, 2017:

Many different places use IsWitnessEnabled but not all look at the tip

in src/validation.cpp:2883 in 48e683d18d outdated

2879@@ -2880,7 +2880,7 @@ std::vector<unsigned char> GenerateCoinbaseCommitment(CBlock& block, const CBloc
2880     std::vector<unsigned char> commitment;
2881     int commitpos = GetWitnessCommitmentIndex(block);
2882     std::vector<unsigned char> ret(32, 0x00);
2883-    if (consensusParams.vDeployments[Consensus::DEPLOYMENT_SEGWIT].nTimeout != 0) {
2884+    if (gArgs.GetArg("-segwitheight", consensusParams.SEGWITHeight) >= 0) {

jnewbery commented at 8:08 pm on September 27, 2017:

why do you need to check the -segwitheight argument here? Hasn’t consensusParams.SEGWITHeight been updated by the time this function is called?

jl2012 commented at 4:03 am on September 28, 2017:

I try to keep the current behavior that the witness commitment is not made if segwit is disabled (before: nTimeout == 0 / after: SEGWITHeight < 0). If you use -segwitheight=-1, it should work identical to a non-segwit node. This is needed for some tests in p2p-segwit.py related to the witness commitment. (we could remove those tests, of course)

jl2012 commented at 4:05 am on September 28, 2017:

ok. Maybe I should use consensusParams.SEGWITHeight != std::numeric_limits<int>::max() instead

jnewbery commented at 5:57 pm on September 28, 2017:

sorry, I misunderstood. I think your existing change is fine (and I agree that we should remove this at a later date once the tests are updated)

jnewbery commented at 8:10 pm on September 27, 2017: member

Concept ACK pinning the segwit activation height. I know that @sdaftuar was working on something similar, so you should link up with him.

A few more comments inline.

jtimon commented at 12:27 pm on September 30, 2017: contributor

Concept ACK, I was about to write this same thing on top of #11426 Would you consider rebasing on top of it? I think it would make the code slightly nicer (you can just move DEPLOYMENT_CSV and DEPLOYMENT_SEGWIT from DeploymentPos to BuriedDeploymentPos instead of having to declare consensusParams.CSVHeight and consensusParams.SEGWITHeight ).

jl2012 commented at 12:49 pm on September 30, 2017: contributor

@jtimon will do

BIP90: Make buried deployments slightly more easily extensible 4a878203d8

Hardcode CSV deployment 9ff60be3a4

Hardcode SEGWIT deployment ff357fda6c

jl2012 force-pushed on Sep 30, 2017

jl2012 commented at 5:38 pm on September 30, 2017: contributor

rebased on #11426

jtimon commented at 6:09 pm on September 30, 2017: contributor

Review by stages, so far up to 9ff60be3a45c9a02ac65e126976de9c54b188495

EDIT: perhaps it would be nice to do the rpc preparations before chaging anything from bip9 to bip90.

Although I know some people don’t like dividing things so much, I wouldn’t complain about this rebasing on top of a PR with all the common preparations and the move only for csv (than whatever extra test preparations for segwit and the move for segit in this PR).

[rpc] Combine reporting of buried and ongoing softforks ae3ed97df1

[test] Test for buried CSV and SEGWIT deployments 92c5eccf01

jl2012 force-pushed on Oct 1, 2017

jnewbery commented at 8:53 pm on October 3, 2017: member

I don’t think this should be dependant on #11426. I’m indifferent to whether #11426 is merged, but there’s at least some disagreement as to whether it’s beneficial.

This PR had already received some code review prior to the rebase, so I think it makes sense to leave it based on master.

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

Concept ACK. As @jnewbery pointed out in #11389#pullrequestreview-67450833, the script interpreter doesn’t allow SCRIPT_VERIFY_WITNESS if P2SH is not also set. I think we can just pin P2SH to also always be on in regtest to ensure this never happens?

sipa commented at 8:34 pm on October 10, 2017: member

Perhaps this is beyond the scope of this PR, but I would prefer it if a buried deployment wouldn’t require all code paths that check the BIP9 status to require changing (and instead plug in the burying into the BIP9 status logic, so that it just always returns DEFINED before the bury height, and always ACTIVE after?). Advantages:

Easier to review the change that buries a deployment.
Support making a deployment buried in regtest while it isn’t yet in mainnet/testnet (which can simplify functional testing)

MarcoFalke commented at 6:28 pm on November 10, 2017: member

Needs rebase

in src/rpc/blockchain.cpp:1074 in 92c5eccf01

1097+    // Deployments (e.g. testdummy) with timeout value before Jan 1, 2009 are hidden.
1098+    // A timeout value of 0 guarantees a softfork will never be activated.
1099+    // This is used when softfork codes are merged without specifying the deployment schedule.
1100+    if (consensusParams.vDeployments[id].nTimeout <= 1230768000)
1101+        return;
1102+    UniValue bip9(UniValue::VOBJ);

jtimon commented at 7:53 pm on November 11, 2017:

bikeshed: s/bip9/versionbits/ ?

jnewbery commented at 7:05 pm on January 26, 2018: member

Needs rebase. @jl2012 - is this abandoned? I think it’d be great to get it merged around the same time as #11739 to formalize segwit activation rules and clear up the testing (although there are no dependencies between the two PRs).

I still think you should remove the dependency on #11426.

jnewbery commented at 5:23 pm on February 5, 2018: member

It appears that this PR is abandoned. I’ve opened #12360 as a replacement.

fanquake commented at 10:24 pm on February 5, 2018: member

Closing in favour of #12360

fanquake closed this on Feb 5, 2018

laanwj referenced this in commit 3843780fd8 on Feb 8, 2018

MarcoFalke referenced this in commit 1bf2ff2bf8 on Aug 15, 2019

PastaPastaPasta referenced this in commit 168e93162f on Jun 9, 2020

PastaPastaPasta referenced this in commit 7e04a9a3b1 on Jun 9, 2020

PastaPastaPasta referenced this in commit 8254d30d21 on Jun 10, 2020

PastaPastaPasta referenced this in commit 55cc7d8f89 on Jun 10, 2020

PastaPastaPasta referenced this in commit da0042972b on Jun 11, 2020

PastaPastaPasta referenced this in commit 1b7e6b45af on Jun 11, 2020

MarcoFalke referenced this in commit ddc6979b8b on Jul 1, 2021

DrahtBot locked this on Sep 8, 2021

Hardcode CSV and SEGWIT deployment #11398