bip9: add speedy trial #1101

pull Sjors wants to merge 2 commits into bitcoin:master from Sjors:2021/04/bip9_state changing 6 files +69 −16
  1. Sjors commented at 4:38 pm on April 12, 2021: member

    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.

  2. buildtable: allow Updated field c1fb9f0b9a
  3. Sjors force-pushed on Apr 12, 2021
  4. Sjors force-pushed on Apr 12, 2021
  5. in bip-0009.mediawiki:133 in 4dc59a7411 outdated
    122@@ -119,10 +123,7 @@ other one simultaneously transitions to STARTED, which would mean both would dem
    123 
    


    MarcoFalke commented at 5:24 pm on April 12, 2021:
    This section is wrong. Also, the selection criteria above needs to be adjusted. See https://github.com/bitcoin/bitcoin/pull/21377#discussion_r611492633
  6. in bip-0009.mediawiki:245 in 4dc59a7411 outdated
    234@@ -229,6 +235,10 @@ upgrades for successful soft forks.
    235 
    


    MarcoFalke commented at 5:24 pm on April 12, 2021:
    The threshold needs to be changed as well see https://github.com/bitcoin/bitcoin/pull/21377#discussion_r611493576

    Sjors commented at 6:56 pm on April 12, 2021:
    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.
  7. in bip-0009/states.dot:7 in 4dc59a7411 outdated
    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"];
    


    MarcoFalke commented at 5:28 pm on April 12, 2021:
    This is wrong, too. The correct state diagram is at https://github.com/bitcoin/bitcoin/pull/21377#issuecomment-812023131 (Edit: wrong link, see below)

    Sjors commented at 7:24 pm on April 12, 2021:
    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

    ajtowns commented at 0:12 am on April 13, 2021:

    Sjors commented at 11:30 am on April 13, 2021:
    I think I was only missing height + 1,so added that. Also expanded the text a bit.
  8. Sjors force-pushed on Apr 12, 2021
  9. Sjors force-pushed on Apr 12, 2021
  10. Sjors force-pushed on Apr 12, 2021
  11. Sjors force-pushed on Apr 12, 2021
  12. Sjors force-pushed on Apr 13, 2021
  13. michaelfolkson commented at 11:53 am on April 13, 2021: contributor

    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).

  14. Sjors commented at 12:17 pm on April 13, 2021: member

    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.

  15. in bip-0009.mediawiki:34 in 6cd6b757ff outdated
    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.
    


    jonatack commented at 2:39 pm on April 13, 2021:
    s/burried/buried/

    jonatack commented at 2:41 pm on April 13, 2021:
    BIP-wide: use either “soft fork” or “softfork”, but maybe not both

    ajtowns commented at 3:07 pm on April 13, 2021:
    The STARTED to LOCKED_IN/FAILED transitions also change. Everywhere else in the bip capitalizes the state names as well.

    Sjors commented at 6:42 pm on April 13, 2021:
    Or soft-fork? It’s already inconsistent. I might just add a commit that picks one and fixes that for the existing text.

    jonatack commented at 6:48 pm on April 13, 2021:
    When in doubt, I consult https://github.com/bitcoinops/bitcoinops.github.io/blob/master/STYLE.md (which says to use “soft fork”)
  16. in bip-0009.mediawiki:33 in 6cd6b757ff outdated
    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.
    


    jonatack commented at 2:42 pm on April 13, 2021:
    s/Segwit/segwit/ per most frequent use in the BIPs repo
  17. in bip-0009.mediawiki:54 in 6cd6b757ff outdated
    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
    


    jonatack commented at 2:43 pm on April 13, 2021:
    0# '''min_activation_height''' should be 0 if the recommendation for '''starttime''' is followed.
    
  18. in bip-0009.mediawiki:119 in 6cd6b757ff outdated
    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
    


    jonatack commented at 2:44 pm on April 13, 2021:
    0After 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”

  19. in bip-0009.mediawiki:149 in 6cd6b757ff outdated
    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)
    


    ajtowns commented at 2:45 pm on April 13, 2021:
    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.

    Sjors commented at 6:58 pm on April 13, 2021:
    Thanks, fixed and added your clarification from below.
  20. in bip-0009.mediawiki:122 in 6cd6b757ff outdated
    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
    


    jonatack commented at 2:46 pm on April 13, 2021:
    0Note that if the first period meets the signalling threshold, it will transition
    
  21. in bip-0009.mediawiki:117 in 6cd6b757ff outdated
    120-            }
    121-            if (GetMedianTimePast(block.parent) >= starttime) {
    122-                return STARTED;
    123-            }
    124-            return DEFINED;
    125+            return STARTED;
    


    ajtowns commented at 2:47 pm on April 13, 2021:
    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.
  22. in bip-0009.mediawiki:146 in 6cd6b757ff outdated
    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
    


    jonatack commented at 2:51 pm on April 13, 2021:
    0After one or more retarget periods of LOCKED_IN, if the activation height has been reached, we transition to ACTIVE.
    
  23. jonatack commented at 2:53 pm on April 13, 2021: contributor
    I’m starting to go through the state machine logic. Some typo fixups below.
  24. in bip-0009/states.dot:10 in 6cd6b757ff outdated
     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"];
    


    ajtowns commented at 2:54 pm on April 13, 2021:
    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.

    Sjors commented at 7:04 pm on April 13, 2021:
    Added a comment below the figure, let me know if I got it right this time…
  25. in bip-0009/states.dot:18 in 6cd6b757ff outdated
    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"];
    


    ajtowns commented at 2:56 pm on April 13, 2021:
    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”.
  26. ajtowns changes_requested
  27. ajtowns commented at 3:17 pm on April 13, 2021: contributor

    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]

  28. achow101 commented at 4:39 pm on April 13, 2021: member
    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).
  29. harding commented at 4:40 pm on April 13, 2021: contributor
    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.
  30. Sjors force-pushed on Apr 13, 2021
  31. Sjors commented at 7:08 pm on April 13, 2021: member

    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).

  32. bip9: add speedy trial 79de570b7d
  33. Sjors force-pushed on Apr 13, 2021
  34. Sjors cross-referenced this on Apr 13, 2021 from issue BIP341: speedy trial activation parameters by ajtowns
  35. Rspigler commented at 8:20 pm on April 13, 2021: contributor

    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

  36. ajtowns commented at 1:36 am on April 14, 2021: contributor

    @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.

  37. luke-jr added the label Proposed BIP modification on Apr 22, 2021
  38. luke-jr commented at 10:58 pm on April 22, 2021: member
    Closing this since BIP 9 is already Final status.
  39. luke-jr closed this on Apr 22, 2021


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bips. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 03:10 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me