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

identify spec #97

Merged
merged 7 commits into from May 1, 2019
Merged

identify spec #97

merged 7 commits into from May 1, 2019

Conversation

@vyzo
Copy link
Contributor

@vyzo vyzo commented Oct 10, 2018

No description provided.

@vyzo vyzo requested review from bigs, mgoelzer, raulk and marten-seemann Oct 10, 2018
@ghost ghost assigned vyzo Oct 10, 2018
@ghost ghost added the in progress label Oct 10, 2018

These are the addresses on which the peer is listening as multi-addresses.

### observedAddr
Copy link
Member

@tomaka tomaka Oct 10, 2018

Choose a reason for hiding this comment

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

There are two possibilities here: this can be either the stack of protocols the listener used in order to reach back the dialer, or the stack of protocols we think the dialer used to reach us.
For the sake of forward compatibility, I think it'd be a nice idea to define this more precisely, and I'd be in favour of the former.

Copy link
Contributor Author

@vyzo vyzo Oct 10, 2018

Choose a reason for hiding this comment

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

Added a source disambiguator, per discussion in meatspace.

Copy link
Member

@tomaka tomaka Oct 10, 2018

Choose a reason for hiding this comment

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

Per discussion I'm not satisfied with the wording, because I'd like to disambiguate what is returned when p2p-circuit is used, but I can't find any appropriate wording.

Copy link
Contributor Author

@vyzo vyzo Oct 10, 2018

Choose a reason for hiding this comment

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

We can add an example for circuit addresses specifically, as they seem to be the contentious issue.

Copy link
Contributor Author

@vyzo vyzo Oct 10, 2018

Choose a reason for hiding this comment

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

Added some examples, hopefully it's clearer now.

Copy link
Contributor

@bigs bigs Oct 12, 2018

Choose a reason for hiding this comment

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

maybe something like: "This address describes the dialed peer's observed route to the dialing peer."

so wordy tho

Copy link
Contributor Author

@vyzo vyzo Oct 12, 2018

Choose a reason for hiding this comment

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

yeah, and it's not really any better than the current wording.


### protocols

This is a list of protocols supported by the peer.
Copy link
Member

@raulk raulk Oct 11, 2018

Choose a reason for hiding this comment

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

not ordered in any specific manner.

Copy link
Contributor Author

@vyzo vyzo Oct 11, 2018

Choose a reason for hiding this comment

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

right, but do we need to say that?

Copy link
Member

@raulk raulk Oct 11, 2018

Choose a reason for hiding this comment

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

A list implies order. If order is not important, then we can say it's an unordered set. We shouldn't be sending duplicates anyway, right?

Copy link
Contributor Author

@vyzo vyzo Oct 12, 2018

Choose a reason for hiding this comment

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

it's a list on the wire, there is no set datatype.

Copy link
Member

@dryajov dryajov Oct 13, 2018

Choose a reason for hiding this comment

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

might be worth clarifying that this are multistream protocols (or protocol strings)?

@@ -0,0 +1,56 @@
# Identify v1.0.0

The identify protocol is used to identify a peer and its capabilities.
Copy link
Member

@raulk raulk Oct 11, 2018

Choose a reason for hiding this comment

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

can we unpack the definition of "identify a peer"? the recursive definition is confusing for a spec

Copy link
Contributor Author

@vyzo vyzo Oct 11, 2018

Choose a reason for hiding this comment

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

uhm ok; me loves recursion.

Copy link
Contributor Author

@vyzo vyzo Oct 11, 2018

Choose a reason for hiding this comment

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

i really can't think of a non-verbose way of expressing this.

Copy link
Member

@raulk raulk Oct 11, 2018

Choose a reason for hiding this comment

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

How about "the identify protocol is used to interrogate a peer for basic information and its capabilities, the details of which are defined below"?

Copy link
Contributor Author

@vyzo vyzo Oct 12, 2018

Choose a reason for hiding this comment

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

query, not interrogate.

### protocolVersion

The protocol version identifies the family of protocols used by the peer.
The current protocol version is `ipfs/0.1.0`; if the protocol does not match
Copy link
Member

@raulk raulk Oct 11, 2018

Choose a reason for hiding this comment

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

