Open thomaseizinger opened 3 years ago
Is there a plan of integrating signed peer records into
/identify
?
Yes, very much, i.e. it is on our roadmap (see ### Peer Routing Records
in https://github.com/libp2p/specs/pull/339/).
As far as I can tell go-libp2p already does via a combination of the two suggestions above, more specifically via:
/p2p/id/1.1.0
.optional bytes signedPeerRecord = 8;
.See https://github.com/libp2p/go-libp2p/pull/747 for details.
Unfortunately the identify protocol specification has not been updated.
A new protocol name:
/p2p/id/1.1.0
.
Is there any reasoning why version 1.1.0
was chosen? Is there a /p2p/id/1.0.0
as well?
Unfortunately the identify protocol specification has not been updated.
I might set out and try to update the spec then.
Is there any reasoning why version
1.1.0
was chosen? Is there a/p2p/id/1.0.0
as well?
I would guess that /p2p/id/1.1.0
was chosen instead of /p2p/id/1.0.0
to avoid confusion with /ipfs/id/1.0.0
.
I might set out and try to update the spec then.
That would be very much appreciated. Thank you Thomas!
I tried to update the spec and I am in-between two minds.
For one, we are defining a completely new protocol here (i.e. /p2p/id/1.1.0
) but at the same time, what the go implementation defines as /p2p/id/1.1.0
carries old legacy stuff around like listenAddrs
and un-specified functionality like delta
.
I am tempted to - instead of specifying go's variation of identify as /p2p/id/1.1.0
- specify a new protocol /p2p/id/2.0.0
that removes old legacy fields and is purely based on signed peer records.
Thoughts?
A new protocol name: /p2p/id/1.1.0.
It doesn't have a new protocol ID. Unfortunately, because we run identify before we know the peer's protocols (at the moment) negotiating between two versions would take a full round-trip and add a round-trip to connection initiation.
Instead, we just added the field to the the protocol. It's backwards compatible, and old versions will simply ignore it.
I think the move to signed peer records in the identify protocol should happen in a non-breaking way at the Protocol Buffer level and not in a "breaking" way at the protocol name level. In other words, I am in favor of the go-libp2p way, adding the signedPeerRecord
to the Protocol Buffer schema instead of changing the protocol name.
I agree that the additional field in the Protocol Buffer schema adds complexity to the protocol, though I would argue that (a) complexity is rather small, especially since we can ultimately deprecate the old listenAddrs
field and (b) the work required to role out a new protocol, while not very large, still outweights the complexity of the additional signedPeerRecord
field.
Given that listenAddrs
is a repeated
field and thus can be left empty once most nodes of a network use signedPeerRecord
I don't think the non-breaking approach (adding the signedPeerRecord
field) has any notable performance impact.
With the above in mind, what do you think @thomaseizinger? I am sorry for (a) this not being properly specified before rolled out in go-libp2p and (b) the extra work you already did in https://github.com/libp2p/specs/pull/350/.
It doesn't have a new protocol ID.
Alright, that completely changes the game then :)
I'll drop #350 then and update the spec to reflect what /ipfs/id/1.1.0
is.
Migration to a new protocol identifier can be done at a later stage (if ever).
I'll drop #350 then and update the spec to reflect what /ipfs/id/1.1.0 is.
Friendly ping. Help here would still be very much appreciated @thomaseizinger :innocent:
I'll drop #350 then and update the spec to reflect what /ipfs/id/1.1.0 is.
Friendly ping. Help here would still be very much appreciated @thomaseizinger :innocent:
Thanks! I did not forget about it, just got a lot on my plate at the moment :)
Safe to say that this is up for grabs if anyone else wants to pick it up 😅
@thomaseizinger
Migration to a new protocol identifier can be done at a later stage (if ever).
Speaking as a new user of libp2p, the remaining references to ipfs
is a bit confusing. Of course, I do not know the techincal difficulties of changing the protocol id to use p2p
instead of ipfs
, but disregarding that, I think using p2p
would add clarity, given that p2p
is used in multiaddr and other protocols.
A new protocol name: /p2p/id/1.1.0.
It doesn't have a new protocol ID. Unfortunately, because we run identify before we know the peer's protocols (at the moment) negotiating between two versions would take a full round-trip and add a round-trip to connection initiation.
Instead, we just added the field to the the protocol. It's backwards compatible, and old versions will simply ignore it.
@Stebalien Are there any other issues apart from this that are blockers for an upgrade to a new ID?
It is my understanding that this is primarily a go-libp2p
specific problem. Over at rust-libp2p
, we don't specialize the identify protocol.
I am not familiar with the go-libp2p
codebase but would it be possible to run two versions of the identify protocol in parallel? This way, there is no extra roundtrip, just more bandwidth being consumed and over time, you could phase out the old one once enough nodes have updated.
go-libp2p added signed peer records (https://github.com/libp2p/go-libp2p/blob/8cf67ba1d06271050c4211eb383189214bb535c1/p2p/protocol/identify/pb/identify.proto#L45) ages ago (with the assumption that the spec change would be made).
However, we never changed the protocol name because that's an unrelated issue.
In terms of how to implement the protocol name change... there isn't actually a round-trip issue as long as we prefer the old name (/ipfs/id/1.0.0
) for now. I.e.:
Eventually, once most of the network has upgraded, we can switch the clients to preferring the new protocol.
Cool that is good to know!
It is OT for this issue but I might open another one soon then where we can discuss moving to a new identifier.
The
/identify
protocol was conceived before signed peer records were added.Is there a plan of integrating signed peer records into
/identify
? I think there are a number of ways:/identify/1.1.0
wherelistenAddrs
is deprecated and a new fieldlistenPeerRecords
is added.(1) could be implemented in a backwards-compatible way where upgraded peers always populate both fields. (2) could also be a good chance to do https://github.com/libp2p/specs/issues/45
Given the simplicity of the identify protocol, it might just be easier to design a completely new one. Nodes can always stay backwards compatible by supporting both protocols simultaneously.