fix(account): track per-account send counter; emit via /api/balance - #129
Merged
Merged
Conversation
The wallet derives its BIP-32 child-index counter (`numPubkeys`) purely from local state, which a seed restore resets to 0 — even when the server holds `account.proof = Some(...)` from a previous session. The next send then either (a) omits `prev_commitment_pubkey` and gets `"prev_commitment_pubkey required for account update"` (400) from `send_coin_handler`, or (b) re-uses pubkey[0] and collides on the same SMT slot at commit time. Both modes surfaced as `app/e2e/07-send.spec.ts::send-success` failing with the mapped user-facing string `"Interner Fehler: Vorheriger Public Key fehlt."`. Add the authoritative counter server-side and surface it on the balance endpoint so the wallet hydrates `numPubkeys` from the source of truth on every balance tick: * `Account.num_sends: u32` — bumped atomically with `account.proof = Some(...)` inside `send_coins_inner`. The `num_sends > 0 iff proof.is_some()` invariant is documented on the field and enforced at the only mutation site. * `BalanceResponse.num_sends` — emitted unconditionally (default 0 for an unobserved address, matching `Account::new()`). Migration 0011 wipes the `accounts` table because the bincode shape is non-additive: a pre-PR blob ends after `balance: u64` and bincode reports "unexpected end of input" when the post-PR deserialiser tries to read the new `num_sends` field. The closed test env precedent for "wipe-and-replay accepts the dataloss" was set by 0010; persisted accounts are reconstructable from the on-chain commitment SMT + the MMR via the scanner-replay path. Tests: * `router_tests::balance_response_emits_num_sends_from_account` — verifies the handler emits the per-account counter. * `router_tests::balance_*` — assert `num_sends == 0` on all unobserved/zero-balance paths. * `api_remote::balance_response_num_sends_starts_zero_and_bumps_on_send` — value-bearing end-to-end check across fresh wallet → mint (no bump) → send (bump to 1) → commit (still 1).
TaprootFreak
added
the
ci:full
TaprootFreak
marked this pull request as ready for review
May 27, 2026 14:48
TaprootFreak
added
ci:full
8 tasks
TaprootFreak
added a commit
that referenced
this pull request
May 28, 2026
…132) 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
E2E
07-send.spec.ts::send-successfails consistently withInterner Fehler: Vorheriger Public Key fehlt.(serverprev_commitment_pubkey required for account update, 400).Root cause: the wallet tracks its BIP-32 child-index counter (
numPubkeys) locally.restoreSeedWallet(driven by every E2E worker) resets it to 0 — but the server-sideaccount.proofmay already beSome(...)from a previous session. The next send either omitsprev_commitment_pubkey(→ 400) or re-uses pubkey[0] and collides on the SMT slot.Fix: server owns the counter authoritatively.
Account.num_sends: u32bumped atomically withaccount.proof = Some(...)insend_coins_inner(invariant:num_sends > 0 iff proof.is_some()).BalanceResponse.num_sendsreturned unconditionally; the wallet syncs its local counter on every balance tick (paired app PR).0011wipesaccounts— the bincode shape is non-additive; closed-test-env precedent set by 0010.The app side of this contract is in zk-coins/app PR #TBD (will link once opened). The app PR must wait for this one to land + deploy before merging — otherwise the wallet would read
undefinedfornum_sendsagainst the old server and we'd be back to square one.Behavioural changes
/api/balancenow always returnsnum_sends: number. Pre-PR clients ignore unknown fields (Zod.passthrough()in the app), so the field addition is non-breaking on the read side.Test plan
cargo fmt --checkcargo clippy -p node -p shared -- -D warningscargo clippy -p node --all-features -- -D warningscargo check --workspace --all-featuresci:fulllabel set for full coverage gate +api_remotesuite via deploy-dev)api_remote::balance_response_num_sends_starts_zero_and_bumps_on_sendpasses against DEV after deploysend_commit_roundtrip_moves_balancecontinues to pass (no semantic change to the send flow)Bug filed via downstream E2E run
app#26514150867(2026-05-27 13:36 UTC).