Introduce deploymentstatus #19438

ajtowns · 2020-07-03T17:03:16Z

Introduces helper functions to make it easy to bury future deployments, along the lines of the suggestion from 11398 "I would prefer it if a buried deployment wouldn't require all code paths that check the BIP9 status to require changing".

This provides three functions: DeploymentEnabled() which tests if a deployment can ever be active, DeploymentActiveAt() which checks if a deployment should be enforced in the given block, and DeploymentActiveAfter() which checks if a deployment should be enforced in the block following the given block, and overloads all three to work both with buried deployments and versionbits deployments.

This adds a dedicated lock for the versionbits cache, which is acquired internally by the versionbits functions, rather than relying on cs_main. It also moves moves versionbitscache into deploymentstatus to avoid a circular dependency with validation.

DrahtBot · 2020-07-03T19:35:09Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Move pblocktree global to BlockManager #22371 by MarcoFalke
Move CBlockTreeDB to node/blockstorage #22242 by MarcoFalke
Implement BIP-119 Validation (CheckTemplateVerify) #21702 by JeremyRubin
build: Add -Werror=implicit-fallthrough compile flag #21430 by hebasto
Default to NODE_WITNESS in nLocalServices #21090 by dhruv

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.

JeremyRubin

Concept ACK and some lite code review feedback.

JeremyRubin · 2020-07-03T20:10:16Z

src/consensus/params.h

