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

[Signed Envelopes] Protobuf description not valid #329

Closed
thomaseizinger opened this issue May 28, 2021 · 2 comments · Fixed by #333
Closed

[Signed Envelopes] Protobuf description not valid #329

thomaseizinger opened this issue May 28, 2021 · 2 comments · Fixed by #333

Comments

@thomaseizinger
Copy link
Contributor

I've attempted to implement signed envelopes in the Rust libp2p implementation and noticed the following:

  1. The signature field is given the number 5 but there is no number 4. Is this an overlook from an earlier iteration where there used to be another field?
  2. The fields are missing an optional or required specifier. Without that, the protobuf doesn't actually compile.

Happy to submit a patch if there is consensus on what the protobuf should be changed to!

@thomaseizinger thomaseizinger changed the title [Signed Envelopes] Protobuf format [Signed Envelopes] Protobuf description not valid May 28, 2021
@mxinden
Copy link
Member

mxinden commented Jun 11, 2021

Sorry for the late reply. The notification mail somehow ended up in my spam folder.

The signature field is given the number 5 but there is no number 4. Is this an overlook from an earlier iteration where there used to be another field?

Note: I haven't been involved in the design of the signed envelopes RFC, thus the below is solely based on a short git blame adventure.

libp2p/go-libp2p-core#73 introduced a seq field with number 4 in libp2p/go-libp2p-core@5f6b601. The seq field was later on removed in libp2p/go-libp2p-core@5f6b601 without setting the signature field down to 4. The specification was then later on adjusted in 377f05a. I would guess this was a mistake. I doubt it matters much.

The fields are missing an optional or required specifier. Without that, the protobuf doesn't actually compile.

The specification does not mention that this is proto3 instead of proto2 syntax. See corresponding Proto definition in the go-libp2p-core repository. Adding syntax = "proto3"; to the top of the file should resolve the compilation issues.

I am sorry for this not being documented in the RFC. @thomaseizinger do you want to create a pull request updating the the signed envelopes RFC accordingly? If not, I can do so in the next couple of days.


Cross referencing rust-libp2p pull request for those following along: libp2p/rust-libp2p#2081

@thomaseizinger
Copy link
Contributor Author

I've opened a PR here that uses the protobuf descriptions from the go-libp2p repository in the spec.

mxinden pushed a commit that referenced this issue Jun 17, 2021
These protobuf definitions are taken from the go-libp2p repo.

See #329 for details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants