Bump ed25519-dalek to 2.1.1, ed25519-dalek-bip32 to 0.3.0 #26

kevinheavey · 2025-02-11T17:09:12Z

Summary of Changes

Bump the two crate versions as in title and fix what breaks
This involves one breaking change: Keypair::generate now relies on traits from a newer version of the rand crate - it no longer works with rand 0.7.

seanyoung · 2025-02-13T14:25:36Z

How does this affect performance (the bench should tell us that, I think).

kevinheavey · 2025-02-15T16:47:31Z

Looks like the new version is faster. Found this in the CI bench output https://github.com/anza-xyz/solana-sdk/actions/runs/13304474459/job/37152326064?pr=26:

test bench_ed25519_len_032 ... bench:      48,795.52 ns/iter (+/- 238.09)
test bench_ed25519_len_128 ... bench:      49,085.67 ns/iter (+/- 368.34)
test bench_ed25519_len_32k ... bench:     110,201.12 ns/iter (+/- 1,078.12)
test bench_ed25519_len_max ... bench:     171,307.76 ns/iter (+/- 1,212.43)

Meanwhile in another unrelated PR: https://github.com/anza-xyz/solana-sdk/actions/runs/13344518453/job/37273344074?pr=40

test bench_ed25519_len_032 ... bench:      62,532.14 ns/iter (+/- 1,077.31)
test bench_ed25519_len_128 ... bench:      62,774.78 ns/iter (+/- 597.11)
test bench_ed25519_len_32k ... bench:     123,763.83 ns/iter (+/- 1,565.07)
test bench_ed25519_len_max ... bench:     184,640.96 ns/iter (+/- 1,562.99)

juchiast · 2025-03-25T09:38:53Z

keypair/src/lib.rs

