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.
122 | @@ -119,10 +123,7 @@ other one simultaneously transitions to STARTED, which would mean both would dem
123 |
This section is wrong. Also, the selection criteria above needs to be adjusted. See https://github.com/bitcoin/bitcoin/pull/21377#discussion_r611492633
234 | @@ -229,6 +235,10 @@ upgrades for successful soft forks.
235 |
The threshold needs to be changed as well see https://github.com/bitcoin/bitcoin/pull/21377#discussion_r611493576
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.
0 | @@ -0,0 +1,29 @@ 1 | +digraph { 2 | + rankdir=TD; 3 | + 4 | + node [style="rounded,filled,bold", shape=box, fixedsize=true, width=1.5, fontname="Arial"]; 5 | + 6 | + edge [weight = 100]; 7 | + "DEFINED" -> "STARTED" [label="Always"];
This is wrong, too. The correct state diagram is at https://github.com/bitcoin/bitcoin/pull/21377#issuecomment-812023131 (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
https://github.com/bitcoin/bitcoin/pull/21377#issuecomment-814768715 is the current state diagram
I think I was only missing height + 1,so added that. Also expanded the text a bit.
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.
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.
27 | @@ -23,6 +28,11 @@ BIP 34 introduced a mechanism for doing soft-forking changes without a predefine 28 | 29 | In addition, BIP 34 made the integer comparison (nVersion >= 2) a consensus rule after its 95% threshold was reached, removing 2<sup>31</sup>+2 values from the set of valid version numbers (all negative numbers, as nVersion is interpreted as a signed integer, as well as 0 and 1). This indicates another downside this approach: every upgrade permanently restricts the set of allowed nVersion field values. This approach was later reused in BIP 66 and BIP 65, which further removed nVersions 2 and 3 as valid options. As will be shown further, this is unnecessary. 30 | 31 | +==2021 update== 32 | + 33 | +* '''min_activation_height''' was added. Clients unaware of softforks introduced after SegWit can ignore this in the context of their upgrade warnings. 34 | +* removed state transition from '''defined''' to '''failed'''. This change is safe to ignore for all (burried) softforks deployed before 2021.
s/burried/buried/
BIP-wide: use either "soft fork" or "softfork", but maybe not both
The STARTED to LOCKED_IN/FAILED transitions also change. Everywhere else in the bip capitalizes the state names as well.
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")
27 | @@ -23,6 +28,11 @@ BIP 34 introduced a mechanism for doing soft-forking changes without a predefine 28 | 29 | In addition, BIP 34 made the integer comparison (nVersion >= 2) a consensus rule after its 95% threshold was reached, removing 2<sup>31</sup>+2 values from the set of valid version numbers (all negative numbers, as nVersion is interpreted as a signed integer, as well as 0 and 1). This indicates another downside this approach: every upgrade permanently restricts the set of allowed nVersion field values. This approach was later reused in BIP 66 and BIP 65, which further removed nVersions 2 and 3 as valid options. As will be shown further, this is unnecessary. 30 | 31 | +==2021 update== 32 | + 33 | +* '''min_activation_height''' was added. Clients unaware of softforks introduced after SegWit can ignore this in the context of their upgrade warnings.
s/Segwit/segwit/ per most frequent use in the BIPs repo
50 | @@ -40,6 +51,7 @@ The following guidelines are suggested for selecting these parameters for a soft 51 | # '''bit''' should be selected such that no two concurrent softforks use the same bit. 52 | # '''starttime''' should be set to some date in the future, approximately one month after a software release date including the soft fork. This allows for some release delays, while preventing triggers as a result of parties running pre-release software. 53 | # '''timeout''' should be 1 year (31536000 seconds) after starttime. 54 | +# '''min_activation_height''' should be 0 if the recommendation for '''starttime''' is followed
# '''min_activation_height''' should be 0 if the recommendation for '''starttime''' is followed.
124 | - return DEFINED; 125 | + return STARTED; 126 | 127 | -After a period in the STARTED state, if we're past the timeout, we switch to FAILED. If not, we tally the bits set, 128 | -and transition to LOCKED_IN if a sufficient number of blocks in the past period set the deployment bit in their 129 | +After a period in the STARTED state we tally the bits set, and transition to LOCKED_IN
After a period in the STARTED state, we tally the bits set and transition to LOCKED_IN
or can keep the second comma if a subject is added before "transition"
146 | -After a retarget period of LOCKED_IN, we automatically transition to ACTIVE. 147 | +After one or more retarget periods of LOCKED_IN, if the activation height has been reached, we transition to ACTIVE 148 | 149 | case LOCKED_IN: 150 | - return ACTIVE; 151 | + 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.
130 | +if a sufficient number of blocks in the past period set the deployment bit in their 131 | version numbers. The threshold is ≥1916 blocks (95% of 2016), or ≥1512 for testnet (75% of 2016). 132 | -The transition to FAILED takes precedence, as otherwise an ambiguity can arise. 133 | -There could be two non-overlapping deployments on the same bit, where the first one transitions to LOCKED_IN while the 134 | -other one simultaneously transitions to STARTED, which would mean both would demand setting the bit. 135 | +Note that if the first period meets the signalling threshold, if will transition
Note that if the first period meets the signalling threshold, it will transition
120 | - } 121 | - if (GetMedianTimePast(block.parent) >= starttime) { 122 | - return STARTED; 123 | - } 124 | - return DEFINED; 125 | + return STARTED;
Dropping the "GetMedianTimePast >= starttime" doesn't match the implementation. Doing it this way would mean that every deployment enters STARTED in the second retarget period.
142 | + return FAILED; 143 | } 144 | return STARTED; 145 | 146 | -After a retarget period of LOCKED_IN, we automatically transition to ACTIVE. 147 | +After one or more retarget periods of LOCKED_IN, if the activation height has been reached, we transition to ACTIVE
After one or more retarget periods of LOCKED_IN, if the activation height has been reached, we transition to ACTIVE.
I'm starting to go through the state machine logic. Some typo fixups below.
5 | + 6 | + edge [weight = 100]; 7 | + "DEFINED" -> "STARTED" [label="MTP >= begin"]; 8 | + "STARTED" -> "FAILED" [label="nsig < thresh\nMTP >= timeout"]; 9 | + "LOCKED_IN" -> "LOCKED_IN" [label="height + 1 < min_height"]; 10 | + "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...
13 | + "STARTED" -> "LOCKED_IN" [label="nsig >= thresh"]; 14 | + 15 | + "FAILED" -> "LOCKED_IN" [style=invis]; 16 | + 17 | + "DEFINED"-> "DEFINED" [label="MTP < begin"]; 18 | + "STARTED"-> "STARTED" [label="nsig < thresh\nMTP < timeout"];
Probably best to use "count" and "threshold" rather than "nsig" and "thresh" to match the BIP text. Likewise "starttime" instead of "begin" and "min_activation_height" instead of "min_height".
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]
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).
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).
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 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.
Closing this since BIP 9 is already Final status.