#16060 Bury bip9 deployments (consensus)
- 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
- Review/test of this PR should include verifying that the hard-coded activation heights are correct.
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!