Divergences in comparison with #9620. Part 4: Other changes spotted#9927
Divergences in comparison with #9620. Part 4: Other changes spotted#9927sergio-mena merged 9 commits intofeature/abci++veffrom
Conversation
|
|
||
| if vote.Height < 0 { | ||
| return errors.New("negative Height") | ||
| if vote.Height <= 0 { |
There was a problem hiding this comment.
What's the impact of votes having a height of zero in previous versions, like v0.37 and v0.34?
There was a problem hiding this comment.
A vote with height of 0 would mean there is a bug.
I just checked Vote::ValidateBasic in v0.37.x and in v0.34.x and we can see the same problem with this "if". It is likely that we never have a vote with height 0 in production in v0.37.x or in v0.34.x, and so this check wouldn't be catching any existing problem? (wishful thinking?).
Anyway, the fact that UTs pass in v0.37.x and in v0.34.x shows there is a gap in UTs.
Here's what I propose we can do: in order not to delay this PR, let us:
- Revert this change in this PR
- Open an issue to fix it on all maintained branches, and add a UT that fails without this change and passes with this change (TDD-style)
What do you think @thanethomson ?
There was a problem hiding this comment.
If the correct behaviour is to reject votes with zero height, we should keep it. The same behaviour should be applied to older versions, but it doesn't seem urgent right now to do so, especially since we make no compatibility guarantees across "major" releases.
So if you have time, please add an issue to keep track of that so we don't forget to come back to it?
There was a problem hiding this comment.
Thanks, makes sense. I added an extra point in #9916 to deal with this in v0.34.x and v0.37.x. We can convert it into an issue at any time.
Co-authored-by: Lasaro <lasaro@informal.systems>
| extensionsEnabled bool, | ||
| ) (bool, error) { | ||
| v := vote.ToProto() | ||
| if err := privVal.SignVote(chainID, v); err != nil { |
There was a problem hiding this comment.
In what cases is failing to sign a vote recoverable?
There was a problem hiding this comment.
That's a good question.
I added the recoverable flag because this is part of a small refactoring, and didn't want to modify the original logic in any way... in the sense I didn't want to add a panic where there used to be none.
If you look at the changes of this PR in the only callsite in production code (consensus/state.go line 2324) you will see that there was no panic when SignVote failed, so I wanted to keep that (battle-tested) behavior unchanged. However, the new checks (introduced by SignAndCheckVote, which involve vote extensions are situations we can't (or don't want to) recover from.
I'll see if I can add a meaningful comment.
Contributes to #9887
This PR contains other minor differences spotted between #9620 and the cherry-picked vote extension branch.
After this PR, all differences between these two branches (#9620 and the cherry-picked) will now be integrated in branch
feature/abci++vefThese are the differences included in this PR (can be reviewed independently by selecting the right commit):
IsZeromethod at the places it was removed when solving cherry-pick conflictsPR checklist
CHANGELOG_PENDING.mdupdated, or no changelog entry neededdocs/) and code comments, or nodocumentation updates needed