refactor: address PR #449 review feedback

This commit is contained in:
Moudy 2026-04-29 10:11:37 +02:00
parent 9927e6e690
commit 198eac1cf1
6 changed files with 113 additions and 115 deletions

View File

@ -13,13 +13,13 @@ use super::secret_holders::{PrivateKeyHolder, SecretSpendingKey, ViewingSecretKe
/// Manages shared viewing keys for a group of controllers owning private PDAs.
///
/// The Group Master Secret (GMS) is a 32-byte random value shared among controllers.
/// Each private PDA owned by the group gets a unique `SecretSpendingKey` derived from
/// Each private PDA owned by the group gets a unique [`SecretSpendingKey`] derived from
/// the GMS by mixing the PDA seed into the SHA-256 input (see `secret_spending_key_for_pda`).
///
/// # Distribution
///
/// The GMS is a long-term secret and must never cross a trust boundary in raw form.
/// Controllers share it off-chain by sealing it under each recipient's `ViewingPublicKey`
/// Controllers share it off-chain by sealing it under each recipient's [`ViewingPublicKey`]
/// (see `seal_for` / `unseal`). Wallets persisting a `GroupKeyHolder` must encrypt it at
/// rest; the raw bytes are exposed only via [`GroupKeyHolder::dangerous_raw_gms`], which
/// is intended for the sealing path exclusively.
@ -91,14 +91,10 @@ impl GroupKeyHolder {
/// The new GMS is `SHA256(PREFIX || rotation_salt || old_gms)`. The rotation salt must
/// be a fresh 32-byte random value contributed by the member who initiates the rotation.
/// Reusing a salt from a previous ratchet produces the same GMS as that previous
/// ratchet, collapsing the key rotation. Callers must generate the salt from a CSPRNG.
/// ratchet, collapsing the key rotation. Callers must generate the salt from a secure random source.
///
/// After ratcheting, all remaining controllers must receive the new `GroupKeyHolder`
/// via `seal_for` / `unseal`.
#[expect(
clippy::arithmetic_side_effects,
reason = "epoch overflow at 2^32 ratchets is not a realistic scenario"
)]
pub fn ratchet(&mut self, rotation_salt: [u8; 32]) {
const PREFIX: &[u8; 32] = b"/LEE/v0.3/GroupKeyRatchet/GMS\x00\x00\x00";
let mut hasher = sha2::Sha256::new();
@ -106,7 +102,7 @@ impl GroupKeyHolder {
hasher.update(rotation_salt);
hasher.update(self.gms);
self.gms = hasher.finalize_fixed().into();
self.epoch += 1;
self.epoch = self.epoch.checked_add(1).expect("epoch overflow");
}
/// Derive a per-PDA [`SecretSpendingKey`] by mixing the seed into the SHA-256 input.
@ -133,7 +129,7 @@ impl GroupKeyHolder {
.produce_private_key_holder(None)
}
/// Encrypts this holder's GMS and epoch under the recipient's `ViewingPublicKey`.
/// Encrypts this holder's GMS and epoch under the recipient's [`ViewingPublicKey`].
///
/// Uses an ephemeral ECDH key exchange to derive a shared secret, then AES-256-GCM
/// to encrypt the payload. The returned bytes are
@ -142,10 +138,6 @@ impl GroupKeyHolder {
/// Each call generates a fresh ephemeral key, so two seals of the same holder produce
/// different ciphertexts.
#[must_use]
#[expect(
clippy::arithmetic_side_effects,
reason = "capacity arithmetic on small constants cannot overflow"
)]
pub fn seal_for(&self, recipient_vpk: &ViewingPublicKey) -> Vec<u8> {
let mut ephemeral_scalar: Scalar = [0_u8; 32];
OsRng.fill_bytes(&mut ephemeral_scalar);
@ -166,7 +158,11 @@ impl GroupKeyHolder {
.encrypt(&nonce, plaintext.as_ref())
.expect("AES-GCM encryption should not fail with valid key/nonce");
let mut out = Vec::with_capacity(33 + 12 + ciphertext.len());
let capacity = 33_usize
.checked_add(12)
.and_then(|n| n.checked_add(ciphertext.len()))
.expect("seal capacity overflow");
let mut out = Vec::with_capacity(capacity);
out.extend_from_slice(&ephemeral_pubkey.0);
out.extend_from_slice(&nonce_bytes);
out.extend_from_slice(&ciphertext);

View File

@ -29,7 +29,7 @@ pub struct NSSAUserData {
/// An older wallet binary that re-serializes this struct will drop the field.
#[serde(default)]
pub group_key_holders: BTreeMap<String, GroupKeyHolder>,
/// Cached plaintext state of group PDA accounts, keyed by `AccountId`.
/// Cached plaintext state of group private PDA accounts, keyed by `AccountId`.
/// Updated after each group PDA transaction by decrypting the circuit output.
/// The sequencer only stores encrypted commitments, so this local cache is the
/// only source of plaintext state for group PDAs.
@ -194,7 +194,7 @@ impl NSSAUserData {
/// Returns the `GroupKeyHolder` for the given label, if it exists.
#[must_use]
pub fn get_group_key_holder(&self, label: &str) -> Option<&GroupKeyHolder> {
pub fn group_key_holder(&self, label: &str) -> Option<&GroupKeyHolder> {
self.group_key_holders.get(label)
}
@ -227,13 +227,13 @@ mod tests {
#[test]
fn group_key_holder_storage_round_trip() {
let mut user_data = NSSAUserData::default();
assert!(user_data.get_group_key_holder("test-group").is_none());
assert!(user_data.group_key_holder("test-group").is_none());
let holder = GroupKeyHolder::from_gms([42_u8; 32]);
user_data.insert_group_key_holder(String::from("test-group"), holder.clone());
let retrieved = user_data
.get_group_key_holder("test-group")
.group_key_holder("test-group")
.expect("should exist");
assert_eq!(retrieved.dangerous_raw_gms(), holder.dangerous_raw_gms());
assert_eq!(retrieved.epoch(), holder.epoch());

View File

@ -186,6 +186,7 @@ mod tests {
use nssa_core::{
Commitment, DUMMY_COMMITMENT_HASH, EncryptionScheme, Nullifier, SharedSecretKey,
account::{Account, AccountId, AccountWithMetadata, Nonce, data::Data},
program::PdaSeed,
};
use super::*;
@ -417,4 +418,100 @@ mod tests {
assert!(matches!(result, Err(NssaError::CircuitProvingError(_))));
}
/// Group PDA deposit: creates a new PDA and transfers balance from the
/// counterparty. Both accounts owned by `group_pda_spender`.
#[test]
fn group_pda_deposit() {
let program = Program::group_pda_spender();
let noop = Program::noop();
let keys = test_private_account_keys_1();
let npk = keys.npk();
let seed = PdaSeed::new([42; 32]);
let shared_secret_pda = SharedSecretKey::new(&[55; 32], &keys.vpk());
// PDA (new, mask 3)
let pda_id = AccountId::for_private_pda(&program.id(), &seed, &npk);
let pda_pre = AccountWithMetadata::new(Account::default(), false, pda_id);
// Sender (mask 0, public, owned by this program, has balance)
let sender_id = AccountId::new([99; 32]);
let sender_pre = AccountWithMetadata::new(
Account {
program_owner: program.id(),
balance: 10000,
..Account::default()
},
true,
sender_id,
);
let noop_id = noop.id();
let program_with_deps = ProgramWithDependencies::new(program, [(noop_id, noop)].into());
let instruction = Program::serialize_instruction((seed, noop_id, 500_u128, true)).unwrap();
// PDA is mask 3 (private PDA), sender is mask 0 (public).
// The noop chained call is required to establish the mask-3 (seed, npk) binding
// that the circuit enforces for private PDAs. Without a caller providing pda_seeds,
// the circuit's binding check rejects the account.
let result = execute_and_prove(
vec![pda_pre, sender_pre],
instruction,
vec![3, 0],
vec![(npk, shared_secret_pda)],
vec![],
vec![None],
&program_with_deps,
);
let (output, _proof) = result.expect("group PDA deposit should succeed");
// Only PDA (mask 3) produces a commitment; sender (mask 0) is public.
assert_eq!(output.new_commitments.len(), 1);
}
/// Group PDA spend binding: the noop chained call with `pda_seeds` establishes
/// the mask-3 binding for an existing-but-default PDA. Uses amount=0 because
/// testing with a pre-funded PDA requires a two-tx sequence with membership proofs.
#[test]
fn group_pda_spend_binding() {
let program = Program::group_pda_spender();
let noop = Program::noop();
let keys = test_private_account_keys_1();
let npk = keys.npk();
let seed = PdaSeed::new([42; 32]);
let shared_secret_pda = SharedSecretKey::new(&[55; 32], &keys.vpk());
let pda_id = AccountId::for_private_pda(&program.id(), &seed, &npk);
let pda_pre = AccountWithMetadata::new(Account::default(), false, pda_id);
let bob_id = AccountId::new([88; 32]);
let bob_pre = AccountWithMetadata::new(
Account {
program_owner: program.id(),
balance: 10000,
..Account::default()
},
true,
bob_id,
);
let noop_id = noop.id();
let program_with_deps = ProgramWithDependencies::new(program, [(noop_id, noop)].into());
let instruction = Program::serialize_instruction((seed, noop_id, 0_u128, false)).unwrap();
let result = execute_and_prove(
vec![pda_pre, bob_pre],
instruction,
vec![3, 0],
vec![(npk, shared_secret_pda)],
vec![],
vec![None],
&program_with_deps,
);
let (output, _proof) = result.expect("group PDA spend binding should succeed");
assert_eq!(output.new_commitments.len(), 1);
}
}

View File

@ -2567,101 +2567,6 @@ pub mod tests {
assert!(matches!(result, Err(NssaError::CircuitProvingError(_))));
}
/// Group PDA deposit: creates a new PDA and transfers balance from the
/// counterparty. Both accounts owned by `group_pda_spender`.
#[test]
fn group_pda_deposit() {
let program = Program::group_pda_spender();
let noop = Program::noop();
let keys = test_private_account_keys_1();
let npk = keys.npk();
let seed = PdaSeed::new([42; 32]);
let shared_secret_pda = SharedSecretKey::new(&[55; 32], &keys.vpk());
// PDA (new, mask 3)
let pda_id = AccountId::for_private_pda(&program.id(), &seed, &npk);
let pda_pre = AccountWithMetadata::new(Account::default(), false, pda_id);
// Sender (mask 0, public, owned by this program, has balance)
let sender_id = AccountId::new([99; 32]);
let sender_pre = AccountWithMetadata::new(
Account {
program_owner: program.id(),
balance: 10000,
..Account::default()
},
true,
sender_id,
);
let noop_id = noop.id();
let program_with_deps = ProgramWithDependencies::new(program, [(noop_id, noop)].into());
let instruction = Program::serialize_instruction((seed, noop_id, 500_u128, true)).unwrap();
// PDA is mask 3 (private PDA), sender is mask 0 (public).
// Public accounts don't need keys, nsks, or membership proofs.
let result = execute_and_prove(
vec![pda_pre, sender_pre],
instruction,
vec![3, 0],
vec![(npk, shared_secret_pda)],
vec![],
vec![None],
&program_with_deps,
);
let (output, _proof) = result.expect("group PDA deposit should succeed");
// Only PDA (mask 3) produces a commitment; sender (mask 0) is public.
assert_eq!(output.new_commitments.len(), 1);
}
/// Group PDA spend binding: the noop chained call with `pda_seeds` establishes
/// the mask-3 binding for an existing-but-default PDA. Uses amount=0 because
/// testing with a pre-funded PDA requires a two-tx sequence with membership proofs.
#[test]
fn group_pda_spend_binding() {
let program = Program::group_pda_spender();
let noop = Program::noop();
let keys = test_private_account_keys_1();
let npk = keys.npk();
let seed = PdaSeed::new([42; 32]);
let shared_secret_pda = SharedSecretKey::new(&[55; 32], &keys.vpk());
let pda_id = AccountId::for_private_pda(&program.id(), &seed, &npk);
let pda_pre = AccountWithMetadata::new(Account::default(), false, pda_id);
let bob_id = AccountId::new([88; 32]);
let bob_pre = AccountWithMetadata::new(
Account {
program_owner: program.id(),
balance: 10000,
..Account::default()
},
true,
bob_id,
);
let noop_id = noop.id();
let program_with_deps = ProgramWithDependencies::new(program, [(noop_id, noop)].into());
let instruction = Program::serialize_instruction((seed, noop_id, 0_u128, false)).unwrap();
let result = execute_and_prove(
vec![pda_pre, bob_pre],
instruction,
vec![3, 0],
vec![(npk, shared_secret_pda)],
vec![],
vec![None],
&program_with_deps,
);
let (output, _proof) = result.expect("group PDA spend binding should succeed");
assert_eq!(output.new_commitments.len(), 1);
}
#[test]
fn circuit_should_fail_with_too_many_nonces() {
let program = Program::simple_balance_transfer();

View File

@ -41,7 +41,7 @@ pub mod cli;
pub mod config;
pub mod helperfunctions;
pub mod poller;
pub mod privacy_preserving_tx;
mod privacy_preserving_tx;
pub mod program_facades;
pub const HOME_DIR_ENV_VAR: &str = "NSSA_WALLET_HOME_DIR";

View File

@ -236,7 +236,7 @@ async fn group_pda_preparation(
let holder = wallet
.storage
.user_data
.get_group_key_holder(group_label)
.group_key_holder(group_label)
.ok_or(ExecutionFailureKind::KeyNotFoundError)?;
let keys = holder.derive_keys_for_pda(seed);