Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speedy trial support for versionbits #21377

Merged
merged 9 commits into from
Apr 15, 2021

Conversation

ajtowns
Copy link
Contributor

@ajtowns ajtowns commented Mar 6, 2021

BIP9-based implementation of "speedy trial" activation specification, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-March/018583.html

Edge cases are tested by fuzzing added in #21380.

@evoskuil
Copy link

evoskuil commented Mar 6, 2021

+1

@jonatack
Copy link
Member

jonatack commented Mar 6, 2021

Concept ACK

1 similar comment
@michaelfolkson
Copy link

Concept ACK

@roconnor-blockstream
Copy link
Contributor

I take it that this PR doesn't preclude also migrating to BIP8-style height based activation?

@benthecarman
Copy link
Contributor

I take it that this PR doesn't preclude also migrating to BIP8-style height based activation?

Concept ACK but would much rather have height based activation

@JeremyRubin
Copy link
Contributor

Concept ACK.

It does seem like there is a preference for height based activation as opposed to time.

i'd further request that -- to the extent this is getting BIP'd, that the activation date must not be within the range of the provided interval or we introduce some other sort of quantization / min interval parameter (e.g., every 3 block-months there is a new-rule activation window and we use the next one, or the activation blocks is something like current + max(2016*N, earliest - current).

@michaelfolkson
Copy link

michaelfolkson commented Mar 6, 2021

I don't know (though I can guess) why people are trying to refer to BIP 9 now instead of BIP 8, especially as height based activation seems the superior option.

As a reminder, there have been community IRC meetings (which many Core contributors attended and disclosed their views) where there was broad consensus on using revised BIP 8 e.g. http://gnusha.org/taproot-activation/2021-02-02.log

Rusty's conclusion post that meeting was "Unanimous support for BIP 8. RIP BIP 9."

The outline of the various proposals used BIP 8 rather than BIP 9: https://en.bitcoin.it/wiki/Taproot_activation_proposals

And various media articles reporting on the discussion referred to BIP 8 rather than BIP 9.

Though I don't expect it to scupper the momentum this promising proposal is generating if people want to call this BIP 9 (or do mental gymnastics by opening a new BIP) please be clear on why it doesn't fit within the template of BIP 8.

Other than that, I am delighted with the progress this proposal is making and having a PR up in such quick time really is great, so many thanks for your work on this.

edit: Thinking about this more I don't think it would be mental gymnastics to open a new BIP for "Speedy trial" especially if it doesn't fit cleanly into a more flexible BIP 8. New BIP or more flexible BIP 8. BIP 9 is dead and if "Speedy trial" could be used for other soft forks it shouldn't be included in the Taproot BIPs.

@roconnor-blockstream
Copy link
Contributor

I believe the best way to proceed would be to rebase a BIP8-height PR on top of this PR. Once this PR is merged, we have the ability to produce an acceptable set of parameters for speedy trial. If then the subsequent BIP8-height PR is merged afterwards then we get, what I believe to be, a superior set of activation parameters, but if the BIP8-height subsequent PR fails for whatever reason, that is okay.

I'm anticipating test cases added to this PR that would in turn be valuable for the BIP8-height PR, and the BIP8-height speedy trial will need an analogous activation_height field as well.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 6, 2021

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, 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.

@JeremyRubin
Copy link
Contributor

@roconnor-blockstream that sounds like one reasonable plan of attack -- merging this now is a concrete step, patching to use height can occur independently. I do fear there would not be consensus on a non-height based activation proposal, so it may be important to not rest on laurels for accepting those changes.

Making the case for BIP8 merge first then these changes, the BIP8 code is much more reviewed presently (with no code nacks, just conceptual ones?) and it may be a better utilization of developer time to rebase this onto BIP8.

I think it would be best to hear from @luke-jr and @ajtowns on their preference as they seem to be primary contributors on the implementation/review of both.

@benthecarman
Copy link
Contributor

Once this PR is merged, we have the ability to produce an acceptable set of parameters for speedy trial. If then the subsequent BIP8-height PR is merged afterwards

This seems dangerous as we could have people compile and start running after the activation params are merged, merging a following change to the params could cause people to be running different activation logic (one time based, one hieght based).

@roconnor-blockstream
Copy link
Contributor

@benthecarman. Not to worry this PR doesn't have activation parameters in it. I would expect merger of activation parameters to be delayed until after a BIP8-height PR is done consideration.

That being said, I've chatted a bit with @achow101 and it may be the case that a BIP8-min-activation-height PR ends up being easier to implement as a stand alone PR rather than on top of this one. There are issues with merging, and backporting etc. So I'm going to stop myself from further back-seat deving and let the pros figure out the best way to hammer out PRs.

@ajtowns
Copy link
Contributor Author

ajtowns commented Mar 6, 2021

Making the case for BIP8 merge first then these changes, the BIP8 code is much more reviewed presently (with no code nacks, just conceptual ones?) and it may be a better utilization of developer time to rebase this onto BIP8.

I don't think the BIP8 code is adequately reviewed yet -- we had an off-by-one bug that would have left lot=true and lot=false nodes out of consensus found this past week, that wasn't discovered by reviewers or the automated tests. (Currently it isn't a clean merge either)

The main benefits of bip8 are lockinontimeout=true related -- that it specifies the option at all, and that it's height based, so loss of hashpower (due to miners preferring a chain that is invalid according to lockinontimeout=true) doesn't reduce the number of retarget periods available for activation. So I don't think there's any real benefits to doing this via bip8 rather than bip9 as far as the "Speedy trial" itself goes; but if bip8/height based code is thoroughly reviewed and ready to merge prior to setting activation parameters, that seems fine too as far as I can see.

@michaelfolkson
Copy link

michaelfolkson commented Mar 6, 2021

I don't think the BIP8 code is adequately reviewed yet -- we had an off-by-one bug that would have left lot=true and lot=false nodes out of consensus found this past week, that wasn't discovered by reviewers or the automated tests.

@ajtowns: Are you referring to a bug Sjors found in the tests? That is hardly a consensus bug for LOT=true and LOT=false nodes on mainnet but you are right to say it wasn't yet ready to be merged. I have since recommended it be closed as some have expressed that they would not merge it with any LOT code (even if dead code).

@ajtowns
Copy link
Contributor Author

ajtowns commented Mar 6, 2021

@michaelfolkson No, that one wasn't a bug. I'm referring to this one.

@harding
Copy link
Contributor

harding commented Mar 7, 2021

Concept ACK. Thank you, @ajtowns! I'm happy to see that the change really is minimal.

Do I understand correctly from reading the code that activation will occur in the first block of the next retarget period after the activation_time? E.g., if the minimum activation time just passed but the height is 1234, the first block where taproot's rules are enforced still won't be until height 2016? That seems fine to me for the sake of code simplicity, but it does mean that, if it looks like activation_time is going to fall near a retarget block, we'll have to communicate that taproot could either happen tomorrow or two weeks from now. Maybe to avoid that communication challenge, it'd be better to use a height?

@michaelfolkson

I don't know (though I can guess) why people are trying to refer to BIP 9 now instead of BIP 8, especially as height based activation seems the superior option.

As mentioned in my email, I thought basing the proposal on the existing BIP9 code in Bitcoin Core (as done in this PR) would be the smallest change and so fastest to get reviewed, merged, and released. Since the goal of the Speedy Trial proposal is speed (with safety), that seems highly advantageous to me.

I completely agree that using block heights for the start and timeout parameters has advantages for this proposal (giving miners a known number of signaling periods) and that, as also mentioned in my email, getting BIP8 included in the code now would have advantages if Speedy Trial fails and we decide to use BIP8 as originally imagined for a follow-up activation attempt.

My preference would be for whatever solution is most preferred by reviewers.

@ghost
Copy link

ghost commented Mar 7, 2021

concept ACK.

@@ -80,7 +80,7 @@ class CMainParams : public CChainParams {
consensus.nPowTargetSpacing = 10 * 60;
consensus.fPowAllowMinDifficultyBlocks = false;
consensus.fPowNoRetargeting = false;
consensus.nRuleChangeActivationThreshold = 1916; // 95% of 2016
consensus.nRuleChangeActivationThreshold = 1815; // 90% of 2016
Copy link
Member

@Sjors Sjors Mar 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a note here to never change this unless all soft forks are burried. Alternatively maybe we should have a separate variable for this speedy trial.

Copy link
Member

@maflcko maflcko Mar 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be fixed (in a follow-up refactor) with commit 2be730c + commit 50eb7f0

@Sjors
Copy link
Member

Sjors commented Mar 7, 2021

Concept ACK for speedy trial support. However I'm not convinced we should permanently drop the BIP9 threshold to 90%.

@JeremyRubin wrote:

merging this now is a concrete step, patching to use height can occur independently

I'm fine with a height based approach too; slightly more code changes, but it seems easier to communicate when things happen. I also agree with Jeremy's point. Nothing here is set in stone until we actually apply it to Taproot (or another soft fork).

There is one downside though, in addition to more review work: other implementations probably already have BIP 9 code ready to go. Although compared to implementing all of Taproot this is not a big deal (and without implementing Taproot they can't verify the new consensus rules anyway).

Making the case for BIP8 merge first then these changes, the BIP8 code is much more reviewed presently (with no code nacks, just conceptual ones?)

For me that's only an option if the LOT=true stuff is stripped from that PR.

It doesn't seem terribly complicated to cherry-pick 9762562, bcec418 (though I suggest a fresh BIP for the speedy trial) and a few more.

@@ -29,6 +29,8 @@ struct BIP9Deployment {
int64_t nStartTime;
/** Timeout/expiry MedianTime for the deployment attempt. */
int64_t nTimeout;
/** If lock in occurs, delay activation until at least this time. */
int64_t activation_time;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add reduced_threshold which, if non-zero, overrides nRuleChangeActivationThreshold.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can worry about genericity if we ever need to add 2 simultaneous vbit releases... which I've heard rumor not too many people think we'd be likely to ever do that (although I think it is fine)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC with the latest change, nRuleChangeActivationThreshold is only used for the "the might be an unknown rule change" warning system. Each deployment now has its own threshold.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also 9e50401 which in addition lowers the warning threshold even further.

@ajtowns
Copy link
Contributor Author

ajtowns commented Mar 8, 2021

but it does mean that, if it looks like activation_time is going to fall near a retarget block, we'll have to communicate that taproot could either happen tomorrow or two weeks from now.

I think it's worse than that -- if activation_time falls very near a retarget block, I think you could have the eleven last blocks of the retarget period have timestamps (relative to activation_time) like: -3000, -2400, -1800, -1200, -600, 0, 600, 1200, 1800, 2400, 3000 giving the last block a median time which satisfies MedianTimePast() >= activation_time, so the next block would switch to ACTIVE and people would likely start transacting with taproot, perhaps even racing to get a tx in the first block as some did with segwit, and setting nlocktime to ensure their transaction isn't mined early and the funds stolen.

However in that case a 2 block reorg could change the last block's timestamp to -400 (still greater than the mediantime of the prior block which is likely -600), which would change its median time from 0 to -400, which would then cause the new first block of the next period to still be in state LOCKED_IN. At that point replaying the taproot-using transactions people had sent prior to the reorg would allow funds to be stolen as taproot is not yet active after the reorg, and nlocktime would not have protected you.

(EDIT: fixed mediantime rules misinterpretation -- blocks must have a timestamp greater than the previous block's mediantime; block's mediantime does include their own timestamp. And add the following...)

I think the conclusion from this is just "the height at which you transition from LOCKED_IN to ACTIVE must be fully determined as soon as you transition from STARTED to LOCKED_IN". That way the entire LOCKED_IN period has to be reorged if you want to steal funds protected by both nlocktime and the new rules.

@ajtowns ajtowns force-pushed the 202103-bip9-speedy-trial-support branch from d614299 to cb5f1a2 Compare March 8, 2021 03:25
@ajtowns
Copy link
Contributor Author

ajtowns commented Mar 8, 2021

Bumped to min_activation_height instead of activation_time; also helps github notice that #21334 already merged.

@Sjors
Copy link
Member

Sjors commented Mar 8, 2021

Shameless plug for #19013 which adds backwards compatibility testing for v0.21; this should be useful when testing soft fork deployment.

@JeremyRubin
Copy link
Contributor

based on the reasoning here: https://gist.github.com/michaelfolkson/92899f27f1ab30aa2ebee82314f8fe7f#gistcomment-3658078 I have very limited concerns around using start/stop times (could be wrong)

@ajtowns that's a great point -- MTP just has ambiguity baked into it, and we should prefer heights for the activation point.

achow101 pushed a commit to achow101/bitcoin that referenced this pull request Apr 15, 2021
achow101 pushed a commit to achow101/bitcoin that referenced this pull request Apr 15, 2021
achow101 pushed a commit to achow101/bitcoin that referenced this pull request Apr 15, 2021
Previously we used deployments that would timeout prior to Bitcoin's
invention, which allowed the deployment to still be activated in unit
tests. This switches those deployments to be truly never active.

Github-Pull: bitcoin#21377
Rebased-From: 55ac5f5
achow101 pushed a commit to achow101/bitcoin that referenced this pull request Apr 15, 2021
This removes the DEFINED->FAILED transition and changes the
STARTED->FAILED transition to only occur if signalling didn't pass the
threshold. This ensures that it is always possible for activation to
occur, no matter what settings are chosen, or the speed at which blocks
are found.

Github-Pull: bitcoin#21377
Rebased-From: f054f6b
achow101 pushed a commit to achow101/bitcoin that referenced this pull request Apr 15, 2021
achow101 pushed a commit to achow101/bitcoin that referenced this pull request Apr 15, 2021
achow101 pushed a commit to achow101/bitcoin that referenced this pull request Apr 15, 2021
achow101 pushed a commit to achow101/bitcoin that referenced this pull request Apr 15, 2021
Previously we used deployments that would timeout prior to Bitcoin's
invention, which allowed the deployment to still be activated in unit
tests. This switches those deployments to be truly never active.

Github-Pull: bitcoin#21377
Rebased-From: 55ac5f5
achow101 pushed a commit to achow101/bitcoin that referenced this pull request Apr 15, 2021
This removes the DEFINED->FAILED transition and changes the
STARTED->FAILED transition to only occur if signalling didn't pass the
threshold. This ensures that it is always possible for activation to
occur, no matter what settings are chosen, or the speed at which blocks
are found.

Github-Pull: bitcoin#21377
Rebased-From: f054f6b
achow101 pushed a commit to achow101/bitcoin that referenced this pull request Apr 15, 2021
@fanquake
Copy link
Member

Being backported in #21701.

achow101 pushed a commit to achow101/bitcoin that referenced this pull request Apr 16, 2021
achow101 pushed a commit to achow101/bitcoin that referenced this pull request Apr 16, 2021
achow101 pushed a commit to achow101/bitcoin that referenced this pull request Apr 16, 2021
Previously we used deployments that would timeout prior to Bitcoin's
invention, which allowed the deployment to still be activated in unit
tests. This switches those deployments to be truly never active.

Github-Pull: bitcoin#21377
Rebased-From: 55ac5f5
achow101 pushed a commit to achow101/bitcoin that referenced this pull request Apr 16, 2021
This removes the DEFINED->FAILED transition and changes the
STARTED->FAILED transition to only occur if signalling didn't pass the
threshold. This ensures that it is always possible for activation to
occur, no matter what settings are chosen, or the speed at which blocks
are found.

Github-Pull: bitcoin#21377
Rebased-From: f054f6b
achow101 pushed a commit to achow101/bitcoin that referenced this pull request Apr 16, 2021
@jaybny
Copy link

jaybny commented Apr 16, 2021

ACK - no need for BIP8 at this time

maflcko pushed a commit that referenced this pull request Apr 16, 2021
cbd64c3 Add mainnet and testnet taproot activation params (Andrew Chow)
ec78243 chainparams: drop versionbits threshold to 90% for mainnnet and signet (Anthony Towns)
6003573 versionbits: simplify state transitions (Anthony Towns)
3acf037 versionbits: Add explicit NEVER_ACTIVE deployments (Anthony Towns)
b529222 fuzz: test versionbits delayed activation (Anthony Towns)
71917e0 tests: test versionbits delayed activation (Anthony Towns)
4cab84c versionbits: Add support for delayed activation (Anthony Towns)
f9517e6 tests: clean up versionbits test (Anthony Towns)
1c01645 tests: test ComputeBlockVersion for all deployments (Anthony Towns)
2e9e7f4 tests: pull ComputeBlockVersion test into its own function (Anthony Towns)

Pull request description:

  Backport of #21377 and #21686

ACKs for top commit:
  instagibbs:
    cherry-pick ACK cbd64c3
  jnewbery:
    ACK cbd64c3
  Sjors:
    tACK cbd64c3
  MarcoFalke:
    cherry-pick-only ACK cbd64c3 🌾

Tree-SHA512: e9efb0ca9986d685161bcba5ed43efdc5f1dca88322cf65faccf17009b567c2d930c2aba4d1541539fc65347574ed4faa3d4558b907c779d1c128b3d2c681f31
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Apr 17, 2021
This allows to remove check that windows for the same bit are disjoint

This addresses bitcoin#21377 (comment)
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Apr 17, 2021
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Apr 17, 2021
This allows to remove check that windows for the same bit are disjoint

This addresses bitcoin#21377 (comment)
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Apr 17, 2021
@fjahr
Copy link
Contributor

fjahr commented Apr 17, 2021

Post merge Code Review ACK ffe33df

Also built and ran unit tests locally.

Copy link

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not reviewed, just confused, sorry for the noise.

consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nTimeout = 1230767999; // December 31, 2008
consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nStartTime = Consensus::BIP9Deployment::NEVER_ACTIVE;
consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT;
consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].min_activation_height = 0; // No activation delay

This comment was marked as resolved.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kixunil The activation parameters were add in a separate PR, #21686

windsok pushed a commit to windsok/bitcoin that referenced this pull request Apr 23, 2021
This allows to remove check that windows for the same bit are disjoint

This addresses bitcoin#21377 (comment)
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.