Introduce deploymentstatus (consensus)

https://github.com/bitcoin/bitcoin/pull/19438

Host: jnewbery  -  PR author: ajtowns

The PR branch HEAD was e72e062 at the time of this review club meeting.

Notes

  • A soft fork is a forwards-compatible change to the Bitcoin consensus rules. Any block or transaction that was invalid for nodes that don’t implement the new soft fork rules will be invalid for nodes that do implement the soft fork rules.

  • There have been many consensus changes in Bitcoin’s 12 year history. A BitMEX research blog post lists those consensus changes.

  • Changing Bitcoin’s consensus rules requires coordination across a decentralized network. We want as close as possible to 100% of economic nodes to start enforcing the new consensus rules and as close as possible to 100% of miners to be mining valid blocks according to the new consensus rules.

  • There have been several different methods to coordinate these changes to the consensus rules:

    • Flag Day upgrades were used to activate early consensus rules changes. Developers simply chose a time after which the new rules would be enforced.

    • IsSuperMajority was used for three changes to the consensus rules between 2012 and 2015. This activation method is described in BIP34.

    • Version Bits was used to activate two changes to the consensus rules between 2015 and 2017. The method is described in BIP9.

    The Optech Soft Fork Activation topic page provides an excellent summary of the history of activation methods.

  • Once a change to the consensus rules has been activated and buried under many months or years of work, the soft fork activation height is established fact. We assume that a competing fork of the chain will not re-org back to before the activation and change the activation height.

  • At that point, the exact method of activation is no longer interesting, and any logic in the node that enforces that activation method is technical debt. We can hard code the activation height into the node’s logic. This is called “burying” the deployment method, and is documented in BIP90.

  • Bitcoin Core PR8391 buried the IsSuperMajority soft fork deployments (BIP34, BIP65, and BIP66). PR16060 later buried the version bits soft fork deployments (CLTV and segwit).

  • Both of those PRs required changing multiple code paths in the validation logic. In a review comment, Pieter Wuille suggested abstracting out the difference between a soft fork activation and a buried deployment, so that burying deployments in future wouldn’t require so many changes to different code paths.

  • This PR does exactly that, by introducing Deployment{ActiveAfter|ActiveAt|Enabled} functions that are defined for both version bits deployments and buried deployments.

Questions

  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK? What was your review approach?

  2. How many buried deployments are there? Where are they listed in the code?

  3. How many version bits deployments are there? Where are they listed in the code?

  4. What are the advantages of using a buried deployment instead of a version bits deployment?

  5. Since C++11, we generally prefer to use scoped enumerations. What are the advantages of using scoped enums? Are they used in this PR? Why?

  6. What is DeploymentEnabled() used for? Can it ever return false for a buried deployment?

  7. What is the version bits cache used for?

  8. If the taproot soft fork is activated, and sometime later we want to bury that activation method, what changes need to be made to the code?

  9. If you wanted to revert segwit to a version bits deployment, what changes would you need to make to the code? Why might you want to do that?

