transports/noise: Change protobuf files to proto2 #3007

nloadholtes · 2022-10-10T20:39:34Z

This is the same format as used in go-libp2p

Description

To keep with the structure defined in go-libp2p here, I changed the syntax to proto2 and set the fields to optional.

Links to any relevant issues

Issue #2924

Open Questions

This is my first time messing with a rust project, and I'm new to libp2p. I used cargo test to test things locally and it reported errors in src/tutorials/ping.rs. I am still figuring things out, so any feedback/advice is welcomed. 😃

Also, I did a search for documentation and didn't see any that referenced this change, if there's something I missed please let me know.

I also did not add an entry to the changelog, I wanted to get feedback on this first before getting ahead of myself.

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

thomaseizinger

This looks good to me!

Can you please:

Add a changelog entry
Make a patch version bump
Pull the version bump through to all crates that depend on libp2p-noise
Run a test where you connect a node with this version against the latest release.

Thank you!

thomaseizinger · 2022-10-16T23:33:55Z

I kicked CI and it does show up with errors in libp2p-noise. That is expected because the generated code from the protobufs is now different. You'll have to amend the code to handle the now optional fields in the deserialised message :)

thomaseizinger · 2022-10-16T23:36:34Z

This is my first time messing with a rust project, and I'm new to libp2p. I used cargo test to test things locally and it reported errors in src/tutorials/ping.rs. I am still figuring things out, so any feedback/advice is welcomed. smiley

I typically run cargo clippy --workspace --all-targets --all-features locally to validate my code. That will run the type-checker with some useful lints which is much faster than actually compiling the code. Once that is passing, you can probably focus on running the tests in transports/noise.

For this particular PR, you can probably amend the above clippy command to just be: cargo clippy --package libp2p-noise --all-targets --all-features. I don't expect any other code to need modification!

This is the same format as used in go-libp2p

…angeslogs

nloadholtes · 2022-10-17T02:29:32Z

Thank you for the great feedback! I've made those changes and I think the changes to transports/noise/src/io/handshake.rs are what was needed. (I'm not used to the compiler giving such useful feedback, it kinda made me nervous! 😆)

If there's anything I missed or could make better, please let me know. Thanks!

thomaseizinger

Thanks! A few more comments :)

thomaseizinger · 2022-10-17T04:12:37Z

transports/noise/src/io/handshake.rs

@@ -207,8 +207,8 @@ where
    }
    let pb = pb_result?;

-    if !pb.identity_key.is_empty() {
-        let pk = identity::PublicKey::from_protobuf_encoding(&pb.identity_key)
+    if !pb.identity_key.as_ref().expect("optional").is_empty() {


We can't just panic here :)

We need more graceful error handling to check whether or not the key is actually present. Probably best done with a match and a match-arm guard so you can do the existing if and the destructing in one go.

Have a google around if some of these terms don't mean anything to you (yet)! :)

thomaseizinger · 2022-10-17T04:13:13Z

transports/noise/src/io/handshake.rs

-    if !pb.identity_key.is_empty() {
-        let pk = identity::PublicKey::from_protobuf_encoding(&pb.identity_key)
+    if !pb.identity_key.as_ref().expect("optional").is_empty() {
+        let pk = identity::PublicKey::from_protobuf_encoding(&pb.identity_key.unwrap())


Same here. Look into ok_or_else.

thomaseizinger · 2022-10-17T04:13:45Z

transports/noise/src/io/handshake.rs

@@ -218,8 +218,8 @@ where
        state.id_remote_pubkey = Some(pk);
    }

-    if !pb.identity_sig.is_empty() {
-        state.dh_remote_pubkey_sig = Some(pb.identity_sig);
+    if !pb.identity_sig.as_ref().expect("optional").is_empty() {


Same here, we need to gracefully handle the None case.

…elds are optional. If they are missing, nothing happens (which is what was happening before just with an if statement instead of a match).

nloadholtes · 2022-10-18T22:11:22Z

transports/noise/src/io/handshake.rs

        }
-        state.id_remote_pubkey = Some(pk);
+        None => {}


@thomaseizinger Thanks for the tips on panic/error checking/match, that was a a good bit to read up on as I lean rust. I think this code both fulfills the spirit of what was happening before (if there was no key, just keep going) but now does it without the panic (I think).

Again, thank you for your guidance on this. I really appreciate it.

Please let me know if I'm moving in the right direction, or if any other changes are needed.

If you run clippy, it will tell you that you can replace this with an if let Some(identity_key) construct :)

🤯 😮 whoa. That is really cool. Thanks for the tip about that, you just really flattened my learning curve with that one!

thomaseizinger

LGTM!

I'll kick CI again. Let's see what it says!

transports/noise/CHANGELOG.md

thomaseizinger · 2022-10-18T23:23:56Z

transports/noise/src/io/handshake.rs

        }
-        state.id_remote_pubkey = Some(pk);
+        None => {}


If you run clippy, it will tell you that you can replace this with an if let Some(identity_key) construct :)

thomaseizinger · 2022-10-18T23:24:19Z

transports/noise/src/io/handshake.rs

+    match pb.identity_sig {
+        Some(identity_sig) => {
+            state.dh_remote_pubkey_sig = Some(identity_sig);
+        }
+        None => {}
    }


CHANGELOG.md

transports/noise/CHANGELOG.md

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

thomaseizinger

Thanks! This LGTM :)

