#16060 Bury bip9 deployments (consensus)

https://github.com/bitcoin/bitcoin/16060

Host: jnewbery

Notes

  • Softforks have used a variety of deployment methods in the past:
  • After a soft fork is activated and some time has passed, the exact activation method is no longer interesting. For example, we don’t care whether segwit was activated by BIP9, BIP91 or UASF. What we know is that the majority of economic nodes started enforcing the segwit rules from block 481824.
  • The ISM soft fork heights were hard-coded in Bitcoin Core PR #8391, and documented in BIP 90. Depending on your definition, this could be considered a ‘hard fork’ since a previously invalid chain becomes valid. However, for such a chain to be the most-work chain would involve a multi-year reorg, so such a definition is not very useful.
  • P2SH and segwit script validation was enforced back to genesis (with one exception) in Bitcoin Core PR #11739.
  • This PR attempts to hard-code the heights of CSV and segwit activation.
  • It also changes the format of soft fork reporting in the return object of getblockchaininfo.
  • Review/test of this PR should include verifying that the hard-coded activation heights are correct.

Meeting Log

13:00 < jnewbery> hi!
13:00 < jonatack__> hi!
13:00 < pinheadmz> Yo
13:00 < kanzure> hi
13:00 < davterra> hi
13:00 < merehap> hey
13:00 < peevsie> hi
13:00 < sipa> comcept nack
13:00 < sipa> :p
13:00 < jnewbery> welcome all (except sipa, who's trolling)
13:00 < schmidty> Hola
13:00 < lightlike> hi
13:00 < pinheadmz> comcept!
13:01 < jnewbery> did everyone have a chance to read the notes?
13:01 < b10c> hi
13:01 < michaelfolkson> Haha. Is he nacking this channel? Nack overruled
13:01 < kanzure> http://diyhpl.us/wiki/transcripts/bitcoin-core-dev-tech/2019-06-05-code-review/
13:02 < jnewbery> hi folks. We're here at a Bitcoin Core meeting in Amsterdam
13:02 < sipa> i'm on my phone, sitting next to john, mildly intoxicated
13:02 < jnewbery> We talked about the review process today. kanzure's transcript may be of interest to people here
13:03 < kanzure> sipa is intoxicated by code review
13:03 < kanzure> hours and hours of code review today.
13:03 < jonatack__> :D
13:04 < jnewbery> This is the first PR that we've looked at that touches consensus code. It's a pretty minor change, but we always want to be especially careful when touching anything that goes near consensus
13:04 < michaelfolkson> That transcript was really interesting. Hopefully people experiment with some of the ideas in it in future
13:04 < merehap> I think this channel has to be shutdown if sipa nacks it. Not really sure we have a choice here.
13:05 < michaelfolkson> Sorry John, please continue
13:05 < jnewbery> No, it's possible to NACK a sipa NACK to neutralize it
13:05 < jnewbery> it's a rarely used technique
13:05 < moneyball> sipa NACK
13:05 < xsb> snack
13:05 < jnewbery> did anyone have any questions about the PR?
13:06 < michaelfolkson> Yup
13:06 < jb55> what does it mean to bury a deployment
13:06 < jb55> start with something simple
13:06 < jnewbery> thanks jb55
13:07 < jnewbery> I think the terminology was introduced in BIP 90
13:07 < jnewbery> the idea is that the soft fork activation has been buried under so much work, that there's no longer any doubt about when it activated
13:07 < michaelfolkson> So it is effectively introducing a checkpoint right?
13:08 < jnewbery> the logic in the client that tests for activation (whether that's ISM or BIP9) is effectively obsolete
13:08 < merehap> Can LOCK(cs_main) be removed from IsNullDummyEnabled? It looks like it. It was already removed from the similar IsWitnessEnabled.
13:08 < jnewbery> so instead of relying on difficult-to-understand logic, we just hard-code the activation height
13:09 < pinheadmz> Does this make the bip9 signal bit reuseable? Or is that already true
13:09 < jnewbery> michaelfolkson: it's not a checkpoint, but there are similarities in the concept. We're asserting a fact about the valid chain, and hardcoding that into the client.
13:10 < jnewbery> but it's not a checkpoint. It doesn't limit the valid chain at that height to a certain block.
13:10 < michaelfolkson> Ah ok so a re-org is still possible before that block height but deactivating it at that block height becomes impossible
13:10 < peevsie> it's like a retroactive hardfork
13:10 < jonatack__> The "what" of any consensus change is interesting. Nevertheless, I found the pros and cons of "why" even more so.
13:11 < jonatack__> e.g. the discussion from here: https://github.com/bitcoin/bitcoin/pull/12360#issuecomment-383342462
13:11 < jnewbery> peevsie: in a narrow technical sense, you could claim this is a hardfork, but I don't think this is a useful definition
13:12 < jnewbery> jonatack__: yes, I thought the discussion around whether such a change is desirable was more interesting than the code changes this week
13:12 < michaelfolkson> So the reasons why previous attempts failed was mainly lack of engagement? The only criticism seemed to be from Marco on splitting RPC and consensus changes
13:12 < jonatack__> yes... the discussion between you, marco and suhas
13:13 < jnewbery> merehap: I think you may be right. You should leave a review comment!
13:13 < lightlike> jnewbery: in the first PR #11398, you suggested that this might need might need a BIP itself and got some approval. In the subsequent PRs, this was not even a topic. What did change?
13:13 < merehap> jnewbery: Will do.
13:13 < jnewbery> pinheadmz: bip9 bits are reusable as long as the activation dates don't overlap. That was part of the design motivation for BIP9
13:15 < jnewbery> michaelfolkson: yes, I closed the previous PR because I couldn't convince other contributors to review
13:15 < jnewbery> I think this change is useful, especially with the upcoming schnorr/taproot softfork changes
13:15 < michaelfolkson> Is there a certain time period that we would want since activation before "burying"?
13:16 < michaelfolkson> Or does it not matter? 6 months, 12 months, 18 months whatever?
13:17 < jnewbery> I think if Bitcoin experienced a 6 month re-org then we might question the usefulness of the system
13:17 < jnewbery> so I'd consider a softfork that's buried by 6 months' work as a historic fact
13:18 < pinheadmz> Where do you think that line is?
13:18 < pinheadmz> Like how big a reorg is really “were done let’s go home”
13:18 < jnewbery> I don't know!
13:18 < michaelfolkson> Why do you say it is useful for upcoming Schnorr/taproot? Generally a good idea to address code complexity before attempting major change?
13:19 < michaelfolkson> The benefits of this are clearing up code complexity and testing right? Time of IBD isn't materially impacted
13:20 < jnewbery> Yes. This is just a tidy-up in the way softforks are reported. It also allows us to remove a bunch of tests for stuff that is no longer needed
13:21 < jnewbery> I think p2p_segwit.py and p2p_compactblocks.py
13:21 < jnewbery> lightlike: I don't know if a BIP is required. I think a post to the mailing list is probably sufficient
13:23 < michaelfolkson> Is there a good resource explaining the different activation methods? I didn't know what flag day, IsSuperMajority were.
13:23 < jnewbery> I don't know if there's a single resource that describes them all
13:24 < jnewbery> Here's a bitmex blog post on the history of consensus changes: https://blog.bitmex.com/bitcoins-consensus-forks/
13:25 < jnewbery> I've given talks on the history of consensus changes before. Perhaps I'll turn that into a video or blog post at some point
13:26 < michaelfolkson> Cool, thanks
13:26 < jonatack__> Kudos on being persistent. Taking note of the good idea to recap the ACKs from the previous PRs to rope in support.
13:27 < jnewbery> until then, the links to the BIPs in the notes describe all the activation methods
13:28 < michaelfolkson> Yeah... As a thought experiment from <kanzure> <moneyball> discussion is there anything to try to get this pushed through?
13:28 < jnewbery> thanks jonatack__. Nagging is an important skill to cultivate :)
13:29 < jnewbery> michaelfolkson: f2f nagging is my next tactic
13:29 < michaelfolkson> Assuming it is agreed that PR doesn't need splitting between RPC/consensus
13:30 < merehap> I think that a lot of the cases where different decisions were made in very similar situations tend to be disorganized in most peoples' minds. If you were to make a blog post that gets everything about historical activation methods in one place, I think a lot of people would benefit. I'd certainly be happy. I think blog posts are better than videos for that kind of thing since they are easily skim-able/reference-able.
13:31 < jonatack__> f2f ?
13:31 < jnewbery> face to face
13:31 < jonatack__> ah!
13:32 < jnewbery> merehap: I'm planning to give a talk on this at our upcoming residency. I'll try to turn it into something more widely usable
13:32 < merehap> Awesome! Greatly appreciated.
13:32 < jonatack__> If we segue at some point to general questions, I have one about timestamping reviews.
13:33 < lightlike> i was a bit confused with the activation heights in test: in regtest, default heights are CSV:432, Segwit:0 (always active unless overridden). But in the tests, activation height for segwit seems to be hardcoded to 432 is as well. Why are CSV and segwit coupled like that?
13:33 < sipa> it's not hardcoded to 432; it just has bip9 start time in the past
13:33 < michaelfolkson> <merehap> <jnewbery> Cool. What they were and the pros and cons of each, followed by which is likely to be used in future
13:33 < sipa> so at block 144 (first period), it becomes active
13:33 < sipa> and blocm 288 it's locked in
13:34 < sipa> at block 432 it's active
13:35 < jnewbery> I can't remember exactly why the PR doesn't change CSV actication height to 0. It might have been too much effort to change the tests
13:36 < jnewbery> michaelfolkson: Discussion of the pros/cons of activation method is a bit too broad to be discussed here
13:36 < michaelfolkson> Oh no I mean in the blog post
13:37 < jnewbery> ah ok
13:37 < jnewbery> jonatack__: go ahead with general questions
13:38 < jonatack__> Ok, so timestamping commits is easy thanks to the info at https://github.com/opentimestamps/opentimestamps-client/blob/master/doc/git-integration.md
13:39 < jonatack__> MarcoFalke: how do you timestamp reviews like here https://github.com/bitcoin/bitcoin/pull/15988#issuecomment-491949275
13:39 < jonatack__> to ensure GitHub doesn't change your review
13:39 < jonatack__> idk if Marco is afk
13:40 < jnewbery> MarcoFalke isn't here
13:40 < jonatack__> if away at least he might see it
13:40 < jonatack__> ok
13:40 < jnewbery> but feel free to ask in #bitcoin-core-dev !
13:41 < sipa> he's about 2.5 metets behind you, jnewbery
13:41 < sipa> *meters
13:41 < jonatack__> * suspense builds *
13:41 < jnewbery> That goes to everyone here. Please feel free to post in #bitcoin-core-dev if you have any questions. People are friendly there!
13:42 < jnewbery> he's just charged his wine glass. I wouldn't want to interupt a man with opentimestamps questions in such a condition.
13:44 < jnewbery> alright, any more questions?
13:44 < merehap> So there are a lot of magical int values used in these code sections for special block height conditions, potentially leading to missed error case handling cases. Would a drive to make some kind of int wrapper type for block heights that has helper methods exposing the special cases be well received? Or is that just the type of refactoring of consensus code that is considered too dangerous? Example magical block heights i
13:44 < merehap> nclude null, -1, and max int here: "consensusParams.SegwitHeight != std::numeric_limits<int>::max()".
13:45 < jnewbery> merehap: I wouldn't personally be interested in reviewing such a PR :)
13:46 < merehap> (In general my potential value-add to a project is figuring out compile time ways to eliminate or reduce missed edge case, which is why I'm asking)
13:46 < jnewbery> The payoff doesn't seem worth the codechurn/review burden
13:46 < merehap> Makes sense. Is that in general for Bitcoin Core, or more just for consensus critical stuff?
13:47 < jnewbery> Mostly consensus-critical stuff, but in general too
13:47 < michaelfolkson> I have some on setting up your review process from last week John but let's leave that for another week. Or maybe I could just post in #bitcoin-core-dev.
13:47 < merehap> Gotcha
13:48 < sipa> morcos is applauding jnewbery's dedication to the cause
13:48 < michaelfolkson> Let's wrap up. I feel we're delaying the drinking haha
13:49 < jonatack__> I began setting up a twitter/mastodon bot for live Bitcoin Core PR activity and code reviews. The goal is to encourage quality review and reviewers.
13:49 < jnewbery> My thoughts about opening PRs: no-one owes you a review. Anyone who reviews your code is doing you a favour. If you open a PR, you're competing with other PRs for review time.
13:49 < jonatack__> Anyone against that idea please let me know ;)
13:49 < michaelfolkson> Sounds good to me
13:49 < jonatack__> Follow it on https://twitter.com/BitcoinCorePRs
13:49 < jnewbery> That shouldn't discourage you from opening PRs, but you should think about how to prioritize your PR against the other open PRs
13:51 < jnewbery> if in doubt about how useful other people think your PR will be, feel free to ask in #bitcoin-core-dev, or by directly asking other contributors
13:51 < michaelfolkson> Cool, thanks everyone, thanks John. Thanks <sipa> for your guest appearance
13:51 < jonatack__> The bot could hook into the PR reviews club website to broadcast the meetings if that is ok
13:52 < jnewbery> thanks jonatack__. I'm following!
13:52 < jonatack__> Thanks!
13:53 < jnewbery> Any other questions?
13:53 < pinheadmz> cheers!
13:53 < jnewbery> Did anyone build and test `getblockchaininfo` ?
13:54 < lightlike> i did.
13:55 < merehap> jnewbery: Yeah, I just want to figure out how we deal with potentially compile-time preventable bugs like the duplicate block validation bug (https://nvd.nist.gov/vuln/detail/CVE-2018-17144). That kind of thing can't be statically prevented with the current code base, but there seems to be no feasible way to get the code base into a more compile-time safe state.
13:55 < jnewbery> lightlike: great!
13:56 < merehap> The tests didn't get that far for me, feature_dbcrash.py failed first unfortunately. I could manual run that as a one-off though.
13:57 < jnewbery> merehap: I think you're right thatCVE-2018-17144 can't be statically prevented.
13:58 < jnewbery> I don't know how you'd catch it with testing
13:58 < jnewbery> merehap: that's a shame. That test isn't run by travis, so I wouldn't be surprised if there are intermittent failures in master
13:58 < merehap> But could be, with a moderate amount of refactoring/better class design. But that introduces different refactor risks, and refactor risks are what caused the CVE in the first place.
13:59 < merehap> Basically by encapsulating all the validation into an object, and passing that validated object around, you can avoid cases like the CVE.
13:59 < merehap> But it's probably a bridge too far.
13:59 < jnewbery> refactors are ok if they're motivated. I think what people react agaionst is when someone opens a PR to refactor something without any obvious reason why it's an improvement
14:00 < jnewbery> merehap: seems very ambitious!
14:00 < jnewbery> ok, let's wrap it up there
14:00 < jnewbery> Thanks everyone! Same time again next week
14:00 < merehap> Thanks jnewbery! Informative as always.
14:01 < pinheadmz> have fun at 'Breaking!
14:01 < jonatack__> Thanks jnewbery, sipa, michael and everyone!
14:01 < lightlike> thanks!
14:01 < merehap> Also thanks to those who took the rare step of overruling a sipa NACK so we could have this discussion.
14:01 < jonatack__> PS - If anyone is interested in reviewing p2p tests of https://bitcoin-core-review-club.github.io/15834.html that we looked at 3 weeks back, let's talk. I've been attempting to deep dive on it.
14:02 < jonatack__> The test is here: https://github.com/bitcoin/bitcoin/compare/master...sdaftuar:test-15834
14:03 < michaelfolkson> Sure I would
14:03 < peevsie> thanks all!