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

Add spec on addressing & use of multiaddr #191

Merged
merged 17 commits into from
Jul 24, 2021
Merged

Conversation

yusefnapora
Copy link
Contributor

This is a spec on addressing I've been chipping away at for a while now. So far it covers how multiaddrs basically work, the rules for DNS resolution, and the address formats for (most) transports.

TODO:

  • add missing transport address formats (webrtc-direct, ws-star, etc)

Also TODO, but probably in a second pass:

  • Add section about address discovery and exchange. This could potentially be where we spell out the abstract "peer routing" interface, which would be useful to xref from e.g. the DHT spec.

@Stebalien, the DNS resolution described here aligns with where you're going with this PR, I think: multiformats/go-multiaddr-dns#17

addresses are generally exchanged over the wire as binary-encoded multiaddrs in
libp2p's core protocols.

When exchanging addresses, peers send a multiaddr containing both their network
Copy link
Member

Choose a reason for hiding this comment

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

Nit: on the wire, we usually don't include the peer-id part (it's usually implicit). I'm not sure how to convey this.

Copy link
Member

Choose a reason for hiding this comment

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

When sending self addresses over the wire, we omit the identity part, as it's considered superfluous because libp2p connections are authenticated by principle. If the other party needs to relay our addresses to a third party, it should add the identity part to form a fully qualified address.

Copy link
Member

Choose a reason for hiding this comment

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

Revisiting this...

  1. "network" address is usually referred to as the "transport" address (not critical, but it could involve relays and multiple network addresses).
  2. We usually send AddrInfos (AddrInfo{ID: <peer-id>, Addrs: <transport-addrs>}).

But this section is really fine as-is, this is just me nit picking.

addressing/README.md Outdated Show resolved Hide resolved
addressing/README.md Outdated Show resolved Hide resolved
addressing/README.md Outdated Show resolved Hide resolved
addressing/README.md Outdated Show resolved Hide resolved
addressing/README.md Outdated Show resolved Hide resolved
@tomaka
Copy link
Member

tomaka commented Jul 23, 2019

👍 for not trying to be too generic, and instead exhaustively listing the valid multiaddresses. That's the way to go!

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on! 🎉🎉🎉

A few comments:

  1. It feels like the term multiaddr is somewhat overloaded to refer to multiaddr particles and full multiaddrs, but there's no formal concept of validity/conformance/soundness.
  2. How about we make this spec solely about defining sound/functional multiaddrs in the context of libp2p? i.e. what combinations/compositions are valid and should be understood by all implementations.
  3. We can move the valuable background context narrative to a non-normative doc, and refer to it from here. This doc would then stay on as a normative spec.
  4. If we do (2) and (3), we can remove the overloading by making "multiaddr" refer to "sound multiaddr" everywhere, and making its constituents just components.

addressing/README.md Outdated Show resolved Hide resolved
addressing/README.md Outdated Show resolved Hide resolved
addressing/README.md Outdated Show resolved Hide resolved
addresses are generally exchanged over the wire as binary-encoded multiaddrs in
libp2p's core protocols.

When exchanging addresses, peers send a multiaddr containing both their network
Copy link
Member

Choose a reason for hiding this comment

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

When sending self addresses over the wire, we omit the identity part, as it's considered superfluous because libp2p connections are authenticated by principle. If the other party needs to relay our addresses to a third party, it should add the identity part to form a fully qualified address.

addressing/README.md Outdated Show resolved Hide resolved
addressing/README.md Outdated Show resolved Hide resolved
addressing/README.md Outdated Show resolved Hide resolved
addressing/README.md Outdated Show resolved Hide resolved
addressing/README.md Outdated Show resolved Hide resolved
addressing/README.md Outdated Show resolved Hide resolved
addressing/README.md Outdated Show resolved Hide resolved
@mxinden mxinden linked an issue Apr 16, 2021 that may be closed by this pull request
marten-seemann and others added 2 commits July 21, 2021 15:49
Co-authored-by: raulk <raul@protocol.ai>
Co-authored-by: Marcin Rataj <lidel@lidel.org>
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Left a couple of comments. All but one contain a direct change suggestion.

Other than that, this looks good to me. Thanks @yusefnapora and @marten-seemann for the work!

Small ask: Can we wrap the lines to e.g. 80 characters before merging? I am happy to do it. Just let me know once ready.

addressing/README.md Outdated Show resolved Hide resolved
addressing/README.md Outdated Show resolved Hide resolved
addressing/README.md Outdated Show resolved Hide resolved
addressing/README.md Outdated Show resolved Hide resolved
addressing/README.md Outdated Show resolved Hide resolved
addressing/README.md Outdated Show resolved Hide resolved
addressing/README.md Outdated Show resolved Hide resolved
addressing/README.md Outdated Show resolved Hide resolved
addressing/README.md Outdated Show resolved Hide resolved
marten-seemann and others added 2 commits July 22, 2021 12:14
@marten-seemann
Copy link
Contributor

Small ask: Can we wrap the lines to e.g. 80 characters before merging? I am happy to do it. Just let me know once ready.

It's ready now. But is this something we actually care about? If so, should we check this in CI?

@mxinden
Copy link
Member

mxinden commented Jul 22, 2021

It's ready now.

Wow, that was a fast turnaround time. Thanks.

But is this something we actually care about? If so, should we check this in CI?

While i prefer Markdown documents to be wrapped at XXX chars, the thing I mostly care about is consistency across the repository. The majority of documents is wrapped at 80 chars, thus I would prefer new documents to comply with that convention.

If so, should we check this in CI?

I am not opposed to it, though I am not sure it is worth it, given that there are likely some cases where more than 80 chars are needed, e.g. for visualizations.

@marten-seemann
Copy link
Contributor

@Stebalien @lidel @yusefnapora @raulk This PR has been sitting here for quite a while, and we only made a bunch of cosmetic changes to it now. In the interest of moving things forward (both this PR and Protocol Select, which will modify this document), we're going to merge this PR this week, unless we hear any blocking feedback by Friday EOD.

As always, merging this PR doesn't mean that this document is final, further improvements are possible (and welcome!) via separate PRs.

Copy link
Contributor

@BigLep BigLep left a comment

Choose a reason for hiding this comment

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

Good stuff. Given how long this has been open and you've gotten some approval, I think we are good to merge. We can handle additional comments after the face.

addressing/README.md Outdated Show resolved Hide resolved
addressing/README.md Show resolved Hide resolved
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Thank you for pushing this over the finish line!

addresses are generally exchanged over the wire as binary-encoded multiaddrs in
libp2p's core protocols.

When exchanging addresses, peers send a multiaddr containing both their network
Copy link
Member

Choose a reason for hiding this comment

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

Revisiting this...

  1. "network" address is usually referred to as the "transport" address (not critical, but it could involve relays and multiple network addresses).
  2. We usually send AddrInfos (AddrInfo{ID: <peer-id>, Addrs: <transport-addrs>}).

But this section is really fine as-is, this is just me nit picking.

addressing/README.md Show resolved Hide resolved
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

this is looking great!

addressing/README.md Outdated Show resolved Hide resolved
/p2p/QmYyQSo1c1Ym7orWxLYvCrM2EmxFTANf8wXmmE7DWjhx5N
```

Where `QmYyQSo1c1Ym7orWxLYvCrM2EmxFTANf8wXmmE7DWjhx5N` is the string representation
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: encoded string representation

@marten-seemann marten-seemann merged commit 98e0ca0 into master Jul 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add small spec on addressing
9 participants