feat!: add Noise Extensions and update deps

[MarcoPolo]

• Updates some deps
• Adds the Noise Extensions registry

The Noise extensions is used by https://github.com/libp2p/js-libp2p-webtransport/

BREAKING CHANGE: earlyData option is replaced by noise extensions

[CLAassistant]

All committers have signed the CLA.

[MarcoPolo]

cc @achingbrain since I'm not sure who to tag here for a review.

[mpetrunic]

Thnx for the effort 👍🏻
We are removing data/early data which is a breaking change so the title and description should be updated accordingly.

We should merge this after libp2p/js-libp2p-interfaces#293 is released

[mpetrunic]

please leave syntax to be proto3, protons library is not supporting proto2 officially and can sometimes generate weird results. In proto3 fields are optional by default so you can omit optional keywords.

[MarcoPolo]

This is on purpose. The spec recently changed to go to proto2 since proto3 has some unwanted semantics: libp2p/specs#456

[mpetrunic]

I understand that but it also says it's compatible with proto3. And I would rather not switch to proto2 because of : ipfs/protons#34

[MarcoPolo]

The old protobuf was compatible with proto3 i.e this one:

message NoiseHandshakePayload {
  bytes identity_key = 1;
  bytes identity_sig = 2;
  bytes data         = 3;
}

I don't think this is binary compatible with proto3. Namely the optional NoiseExtensions extensions = 4; is represented as a oneof in proto3. I think we could make this

message NoiseHandshakePayload {
  bytes identity_key = 1;
  bytes identity_sig = 2;
  NoiseExtensions extensions = 4;
}

and I think it would be binary compatible, but it's a bit of a hack.

What do you think?

[achingbrain]

optional NoiseExtensions extensions = 4; is represented as a oneof in proto3

go-libp2p's older noise .proto file with protobuf v3 doesn't have the oneof keyword. I don't think it's implicit if not set so I'm not sure this is correct. Am I missing something?

but it's a bit of a hack.

Is this because the field identifier is non-sequential? I think this is a feature of protocol buffers, any non-defined field should be skipped by the parser so this seems ok.

[MarcoPolo]

optional NoiseExtensions extensions = 4; is represented as a oneof in proto3

go-libp2p's older noise .proto file with protobuf v3 doesn't have the oneof keyword. I don't think it's implicit if not set so I'm not sure this is correct. Am I missing something?

Sorry I didn't explain this fully. I'll explain more in a bit

but it's a bit of a hack.

Is this because the field identifier is non-sequential? I think this is a feature of protocol buffers, any non-defined field should be skipped by the parser so this seems ok.

It's a hack because the real definition is proto2 but we're using a proto3 definition to workaround a protobuf-compiler bug.

[achingbrain]

In proto3 fields are optional by default so you can omit optional keywords.

@mpetrunic They're singluar by default so we need the optional keyword - https://developers.google.com/protocol-buffers/docs/proto3#specifying_field_rules

[MarcoPolo]

These two protobufs generate the same exact code (which is probably a separate bug since a proto2 encoder should write empty bytes if set. See libp2p/specs#465 for a discussion around proto2 vs proto3):

syntax = "proto2";

message NoiseExtensions {
    repeated bytes webtransport_certhashes = 1;
}

message NoiseHandshakePayload {
  optional bytes identity_key = 1;
  optional bytes identity_sig = 2;
  optional NoiseExtensions extensions = 4;
}

syntax = "proto3";

message NoiseExtensions {
    repeated bytes webtransport_certhashes = 1;
}

message NoiseHandshakePayload {
  bytes identity_key = 1;
  bytes identity_sig = 2;
  NoiseExtensions extensions = 4;
}

---

I think it may make sense to use proto3 long term here, but we still need to discuss that. For the mean time, I can set this as a proto3 definition that I've manually validated is compatible with proto2 parsers: https://github.com/MarcoPolo/proto2and3-playground/blob/9eb3987c04b348d64d825104813ad27c4df691e7/src/main.rs#L14

[mpetrunic]

@MarcoPolo interfaces are released so when you get a chance please update PR.

[MarcoPolo]

@MarcoPolo interfaces are released so when you get a chance please update PR.

Rebased

[MarcoPolo]

Let me fix the ts-expect-errors

[MarcoPolo]

@mpetrunic Ready again :)

[mpetrunic]

@MarcoPolo Had to mark extensions as optional in the proto file otherwise interop tests with the old proto file would fail with "Protocol error: value for required field "extensions" was not found in protobuf".

@achingbrain But according to the proto3 language guide this shouldn't happen for singular fields, right? Would that mean that protons is deserializing incorrectly?

[MarcoPolo]

@MarcoPolo Had to mark extensions as optional in the proto file otherwise interop tests with the old proto file would fail with "Protocol error: value for required field "extensions" was not found in protobuf".

The interop test is probably wrong because extensions was never required and proto3 doesn't have a notion of required fields.

@achingbrain But according to the proto3 language guide this shouldn't happen for singular fields, right?

Correct.

Would that mean that protons is deserializing incorrectly?

I'm not sure. I'd have to look into it a bit closer. Could you link the failing test please?

[MarcoPolo]

It should be safe to mark extensions as optional since either way explicit presence is tracked: https://github.com/protocolbuffers/protobuf/blob/main/docs/field_presence.md#presence-in-proto3-apis.

[mpetrunic]

I'm not sure. I'd have to look into it a bit closer. Could you link the failing test please?

https://github.com/ChainSafe/js-libp2p-noise/blob/master/test/interop.ts It tries to handshake go node (running old proto because go daemon is not updated probably).

I'm fine with merging this, but it would be nice to get @achingbrain input on why protons error on missing singular field. This is removed line after adding optional (but it shouldn't be here for singular field): 06f5673#diff-398a29cfaac5308d03639c7c0db977fdd195792a880c7672af6c7c4632c1f957L104

[achingbrain]

why protons error on missing singular field

It's a bug, I've nearly got a PR ready to fix it