Speedy trial support for versionbits #21377

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.

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

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

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.

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.

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #21401 (Refactor versionbits deployments to avoid potential uninitialized variables by achow101)
• #19573 (Replace unused BIP 9 logic with draft BIP 8 by luke-jr)
• #17783 (util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky)
• #17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
• #17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
• #17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)

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.

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

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

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

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.

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

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.

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 9762562cc6a2c771ad0ddfb055ea17b90085425c, bcec418304ea3dd524cb3591e4af1b77a04cc968 (though I suggest a fresh BIP for the speedy trial) and a few more.

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.

concept ACK, I agree with @harding’s line of reasoning:

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.

Combined with how minimal this change is- the bulk of this PR is tests, the changes in the source code are +26/-4 lines over 2 commits.

I’ve taken a first pass and it all looks reasonable, but I plan to do a full review soon. These changes are simple but I’m currently trying to gain the appropriate codebase context.

I don’t have a strong view but it appears that @achow101 in #21392 and just by reading the comments in this PR @luke-jr @benthecarman @JeremyRubin @Sjors @roconnor-blockstream @harding all have a preference for a fully height based approach over any use of MTP.

If that isn’t the case please correct me. And if anyone else has a view on entirely block height versus some limited use of MTP can you post it on IRC (##taproot-activation)?

The fuzzing PR looks very cool. On that PR @MarcoFalke also expresses an expectation that activation will be based on block heights.

edit: Some additional comments from today’s Core dev meeting

“it comes down to mtp vs block height” @achow101

“the main difference between these two PRs are using block height vs MTP " @amitiuttarwar

“with mtp, we run the risk of losing 2 signaling periods. with already few signaling periods, this has the possibility of failing to activate due to bad luck” @achow101

“the property that achow101’s PR improves is a fixed (number) of signals” @JeremyRubin

I have a preference for fully height based design, correct. But I think that given the code base as it stands today, and comparing the diffs across the PR, and the tight timeline desired from merge to release using MTP really is a decent option, especially since the bulk of the code is already more widely tested. Longer term it makes sense to fully develop and test height based – whether we do that, I don’t know. @MarcoFalke’s comment is a slight misunderstanding I believe of how the fuzzer simulates time; I recall AJ saying (somewhere) that block times are steady interval in the fuzzing so that it doesn’t matter (can’t find that comment now tho). Is that correct?

I also emphasized numerous times in the meeting that I think for the purposes of a 3 month ST, forecasting to be a mid-period start/stopheight means that drift is very unlikely to hurt us, so the notion that we’re “risking two periods” is a bit moot to me.

The counterarguments can be made that we have bigger issues to solve for if we miss two periods as a result of a hashrate decrease (where did the hashrate go – we probably wouldn’t want to activate if we’re missing a big chunk of hashrate contemporaneously anyways) and that on the flip side, a hashrate increase would yield in the block heights going by more quickly which means there is less real time to coordinate enabling signaling. There are pros and cons either way, I think that height is superior longer term, but in this case I don’t think there is sufficient technical risk to disqualify this PR as an activation method in favor of start/stop height. @luke-jr I would appreciate if you could justify your concept NACK more explicitly as I do not understand your concern. @amitiuttarwar made a great point in the meeting

my two cents: I don’t think either PR is close to being RFM, both need significantly more review. looks like #21392 is currently failing CI. If the goal is to quickly merge with safety, I think #21377 makes more sense (as I stated on the PR)

Rough consensus and running code – it seems to me that this is closer to a mergeable state. If there’s a strong reason to the contrary, or if people decide to invest the requisite energy to get #21392 reviewed and RTM before then ok.

Assuming we would like a ST-style taproot activation logic ready by April 3rd, I’m leaning toward reusing current bip9. It has been successfully exercised at least twice for csv and segwit softforks and the diff to achieve ST semantic is far smaller to focus on.

Ideally, height-based locked_in parameters would be better, as consensus rules around MTP are quite loose. AFAIU, it doesn’t prevent a coalition of miners to scale down their mined blocks nTime to MTP + 1, thus preventing to reach a bip9 starttime at the expected real-world time. That said, if we see such behavior showing up, it would be at least a good proof of a malicious intent and ST would be a success by providing new, useful data to the softfork conversation . Beyond malicious MTP manipulation, there is a still a concern about a too-small signaling window, but AFAICT selecting starttime/timeoout in mid expected retarget period should minimize such risks.

This PR might also set a precedent to prefer refinement of existent activation logic in function of social consensus aleas. If so, we should be careful about “activation height” not introducing unintended chain split risks or permanent restriction of softfork deployment flexibility like bip34 did it w.r.t to versionbits. If you have time, maybe propose a quick amendment to bip9, formalizing might help reasoning about such technical risks ?

That said, I’ll look on competing #21392 before to focus on one or the other.

Ideally, height-based locked_in parameters would be better, as consensus rules around MTP are quite loose. AFAIU, it doesn’t prevent a coalition of miners to scale down their mined blocks nTime to MTP + 1, thus preventing to reach a bip9 starttime at the expected real-world time

Since it uses median time past, you’d need to be doing that for 6 out of every 11 blocks, which would require >50% of hashpower, and you’d have to take care to not manipulate the nTime of the first and last block of each retarget period so as not to make difficulty skyrocket; but you’d only need 10% of hashpower to block signalling.

Losing hashpower was the concern for MTP with bip148, and it could be a theoretical concern here too – if you lost 75% of hashpower, then instead of 14 weeks between timeout and starttime resulting in 6 or 7 retarget periods, it could result in just 1 or 2 retarget periods. But provided no new rules are added to cause blocks to be considered invalid (such as the required signalling rule in bip148), there’s no reason to expect a drastic loss of hashpower.

But as you point out, in any event, we’d learn something new, and could try again afterwards taking that new knowledge into account.

review ACK 4c7d858167a7f8cd4cb6b09787f07929d2abc3a1 🍦

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 4c7d858167a7f8cd4cb6b09787f07929d2abc3a1 🍦
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiG6wwAsBIP2/QWvAMfor3Wt8lSqJnHmHmBzmZY0Vw2Z4QnM+qknHPBHnR9adkb
 8x/G5m/gUnBGYOxMW2AJsRvooU8WiVfvxfhlZE95cjknYkTKTiirsum9dU7idbCxr
 99ODwd44LwO1Y0KknerNXPyTwQ0slHvtMGGpcWgHdHHqSchrVy9aduBNSGbs+i+dh
10e2YqhcKh2cGtygdpbkDFJrpF3102hDfSr2A85CO0C1lmKpSAE1v0kjltZrshUoVY
11NPL9ap/srainn1f6dAFf1JHErLORug8UOD3T4UO8Xbf9RrRu22xEt5AYd3xumSw+
12ZRNOdtMGGgcMi3LebHOU1OewKb7SQIPMjbWs9BC6kV0vI+i4hvP6pxJLV7+kZi/r
13XXohowtItscV+oP8tmaXcvapPNISSN+DIEXiECObVdvHZ5xsVmag/dKWxQzAK1C5
14Sr1GLCD8/GiIVGyvqDIs100g4EMHs0bluj0qb2Ylmkvl+PZmMlU7TwT9GS9EuGoP
15X3ygOyma
16=xHYS
17-----END PGP SIGNATURE-----

Timestamp of file with hash be5f7748fa7afdbed03e27c4c76aa836a95078861703dc7750d0df1a9da0dbd7 -

Approach NACK.

We need to focus review effort on a single Speedy Trial PR at this stage. Although AJ has done as much as anyone to advance Taproot activation with all sorts of contributions (conceptual, code, tests, fuzzing) I am at a loss as to why to choose a PR with a mix of block height and MTP over a PR with a consistent use of block height.

I have attempted to summarize the discussion on block height versus mix of block height and MTP here. To me a consistent use of block height is a no brainer. If that needs a little more review and testing so be it. (I’m not convinced it does btw, I find it much easier to reason and think about edge cases with entirely block height than a mixture. It is a slightly larger code diff, that is true.)

I also have no idea why block height versus mix of block height and MTP has any impact whatsoever on UASF plans. It doesn’t seem to impact the difficulty of implementing a competing compatible UASF and any possible “marketing” gain for a UASF seems illusory.

Disclosure: I have been happy to work on a UASF project in the recent past when there seemed little chance of progress with Taproot activation in Core. I would also consider doing so again in future depending on the circumstances.

I hope we can choose a Speedy Trial PR as soon as possible to focus all review effort on it. If this PR is chosen over Andrew’s PR I will disagree with that decision but I will live with it. I think it will be the inferior choice (assuming a new rationale isn’t explained to me) but I don’t think it will cause any material problems with activation.

What am I doing wrong that I can’t get minimum_activation_height to have any effect on regtest?

 0diff --git a/src/chainparams.cpp b/src/chainparams.cpp
 1index 40c58f0e67..73be5d3d6d 100644
 2--- a/src/chainparams.cpp
 3+++ b/src/chainparams.cpp
 4@@ -412,7 +412,7 @@ public:
 5         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].bit = 2;
 6         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nStartTime = Consensus::BIP9Deployment::ALWAYS_ACTIVE;
 7         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT;
 8-        consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].min_activation_height = 0; // No activation delay
 9+        consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].min_activation_height = 576; // No activation delay