with the current logic, the micro/patch version can be different. only major and minor need to be equal.

Copy link
Contributor Author

@vyzo vyzo Oct 11, 2018

Choose a reason for hiding this comment

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

ok, will update. We'd also like to get to a point we don't close conns because of this.


### agentVersion

This is a free-form string, identifying the implementation of the peer.
Copy link
Member

@raulk raulk Oct 11, 2018

Choose a reason for hiding this comment

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

can we encourage a particular format, even if non-normative? the default is go-libp2p/<semver>, and IPFS changes this to "go-ipfs/" + version.CurrentVersionNumber + "/" + version.CurrentCommit.

https://github.com/ipfs/go-ipfs/blob/master/core/core.go#L99

Copy link
Contributor Author

@vyzo vyzo Oct 11, 2018

Choose a reason for hiding this comment

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

nah, let's let people call their agents whatever they want!

Copy link
Contributor Author

@vyzo vyzo Oct 11, 2018

Choose a reason for hiding this comment

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

although an example might be useful.

Copy link
Member

@raulk raulk Oct 11, 2018

Choose a reason for hiding this comment

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

example sounds good

Copy link
Contributor

@bigs bigs left a comment

observedAddr semantics are shockingly hard to describe. beyond that, i'm good


These are the addresses on which the peer is listening as multi-addresses.

### observedAddr
Copy link
Contributor

@bigs bigs Oct 12, 2018

Choose a reason for hiding this comment

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

maybe something like: "This address describes the dialed peer's observed route to the dialing peer."

so wordy tho

### listenAddrs

These are the addresses on which the peer is listening as multi-addresses.

Copy link
Contributor

@richardschneider richardschneider Nov 9, 2018

Choose a reason for hiding this comment

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

Does the multiaddress also include /ipfs/Qm...? If so, is /p2p also supported?

Copy link
Member

@Stebalien Stebalien Nov 9, 2018

Choose a reason for hiding this comment

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

It doesn't include it. Well, it shouldn't.

@daviddias
Copy link
Member

@daviddias daviddias commented Nov 22, 2018

Please make sure to take https://github.com/libp2p/js-libp2p-switch/issues/78 into account

@yusefnapora
Copy link
Contributor

@yusefnapora yusefnapora commented Apr 16, 2019

@vyzo this looks pretty solid to me - should we merge?

I don't have the context behind @daviddias comment about the js / go interop, so maybe we need to address the changes made there?

@vyzo
Copy link
Contributor Author

@vyzo vyzo commented Apr 16, 2019

We also have identify/push now, which is the active update variant of the protocol and should be in the spec.

@vyzo
Copy link
Contributor Author

@vyzo vyzo commented Apr 16, 2019

@yusefnapora feel free to edit!

@ghost ghost assigned yusefnapora May 1, 2019
@yusefnapora
Copy link
Contributor

@yusefnapora yusefnapora commented May 1, 2019

@vyzo I added a bit about identify/push - lmk if it looks alright. If so, I think we should press the button 😃

Copy link
Contributor Author

@vyzo vyzo left a comment

LGTM, but I can't actually approve as I am the original author of the pr :)

@yusefnapora
Copy link
Contributor

@yusefnapora yusefnapora commented May 1, 2019

hehe, fair enough :) I'll let you do the honor of pressing the big green button then

@vyzo
Copy link
Contributor Author

@vyzo vyzo commented May 1, 2019

Any objections to merging this?

`/ipfs/id/push/1.0.0` as the protocol id string. When the remote peer accepts the stream,
the local peer will send an `Identify` message and close the stream.

## The Identify Message
Copy link
Member

@Stebalien Stebalien May 1, 2019

Choose a reason for hiding this comment

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

We should probably note that any missing fields should be ignored (so we can do a partial identify push).

Copy link
Contributor Author

@vyzo vyzo May 1, 2019

Choose a reason for hiding this comment

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

@yusefnapora want to add it?

@vyzo
Copy link
Contributor Author

@vyzo vyzo commented May 1, 2019

Alright, let's merge!

@vyzo vyzo merged commit 039a62f into master May 1, 2019
@ghost ghost removed the in progress label May 1, 2019
@vyzo vyzo deleted the feat/identify branch May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants