feat(keysplit): Split multiple validator keys into one file #440

dknopik · 2025-07-18T12:09:52Z

Proposed Changes

Support splitting multiple keys into a single json file, to allow the user registering multiple validators in one go.

Additional Info

This is part of finally mainlining some features I implemented for a demo a while ago.

Copilot

Pull Request Overview

This PR adds support for splitting multiple validator keys into a single JSON file, enabling users to register multiple validators in one operation. The changes refactor the keysplit functionality to handle multiple keys instead of just one.

Support for multiple keystore file paths instead of a single path
Refactored splitting functions to process multiple keys and return a Split struct containing key shares and nonce
Updated output format to handle multiple validator keys in a single file

Reviewed Changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`anchor/keysplit/src/split.rs`	Modified split functions to handle multiple keys and return `Split` struct
`anchor/keysplit/src/output.rs`	Updated output generation to handle multiple encrypted key splits
`anchor/keysplit/src/lib.rs`	Modified main logic to process multiple keystore files with parallel processing
`anchor/keysplit/src/crypto.rs`	Updated encryption function to work with `Split` struct
`anchor/keysplit/src/cli.rs`	Changed CLI to accept multiple keystore paths
`anchor/keysplit/Cargo.toml`	Added rayon dependency for parallel processing
`anchor/database/src/keysplit_operations.rs`	Updated function signature to accept slice instead of owned vector

anchor/keysplit/src/split.rs

Copilot · 2025-07-18T12:24:29Z

anchor/keysplit/src/lib.rs

    // 4) Encrypt the keyshares with the operators public keys
    info!("Encrypting keyshares...");
-    let encrypted_keyshares = encrypt_keyshares(keyshares)?;
+    let encrypted_keyshares = splits


[nitpick] Consider using parallel processing with par_iter() for encrypting keyshares, similar to how key extraction is parallelized, since encryption can be computationally expensive.

diegomrsantos · 2025-07-18T14:48:30Z

anchor/keysplit/src/crypto.rs