10 
11         consensus.nMinimumChainWork = uint256{};
12         consensus.defaultAssumeValid = uint256{};

$ make
[...]

$ src/bitcoind -regtest -daemon -vbparams=taproot:1:1932252724
[...]

$ src/bitcoin-cli -regtest getblockchaininfo | jq .blocks,.softforks.taproot
0
{
  "type": "bip9",
  "bip9": {
    "status": "defined",
    "start_time": 1,
    "timeout": 1932252724,
    "since": 0
  },
  "active": false
}

I’m expecting to see a min_activation_height field in the getblockchaininfo results, as my reading of the code says it should be there for cases where the min activation is greater than zero.

It’d be nice if -vbparams here took a fourth argument for the minimum activation height, similar to in #21392 , so recompiling (and whatever mistake I’m making there) wouldn’t be necessary.

Changed from “delayed activation” to “delayed lock in” – that is, transitions go from STARTED to DELAYED to LOCKED_IN to ACTIVE, with LOCKED_IN not being entered until MTP passes min_lock_in_time. This fixes the problem noted earlier while retaining the ability to know 2016 blocks in advance when activation will occur.

Using MTP timestamps for activation retains compatible with deployments for testnet and signet, where block heights are unpredictable (for testnet) or inconsistent (different custom signets will not have common block heights).

