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

BIP 8: Add minimum activation height #1080

Merged
merged 1 commit into from Mar 20, 2021

Conversation

achow101
Copy link
Member

@achow101 achow101 commented Mar 8, 2021

The Speedy Trial activation proposal introduces a new parameter named minimum_activation_height. This PR adds that to BIP 8 so that such Speedy Trials can be implemented using entirely BIP 8.

@achow101 achow101 changed the title Add minimum activation height to BIP 8 BIP 8: Add minimum activation height Mar 8, 2021
Copy link
Contributor

@michaelfolkson michaelfolkson left a comment

Choose a reason for hiding this comment

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

A couple of nits but generally this looks good. Additional mega nit (!) that the font looks different (weird because it is still Arial) on the state transition diagram but its fine.

Directly below the state transition diagram it still says "Note that when lockinontimeout is true, the LOCKED_IN state will be reached no later than at a height of timeoutheight, and ACTIVE will be reached no later than at a height of timeoutheight + 2016."

This needs to be changed so that it says "ACTIVE will be reached no later than at the minimum_activation_height".

minimum_activation_height still applies even when lockinontimeout is true.

@@ -50,12 +51,13 @@ The following guidelines are suggested for selecting these parameters for a soft
# '''startheight''' should be set to some block height in the future when a majority of economic activity is expected to have upgraded to a software release including the activation parameters. Some allowance should be made for potential release delays. It should be rounded up to the next height which begins a retarget period for simplicity.
# '''timeoutheight''' should be set to a block height when it is considered reasonable to expect the entire economy to have upgraded by, probably at least 1 year, or 52416 blocks (26 retarget intervals) after '''startheight'''.
# '''threshold''' should be 1815 blocks (90% of 2016), or 1512 (75%) for testnet.
# '''minimum_activation_height''' should be set to several retarget periods in the future if the startheight is to be very soon after a release. This allows more time to be spent in the locked in state so that nodes can upgrade. For a typical soft fork, this may be set to 0 to have the locked in state be the traditional single retarget period.
Copy link
Contributor

Choose a reason for hiding this comment

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

The minimum_activation_height is added primarily (imo) because the timeoutheight is short for Speedy Trial rather than because the startheight is soon after a release (though this is also true in Speedy Trial and also a factor). We would still want minimum_activation_height even if the startheight wasn't soon after a release.

Copy link
Member Author

Choose a reason for hiding this comment

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

My understanding is that having a longer locked in period is so that the startheight can be sooner. It allows for the upgrade time to be moved from before the start height to after locked in. Perhaps @roconnor-blockstream or @harding could comment on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

acho101 is correct. I think under previous considerations a short activation would have it's start date delayed to give time for nodes to update. Instead ST uses a miniumum_activation_height to allow for a start date soon after release while still giving time for nodes up to update.

bip-0008.mediawiki Outdated Show resolved Hide resolved
@achow101
Copy link
Member Author

Directly below the state transition diagram it still says "Note that when lockinontimeout is true, the LOCKED_IN state will be reached no later than at a height of timeoutheight, and ACTIVE will be reached no later than at a height of timeoutheight + 2016."

This needs to be changed so that it says "ACTIVE will be reached no later than at the minimum_activation_height".

minimum_activation_height still applies even when lockinontimeout is true.

Changed to "no earlier than minimum_activation_height". Also added a sentence to discuss the situation where the minimum activation height is less than the timeoutheight + 2016.

bip-0008.mediawiki Outdated Show resolved Hide resolved
bip-0008.mediawiki Outdated Show resolved Hide resolved
Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

Concept ACK

Note that when '''lockinontimeout''' is true, the LOCKED_IN state will be reached no later than at a height of '''timeoutheight''', and ACTIVE will be reached no later than at a height of '''timeoutheight + 2016'''.
Note that when '''lockinontimeout''' is true, the LOCKED_IN state will be reached no later than at a height of '''timeoutheight'''.
Regardless of value of '''lockinontimeout''', ACTIVE will be reached no earlier than at a height of '''minimum_activation_height'''.
When '''minimum_activation_height''' is less than or equal to '''timeoutheight + 2016''', ACTIVE will be reached no later than '''timeoutheight + 2016'''.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: probably (slightly) easier to understand if this wasn't special cased and instead a note would be added to the parameter selection guidelines that minimum_activation _height must be at least timeoutheight + 2016. Then you could also remove the After at least one retarget period of LOCKED_IN [...] and change the sentence to something like "From the LOCKED_IN state we transition to ACTIVE if the minimum activation height is reached."

Copy link
Contributor

Choose a reason for hiding this comment

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

Afaics right now the diagram isn't correct, because if minimum_activation_height = 0 then it looks like LOCKED_IN -> ACTIVE happens without a retarget period. So should either make minimum_activation_height >= timeoutheight + 2016 or change arrow in diagram to height >= max(timeoutheight + 2016, minimum_activation_height).

Copy link
Member Author

Choose a reason for hiding this comment

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

minimum_activation_height can't be at least timeoutheight + 2016 as otherwise the normal deployment won't activate one retarget period after LOCKED_IN.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, nevermind. In that case the diagram isn't made worse by this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I personally intended that a min-activation-height that is less than timeoutheight + 2016 would be allowed in general. That said, my personal intentions may not be all that relevant.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jonasnick "it looks like LOCKED_IN -> ACTIVE happens without a retarget period" -- if that were going to happen, there'd be an arrow from "STARTED -> ACTIVE" without going through "LOCKED_IN" -- if you enter a state you're there for a retarget period, and you get to follow one of the arrows (maybe just the loop back and stay in the same state) when you hit the next retarget period.

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

ACK

bip-0008.mediawiki Outdated Show resolved Hide resolved
@michaelfolkson
Copy link
Contributor

ACK

bip-0008.mediawiki Outdated Show resolved Hide resolved
bip-0008.mediawiki Outdated Show resolved Hide resolved
bip-0008/states.dot Show resolved Hide resolved
Note that when '''lockinontimeout''' is true, the LOCKED_IN state will be reached no later than at a height of '''timeoutheight''', and ACTIVE will be reached no later than at a height of '''timeoutheight + 2016'''.
Note that when '''lockinontimeout''' is true, the LOCKED_IN state will be reached no later than at a height of '''timeoutheight'''.
Regardless of value of '''lockinontimeout''', ACTIVE will be reached no earlier than at a height of '''minimum_activation_height'''.
When '''minimum_activation_height''' is less than or equal to '''timeoutheight + 2016''', ACTIVE will be reached no later than '''timeoutheight + 2016'''.
Copy link
Contributor

Choose a reason for hiding this comment

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

@jonasnick "it looks like LOCKED_IN -> ACTIVE happens without a retarget period" -- if that were going to happen, there'd be an arrow from "STARTED -> ACTIVE" without going through "LOCKED_IN" -- if you enter a state you're there for a retarget period, and you get to follow one of the arrows (maybe just the loop back and stay in the same state) when you hit the next retarget period.

@Sjors
Copy link
Member

Sjors commented Mar 20, 2021

ACK e86fd7c

bip-0008.mediawiki Outdated Show resolved Hide resolved
bip-0008.mediawiki Outdated Show resolved Hide resolved
bip-0008.mediawiki Outdated Show resolved Hide resolved
@luke-jr luke-jr merged commit dacd6a2 into bitcoin:master Mar 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants