bip9: add speedy trial #1101

Sjors · 2021-04-12T16:38:18Z

Also recreates the state diagram and adds its source (based on BIP 8).

As an alternative to updating BIP 9 we could also make a new BIP from scratch, but that seems overkill since these changes are backwards compatible. When specifying activation parameters for future softforks like Taproot - if min_activation_height is used - we can point to this update. Clients that don't implement these softforks can safely ignore the changes.

maflcko · 2021-04-12T17:24:13Z

bip-0009.mediawiki

@@ -119,10 +123,7 @@ other one simultaneously transitions to STARTED, which would mean both would dem



This section is wrong. Also, the selection criteria above needs to be adjusted. See bitcoin/bitcoin#21377 (comment)

maflcko · 2021-04-12T17:24:54Z

bip-0009.mediawiki

@@ -229,6 +235,10 @@ upgrades for successful soft forks.



The threshold needs to be changed as well see bitcoin/bitcoin#21377 (comment)

It's not clear to me if the speedy trial permanently reduces the threshold, or if this was just a lazy way to set it to 90% for the trial itself. BIP 9 already allows for a lower threshold.

maflcko · 2021-04-12T17:28:01Z

bip-0009/states.dot

+  node [style="rounded,filled,bold", shape=box, fixedsize=true, width=1.5, fontname="Arial"];
+
+  edge [weight = 100];
+  "DEFINED" -> "STARTED" [label="Always"];


This is wrong, too. The correct state diagram is at bitcoin/bitcoin#21377 (comment) (Edit: wrong link, see below)

I took over most of the diagram, but left out the DELAYED pseudo-state. I also clarified that you can transition from STARTED to LOCKED_IN after the timeout (but only if it reaches the threshold in the first period). I'm not sure if this edge case is worth explaining, since AFAIK it's just there to make fuzzing easier? cc @ajtowns

bitcoin/bitcoin#21377 (comment) is the current state diagram

I think I was only missing height + 1,so added that. Also expanded the text a bit.

michaelfolkson · 2021-04-13T11:53:31Z

For the record I disapproved of revising BIP 9 back on March 17th and I'm not exactly impressed today. I can only speak for myself though and not having a BIP (or at least a pull request such as this one) to refer to for the latest state diagram of #21377 of Speedy Trial is not healthy for the review of #21377.

I think the intention here is to make BIP 9 into a generalized activation BIP (like BIP 8 currently is) with multiple parameters and incorporating multiple activation mechanisms, the only parameter missing of course being the LOT parameter. And using this mix of MTP and block height for Speedy Trial instead of using block height consistently.

I prefer a new BIP number for Speedy Trial personally if people are going to go to these lengths to avoid using BIP 8. BIP 9 has a lot of history and so it can't be a fresh exclusively Speedy Trial BIP. But then having two flavors of Speedy Trial (BIP 8 and BIP 9 Speedy Trial) is nonsense. Someone will need to explain the differences between BIP 8 and BIP 9 for future soft forks (good luck to whoever tries that). I honestly don't know what some people are doing anymore. The amount of time wasted on this BIP 8 vs BIP 9 and block height vs mix of block height and MTP just appalls me.

Again I've said all this on many occasions (1, 2).

Sjors · 2021-04-13T12:17:28Z

the only parameter missing of course being the LOT parameter

I think BIP 8 is a better place to introduce a more drastic change like LOT. The main thing this PR introduces is a longer LOCKIN phase. The rest is cosmetics.

BIP 9 has a lot of history

Yes, but these changes are backwards compatible, as explained in this PR. Since BIP 8 is entirely height based (which I also prefer) implementing it in existing clients is much more involved. The fact that min_activation_height makes BIP 9 a weird mix of time and height is easier in the implementation than it might sound.

Someone will need to explain the differences between BIP 8 and BIP 9 for future soft forks (good luck to whoever tries that).

Why would that be more difficult in the future than it is now? "We're ditching time based in favor of block based, that's all" would be the explanation.

The amount of time wasted on this BIP 8 vs BIP 9 and block height vs mix of block height and MTP just appalls me.

I still favor a height based approach, i.e. speedy trial with BIP 8, or just a regular BIP 8 deployment with LOT=false (no delayed activation). But given that even the author of the BIP 8 PR switched his effort to BIP 9, the most useful thing for me to do for is just to review and document a BIP 9 speedy trial.

One final point: it's still possible to refactor the BIP 9 speedy trial code to a BIP 8 speedy trial. The changes needed to switch from BIP 9 to BIP 8, which have already been coded up, can be mostly reused for that. The use of min_activation_height (rather than time) in fact makes that easier.

jonatack

I'm starting to go through the state machine logic. Some typo fixups below.

bip-0009.mediawiki