Extends the ComputeBlockVersion unit tests to test all deployments across all chains. Adds an optional parameter to -vbparams to allow testing, and uses that parameter in the unit tests.

Note that this approach can also be used to make bip-148/bip-91 style mandatory signalling compatible with MTP-based activation; see https://github.com/ajtowns/bitcoin/commits/202103-bip9-uasf for a branch that adds a LAST_CHANCE signalling phased that will always be reached just prior to failure, so can be used for mandatory signalling to guarantee lock in.

Approach NACK

At this point, it seems like this PR has gotten more complicated due to MTP and reviewing this is going to be more complicated due to the edge cases around MTP. When this used a minimum activation height, I would have agreed that this would be easier and faster to review, but now that it has reverted to using a minimum (lock in) time, I am not comfortable with this approach anymore. While I agree that MTP is conceptually easier to work with, practically it has many edge cases and gotchas that I don’t think we should continue forward with using MTP. Heights are strictly easier to understand. The only edge cases to consider are off by one errors and not miners screwing with timestamps.

Using MTP timestamps for activation retains compatible with deployments for testnet and signet, where block heights are unpredictable (for testnet)

I agree that using MTP for testnet is better, but I don’t think that is important enough to block the use of heights for mainnet. I also think that testnet3 should be abandoned at this point, but that’s a discussion for another place and time.

or inconsistent (different custom signets will not have common block heights).

I think that either approach is incompatible with custom signets. I don’t think it makes sense that every signet would also have the same start time and timeout time, especially for signets that are created afterwards. It would make more sense to refactor the parameter setting and allow for the custom signets to set their own activation parameters, regardless of height or MTP.

It would make more sense to refactor the parameter setting and allow for the custom signets to set their own activation parameters, regardless of height or MTP.

wouldn’t that mean that you can never bury a deployment again because there could be some signet where it hasn’t activated yet?

@ajtowns

Transitions go from STARTED to DELAYED to LOCKED_IN to ACTIVE, with LOCKED_IN not being entered until MTP passes min_lock_in_time. This fixes the problem noted earlier while retaining] the ability to know 2016 blocks in advance when activation will occur.

I like that. I think it does a better job of communicating to users what state the deployment is in and, by virtue of min_lock_in_time being a time, gives a more reliable and easier to understand estimate of when activation will happen (e.g. “min_lock_in_time + 10 - 24 days with high confidence” instead of using heights, where it could be as much as “140 to 220 days”).

Adds an optional parameter to -vbparams to allow testing, and uses that parameter in the unit tests.

Thank you! @achow101

it seems like this PR has gotten more complicated due to MTP

