Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

identify: implement SignedPeerRecord #4017

Open
Marcel-G opened this issue Jun 1, 2023 · 10 comments · May be fixed by #5785
Open

identify: implement SignedPeerRecord #4017

Marcel-G opened this issue Jun 1, 2023 · 10 comments · May be fixed by #5785
Labels
difficulty:easy help wanted

Comments

@Marcel-G
Copy link
Contributor

Marcel-G commented Jun 1, 2023 •
edited
Loading

Description

As part of the identify protocol signedPeerRecord contains a serialized SignedEnvelope containing a PeerRecord,
signed by the sending node. It contains the same addresses as the listenAddrs field, but in a form that lets us share authenticated addrs with other peers.

Currently the rust-libp2p identify protocol does not implement signedPeerRecord
https://github.com/libp2p/rust-libp2p/blob/master/protocols/identify/src/generated/structs.proto#L5

For reference go-libp2p does:
https://github.com/libp2p/go-libp2p/blob/master/p2p/protocol/identify/pb/identify.proto#L35

Is there a plan to implement this in the near future, is there something blocking it?

Motivation

There was an change in js-libp2p v0.45.0 which makes it no longer work the same for identify records where signedPeerRecord is omitted libp2p/js-libp2p#1789. This results in there being a requirement for signedPeerRecord in order for peer identify records to be consumed in v0.45.0.

This appears to be a bug on the js-libp2p side, however leaving this issue open to track progress of signedPeerRecord in the identify protocol.

Are you planning to do it yourself in a pull request?

I have implemented something locally and can verify it can work as expected with js-libp2p v0.45.0. However, I'm not sure I have enough context to contribute to this at this stage.
@Marcel-G Marcel-G changed the title identity: implement signedPeerRecord identify: implement signedPeerRecord Jun 1, 2023
@thomaseizinger
Copy link
Contributor

I'd love to see what you built locally and have it land here eventually.

I'd be happy to iterate with you on it in a PR :)

@maschad
Copy link
Member

maschad commented Jun 1, 2023

Thanks for taking the time to implement a solution, it would be cool to see what you did.

You should still be able to use rust as a relay server over webRTCDirect as it's not dependent on SignedPeerRecords since they are not rolled out across many libp2p protocols.

I would be interested to see what your configuration is, but as a reference you could take a look at our unviersal connectivity example chat app, where establish a webRTCDirect connection on with rust server from a browser (js) node to then dial another browser node over webRTC

@maschad maschad mentioned this issue Jun 1, 2023
Closed
@Marcel-G
Copy link
Contributor Author

Marcel-G commented Jun 1, 2023 •
edited
Loading

Thanks for your support on this!

look at our unviersal connectivity example chat app, where establish a webRTCDirect connection on with rust server from a browser (js) node to then dial another browser node over webRTC

Actually this is the project is where my investigations began. You're absolutely right that the relay connection via webrtc-direct does work (regardless of the SignedPeerRecords). That reminds me (which I forgot about while creating the issue) is that the problem I'm trying to solve here is not about connection but rather about peer discovery (hence identify protocol)

In my configuration I was trying out pubsub peer discovery, rather than manual dialing.
I found was that js-libp2p ignores the relay address (for any transports) advertised by the rust-libp2p nodes. Or, at least it's not added automatically to the peer-store. As far as I can tell this was related to it missing SignedPeerRecords. I would say now that this problem is completely unrelated to webrtc.
This is all pretty new so I may be going in the wrong direction though, what do you think?

@maschad
Copy link
Member

maschad commented Jun 1, 2023 •
edited
Loading

I would say that there a variety of peer discovery mechanisms which may be applicable to your use case. In the case of universal connectivity we use kademlia DHTs for discovery. Those resources should point you in the right direction.

@Marcel-G
Copy link
Contributor Author

Marcel-G commented Jun 2, 2023

Update:

  1. Dial the rust-libp2p relay node address with js-libp2p
  2. Relay node uses the identify protocol to advertise that it supports circuit relay, kademlia etc.
  3. js-libp2p client consumes this info into the peerStore
  4. From there js-libp2p starts the relay dance, etc

This is currently working correctly in the universal-connectivity project, however the behavior has changed between js-libp2p v0.44.0 and v0.45.0.

More precisely the "legacy" identify path has been removed. This results in there being a requirement for signedPeerRecord in order for peer identify records to be consumed in v0.45.0. This seems like a breaking change, which isn't mentioned. So I'm thinking this is more of a bug on the js-libp2p side.

If you think thats accurate I can close this issue and create an issue there.

@thomaseizinger
Copy link
Contributor

Even if it is a confirmed bug, it would still be great to implement signed peer records in identify in rust-libp2p!

So I'd like to keep this open to track the effort.

@maschad
Copy link
Member

maschad commented Jun 2, 2023

More precisely the "legacy" identify path has been removed. This results in there being a requirement for signedPeerRecord in order for peer identify records to be consumed in v0.45.0. This seems like a breaking change, which isn't mentioned. So I'm thinking this is more of a bug on the js-libp2p side.

Thanks for finding this @Marcel-G but I am not sure this is accurate because the peer:identify event would just have an undefined SignedPeerRecord if none was exchanged whilst consuming the identify message.

I am actually in the process of upgrading universal connectivity to use 0.45.x which should help me to recreate these scenarios, but it may be more fruitful for you to open an issue on the js-libp2p issue using our issue template so that we can rightly diagnose.

Since rust-libp2p should support SignedPeerRecord as @thomaseizinger mentioned I think we should leave this ticket open.

@Marcel-G Marcel-G mentioned this issue Jun 5, 2023
Closed
@thomaseizinger thomaseizinger mentioned this issue Oct 8, 2023
Closed
@tesol2y090
Copy link

Hi @thomaseizinger, My name is Gang. I'm part of PLDG cohort 1. Could i claim this issues.

@thomaseizinger
Copy link
Contributor

Hi @thomaseizinger, My name is Gang. I'm part of PLDG cohort 1. Could i claim this issues.

Please refer to one of the current maintainers for that.

@guillaumemichel
Copy link
Contributor

@tesol2y090 you are welcome to open a PR to address this issue

@kt-wawro kt-wawro moved this to In Progress in PLDG Cohort 1 Github Board Oct 22, 2024
@kt-wawro kt-wawro moved this from In Progress to Done in PLDG Cohort 1 Github Board Oct 22, 2024
@kt-wawro kt-wawro moved this from Done to In Progress in PLDG Cohort 1 Github Board Oct 29, 2024
@jxs jxs changed the title identify: implement signedPeerRecord identify: implement SignedPeerRecord Dec 11, 2024
@drHuangMHT drHuangMHT linked a pull request Dec 31, 2024 that will close this issue
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
difficulty:easy help wanted
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(identify): implement signedPeerRecord drHuangMHT/rust-libp2p
5 participants
@Marcel-G @thomaseizinger @maschad @guillaumemichel @tesol2y090