Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bury bip9 deployments #16060

Merged
merged 5 commits into from Aug 15, 2019
Merged

Bury bip9 deployments #16060

merged 5 commits into from Aug 15, 2019

Conversation

@jnewbery
Copy link
Member

jnewbery commented May 20, 2019

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

This comment has been minimized.

Copy link
Member Author

jnewbery commented May 20, 2019

@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

This comment has been minimized.

Copy link
Member

practicalswift commented May 20, 2019

Concept ACK

@sipa

This comment has been minimized.

Copy link
Member

sipa commented May 20, 2019

Concept ACK

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented May 21, 2019

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 jnewbery force-pushed the jnewbery:bury_bip9_deployments branch from 8fcba73 to 152af4d May 22, 2019
@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented May 22, 2019

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

This comment has been minimized.

Copy link
Member

laanwj commented May 29, 2019

Concept ACK

src/chainparams.cpp Outdated Show resolved Hide resolved
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
@jnewbery jnewbery force-pushed the jnewbery:bury_bip9_deployments branch from 152af4d to f395b23 Jun 5, 2019
@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Jun 5, 2019

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

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

This comment has been minimized.

Copy link
@laanwj

laanwj Jun 5, 2019

Member

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
    }
  },

This comment has been minimized.

Copy link
@laanwj

laanwj Jun 5, 2019

Member

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.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jul 2, 2019

Author Member

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.

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

This comment has been minimized.

Copy link
@laanwj

laanwj Jun 5, 2019

Member

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

This comment has been minimized.

Copy link
@promag

promag Jun 30, 2019

Member

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jul 2, 2019

Author Member

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).

This comment has been minimized.

Copy link
@promag

promag Jul 2, 2019

Member

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

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);

This comment has been minimized.

Copy link
@merehap

merehap Jun 5, 2019

Contributor

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?

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jul 2, 2019

Author Member

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().

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

This comment has been minimized.

Copy link
@jtimon

jtimon Jun 13, 2019

Member

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.

This comment has been minimized.

Copy link
@jtimon

jtimon Jun 13, 2019

Member

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jul 2, 2019

Author Member

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

This comment has been minimized.

Copy link
Member

jtimon commented Jun 13, 2019

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

@laanwj laanwj added this to Blockers in High-priority for review Jun 13, 2019
@meshcollider

This comment has been minimized.

Copy link
Member

meshcollider commented Jun 13, 2019

Concept ACK

@moneyball

This comment has been minimized.

Copy link
Contributor

moneyball commented Jun 14, 2019

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

This comment has been minimized.

Copy link
Member

instagibbs commented Jun 18, 2019

just noting there is a competing proposal at #16229

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jul 2, 2019

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 jnewbery force-pushed the jnewbery:bury_bip9_deployments branch from f395b23 to d251466 Jul 2, 2019
@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Jul 2, 2019

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

This combines reporting of buried (formally ISM) softfork deployments
and BIP9 versionbits softfork deployments into one JSON object in the
getblockchaininfo return object.
@jnewbery jnewbery force-pushed the jnewbery:bury_bip9_deployments branch from 20e6000 to dbe75a0 Aug 13, 2019
@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Aug 13, 2019

Thanks @ajtowns . I've force-pushed a fix to those nits and also release notes.