A discussion started in the meantime on whether we actually want this: libp2p/specs#465

I'll hold off with merging until that is settled. Sorry for that.

nloadholtes · 2022-10-19T13:02:54Z

hahaha, I have great timing! Its totally cool and I understand on holding off on merging this. I'm going to go look for another help-wanted ticket and see if I can help out some more. Thanks for all your mentoring on this PR, I really appreciate it!

thomaseizinger · 2022-10-19T13:12:42Z

hahaha, I have great timing! Its totally cool and I understand on holding off on merging this. I'm going to go look for another help-wanted ticket and see if I can help out some more.

Thank you! There should be plenty around :)

Thanks for all your mentoring on this PR, I really appreciate it!

You are welcome!

jxs · 2022-12-16T16:56:11Z

this PR was probably lost in the transition to mergify :D @nloadholtes can you solve the merge conflicts?

mxinden · 2022-12-19T12:27:39Z

A discussion started in the meantime on whether we actually want this: libp2p/specs#465

I'll hold off with merging until that is settled. Sorry for that.

@jxs I think the above comment by @thomaseizinger still applies here.

jxs · 2022-12-19T12:55:11Z

A discussion started in the meantime on whether we actually want this: libp2p/specs#465
I'll hold off with merging until that is settled. Sorry for that.

@jxs I think the above comment by @thomaseizinger still applies here.

ah right sorry, I missed it!

mergify · 2023-03-19T18:23:17Z

This pull request has merge conflicts. Could you please resolve them @nloadholtes? 🙏

thomaseizinger · 2023-03-19T19:02:45Z

I think we settled on moving to proto3.

nloadholtes force-pushed the nloadholtes/noise-use-proto2 branch from 97974db to 0e761ef Compare October 15, 2022 16:41

thomaseizinger linked an issue Oct 16, 2022 that may be closed by this pull request

transports/noise: Use proto2 #2924

Closed

thomaseizinger changed the title ~~Setting transports/noise proto to use proto2~~ transports/noise: Change protobuf files to proto2 Oct 16, 2022

thomaseizinger reviewed Oct 16, 2022

View reviewed changes

nloadholtes added 3 commits October 16, 2022 21:39

Setting noise proto to use proto2

eec4c6d

This is the same format as used in go-libp2p

Version bump, updating crates that depend on this, adding notes to ch…

8b52cbe

…angeslogs

Making sure the now optional identity_key is properly accessed

0b7f11d

nloadholtes force-pushed the nloadholtes/noise-use-proto2 branch from 0e761ef to 0b7f11d Compare October 17, 2022 02:27

thomaseizinger requested changes Oct 17, 2022

View reviewed changes

Changing identity_key and identit_sig checks to reflect that those fi…

b1ce46b

…elds are optional. If they are missing, nothing happens (which is what was happening before just with an if statement instead of a match).

nloadholtes commented Oct 18, 2022

View reviewed changes

nloadholtes requested a review from thomaseizinger October 18, 2022 22:11

thomaseizinger reviewed Oct 18, 2022

View reviewed changes

nloadholtes and others added 3 commits October 18, 2022 21:33

Update transports/noise/CHANGELOG.md

d41913f

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

Apply suggestions from code review

89611f3

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

Taking the compiler's suggestions to improve the code

191310a

nloadholtes requested a review from thomaseizinger October 19, 2022 01:45

thomaseizinger approved these changes Oct 19, 2022

View reviewed changes

thomaseizinger closed this Mar 19, 2023

jxs mentioned this pull request Mar 13, 2024

transports/noise: Use proto2 #2924

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transports/noise: Change protobuf files to proto2 #3007

transports/noise: Change protobuf files to proto2 #3007

nloadholtes commented Oct 10, 2022 •

edited

Loading

thomaseizinger left a comment

thomaseizinger commented Oct 16, 2022

thomaseizinger commented Oct 16, 2022

nloadholtes commented Oct 17, 2022

thomaseizinger left a comment

thomaseizinger Oct 17, 2022

thomaseizinger Oct 17, 2022

thomaseizinger Oct 17, 2022

nloadholtes Oct 18, 2022

thomaseizinger Oct 18, 2022

nloadholtes Oct 19, 2022

thomaseizinger left a comment

thomaseizinger Oct 18, 2022

thomaseizinger Oct 18, 2022

thomaseizinger left a comment

nloadholtes commented Oct 19, 2022

thomaseizinger commented Oct 19, 2022

jxs commented Dec 16, 2022

mxinden commented Dec 19, 2022

jxs commented Dec 19, 2022

mergify bot commented Mar 19, 2023

thomaseizinger commented Mar 19, 2023

transports/noise: Change protobuf files to proto2 #3007

transports/noise: Change protobuf files to proto2 #3007

Conversation

nloadholtes commented Oct 10, 2022 • edited Loading

Description

Links to any relevant issues

Open Questions

Change checklist

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger commented Oct 16, 2022

thomaseizinger commented Oct 16, 2022

nloadholtes commented Oct 17, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

nloadholtes commented Oct 19, 2022

thomaseizinger commented Oct 19, 2022

jxs commented Dec 16, 2022

mxinden commented Dec 19, 2022

jxs commented Dec 19, 2022

mergify bot commented Mar 19, 2023

thomaseizinger commented Mar 19, 2023

nloadholtes commented Oct 10, 2022 •

edited

Loading