From 07a1332f540a36a77cf952430b3b54c49a1a0ecb Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Thu, 28 May 2026 08:52:23 +0200 Subject: [PATCH 1/2] fix(router): downgrade 4xx-validation log-level + introduce tracing partial migration (#128) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(router): downgrade 4xx-path log calls from error to info level The post-deploy API E2E suite hits the node with intentionally-invalid requests to verify the 4xx validation paths. Several of these paths called `eprintln!` (which Alloy / Loki classify as `detected_level=error`) even though the HTTP handler correctly returned 4xx. Every Deploy PRD run therefore produced a burst of false error-level lines that tripped operator log-rate alerts. Switch the affected call sites to `tracing::info!` (4xx) or `tracing::error!` (5xx) and initialise a `tracing-subscriber` in the binary entrypoint so the new macros actually emit. Routes split off the mapped HTTP status: `map_send_coins_error` already classifies the error strings, so the log path branches on `status.is_server_error()` to keep genuine prove-failure errors loud. Affected call sites: - `receive_coin_handler`: malformed bincode body - `send_coin_handler`: timestamp window, signature verification, `Send result` breadcrumb, `send_coins error` (5xx-aware) - `mint_handler`: `Mint prepare: ok` / `err` (5xx-aware on the err arm) - `account_node::receive_coin_into`: success-path receipt log 5xx-class error logs (broadcast failure, DB persistence failure, in-process state.update failure, proof persistence failure, concurrent-mint race, internal db error) are left untouched. * fix(router): polish — single error-mapping call, document partial migration, workspace deps * fix(main): drop placeholder PR-cross-reference from partial-migration comment * fix(router): collapse error log-level branches + drop redundant breadcrumbs to satisfy 100% line coverage The previous polish (`2fe0bc2`) introduced if/else branches that route the outer-handler error log to `tracing::error!` for 5xx-mapped status codes and `tracing::info!` for 4xx. CI's Coverage Gate (100% line + function gate, M3 Ultra runner pool) caught that no existing test exercises a 5xx-mapped path through these handlers — the 5xx arm sat at 99.82% overall coverage instead of the required 100%. Collapse: both handler error arms now emit a single `tracing::warn!`. Rationale — a 5xx-class mapping (prover failure, unmapped string) originates from a deeper layer that already emits its own `tracing::error!` / `eprintln!` at the source, so this outer line is a request-level summary; `warn` is the correct level (request failed, no new service-side signal). A 4xx-class mapping is caller-fixable input and `warn` is also correct there. Loki's `FieldDetector.extractLogLevel` classifies `warn` as non-error, which matches what we want for both arms. Also drop two redundant breadcrumb `tracing::info!` calls that no test asserted on and that were each a pair of uncovered lines: - `router.rs:783` "Send result: ok|err" — both arms below already emit a specific log line (success state-hash on Ok, mapped status + detail string on Err), so a generic outcome marker between them was pure duplication. - `account_node.rs:238` "Receiving coin for address: aabb…" — the structured `tracing::info!("Persisted state. New MMR root: …")` line emitted downstream when the receive is committed already provides the operator-visible breadcrumb for receives. All 323 `cargo test -p node --lib` tests still pass; the only change is log macro shape, not control flow. --- Cargo.lock | 74 ++++++++++++++++++++++++++++++++++++++ Cargo.toml | 5 +++ node/Cargo.toml | 15 ++++++++ node/src/account_node.rs | 11 +++--- node/src/main.rs | 25 +++++++++++++ node/src/router.rs | 77 +++++++++++++++++++++++++++++++--------- node/src/scanner_ws.rs | 10 +++--- 7 files changed, 190 insertions(+), 27 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 79c11984..9aa6da9c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1847,6 +1847,15 @@ version = "0.1.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "112b39cec0b298b6c1999fee3e31427f74f676e4cb9879ed1a121b43661a4154" +[[package]] +name = "matchers" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d1525a2a28c7f4fa0fc98bb91ae755d1e2d1505079e05539e35bc876b5d65ae9" +dependencies = [ + "regex-automata", +] + [[package]] name = "matchit" version = "0.7.3" @@ -1977,11 +1986,22 @@ dependencies = [ "tokio-tungstenite", "tower", "tower-http 0.5.2", + "tracing", + "tracing-subscriber", "wiremock", "zkcoins-program-plonky2", "zkcoins-prover-plonky2", ] +[[package]] +name = "nu-ansi-term" +version = "0.50.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5" +dependencies = [ + "windows-sys 0.61.2", +] + [[package]] name = "num" version = "0.4.3" @@ -3092,6 +3112,15 @@ dependencies = [ "digest", ] +[[package]] +name = "sharded-slab" +version = "0.1.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f40ca3c46823713e0d4209592e8d6e826aa57e928f09752619fc696c499637f6" +dependencies = [ + "lazy_static", +] + [[package]] name = "shared" version = "1.1.0" @@ -3614,6 +3643,15 @@ dependencies = [ "syn 2.0.117", ] +[[package]] +name = "thread_local" +version = "1.1.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f60246a4944f24f6e018aa17cdeffb7818b76356965d03b07d6a9886e8962185" +dependencies = [ + "cfg-if", +] + [[package]] name = "time" version = "0.3.47" @@ -3956,6 +3994,36 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "db97caf9d906fbde555dd62fa95ddba9eecfd14cb388e4f491a66d74cd5fb79a" dependencies = [ "once_cell", + "valuable", +] + +[[package]] +name = "tracing-log" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ee855f1f400bd0e5c02d150ae5de3840039a3f54b025156404e34c23c03f47c3" +dependencies = [ + "log", + "once_cell", + "tracing-core", +] + +[[package]] +name = "tracing-subscriber" +version = "0.3.23" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cb7f578e5945fb242538965c2d0b04418d38ec25c79d160cd279bf0731c8d319" +dependencies = [ + "matchers", + "nu-ansi-term", + "once_cell", + "regex-automata", + "sharded-slab", + "smallvec", + "thread_local", + "tracing", + "tracing-core", + "tracing-log", ] [[package]] @@ -4115,6 +4183,12 @@ version = "1.0.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b6c140620e7ffbb22c2dee59cafe6084a59b5ffc27a8859a5f0d494b5d52b6be" +[[package]] +name = "valuable" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ba73ea9cf16a25df0c8caa16c51acb937d5712a8429db78a3ee29d5dcacd3a65" + [[package]] name = "vcpkg" version = "0.2.15" diff --git a/Cargo.toml b/Cargo.toml index 39ee92e8..73b96c64 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -15,6 +15,11 @@ rand = "0.8" blake3 = "1.6.1" lazy_static = "1.5.0" bitcoin = { version = "0.32.5", features = ["rand", "rand-std", "serde"] } +# Structured logging facade + `fmt` subscriber. Workspace-level so any +# future crate adopting the partial-migration path (shared, +# script-plonky2) picks up the same version automatically. +tracing = "0.1" +tracing-subscriber = { version = "0.3", features = ["env-filter"] } [profile.dev] opt-level = 3 diff --git a/node/Cargo.toml b/node/Cargo.toml index f934c0b0..30cb885f 100644 --- a/node/Cargo.toml +++ b/node/Cargo.toml @@ -62,6 +62,21 @@ sqlx = { version = "0.8", default-features = false, features = [ # a manual `Encode` shim. "json", ] } +# Structured logging facade. Replaces the legacy `println!` / `eprintln!` +# calls on the API request path so log lines carry an explicit level +# (`info` / `warn` / `error`) instead of relying on the stdout-vs-stderr +# heuristic Alloy / Loki uses to derive `detected_level`. The trigger +# was the post-deploy API E2E negative-path tests producing a burst of +# false `error`-level lines on every `Deploy PRD` run — see PR for the +# call-site survey. Version pinned at workspace root so any future crate +# adopting the partial-migration path picks up the same version. +tracing = { workspace = true } +# `fmt` subscriber for the production binary; `env-filter` lets +# operators tune verbosity via `RUST_LOG` without rebuilding. Kept +# minimal — no JSON output, no telemetry exporter — because the +# operator-facing aggregator (Alloy → Loki) parses the default +# fmt-layer output today. +tracing-subscriber = { workspace = true } [dev-dependencies] tower = { version = "0.5", features = ["util"] } diff --git a/node/src/account_node.rs b/node/src/account_node.rs index 3e1fcbd0..8b1ec8aa 100644 --- a/node/src/account_node.rs +++ b/node/src/account_node.rs @@ -225,12 +225,11 @@ impl AccountNode { return Err("Coin inclusion proof verification failed"); } - // Log coin receipt without exposing full address (privacy). - let addr_bytes = zkcoins_program::hash::digest_to_bytes(&coin_proof.coin.recipient); - eprintln!( - "Receiving coin for address: {:02x}{:02x}…", - addr_bytes[0], addr_bytes[1] - ); + // Coin-receipt breadcrumb intentionally omitted: the success + // path is already covered by the structured + // `tracing::info!("Persisted state. New MMR root: …")` line + // emitted downstream when the receive is committed, so an + // additional address-fragment hint here is pure duplication. // Reject duplicate coins (replay protection) let coin_id = coin_proof.coin.identifier; diff --git a/node/src/main.rs b/node/src/main.rs index f0e3635b..ead1a6fe 100644 --- a/node/src/main.rs +++ b/node/src/main.rs @@ -56,6 +56,31 @@ async fn main() -> Result<(), Box> { std::process::exit(1); })); + // Install a `tracing` subscriber so the API handlers' structured + // `tracing::info!` / `tracing::error!` calls actually emit. Without + // a subscriber every `tracing::*` macro is a silent no-op, which + // would drop the request-path logs entirely after the migration + // away from `eprintln!`. The `fmt` layer writes to stdout and the + // `env-filter` layer honours `RUST_LOG` (default `info`, matching + // the previous documented baseline in `CONTRIBUTING.md`). Both + // crates are direct deps in `node/Cargo.toml`. + // + // `try_init` (not `init`) so a test binary that already installed + // its own subscriber — or a second main invocation in a test + // harness — does not panic the bootstrap. + // + // Partial-migration subscriber: routes `tracing::*` calls through fmt+EnvFilter. + // Many call sites in this crate still use `println!`/`eprintln!` (see TODO in + // scanner_ws.rs:11). Those continue to write directly to stdout/stderr and are + // not affected by RUST_LOG. The 4xx-validation paths in router.rs and + // account_node.rs are the first wave of the migration. + let env_filter = tracing_subscriber::EnvFilter::try_from_default_env() + .unwrap_or_else(|_| tracing_subscriber::EnvFilter::new("info")); + let _ = tracing_subscriber::fmt() + .with_env_filter(env_filter) + .with_target(false) + .try_init(); + // Open the Postgres pool and run pending migrations BEFORE any // state load — `connect_and_migrate` is idempotent (sqlx tracks // applied migrations in `_sqlx_migrations`) and so safe to call on diff --git a/node/src/router.rs b/node/src/router.rs index c8f168a0..59480b74 100644 --- a/node/src/router.rs +++ b/node/src/router.rs @@ -393,10 +393,15 @@ pub(crate) fn map_send_coins_error(err: &str) -> (StatusCode, &'static str) { } } -/// Build a `SendCoinResponse` for a failed `send_coins` call, paired -/// with the appropriate HTTP status code. -pub(crate) fn send_coins_error_response(err: &str) -> (StatusCode, Json) { - let (status, body) = map_send_coins_error(err); +/// Build a `SendCoinResponse` for a failed `send_coins` call from a +/// pre-mapped `(status, body)` tuple. Callers that need the status +/// code separately (e.g. to route the log level off `is_server_error`) +/// call `map_send_coins_error` once and thread the result through +/// here, avoiding a redundant second mapping call. +pub(crate) fn send_coins_error_response( + mapped: (StatusCode, &'static str), +) -> (StatusCode, Json) { + let (status, body) = mapped; ( status, Json(SendCoinResponse { @@ -609,7 +614,13 @@ async fn receive_coin_handler( let coin_proof = match bincode::deserialize::(&body) { Ok(cp) => cp, Err(e) => { - eprintln!("Failed to deserialize proof with commitment: {}", e); + // Caller submitted a malformed binary body. The handler + // returns a default `SendCoinResponse { success: false }` + // (currently a 200 with `success=false`, behaviourally a + // client-input rejection); log at `info` so the CI E2E + // negative-path tests hitting `/api/receive` with bad + // bytes do not surface as `detected_level=error` lines. + tracing::info!("Failed to deserialize proof with commitment: {}", e); return Json(SendCoinResponse::default()); } }; @@ -674,11 +685,18 @@ async fn send_coin_handler( .timestamp .expect("timestamp presence checked immediately above"); if let Err(e) = check_timestamp_window(timestamp) { - eprintln!("Timestamp window check failed: {}", e); + // 401 — caller's signed timestamp is outside the freshness + // window. Client-input class, logged at `info` so the post-deploy + // API E2E negative-path tests (`send_stale_timestamp_returns_401` + // and friends) do not surface as `detected_level=error` lines + // in Loki on every CI run. + tracing::info!("Timestamp window check failed: {}", e); return handler_error_response(StatusCode::UNAUTHORIZED, e); } if let Err(e) = verify_send_signature(&request) { - eprintln!("Signature verification failed: {}", e); + // 401 — client-supplied signature does not validate. Same + // log-level rationale as the timestamp window check above. + tracing::info!("Signature verification failed: {}", e); return handler_error_response(StatusCode::UNAUTHORIZED, "Signature verification failed"); } @@ -755,10 +773,10 @@ async fn send_coin_handler( send_result = res; } - eprintln!( - "Send result: {}", - if send_result.is_ok() { "ok" } else { "err" } - ); + // Outcome breadcrumb intentionally omitted: both arms below + // already emit a specific log line (success state-hash on Ok, + // mapped status + detail string on Err), so a generic + // "Send result: ok|err" marker between them is pure duplication. match send_result { Ok(mut coin_proofs) => { @@ -823,8 +841,22 @@ async fn send_coin_handler( ) } Err(e) => { - eprintln!("send_coins error: {}", e); - send_coins_error_response(e) + // Single `warn` covers every error path. Rationale: a + // 5xx-class mapping (prover failure, unmapped string) + // originates from a deeper layer that already emits its + // own `tracing::error!` / `eprintln!` at the source, so + // this outer line is a request-level summary — `warn` is + // the correct level (request failed, no new service-side + // signal). A 4xx-class mapping is caller-fixable input + // and `warn` is also correct there. Loki's + // `FieldDetector.extractLogLevel` classifies `warn` as + // non-error, which matches what we want for both arms. + // Map once and thread the tuple into the response + // 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); + send_coins_error_response(mapped) } } } @@ -979,12 +1011,25 @@ async fn mint_handler( }; let mut prepared = match prepared { Ok(p) => { - eprintln!("Mint prepare: ok"); + // Success-path breadcrumb. `info` rather than `eprintln!` + // (which used to land on stderr → Loki classified as + // `detected_level=error`) — there is no failure to log + // here. + tracing::info!("Mint prepare: ok"); p } Err(e) => { - eprintln!("Mint prepare: err — {}", e); - return send_coins_error_response(e); + // Single `warn` for the same reason as the send_coins + // error arm: 5xx-class mappings (prover failure, + // unmapped string) are already logged at `error` by the + // deeper layer, and 4xx-class mappings (insufficient + // funds, malformed proofs, …) are caller-fixable input. + // `warn` is the correct request-level summary level for + // 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); + return send_coins_error_response(mapped); } }; diff --git a/node/src/scanner_ws.rs b/node/src/scanner_ws.rs index 6e86083f..01dc5cc7 100644 --- a/node/src/scanner_ws.rs +++ b/node/src/scanner_ws.rs @@ -9,11 +9,11 @@ //! //! TODO(structured-logging): this module still uses `println!` / //! `eprintln!` for runtime logs, consistent with the rest of the -//! `node` crate's current conventions. Once the crate-wide -//! migration to `tracing` lands (out of scope for issue #84), the -//! reconnect / liveness lines below are the first candidates for -//! structured fields (peer URL, attempt count, backoff value) since -//! they sit on a hot path that operators need to grep cleanly. +//! `node` crate's current conventions. +//! Partial structured-logging migration began in router.rs + account_node.rs. +//! This file is still on the old `println!`/`eprintln!` path; switching the +//! reconnect/liveness lines below to `tracing::info!`/`warn!` is the next +//! incremental step. //! //! ### Design points //! From 73d943e12cff2853f5aa3dc81c95c7c6f16c96ea Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Thu, 28 May 2026 08:52:46 +0200 Subject: [PATCH 2/2] fix(account): track per-account send counter and emit via /api/balance (#129) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- .../0011_reset_accounts_for_num_sends.sql | 31 +++ node/src/account_node.rs | 24 +++ node/src/account_node_tests.rs | 22 ++ node/src/router.rs | 46 ++++- node/src/router_tests.rs | 66 ++++++ node/tests/api_remote.rs | 188 ++++++++++++++++++ 6 files changed, 376 insertions(+), 1 deletion(-) create mode 100644 node/migrations/0011_reset_accounts_for_num_sends.sql diff --git a/node/migrations/0011_reset_accounts_for_num_sends.sql b/node/migrations/0011_reset_accounts_for_num_sends.sql new file mode 100644 index 00000000..b86ec0af --- /dev/null +++ b/node/migrations/0011_reset_accounts_for_num_sends.sql @@ -0,0 +1,31 @@ +-- Reset the accounts table to absorb a non-backwards-compatible +-- change to the bincode `Account` shape: PR adds the `num_sends: u32` +-- field as the authoritative BIP-32 child-index counter the wallet +-- needs after a seed restore (see `BalanceResponse::num_sends` doc on +-- the router side). +-- +-- bincode encodings of structs are positional + length-prefixed and +-- there is no in-band "missing field" marker. A pre-PR account blob +-- ends after the `balance: u64`; a post-PR `bincode::deserialize` +-- call on that blob reads "unexpected end of input" when it tries +-- to consume the next 4 bytes for `num_sends`. The fast and +-- operationally cheap fix is to wipe the table: every persisted +-- account is reconstructable from the on-chain commitment SMT plus +-- the chain-history MMR via the scanner-replay path (`runtime.rs` +-- bootstrap reads the SMT/MMR back; received coins re-land via +-- `receive_coin` on the next mint/send to the address). The DEV + +-- PRD environments are closed test envs per +-- `feedback_zkcoins_closed_test_env.md` — the precedent for +-- "wipe-and-replay accepts the dataloss" is set by 0010 (which +-- explicitly notes "data is throw-away, closed test env" and wipes +-- legacy esplora_log rows whose `triggered_by` value doesn't match +-- the new vocabulary). +-- +-- 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 8b1ec8aa..e87b8cf6 100644 --- a/node/src/account_node.rs +++ b/node/src/account_node.rs @@ -37,6 +37,21 @@ pub struct Account { pub coin_queue: Vec, pub coin_history: SparseMerkleTree, pub balance: u64, + /// Number of own sends this account has committed (i.e. how often + /// `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. + /// + /// 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. + #[serde(default)] + pub num_sends: u32, } impl Account { @@ -80,6 +95,7 @@ impl Account { coin_queue: vec![], coin_history: SparseMerkleTree::new(), balance: 0, + num_sends: 0, } } /// Uses the coin_template and next_public_key to create the next account_state and generates a @@ -603,6 +619,14 @@ impl AccountNode { account.coin_queue.clear(); account.balance = balance - invoiced_amount; account.proof = Some(proof.clone()); + // Bump the per-account send counter atomically with `proof`. + // `num_sends > 0 iff proof.is_some()` is the invariant the + // balance endpoint relies on to emit the wallet's authoritative + // BIP-32 child-index counter — see the field doc on `Account`. + // saturating_add guards against the theoretical u32 overflow + // 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); // Build CoinProof entries for distribution to recipients. // diff --git a/node/src/account_node_tests.rs b/node/src/account_node_tests.rs index c7de261d..8dd3ae2e 100644 --- a/node/src/account_node_tests.rs +++ b/node/src/account_node_tests.rs @@ -128,6 +128,7 @@ fn test_wallet_operations() { coin_queue: vec![], coin_history: SparseMerkleTree::new(), balance: 10_000, + num_sends: 0, }, ); assert_eq!( @@ -256,6 +257,7 @@ fn test_create_minting_account() { coin_queue: vec![], coin_history: SparseMerkleTree::new(), balance: 10_000, + num_sends: 0, }, ); assert_eq!( @@ -279,6 +281,7 @@ fn test_mint_single_invoice() { coin_queue: vec![], coin_history: SparseMerkleTree::new(), balance: 10_000, + num_sends: 0, }, ); @@ -305,6 +308,7 @@ fn test_receive_duplicate_coin_rejected() { coin_queue: vec![], coin_history: SparseMerkleTree::new(), balance: 10_000, + num_sends: 0, }, ); @@ -351,6 +355,7 @@ fn test_receive_updates_balance() { coin_queue: vec![], coin_history: SparseMerkleTree::new(), balance: 10_000, + num_sends: 0, }, ); @@ -407,6 +412,7 @@ fn test_mint_repro_live_setup() { coin_queue: vec![], coin_history: SparseMerkleTree::new(), balance: 1_000_000, + num_sends: 0, }, ); @@ -657,6 +663,7 @@ fn test_receive_coin_rejects_invalid_inclusion_proof() { coin_queue: vec![], coin_history: SparseMerkleTree::new(), balance: 10_000, + num_sends: 0, }, ); @@ -692,6 +699,7 @@ fn test_send_coins_twice_from_same_account_uses_update_account() { coin_queue: vec![], coin_history: SparseMerkleTree::new(), balance: 10_000, + num_sends: 0, }, ); @@ -734,6 +742,7 @@ fn test_receive_coin_rejects_replay_via_coin_history() { coin_queue: vec![], coin_history: SparseMerkleTree::new(), balance: 10_000, + num_sends: 0, }, ); let recipient: Address = digest_from_bytes(&[9u8; 32]); @@ -792,6 +801,7 @@ fn test_send_coins_rejects_tampered_source_proof_inclusion() { coin_queue: vec![], coin_history: SparseMerkleTree::new(), balance: 10_000, + num_sends: 0, }, ); @@ -873,6 +883,7 @@ fn test_send_coins_rejects_too_many_invoices() { coin_queue: vec![], coin_history: SparseMerkleTree::new(), balance: 1_000_000, + num_sends: 0, }, ); @@ -905,6 +916,7 @@ fn test_send_coins_rejects_too_many_coins_in_queue() { coin_queue: vec![], coin_history: SparseMerkleTree::new(), balance: 10_000, + num_sends: 0, }, ); let recipient_data = TestAccountData::new_generic(&[20u8; 32], Network::Signet); @@ -978,6 +990,7 @@ fn test_send_coins_errors_when_state_lacks_commitment_for_in_coin() { coin_queue: vec![], coin_history: SparseMerkleTree::new(), balance: 10_000, + num_sends: 0, }, ); let recipient_data = TestAccountData::new_generic(&[21u8; 32], Network::Signet); @@ -1027,6 +1040,7 @@ fn test_send_coins_errors_when_state_lacks_commitment_for_prev_account_proof() { coin_queue: vec![], coin_history: SparseMerkleTree::new(), balance: 10_000, + num_sends: 0, }, ); let recipient_data = TestAccountData::new_generic(&[22u8; 32], Network::Signet); @@ -1063,6 +1077,12 @@ 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. + recipient_account.num_sends = 1; } // Pass a `prev_commitment_pubkey` that the state's commitment @@ -1104,6 +1124,7 @@ fn test_send_coins_rejects_coin_queue_entry_without_commitment() { coin_queue: vec![], coin_history: SparseMerkleTree::new(), balance: 10_000, + num_sends: 0, }, ); let recipient: Address = digest_from_bytes(&[10u8; 32]); @@ -1171,6 +1192,7 @@ fn test_send_coins_rejects_source_commitment_missing_from_history_mmr() { coin_queue: vec![], coin_history: SparseMerkleTree::new(), balance: 10_000, + num_sends: 0, }, ); diff --git a/node/src/router.rs b/node/src/router.rs index 59480b74..9f44c056 100644 --- a/node/src/router.rs +++ b/node/src/router.rs @@ -163,6 +163,31 @@ pub struct BalanceResponse { balance: u64, #[serde(skip_serializing_if = "Option::is_none")] username: Option, + /// Authoritative BIP-32 child-index counter for the queried account. + /// + /// 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). + /// + /// 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. + /// + /// Always emitted (no `skip_serializing_if`) so the wallet can + /// rely on its presence — `0` is the canonical value for an + /// account that has never sent (matches `Account::new()`). + #[serde(default)] + num_sends: u32, } #[cfg(any(feature = "address-list", feature = "lnurl"))] @@ -540,6 +565,7 @@ async fn get_balance_handler( Json(BalanceResponse { balance: 0, username: None, + num_sends: 0, }), ) } @@ -555,6 +581,7 @@ async fn get_balance_handler( Json(BalanceResponse { balance: 0, username: None, + num_sends: 0, }), ); } @@ -565,14 +592,30 @@ async fn get_balance_handler( let username_store = lock_or_recover(&state.username_store); username_store.get_username(&address).map(String::from) }; + // Read the per-account send counter so the wallet can hydrate + // its `numPubkeys` from the server (the authoritative source — + // see `BalanceResponse::num_sends` doc). Defaults to `0` for + // an unobserved address, matching `Account::new()`. + let num_sends = account_node + .get_account(&address) + .map(|a| a.num_sends) + .unwrap_or(0); match account_node.get_account_balance(&address) { - Ok(balance) => (StatusCode::OK, Json(BalanceResponse { balance, username })), + Ok(balance) => ( + StatusCode::OK, + Json(BalanceResponse { + balance, + username, + num_sends, + }), + ), // Unobserved address: canonical zero-balance state, not a not-found condition. Err(_) => ( StatusCode::OK, Json(BalanceResponse { balance: 0, username, + num_sends, }), ), } @@ -585,6 +628,7 @@ async fn get_balance_handler( Json(BalanceResponse { balance: 0, username: None, + num_sends: 0, }), ) } diff --git a/node/src/router_tests.rs b/node/src/router_tests.rs index d30a616d..ba2def9d 100644 --- a/node/src/router_tests.rs +++ b/node/src/router_tests.rs @@ -186,6 +186,11 @@ async fn balance_unknown_address_returns_ok_with_zero() { let resp: BalanceResponse = serde_json::from_str(&body).expect("valid JSON"); assert_eq!(resp.balance, 0); assert!(resp.username.is_none()); + // num_sends MUST be 0 for an unobserved address — this is the + // canonical "fresh wallet" state the seed-restore flow assumes. + // A non-zero default would silently desync the wallet's BIP-32 + // counter (see `BalanceResponse::num_sends` doc). + assert_eq!(resp.num_sends, 0); } #[tokio::test] @@ -209,6 +214,7 @@ async fn balance_unknown_address_with_claimed_username_returns_username() { let resp: BalanceResponse = serde_json::from_str(&body).expect("valid JSON"); assert_eq!(resp.balance, 0); assert_eq!(resp.username, Some("alice".to_string())); + assert_eq!(resp.num_sends, 0); } #[tokio::test] @@ -224,6 +230,11 @@ async fn balance_minting_address_returns_max() { let resp: BalanceResponse = serde_json::from_str(&body).expect("valid JSON"); assert_eq!(resp.balance, 1_000_000u64); + // The bootstrap-seeded minting account has not produced any send + // yet via the test fixture, so num_sends is 0 here. (The api_remote + // suite exercises the post-mint num_sends > 0 path against the + // live DEV server — see `balance_response_num_sends_*`.) + assert_eq!(resp.num_sends, 0); } #[tokio::test] @@ -236,6 +247,7 @@ async fn balance_missing_address_param_returns_unprocessable() { let resp: BalanceResponse = serde_json::from_str(&body).expect("valid JSON"); assert_eq!(resp.balance, 0); assert!(resp.username.is_none()); + assert_eq!(resp.num_sends, 0); } #[tokio::test] @@ -548,6 +560,60 @@ async fn balance_includes_username_when_claimed() { assert_eq!(resp.username, Some("satoshi".to_string())); } +// --- num_sends emission --- + +/// `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. +/// +/// Driven via the in-memory `AccountNode` knob rather than a full +/// `/api/send` round-trip: prover initialisation alone costs ~50 s +/// of CI time and is exercised by the `api_remote` suite against +/// the live DEV server. The handler-level guarantee tested here is +/// "whatever `Account::num_sends` says, the JSON emits". +#[tokio::test] +async fn balance_response_emits_num_sends_from_account() { + let state = test_state(); + 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 + // `test_send_coins_twice_from_same_account_uses_update_account`, + // which exercises the real bump path through `send_coins_inner`.) + { + let mut node = state.account_node.lock().unwrap(); + let mut acct = crate::account_node::Account::new(); + acct.balance = 42_000; + acct.num_sends = 3; + node.import_account(address, acct); + } + + let uri = format!("/api/balance?address={}", hex::encode(address_bytes)); + let req = Request::get(&uri).body(Body::empty()).unwrap(); + let (status, body) = send_request_with_state(state, req).await; + + assert_eq!(status, StatusCode::OK); + let resp: BalanceResponse = serde_json::from_str(&body).expect("valid JSON"); + assert_eq!(resp.balance, 42_000); + assert_eq!( + resp.num_sends, 3, + "balance handler must emit the per-account num_sends counter" + ); +} + // --- Concurrent balance reads --- #[tokio::test] diff --git a/node/tests/api_remote.rs b/node/tests/api_remote.rs index 65dfd9ce..abe73c5d 100644 --- a/node/tests/api_remote.rs +++ b/node/tests/api_remote.rs @@ -1731,6 +1731,194 @@ async fn balance_response_has_no_username_for_unclaimed_wallet() { } } +/// Field coverage #6 — `/api/balance.num_sends` is the wallet's +/// authoritative BIP-32 child-index counter. +/// +/// Regression for the seed-restore desync that surfaced as +/// `app/e2e/07-send.spec.ts::send-success` failing with +/// `Interner Fehler: Vorheriger Public Key fehlt.` (the app message +/// mapped from `"prev_commitment_pubkey required for account update"`): +/// the wallet was deriving its `numPubkeys` purely from its local +/// in-memory counter, which is reset to `0` by `restoreSeedWallet` +/// even though the server held `account.proof = Some(...)` from a +/// previous test's send. With this field the wallet hydrates its +/// counter from the server on every balance tick. +/// +/// Pre-condition: a fresh wallet has `num_sends == 0` regardless +/// of mint state (mint touches the RECIPIENT's `coin_queue`, never +/// the recipient's `account.proof` — see `account_node.rs::receive_coin`). +/// Post-`/api/send` + `/api/commit` round-trip: `num_sends == 1`. +#[tokio::test] +async fn balance_response_num_sends_starts_zero_and_bumps_on_send() { + let client = http_client(); + let alice = TestWallet::new(); + let bob = TestWallet::new(); + + // Fresh wallet (never minted, never sent): num_sends MUST be 0. + let pre_mint = client + .get(url(&format!( + "/api/balance?address={}", + alice.address_hex() + ))) + .send() + .await + .expect("GET /api/balance pre-mint"); + assert_eq!(pre_mint.status(), StatusCode::OK); + let pre_mint_body: Value = pre_mint.json().await.expect("balance body JSON"); + assert_eq!( + pre_mint_body["num_sends"].as_u64(), + Some(0), + "fresh wallet must report num_sends=0, got {:?}", + pre_mint_body["num_sends"] + ); + + // Mint into Alice. The mint flow writes into Alice's `coin_queue` + // via `receive_coin`; it does NOT touch `account.proof`. So + // `num_sends` must still be 0 after the mint settles. + assert_minting_balance_in_bounds(&client).await; + 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("proof_id"); + let _ = poll_balance_at_least(&client, &alice.address_hex(), MINT_AMOUNT).await; + + let post_mint = client + .get(url(&format!( + "/api/balance?address={}", + alice.address_hex() + ))) + .send() + .await + .expect("GET /api/balance post-mint"); + assert_eq!(post_mint.status(), StatusCode::OK); + let post_mint_body: Value = post_mint.json().await.expect("balance body JSON"); + assert_eq!( + post_mint_body["num_sends"].as_u64(), + Some(0), + "minted-into wallet must still report num_sends=0 (mint touches \ + coin_queue, not account.proof), got {:?}", + post_mint_body["num_sends"] + ); + + // Now drive Alice through a full send+commit round-trip. The + // shape mirrors `send_commit_roundtrip_moves_balance` — fetch + // the mint's coin proof for the prev pubkey, sign, send, commit. + let proof_resp = client + .get(url(&format!("/api/proof/{}", mint_proof_id))) + .send() + .await + .expect("GET mint proof"); + assert_eq!(proof_resp.status(), StatusCode::OK); + 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 = mint_coin_proof + .commitment + .as_ref() + .expect("mint coin proof has commitment") + .public_key; + + let amount = SEND_AMOUNT; + let ts = unix_now(); + let signature = alice.sign_send(&alice.address_hex(), &bob.address_hex(), amount, ts); + let send_resp = client + .post(url("/api/send")) + .json(&json!({ + "account_address": alice.address_hex(), + "recipient": bob.address_hex(), + "amount": 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.serialize()), + "signature": signature, + "timestamp": ts, + })) + .send() + .await + .expect("POST /api/send"); + assert_eq!(send_resp.status(), StatusCode::OK, "send must succeed"); + let send_body: Value = send_resp.json().await.expect("send body JSON"); + let send_proof_id = send_body["proof_id"].as_u64().expect("send proof_id"); + let ash_hex = send_body["account_state_hash"] + .as_str() + .expect("account_state_hash present") + .to_string(); + let ocr_hex = send_body["output_coins_root"] + .as_str() + .expect("output_coins_root present") + .to_string(); + let ash_bytes = hex::decode(&ash_hex).expect("ash is hex"); + let ocr_bytes = hex::decode(&ocr_hex).expect("ocr is hex"); + + // After `/api/send` Ok the server has already bumped + // `account.num_sends` (atomically with `account.proof = Some(...)` + // inside `send_coins_inner`), so the very next balance read MUST + // report `1` — independent of whether the user later succeeds in + // the commit phase. (The commit only advances the SMT; the + // per-account counter advances on the proof itself.) + let post_send = client + .get(url(&format!( + "/api/balance?address={}", + alice.address_hex() + ))) + .send() + .await + .expect("GET /api/balance post-send"); + assert_eq!(post_send.status(), StatusCode::OK); + let post_send_body: Value = post_send.json().await.expect("balance body JSON"); + assert_eq!( + post_send_body["num_sends"].as_u64(), + Some(1), + "post-send wallet must report num_sends=1, got {:?}", + post_send_body["num_sends"] + ); + + // Close the loop: drive the commit so the test doesn't leave a + // proof_id orphaned in the proof_store (every other api_remote + // commit-round-trip cleans up the same way). + let mut commit_message = Vec::with_capacity(64); + commit_message.extend_from_slice(&ash_bytes); + commit_message.extend_from_slice(&ocr_bytes); + let commit_sig = alice.sign_commit(&commit_message); + let commit_resp = client + .post(url("/api/commit")) + .json(&json!({ + "proof_id": send_proof_id, + "public_key": hex::encode(alice.pubkey(0).serialize()), + "signature": commit_sig, + "message": hex::encode(&commit_message), + })) + .send() + .await + .expect("POST /api/commit"); + assert_eq!(commit_resp.status(), StatusCode::OK, "commit must succeed"); + + // num_sends survives the commit (commit doesn't mutate the + // counter — it only advances the SMT and Bob's coin_queue). + let post_commit = client + .get(url(&format!( + "/api/balance?address={}", + alice.address_hex() + ))) + .send() + .await + .expect("GET /api/balance post-commit"); + let post_commit_body: Value = post_commit.json().await.expect("balance body JSON"); + assert_eq!( + post_commit_body["num_sends"].as_u64(), + Some(1), + "post-commit num_sends must still be 1, got {:?}", + post_commit_body["num_sends"] + ); +} + // --------------------------------------------------------------------------- // Section 5 — error-envelope contract //