refactor(kad): make `Kademlia::add_address` return Result #4280

arsenron · 2023-08-01T11:34:23Z

Description

This PR makes a clear separation between successful and unsuccessful result of adding a peer to the DHT. It seems to me that it improves ergonomics of the public API.

Notes & open questions

If you agree on this PR, I will add some tests and update examples.

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

Thanks! This looks good to me :)

It needs a changelog entry and a version bump. Given that this is a breaking change, we'll hold off on merging this for a bit to allow us to batch breaking changes together. Hence, I'll convert this to a draft so that it doesn't get merged accidentially!

protocols/kad/src/lib.rs

mxinden

I am in favor of the change. Thanks.

examples/file-sharing/src/network.rs

thomaseizinger · 2023-09-21T08:16:29Z

@arsenron Can you fix the CI errors please? We are thinking of cutting a new (breaking) release and thus would like all the queued PRs to be ready for merge :)

mergify · 2023-09-21T08:44:30Z

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

arsenron · 2023-09-21T08:46:37Z

@arsenron Can you fix the CI errors please? We are thinking of cutting a new (breaking) release and thus would like all the queued PRs to be ready for merge :)

Sure! I will do it today.

arsenron · 2023-09-21T16:48:01Z

@thomaseizinger Done. But just cargo-deny fails, but it is unrelevant to the updates made by this PR, so I skipped it.

thomaseizinger · 2023-09-21T22:04:13Z

@thomaseizinger Done. But just cargo-deny fails, but it is unrelevant to the updates made by this PR, so I skipped it.

Great! Yeah good call on that one, we already have a PR queued to fix that :)

thomaseizinger

I noticed an overlap with an upcoming feature that I think we have to discuss before moving forward here. Sorry for the late notice!

thomaseizinger · 2023-09-21T23:25:37Z

protocols/kad/src/behaviour.rs