Having reviewed both (as of last week), I thought this was moderately simpler than yours. I skimmed the rebase diff and don’t think the last push increases complexity (indeed, I think it simplifies it from the earlier minimum_activation_height change.

MTP […] practically has many edge cases and gotchas [and] I don’t think we should continue forward with using MTP. Heights are strictly easier to understand.

What edge cases does MTP have for a deployment such as this where there’s no mandatory signaling or mandatory activation? MTP abuse generally assumes miner adversity (usually at least enough miners to execute a selfish mining attack, e.g. ~30%) but a mere 15% of miners opposed to Speedy Trial should be able to prevent its activation by following the allowed protocol.

Heights may be easier to reason about in some ways, but they don’t directly provide the most important assurance we want in a Speedy Trial activation: that we will have a certain minimum amount of time to get many nodes upgraded to enforce taproot when it activates. As far as I’m aware, miners screwing with time stamps—at a level less than that necessary to produce a time warp attack[1]—can only delay a deployment of this PR as written, and only at cost to the miners in the form of needing to do more PoW. I think that’s fine; we can be a little bit more patient. But for a height-based activation, miners can accelerate the deployment either non-deliberately (e.g. because BTC price increases lead to increased mining) or deliberately (again at the cost of doing more PoW); I think that’s an acceptable risk, but to my mind it’s not as good as the assurance MTP can bring us.

Again, if there’s an MTP edge case that applies to this PR that I’m not aware of, I’d certainly be interested in learning about it. Overall, I feel this latest update improves this PR to where I slightly prefer it over height-based activation (although I’m still fine with either approach).

[1] Time warp attacks break important properties of both MTP-based and height-based activation mechanisms; it also breaks important fundamental properties of Bitcoin. Given that, I don’t think we need to consider it here: I think miners know that trying a time warp attack on mainnet could easily lead to a PoW algorithm change.

wouldn’t that mean that you can never bury a deployment again because there could be some signet where it hasn’t activated yet?

Not necessarily. A deployment can be buried in one chain and not in another. With #19438, I think it would also be trivial to have a deployment buried in one chain and not in another.

What edge cases does MTP have for a deployment such as this where there’s no mandatory signaling or mandatory activation? MTP abuse generally assumes miner adversity (usually at least enough miners to execute a selfish mining attack, e.g. ~30%) but a mere 15% of miners opposed to Speedy Trial should be able to prevent its activation by following the allowed protocol.

Sure, but my main concern is about analyzing the correctness of the code around these scenarios. I am sure that the code can handle those situations correctly, but when reviewing it, it is harder to analyze and conclude that all of the state transitions that are based around MTP are safe. It is easier to do this analysis for height based deployments.

@flack

wouldn’t that mean that you can never bury a deployment again because there could be some signet where it hasn’t activated yet?

The only plausible way I’ve come up with for burying deployments on signet is via MTP – ie, declare that a soft-fork is unconditionally active on signet once the MTP passes some point, probably the MTP at which it activated on mainnet plus some delay; perhaps something like:

0constexpr uint32_t ACTIVATION_MTP_THRESHOLD = 500000000;
1bool DeploymentActiveAt(block, params, dep) {
2    int h = params.DeploymentHeight(dep);
3    return (h < ACTIVATION_MTP_THRESHOLD ? block.nHeight >= h : block.GetMedianTimePast() >= h);
4}

For custom signets that have chosen to activate the soft fork already, that’s fine, because it’s a soft fork the old blocks will still be valid even if they use rules that won’t be enforced on a -reindex or by a new node doing IBD, and because blocks are signed, there is no risk of a reorg stealing funds. For custom signets that have not already activated the soft fork, it’s fine as long as either miners upgrade to the new software before the signet flag day activation, or as long as the soft fork is safe and miners are not doing something like -acceptnonstdtxn so they won’t mine blocks that are invalid under the new rules. @achow101

It would make more sense to refactor the parameter setting and allow for the custom signets to set their own activation parameters, regardless of height or MTP.

I don’t think that’s feasible from a usability standpoint: to use a custom signet you would have to define all the consensus parameters for all post-taproot forks, and do so in a way that’s exactly compatible with all the other users of that custom signet.

Using MTP-based signalling allows a signet to be upgraded to a new soft fork simply by the signet miners signalling, with no action required by users at all beyond running an updated binary, which I don’t think is achievable otherwise. (At minimum you’d need to update your config to set a “activate this deployment early” flag for each deployment on each custom signet)

Not necessarily. A deployment can be buried in one chain and not in another. With #19438, I think it would also be trivial to have a deployment buried in one chain and not in another.

The latter part of that is not true – #19438 differentiates buried and signalled deployments by data type, which is constant for all chains at compile time.

But the point of burying deployments is so that we don’t need to keep dealing with conditional activation – eg, we don’t need to support the possibility that segwit is activated but p2sh is not. We don’t want to force ourselves to maintain those code paths to support random custom signets.

I believe this is the intended state machine:

To generate:

 0$ dot -Tpng >states.png
 1digraph versionbits {
 2    defined [shape=box,label="DEFINED"];
 3    started [shape=box,label="STARTED"];
 4    failed [shape=box,label="FAILED"];
 5    locked [shape=box,label="LOCKED_IN"];
 6    delayed [shape=box,label="DELAYED"];
 7    active [shape=box,label="ACTIVE"];
 8
 9    defined -> defined [label="time < begin\ntime < end"];
10    defined -> started [label="time >= begin\ntime < end"];
11    defined -> failed [label="time >= end"];
12    started -> started [label="time < end\nsig < thresh"];
13    started -> failed [label="time >= end\n"];
14    started -> delayed [label="time < end\nsig >= thresh\ntime < min_lock"];
15    started -> locked [label="time < end\nsig >= thresh\ntime >= min_lock"];
16    delayed -> delayed [label="time < min_lock"];
17    delayed -> locked [label="time >= min_lock"];
18    locked -> active [label="always"];
19    active -> active [label="always"];
20    failed -> failed [label="always"];
21}

When this was updated I was originally a bit frustrated since the addition of the DELAYED state + switch to all MTP invalidates the ACKs on this PR.

However, now that I understand @ajtowns reasoning around signet compatibility, I see the merit with this approach and retract my earlier preference for height based ST. With regards to review burden, I still think this is slightly easier to reason about & a smaller diff, but I think we’ve accepted the risks of delaying the proposed release schedule in favor of creating a more robust mechanism. As a Signet Operator*, a pure MTP design should make it easier for me to activate taproot for users of my network.

My only critique of the modified design is that it might be nice to accommodate a min number of signal periods (e.g., 6) so that signets with 1 hour block rate would have sufficient time to activate. 1 hour isn’t crazy for a signet because such blocks are seen somewhat often on the network, so a long-delay signet might have utility for testing infrastructure robustness.

I don’t think this is a show-stopping issue for merging this code as-is, because the min number of signal periods functionality can be added later relatively easily without affecting any ongoing release (just default set it to 0).

* my signet is currently crashed, hope to bring it online soon when I can add disk space…

a pure MTP design should make it easier for me to activate taproot for users of my network

Note – taproot is already active on any signet; activation on signet is only a concern for any future soft forks (eg CTV).

@jnewbery

I wonder if those first three commits should be pulled out into a separate PR?

The could be, but they aren’t terribly compatible with #21392 which stops checking ComputeBlockVersion behaviour against mainnet, and has a bunch of other changes to the ComputeBlockVersion test in order to convert from mtp to heights.

I’d like to rescind my previous Approach NACK

It was given in the spirit of wanting all review to be focused on a single Speedy Trial PR (something I think should be top priority above all else) rather than two PRs, thinking an alternative PR (#21392) was better placed to be merged and personal preference too.

However, I am less sure the alternative PR stands a better chance of being merged at this point. And my personal preference for the alternative PR is not strong enough to Approach NACK this PR otherwise.

Hence, Approach ACK for this PR. My preference is #21392 but (to the extent I can) I won’t stand in the way of this PR being merged if it can be.

My only critique of the modified design is that it might be nice to accommodate a min number of signal periods (e.g., 6) so that signets with 1 hour block rate would have sufficient time to activate. 1 hour isn’t crazy for a signet because […]

I’m not sure that a 1 hour block rate isn’t crazy – nPowTargetSpacing is still 10min, so your difficulty would keep getting automatically lowered unless you had bursts of blocks to compensate so that the average rate hits 10min every 2 weeks. If you have bursts like that to maintain difficulty anyway, you can just use them to get signalling done in a normal timeframe.

But ignoring that, I think an easy fix would be to lower signet’s nMinerConfirmationWindow from 2016 to 288; that way normal signets can activate changes in four days instead of four weeks; and even a 1-hour-per-block signet that wasn’t willing to do bursts wouldn’t need to to take much longer than a month either (<12 days to hit a 288-block boundary, ~12 days to signal, ~12 days to lock in, so <36 days total).

@ajtowns i thought you could set arbitrary block rates for signet?

The only signet consensus parameter you can change (without everyone recompiling) is the block challenge. The old miner script used to let you target a block rate, but that would have an impact on difficulty that might be annoying to manage, so the new one lets you target a difficulty (--nbits) and adjusts the block rate to match; when you hit your difficulty target, it’ll just average out at ~10min per block.

I believe this is the intended state machine:

Yeah. It’s somewhat more complicted than it could be because it’s maintaining some of the complexities of the bip9 state machine. It could be simplified a bit by dropping the DEFINED->FAILED case and not ignoring signalling from the last STARTED period:

Excepting the addition of the DELAYED state, that pretty much makes it the same as the corresponding bip8 diagram, as bip8 has already made those changes compared to bip9.

 0digraph versionbits {
 1    defined [shape=box,label="DEFINED"];
 2    started [shape=box,label="STARTED"];
 3    failed [shape=box,label="FAILED"];
 4    locked [shape=box,label="LOCKED_IN"];
 5    delayed [shape=box,label="DELAYED"];
 6    active [shape=box,label="ACTIVE"];
 7
 8    delayorlock [shape=circle,label=""];
 9
10    edge [weight=1];
11    defined -> defined [label="time < begin"];
12    started -> started [label="nsig < thresh\ntime < end"];
13    active -> active [label="always"];
14    failed -> failed [label="always"];
15    delayed -> delayorlock [label="always"];
16
17    edge [weight=100];
18    defined -> started [label="time >= begin"];
19    started -> failed [label="nsig < thresh\ntime >= end\n"];
20
21    started -> delayorlock [label="nsig >= thresh"];
22    delayorlock -> delayed [label="time < min_lock"];
23    delayorlock -> locked [label="time >= min_lock"];
24
25    locked -> active [label="always"];
26
27    { rank=same; delayed delayorlock }
28}

If it is currently not possible to bury/flag-day activate/vb signal at different heights/times in signet, it seems that this should be made possible via command line options, not by preferring MTP over height based vb code (or the other way round).

Also, I fail to see how MTP solves an issue that height based can’t. Let’s assume the signet default params for deployment mention a start and end parameter for a soft-fork, but someone wants to start a signet with the softfork always active. This can only be fixed by supplying modified source/binaries or by allowing the users/miners to pick the vbparams at runtime. It can’t be fixed by using MTP over height (or the other way round).

the benefit of MTP is not needing to disseminate additional parameters per-signet for activation. If you upgrade to the latest bitcoin-core with a new Soft fork deployment, your existing config file (with whatever signetchallenge) will be able to activate without further configuration.

this is not possible with height.

this is not possible with height.

Is it really that much work to check the place where you copied the signetchallenge to also copy the vbparams? You’ll have to do that anyway, otherwise you won’t know if your signet picked always active, flag day, or signalling activation.

apologies, I just am not going to be awake for a 3am IRC meeting my time, so dropping my feedback here:

I’m unconvinced by the arguments set out here for MTP: the one thing you do get is testnet being an activation testing ground for the MTP-based implementation. Other than that it’s a wash, or it seems to be a net negative for clarity and testing.

If signets require a little more config data to be passed around, so be it. We should not be tying mainnet safety to configs of these networks. Based on my years of experience any proper operation of a “non-official” signet-like chain will be carting around a config file anyways.

fwiw I’m deferring proper review of a PR until I think we can approach consensus on this topic. I think this can be solved provided the review burden of both is similar in magnitude.

@MarcoFalke

Also, I fail to see how MTP solves an issue that height based can’t. Let’s assume the signet default params for deployment mention a start and end parameter for a soft-fork, but someone wants to start a signet with the softfork always active. This can only be fixed by supplying modified source/binaries or by allowing the users/miners to pick the vbparams at runtime. It can’t be fixed by using MTP over height (or the other way round).

The issue for signet that MTP signalling solves that height based signalling doesn’t is allowing multiple signets to operate with the same fixed-at-compile-time parameters. That avoids having to have every user of every signet add custom command line parameters that will change over the lifetime of the signet (as additional soft forks are deployed/activated).

With MTP signalling, if you want a non-buried soft fork “always active” on a new signet, you create the signet and mine 3 periods worth of blocks, signalling in the second period – no need for anyone to set vbparams. (For a soft fork that’s buried via MTP, you simply need to get MTP past the burial MTP, so likely only need to mine two or three blocks if you’re creating a new signet for an already buried fork)

If you’re adding height-based params to signetchallenge, you’re making the challenge incompatible with existing nodes (that obviously don’t support parsing and removing the vbparams from the challenge), but you’re also requiring the challenge to change with each new soft fork: you’ll need to add ‘anyprevout=height’ one day, then ‘ctv=height’ some other day, then ‘consensuscleanup=height’ some other day, and can’t remove any of them, even after they’re buried on mainnet. And you’re also providing an extra way for people to make consensus-breaking mistakes, in hard to detect ways – currently if you get the signet challenge wrong you’ll just end up having a different network magic and not connect to the signet at all, but with optional parameters added you can’t do the same, since you want to be backwards compatible by allowing the optional values to be missing. @Rspigler

I don’t think signet should be a consideration for MTP vs height, since taproot is already activated on signet, and there’s no indication that ST will be used in the future.

If we’re not trying to use this as a stepping stone for future activations, then it would make sense to keep the changes for ST as simple as possible, and sticking with MTP is the smaller and more easily backportable set of changes…

Even restricting the analysis purely to the effect on mainnet, and ignoring testing, implementation or backporting complexity, I think MTP is superior; if it were blocking the ability to reasonably do a UASF then that would still be a showstopper, but as per the LAST_CHANCE comments above, I don’t see any reason why that would be the case.

I think that the min_lock_in_time should be reverted to min_activation_height. As noted on the mailing list, min_lock_in_time adds an additional 14 day uncertainty to the true activation time, whereas predicting the time for min_activation_height to be reached only gets better as that height approaches. A good enough prediction of the actual activation time can be known several days or weeks ahead. With min_lock_in_time, miners could also manipulate timestamps to introduce uncertainty in the activation time.

Although I think that block heights is technically superior, many of the specific things that it guarantees are still provided by an MTP implementation probabilistically and some changes can be made to provide the same guarantees. With that in mind, I would like to see that the DEFINED->FAILED transition is removed and the unconditional STARTED->FAILED when the timeout is reached. This would make it more like BIP 8 where at least one signaling period is guaranteed, and if the timeout falls in the middle of a signaling period, then that period will still count towards the activation.

Although I think that block heights is technically superior, many of the specific things that it guarantees are still provided by an MTP implementation probabilistically and some changes can be made to provide the same guarantees. With that in mind, I would like to see that the DEFINED->FAILED transition is removed and the unconditional STARTED->FAILED when the timeout is reached. This would make it more like BIP 8 where at least one signaling period is guaranteed, and if the timeout falls in the middle of a signaling period, then that period will still count towards the activation. @achow101 would the LAST_CHANCE https://github.com/ajtowns/bitcoin/commits/202103-bip9-uasf solution work for the guaranteed at least 1 signal period?

@achow101 would the LAST_CHANCE https://github.com/ajtowns/bitcoin/commits/202103-bip9-uasf solution work for the guaranteed at least 1 signal period?

Originally this paragraph suggested that, but @ajtowns pointed out that changing these state transitions achieves the same effect. I prefer changing these transitions rather than adding a new state as these were part of BIP 8 and are thus already (partially) understood. However I think it is important to add LAST_CHANCE later since it enables a UASF.

Updates as per achow101’s suggestions – switches back from fully-mtp-based to using min_activation_height, and drops bip9’s “fail immediately when timeout passes” handling (also adds a NEVER_ACTIVE special case, since the lack of an immediate failure mode means you can’t just use a super-early timeout for that purpose anymore).

I believe this should capture the updated state machine:

[EDIT: note that these all map attributes of the last block in a period to the activation state of all blocks in the next period. Replace “time” by “previous block’s time” and “height + 1” by “height” to map attributes of the first block in a period to the activation state of all blocks in the same period]

I believe this backports to 0.21 cleanly (on top of #21614 anyway).

(CI is currently timing out on the fuzzer run)

ACK be7d1368d8e98b6f9f692289b7d25ac369bf2984

All of the changes that I suggested have been implemented.

utACK https://github.com/bitcoin/bitcoin/pull/21377/commits/be7d1368d8e98b6f9f692289b7d25ac369bf2984

logic appears to match the latest state machine image, read through all the various discussions on the design choices, agree that the current state machine in the PR is much better than previous iterations.

Will do some follow-on testing soon.

Reviewed everything. All the changes look good to me. utACK be7d1368d8e98b6f9f692289b7d25ac369bf2984.

I’ve left a bunch of style suggestions inline. Nothing critical, so feel free to ignore.

Thank you for persevering with this, @ajtowns, and for all of the prerequisite work that you’ve done over the last couple of years to improve testing and develop activation proposals. Thanks also to @achow101 and all other reviewers whose suggestions have improved and strengthened this code.

From @maaku on using block height versus a mix of block height and MTP. Understandably he doesn’t want to be involved.

“Segwit activation exposed a number of problems with the BIP9 activation state machine which was based on MTP windows. These concerns include:

Difficulty of interfacing with in-parallel activation mechanisms like BIP148, BIP91, or the presumed future LOT=true UASF for taproot. (You may not like UASF. You may be against UASF and wish is never happened. Well reality doesn’t care. There will be a UASF client, that is simply a fact. That cat is out of the bag. Our job is to make sure that UASF risks are minimized for the sake of the network, which height-based activation mechanisms do better. I’m not saying MTP can’t be UASF compatible, I’m just saying it’s way easier to avoid foot-gunning a UASF deployment with height-based windows. And while your inner schadenfreude might laugh at a UASF foot-gun, such a chain-splitting failure affects everyone.)

Perverse timestamp-manipulation incentives for miners, who can use timestamp manipulation to slow or speed up MTP to delay or speed up the closing of an activation window. People rely on accurate MTP timestamps for things like lock times, and of course it affects difficulty adjustments, and so it is very bad for there to be incentives for miners to manipulate timestamps.

Two-week uncertainty over activation window start or end times based on alignment of 2016-block window with calendar dates. Makes it very difficult for people to schedule travel or events around a single likely activation date (+/- a day or two). This affects not just activation parties (which is a silly reason IMHO, even more so in the midst of a pandemic), but also mining and infrastructure organizations which will want staff at hand in case there are activation issues.

BIP8 was specifically created to codify our lessons learned from activating segwit into a new activation protocol to be used in the next soft-fork. It got significant review 4 years ago and in the time since, and was strengthened by that review. There is no way that some new state machine for MTP-based activation could receive the same amount of review in the speedy trial timeline. We should use the reviewed mechanisms that are already in place (or very nearly so) and which incorporate lessons learned. If you think there are flaws in BIP8(LOT=false), then start working now on an alternative activation mechanism for the next soft-fork.

Every single one of the above points has been made on the mailing list or in relevant Github discussions. I don’t think that any of them have been adequately addressed by MTP advocates. For example, pointing out that “timewarp could cause more uncertainty than +/- 2 weeks” ignores a few facts: (1) timewarp is NOT a normal condition and would itself be seen as an attack on the network; (2) timewarp would require the coordination of more miners than it would take to block taproot anyway; and (3) having monotonically decreasing estimation error without a two-week floor is critical to scheduling needs.

I think many of us who were involved in that prior work that went into segwit lessons learned and BIP8 review are sitting around dumbfounded and speechless at what has happened. What is the point of doing all that advanced planning if it’s immediately thrown away precisely when it is needed? I’m not surprised if Luke-Jr and Jorge seem obstinate on this. They’ve invested literally years on this problem so that we’d have a solution when it is needed, as it is now, and getting buy-in from the developers at the time… only to have all that casually thrown away. (Because of a freaking coin flip?)”

Reviewed the code of each commit individually and tested its tests. Found one comment that confuses me and one scenario where there could maybe be a better test. See line comments below; complete review and testing journal here: https://gist.github.com/harding/e622323eaf80d620826a7cb74ab3fb40

Tomorrow I’ll try to do a more holistic review of the final mechanism to convince myself all the state transitions are well tested. I’d also like to play around a bit more with the fuzz testing to get a better handle on it.

Overall I’m pleased with the code, especially it’s simplicity (which is emphasized when going commit-by-commit with the PR’s excellent commit organization).

CR-ACK be7d1368d8e98b6f9f692289b7d25ac369bf2984

Will put some additional effort into futzing with the fuzzers and regtests and stuff before a tAck. Seems like there are some fuzzing edge cases that need better coverage, but overall looks great (and those can be added as follow on commits).

Updated for feedback, git diff be7d136..ffe33df should be easy to follow if you’d reviewed the previous head.

[EDIT: bumped the final commit with no changes and re-pushed so as to retrigger CI since the incident has apparently finished]

re-utACK https://github.com/bitcoin/bitcoin/pull/21377/commits/ffe33dfbd4c3b11e3475b022b6c1dd077613de79

API changed to return min_height_activation regardless of value, other assorted comments. I’ll verify the new test case triggers when mutating logic later today to change to a tACK

tACK https://github.com/bitcoin/bitcoin/pull/21377/commits/ffe33dfbd4c3b11e3475b022b6c1dd077613de79

I can’t trigger any more mutation false-successful runs.

utACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79

Verified range-diff from be7d1368d8

review ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79 💈

Also checked that it addresses all my style feedback I left on the other pull ( #21392#pullrequestreview-621891984 )

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79 💈
 4
 5Also checked that it addresses all my style feedback I left on the other pull ( [#21392](/bitcoin-bitcoin/21392/)#pullrequestreview-621891984 )
 6-----BEGIN PGP SIGNATURE-----
 7
 8iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 9pUjIiwv/elbuMaynxLCVMxZAuduzypZgl4ZM/6IysmHZSw9FxhYgXWih3UrIVlHG
10jQpEqUorssfPE0dUeVNG7ToVCs9w5t+DSzJpKJUDzHsESnoXkxvZm/PqWc9q3+vr
11fVv3azJ6LOnyUmxBUy8vZf+FMck66cJq6VPhfG24kL57MgX24JRk8NT0swiuWLS9
12Ekz1N2bSKB7iGeTyhLbyjrNiSNK29BUeT0dqKNTx2jqsnrDAyzD4skZLT8ZubWdY
13kW+njZwz2ZNvvs+ffHcG/EJpCeRAmlDBw+F6TD1bm+WilcmXY2bp9uLiUE1Uo2eR
14kPk2RlQxJtVDGwFpUfGYX7847eePYrMgUsDVsrSCkdqhHh+2UAwlyzqQrr0Z8lAS
15NF02RYBFcFmwbuJ2Z7+Fu5pvlqcZ4q9YHepj8R3BTHKJURCj6ss9FNby0mEUYdfJ
16jF63r+KwMljW3jbox10SEe1T32MZHuOyu4VaBv+hs1Rjna9JgKbZQaHPo1pOAcid
17AlhYcuHs
18=pYnx
19-----END PGP SIGNATURE-----

Timestamp of file with hash 9edb49a6fd6bab8f19ff268a5fde21eb741d236d3f07ee453643334bfb0a3cac -