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

Protobuf decode absent values as default #921

Closed
fryorcraken opened this issue Sep 5, 2022 · 3 comments · Fixed by #1169
Closed

Protobuf decode absent values as default #921

fryorcraken opened this issue Sep 5, 2022 · 3 comments · Fixed by #1169

Comments

@fryorcraken
Copy link
Collaborator

Problem

Proto3 language guide states:

When a message is parsed, if the encoded message does not contain a particular singular element, the corresponding field in the parsed object is set to the default value for that field.

We currently use protons for protobuf encoding/decoding and its behavior does not match the language guide: ipfs/protons#43

Decoding

Because of that, we manually replace an undefined value with the default value in some places: https://github.com/status-im/js-waku/blob/d79984fdba57c8e89400fc2a47958d49e0ed9645/src/lib/waku_message/index.ts#L273 as other implementations (go-waku, nwaku), do not encode default values on wire.

Encoding

Encoding default values over wire or not do not change the functionality.
Avoiding the encoding would few bytes of bandwith and data storage.

Solution

Best case, ipfs/protons#48, or equivalent gets merged and we use it forward.
if not, then either:

  • fork protons to have a library that contains patch above (best to do that once protons' performance issues are solved Slow compared to protobufjs ipfs/protons#51)
  • OR, somehow create a wrapper/proxy module that generically implement desired behaviour on protobuf decoders.
@fryorcraken fryorcraken added this to Waku Sep 5, 2022
@fryorcraken fryorcraken mentioned this issue Sep 5, 2022
6 tasks
@fryorcraken fryorcraken moved this to Todo in Waku Sep 5, 2022
@fryorcraken
Copy link
Collaborator Author

Looks like protons did a fix: ipfs/protons#66 so it's just a matter of upgrading to latest version

@fryorcraken
Copy link
Collaborator Author

Latest update:

optional attribute in proto3 has a new meaning and it influences whether or not a field should be encoded over the wire.

Now:

  • optional field: if default value (e.g. 0), still encode over wire. When decoding, if not present, assume to be "absent"
  • normal field: if default value (e.g. 0), do not encode over the wire. When decoding, if not present, assume it's default value (e.g. 0)

In Waku, we did not pay attention to any of this when defining protocols. Until now. See work done with vacp2p/rfc#552 and vacp2p/rfc#553

Steps:

  1. Check that protons' behaviour matches latest proto3 language definition around optional field.
  2. Hopefully (1) is true, so upgrade proton
  3. Update protobuf definition to match the latest RFC (ie Waku Message first, also check https://github.com/vacp2p/waku

@fryorcraken
Copy link
Collaborator Author

Some recent development (I haven't reviewed yet) ipfs/protons#83

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant