Skip to content
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.

Sign up for GitHub

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

Jump to bottom

keys: Should support ed25519 #1861

Closed
ValarDragon opened this issue Jul 28, 2018 · 8 comments · May be fixed by #23283
Closed

keys: Should support ed25519 #1861

ValarDragon opened this issue Jul 28, 2018 · 8 comments · May be fixed by #23283
Labels
C:Keys Keybase, KMS and HSMs S:proposed

Comments

@ValarDragon
Copy link
Contributor

ValarDragon commented Jul 28, 2018 •
edited
Loading

Currently we don't support ed25519

gaiacli keys add --type ed25519 potato
override the existing name potato [y/n]:y
Enter a passphrase for your key:
Repeat the passphrase:
ERROR: unsupported signing algo: only secp256k1 is supported

I'm pretty sure we want to support ed25519 at launch. (This error was a pretty big surprise to me, thanks @amrali for asking questions that led to debugging this!) If desired, I can argue for supporting it at launch here, if for some reason ed25519 isn't supposed to be here at launch. (this can easily be added back post launch)

My guess is that ed25519 support got removed from gaiacli when we added proper HD derivation support. There is currently an ed25519-bip32 spec by dmitry written here (https://cardanolaunch.com/assets/Ed25519_BIP.pdf - not sure if this is the latest version, I don't have a quick way to check), for the case without ristretto. I don't think we should support this spec at launch due to it not being standardized. (Plus it would be awesome if we could just jump to using ristretto soon after launch)

However all we really need is to support a mnemonic to remember. This is quite easy to do (conceptually, implementation may not be). We can just use an error correction code of suitable strength on the private key, and then convert those bytes into words in the same way we do for secp256k1. We should probably prefix the bytes after the error correction code but before the wordlist with a 0x00 byte, just so its easy to identify in the future.

(Note that we should only be using the first 32 bytes of the private key, since the latter 32 bytes are the public key, see tendermint/ed25519 godoc for more details)
@ValarDragon ValarDragon added C:Keys Keybase, KMS and HSMs T:Bug S:proposed labels Jul 28, 2018
@ValarDragon
Copy link
Contributor Author

ValarDragon commented Jul 28, 2018 •
edited
Loading

If we add back ed25519 support for users, we should also add an option to gaiad init to make the account pubkey ed25519. (The validator privkey is still ed25519, we just don't display that to users, perhaps we should?)

@amrali
Copy link

amrali commented Jul 28, 2018

If we add back ed25519 support for users, we should also add an option to gaiad init to make the account pubkey ed25519. (The validator privkey is still ed25519, we just don't display that to users, perhaps we should?)

I looked into the code base to figure out what this mnemonic seed is for (the one from gaiad init), it turned out it is for the Secp256k1 account key. So there's at least a UX issue here where it is unclear what the output information is for.

BIP-39 implements its own checksum, it's not clear why the error correction code (for the sake of consistency with how it is done for Secp256k1 private key?)

I think it be better to have different mnemonic seeds for each of the validator and the owner keys.

@ValarDragon
Copy link
Contributor Author

ValarDragon commented Jul 28, 2018 •
edited
Loading

BIP-39 implements its own checksum, it's not clear why the error correction code (for the sake of consistency with how it is done for Secp256k1 private key?)

Oh cool. I didn't realize bip-39 was the thing that handled mnemonics + error correction codes. I had only read through bip-32 before. We should totally just use bip-39.

So there's at least a UX issue here where it is unclear what the output information is for.

Agreed. We should definitely have a better message here. I also agree that we should provide different mnemonics for both the validator and owner key.

@cwgoes
Copy link
Contributor

cwgoes commented Jul 29, 2018

cc @liamsi

I think we could add in ed25519 support (without HD derivation) for user keys pretty easily, although I guess it would require adding extra functions to the keybase API.

@zmanian
Copy link
Member

zmanian commented Aug 5, 2018

Disagree on Prelaunch.

All fundraiser keys are secp keys. No one can even really use a new keypair until transfers are enabled.

@ValarDragon
Copy link
Contributor Author

You're right. Removed the prelaunch tags.

@liamsi
Copy link
Contributor

liamsi commented Aug 5, 2018

I've somehow missed this discussion. I agree with @zmanian on this!

I think we could add in ed25519 support (without HD derivation) for user keys pretty easily, although I guess it would require adding extra functions to the keybase API.

That doesn't sound right to me. We should either add ed25519 with some form of HD derivation, or, we should not have the interfaces changed. Maybe, we could add support for ed25519 without changing the keybase APi though.

@ValarDragon
Copy link
Contributor Author

That sounds good to me! This is definitely something that will be easier to support later. (Doesn't even involve changing the version) I think I'll just close this, and we can look into ed25519 again post launch / after hd derivation is suitably standardized.

@cwgoes cwgoes mentioned this issue Oct 5, 2018
Closed
23 tasks
@Unique-Divine Unique-Divine mentioned this issue May 5, 2023
Closed
@zivkovicmilos zivkovicmilos mentioned this issue Jan 9, 2025
Open
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C:Keys Keybase, KMS and HSMs S:proposed
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: add support for ed25519 tx signature verification zivkovicmilos/cosmos-sdk
5 participants
@zmanian @amrali @ValarDragon @liamsi @cwgoes