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

fix: add missing signed peer record to identify spec #630

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

2color
Copy link
Contributor

@2color 2color commented Sep 5, 2024

This adds mention of the signed peer records in the identify protocol.

@2color 2color changed the title feat: add signed peer record to identify spec fix: add missing signed peer record to identify spec Sep 6, 2024
identify/README.md Outdated Show resolved Hide resolved
identify/README.md Outdated Show resolved Hide resolved
Comment on lines 138 to 147
### 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
Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

@sukunrt sukunrt Sep 9, 2024

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.

Copy link
Contributor Author

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?

Copy link
Member

@sukunrt sukunrt Sep 10, 2024

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.

Copy link
Member

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.

identify/README.md Outdated Show resolved Hide resolved
identify/README.md Outdated Show resolved Hide resolved
2color and others added 2 commits September 11, 2024 14:52
Co-authored-by: Alex Potsides <[email protected]>
use r3 to account for #502
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Triage
Development

Successfully merging this pull request may close these issues.

3 participants