-
Notifications
You must be signed in to change notification settings - Fork 275
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
fix: add missing signed peer record to identify spec #630
base: master
Are you sure you want to change the base?
Conversation
identify/README.md
Outdated
### signedPeerRecord | ||
|
||
This is a serialized [SignedEnvelope][envelope-rfc] containing a [PeerRecord][peer-record-rfc], | ||
signed by the sending node. It contains the same addresses as the `listenAddrs` field, but in a form that lets us share authenticated addrs with other peers. | ||
|
||
This field was introduced in a backwards compatible manner (meaning that it is sent along with the `listenAddrs` field), therefore, it is optional and may be omitted by older implementations. If the `signedPeerRecord` is present, implementations MUST use the data contained within it and ignore duplicated fields present in the main identify message | ||
|
||
|
||
[envelope-rfc]: ../RFC/0002-signed-envelopes.md#wire-format | ||
[peer-record-rfc]: ../RFC/0003-routing-records.md#address-record-format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to make this a separate spec peer-record
spec. There we should add peer-record bits from https://github.com/libp2p/specs/blob/master/RFC/0003-routing-records.md and signed peer record bits from https://github.com/libp2p/specs/blob/master/RFC/0002-signed-envelopes.md
We can then reference it from places like: https://github.com/libp2p/specs/blob/master/pubsub/gossipsub/gossipsub-v1.1.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand. What do you think should be the scope of the separate spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
peer record and signed peer record.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything missing in the two RFCs? Or do they just need to be ratified into a spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the second one. I'm not sure why we ever did RFCs. @MarcoPolo thoughts?
While I dislike the fact that those 3 documents are RFCs and everything else in the specs is not, the real problem is that those documents are very dated:
Consider: https://github.com/libp2p/specs/blob/master/RFC/0003-routing-records.md#address-record-format
A peer SHOULD only include addresses that it believes are routable via the public internet, ideally having confirmed that this is the case via some external mechanism such as a successful AutoNAT dial-back.
I see no reason why we should do this. And go-libp2p doesn't. Depends on what you're using them for.
There's some information that we don't need, like this discussion on Routing State
To produce a "self-certified" address, a peer will construct a RoutingState containing their listen addresses and serialize it to a byte array using a protobuf encoder. The serialized records will then be wrapped in a signed envelope, which is signed with the libp2p peer's private host key. The corresponding public key MUST be included in the envelope's public_key field.
What is a RoutingState
?
or the go-libp2p API suggestion elsewhere in RFC-0003.
Most importantly,
it doesn't mention the that the domain string is libp2p-peer-record
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be very useful to have a canonical reference that defines a peer record. The RFCs read like a work-in-progress which isn't helpful for implementers.
Co-authored-by: Alex Potsides <[email protected]>
use r3 to account for #502
This adds mention of the signed peer records in the identify protocol.