@@ -76,12 +71,12 @@ impl Keypair {

    /// Returns this `Keypair` as a base58-encoded string
    pub fn to_base58_string(&self) -> String {
-        bs58::encode(&self.0.to_bytes()).into_string()
+        bs58::encode(&self.to_bytes()).into_string()
    }

    /// Gets this `Keypair`'s SecretKey
    pub fn secret(&self) -> &ed25519_dalek::SecretKey {


I think this should just return [u8; 32] directly, skip the type alias of ed25519-dalek.

This function has been deprecated and will be removed anyway

juchiast · 2025-03-25T09:42:38Z

keypair/src/lib.rs

@@ -271,11 +262,9 @@ pub fn keypair_from_seed(seed: &[u8]) -> Result<Keypair, Box<dyn error::Error>>
    if seed.len() < ed25519_dalek::SECRET_KEY_LENGTH {
        return Err("Seed is too short".into());
    }
-    let secret = ed25519_dalek::SecretKey::from_bytes(&seed[..ed25519_dalek::SECRET_KEY_LENGTH])
+    let secret_key = ed25519_dalek::SecretKey::try_from(&seed[..ed25519_dalek::SECRET_KEY_LENGTH])


this function never fail, should call unwrap here

joncinque · 2025-07-09T20:57:10Z

We're ready to integrate this now! Do you mind rebasing your branch? Otherwise, I can take care of it. Just let me know

joncinque

Looks great to me! Just some small things

@samkim-crypto can you also take a quick look and make sure it's all ok?

joncinque · 2025-07-11T21:16:37Z

keypair/src/lib.rs

@@ -78,19 +75,19 @@ impl Keypair {
    /// Returns this `Keypair` as a base58-encoded string
    pub fn to_base58_string(&self) -> String {
        let mut out = [0u8; five8::BASE58_ENCODED_64_MAX_LEN];
-        let len = five8::encode_64(&self.0.to_bytes(), &mut out);
+        let len = five8::encode_64(&self.to_bytes(), &mut out);


nit: to avoid the copy, this should be able to use as_bytes()

Suggested change

let len = five8::encode_64(&self.to_bytes(), &mut out);

let len = five8::encode_64(self.as_bytes(), &mut out);

self.0.as_bytes() would give just the secret 32 bytes, not the full 64 bytes

Ah you're right, sorry, I was looking at methods on SigningKey

joncinque · 2025-07-11T21:17:15Z

keypair/src/lib.rs

@@ -76,12 +71,12 @@ impl Keypair {

    /// Returns this `Keypair` as a base58-encoded string
    pub fn to_base58_string(&self) -> String {
-        bs58::encode(&self.0.to_bytes()).into_string()
+        bs58::encode(&self.to_bytes()).into_string()
    }

    /// Gets this `Keypair`'s SecretKey
    pub fn secret(&self) -> &ed25519_dalek::SecretKey {


This function has been deprecated and will be removed anyway

joncinque · 2025-07-11T21:19:10Z

Cargo.toml

@@ -158,8 +158,8 @@ curve25519-dalek = { version = "4.1.3", features = ["digest", "rand_core"] }
 dashmap = { version = "5.5.3", features = ["serde"] }
 derivation-path = { version = "0.2.0", default-features = false }
 digest = "0.10.7"
-ed25519-dalek = "=1.0.1"
-ed25519-dalek-bip32 = "0.2.0"
+ed25519-dalek = "=2.1.1"


note to self for later: 2.2.0 just got released, so we can bump to that after this lands

joncinque · 2025-07-11T21:20:49Z

keypair/src/lib.rs

-                .map_err(SignatureError::from_source)?;
-        let expected_public = ed25519_dalek::PublicKey::from(&secret);
+        };
+        // compiler elides the second len check so try_into().unwrap() is as fast as unsafe code


cool! Just for my own knowledge, how did you learn / measure that?

compared safe and unsafe versions with godbolt, and saw they were the same: https://godbolt.org/z/qf4cnaWax

joncinque · 2025-07-11T21:23:49Z

keypair/src/lib.rs

+        let keypair_bytes: [u8; ed25519_dalek::KEYPAIR_LENGTH] = keypair_slice.try_into().unwrap();
+        let secret: ed25519_dalek::SecretKey = keypair_bytes[..ed25519_dalek::SECRET_KEY_LENGTH]
+            .try_into()
+            .unwrap();
+        let signing_key = ed25519_dalek::SigningKey::from_bytes(&secret);
+        let public = ed25519_dalek::VerifyingKey::try_from(
+            &keypair_bytes[ed25519_dalek::SECRET_KEY_LENGTH..],
+        )
+        .map_err(SignatureError::from_source)?;
+        let expected_public = signing_key.verifying_key();
        (public == expected_public)
-            .then_some(Self(ed25519_dalek::Keypair { secret, public }))
+            .then_some(Self(signing_key))
            .ok_or(SignatureError::from_source(String::from(
                "keypair bytes do not specify same pubkey as derived from their secret key",
            )))


This looks like it's doing pretty much the same exact thing as from_keypair_bytes

How about just calling that instead?

joncinque

Looks good to me!

joncinque · 2025-07-12T08:52:12Z

keypair/src/lib.rs

@@ -78,19 +75,19 @@ impl Keypair {
    /// Returns this `Keypair` as a base58-encoded string
    pub fn to_base58_string(&self) -> String {
        let mut out = [0u8; five8::BASE58_ENCODED_64_MAX_LEN];
-        let len = five8::encode_64(&self.0.to_bytes(), &mut out);
+        let len = five8::encode_64(&self.to_bytes(), &mut out);


Ah you're right, sorry, I was looking at methods on SigningKey

samkim-crypto

Some minor comments below, but looks good to me!

samkim-crypto · 2025-07-13T01:43:33Z

keypair/src/lib.rs

-            )))
+        };
+        // compiler elides the second len check so try_into().unwrap() is as fast as unsafe code
+        let keypair_bytes: &[u8; ed25519_dalek::KEYPAIR_LENGTH] = keypair_slice.try_into().unwrap();


What do you think about merging the length check with this line? It seems like the code will be slightly more concise and we can remove the unwrap as well.

let keypair_bytes: &[u8; ed25519_dalek::KEYPAIR_LENGTH] = bytes .try_into() .map_err(|_| SignatureError::from_source(String::from( "candidate keypair byte array is the wrong length", )))?;

samkim-crypto · 2025-07-13T01:53:44Z

keypair/src/lib.rs

-    Ok(Keypair(dalek_keypair))
+    // this won't fail as we've already checked the length
+    let secret_key =
+        ed25519_dalek::SecretKey::try_from(&seed[..ed25519_dalek::SECRET_KEY_LENGTH]).unwrap();


Nit: an unwrap should be safe here, but why not just use ??

Suggested change

ed25519_dalek::SecretKey::try_from(&seed[..ed25519_dalek::SECRET_KEY_LENGTH]).unwrap();

ed25519_dalek::SecretKey::try_from(&seed[..ed25519_dalek::SECRET_KEY_LENGTH])?;

I can't imagine this changing, but this can future-proof any api changes that introduce a new way of erroring.

kevinheavey force-pushed the ed25519-dalek-2.1.1 branch from 30b1850 to 4dce186 Compare February 13, 2025 08:52

joncinque mentioned this pull request Mar 24, 2025

Tracking issue for upcoming breaking changes #84

Open

juchiast reviewed Mar 25, 2025

View reviewed changes

kevinheavey force-pushed the ed25519-dalek-2.1.1 branch from 24cb6ce to c15a222 Compare March 25, 2025 14:14

joncinque added the breaking PR contains breaking changes label Mar 31, 2025

joncinque mentioned this pull request Mar 31, 2025

keypair / signature / ed25519: Deprecate dalek types from public API #107

Merged

kevinheavey added 2 commits July 11, 2025 10:43

Bump ed25519-dalek to 2.1.1, ed25519-dalek-bip32 to 0.3.0

371bdc0

unwrap where error is impossible

aabc669

kevinheavey force-pushed the ed25519-dalek-2.1.1 branch from c15a222 to aabc669 Compare July 11, 2025 09:46

joncinque reviewed Jul 11, 2025

View reviewed changes

use from_keypair_bytes

382698f

joncinque reviewed Jul 12, 2025

View reviewed changes

samkim-crypto self-requested a review July 13, 2025 00:37

samkim-crypto reviewed Jul 13, 2025

View reviewed changes

	let len = five8::encode_64(&self.to_bytes(), &mut out);
	let len = five8::encode_64(self.as_bytes(), &mut out);

	ed25519_dalek::SecretKey::try_from(&seed[..ed25519_dalek::SECRET_KEY_LENGTH]).unwrap();
	ed25519_dalek::SecretKey::try_from(&seed[..ed25519_dalek::SECRET_KEY_LENGTH])?;

Bump ed25519-dalek to 2.1.1, ed25519-dalek-bip32 to 0.3.0 #26

Are you sure you want to change the base?

Bump ed25519-dalek to 2.1.1, ed25519-dalek-bip32 to 0.3.0 #26

Uh oh!

Conversation

kevinheavey commented Feb 11, 2025

Summary of Changes

Uh oh!

seanyoung commented Feb 13, 2025

Uh oh!

kevinheavey commented Feb 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joncinque commented Jul 9, 2025

Uh oh!

joncinque left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinheavey Jul 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joncinque left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samkim-crypto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinheavey Jul 12, 2025 •

edited

Loading