libp2p / specs

Technical specifications for the libp2p networking stack
https://libp2p.io
1.52k stars 272 forks source link

Allow peer to find itself in FIND_NODE #535

Open sgdxbc opened 1 year ago

sgdxbc commented 1 year ago

In https://github.com/libp2p/rust-libp2p/issues/3692 I was informed that several implementation exclude the calling peer from FIND_NODE result, and this behavior is not required by spec and does not make sense to me.

In my use case, I want to get consistent FIND_NODE result no mater which peer is calling it. However when local peer is indeed the closest peer, e.g., the input hash is identical to local peer's id, the result from local peer is different from the results from others.

mxinden commented 1 year ago

Instead of requiring a change in a libp2p implementation directly, I suggest wrapping the Kademlia::find_node method where that wrapper adds the local peer ID in case it was requested.

sgdxbc commented 1 year ago

That's exactly how I workaround for my use case currently, my code for reference

match event {
  SwarmEvent::Behaviour(AppEvent::Kad(KademliaEvent::OutboundQueryProgressed {
      id,
      result: QueryResult::GetClosestPeers(result),
      ..
  })) if *id == find_id => {
      let Ok(result) = result else {
          unimplemented!()
      };
      // kad excludes local peer id from `GetClosestPeers` result for unknown reason
      // so by default, the result from closest peers themselves is different from the others
      // add this workaround to restore a consistent result
      let mut peers = result.peers.clone();
      let k = |peer_id: &PeerId| {
          kbucket::Key::from(*peer_id).distance(&kbucket::Key::from(key))
      };
      let local_id = *swarm.local_peer_id();
      let index = peers.binary_search_by_key(&k(&local_id), k).expect_err(
          "local peer id is always excluded from get closest peers result",
      );
      peers.insert(index, local_id);
      peers.pop();
      // ...
  }
  // ...
})
sgdxbc commented 1 year ago

I am not asking for implementations to change their behavior as long as their current behavior fits the common demand for this interface (which is probably true but I'm not sure). The key is to keep consistency between spec and implementations so other people will not get surprised by the result after reading the spec like I did.

So I can create a PR to mention this behavior in the spec if desired (not sure about the exact workflow to update spec but always here to help). And I am also curious about why implementation choose to do the exclusion explicitly. rust-libp2p says it following go-libp2p, and that's all I know.