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.

Sign up for GitHub

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
Open

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

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5b2c7df
feat: add signed peer record to identify spec
2color Sep 5, 2024
f16a3f4
fix: address feedback
2color Sep 6, 2024
0b0d5b9
Update identify/README.md
2color Sep 11, 2024
9810e69
Update identify/README.md
2color Sep 11, 2024
ba3d797
chore: update revision
2color Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion identify/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ and spec status.
- [listenAddrs](#listenaddrs)
- [observedAddr](#observedaddr)
- [protocols](#protocols)

- [signedPeerRecord](#signedpeerrecord)

## Overview

Expand Down Expand Up @@ -81,6 +81,7 @@ message Identify {
repeated bytes listenAddrs = 2;
optional bytes observedAddr = 4;
repeated string protocols = 3;
optional bytes signedPeerRecord = 8;
achingbrain marked this conversation as resolved.
Show resolved Hide resolved
}
```

Expand Down Expand Up @@ -133,3 +134,14 @@ clients only support initiating requests while some servers (only) support
responding to requests. To prevent clients from initiating requests to other
clients, which given them being clients they fail to respond, clients should not
advertise `foo` in their `protocols` list.

### 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
sukunrt marked this conversation as resolved.
Show resolved Hide resolved


[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 •
edited
Loading

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 •
edited
Loading

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.