Fix deficiency in ABCI interface: SignedLastBlock#540
Merged
sergio-mena merged 5 commits intofeature/abci++veffrom Mar 17, 2023
Merged
Fix deficiency in ABCI interface: SignedLastBlock#540sergio-mena merged 5 commits intofeature/abci++veffrom
SignedLastBlock#540sergio-mena merged 5 commits intofeature/abci++veffrom
Conversation
This was referenced Mar 16, 2023
Closed
jmalicevic
reviewed
Mar 16, 2023
| if vote.BlockIdFlag == cmtproto.BlockIDFlagAbsent || vote.BlockIdFlag == cmtproto.BlockIDFlagNil { | ||
| if len(vote.VoteExtension) != 0 { | ||
| return 0, fmt.Errorf("non-empty vote extension at height %d, which has extensions disabled", currentHeight) | ||
| return 0, fmt.Errorf("non-empty vote extension at height %d, for a vote with blockID flag %d", |
Collaborator
There was a problem hiding this comment.
Essentially, if this validator had voted correctly, and there is an extension, the flag had to have been Commit?
Collaborator
Author
There was a problem hiding this comment.
Exactly:
- BlockIDFlagAbsent means it didn't precommit so, how come we got a vote extension?
- BlockIDFlagNil means it precommitted for
nil, butnilprecommits don't have vote extensions
In both cases the info we get is contradictory, so we should reject it.
Collaborator
Author
There was a problem hiding this comment.
Oh, did you mean that we could simplify the condition in line 583 like this?
if vote.BlockIdFlag != cmtproto.BlockIDFlagCommit {
jmalicevic
reviewed
Mar 16, 2023
jmalicevic
approved these changes
Mar 16, 2023
Collaborator
jmalicevic
left a comment
There was a problem hiding this comment.
Minor comments, otherwise good.
Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Contributes to #485
Current ABCI 2.0 implementation has
VoteInfoandExtendedVoteInfoto send vote signature information to the application at several points in the interaction:PrepareProposalProcessProposalFinalizeBlockThe problem with
VoteInfoandExtendedVoteInfois that they contain abool, calledSignedLastBlock, which collapses several cases: (a) did the sending validator precommit fornil? (b) did it precommit for the decided block? (c) did they not precommit (or no precommit received, absent)?SignedLastBlockis collapsing cases (a) and (b) into valuetrueof the boolean, hiding potentially useful information to the application.With vote extensions, this has become a bigger problem, as we are unable to "ensure" from a testing application that the vote extensions are present or absent at the right time. If we (CometBFT team) are unable to test this properly, then real apps may have a very hard time troubleshooting potential problems on the field, due to some info being trapped south of ABCI.
After a discussion with @ebuchman, we have decided to fix this problem, which was already fixed long ago in the block-related protos (see discussion). The conclusion was that this is the right moment, as we are heavily changing ABCI in this release, and it's not certain when we will need to make substantial changes to it in the mid-term.
This PR contains the changes needed in the protobufs and in the code.
PR checklist
.changelog(we use unclog to manage our changelog)docs/orspec/) and code comments