Bury bip9 deployments #12360

jnewbery · 2018-02-05T17:23:17Z

This hardcodes CSV and segwit activation heights, similar to the BIP 90 buried deployments for BIPs 34, 65 and 66.

Both CSV and segwit have been active for over 8 months. Hardcoding the activation height is a code simplification, makes it easier to understand segwit activation status, and reduces technical debt.

jtimon · 2018-02-05T17:33:51Z

This will need rebase after #11739 , won't it?

jnewbery · 2018-02-05T18:08:00Z

This will need rebase after #11739 , won't it?

There are no dependencies between the two, but you're correct that there are some minor conflicts in the implementations. I'm happy to rebase if 11739 is merged first.

sipa · 2018-02-05T21:59:58Z

src/rpc/blockchain.cpp

-    rv.push_back(Pair("reject", SoftForkMajorityDesc(version, pindex, consensusParams)));
-    return rv;
+    rv.pushKV("type", "buried");
+    rv.pushKV("type", "buried");


Duplicated line?

promag · 2018-02-05T22:04:23Z

src/chainparams.cpp

@@ -59,6 +59,11 @@ void CChainParams::UpdateVersionBitsParameters(Consensus::DeploymentPos d, int64
    consensus.vDeployments[d].nTimeout = nTimeout;
 }

+void CChainParams::UpdateSegwitHeight(const int& height)


Any good reason for reference?

None. Removed.

Empact · 2018-02-08T21:04:39Z

src/consensus/params.h

@@ -58,6 +56,10 @@ struct Params {
    int BIP65Height;
    /** Block height at which BIP66 becomes active */
    int BIP66Height;
+    /** Block height at which CSV becomes active */
+    int CSVHeight;
+    /** Block height at which Segwit becomes active */


Is it helpful to continue referencing the BIP numbers in these comments? I would guess so.

Sure. Comments added.

Why not refer to the BIP numbers solely instead of alternating?

the segwit deployment covered more than one bip

jamesob

Concept ACK 32cbe48

jamesob · 2018-02-12T12:57:00Z

src/rpc/blockchain.cpp

+    rv.pushKV("type", "buried");
+    // getblockchaininfo reports the softfork as active from when the chain height is
+    // one below the activation height
+    rv.push_back(Pair("active", chainActive.Tip()->nHeight + 1 >= height));


Nit: any reason this isn't just rv.pushKV("active", ...); for consistency?

Requires #12193

ah. It's just been merged. Will rebase.

jamesob · 2018-02-12T13:08:58Z

src/rpc/blockchain.cpp

+            "  \"chainwork\": \"xxxx\"     (string) total amount of work in active chain, in hexadecimal\n"
+            "  \"pruned\": xx,             (boolean) if the blocks are subject to pruning\n"
+            "  \"pruneheight\": xxxxxx,    (numeric) lowest-height complete block stored\n"
+            "  \"softforks\": {               (object) status of softforks\n"


We don't seem to have any testing for the contents of this key - is this the case? Happy to either write some for this PR or file a follow-up.

You're right. rpc_blockchain.py should be updated. If you're happy to contribute a commit to add tests, I can include it in this PR.

jamesob · 2018-02-12T13:15:44Z

src/rpc/blockchain.cpp

@@ -1160,36 +1155,28 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
            "  \"difficulty\": xxxxxx,         (numeric) the current difficulty\n"
            "  \"mediantime\": xxxxxx,         (numeric) median time for the current best block\n"
            "  \"verificationprogress\": xxxx, (numeric) estimate of verification progress [0..1]\n"
-            "  \"initialblockdownload\": xxxx, (bool) (debug information) estimate of whether this node is in Initial Block Download mode.\n"


Curious why removal of this key and others (e.g. size_on_disk) is bundled with this changeset. Seems like RPC interface changes should be handled separately.

Edit: looks like these were just removed from the help message; still probably don't want to do that in this changeset, no?

Oops. Bad rebase. Should be fixed now.

jnewbery · 2018-02-13T16:10:47Z

Rebased now that #12193 has been merged.

ACK @jamesob's 0be1f9d8a6bb2b8c47d6816b46d6764aee3cc06e commit.

jamesob · 2018-02-14T18:09:41Z

Biased ACK 0be1f9d

jtimon · 2018-02-14T20:08:03Z

utACK 0be1f9d8a6bb2b8c47d6816b46d6764aee3cc06e

jnewbery · 2018-03-07T18:43:18Z

rebased

jamesob · 2018-03-08T15:36:24Z

Rebased.

jnewbery · 2018-03-19T13:31:58Z

Rebased

jtimon · 2018-03-19T18:08:48Z

src/init.cpp

@@ -1158,6 +1159,21 @@ bool AppInitParameterInteraction()
            }
        }
    }
