Standardise deployment handling by ajtowns · Pull Request #16229 · bitcoin/bitcoin

ajtowns · 2019-06-18T07:48:23Z

This is a different approach to burying csv/segwit deployments -- rather than converting them from Consensus::BIP9Deployment to dedicated members, it uses a generalised Consensus::Deployment type to cover both BIP9 activations and fixed height activations.

I think this should make future burials much simpler (it becomes a one-line change), but there's two other benefits: it doesn't change RPC or tests much, and I think it will make it easier to make future changes to activation methods should we wish to do so. The downside is that the activations aren't hardcoded anymore, so there's a function call and a few indirect lookups to determine a fixed-height activation is enabled, rather than just a direct test.

I've added some extra commits to tidy up the getblockchaininfo output. It now changes from:
The output of getblockchaininfo changes from:

"softforks": [
  {
    "id": "bip34",
    "version": 2,
    "reject": {
      "status": true
    }
  },
  {
    "id": "bip66",
    "version": 3,
    "reject": {
      "status": true
    }
  },
  {
    "id": "bip65",
    "version": 4,
    "reject": {
      "status": true
    }
  }
],
"bip9_softforks": {
  "csv": {
    "status": "active",
    "startTime": 1462060800,
    "timeout": 1493596800,
    "since": 419328
  },
  "segwit": {
    "status": "active",
    "startTime": 1479168000,
    "timeout": 1510704000,
    "since": 481824
  }
},

to

"softforks": {
  "bip34": {
    "status": "active",
    "fixedHeight": 227931,
    "since": 227931
  },
  "strictder": {
    "status": "active",
    "fixedHeight": 363725,
    "since": 363725
  },
  "cltv": {
    "status": "active",
    "fixedHeight": 388381,
    "since": 388381
  },
  "csv": {
    "status": "active",
    "fixedHeight": 419328,
    "since": 419328
  },
  "segwit": {
    "status": "active",
    "fixedHeight": 481824,
    "since": 481824
  }
},

with this patch set, or, on regtest something like:

"softforks": {
  "bip34": {
    "status": "active",
    "fixedHeight": 500,
    "since": 500
  },
  "testdummy": {
    "status": "started",
    "bit": 28,
    "startTime": 0,
    "timeout": 9223372036854775807,
    "since": 144,
    "statistics": {
      "period": 144,
      "threshold": 108,
      "elapsed": 141,
      "count": 141,
      "possible": true
    }
  },
  "strictder": {
    "status": "defined",
    "fixedHeight": 1251,
    "since": 0
  },
  "cltv": {
    "status": "defined",
    "fixedHeight": 1351,
    "since": 0
  },
  "csv": {
    "status": "started",
    "bit": 0,
    "startTime": 0,
    "timeout": 9223372036854775807,
    "since": 144,
    "statistics": {
      "period": 144,
      "threshold": 108,
      "elapsed": 141,
      "count": 141,
      "possible": true
    }
  },
  "segwit": {
    "status": "active",
    "alwaysActive": true,
    "since": 0
  }
},

Work towards using a single structure to describe deployments rather than separate ones for each deployment method. -BEGIN VERIFY SCRIPT- find src/ -name "*.cpp" -o -name "*.h" | xargs perl -i -pe 's/BIP9Deployment/Deployment/g' -END VERIFY SCRIPT-

Helper function that encapsulates the logic to check if a version bits deployment is active.

…c checks

(This does not include pre-CSV soft-forks however, due to special casing in the loop that calls BIP9SoftForkDescPushBack)

ajtowns · 2019-06-18T07:48:55Z

This is an alternative approach to #16060

DrahtBot · 2019-06-18T09:58:35Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16060 (Bury bip9 deployments by jnewbery)
#13972 (Remove 16 bits from versionbits signalling system (BIP320) by btcdrak)

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.

maflcko · 2019-06-18T14:27:37Z

I did like the RPC changes in the other pull. It seems odd to return startTime, where it may refer to either time or height.

ajtowns · 2019-06-19T06:00:54Z

I did like the RPC changes in the other pull. It seems odd to return startTime, where it may refer to either time or height.

I've added an extra commit to change the RPC results to something similar to 16060.

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

I prefer the approach in #16060. The changes in this PR seem more complex for achieving a similar goal (+110 lines here vs -70 lines in 16060)

I think this should make future burials much simpler (it becomes a one-line change)

Softfork activation/burials are sufficiently rare that I don't think we have to optimize to minimize the code changes when they do happen.

it doesn't change RPC or tests much, and I think it will make it easier to make future changes to activation methods should we wish to do so.

I like the new RPC return object softfork in 16060. It seems simple, non-breaking when a deployment goes from bip9 to buried (the type field determines which other fields or sub-objects appear in the softfork object), and allows for other deployment methods (just add a new type).

We've been trying to bury these deployments for almost two years now (see #11398), so I just want to see it done. If people prefer this approach, I'm happy to close 16060 in favour of this.

ajtowns · 2019-07-02T15:14:53Z

Softfork activation/burials are sufficiently rare that I don't think we have to optimize to minimize the code changes when they do happen.

