circuitv2: upgrade definition to proto3

[ckousik]

• [x] spec: https://github.com/libp2p/specs/pull/511
• [ ] go-libp2p: https://github.com/libp2p/go-libp2p/pull/2029
• [ ] rust-libp2p: https://github.com/libp2p/rust-libp2p/issues/3438

The current protobuf definitions for circuit v2 use proto2. For the new js implementation, it would be preferable to use proto3, since js-libp2p uses proto3 almost everywhere and protons only supports proto3. This change would require adding a default field to the Status enum as it is not currently compatible with proto3.

Example updated protobuf:

syntax = "proto3";
message HopMessage {
  enum Type {
    RESERVE = 0;
    CONNECT = 1;
    STATUS = 2;
  }

  Type type = 1;

  optional Peer peer = 2;
  optional Reservation reservation = 3;
  optional Limit limit = 4;

  optional Status status = 5;
}

message StopMessage {
  enum Type {
    CONNECT = 0;
    STATUS = 1;
  }

  Type type = 1;

  optional Peer peer = 2;
  optional Limit limit = 3;

  optional Status status = 4;
}

message Peer {
  bytes id = 1;
  repeated bytes addrs = 2;
}

message Reservation {
  uint64 expire = 1; // Unix expiration time (UTC)
  repeated bytes addrs = 2;   // relay addrs for reserving peer
  optional bytes voucher = 3; // reservation voucher
}

message Limit {
  optional uint32 duration = 1; // seconds
  optional uint64 data = 2;     // bytes
}

enum Status {
  // zero value field required for proto3 compatibility
  UNSPECIFIED             = 0;
  OK                      = 100;
  RESERVATION_REFUSED     = 200;
  RESOURCE_LIMIT_EXCEEDED = 201;
  PERMISSION_DENIED       = 202;
  CONNECTION_FAILED       = 203;
  NO_RESERVATION          = 204;
  MALFORMED_MESSAGE       = 400;
  UNEXPECTED_MESSAGE      = 401;
}

[MarcoPolo]

This change would require adding a default field to the Status enum as it is not currently compatible with proto3.

Is that required? Status is optional so we'll know if it's not set. I don't think we'll want it set but left empty.

Are there any on-the-wire changes with this change?

[ckousik]

@MarcoPolo yes, it's required by proto3 https://developers.google.com/protocol-buffers/docs/proto3#enum As for wire changes, the default value for Status would change to the new zero-value from OK.

[MarcoPolo]

Ah I see. We need to have a 0 value. But we can simply not use it. I would suggest calling the 0 value UNUSED then.

As for wire changes, the default value for Status would change to the new zero-value from OK.

The Status is used as optional Status status; in the messages. Because it is marked as optional we have explicit presence (see this table). So by default in the message we'll know it's not preset. Yes, an implementation could explicitly set the value to the 0 value, but they'd have to do this on purpose. That would be a bug, just like they could set the value explicitly to a wrong status.

So my question is, are there are changes on the wire with this change then? Given that we have this explicit presence. I'm pretty sure the answer is no, but we should confirm. If there are no changes, then this is easy. If there are it breaks backwards compatibility.

To answer this question I'd suggest doing something like https://github.com/MarcoPolo/proto2and3-playground (you can fork that and change it). That checks the bytes are equal or, barring that, that each can be decoded to the other.

[ckousik]

@MarcoPolo I've run similar tests for circuit v2 and it doesn't seem to break any backward compatibility. I've used the table as a guide for explicit presence. I'm currently only testing the status field.

[MarcoPolo]

Great. Thanks for that! Those tests make sense to me. I'm for this change. Could you open a PR here in the Specs repo with this change that closes this issue please?

[MarcoPolo]

Some more discussion here around subtleties and the path forward https://github.com/libp2p/specs/pull/511#discussion_r1092435671

[MarcoPolo]

Alright I've run a bunch of experiments: https://github.com/MarcoPolo/proto3-and-2-compat-experiments

Lets use the proto3 definition in that repo.

[p-shahi]

cc: @thomaseizinger since @mxinden is out. A corresponding change needs to be made to rust-libp2p here https://github.com/libp2p/rust-libp2p/blob/master/protocols/relay/src/message.proto right?

[p-shahi]

cc: @Menduist for nim-libp2p considerations

[thomaseizinger]

cc: @thomaseizinger since @mxinden is out. A corresponding change needs to be made to rust-libp2p here libp2p/rust-libp2p@master/protocols/relay/src/message.proto right?

Yes, thanks for the ping!