+


This can be done directly in CRegtestParams's constructor, that way, the function UpdateSegwitHeight would not be needed.

Seems like a good idea, but segwit height is also updated in one of the unit tests:

https://github.com/bitcoin/bitcoin/pull/12360/files#diff-d5ba361c5f8be78eb4cc0c787c1fc78eR118

I think we need the UpdateSegwitHeight() function call there.

I guess we still need UpdateSegwitHeight() then, or perhaps we can pass the other args (instead of g_args, or rather g_args plus "-segwitheight=432") to SelectParams in https://github.com/bitcoin/bitcoin/pull/12360/files#diff-d5ba361c5f8be78eb4cc0c787c1fc78eR54 ?
I guess not all BasicTestingSetup want 432 there...

I think I'll leave it as is for now. It may be possible to tidy this up in a future commit.

jnewbery · 2018-03-27T13:35:24Z

Rebased

jnewbery · 2018-03-29T14:56:43Z

rebased

maflcko · 2018-06-06T17:11:15Z

Could the rpc changes potentially be split up into a separate pull request, to not distract from the consensus changes?

jnewbery · 2018-06-06T17:43:08Z

Could the rpc changes potentially be split up into a separate pull request, to not distract from the consensus changes?

It's possible, but this PR already has two ACKs and a Concept ACK. I think that splitting it up would be generating more work for reviewers.

This combines reporting of buried (formally ISM) softfork deployments and BIP9 versionbits softfork deployments into one JSON object in the getblockchaininfo return object. It also removes the redundant feature_bip9_softforks.py. feature_bip9_sofforks.py was intended to be a generic test for versionbits deployments. However, it only tests CSV activation and was not updated to test segwit activation. CSV activation is tested by feature_csv_activation.py, so this test is duplicated effort.

Hard code CSV deployment height to 419328 for mainnet.

jnewbery · 2018-06-12T16:06:46Z

rebased

maflcko · 2018-06-12T16:50:06Z

It's possible, but this PR already has two ACKs and a Concept ACK. I think that splitting it up would be generating more work for reviewers.

The ACK are outdated and would need to be re-ACKed on the new commits anyway.

Hardcode segwit deployment height to 481824 for mainnet.

jnewbery · 2018-06-12T20:07:28Z

Fixed failing p2p_segwit.py

jnewbery · 2018-06-14T19:20:30Z

This is a revamped version of @jl2012's #11398, which had concept ACKs from @gmaxwell @dcousens and @sdaftuar . This PR also has ACKs from @jamesob and @jtimon .

Are people still interested in this? I haven't had any feedback for the last three rebases (except @MarcoFalke's NACKs), and I'd rather not continue rebasing if people have cooled on this idea.

DrahtBot · 2018-07-07T08:45:11Z

Needs rebase

jnewbery · 2018-07-09T14:42:39Z

Closing - 6 concept ACKs but no review so I guess this isn't worth rebasing.

e78aaf4 [docs] Add release notes for burying bip 9 soft fork deployments (John Newbery) 8319e73 [tests] Add coverage for the content of getblockchaininfo.softforks (James O'Beirne) 0328dcd [Consensus] Bury segwit deployment (John Newbery) 1c93b9b [Consensus] Bury CSV deployment height (John Newbery) 3862e47 [rpc] Tidy up reporting of buried and ongoing softforks (John Newbery) Pull request description: This hardcodes CSV and segwit activation heights, similar to the BIP 90 buried deployments for BIPs 34, 65 and 66. CSV and segwit have been active for over 18 months. Hardcoding the activation height is a code simplification, makes it easier to understand segwit activation status, and reduces technical debt. This was originally attempted by jl2012 in #11398 and again by me in #12360. ACKs for top commit: ajtowns: ACK e78aaf4 ; checked diff to previous acked commit, checked tests still work ariard: ACK e78aaf4, check diff, run the tests again and successfully activated csv/segwit heights on mainnet as expected. MarcoFalke: ACK e78aaf4 (still didn't check if the mainnet block heights are correct, but the code looks good now) Tree-SHA512: 7e951829106e21a81725f7d3e236eddbb59349189740907bb47e33f5dbf95c43753ac1231f47ae7bee85c8c81b2146afcdfdc11deb1503947f23093a9c399912

