noise: specify proto2

[marten-seemann]

Proto2 allows us to distinguish between unset and default values, which Proto3 doesn't. Since the protobuf we used for Noise was that simple, the serialization is compatible, so we can change to Proto2, even though some implementations interpreted the Protobuf defined here as Proto3.

I confirmed that a node running v0.22.0 (using a proto3 protobuf) can connect to a node using the proto2 protobuf, and vice versa.

Server

func main() {
	h1, err := libp2p.New(
		libp2p.ListenAddrStrings("/ip4/127.0.0.1/tcp/1234"),
		libp2p.Security("noise", noise.New),
	)
	if err != nil {
		log.Fatal(err)
	}
	fmt.Println("I am:", h1.ID())

	h1.SetStreamHandler("/proto1", func(str network.Stream) {
		data, err := io.ReadAll(str)
		if err != nil {
			log.Fatal(err)
		}
		fmt.Println("received data for proto1:", string(data))
	})
	select {}
}

client (using the ID printed by the server):

func main() {
	id, err := peer.Decode("QmcWzKr7pPn7TXc5rYiW6YtLSHBc1Bjw3ScvUK9wHn4rgW")
	if err != nil {
		log.Fatal(err)
	}
	addr := ma.StringCast("/ip4/127.0.0.1/tcp/1234")

	h2, err := libp2p.New(
		libp2p.ListenAddrStrings("/ip4/127.0.0.1/tcp/1235"),
		libp2p.Security("noise", noise.New),
	)
	if err != nil {
		log.Fatal(err)
	}
	if err := h2.Connect(context.Background(), peer.AddrInfo{
		ID:    id,
		Addrs: []ma.Multiaddr{addr},
	}); err != nil {
		log.Fatal(err)
	}

	str, err := h2.NewStream(context.Background(), id, "/proto1")
	if err != nil {
		log.Fatal(err)
	}
	str.Write([]byte("foobar"))
	str.Close()
	select {}
}

[MarcoPolo]

Can you include some context here too please. Why do we want to use proto2 instead of proto3? Also do you have an example test program you used to verify the bytes are the same over the wire? Thanks!

[marten-seemann]

@MarcoPolo I added the code and a short motivation.

[mxinden]

Very grateful for this change! @marten-seemann thanks for investigating here.

I think this deserves a revision bump.

I also tested this on the rust-libp2p side.

To reproduce

diff --git a/transports/noise/build.rs b/transports/noise/build.rs
index c9cf6041..c1bda422 100644
--- a/transports/noise/build.rs
+++ b/transports/noise/build.rs
@@ -20,4 +20,5 @@

fn main() {
   prost_build::compile_protos(&["src/io/handshake/payload.proto"], &["src"]).unwrap();
+    prost_build::compile_protos(&["src/io/handshake/payload2.proto"], &["src"]).unwrap();
}
diff --git a/transports/noise/src/io/handshake.rs b/transports/noise/src/io/handshake.rs
index 35099ea8..a1207ee6 100644
--- a/transports/noise/src/io/handshake.rs
+++ b/transports/noise/src/io/handshake.rs
@@ -24,6 +24,10 @@
mod payload_proto {
   include!(concat!(env!("OUT_DIR"), "/payload.proto.rs"));
}
+#[allow(clippy::derive_partial_eq_without_eq)]
+mod payload_proto_2 {
+    include!(concat!(env!("OUT_DIR"), "/payload2.proto.rs"));
+}