@@ -11,6 +11,15 @@

 namespace Consensus {

+enum BuriedDeployment


prefer enum class to prevent leaking names.

Also helps with avoiding overlap with SignalledDeployment.

Leaking the names is the point -- otherwise you'd need to change every use from Consensus::DeploymentPos::SEGWIT to Consensus::BuriedDeployment::SEGWIT. With C++20, you will be able to specify enum class DeploymentPos { .. }; using enum DeploymentPos; but that's a while away.

JeremyRubin · 2020-07-03T20:11:55Z

src/consensus/params.h

+    DEPLOYMENT_CLTV = -255,
+    DEPLOYMENT_DERSIG,
+    DEPLOYMENT_CSV,
+    DEPLOYMENT_SEGWIT,


prefer explicitly numbering these.

-255 is an extremely odd number, can you clarify why you picked it? It doesn't fit in a signed byte.

Explicitly numbering them risks accidentally repeating a number. No particular objection to a different starting point. Could perhaps make it enum DeploymentPos : uint8_t and enum BuriedDeployment : int16_t { DEPLOYMENT_CLTV = 256, // larger than largest possible DeploymentPos value.

(I guess 2**n is an "extremely even" number, so calling +/- 2**n-1 "extremely odd" makes a lot of sense...)

JeremyRubin · 2020-07-03T20:13:43Z

src/consensus/params.h

@@ -80,7 +89,19 @@ struct Params {
    int64_t DifficultyAdjustmentInterval() const { return nPowTargetTimespan / nPowTargetSpacing; }
    uint256 nMinimumChainWork;
    uint256 defaultAssumeValid;
+
+    inline int DeploymentHeight(BuriedDeployment dep) const


Maybe tighter to make this a templated function as we're never calling DeploymentHeight with a variable, only literal.

Then we get not just a warning, but a compiler error.

I had tried that, but thought that it might be useful to be able to do for (dep = 0; dep < MAX_VERSIONBITS_DEPLOYMENTS; ++dep) { ... DeploymentActiveAt(pindex, consParams, dep) .. }. I think in all the cases where that currently comes up you want to know more fine grained details though. (Changing BuriedForkDescPushBack to take a BuriedDeployment instead of a height would make use of that, eg)

I don't think the type checking is any better with the template though, as unfortunately you can invoke DeploymentHeight<(BuriedDeployment)9000>() just as easily as DeploymentHeight((BuriedDeployment)9000) with both g++ and clang++.

src/deploymentstatus.h

JeremyRubin · 2020-07-03T20:18:25Z

src/deploymentstatus.h

+extern VersionBitsCache versionbitscache GUARDED_BY(cs_main);
+
+/** Determine if a deployment is active for the next block */
+inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::BuriedDeployment dep)


This dispatch scares me a bit unless we use enum class -- enums could coerce poorly right?

Nope; int (and the like) won't automatically coerce to enum, so with int x = Consensus::DEPLOYMENT_SEGWIT; return DeploymentActiveAt(pindex, params, x) you get errors:

validation.cpp:1921:12: error: no matching function for call to 'DeploymentActiveAt' return DeploymentActiveAt(pindex, consensusparams, x); ^~~~~~~~~~~~~~~~~~ ./deploymentstatus.h:32:13: note: candidate function not viable: no known conversion from 'int' to 'Consensus::BuriedDeployment' for 3rd argument inline bool DeploymentActiveAt(const CBlockIndex* pindex, const Consensus::Params& params, Consensus::BuriedDeployment dep) ^ ./deploymentstatus.h:37:13: note: candidate function not viable: no known conversion from 'int' to 'Consensus::DeploymentPos' for 3rd argument inline bool DeploymentActiveAt(const CBlockIndex* pindex, const Consensus::Params& params, Consensus::DeploymentPos dep) EXCLUSIVE_LOCKS_REQUIRED(cs_main) ^

(even if it did, which enum it coerced to is ambiguous between the two types, resulting in a failure like error: call to 'DeploymentActiveAt' is ambiguous)

ajtowns · 2021-06-29T22:04:06Z

Rebased past #21789

Adds support for versionbits deployments to DeploymentEnabled, DeploymentActiveAfter and DeploymentActiveAt. Also moves versionbitscache from validation to deploymentstatus.

-BEGIN VERIFY SCRIPT- sed -i -e 's/versionbitscache/g_versionbitscache/g' $(git grep -l versionbitscache) -END VERIFY SCRIPT-

Rename BIP9SoftForkPushBack and BuriedSoftForkPushBack to SoftForkPushBack and have the compiler figure out which one to use based on the deployment type. Avoids the need to update the file when burying a deployment.

Moves the VersionBits* functions to be methods of the cache class, and makes the cache and its lock private to the class.

This also changes ComputeBlockVersion to take the versionbits cache mutex once, rather than once for each versionbits deployment.

maflcko

re-ACK e48826a 🥈

Checked with:
git range-diff bitcoin-core/master e14966e e48826a --word-diff-regex=. -U0

Show signature and timestamp

Signature:

Timestamp of file with hash 51b700b2e20a4085313cce0dc833915df20496b87f40dbee5ed2e7651008877d -

jnewbery · 2021-06-30T15:50:03Z

ACK e48826a

gruve-p · 2021-07-01T14:10:47Z

ACK e48826a

ajtowns added Refactoring Consensus labels Jul 3, 2020

JeremyRubin previously requested changes Jul 3, 2020

View reviewed changes

ajtowns force-pushed the 202007-deployment-refactor branch from 5834cc8 to 6b08b76 Compare July 3, 2020 21:48

This was referenced Jul 4, 2020

refactor: Make CCheckQueue RAII-styled #18731

Closed

BIP-325: Signet [consensus] #18267

Merged

Implement BIP 340-342 validation (Schnorr/taproot/tapscript) #17977

Closed

DrahtBot mentioned this pull request Jul 11, 2020

Refactor mempool.dat to be extensible, and store missing info #19488

Closed

This was referenced Jul 24, 2020

Replace unused BIP 9 logic with draft BIP 8 #19573

Closed

[RFC] Package-relay: sender-initiated #19621

Closed

This was referenced Aug 2, 2020

assumeutxo #15606

Closed

Switch BlockMap to use an unordered_set under the hood #19677

Closed

This was referenced Aug 20, 2020

Net processing: move ProcessMessage() to PeerLogicValidation #19704

Merged

refactor: Keep mempool interface in validation #19629

Closed

DrahtBot mentioned this pull request Aug 28, 2020

[net processing] Move Misbehaving() to PeerManager #19791

Merged

DrahtBot added the Needs rebase label Sep 7, 2020

ajtowns force-pushed the 202007-deployment-refactor branch from 6b08b76 to 787040b Compare September 8, 2020 15:04

DrahtBot added Needs rebase and removed Needs rebase labels Sep 8, 2020

ajtowns force-pushed the 202007-deployment-refactor branch from 787040b to 1f1d51d Compare September 8, 2020 22:27

DrahtBot removed the Needs rebase label Sep 8, 2020

This was referenced Sep 19, 2020

Implement BIP 340-342 validation (Schnorr/taproot/tapscript) #19953

Merged

signet mining utility #19937

Merged

DrahtBot added the Needs rebase label Sep 21, 2020

ajtowns force-pushed the 202007-deployment-refactor branch from 1f1d51d to 91572f3 Compare September 22, 2020 03:00

DrahtBot removed the Needs rebase label Sep 22, 2020

This was referenced Sep 22, 2020

validation: re-delegate absurd fee checking from mempool to clients #19339

Merged

History for Taproot PR #19953 #19997

Closed

validation/util: add GetTransactionFee #20025

Closed

ajtowns force-pushed the 202007-deployment-refactor branch from e14966e to df308ab Compare June 29, 2021 22:03

ajtowns added 9 commits June 30, 2021 08:18

[refactor] Add versionbits deployments to deploymentstatus.h

de55304

Adds support for versionbits deployments to DeploymentEnabled, DeploymentActiveAfter and DeploymentActiveAt. Also moves versionbitscache from validation to deploymentstatus.

scripted-diff: rename versionbitscache

c64b2c6

-BEGIN VERIFY SCRIPT- sed -i -e 's/versionbitscache/g_versionbitscache/g' $(git grep -l versionbitscache) -END VERIFY SCRIPT-

[move-only] Rename versionbitsinfo to deploymentinfo

ea68b3a

deploymentinfo: Add DeploymentName()

92f48f3

[refactor] rpc/blockchain.cpp: SoftForkPushBack

8ee3e0b

Rename BIP9SoftForkPushBack and BuriedSoftForkPushBack to SoftForkPushBack and have the compiler figure out which one to use based on the deployment type. Avoids the need to update the file when burying a deployment.

[refactor] versionbits: make VersionBitsCache a full class

0cfd6c6

Moves the VersionBits* functions to be methods of the cache class, and makes the cache and its lock private to the class.

[move-only] Move ComputeBlockVersion from validation to versionbits

4a69b4d

[refactor] Move ComputeBlockVersion into VersionBitsCache

c5f3672

This also changes ComputeBlockVersion to take the versionbits cache mutex once, rather than once for each versionbits deployment.

tests: remove ComputeBlockVersion shortcut from versionbits tests

e48826a

DrahtBot removed the Needs rebase label Jun 29, 2021

ajtowns force-pushed the 202007-deployment-refactor branch from df308ab to e48826a Compare June 30, 2021 01:24

ajtowns requested a review from maflcko June 30, 2021 02:05

maflcko approved these changes Jun 30, 2021

View reviewed changes

DrahtBot mentioned this pull request Jun 30, 2021

Move pblocktree global to BlockManager #22371

Merged

maflcko merged commit ddc6979 into bitcoin:master Jul 1, 2021

bitcoin deleted a comment Jul 1, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 3, 2021

Merge bitcoin#19438: Introduce deploymentstatus

560ae21

ajtowns mentioned this pull request Jul 13, 2021

refactor: Use DeploymentEnabled to hide VB deployments #22385

Merged

tryphe mentioned this pull request Jul 30, 2021

gcc -Wtype-limits warnings in consensus/params.h #22587

Closed

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge ddc6979 into merged_master (Bitcoin PR bitcoin/bitcoin#19438)

47164a1

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce deploymentstatus #19438

Introduce deploymentstatus #19438

ajtowns commented Jul 3, 2020 •

edited

DrahtBot commented Jul 3, 2020 •

edited

JeremyRubin left a comment

JeremyRubin Jul 3, 2020

JeremyRubin Jul 3, 2020

ajtowns Jul 3, 2020

JeremyRubin Jul 3, 2020

ajtowns Jul 3, 2020

JeremyRubin Jul 3, 2020

ajtowns Jul 3, 2020

JeremyRubin Jul 3, 2020

ajtowns Jul 3, 2020

ajtowns commented Jun 29, 2021

maflcko left a comment

jnewbery commented Jun 30, 2021

gruve-p commented Jul 1, 2021

		@@ -11,6 +11,15 @@

		namespace Consensus {

		enum BuriedDeployment

Introduce deploymentstatus #19438

Introduce deploymentstatus #19438

Conversation

ajtowns commented Jul 3, 2020 • edited

DrahtBot commented Jul 3, 2020 • edited

Conflicts

JeremyRubin left a 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

Choose a reason for hiding this comment

ajtowns commented Jun 29, 2021

maflcko left a comment

Choose a reason for hiding this comment

jnewbery commented Jun 30, 2021

gruve-p commented Jul 1, 2021

ajtowns commented Jul 3, 2020 •

edited

DrahtBot commented Jul 3, 2020 •

edited