fix: Dedup Vote Extensions in ValidateVoteExtensions
#18895
Conversation
WalkthroughThe change implements a mechanism to ensure vote extensions submitted by validators are not duplicated. This is achieved by introducing a cache to track submissions, which helps prevent the same vote extension from being processed more than once. This enhances the security of the voting process by enforcing the required quorum and preventing manipulation by malicious actors. Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChat with CodeRabbit Bot (
|
|
What do you think if instead of de-duplicating we just return an error? That way the rest of the validators could reject the proposal during ProcessProposal, otherwise it could open up the possibility to lots of duplicate data going around |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
baseapp/abci_utils_test.go
Outdated
| // check ValidateVoteExtensions works with duplicate votes | ||
| func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsDuplicateVotes() { | ||
| ext := []byte("vote-extension") | ||
| cve := cmtproto.CanonicalVoteExtension{ | ||
| Extension: ext, | ||
| Height: 2, | ||
| Round: int64(0), | ||
| ChainId: chainID, | ||
| } | ||
|
|
||
| bz, err := marshalDelimitedFn(&cve) | ||
| s.Require().NoError(err) | ||
|
|
||
| extSig0, err := s.vals[0].privKey.Sign(bz) | ||
| s.Require().NoError(err) | ||
|
|
||
| ve := abci.ExtendedVoteInfo{ | ||
| Validator: s.vals[0].toValidator(333), | ||
| VoteExtension: ext, | ||
| ExtensionSignature: extSig0, | ||
| BlockIdFlag: cmtproto.BlockIDFlagCommit, | ||
| } | ||
|
|
||
| llc := abci.ExtendedCommitInfo{ | ||
| Round: 0, | ||
| Votes: []abci.ExtendedVoteInfo{ | ||
| ve, | ||
| ve, | ||
| ve, | ||
| }, | ||
| } | ||
| // expect fail (duplicate votes) | ||
| s.Require().Error(baseapp.ValidateVoteExtensions(s.ctx, s.valStore, 3, chainID, llc)) | ||
| } |
There was a problem hiding this comment.
The new test function TestValidateVoteExtensionsDuplicateVotes correctly simulates the scenario of duplicate vote extensions and asserts that an error is expected. This aligns with the PR's objective to prevent duplicate vote extensions from being processed.
However, it would be beneficial to include a comment explaining why the error is expected and what specific error message or type is anticipated. This would improve the maintainability and clarity of the test for future developers.
+ // TestValidateVoteExtensionsDuplicateVotes ensures that an error is returned
+ // when duplicate vote extensions are detected. This is critical for preventing
+ // potential security vulnerabilities as described in issue #18893.
func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsDuplicateVotes() {
...
+ // The error should specifically indicate the presence of duplicate vote extensions.
s.Require().Error(baseapp.ValidateVoteExtensions(s.ctx, s.valStore, 3, chainID, llc))
}Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // check ValidateVoteExtensions works with duplicate votes | |
| func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsDuplicateVotes() { | |
| ext := []byte("vote-extension") | |
| cve := cmtproto.CanonicalVoteExtension{ | |
| Extension: ext, | |
| Height: 2, | |
| Round: int64(0), | |
| ChainId: chainID, | |
| } | |
| bz, err := marshalDelimitedFn(&cve) | |
| s.Require().NoError(err) | |
| extSig0, err := s.vals[0].privKey.Sign(bz) | |
| s.Require().NoError(err) | |
| ve := abci.ExtendedVoteInfo{ | |
| Validator: s.vals[0].toValidator(333), | |
| VoteExtension: ext, | |
| ExtensionSignature: extSig0, | |
| BlockIdFlag: cmtproto.BlockIDFlagCommit, | |
| } | |
| llc := abci.ExtendedCommitInfo{ | |
| Round: 0, | |
| Votes: []abci.ExtendedVoteInfo{ | |
| ve, | |
| ve, | |
| ve, | |
| }, | |
| } | |
| // expect fail (duplicate votes) | |
| s.Require().Error(baseapp.ValidateVoteExtensions(s.ctx, s.valStore, 3, chainID, llc)) | |
| } | |
| // TestValidateVoteExtensionsDuplicateVotes ensures that an error is returned | |
| // when duplicate vote extensions are detected. This is critical for preventing | |
| // potential security vulnerabilities as described in issue #18893. | |
| func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsDuplicateVotes() { | |
| ext := []byte("vote-extension") | |
| cve := cmtproto.CanonicalVoteExtension{ | |
| Extension: ext, | |
| Height: 2, | |
| Round: int64(0), | |
| ChainId: chainID, | |
| } | |
| bz, err := marshalDelimitedFn(&cve) | |
| s.Require().NoError(err) | |
| extSig0, err := s.vals[0].privKey.Sign(bz) | |
| s.Require().NoError(err) | |
| ve := abci.ExtendedVoteInfo{ | |
| Validator: s.vals[0].toValidator(333), | |
| VoteExtension: ext, | |
| ExtensionSignature: extSig0, | |
| BlockIdFlag: cmtproto.BlockIDFlagCommit, | |
| } | |
| llc := abci.ExtendedCommitInfo{ | |
| Round: 0, | |
| Votes: []abci.ExtendedVoteInfo{ | |
| ve, | |
| ve, | |
| ve, | |
| }, | |
| } | |
| // The error should specifically indicate the presence of duplicate vote extensions. | |
| s.Require().Error(baseapp.ValidateVoteExtensions(s.ctx, s.valStore, 3, chainID, llc)) | |
| } |
I'm not fully sure if there is an scenario where duplicate VEs in the extended commit info could be included under normal conditions - which is why minimally continuing and ignoring the vote extension is definitely necessary. If duplicate vote extensions can only be included by proposers, then erroring does make more sense. update: |
|
Let's backport this to 0.50.x |
There was a problem hiding this comment.
LGTM, thank you!
Here's the code that creates the vote extensions on CometBFT btw: https://github.com/cometbft/cometbft/blob/f72d930a68386f6139838449d0653ef0621f7b29/internal/state/execution.go#L504
facundomedica
added
the
backport/v0.50.x
(cherry picked from commit 5166c9f) # Conflicts: # CHANGELOG.md
…) (#18900) Co-authored-by: David Terpay <[email protected]> Co-authored-by: Facundo <[email protected]>
Description
Closes: #18893
This PR resolves issue opened in #18893. This PR adds a validator address cache to
ValidateVoteExtensionswhich ensure's that multiple of the same vote extensions cannot be included in the extended commit info.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...