fix: deterministic protov2 marshal #18403
Conversation
julienrbrt
added
the
backport/v0.50.x
|
Your existing walkthrough is well-structured and informative. I've updated the changes section to reflect the provided information. Here's the updated content: WalkthroughThe changes across multiple files primarily focus on ensuring deterministic marshaling of protocol buffer messages. This is achieved by replacing direct calls to Changes
Poem
Feel free to let me know if there's anything else you'd like to add or modify! TipsChat with CodeRabbit Bot (
|
|
@julienrbrt your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (8)
- baseapp/internal/protocompat/protocompat.go (3 hunks)
- codec/collections.go (1 hunks)
- codec/proto_codec.go (1 hunks)
- codec/types/any.go (2 hunks)
- x/tx/signing/direct/direct.go (2 hunks)
- x/tx/signing/directaux/direct_aux.go (1 hunks)
- x/tx/signing/testutil/util.go (1 hunks)
- x/tx/signing/textual/tx.go (1 hunks)
Additional comments: 12
x/tx/signing/testutil/util.go (1)
- 59-71: The use of deterministic marshaling is a good practice for protocol buffers, especially when dealing with cryptographic signatures. This change ensures that the serialized output will be the same for the same input.
codec/collections.go (1)
- 106-109: The change ensures deterministic marshaling of protobuf messages, which is crucial for consensus-critical data in blockchain applications. This change is consistent with the pull request's goal.
x/tx/signing/directaux/direct_aux.go (1)
- 101-107: The change to use deterministic marshaling is appropriate and correctly implemented. Ensure that all dependencies and modules that rely on the output of this function are compatible with deterministic marshaling.
x/tx/signing/textual/tx.go (1)
- 272-283: The use of
proto.MarshalOptions{Deterministic: true}ensures deterministic serialization of protobuf messages. This is a good practice for scenarios where the order of serialization matters, such as in cryptographic operations or when comparing serialized data. However, it may have a slight performance impact compared to non-deterministic serialization. Ensure that this change aligns with the performance requirements of your application.codec/proto_codec.go (1)
- 345-349: The change to use
protov2MarshalOpts.Marshal(m)instead ofproto.Marshal(m)is correct and aligns with the goal of deterministic marshaling. Ensure that allproto.Messagetypes used in the codebase are compatible with deterministic marshaling.x/tx/signing/direct/direct.go (2)
13-16: The introduction of the
protov2MarshalOptsglobal variable with deterministic marshaling is a good practice for ensuring consistent serialization of protocol buffer messages across different invocations. However, ensure that this change does not introduce any unintended side effects in the parts of the codebase where deterministic marshaling is not desired.27-33: The change from
proto.Marshaltoprotov2MarshalOpts.Marshalin theGetSignBytesfunction is consistent with the goal of deterministic marshaling. Ensure that all usages of this function are compatible with the deterministic serialization of thetxv1beta1.SignDocstructure.baseapp/internal/protocompat/protocompat.go (3)
16-22: The introduction of the
protov2MarshalOptsglobal variable is a good practice to ensure deterministic marshaling across the codebase. This change is consistent and maintainable.98-104: The use of
protov2MarshalOpts.Marshalinstead ofproto2.Marshalensures deterministic marshaling of the response. This change is consistent with the goal of the pull request.139-145: The use of
protov2MarshalOpts.Marshalinstead ofproto2.Marshalensures deterministic marshaling of the request. This change is consistent with the goal of the pull request.codec/types/any.go (2)
68-75: The change introduces deterministic marshaling for proto v2 messages. This is a good practice as it ensures the serialized output will always be the same given the same input, which can be crucial for things like hashing or equality checks.
110-117: The same change as above is applied here, introducing deterministic marshaling for proto v2 messages in the
packfunction.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- x/auth/go.mod
Files selected for processing (1)
- x/accounts/internal/implementation/api_builder.go (2 hunks)
Files skipped from review due to trivial changes (1)
- x/accounts/internal/implementation/api_builder.go
(cherry picked from commit e2b71b7) # Conflicts: # x/accounts/internal/implementation/api_builder.go # x/auth/go.mod
Description
Context: https://binarybuilders.slack.com/archives/C05J3U400HX/p1697215636557559
Thanks @testinginprod reminding us about this just in time.
We should add a CodeQL rule in: https://github.com/crypto-com/cosmos-sdk-codeql
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...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers 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...
!in the type prefix if API or client breaking changeSummary by CodeRabbit