Skip to content

fix(account): server reads prev commitment pubkey from its own state - #132

Merged
TaprootFreak merged 2 commits into
developfrom
fix/server-owns-prev-commitment-pubkey
May 28, 2026
Merged

fix(account): server reads prev commitment pubkey from its own state#132
TaprootFreak merged 2 commits into
developfrom
fix/server-owns-prev-commitment-pubkey

Conversation

@TaprootFreak

Copy link
Copy Markdown
Contributor

Summary

07-send.spec.ts::send-success still fails on DEV after PR #129 deployed: Interner Fehler: Vorheriger Public Key fehlt. (server prev_commitment_pubkey required for account update, 400). The wallet's local BIP-32 child-index counter (numPubkeys) cannot be kept in sync with the server's persisted state across all the edge cases (seed restore, stale-app deploy, TOCTOU between balance fetch and request signing) — every desync surfaces as a 400.

Direct evidence — the failing send body captured live on dfxdev (request_log id 8132):

{
  \"account_address\": \"75bf7aa9…b2a8\",
  \"recipient\":       \"05c6…b438\",
  \"amount\": 1000,
  \"public_key\":      \"03d29d65…74c1\",       // pubkey at index 0 — alice's FIRST send used this
  \"next_public_key\": \"03cb5c61…360b\",
  \"signature\":       \"2dea93c1…\",
  \"timestamp\":       1779952953
  // ← no \`prev_commitment_pubkey\` field
}

Alice's server-side state was proof=Some, num_sends=1, balance=99000 — and the /api/balance call 0.65s earlier did return num_sends: 1. So either the wallet code did not consume the field, or it consumed it and still derived the wrong index. The deployed zk-coins/app:beta container is from 2026-05-25 — three days before PR #129 — and predates the app-side fix.

Fix

Move the lookup to where the data lives: the server stores
Account::commitment_public_key: Option<PublicKey> and the AccountUpdate branch of send_coins_inner reads it from there instead of from the caller-supplied prev_commitment_pubkey.

  • New field on Account, set atomically with proof + num_sends after a successful prove.
  • Invariant: proof.is_some() iff num_sends > 0 iff commitment_public_key.is_some().
  • prev_commitment_pubkey request field stays on the wire (for backwards-compat with deployed wallets that still emit it) but is ignored.
  • \"prev_commitment_pubkey required for account update\" is dropped from map_send_coins_error. The string is unreachable post-refactor; if a future regression re-introduces it, it falls through to the catch-all 500 — the pinned-and-renamed unit test surfaces it loudly.
  • Migration 0012 wipes accounts (same closed-test-env precedent as 0010 / 0011): the bincode shape is non-additive, AND 0011 left post-fix(account): track per-account send counter; emit via /api/balance #129 rows in the inconsistent proof=Some, commitment_public_key=None state that would panic the AccountUpdate branch's invariant expect.

Why this is the structurally right fix

The class of bugs this eliminates: every "client-side lookup-key derivation must stay in lockstep with server-persisted state across [seed restore | app deploy | TOCTOU race]" failure mode that surfaced as a 400 / SMT collision. The server already has authoritative knowledge of which pubkey the last commitment used — putting the lookup there removes the only synchronization point that could go stale.

The wallet's numPubkeys counter still matters for choosing the next SIGNING key (the field on the request that's actually security-load-bearing). That one is fine: the server's num_sends is its source of truth, and a stale value just means the wallet would re-use an already-used pubkey on the SMT — which produces a different, caller-fixable error (Unable to get merkle proofs for provided public key, 422), not a 400 with no recovery path.

Behavioural changes

  • /api/send AccountUpdate branch (i.e. account.proof.is_some()) succeeds even when the request body omits prev_commitment_pubkey or sends a stale one. The deployed zk-coins/app:beta on DEV stops 400ing without needing the app-side PR hotfix(migrations): revert ce4307c SQL comment edits to restore sqlx hash #125 (or any subsequent app deploy) to land.
  • The accounts table is wiped on deploy of this PR.

Test plan

  • cargo fmt --check
  • cargo clippy -p node -p shared -- -D warnings
  • cargo clippy -p node --all-features -- -D warnings
  • cargo check --workspace --all-features --tests
  • CI green (ci:full label set)
  • api_remote::second_send_succeeds_without_prev_commitment_pubkey_field passes against DEV after deploy
  • account_node_tests::test_send_coins_second_send_succeeds_without_prev_commitment_pubkey passes in the slim lane
  • 07-send.spec.ts::send-success passes against DEV on the app side (no app-side change required — the deployed app's undefined for prev_commitment_pubkey is now harmless)

Follow-up

  • zk-coins/app PR hotfix(migrations): revert ce4307c SQL comment edits to restore sqlx hash #125 can stay as-is (it now does redundant-but-harmless work). It can also be reduced to just the num_sendsnumPubkeys hydration for the SIGNING pubkey — the prev_commitment_pubkey derivation branch can be dropped.
  • Once every published wallet has cycled off emitting prev_commitment_pubkey, drop the field from SendCoinRequest entirely.

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<PublicKey>` 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.
@TaprootFreak TaprootFreak added ci:full Trigger heavy CI jobs (Server + Shared Tests + Coverage Gate, ~60-90 min on M3 Ultra) and removed ci:full Trigger heavy CI jobs (Server + Shared Tests + Coverage Gate, ~60-90 min on M3 Ultra) labels May 28, 2026
@TaprootFreak
TaprootFreak marked this pull request as ready for review May 28, 2026 07:53
@TaprootFreak TaprootFreak added ci:full Trigger heavy CI jobs (Server + Shared Tests + Coverage Gate, ~60-90 min on M3 Ultra) and removed ci:full Trigger heavy CI jobs (Server + Shared Tests + Coverage Gate, ~60-90 min on M3 Ultra) labels May 28, 2026
@TaprootFreak
TaprootFreak merged commit f4e2aa0 into develop May 28, 2026
7 checks passed
TaprootFreak added a commit that referenced this pull request May 28, 2026
…135)

`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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:full Trigger heavy CI jobs (Server + Shared Tests + Coverage Gate, ~60-90 min on M3 Ultra)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant