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.

Sign up for GitHub

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

Jump to bottom

iroh p2p peers: command to list peers #397

Merged
merged 6 commits into from
Oct 24, 2022
Merged

iroh p2p peers: command to list peers #397

merged 6 commits into from
Oct 24, 2022

Conversation

b5
Copy link
Member

@b5 b5 commented Oct 22, 2022 •
edited
Loading 

$ iroh p2p peers --help
List connected peers

Usage: iroh p2p peers

Options:
  -h, --help     Print help information
  -V, --version  Print version information


'p2p peers' lists the set of peer addresses this node is currently connected to.
The address format is a multiaddress, or 'multiaddr' for short. For example:

  /ip4/104.131.131.82/tcp/4001/p2p/QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ

The last element after the '/' is the Peer Identifier or 'PeerID'. Either the
PeerID or the entire multiaddr can be given to the 'p2p lookup' command
for additional details about the peer. For example:

  > iroh p2p lookup QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ
  > iroh p2p lookup /ip4/104.131.131.82/tcp/4001/p2p/QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ

For more info on multiaddrs see https://iroh.computer/docs/concepts#multiaddr.

builds on #402

@b5 b5 added this to the v0.1.0 milestone Oct 23, 2022
@b5 b5 added the c-ctl label Oct 23, 2022
@b5 b5 self-assigned this Oct 23, 2022
@b5 b5 marked this pull request as ready for review October 23, 2022 17:41
b5
b5 commented Oct 23, 2022
View reviewed changes
iroh/src/p2p.rs
fn display_peers(peers: HashMap<PeerId, Vec<Multiaddr>>) {
// let mut pid_str: String;
for (peer_id, addrs) in peers {
if let Some(addr) = addrs.first() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just grabbing the first address, which I'm doubtful is actually the address we're connected to. We should make sure the addresses listed here are actually connected

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a rule of thumb for this in lookup as well: some entries return 20+ addrs, and that's... not necessary.

For the lookup of the currect peer, we can be more comfortable just displaying the first 5 (or whatever number we choose), because according to the docs they rank the external_addrs that we can be dialed at, but like you mention, we don't have that luxury here, or for the other lookup commands.

But also, we may want to pair this down. Maybe you use p2p peers to just get a list of the peer ids (not peer_ids and addrs), and use p2p lookup PEER_ID to investigate possible addresses?

@b5 b5 requested a review from ramfox October 24, 2022 00:29
ramfox
ramfox previously approved these changes Oct 24, 2022
View reviewed changes
Copy link
Contributor

@ramfox ramfox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving this, but left 2 comments you can take or leave!

iroh-api/src/error.rs Outdated
@@ -0,0 +1,27 @@
use std::io;
use thiserror::Error as ThisError;
// use std::error::Error;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you gotten the error talk from dig yet? don't want to get in trouble for approving thiserror usage on top of anyhow usage 🤣

@b5 b5 requested a review from ramfox October 24, 2022 13:34
rklaehn
rklaehn approved these changes Oct 24, 2022
View reviewed changes
iroh-api/src/p2p.rs
@@ -69,6 +71,13 @@ impl P2p for ClientP2p {
}
.map_err(|e| map_service_error("p2p", e))
}

async fn peers(&self) -> Result<HashMap<PeerId, Vec<Multiaddr>>> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer BTreeMap, less random...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting. I'll make a note in a follow-up issue

@b5 b5 merged commit 6f3d487 into main Oct 24, 2022
@ramfox
Copy link
Contributor

ramfox commented Oct 24, 2022

closes n0-computer/beetle#127

@b5 b5 deleted the b5/feat_peers_list_cmd branch October 26, 2022 02:02
@b5 b5 mentioned this pull request Nov 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rklaehn rklaehn rklaehn approved these changes

@ramfox ramfox Awaiting requested review from ramfox

Assignees

@b5 b5

Labels
None yet
Projects
iroh
Archived in project
Milestone
v0.1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@b5 @ramfox @rklaehn