Why GetProvidersOk returns PeerId only instead of PeerInfo?

[clovas]

EDIT: I'm currently using libp2p 0.56.0, and I've noticed that 0.57.1 appears to have introduced libp2p-peer-store, but I'm not sure how relevant this is to my issue, since the CHANGELOG isn't very detailed, and the API documentation has been broken since 0.57.0.

I apologize if this question seems silly. Please allow me to outline what I'm currently working on, and then I'll ask my question at the end.

1. I want to use get_providers to query the providers for an IPFS CID, and then connect to some of those providers to request the content.
2. After calling get_providers, I will receive a series of GetProvidersOk responses. However, GetProvidersOk only returns a PeerId. Although the dial function accepts a PeerId, a valid Multiaddr is ultimately required to initiate a dial.
3. While a query is in progress, dial can use the Multiaddrs of queries currently in progress in the QueryPool, however, once the query is complete, these addresses are no longer valid for dial because they are removed from the QueryPool, and since GetProvidersOk returns only PeerId, these Multiaddrs are ultimately discarded.
4. Tests on the public IPFS network have shown that responses to external queries from this machine are likely to be empty. Due to the existence of #6696 even if dial is called during the query process, the probability of successfully establishing a connection remains limited.

The above things are based on my review of the source code and my experiments. I don't know to what extent they reflect reality, but I did my best.

Anyway, I believe that GetProvidersOk should carry a PeerInfo rather than just a PeerId. The reason is that GetClosestPeersOk currently returns a Vec<PeerInfo>, and the functionality of get_closest_peers and get_providers is similar. Furthermore, this approach would make it easier for the caller to manage these addresses on their own.

Given that libp2p is a project that has been ongoing for several years, and I haven’t been able to find any documentation explaining the design philosophy or guidelines for libp2p-rust,

So I’m not sure if the idea I’ve proposed goes against any principles, and I also do have a lot of questions. So I’d like to know what others think of my idea.

Also, if you have any additional information about libp2p-rust (e.g. documentation) that you’d be willing to share, I’d greatly appreciate it.

[DahanItamar]

Your reading of the source is right. I went through libp2p-kad 0.48.0 at the libp2p-v0.56.0 tag, and master is the same in this area. I haven't run this against the network; the links below are pinned to that tag.

Where the provider addresses go

When a GET_PROVIDERS response arrives, the provider peers and the closer peers both go through discovered. Their multiaddrs are stored only in that query's temporary query.peers.addresses map, and FoundProviders is then emitted with just the PeerIds (L2362-L2393). The only other place those addresses are read is handle_pending_outbound_connection, which merges the routing-table entry with the maps of the queries that are still running. Once the query is removed, the addresses are gone, as you found.

Kademlia also doesn't announce them anywhere else. It emits ToSwarm::NewExternalAddrOfPeer only when a peer is actually inserted into the routing table (L2603-L2609). That matters for libp2p-peer-store, since its MemoryStore only records addresses from FromSwarm::NewExternalAddrOfPeer (memory_store.rs#L197). Adding it won't capture provider addresses by itself. It will capture what identify announces after you connect, though, and that is useful for reconnecting later.

What works with 0.56 today

Dial from inside the FoundProviders arm, while the query is still alive:

// `MyBehaviourEvent::Kad` is whatever your `#[derive(NetworkBehaviour)]` generates for the kad field.
SwarmEvent::Behaviour(MyBehaviourEvent::Kad(kad::Event::OutboundQueryProgressed {
    result: kad::QueryResult::GetProviders(Ok(kad::GetProvidersOk::FoundProviders { providers, .. })),
    ..
})) => {
    for peer in providers {
        // `Swarm::dial` asks the behaviours for addresses synchronously, so kad
        // still sees this query's map here.
        let _ = swarm.dial(peer);
    }
}

This works because Swarm::dial calls handle_pending_outbound_connection inside the call, before returning. The addresses only need to exist at the moment you call it, not when the connection completes. Dialing later, for example after step.last, is what loses them.

Why the success rate is still low: #6696

discovered uses HashMap::insert, so each response replaces the addresses for a peer instead of adding to them. On the public DHT, the same provider is often reported by several peers, and some of those responses carry no addresses or stale ones. When that happens, a later report wipes out an earlier good one before your dial runs. That fits what you're seeing, and it makes #6696 worth fixing on its own, independent of the API question.

On your proposal

There's a direct precedent. GetClosestPeersOk.peers is a Vec<PeerInfo> built from this same per-query map (L1525-L1534, via into_peerinfos_iter). So returning PeerInfo from FoundProviders would expose data kad already holds at that point, in the shape it already uses elsewhere. The catch is that providers is a public HashSet<PeerId> field, so changing it is a breaking change. An additional field or a new variant would avoid that. Which of those to take is a call for the maintainers, so it's probably worth opening as an issue that links #6696. The two changes together would cover the whole "find providers, then connect" flow.

[clovas]

Thank you for your reply, especially your explanation of peer-store, it was very helpful and saved me a lot of time (for reading source code).