identity::Keypair ergonomics improvements

[divagant-martian]

Motivation

After #3350 a couple of things that were easily achievable with a match statement are convoluted/potentially less performant.

Simple examples:

Accessing the kind of key by value when supporting more than one kind requires clone

Getting the value of the key while supporting more than one key type

match key {
    Secp256k1(pk) => {         
        do_smth_with_secp256k1_value(pk)
    }                                     
    Ed25519(pk) => {           
        do_smth_with_ed25519_value(pk)  
    }
}

Now this requires a clone since into_secp256k1 consumes the value

if let Some(pk) = key.clone().into_secp256k1() {
        do_smth_with_secp256k1_value(pk)
} else if let Some(pk) = key.into_ed25519() {
        do_smth_with_ed25519_value(pk)  
}

Accessing the kind of key by reference

match &key {
    Secp256k1(pk /* A reference */) => {         
        do_smth_with_secp256k1_ref(pk)
    }                                     
    Ed25519(pk /* A reference */ ) => {           
        do_smth_with_ed25519_ref(pk)  
    }
}

Now this requires a clone since into_secp256k1 consumes the value

if let Some(pk) = key.clone().into_secp256k1() {
        do_smth_with_secp256k1_ref(&pk)
} else if let Some(pk) = key.into_ed25519() {
        do_smth_with_ed25519_ref(&pk)  
}

Supporting a new key kind

Before: Enable the feature and compiler would tell of a missing match branch.
Now: Enable feature and dig in the code where the into_keykind() are, to add a new one

Proposed sol

Assuming the code base is going ahead with this deprecation,

for 1: Implement TryFrom/TryInto for the necessary types. Alternative, add the matching try_into_secp256k1 and others. In these, the error type should give back the original key to be used.

for 2: Implement AsRef for the necessary types. Alternative, add the matching as_secp245k1() and others

for 3. Can't see a good solution right now

Are you planning to do it yourself in a pull request?

Maybe.

[thomaseizinger]

What is do_smth_with_secp256k1_value? Keypair supports signing and verifying of signatures, regardless of the key type. What else do you need to do with it?

Generally, the path forward from here is to add whatever do_something function you have to Keypair and do it internally for all three keys.

[drHuangMHT]

Working on #3681 to solve this. Discussion welcome.

[divagant-martian]

What is do_smth_with_secp256k1_value? Keypair supports signing and verifying of signatures, regardless of the key type. What else do you need to do with it?

Generally, the path forward from here is to add whatever do_something function you have to Keypair and do it internally for all three keys.

in LH I don't think we do anything by value, but I don't think it's an unrealistic scenario to have

---

Working on #3681 to solve this. Discussion welcome.

Thanks! looking forward to this

[thomaseizinger]

What is do_smth_with_secp256k1_value? Keypair supports signing and verifying of signatures, regardless of the key type. What else do you need to do with it?

Generally, the path forward from here is to add whatever do_something function you have to Keypair and do it internally for all three keys.

in LH I don't think we do anything by value, but I don't think it's an unrealistic scenario to have

Okay thanks! Please open a new issue once we have something actionable.

[divagant-martian]

What's the point of closing this @thomaseizinger. I raised a couple of problems. Access by value is just one of them, access by reference is already happening in our repo and there is someone working on improving this

[divagant-martian]

It would be more productive to address the proposed solutions and weight in which sounds more reasonable in each case

[thomaseizinger]

I closed it because you said you don't have a usecase?

What are you accessing the individual keypairs for that you can't do via the composed one?

[thomaseizinger]

I closed it because you said you don't have a usecase?

What are you accessing the individual keypairs for that you can't do via the composed one?

Just re-read your comment and I missed the "by value".

My overall point still stands though: You should be using the Keypair and its functions and not the individual variants.

Can you point me to code that is currently accessing the individual variants?

[divagant-martian]

Yeah I thought we did for accessing individual keys by value, but we don't. We do need to access individual keys by reference. This is since Ethereum node records have defined obtaining a peer id for two of the keys.

Before the deprecation this would be a simple match over a reference. This now requires a clone and it's an operation that occurs at least every time a peer connects. This is less performant now beside the simple lack of convenience we had before.

Happy to implement the required methods. As I said in the issue, we can either do an AsRef impl or add the as_keyname following the same convention of the new into_keyname methods. Just need you people to pick what you prefer

[drHuangMHT]

Some keys can't be accessed by reference because of internal details, like RSA keys and Ed25519 keys. And currently top-level Keypair is still a enum, making it tricky to implement AsRef. Further decision needed.

[thomaseizinger]

We do need to access individual keys by reference. This is since Ethereum node records have defined obtaining a peer id for two of the keys.

Are you talking about this code here?

https://github.com/sigp/lighthouse/blob/a53830fd60a119bf3f659b253360af8027128e83/beacon_node/lighthouse_network/src/discovery/enr_ext.rs#L220-L238

Funnily enough, it is code like this why I want to make the Keypair opaque. Your code is missing the following branch:

#[cfg(all(feature = "rsa", not(target_arch = "wasm32")))]
Keypair::Rsa(_) => Err("rsa keypairs not supported"),

This is very hard to get right and I don't expect people to. Which is why I want to hide hit :)

In this case, the lighthouse-network crate isn't published anywhere right? However, if it were and somebody makes a project that a also pulls in libp2p-identity and activates the rsa feature, your code will no longer compile because your suddenly not matching on all variants.

In regards to the actual usecase, I want to say two things:

1. I am happy to discuss code changes in the name of ergonomics.
2. Any performance claim however will need to come with a benchmark that demonstrates that the new code is actually slower. Neither secp256k1 nor ed25519 incur a heap-allocation when being cloned. After feat(identity): implement Copy for identity::secp256k1::KeyPair #3707, secp256k1 keypairs are even a bit-wise copy.

To make things more ergonomic, I'd suggest that we add our own KeyType enum to libp2p-identity:

enum KeyType {
	Ed25519,
	Rsa,
	Secp256k1,
	Ecdsa
}

That allows you to correctly call the corresponding into_ function. Can you send a PR?

---

Unrelated feedback:

https://github.com/sigp/lighthouse/blob/a53830fd60a119bf3f659b253360af8027128e83/beacon_node/lighthouse_network/src/discovery/enr_ext.rs#L231

Instead of taking the first 32-bytes of the "encoding", you are probably better of calling secret() and then utilizing the AsRef[u8] impl to access the bytes of the secret key.

[thomaseizinger]

Once #3775 lands, we have a KeyType. We'd only need to expose it from KeyPair then to actually resolve this issue.

[thomaseizinger]

@divagant-martian Would you be willing to contribute a PR that exposes a Keytype for a KeyPair?

[divagant-martian]

Hi @thomaseizinger due to life changes since the past month and at least for one more month I won't have time to contribute to libp2p, sorry

[thomaseizinger]

Hi @thomaseizinger due to life changes since the past month and at least for one more month I won't have time to contribute to libp2p, sorry

No worries at all, thanks for letting me know and hopefully all is right :)

[folex]

@divagant-martian Would you be willing to contribute a PR that exposes a Keytype for a KeyPair?

@thomaseizinger, hello :)

Could you please elaborate on how do you envision the exposition?

impl Keypair {
  fn key_type(&self) -> KeyType {
    match self.inner {
       Ed25519(_) => KeyType::Ed25519,
       ...
    }
  }
}

Something like that?

[thomaseizinger]

Yep that looks good!

[divagant-martian]

a patch with this would be indeed welcomed!

[thomaseizinger]

Tagged and published.