Signed Peer Records #2081
Signed Peer Records #2081
Conversation
|
@thomaseizinger let me know once this is ready for a review. Pinging @blacktemplar. Maybe the two of you can collaborate on this effort. |
Can you give it an initial pass that focusses on public APIs and overall modularisation / code layout? The internals are dirty atm but that is easy to clean up once we settle on the APIs. We are also currently dog-fooding this in our implementation of the rendezvous protocol but it would still be good to get your initial view in. For example, are you happy with those things in -core or would you want another crate? |
|
Just chiming in on @blacktemplar's behalf. The original version of this was written when the specs were not solidified and we potentially wanted to include them into the gossipsub protocol. We're not using them at the moment, and the old version in #1803 is likely obsolete. @blacktemplar is no longer working on this, but feel free to ping me if you need any input on this or discussion about the previous PR. |
There was a problem hiding this comment.
We are also currently dog-fooding this in our implementation of the rendezvous protocol but it would still be good to get your initial view in.
Perfect. Looking forward to seeing this in action.
Thank you for tackling this @thomaseizinger.
For example, are you happy with those things in -core or would you want another crate?
I expect and hope for the concept of peer records to be widely adopted across the libp2p space across the various protocols. With that in mind, including it in libp2p-core makes sense to me.
core/src/peer_record.rs
Outdated
| .unwrap() // TODO: Error handling | ||
| } | ||
|
|
||
| pub fn authenticate(self, key: Keypair) -> AuthenticatedPeerRecord { |
There was a problem hiding this comment.
I find the detour from PeerRecord::authenticate over to AuthenticatedPeerRecord::from_record back to PeerRecord::wrap_in_envelope a bit confusing. Could this be simplified? Are all 3 methods needed? Why not offer an From<AuthenticatedPeerRecord> for SignedEnvelope and TryFrom<SignedEnvelope> for AuthenticatedPeerRecord?
There was a problem hiding this comment.
Yes, agreed that the indirections are not good. I wasn't quite sure what the most natural and expressive way of constructing one of these would be.
Will see if we can make this work using From/TryFrom for the internals. I'd like to keep domain-specific functions for the actual public API because I consider them more expressive.
There was a problem hiding this comment.
I removed PeerRecord::authenticate.
I think PeerRecord::wrap_in_envelope should stay as the information about the domain-separator should be tied to the PeerRecord because a SignedEnvelope is more abstract than a PeerRecord.
There was a problem hiding this comment.
What might be debatable is whether or not we even need the concept of a normal PeerRecord. There is probably no point in having an "unauthenticated" peer record.
core/src/peer_record.rs
Outdated
| } | ||
|
|
||
| // TODO: docs | ||
| pub fn warp_in_envelope(self, key: Keypair) -> SignedEnvelope { |
There was a problem hiding this comment.
As a first thought I would prefer passing the data to some component owning the keypair instead of passing the keypair to the data. The former would reduce the exposure of the keypair minimizing the possibilities for mistakes / attacks.
That said, the notion of peer records is still in an early phase, thus any abstractions that we come up with today are likely premature.
There was a problem hiding this comment.
Interesting idea but I don't quite see how it would work. Your identity keypair is constructed as the very first thing and usually only used during the construction of Network.
The way I see this being used is that the user just clones the KeyPair at that stage and also passes it to the Behaviour (like Rendezvous) for example. Such a behaviour can then further use it to construct SignedEnvelopes or other things.
|
@mxinden I incorporated some more learnings and feedback. Let me know what you think! |
There was a problem hiding this comment.
Looks good from a high level. Let me know once this is ready for a proper review.
Also, if you don't mind, I would suggest holding off merging here until we see the code in action, e.g. within the Rendezvous pull request.
Merge PeerRecord into AuthenticatedPeerRecord for simplicity
👍
Sure, that makes total sense. |
I prefer separate PRs as they are "functionally independent", but merging them back-to-back. |
thomaseizinger
force-pushed
the
signed-peer-records
branch
from
f055447
to
56c92c2
Compare
|
Included in #2107. |
This is a very rough draft of implementing RFC0002 (signed envelopes) and RFC0003 (peer records).
It uses a quite different API from #1803 which is why I am opening this in such an early stage to get feedback on which one is more likely to be accepted.
We are currently working on an implementation of the rendezvous protocol (#297) which requires signed peer records.