jnewbery mentioned this pull request Feb 5, 2018

Hardcode CSV and SEGWIT deployment #11398

Closed

sipa reviewed Feb 5, 2018

View reviewed changes

promag reviewed Feb 5, 2018

View reviewed changes

jnewbery force-pushed the bury_bip9_deployments branch from b58d560 to caab009 Compare February 5, 2018 22:27

laanwj added Validation Consensus labels Feb 6, 2018

Empact reviewed Feb 8, 2018

View reviewed changes

jnewbery force-pushed the bury_bip9_deployments branch from caab009 to 32cbe48 Compare February 8, 2018 21:27

jamesob reviewed Feb 12, 2018

View reviewed changes

jnewbery force-pushed the bury_bip9_deployments branch 2 times, most recently from 07f8153 to 0be1f9d Compare February 13, 2018 16:08

jamesob force-pushed the bury_bip9_deployments branch from 0be1f9d to 916aa73 Compare February 13, 2018 20:17

jnewbery force-pushed the bury_bip9_deployments branch from 916aa73 to 0be1f9d Compare February 13, 2018 20:22

jnewbery force-pushed the bury_bip9_deployments branch from 0be1f9d to 818b7af Compare March 7, 2018 18:39

jamesob force-pushed the bury_bip9_deployments branch from 818b7af to 3abc65f Compare March 8, 2018 15:35

jnewbery force-pushed the bury_bip9_deployments branch from 3abc65f to fc32e68 Compare March 19, 2018 13:31

jtimon suggested changes Mar 19, 2018

View reviewed changes

jnewbery force-pushed the bury_bip9_deployments branch from fc32e68 to 49c5e7d Compare March 27, 2018 13:35

jnewbery force-pushed the bury_bip9_deployments branch from 49c5e7d to 2507a45 Compare March 29, 2018 14:56

jnewbery mentioned this pull request Apr 13, 2018

Enforce SCRIPT_VERIFY_P2SH and SCRIPT_VERIFY_WITNESS from genesis #11739

Merged

DrahtBot mentioned this pull request Jun 7, 2018

[net,mempool] Call AcceptToMemoryPool() asynchronously in p2p #13413

Closed

jl2012 added 2 commits June 12, 2018 11:42

[Consensus] Bury CSV deployment height

c2029c2

Hard code CSV deployment height to 419328 for mainnet.

jnewbery force-pushed the bury_bip9_deployments branch from bef29cf to e25d4de Compare June 12, 2018 16:06

jl2012 and others added 2 commits June 12, 2018 16:03

[Consensus] Bury segwit deployment

ff98898

Hardcode segwit deployment height to 481824 for mainnet.

[tests] Add coverage for the content of getblockchaininfo.softforks

ab740a0

jnewbery force-pushed the bury_bip9_deployments branch from e25d4de to ab740a0 Compare June 12, 2018 20:07

DrahtBot mentioned this pull request Jun 26, 2018

policy: Remove promiscuousmempoolflags #13527

Merged

DrahtBot added the Needs rebase label Jul 7, 2018

jnewbery closed this Jul 9, 2018

jnewbery mentioned this pull request May 20, 2019

Bury bip9 deployments #16060

Merged

laanwj removed the Needs rebase label Oct 24, 2019

maflcko mentioned this pull request Nov 13, 2021

Bury taproot deployment #23505

Closed

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

@@ @@ -1158,6 +1159,21 @@ bool AppInitParameterInteraction() @@
                           }
                       }
                   }

Bury bip9 deployments #12360

Bury bip9 deployments #12360

Uh oh!

Conversation

jnewbery commented Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtimon commented Feb 5, 2018

Uh oh!

jnewbery commented Feb 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Feb 13, 2018

Uh oh!

jamesob commented Feb 14, 2018

Uh oh!

jtimon commented Feb 14, 2018

Uh oh!

jnewbery commented Mar 7, 2018

Uh oh!

jamesob commented Mar 8, 2018

Uh oh!

jnewbery commented Mar 19, 2018

Uh oh!

jtimon Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Mar 27, 2018

Uh oh!

jnewbery commented Mar 29, 2018

Uh oh!

maflcko commented Jun 6, 2018

Uh oh!

jnewbery commented Jun 6, 2018

Uh oh!

jnewbery commented Jun 12, 2018

Uh oh!

jnewbery commented Feb 5, 2018 •

edited

Loading

jamesob Feb 12, 2018 •

edited

Loading

jtimon Mar 19, 2018 •

edited

Loading

jnewbery commented Jun 12, 2018 •

edited

Loading