jonatack · 2021-04-13T14:41:25Z

bip-0009.mediawiki

+==2021 update==
+
+* '''min_activation_height''' was added. Clients unaware of softforks introduced after SegWit can ignore this in the context of their upgrade warnings.
+* removed state transition from '''defined''' to '''failed'''. This change is safe to ignore for all (burried) softforks deployed before 2021.


BIP-wide: use either "soft fork" or "softfork", but maybe not both

Or soft-fork? It's already inconsistent. I might just add a commit that picks one and fixes that for the existing text.

When in doubt, I consult https://github.com/bitcoinops/bitcoinops.github.io/blob/master/STYLE.md (which says to use "soft fork")

bip-0009.mediawiki

ajtowns

Some serious errors here...

I think that it's correct to say that the changes are purely soft-forks for any soft-fork activation attempt under the old rules -- some conceivable chains might switch from FAILED to ACTIVE, but none that were ACTIVE would switch to any other state. And all of csv, segwit and bip 91 did reach the ACTIVE state so even if they weren't considered buried, interpretation shouldn't change.

FWIW, I'm not convinced there's a need to have this as a separate bip rather than just documenting the delta in the taproot bip directly -- that should only be necessary if we wanted to reuse exactly this activation method again in the future, in which case having a separate bip to refer to would be helpful. That seems pretty unlikely. I have an update to bip341 that documents the activation details drafted at https://github.com/ajtowns/bips/blob/202103-bip341-speedy-trial-mtp/bip-0341.mediawiki#Deployment that I was aiming to PR today, but I would really rather avoid having another set of competing PRs on this topic. LMK if you want to persevere with this or would rather switch to that.