+    pub fn add_address(
+        &mut self,
+        peer: &PeerId,
+        address: Multiaddr,
+    ) -> Result<RoutingUpdateOk, RoutingUpdateError> {


Sorry that this is only coming up now but I just realized that we have a feature coming up that conflicts with this idea. It is here: #4371

This function will likely be deprecated in favor of a general add_address_of_peer function on Swarm. This means, we won't be able to return a specific error there!

In fact, we won't be able to return anything at all.

Thus, I think we should actually change this PR into preparing this API for the above change. Here is what I'd suggest but I am interested to hear your and @mxinden's opinion:

Mark RoutingUpdate as deprecated but retain the current functionality.

Always emit KademliaEvent::RoutingUpdated, even for the immediate RoutingUpdate::Success case

Add a debug_assert to the SelfEntry case. If we attempt to add ourselves to the routing table, this is a programmer bug and should not happen. We can ignore that in production (but should probably emit a fat warn!), hence the debug_assert.

We already log a warning for the bucket-full case so we can just leave it at that.

This makes this a backwards-compatible change and we can land it right away.

I missed this as well. I am sorry @arsenron.

Suggestion overall sounds good to me.

Add a debug_assert to the SelfEntry case. If we attempt to add ourselves to the routing table, this is a programmer bug and should not happen. We can ignore that in production (but should probably emit a fat warn!), hence the debug_assert.

Panicing on invalid user input seems rather drastic. Wouldn't a warn! suffice in all cases? Also, not as part of this pull request, we should handle the SelfEntry on the libp2p-swarm level. We should at least log when a NetworkBehaviour or user (via Swarm::add_address_of_peer) wants to report a an external address of a remote peer where that peer is actually the local peer.

I missed this as well. I am sorry @arsenron.

Suggestion overall sounds good to me.

Add a debug_assert to the SelfEntry case. If we attempt to add ourselves to the routing table, this is a programmer bug and should not happen. We can ignore that in production (but should probably emit a fat warn!), hence the debug_assert.

Panicing on invalid user input seems rather drastic. Wouldn't a warn! suffice in all cases?

The warn will be there in all cases but pancking in debug builds is fine, no? You didn't validate your inputs correctly in that case.

Also, not as part of this pull request, we should handle the SelfEntry on the libp2p-swarm level. We should at least log when a NetworkBehaviour or user (via Swarm::add_address_of_peer) wants to report a an external address of a remote peer where that peer is actually the local peer.

True, as part of #4371, we should probably not forward any of these events! cc @StemCll

@arsenron I fixed the merge conflicts for you. Are you still interested in working on this given the above insight?

@thomaseizinger sorry, little busy now, but I will check it a bit later

mergify · 2023-09-22T11:04:33Z

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

mergify · 2023-10-20T06:56:53Z

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

mergify · 2023-10-20T10:36:42Z

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

thomaseizinger · 2023-10-24T00:16:22Z

Removing this from the milestone because the proposed way forward is backwards-compatible: https://github.com/libp2p/rust-libp2p/pull/4280/files#r1333693065.

Make Kademlia::add_address return Result

aef9507

thomaseizinger changed the title ~~refactor(kad): Make Kademlia::add_address return Result~~ refactor(kad): make Kademlia::add_address return Result Aug 1, 2023

thomaseizinger approved these changes Aug 1, 2023

View reviewed changes

thomaseizinger added this to the v0.53.0 milestone Aug 1, 2023

thomaseizinger marked this pull request as draft August 1, 2023 12:56

thomaseizinger reviewed Aug 1, 2023

View reviewed changes

protocols/kad/src/lib.rs Outdated Show resolved Hide resolved

mxinden reviewed Aug 1, 2023

View reviewed changes

arsenron added 2 commits August 1, 2023 16:30

Expose RoutingUpdateError

60c25b7

Update examples

82133f3

folex reviewed Aug 22, 2023

View reviewed changes

examples/file-sharing/src/network.rs Outdated Show resolved Hide resolved

arsenron and others added 3 commits August 22, 2023 21:38

Add expect to network example

fd1b37c

fmt

992cddf

Merge branch 'master' into kad-add-address

a650274

Try fix CI and update changelog

a117e25

Merge remote-tracking branch 'upstream/master' into kad-add-address

11f3a17

thomaseizinger requested changes Sep 21, 2023

View reviewed changes

libp2p deleted a comment from mxinden Sep 22, 2023

Merge branch 'master' into kad-add-address

c79058b

Merge branch 'master' into kad-add-address

5fe3128

Merge branch 'master' into kad-add-address

64f3243

thomaseizinger removed this from the v0.53.0 milestone Oct 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(kad): make `Kademlia::add_address` return Result #4280

refactor(kad): make `Kademlia::add_address` return Result #4280

arsenron commented Aug 1, 2023

thomaseizinger left a comment

mxinden left a comment

thomaseizinger commented Sep 21, 2023

mergify bot commented Sep 21, 2023

arsenron commented Sep 21, 2023

arsenron commented Sep 21, 2023

thomaseizinger commented Sep 21, 2023

thomaseizinger left a comment

thomaseizinger Sep 21, 2023

mxinden Sep 22, 2023

thomaseizinger Sep 22, 2023

thomaseizinger Oct 20, 2023

arsenron Oct 20, 2023

mergify bot commented Sep 22, 2023

mergify bot commented Oct 20, 2023

mergify bot commented Oct 20, 2023

thomaseizinger commented Oct 24, 2023

refactor(kad): make Kademlia::add_address return Result #4280

Are you sure you want to change the base?

refactor(kad): make Kademlia::add_address return Result #4280

Conversation

arsenron commented Aug 1, 2023

Description

Notes & open questions

Change checklist

thomaseizinger left a comment

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

thomaseizinger commented Sep 21, 2023

mergify bot commented Sep 21, 2023

arsenron commented Sep 21, 2023

arsenron commented Sep 21, 2023

thomaseizinger commented Sep 21, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger Sep 21, 2023

Choose a reason for hiding this comment

mxinden Sep 22, 2023

Choose a reason for hiding this comment

thomaseizinger Sep 22, 2023

Choose a reason for hiding this comment

thomaseizinger Oct 20, 2023

Choose a reason for hiding this comment

arsenron Oct 20, 2023

Choose a reason for hiding this comment

mergify bot commented Sep 22, 2023

mergify bot commented Oct 20, 2023

mergify bot commented Oct 20, 2023

thomaseizinger commented Oct 24, 2023

refactor(kad): make `Kademlia::add_address` return Result #4280

refactor(kad): make `Kademlia::add_address` return Result #4280