Bury bip9 deployments (consensus)


Host: jnewbery  -  PR author: jnewbery


  • 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

  113:00 <jnewbery> hi!
  213:00 <jonatack__> hi!
  313:00 <pinheadmz> Yo
  413:00 <kanzure> hi
  513:00 <davterra> hi
  613:00 <merehap> hey
  713:00 <peevsie> hi
  813:00 <sipa> comcept nack
  913:00 <sipa> :p
 1013:00 <jnewbery> welcome all (except sipa, who's trolling)
 1113:00 <schmidty> Hola
 1213:00 <lightlike> hi
 1313:00 <pinheadmz> comcept!
 1413:01 <jnewbery> did everyone have a chance to read the notes?
 1513:01 <b10c> hi
 1613:01 <michaelfolkson> Haha. Is he nacking this channel? Nack overruled
 1713:01 <kanzure> http://diyhpl.us/wiki/transcripts/bitcoin-core-dev-tech/2019-06-05-code-review/
 1813:02 <jnewbery> hi folks. We're here at a Bitcoin Core meeting in Amsterdam
 1913:02 <sipa> i'm on my phone, sitting next to john, mildly intoxicated
 2013:02 <jnewbery> We talked about the review process today. kanzure's transcript may be of interest to people here
 2113:03 <kanzure> sipa is intoxicated by code review
 2213:03 <kanzure> hours and hours of code review today.
 2313:03 <jonatack__> :D
 2413: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
 2513:04 <michaelfolkson> That transcript was really interesting. Hopefully people experiment with some of the ideas in it in future
 2613:04 <merehap> I think this channel has to be shutdown if sipa nacks it. Not really sure we have a choice here.
 2713:05 <michaelfolkson> Sorry John, please continue
 2813:05 <jnewbery> No, it's possible to NACK a sipa NACK to neutralize it
 2913:05 <jnewbery> it's a rarely used technique
 3013:05 <moneyball> sipa NACK
 3113:05 <xsb> snack
 3213:05 <jnewbery> did anyone have any questions about the PR?
 3313:06 <michaelfolkson> Yup
 3413:06 <jb55> what does it mean to bury a deployment
 3513:06 <jb55> start with something simple
 3613:06 <jnewbery> thanks jb55
 3713:07 <jnewbery> I think the terminology was introduced in BIP 90
 3813: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
 3913:07 <michaelfolkson> So it is effectively introducing a checkpoint right?
 4013:08 <jnewbery> the logic in the client that tests for activation (whether that's ISM or BIP9) is effectively obsolete
 4113:08 <merehap> Can LOCK(cs_main) be removed from IsNullDummyEnabled? It looks like it. It was already removed from the similar IsWitnessEnabled.
 4213:08 <jnewbery> so instead of relying on difficult-to-understand logic, we just hard-code the activation height
 4313:09 <pinheadmz> Does this make the bip9 signal bit reuseable? Or is that already true
 4413: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.
 4513:10 <jnewbery> but it's not a checkpoint. It doesn't limit the valid chain at that height to a certain block.
 4613:10 <michaelfolkson> Ah ok so a re-org is still possible before that block height but deactivating it at that block height becomes impossible
 4713:10 <peevsie> it's like a retroactive hardfork
 4813:10 <jonatack__> The "what" of any consensus change is interesting. Nevertheless, I found the pros and cons of "why" even more so.
 4913:11 <jonatack__> e.g. the discussion from here: https://github.com/bitcoin/bitcoin/pull/12360#issuecomment-383342462
 5013: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
 5113:12 <jnewbery> jonatack__: yes, I thought the discussion around whether such a change is desirable was more interesting than the code changes this week
 5213: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
 5313:12 <jonatack__> yes... the discussion between you, marco and suhas
 5413:13 <jnewbery> merehap: I think you may be right. You should leave a review comment!
 5513: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?
 5613:13 <merehap> jnewbery: Will do.
 5713: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
 5813:15 <jnewbery> michaelfolkson: yes, I closed the previous PR because I couldn't convince other contributors to review
 5913:15 <jnewbery> I think this change is useful, especially with the upcoming schnorr/taproot softfork changes
 6013:15 <michaelfolkson> Is there a certain time period that we would want since activation before "burying"?
 6113:16 <michaelfolkson> Or does it not matter? 6 months, 12 months, 18 months whatever?
 6213:17 <jnewbery> I think if Bitcoin experienced a 6 month re-org then we might question the usefulness of the system
 6313:17 <jnewbery> so I'd consider a softfork that's buried by 6 months' work as a historic fact
 6413:18 <pinheadmz> Where do you think that line is?
 6513:18 <pinheadmz> Like how big a reorg is really “were done let’s go home”
 6613:18 <jnewbery> I don't know!
 6713: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?
 6813:19 <michaelfolkson> The benefits of this are clearing up code complexity and testing right? Time of IBD isn't materially impacted
 6913: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
 7013:21 <jnewbery> I think p2p_segwit.py and p2p_compactblocks.py
 7113:21 <jnewbery> lightlike: I don't know if a BIP is required. I think a post to the mailing list is probably sufficient
 7213:23 <michaelfolkson> Is there a good resource explaining the different activation methods? I didn't know what flag day, IsSuperMajority were.
 7313:23 <jnewbery> I don't know if there's a single resource that describes them all
 7413:24 <jnewbery> Here's a bitmex blog post on the history of consensus changes: https://blog.bitmex.com/bitcoins-consensus-forks/
 7513: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
 7613:26 <michaelfolkson> Cool, thanks
 7713:26 <jonatack__> Kudos on being persistent. Taking note of the good idea to recap the ACKs from the previous PRs to rope in support.
 7813:27 <jnewbery> until then, the links to the BIPs in the notes describe all the activation methods
 7913:28 <michaelfolkson> Yeah... As a thought experiment from <kanzure> <moneyball> discussion is there anything to try to get this pushed through?
 8013:28 <jnewbery> thanks jonatack__. Nagging is an important skill to cultivate :)
 8113:29 <jnewbery> michaelfolkson: f2f nagging is my next tactic
 8213:29 <michaelfolkson> Assuming it is agreed that PR doesn't need splitting between RPC/consensus
 8313: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.
 8413:31 <jonatack__> f2f ?
 8513:31 <jnewbery> face to face
 8613:31 <jonatack__> ah!
 8713: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
 8813:32 <merehap> Awesome! Greatly appreciated.
 8913:32 <jonatack__> If we segue at some point to general questions, I have one about timestamping reviews.
 9013: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?
 9113:33 <sipa> it's not hardcoded to 432; it just has bip9 start time in the past
 9213: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
 9313:33 <sipa> so at block 144 (first period), it becomes active
 9413:33 <sipa> and blocm 288 it's locked in
 9513:34 <sipa> at block 432 it's active
 9613: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
 9713:36 <jnewbery> michaelfolkson: Discussion of the pros/cons of activation method is a bit too broad to be discussed here
 9813:36 <michaelfolkson> Oh no I mean in the blog post
 9913:37 <jnewbery> ah ok
10013:37 <jnewbery> jonatack__: go ahead with general questions
10113: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
10213:39 <jonatack__> MarcoFalke: how do you timestamp reviews like here https://github.com/bitcoin/bitcoin/pull/15988#issuecomment-491949275
10313:39 <jonatack__> to ensure GitHub doesn't change your review
10413:39 <jonatack__> idk if Marco is afk
10513:40 <jnewbery> MarcoFalke isn't here
10613:40 <jonatack__> if away at least he might see it
10713:40 <jonatack__> ok
10813:40 <jnewbery> but feel free to ask in #bitcoin-core-dev !
10913:41 <sipa> he's about 2.5 metets behind you, jnewbery
11013:41 <sipa> *meters
11113:41 <jonatack__> * suspense builds *
11213: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!
11313:42 <jnewbery> he's just charged his wine glass. I wouldn't want to interupt a man with opentimestamps questions in such a condition.
11413:44 <jnewbery> alright, any more questions?
11513: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
11613:44 <merehap> nclude null, -1, and max int here: "consensusParams.SegwitHeight != std::numeric_limits<int>::max()".
11713:45 <jnewbery> merehap: I wouldn't personally be interested in reviewing such a PR :)
11813: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)
11913:46 <jnewbery> The payoff doesn't seem worth the codechurn/review burden
12013:46 <merehap> Makes sense. Is that in general for Bitcoin Core, or more just for consensus critical stuff?
12113:47 <jnewbery> Mostly consensus-critical stuff, but in general too
12213: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.
12313:47 <merehap> Gotcha
12413:48 <sipa> morcos is applauding jnewbery's dedication to the cause
12513:48 <michaelfolkson> Let's wrap up. I feel we're delaying the drinking haha
12613: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.
12713: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.
12813:49 <jonatack__> Anyone against that idea please let me know ;)
12913:49 <michaelfolkson> Sounds good to me
13013:49 <jonatack__> Follow it on https://twitter.com/BitcoinCorePRs
13113: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
13213: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
13313:51 <michaelfolkson> Cool, thanks everyone, thanks John. Thanks <sipa> for your guest appearance
13413:51 <jonatack__> The bot could hook into the PR reviews club website to broadcast the meetings if that is ok
13513:52 <jnewbery> thanks jonatack__. I'm following!
13613:52 <jonatack__> Thanks!
13713:53 <jnewbery> Any other questions?
13813:53 <pinheadmz> cheers!
13913:53 <jnewbery> Did anyone build and test `getblockchaininfo` ?
14013:54 <lightlike> i did.
14113: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.
14213:55 <jnewbery> lightlike: great!
14313: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.
14413:57 <jnewbery> merehap: I think you're right thatCVE-2018-17144 can't be statically prevented.
14513:58 <jnewbery> I don't know how you'd catch it with testing
14613: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
14713: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.
14813:59 <merehap> Basically by encapsulating all the validation into an object, and passing that validated object around, you can avoid cases like the CVE.
14913:59 <merehap> But it's probably a bridge too far.
15013: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
15114:00 <jnewbery> merehap: seems very ambitious!
15214:00 <jnewbery> ok, let's wrap it up there
15314:00 <jnewbery> Thanks everyone! Same time again next week
15414:00 <merehap> Thanks jnewbery! Informative as always.
15514:01 <pinheadmz> have fun at 'Breaking!
15614:01 <jonatack__> Thanks jnewbery, sipa, michael and everyone!
15714:01 <lightlike> thanks!
15814:01 <merehap> Also thanks to those who took the rare step of overruling a sipa NACK so we could have this discussion.
15914: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.
16014:02 <jonatack__> The test is here: https://github.com/bitcoin/bitcoin/compare/master...sdaftuar:test-15834
16114:03 <michaelfolkson> Sure I would
16214:03 <peevsie> thanks all!