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

Standardise deployment handling #16229

Closed
wants to merge 10 commits into from

Conversation

@ajtowns
Copy link
Contributor

commented Jun 18, 2019

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

ajtowns added some commits Feb 22, 2019

scripted-diff: change to Consensus::Deployment
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-
[refactor] DeploymentActive helper
Helper function that encapsulates the logic to check if a version bits
deployment is active.
Display fixed height deployments in bip9_softforks
(This does not include pre-CSV soft-forks however, due to special casing
in the loop that calls BIP9SoftForkDescPushBack)
@ajtowns

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

This is an alternative approach to #16060

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 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:

  • #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.

@instagibbs instagibbs referenced this pull request Jun 18, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

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

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

(+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

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

#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 ajtowns closed this Aug 5, 2019

@fanquake fanquake removed this from Chasing Concept ACK in High-priority for review Aug 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.