Replace unused BIP 9 logic with draft BIP 8 #19573
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
luke-jr
changed the title
|
Rebased and updated with some of @ajtowns 's changes to BIP 8. This is probably still not the final form of BIP 8 (see bitcoin/bips#950), but merging this sooner means we have fewer changes to backport together with the activation parameters later on. (It also means the RPC interfaces are updated now instead of later) |
There was a problem hiding this comment.
This has some bugs, see below.
I've done a branch with my suggestions at https://github.com/ajtowns/bitcoin/tree/202010-bip8-suggestions -- it does the following:
- makes the output of the versionbits unit test more helpful when it finds bugs
- updates the unit tests to check the "never active" case (which catches a bug in this PR)
- adapts your commits for the changes above, since there were merge conflicts
- has some explicit "fixups" for your commits fixing bugs or updating the changes above for your patches
- has a few refactors for versionbits and bip8 support that I think makes things a bit easier to follow
- has draft implementations for the open PRs against BIP8
|
Concept ACK. Since revised BIP 8 is a strict improvement on BIP 9 it doesn't hurt to replace BIP 9 logic with revised BIP 8 logic. AJ has highlighted some bugs, improvements that haven't been addressed yet. I also don't know if you are waiting for revised BIP 8 to be finalized at this point before a rebased version of this is ready to be merged. |
As thresholds are now parameterized, nRuleChangeActivationThreshold is no longer the threshold used for activating new rule changes. Instead it is now only used to warn if there is an unkonwn versionbits deployment. To make this clear, rename to m_vbits_min_threshold and update the comment describing it. Additionally, because this is just a minimum used for a warning, reduce the threshold to 75% so that future soft forks which may have thresholds lower than 95% will still have warnings.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
Co-authored-by: Luke Dashjr <luke-jr+git@utopios.org>
|
Rebased (CI failure appears unrelated) |
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
|
Note that we may still want to use this in the future. Merging #21377 now does not preclude the use of BIP 8 in a future deployment. |
|
Ok, let me know when to reopen, but I don't think this needs to stay in "High-priority for review" |
|
Agreed, it's probably a good idea to keep working on BIP 8 between now and the next soft fork attempt (especially the LOT=false part). That seems better than to develop these mechanisms only just in time. But in order to keep backporting easy, I suggest we don't merge a BIP 8 implementation until Speedy Trial expires or locks in. |
|
Thinking about it, it doesn't make sense to continue this PR here. This has LOT=True support and such, and what is needed right now is simply heights. When/if the folks impeding progress of BIP 8 change their mind, I can do that. But not sure there's any point rebasing until then. |
Since all BIP 9 deployments are buried, we can simply replace it with BIP 8 cleanly.