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.

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

libp2p-kad: provide a way to get known addresses of a peer from address book #3633

Closed
folex opened this issue Mar 18, 2023 · 11 comments
Closed
Labels
need/author-input Needs input from the original author

Comments

@folex
Copy link
Contributor

folex commented Mar 18, 2023

Description

Since Kademlia is basically an address book of peers (aka routing table), it would be great to be able to actually read from that address book. Check whether peer is presented in that address book or not.

For example, one might (and we do, see for example our fn discover_peer) use Kademlia to discover Peer address, establish connection, and exchange some messages.

But since addresses_of_peer is deprecated, and Kademlia::kbucket is private, how to read from the address book?

There's handle_pending_outbound_connection which is proposed to replace addresses_of_peer, but it implies an effect, since it signifies that a new connection happened. So it is a bad fit for the goal of "read address from Kademlia address book".

That being said, what are alternatives that you see viable?

Motivation

We need to read addresses from the Kademlia by PeerId, because we use Kademlia as a peers' addresses discovery mechanism.

In addition, I think it makes sense to separate "read model" from effectful methods like handle_pending_outbound_connection.

Current Implementation

We've relied on addresses_of_peer, but it is now removed from Kademlia completely.

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

Maybe, but need a guidance on the design and alignment with rust-libp2p team's vision.

Let's discuss!

I'm happy to provide more information on our use-case and discuss different design approaches.

Thank you for fast-paced development of rust-libp2p, it is a tremendous effort, and I really appreciate the complexity you are tackling! ❤️

@dariusc93
Copy link
Member

dariusc93 commented Mar 19, 2023

I can agree that there should be some way in going about it since handle_pending_outbound_connection intentions is more related to a pending connection (although not sure if that was also a similar intention to addresses_of_peer. Although for me, I would be able to begin to move away from it, but for you, I would assume using Kademlia::kbuckets or Kademlia::kbucket is not an option? Might add a little more work but you should be able to get the addresses from the peer there.

@folex
Copy link
Contributor Author

folex commented Mar 19, 2023

Kademlia::kbucket might be an answer :)

I read our and rust-libp2p code for a bit, and it seems that we do not depend on addresses being available in queries. That's because we rely on peers being reachable and connected after ClosestQueryIter is finished.

I'll try and see how our tests are feeling, and get back with results!

@dariusc93
Copy link
Member

Hey @folex were you able to get the results you were looking for?

@mxinden
Copy link
Member

mxinden commented Mar 22, 2023

Thanks for the detailed report. I always considered the NetworkBehaviour::addresses_of_peer an internal method, i.e. a method that is called by libp2p-swarm only. Thus I did not think of the deprecation breaking something downstream. I understand your use-case and want to support it with libp2p-kad. In case the Kademlia::kbucket API is not enough, let's discuss introducing a new method on Kademlia (not on NetworkBehaviour).

@gurinderu
Copy link
Contributor

gurinderu commented Mar 22, 2023

Hey, @mxinden I work with @folex
I have run our tests with handle_pending_outbound_connection and pure kbucket fetch. The second option doesn't work for our case. Using handle_pending_outbound_connection looks strange to me because of unused parameters and unclear (name that implies effect) naming.
I suggest adding a method

fn addresses_of_peer(
    &mut self,
    peer_id: PeerId,
) -> Vec<Multiaddr> {
    // We should order addresses from decreasing likelyhood of connectivity, so start with
    // the addresses of that peer in the k-buckets.
    let key = kbucket::Key::from(peer_id);
    let mut peer_addrs =
        if let kbucket::Entry::Present(mut entry, _) = self.kbuckets.entry(&key) {
            let addrs = entry.value().iter().cloned().collect::<Vec<_>>();
            debug_assert!(!addrs.is_empty(), "Empty peer addresses in routing table.");
            addrs
        } else {
            Vec::new()
        };

    // We add to that a temporary list of addresses from the ongoing queries.
    for query in self.queries.iter() {
        if let Some(addrs) = query.inner.addresses.get(&peer_id) {
            peer_addrs.extend(addrs.iter().cloned())
        }
    }

    peer_addrs
}

into Kademlia impl and reuse it into handle_pending_outbound_connection.

@dariusc93
Copy link
Member

Hey, @mxinden I work with @folex I have run our tests with handle_pending_outbound_connection and pure kbucket fetch. The second option doesn't work for our case. Using handle_pending_outbound_connection looks strange to me because of unused parameters and unclear (name that implies effect) naming. I suggest adding a method

fn addresses_of_peer(
    &mut self,
    peer_id: PeerId,
) -> Vec<Multiaddr> {
    // We should order addresses from decreasing likelyhood of connectivity, so start with
    // the addresses of that peer in the k-buckets.
    let key = kbucket::Key::from(peer_id);
    let mut peer_addrs =
        if let kbucket::Entry::Present(mut entry, _) = self.kbuckets.entry(&key) {
            let addrs = entry.value().iter().cloned().collect::<Vec<_>>();
            debug_assert!(!addrs.is_empty(), "Empty peer addresses in routing table.");
            addrs
        } else {
            Vec::new()
        };

    // We add to that a temporary list of addresses from the ongoing queries.
    for query in self.queries.iter() {
        if let Some(addrs) = query.inner.addresses.get(&peer_id) {
            peer_addrs.extend(addrs.iter().cloned())
        }
    }

    peer_addrs
}

into Kademlia impl and reuse it into handle_pending_outbound_connection.

I am curious. Is there anything about Kademlia::kbucket that doesnt work for you or are you looking for the addresses from the query iterator too?

@github-actions
Copy link

Oops, seems like we needed more information for this issue, please comment with more details or this issue will be closed in 7 days.

@folex
Copy link
Contributor Author

folex commented Jun 22, 2023

I am curious. Is there anything about Kademlia::kbucket that doesnt work for you or are you looking for the addresses from the query iterator too?

It seems that address inside the Query Iterator are crucial: we were not able to do a discovery based on get_closest_peers to make discovered addresses available in Kademlia::kbucket after GetClosest request is finished. They are only available in the iterator at that point in time.

@thomaseizinger
Copy link
Contributor

Related: #4101.

@github-actions
Copy link

Oops, seems like we needed more information for this issue, please comment with more details or this issue will be closed in 7 days.

@github-actions
Copy link

github-actions bot commented Jul 7, 2023

This issue was closed because it is missing author input.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/author-input Needs input from the original author
Projects
None yet
Development

No branches or pull requests

5 participants