From 60ee06898debedd6af42ebd2e758b14849b85860 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Thu, 28 May 2026 10:21:45 +0200 Subject: [PATCH 1/3] Merge pull request #131 from zk-coins/fix/log-keyword-avoidance fix(router): replace "error" keyword in warn/info log strings (Loki classifier workaround) --- node/src/router.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/node/src/router.rs b/node/src/router.rs index 9f44c056..8ff80ec1 100644 --- a/node/src/router.rs +++ b/node/src/router.rs @@ -899,7 +899,7 @@ async fn send_coin_handler( // builder — `map_send_coins_error` is pure but the // duplicate call was needless work. let mapped = map_send_coins_error(e); - tracing::warn!("send_coins error: {} (status={})", e, mapped.0); + tracing::warn!("send_coins rejected: {} (status={})", e, mapped.0); send_coins_error_response(mapped) } } @@ -1072,7 +1072,7 @@ async fn mint_handler( // both. Map once and thread the tuple into the response // builder. let mapped = map_send_coins_error(e); - tracing::warn!("Mint prepare: err — {} (status={})", e, mapped.0); + tracing::warn!("Mint prepare rejected — {} (status={})", e, mapped.0); return send_coins_error_response(mapped); } }; From f4e2aa0074e778ad8711eba342e04a5e0c59f233 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Thu, 28 May 2026 11:20:11 +0200 Subject: [PATCH 2/3] fix(account): server reads prev commitment pubkey from its own state (#132) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous fix (PR #129) attacked the symptom (wallet's local BIP-32 child-index counter desyncing from the server after a seed restore) by emitting `num_sends` from `/api/balance` so the wallet could hydrate its counter and derive `prev_commitment_pubkey` correctly. That works only when the wallet's deployed code actually syncs the counter — which the stale DEV deploy of `zk-coins/app` demonstrably did not, so `07-send.spec.ts::send-success` kept failing with `Interner Fehler: Vorheriger Public Key fehlt.` after PR #129 went live. Root cause is structural: making the client responsible for `prev_commitment_pubkey` puts a derivable lookup key on the client side that has to stay in lockstep with the server's persisted state across seed restores, app deploys, and TOCTOU windows between balance fetch and signing. Every desync surfaces as a 400. The class of bugs is not solvable by counter-syncing. This change moves the lookup to where the data lives. `Account` gains a `commitment_public_key: Option` field set atomically with `proof` + `num_sends` inside `send_coins_inner`. The AccountUpdate branch reads it directly from the persisted account; the caller-supplied `prev_commitment_pubkey` is ignored. The 400 error string disappears from `map_send_coins_error` — it is unreachable as long as the field invariant (`proof.is_some() iff num_sends > 0 iff commitment_public_key.is_some()`) holds, which `send_coins_inner` is the only mutator of. Net result: a wallet that omits `prev_commitment_pubkey` entirely (or sends a stale one from a desynced counter) now succeeds. The deployed `zk-coins/app:beta` already on DEV stops 400ing without needing the app-side PR #125 to deploy first. Migration 0012 wipes `accounts` (same closed-test-env precedent as 0010 / 0011): the bincode shape is non-additive, and 0011 left post-#129 rows in the inconsistent `proof=Some, commitment_public_key=None` state that would panic the AccountUpdate branch's invariant `expect`. The legacy `SendCoinRequest::prev_commitment_pubkey` field stays on the wire so deployed wallets (including `app` PR #125, which still emits it) keep parsing. Drop it from the API once every published client has cycled off the contract. Regression coverage: - `account_node_tests::test_send_coins_second_send_succeeds_without_prev_commitment_pubkey` drives the AccountUpdate branch with `prev_commitment_pubkey = None` directly through `send_coins`. - `api_remote::second_send_succeeds_without_prev_commitment_pubkey_field` drives the same contract end-to-end against the live DEV server via the slim `ci:full` lane. - `account_node_tests::test_send_coins_twice_from_same_account_uses_update_account` pins the post-condition that all three coupled fields advance together (`proof.is_some()`, `num_sends == 2`, and `commitment_public_key == Some(pubkey_used_in_send_2)`). - The historical 400 mapping unit test (`map_send_coins_error_prev_commitment_pubkey_required_is_400`) becomes `map_send_coins_error_legacy_prev_commitment_pubkey_string_is_unmapped_500` — pinning that the string falls through to the catch-all 500 arm so any future regression that re-introduces it can't be silently re-mapped to 400 without also walking back the architecture. --- ...set_accounts_for_commitment_public_key.sql | 40 +++ node/src/account_node.rs | 86 ++++++- node/src/account_node_tests.rs | 177 +++++++++++-- node/src/router.rs | 47 ++-- node/src/router_tests.rs | 46 ++-- node/tests/api_remote.rs | 238 +++++++++++++++++- 6 files changed, 562 insertions(+), 72 deletions(-) create mode 100644 node/migrations/0012_reset_accounts_for_commitment_public_key.sql diff --git a/node/migrations/0012_reset_accounts_for_commitment_public_key.sql b/node/migrations/0012_reset_accounts_for_commitment_public_key.sql new file mode 100644 index 00000000..b2ab0a6b --- /dev/null +++ b/node/migrations/0012_reset_accounts_for_commitment_public_key.sql @@ -0,0 +1,40 @@ +-- Reset the accounts table to absorb a non-backwards-compatible +-- change to the bincode `Account` shape: the +-- `Account::commitment_public_key: Option` field is the +-- server-authoritative source for the previous-commitment-pubkey +-- lookup that the `send_coins_inner` AccountUpdate branch used to +-- read from the caller-supplied `prev_commitment_pubkey` parameter. +-- The caller-supplied field is now ignored, the new field lives on +-- the persisted account struct, and the invariant the rest of the +-- module relies on becomes +-- `proof.is_some() iff num_sends > 0 iff commitment_public_key.is_some()`. +-- +-- bincode encodings of structs are positional + length-prefixed and +-- there is no in-band "missing field" marker. A pre-PR account blob +-- ends after `num_sends: u32`; a post-PR `bincode::deserialize` call +-- on that blob reads "unexpected end of input" when it tries to +-- consume the next bytes for `commitment_public_key`. As with +-- migration 0011 the fast and operationally cheap fix is to wipe +-- the table: every persisted account is reconstructable from the +-- next mint/send round-trip the user makes against the address. +-- The DEV + PRD environments are closed test envs per +-- `feedback_zkcoins_closed_test_env.md`, so the "wipe-and-rebuild +-- accepts the dataloss" trade-off is the same one 0010 / 0011 set. +-- +-- A SECOND consequence of the refactor is that any account whose +-- bincode blob was persisted with `proof = Some(...)` (i.e. +-- `num_sends > 0`) but without a `commitment_public_key` would +-- panic the AccountUpdate branch at the `expect(...)` documenting +-- the invariant. Migration 0011 left us in exactly this state for +-- every account that had successfully sent post-deploy. Wiping +-- here covers BOTH the bincode-shape change AND that invariant gap +-- in one go. +-- +-- The dependent log/history tables (`account_history`, +-- `coin_proof_store`) are NOT wiped — their rows are historical +-- evidence of past sends/mints and don't reference the wiped +-- account blob's bincode shape. The trigger `accounts_history_capture` +-- that backfills `account_history` on every UPDATE will simply not +-- fire until the next `/api/send` re-populates the accounts row. + +DELETE FROM accounts; diff --git a/node/src/account_node.rs b/node/src/account_node.rs index e87b8cf6..26fc9e22 100644 --- a/node/src/account_node.rs +++ b/node/src/account_node.rs @@ -41,17 +41,48 @@ pub struct Account { /// `account.proof` has been advanced via `send_coins_inner`). /// /// Authoritative source of truth for the wallet's BIP-32 child - /// index counter. After a seed restore the wallet has no local - /// memory of past sends; the server returns this count on the - /// balance endpoint so the wallet can derive the correct current - /// pubkey and the correct `prev_commitment_pubkey` (= pubkey at - /// `num_sends - 1`) without local bookkeeping. + /// index counter on the SIGNING side (which key to sign the + /// outgoing send with). After a seed restore the wallet has no + /// local memory of past sends; the server returns this count on + /// the balance endpoint so the wallet can derive the correct + /// current signing pubkey without local bookkeeping. /// - /// Invariant: `num_sends > 0` iff `proof.is_some()`. Both fields - /// are mutated atomically inside `send_coins_inner` once prove - /// succeeded; no public mutator exists outside that path. + /// The wallet no longer derives `prev_commitment_pubkey` from + /// this counter — that one is supplied authoritatively by the + /// server via [`Self::commitment_public_key`] and the + /// `send_coins_inner` AccountUpdate branch reads it directly + /// from this struct instead of trusting a caller-supplied value. + /// See the field doc on `commitment_public_key` for the rationale. + /// + /// Invariant: `num_sends > 0` iff `proof.is_some()` iff + /// `commitment_public_key.is_some()`. All three fields are mutated + /// atomically inside `send_coins_inner` once prove succeeded; no + /// public mutator exists outside that path. #[serde(default)] pub num_sends: u32, + /// Pubkey of the COMMITMENT the previous successful send produced. + /// + /// Equals the `public_key` argument that `send_coins_inner` used + /// the last time it advanced this account's `proof`. The next + /// AccountUpdate transition looks up that commitment in the + /// SMT to build its `prev_cmp` merkle proofs — historically the + /// client passed this in as `prev_commitment_pubkey`, which broke + /// every time the client's local BIP-32 child-index counter + /// drifted from the server's (typical after a seed restore, an + /// app deploy with an unrelated state-shape change, or a TOCTOU + /// race between a balance fetch and the actual send). + /// + /// Storing it here makes the server the single source of truth + /// for this lookup and reduces the client's send-request payload + /// to inputs that ARE the client's authoritative concern + /// (the signing pubkey + the next pubkey). The legacy + /// `prev_commitment_pubkey` request field is kept on the wire for + /// backwards-compat with already-deployed wallets but is ignored + /// on this code path. + /// + /// Invariant: see [`Self::num_sends`] — `Some` iff `proof.is_some()`. + #[serde(default)] + pub commitment_public_key: Option, } impl Account { @@ -96,6 +127,7 @@ impl Account { coin_history: SparseMerkleTree::new(), balance: 0, num_sends: 0, + commitment_public_key: None, } } /// Uses the coin_template and next_public_key to create the next account_state and generates a @@ -583,8 +615,30 @@ impl AccountNode { let proof: Proof = match &account.proof { Some(account_proof) => { - let account_commitment_public_key = prev_commitment_pubkey - .ok_or("prev_commitment_pubkey required for account update")?; + // The server is the single source of truth for the + // previous commitment's pubkey: it set this field + // atomically with `account.proof` the last time + // `send_coins_inner` succeeded for this account. The + // legacy caller-supplied `prev_commitment_pubkey` is + // ignored on this branch — it produced a class of + // 400s every time the wallet's local BIP-32 + // child-index counter drifted from the server's + // (seed restore + stale app deploy + TOCTOU between + // balance fetch and send-request signing). See the + // field doc on `Account::commitment_public_key` for + // the full story. + // + // The `expect` is the documentation of the invariant + // `proof.is_some() iff commitment_public_key.is_some()` + // (also `iff num_sends > 0`). It is mutated only here, + // atomically with `proof`, so the only way to reach + // the panic is a persisted blob that violates the + // invariant — which migration 0012 wipes pre-emptively + // and which no code path can produce going forward. + let _ = prev_commitment_pubkey; // legacy field, see note above. + let account_commitment_public_key = account + .commitment_public_key + .expect("commitment_public_key is Some whenever proof is Some — see invariant on Account"); let prev_cmp = Self::get_merkle_proofs( account_proof.clone(), account_commitment_public_key, @@ -627,6 +681,18 @@ impl AccountNode { // at 2^32 sends (4 billion); the prover would melt long before // that, but we don't want a panic on the hot path. account.num_sends = account.num_sends.saturating_add(1); + // Record the pubkey that backed THIS send's commitment. The + // NEXT AccountUpdate transition for this account will read it + // back from here to build the previous-commitment merkle proof + // — making the server the single source of truth for the + // `prev_commitment_pubkey` lookup instead of trusting the + // client to re-derive it from a BIP-32 child index that + // routinely drifts after a seed restore. See the field doc on + // `Account::commitment_public_key`. Set last (after the proof + // + num_sends mutations) so the three fields commit together + // — the function as a whole is the atomic unit (the caller + // commits the account-bytes upsert post-prove). + account.commitment_public_key = Some(public_key); // Build CoinProof entries for distribution to recipients. // diff --git a/node/src/account_node_tests.rs b/node/src/account_node_tests.rs index 8dd3ae2e..dfc5687e 100644 --- a/node/src/account_node_tests.rs +++ b/node/src/account_node_tests.rs @@ -129,6 +129,7 @@ fn test_wallet_operations() { coin_history: SparseMerkleTree::new(), balance: 10_000, num_sends: 0, + commitment_public_key: None, }, ); assert_eq!( @@ -258,6 +259,7 @@ fn test_create_minting_account() { coin_history: SparseMerkleTree::new(), balance: 10_000, num_sends: 0, + commitment_public_key: None, }, ); assert_eq!( @@ -282,6 +284,7 @@ fn test_mint_single_invoice() { coin_history: SparseMerkleTree::new(), balance: 10_000, num_sends: 0, + commitment_public_key: None, }, ); @@ -309,6 +312,7 @@ fn test_receive_duplicate_coin_rejected() { coin_history: SparseMerkleTree::new(), balance: 10_000, num_sends: 0, + commitment_public_key: None, }, ); @@ -356,6 +360,7 @@ fn test_receive_updates_balance() { coin_history: SparseMerkleTree::new(), balance: 10_000, num_sends: 0, + commitment_public_key: None, }, ); @@ -413,6 +418,7 @@ fn test_mint_repro_live_setup() { coin_history: SparseMerkleTree::new(), balance: 1_000_000, num_sends: 0, + commitment_public_key: None, }, ); @@ -664,6 +670,7 @@ fn test_receive_coin_rejects_invalid_inclusion_proof() { coin_history: SparseMerkleTree::new(), balance: 10_000, num_sends: 0, + commitment_public_key: None, }, ); @@ -700,6 +707,7 @@ fn test_send_coins_twice_from_same_account_uses_update_account() { coin_history: SparseMerkleTree::new(), balance: 10_000, num_sends: 0, + commitment_public_key: None, }, ); @@ -727,6 +735,107 @@ fn test_send_coins_twice_from_same_account_uses_update_account() { .execute_send_coins(&mut node, vec![Invoice::new(50, recipient)]) .expect("second send should succeed (update_account path)"); assert_eq!(coin_proofs_2.len(), 1); + + // Invariant check: after two sends the three coupled fields are + // all "updated" — `proof = Some`, `num_sends = 2`, and + // `commitment_public_key = Some(pubkey_used_in_send_2)`. The + // AccountUpdate branch reads this last value (not a caller + // parameter) on the NEXT send, so its presence here is the + // load-bearing post-condition. + let acct = node + .get_account(&minting.address) + .expect("minting account still in map after send"); + assert!( + acct.proof.is_some(), + "account.proof must be Some after send" + ); + assert_eq!( + acct.num_sends, 2, + "num_sends bumps once per successful send_coins_inner" + ); + let expected_cpk = + generate_test_public_key(&minting.xpriv, minting.num_pubkeys.saturating_sub(1)); + assert_eq!( + acct.commitment_public_key, + Some(expected_cpk), + "commitment_public_key holds the pubkey used in the most recent send" + ); +} + +/// Regression: a second `send_coins` from an account whose +/// `account.proof = Some(...)` MUST succeed when the caller passes +/// `None` for `prev_commitment_pubkey` — the AccountUpdate branch +/// reads `account.commitment_public_key` from its own state instead +/// of consulting the caller-supplied parameter. Pre-refactor this +/// returned the 400-mapped error +/// `"prev_commitment_pubkey required for account update"`. +/// +/// Live-server analogue is the api_remote test +/// `second_send_succeeds_without_prev_commitment_pubkey_field` — +/// this one drives the same code path through `account_node` directly +/// (no prover, no HTTP) so the contract is pinned even when the +/// `api_remote` suite is skipped (slim CI). +#[test] +fn test_send_coins_second_send_succeeds_without_prev_commitment_pubkey() { + let state_arc = Arc::new(Mutex::new(State::new())); + let mut node = AccountNode::new(Arc::clone(&state_arc)); + + let mut minting = TestAccountData::new_minting_account(); + node.import_account( + minting.address, + Account { + proof: None, + coin_queue: vec![], + coin_history: SparseMerkleTree::new(), + balance: 10_000, + num_sends: 0, + commitment_public_key: None, + }, + ); + + let recipient: Address = digest_from_bytes(&[43u8; 32]); + + // First send: account.proof is None -> prove_initial branch. + // The caller-supplied prev_commitment_pubkey is ignored on this + // branch (it's only consulted on the AccountUpdate branch, and + // post-refactor not even there); pass None to make that explicit. + let coin_proofs_1 = minting + .execute_send_coins(&mut node, vec![Invoice::new(100, recipient)]) + .expect("first send should succeed"); + state_arc + .lock() + .unwrap() + .update( + &coin_proofs_1 + .iter() + .map(|cp| cp.commitment.clone().unwrap()) + .collect::>(), + ) + .unwrap(); + + // Second send WITHOUT prev_commitment_pubkey. Pre-refactor this + // returned `"prev_commitment_pubkey required for account update"` + // and was mapped to 400 by `map_send_coins_error`. Post-refactor + // the AccountUpdate branch reads `account.commitment_public_key` + // (set atomically in the first send) and the prove succeeds. + let current_pk = generate_test_public_key(&minting.xpriv, minting.num_pubkeys); + let next_pk = generate_test_public_key(&minting.xpriv, minting.num_pubkeys + 1); + let coin_proofs_2 = node + .send_coins( + vec![Invoice::new(50, recipient)], + minting.address, + current_pk, + next_pk, + None, // <-- the contract under test: prev_commitment_pubkey omitted + ) + .expect("second send must succeed without prev_commitment_pubkey"); + assert_eq!(coin_proofs_2.len(), 1); + + let acct = node + .get_account(&minting.address) + .expect("minting account still in map after send"); + assert_eq!(acct.num_sends, 2); + assert_eq!(acct.commitment_public_key, Some(current_pk)); } #[test] @@ -743,6 +852,7 @@ fn test_receive_coin_rejects_replay_via_coin_history() { coin_history: SparseMerkleTree::new(), balance: 10_000, num_sends: 0, + commitment_public_key: None, }, ); let recipient: Address = digest_from_bytes(&[9u8; 32]); @@ -802,6 +912,7 @@ fn test_send_coins_rejects_tampered_source_proof_inclusion() { coin_history: SparseMerkleTree::new(), balance: 10_000, num_sends: 0, + commitment_public_key: None, }, ); @@ -884,6 +995,7 @@ fn test_send_coins_rejects_too_many_invoices() { coin_history: SparseMerkleTree::new(), balance: 1_000_000, num_sends: 0, + commitment_public_key: None, }, ); @@ -917,6 +1029,7 @@ fn test_send_coins_rejects_too_many_coins_in_queue() { coin_history: SparseMerkleTree::new(), balance: 10_000, num_sends: 0, + commitment_public_key: None, }, ); let recipient_data = TestAccountData::new_generic(&[20u8; 32], Network::Signet); @@ -991,6 +1104,7 @@ fn test_send_coins_errors_when_state_lacks_commitment_for_in_coin() { coin_history: SparseMerkleTree::new(), balance: 10_000, num_sends: 0, + commitment_public_key: None, }, ); let recipient_data = TestAccountData::new_generic(&[21u8; 32], Network::Signet); @@ -1021,12 +1135,18 @@ fn test_send_coins_errors_when_state_lacks_commitment_for_in_coin() { } /// AccountUpdate branch: when `account.proof = Some(...)` and the -/// caller passes a `prev_commitment_pubkey` that the state's -/// commitment-proof index does not contain, the second call to -/// `get_merkle_proofs` (inside the AccountUpdate-prove preparation) -/// surfaces "Unable to get merkle proofs..." just like the in-coin -/// loop's call. Set up via one honest mint + receive + state.update; -/// then pass a fresh, never-indexed `prev_commitment_pubkey`. +/// account's stored `commitment_public_key` is for a commitment that +/// the state's commitment-proof index does not contain, the second +/// call to `get_merkle_proofs` (inside the AccountUpdate-prove +/// preparation) surfaces "Unable to get merkle proofs..." just like +/// the in-coin loop's call. Set up via one honest mint + receive + +/// state.update; then forge an `account.proof = Some(...)` plus a +/// `commitment_public_key` that is fresh and not indexed in the SMT. +/// +/// As of the `Account::commitment_public_key` refactor the +/// AccountUpdate branch reads the previous commitment pubkey from the +/// account itself (not from a caller-supplied parameter), so the test +/// drives the failure through that field. #[test] fn test_send_coins_errors_when_state_lacks_commitment_for_prev_account_proof() { let state_arc = Arc::new(Mutex::new(State::new())); @@ -1041,6 +1161,7 @@ fn test_send_coins_errors_when_state_lacks_commitment_for_prev_account_proof() { coin_history: SparseMerkleTree::new(), balance: 10_000, num_sends: 0, + commitment_public_key: None, }, ); let recipient_data = TestAccountData::new_generic(&[22u8; 32], Network::Signet); @@ -1065,7 +1186,15 @@ fn test_send_coins_errors_when_state_lacks_commitment_for_prev_account_proof() { // Forge an `account.proof = Some(...)` on the recipient by reusing // the minting account's proof we just produced (signature // verification doesn't happen on this path — `get_merkle_proofs` - // only consults state for the prev_commitment_pubkey lookup). + // only consults state for the commitment-pubkey lookup). + // + // To drive the "Unable to get merkle proofs..." error path we + // also set the recipient's `commitment_public_key` to a fresh, + // never-indexed pubkey. Post-refactor the AccountUpdate branch + // reads THIS field (not a caller parameter) for the lookup, so + // the unknown pubkey lives on the account itself. + let stranger_seed = Xpriv::new_master(Network::Signet, &[99u8; 32]).expect("stranger xpriv"); + let unknown_commitment_pk = generate_test_public_key(&stranger_seed, 0); { let mint_account = node .accounts @@ -1077,20 +1206,23 @@ fn test_send_coins_errors_when_state_lacks_commitment_for_prev_account_proof() { .get_mut(&recipient_addr) .expect("recipient account present after receive_coin"); recipient_account.proof = proof; - // Maintain the `num_sends > 0 iff proof.is_some()` invariant - // documented on the `Account` struct. The forge above only - // moves `proof`; without bumping `num_sends` the recipient - // would carry an inconsistent (proof=Some, num_sends=0) - // shape that the balance handler would mis-emit. + // Maintain the invariant documented on `Account`: + // `proof.is_some() iff num_sends > 0 iff + // commitment_public_key.is_some()`. Forging only `proof` + // would leave an inconsistent shape that the balance handler + // would mis-emit AND that the AccountUpdate branch would + // panic on (the field's `expect` guards the invariant). recipient_account.num_sends = 1; + recipient_account.commitment_public_key = Some(unknown_commitment_pk); } - // Pass a `prev_commitment_pubkey` that the state's commitment - // index has never seen — the lookup fails inside - // get_merkle_proofs and propagates "Unable to get merkle proofs...". - let stranger_seed = Xpriv::new_master(Network::Signet, &[99u8; 32]).expect("stranger xpriv"); - let unknown_prev_pk = generate_test_public_key(&stranger_seed, 0); - + // Caller-supplied `prev_commitment_pubkey` is ignored by the + // post-refactor server — pass `None` here to make that explicit. + // The AccountUpdate branch reads the recipient's stored + // `commitment_public_key` (the stranger pubkey installed above), + // hits the SMT lookup miss, and surfaces "Unable to get merkle + // proofs...". The HTTP mapping in `map_send_coins_error` + // translates this to 422 (caller-fixable). let current_pk = generate_test_public_key(&recipient_data.xpriv, 0); let next_pk = generate_test_public_key(&recipient_data.xpriv, 1); let result = node.send_coins( @@ -1098,13 +1230,8 @@ fn test_send_coins_errors_when_state_lacks_commitment_for_prev_account_proof() { recipient_addr, current_pk, next_pk, - Some(unknown_prev_pk), + None, ); - // The AccountUpdate-branch get_merkle_proofs call uses - // `prev_commitment_pubkey`, which is not in state, so the lookup - // fails. The error string is identical to the in-coin loop's, - // which is fine — both signal the same caller-fixable malformed - // witness, and Item 1's HTTP mapping translates both to 422. assert_eq!( result.unwrap_err(), "Unable to get merkle proofs for provided public key" @@ -1125,6 +1252,7 @@ fn test_send_coins_rejects_coin_queue_entry_without_commitment() { coin_history: SparseMerkleTree::new(), balance: 10_000, num_sends: 0, + commitment_public_key: None, }, ); let recipient: Address = digest_from_bytes(&[10u8; 32]); @@ -1193,6 +1321,7 @@ fn test_send_coins_rejects_source_commitment_missing_from_history_mmr() { coin_history: SparseMerkleTree::new(), balance: 10_000, num_sends: 0, + commitment_public_key: None, }, ); diff --git a/node/src/router.rs b/node/src/router.rs index 8ff80ec1..b9179534 100644 --- a/node/src/router.rs +++ b/node/src/router.rs @@ -167,21 +167,19 @@ pub struct BalanceResponse { /// /// Equals the number of times this account has executed a /// `/api/send` (`account.num_sends`). The wallet uses this value - /// in two places: - /// 1. As `numPubkeys` for the next signing/derivation: the - /// pubkey for the next send is at index `num_sends`. - /// 2. To derive `prev_commitment_pubkey`: the pubkey committed - /// by the previous send is at index `num_sends - 1` (or - /// `None` when `num_sends == 0`, i.e. the wallet has never - /// sent before). + /// as `numPubkeys` for the next signing/derivation: the pubkey + /// for the next send is at index `num_sends`. /// - /// A freshly seed-restored wallet has no local memory of past - /// sends. Without this field the wallet would default to - /// `numPubkeys = 0` and either (a) collide on a second send - /// against the same SMT key, or (b) omit `prev_commitment_pubkey` - /// and receive `"prev_commitment_pubkey required for account - /// update"` from `send_coin_handler`. Both failure modes were - /// observed in the E2E `07-send.spec.ts::send-success` test. + /// The wallet does NOT use it to derive `prev_commitment_pubkey` + /// anymore — the server reads that one from its own state + /// (`Account::commitment_public_key`) and the legacy + /// `prev_commitment_pubkey` field on `SendCoinRequest` is + /// ignored. See the field doc on `Account::commitment_public_key` + /// for the bug class this eliminated (seed restore + + /// stale-deploy + TOCTOU drift between local counter and server + /// state, all surfacing as 400 + /// `"prev_commitment_pubkey required for account update"` in + /// `07-send.spec.ts::send-success`). /// /// Always emitted (no `skip_serializing_if`) so the wallet can /// rely on its presence — `0` is the canonical value for an @@ -203,6 +201,15 @@ pub struct SendCoinRequest { amount: u64, public_key: bitcoin::secp256k1::PublicKey, next_public_key: bitcoin::secp256k1::PublicKey, + /// Legacy field — IGNORED by `send_coin_handler` as of the + /// [`crate::account_node::Account::commitment_public_key`] + /// refactor. The server reads the previous commitment pubkey + /// from its own state instead. Kept on the wire so deployed + /// wallets (and the in-tree `app` PR #125) that still emit it + /// continue to parse against the post-refactor server with no + /// 4xx for an unknown field. Drop entirely once every published + /// wallet has cycled off this contract. + #[serde(default)] prev_commitment_pubkey: Option, signature: Option, timestamp: Option, @@ -347,19 +354,19 @@ pub struct SendCoinResponse { /// string lets clients distinguish "fix your inclusion proof" from /// "fix your account selection". /// - **404 NOT_FOUND** — sender address is not known to the node. -/// - **400 BAD_REQUEST** — request structure violates the API contract -/// (e.g. AccountUpdate transition without `prev_commitment_pubkey`). /// - **500 INTERNAL_SERVER_ERROR** — the prover failed. Body collapses /// to a generic `"prove failed"` to avoid leaking prover-internal /// state to the caller. The full error string is logged via /// `eprintln!` in the handler. +/// +/// The historical 400 `"prev_commitment_pubkey required for account +/// update"` is unreachable as of the +/// [`Account::commitment_public_key`] refactor: the server reads the +/// previous commitment pubkey from its own state instead of trusting +/// the caller. The match arm is therefore gone. pub(crate) fn map_send_coins_error(err: &str) -> (StatusCode, &'static str) { match err { "Unknown account address" => (StatusCode::NOT_FOUND, "Unknown account address"), - "prev_commitment_pubkey required for account update" => ( - StatusCode::BAD_REQUEST, - "prev_commitment_pubkey required for account update", - ), "Insufficient funds" => (StatusCode::UNPROCESSABLE_ENTITY, "Insufficient funds"), // `get_merkle_proofs` failures — reachable from `send_coins` // via the `prev_commitment_pubkey` path. The client supplied diff --git a/node/src/router_tests.rs b/node/src/router_tests.rs index ba2def9d..8db2c776 100644 --- a/node/src/router_tests.rs +++ b/node/src/router_tests.rs @@ -565,13 +565,14 @@ async fn balance_includes_username_when_claimed() { /// `BalanceResponse::num_sends` must reflect the queried account's /// per-account send counter (`Account::num_sends`). /// -/// Regression for the seed-restore desync that surfaced as -/// `07-send.spec.ts::send-success` failing with -/// `"prev_commitment_pubkey required for account update"` (400). -/// The wallet derives both its current pubkey and -/// `prev_commitment_pubkey` from this counter; a stale `0` from the -/// balance endpoint sends the wallet into the wrong SMT slot or -/// omits the `prev` parameter when the server side expects it. +/// The wallet uses this counter to choose its next signing pubkey +/// (BIP-32 child index). `prev_commitment_pubkey` is no longer +/// derived from this counter — the server reads it directly from +/// `Account::commitment_public_key`. See the field doc on +/// `Account::commitment_public_key` for the bug class that change +/// eliminated (the wallet's local counter drifting from the server's +/// after a seed restore or stale-app deploy and surfacing as +/// `07-send.spec.ts::send-success` 400ing). /// /// Driven via the in-memory `AccountNode` knob rather than a full /// `/api/send` round-trip: prover initialisation alone costs ~50 s @@ -584,13 +585,15 @@ async fn balance_response_emits_num_sends_from_account() { let address_bytes = [0x77u8; 32]; let address = zkcoins_program::hash::digest_from_bytes(&address_bytes); - // Inject an account whose `proof` is None but `num_sends` is - // non-zero — an impossible production state (the invariant says - // `num_sends > 0 iff proof.is_some()`), but the handler does not - // re-check the invariant on read; it emits whatever the field - // holds. Setting `num_sends` directly is the smallest possible - // signal that the handler reads the right field. (The invariant - // itself is covered by the `account_node_tests` unit test + // Inject an account whose `proof` is None and + // `commitment_public_key` is None but `num_sends` is non-zero — + // an impossible production state (the invariant says + // `num_sends > 0 iff proof.is_some() iff commitment_public_key.is_some()`), + // but the handler does not re-check the invariant on read; it + // emits whatever the field holds. Setting `num_sends` directly + // is the smallest possible signal that the handler reads the + // right field. (The invariant itself is covered by the + // `account_node_tests` unit test // `test_send_coins_twice_from_same_account_uses_update_account`, // which exercises the real bump path through `send_coins_inner`.) { @@ -3169,12 +3172,21 @@ fn map_send_coins_error_unknown_account_address_is_404() { assert_eq!(body, "Unknown account address"); } +/// Historical `"prev_commitment_pubkey required for account update"` +/// 400 is unreachable as of the `Account::commitment_public_key` +/// refactor — the server reads the previous commitment pubkey from +/// its own state, and the `send_coins_inner` AccountUpdate branch no +/// longer consults the caller-supplied `prev_commitment_pubkey`. The +/// error string is no longer mapped, so it falls through the catch-all +/// 500 arm. The test pins THAT (i.e. "if some future regression +/// re-introduces this string, it must NOT be silently mapped to 400 +/// without also restoring the architectural choice it implies"). #[test] -fn map_send_coins_error_prev_commitment_pubkey_required_is_400() { +fn map_send_coins_error_legacy_prev_commitment_pubkey_string_is_unmapped_500() { let (status, body) = crate::router::map_send_coins_error("prev_commitment_pubkey required for account update"); - assert_eq!(status, StatusCode::BAD_REQUEST); - assert_eq!(body, "prev_commitment_pubkey required for account update"); + assert_eq!(status, StatusCode::INTERNAL_SERVER_ERROR); + assert_eq!(body, "internal error"); } #[test] diff --git a/node/tests/api_remote.rs b/node/tests/api_remote.rs index abe73c5d..ce77ee52 100644 --- a/node/tests/api_remote.rs +++ b/node/tests/api_remote.rs @@ -286,6 +286,20 @@ impl TestWallet { recipient: &str, amount: u64, timestamp: u64, + ) -> String { + self.sign_send_at(account_address, recipient, amount, timestamp, 0) + } + + /// Same as [`Self::sign_send`] but at an arbitrary BIP-32 child + /// index. Needed for the multi-send regression test that drives + /// `account.num_sends >= 2` against the live server. + fn sign_send_at( + &self, + account_address: &str, + recipient: &str, + amount: u64, + timestamp: u64, + idx: u32, ) -> String { let mut hasher = Sha256::new(); hasher.update(account_address.as_bytes()); @@ -294,7 +308,7 @@ impl TestWallet { hasher.update(timestamp.to_le_bytes()); let hash: [u8; 32] = hasher.finalize().into(); let msg = Message::from_digest(hash); - let sig = self.secp.sign_schnorr_no_aux_rand(&msg, &self.keypair(0)); + let sig = self.secp.sign_schnorr_no_aux_rand(&msg, &self.keypair(idx)); hex::encode(sig.as_ref()) } @@ -1919,6 +1933,228 @@ async fn balance_response_num_sends_starts_zero_and_bumps_on_send() { ); } +/// Regression: the second `/api/send` for an account whose +/// `account.proof = Some(...)` MUST succeed even when the client +/// omits `prev_commitment_pubkey` from the request body. +/// +/// Pre-`Account::commitment_public_key`-refactor this surfaced as a +/// 400 `prev_commitment_pubkey required for account update` — +/// observed live as `07-send.spec.ts::send-success` failing with +/// `Interner Fehler: Vorheriger Public Key fehlt.` against DEV every +/// time the wallet's local BIP-32 child-index counter drifted from +/// the server's (seed restore + stale-app deploy + TOCTOU between +/// balance fetch and signing). Post-refactor the server reads the +/// previous commitment pubkey from `account.commitment_public_key` +/// (set atomically with `proof` inside `send_coins_inner`), so the +/// caller-supplied field is purely advisory and a missing one is +/// fully recoverable. +/// +/// Flow: mint → send #1 (first send, `account.proof = None` → prove +/// initial → server stamps `commitment_public_key = pubkey_0`) → +/// send #2 with `prev_commitment_pubkey` deliberately omitted → +/// MUST succeed (AccountUpdate branch reads its own stored value). +#[tokio::test] +async fn second_send_succeeds_without_prev_commitment_pubkey_field() { + let client = http_client(); + let alice = TestWallet::new(); + let bob = TestWallet::new(); + + assert_minting_balance_in_bounds(&client).await; + + // ---- Mint ---- + let mint_resp = client + .post(url("/api/mint")) + .json(&json!({ + "account_address": alice.address_hex(), + "amount": MINT_AMOUNT, + })) + .send() + .await + .expect("POST /api/mint"); + assert_eq!(mint_resp.status(), StatusCode::OK, "mint must succeed"); + let mint_body: Value = mint_resp.json().await.expect("mint body JSON"); + let mint_proof_id = mint_body["proof_id"].as_u64().expect("mint proof_id"); + + let _ = poll_balance_at_least(&client, &alice.address_hex(), MINT_AMOUNT).await; + + // ---- Fetch the mint proof; capture the minting pubkey for the + // FIRST send's `prev_commitment_pubkey`. (The first send hits the + // `prove_initial` branch and ignores the field, but we pass it + // anyway to mirror the "what an old wallet would send" shape.) + let proof_resp = client + .get(url(&format!("/api/proof/{}", mint_proof_id))) + .send() + .await + .expect("GET mint proof"); + let proof_bytes = proof_resp.bytes().await.expect("mint proof bytes"); + let mint_coin_proof: CoinProof = bincode::deserialize(&proof_bytes).expect("decode CoinProof"); + let prev_pk_minting = mint_coin_proof + .commitment + .as_ref() + .expect("mint coin proof has commitment") + .public_key; + + // ---- First send (proves initial, sets account.proof = Some + commitment_public_key = pubkey_0) ---- + let ts1 = unix_now(); + let sig1 = alice.sign_send(&alice.address_hex(), &bob.address_hex(), SEND_AMOUNT, ts1); + let send1_resp = client + .post(url("/api/send")) + .json(&json!({ + "account_address": alice.address_hex(), + "recipient": bob.address_hex(), + "amount": SEND_AMOUNT, + "public_key": hex::encode(alice.pubkey(0).serialize()), + "next_public_key": hex::encode(alice.pubkey(1).serialize()), + "prev_commitment_pubkey": hex::encode(prev_pk_minting.serialize()), + "signature": sig1, + "timestamp": ts1, + })) + .send() + .await + .expect("POST /api/send #1"); + assert_eq!( + send1_resp.status(), + StatusCode::OK, + "first send must succeed" + ); + let send1_body: Value = send1_resp.json().await.expect("send #1 body JSON"); + let send1_proof_id = send1_body["proof_id"].as_u64().expect("send #1 proof_id"); + let ash1_hex = send1_body["account_state_hash"] + .as_str() + .expect("send #1 account_state_hash") + .to_string(); + let ocr1_hex = send1_body["output_coins_root"] + .as_str() + .expect("send #1 output_coins_root") + .to_string(); + + // Commit the first send so its commitment lands in the SMT — + // the second send's prev-commitment lookup needs it indexed. + let mut commit1_msg = Vec::with_capacity(64); + commit1_msg.extend_from_slice(&hex::decode(&ash1_hex).expect("ash1 hex")); + commit1_msg.extend_from_slice(&hex::decode(&ocr1_hex).expect("ocr1 hex")); + let commit1_sig = alice.sign_commit(&commit1_msg); + let commit1_resp = client + .post(url("/api/commit")) + .json(&json!({ + "proof_id": send1_proof_id, + "public_key": hex::encode(alice.pubkey(0).serialize()), + "signature": commit1_sig, + "message": hex::encode(&commit1_msg), + })) + .send() + .await + .expect("POST /api/commit #1"); + assert_eq!( + commit1_resp.status(), + StatusCode::OK, + "commit #1 must succeed" + ); + + // Verify the server bumped `num_sends` to 1 (the wallet would + // sync this on its next balance tick to choose `pubkey(1)` as + // its next signing key). + let post_send1 = client + .get(url(&format!( + "/api/balance?address={}", + alice.address_hex() + ))) + .send() + .await + .expect("GET /api/balance post-send-1"); + let post_send1_body: Value = post_send1.json().await.expect("balance body JSON"); + assert_eq!( + post_send1_body["num_sends"].as_u64(), + Some(1), + "post-send-1 num_sends must report 1" + ); + + // Mint a second time into Alice so she has balance for send #2 + // (after send #1, alice's balance is `MINT_AMOUNT - SEND_AMOUNT`, + // which is still enough for another SEND_AMOUNT — but minting + // again keeps the test symmetric with `send_commit_roundtrip`). + let _ = client + .post(url("/api/mint")) + .json(&json!({ + "account_address": alice.address_hex(), + "amount": MINT_AMOUNT, + })) + .send() + .await + .expect("POST /api/mint #2"); + let _ = poll_balance_at_least( + &client, + &alice.address_hex(), + MINT_AMOUNT - SEND_AMOUNT + MINT_AMOUNT, + ) + .await; + + // ---- Second send WITHOUT `prev_commitment_pubkey`. ---- + // + // The whole point of the refactor: the AccountUpdate branch reads + // `account.commitment_public_key` from its own state (set + // atomically with `proof` by send #1 above), so the caller can + // omit the field entirely and the prove still succeeds. Pre- + // refactor this returned 400 + // `"prev_commitment_pubkey required for account update"`. + // + // The wallet's signing key for this send is `pubkey(1)` because + // `num_sends == 1` (the server's authoritative counter); the + // `public_key` field on the request reflects that. + let ts2 = unix_now(); + let sig2 = alice.sign_send_at( + &alice.address_hex(), + &bob.address_hex(), + SEND_AMOUNT, + ts2, + 1, + ); + let send2_body = json!({ + "account_address": alice.address_hex(), + "recipient": bob.address_hex(), + "amount": SEND_AMOUNT, + "public_key": hex::encode(alice.pubkey(1).serialize()), + "next_public_key": hex::encode(alice.pubkey(2).serialize()), + // NOTE: `prev_commitment_pubkey` deliberately omitted from + // the payload. With the refactor this must NOT 400. + "signature": sig2, + "timestamp": ts2, + }); + let send2_resp = client + .post(url("/api/send")) + .json(&send2_body) + .send() + .await + .expect("POST /api/send #2 (no prev_commitment_pubkey)"); + let send2_status = send2_resp.status(); + let send2_body_text = send2_resp.text().await.unwrap_or_default(); + assert_eq!( + send2_status, + StatusCode::OK, + "second send WITHOUT prev_commitment_pubkey must succeed \ + (refactor: server reads its own stored commitment_public_key); \ + got {} body={}", + send2_status, + send2_body_text + ); + + // Server-side counter advanced. + let post_send2 = client + .get(url(&format!( + "/api/balance?address={}", + alice.address_hex() + ))) + .send() + .await + .expect("GET /api/balance post-send-2"); + let post_send2_body: Value = post_send2.json().await.expect("balance body JSON"); + assert_eq!( + post_send2_body["num_sends"].as_u64(), + Some(2), + "post-send-2 num_sends must report 2" + ); +} + // --------------------------------------------------------------------------- // Section 5 — error-envelope contract // From 7ddbc94122285ed2f0f5af53b2ebb1411da5d57a Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Thu, 28 May 2026 22:52:22 +0200 Subject: [PATCH 3/3] fix(state): canonical SMT value from 64-byte wallet commit messages (#135) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `State::update` previously inserted the BIP-340 message digest into the commitment SMT, which only happened to match the in-circuit `CommitmentMerkleProofs::commitment()` invariant for 32-byte canonical-digest messages (mint flow). The wallet wire format ships a 64-byte `account_state_hash || output_coins_root` concatenation, and `get_account_state_hash` returned `sha256(message)` for that shape — so the SMT leaf was sha256(asth||ocr) instead of the canonical `hash_concat(asth, ocr)`. The in-circuit SMT inclusion check rejected wallet-shaped commitments, surfacing as `prove_account_update_with_in_and_out_coins_and_sources failed` on the second send from any wallet-built account (`second_send_succeeds_without_prev_commitment_pubkey_field`, PR #132). Special-case the 64-byte message shape: split into `ash || ocr`, reinterpret each half via `digest_from_bytes`, and store `hash_concat(ash, ocr)`. The 32-byte canonical path round-trips through `digest_from_bytes` to the same canonical digest, so mint commitments keep working unchanged. Non-32/non-64 fixtures (test-only) keep the legacy sha256 fallback to avoid a tests-only refactor; production callers never produce that shape. Regression tests pin the contract that 64-byte wallet messages produce the in-circuit canonical SMT value and that the two on-the-wire shapes agree on the SMT entry over identical (ash, ocr) halves. No migration needed: stale wallet-commitment entries refer to account rows wiped by migration 0012 and are unreferenced. Signed-off-by: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> --- node/src/state.rs | 67 +++++++++++++++++--- node/src/state_tests.rs | 131 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 190 insertions(+), 8 deletions(-) diff --git a/node/src/state.rs b/node/src/state.rs index 83a6a587..be177e88 100644 --- a/node/src/state.rs +++ b/node/src/state.rs @@ -7,7 +7,7 @@ use shared::SECP256K1; use sqlx::PgPool; use std::collections::HashMap; use zkcoins_program::circuit::main::MMR_PROOF_PATH_LEN; -use zkcoins_program::hash::{hash_concat, HashDigest, ZERO_HASH}; +use zkcoins_program::hash::{digest_from_bytes, hash_concat, HashDigest, ZERO_HASH}; use zkcoins_program::merkle::merkle_mountain_range::{MMRProof, MerkleMountainRange}; use zkcoins_program::merkle::sparse_merkle_tree::{InclusionProof, SparseMerkleTree}; @@ -178,14 +178,65 @@ impl State { let key_bytes = commitment.public_key.serialize(); let key: [u8; 32] = bitcoin::hashes::sha256::Hash::hash(&key_bytes).to_byte_array(); - // Store the BIP-340 message digest (32 raw bytes) reinterpreted - // as a Poseidon `HashOut` — `digest_from_bytes` is the - // canonical inverse of `digest_to_bytes` (round-trip safe). - let message_bytes = commitment.get_account_state_hash(); - let message_data = zkcoins_program::hash::digest_from_bytes(&message_bytes); + // The SMT value is the canonical Poseidon combiner of the + // commitment's two halves: + // + // smt_value = hash_concat(asth, ocr) + // + // That is exactly what the in-circuit gadget reconstructs in + // `CommitmentMerkleProofs::commitment()` + // (`program-plonky2/src/inputs.rs`) and feeds back into the + // SMT inclusion check. Any other value here surfaces as the + // server-side `prove_account_update_*` failing on the second + // send from an account (the first end-to-end test that + // exercises a non-initial proof is + // `second_send_succeeds_without_prev_commitment_pubkey_field`, + // added in PR #132). + // + // The protocol ships two on-the-wire shapes for + // `Commitment.message`, and both must produce the canonical + // SMT value: + // + // * 64 bytes — wallet wire format + // (`zk-coins/app/rust/client/src/lib.rs::create_commitment`, + // mirrored by `TestWallet::sign_commit` in + // `node/tests/api_remote.rs`): raw concatenation + // `asth_bytes || ocr_bytes`. The Schnorr signature is + // over `sha256(message)` (see `Commitment::verify` in + // `shared/src/commitment.rs`), but the SMT value MUST + // ignore that signature digest and reconstruct the + // canonical Poseidon combiner over the two halves. + // * 32 bytes — mint flow (`ClientAccount::create_commitment` + // in `shared/src/lib.rs`): the already-canonical + // `digest_to_bytes(hash_concat(asth, ocr))`. Round-trips + // through `digest_from_bytes` and recovers the same + // canonical `hash_concat(asth, ocr)` digest the 64-byte + // path produces — so the two forms agree on the SMT + // entry, by construction. + // + // Any other length is a test-only fixture (existing + // `state_tests.rs` uses arbitrary byte slices to exercise + // the surrounding state machinery); production callers + // never produce that shape, so we preserve the legacy + // sha256-fallback path via `get_account_state_hash` rather + // than forcing a tests-only refactor. The SMT value on + // that path is opaque but consistent — fine for the test + // surface, never reached by deployed code. + let smt_value = if commitment.message.len() == 64 { + let mut ash_bytes = [0u8; 32]; + let mut ocr_bytes = [0u8; 32]; + ash_bytes.copy_from_slice(&commitment.message[..32]); + ocr_bytes.copy_from_slice(&commitment.message[32..]); + let ash = digest_from_bytes(&ash_bytes); + let ocr = digest_from_bytes(&ocr_bytes); + hash_concat(&ash, &ocr) + } else { + let message_bytes = commitment.get_account_state_hash(); + digest_from_bytes(&message_bytes) + }; - // Update the SMT with just the message - self.smt.insert(key, message_data)?; + // Update the SMT with the canonical commitment value. + self.smt.insert(key, smt_value)?; } // 2. Get the current SMT root diff --git a/node/src/state_tests.rs b/node/src/state_tests.rs index 7e6ecd23..abfc0730 100644 --- a/node/src/state_tests.rs +++ b/node/src/state_tests.rs @@ -918,3 +918,134 @@ fn derive_num_pubkeys_from_smt_panics_on_loop_bound_exceeded() { } let _ = derive_num_pubkeys_from_smt_with_bound(&xpriv, &smt, BOUND); } + +// ---- canonical SMT value from wallet-shaped commit message ---------------- + +/// A 64-byte wallet-shaped `Commitment.message` (raw +/// `account_state_hash || output_coins_root` concatenation, as built by +/// `zk-coins/app/rust/client/src/lib.rs::create_commitment` and mirrored +/// by `TestWallet::sign_commit` in `node/tests/api_remote.rs`) must end +/// up in the SMT as the canonical Poseidon combiner +/// `hash_concat(digest_from_bytes(ash), digest_from_bytes(ocr))`. +/// +/// This is the value the in-circuit `CommitmentMerkleProofs::commitment()` +/// (`program-plonky2/src/inputs.rs`) reconstructs and feeds back into +/// the SMT inclusion check. Storing the sha256 of the 64-byte message +/// (the legacy `get_account_state_hash` shape) caused +/// `prove_account_update_with_in_and_out_coins_and_sources` to reject +/// the second send from any wallet-built account — the e2e regression +/// is `second_send_succeeds_without_prev_commitment_pubkey_field` in +/// PR #132. +#[test] +fn update_with_64_byte_wallet_commitment_stores_canonical_hash_concat() { + let mut state = State::new(); + + // Build a 64-byte message: 32 ash bytes || 32 ocr bytes. Distinct + // byte patterns so the two halves can't accidentally agree. + let ash_bytes: [u8; 32] = [0xAAu8; 32]; + let ocr_bytes: [u8; 32] = [0xCCu8; 32]; + let mut message = Vec::with_capacity(64); + message.extend_from_slice(&ash_bytes); + message.extend_from_slice(&ocr_bytes); + assert_eq!(message.len(), 64); + + let secret_key = + SecretKey::from_str("000000000000000000000000000000000000000000000000000000000000000a") + .expect("Invalid key"); + let commitment = Commitment::new(&secret_key, message).expect("commitment"); + + state + .update(std::slice::from_ref(&commitment)) + .expect("update"); + + // Independently compute the canonical SMT value the in-circuit + // gadget reconstructs. + let expected = hash_concat( + &digest_from_bytes(&ash_bytes), + &digest_from_bytes(&ocr_bytes), + ); + + // Retrieve the stored leaf via the SMT inclusion proof and assert + // it equals the canonical value. + let key_bytes = commitment.public_key.serialize(); + let key: [u8; 32] = bitcoin::hashes::sha256::Hash::hash(&key_bytes).to_byte_array(); + let (_proof, stored) = state + .smt + .generate_inclusion_proof(&key) + .expect("inclusion proof for wallet commitment key"); + assert_eq!( + stored, expected, + "wallet 64-byte commit message must produce the canonical hash_concat(ash, ocr) SMT value" + ); +} + +/// Equivalence test: a 32-byte canonical-digest commit message (mint +/// flow shape, `ClientAccount::create_commitment` in `shared/src/lib.rs`) +/// and a 64-byte wallet-shape commit message over the SAME (ash, ocr) +/// pair must produce the SAME SMT entry. Documents that the two +/// on-the-wire shapes agree on the canonical SMT value, so a wallet +/// commitment and a mint commitment over identical halves are +/// indistinguishable from the SMT's perspective. +/// +/// Uses two distinct secret keys so both commitments coexist in the +/// same SMT — the assertion is on the stored leaf VALUES, not on the +/// keys. +#[test] +fn update_with_32_byte_canonical_commitment_stores_same_hash_concat_as_64_byte_form() { + let mut state = State::new(); + + let ash_bytes: [u8; 32] = [0x11u8; 32]; + let ocr_bytes: [u8; 32] = [0x22u8; 32]; + let ash = digest_from_bytes(&ash_bytes); + let ocr = digest_from_bytes(&ocr_bytes); + let canonical_digest = hash_concat(&ash, &ocr); + let canonical_bytes = zkcoins_program::hash::digest_to_bytes(&canonical_digest); + + // 32-byte canonical-digest form (mint flow shape). + let mint_secret = + SecretKey::from_str("000000000000000000000000000000000000000000000000000000000000000b") + .expect("invalid key"); + let mint_commitment = + Commitment::new(&mint_secret, canonical_bytes.to_vec()).expect("mint commitment"); + assert_eq!(mint_commitment.message.len(), 32); + + // 64-byte wallet wire form over the SAME (ash, ocr). + let mut wallet_message = Vec::with_capacity(64); + wallet_message.extend_from_slice(&ash_bytes); + wallet_message.extend_from_slice(&ocr_bytes); + let wallet_secret = + SecretKey::from_str("000000000000000000000000000000000000000000000000000000000000000c") + .expect("invalid key"); + let wallet_commitment = + Commitment::new(&wallet_secret, wallet_message).expect("wallet commitment"); + assert_eq!(wallet_commitment.message.len(), 64); + + state + .update(&[mint_commitment.clone(), wallet_commitment.clone()]) + .expect("update"); + + let mint_key: [u8; 32] = + bitcoin::hashes::sha256::Hash::hash(&mint_commitment.public_key.serialize()) + .to_byte_array(); + let wallet_key: [u8; 32] = + bitcoin::hashes::sha256::Hash::hash(&wallet_commitment.public_key.serialize()) + .to_byte_array(); + + let (_mint_proof, mint_stored) = state + .smt + .generate_inclusion_proof(&mint_key) + .expect("mint inclusion proof"); + let (_wallet_proof, wallet_stored) = state + .smt + .generate_inclusion_proof(&wallet_key) + .expect("wallet inclusion proof"); + + assert_eq!( + mint_stored, wallet_stored, + "32-byte canonical and 64-byte wallet commit-message forms must store the same SMT value" + ); + assert_eq!( + mint_stored, canonical_digest, + "stored SMT value must equal hash_concat(ash, ocr)" + ); +}