-    key_shares: Vec<KeyShare>,
-) -> Result<Vec<EncryptedKeyShare>, KeysplitError> {
-    key_shares
+    split: Split<KeyShare>,


Personally, it's not clear to me what the difference between Vec<KeyShare> and Split<KeyShare> is.

added a comment

anchor/keysplit/src/split.rs

Zacholme7 · 2025-07-22T16:25:26Z

anchor/keysplit/src/cli.rs


 // Options that are releveant to both onchain and manual keysplitting
 #[derive(Parser, Clone, Debug)]
 pub struct SharedKeygenOptions {


something I thought of is that we should probably adjust our cli names here to match their docs https://docs.ssv.network/stakers/tools/ssv-keys-cli/

Some of our flags function differently, e.g. keystore_paths takes multiple paths instead of a folder if the user wants to split multiple and output_path points to the folder to place the input into (instead of being the path for the file to be written). We could change that of course, but personally I prefer our current behavior. wdyt?

My point is that matching the names (IMO) only has value if we also match the behavior.

dknopik · 2025-07-25T12:44:17Z

Thanks both, will address

mergify · 2025-07-29T15:43:29Z

This pull request has merge conflicts. Could you please resolve them @dknopik? 🙏

# Conflicts: # anchor/keysplit/src/split.rs

mergify · 2025-10-11T11:10:56Z

Hi @dknopik, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time.

claude-code-actions-sigp · 2025-10-14T12:01:00Z

anchor/keysplit/src/crypto.rs

        .operators
        .0
        .iter()
        .map(|id| KeyId::try_from(*id).unwrap());


Error Handling Issue: The .unwrap() here is unsafe for production code. According to CLAUDE.md, production code should avoid .unwrap() without clear safety justification.

Consider using proper error propagation:

Suggested change

.map(|id| KeyId::try_from(*id).unwrap());

.map(|id| KeyId::try_from(*id).map_err(|e| KeysplitError::Misc(format!("Invalid operator ID: {e:?}"))))?

However, this would require changing the return type. A better approach might be to validate operator IDs earlier in the flow before calling this function.

claude-code-actions-sigp · 2025-10-14T12:01:01Z

anchor/keysplit/src/lib.rs

 ) -> Result<(), KeysplitError> {
-    let shared = keysplit.get_shared().clone();
+    let mut shared = keysplit.get_shared();
+    shared.operators.0.sort_unstable();


Side Effect Ordering: Sorting the operators in-place modifies the shared configuration. This is good for consistency, but consider:

Documentation: Add a comment explaining why sorting is necessary (presumably to ensure consistent ordering across multiple validators)

Early validation: This should happen before any file I/O operations for fail-fast behavior

Deduplication: Should duplicate operator IDs be rejected? Currently they would be silently accepted.

claude-code-actions-sigp · 2025-10-14T12:01:02Z

anchor/keysplit/src/lib.rs

-        .map_err(|e| KeysplitError::Keystore(format!("Failed to read keystore file: {e:?}")))?;
-    info!("Successfully read in validator keystore file");
+    // 1) Read in the keystore files and parse them into a usable format
+    let keystores = shared


Input Validation Missing: Multiple keystore paths are now accepted, but there's no validation that:

At least one path is provided (empty vec would be accepted)

All paths exist before attempting to read

Paths are not duplicates

Reasonable limits on the number of keystores (DoS protection)

Consider adding validation:

if shared.keystore_paths.is_empty() { return Err(KeysplitError::Keystore("At least one keystore path must be provided".to_string())); } if shared.keystore_paths.len() > MAX_KEYSTORES { return Err(KeysplitError::Keystore(format!("Too many keystores provided. Maximum: {}", MAX_KEYSTORES))); }

claude-code-actions-sigp · 2025-10-14T12:01:26Z

anchor/keysplit/src/lib.rs

+    let password = read_password(shared.password_file.as_deref())
+        .map_err(|e| KeysplitError::Keystore(format!("Unable to get password: {e}")))?;
+    let keys = keystores
+        .into_par_iter()


Performance: Good use of Rayon! The parallel decryption with into_par_iter() is excellent for handling multiple keystores efficiently.

Security Concern - Password in Memory: The password is shared across all parallel operations. While this is correct behavior, ensure that:

The password is properly zeroized after use (check if read_password returns a zeroizing type)

Memory isn't swapped to disk during parallel operations

Consider documenting the security implications

Edge Case: All keystores must use the same password. Should this be documented in the CLI help text? Or should different passwords be supported per keystore?

claude-code-actions-sigp · 2025-10-14T12:01:28Z

anchor/keysplit/src/split.rs

+    secret_keys
+        .into_iter()
+        .map(|secret_key| {
+            let ret = create_keyshares_for_key(nonce, shared, secret_key, public_keys);


Nonce Increment Logic - Potential Issue: The nonce is incremented sequentially for each key, but this happens in a map() which creates intermediate bindings. The logic is correct but could be clearer.

Security Consideration: If this function is called multiple times or errors occur midway, nonce values could be skipped or reused. Consider:

Documenting that nonce management is the caller's responsibility

Ensuring atomic nonce updates if this is used in concurrent contexts

Validating that nonces don't overflow (though u64 is effectively unlimited)

The current implementation is correct for single-use scenarios but could be fragile if refactored.

claude-code-actions-sigp · 2025-10-14T12:01:29Z

anchor/keysplit/src/split.rs

            .map(|(split_key, rsa)| KeyShare {
                id: u64::from(split_key.0),
-                public_key: rsa,
+                public_key: rsa.clone(),


Performance - Unnecessary Clone: The rsa.clone() on each iteration clones the entire RSA public key structure. This is expensive for RSA keys (2048+ bits).

Consider:

Using Arc<Rsa<Public>> to share keys across splits

Accepting the clone as acceptable cost (RSA public keys are not huge)

Benchmarking if this becomes a bottleneck with many validators

For now, this is probably acceptable since RSA public key cloning isn't catastrophic, but it's worth noting for future optimization.

claude-code-actions-sigp · 2025-10-14T12:01:30Z

anchor/keysplit/src/output.rs

-            public_key: keys.pk,
-            operators,
-        };
+        let shares = encrypted_keys


Logic Correctness: The .zip(keys) assumes that encrypted_keys and keys have the same length and order. This is correct based on the calling code, but could be fragile.

Defensive Programming: Consider adding a debug assertion or runtime check:

assert_eq!(encrypted_keys.len(), keys.len(), "Mismatch between encrypted keys and keypairs");

Or return an error if lengths don't match:

if encrypted_keys.len() != keys.len() { return Err(KeysplitError::Misc("Mismatch between encrypted keys and keypairs".to_string())); }

This makes the invariant explicit and prevents silent bugs if refactored.

claude-code-actions-sigp · 2025-10-14T12:01:31Z

anchor/database/src/keysplit_operations.rs

    pub fn get_keys_for_operators(
        &self,
-        operators: Vec<u64>,
+        operators: &[u64],


Good Change: Changing from Vec<u64> to &[u64] is more idiomatic Rust and avoids unnecessary allocations. This is a positive improvement! ✅

The caller now passes &onchain.shared.operators.0 instead of moving the vector, which is more efficient.

claude-code-actions-sigp · 2025-10-14T12:01:45Z

anchor/keysplit/Cargo.toml

 hex = { workspace = true }
 openssl = { workspace = true }
 operator_key = { workspace = true }
+rayon = "1.10.0"


Dependency Management: Adding rayon = "1.10.0" for parallel processing is appropriate for this use case.

Recommendation: Consider using workspace version management for consistency:

rayon = { workspace = true }

Then define the version once in the workspace Cargo.toml. This ensures consistent versions across all crates using rayon.

claude-code-actions-sigp · 2025-10-14T12:01:46Z

anchor/keysplit/src/cli.rs

    #[clap(
        long,
-        help = "Path to the validator keystore file",
+        help = "Path(s) to the validator keystore file",


Documentation & UX: The help text and parameter name changes are good, but consider:

Plural naming: keystore_paths (plural) is clearer than allowing confusion about singular/plural

Help text enhancement: Specify if paths can be files or directories, and any ordering guarantees

Examples: Add a usage example in help:

help = "Path(s) to the validator keystore files. Multiple paths can be specified. Example: --keystore-path key1.json --keystore-path key2.json"

Validation note: Document that all keystores must use the same password

claude-code-actions-sigp · 2025-10-14T12:03:35Z

anchor/keysplit/src/crypto.rs

        .operators
        .0
        .iter()
        .map(|id| KeyId::try_from(*id).unwrap());


🚨 Critical: Unsafe .unwrap() on user-provided data

This will panic if any operator ID cannot be converted to KeyId. Operator IDs come from user input and must be validated properly.

Suggested change

.map(|id| KeyId::try_from(*id).unwrap());

let key_ids: Result<Vec<KeyId>, _> = shared

.operators

.0

.iter()

.map(|id| KeyId::try_from(*id))

.collect();

let key_ids = key_ids.map_err(|e|

KeysplitError::InvalidOperator(format!("Invalid operator ID: {e}")))?;

split(sk, threshold as u64, key_ids.into_iter())

.map_err(|e| KeysplitError::SplitFailure(format!("Failed to split key: {e:?}")))

This follows the project's code quality principle: Never use .unwrap() or .expect() without clear safety justification in production code.

claude-code-actions-sigp · 2025-10-14T12:03:37Z

anchor/keysplit/src/split.rs

+        .map(|secret_key| {
+            let ret = create_keyshares_for_key(nonce, shared, secret_key, public_keys);
+            nonce += 1;
+            ret
+        })
+        .collect()


Suggestion: Use enumerate() instead of mutable capture

Mutating a captured variable inside map is unconventional in Rust. Consider this more idiomatic approach:

Suggested change

.map(|secret_key| {

let ret = create_keyshares_for_key(nonce, shared, secret_key, public_keys);

nonce += 1;

ret

})

.collect()

secret_keys

.into_iter()

.enumerate()

.map(|(i, secret_key)| {

create_keyshares_for_key(nonce + i as u64, shared, secret_key, public_keys)

})

.collect()

This makes the nonce progression explicit and avoids mutable state in the iterator.

claude-code-actions-sigp · 2025-10-14T12:04:07Z

anchor/keysplit/src/output.rs

+        let shares = encrypted_keys
+            .into_iter()
+            .zip(keys)
+            .map(|(share, key)| {
+                let payload = Payload::new(&share.key_shares, &key, share.nonce, shared.owner);
+                let operators: Vec<Operator> =
+                    share.key_shares.into_iter().map(Operator::from).collect();
+
+                let output_key_data = OutputKeyData {
+                    owner_nonce: share.nonce,
+                    owner_address: shared.owner,
+                    public_key: key.pk,
+                    operators,
+                };
+
+                OutputKeyShare {
+                    data: output_key_data,
+                    payload,
+                }
+            })
+            .collect();


✅ Good: Clean refactoring with proper structure

The use of zip() here is elegant and maintains the relationship between encrypted keys and their corresponding validator keys. The transformation is clear and functional.

claude-code-actions-sigp · 2025-10-14T12:04:08Z

anchor/keysplit/src/lib.rs

+    let keys = keystores
+        .into_par_iter()
+        .map(|keystore| {
+            keystore.decrypt_keypair(password.as_bytes()).map_err(|e| {
+                KeysplitError::Keystore(format!("Failed to decrypt keystore file: {e:?}"))
+            })
+        })
+        .collect::<Result<Vec<_>, _>>()?;


✅ Security: Good use of parallel processing with thread-safe password handling

The parallel decryption using into_par_iter() is appropriate since:

Keystore decryption is CPU-intensive (good candidate for parallelization)

The password is immutable and properly Sync

Each keystore is processed independently (no data races)

Using Zeroizing<String> ensures the password is securely wiped from memory

Great use of rayon for performance without compromising security!

claude-code-actions-sigp · 2025-10-14T12:04:09Z

anchor/keysplit/src/lib.rs

 ) -> Result<(), KeysplitError> {
-    let shared = keysplit.get_shared().clone();
+    let mut shared = keysplit.get_shared();
+    shared.operators.0.sort_unstable();


✅ Good: Sorting operators ensures deterministic nonce assignment

Sorting the operator IDs ensures consistent ordering across runs, which is important for deterministic nonce assignment in the multi-key scenario.

claude-code-actions-sigp · 2025-10-14T12:04:10Z

anchor/database/src/keysplit_operations.rs

    pub fn get_keys_for_operators(
        &self,
-        operators: Vec<u64>,
+        operators: &[u64],


✅ Performance: Good optimization from Vec<u64> to &[u64]

This change eliminates an unnecessary allocation by accepting a slice instead of an owned Vec. This is idiomatic Rust and improves performance by avoiding clones when calling this method.

claude-code-actions-sigp · 2025-10-14T12:04:11Z

anchor/keysplit/src/split.rs

+/// A single successfully split validator key. Contains a Vec of the key shares ([`KeyShare`] or
+/// [`EncryptedKeyShare`]) and the nonce needed to sign the shares.
+pub struct Split<T> {
+    pub key_shares: Vec<T>,
+    pub nonce: u64,
+}


✅ Documentation: Well-documented generic struct

Good documentation that explains what Split<T> represents and how it's used with different types (KeyShare or EncryptedKeyShare). This makes the code more maintainable.

claude-code-actions-sigp · 2025-10-14T12:04:13Z

anchor/keysplit/Cargo.toml

 hex = { workspace = true }
 openssl = { workspace = true }
 operator_key = { workspace = true }
+rayon = "1.10.0"


📦 Dependency Addition: rayon for parallel processing

Adding rayon is justified for parallel keystore decryption. However, consider:

Version pinning: The version "1.10.0" is not using workspace version management. Check if this should be added to the workspace-level Cargo.toml for consistency.

Audit status: Ensure rayon has been security audited as per project policy (run make audit).

If multiple crates will use rayon, consider adding it to the workspace dependencies.

Split multiple keys into one file

036ba55

dknopik added the ready-for-review This PR is ready to be reviewed label Jul 18, 2025

diegomrsantos requested a review from Copilot July 18, 2025 12:23

Copilot AI reviewed Jul 18, 2025

View reviewed changes

diegomrsantos reviewed Jul 18, 2025

View reviewed changes

anchor/keysplit/src/split.rs Outdated Show resolved Hide resolved

dknopik added 2 commits July 21, 2025 13:52

Clone later

6f28556

split function

496c980

Zacholme7 reviewed Jul 22, 2025

View reviewed changes

Clarify Split

596ac3b

mergify bot added waiting-on-author and removed ready-for-review This PR is ready to be reviewed labels Jul 29, 2025

dknopik added 3 commits August 5, 2025 11:54

Merge branch 'unstable' into multi-split

db11848

Merge branch 'unstable' into multi-split

fc26113

# Conflicts: # anchor/keysplit/src/split.rs

Merge branch 'unstable' into multi-split

18fffcf

mergify bot closed this Oct 11, 2025

mergify bot added the stale label Oct 11, 2025

dknopik removed the stale label Oct 14, 2025

Merge branch 'unstable' into multi-split

f8de81f

dknopik reopened this Oct 14, 2025

claude-code-actions-sigp bot reviewed Oct 14, 2025

View reviewed changes

	.map(\|id\| KeyId::try_from(*id).unwrap());
	.map(\|id\| KeyId::try_from(*id).map_err(\|e\| KeysplitError::Misc(format!("Invalid operator ID: {e:?}"))))?

-        .map(|id| KeyId::try_from(*id).unwrap());
+    let key_ids: Result<Vec<KeyId>, _> = shared
+        .operators
+        .0
+        .iter()
+        .map(|id| KeyId::try_from(*id))
+        .collect();
+    let key_ids = key_ids.map_err(|e|
+        KeysplitError::InvalidOperator(format!("Invalid operator ID: {e}")))?;
+    split(sk, threshold as u64, key_ids.into_iter())
+        .map_err(|e| KeysplitError::SplitFailure(format!("Failed to split key: {e:?}")))

feat(keysplit): Split multiple validator keys into one file #440

Are you sure you want to change the base?

feat(keysplit): Split multiple validator keys into one file #440

Uh oh!

Conversation

dknopik commented Jul 18, 2025

Proposed Changes

Additional Info

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

diegomrsantos Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

dknopik Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Zacholme7 Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

dknopik Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

dknopik commented Jul 25, 2025

Uh oh!

mergify bot commented Jul 29, 2025

Uh oh!

mergify bot commented Oct 11, 2025

Uh oh!

claude-code-actions-sigp bot Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

claude-code-actions-sigp bot Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

claude-code-actions-sigp bot Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

claude-code-actions-sigp bot Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

claude-code-actions-sigp bot Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

claude-code-actions-sigp bot Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

claude-code-actions-sigp bot Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

claude-code-actions-sigp bot Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

claude-code-actions-sigp bot Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

claude-code-actions-sigp bot Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

claude-code-actions-sigp bot Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

claude-code-actions-sigp bot Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

claude-code-actions-sigp bot Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

claude-code-actions-sigp bot Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!