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

consensus: adjust bip9 periods #1124

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

fdoving
Copy link
Contributor

@fdoving fdoving commented Nov 7, 2021

10x the default counting periods.
Previous period was 2016 blocks or about 1.4 day.
Proposed period is 20160 blocks or about 14 days.

  • The LOCKED_IN period is extended from 1.4 to 14 days, during which non upgraded nodes will be warned of the upcoming fork.
  • 10 times as expensive to potentially try to force activation with rented hashpower.
  • Slower activation. Previous minimum activation time from counting starts, about 3 days, with this proposal about 30 days.

@fdoving
Copy link
Contributor Author

fdoving commented Nov 7, 2021

I suggest to change the confirmation window, not only for P2SH assets, as an override, but as a feature for all changes to mainnet.

Copy link
Contributor

@hans-schmidt hans-schmidt left a comment

Choose a reason for hiding this comment

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

Approval is pending a realistic choice of nOverrideRuleChangeActivationThreshold

@TronBlack
Copy link
Collaborator

You can't change the defaults. They need to stay the same because otherwise as the code is syncing it will be expecting different times for all previous hardforks (DGW, etc). Just do the overrides. The code would work for anyone just upgrading, but break for new users that sync the chain from scratch.

10x the default counting periods.
Previous period was 2016 blocks or about 1.4 day.
Proposed period is 20160 blocks or about 14 days.

- The LOCKED_IN period is extended from 1.4 to 14 days, during which non upgraded nodes will be warned of the upcoming fork.
- 10 times as expensive to potentially try to force activation with rented hashpower.
- Slower activation. Previous minimum activation time from counting starts, about 3 days, with this proposal about 30 days.
@fdoving
Copy link
Contributor Author

fdoving commented Jan 18, 2022

You can't change the defaults. They need to stay the same because otherwise as the code is syncing it will be expecting different times for all previous hardforks (DGW, etc). Just do the overrides. The code would work for anyone just upgrading, but break for new users that sync the chain from scratch.

Fixed.

Copy link
Contributor

@hans-schmidt hans-schmidt left a comment

Choose a reason for hiding this comment

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

Tron, thanks for the clear explanation. Last year we adopted a policy that PRs which touch consensus can't get PR approvals until after explicit testing including sync from zero (which showed that the P2SH code needed tweaking). I was unsure about changing the defaults but wasn't worried since the test would definitely catch such a problem before PR approval. This PR is a good reminder why we have that policy. Again, thanks for the assist. Your expertise is very much needed and appreciated.

I do want to get a dicussion going about nOverrideRuleChangeActivationThreshold, which is why I focused on that. Given the importance of P2SH, the difficulty identifying miners, and the long time constants we have chosen, I think we shouldn't set it too high. What are your thoughts?

@fdoving fdoving added this to the 4.7.0 milestone Jan 22, 2022
@TronBlack
Copy link
Collaborator

Approved as modified to just the overrides.

It would make sense to run this change against the main chain before and after BIP9 activation.

A force of the BIP9 activation (in code) could be done, then a full chain sync. It should continue to run as long as no P2SH (or other BIP9 wrapped transactions are published).

I think it makes sense to have a longer activation threshold this time around. If we've agreed that all future ones should also be longer, then a comment around where the activation threshold override gets set should suffice, so that future threshold overrides are also set to 10x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants