-
Notifications
You must be signed in to change notification settings - Fork 984
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.
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
feat(swarm): allow behaviours to share addresses of peers #4371
feat(swarm): allow behaviours to share addresses of peers #4371
Conversation
swarm
): Add NewExternalAddrOfPeer
e435d95
to
711b03b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going in the right direction. Thank you for the work!
711b03b
to
3c4d34e
Compare
swarm
): Add NewExternalAddrOfPeerThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great progress! I've left a few comments :)
What I'd also like to see in this PR is:
- mDNS emitting
ToSwarm::NewExternalAddrOfPeer
- identify emitting
ToSwarm::NewExternalAddrOfPeer
for the self-advertised listen addresses of a peer - the rendezvous client emitting
ToSwarm::NewExternalAddrOfPeer
whenever it receives a peer record
Something that we should discuss is whether we want to add a public function to Swarm
that allows users to trigger this event without having to call particular behaviour. For example, at the moment, libp2p-request-response
offers a function add_address
to populate an internal map of peer addresses. I'd like that function to go away because I think it is none of libp2p-request-response
s business to do address management.
If we were to add a function to Swarm
that triggers this event, we could deprecate the function in libp2p-request-response
and instead add an internal cache that stores peer addresses. To avoid duplicated code across multiple behaviours, we could add a PeerAddresses
to libp2p-swarm
similar to how we have ExternalAddresses
.
I don't think all of this has to be done in one PR but what we do should be consistent. For example, I think it would be to merge this whilst also deprecating libp2p-request-response
's add_address
function to point to the new Swarm
function instead. We can always introduce the PeerAddresses
struct at a later point because it is a backwards-compatible change.
The same actually applies to |
18ba25d
to
0b292c0
Compare
@mxinden Let me know what you think of this. |
I am in favor of the additional method on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As always, appreciate the thorough work @StemCll!
I am wondering what a good scope is for this PR. It is not very big at this stage. Given that it is a breaking change, perhaps we should at least use it "end-to-end":
If that works, we can write follow-up issues to "transfer" the design in this PR to other behaviours. |
bec1aaf
to
6534f13
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the continued work!
I made some more comments :)
Plus, I'd still like to see the Swarm::add_address_of_peer
function and deprecate Kademlia::add_address
with a note pointing to the other function. In the future, we will then make that private.
We will also need to update the docs here: https://github.com/libp2p/rust-libp2p/blob/master/protocols/kad/src/lib.rs#L21-L34 |
This pull request has merge conflicts. Could you please resolve them @StemCll? 🙏 |
I am removing this from the next milestone because we can hopefully ship it as a non-breaking change once #4581 lands. |
3cdcfac
to
1704ee9
Compare
hi @thomaseizinger @mxinden 👋 |
I have limited availability the next weeks, will re-review mid-January :) |
This pull request has merge conflicts. Could you please resolve them @StemCll? 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Sorry for the delay on this.
Let me know if you are able to work in a few more comments, otherwise I'll finalize this at some point in the next two weeks.
Hi @thomaseizinger. No worries at all and thank you for getting back to this 😄. I will add these changes in a day or two. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thank you so much for this contribution and working in all my feedback :)
I've made a note for myself to add another test, will merge that in the upcoming days!
This allows us to remove the `put` function and we can now safely add all addresses individually to the cache. With the addition of the `ToSwarm::NewExternalAddrOfPeer` event, the safeguard of overwriting all addresses in the peer cache was no longer useful because any other behaviour would have to apply this as well. As such, we needed to build this safe-guard directly _into_ `PeerAddresses`. This also makes the added test pass. Previously, the events would never get emitted because after calling `.put`, the addresses were already in the cache and the `add`-function always returned `false`.
The test didn't initially pass so I had to make some more changes to the implementation. |
Approvals have been dismissed because the PR was updated after the send-it
label was applied.
Resolves: libp2p#4302. Pull-Request: libp2p#4371.
Description
Resolves: #4302.
Notes & open questions
Still WIP
Change checklist