BIP341: speedy trial activation parameters #1104
Conversation
|
Yes, @jnewbery's nit picking is so powerful it can travel back in time to before a PR is even opened. Beware. Anyway: added rationale links to the PR description, rather than the BIP text. I left the "midnight" there since that's the same language that was used in the csv and segwit deployment text and the epoch timestamps should already clear up any ambiguity. |
|
ACK 93d1b15 |
|
I'm not a fan of documenting (potentially) structural changes to BIP 9 inside the BIP of a specific soft fork. See also #1101 (comment) |
|
ACK 93d1b15 |
|
ACK 93d1b15; verified all timestamps make sense |
I agree with @Sjors . If Speedy Trial may be used for future soft forks it is not Taproot specific. The PR author has talked at length about using Speedy Trial for future soft forks. |
In the interest of getting taproot activated soon, this PR is the fastest option. However, for future soft forks (and the addition of the |
|
As mentioned here, I also agree a new BIP should be written, but I see no harm in it happening later (and there needs to be activation details in BIP341 anyway). |
| @@ -294,7 +294,43 @@ Examples of preimage for sighashing for each of the sighash modes. | |||
|
|
|||
| == Deployment == | |||
|
|
|||
| TODO | |||
| This BIP is deployed concurrently with [[bip-0342.mediawiki|BIP342]]. | |||
There was a problem hiding this comment.
bip340 on itself can not be deployed because it doesn't describe consensus rules. It is deployed only as part of bip341 and 342.
|
I don't support modifying existing BIPs that have been deployed on the network except to correct them when they diverge. Better to make a new BIP entirely as a descriptive summary of ST afterwards. This is OK for now. |
There was a problem hiding this comment.
Re [1] bitcoin/bitcoin#21377 (comment) and changing the state transitions compared to BIP9. The reasons given are "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". The former seems to be only relevant in case >50% of hashrate set inaccurate timestamps and the latter can help if the time for signalling is short. However, one could alternatively imagine just moving the timeout two weeks back if this is a concern. Am I missing the true reasons for changing the state transitions?
It may also be helpful to refer to the state machine diagram.
I've used the following script to estimate the occurance of block 709632, which outputs Tue 09 Nov 2021 with an average block interval of 593 seconds (and Nov 11 with 10 minute intervals). Fwiw, I think the language in the BIP is fine despite the slight difference.
block1=600000
time1=$(bitcoin-cli getblockheader $(bitcoin-cli getblockhash $block1) | jq .time)
bestblockheight=$(bitcoin-cli getblockheader $(bitcoin-cli getbestblockhash) | jq .height)
time2=$(bitcoin-cli getblockheader $(bitcoin-cli getblockhash $bestblockheight) | jq .time)
av_blk_interval=$(awk "BEGIN { print ($time2 - $time1)/($bestblockheight - $block1) }")
echo $av_blk_interval
secstogo=$(awk "BEGIN { OFMT=\"%f\"; print (709632 - $bestblockheight) * $av_blk_interval }")
date -d "+$secstogo sec"
The purpose of changing these state transitions is to resolve one of the issues with BIP 9, specifically the one where miners can choose to skip the activation period. In this case, it was done in order to make MTP versionbits deployments more similar to BIP 8. Both changes are necessary in order to allow the guaranteed single signaling period be sufficient to activate the soft fork. Consider the following scenario: a MTP versionbits deployment where start time is at time A, and timeout time is at time B. Miners then perform a timewarp attack which keeps the MTP before time A, and then once real time passes time B, they timewarp to make MTP past time B.
Thus both changes to the state machine are required in order to have at least one useful signaling period. Now you could have a debate about whether this is actually useful, but I proposed this change because it was one of the benefits provided by BIP 8. By doing these state machine changes, we are able to get similar signaling period guarantees that BIP 8 provides, and doing so seemed to be not invasive as well as somewhat simplifying the state machine |
|
@achow101 Thanks for the explanation. I was aware of the suggested changes to the state machine are required to guarantee a signaling period, so it seems like this is an optimization worth having in ST IMO but also not strictly required. |
|
ACK 93d1b15 |
| } | ||
| return ACTIVE; | ||
|
|
||
| For Bitcoin mainnet, the starttime is epoch timestamp 1619222400 (midnight 24 April 2021 UTC), timeout is epoch timestamp 1628640000 (midnight 11 August 2021 UTC), the threshold is 1815 blocks (90%) instead of 1916 blocks (95%), and the min_activation_height is block 709632 (expected approximately 12 November 2021). |
There was a problem hiding this comment.
Verified the datetime conversions but agree with @jnewbery that the wording is ambiguous.
>> Time.at(1619222400).to_time.utc
=> 2021-04-24 00:00:00 UTC
>> Time.at(1628640000).to_time.utc
=> 2021-08-11 00:00:00 UTC| For Bitcoin mainnet, the starttime is epoch timestamp 1619222400 (midnight 24 April 2021 UTC), timeout is epoch timestamp 1628640000 (midnight 11 August 2021 UTC), the threshold is 1815 blocks (90%) instead of 1916 blocks (95%), and the min_activation_height is block 709632 (expected approximately 12 November 2021). | |
| For Bitcoin mainnet, the starttime is epoch timestamp 1619222400 (2021-04-24 00:00:00 UTC), timeout is epoch timestamp 1628640000 (2021-08-11 00:00:00 UTC), the threshold is 1815 blocks (90%) instead of 1916 blocks (95%), and the min_activation_height is block 709632 (expected approximately 12 November 2021). |
There was a problem hiding this comment.
The base value of 2016 isn't currently mentioned in BIP341.
| For Bitcoin mainnet, the starttime is epoch timestamp 1619222400 (midnight 24 April 2021 UTC), timeout is epoch timestamp 1628640000 (midnight 11 August 2021 UTC), the threshold is 1815 blocks (90%) instead of 1916 blocks (95%), and the min_activation_height is block 709632 (expected approximately 12 November 2021). | |
| For Bitcoin mainnet, the starttime is epoch timestamp 1619222400 (midnight 24 April 2021 UTC), timeout is epoch timestamp 1628640000 (midnight 11 August 2021 UTC), the threshold is 1815 blocks (90% of the difficulty adjustment period of 2016 blocks) instead of 1916 blocks (95%), and the min_activation_height is block 709632 (expected approximately 12 November 2021). |
There was a problem hiding this comment.
it's actually incidental that it lines up with difficulty, can be configured differently if needed.
1815/0.9 = 2016.66, 1814/0.9 = 2015.55 so 2016 is directly implied as the threshold block.
There was a problem hiding this comment.
Sure, but explicit would be better. It's only indirectly referred to in the code example.
|
|
||
| For Bitcoin mainnet, the starttime is epoch timestamp 1619222400 (midnight 24 April 2021 UTC), timeout is epoch timestamp 1628640000 (midnight 11 August 2021 UTC), the threshold is 1815 blocks (90%) instead of 1916 blocks (95%), and the min_activation_height is block 709632 (expected approximately 12 November 2021). | ||
|
|
||
| For Bitcoin testnet3, the starttime is epoch timestamp 1619222400 (midnight 24 April 2021 UTC), timeout is epoch timestamp 1628640000 (midnight 11 August 2021 UTC), the threshold is 1512 blocks (75%), and the min_activation_height is block 0. |
There was a problem hiding this comment.
(The same observations apply for this testnet3 sentence.)
@luke-jr Can you please explain this part or share relevant links where you have already mentioned it in details? I have been following ML, SE, GitHub, IRC, Reddit and Twitter but don't want to assume things that maybe incorrect. |
I think for speedy trial the practical difference between these state changes and adding an extra two weeks to the timeout for mainnet is effectively zero. (For testnet3 two weeks might be more than one period, so that might have measurably different behaviour, but not in any important way) But I think there was a benefit in making it easier for some people to review: if you're reviewing by checking that the final result makes sense, the final result being simpler is a big win. (That's a trade off against making it harder for reviewers who were comfortable with the old code and are verifying the differences, because there are more differences; but it seems to me there aren't that many people who understand the current code in depth for that to work, and so this was a good trade off to make) |
|
ACK 93d1b15 If the timeout date needs to be tweaked a bit to account for community holy days that's also fine by me :-) @luke-jr are you refering to BIP 8 with LOT=true or to the speedy trial variant? In the latter case, I don't think the difference in technical quality is that big. Height based is also not strictly better, because it's a bigger change. That's why I merely have a preference for the height based variant, but it's not a show stopper for me. |
|
ACK Others have mentioned preferences for height-based landmarks and I also agree that height is a better thing to use. However, because we have not finalized consensus for height-based landmarks, pure BIP9 ST is what I think we should go with. I would very much like to see improvements to BIP9 move forward concurrently with and independently of a BIP9 Speedy Trial. But I strongly disagree that improvements to the deployment process should block taproot. However, if the fastest way to taproot is via upgrading BIP9 with height-based landmarks in replacement of MTP, or using a LOT=false BIP8, I would happily ACK that as well. LOT=true I very much NACK however, since it guarantees chain splits in all but the least contentious cases. |
How? If this activates together with BIP 8 it will not lead to a chain split, so they should be compatible. That being said BIP 8 seems superior to me as well. I view ST as a way to resolve deadlock. |
There was a problem hiding this comment.
if you're reviewing by checking that the final result makes sense, the final result being simpler is a big win.
@ajtowns Fair point.
I share the view that the deployment section of BIP 341 isn't the perfect place to specify subtle BIP9 changes, but given the speediness objective and that the rationale has been collected in this thread, it's fine.
ACK 93d1b15
If you have a problem with process or a merge decision on another BIP PR please raise it on that PR. A suboptimal process or merge decision (on the assumption it was that, which would be disputed) is not an argument for a suboptimal process or merge decision on this PR. |
|
does this PR still need to be merged by @luke-jr and if yes, what is the holdup, Luke? |
|
@viaj3ro: It doesn't help when individuals state:
followed by
when that individual knows perfectly well there is an alternative release "Bitcoin Core 0.21.0-based Taproot Client" that is using BIP 8. I am not in favor of a rushed merge here. This PR is not blocking progress for Core or the alternative release and I'd request individuals don't continue to pressure for a fast merge. If the BIP 341 authors (@sipa, @jonasnick, @ajtowns) would weigh in on whether they would ACK or NACK (or have no opinion) including the parameters of the alternative release "Bitcoin Core 0.21.0-based Taproot Client" that would be helpful. They are the same as Bitcoin Core's other than |
|
@michaelfolkson I agree; BIP8 is irrelevant. I guess we have consensus then ;). If you want BIP8 to somehow informationally apply to deployment for BIP-341, once again I am asking to 1) merge this first 2) make a separate PR describing the alternative and seek similar acknowledgement that this PR has received before merging. As it stands and without reference to anything else this PR accurately describes the code available in Bitcoin Core and has been widely ACK'd so it is sticking one's head in the sand whilst claiming the sky isn't blue not to proceed. We've eclipsed @harding's request of merge before this week's OpTech, and the start time is 3 days away, and signalling 10. What goal is it furthering to continue to delay here? Whose agenda is it serving? Feigning neutrality while continuing to instigate tired arguments on this BIP is not a neutral stance. What's stopping you from opening a PR to the BIP describing the UASF parameters? |
I am very happy to open a competing PR to this one with parameters of both Core and the alternative release. If @ajtowns (this PR's author) would like me to do that he can let me know and close this PR. I am not particularly interested in opening a competing PR if this PR can simply be updated with the alternative release's parameters. |
|
You've misinterpreted what I've asked of you, here is what I am saying you can do if you want: gh pr checkout 1104
git checkout -b taproot-uasf-params
vim bip-0341.mediawiki
git add bip-0341.mediawiki
git commit -m "Updated BIP341 to contain informational references for the UASF Client"
gh pr create |
|
@JeremyRubin: Sure I can open a PR to AJ's branch. I will do so tomorrow if that will help this PR along. |
|
This doesn't open a PR to AJ's branch, it opens a PR to the bitcoin/bips repository. This PR should be merged, and your updates can be considered independently when you've prepared them. |
|
@JeremyRubin: In your opinion that is what should happen. Thank you for letting us know your opinion. Again I'd ask you not to pressure for a quick merge. If you wouldn't pressurize Bitcoin Core maintainers for quick merges please don't do it to BIP maintainers either. |
|
It's not my opinion that this should be merged, it's the professional advice and consent of the 13 contributors who have ACKd this revision including 3 out of 3 of the original authors of the BIP. Should Bitcoin Core Maintainers leverage their role similarly against overwhelming consensus without providing any justification, or 'pocket vetoing' by refusing to review in a timely fashion, I would recommend that their commit access be revoked or another maintainer be added. Once again I refer you to https://bitcoincore.org/en/about/. Emphasis added below:
|
@michaelfolkson I don't understand what exactly you're suggesting and why. Feel free to open a separate PR, but I'd prefer to avoid discussing this here because this PR is approved and just waiting to be merged. |
|
@JeremyRubin if @ajtowns thinks this can be merged, it can be merged, however the bitcoincore.org page you are quoting is not relevant here, only the BIP process (as described in BIP1 and BIP2) is applicable to this repository |
|
Indeed. Bitcoin Core processes (as well as ACKs, apart the ones from BIP authors) are largely meaningless to the BIP process. The BIP authors are free to write anything they want in their BIP (as long as it is technically remotely sound). For the exact details on the BIP process, you can refer to https://github.com/bitcoin/bips/blob/master/bip-0002.mediawiki |
|
Yes, I'm merely responding to @michaelfolkson's claim that "If you wouldn't pressurize Bitcoin Core maintainers for quick merges please don't do it to BIP maintainers either." I understand they're separate processes with separate rules, and am pointing out that the bitcoin core community principals state plainly that maintainership is accountable to the community. To the extent that one wants (as Michael seems to) to consider Core procedure, I don't think it makes the point that Michael was hoping which is more or less "let Luke decide when he wants to decide, and let him decide what he wants to". That BIP-0002 doesn't document any sort of accountability to the community in the editor's fulfillment or neglect of their duties seems like a bug that should be fixed, absent any current controversy. Understandably so, as the current situation is surprising to me, but perhaps such a loophole is not surprising to BIP-0002's author, who is also the current BIP Editor. edit: in any case, this seems like a separate issue from the fact that #1104 is RTM, perhaps this discussion should take place on the mailing list or as a PR to BIP-0002 |
|
ACK 93d1b15 We can always have different opinions and disagreements however don't see any good reason to delay this PR being merged after so many ACKs |
That isn't a claim. That is a strong request that if you wouldn't do one please don't do the other. It looks like a new BIP maintainer (@kallewoof) has been proposed to the mailing list so hopefully he can take a look at this if Luke doesn't look at this in the meantime. Please do not make a habit of calling for a new maintainer every time you disagree with a merge decision or the timeliness of a merge. For the sake of @kallewoof (or whoever the new BIP maintainer is) this has ACKs from all the BIP 341 authors and according to existing BIP processes this should be merged. It is complicated by the existence of an alternative release (Bitcoin Core 0.21.0-based Taproot Client) which is using BIP 8 and consistent block height rather than no existing activation BIP and a mixture of MTP and block height (Bitcoin Core). But as I said since the BIP 341 authors have all ACKed the parameters in this BIP 341 PR then according to existing BIP processes this should be merged as is. edit: s/if/since as @gmaxwell suggestion below. No deliberate intent from me on that wording. |
Nice pun? (Or you meant to write bitwise (Last commit message).) LGTM otherwise. I will merge once/if I have editor rights, unless convinced otherwise in the meantime. |
Well, the idea is to add @kallewoof because having just one maintainer is problematic in many ways. Luke himself wanted to add another one in 2018 already and is now asking again. In light of this, I'm not sure if the reasoning for your opposition is warranted. |
|
Agreed. If @kallewoof becomes a BIP maintainer and merges this then this PR situation is resolved. If @gmaxwell has a broader concern about @luke-jr in general in his capacity as a BIP maintainer then he should outline his concerns in detail on the bitcoin-dev mailing list. Personally I think asking someone to merge in activation parameters into a BIP that are not identical to the activation parameters that are being used in an alternative release that @luke-jr is contributing to is not fair to @luke-jr. Either it is important this PR is merged speedily or it isn't important this PR is merged speedily. If it is the former, then merging in activation parameters must somehow convey some significance in "finalizing" what the activation parameters are. If this is true then I am not sure the BIP authors should decide what the finalized activation parameters and what activation BIP is used. I'd rather there was a separation of concerns and BIP authors weren't deciding how their own soft fork BIP is activated. If it is the latter, I don't know why anyone is putting pressure on Luke to merge this speedily. It doesn't matter and is entirely insignificant. Before attacking Luke they should probably reexamine their own logic. |
|
Maintainers are not supposed to be gatekeepers. Holding this PR hostage due to some potentially consensus incompatible client seems strange. That being said, Luke insists the only reason he hasn't merged it, is due to him being too time constrained at the moment. Removing some of the burden by having more than one maintainer seems like the best and only reasonable solution. |
|
(For the avoidance of doubt: Merging this does not imply anything other than the BIP 2 criteria being met; the community has chosen a different activation method than that described here, and this specification does not change the network rules without community support any more than the BIP 10x sequence did) |
@JeremyRubin As I have said many times previously, this is impractical for me to support in Knots. |
What on gods green earth are you talking about? Support for ST has been overwhelming and most in discussions nearly close to unanimous barring a couple people (mostly yourself and folkson) seemingly gaslighting by opposing it with an argument that perplexingly "the community" has already chosen something else. Even your deceptively stated-- deceptive because it explicitly promoted people to choose your option EVEN IF they preferred this proposal-- twitter poll found your own position in a minority: https://twitter.com/LukeDashjr/status/1384196789184581633 a more neutrally presented twitter poll is almost 4:1 against your proposal: |
|
Thank you @luke-jr for merging this PR. A community lead process to fix and restore trust in the BIP process begins here: |
|
@luke-jr noted. Checking out https://github.com/bitcoinknots/bitcoin/blob/0.21.x-knots/src/consensus/params.h, it appears to be BIP9 height based activation parameters, so bitcoin PR 21377 should be able to be merged relatively cleanly, no?
Or are there other considerations I'm missing? edit: struck through point was inaccurate -- github search doesn't index branches other than master/main it seems. |
@luke-jr: I disagree with this statement. The community on the whole seems unconcerned by the BIP 8 versus no BIP and block height versus MTP/block height aspects of Speedy Trial though I share your concerns on a personal level that a huge amount of time was wasted on this and I think what was merged in Core ended up being marginally the inferior solution that went against the majority of reviewers' slight preferences.
@gmaxwell: I have not stated "the community has already chosen something else" or expressed that argument anywhere. I'd ask that you don't blindly associate all of Luke's views with me just because I agree with him on some things. |
@michaelfolkson Please stop doing this.
Isolated miscommunications happen, for sure, but:
I don't see how it's possible to misinterpret these statements. But if I am misreading them I am surely not alone-- my search for a few examples showed a number of similar complaints in public specifically about you:
I'd be more prone to assume you simply changed your mind ... since all those examples are from at least a couple days ago but You could start by actually arguing your positions on their own merits instead of constantly argument-from-authority channeling luke, especially when arguing with people like Pieter who has absurdly more experience and authority on this subject matter. ... or other contributors who, on investigation, have actually been opposing your position. |
All of these were defending against the argument that consensus around BIP 8 in earlier community meetings somehow dissipated afterwards. I have said on multiple occasions that Speedy Trial garnered more community consensus than either BIP 8(LOT=false) or BIP 8(LOT=true). The slightly inferior MTP, undefined BIP Speedy Trial didn't result in the community withdrawing their support for Speedy Trial either. But there was no reason not to use consistent block height and allow Speedy Trial to fit neatly into BIP 8 in my view. Community support for BIP 8 never dissipated.
I am not going to pit Luke against Pieter on soft fork activation experience and authority. I have no idea what Pieter's view is on revised BIP 8, UASF for example. I do know Pieter has stayed out the activation discussion for months (wisely in my view). But whoever it is, I make no excuse for channeling Luke, Pieter, yourself, Rusty or whoever else who has been thinking about these topics for many years more than I have. |
Adds mainnet and testnet3 activation parameters. This uses the "speedy trial" approach [0] with some refinements to the exact mechanism [1], as implemented in bitcoin/bitcoin#21377 . Parameters are selected based on previous discussions [2] [3] [4]. The parameters are:
In terms of block heights, that will likely mean the first signalling period for mainnet should start at height 681408, approximately April 29th UTC, and will likely mean signalling will continue for 8 retarget periods until height 697536 (which if blocks arrived at 10 minute intervals, would be due around 2021-08-20).
[0] https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-March/018583.html
[1] bitcoin/bitcoin#21377 (comment)
[2] (90%) https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-February/018425.html
[3] (May/August/Nov) https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-March/018715.html
[4] (specific parameters) #1081