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

Allow peer to find itself in FIND_NODE #535

Open
sgdxbc opened this issue Mar 31, 2023 · 3 comments
Open

Allow peer to find itself in FIND_NODE #535

sgdxbc opened this issue Mar 31, 2023 · 3 comments

Comments

@sgdxbc
Copy link

sgdxbc commented Mar 31, 2023

In libp2p/rust-libp2p#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
Copy link
Member

mxinden commented Apr 9, 2023

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
Copy link
Author

sgdxbc commented Apr 10, 2023

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
Copy link
Author

sgdxbc commented Apr 10, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Triage
Development

No branches or pull requests

2 participants