-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
abci++: Vote extension cleanup #9848
abci++: Vote extension cleanup #9848
Conversation
* Split vote verification/validation based on vote extensions Some parts of the code need vote extensions to be verified and validated (mostly in consensus), and other parts of the code don't because its possible that, in some cases (as per RFC 017), we won't have vote extensions. This explicitly facilitates that split. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Only sign extensions in precommits, not prevotes Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update privval/file.go Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Apply suggestions from code review Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Temporarily disable extension requirement again for E2E testing Signed-off-by: Thane Thomson <connect@thanethomson.com> * Reorganize comment for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * Leave vote validation and pre-call nil check up to caller of VoteToProto Signed-off-by: Thane Thomson <connect@thanethomson.com> * Split complex vote validation test into multiple tests Signed-off-by: Thane Thomson <connect@thanethomson.com> * Universally enforce no vote extensions on any vote type but precommits Signed-off-by: Thane Thomson <connect@thanethomson.com> * Make error messages more generic Signed-off-by: Thane Thomson <connect@thanethomson.com> * Verify with vote extensions when constructing a VoteSet Signed-off-by: Thane Thomson <connect@thanethomson.com> * Expand comment for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add extension check for prevotes prior to signing votes Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix supporting test code to only inject extensions into precommits Signed-off-by: Thane Thomson <connect@thanethomson.com> * Separate vote malleation from signing in vote tests for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add extension signature length check and corresponding test Signed-off-by: Thane Thomson <connect@thanethomson.com> * Perform basic vote validation in CommitToVoteSet Signed-off-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
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.
Seems like a perfect cherry-pick to me 🙂
Weird that the one test group keeps failing though - seems like it's different tests that fail each time though?
There's I'll push the fix after the dinner (I don't want to push it until I test it well) |
// TODO(thane): Remove extension length check once | ||
// https://github.com/tendermint/tendermint/issues/8272 is | ||
// resolved. | ||
if len(vote.Extension) > 0 && len(vote.ExtensionSignature) == 0 { |
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.
The details may be a bit foggy to me, but our decision was to ensure that, once vote extensions are enabled, the vote extension must always have a signature regardless of the extension length.
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.
Looking at #9396, if I understand correctly, that will come in a follow-up PR?
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.
Great, then no issue!
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.
@thanethomson is right, this is changing in upcoming PRs
Looks correct, left a comment making sure my understanding is correct. |
Closes #9846
This is a PR cherry-picking one single PR: #8402
The contents of this patch were necessary in preparation for implementing the solution for vote extension propagation (which will be in an upcoming PR)
N.B: The changelog will be updated once branch
feature/abci++vef
is merged back intomain
PR checklist
CHANGELOG_PENDING.md
updated, or no changelog entry neededdocs/
) and code comments, or nodocumentation updates needed