From 449455a4930bccc3b8e788f6f472bd0bab0752ff Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Thu, 28 May 2026 09:51:39 +0200 Subject: [PATCH] fix(account): server reads prev commitment pubkey from its own state 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 9f44c056..602bfeff 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 //