[EDIT: opened as #1104]

ajtowns · 2021-04-13T14:45:37Z

bip-0009.mediawiki


        case LOCKED_IN:
-            return ACTIVE;
+            if (block.height + 1 >= min_activation_height)


This isn't correct -- bip 9's "block" is the first block in the retarget period, while versionbits.cpp's pindexPrev is the last block in the prior retarget period, so this should be block.height >= min_activation_height.

Thanks, fixed and added your clarification from below.

bip-0009.mediawiki

ajtowns · 2021-04-13T14:54:12Z

bip-0009/states.dot

+  "DEFINED" -> "STARTED" [label="MTP >= begin"];
+  "STARTED" -> "FAILED" [label="nsig < thresh\nMTP >= timeout"];
+  "LOCKED_IN" -> "LOCKED_IN" [label="height + 1 < min_height"];
+  "LOCKED_IN" -> "ACTIVE" [label="height + 1 >= min_height"];


This probably needs to be clear that "MTP" is calculated against the last block of the previous period, while "height+1" is the height of the first block of the new period too.

Added a comment below the figure, let me know if I got it right this time...

bip-0009/states.dot

bip-0009.mediawiki

achow101 · 2021-04-13T16:39:03Z

Tend to NACK. Procedurally, BIP 9 is in Final status which implies that it cannot be modified. Additionally, I consider the state machine changes to be a significant change to BIP 9, although the actual code changes themselves are small. Given those two facts, I don't think that we should be modifying BIP 9 itself to include speedy trial. Rather I think that a new BIP should be written which specifies speedy trial, the state machine changes, and the proposed LAST_CHANCE state for UASFs. I think that for now, it would be okay to write the speedy trial modifications in BIPs 341/342, but I would prefer that a new document is drafted for the full MTP versionbits methodology that has been proposed. This new document does not need to be a full BIP that describes everything from scratch (like BIP 8 and 9 do) but rather one which refers to BIP 9 and provides additional modifications on top of it (in a similar way to BIP 350 specifying the changes from BIP 173).

harding · 2021-04-13T16:40:29Z

I'm not a fan of making such significant changes to a Status: final BIP. I think @ajtowns's approach of including the delta to BIP341 is much more congruent with the normal BIP process.

Sjors · 2021-04-13T19:08:32Z

I think it's very confusing for anyone trying to implement BIP 9 from scratch to have to look at this BIP and then look in BIP 341 for changes. Most likely they would start with reading this BIP and then look at the Bitcoin Core code, only to find it doesn't match in strange ways.

Any client that already implements BIP 9 will have to make changes in order to correctly detect the Taproot fork. For them it probably doesn't matter where the info is, but this seems a more natural spot.

It seems exceedingly unlikely to me that if we activate Taproot with this, that the next soft fork would use vanilla BIP 9. And it makes no sense in e.g. a ANYPREVOUT softfork to have to point to Taproot documentation. Even if the speedy trial doesn't activate, I doubt we'll revert the changes in Bitcoin Core.

Hence my preference for updating this.

My second preferred solution would be a fresh BIP for the activation (rather than mixing it in with Taproot specific stuff).

Rspigler · 2021-04-13T20:20:58Z

I'm not convinced there's a need to have this as a separate bip rather than just documenting the delta in the taproot bip directly -- that should only be necessary if we wanted to reuse exactly this activation method again in the future,

@ajtowns I'm confused by this. On IRC wasn't the reason for going with MTP because future soft forks will use ST, and doing so will make testing easier? Otherwise, safety and predictability favored height/BIP8

ajtowns · 2021-04-14T01:36:08Z

@ajtowns I'm confused by this. On IRC wasn't the reason for going with MTP because future soft forks will use ST, and doing so will make testing easier? Otherwise, safety and predictability favored height/BIP8

I think the next soft fork attempt (which might be taproot again if ST fails to activate) would want to make at least two changes compared to ST as it stands: including some form of UASF mechanism (eg, LAST_CHANCE and mandatory signalling), and using MTP rather than height for the delayed activation, presuming we can come up with a sensible way of doing that (eg addressing issues like the one Russell raised). It might be good for such a BIP to also document how activations actually occur for signet in order to test new soft forks.

luke-jr · 2021-04-22T22:58:27Z

Closing this since BIP 9 is already Final status.

buildtable: allow Updated field

c1fb9f0

Sjors force-pushed the 2021/04/bip9_state branch 2 times, most recently from b04b049 to 4dc59a7 Compare April 12, 2021 16:53

maflcko reviewed Apr 12, 2021

View reviewed changes

Sjors force-pushed the 2021/04/bip9_state branch 5 times, most recently from 0c98c0f to 6cd6b75 Compare April 13, 2021 11:30

jonatack reviewed Apr 13, 2021

View reviewed changes

ajtowns suggested changes Apr 13, 2021

View reviewed changes

Sjors force-pushed the 2021/04/bip9_state branch from 6cd6b75 to 74d9008 Compare April 13, 2021 19:07

bip9: add speedy trial

79de570

Sjors force-pushed the 2021/04/bip9_state branch from 74d9008 to 79de570 Compare April 13, 2021 19:12

Sjors mentioned this pull request Apr 13, 2021

BIP341: speedy trial activation parameters #1104

Merged

luke-jr added the Proposed BIP modification label Apr 22, 2021

luke-jr closed this Apr 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bip9: add speedy trial #1101

bip9: add speedy trial #1101

Sjors commented Apr 12, 2021 •

edited

maflcko Apr 12, 2021

maflcko Apr 12, 2021

Sjors Apr 12, 2021

maflcko Apr 12, 2021 •

edited

Sjors Apr 12, 2021 •

edited

ajtowns Apr 13, 2021

Sjors Apr 13, 2021

michaelfolkson commented Apr 13, 2021

Sjors commented Apr 13, 2021 •

edited

jonatack left a comment •

edited

jonatack Apr 13, 2021

Sjors Apr 13, 2021 •

edited

jonatack Apr 13, 2021 •

edited

ajtowns left a comment •

edited

ajtowns Apr 13, 2021

Sjors Apr 13, 2021

ajtowns Apr 13, 2021

Sjors Apr 13, 2021

achow101 commented Apr 13, 2021

harding commented Apr 13, 2021

Sjors commented Apr 13, 2021 •

edited

Rspigler commented Apr 13, 2021

ajtowns commented Apr 14, 2021 •

edited

luke-jr commented Apr 22, 2021

		@@ -119,10 +123,7 @@ other one simultaneously transitions to STARTED, which would mean both would dem

bip9: add speedy trial #1101

bip9: add speedy trial #1101

Conversation

Sjors commented Apr 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko Apr 12, 2021 • edited

Choose a reason for hiding this comment

Sjors Apr 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelfolkson commented Apr 13, 2021

Sjors commented Apr 13, 2021 • edited

jonatack left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors Apr 13, 2021 • edited

Choose a reason for hiding this comment

jonatack Apr 13, 2021 • edited

Choose a reason for hiding this comment

ajtowns left a comment • edited

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

achow101 commented Apr 13, 2021

harding commented Apr 13, 2021

Sjors commented Apr 13, 2021 • edited

Rspigler commented Apr 13, 2021

ajtowns commented Apr 14, 2021 • edited

luke-jr commented Apr 22, 2021

Sjors commented Apr 12, 2021 •

edited

maflcko Apr 12, 2021 •

edited

Sjors Apr 12, 2021 •

edited

Sjors commented Apr 13, 2021 •

edited

jonatack left a comment •

edited

Sjors Apr 13, 2021 •

edited

jonatack Apr 13, 2021 •

edited

ajtowns left a comment •

edited

Sjors commented Apr 13, 2021 •

edited

ajtowns commented Apr 14, 2021 •

edited