Meeting Log

  118:00 <jnewbery> #startmeeting
  218:00 <maqusat> hi
  318:00 <amiti> hi
  418:00 <glozow> happy 100th meeting!
  518:00 <b10c> hi
  618:00 <Murch> hello
  718:00 <prayank> hi
  818:01 <jnewbery> happy 100 everybody!
  918:01 <schmidty> hi
 1018:01 <genef> hi
 1118:01 <michaelfolkson> hi
 1218:01 <gniemeyer> woooo
 1318:01 <amiti> WOW... thats wild!!
 1418:01 <ivanacostarubio> hello... happy 100th!
 1518:01 <ember_> wow 100
 1618:01 <emzy> hi
 1718:02 <michaelfolkson> Time to bury the first 50 PR review clubs?
 1818:02 <jnewbery> oooh maybe we're at 101 actually
 1918:02 <b10c> 💯
 2018:02 <jnewbery> whatever it is, welcome!
 2118:02 <glozow> đŸ˜±
 2218:02 <jnewbery> anyone here for the first time?
 2318:02 <sugarjig> Me!
 2418:02 <jnewbery> sugarjig: welcome!
 2518:02 <genef> 2nd time, still count?
 2618:03 <sugarjig> Thanks!
 2718:03 <jnewbery> glad you could join us
 2818:03 <alteralteregoism> It's my first time here.
 2918:03 <glozow> genef: welcome, first time being a returning review clubbie!
 3018:03 <genef> :)
 3118:03 <jnewbery> 2nd time doesn't count as first time. We call that an off-by-one bug
 3218:03 <ember_> welcome alteralteregoism
 3318:04 <gniemeyer> jnewberry: my first time
 3418:04 <b10c> welcome sugarjig alteralteregoism and gniemeyer!
 3518:04 <jnewbery> ok, couple of hints if it's your first time, and reminders if it's not: all questions are welcome! Everyone is here to learn.
 3618:04 <jnewbery> and 2: you don't have to ask to ask. If you have a question, just go right ahead and ask.
 3718:04 <jnewbery> more tips here: https://bitcoincore.reviews/your-first-meeting
 3818:05 <jnewbery> ok, who had time to review the PR this week? (y/n)
 3918:05 <genef> y
 4018:05 <amiti> 0.5y
 4118:05 <glozow> 0.8y
 4218:05 <ccdle12> y
 4318:05 <b10c> y
 4418:05 <emzy> n
 4518:05 <gniemeyer> n
 4618:05 <michaelfolkson> y
 4718:05 <ivanacostarubio> n
 4818:05 <schmidty> n
 4918:05 <sugarjig> 0.5y
 5018:05 <ember_> y
 5118:05 <maqusat> just had time to glance over the diff
 5218:06 <maqusat> .5y
 5318:06 <jnewbery> great. That's a lot of reviewers!
 5418:06 <jnewbery> First question: Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK? What was your review approach?
 5518:06 <maqusat> Concept ACK
 5618:07 <genef> reviewed, ACK. manual code review, no test
 5718:07 <ember_> Concept ACK
 5818:07 <ccdle12> concept ack
 5918:07 <Murch> n
 6018:07 <michaelfolkson> Concept ACK, Approach ACK. Read the links in the PR review club notes and then looked at the code. Didn't test
 6118:07 <jnewbery> does anyone want to give a short summary of the motivation?
 6218:07 <amiti> approach ACK ! I think its a clever way to make it simple to bury deployments & the way the enums / functions are defined provide some safety around accidentally mixing them up
 6318:08 <michaelfolkson> jnewbery: As you said in your notes we don't care how a soft fork was activated a long time after it has activated. "Technical debt"
 6418:09 <b10c> we want to make future softfork buries a small-as-possible code change
 6518:09 <glozow> yes we like to bury deployments, this PR is refactoring to make burying simpler
 6618:09 <jnewbery> michaelfolkson: right, that's the motivation for "burying" a deployment. This PR isn't burying any new deployments. What's it doing?
 6718:09 <ccdle12> it looks it provides a common interface for the code to switch on code paths according to predefined enums for certain softfork activations
 6818:09 <jnewbery> b10c glozow: exactly!
 6918:09 <jnewbery> ccdle12: yes
 7018:10 <jnewbery> let's get a bit more concrete
 7118:10 <michaelfolkson> Oh sure adding some helper functions to make it easier to bury future deployments
 7218:10 <jnewbery> How many ‘buried deployments’ are there? Where are they listed in the code?
 7318:10 <ivanacostarubio> easier way to deal with tecnical debt about burying deployments
 7418:10 <ccdle12> maybe 7-8?
 7518:10 <glozow> I counted 5: height in coinbase, CLTV, DER sigs, CSV, and segwit. They're in src/consensus/params.h.
 7618:11 <jnewbery> glozow: yus!
 7718:11 <jnewbery> https://github.com/bitcoin/bitcoin/blob/e72e062e5a8279864d746776dc9072c112ddc014/src/consensus/params.h#L14-L22
 7818:12 <jnewbery> A nice thing about this PR is that they're all enumerated now
 7918:12 <amiti> yeah, the PR makes it much easier to answer this question :)
 8018:12 <maqusat> 4, enum BuriedDeployment in consensus/params.h
 8118:12 <jnewbery> did anyone read the optech "soft fork activation" draft?
 8218:13 <jnewbery> I suppose we could argue that some of the very old satoshi-era softforks described here are also buried: https://deploy-preview-531--bitcoinops.netlify.app/en/topics/soft-fork-activation/#2009-hardcoded-height-consensus-nlocktime-enforcement
 8318:13 <maqusat> sorry 5 ;)
 8418:13 <jnewbery> since those were retroactively applied to the whole block chain and became part of the regular consensus rules
 8518:14 <jnewbery> but I like the answer 5
 8618:14 <amiti> that write up was fantastic!
 8718:14 <jnewbery> I agree!
 8818:14 <jnewbery> Onwards
 8918:14 <jnewbery> How many version bits deployments are there? Where are they listed in the code?
 9018:14 <glozow> question, is it ok to think of it as synonymous with BIP9 deployments?
 9118:15 <maqusat> 2, enum DeploymentPos in consensus/params.h
 9218:15 <glozow> I counted just 2, CSV and segwit but idk :(
 9318:15 <ccdle12> I think its `vDeployments` in chainparams?
 9418:16 <glozow> oh wait, i think i misunderstood the question. are we not talking about past deployments? :O
 9518:16 <jnewbery> glozow: right, two soft forks have been activated using version bits. What I meant to ask was "how many version bits deployments are defined in the code after this PR?"
 9618:16 <amiti> yeah, I think its the DeploymentPos values that are not MAX, which are assigned to chainparams vDeployments. so 2: testdummy and taproot.
 9718:17 <glozow> oh oops đŸ€Š
 9818:17 <jnewbery> maqusat amiti: yes!
 9918:17 <michaelfolkson> glozow: BIP 8 and BIP 148 and BIP 91 are all version bits deployments. But in the codebase you'll only find BIP 9 reference
10018:18 <jnewbery> and they're listed here: https://github.com/bitcoin/bitcoin/blob/e72e062e5a8279864d746776dc9072c112ddc014/src/consensus/params.h#L25-L31
10118:18 <jnewbery> Next question. What are the advantages of using a buried deployment instead of a version bits deployment?
10218:18 <b10c> It's super clear now, originaly tried to find the buried deployments and version bits on master...
10318:19 <jnewbery> b10c: indeed. This is a very nice cleanup.
10418:19 <amiti> with the version bits deployments there are more logical forks.. check the state to decide what validation rules to apply to a txn / block. with buried deployment you just assume the new rules apply
10518:19 <jnewbery> amiti: right, it simplifies the code
10618:19 <glozow> michaelfolkson: those are names of proposed activation methods not deployments
10718:20 <jnewbery> any other motiviations for burying a deployment?
10818:20 <ccdle12> removing technical debt/stale code
10918:20 <michaelfolkson> glozow: BIP 148 was deployed in a non-Core release
11018:21 <maqusat> limited bit space?
11118:21 <jnewbery> ccdle12: I think that's the same as amiti's reason: simplifying the code
11218:21 <ember_> maqusat versionbits get repurposed
11318:22 <ember_> less logic, less potential for bugs, theoretically
11418:22 <ccdle12> jnewbery: ah sorry, was just thinking in terms of just removing the code, I think I misread the original question
11518:22 <jnewbery> maqusat: I _think_ the versionbits code already allowed bits to be reused if the dates don't overlap. It's certainly allowed in BIP9
11618:22 <b10c> jnewbery: avoid deployment problems with super deep reorgs? not sure if the super deep reorgs are the bigger problem here
11718:23 <glozow> if the code is still in there, IBD nodes will use it to calculate when to activate stuff when they're catching up. could it be possible to accidentally introduce a bug while we're touching it, and affect IBD nodes?
11818:23 <glozow> i guess that falls into the technical debt category
11918:23 <maqusat> oh ok thx
12018:23 <jnewbery> b10c: that's not what I was thinking, but it is an interesting point. A very deep reorg could result in a consensus failure. We're talking a reorg of many years worth of blocks, and if that happens we have other problems
12118:23 <jnewbery> Did anyone read the original buried deployments BIP?
12218:24 <jnewbery> https://github.com/bitcoin/bips/blob/master/bip-0090.mediawiki#motivation
12318:24 <michaelfolkson> jnewbery: I did, yeah
12418:25 <maqusat> generally easier burying, less code changes. does it also allow for earlier testability (regtest)? seen that mentioned in one of original comments with motivations
12518:25 <glozow> applying the logic for activation is more computationally expensive than just checking a block height
12618:25 <michaelfolkson> Comments summary of BIP 90 was "Mostly Recommended for implementation, with some Discouragement"
12718:25 <jnewbery> BIP 90 included performance as a motivation. The activation method before BIP 9 was ISM, and that was inefficient. Burying those ISM deployments is a performance win.
12818:25 <jnewbery> BIP 90 is much more efficient because we cache the deployment state for each retarget period
12918:26 <jnewbery> *BIP 9 is much more efficient
13018:26 <michaelfolkson> I think the discouragement is in reference to that extremely large re-org risk and not burying too soon after activation
13118:27 <prayank> jnewbery: BIP 90 is for achieving the same thing as this PR but Bitcoin Core already follows BIP 90. Sorry, I am confused.
13218:27 <jnewbery> So the original motivation for BIP 90 was both performance and code simplicity. The performance part isn't such a consideration for burying BIP 9 deployments, but the code simplicity motivation is still there
13318:28 <michaelfolkson> prayank: BIP 90 is just for buried deployments. This PR makes it easier to bury deployments. They were still possible before this PR
13418:28 <jnewbery> prayank: there have been buried deployments in Bitcoin Core since BIP 90 was implemented. This PR simply refactors the code to make it simpler to bury future deployments
13518:28 <b10c> prayank: this PR cleans the implementation of BIP 90 up
13618:28 <prayank> Thanks
13718:28 <jnewbery> let's keep moving! Since C++11, we generally prefer to use scoped enumerations. What are the advantages of using scoped enums? Are they used in this PR? Why?
13818:29 <jonatack> hi
13918:29 <genef> scoped enums prevent leaking names/enum variants
14018:29 <ccdle12> prevent implicit conversions
14118:29 <glozow> Scoped enums are usually nice: the enumerators can't be implicitly converted to another type and they can be forward-declared.
14218:29 <jonatack> scoped enums are...scoped, don't pollute the global namespace, and don't implicitly convert to int
14318:30 <jnewbery> woohoo lots of c++ gurus in here :)
14418:30 <b10c> but they aren't used here!
14518:30 <glozow> leaking refers to, you can't confuse a `Blue::Berry` with `JNew::Berry`
14618:30 <jonatack> with plain old enums, the enum type isn't explicitly defined
14718:30 <jnewbery> glozow: it _is_ possible to forward declare an unscoped enum as long as you've explicitly declared the underlying type :p
14818:30 <jonatack> it just has to be large enough to hold the enumerators
14918:30 <glozow> jnewbery: ok tru
15018:31 <jnewbery> jonatack: are you talking about the underlying type?
15118:31 <glozow> here you can just say `Consensus::DEPLOYMENT_TAPROOT` when you're adding the activation code,
15218:31 <glozow> and then you don't need to change it after you bury it. You just move `DEPLOYMENT_TAPROOT` from `DeploymentPos` to `DeploymentBuried`.
15318:31 <jonatack> yes
15418:31 <jonatack> just went through these things for https://github.com/bitcoin/bitcoin/pull/21506
15518:31 <glozow> they still don't leak into global namespace because they're in the `Consensus` namespace yeah?
15618:31 <jonatack> "p2p, refactor: make NetPermissionFlags an enum class"
15718:32 <jnewbery> jonatack: you can explicitly declare the underlying type (or not) for both scoped and unscoped enums
15818:32 <jonatack> jnewbery: you can
15918:32 <jonatack> https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#S-enum
16018:32 <jonatack> is a good resource
16118:33 <amiti> glozow: +1
16218:33 <jnewbery> glozow: v good! yes, the enum is declared in the Consensus namespace, so it's scoped to that namespace
16318:33 <jnewbery> jonatack: yes! The cpp core guidelines are an excellent resource!
16418:33 <jnewbery> everyone happy with the enums? Any questions or should we move on
16518:34 <jnewbery> What is DeploymentEnabled() used for? Can it ever return false for a buried deployment?
16618:34 <maqusat> determine if a deployment has a block height assigned. probably could return false for deployments that have got buried without ever being activated (not having block height assigned), though probably it's cleaner to remove such from the code altogether?
16718:35 <glozow> For ongoing deployments, it checks the `BIP9Deployment` struct. For buried deployments, it uses the `DeploymentHeight()` to get a height.
16818:35 <glozow> All of the buried deployments have assigned DeploymentHeights that are not `std::numeric_limits<int>::max()`, so the condition should always return true.
16918:35 <glozow> So, no, it can't return false for a buried deployment.
17018:35 <glozow> I interpreted false to mean "we don't know about this deployment"
17118:35 <glozow> "dis not possible"
17218:36 <jnewbery> maquasat: yes, I agree
17318:36 <jnewbery> glozow: is it possible for any of the buried deployments to have a height that is "std::numeric_limits<int>::max()"?
17418:36 <glozow> is it? idk
17518:37 <b10c> it can return false if you don't impelement the case in DeploymentHeight() (but it gave me compiler warnings when testing)
17618:37 <genef> no, it's type is int16_t
17718:37 <jnewbery> This is the logic we're talking about: https://github.com/bitcoin/bitcoin/blob/e72e062e5a8279864d746776dc9072c112ddc014/src/deploymentstatus.h#L46
17818:38 <glozow> can you bury without assigning it an enumerator in `BuriedDeployment`?
17918:38 <glozow> i confused :(
18018:38 <jnewbery> glozow: oh no! What dis??? https://github.com/bitcoin/bitcoin/blob/e72e062e5a8279864d746776dc9072c112ddc014/src/chainparams.cpp#L483-L486
18118:38 <jnewbery> "Segwit disabled for testing\n"
18218:39 <glozow> ohhh
18318:39 <jnewbery> so currently, it _is_ possible for segwit to be 'undefined'. We use that in some of the tests.
18418:40 <jnewbery> Although I would very much like to get rid of that. It's not very useful, since segwit has been active for about 3 years now.
18518:40 <jnewbery> Does that make sense to everyone? DeploymentEnabled() can return false for segwit if we disabled it for testing
18618:41 <glozow> yessir
18718:41 <b10c> makes sense
18818:41 <jnewbery> Next question
18918:41 <jnewbery> What is the version bits cache used for?
19018:42 <michaelfolkson> Caching per-period state for soft forks deployed in parallel
19118:42 <b10c> caches the per-period and per-softfork deployment state. this works because all blocks in a period have the same deployment state
19218:42 <michaelfolkson> Which will probably never get used?
19318:43 <jnewbery> michaelfolkson: why do you say it's only for forks in parallel? All version bits deployments use the version bits cache
19418:43 <michaelfolkson> jnewbery: Gotcha, I misunderstood that. Thanks
19518:44 <glozow> so you calculate the state for every block that's 0 mod 2016 and then use the cache for all the others?
19618:45 <jnewbery> glozow: right
19718:45 <jnewbery> state can only change on retarget boundaries
19818:45 <b10c> where is the implemtation of the 'mod 2016'?
19918:45 <b10c> must have missed it
20018:46 <amiti> I think its in `GetStateFor`
20118:46 <jnewbery> Here you go: https://github.com/bitcoin/bitcoin/blob/e72e062e5a8279864d746776dc9072c112ddc014/src/versionbits.cpp#L20-L23
20218:46 <amiti> https://github.com/bitcoin/bitcoin/blob/master/src/versionbits.cpp#L22
20318:46 <amiti> oh darn
20418:46 <jnewbery> snap!
20518:46 <amiti> you beat me :)
20618:47 <jnewbery> nPeriod is 2016 for mainnet
20718:47 <b10c> thanks jnewbry amiti!
20818:47 <jonatack> jnewbery is really fast (recalling that scavenger hunt in NYC)
20918:48 <prayank> lol
21018:48 <jnewbery> on regtest, the retarget period is 144. Look for nMinerConfirmationWindow in chainparams.cpp
21118:49 <jnewbery> jonatack: (:
21218:49 <glozow> why does regtest retarget at all?
21318:49 <jnewbery> so we can test things
21418:49 <sipa> glozow: because we want to be able to test the retargetting mechanism
21518:50 <jnewbery> actually retarget boundaries are a fertile source of bugs
21618:50 <jnewbery> there have been plenty of off-by-ones around those
21718:50 <jnewbery> Next question. If the taproot soft fork is activated, and sometime later we want to bury that activation method, what changes need to be made to the code?
21818:50 <glozow> oh mm. but it doesn't actually change difficulty right?
21918:51 <jnewbery> (assuming this PR is merged)
22018:51 <michaelfolkson> That why we have both DeploymentActiveAt and DeploymentActiveAfter? Because we are worried by off-by-ones?
22118:51 <sugarjig> Is that where we would move the value from `DeploymentPos` to `BuriedDeployment`?
22218:51 <amiti> I think you'd just need to move taproot from DeploymentPos enum to BuriedDeployment?
22318:52 <sipa> glozow: consensus.fPowNoRetargeting = true;
22418:52 <sipa> you're right; regtest doesn't actually retarget
22518:52 <sipa> but it does still have versionbits logic
22618:53 <glozow> that makes sense
22718:53 <maqusat> move DEPLOYMENT_TAPROOT form DeploymentPos to BuriedDeployment
22818:53 <ccdle12> apologies if this is a silly question but.. with burying taproot, I think theres still some taproot_active bools floating around, would those have to be removed as well to constitute a bury?
22918:53 <felixweis> could on regtest have the diff change (recalculate) but not actually enforce?
23018:54 <jnewbery> sugarjig amiti maqusat: exactly! There are a couple more small changes, but that's essentially it. Crucially, I don't think you'd need to change any logic in validation.
23118:54 <glozow> also need to add the height ya?
23218:54 <michaelfolkson> felixweis: I think that is what sipa and glozow concluded for regtest
23318:54 <glozow> is there an rpc or something that could tell you when taproot activated?
23418:54 <genef> what's the typical time to bury a deployment after activation?
23518:55 <michaelfolkson> genef: SegWit was done 2 years after
23618:55 <b10c> glozow: ha! :d
23718:55 <jnewbery> ccdle: not a silly question at all! Where does the taproot_active bool get set?
23818:55 <ccdle12> I saw it in policy.h
23918:55 <ccdle12> https://github.com/bitcoin/bitcoin/blob/4a540683ec40393d6369da1a9e02e45614db936d/src/policy/policy.h#L111
24018:55 <michaelfolkson> genef: No science though. Whenever a PR gets opened, reviewed and merged. As long as it isn't too soon after activation
24118:56 <sipa> ccdle12: that's where it is used
24218:56 <jnewbery> ccdle12: Right, it's in the function signature. Where does the caller of that function set it?
24318:56 <genef> michaelfolkson: thanks
24418:56 <ccdle12> ah right in validation.cpp before accepting the tx to the mempool
24518:57 <b10c> glozow: I saw this: https://github.com/bitcoin/bitcoin/pull/16060/files#r290696589
24618:57 <jnewbery> ccdle12: yes, and how is the bool set?
24718:58 <amiti> glozow, b10c: yeah, getblockchaininfo has a `bip9_softforks` section that gives you `status`
24818:58 <glozow> b10c: amiti: ooh thanks
24918:58 <ccdle12> jnewbery: ThresholdState::ACTIVE
25018:58 <jnewbery> It's set here: https://github.com/bitcoin/bitcoin/blob/e72e062e5a8279864d746776dc9072c112ddc014/src/validation.cpp#L718 by DeploymentActiveAt()
25118:59 <jnewbery> so that code doesn't need to be changed to move the taproot deployment from versionbits to buried
25218:59 <ccdle12> jnewbery: I see thank you!
25318:59 <jnewbery> This is all assuming that this PR #19438 is merged, of course.
25419:00 <jnewbery> ok, there was one more question that aj added that I'm afraid we don't have time for! We can leave it as an exercise for the reader.
25519:00 <jnewbery> #endmeeting