We've been trying to bury these deployments for almost two years now (see #11398), [...]

I think the reason it's taken that long is because burying deployments isn't trivial. "Optimising to minimise the code changes when they do happen" makes them trivial -- after the upfront work to setup Consensus::Deployment, burying segwit is just two one-line changes (one for mainnet, one for testnet):

-        consensus.vDeployments[Consensus::DEPLOYMENT_SEGWIT] = DeploymentByBIP9<1, 1479168000, 1510704000>(); // November 15th, 2016 - November 15th, 2017.
+        consensus.vDeployments[Consensus::DEPLOYMENT_SEGWIT] = DeploymentAtFixedHeight<481824>(); // 0000000000000000001c8018d9cb3b742ef25114f27563e3fc4a1902167f9893

-        consensus.vDeployments[Consensus::DEPLOYMENT_SEGWIT] = DeploymentByBIP9<1, 1462060800, 1493596800>(); // May 1st 2016 - May 1st 2017
+        consensus.vDeployments[Consensus::DEPLOYMENT_SEGWIT] = DeploymentAtFixedHeight<834624>(); // 00000000002b980fcd729daaa248fd9316a5200e9b367f4ff2c42453e84201ca

ajtowns · 2019-07-02T16:18:39Z

(+110 lines here vs -70 lines in 16060)

To expand on those numbers:

16060's -73 total comes from -57 lines from csv and segwit functional tests, -12 from rpc/blockchain.cpp, -9 from chainparams.cpp, -8 from dropping IsNullDummyEnabled, -8 from versionbitsinfo.cpp, +24 from new tests for getblockchaininfo changes, and -3 from the rest
16229's +109 total comes from +78 lines for chainparams/versionbits, +34 from new tests for getblockchaininfo changes, +9 for adding cltv/strictder to versionbitsinfo, -16 for rpc/blockchain.cpp, +4 for the rest

I think that's pretty representative: there's a chunk of new infrastructure in chainparams/versionbits with this PR, versus a bunch of dropped test code that's not compatible with hardcode fixed height deployments in 16060; versionbitsinfo is kind of verbose; and 16060 makes a few other choices that saves lines of code that aren't in this PR.

jnewbery · 2019-07-02T18:03:45Z

Thanks for running the numbers! Yes, 16060 removes both node and test code that are no longer required/relevant once the deployment heights are hard-coded.

ajtowns · 2019-08-05T15:51:48Z

#16060 is more straightforward for burying csv/segwit, and it doesn't really make things any more difficult to generalise than they already are, so abandoning this PR in favour of it

ajtowns added 7 commits June 18, 2019 15:18

[refactor] DeploymentActive helper

e54136f

Helper function that encapsulates the logic to check if a version bits deployment is active.

[refactor] vDeployments initialisation by template function for stati…

7b29ee2

…c checks

Support for fixed height activation and always-disabled deployments

da28ef8

Use Deployment struct for BIP 65 and BIP 66

7e6a90d

Display fixed height deployments in bip9_softforks

caf9848

(This does not include pre-CSV soft-forks however, due to special casing in the loop that calls BIP9SoftForkDescPushBack)

Hardcode csv/segwit activation

d4865b5

fanquake added Consensus Refactoring Needs Conceptual Review labels Jun 18, 2019

instagibbs mentioned this pull request Jun 18, 2019

Bury bip9 deployments #16060

Merged

ajtowns and others added 3 commits June 19, 2019 15:40

Merge bip9 info into getblockchaininfo softforks

c4ca812

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

48d5fef

Add releases notes for RPC change

845deb1

ajtowns closed this Aug 5, 2019

fanquake removed the Needs Conceptual Review label Aug 5, 2019

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

Standardise deployment handling#16229

Standardise deployment handling#16229
ajtowns wants to merge 10 commits intobitcoin:masterfrom
ajtowns:201906-deployments

ajtowns commented Jun 18, 2019 •

edited

Loading

Uh oh!

ajtowns commented Jun 18, 2019

Uh oh!

DrahtBot commented Jun 18, 2019 •

edited

Loading

Uh oh!

maflcko commented Jun 18, 2019

Uh oh!

ajtowns commented Jun 19, 2019

Uh oh!

jnewbery commented Jul 2, 2019

Uh oh!

ajtowns commented Jul 2, 2019

Uh oh!

ajtowns commented Jul 2, 2019

Uh oh!

jnewbery commented Jul 2, 2019

Uh oh!

ajtowns commented Aug 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

ajtowns commented Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajtowns commented Jun 18, 2019

Uh oh!

DrahtBot commented Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented Jun 18, 2019

Uh oh!

ajtowns commented Jun 19, 2019

Uh oh!

jnewbery commented Jul 2, 2019

Uh oh!

ajtowns commented Jul 2, 2019

Uh oh!

ajtowns commented Jul 2, 2019

Uh oh!

jnewbery commented Jul 2, 2019

Uh oh!

ajtowns commented Aug 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ajtowns commented Jun 18, 2019 •

edited

Loading

DrahtBot commented Jun 18, 2019 •

edited

Loading