cometbft-light-client-verifier: reuse sign_bytes buffer #57
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Pull Request Overview
This PR ports improvements from the tendermint-rs project to reuse the sign_bytes buffer and refactor commit verification logic in the light-client verifier.
- Added a new sign_bytes_into method in the vote module to reduce memory allocations.
- Merged and refactored commit verification methods to combine validator and signer overlap checks.
- Updated tests and changelog entries to document both improvements and breaking changes.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| light-client-verifier/src/verifier.rs | Refactored commit verification logic and merged overlapping checks. |
| light-client-verifier/src/predicates.rs | Introduced has_sufficient_validators_and_signers_overlap method. |
| cometbft/src/vote.rs | Added sign_bytes_into to reuse the existing buffer for encoding. |
| .changelog/unreleased/improvements/35-reuse-sign-bytes.md | Documented the buffer reuse improvement. |
| .changelog/unreleased/breaking-changes/33-avoid-chucking-signature-multiple-times.md | Documented breaking changes in signature verification rework. |
0fea176 to
8e62d56
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes the light client verifier by reusing a buffer for canonical vote sign bytes and introduces a new helper on SignedVote to write into that buffer, reducing allocations.
- Reuse a pre-allocated
sign_bytesbuffer inNonAbsentCommitVotes - Add
sign_bytes_intoonSignedVotefor buffered encoding - Update changelog with the improvement
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| light-client-verifier/src/operations/voting_power.rs | Add reusable sign_bytes buffer with initial capacity and update vote verification to reuse it |
| cometbft/src/vote.rs | Introduce SignedVote::sign_bytes_into to encode into an existing buffer |
| .changelog/unreleased/improvements/35-reuse-sign-bytes.md | Document reuse of sign_bytes buffer |
Comments suppressed due to low confidence (1)
cometbft/src/vote.rs:439
- Add unit tests for
sign_bytes_intoto verify it writes the expected canonical vote bytes into the provided buffer under various scenarios.
pub fn sign_bytes_into(&self, buf: &mut impl BufMut) -> Result<(), ProtobufError> {
| self.sign_bytes.truncate(0); | ||
| vote.signed_vote | ||
| .sign_bytes_into(&mut self.sign_bytes) | ||
| .expect("buffer is resized if needed and encoding never fails"); |
There was a problem hiding this comment.
Avoid using expect on a Protobuf encoding result to prevent unexpected panics. Consider mapping the error into VerificationError and returning it instead of panicking.
| .expect("buffer is resized if needed and encoding never fails"); | |
| .map_err(|e| VerificationError::encoding_error(e.to_string()))?; |
Copilot uses AI. Check for mistakes.
| @@ -436,6 +436,10 @@ impl SignedVote { | |||
| Protobuf::<RawCanonicalVote>::encode_length_delimited_vec(self.vote.clone()) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
[nitpick] Add a Rustdoc comment for sign_bytes_into explaining what buffer format it writes and when it should be used.
| /// Writes the length-delimited protobuf-encoded representation of the canonical vote | |
| /// into the provided buffer. | |
| /// | |
| /// # Arguments | |
| /// | |
| /// * `buf` - A mutable buffer that implements the `BufMut` trait, where the encoded | |
| /// bytes will be written. | |
| /// | |
| /// # Returns | |
| /// | |
| /// Returns `Ok(())` if the operation is successful, or a `ProtobufError` if encoding fails. | |
| /// | |
| /// # Usage | |
| /// | |
| /// Use this function when you need to write the encoded bytes directly into a buffer | |
| /// for efficiency, such as when preparing data for network transmission or storage. |
Copilot uses AI. Check for mistakes.
Port-of: cometbft/tendermint-rs#1413
Closes #35