-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Update BIP9 assignments list #400
Conversation
| defined | ||
| 2016-03-01 00:00:00 | ||
| 2017-05-01 00:00:00 | ||
| active since #770111 | ||
| active since #770112 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether this should be #770111 or #770112 depends on how the column is defined.
You're right that the block mining on top of block #770111 is the first for which the CSV softfork rules are activated, which is block #770112.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it should say "activated on #770112"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's #770111 that changes the state to "active", remember that it checks the state for previous block to see what rules to enforce. #770112 is the second block that has "active" state, but the first for which the rules are enforced.
I tend to agree that mentining #770112 seems more intuitive, but this is a more subtle distinction than most people take into account :) Very easy to introduce off-by-one errors here.
To be precise:
VersionBitsBlockState(chainActive[770110], consensusParams, Consensus::DEPLOYMENT_CSV)
isTHRESHOLD_LOCKED_IN
VersionBitsBlockState(chainActive[770111], consensusParams, Consensus::DEPLOYMENT_CSV)
isTHRESHOLD_ACTIVE
VersionBitsBlockState(chainActive[770112], consensusParams, Consensus::DEPLOYMENT_CSV)
isTHRESHOLD_ACTIVE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case "active since #770112" is correct, which was my original thinking. Alternatively "enforced since #770112" would make the distinction that #770112 is the first block on which the new rules are applied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@laanwj Sounds like you're talking about implementation details there... If #770112 is the first block the rules are enforced in, how nodes implement that logic is not really relevant...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest to change the text to "enforced since #XXX" to prevent confusion then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@laanwj You're calling the function incorrectly. The first argument to VersionBitsBlockState is pindexPrev. Using BIP9 terminology, block 770112 is the first which has state ACTIVE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
Someone else must have made that mistake too then, otherwise this number wouldn't be one-off in the first place.
@mappum You can add CSV activation on mainnet now #419328 |
It seems a majority in this thread agree that the activation heights in the document should contain the first block which has state |
I think 'active' is perfect, as it is the name of the state in BIP9. If
there is confusion about what active means, we should clarify the BIP
itself.
|
@sipa OK then. You should ACK this so it can get merged. |
Yes, let's just merge this. I had the definition wrong (probably my own fault, not the BIP's). |
ACK |
This PR updates the BIP9 bit assignments list since the
segwit
deployment is now active on testnet. It also changes thecsv
testnet activation height from "770111" to "770112" since that is the first block where the deployment is active.