search

libp2p / specs

Technical specifications for the libp2p networking stack
https://libp2p.io
1.58k stars 275 forks source link

[Signed Envelopes] Protobuf description not valid #329

Closed thomaseizinger closed 3 years ago

thomaseizinger commented 3 years ago

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!

mxinden commented 3 years ago

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.

https://github.com/libp2p/go-libp2p-core/pull/73 introduced a seq field with number 4 in https://github.com/libp2p/go-libp2p-core/commit/5f6b601ac96784f73c03b7ffbab1a5cb84fed6e4. The seq field was later on removed in https://github.com/libp2p/go-libp2p-core/commit/5f6b601ac96784f73c03b7ffbab1a5cb84fed6e4 without setting the signature field down to 4. The specification was then later on adjusted in https://github.com/libp2p/specs/commit/377f05abe37fca3bfe392b5e07c134da94306c02. 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: https://github.com/libp2p/rust-libp2p/pull/2081

thomaseizinger commented 3 years ago

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