I also intend to send a post to the mailing list if this gets merged (I don't think a full BIP is necessary).

I tried drafting up a bip9 functional test making use of testdummy on regtest, but I'm not convinced it makes sense to do that directly rather than improving our bip9 implementation or improving bip9 first. Either way, that can be left to a later PR.

Very happy to review any tests or changes to BIP 9 you PR.

@ajtowns

This comment has been minimized.

Copy link
Contributor

ajtowns commented Aug 14, 2019

(I don't think a full BIP is necessary)

I agree; IMO that the heights are already included in https://github.com/bitcoin/bips/blob/master/bip-0009/assignments.mediawiki means the BIP side of things is already covered.

@ariard
ariard approved these changes Aug 14, 2019
Copy link
Contributor

ariard left a comment

ACK dbe75a0, reviewed code, tests ok. I've had a look on python tests, comments have been addressed since last time and generally they look good.

@@ -1644,8 +1644,8 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consens
flags |= SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY;
}

// Start enforcing BIP68 (sequence locks) and BIP112 (CHECKSEQUENCEVERIFY) using versionbits logic.
if (VersionBitsState(pindex->pprev, consensusparams, Consensus::DEPLOYMENT_CSV, versionbitscache) == ThresholdState::ACTIVE) {
// Start enforcing BIP68 (sequence locks) and BIP112 (CHECKSEQUENCEVERIFY)

This comment has been minimized.

Copy link
@ariard

ariard Aug 14, 2019

Contributor

nit: seems to me this check is just about enforcing BIP112, BIP 68 reference should be there ? Same with LOCKTIME_VERIFY_SEQUENCE beneath (even if these 2 BIPs are strongly tied, their enforcement is done by different components)

This comment has been minimized.

Copy link
@jnewbery

jnewbery Aug 14, 2019

Author Member

good spot. Fixed!

@@ -340,10 +304,10 @@ def run_test(self):
self.send_blocks([self.create_test_block(success_txs)])
self.nodes[0].invalidateblock(self.nodes[0].getbestblockhash())

# 1 more version 4 block to get us to height 575 so the fork should now be active for the next block
test_blocks = self.generate_blocks(1, 4)
# 1 more version 4 block to get us to height 432 so the fork should now be active for the next block

This comment has been minimized.

Copy link
@ariard

ariard Aug 14, 2019

Contributor

nit : maybe you could add an assert not active before activation, just to be sure everything is clean after the send_blocks/invalidateblock sequence

This comment has been minimized.

Copy link
@jnewbery

jnewbery Aug 14, 2019

Author Member

done

jamesob and others added 4 commits Feb 13, 2018
@jnewbery jnewbery force-pushed the jnewbery:bury_bip9_deployments branch from dbe75a0 to e78aaf4 Aug 14, 2019
@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Aug 14, 2019

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

@ajtowns

This comment has been minimized.

Copy link
Contributor

ajtowns commented Aug 15, 2019

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

@ariard

This comment has been minimized.

Copy link
Contributor

ariard commented Aug 15, 2019

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Aug 15, 2019

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

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK e78aaf41f43d0e2ad78fa6d8dad61032c8ef73d0 (still didn't check if the mainnet block heights are correct, but the code looks good now)
-----BEGIN PGP SIGNATURE-----

iQGyBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhaqwv42/MfP5paFChypsDXr5M3MZtfZyD36YtC9JCjdtdBff2zKLKONh1PvrYj
d7MuAJ6fgsaS7qE4GIbCo77EDzJziqTsgoruoWckqzLrHgr2Bt4gaxEpciQhUrVD
sR4GjnWuVPkawnIPkr2+hJkScU2/fFGVnVzF1Hg0A7mIzkr9xdt1ofJvb9GQ4O2Y
fteE78HpUAgwvmoudddIsRDKGui/0tTClzhx8D+1cVz7MtSU7+2bmpcvdhn++vHl
q6apUyGTzOFZtC4cJeV/1cv7Jka3M59IC/U05zZU6CD/ocDM3xrXY3z+tZCJEUIJ
MdwWcqBuaK01ytvEr9ZmQ4cutLZFIkVPkz7FgayVU7XZ1X9666TGA1vnY/k1oh7e
vl5+QZl+2bgleQRLot1vGRM5WXzHyksQUGT/XFTmumYmBY36zu4+pt4cUtLabnhW
G3S2joUyMM9JonKCgYGcyPx3XUvLan5E5pZp092RXGoRB2f2T3ZwwDqwhIfmDFhj
1HTWBpo=
=HDUJ
-----END PGP SIGNATURE-----

Timestamp of file with hash f62d1be3493fe779f9feb2f59b46db1f25e6e089a7d58db2e083156c727bb44a -

@MarcoFalke MarcoFalke merged commit e78aaf4 into bitcoin:master Aug 15, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
MarcoFalke added a commit that referenced this pull request Aug 15, 2019
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 jnewbery deleted the jnewbery:bury_bip9_deployments branch Aug 15, 2019
@fanquake fanquake removed this from Blockers in High-priority for review Aug 15, 2019
domob1812 added a commit to domob1812/namecore that referenced this pull request Aug 19, 2019
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.
laanwj added a commit that referenced this pull request Oct 1, 2019
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
sidhujag added a commit to syscoin/syscoin that referenced this pull request Oct 2, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.