From b152d8b05e7e6cd6edfd8471432b471712c96a9b Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Sun, 24 May 2026 23:12:13 +0200 Subject: [PATCH 1/3] =?UTF-8?q?test:=20harden=20suite=20=E2=80=94=20remove?= =?UTF-8?q?=20dev=5Fskip=20masking=20+=20add=20publisher=20preflight?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The api_remote suite reported "33 passed" while critical paths were silently skipped via dev_skip!() on 5xx errors and 120-s retry loops on scanner-lag 422s. The Coverage Gate and the deploy-dev API E2E became a green stamp instead of a real signal — an empty publisher wallet caused every mint to 503, every 5xx was masked as "ok", and CI stayed green on a broken DEV. Tier 1 — must-fix: - Remove all dev_skip!() on is_server_error() (4 sites) - Remove dev_skip!() on /health/ready, balance-not-observed, and /api/username/claim 503 - Remove SEND_RETRY_DEADLINE retry loops on "Unable to get merkle/mmr proofs" 422 — scanner is event-driven post-#87, the stopgaps are obsolete. Replace with poll_until_balance before the send op (15-s ceiling). - Add fresh-state assertion to happy-path roundtrips - feature_skip!() becomes a hard panic when CI=true env is set - mint_handler_concurrent_mint_during_proof_returns_503 now synchronizes via a #[cfg(test)] tokio::sync::Notify instead of a 200-ms sleep - commit_with_valid_signature_fails_broadcast_returns_503 now wiremocks Esplora and asserts exactly 503 (no more accept-either) - fetch_capabilities .expect() instead of .unwrap_or(false) — a missing capabilities field is a contract regression - New /health/publisher endpoint exposes the publisher wallet's UTXO count + total sats - New deploy-dev preflight step probes /health/publisher before the API E2E job runs — empty wallet -> job fails with a clear "top up publisher" message Tier 2 — same-PR quality: - Value-bearing assertions replace .is_some()/.is_ok() shape checks in api_remote, server_tests, state_tests - Hash-byte-length + non-zero assertions on send response payloads - Concrete bounds on LNURLp min/maxSendable - tokio::time::sleep(60s) in test handlers replaced with std::future::pending::<()>().await - Ad-hoc tempdir cleanup replaced with tempfile::tempdir() - Delete proof_id_one_returns_200_or_404 — accept-either status was tautological Tier 3 — documentation: - Comment block on lock-poisoning tests' nextest isolation requirement The three TODO comments in account_server.rs (lines 147, 170, 416) are tracked separately and not addressed here. --- .github/workflows/deploy-dev.yaml | 40 +++ Cargo.lock | 1 + node/Cargo.toml | 8 +- node/src/publisher_tests.rs | 10 +- node/src/router.rs | 94 ++++++ node/src/router_tests.rs | 274 +++++++++++------ node/src/runtime.rs | 2 + node/src/runtime_tests.rs | 17 ++ node/src/scanner_ws_tests.rs | 24 +- node/src/state_tests.rs | 14 +- node/tests/api_remote.rs | 468 ++++++++++++++++++------------ 11 files changed, 656 insertions(+), 296 deletions(-) diff --git a/.github/workflows/deploy-dev.yaml b/.github/workflows/deploy-dev.yaml index c1f4c845..76621622 100644 --- a/.github/workflows/deploy-dev.yaml +++ b/.github/workflows/deploy-dev.yaml @@ -155,6 +155,46 @@ jobs: sccache --start-server >/dev/null 2>&1 || true sccache --show-stats + # Operational preflight: hit /health/ready and /health/publisher + # BEFORE running the API E2E suite, so an empty publisher wallet + # or a non-ready DB fails THIS step with a clear "top up the + # publisher" / "DB not ready" message instead of cascading + # through the test suite as opaque 503s. + # + # Historically a green E2E run masked an empty publisher wallet + # because the suite silently dev_skip!()'d 5xx errors; PR + # "test: harden suite" (this PR) removed the masking and added + # this preflight as the load-bearing operational gate. + # + # 50_000 sats is a conservative floor: a single inscription + # commit + reveal pair at typical Mutinynet fee rates needs + # ~1_500 sats; 50_000 buys ~30 mints before the next top-up. + # Adjust upward if the suite grows. + - name: Preflight — publisher wallet has UTXOs + env: + DEV_API: https://dev-api.zkcoins.app + run: | + set -euo pipefail + ready=$(curl -sS --max-time 10 "$DEV_API/health/ready") + if ! echo "$ready" | jq -e '.ready == true' > /dev/null; then + echo "::error::/health/ready not ready: $ready" + exit 1 + fi + pub=$(curl -sS --max-time 15 -w '|%{http_code}' "$DEV_API/health/publisher") + code="${pub##*|}" + body="${pub%|*}" + if [ "$code" != "200" ]; then + echo "::error::/health/publisher returned $code: $body" + exit 1 + fi + utxos=$(echo "$body" | jq -r '.utxo_count') + sats=$(echo "$body" | jq -r '.total_sats') + if [ "$utxos" -lt 1 ] || [ "$sats" -lt 50000 ]; then + echo "::error::publisher wallet too low (utxos=$utxos, sats=$sats) — top up before re-running" + exit 1 + fi + echo "publisher OK: utxos=$utxos, sats=$sats" + - name: Run API E2E suite against DEV run: cargo test -p node --release --all-features --test api_remote -- --test-threads=1 --nocapture diff --git a/Cargo.lock b/Cargo.lock index 7c4af4e2..f170096f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1969,6 +1969,7 @@ dependencies = [ "sha2", "shared", "sqlx", + "tempfile", "testcontainers", "testcontainers-modules", "tokio", diff --git a/node/Cargo.toml b/node/Cargo.toml index 8e8f458b..3b581a05 100644 --- a/node/Cargo.toml +++ b/node/Cargo.toml @@ -58,7 +58,7 @@ sqlx = { version = "0.8", default-features = false, features = [ [dev-dependencies] tower = { version = "0.5", features = ["util"] } http-body-util = "0.1" -# Used by `publisher_tests` for Esplora mocking and by `server_tests` +# Used by `publisher_tests` for Esplora mocking and by `router_tests` # to mock the Esplora HTTP endpoint behind the `/health/ready` # readiness probe so the tests never hit the real # `https://mutinynet.com/api` from CI. @@ -76,6 +76,12 @@ reqwest = { version = "0.12", default-features = false, features = ["json", "rus # run picks a fresh wallet and avoids collisions with concurrent # DEV-server consumers. rand = "0.8" +# Auto-cleaning scratch directories for the ProofStore tests in +# `router_tests`. Replaces the ad-hoc `std::env::temp_dir() + nanos +# + remove_dir_all().ok()` shape — the `TempDir` Drop impl removes +# the directory even when the test panics, so no test leaves a +# leaked /tmp/zkcoins-* tree behind. +tempfile = "3" [features] # All non-MVP features are off by default. When a feature is not enabled, the diff --git a/node/src/publisher_tests.rs b/node/src/publisher_tests.rs index 1fbfed80..a4fdf038 100644 --- a/node/src/publisher_tests.rs +++ b/node/src/publisher_tests.rs @@ -96,10 +96,12 @@ async fn spawn_track_tx_ws(mode: &'static str) -> String { } } } - // Hold the connection open so the publisher does not see - // a clean close before consuming the echo frame; the - // publisher's helper exits after the event arrives. - let _ = tokio::time::sleep(Duration::from_secs(60)).await; + // Hold the connection open until the test aborts the + // task. `std::future::pending` keeps the socket alive + // indefinitely so a slow CI runner can never let the + // helper observe a clean close before the event arrives; + // a bounded `sleep(60s)` could expire and mask a race. + std::future::pending::<()>().await; } }); url diff --git a/node/src/router.rs b/node/src/router.rs index b41473b9..02eb7999 100644 --- a/node/src/router.rs +++ b/node/src/router.rs @@ -92,6 +92,15 @@ pub(crate) struct AppState { /// clones `NETWORK_CONFIG` into this slot so the runtime /// behaviour is unchanged. pub(crate) esplora_config: Arc, + /// Test-only synchronisation primitive used by + /// `mint_handler_concurrent_mint_during_proof_returns_503`. The + /// production code path notifies via `notify_one()` after entering + /// phase 2 of `mint_handler` (after the `account_node` guard is + /// acquired) so the test can `.notified().await` deterministically + /// instead of `tokio::time::sleep(200ms)`. Hidden behind + /// `cfg(test)` so the field does not exist in release builds. + #[cfg(test)] + pub(crate) phase2_reached: Arc, } // Response types for our API @@ -846,6 +855,13 @@ async fn mint_handler( // ---- 2. PROOF phase (no mutation, clone-based) ----------------------- let prepared = { let account_node_guard = lock_or_recover(&state.account_node); + // Test-only barrier: notify any test waiting on + // `state.phase2_reached` that the handler has acquired the + // account_node guard and is about to invoke `prepare_mint`. + // Production builds compile this out entirely (the field does + // not exist in release). + #[cfg(test)] + state.phase2_reached.notify_one(); // get_minting_account_address borrows immutably below, fine. if account_node_guard .get_account(&zkcoins_program::types::MINTING_ADDRESS) @@ -1245,6 +1261,83 @@ async fn check_esplora( Ok(()) } +/// JSON body returned by `GET /health/publisher`. Surface enough state +/// for the deploy-dev preflight (and a curious operator) to make the +/// "should I top up the publisher wallet?" decision without scraping +/// Esplora directly. `address` is the publisher's Taproot bech32 — log- +/// only, NOT a secret (the matching key lives in `PUBLISHER_KEY`). +#[derive(Serialize)] +struct PublisherHealthResponse { + address: String, + utxo_count: u64, + total_sats: u64, +} + +/// Operational preflight (`GET /health/publisher`). +/// +/// Reads the publisher Taproot wallet's UTXO set via the configured +/// Esplora endpoint and reports `(address, utxo_count, total_sats)`. +/// The deploy-dev workflow probes this BEFORE running the API E2E +/// suite — an empty wallet would otherwise cause every mint to 503 +/// and historically masked as a "green" run because the E2E suite +/// itself silently treated 5xx as a skip. Returning 503 on an +/// Esplora-side error is intentional: the operator should see the +/// failure mode, not a fabricated empty response. +async fn publisher_health_handler(State(state): State) -> impl IntoResponse { + use bitcoin::secp256k1::{Secp256k1, SecretKey, XOnlyPublicKey}; + use std::str::FromStr; + + let publisher_key = &*crate::PUBLISHER_KEY; + let secp = Secp256k1::new(); + let sk = match SecretKey::from_str(publisher_key) { + Ok(sk) => sk, + Err(e) => { + return ( + StatusCode::INTERNAL_SERVER_ERROR, + Json(serde_json::json!({ + "error": "invalid PUBLISHER_KEY", + "detail": e.to_string(), + })), + ) + .into_response(); + } + }; + let key_pair = bitcoin::secp256k1::Keypair::from_secret_key(&secp, &sk); + let (xonly, _parity) = XOnlyPublicKey::from_keypair(&key_pair); + let network = state.esplora_config.network(); + let publisher_address = bitcoin::Address::p2tr(&secp, xonly, None, network); + + match crate::publisher::get_publisher_utxo(&publisher_address, &state.esplora_config, None) + .await + { + Ok(utxos) => { + let utxo_count = utxos.len() as u64; + let total_sats: u64 = utxos.iter().map(|(_, sats)| sats).sum(); + ( + StatusCode::OK, + Json( + serde_json::to_value(PublisherHealthResponse { + address: publisher_address.to_string(), + utxo_count, + total_sats, + }) + .expect("publisher health response serializes"), + ), + ) + .into_response() + } + Err(e) => ( + StatusCode::SERVICE_UNAVAILABLE, + Json(serde_json::json!({ + "error": "Esplora-side error fetching publisher UTXOs", + "detail": e.to_string(), + "address": publisher_address.to_string(), + })), + ) + .into_response(), + } +} + async fn info_handler() -> impl IntoResponse { Json(InfoResponse { network: NETWORK_CONFIG.network_name.clone(), @@ -1629,6 +1722,7 @@ pub(crate) fn create_router(state: AppState) -> Router { .route("/", get(root_handler)) .route("/health", get(|| async { "ok" })) .route("/health/ready", get(ready_handler)) + .route("/health/publisher", get(publisher_health_handler)) .route("/api/info", get(info_handler)) .route("/api/balance", get(get_balance_handler)) .route("/api/send", post(send_coin_handler)) diff --git a/node/src/router_tests.rs b/node/src/router_tests.rs index 910a4444..f1870a24 100644 --- a/node/src/router_tests.rs +++ b/node/src/router_tests.rs @@ -65,6 +65,7 @@ fn test_state() -> AppState { ws_url: None, track_tx_timeout: None, }), + phase2_reached: Arc::new(tokio::sync::Notify::new()), } } @@ -1591,7 +1592,9 @@ fn send_signature_accepts_valid_signature() { signature: Some(hex::encode(sig.serialize())), timestamp: Some(now), }; - assert!(verify_send_signature(&request).is_ok()); + // `.expect` surfaces the actual error string on failure; the + // previous `is_ok()` shape silently swallowed it. + verify_send_signature(&request).expect("valid Schnorr signature must verify"); } // --- POST /api/send (happy path, exercises the full handler) --- @@ -1678,17 +1681,34 @@ async fn send_with_valid_signature_returns_proof_id_and_hashes() { let response_json: serde_json::Value = serde_json::from_str(&body).expect("response is valid JSON"); assert_eq!(response_json["success"], true); + let proof_id = response_json["proof_id"] + .as_u64() + .expect("proof_id missing from response"); + assert!(proof_id > 0, "proof_id must be a positive u64"); + + // Value-bearing assertions on the send response payload. The + // previous `.as_str().is_some()` shape passed for any non-null + // string — including the all-zero placeholder a buggy handler + // could emit, or a truncated hex string. Decoding to bytes and + // asserting 32-byte length + non-zero pins both regressions. + let account_state_hash_hex = response_json["account_state_hash"] + .as_str() + .expect("account_state_hash present"); + let ash_bytes = hex::decode(account_state_hash_hex).expect("ash is hex"); + assert_eq!(ash_bytes.len(), 32, "account_state_hash must be 32 bytes"); assert!( - response_json["proof_id"].as_u64().is_some(), - "proof_id missing from response: {body}" - ); - assert!( - response_json["account_state_hash"].as_str().is_some(), - "account_state_hash missing: {body}" + ash_bytes.iter().any(|&b| b != 0), + "account_state_hash must be non-zero" ); + + let output_coins_root_hex = response_json["output_coins_root"] + .as_str() + .expect("output_coins_root present"); + let ocr_bytes = hex::decode(output_coins_root_hex).expect("ocr is hex"); + assert_eq!(ocr_bytes.len(), 32, "output_coins_root must be 32 bytes"); assert!( - response_json["output_coins_root"].as_str().is_some(), - "output_coins_root missing: {body}" + ocr_bytes.iter().any(|&b| b != 0), + "output_coins_root must be non-zero" ); } @@ -2218,6 +2238,7 @@ async fn send_with_insufficient_funds_returns_422_with_error_string() { ws_url: None, track_tx_timeout: None, }), + phase2_reached: Arc::new(tokio::sync::Notify::new()), }; let secret_bytes = include_bytes!("../minting_secret.bin"); @@ -2348,6 +2369,26 @@ async fn send_with_non_hex_recipient_returns_422() { assert_eq!(status, StatusCode::UNPROCESSABLE_ENTITY); } +// ----------------------------------------------------------------- +// `lock_or_recover_*` tests — nextest per-test process isolation note +// ----------------------------------------------------------------- +// +// The three `lock_or_recover_*_poisoned` tests below intentionally +// panic inside a spawned thread to poison the mutex they hold, then +// call `lock_or_recover` on the same `Arc>` to assert that +// the helper recovers the inner value via `into_inner`. Each test +// MUST run in its own process — under the default `cargo test` +// runner (single binary, threadpool) the second-test poison setup +// can race against the first test's recovery path because both +// share the libtest thread that observes panics. We rely on +// `cargo-nextest`'s per-test process isolation (see `CONTRIBUTING.md` +// > "Tests" and `.config/nextest.toml`) to give each test a fresh +// process. Running these tests outside nextest is supported (the +// project's CI uses `cargo nextest run`); a bare `cargo test` will +// occasionally surface a spurious "double panic" diagnostic in the +// shared libtest panic handler. Switch to nextest if you reproduce +// this locally. + #[test] fn lock_or_recover_recovers_from_poisoned_mutex() { let mutex = Arc::new(Mutex::new(42i32)); @@ -2374,7 +2415,60 @@ fn lock_or_recover_recovers_from_poisoned_mutex() { async fn commit_with_valid_signature_fails_broadcast_returns_503() { use bitcoin::bip32::{ChildNumber, Xpriv, Xpub}; use bitcoin::secp256k1::{Keypair, PublicKey, SecretKey}; - let state = test_state(); + use wiremock::matchers::{method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + // Spin up a wiremock Esplora that returns the publisher's UTXOs + // (so `get_publisher_utxo` finds inputs) but FAILS the broadcast + // with a 400. This pins the test to "valid signature, broadcast + // genuinely fails → 503" instead of "valid signature, broadcast + // might or might not succeed against a public Mutinynet". The + // previous accept-either assertion masked a hypothetical + // regression where the handler returned 200 without actually + // broadcasting. + let mock_server = MockServer::start().await; + let secp = secp::Secp256k1::new(); + let publisher_sk = SecretKey::from_slice( + &hex::decode("1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef").unwrap(), + ) + .expect("default publisher key parses"); + let publisher_kp = Keypair::from_secret_key(&secp, &publisher_sk); + let (publisher_xonly, _) = bitcoin::secp256k1::XOnlyPublicKey::from_keypair(&publisher_kp); + let publisher_address = + bitcoin::Address::p2tr(&secp, publisher_xonly, None, bitcoin::Network::Signet); + Mock::given(method("GET")) + .and(path(format!("/address/{}/utxo", publisher_address))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([ + { + "txid": "4444444444444444444444444444444444444444444444444444444444444444", + "vout": 0, + "value": 100_000, + "status": { + "confirmed": true, + "block_height": 100, + "block_hash": "0000000000000000000000000000000000000000000000000000000000000001", + "block_time": 1_700_000_000 + } + } + ]))) + .mount(&mock_server) + .await; + Mock::given(method("POST")) + .and(path("/tx")) + .respond_with( + ResponseTemplate::new(400).set_body_string("sendrawtransaction RPC error -25"), + ) + .mount(&mock_server) + .await; + + let mut state = test_state(); + state.esplora_config = Arc::new(crate::publisher::EsploraConfig { + url: mock_server.uri(), + is_mainnet: false, + network_name: "Mutinynet".to_string(), + ws_url: None, + track_tx_timeout: None, + }); let secret_bytes = include_bytes!("../minting_secret.bin"); let xpriv = Xpriv::new_master(bitcoin::Network::Signet, secret_bytes).unwrap(); @@ -2459,14 +2553,16 @@ async fn commit_with_valid_signature_fails_broadcast_returns_503() { .body(Body::from(commit_body.to_string())) .unwrap(); let (status, _) = send_request_with_state(state, commit_req).await; - // The commitment verifies, the handler proceeds to broadcast. Without - // a reachable Bitcoin node in the unit test environment, that call - // fails and the handler returns SERVICE_UNAVAILABLE. We accept either - // 503 (broadcast attempted and failed) or 200 (network was reachable - // and broadcast happened to succeed against a public Mutinynet). - assert!( - status == StatusCode::SERVICE_UNAVAILABLE || status == StatusCode::OK, - "expected 503 or 200, got {status}" + // The commitment verifies, the handler proceeds to broadcast. The + // wiremock Esplora rejects the broadcast (400) so the handler MUST + // return SERVICE_UNAVAILABLE. Anything else means the handler + // either bypassed the broadcast (a regression — it should always + // attempt it on a valid commitment) or fabricated a 200 response + // despite the upstream failure (a worse regression). + assert_eq!( + status, + StatusCode::SERVICE_UNAVAILABLE, + "expected 503 from valid-commit + broken-broadcast, got {status}" ); } @@ -2488,14 +2584,10 @@ fn proof_store_proof_path_returns_none_for_nonexistent_directory() { #[test] fn proof_store_new_picks_up_max_id_from_existing_files() { - let dir = std::env::temp_dir().join(format!( - "zkcoins-proof-store-max-{}", - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap() - .as_nanos() - )); - std::fs::create_dir_all(&dir).unwrap(); + // `tempfile::tempdir` removes the directory on Drop even when the + // test panics, so no /tmp/zkcoins-* tree leaks on failure. + let tmp = tempfile::tempdir().expect("create tempdir"); + let dir = tmp.path(); // Drop a few well-formed and one malformed filename. std::fs::write(dir.join("3.bin"), b"placeholder").unwrap(); std::fs::write(dir.join("17.bin"), b"placeholder").unwrap(); @@ -2506,8 +2598,6 @@ fn proof_store_new_picks_up_max_id_from_existing_files() { // next_id starts at max(3, 17) + 1 = 18; the malformed names are skipped. let id = store.next_id.load(std::sync::atomic::Ordering::SeqCst); assert_eq!(id, 18); - - std::fs::remove_dir_all(&dir).ok(); } #[test] @@ -2524,18 +2614,11 @@ fn persist_proof_bytes_logs_error_when_write_fails() { #[test] fn persist_proof_bytes_succeeds_when_write_succeeds() { // Mirror test for the Ok arm so the helper is fully exercised. - let dir = std::env::temp_dir().join(format!( - "zkcoins-persist-{}", - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap() - .as_nanos() - )); - std::fs::create_dir_all(&dir).unwrap(); - let path = dir.join("99.bin"); + // `tempfile::tempdir` cleans up on Drop, even on test panic. + let tmp = tempfile::tempdir().expect("create tempdir"); + let path = tmp.path().join("99.bin"); ProofStore::persist_proof_bytes(&path, b"payload", 99); assert_eq!(std::fs::read(&path).unwrap(), b"payload"); - std::fs::remove_dir_all(&dir).ok(); } #[tokio::test] @@ -3351,6 +3434,7 @@ fn mint_test_state() -> AppState { ws_url: None, track_tx_timeout: None, }), + phase2_reached: Arc::new(tokio::sync::Notify::new()), } } @@ -3668,11 +3752,15 @@ async fn mint_happy_path_broadcasts_and_returns_proof_id() { assert_eq!(status, StatusCode::OK, "body: {}", resp_body); let v: serde_json::Value = serde_json::from_str(&resp_body).expect("valid JSON"); assert_eq!(v["success"], true); - assert!( - v["proof_id"].as_u64().is_some(), - "proof_id missing from response: {}", - resp_body - ); + // Fresh-state pinning: this is the FIRST proof emitted by the + // ProofStore in this test, so the id must be exactly 1. A drift + // (e.g. ProofStore counter not reset between tests, or proof_id + // generator reordered) would slip through a shape-only + // `is_some()` check. + let proof_id = v["proof_id"] + .as_u64() + .expect("proof_id missing from response"); + assert_eq!(proof_id, 1, "fresh-state mint must emit proof_id == 1"); // Per the mint_handler contract, the mint response intentionally // omits `account_state_hash` and `output_coins_root` (those are // returned by /api/send instead). @@ -3998,11 +4086,13 @@ async fn mint_receive_coin_failure_logs_and_returns_ok() { assert_eq!(status, StatusCode::OK, "body: {}", resp_body); let v: serde_json::Value = serde_json::from_str(&resp_body).expect("valid JSON"); assert_eq!(v["success"], true); - assert!( - v["proof_id"].as_u64().is_some(), - "proof_id missing from response: {}", - resp_body - ); + // Fresh-state pinning: this test's mint is the first to write a + // proof, so the id must be exactly 1. See the matching note in + // the happy-path mint test above. + let proof_id = v["proof_id"] + .as_u64() + .expect("proof_id missing from response"); + assert_eq!(proof_id, 1, "fresh-state mint must emit proof_id == 1"); } /// Retry-after-broadcast-failure (zk-coins/node#89). @@ -4109,10 +4199,19 @@ async fn mint_retry_after_broadcast_failure_succeeds() { assert_eq!(state.minting_account.lock().unwrap().num_pubkeys, 1); let recipient_digest = zkcoins_program::hash::digest_from_bytes(&recipient_bytes); { - let server_guard = state.account_node.lock().unwrap(); - assert!( - server_guard.get_account(&recipient_digest).is_some(), - "recipient account must be created on successful mint" + let node_guard = state.account_node.lock().unwrap(); + let recipient_account = node_guard + .get_account(&recipient_digest) + .expect("recipient account must be created on successful mint"); + // The second mint above credits `1u64`; the recipient's + // coin_queue must reflect exactly that single inflow. A + // shape-only `is_some()` previously masked a bug where the + // account row was inserted with an empty queue. + assert_eq!( + recipient_account.coin_queue.len(), + 1, + "recipient coin_queue must hold exactly the minted coin, got {:?}", + recipient_account.coin_queue.len() ); } } @@ -4242,40 +4341,42 @@ async fn concurrent_mint_during_proof_response_returns_503() { /// End-to-end race that drives the post-proof "concurrent mint /// detected during proof phase" branch of `mint_handler` through the /// HTTP layer so the `return concurrent_mint_during_proof_response(...)` -/// call site (router.rs ~L891) is covered, not just the helper. +/// call site (router.rs) is covered, not just the helper. /// -/// Synchronisation strategy (deterministic, not time-based): the test -/// pre-acquires the `state.account_node` mutex BEFORE issuing the -/// `/api/mint` request. The handler completes phase 1 (lock -/// `minting_account`, snapshot `expected_num_pubkeys = 0`, release) -/// and then blocks at phase 2 trying to lock `account_node`. While -/// the handler is parked on that lock, the test acquires -/// `state.minting_account` and bumps `num_pubkeys` to a non-matching -/// value, then drops the `account_node` guard. The handler proceeds -/// through phase 2 (prover work), reaches phase 3, re-locks +/// Synchronisation strategy (deterministic, NOT time-based): the +/// handler signals it has acquired the `state.account_node` guard +/// at the top of phase 2 via the test-only +/// `state.phase2_reached: Arc` field; the test +/// `.notified().await`s on it, then acquires `state.minting_account` +/// and bumps `num_pubkeys` to a non-matching value. The handler +/// proceeds through phase 2 (prover work), reaches phase 3, re-locks /// `minting_account`, observes the bumped counter, and returns 503 /// before ever touching the broadcast / Esplora / Postgres paths — /// so the bare `mint_test_state()` (dead pool, unreachable Esplora) /// is sufficient. /// +/// Previously this test used a 200 ms `tokio::time::sleep`, which +/// was both racy (a slow CI scheduler could let phase 2 enter and +/// finish before the bump landed) and opaque (a failure mode looked +/// like "test occasionally returns 200 instead of 503"). The Notify +/// barrier is a hard happens-before edge: the bump cannot run until +/// the handler has reached phase 2. +/// /// Requires the multi-thread runtime: phase 2's `prepare_mint` is /// blocking CPU work that would otherwise stall the single-threaded /// executor and prevent the test thread from running the bump step. -/// -/// `clippy::await_holding_lock` is silenced because holding the -/// `account_node` `MutexGuard` across the `sleep().await` IS the -/// synchronisation primitive — releasing it earlier would defeat the -/// test by letting phase 2 finish before the bump. -#[allow(clippy::await_holding_lock)] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn mint_handler_concurrent_mint_during_proof_returns_503() { let state = mint_test_state(); - // Pre-acquire the account_node lock so phase 2 of mint_handler - // parks until we release it. Phase 1 only touches - // `state.minting_account`, so the handler can still complete its - // snapshot (capturing expected_num_pubkeys = 0) before parking. - let account_node_guard = state.account_node.lock().unwrap(); + // Pre-subscribe to the phase-2 notify BEFORE spawning the request + // so a fast handler that acquires `account_node` and fires + // `notify_one()` immediately cannot lose the signal. `Notified` is + // a future created up-front; the `notify_one` call buffers the + // wake-up even when no one is currently awaiting, so dropping the + // `Notified` before the await would be unsound here. + let notified = state.phase2_reached.notified(); + tokio::pin!(notified); let recipient = "0x".to_string() + &hex::encode([7u8; 32]); let body = serde_json::json!({ @@ -4288,34 +4389,29 @@ async fn mint_handler_concurrent_mint_during_proof_returns_503() { .unwrap(); // Drive the request on a worker so we can manipulate state from - // this task while the handler is parked on the account_node - // mutex inside phase 2. + // this task while the handler runs. let state_for_request = state.clone(); let request_task = tokio::spawn(async move { send_request_with_state(state_for_request, req).await }); - // Give the handler a generous window to enter phase 2 and park on - // the account_node lock. Phase 1 is microseconds of work; 200ms - // is overkill but cheap. Note: we cannot rely on `lock().is_locked` - // because std::sync::Mutex offers no such API — but holding the - // guard here is enough, because phase 2 will block until we drop - // it regardless of when the handler arrives. - tokio::time::sleep(std::time::Duration::from_millis(200)).await; + // Wait until the handler signals it has acquired the + // `account_node` guard at the top of phase 2. Phase 1 + // (`minting_account` snapshot of `num_pubkeys = 0`) has finished + // by this point because it runs BEFORE phase 2 in `mint_handler`. + // This is a hard happens-before edge: the bump below cannot run + // until the handler is observably past the phase-1 snapshot. + notified.as_mut().await; // Now bump num_pubkeys on the minting_account. Phase 1 already // captured expected_num_pubkeys = 0, so any non-zero value here - // trips the phase-3 inequality check. + // trips the phase-3 inequality check. Phase 3 acquires the + // `minting_account` lock after the prover finishes; we hold the + // bump-mutating guard only briefly. { let mut minting = state.minting_account.lock().unwrap(); minting.num_pubkeys = 1; } - // Release the account_node lock so phase 2 can proceed. The - // handler now runs the prover, re-locks minting_account, observes - // num_pubkeys = 1 != expected 0, and returns 503 via - // `concurrent_mint_during_proof_response`. - drop(account_node_guard); - let (status, resp_body) = request_task.await.expect("request task panicked"); assert_eq!( diff --git a/node/src/runtime.rs b/node/src/runtime.rs index 0a3ca36d..9d33db71 100644 --- a/node/src/runtime.rs +++ b/node/src/runtime.rs @@ -140,6 +140,8 @@ pub async fn start_rest_node( // The readiness probe uses this to ping Esplora; in production // it points at the same `ESPLORA_URL` as the scanner / publisher. esplora_config: Arc::new(NETWORK_CONFIG.clone()), + #[cfg(test)] + phase2_reached: Arc::new(tokio::sync::Notify::new()), }; // Bootstrap the minting account if it isn't already in the DB. diff --git a/node/src/runtime_tests.rs b/node/src/runtime_tests.rs index a77098f6..271d901f 100644 --- a/node/src/runtime_tests.rs +++ b/node/src/runtime_tests.rs @@ -146,6 +146,23 @@ async fn start_rest_node_binds_and_serves_health() { "expected 200 on /health, got: {}", &resp[..resp.len().min(300)] ); + // `/health` is the documented liveness probe whose + // body is the literal string "ok" (see the route + // registration in `server::create_router`). A 200 + // status with a different body would still satisfy + // the old assertion but signal a regression in the + // contract Kuma watches. + let body = resp + .split("\r\n\r\n") + .nth(1) + .unwrap_or("") + .trim_end_matches('\0') + .trim(); + assert!( + body.starts_with("ok"), + "expected /health body to start with `ok`, got: {:?}", + body + ); return; } Err(e) => last_err = Some(e), diff --git a/node/src/scanner_ws_tests.rs b/node/src/scanner_ws_tests.rs index 8e1586a4..a363708e 100644 --- a/node/src/scanner_ws_tests.rs +++ b/node/src/scanner_ws_tests.rs @@ -97,10 +97,12 @@ async fn run_scanner_ws_publishes_blocks_from_server() { ); ws.send(WsMessage::Text(initial)).await.unwrap(); ws.send(WsMessage::Text(tip)).await.unwrap(); - // Hold the socket open so the scanner's anchor-on-reconnect - // path does not race; the test asserts on the channel and - // then drops the task. - let _ = tokio::time::sleep(Duration::from_secs(60)).await; + // Hold the socket open until the test aborts the task. A + // bounded `sleep(60s)` would silently expire on a slow CI + // runner and let the scanner observe a clean close, masking + // any race the test is trying to pin. `pending` has the + // identical "hold forever" semantic without the bound. + std::future::pending::<()>().await; }) .await; @@ -156,7 +158,9 @@ async fn run_scanner_ws_reconnects_after_server_close() { SAMPLE_BLOCK_HASH_HEX_2 ); ws2.send(WsMessage::Text(m2)).await.unwrap(); - tokio::time::sleep(Duration::from_secs(60)).await; + // Hold forever until the test aborts (see the matching note + // on the first sleep replacement above). + std::future::pending::<()>().await; }); let (tx, mut rx) = mpsc::channel::(8); @@ -229,7 +233,9 @@ async fn run_scanner_ws_force_reconnects_on_liveness_timeout() { SAMPLE_BLOCK_HASH_HEX_2 ); ws2.send(WsMessage::Text(m2)).await.unwrap(); - tokio::time::sleep(Duration::from_secs(60)).await; + // Hold forever until the test aborts (see the matching note + // on the first sleep replacement above). + std::future::pending::<()>().await; }); let (tx, mut rx) = mpsc::channel::(8); @@ -298,7 +304,8 @@ async fn subscribe_track_tx_then_wait_returns_when_peer_emits_txid() { txid_for_handler ); ws.send(WsMessage::Text(frame)).await.unwrap(); - tokio::time::sleep(Duration::from_secs(60)).await; + // Hold forever until the test aborts. + std::future::pending::<()>().await; }) .await }; @@ -319,7 +326,8 @@ async fn track_tx_wait_returns_timeout_when_event_never_arrives() { let url = spawn_ws_server(|mut ws| async move { // Consume the subscribe frame but never echo the event. let _ = ws.next().await; - tokio::time::sleep(Duration::from_secs(60)).await; + // Hold forever until the test aborts. + std::future::pending::<()>().await; }) .await; diff --git a/node/src/state_tests.rs b/node/src/state_tests.rs index 6cfdeec7..671eeb56 100644 --- a/node/src/state_tests.rs +++ b/node/src/state_tests.rs @@ -300,14 +300,12 @@ async fn test_get_commitment_proof_with_mmr() { // Update state with this commitment let mmr_root = state.update(std::slice::from_ref(&commitment)).unwrap(); - // Get the complete proof (SMT + MMR) - let proof_result = state.get_commitment_proof(&commitment.public_key); - assert!( - proof_result.is_ok(), - "Should return a valid proof for existing commitment" - ); - - let (commitment_msg, smt_proof, smt_root, mmr_proof) = proof_result.unwrap(); + // Get the complete proof (SMT + MMR). `.expect` itself asserts + // the Ok arm — a redundant `assert!(.is_ok())` before unwrap would + // double-emit on the same failure mode. + let (commitment_msg, smt_proof, smt_root, mmr_proof) = state + .get_commitment_proof(&commitment.public_key) + .expect("Should return a valid proof for existing commitment"); // Verify the message assert_eq!( diff --git a/node/tests/api_remote.rs b/node/tests/api_remote.rs index 93ae769a..e7b2dbb0 100644 --- a/node/tests/api_remote.rs +++ b/node/tests/api_remote.rs @@ -23,11 +23,11 @@ //! The DEV server is shared by other workflows (per-PR app E2E, //! interactive testing). To keep this suite race-free we always: //! - mint into freshly-generated wallets (no fixed addresses) -//! - tolerate `503 Service Unavailable` on mutating endpoints, -//! which the server returns when the Mutinynet publisher wallet -//! has no UTXOs — a benign DEV condition //! - assert strictly on 4xx codes (client-fixable contract bugs) -//! - skip on 5xx codes with a logged warning (server-side flake) +//! - assert strictly on 5xx codes as well (server-side regressions +//! are real bugs, not flakes — the deploy-dev preflight verifies +//! publisher wallet + /health/ready BEFORE this suite runs, so a +//! 503 here is unambiguous: it means something regressed) //! //! Read by: //! - `cargo test -p node --release --test api_remote` (locally) @@ -48,6 +48,8 @@ use serde_json::{json, Value}; use sha2::{Digest, Sha256}; use shared::commitment::Commitment; use std::time::{Duration, SystemTime, UNIX_EPOCH}; +use zkcoins_program::hash::digest_to_bytes; +use zkcoins_program::types::MINTING_ADDRESS; // --------------------------------------------------------------------------- // Constants @@ -57,18 +59,26 @@ const DEFAULT_API_URL: &str = "https://dev-api.zkcoins.app"; const HTTP_TIMEOUT: Duration = Duration::from_secs(120); const POLL_INTERVAL: Duration = Duration::from_secs(2); const POLL_TIMEOUT: Duration = Duration::from_secs(60); -/// How long to keep retrying the user-level `/api/send` while the -/// server reports "Unable to get merkle proofs for provided public -/// key" — see the inline comment in `send_commit_roundtrip_moves_balance` -/// for why this is timing-bound on the scanner picking up the mint's -/// Taproot inscription. Mutinynet block time is ~30 s, the scanner -/// polls every 30 s, so 2 minutes is enough on a healthy network -/// without dragging the suite past the workflow timeout when the -/// publisher is offline. -const SEND_RETRY_DEADLINE: Duration = Duration::from_secs(120); -const SEND_RETRY_INTERVAL: Duration = Duration::from_secs(15); +/// Inscription-landing budget for `poll_until_balance`. Post-#87 the +/// scanner is event-driven (WebSocket subscription on the Esplora +/// block stream, not a 30 s polling loop), so the wait time is bounded +/// by Mutinynet block time (~30 s) plus a small ingest margin. 15 s is +/// the worst-case healthy window between successive mints in this +/// suite; exceeding it indicates a real scanner-side regression +/// (lost subscription, dropped block, stuck SMT update) and is +/// asserted as a hard failure, never silently retried. +const SEND_RETRY_DEADLINE: Duration = Duration::from_secs(15); const MINT_AMOUNT: u64 = 50_000; const SEND_AMOUNT: u64 = 10_000; +/// Bootstrap balance seeded into the `MINTING_ADDRESS` account at +/// startup by `start_rest_node` (see `node::runtime`). +/// Must stay strictly less than `2^48` for Plonky2 Goldilocks safety +/// — see the matching constant guard in `runtime_tests`. The +/// happy-path roundtrips probe `/api/balance` on `MINTING_ADDRESS` +/// before their first mint and assert this exact value, so a dirty +/// DEV state (prior test residue, missed `reset_state` run) trips a +/// hard failure instead of being silently treated as "balance dipped". +const BOOTSTRAP_MINTING_BALANCE: u64 = 1u64 << 48; fn api_base() -> String { std::env::var("ZKCOINS_API_URL").unwrap_or_else(|_| DEFAULT_API_URL.to_string()) @@ -92,20 +102,23 @@ fn url(path: &str) -> String { format!("{}{}", api_base().trim_end_matches('/'), path) } -/// Helper: log a one-line "skip" with reason and return. -macro_rules! dev_skip { - ($reason:expr) => {{ - eprintln!("DEV environment skip: {}", $reason); - return; - }}; -} - -/// Helper: log a one-line "feature off" skip and return. Distinct -/// from [`dev_skip!`] so the workflow log line clearly marks "the -/// route is absent by design" vs. "the route is present but flaked -/// on the network". +/// Helper: log a one-line "feature off" skip and return. +/// +/// When running in CI (env `CI=true`) this is a hard panic instead of +/// a silent skip: CI is supposed to build with `--all-features`, so a +/// `feature_skip!` firing in CI is the canary for an accidentally +/// dropped `--all-features` flag in a workflow (e.g. someone copied +/// the local `cargo test` invocation into the workflow). Outside CI +/// the macro is still a skip — the suite is also runnable against a +/// feature-trimmed PRD deploy, where an absent route is expected. macro_rules! feature_skip { ($feature:expr, $test:expr) => {{ + if std::env::var("CI").is_ok() { + panic!( + "feature `{}` disabled but running in CI — all-features build is required", + $feature + ); + } eprintln!( "SKIP {}: feature `{}` disabled on this server", $test, $feature @@ -153,13 +166,22 @@ async fn fetch_capabilities(client: &reqwest::Client) -> Capabilities { .json() .await .expect("/api/info body is JSON for capability detection"); + // Each capability field MUST be a bool — a missing field or a + // non-bool value is a contract regression in `/api/info` and a + // `.unwrap_or(false)` would silently mask it as "feature off". let mut caps = Capabilities { - address_list: body["capabilities"]["address_list"] - .as_bool() - .unwrap_or(false), - faucet: body["capabilities"]["faucet"].as_bool().unwrap_or(false), - usernames: body["capabilities"]["usernames"].as_bool().unwrap_or(false), - lnurl: body["capabilities"]["lnurl"].as_bool().unwrap_or(false), + address_list: body["capabilities"]["address_list"].as_bool().expect( + "/api/info capabilities.address_list must be a bool — missing field is a contract regression", + ), + faucet: body["capabilities"]["faucet"].as_bool().expect( + "/api/info capabilities.faucet must be a bool — missing field is a contract regression", + ), + usernames: body["capabilities"]["usernames"].as_bool().expect( + "/api/info capabilities.usernames must be a bool — missing field is a contract regression", + ), + lnurl: body["capabilities"]["lnurl"].as_bool().expect( + "/api/info capabilities.lnurl must be a bool — missing field is a contract regression", + ), }; if let Ok(force) = std::env::var("ZKCOINS_FORCE_DISABLE_FEATURES") { for flag in force.split(',').map(|s| s.trim()).filter(|s| !s.is_empty()) { @@ -331,12 +353,12 @@ async fn health_ready_returns_ready_with_no_failures() { .expect("GET /health/ready"); let status = resp.status(); let body: Value = resp.json().await.expect("/health/ready body is JSON"); - if status != StatusCode::OK { - dev_skip!(format!( - "/health/ready returned {} with body {}", - status, body - )); - } + assert_eq!( + status, + StatusCode::OK, + "/health/ready must return 200 — failures: {:?}", + body["failures"] + ); assert_eq!(body["ready"], Value::Bool(true)); let failures = body["failures"].as_array().expect("failures is an array"); assert!( @@ -385,6 +407,42 @@ async fn info_returns_well_formed_response() { } } +/// Shape-only probe of `/health/publisher` — the JSON contract is +/// asserted here so the suite breaks if the field set changes, even +/// when the publisher wallet itself is empty (the deploy-dev +/// preflight separately enforces a non-zero UTXO count). 200 is +/// required: an Esplora-side error surfaces as 503 and we want that +/// to fail the suite, not be silently tolerated. +#[tokio::test] +async fn health_publisher_returns_well_formed_response() { + let resp = http_client() + .get(url("/health/publisher")) + .send() + .await + .expect("GET /health/publisher"); + assert_eq!( + resp.status(), + StatusCode::OK, + "/health/publisher must return 200 — anything else means Esplora is unreachable or the publisher route regressed" + ); + let body: Value = resp.json().await.expect("/health/publisher body is JSON"); + assert!( + body["address"].as_str().is_some_and(|v| !v.is_empty()), + "publisher address must be a non-empty string, got {:?}", + body["address"] + ); + assert!( + body["utxo_count"].as_u64().is_some(), + "utxo_count must be a u64, got {:?}", + body["utxo_count"] + ); + assert!( + body["total_sats"].as_u64().is_some(), + "total_sats must be a u64, got {:?}", + body["total_sats"] + ); +} + #[tokio::test] async fn balance_unknown_address_returns_ok_with_zero() { let address = format!("0x{}", "00".repeat(32)); @@ -466,34 +524,6 @@ async fn proof_for_huge_id_returns_404() { assert_eq!(resp.status(), StatusCode::NOT_FOUND); } -#[tokio::test] -async fn proof_id_one_returns_200_or_404() { - // proof_id=1 may exist (a prior test minted) or not (fresh state). - // Both 200 (binary) and 404 are valid; anything else is a regression. - let resp = http_client() - .get(url("/api/proof/1")) - .send() - .await - .expect("GET /api/proof/1"); - let status = resp.status(); - assert!( - status == StatusCode::OK || status == StatusCode::NOT_FOUND, - "proof/1 returned unexpected status: {}", - status - ); - if status == StatusCode::OK { - let bytes = resp.bytes().await.expect("body bytes"); - // A valid CoinProof bincode payload is at least a few hundred - // bytes (Plonky2 proof + commitment). 100 is a loose lower - // bound that just guards against an empty response. - assert!( - bytes.len() > 100, - "expected non-trivial CoinProof bytes, got {}", - bytes.len() - ); - } -} - #[tokio::test] async fn resolve_unknown_username_returns_404() { let client = http_client(); @@ -875,6 +905,14 @@ async fn mint_roundtrip_lands_balance_and_proof() { let client = http_client(); let alice = TestWallet::new(); + // Fresh-state guard: the deploy-dev workflow runs `reset_state` + // before this suite, so the minting account must be at the + // bootstrap balance untouched by any prior mint. A non-bootstrap + // value here means either the reset did not run, or another + // workflow racing on the shared DEV server has already minted. + // Either way the assertions below are no longer meaningful. + assert_minting_balance_is_bootstrap(&client).await; + let mint_resp = client .post(url("/api/mint")) .json(&json!({ @@ -885,12 +923,6 @@ async fn mint_roundtrip_lands_balance_and_proof() { .await .expect("POST /api/mint"); let mint_status = mint_resp.status(); - if mint_status.is_server_error() { - dev_skip!(format!( - "mint returned {} — DEV environment flake", - mint_status - )); - } assert_eq!(mint_status, StatusCode::OK, "unexpected mint status"); let mint_body: Value = mint_resp.json().await.expect("mint body JSON"); assert_eq!( @@ -936,56 +968,44 @@ async fn send_commit_roundtrip_moves_balance() { let alice = TestWallet::new(); let bob = TestWallet::new(); + // Fresh-state guard — mirror of the one in + // `mint_roundtrip_lands_balance_and_proof`. If the minting + // account is below bootstrap by any non-multiple of `MINT_AMOUNT`, + // the SMT shape is unknown and the assertions below are unsafe. + // Allow exactly bootstrap (suite ran first) OR + // bootstrap - MINT_AMOUNT (suite-A test 1 ran before this one in + // a single-test-thread invocation). + let minting_balance = fetch_minting_balance(&client).await; + assert!( + minting_balance == BOOTSTRAP_MINTING_BALANCE + || minting_balance == BOOTSTRAP_MINTING_BALANCE - MINT_AMOUNT, + "DEV state not fresh — minting balance {} not in expected set {{{}, {}}}; \ + last reset_state run did not run or failed; check deploy-dev workflow", + minting_balance, + BOOTSTRAP_MINTING_BALANCE, + BOOTSTRAP_MINTING_BALANCE - MINT_AMOUNT, + ); + // ---- Mint ---- - // 422 with "Unable to get merkle proofs for provided public key" is - // the documented signal that a PRIOR mint's on-chain Taproot - // inscription has not yet been observed by the scanner. This test - // runs sequentially after `mint_roundtrip_lands_balance_and_proof` - // in the single-threaded suite, so the second mint hits the - // server's "look up prev commitment" branch and depends on the - // scanner having caught up. Mutinynet block time is ≈30 s and the - // scanner polls Esplora on a 30 s interval — so until both delays - // elapse, the SMT does not know about the prev_commitment_pubkey - // the server needs to attach to this mint. Apply the same retry - // pattern that `/api/send` below uses for the same condition. - let mint_body_json = json!({ - "account_address": alice.address_hex(), - "amount": MINT_AMOUNT, - }); - let (mint_status, mint_body_text) = { - let deadline = std::time::Instant::now() + SEND_RETRY_DEADLINE; - loop { - let resp = client - .post(url("/api/mint")) - .json(&mint_body_json) - .send() - .await - .expect("POST /api/mint"); - let status = resp.status(); - let text = resp.text().await.unwrap_or_default(); - let should_retry = status == StatusCode::UNPROCESSABLE_ENTITY - && text.contains("Unable to get merkle proofs"); - if !should_retry || std::time::Instant::now() >= deadline { - break (status, text); - } - eprintln!( - "mint 422 (merkle proofs not yet observed); retrying in {:?}", - SEND_RETRY_INTERVAL - ); - tokio::time::sleep(SEND_RETRY_INTERVAL).await; - } - }; - if mint_status.is_server_error() { - dev_skip!(format!("mint returned {} — DEV flake", mint_status)); - } - if mint_status == StatusCode::UNPROCESSABLE_ENTITY - && mint_body_text.contains("Unable to get merkle proofs") - { - dev_skip!(format!( - "mint returned 422 after {:?} of retries — scanner did not observe the prior mint inscription in time; body={}", - SEND_RETRY_DEADLINE, mint_body_text - )); - } + // Post-#87 the scanner is event-driven (Esplora WS subscription), + // so by the time `mint_roundtrip_lands_balance_and_proof` returns + // 200 and writes alice-1's balance, the prior commitment is + // already at-most-one-block away from being indexed in the SMT. + // A `422 Unable to get merkle proofs` here is therefore a real + // scanner-side regression, not a benign timing flake — the + // previous PR-83-era retry loop is gone. Asserting `== 200` + // surfaces it. + let mint_resp = client + .post(url("/api/mint")) + .json(&json!({ + "account_address": alice.address_hex(), + "amount": MINT_AMOUNT, + })) + .send() + .await + .expect("POST /api/mint"); + let mint_status = mint_resp.status(); + let mint_body_text = mint_resp.text().await.unwrap_or_default(); assert_eq!( mint_status, StatusCode::OK, @@ -998,12 +1018,12 @@ async fn send_commit_roundtrip_moves_balance() { // Wait for the balance to settle so send_coins has something to spend. let balance_before = poll_balance_at_least(&client, &alice.address_hex(), MINT_AMOUNT).await; - if balance_before < MINT_AMOUNT { - dev_skip!(format!( - "balance never settled to {} after mint (saw {})", - MINT_AMOUNT, balance_before - )); - } + assert!( + balance_before >= MINT_AMOUNT, + "scanner never observed mint after MINT_AMOUNT={} (saw {})", + MINT_AMOUNT, + balance_before + ); // ---- Fetch the mint's CoinProof to discover prev_commitment_pubkey ---- let proof_resp = client @@ -1020,6 +1040,27 @@ async fn send_commit_roundtrip_moves_balance() { .expect("mint coin proof has commitment") .public_key; + // Wait for THIS test's mint inscription to land in DEV state + // (i.e. the scanner has indexed it into the SMT) BEFORE + // attempting `/api/send`. Post-#87 the scanner is event-driven, + // so 15 s is an order-of-magnitude safety margin over the typical + // <1 s latency between mint_handler returning and the WS block + // event firing. If poll_until_balance times out, that is a real + // scanner-side regression and the test fails hard — no + // dev_skip retry. The signal we wait on is alice's balance + // staying at MINT_AMOUNT (already settled above); the explicit + // call here documents the intent and keeps the wait bound + // configurable via `SEND_RETRY_DEADLINE`. + let observed_before_send = poll_until_balance(&client, &alice.address_hex(), MINT_AMOUNT).await; + assert!( + observed_before_send >= MINT_AMOUNT, + "scanner never observed mint inscription in DEV state within {:?} \ + (saw balance={}, expected >= {}) — real bug, not a flake", + SEND_RETRY_DEADLINE, + observed_before_send, + MINT_AMOUNT, + ); + // ---- Send ---- let amount = SEND_AMOUNT; let ts = unix_now(); @@ -1034,53 +1075,14 @@ async fn send_commit_roundtrip_moves_balance() { "signature": signature, "timestamp": ts, }); - // 422 with "Unable to get merkle proofs for provided public key" - // is the documented signal that the on-chain commitment for the - // freshly-minted account has not yet been observed by the scanner. - // Mints broadcast a Taproot inscription whose confirmation depends - // on Mutinynet block time (≈30 s), and the scanner polls Esplora - // on a 30 s interval — so until both delays elapse, the SMT does - // not know about the prev_commitment_pubkey we just discovered. - // Poll for up to [`SEND_RETRY_DEADLINE`] before treating it as a - // DEV-environment skip, so a typical run on a healthy Mutinynet - // (block time 30 s) completes the full roundtrip. - let (send_status, send_body_text) = { - let deadline = std::time::Instant::now() + SEND_RETRY_DEADLINE; - loop { - let resp = client - .post(url("/api/send")) - .json(&send_body) - .send() - .await - .expect("POST /api/send"); - let status = resp.status(); - let text = resp.text().await.unwrap_or_default(); - let should_retry = status == StatusCode::UNPROCESSABLE_ENTITY - && text.contains("Unable to get merkle proofs"); - if !should_retry || std::time::Instant::now() >= deadline { - break (status, text); - } - eprintln!( - "send 422 (merkle proofs not yet observed); retrying in {:?}", - SEND_RETRY_INTERVAL - ); - tokio::time::sleep(SEND_RETRY_INTERVAL).await; - } - }; - if send_status.is_server_error() { - dev_skip!(format!( - "send returned {} — DEV flake; body={}", - send_status, send_body_text - )); - } - if send_status == StatusCode::UNPROCESSABLE_ENTITY - && send_body_text.contains("Unable to get merkle proofs") - { - dev_skip!(format!( - "send returned 422 after {:?} of retries — scanner did not observe the mint inscription in time; body={}", - SEND_RETRY_DEADLINE, send_body_text - )); - } + let send_resp = client + .post(url("/api/send")) + .json(&send_body) + .send() + .await + .expect("POST /api/send"); + let send_status = send_resp.status(); + let send_body_text = send_resp.text().await.unwrap_or_default(); assert_eq!( send_status, StatusCode::OK, @@ -1091,18 +1093,34 @@ async fn send_commit_roundtrip_moves_balance() { let send_body: Value = serde_json::from_str(&send_body_text).expect("send body JSON"); assert_eq!(send_body["success"], Value::Bool(true)); let send_proof_id = send_body["proof_id"].as_u64().expect("send proof_id"); + + // Value-bearing assertions on the response payload: each hash + // field must decode to exactly 32 bytes and be non-zero. A + // shape-only `.is_some()` check was masking server bugs that + // returned a placeholder zero-hash or a truncated hex string. let ash_hex = send_body["account_state_hash"] .as_str() - .expect("account_state_hash") + .expect("account_state_hash present") .to_string(); + let ash_bytes = hex::decode(&ash_hex).expect("ash is hex"); + assert_eq!(ash_bytes.len(), 32, "account_state_hash must be 32 bytes"); + assert!( + ash_bytes.iter().any(|&b| b != 0), + "account_state_hash must be non-zero" + ); let ocr_hex = send_body["output_coins_root"] .as_str() - .expect("output_coins_root") + .expect("output_coins_root present") .to_string(); + let ocr_bytes = hex::decode(&ocr_hex).expect("ocr is hex"); + assert_eq!(ocr_bytes.len(), 32, "output_coins_root must be 32 bytes"); + assert!( + ocr_bytes.iter().any(|&b| b != 0), + "output_coins_root must be non-zero" + ); + assert!(send_proof_id > 0, "proof_id must be a positive u64"); // ---- Commit ---- - let ash_bytes = hex::decode(&ash_hex).expect("decode ash"); - let ocr_bytes = hex::decode(&ocr_hex).expect("decode ocr"); let mut commit_message = Vec::with_capacity(64); commit_message.extend_from_slice(&ash_bytes); commit_message.extend_from_slice(&ocr_bytes); @@ -1121,9 +1139,6 @@ async fn send_commit_roundtrip_moves_balance() { .await .expect("POST /api/commit"); let commit_status = commit_resp.status(); - if commit_status.is_server_error() { - dev_skip!(format!("commit returned {} — DEV flake", commit_status)); - } assert_eq!( commit_status, StatusCode::OK, @@ -1174,9 +1189,9 @@ async fn username_claim_resolve_lnurlp_roundtrip() { .await .expect("POST /api/username/claim"); let claim_status = claim_resp.status(); - if claim_status == StatusCode::SERVICE_UNAVAILABLE { - dev_skip!("username claim returned 503 — DB unavailable"); - } + // DB availability is covered separately by `/health/ready`'s `db` + // failure tag; a 503 here means the username claim path itself + // regressed and is treated as a hard failure (no `dev_skip!`). assert_eq!( claim_status, StatusCode::OK, @@ -1213,8 +1228,23 @@ async fn username_claim_resolve_lnurlp_roundtrip() { "callback must reference the username, got {:?}", lnurlp_body["callback"] ); - assert!(lnurlp_body["minSendable"].as_u64().is_some()); - assert!(lnurlp_body["maxSendable"].as_u64().is_some()); + let min_sendable = lnurlp_body["minSendable"] + .as_u64() + .expect("minSendable must be a u64"); + let max_sendable = lnurlp_body["maxSendable"] + .as_u64() + .expect("maxSendable must be a u64"); + assert!( + min_sendable >= 1, + "minSendable must be >= 1 msat, got {}", + min_sendable + ); + assert!( + max_sendable >= min_sendable, + "maxSendable ({}) must be >= minSendable ({})", + max_sendable, + min_sendable + ); assert!(lnurlp_body["metadata"] .as_str() .is_some_and(|s| !s.is_empty())); @@ -1280,6 +1310,72 @@ async fn poll_balance_at_most(client: &reqwest::Client, address: &str, target: u } } +/// Tightly-bounded variant of [`poll_balance_at_least`] used when +/// the wait is supposed to be ~1 s (the event-driven scanner picks +/// up the WS block event almost immediately post-#87) and a longer +/// stall is a real regression, not a timing flake. The 15 s budget +/// is `SEND_RETRY_DEADLINE` — see the constant's doc-comment for +/// why it's not `POLL_TIMEOUT` (60 s). +async fn poll_until_balance(client: &reqwest::Client, address: &str, target: u64) -> u64 { + let deadline = std::time::Instant::now() + SEND_RETRY_DEADLINE; + let mut last_seen = 0u64; + loop { + let resp = client + .get(url(&format!("/api/balance?address={}", address))) + .send() + .await + .expect("GET balance"); + if resp.status() == StatusCode::OK { + let body: Value = resp.json().await.unwrap_or(Value::Null); + if let Some(b) = body["balance"].as_u64() { + last_seen = b; + if b >= target { + return b; + } + } + } + if std::time::Instant::now() >= deadline { + return last_seen; + } + tokio::time::sleep(POLL_INTERVAL).await; + } +} + +/// Fetch the current balance of the well-known `MINTING_ADDRESS`. +/// Used by the fresh-state guard at the top of the happy-path +/// roundtrips to detect a dirty DEV state (prior mint residue or a +/// missed `reset_state` run). +async fn fetch_minting_balance(client: &reqwest::Client) -> u64 { + let minting_hex = format!("0x{}", hex::encode(digest_to_bytes(&MINTING_ADDRESS))); + let resp = client + .get(url(&format!("/api/balance?address={}", minting_hex))) + .send() + .await + .expect("GET /api/balance for MINTING_ADDRESS"); + assert_eq!( + resp.status(), + StatusCode::OK, + "/api/balance must return 200 for MINTING_ADDRESS" + ); + let body: Value = resp.json().await.expect("balance body is JSON"); + body["balance"].as_u64().expect("balance must be a u64") +} + +/// Fresh-state guard: assert that `MINTING_ADDRESS` balance is +/// exactly the bootstrap value. The deploy-dev workflow runs +/// `reset_state` before this suite, so any deviation here means the +/// reset did not run or another process is racing on the shared +/// DEV server. +async fn assert_minting_balance_is_bootstrap(client: &reqwest::Client) { + let balance = fetch_minting_balance(client).await; + assert_eq!( + balance, BOOTSTRAP_MINTING_BALANCE, + "DEV state not fresh — minting balance {} != bootstrap {}; \ + last reset_state run did not run or failed; check deploy-dev workflow", + balance, BOOTSTRAP_MINTING_BALANCE + ); +} + fn random_suffix() -> String { let mut bytes = [0u8; 8]; rand::thread_rng().fill_bytes(&mut bytes); From 928c84a462f6218458f122fb88d503802e719b6e Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Sun, 24 May 2026 23:26:47 +0200 Subject: [PATCH 2/3] test: relax fresh-state assertion to upper-bound (push-trigger safe) The strict `assert_minting_balance_is_bootstrap` helper would tripwire CI on every develop push after a manual reset: the deploy-dev workflow only runs `reset-zkcoins-server` on explicit workflow_dispatch with reset_state=true, not on the default push trigger. After this PR's first run, the minting balance drops to `bootstrap - 2*MINT_AMOUNT` and the strict equality fails forever. Replace with `assert_minting_balance_in_bounds`: upper-bound on BOOTSTRAP_MINTING_BALANCE (catches unauthorized re-seed bugs) plus a non-zero lower bound (catches unexpected wipe). Both happy-path tests now use the same helper. Also drops the redundant second `poll_until_balance` call in `send_commit_roundtrip_moves_balance` (the prior `poll_balance_at_least` already covered it) and documents the deliberately-deferred B5 proof_id pin in server_tests.rs (proof store ID grows across DB lifetime, same constraint as the minting balance bound). --- node/src/router_tests.rs | 28 +++++--- node/tests/api_remote.rs | 146 ++++++++++++++------------------------- 2 files changed, 69 insertions(+), 105 deletions(-) diff --git a/node/src/router_tests.rs b/node/src/router_tests.rs index f1870a24..36717711 100644 --- a/node/src/router_tests.rs +++ b/node/src/router_tests.rs @@ -3752,15 +3752,18 @@ async fn mint_happy_path_broadcasts_and_returns_proof_id() { assert_eq!(status, StatusCode::OK, "body: {}", resp_body); let v: serde_json::Value = serde_json::from_str(&resp_body).expect("valid JSON"); assert_eq!(v["success"], true); - // Fresh-state pinning: this is the FIRST proof emitted by the - // ProofStore in this test, so the id must be exactly 1. A drift - // (e.g. ProofStore counter not reset between tests, or proof_id - // generator reordered) would slip through a shape-only - // `is_some()` check. let proof_id = v["proof_id"] .as_u64() .expect("proof_id missing from response"); - assert_eq!(proof_id, 1, "fresh-state mint must emit proof_id == 1"); + // NOTE (B5 deferred): can't pin proof_id == 1 because proof store ID + // grows across DB lifetime — same constraint as the minting balance + // bound. We assert > 0 (= the proof was actually persisted) and rely + // on the integration test (api_remote `mint_roundtrip_lands_balance_and_proof`) + // to verify the proof file is fetchable + bincode-decodable. + assert!( + proof_id > 0, + "fresh-state mint must emit a non-zero proof_id" + ); // Per the mint_handler contract, the mint response intentionally // omits `account_state_hash` and `output_coins_root` (those are // returned by /api/send instead). @@ -4086,13 +4089,18 @@ async fn mint_receive_coin_failure_logs_and_returns_ok() { assert_eq!(status, StatusCode::OK, "body: {}", resp_body); let v: serde_json::Value = serde_json::from_str(&resp_body).expect("valid JSON"); assert_eq!(v["success"], true); - // Fresh-state pinning: this test's mint is the first to write a - // proof, so the id must be exactly 1. See the matching note in - // the happy-path mint test above. let proof_id = v["proof_id"] .as_u64() .expect("proof_id missing from response"); - assert_eq!(proof_id, 1, "fresh-state mint must emit proof_id == 1"); + // NOTE (B5 deferred): can't pin proof_id == 1 because proof store ID + // grows across DB lifetime — same constraint as the minting balance + // bound. We assert > 0 (= the proof was actually persisted) and rely + // on the integration test (api_remote `mint_roundtrip_lands_balance_and_proof`) + // to verify the proof file is fetchable + bincode-decodable. + assert!( + proof_id > 0, + "fresh-state mint must emit a non-zero proof_id" + ); } /// Retry-after-broadcast-failure (zk-coins/node#89). diff --git a/node/tests/api_remote.rs b/node/tests/api_remote.rs index e7b2dbb0..20bf3fa6 100644 --- a/node/tests/api_remote.rs +++ b/node/tests/api_remote.rs @@ -59,15 +59,6 @@ const DEFAULT_API_URL: &str = "https://dev-api.zkcoins.app"; const HTTP_TIMEOUT: Duration = Duration::from_secs(120); const POLL_INTERVAL: Duration = Duration::from_secs(2); const POLL_TIMEOUT: Duration = Duration::from_secs(60); -/// Inscription-landing budget for `poll_until_balance`. Post-#87 the -/// scanner is event-driven (WebSocket subscription on the Esplora -/// block stream, not a 30 s polling loop), so the wait time is bounded -/// by Mutinynet block time (~30 s) plus a small ingest margin. 15 s is -/// the worst-case healthy window between successive mints in this -/// suite; exceeding it indicates a real scanner-side regression -/// (lost subscription, dropped block, stuck SMT update) and is -/// asserted as a hard failure, never silently retried. -const SEND_RETRY_DEADLINE: Duration = Duration::from_secs(15); const MINT_AMOUNT: u64 = 50_000; const SEND_AMOUNT: u64 = 10_000; /// Bootstrap balance seeded into the `MINTING_ADDRESS` account at @@ -905,13 +896,14 @@ async fn mint_roundtrip_lands_balance_and_proof() { let client = http_client(); let alice = TestWallet::new(); - // Fresh-state guard: the deploy-dev workflow runs `reset_state` - // before this suite, so the minting account must be at the - // bootstrap balance untouched by any prior mint. A non-bootstrap - // value here means either the reset did not run, or another - // workflow racing on the shared DEV server has already minted. - // Either way the assertions below are no longer meaningful. - assert_minting_balance_is_bootstrap(&client).await; + // Minting-account sanity guard: the deploy-dev workflow's + // `push: branches: [develop]` trigger does NOT run + // `reset-zkcoins-server`, so the minting balance is allowed to be + // anywhere in (0, BOOTSTRAP_MINTING_BALANCE]. We only fail hard + // on the genuinely impossible states (balance > bootstrap = code + // regression or unauthorized re-seed; balance == 0 = unexpected + // DB wipe). See `assert_minting_balance_in_bounds` for details. + assert_minting_balance_in_bounds(&client).await; let mint_resp = client .post(url("/api/mint")) @@ -968,23 +960,17 @@ async fn send_commit_roundtrip_moves_balance() { let alice = TestWallet::new(); let bob = TestWallet::new(); - // Fresh-state guard — mirror of the one in - // `mint_roundtrip_lands_balance_and_proof`. If the minting - // account is below bootstrap by any non-multiple of `MINT_AMOUNT`, - // the SMT shape is unknown and the assertions below are unsafe. - // Allow exactly bootstrap (suite ran first) OR - // bootstrap - MINT_AMOUNT (suite-A test 1 ran before this one in - // a single-test-thread invocation). - let minting_balance = fetch_minting_balance(&client).await; - assert!( - minting_balance == BOOTSTRAP_MINTING_BALANCE - || minting_balance == BOOTSTRAP_MINTING_BALANCE - MINT_AMOUNT, - "DEV state not fresh — minting balance {} not in expected set {{{}, {}}}; \ - last reset_state run did not run or failed; check deploy-dev workflow", - minting_balance, - BOOTSTRAP_MINTING_BALANCE, - BOOTSTRAP_MINTING_BALANCE - MINT_AMOUNT, - ); + // Minting-account sanity guard — mirror of the one in + // `mint_roundtrip_lands_balance_and_proof`. The deploy-dev + // workflow's `push: branches: [develop]` trigger does NOT run + // `reset-zkcoins-server`, so we cannot pin the minting balance to + // an exact value (or even a small accept-set keyed off + // `MINT_AMOUNT`): the balance accumulates `bootstrap - N*MINT_AMOUNT` + // across every prior develop push that ran this suite. The + // bounds-check still catches the impossible / catastrophic states + // (balance > bootstrap = code regression or unauthorized re-seed; + // balance == 0 = unexpected DB wipe). + assert_minting_balance_in_bounds(&client).await; // ---- Mint ---- // Post-#87 the scanner is event-driven (Esplora WS subscription), @@ -1040,26 +1026,11 @@ async fn send_commit_roundtrip_moves_balance() { .expect("mint coin proof has commitment") .public_key; - // Wait for THIS test's mint inscription to land in DEV state - // (i.e. the scanner has indexed it into the SMT) BEFORE - // attempting `/api/send`. Post-#87 the scanner is event-driven, - // so 15 s is an order-of-magnitude safety margin over the typical - // <1 s latency between mint_handler returning and the WS block - // event firing. If poll_until_balance times out, that is a real - // scanner-side regression and the test fails hard — no - // dev_skip retry. The signal we wait on is alice's balance - // staying at MINT_AMOUNT (already settled above); the explicit - // call here documents the intent and keeps the wait bound - // configurable via `SEND_RETRY_DEADLINE`. - let observed_before_send = poll_until_balance(&client, &alice.address_hex(), MINT_AMOUNT).await; - assert!( - observed_before_send >= MINT_AMOUNT, - "scanner never observed mint inscription in DEV state within {:?} \ - (saw balance={}, expected >= {}) — real bug, not a flake", - SEND_RETRY_DEADLINE, - observed_before_send, - MINT_AMOUNT, - ); + // (No second poll needed — `poll_balance_at_least` above already + // observed alice.balance >= MINT_AMOUNT; the inscription is therefore + // on-chain and the scanner has ingested it. Removing the redundant + // 15-s wait shaves test runtime without losing signal — if the + // scanner regresses, the FIRST wait will fail.) // ---- Send ---- let amount = SEND_AMOUNT; @@ -1310,37 +1281,6 @@ async fn poll_balance_at_most(client: &reqwest::Client, address: &str, target: u } } -/// Tightly-bounded variant of [`poll_balance_at_least`] used when -/// the wait is supposed to be ~1 s (the event-driven scanner picks -/// up the WS block event almost immediately post-#87) and a longer -/// stall is a real regression, not a timing flake. The 15 s budget -/// is `SEND_RETRY_DEADLINE` — see the constant's doc-comment for -/// why it's not `POLL_TIMEOUT` (60 s). -async fn poll_until_balance(client: &reqwest::Client, address: &str, target: u64) -> u64 { - let deadline = std::time::Instant::now() + SEND_RETRY_DEADLINE; - let mut last_seen = 0u64; - loop { - let resp = client - .get(url(&format!("/api/balance?address={}", address))) - .send() - .await - .expect("GET balance"); - if resp.status() == StatusCode::OK { - let body: Value = resp.json().await.unwrap_or(Value::Null); - if let Some(b) = body["balance"].as_u64() { - last_seen = b; - if b >= target { - return b; - } - } - } - if std::time::Instant::now() >= deadline { - return last_seen; - } - tokio::time::sleep(POLL_INTERVAL).await; - } -} - /// Fetch the current balance of the well-known `MINTING_ADDRESS`. /// Used by the fresh-state guard at the top of the happy-path /// roundtrips to detect a dirty DEV state (prior mint residue or a @@ -1361,18 +1301,34 @@ async fn fetch_minting_balance(client: &reqwest::Client) -> u64 { body["balance"].as_u64().expect("balance must be a u64") } -/// Fresh-state guard: assert that `MINTING_ADDRESS` balance is -/// exactly the bootstrap value. The deploy-dev workflow runs -/// `reset_state` before this suite, so any deviation here means the -/// reset did not run or another process is racing on the shared -/// DEV server. -async fn assert_minting_balance_is_bootstrap(client: &reqwest::Client) { +/// Assert that the minting account exists and its balance has not +/// somehow exceeded the bootstrap value. Allows for arbitrary prior +/// mints in the same DB lifetime (each mint reduces the balance, never +/// increases it). +/// +/// Hard-fails if: +/// - balance > BOOTSTRAP_MINTING_BALANCE (impossible without a code bug +/// or unauthorized re-seed), OR +/// - balance == 0 with no inflight mints (suggests an unwanted reset +/// or DB wipe between deploys) +/// +/// The deploy-dev workflow's `push: branches: [develop]` trigger does +/// NOT run `reset-zkcoins-server`; that command requires explicit +/// `workflow_dispatch` with `reset_state: true`. Strict equality with +/// BOOTSTRAP_MINTING_BALANCE would therefore tripwire CI on the second +/// push after any reset. Use this upper-bound assertion instead. +async fn assert_minting_balance_in_bounds(client: &reqwest::Client) { let balance = fetch_minting_balance(client).await; - assert_eq!( - balance, BOOTSTRAP_MINTING_BALANCE, - "DEV state not fresh — minting balance {} != bootstrap {}; \ - last reset_state run did not run or failed; check deploy-dev workflow", - balance, BOOTSTRAP_MINTING_BALANCE + assert!( + balance <= BOOTSTRAP_MINTING_BALANCE, + "minting balance {} > bootstrap {} — code regression or unauthorized re-seed", + balance, + BOOTSTRAP_MINTING_BALANCE, + ); + assert!( + balance > 0, + "minting balance is 0 — likely an unexpected reset_state run or DB wipe; \ + check the deploy-dev workflow's recent runs" ); } From 687f412da86937b59b33e22c17e2da93d26ce585 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Mon, 25 May 2026 08:59:16 +0200 Subject: [PATCH 3/3] test: cover publisher_health_handler unit tests + polish MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Coverage Gate audit identified publisher_health_handler (router.rs) as uncovered by unit tests — only api_remote E2E exercises it, and api_remote is explicitly excluded from the coverage gate via `-E 'not binary(api_remote)'`. Add two unit tests in router_tests.rs mirroring the /health/ready pattern: - 200 Ok arm with wiremocked Esplora returning two UTXOs - 503 Err arm via mint_test_state's unreachable Esplora URL Refactor publisher_health_handler to derive the Taproot address from PUBLISHER_KEY once at startup (lazy_static PUBLISHER_ADDRESS in lib.rs), removing the SecretKey::from_str / Address::p2tr from the request path. Side benefits: - Handler is now pure I/O (Ok/Err on get_publisher_utxo only) - One fewer panic-able branch per request - Coverage Gate reaches 100% with the two new tests Also: - Fix stale "server::create_router" comment in runtime_tests.rs (introduced by the test-quality commit, before PR #93's rename sweep landed) - Update BOOTSTRAP_MINTING_BALANCE doc-comment to describe the bound semantic (not the strict equality that the second commit of this branch relaxed) - Defensive `command -v jq` install in deploy-dev.yaml preflight --- .github/workflows/deploy-dev.yaml | 3 + node/src/lib.rs | 20 +++++++ node/src/router.rs | 26 +------- node/src/router_tests.rs | 98 +++++++++++++++++++++++++++++++ node/src/runtime_tests.rs | 2 +- node/tests/api_remote.rs | 9 ++- 6 files changed, 130 insertions(+), 28 deletions(-) diff --git a/.github/workflows/deploy-dev.yaml b/.github/workflows/deploy-dev.yaml index 76621622..5ae7433a 100644 --- a/.github/workflows/deploy-dev.yaml +++ b/.github/workflows/deploy-dev.yaml @@ -170,6 +170,9 @@ jobs: # commit + reveal pair at typical Mutinynet fee rates needs # ~1_500 sats; 50_000 buys ~30 mints before the next top-up. # Adjust upward if the suite grows. + - name: Ensure jq is installed (preflight dependency) + run: command -v jq >/dev/null || brew install jq + - name: Preflight — publisher wallet has UTXOs env: DEV_API: https://dev-api.zkcoins.app diff --git a/node/src/lib.rs b/node/src/lib.rs index 4914c03d..55473311 100644 --- a/node/src/lib.rs +++ b/node/src/lib.rs @@ -38,8 +38,10 @@ pub mod state; pub mod username; use crate::publisher::EsploraConfig; +use bitcoin::secp256k1::{Keypair, Secp256k1, SecretKey, XOnlyPublicKey}; use lazy_static::lazy_static; use sqlx::PgPool; +use std::str::FromStr; const DEFAULT_PUBLISHER_KEY: &str = "1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef"; @@ -86,6 +88,24 @@ lazy_static! { key }; + /// Taproot publisher address derived once at startup from + /// `PUBLISHER_KEY` against the configured `NETWORK_CONFIG`. Folding + /// the secp256k1 work into `lazy_static` keeps the request path of + /// `publisher_health_handler` pure I/O (no per-request `SecretKey + /// ::from_str` / `Address::p2tr`) and removes a structurally + /// unreachable `Err` arm — `PUBLISHER_KEY` is validated here, so + /// an invalid key panics at startup, not on the first health + /// probe. Log-only, NOT a secret (the matching key lives in + /// `PUBLISHER_KEY`). + pub static ref PUBLISHER_ADDRESS: bitcoin::Address = { + let secp = Secp256k1::new(); + let sk = SecretKey::from_str(&PUBLISHER_KEY) + .expect("PUBLISHER_KEY must be a valid 32-byte hex secp256k1 secret"); + let key_pair = Keypair::from_secret_key(&secp, &sk); + let (xonly, _parity) = XOnlyPublicKey::from_keypair(&key_pair); + bitcoin::Address::p2tr(&secp, xonly, None, NETWORK_CONFIG.network()) + }; + /// Postgres connection string for the state-layer. Required; the /// bootstrap refuses to start without it because there is no /// sensible default for a database URL. diff --git a/node/src/router.rs b/node/src/router.rs index 02eb7999..488a55d8 100644 --- a/node/src/router.rs +++ b/node/src/router.rs @@ -1284,31 +1284,9 @@ struct PublisherHealthResponse { /// Esplora-side error is intentional: the operator should see the /// failure mode, not a fabricated empty response. async fn publisher_health_handler(State(state): State) -> impl IntoResponse { - use bitcoin::secp256k1::{Secp256k1, SecretKey, XOnlyPublicKey}; - use std::str::FromStr; + let publisher_address = &*crate::PUBLISHER_ADDRESS; - let publisher_key = &*crate::PUBLISHER_KEY; - let secp = Secp256k1::new(); - let sk = match SecretKey::from_str(publisher_key) { - Ok(sk) => sk, - Err(e) => { - return ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(serde_json::json!({ - "error": "invalid PUBLISHER_KEY", - "detail": e.to_string(), - })), - ) - .into_response(); - } - }; - let key_pair = bitcoin::secp256k1::Keypair::from_secret_key(&secp, &sk); - let (xonly, _parity) = XOnlyPublicKey::from_keypair(&key_pair); - let network = state.esplora_config.network(); - let publisher_address = bitcoin::Address::p2tr(&secp, xonly, None, network); - - match crate::publisher::get_publisher_utxo(&publisher_address, &state.esplora_config, None) - .await + match crate::publisher::get_publisher_utxo(publisher_address, &state.esplora_config, None).await { Ok(utxos) => { let utxo_count = utxos.len() as u64; diff --git a/node/src/router_tests.rs b/node/src/router_tests.rs index 36717711..d7e02caa 100644 --- a/node/src/router_tests.rs +++ b/node/src/router_tests.rs @@ -3371,6 +3371,104 @@ async fn ready_returns_503_when_esplora_unreachable() { assert_eq!(failures, vec!["esplora".to_string()]); } +// ======================================================================= +// GET /health/publisher — operational preflight +// ======================================================================= +// +// The publisher health probe surfaces (address, utxo_count, total_sats) +// for the deploy-dev preflight. Two reachable arms after the lazy_static +// `PUBLISHER_ADDRESS` refactor: Ok (Esplora responded) and Err (Esplora- +// side error). The `SecretKey::from_str` panic-arm is no longer in the +// request path — `PUBLISHER_KEY` is validated once at startup. + +#[tokio::test] +async fn health_publisher_returns_200_with_utxo_count_and_total_sats_when_esplora_responds() { + // Mock Esplora returning a known UTXO set so the handler's Ok arm + // is exercised: GET /address/{publisher_addr}/utxo returns a JSON + // array of UTXOs that get_publisher_utxo parses and sums. + use wiremock::matchers::{method, path_regex}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let esplora_mock = MockServer::start().await; + Mock::given(method("GET")) + .and(path_regex(r"^/address/.+/utxo$")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([ + { + "txid": "a".repeat(64), + "vout": 0, + "value": 50_000, + "status": { "confirmed": true, "block_height": 1, "block_hash": "b".repeat(64), "block_time": 0 } + }, + { + "txid": "c".repeat(64), + "vout": 1, + "value": 12_345, + "status": { "confirmed": true, "block_height": 2, "block_hash": "d".repeat(64), "block_time": 0 } + } + ]))) + .mount(&esplora_mock) + .await; + + let mut state = mint_test_state(); + state.esplora_config = Arc::new(crate::publisher::EsploraConfig { + url: esplora_mock.uri(), + is_mainnet: false, + network_name: "Mutinynet".to_string(), + ws_url: None, + track_tx_timeout: None, + }); + + let req = Request::get("/health/publisher") + .body(Body::empty()) + .unwrap(); + let (status, body) = send_request_with_state(state, req).await; + + assert_eq!(status, StatusCode::OK, "body={}", body); + let v: serde_json::Value = serde_json::from_str(&body).expect("publisher health body is JSON"); + assert!( + v["address"] + .as_str() + .expect("address present") + .starts_with("tb1p"), + "publisher address must be Mutinynet bech32 Taproot, got: {:?}", + v["address"] + ); + assert_eq!(v["utxo_count"].as_u64().expect("utxo_count u64"), 2); + assert_eq!(v["total_sats"].as_u64().expect("total_sats u64"), 62_345); +} + +#[tokio::test] +async fn health_publisher_returns_503_when_esplora_unreachable() { + // Drive the Err arm: mint_test_state() already points esplora at + // 127.0.0.1:1 (unreachable), so get_publisher_utxo returns Err + // and the handler must map to 503. + let state = mint_test_state(); + let req = Request::get("/health/publisher") + .body(Body::empty()) + .unwrap(); + let (status, body) = send_request_with_state(state, req).await; + + assert_eq!(status, StatusCode::SERVICE_UNAVAILABLE, "body={}", body); + let v: serde_json::Value = + serde_json::from_str(&body).expect("publisher health err body is JSON"); + assert_eq!( + v["error"].as_str().expect("error field present"), + "Esplora-side error fetching publisher UTXOs" + ); + assert!( + v["address"] + .as_str() + .expect("address present") + .starts_with("tb1p"), + "publisher address must be returned even on Esplora failure, got: {:?}", + v["address"] + ); + assert!( + v["detail"].as_str().is_some(), + "detail field must be present for diagnostics" + ); +} + // ======================================================================= // POST /api/mint — handler coverage // ======================================================================= diff --git a/node/src/runtime_tests.rs b/node/src/runtime_tests.rs index 271d901f..a3828d09 100644 --- a/node/src/runtime_tests.rs +++ b/node/src/runtime_tests.rs @@ -148,7 +148,7 @@ async fn start_rest_node_binds_and_serves_health() { ); // `/health` is the documented liveness probe whose // body is the literal string "ok" (see the route - // registration in `server::create_router`). A 200 + // registration in `router::create_router`). A 200 // status with a different body would still satisfy // the old assertion but signal a regression in the // contract Kuma watches. diff --git a/node/tests/api_remote.rs b/node/tests/api_remote.rs index 20bf3fa6..5dfda8a8 100644 --- a/node/tests/api_remote.rs +++ b/node/tests/api_remote.rs @@ -66,9 +66,12 @@ const SEND_AMOUNT: u64 = 10_000; /// Must stay strictly less than `2^48` for Plonky2 Goldilocks safety /// — see the matching constant guard in `runtime_tests`. The /// happy-path roundtrips probe `/api/balance` on `MINTING_ADDRESS` -/// before their first mint and assert this exact value, so a dirty -/// DEV state (prior test residue, missed `reset_state` run) trips a -/// hard failure instead of being silently treated as "balance dipped". +/// before their first mint and use this as an upper bound — +/// `0 < balance <= BOOTSTRAP_MINTING_BALANCE`. The exact value is +/// not asserted because the deploy-dev push trigger does not run +/// `reset_state`, so prior test residue legitimately reduces the +/// minting balance; the bound still catches a fully empty / negative +/// state. const BOOTSTRAP_MINTING_BALANCE: u64 = 1u64 << 48; fn api_base() -> String {