use crate::error::NoiseError;
use crate::io::{framed::NoiseFramed, NoiseOutput};
@@ -449,3 +453,38 @@ where

   Ok(())
}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use quickcheck::*;
+
+    #[test]
+    fn proto_2_3_round_trip() {
+        fn prop(identity_key: Vec<u8>, identity_sig: Vec<u8>, data: Vec<u8>) {
+            let pb3 = payload_proto::NoiseHandshakePayload {
+                identity_key: identity_key.clone(),
+                identity_sig: identity_sig.clone(),
+                data: data.clone(),
+            };
+
+            let mut buf = vec![];
+            pb3.encode(&mut buf).unwrap();
+
+            let pb2 = payload_proto_2::NoiseHandshakePayload::decode(&buf[..]).unwrap();
+
+            assert_eq!(pb2.identity_sig, identity_sig);
+            assert_eq!(pb2.identity_key, identity_key);
+            assert_eq!(pb2.data, data);
+
+            pb2.encode(&mut buf).unwrap();
+            let pb3 = payload_proto::NoiseHandshakePayload::decode(&buf[..]).unwrap();
+
+            assert_eq!(pb3.identity_sig, identity_sig);
+            assert_eq!(pb3.identity_key, identity_key);
+            assert_eq!(pb3.data, data);
+        }
+
+        quickcheck(prop as fn(_, _, _));
+    }
+}
diff --git a/transports/noise/src/io/handshake/payload2.proto b/transports/noise/src/io/handshake/payload2.proto
new file mode 100644
index 00000000..8437738e
--- /dev/null
+++ b/transports/noise/src/io/handshake/payload2.proto
@@ -0,0 +1,11 @@
+syntax = "proto2";
+
+package payload2.proto;
+
+// Payloads for Noise handshake messages.
+
+message NoiseHandshakePayload {
+  required bytes identity_key = 1;
+  required bytes identity_sig = 2;
+  required bytes data         = 3;
+}
\ No newline at end of file

[mxinden]

Suggested change

	bytes identity_key = 1;
	required bytes identity_key = 1;

If I am not mistaken proto2 requires either required, optional or repeated.

https://developers.google.com/protocol-buffers/docs/proto#specifying-rules

[marten-seemann]

Any objections against making them all optional? If we ever want to send early data with the first handshake message, we can then use this Protobuf.

[mxinden]

For the record, a proto3 encoder will simply provide the default value to the user in case any of these fields are optional and are not set before encoding with a proto2 encoder.

I am fine with these fields being optional. Though keep in mind that that would allow the above incompatibility at the schema level. I.e. a proto2 implementation could send a message to an old proto3 implementation that is incompatible.

How about making them optional and documenting that some implementations (using proto3) expect these fields to be set on the second and third Noise handshake message?

[marten-seemann]

How about making them optional and documenting that some implementations (using proto3) expect these fields to be set on the second and third Noise handshake message?

I think our handshake description specifies exactly that. Not sure if we need to add any text, mind suggesting something if you feel strongly?
Sending this Protobuf in the first handshake is forbidden by this spec anyway (although we don't enforce it), so using this Protobuf for that purpose would require a spec change anyway.

[mxinden]

I think our handshake description specifies exactly that.

Yes, you are right. Sorry for that.

[mxinden]

I think this deserves a revision bump.

Missing this one though, right?

[marten-seemann]

I think this deserves a revision bump.

Sorry, missed that. I bumped it. Not sure if it technically requires one, since this should be a backwards-compatible change, but then it doesn't hurt either.

[mxinden]

I think this deserves a revision bump.

Sorry, missed that. I bumped it. Not sure if it technically requires one, since this should be a backwards-compatible change, but then it doesn't hurt either.

If I am not mistaken, a non-breaking change qualifies as a revision bump:

Revision numbers start with lowercase r followed by an integer, which gets bumped whenever the spec is modified by merging a new PR.

https://github.com/libp2p/specs/blob/master/00-framework-02-document-header.md

[marten-seemann]

If I am not mistaken, a non-breaking change qualifies as a revision bump

Revision numbers start with lowercase r followed by an integer, which gets bumped whenever the spec is modified by merging a new PR.

https://github.com/libp2p/specs/blob/master/00-framework-02-document-header.md

Indeed, that's what the document says. We should change that. It doesn't make sense to bump the version for every change, e.g. in the extreme case fixing a typo.

[achingbrain]

Proto2 allows us to distinguish between unset and default values, which Proto3 doesn't

The protocol buffers v3 spec says of the optional keyword (emphasis mine):

• optional: the same as singular, except that you can check to see if the value was explicitly set. An optional field is in one of two possible states:

  • the field is set, and contains a value that was explicitly set or parsed from the wire. It will be serialized to the wire.
  • the field is unset, and will return the default value. It will not be serialized to the wire.

This seems to contradict the above sentence from the OP? Is it just the generated go code that doesn't let you check to see if a value was explicitly set? If so it sounds more like singular:

• singular: a well-formed message can have zero or one of this field (but not more than one). When using proto3 syntax, this is the default field rule when no other field rules are specified for a given field. You cannot determine whether it was parsed from the wire. It will be serialized to the wire unless it is the default value. For more on this subject, see