Add signedPeerRecord and discussion of message sizes to identify - #709
Add signedPeerRecord and discussion of message sizes to identify#709achingbrain wants to merge 2 commits into
Conversation
Adds the missing `signedPeerRecord` field and an implementation notes section to discuss max message sizes and how to send/receive multiple identify messages during one transaction.
Co-authored-by: Dennis Trautwein <dennis.trautwein@posteo.de>
| should break it up into smaller chunks. | ||
|
|
||
| For optimum backwards compatibility the first message SHOULD NOT exceed 2KB. | ||
| Subsequent messages SHOULD NOT exceed 4KB. |
There was a problem hiding this comment.
Can signed peer records be subdivided? I suspect they can themselves exceed 4 KB.
There was a problem hiding this comment.
One possible issue is signature size of signed peer records if/when libp2p supports post quantum signatures - 4KB would be too small for ML-DSA-44 and ML-DSA-65, 8KB would be too small for ML-DSA-87
| Variant | Public key (bytes) | Signature (bytes) |
| --- | ---: | ---: | ---: |
| ML-DSA-44 | 1312 | 2420 |
| ML-DSA-65 | 1952 | 3309 |
| ML-DSA-87 | 2592 | 4627 |
| node. | ||
|
|
||
| It normally contains the same addresses as the `listenAddrs` field, but in a | ||
| form that lets us share authenticated addresses with other peers. |
There was a problem hiding this comment.
| form that lets us share authenticated addresses with other peers. | |
| It normally contains the same addresses as the `listenAddrs` field, but in a | |
| format that allows to share authenticated addresses with other peers. |
| Early implementations of the Identify protocol limited the incoming message size | ||
| to 2KB and would reject any messages larger than this. Others have since | ||
| increased this limit to 4KB or 8KB. | ||
|
|
||
| With the addition of signed peer records, Circuit Relay addresses, multiaddrs | ||
| that contain certificate hashes, etc, it is now much more likely that this | ||
| threshold will be exceeded. |
There was a problem hiding this comment.
I think this needs to reason about which is better in which case and why. As it's written rn it just seems like research on current state of implementations
| If a modern implementation's Identify message would exceed this limit, it | ||
| should break it up into smaller chunks. |
There was a problem hiding this comment.
i think the past 2 paragraphs and this one could all be replaced by:
An Identify message that exceeds this limit SHOULD be broken up into smaller chunks [of size XYZ].
| Listen addresses SHOULD be ordered such that addresses that have a higher | ||
| likelihood of being dialed successfully (e.g. public, non-NAT or Circuit Relay) |
There was a problem hiding this comment.
This spec should say which are more likely. This feels a bit too vague
| When multiple messages are received, all fields SHOULD be respected, with later | ||
| updates for fields with a cardinality of one taking priority. |
There was a problem hiding this comment.
| When multiple messages are received, all fields SHOULD be respected, with later | |
| updates for fields with a cardinality of one taking priority. | |
| When multiple messages are received, later updates for fields with a cardinality of one SHOULD take priority. |
| Where repeated fields encountered in subsequent messages, they SHOULD be | ||
| appended to the earlier occurrences of the field and deduplicated as necessary. | ||
|
|
||
| A maximum of 10 Identify messages SHOULD be accepted. |
There was a problem hiding this comment.
Would be good to add a note on why 10 is the chosen number
| Where repeated fields encountered in subsequent messages, they SHOULD be | ||
| appended to the earlier occurrences of the field and deduplicated as necessary. |
There was a problem hiding this comment.
| Where repeated fields encountered in subsequent messages, they SHOULD be | |
| appended to the earlier occurrences of the field and deduplicated as necessary. | |
| Repeated fields encountered in subsequent messages SHOULD be deduplicated and | |
| appended to earlier occurrences of the field. |
| It normally contains the same addresses as the `listenAddrs` field, but in a | ||
| form that lets us share authenticated addresses with other peers. | ||
|
|
||
| When present the fields from a `PeerRecord` MUST be preferred over those in the |
There was a problem hiding this comment.
| When present the fields from a `PeerRecord` MUST be preferred over those in the | |
| When present, the fields from a `PeerRecord` MUST be preferred over those in the |
| should break it up into smaller chunks. | ||
|
|
||
| For optimum backwards compatibility the first message SHOULD NOT exceed 2KB. | ||
| Subsequent messages SHOULD NOT exceed 4KB. |
There was a problem hiding this comment.
If a signed peer record alone exceeds 4kb what should the sender do? it seems that splitting a signed peer record would not work with the merge rules below, because it is a single bytes field, so the later parts would replace the earlier parts instead of joining them.
8 participants
Adds the missing
signedPeerRecordfield and an implementation notes section to discuss max message sizes and how to send/receive multiple identify messages during one transaction.Also updates the language around receiving messages to make it clear that multiple messages may be sent.