Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
docs: add peer id spec #100
docs: add peer id spec #100
Changes from 2 commits
714a6c7
565767a
902fbfe
95c2354
d8459bc
e2dfbe2
6c318c9
878f2fa
eda2295
242afbe
52057ac
3302991
f277f41
046c7e8
9bfb370
a7de2f6
1237100
870b71a
d14a44d
10043ec
6c4a587
2ec0867
5173834
ed01eb1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 any situation where we want to transmit the
PrivateKey
? That seems... dangerous. If not, we don't need to specify thePrivateKey
here at all.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.
Yeah, storage of private key is implementation specific, so no need to cover them in this doc I think.
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.
Unfortunately, users do need to be able to take their private keys with them (especially because we use these for things like IPNS).
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's true that removing the private key format from this doc leaves a gap. We still need to specify somewhere how we handle them.
We could bring back the private key references and add a call-out at the top of the doc that they're not related to peer-id calculation and are shown for reference.
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.
Really, we should probably rename this doc to the "libp2p key spec" and make peer ID calculation a part of that.
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.
👍 for that
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.
1=0,2=bytes
and2=bytes,1=0
are two equally valid encodings for an RSA key. Indeed, so is1=2,2=bytes,1=0
given that the last value overrides the first, per the protobuf spec. Protobuf is not deterministic in its encoding - this would be a problem in general.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.
Hmm, that's true. From the linked doc:
Are we doing anything to enforce deterministic field ordering? If not, maybe we should be...
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.
Some notes about the lack of deterministic serialization for protobuf: https://gist.github.com/kchristidis/39c8b310fd9da43d515c4394c3cd9510
This is a thorny issue... a libp2p implementor could do everything according to the spec and still end up with peer ids that other peers wouldn't validate, if their protobuf implementation happened to order the keys differently.
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 helps to think of a protobuf message as a hash map, essentially - semantically it's a very similar situation - the last thing you put in remains, and the order is unspecified.
In fact, that's cheap and easy way to implement it - I'd expect dynamic languages with strong dictionary implementations to do it this way.
Locking down field order solves the issue, but I'm not sure I'd call it protobuf any more at that point - perhaps protobuf-compatible.
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.
Yeah, I'm not crazy about requiring semantics on top of protobuf - at that point I'd rather just specify a different serialization that is deterministic. Then you have a backwards compatibility problem though...
@marten-seemann do you have any concerns about the non-deterministic encoding for this public key structure? Asking because it gets embedded in the certificate extension for the TLS 1.3 spec you're working on.
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.
Specifically, it says:
(i.e., implementation defined)
Annoyingly, they actually changed this. It used to say:
Which is why we relied on this. All fully-featured and correct implementations ensured their field orders matched.
We currently require proto2 (these fields are actually marked as required which doesn't exist in proto3) but yeah, implementers may ignore this.
IMO, that's a design goal when editing. Any edits will change the peer ID anyways.
My worry with saying "this is not protobuf but you can use a protobuf decoder" is that people will use protobuf encoders anyways because they'll usually "just work". Then, when the underlying protobuf implementation changes, bad things will happen.
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.
@Stebalien
We can include test vectors for that. It's fair for implementors to use protobuf encoders, and I expect that to be the dominant implementation approach. However, if between version X and Y a specific encoder changed in a way that it broke our requirement, that's when the implementor would have to craft a manual encoding. It's a fair trade-off. We can provide guidance in this spec (in the form of "implementor's notes").
Woah, thanks for tracing that back 😓
IIUC, you'd like nodes to tolerate unrecognised fields and use the serialised form verbatim, as transmitted by the other party, when calculating the peer ID?
Isn't this dangerous? This creates a surface for polymorphic peer IDs, where node
A
with pubkeyP
could produce infinite peer ID (or craft specific ones) for itself – and hence break protocols that rely on peer IDs – merely by setting random fields in the protobuf.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.
This should be just fine.
data
part of the key is completely deterministic.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.
Could you elaborate?
That feels wrong. Identity should be derived from cryptographic material (a pubkey), not from random metadata.
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 exactly what @Stebalien is referring to about the data field, but the spec describes one case where the same public key could lead to different data fields (and thus peer ids).
Since there are two supported methods for encoding Ed25519 keys, you could end up with different peer ids for the same key depending on which you choose.
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.
We should say something about how these are commonly represented as strings: base58btc encoding raw, without using multibase.
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 added a bit about base58btc, but didn't mention multibase, since we hadn't defined it yet in the doc. Should I bring it up? I think if people are likely to expect Peer Ids to use multibase we should clarify.
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.
That seems like an implementation decision. Remove this sentence?
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.
Same here.
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.
This seems like an implementation decision, so we probably don't need to specify it.
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 entirely. We do want users to be able to port keys from one implementation to another.
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.
And here.
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.
And here.
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.
And here.