-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
Also add ExtractEd25519PublicKey helper function
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 largely LGTM and I think go-libp2p-peer is the right place, but I feel slightly uncomfortable throwing away all errors. There should be at least a "wasn't able to extract pubkey" error returned.
Also needs tests for the happy path and for error cases.
func (id ID) ExtractEd25519PublicKey() ic.PubKey { | ||
// ed25519 pubkey identity format | ||
// <identity mc><length (2 + 32 = 34)><ed25519-pub mc><ed25519 pubkey> | ||
// <0x00 ><0x22 ><0xed01 ><ed25519 pubkey> |
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 am not sure if 0xed01
is the varint encoded multicodec of the ed25519-pub code
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.
Ahh, the check is correct the doc is just off.
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 a Go function that will varint encode a byte slice for me?
The ed25519-pub code 0xed
is 0b11101101
in binary. The last byte needs to have it's MSB set to 0 (according to this) and each byte except the last contains 7 bits of information. So I imagine the MSB (which is 1) gets chopped off from the first byte and added to the next byte. That would yield 0b(1)1101101(0)1000000
. Or is it that we shift everything by one bit, yielding 0b(1)1110110(0)1000000
? I'm so confused by varint!
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.
You can do binary.PutUvarint with the value. It is compliant with our varint spec up until 63 bits of to be encoded value.
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.
Ok. It seems the documentation is correct with 0xed01
(see code here). Could you confirm this? And could you expand on why you think that the length might be off in this comment.
} | ||
|
||
// Check multihash length | ||
if decoded.Length != 2+32 { |
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 this might be off.
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.
Why is this off? I think the varint-encoded ed25519-pub code is 2 bytes and the ed25519 pubkey is 32 bytes.
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.
Sorry I think it should be ok, I accounted for length prefix.
The way I see it is that the default case for the functions is to fail to extract a public key. If we have Furthermore the error in As for the error in |
|
||
// Decode multihash | ||
decoded, err := mh.Decode([]byte(id)) | ||
if err != nil { |
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.
@JustinDrake similar thoughts as @lgierth on the error handling, we shouldnt throw them away, lets log them at the very least. Also, do we need both methods? Or can we just have the generic |
Is the extraction of public keys from peer ids a necessity of the ipfs infrastructure, or is it only used for showing the user the public keys for reporting/logging/user interface? Because not all peer ids allow the public key to be extracted. And in fact the current code base only shows that Ed25519 peer ids allow the public key to be extracted. According to various descriptions of the peer id it appears that in general the public key shouldn't be extractable. Would it be better such that public keys can always be extracted from a peer id (hence you're just using the public key as the peer id)? In terms porting this to Haskell, we would like to know exactly when a conversion of PeerID -> PublicKey is actually meant to be allowed. |
In the general case keys are not extractable, because keys can be larger, and identities are meant to be short. Currently the only extractable case is with Ed25519, when the identity multihash is used, as well as the Ed25519-pub multicodec. See https://github.com/libp2p/go-libp2p-peer/blob/master/peer.go#L76 |
This is a draft function to extract the public key from the identity. It is a helper function for ipfs/kubo#3896 which will be used in
go-libp2p-kad-dht
andgo-ipfs/namesys
. I'll add tests when the IPFS team is happy with the approach.A few questions:
go-libp2p-peer
?UnmarshalEd25519PublicKey
function (see PR here). Is that OK?cc @whyrusleeping, @Kubuxu