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

Jump to bottom

Reframe peer records #279

Closed
wants to merge 2 commits into from
Closed

Reframe peer records #279

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
647c842
spec(reframe): draft for getting peer records
aschmahmann Apr 21, 2022
fac8b2d
spec(reframe): fix typos
aschmahmann Apr 21, 2022
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
45 changes: 43 additions & 2 deletions REFRAME.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ type Request union {
| "FindProvidersRequest" FindProvidersRequest
| "GetIPNSRequest" GetIPNSRequest
| "PutIPNSRequest" PutIPNSRequest
| "GetPeerAddressesRequest" GetPeerAddressesRequest
}
```

Expand All @@ -87,6 +88,7 @@ type Response union {
| "FindProvidersResponse" FindProvidersResponse
| "GetIPNSResponse" GetIPNSResponse
| "PutIPNSResponse" PutIPNSResponse
| "GetPeerAddressesResponse" GetPeerAddressesResponse
| "Error" Error
}
```
Expand Down Expand Up @@ -171,8 +173,8 @@ Note: While the Key is a CID it is highly recommended that server implementation
}

type TransferProtocol union {
| Bitswap "2304" # Table entry interpretted as decimal string https://github.com/multiformats/multicodec/blob/f5dd49f35b26b447aa380e9a491f195fd49d912c/table.csv#L133
| GraphSync-FILv1 "2320" # Table entry interpretted as decimal string https://github.com/multiformats/multicodec/blob/f5dd49f35b26b447aa380e9a491f195fd49d912c/table.csv#L134
| Bitswap "2304" # Table entry interpreted as decimal string https://github.com/multiformats/multicodec/blob/f5dd49f35b26b447aa380e9a491f195fd49d912c/table.csv#L133
| GraphSync-FILv1 "2320" # Table entry interpreted as decimal string https://github.com/multiformats/multicodec/blob/f5dd49f35b26b447aa380e9a491f195fd49d912c/table.csv#L134
| Any default
} representation keyed

Expand Down Expand Up @@ -280,6 +282,45 @@ Response:
{"PutIPNSResponse : {}"}
```

#### GetPeerAddresses

A message for getting the addresses of a libp2p peer

```ipldsch
type GetPeerAddressesRequest struct {
ID Bytes # libp2p PeerID
RecordTypes [String]
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having RecordTypes doesn't seem necessary as the responder could just send back all record types it knows about. Is there value in being able to ask to send back less data here? The same could be argued in other places (e.g. not sending back TransferProtocols we don't support anyway).

I'm planning to remove this, but wanted to leave this as a place for comment if there was demand for it to stay

Copy link
Contributor

Choose a reason for hiding this comment

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

if we do have it, we should have some recommendation of how we identify types - what are these strings supposed to be?

It doesn't seem overly harmful to have, but it does add another point of complexity in implementation.

Copy link
Contributor

@petar petar Apr 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

+1: if we are to have it, it should be a union of sorts that becomes the formal spec of known record types.

if we are to have it, there must also be a case for "return all types", otherwise we preclude a dumb middlebox from pre-fetching data.

Copy link
Contributor

Choose a reason for hiding this comment

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

on the whole, this does seem as an unnecessary micro-optimization (at complexity cost), because the number of records a peer can ever have is limited by the number of types that exist, which is a small number. this is not something that can grow arbitrarily, so ... i would vote no here. the rule of thumb being: can the number of returned results grow in a runtime-dependent manner (e.g. # of providers for a cid depends on runtime conditions, vs number of address records is always capped by the number of types of records which is a compile-time constant)


type Multiaddresses [Bytes] # Each element in the list is the binary representation of a complete multiaddr without a peerID suffix
type Libp2pSignedPeerRecord Bytes # https://github.com/libp2p/specs/blob/8c967a22bfbaff1ae41072b358fdba7c5883b6a4/RFC/0003-routing-records.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.

Including raw bytes like this doesn't feel great, although in order to use this peers will likely need to understand this format to send addresses in Identify anyhow so not so bad.

Are there better ideas here given that we can't just do something like the below due to the use of (non-canonically encoded) protobufs in signed peer records?

type PeerRecord struct {
      Multiaddresses [Bytes]
      SeqNo optional Int
      Signature optional Bytes
}

Copy link
Contributor

@petar petar Apr 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is fine. One variation, purely for convenience, would be:

type Libp2pSignedPeerRecord struct {
  Multiaddresses [Bytes] # copy of the multiaddresses from the record, for the benefit of systems that don't know/want to check signatures
  Libp2pSignedRecord Bytes
}


type PeerRecordType union {
| Multiaddresses "multiaddrs"
| Libp2pSignedPeerRecord "769" // the libp2p signed peer record entry interpreted as decimal https://github.com/multiformats/multicodec/blob/f5dd49f35b26b447aa380e9a491f195fd49d912c/table.csv#L129
Comment on lines +299 to +300
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't sure which identifiers to use here. In particular it feels weird to be mixing text and codes like this without any sort of standard for how we do this.

@Stebalien has occasionally proposed utilizing / (since it's the 0x90 codec) as a way to bridge text + codes although I'm not sure if that applies here.

Copy link
Contributor

Choose a reason for hiding this comment

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

if we have codes for some of them already, can we make a code for the reframe-multiaddrs alternative to libp2p signed peer records and standardize on codes?

| Any default
} representation keyed

type GetPeerAddressesResponse struct {
Records [PeerRecordType]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it make sense to return [PeerRecordType] here rather than just a single PeerRecordType since we're allowed to send back a stream of GetPeerAddressesResponse? Probably doesn't hurt, but we might want to build some conventions here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd keep the array. A receiver might want to forward/use results as soon as the first one arrives. They have no good way of knowing that more are coming back to back.

}
```

##### DAG-JSON Examples

Request: // TODO
```
{"GetPeerAddressesRequest" : {
"ID" : {"/":{"bytes":"AXIUBPnagss"}},
"Record" : {"/":{"bytes":"AXIUBPnagss"}}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: there presumably isn't a record in the request, this would be in the response

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I didn't update any of the JSON examples yet (marked with TODO)

}}
```

Response: // TODO
```
{"GetPeerAddressesResponse : {}"}
```

# Method Upgrade Paths

It is acknowledged that the initial methods and mechanisms of this protocol will likely need to change over time and that we should prepare for how to do so without the need to wholesale replace this protocol with an alternative.
Expand Down