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..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 @@ -899,7 +906,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 +1079,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); } }; 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/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)" + ); +} 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 //