fix(types): Only require extension signature if extensions are enabled#3565
fix(types): Only require extension signature if extensions are enabled#3565andynog merged 6 commits intocometbft:v0.38.xfrom
Conversation
d2f3f5e to
4dfd196
Compare
|
Could you please open an issue first? Are you using a remote signer like tmkms? If so, what version? There was a bug recently: #2711, which is very similar from the looks of it. |
|
Also, can you check whether in |
4dfd196 to
0a9ce40
Compare
|
The first proposed fix does not solve the problem fully in seda mainnet. Additional debugging indciates the root cause is this line: When extensionsEnabled = false (correct, previously assumed incorrect) We are testing this proposed in seda mainnet. Our proposed fix matches the new logic already in the main branch. |
0a9ce40 to
c3d98ea
Compare
|
@chillyvee please add a changelog entry 🙏 |
|
Changelog added. Seda mainnet node is stable for 16 hours. Previous panics appeared within 8-72 hours. |
types/vote.go
Outdated
|
|
||
| vote.ExtensionSignature = nil | ||
| if extensionsEnabled { | ||
| isNil := vote.BlockID.IsZero() |
There was a problem hiding this comment.
This fix is disabling various sanity checks we wouldn't like to disable:
- Make sure that a prevote does not contain an extension signature
- Make sure that
nilprecommit does not contain an extension signature
Therefore this fix needs to "undo" the XOR in the if into two conditions, and only include one in the if extensionsEnabled.
I will post a diff here to explain my point
There was a problem hiding this comment.
This is how I believe the fix should look like:
diff --git a/types/vote.go b/types/vote.go
--- a/types/vote.go
+++ b/types/vote.go
@@ -430,13 +430,17 @@ func SignAndCheckVote(
isNil := vote.BlockID.IsZero()
extSignature := (len(v.ExtensionSignature) > 0)
- if extSignature == (!isPrecommit || isNil) {
- // Non-recoverable because the vote is malformed
+ if extSignature && (!isPrecommit || isNil) {
+ // Non-recoverable because the vote is malformed (wrongly contains an extension signature)
return false, &ErrVoteExtensionInvalid{ExtSignature: v.ExtensionSignature}
}
vote.ExtensionSignature = nil
if extensionsEnabled {
+ if !extSignature && isPrecommit && !isNil {
+ // Non-recoverable because the vote is malformed (extension signature missing for a precommit)
+ return false, &ErrVoteExtensionInvalid{ExtSignature: v.ExtensionSignature}
+ }
vote.ExtensionSignature = v.ExtensionSignature
}
There was a problem hiding this comment.
However, this fix may only be buying you time. If the chain decides to start using vote extensions (in the SDK, it would be via a gov proposal)... and your remote signer still doesn't sign vote extensions, it will start failing again.
There was a problem hiding this comment.
Our internal fix is actually "if extSignature && (!isPrecommit || isNil) " as you suggest and would be supportive of the format you suggest. We agree the previous code misses one of the checks. The new commit includes your suggestion.
The original incomplete fix matches the main branch which is missing:
+ if extSignature && (!isPrecommit || isNil) {
+ // Non-recoverable because the vote is malformed (wrongly contains an extension signature)
return false, &ErrVoteExtensionInvalid{ExtSignature: v.ExtensionSignature}
}
Does the main branch also need to check this condition?
Also Horcrux signer supports vote extensions. Appreciate the check.
There was a problem hiding this comment.
Does the main branch also need to check this condition?
Thanks for pointing out. Taking care of it (#3642)
|
Updated to include condition checks as suggested by @sergio-mena |
|
|
On the last comment from @cason , I had something similar in #3649. @chillyvee you might need to do |
Commit added with changed files. Protobuf lint check appears to be failing. Adding version: v1 to match other existing files Trying version: v2 Does not work and other tags in this branch series doesn't need a version set in /buf.yaml Reverting back to original buf.yaml. Change proto/buf.yaml add version: v2 make proto-gen Reverting back to original proto/buf.yaml. make proto-gen needs version: v1 buf.yaml and proto/buf.yaml are both unchanged. Is there a reason protobuf lint should fail on buf.yaml? Any ideas @sergio-mena ? $GOPATH/bin/protoc-gen-gocosmos does not exist |
ac05675 to
008bab0
Compare
…n signing a vote (#3649) Closes #3642 When vote extensions are disabled, we still need to (sanity-)check that: * Prevotes don't contain an extension signature * `nil` precommits don't contain an extension signature This PR is re-enabling those checks, in alignment with #3565 in `v0.38.x` --- #### PR checklist - ~[ ] Tests written/updated~ - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - ~[ ] Updated relevant documentation (`docs/` or `spec/`) and code comments~ - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
Thanks @sergio-mena for figuring it out (#3565 (comment)) --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
|
@chillyvee I spent some time diagnosing. Can you please cherry-pick 3320ae3 to see if that fixes the problem? |
4f7e1f8 to
024da4b
Compare
Picked your commit Also updated commit messages to (types) instead of the original (consensus) |
024da4b to
584515f
Compare
|
Green check mark. It worked @sergio-mena |
|
@sergio-mena @cason @melekes - Please share your twitter handles so we can thank you in a tweet for all the help |
|
Is it possible to tag a release of v0.38 for this fix? The v0.38 branch will be live on cosmoshub gaiad in under a week. |
Yes, we'll try to create a new release today. |
|
|
@chillyvee I'm inactive on X (@chav8 - last tweet in 2012), so if you'd like to refer to me, just say "Sergio Mena" or link to my GitHub profile, thanks. |
…n signing a vote (#3649) Closes #3642 When vote extensions are disabled, we still need to (sanity-)check that: * Prevotes don't contain an extension signature * `nil` precommits don't contain an extension signature This PR is re-enabling those checks, in alignment with #3565 in `v0.38.x` --- #### PR checklist - ~[ ] Tests written/updated~ - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - ~[ ] Updated relevant documentation (`docs/` or `spec/`) and code comments~ - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec (cherry picked from commit a33b8ac) # Conflicts: # .changelog/v1.0.0/bug-fixes/3642-re-enable-vote-extension-checks.md # go.mod # go.sum
…n signing a vote (backport #3649) (#4789) Closes #3642 When vote extensions are disabled, we still need to (sanity-)check that: * Prevotes don't contain an extension signature * `nil` precommits don't contain an extension signature This PR is re-enabling those checks, in alignment with #3565 in `v0.38.x` --- #### PR checklist - ~[ ] Tests written/updated~ - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - ~[ ] Updated relevant documentation (`docs/` or `spec/`) and code comments~ - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request #3649 done by [Mergify](https://mergify.com). --------- Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
Closes #3570
extEnabled is not set to false for nil precommit which causes SiteAndCheckVote() to panic
This is seen in seda chain with the following error:
PR checklist
.changelog(we use unclog to manage our changelog)docs/orspec/) and code comments