From 34f8b6cac896894de5327d4172326e35e35940f5 Mon Sep 17 00:00:00 2001 From: Moudy Date: Tue, 21 Apr 2026 00:37:06 +0200 Subject: [PATCH] docs: split miscoupled private-PDA test docs and clean phrasing Addresses the following review comments: - "Isn't two_mask_3_claims_under_same_seed_are_rejected already checking that there's a mechanism protecting against this exploit scenario?" The doc block at nssa/src/state.rs:2488-2504 mixes three paragraphs, one about reuse, one TODO about wallet side input, one exploit pin, all attached to two_mask_3_claims_under_same_seed_are_rejected. The reuse test below it had no doc at all. I split as follows: the exploit-pin paragraph stays on two_mask_3_claims_..., the reuse paragraph moves to a fresh docstring on mask_3_reuse_across_txs_currently_unsupported. - "I don't understand this. I think this should fail because ... the input pre_state which is marked with is_authorized=true will make things fail." The reuse test's new docstring cites the actual reject site, the post-loop private_pda_bound_positions assertion in privacy_preserving_circuit.rs:185-192. At top level the Entry::Vacant arm accepts is_authorized=true unconditionally, the rejection comes from the bound-positions check firing because noop emits no Claim::Pda and there is no caller ChainedCall.pda_seeds. - "let's dont have this TODO as part of the doc" The block is moved out into regular // comments immediately above mask_3_reuse_across_txs_currently_unsupported. - "let's not add implementation details to docs" In caller_pda_seeds_authorize_mask_3_private_pda_for_callee's docstring, I dropped the parenthetical "(Occupied branch)" and the trailing sentence about which validate_and_sync_states code path gets exercised. - "what does \`Claim::Pda(seed)\` / \`pda_seeds\` mean?" I rewrote the pda_family_binding docstring at privacy_preserving_circuit.rs:33-39: replaced the ambiguous "Claim::PrivatePda and ChainedCall's private seeds into plain Claim::Pda(seed) / pda_seeds" phrase with "a Claim::Pda(seed) in a program's post_state or a caller's ChainedCall.pda_seeds entry". - Suggestion on nssa/src/validated_state_diff.rs:226 rewriting "The public-execution path only sees mask-0 accounts" to "The public-execution path only sees public accounts". Applied: "The public-execution path only sees public accounts". - Clarification requested on the private_pda_bound_positions field: I expanded the docstring at privacy_preserving_circuit.rs:26-31 to state that binding is an idempotent property, not an event, and to enumerate the two proof paths that populate it (a Claim::Pda on a mask-3 pre_state, or a caller's pda_seeds matching under the private derivation). --- nssa/src/state.rs | 34 ++++++++++--------- nssa/src/validated_state_diff.rs | 2 +- .../src/bin/privacy_preserving_circuit.rs | 30 +++++++++------- 3 files changed, 37 insertions(+), 29 deletions(-) diff --git a/nssa/src/state.rs b/nssa/src/state.rs index f09d6b08..81d441e6 100644 --- a/nssa/src/state.rs +++ b/nssa/src/state.rs @@ -2417,10 +2417,9 @@ pub mod tests { /// Happy path for the caller-seeds authorization of a mask-3 PDA. The delegator claims a /// private PDA via `Claim::Pda(seed)`, then chains to a callee (`noop`) delegating the same - /// seed via `ChainedCall.pda_seeds`. In the callee's step, the `pre_state` is already in - /// `self.pre_states` (Occupied branch) and its authorization is established via the private - /// derivation `private_pda_account_id(delegator, seed, npk) == pre.account_id`. This exercises - /// the `authorized_via_private` path in `validate_and_sync_states`. + /// seed via `ChainedCall.pda_seeds`. In the callee's step, the `pre_state`'s authorization + /// is established via the private derivation + /// `private_pda_account_id(delegator, seed, npk) == pre.account_id`. #[test] fn caller_pda_seeds_authorize_mask_3_private_pda_for_callee() { let delegator = Program::private_pda_delegator(); @@ -2485,20 +2484,11 @@ pub mod tests { assert!(matches!(result, Err(NssaError::CircuitProvingError(_)))); } - /// Pins the current limitation: a mask-3 PDA that was claimed in a previous transaction - /// cannot be re-used in a new transaction as-is, because this PR only binds wallet-supplied - /// npks via a fresh `Claim::Pda` or a caller's `ChainedCall.pda_seeds` — neither is present - /// when a program operates on an already-owned private PDA at top level. The circuit rejects. - /// - /// TODO: a follow-up PR in the Private PDAs series needs to let the wallet supply a - /// `(seed, original_owner_program_id)` side input per mask-3 `pre_state` so the circuit - /// can re-verify `private_pda_account_id(owner, seed, npk) == pre.account_id` without a - /// claim. /// Exploit-scenario pin. A single `(program_id, seed)` pair can derive a family of - /// `AccountId`s (one public PDA and one private PDA per distinct npk). Without the tx-wide + /// `AccountId`s, one public PDA and one private PDA per distinct npk. Without the tx-wide /// family-binding check, a program could claim `PDA_alice` (mask-3, `alice_npk`) and - /// `PDA_bob` (mask-3, `bob_npk`) under the same seed in one transaction, and — once reuse - /// is supported — a later chained call could delegate both to a callee via + /// `PDA_bob` (mask-3, `bob_npk`) under the same seed in one transaction, and once reuse + /// is supported a later chained call could delegate both to a callee via /// `pda_seeds: [S]` and mix balances across them. The binding check rejects the setup /// here: after the first claim records `(program, seed) → PDA_alice`, the second claim /// tries to record `(program, seed) → PDA_bob` and panics. @@ -2530,6 +2520,18 @@ pub mod tests { assert!(matches!(result, Err(NssaError::CircuitProvingError(_)))); } + /// Pins the current limitation: a mask-3 PDA that was claimed in a previous transaction + /// cannot be re-used in a new transaction as-is. This PR only binds supplied npks via a + /// fresh `Claim::Pda` or a caller's `ChainedCall.pda_seeds`, neither is present when a + /// program operates on an already-owned private PDA at top level. The reject site is the + /// post-loop `private_pda_bound_positions` assertion in + /// `privacy_preserving_circuit.rs`: `noop` emits no `Claim::Pda` and there is no caller + /// `ChainedCall.pda_seeds`, so position 0 is never bound and the assertion fires. + // + // TODO: a follow-up PR in the Private PDAs series needs to let the wallet supply a + // `(seed, original_owner_program_id)` side input per mask-3 `pre_state` so the circuit + // can re-verify `private_pda_account_id(owner, seed, npk) == pre.account_id` without a + // claim. #[test] fn mask_3_reuse_across_txs_currently_unsupported() { let program = Program::noop(); diff --git a/nssa/src/validated_state_diff.rs b/nssa/src/validated_state_diff.rs index 46f240ea..4528b0ae 100644 --- a/nssa/src/validated_state_diff.rs +++ b/nssa/src/validated_state_diff.rs @@ -223,7 +223,7 @@ impl ValidatedStateDiff { } Claim::Pda(seed) => { // The program can only claim accounts that correspond to the PDAs it is - // authorized to claim. The public-execution path only sees mask-0 + // authorized to claim. The public-execution path only sees public // accounts, so the public-PDA derivation is the correct formula here. let pda = AccountId::from((&chained_call.program_id, &seed)); ensure!( diff --git a/program_methods/guest/src/bin/privacy_preserving_circuit.rs b/program_methods/guest/src/bin/privacy_preserving_circuit.rs index d6885aaf..597a0c98 100644 --- a/program_methods/guest/src/bin/privacy_preserving_circuit.rs +++ b/program_methods/guest/src/bin/privacy_preserving_circuit.rs @@ -23,20 +23,26 @@ struct ExecutionState { post_states: HashMap, block_validity_window: BlockValidityWindow, timestamp_validity_window: TimestampValidityWindow, - /// Positions (in `pre_states`) of mask-3 accounts whose wallet-supplied npk has been bound - /// to their `AccountId` via a proven `private_pda_account_id(program_id, seed, npk)` check. - /// The binding happens when the circuit validates a `Claim::Pda(seed)` on that `pre_state`, - /// or when it authorizes that `pre_state` via a caller's `ChainedCall.pda_seeds`. After the - /// main loop, every mask-3 position must appear in this set; otherwise the npk is unbound - /// and the circuit rejects. + /// Positions (in `pre_states`) of mask-3 accounts whose supplied npk has been bound to + /// their `AccountId` via a proven `private_pda_account_id(program_id, seed, npk)` check. + /// Two proof paths populate this set: + /// 1. A `Claim::Pda(seed)` in a program's post_state on that `pre_state`. + /// 2. A caller's `ChainedCall.pda_seeds` entry matching that `pre_state` under the + /// private derivation. + /// Binding is an idempotent property, not an event: the same position can legitimately be + /// bound through both paths in the same tx (e.g. a program claims a private PDA and then + /// delegates it to a callee), and the set uses `contains`, not `assert!(insert)`. After + /// the main loop, every mask-3 position must appear in this set; otherwise the npk is + /// unbound and the circuit rejects. mask3_bound_positions: HashSet, /// Across the whole transaction, each `(program_id, seed)` pair may resolve to at most one - /// `AccountId`. A seed under a program can derive a family of accounts (one public PDA and - /// one private PDA per distinct npk), and unifying `Claim::PrivatePda` and `ChainedCall`'s - /// private seeds into plain `Claim::Pda(seed)` / `pda_seeds` would otherwise let a single - /// `pda_seeds: [S]` in a chained call authorize multiple family members at once. We record - /// every claim and caller-authorization resolution here and reject any mismatch, making the - /// rule: one `(program, seed)` → one account per tx. + /// `AccountId`. A seed under a program can derive a family of accounts, one public PDA and + /// one private PDA per distinct npk. Without this check, a single `pda_seeds: [S]` entry in + /// a chained call could authorize multiple family members at once (different npks under the + /// same seed) and let a callee mix balances across them. Every claim and every + /// caller-authorization resolution is recorded here, either as a new `(program, seed)` → + /// `AccountId` entry or as an equality check against the existing one, making the rule: one + /// `(program, seed)` → one account per tx. pda_family_binding: HashMap<(ProgramId, PdaSeed), AccountId>, }