Bury bip9 deployments #16060

jnewbery · 2019-05-20T20:38:20Z

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.

jnewbery · 2019-05-20T20:40:01Z

@jl2012 originally attempted this in #11398, which had concept ACKs from @gmaxwell @dcousens and @sdaftuar . I then tried again in #12360, which got ACKs from @jamesob and @jtimon .

practicalswift · 2019-05-20T20:47:05Z

Concept ACK

sipa · 2019-05-20T21:35:22Z

Concept ACK

DrahtBot · 2019-05-21T01:21:14Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#8994 (Testchains: Introduce custom chain whose constructor... by jtimon)

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.

jnewbery · 2019-05-22T13:28:56Z

Force-pushed to fix some minor issues from rebasing.

Wouldn't it help to split out the first commit, which changes how ISM deployments are reported?

All three commits change the output of getblockchaininfo and how soft forks are reported, so it makes sense to merge them in the same PR.

laanwj · 2019-05-29T10:18:39Z

Concept ACK

src/chainparams.cpp

src/rpc/blockchain.cpp

jnewbery · 2019-06-05T09:50:18Z

Thanks for the review @PastaPastaPasta . I've fixed your two comments.

laanwj · 2019-06-05T11:41:59Z

src/chainparams.cpp

@@ -69,6 +69,8 @@ class CMainParams : public CChainParams {
        consensus.BIP34Hash = uint256S("0x000000000000024b89b42a942fe0d9fea3bb44ab7bd1b19115dd6a759c0808b8");
        consensus.BIP65Height = 388381; // 000000000000000004c2b624ed5d7756c508d90fd0da2c7c679febfa6c4735f0
        consensus.BIP66Height = 363725; // 00000000000000000379eaa19dce8c9b722d46ae6a57c2f1a988119488b50931
+        consensus.CSVHeight = 419328; // 000000000000000004a1b34462cb8aeebd5799177f7a29cf28f2d1961716b5b5
+        consensus.SegwitHeight = 481824; // 0000000000000000001c8018d9cb3b742ef25114f27563e3fc4a1902167f9893


Checked blockhashes against bock chain, and checked the activation heights against getblockchaininfo that they match since

"bip9_softforks": { "csv": { "status": "active", "startTime": 1462060800, "timeout": 1493596800, "since": 419328 }, "segwit": { "status": "active", "startTime": 1479168000, "timeout": 1510704000, "since": 481824 } },

It's slightly inconsistent how all of these (BIP34, BIP65, BIP66) are BIPxx and the new ones are written out as name. I don't personally mind, to be honest, I even think this is clearer than having numbers.

Neither the CSV nor segwit softforks were specified by a single BIP. CSV was BIPs 68, 112, and 113 and segwit was BIPs 141, 143, and 147. We could go back and give BIP34, BIP65 and BIP66 'descriptive' names, but I think that's an unnecessary breaking change.

laanwj · 2019-06-05T12:09:07Z

src/rpc/blockchain.cpp

-            "     }, ...\n"
-            "  ],\n"
-            "  \"bip9_softforks\": {           (object) status of BIP9 softforks in progress\n"
+            "  \"softforks\": {                (object) status of softforks\n"


I like making this a object indexed by name, makes it easier to look one up in languages that treat them as dictionaries.

Agree, but this is a breaking change, seems unnecessary.

I like making this a object indexed by name, makes it easier to look one up in languages that treat them as dictionaries.

For the benefit of other reviewers - that's what this PR does already.

this is a breaking change, seems unnecessary

I doubt that there are many clients programmatically querying this, since the value never changes once it's locked in. After this PR, there don't need to be further breaking changes, since the new softfork object is able to hold both bip9 and buried fork details (or any other potential activation method types).

We are on the same page then, but for the sake of complaints, just add a small release note? @MarcoFalke also asked it above.

merehap · 2019-06-05T17:19:27Z

src/validation.cpp

-    LOCK(cs_main);
-    return (VersionBitsState(pindexPrev, params, Consensus::DEPLOYMENT_SEGWIT, versionbitscache) == ThresholdState::ACTIVE);
+    int height = pindexPrev == nullptr ? 0 : pindexPrev->nHeight + 1;
+    return (height >= params.SegwitHeight);
 }

 bool IsNullDummyEnabled(const CBlockIndex* pindexPrev, const Consensus::Params& params)
 {
    LOCK(cs_main);


Can this LOCK(cs_main) be removed just like the one in IsWitnessEnabled was? If it shouldn't be, the reason would be unintuitive, so could you leave a comment why it is still needed/desirable in that case?

Yes, it can. In fact, this function is only called from within GetBlockScriptFlags(), which holds cs_main. After this PR, this is now a simple one-line check, so I've removed the function and placed the conditional directly into GetBlockScriptFlags().

jtimon · 2019-06-13T10:47:36Z

src/chainparams.cpp

 {
+    if (gArgs.IsArgSet("-segwitheight")) {


It would be nicer to do something more generic that works for other deployments, not just segwit. For example, if anybody was relying on vparams to test csv activation, it won't be possible after this PR.
But perhaps this is out of the scope for this PR,
With something like #8994 we could simply make the all the buried heights configurable for custom regtests.

Perhaps we can call the argument "-con_segwitheight" like in #8994 ?

This argument is just for testing, so it's not disruptive to change this in future if we think the -con_xxx naming scheme is better. For now, I'd like to keep this as it is, rather than optimizing for a PR that hasn't been merged yet.

jtimon · 2019-06-13T10:54:51Z

I didn't check the actual activation heights, but beyond that and my small nit, utACK f395b23

meshcollider · 2019-06-13T19:12:51Z

Concept ACK

moneyball · 2019-06-14T19:04:03Z

I've looked through the history of the 3 PRs trying to get this change merged, and there is widespread support for it, but we need to get it to the finish line! Now that it has been added to the high priority list, would one or two of the following folks be able to review this in the next week or so? @sdaftuar @TheBlueMatt @sipa @jl2012 🙏

instagibbs · 2019-06-18T13:57:52Z

just noting there is a competing proposal at #16229

laanwj · 2019-07-02T13:06:46Z

just noting there is a competing proposal at #16229

Why are there competing proposals? Is there a comparison sometimes of the pros/cons of either approach?

jnewbery · 2019-07-02T14:51:32Z

This needs release notes if it reaches a stage where it's ready for merge.

ajtowns · 2019-07-02T15:08:54Z

Why are there competing proposals? Is there a comparison sometimes of the pros/cons of either approach?

The propsals take different approaches: this one (#16060) changes segwit/csv from bip9 deployments to be hardcoded fixed height deployments; the other one (#16229) generalises bip9 deployments to also support fixed height deployments, and converts strictder and cltv into this generalised deployment method and updates segwit/csv to be fixed height rather than bip9 based.

I think the benefits of hardcoded fixed height stuff is that it's a bit more efficient (the height's hardcoded and every codepath knows the height is hardcoded), while the cost is that anytime you change a bip9 deployment to a fixed height deployment you have to change the code in a few places. Doing it this way also means you can't have tests that want to control how the forks activated, which results in substantive changes to the test suite when burying a deployment.

The cost of the generalised approach is that you can't assume that the deployments are just activated at a particular height, which slows mainnet handling of the deployments down for (at most) a benefit in regtest. I think this slowdown is pretty trivial, but haven't benchmarked it. The benefits (I think) are that it means we can bury BIP9 deployments in future just by changing the declaration in chainparams.cpp, with tests generally continuing to work unchanged, and no code changes needed for getblockchaininfo or in checking when the soft-fork is enabled. This approach can also be done with minimal changes to getblockchaininfo output if we thought that was a benefit.

Hard code CSV deployment height to 419328 for mainnet.

Hardcode segwit deployment height to 481824 for mainnet.

jnewbery · 2019-08-14T19:54:06Z

Thanks for the review @ariard - I've addressed your comments.

ajtowns · 2019-08-15T08:32:08Z

ACK e78aaf4 ; checked diff to previous acked commit, checked tests still work

ariard · 2019-08-15T15:02:26Z

ACK e78aaf4, check diff, run the tests again and successfully activated csv/segwit heights on mainnet as expected.

maflcko · 2019-08-15T20:00:54Z

ACK e78aaf4 (still didn't check if the mainnet block heights are correct, but the code looks good now)

Show signature and timestamp

Signature:

Timestamp of file with hash f62d1be3493fe779f9feb2f59b46db1f25e6e089a7d58db2e083156c727bb44a -

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

Unify our own height-based activation of Segwit and CSV (previously together with BIP16) with the upstream change bitcoin/bitcoin#16060. Also includes some basic fixes to stuff in the diff of bitcoin..auxpow that is not needed (anymore) and has been forgotten to be cleaned up.

60e855f doc: Bump version in bips.md, mention bumping in release process (Wladimir J. van der Laan) 82c1177 doc: Add mention of BIP158 indexing since v0.19.0 (Wladimir J. van der Laan) 2267006 doc: Add mention of BIP125 used by wallet GUI by default since v0.18.1 (Wladimir J. van der Laan) b11514d doc: Add mention of BIP70 disabling by default in bips.md (Wladimir J. van der Laan) Pull request description: - Add mention of BIP70 disabling by default at build time. Any others? E.g. does the burying of deployments of #16060 need to be mentioned? If so, where and how? For all of BIPs 34, 65 and 66? ACKs for top commit: hebasto: ACK 60e855f Tree-SHA512: 76aac3118bb9b56eeea75d046a55d8678a4c5c43004bec98a653f285ef59c34e67af01b0af3ddcefe4e92d37eea89f4f6627e4d056194f54e2e6168c79b4865c

60e855f doc: Bump version in bips.md, mention bumping in release process (Wladimir J. van der Laan) 82c1177 doc: Add mention of BIP158 indexing since v0.19.0 (Wladimir J. van der Laan) 2267006 doc: Add mention of BIP125 used by wallet GUI by default since v0.18.1 (Wladimir J. van der Laan) b11514d doc: Add mention of BIP70 disabling by default in bips.md (Wladimir J. van der Laan) Pull request description: - Add mention of BIP70 disabling by default at build time. Any others? E.g. does the burying of deployments of bitcoin#16060 need to be mentioned? If so, where and how? For all of BIPs 34, 65 and 66? ACKs for top commit: hebasto: ACK 60e855f Tree-SHA512: 76aac3118bb9b56eeea75d046a55d8678a4c5c43004bec98a653f285ef59c34e67af01b0af3ddcefe4e92d37eea89f4f6627e4d056194f54e2e6168c79b4865c

412d5fe QA: feature_segwit: Check that template "rules" includes "!segwit" as appropriate (Luke Dashjr) 2abe8cc Bugfix: Include "csv","!segwit" in "rules" (Luke Dashjr) Pull request description: #16060 removed CSV & segwit from versionbits, breaking the "rules" key returned by GBT. Without this, miners don't know they're mining segwit blocks, and should fall back to pre-segwit block creation. ACKs for top commit: sipa: ACK 412d5fe jnewbery: Tested ACK 412d5fe. Tree-SHA512: 825d72e257dc0dd4941f2fe498d8d4f4f2a21b9505cd21a8f9eb7fb5d6d7dd9219347928cf90bb57a777920ce24295859763e64fa8a22ebb58fc2380f80f5615

412d5fe QA: feature_segwit: Check that template "rules" includes "!segwit" as appropriate (Luke Dashjr) 2abe8cc Bugfix: Include "csv","!segwit" in "rules" (Luke Dashjr) Pull request description: bitcoin#16060 removed CSV & segwit from versionbits, breaking the "rules" key returned by GBT. Without this, miners don't know they're mining segwit blocks, and should fall back to pre-segwit block creation. ACKs for top commit: sipa: ACK 412d5fe jnewbery: Tested ACK 412d5fe. Tree-SHA512: 825d72e257dc0dd4941f2fe498d8d4f4f2a21b9505cd21a8f9eb7fb5d6d7dd9219347928cf90bb57a777920ce24295859763e64fa8a22ebb58fc2380f80f5615

60e855f doc: Bump version in bips.md, mention bumping in release process (Wladimir J. van der Laan) 82c1177 doc: Add mention of BIP158 indexing since v0.19.0 (Wladimir J. van der Laan) 2267006 doc: Add mention of BIP125 used by wallet GUI by default since v0.18.1 (Wladimir J. van der Laan) b11514d doc: Add mention of BIP70 disabling by default in bips.md (Wladimir J. van der Laan) Pull request description: - Add mention of BIP70 disabling by default at build time. Any others? E.g. does the burying of deployments of bitcoin#16060 need to be mentioned? If so, where and how? For all of BIPs 34, 65 and 66? ACKs for top commit: hebasto: ACK 60e855f Tree-SHA512: 76aac3118bb9b56eeea75d046a55d8678a4c5c43004bec98a653f285ef59c34e67af01b0af3ddcefe4e92d37eea89f4f6627e4d056194f54e2e6168c79b4865c

DrahtBot added Consensus Mining RPC/REST/ZMQ Tests Validation labels May 20, 2019

scravy mentioned this pull request May 20, 2019

Do we want to keep Bip9-style deployments? dtr-org/unit-e#472

Open

jnewbery force-pushed the bury_bip9_deployments branch from 8fcba73 to 152af4d Compare May 22, 2019 13:27

PastaPastaPasta suggested changes May 31, 2019

View reviewed changes

src/chainparams.cpp Outdated Show resolved Hide resolved

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

jnewbery force-pushed the bury_bip9_deployments branch from 152af4d to f395b23 Compare June 5, 2019 09:50

laanwj reviewed Jun 5, 2019

View reviewed changes

merehap reviewed Jun 5, 2019

View reviewed changes

jtimon reviewed Jun 13, 2019

View reviewed changes

ajtowns mentioned this pull request Jun 18, 2019

Standardise deployment handling #16229

Closed

jnewbery force-pushed the bury_bip9_deployments branch from f395b23 to d251466 Compare July 2, 2019 14:41

jnewbery added the Needs release note label Jul 2, 2019

jnewbery and others added 4 commits August 14, 2019 15:52

[Consensus] Bury CSV deployment height

1c93b9b

Hard code CSV deployment height to 419328 for mainnet.

[Consensus] Bury segwit deployment

0328dcd

Hardcode segwit deployment height to 481824 for mainnet.

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

8319e73

[docs] Add release notes for burying bip 9 soft fork deployments

e78aaf4

jnewbery force-pushed the bury_bip9_deployments branch from dbe75a0 to e78aaf4 Compare August 14, 2019 19:53

fanquake removed the Needs release note label Aug 15, 2019

maflcko merged commit e78aaf4 into bitcoin:master Aug 15, 2019

maflcko mentioned this pull request Aug 15, 2019

test: Set BIP34Height = 2 for regtest #16333

Merged

jnewbery deleted the bury_bip9_deployments branch August 15, 2019 20:37

achow101 mentioned this pull request Aug 23, 2019

Unknown version bit fork activated warning #16697

Closed

Sjors mentioned this pull request Sep 3, 2019

cannot unmarshal object into Go struct field GetBlockChainInfoResult.softforks lightningnetwork/lnd#3412

Closed

laanwj mentioned this pull request Sep 30, 2019

doc: Update bips.md for 0.19 #16997

Merged

This was referenced Oct 30, 2019

btcjson+rpcclient: add compatibility for bitcoind v0.19.0 btcsuite/btcd#1484

Merged

build: bump btcsuite and neutrino dependencies to latest tagged versions lightningnetwork/lnd#3649

Merged

nopara73 mentioned this pull request Dec 18, 2019

Indicate if trusted bitcoind is connected WalletWasabi/WalletWasabi#2592

Closed

luke-jr mentioned this pull request Jan 17, 2020

Fix GBT: Restore "!segwit" and "csv" to "rules" key #17946

Merged

ariard mentioned this pull request Feb 3, 2021

Remove RewindBlockIndex logic #21009

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bury bip9 deployments #16060

Bury bip9 deployments #16060

jnewbery commented May 20, 2019 •

edited

Loading

jnewbery commented May 20, 2019

practicalswift commented May 20, 2019

sipa commented May 20, 2019

DrahtBot commented May 21, 2019 •

edited

Loading

jnewbery commented May 22, 2019

laanwj commented May 29, 2019

jnewbery commented Jun 5, 2019

laanwj Jun 5, 2019

laanwj Jun 5, 2019

jnewbery Jul 2, 2019

laanwj Jun 5, 2019

promag Jun 30, 2019

jnewbery Jul 2, 2019

promag Jul 2, 2019

merehap Jun 5, 2019

jnewbery Jul 2, 2019

jtimon Jun 13, 2019

jtimon Jun 13, 2019 •

edited

Loading

jnewbery Jul 2, 2019

jtimon commented Jun 13, 2019

meshcollider commented Jun 13, 2019

moneyball commented Jun 14, 2019

instagibbs commented Jun 18, 2019

laanwj commented Jul 2, 2019

jnewbery commented Jul 2, 2019

ajtowns commented Jul 2, 2019

jnewbery commented Aug 14, 2019

ajtowns commented Aug 15, 2019

ariard commented Aug 15, 2019

maflcko commented Aug 15, 2019

Bury bip9 deployments #16060

Bury bip9 deployments #16060

Conversation

jnewbery commented May 20, 2019 • edited Loading

jnewbery commented May 20, 2019

practicalswift commented May 20, 2019

sipa commented May 20, 2019

DrahtBot commented May 21, 2019 • edited Loading

Conflicts

jnewbery commented May 22, 2019

laanwj commented May 29, 2019

jnewbery commented Jun 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtimon Jun 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtimon commented Jun 13, 2019

meshcollider commented Jun 13, 2019

moneyball commented Jun 14, 2019

instagibbs commented Jun 18, 2019

laanwj commented Jul 2, 2019

jnewbery commented Jul 2, 2019

ajtowns commented Jul 2, 2019

jnewbery commented Aug 14, 2019

ajtowns commented Aug 15, 2019

ariard commented Aug 15, 2019

maflcko commented Aug 15, 2019

jnewbery commented May 20, 2019 •

edited

Loading

DrahtBot commented May 21, 2019 •

edited

Loading

jtimon Jun 13, 2019 •

edited

Loading