Tighten the checks we do on vote extensions in e2e app#509
Tighten the checks we do on vote extensions in e2e app#509sergio-mena merged 5 commits intofeature/abci++veffrom
Conversation
5b038a9 to
a1430d0
Compare
| } | ||
| continue | ||
| } | ||
| if len(vote.VoteExtension) == 0 { |
There was a problem hiding this comment.
One last question related to the comment above. Here the length is allowed to be 0 (as it is supposed to be), but then it will fail later? I am just asking to confirm that you don't want to impose your more stringent condition here as well.
There was a problem hiding this comment.
That's an excellent question. :-)
The answer is coming in one of my next PRs. The problem is that the current ABCI data structures can only say if the sender of the vote "signed or not" (i.e., a boolean). But if the sending validator has signed for nil, SignedLastBlock will also be true; however as it is a nil precommit it won't have an extension.
It took me a few hours of troubleshooting with e2e tests to find this out.
We discussed this in @ebuchman's office hours on Monday, and we decided we're gonna fix the ABCI interface (a boolean is not enough), since anyway we're introducing breaking changes.
This is what's coming in one of my next PRs.
jmalicevic
left a comment
There was a problem hiding this comment.
Other than the comment, LGTM.
Contributes to #485
The current checks on vote extensions in the e2e app take vote extensions as something always optional. This is OK, but we can leverage the app to more thoroughly test several cases:
PrepareProposal/ProcessProposaldon't have extensions,ExtendVote/VerifyVoteExtensiondo.Nilvotes in a commitPR checklist
.changelog(we use unclog to manage our changelog)docs/orspec/) and code comments