Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proper dealing with protobuf3 decoding #1043

Closed
5 tasks
soareschen opened this issue Jun 3, 2021 · 0 comments
Closed
5 tasks

Proper dealing with protobuf3 decoding #1043

soareschen opened this issue Jun 3, 2021 · 0 comments
Labels
A: question Admin: further information is requested I: dependencies Internal: related to dependencies
Milestone

Comments

@soareschen
Copy link
Contributor

soareschen commented Jun 3, 2021

Crate

ibc

Summary

Protobuf3 has the unfortunate semantics of allowing all fields to be optional, and leaves no way to mark a field as being required. (protocolbuffers/protobuf#2497) The prost crate follows this semantic, and decodes missing fields into default values such as 0 and "". This can cause unintended bugs when the relayer logic expects a given field to be present. In particular, the semantics allow empty buffers to be decoded successfully, producing nonsense values such as in #1042.

On Rust side, we want to use the Option type to properly denote the absence of values, and have proper distinction between values not being present and the default values. One strategy to deal with this is to mark all scalar fields in the raw prost structs as being optional, so that prost will properly decode them into Option value when the field is not present in the raw buffer. The conversion from raw prost structs into the proper domain types should then return error when the required fields have value of None.

We may need to modify the proto-compiler crate so that it properly marks all scalar fields as being optional in the generated Rust code.

Acceptance Criteria

All scalar fields of the raw prost structs in proto/src/prost should have the Option type.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@adizere adizere added this to the 09.2021 milestone Jun 24, 2021
@adizere adizere added I: dependencies Internal: related to dependencies A: question Admin: further information is requested labels Sep 6, 2021
@adizere adizere modified the milestones: 09.2021, Backlog Sep 6, 2021
@romac romac closed this as not planned Won't fix, can't repro, duplicate, stale Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: question Admin: further information is requested I: dependencies Internal: related to dependencies
Projects
None yet
Development

No branches or pull requests

3 participants