From fc31964273fa54a0d784b6beea53dbfe57af0030 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Sun, 31 May 2026 12:58:24 +0200 Subject: [PATCH 1/5] =?UTF-8?q?ci:=20add=20staging=20=E2=86=92=20develop?= =?UTF-8?q?=20auto-promote-pr=20workflow=20+=20doc=20the=20new=20flow=20(#?= =?UTF-8?q?148)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mirrors the existing develop → main auto-release-pr.yaml one layer down. Triggers on push to staging, opens (or no-ops on existing) "Promote: staging -> develop" PR for the operator to merge once the accumulated feature work is ready to land on DEV. Naming separates the two layers: - staging → develop is a Promote (DEV deploy is a follow-on effect, not the purpose). - develop → main stays Release (production cut). Promote PRs intentionally do NOT apply the ci:full label. The heavy M3 Ultra test + coverage gate stays reserved for the Release PR, which remains the authoritative pre-PRD gate. Promote PRs run the slim Lint & Build job + Analyze / CodeQL, matching what every ready feature PR sees. CONTRIBUTING.md updated: - Branches table now lists staging as the integration buffer between feature/* and develop. - Workflow rule rewritten: open feature PRs against staging, not develop. develop is fed by the staging auto-PR; main by the develop auto-PR. - Explicit protection statement re-anchored: develop + main reject direct pushes, hotfixes go through staging. - CI/CD table extended with the new workflow row. Bootstrap note: this PR targets develop directly because the staging-side workflow doesn't exist yet — once merged + synced to staging, all subsequent feature work follows the staging → develop flow. --- .../workflows/auto-release-pr-staging.yaml | 71 +++++++++++++++++++ CONTRIBUTING.md | 16 +++-- 2 files changed, 81 insertions(+), 6 deletions(-) create mode 100644 .github/workflows/auto-release-pr-staging.yaml diff --git a/.github/workflows/auto-release-pr-staging.yaml b/.github/workflows/auto-release-pr-staging.yaml new file mode 100644 index 00000000..b59606cd --- /dev/null +++ b/.github/workflows/auto-release-pr-staging.yaml @@ -0,0 +1,71 @@ +name: Auto Promote PR (staging → develop) + +on: + push: + branches: [staging] + workflow_dispatch: + +permissions: + contents: read + pull-requests: write + +concurrency: + group: auto-release-pr-staging + cancel-in-progress: false + +jobs: + create-promote-pr: + name: Create Promote PR + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Fetch develop branch + run: git fetch origin develop + + - name: Check for existing PR + id: check-pr + run: | + PR_COUNT=$(gh pr list --base develop --head staging --state open --json number --jq 'length') + echo "pr_exists=$([[ $PR_COUNT -gt 0 ]] && echo 'true' || echo 'false')" >> $GITHUB_OUTPUT + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + + - name: Check for differences + id: check-diff + if: steps.check-pr.outputs.pr_exists == 'false' + run: | + DIFF_COUNT=$(git rev-list --count origin/develop..origin/staging) + echo "has_changes=$([[ $DIFF_COUNT -gt 0 ]] && echo 'true' || echo 'false')" >> $GITHUB_OUTPUT + echo "commit_count=$DIFF_COUNT" >> $GITHUB_OUTPUT + + - name: Create Promote PR + if: steps.check-pr.outputs.pr_exists == 'false' && steps.check-diff.outputs.has_changes == 'true' + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + COMMIT_COUNT: ${{ steps.check-diff.outputs.commit_count }} + run: | + # Promote PRs intentionally do NOT apply the `ci:full` label. + # The heavy M3 Ultra test + coverage gate stays reserved for + # the develop → main Release PR (auto-release-pr.yaml), which + # remains the authoritative pre-PRD gate. Promote PRs run the + # slim `Lint & Build` job + Analyze / CodeQL, mirroring what + # every ready feature PR sees. + printf '%s\n' \ + "## Automatic Promote PR" \ + "" \ + "**Commits:** ${COMMIT_COUNT} new commit(s)" \ + "" \ + "- [ ] Review all changes" \ + "- [ ] Verify CI passes" \ + "- [ ] Merge to promote staging to develop (deploys to DEV)" \ + > /tmp/pr-body.md + + gh pr create \ + --base develop \ + --head staging \ + --title "Promote: staging -> develop" \ + --body-file /tmp/pr-body.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7ed34e6f..ca7815b0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -376,12 +376,15 @@ node/ | Branch | Purpose | Deploy target | |---|---|---| -| `develop` | Default branch, active development | DEV node | -| `main` | Production releases | PRD node | +| `staging` | Integration buffer — feature PRs land here first | none | +| `develop` | Active development, promoted from `staging` in batches | DEV node | +| `main` | Production releases, promoted from `develop` | PRD node | -- **Push to `develop` via feature branch + PR** (branch ruleset active) -- **`main` is protected** — changes only via PR -- Never force-push, never amend +- **Open feature PRs against `staging`** (not `develop`) — `staging` is the integration buffer where multiple feature branches accumulate before being batched into a single `develop` promotion. This keeps `develop` clean for DEV-deploy churn and gives reviewers a smaller blast radius per merge. +- **`develop` and `main` are protected** — direct pushes are rejected. `develop` accepts only the auto-PR from `staging`; `main` accepts only the auto-PR from `develop`. Hotfixes still go through `staging` so the same review path applies. +- **`develop` is auto-PR'd from `staging`** by `auto-release-pr-staging.yaml` whenever new commits land on `staging`. Merge that PR to promote the batch to DEV. Promote PRs intentionally skip the `ci:full` label — heavy M3 Ultra tests stay reserved for the develop → main Release PR. +- **`main` is auto-PR'd from `develop`** by `auto-release-pr.yaml` (with `ci:full` applied automatically). Merge to release to PRD. +- Never force-push, never amend. ### Commit Messages @@ -603,7 +606,8 @@ See [docs.zkcoins.app/infrastructure/backend](https://docs.zkcoins.app/infrastru | `ci.yaml` (Coverage Gate) | Ready PR → develop with `ci:full` label, push to develop | `cargo llvm-cov nextest` with the 100% line + function gate, MVP scope, on the same runner pool. | | `deploy-dev.yaml` | Push to develop | Docker build (ARM64) → push `zkcoins/node:beta` → deploy to DEV | | `deploy-prd.yaml` | Push to main | Docker build (ARM64) → push `zkcoins/node:latest` → deploy to PRD | -| `auto-release-pr.yaml` | Push to develop | Creates Release PR (develop → main) | +| `auto-release-pr-staging.yaml` | Push to staging | Creates Promote PR (staging → develop) | +| `auto-release-pr.yaml` | Push to develop | Creates Release PR (develop → main) with `ci:full` label | **Draft PRs** skip every `ci.yaml` job — the workflow fires once the PR is marked ready-for-review. From 2fc6ae7ea90f118969c75b88d25c62b7ff79e837 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Sun, 31 May 2026 13:01:36 +0200 Subject: [PATCH 2/5] =?UTF-8?q?docs(track-tx):=20correct=20root=20cause=20?= =?UTF-8?q?=E2=80=94=20wrong=20WS=20wire=20format,=20not=20missing=20backe?= =?UTF-8?q?nd=20support=20(#145)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit § 7.24 of MIGRATION_RESEARCH.md and the matching CONTRIBUTING.md historical note previously claimed that self-hosted mempool/backend:v3.3.1 does not implement the track-tx WS action. A direct websocat probe on dfxdev falsifies that: - {"action":"track-tx","data":""} → 0 frames in 6 s - {"track-tx":""} → immediate txPosition frame The publisher's pre-#144 frame (scanner_ws.rs on ae78798^, lines 650-655) used the {action, data} envelope. mempool.js's wsTrackTransaction (canonical client) sends {"track-tx":""} at the top level, and mempool/backend:v3.3.1's websocket-handler.ts keys off parsedMessage['track-tx']. The backend silently dropped the malformed frame — indistinguishable from "action not supported" from the client side. PR #144 still stands on independent grounds: closed test environment (no external Esplora to subscribe against), race-free in-cluster topology (bitcoind local-mempool accept orders the two POSTs), and ~200 LOC removed for zero behavioural loss. Measured DEV impact: /api/mint p50 40 s → 8.7 s (4.6×), /api/send + /api/commit p50 42 s → 12.7 s (3.3×). Doc-only; no source files touched. --- CONTRIBUTING.md | 33 ++++++---- MIGRATION_RESEARCH.md | 138 +++++++++++++++++++++++++++++++----------- 2 files changed, 124 insertions(+), 47 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ca7815b0..90e36250 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -42,19 +42,26 @@ block-time + poll-interval (issue #84); event-driven ingestion brings that down to the WS round-trip. Historical note: issue #84 originally replaced a fixed 5 s -`PROPAGATION_WAIT_SECS` sleep with a `{"action":"track-tx","data":""}` -WS wait + REST safety-net. After the deployment moved to a -self-hosted `mempool/backend:v3.3.1`, empirical measurement showed -that backend version emits zero frames for `track-tx`, so the WS -wait always timed out and the REST fallback always confirmed the -tx as already on-chain. 16/16 fallbacks in 72 h DEV `request_log`, -0 not-found, 0 errors. The wait was pure latency tax (~30 s/mint -and ~30 s/send+commit) for an in-cluster scenario where bitcoind's -local-mempool accept already orders the two POSTs correctly. The -publisher now runs `client.broadcast(commit) → client.broadcast(reveal)` -sequentially; race-freedom follows from the topology (node, electrs, -bitcoind share the Docker `bitcoin` network), not from a WS -subscription. +`PROPAGATION_WAIT_SECS` sleep with a WS `track-tx` wait + REST +safety-net. PR [#144](https://github.com/zk-coins/node/pull/144) +removed that path and replaced it with direct sequential +`client.broadcast(commit) → client.broadcast(reveal)`. A later +re-analysis (see `MIGRATION_RESEARCH.md` § 7.24) established that +the publisher's subscribe frame had been sent in the wrong wire +format — `{"action":"track-tx","data":""}` — whereas the +mempool.js convention and `mempool/backend:v3.3.1`'s +`websocket-handler.ts` both expect `{"track-tx":""}` as a +top-level key. The backend silently dropped the malformed frame, so +the WS wait always timed out and the REST safety-net always +confirmed the tx as already on-chain (16/16 fallbacks in the 72 h +DEV `request_log` sample, 0 not-found, 0 errors). PR #144 stands +on independent grounds: in the in-cluster topology (node, electrs, +bitcoind share the Docker `bitcoin` network) bitcoind's +local-mempool accept already orders the two POSTs race-free, and +the closed-test-env model (no external Esplora) means there is no +upstream to subscribe against in the first place. The +architecture is documented here; the wire-format bug is recorded +for the historical record, not as a justification. Where it applies: diff --git a/MIGRATION_RESEARCH.md b/MIGRATION_RESEARCH.md index ffc9e297..87c52804 100644 --- a/MIGRATION_RESEARCH.md +++ b/MIGRATION_RESEARCH.md @@ -1314,40 +1314,110 @@ is a build-time signal, not a runtime-readiness signal. deploy-dev post-curl-retry fires on every DEV deploy. A regression that brings back the silent-panic shape fails one or both gates. -### 7.24 Self-hosted `mempool/backend:v3.3.1` does not implement the `track-tx` WS action — **codified** - -**Discovered:** DEV operations (May 2026), via combined evidence: -DEV `request_log` showed `/api/mint` p50 ≈ 40 s and -`/api/send` p50 = 11 s + `/api/commit` p50 = 30.7 s — both with the -30 s shape characteristic of the publisher's `track-tx` safety-net; -16/16 REST fallbacks in the 72 h sample succeeded with 0 not-found -and 0 errors; a direct WS probe against -`mempool-api-mutinynet:8999/api/v1/ws` with -`{"action":"track-tx","data":""}` returned 0 frames in 15 s, -while `{"action":"want","data":["blocks"]}` answered immediately. - -**Root cause:** the self-hosted `mempool/backend:v3.3.1` version -does not emit any frame in response to `track-tx` (the action is -recognised but no event arrives). Issue #84's original design -assumed a public mutinynet endpoint where the action does fire; -self-hosting flipped that assumption silently. - -**Fix:** drop the entire WS subscribe + safety-net + REST fallback -path from `publisher::broadcast_inscription_txs`. The publisher now -runs `client.broadcast(commit) → client.broadcast(reveal)` back to -back; race-freedom comes from the deployment topology (node + -electrs + bitcoind in the shared Docker `bitcoin` network, -`bitcoind::sendrawtransaction` returns only after local-mempool -accept), not from a WS subscription. Expected effect: -`/api/mint` p50 ~40 s → ~11 s, `/api/send + /api/commit` -~42 s → ~13 s. See the perf PR for the removed code. - -**Generalisation for future migrations:** when porting an -event-driven path that was designed against a public upstream onto -a self-hosted reimplementation of the same protocol, smoke-test -each WS action against the self-hosted endpoint before assuming -parity. Empty-frame-budget probe is cheap; silent latency tax is -expensive. +### 7.24 Wrong WS subscribe wire format on self-hosted `mempool/backend` — empirical correction — **codified** + +**Status correction.** An earlier revision of this section claimed +that `mempool/backend:v3.3.1` "does not implement the `track-tx` WS +action". That conclusion was wrong. The backend implements +`track-tx` correctly; the zkCoins publisher had been sending the +subscribe frame in the wrong wire format. Both PR +[#144](https://github.com/zk-coins/node/pull/144) (drop the WS +path) and this codification stand — but for different reasons than +the original write-up gave. + +**What the publisher actually sent (pre-PR-#144, +`node/src/scanner_ws.rs:650-655` on `ae78798^`):** + +```rust +let subscribe = serde_json::json!({ + "action": "track-tx", + "data": txid_str, +}); +``` + +**What `mempool/backend:v3.3.1` parses +(`backend/src/api/websocket-handler.ts`, lines ~165-175):** + +```typescript +if (parsedMessage && parsedMessage['track-tx']) { + if (/^[a-fA-F0-9]{64}$/.test(parsedMessage['track-tx'])) { + client['track-tx'] = parsedMessage['track-tx']; + // ... subscribe, will emit txPosition / txConfirmed frames + } +} +``` + +The backend looks at the top-level `track-tx` key. The publisher's +`{action, data}` envelope has no such key, so the handler falls +through silently — no error frame, no log line, no rejection. + +**What mempool.js (the canonical client) actually sends +(`https://raw.githubusercontent.com/mempool/mempool.js/main/src/services/ws/ws-client-node.ts`, +`wsTrackTransaction`):** + +```typescript +export const wsTrackTransaction = (ws: WebSocket, txid: string): void => { + wsActionWrapper(ws, { 'track-tx': txid }); +} +``` + +i.e. `{"track-tx": ""}` as a top-level key — exactly the +shape `websocket-handler.ts` parses. The publisher's frame did +not follow this convention. + +**Empirical verification (dfxdev, post-PR-#144 re-probe, May 2026).** +A direct `websocat` probe against +`ws://mempool-api-mutinynet:8999/api/v1/ws` with a live mempool +txid: + +- `{"action":"track-tx","data":""}` → 0 frames in 6 s + (matches the production observation that motivated PR #144). +- `{"track-tx":""}` → immediate `{"txPosition":...}` frame, + followed by `{"txConfirmed":...}` when the next block arrived. + +So the backend was working all along; the publisher's frame was +malformed. + +**Why PR #144 still stands.** Reverting to a WS path with the +correct wire format is not the right move: + +1. **Closed test environment, no external Esplora.** zkCoins runs + against a self-hosted `mempool/backend` colocated with node, + electrs, and bitcoind in the shared Docker `bitcoin` network. + There is no upstream public endpoint to subscribe against. +2. **Topology is race-free without a subscribe.** In that + topology, `bitcoind::sendrawtransaction` returns only after + local-mempool accept, so a sequential + `client.broadcast(commit) → client.broadcast(reveal)` is + already ordered. The WS round-trip the subscribe gave us was a + confirmation of something the REST call already guaranteed. +3. **Simpler code.** The WS subscribe + reconnect-with-backoff + + REST safety-net was three failure modes for a problem the REST + call alone does not have. Removing it shrinks + `publisher.rs`/`scanner_ws.rs` by ~200 lines (see PR #144 diff). + +**Empirically measured impact of PR #144 on DEV `request_log`:** +`/api/mint` p50 40 s → 8.7 s (4.6×); `/api/send + /api/commit` p50 +42 s → 12.7 s (3.3×). Numbers match the predicted shape — the +removed wait was indeed ~30 s of pure latency tax (15 s WS +timeout + REST fallback round-trip). + +**Generalisation for future migrations.** Two distinct lessons, +neither the original one: + +1. When a WebSocket subscribe "doesn't work", verify the wire + format against the canonical client's source before concluding + the server is broken. `mempool.js` is the reference; copy its + frame shape verbatim, do not reconstruct it from the action + name. +2. The original investigation (latency probe → REST-fallback hit + rate → empty-frame WS probe with the wrong format) reached a + plausible-but-wrong root cause because every signal was + consistent with both "backend broken" and "client malformed". + When a server silently drops a request, "the server doesn't + support it" and "we asked for it wrong" look identical from + the client side. Always cross-check the request against a + known-good client's wire format before blaming the server. --- From b2f45e146754d90329f55233c018f049dde612fe Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Sun, 31 May 2026 13:38:27 +0200 Subject: [PATCH 3/5] =?UTF-8?q?feat(config):=20require=20explicit=20chain?= =?UTF-8?q?=20config=20=E2=80=94=20no=20silent=20Mutinynet=20defaults=20(#?= =?UTF-8?q?149)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(config): require explicit chain config — no silent Mutinynet defaults The chain-shaping env vars (IS_MAINNET, ESPLORA_URL, ESPLORA_WS_URL) used to silently default to Mutinynet endpoints, leaving two distinct silent footguns: (1) a Mainnet deploy that forgot ESPLORA_URL / ESPLORA_WS_URL would scan Mutinynet while answering /api/info as Mainnet, with a green /health/ready and a 5-s HTTP retry loop on the scanner (#84); (2) a Mutinynet deploy that left ESPLORA_WS_URL unset coupled itself to the public wss://mutinynet.com endpoint we do not operate. All three vars are now required-or-panic on both code paths — the same contract as USERNAME_DOMAIN, PUBLISHER_KEY, and DATABASE_URL. IS_MAINNET accepts only the exact strings "true" or "false"; values like "1", "TRUE", "yes" panic instead of silently meaning Mutinynet. Empty strings (ESPLORA_URL= in a compose file) are treated as unset. Code path consolidation: - lib::build_network_config_from_env panics on missing/empty/ambiguous values for all three vars. - scanner_ws no longer carries DEFAULT_ESPLORA_*_URL constants and no longer reads env directly. ScannerWsConfig::from_env is replaced by ScannerWsConfig::from_network_config(&EsploraConfig), consuming the already-resolved NETWORK_CONFIG. main.rs calls the new constructor. - publisher.rs doc-comment on EsploraConfig.ws_url updated to reflect the new "always Some(...) from build_network_config_from_env" invariant. - recover_inscription binary applies the same explicit-or-panic contract for its IS_MAINNET read. Guardrail: - New integration test node/tests/no_chain_hardcodes.rs scans every .rs file under node/src/ (excluding *_tests.rs and comment lines) for literal mutinynet.com / mempool.space URLs and fails the build if any appear in production source. Prevents a future "small refactor" from re-introducing the same class of default. Test + CI alignment: - main_tests.rs rewritten: removed the defaults_to_mutinynet_when_is_mainnet_unset / _is_not_true tests (their semantics no longer exist), added panic-path coverage for every missing / empty / ambiguous combination, plus a happy-path Mutinynet case symmetric to the existing Mainnet one. - scanner_ws_tests.rs swaps the from_env smoke for a from_network_config smoke and adds a panic-on-missing-ws_url test. - runtime_tests.rs adds the two new required vars to its defensive set_var block. - .github/workflows/ci.yaml: both node-tests and coverage env blocks now set IS_MAINNET=false and ESPLORA_WS_URL=ws://127.0.0.1:1/api/v1/ws alongside the existing ESPLORA_URL placeholder so the bootstrap panics no longer fail CI. Docs: - README §Configuration: defaults column flips to "(required, no default)" for the three vars, with a paragraph explaining the bias the change removes and the guardrail that backstops it. Per-stage endpoint examples included. - CONTRIBUTING § Env: same — required across the board. Minimal local- dev env snippet now sets all five required vars explicitly. The four CI env blocks (.github/workflows/ci.yaml) + the two test set_var sites + the new CONTRIBUTING dev-env snippet are the only places callers need to be aware of; production deploys (infrastructure/{dfxdev,dfxprd}/zkcoins/docker-compose.yaml) will need to set IS_MAINNET and ESPLORA_WS_URL explicitly on DEV — that follows in the DFXServer/server PR (PR 2 of the chain-config track). * fix(config): scrub remaining chain URLs from production code and tests Three sites the first commit missed: 1. `recover_inscription` binary carried a `DEFAULT_ESPLORA_URL = "https://mutinynet.com/api"` constant plus an `unwrap_or_else` fallback that would silently broadcast against Mutinynet when the `--esplora-url` flag was omitted. Same class of footgun as the removed node-side default: an operator recovering a Mainnet inscription with the flag forgotten would target the wrong chain. `--esplora-url` is now required; the binary exits with the standard usage error if missing. Constant deleted. 2. `lib::build_network_config_from_env`'s `ESPLORA_URL` panic message embedded the DFX-specific internal hostnames (`electrs-mainnet:3000`, `electrs-mutinynet:3000`) as concrete examples. Per-stage endpoints belong in the README, not in the source — the message now points there. Mirrors the symmetric treatment already adopted for the `ESPLORA_WS_URL` panic in the same function. 3. `main_tests.rs` carried `wss://mempool.space/api/v1/ws`, `ws://mempool-api-mutinynet:8999/api/v1/ws`, and the `electrs-{main,mutinynet}:3000` hostnames as test fixtures. The guardrail test exempts `*_tests.rs`, so this was not a correctness violation — but a Mainnet PRD URL appearing verbatim in source on any merge is the wrong signal regardless. Fixtures migrated to clearly-fake `.test` hostnames (`mainnet-ws.test`, `mutinynet-ws.test`, `electrs-{mainnet,mutinynet}.test:3000`) that document intent without coupling tests to a real-world host. Doc-comment and inline-comment references to `mutinynet.com` / `mempool.space` are retained — they're historical context describing the bias this PR removes, and the guardrail test allows them mechanically. --- .github/workflows/ci.yaml | 19 +++- CONTRIBUTING.md | 23 ++-- README.md | 22 ++-- node/src/bin/recover_inscription.rs | 47 ++++++-- node/src/lib.rs | 151 ++++++++++++++------------ node/src/main.rs | 5 +- node/src/main_tests.rs | 159 ++++++++++++++++++---------- node/src/publisher.rs | 12 ++- node/src/runtime_tests.rs | 14 +++ node/src/scanner_ws.rs | 65 ++++++------ node/src/scanner_ws_tests.rs | 43 ++++++-- node/tests/no_chain_hardcodes.rs | 126 ++++++++++++++++++++++ 12 files changed, 485 insertions(+), 201 deletions(-) create mode 100644 node/tests/no_chain_hardcodes.rs diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 69d0b2a5..e795dbc3 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -166,11 +166,16 @@ jobs: runs-on: [self-hosted, m3-ultra] timeout-minutes: 120 env: - # Force Esplora broadcasts to fail fast. Some unit tests exercise - # the commit pipeline that ends in a real HTTP broadcast; without - # this, runs against the public Mutinynet API can take >60 s per - # test. Mirrors the pre-push hook. + # All three chain-shaping env vars are required by the node + # bootstrap — no defaults exist (see + # `lib::build_network_config_from_env`). CI uses + # `127.0.0.1:1` endpoints so any test that exercises the commit + # pipeline / scanner WS fails fast instead of reaching a public + # third-party host (a previous Mutinynet-flavoured silent + # fallback used to add >60 s per test). + IS_MAINNET: "false" ESPLORA_URL: http://127.0.0.1:1/api + ESPLORA_WS_URL: ws://127.0.0.1:1/api/v1/ws # `USERNAME_DOMAIN` is required by the node bootstrap (no # default — see node/src/main.rs and issue #95). The test value # is irrelevant for the `info_returns_*` assertions (they only @@ -291,7 +296,13 @@ jobs: runs-on: [self-hosted, m3-ultra] timeout-minutes: 90 env: + # All three chain-shaping env vars are required by the node + # bootstrap (see `lib::build_network_config_from_env`). Mirror + # the `node-tests` env block above — `127.0.0.1:1` endpoints + # fail fast for any commit-pipeline / scanner-WS path. + IS_MAINNET: "false" ESPLORA_URL: http://127.0.0.1:1/api + ESPLORA_WS_URL: ws://127.0.0.1:1/api/v1/ws USERNAME_DOMAIN: test.zkcoins.local # `PUBLISHER_KEY` is required on every network (no default — see # `node/src/lib.rs`); the value mirrors `node-tests` above and is diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 90e36250..7261bbb0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -32,9 +32,9 @@ documents in the order given below. ### No polling — events only Bitcoin / Esplora signals on the node's hot path are subscribed to, -never polled. The scanner consumes block events from the -mempool.space-compatible WebSocket stream (`scanner_ws.rs`, -`ESPLORA_WS_URL`, default `wss://mutinynet.com/api/v1/ws`); the +never polled. The scanner consumes block events from the Esplora- +compatible WebSocket stream (`scanner_ws.rs`, `ESPLORA_WS_URL` — +required env var, no default; see README §Configuration); the publisher broadcasts the commit and reveal transactions back-to-back via REST and never sleeps or polls between them. The previous 30-s tip-poll gated `/api/mint` and `/api/send` visibility by up to a full @@ -523,23 +523,26 @@ on startup if unset — there is no silent fallback. | `PUBLISHER_KEY` | _(required, no default)_ | 32-byte hex private key for Taproot inscription publishing. **Required on every network — DEV, signet, and mainnet.** No fallback default exists: the previous `1234…` placeholder was a publicly-known test key that drainer bots swept within minutes of any on-chain top-up (4 historical drains confirmed). Node panics on startup if unset. Generate locally via `openssl rand -hex 32`. In any deployed environment, source it from your secret manager — **never commit a real key**. | | `USERNAME_DOMAIN` | _(required, no default)_ | External hostname returned by `/api/info`; node panics on startup if unset (see PR [#36](https://github.com/zk-coins/node/pull/36) for the regression that introduced the global panic hook). | | `POSTGRES_PASSWORD` | _(required, no default for the DB container)_ | Read by the Postgres container, not by the node process itself; the node's `DATABASE_URL` already embeds the password. Listed here because it is part of the local-dev bootstrap (see `Local Development with Postgres` below). | -| `ESPLORA_URL` | `https://mutinynet.com/api` | Esplora REST API endpoint (electrs or public). | -| `ESPLORA_WS_URL` | `wss://mutinynet.com/api/v1/ws` | Esplora WebSocket endpoint consumed by `scanner_ws` (issue #84). DEV/PRD override only when the upstream WS path changes. | -| `IS_MAINNET` | `false` | `true` for Bitcoin Mainnet, `false` for Mutinynet/Signet. | -| `NETWORK_NAME` | `Mutinynet` / `Mainnet` | Human-readable name returned by `/api/info`. | +| `IS_MAINNET` | _(required, no default)_ | Exact string `true` or `false`; any other value panics. Truthy values like `1`, `TRUE`, `yes` are rejected to prevent silent misconfiguration. | +| `ESPLORA_URL` | _(required, no default)_ | HTTP Esplora endpoint (electrs or public-compatible) for the chain this stage serves. Empty string is treated as unset and panics. | +| `ESPLORA_WS_URL` | _(required, no default)_ | Esplora-compatible WebSocket endpoint consumed by `scanner_ws` (issue #84). Empty string is treated as unset and panics. Previous Mutinynet default was removed because it coupled the deploy to a public third-party host. | +| `NETWORK_NAME` | `Mutinynet` / `Mainnet` | Human-readable name returned by `/api/info`. Derived from `IS_MAINNET` if unset. Purely cosmetic — no behavioural effect. | | `PROOFS_DIR` | `./proofs` | Directory for per-proof bincode files (see `Persistent State` below). | | `SCANNER_INITIAL_SETTLE_TIMEOUT_MS` | (runtime-defined) | Override for the scanner's initial-settle deadline; see `runtime.rs`. | | `RUST_LOG` | `info` | Log level (`debug`, `info`, `warn`, `error`). | ### Minimal local-dev env +All chain-shaping vars are required — there are no defaults. Set them +explicitly, even for local dev: + ```bash export DATABASE_URL="postgresql://postgres:dev@localhost:5432/postgres" export PUBLISHER_KEY="$(openssl rand -hex 32)" export USERNAME_DOMAIN="test.zkcoins.local" -# Optional — defaults are fine for Mutinynet: -# export ESPLORA_URL="https://mutinynet.com/api" -# export IS_MAINNET="false" +export IS_MAINNET="false" +export ESPLORA_URL="http://localhost:3000" # your local electrs +export ESPLORA_WS_URL="ws://localhost:8999/api/v1/ws" # your local mempool/backend, or any Esplora-compatible WS cargo run -p node ``` diff --git a/README.md b/README.md index dfe5b761..17cefab0 100644 --- a/README.md +++ b/README.md @@ -92,7 +92,7 @@ API endpoints, background services, their activation status, and the tests that ¹ `NETWORK_NAME` env var controls the string returned. `IS_MAINNET=true` flips the default to `"Mainnet"`. ² Proof generation routes through the Plonky2 cyclic-recursion circuit. Single host, single Rust process — no zkVM, no external prover service. Mac Studio M3 Ultra is the production hardware target (96 GB unified memory, no external GPU). See [Proving Strategy](#proving-strategy). ³ Requires `PUBLISHER_KEY` set to a real funded key and `ESPLORA_URL` reachable. With the default test key the node panics on `IS_MAINNET=true` startup; on testnet it accepts the call but broadcast will fail without funded UTXOs — DEV and PRD both return `503 SERVICE_UNAVAILABLE` to the client on broadcast failure (the historic `DEV_SKIP_BROADCAST_FAILURE` env-gate that silently swallowed these failures was removed once DEV and PRD were unified on the MVP-only binary; the DEV publisher wallet therefore has to be funded for E2E paths). -⁴ Scanner depends on `ESPLORA_URL` (REST, used for the per-block `get_block_txids` / `get_tx` lookups and for the post-reconnect tip anchor) AND `ESPLORA_WS_URL` (WebSocket, used by `scanner_ws` to receive new-tip events — issue #84). Both default to mutinynet endpoints; on connection failure the WS subscriber reconnects with exponential backoff capped at 30 s. +⁴ Scanner depends on `ESPLORA_URL` (REST, used for the per-block `get_block_txids` / `get_tx` lookups and for the post-reconnect tip anchor) AND `ESPLORA_WS_URL` (WebSocket, used by `scanner_ws` to receive new-tip events — issue #84). Both are required env vars with no default; see [Configuration](#configuration) for per-stage values. On connection failure the WS subscriber reconnects with exponential backoff capped at 30 s. ### Cargo features @@ -215,15 +215,17 @@ Features tagged `mvp` whose current test coverage is insufficient — these bloc ### Configuration -| Variable | Default | Effect | -| --------------- | --------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `ESPLORA_URL` | `https://mutinynet.com/api` | Esplora REST API endpoint (electrs or public) | -| `ESPLORA_WS_URL` | `wss://mutinynet.com/api/v1/ws` | Esplora WebSocket endpoint consumed by `scanner_ws` (issue #84). Override only when the upstream WS path changes | -| `IS_MAINNET` | `false` | `true` for Bitcoin Mainnet, `false` for Mutinynet/Signet | -| `NETWORK_NAME` | `Mutinynet` / `Mainnet` | Human-readable name returned by `/api/info`. Default depends on `IS_MAINNET` | -| `USERNAME_DOMAIN` | _(required, no default)_ | External hostname returned by `/api/info`. The client renders `@` from this. **Node panics on startup if unset.** PRD sets `zkcoins.app`, DEV sets `dev.zkcoins.app` — silent fallback would let a misconfigured stage reproduce the cross-network routing bug (#95) | -| `PUBLISHER_KEY` | test key | 32-byte hex private key for inscription publishing. **Required on mainnet** — node panics on startup if default test key is detected with `IS_MAINNET=true` | -| `RUST_LOG` | `info` | Log level | +| Variable | Default | Effect | +| ----------------- | -------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `IS_MAINNET` | _(required, no default)_ | Exact string `true` or `false` — anything else panics. PRD sets `true`, DEV sets `false`. Drives the `Network` enum (Mainnet vs Signet) used for address derivation. Truthy values like `1`, `TRUE`, `yes` are rejected to prevent silent misconfiguration. | +| `ESPLORA_URL` | _(required, no default)_ | HTTP Esplora endpoint for the chain this stage serves. PRD: `http://electrs-mainnet:3000` (DFX Mainnet stack). DEV: `http://electrs-mutinynet:3000`. Self-host: your electrs URL. Empty string is treated as unset. | +| `ESPLORA_WS_URL` | _(required, no default)_ | Esplora-compatible WebSocket endpoint consumed by `scanner_ws` (issue #84). PRD: `wss://mempool.space/api/v1/ws`. DEV: `ws://mempool-api-mutinynet:8999/api/v1/ws` on the DFX mempool/backend stack. Empty string is treated as unset. | +| `NETWORK_NAME` | `Mutinynet` / `Mainnet` | Human-readable name returned by `/api/info`. Default depends on `IS_MAINNET`. Purely cosmetic — has no behavioural effect on the scanner, publisher, or address derivation. | +| `USERNAME_DOMAIN` | _(required, no default)_ | External hostname returned by `/api/info`. The client renders `@` from this. **Node panics on startup if unset.** PRD sets `zkcoins.app`, DEV sets `dev.zkcoins.app` — silent fallback would let a misconfigured stage reproduce the cross-network routing bug (#95) | +| `PUBLISHER_KEY` | _(required, no default)_ | 32-byte hex private key for inscription publishing. Node panics on startup if unset. On `IS_MAINNET=true` an additional check refuses the well-known test key. | +| `RUST_LOG` | `info` | Log level | + +**Why so many required env vars.** Earlier versions of this table listed Mutinynet defaults for the three chain-shaping vars (`IS_MAINNET`, `ESPLORA_URL`, `ESPLORA_WS_URL`). They were silent footguns: a Mainnet deployment that forgot one would scan Mutinynet while answering `/api/info` as Mainnet, with `/health/ready` green throughout (5-s HTTP retry loop on the scanner — issue #84). On the Mutinynet path the WS default coupled the deploy to a public third-party host we do not operate. Making both paths explicit-or-panic — the same pattern as `USERNAME_DOMAIN`, `PUBLISHER_KEY`, and `DATABASE_URL` — removes both classes of bug. A mechanical guardrail (`node/tests/no_chain_hardcodes.rs`) prevents the literal URLs from creeping back into the source. Runtime config above shapes _behaviour_ of compiled-in routes. _Which_ routes are compiled in is decided at build time by Cargo features — see [Cargo features](#cargo-features). diff --git a/node/src/bin/recover_inscription.rs b/node/src/bin/recover_inscription.rs index 357a0f9f..110a8ed3 100644 --- a/node/src/bin/recover_inscription.rs +++ b/node/src/bin/recover_inscription.rs @@ -40,9 +40,14 @@ //! funds. Recovery aborts if the recovered reveal does not spend //! this address. //! +//! Required flags (chain endpoint): +//! - `--esplora-url ` — HTTP Esplora endpoint for the chain +//! the inscription was committed against. Required, no default — +//! a silent Mutinynet fallback would broadcast a Mainnet recovery +//! against the wrong chain. Same contract as the node binary's +//! `ESPLORA_URL` env var (see `lib::build_network_config_from_env`). +//! //! Optional flags: -//! - `--esplora-url ` — Esplora REST endpoint. Defaults to -//! `https://mutinynet.com/api`. //! - `--dry-run` — log the reveal hex and exit without broadcasting. use std::process::ExitCode; @@ -57,8 +62,6 @@ use esplora_client::{ use node::publisher; -const DEFAULT_ESPLORA_URL: &str = "https://mutinynet.com/api"; - #[derive(Debug)] struct CliArgs { commit_txid: String, @@ -76,7 +79,7 @@ fn print_usage(program: &str) { --commitment-hex \\ --commit-value \\ --anchor-address \\ - [--esplora-url ] \\ + --esplora-url \\ [--dry-run] env: PUBLISHER_KEY (required, 32-byte hex), IS_MAINNET (required, true|false) @@ -131,7 +134,11 @@ fn parse_args(argv: Vec) -> Result { let commit_value = commit_value.ok_or_else(|| "--commit-value is required".to_string())?; let anchor_address = anchor_address.ok_or_else(|| "--anchor-address is required".to_string())?; - let esplora_url = esplora_url.unwrap_or_else(|| DEFAULT_ESPLORA_URL.to_string()); + let esplora_url = esplora_url.ok_or_else(|| { + "--esplora-url is required (no default — silent fallback would \ + broadcast against the wrong chain)" + .to_string() + })?; Ok(CliArgs { commit_txid, @@ -196,12 +203,30 @@ fn validate_args(args: CliArgs, network: Network) -> Result Network { - let is_mainnet = std::env::var("IS_MAINNET") - .map(|v| v == "true") - .unwrap_or(false); + let is_mainnet_raw = std::env::var("IS_MAINNET").expect( + "IS_MAINNET env var must be set explicitly to `true` or `false` — \ + no default exists. Match the env of the node whose inscription \ + you are recovering (PRD: true, DEV: false).", + ); + let is_mainnet = match is_mainnet_raw.as_str() { + "true" => true, + "false" => false, + other => panic!( + "IS_MAINNET must be exactly `true` or `false`, got `{}`. \ + Truthy values like `1`, `TRUE`, or `yes` are rejected to \ + prevent silent misconfiguration.", + other + ), + }; let label = std::env::var("NETWORK_NAME").unwrap_or_else(|_| { if is_mainnet { "Mainnet".to_string() diff --git a/node/src/lib.rs b/node/src/lib.rs index c8a6b20b..538b677f 100644 --- a/node/src/lib.rs +++ b/node/src/lib.rs @@ -60,75 +60,97 @@ use zkcoins_program::hash::HashDigest; /// `lazy_static` cell (whose state would leak across tests in the same /// binary). /// -/// ## Mainnet vs Mutinynet defaults +/// ## No silent chain defaults /// -/// `ESPLORA_URL` and `ESPLORA_WS_URL` have Mutinynet defaults -/// (`https://mutinynet.com/api`, `wss://mutinynet.com/api/v1/ws`) -/// throughout the codebase. They are convenient for DEV (Mutinynet -/// the chain) and harmless for unit/integration tests. But on Mainnet -/// they are silent footguns: an `IS_MAINNET=true` deployment that -/// forgets to set either env publishes Mutinynet block events into -/// the scanner and / or fetches the wrong chain over REST. The -/// failure mode is asymmetric — an HTTP-only mismatch panics quickly -/// on the first publisher round-trip, but the event-driven scanner -/// (#84) sits in a 5 s HTTP-retry loop with no forward progress and -/// a green `/health/ready`. +/// Three env vars shape the chain the node binds to: /// -/// To remove the footgun, both URLs are **required env vars when -/// `IS_MAINNET=true`** — the panic mirrors the existing pattern for -/// `PUBLISHER_KEY`, `USERNAME_DOMAIN`, and `DATABASE_URL`. Empty- -/// string values are treated as unset on the Mainnet path so a -/// misconfigured compose file (`ESPLORA_URL=`) panics with the same -/// diagnostic instead of silently producing `EsploraConfig.url = ""`. -/// When `IS_MAINNET` is unset or `false`, the Mutinynet defaults -/// continue to apply — DEV, the pre-push hook, and the M3 Ultra -/// coverage gate are all unaffected. +/// - `IS_MAINNET` (`true` | `false`) +/// - `ESPLORA_URL` (HTTP Esplora endpoint) +/// - `ESPLORA_WS_URL` (mempool.space-compatible WS endpoint) /// -/// ## Scope of the guard +/// All three are **required, with no default** — the builder panics if +/// any one is missing or empty. This matches the existing pattern for +/// `PUBLISHER_KEY`, `USERNAME_DOMAIN`, and `DATABASE_URL`, and exists +/// because the previous "default to Mutinynet" fallbacks created two +/// distinct silent footguns: /// -/// Only the `NETWORK_CONFIG` access path is hardened here. -/// `scanner_ws::ScannerWsConfig::from_env` and `publisher.rs` still -/// call `std::env::var("ESPLORA_WS_URL")` independently with a -/// Mutinynet fallback. In the production binary the panic in this -/// builder fires before any of those reads — `main.rs` dereferences -/// `NETWORK_CONFIG` during bootstrap — so the structural bypass is -/// unreachable today. A follow-up that has those sites consume -/// `NETWORK_CONFIG.ws_url` (or an explicit `&EsploraConfig`) directly -/// would close the bypass for future entry points and is tracked as -/// a separate refactor. +/// 1. A Mainnet deployment that forgot `ESPLORA_URL` / `ESPLORA_WS_URL` +/// would silently scan Mutinynet and answer `/api/info` as Mainnet — +/// visible only as a 5 s HTTP-retry loop on `scanner_runtime` with a +/// green `/health/ready` (zk-coins/node #84). +/// 2. A Mutinynet deployment that left `ESPLORA_WS_URL` unset would +/// couple itself to the public `wss://mutinynet.com/api/v1/ws` +/// endpoint we do not operate — DEV's entire WS observability would +/// hang on a third-party host going offline. +/// +/// Removing the defaults closes both. Every stage (PRD, DEV, +/// integration tests, the local dev loop, self-hosters) must state +/// explicitly which chain it serves and which endpoints it reaches. +/// +/// `IS_MAINNET` accepts only the exact strings `"true"` or `"false"`; +/// anything else panics. This prevents the historical class of +/// "I typed `1`, `TRUE`, or `yes`, it was silently treated as Mutinynet" +/// bugs. +/// +/// Empty-string values (`ESPLORA_URL=` in a compose file) are treated +/// as unset so they panic with the same diagnostic instead of +/// silently producing `EsploraConfig.url = ""`. +/// +/// `NETWORK_NAME` remains a derived label for `/api/info` only — it +/// has no behavioural effect on the scanner, publisher, or address +/// derivation, so it keeps a default of `"Mainnet"` / `"Mutinynet"` +/// derived from `IS_MAINNET`. +/// +/// ## Single source of truth +/// +/// `NETWORK_CONFIG.url` and `NETWORK_CONFIG.ws_url` are the only +/// places these endpoints are read. `scanner_ws::ScannerWsConfig` is +/// constructed via `from_network_config(&EsploraConfig)`; the +/// publisher consumes the same struct. There is no second `env::var` +/// path that could fall back to a hardcoded chain URL. pub fn build_network_config_from_env(env: F) -> EsploraConfig where F: Fn(&str) -> Option, { - // Treat empty strings as "unset" on the Mainnet path. Without - // this, `ESPLORA_URL=` in a compose file bypasses the `expect` - // below and leaves `EsploraConfig.url = ""` — the same class of - // silent misconfiguration the panic is designed to surface. + // Treat empty / whitespace-only strings as "unset" so an + // `ESPLORA_URL=` line in a compose file panics with the same + // diagnostic as a missing variable instead of silently producing + // an empty URL — same class of silent misconfiguration. let env_or_unset = |k: &str| env(k).filter(|v| !v.trim().is_empty()); - let is_mainnet = env_or_unset("IS_MAINNET").as_deref() == Some("true"); - let url = if is_mainnet { - env_or_unset("ESPLORA_URL").expect( - "IS_MAINNET=true requires ESPLORA_URL to be set to a non-empty value — \ - the Mutinynet default is unsafe on Mainnet. Set ESPLORA_URL \ - to a Mainnet HTTP Esplora endpoint (e.g. http://electrs-mainnet:3000 \ - on the DFX Mainnet stack, or https://mempool.space/api)", - ) - } else { - env_or_unset("ESPLORA_URL").unwrap_or_else(|| "https://mutinynet.com/api".to_string()) - }; - let ws_url = if is_mainnet { - Some(env_or_unset("ESPLORA_WS_URL").expect( - "IS_MAINNET=true requires ESPLORA_WS_URL to be set to a non-empty value — \ - the Mutinynet default (wss://mutinynet.com/api/v1/ws) is unsafe on \ - Mainnet: the event-driven scanner subscribes to Mutinynet block \ - events and 404s against the Mainnet HTTP Esplora in a 5 s retry \ - loop with no forward progress (zk-coins/node #84). Set \ - ESPLORA_WS_URL to a Mainnet mempool.space-compatible WebSocket \ - (e.g. wss://mempool.space/api/v1/ws)", - )) - } else { - env_or_unset("ESPLORA_WS_URL") + + let is_mainnet_raw = env_or_unset("IS_MAINNET").expect( + "IS_MAINNET env var must be set explicitly to `true` or `false` — \ + no default exists. PRD sets `IS_MAINNET=true`, DEV sets \ + `IS_MAINNET=false`. Self-hosters and integration tests must \ + set it explicitly too.", + ); + let is_mainnet = match is_mainnet_raw.as_str() { + "true" => true, + "false" => false, + other => panic!( + "IS_MAINNET must be exactly `true` or `false`, got `{}`. \ + Truthy values like `1`, `TRUE`, or `yes` are rejected to \ + prevent silent misconfiguration (a typo used to land you \ + on Mutinynet).", + other + ), }; + + let url = env_or_unset("ESPLORA_URL").expect( + "ESPLORA_URL env var must be set — no default exists. Set it \ + to the HTTP Esplora endpoint for the chain this stage serves; \ + see README §Configuration for the per-stage endpoints.", + ); + + let ws_url = env_or_unset("ESPLORA_WS_URL").expect( + "ESPLORA_WS_URL env var must be set — no default exists. Set \ + it to the Esplora-compatible WebSocket endpoint for the \ + chain this stage serves; see README §Configuration for the \ + per-stage endpoints. The previous default fell back to a \ + public third-party host, coupling availability to a service \ + we do not operate (zk-coins/node #84).", + ); + let network_name = env_or_unset("NETWORK_NAME").unwrap_or_else(|| { if is_mainnet { "Mainnet".to_string() @@ -136,19 +158,12 @@ where "Mutinynet".to_string() } }); - println!( - "Network config: {} ({}) ws={}", - network_name, - url, - ws_url - .as_deref() - .unwrap_or(crate::scanner_ws::DEFAULT_ESPLORA_WS_URL) - ); + println!("Network config: {} ({}) ws={}", network_name, url, ws_url); EsploraConfig { url, is_mainnet, network_name, - ws_url, + ws_url: Some(ws_url), } } diff --git a/node/src/main.rs b/node/src/main.rs index 9c4feb5a..5f10c6c9 100644 --- a/node/src/main.rs +++ b/node/src/main.rs @@ -190,9 +190,10 @@ async fn main() -> Result<(), Box> { // initial `blocks` seed produces on subscribe (3-15 entries // observed), bounded so a stuck consumer cannot grow the // queue without bound. - let ws_config = ScannerWsConfig::from_env(); + let ws_config = ScannerWsConfig::from_network_config(network_config); println!( - "Event-driven scanner: WS={} (override via ESPLORA_WS_URL)", + "Event-driven scanner: WS={} (sourced from NETWORK_CONFIG; \ + set via ESPLORA_WS_URL — required, no default)", ws_config.url ); let (tip_tx, tip_rx) = mpsc::channel::(64); diff --git a/node/src/main_tests.rs b/node/src/main_tests.rs index 490b358a..c1e753e8 100644 --- a/node/src/main_tests.rs +++ b/node/src/main_tests.rs @@ -8,8 +8,12 @@ use testcontainers_modules::postgres::Postgres; // --- build_network_config_from_env ------------------------------- // -// These tests cover the panic-on-missing rules for the Mainnet path. -// They use a fake `env` closure rather than `std::env::set_var` so +// These tests cover the "explicit-or-panic" contract: every chain- +// shaping env var (`IS_MAINNET`, `ESPLORA_URL`, `ESPLORA_WS_URL`) is +// required, with no default. No stage — PRD, DEV, integration, the +// local dev loop — gets a silent Mutinynet fallback. +// +// Tests use a fake `env` closure rather than `std::env::set_var` so // the panic side-effect cannot poison the `NETWORK_CONFIG` // lazy_static cell (shared across tests in this binary) and so the // tests do not race other test threads via the process-wide @@ -30,80 +34,114 @@ fn fake_env(entries: &'static [(&'static str, &'static str)]) -> impl Fn(&str) - } #[test] -fn build_network_config_defaults_to_mutinynet_when_is_mainnet_unset() { - let cfg = build_network_config_from_env(fake_env(&[])); - assert!(!cfg.is_mainnet); - assert_eq!(cfg.url, "https://mutinynet.com/api"); - assert_eq!(cfg.network_name, "Mutinynet"); - assert!(cfg.ws_url.is_none()); -} - -#[test] -fn build_network_config_defaults_to_mutinynet_when_is_mainnet_is_not_true() { - // Any value other than the literal string "true" is treated as - // "not mainnet" — same semantics as the legacy `.map(|v| v == "true")` - // pattern. Guards against accidental "TRUE" / "1" / "yes" thinking - // it switches the network. - let cfg = build_network_config_from_env(fake_env(&[("IS_MAINNET", "1")])); - assert!(!cfg.is_mainnet); - assert_eq!(cfg.url, "https://mutinynet.com/api"); - assert!(cfg.ws_url.is_none()); -} - -#[test] -fn build_network_config_respects_explicit_urls_on_mutinynet() { +fn build_network_config_full_mutinynet() { let cfg = build_network_config_from_env(fake_env(&[ - ("ESPLORA_URL", "http://electrs-mutinynet:3000"), - ("ESPLORA_WS_URL", "wss://example.test/ws"), - ("NETWORK_NAME", "Custom"), + ("IS_MAINNET", "false"), + ("ESPLORA_URL", "http://electrs-mutinynet.test:3000"), + ("ESPLORA_WS_URL", "ws://mutinynet-ws.test/api/v1/ws"), ])); assert!(!cfg.is_mainnet); - assert_eq!(cfg.url, "http://electrs-mutinynet:3000"); - assert_eq!(cfg.ws_url.as_deref(), Some("wss://example.test/ws")); - assert_eq!(cfg.network_name, "Custom"); + assert_eq!(cfg.url, "http://electrs-mutinynet.test:3000"); + assert_eq!( + cfg.ws_url.as_deref(), + Some("ws://mutinynet-ws.test/api/v1/ws") + ); + assert_eq!(cfg.network_name, "Mutinynet"); } #[test] fn build_network_config_full_mainnet() { let cfg = build_network_config_from_env(fake_env(&[ ("IS_MAINNET", "true"), - ("ESPLORA_URL", "http://electrs-mainnet:3000"), - ("ESPLORA_WS_URL", "wss://mempool.space/api/v1/ws"), + ("ESPLORA_URL", "http://electrs-mainnet.test:3000"), + ("ESPLORA_WS_URL", "wss://mainnet-ws.test/api/v1/ws"), ])); assert!(cfg.is_mainnet); - assert_eq!(cfg.url, "http://electrs-mainnet:3000"); - assert_eq!(cfg.ws_url.as_deref(), Some("wss://mempool.space/api/v1/ws")); + assert_eq!(cfg.url, "http://electrs-mainnet.test:3000"); + assert_eq!( + cfg.ws_url.as_deref(), + Some("wss://mainnet-ws.test/api/v1/ws") + ); assert_eq!(cfg.network_name, "Mainnet"); } #[test] -fn build_network_config_mainnet_with_explicit_network_name() { - // Mainnet path with `NETWORK_NAME` set: the override must win - // over the `if is_mainnet { "Mainnet" } else { "Mutinynet" }` - // default branch. Documents that operators can rename the chain - // label (e.g. "Mainnet-Canary") without changing IS_MAINNET. +fn build_network_config_explicit_network_name_overrides_default_label() { + // `NETWORK_NAME` is the only env var that remains derived (purely + // cosmetic — feeds `/api/info`). Operators can override it without + // touching IS_MAINNET, e.g. "Mainnet-Canary". let cfg = build_network_config_from_env(fake_env(&[ ("IS_MAINNET", "true"), - ("ESPLORA_URL", "http://electrs-mainnet:3000"), - ("ESPLORA_WS_URL", "wss://mempool.space/api/v1/ws"), + ("ESPLORA_URL", "http://electrs-mainnet.test:3000"), + ("ESPLORA_WS_URL", "wss://mainnet-ws.test/api/v1/ws"), ("NETWORK_NAME", "Mainnet-Canary"), ])); assert!(cfg.is_mainnet); assert_eq!(cfg.network_name, "Mainnet-Canary"); } +// --- panic paths: IS_MAINNET ------------------------------------ + +#[test] +#[should_panic(expected = "IS_MAINNET env var must be set")] +fn build_network_config_panics_on_missing_is_mainnet() { + // No silent default to false (Mutinynet). Every stage must say + // explicitly which chain it serves. + let _ = build_network_config_from_env(fake_env(&[ + ("ESPLORA_URL", "http://electrs-mainnet.test:3000"), + ("ESPLORA_WS_URL", "wss://mainnet-ws.test/api/v1/ws"), + ])); +} + #[test] -#[should_panic(expected = "IS_MAINNET=true requires ESPLORA_URL")] -fn build_network_config_panics_on_mainnet_missing_esplora_url() { +#[should_panic(expected = "IS_MAINNET env var must be set")] +fn build_network_config_panics_on_empty_is_mainnet() { + // `IS_MAINNET=` in a compose file → empty string → treated as + // unset, same as missing. + let _ = build_network_config_from_env(fake_env(&[ + ("IS_MAINNET", ""), + ("ESPLORA_URL", "http://electrs-mainnet.test:3000"), + ("ESPLORA_WS_URL", "wss://mainnet-ws.test/api/v1/ws"), + ])); +} + +#[test] +#[should_panic(expected = "IS_MAINNET must be exactly `true` or `false`")] +fn build_network_config_panics_on_truthy_is_mainnet() { + // Historical class of bugs: a typed `1`, `TRUE`, or `yes` used + // to silently mean Mutinynet. Reject ambiguous values loudly. + let _ = build_network_config_from_env(fake_env(&[ + ("IS_MAINNET", "1"), + ("ESPLORA_URL", "http://electrs-mainnet.test:3000"), + ("ESPLORA_WS_URL", "wss://mainnet-ws.test/api/v1/ws"), + ])); +} + +// --- panic paths: ESPLORA_URL ----------------------------------- + +#[test] +#[should_panic(expected = "ESPLORA_URL env var must be set")] +fn build_network_config_panics_on_missing_esplora_url_mainnet() { let _ = build_network_config_from_env(fake_env(&[ ("IS_MAINNET", "true"), - ("ESPLORA_WS_URL", "wss://mempool.space/api/v1/ws"), + ("ESPLORA_WS_URL", "wss://mainnet-ws.test/api/v1/ws"), ])); } #[test] -#[should_panic(expected = "IS_MAINNET=true requires ESPLORA_URL")] -fn build_network_config_panics_on_mainnet_empty_esplora_url() { +#[should_panic(expected = "ESPLORA_URL env var must be set")] +fn build_network_config_panics_on_missing_esplora_url_mutinynet() { + // Symmetric: even on the non-Mainnet path, an unset ESPLORA_URL + // now panics. Previously fell back silently to mutinynet.com. + let _ = build_network_config_from_env(fake_env(&[ + ("IS_MAINNET", "false"), + ("ESPLORA_WS_URL", "ws://mutinynet-ws.test/api/v1/ws"), + ])); +} + +#[test] +#[should_panic(expected = "ESPLORA_URL env var must be set")] +fn build_network_config_panics_on_empty_esplora_url() { // `ESPLORA_URL=` in a compose file resolves to `Some("")`. Without // the empty-string filter in `env_or_unset`, the `expect` would // be bypassed and `EsploraConfig.url` would be left as `""` — @@ -112,27 +150,42 @@ fn build_network_config_panics_on_mainnet_empty_esplora_url() { let _ = build_network_config_from_env(fake_env(&[ ("IS_MAINNET", "true"), ("ESPLORA_URL", ""), - ("ESPLORA_WS_URL", "wss://mempool.space/api/v1/ws"), + ("ESPLORA_WS_URL", "wss://mainnet-ws.test/api/v1/ws"), ])); } +// --- panic paths: ESPLORA_WS_URL -------------------------------- + #[test] -#[should_panic(expected = "IS_MAINNET=true requires ESPLORA_WS_URL")] -fn build_network_config_panics_on_mainnet_missing_esplora_ws_url() { +#[should_panic(expected = "ESPLORA_WS_URL env var must be set")] +fn build_network_config_panics_on_missing_esplora_ws_url_mainnet() { let _ = build_network_config_from_env(fake_env(&[ ("IS_MAINNET", "true"), - ("ESPLORA_URL", "http://electrs-mainnet:3000"), + ("ESPLORA_URL", "http://electrs-mainnet.test:3000"), + ])); +} + +#[test] +#[should_panic(expected = "ESPLORA_WS_URL env var must be set")] +fn build_network_config_panics_on_missing_esplora_ws_url_mutinynet() { + // Symmetric: an unset ESPLORA_WS_URL now panics even on the + // non-Mainnet path. Previously DEV silently bound to the public + // `wss://mutinynet.com/api/v1/ws` — an external host we do not + // operate. + let _ = build_network_config_from_env(fake_env(&[ + ("IS_MAINNET", "false"), + ("ESPLORA_URL", "http://electrs-mutinynet.test:3000"), ])); } #[test] -#[should_panic(expected = "IS_MAINNET=true requires ESPLORA_WS_URL")] -fn build_network_config_panics_on_mainnet_whitespace_esplora_ws_url() { +#[should_panic(expected = "ESPLORA_WS_URL env var must be set")] +fn build_network_config_panics_on_whitespace_esplora_ws_url() { // Whitespace-only values are also rejected — same misconfiguration // class as the empty string, just easier to miss in a diff. let _ = build_network_config_from_env(fake_env(&[ ("IS_MAINNET", "true"), - ("ESPLORA_URL", "http://electrs-mainnet:3000"), + ("ESPLORA_URL", "http://electrs-mainnet.test:3000"), ("ESPLORA_WS_URL", " "), ])); } diff --git a/node/src/publisher.rs b/node/src/publisher.rs index c81f40d9..3ea4497c 100644 --- a/node/src/publisher.rs +++ b/node/src/publisher.rs @@ -29,9 +29,15 @@ pub struct EsploraConfig { pub is_mainnet: bool, pub network_name: String, /// Esplora WebSocket endpoint consumed by the block-tip scanner - /// (`scanner_ws::run_scanner_ws`). `None` falls back to - /// `ESPLORA_WS_URL` (defaulting to `wss://mutinynet.com/api/v1/ws`). - /// The publisher no longer uses this field — see + /// (`scanner_ws::run_scanner_ws`). Sourced from the `ESPLORA_WS_URL` + /// env var via `lib::build_network_config_from_env`, which panics + /// if it is unset or empty — production callers always observe a + /// `Some(...)` here. The `Option` shape is retained to keep this + /// struct constructible from test fixtures that do not need a WS + /// URL (publisher-only paths) without forcing a placeholder URL + /// into the type. + /// + /// The publisher itself does not use this field — see /// `broadcast_inscription_txs` for the direct-broadcast rationale. pub ws_url: Option, } diff --git a/node/src/runtime_tests.rs b/node/src/runtime_tests.rs index 86a87042..087ec5d7 100644 --- a/node/src/runtime_tests.rs +++ b/node/src/runtime_tests.rs @@ -93,8 +93,15 @@ async fn start_rest_node_binds_and_serves_health() { // happen on first access in this test binary. The pre-push hook // exports both of these already; setting them here defensively // makes the test runnable in any environment. + // `NETWORK_CONFIG` is a process-wide `lazy_static` cell — the first + // test in this binary that touches it freezes the values for the + // rest of the run. All three chain-shaping env vars are required + // (see `lib::build_network_config_from_env`), so set them + // defensively here even though pre-push exports them too. std::env::set_var("USERNAME_DOMAIN", "test.zkcoins.local"); + std::env::set_var("IS_MAINNET", "false"); std::env::set_var("ESPLORA_URL", "http://127.0.0.1:1/api"); + std::env::set_var("ESPLORA_WS_URL", "ws://127.0.0.1:1/api/v1/ws"); // PR-A3 moved all sibling-file state (accounts.bin, usernames.bin, // minting_num_pubkeys.bin) into Postgres; the bootstrap only needs @@ -203,8 +210,15 @@ async fn bootstrap_initial_minting_account_balance_is_goldilocks_safe() { drop(probe); let addr = format!("127.0.0.1:{}", port); + // `NETWORK_CONFIG` is a process-wide `lazy_static` cell — the first + // test in this binary that touches it freezes the values for the + // rest of the run. All three chain-shaping env vars are required + // (see `lib::build_network_config_from_env`), so set them + // defensively here even though pre-push exports them too. std::env::set_var("USERNAME_DOMAIN", "test.zkcoins.local"); + std::env::set_var("IS_MAINNET", "false"); std::env::set_var("ESPLORA_URL", "http://127.0.0.1:1/api"); + std::env::set_var("ESPLORA_WS_URL", "ws://127.0.0.1:1/api/v1/ws"); let tmp = std::env::temp_dir().join(format!( "zkcoins-balance-test-{}-{}", diff --git a/node/src/scanner_ws.rs b/node/src/scanner_ws.rs index c5d35b1a..b373730f 100644 --- a/node/src/scanner_ws.rs +++ b/node/src/scanner_ws.rs @@ -1,11 +1,12 @@ //! Event-driven chain ingestion via the Esplora WebSocket stream. //! //! Subscribes to the mempool.space-compatible WebSocket endpoint -//! (`ESPLORA_WS_URL`, default `wss://mutinynet.com/api/v1/ws`) and -//! publishes each new tip `BlockHash` into an `mpsc::Sender` that the -//! existing `scanner_runtime` drains. Replaces the 30-s tip polling -//! loop that previously gated `/api/mint` and `/api/send` visibility -//! by up to a full block-time + poll-interval (issue #84). +//! (`ESPLORA_WS_URL` — required env var, no default; see +//! `lib::build_network_config_from_env`) and publishes each new tip +//! `BlockHash` into an `mpsc::Sender` that the existing +//! `scanner_runtime` drains. Replaces the 30-s tip polling loop that +//! previously gated `/api/mint` and `/api/send` visibility by up to a +//! full block-time + poll-interval (issue #84). //! //! TODO(structured-logging): this module still uses `println!` / //! `eprintln!` for runtime logs, consistent with the rest of the @@ -69,16 +70,9 @@ use futures_util::{SinkExt, StreamExt}; use tokio::sync::mpsc; use tokio_tungstenite::tungstenite::Message as WsMessage; +use crate::publisher::EsploraConfig; pub use crate::scanner_ws_parse::parse_ws_frame; -/// Default endpoint for Mutinynet's mempool.space-compatible WebSocket -/// API. Overridable via `ESPLORA_WS_URL` for self-host operators and -/// for DEV failover (the URL is not officially documented for -/// Mutinynet, but it follows the upstream mempool.space convention -/// and was smoke-tested against `wss://mutinynet.com/api/v1/ws` and -/// `wss://mempool.space/signet/api/v1/ws` before this PR landed). -pub const DEFAULT_ESPLORA_WS_URL: &str = "wss://mutinynet.com/api/v1/ws"; - /// Default for the liveness watchdog. A real new block arrives at /// least every ~10 min on any live signet/mainnet, so 90 s with no /// frame at all (including `pong` / keep-alives) is a strong "the @@ -132,10 +126,6 @@ pub const DEFAULT_RECONNECT_MIN: Duration = Duration::from_millis(500); /// down for that long, we are no worse off than before. pub const DEFAULT_RECONNECT_MAX: Duration = Duration::from_secs(30); -/// Default fallback when no `ESPLORA_URL` is in the environment. -/// Kept in sync with `lib.rs::NETWORK_CONFIG`. -pub const DEFAULT_ESPLORA_HTTP_URL: &str = "https://mutinynet.com/api"; - /// Wall-clock budget for completing a single WS connect handshake. /// A half-broken middlebox can stall the TCP handshake for the /// kernel SYN-retransmit budget (60-180 s on Linux/Darwin); bound it @@ -191,15 +181,18 @@ async fn connect_with_timeout( } } -/// Runtime knobs for the scanner WS task. Sensible defaults are -/// exposed via `from_env`; tests construct it directly with shorter -/// timeouts. +/// Runtime knobs for the scanner WS task. The URL pair is sourced +/// from the central `NETWORK_CONFIG` via `from_network_config`; tests +/// construct it directly with shorter timeouts. #[derive(Clone, Debug)] pub struct ScannerWsConfig { - /// Esplora WebSocket URL. Default: `DEFAULT_ESPLORA_WS_URL`. + /// Esplora WebSocket URL. Sourced from `ESPLORA_WS_URL` via + /// `lib::build_network_config_from_env` — no default exists. pub url: String, /// HTTP Esplora URL used to fetch the current tip after each - /// reconnect (plugs gaps that opened while disconnected). + /// reconnect (plugs gaps that opened while disconnected). Sourced + /// from `ESPLORA_URL` via `lib::build_network_config_from_env` — + /// no default exists. pub http_url: String, /// Initial reconnect delay. Doubles up to `reconnect_max`. pub reconnect_min: Duration, @@ -218,17 +211,27 @@ pub struct ScannerWsConfig { } impl ScannerWsConfig { - /// Read the config from the environment, falling back to the - /// defaults documented above. Logged once at startup by the - /// caller in `main.rs`. - pub fn from_env() -> Self { - let url = - std::env::var("ESPLORA_WS_URL").unwrap_or_else(|_| DEFAULT_ESPLORA_WS_URL.to_string()); - let http_url = - std::env::var("ESPLORA_URL").unwrap_or_else(|_| DEFAULT_ESPLORA_HTTP_URL.to_string()); + /// Build the config from an already-resolved `EsploraConfig`. The + /// single env-resolution path lives in + /// `lib::build_network_config_from_env`, which panics on missing + /// `ESPLORA_URL` / `ESPLORA_WS_URL` — by the time this runs both + /// URLs are guaranteed non-empty. + /// + /// `network_config.ws_url` is `Option` for legacy reasons + /// (the publisher does not need it); production callers pass a + /// config built by `build_network_config_from_env`, which always + /// populates it. The `expect` here documents that invariant — + /// hitting it means somebody constructed an `EsploraConfig` + /// manually without setting `ws_url`, which is a programmer + /// error, not a runtime configuration issue. + pub fn from_network_config(network_config: &EsploraConfig) -> Self { + let url = network_config + .ws_url + .clone() + .expect("EsploraConfig.ws_url must be set — production callers go through build_network_config_from_env"); Self { url, - http_url, + http_url: network_config.url.clone(), reconnect_min: DEFAULT_RECONNECT_MIN, reconnect_max: DEFAULT_RECONNECT_MAX, liveness_timeout: DEFAULT_LIVENESS_TIMEOUT, diff --git a/node/src/scanner_ws_tests.rs b/node/src/scanner_ws_tests.rs index b19bbe97..69cfc784 100644 --- a/node/src/scanner_ws_tests.rs +++ b/node/src/scanner_ws_tests.rs @@ -289,18 +289,43 @@ async fn run_scanner_ws_force_reconnects_on_liveness_timeout() { } // ----------------------------------------------------------------------------- -// Smoke — `from_env` +// Smoke — `from_network_config` // ----------------------------------------------------------------------------- #[test] -fn scanner_ws_config_from_env_uses_defaults_when_unset() { - // Don't touch the process-wide env; just verify the defaults - // are exposed via `DEFAULT_*` constants and that the struct - // assembles. The full `from_env` round-trip is exercised by the - // bootstrap in `main.rs`. - assert_eq!(DEFAULT_ESPLORA_WS_URL, "wss://mutinynet.com/api/v1/ws"); - assert_eq!(DEFAULT_LIVENESS_TIMEOUT, Duration::from_secs(90)); - assert!(DEFAULT_RECONNECT_MIN < DEFAULT_RECONNECT_MAX); +fn scanner_ws_config_from_network_config_threads_urls_from_esplora_config() { + // `ScannerWsConfig` is now derived from `EsploraConfig` — there is + // no parallel env-resolution path. This smoke test pins the + // happy-path wiring (both URLs copied through, timing knobs taken + // from the module defaults) so a future refactor that swaps the + // mapping silently is caught here. + let esplora = crate::publisher::EsploraConfig { + url: "http://electrs-test:3000".to_string(), + is_mainnet: false, + network_name: "Test".to_string(), + ws_url: Some("ws://ws-test:8999/api/v1/ws".to_string()), + }; + let cfg = ScannerWsConfig::from_network_config(&esplora); + assert_eq!(cfg.url, "ws://ws-test:8999/api/v1/ws"); + assert_eq!(cfg.http_url, "http://electrs-test:3000"); + assert_eq!(cfg.liveness_timeout, DEFAULT_LIVENESS_TIMEOUT); + assert!(cfg.reconnect_min < cfg.reconnect_max); +} + +#[test] +#[should_panic(expected = "EsploraConfig.ws_url must be set")] +fn scanner_ws_config_from_network_config_panics_on_missing_ws_url() { + // Production callers go through `build_network_config_from_env`, + // which guarantees `ws_url = Some(...)`. The `expect` documents + // the invariant; this test ensures it panics loudly if a hand- + // constructed `EsploraConfig` ever omits the field. + let esplora = crate::publisher::EsploraConfig { + url: "http://electrs-test:3000".to_string(), + is_mainnet: false, + network_name: "Test".to_string(), + ws_url: None, + }; + let _ = ScannerWsConfig::from_network_config(&esplora); } // ----------------------------------------------------------------------------- diff --git a/node/tests/no_chain_hardcodes.rs b/node/tests/no_chain_hardcodes.rs new file mode 100644 index 00000000..5af56105 --- /dev/null +++ b/node/tests/no_chain_hardcodes.rs @@ -0,0 +1,126 @@ +//! Guardrail: production Rust source must not embed literal chain +//! URLs. +//! +//! ## Why +//! +//! The bias removed in PR `feat/require-explicit-chain-config` was +//! exactly this: a default URL literal in code (`pub const +//! DEFAULT_ESPLORA_WS_URL: &str = "wss://mutinynet.com/api/v1/ws";`) +//! made the wrong chain reachable from a silent fallback path. The +//! single hardest part of preventing the same class of bug from +//! recurring is mechanical: a literal URL in production code is the +//! one footgun-class a reviewer cannot easily catch on a one-line +//! diff that "moves a default into a sensible place". +//! +//! This test fails the build if any of the eight URL-prefix patterns +//! below appear as a string literal anywhere under `node/src/` +//! except `*_tests.rs` and inside comments (`//`, `///`, `//!`). +//! Doc-comments and inline comments are allowed because they +//! frequently reference public URLs for operator context — none of +//! those strings ever reach a runtime read. +//! +//! ## Scope +//! +//! - Scans every `.rs` file under `node/src/` (recursive). +//! - Skips files ending in `_tests.rs` — tests legitimately +//! instantiate URLs to exercise builder shapes and mock servers. +//! - Skips comment lines after a textual prefix strip. +//! - Forbids the eight `://` prefixes that would +//! silently bind to one of the public Mutinynet or mempool.space +//! hosts. Other chain URLs (e.g. self-hosted electrs hostnames +//! like `electrs-mainnet:3000`) are not on this list because they +//! are stage-specific config values, not public-internet +//! defaults. +//! +//! ## When this fails +//! +//! 1. Move the literal into a panic message describing the env var +//! that should be set instead — see +//! `lib::build_network_config_from_env`. +//! 2. Or move it behind a `//` comment if it is operator guidance. +//! 3. Or place it in a `*_tests.rs` file if it is a test fixture. + +use std::fs; +use std::path::{Path, PathBuf}; + +const FORBIDDEN_PREFIXES: &[&str] = &[ + "\"https://mutinynet.com", + "\"http://mutinynet.com", + "\"wss://mutinynet.com", + "\"ws://mutinynet.com", + "\"https://mempool.space", + "\"http://mempool.space", + "\"wss://mempool.space", + "\"ws://mempool.space", +]; + +#[test] +fn no_chain_url_literals_in_production_node_source() { + let crate_root = Path::new(env!("CARGO_MANIFEST_DIR")); + let src_root = crate_root.join("src"); + assert!( + src_root.is_dir(), + "expected `{}` to exist; guardrail must run against the live source tree", + src_root.display() + ); + + let mut offenders: Vec = Vec::new(); + visit_rs_files(&src_root, &mut |path| { + let name = path + .file_name() + .and_then(|s| s.to_str()) + .expect("source path should be valid UTF-8"); + if name.ends_with("_tests.rs") { + return; + } + let body = fs::read_to_string(path) + .unwrap_or_else(|e| panic!("failed to read {}: {}", path.display(), e)); + for (lineno, line) in body.lines().enumerate() { + // Strip leading whitespace and skip comment lines so + // doc-comments and inline comments are allowed to + // reference public URLs for operator context. We do not + // attempt to skip mid-line comments — a `//` after code + // on the same line is rare in this crate and false + // positives there would be the right kind of noise. + let trimmed = line.trim_start(); + if trimmed.starts_with("//") { + continue; + } + for forbidden in FORBIDDEN_PREFIXES { + if line.contains(forbidden) { + offenders.push(format!( + "{}:{}: forbidden chain URL literal `{}` in production source", + path.display(), + lineno + 1, + forbidden.trim_start_matches('"') + )); + } + } + } + }); + + if !offenders.is_empty() { + panic!( + "\n\nGuardrail violation — literal chain URLs in production source.\n\n\ + Chain URLs must be sourced from `ESPLORA_URL` / `ESPLORA_WS_URL` env vars \ + via `lib::build_network_config_from_env`, never hardcoded. Move offending \ + strings into:\n - a panic / expect message (referring to the env var), or\n \ + - a comment line, or\n - a `*_tests.rs` test fixture.\n\nOffending sites:\n{}\n", + offenders.join("\n"), + ); + } +} + +fn visit_rs_files(dir: &Path, visit: &mut dyn FnMut(&Path)) { + let entries = + fs::read_dir(dir).unwrap_or_else(|e| panic!("failed to read dir {}: {}", dir.display(), e)); + let mut paths: Vec = entries.filter_map(|e| e.ok().map(|e| e.path())).collect(); + paths.sort(); + for path in paths { + if path.is_dir() { + visit_rs_files(&path, visit); + } else if path.extension().and_then(|s| s.to_str()) == Some("rs") { + visit(&path); + } + } +} From 150e9cf120b41347b0ebc18a25b6d3eef09e976a Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Sun, 31 May 2026 13:44:25 +0200 Subject: [PATCH 4/5] docs(readme): add Bitcoin chain column to the Live table (#146) The hostname does not tell you which chain a stage backs: api.zkcoins.app runs against Mainnet (electrs-mainnet, IS_MAINNET=true), dev-api.zkcoins.app runs against Mutinynet (electrs-mutinynet, IS_MAINNET=false). Without this column a reader has to cross-reference the deploy compose or trip over the Mutinynet-flavoured source defaults (NETWORK_NAME, DEFAULT_ESPLORA_*_URL) and conclude PRD is on Mutinynet. --- README.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 17cefab0..94d28fdc 100644 --- a/README.md +++ b/README.md @@ -9,10 +9,10 @@ Container images: **[hub.docker.com/r/zkcoins/node](https://hub.docker.com/r/zkc ## Live -| Environment | URL | Image | -| ----------- | -------------------------------------------------- | ------------------------------------------------------------------------------------ | -| **PRD** | [api.zkcoins.app](https://api.zkcoins.app) | [`zkcoins/node:latest`](https://hub.docker.com/r/zkcoins/node/tags?name=latest) | -| **DEV** | [dev-api.zkcoins.app](https://dev-api.zkcoins.app) | [`zkcoins/node:beta`](https://hub.docker.com/r/zkcoins/node/tags?name=beta) | +| Environment | URL | Bitcoin chain | Image | +| ----------- | -------------------------------------------------- | ------------- | ------------------------------------------------------------------------------------ | +| **PRD** | [api.zkcoins.app](https://api.zkcoins.app) | Mainnet | [`zkcoins/node:latest`](https://hub.docker.com/r/zkcoins/node/tags?name=latest) | +| **DEV** | [dev-api.zkcoins.app](https://dev-api.zkcoins.app) | Mutinynet | [`zkcoins/node:beta`](https://hub.docker.com/r/zkcoins/node/tags?name=beta) | ## Stack From dd5bc281d3ddadf40c02cb71cefbe596ff0cd0e6 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Sun, 31 May 2026 15:36:50 +0200 Subject: [PATCH 5/5] fix(commit): sync SMT update in broadcast_commit_and_deliver (#152) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bring the send-commit path in line with the mint-commit Phase E so the SMT integration completes synchronously before /api/commit returns 200, closing the race window where a follow-up send reads `account.commitment_public_key` from server state but finds no matching SMT entry yet (the async scanner has not observed the on-chain inscription). The race surfaced as 422 "Unable to get merkle proofs for provided public key" in the regression test `second_send_roundtrip_succeeds_without_prev_commitment_pubkey_field` when run against dev-api.zkcoins.app: a wallet that chains /api/send + /api/commit + /api/send hit the second send before the scanner finished its ~20 s reveal-observation lap on Mutinynet. There was no server code regression — the send path had always relied exclusively on the scanner to integrate the commit, while the mint path already did it inline after the broadcast (Phase E, router.rs::mint_handler). The asymmetry was latent and Mutinynet latency made it visible. Changes: * Extract the Phase E body (state.update + atomic persist_state_and_mark_complete_tx) into a shared helper `apply_commit_and_persist_phase_e` in router.rs. The helper takes a `flow_label` for logging and returns a structured `PhaseEFailure` so the two call sites can preserve their existing flow-specific public error strings ("mint broadcast..." vs "commit broadcast..."). * `mint_handler` now delegates Phase E to the helper. Behaviour is byte-identical for happy path and both Err arms; existing tests (`mint_handler_advances_state_synchronously_with_broadcast`, `mint_handler_atomic_tx_rollback_leaves_state_and_row_consistent`, `mint_handler_in_process_state_advance_collision_returns_503`) continue to pass. * `broadcast_commit_and_deliver` now invokes the helper synchronously between the Bitcoin broadcast and the recipient `receive_coin` mutation. On failure: 503, no retry, no fallback — scanner-replay remains the single source of repair, exactly as in the mint flow. * `broadcast_commit_and_deliver` also switches from the process-wide `NETWORK_CONFIG` lazy_static to `state.esplora_config` so the send-commit path becomes testable with a wiremock Esplora, matching the testability shape already in place for `mint_handler`. Production behaviour is unchanged because `start_rest_node` clones `NETWORK_CONFIG` into that slot. * Update the `commit_handler` doc to describe the new Phase E symmetry and remove the stale "no analogue of the mint state-desync class here" sentence. * Add two new tests in `router_tests.rs` mirroring the existing mint Phase E coverage: - `commit_handler_advances_state_synchronously_with_broadcast` — happy path: /api/send + /api/commit with mocked accepting Esplora and live Postgres, then verify SMT contains pk_0, MMR leaf_count == 1, root_indices has the new entry, and pending_inscriptions row sits at `complete` so `should_skip_scanner_state_update` fires. - `commit_handler_atomic_tx_rollback_leaves_state_and_row_consistent` — install a trigger that fails the in-tx UPDATE to `complete`, assert 503 with the expected error substring, and verify on-disk SMT/MMR/root_index stays untouched and the row stays at `reveal_broadcast` for scanner-replay to integrate from chain. Lock topology and crash-recovery contract are preserved verbatim (both documented in the helper's docstring). The scanner remains the authoritative path for external recovery inscriptions but is now a redundant observer for our own send commits too — exactly as it already was for mint commits. --- node/src/router.rs | 314 ++++++++++++++++----------- node/src/router_tests.rs | 443 +++++++++++++++++++++++++++++++++++++++ node/src/runtime.rs | 84 ++++++-- 3 files changed, 700 insertions(+), 141 deletions(-) diff --git a/node/src/router.rs b/node/src/router.rs index ee1572c7..a53a367a 100644 --- a/node/src/router.rs +++ b/node/src/router.rs @@ -96,6 +96,150 @@ pub(crate) fn lock_or_recover(mutex: &Mutex) -> MutexGuard<'_, T> { }) } +/// Phase E failure modes returned by [`apply_commit_and_persist_phase_e`]. +/// +/// Each variant maps 1:1 to the two distinct error arms in the shared +/// helper: an in-process `state.update` rejection (typically an SMT +/// key-collision-with-different-value, observed-but-rare), or a +/// post-update durable-write rollback. The caller (mint or send) maps +/// the variant onto its own flow-tagged response string so the public +/// error message stays exactly as the wallet-side +/// `KNOWN_SERVER_ERRORS` table expects per endpoint. +#[derive(Debug)] +pub(crate) enum PhaseEFailure { + /// `update_and_snapshot_for_persist` returned an `Err` — the + /// in-process SMT/MMR could not be advanced (typical cause: SMT + /// key collision with different value). The broadcast already + /// landed on chain; the scanner-replay path will reconcile. + StateUpdate, + /// `persist_state_and_mark_complete_tx` failed — the atomic tx + /// rolled back so SMT/MMR/root_index AND the + /// `pending_inscriptions.status -> 'complete'` advance all stayed + /// at their pre-call values on disk. The in-memory SMT/MMR HAVE + /// already mutated; on restart `State::load_from_pg` returns the + /// pre-update on-disk state and the scanner-replay path heals. + DurablePersist, +} + +/// Apply a freshly-broadcast commitment to the in-memory SMT + MMR +/// and persist the resulting snapshot **atomically** with the matching +/// `pending_inscriptions.status -> 'complete'` advance. +/// +/// This is the shared Phase E body invoked by both flows that originate +/// inscriptions on this node: +/// * [`mint_handler`] — for mint commits, immediately after +/// `create_and_broadcast_inscription` returns Ok. +/// * [`crate::runtime::broadcast_commit_and_deliver`] — for send +/// commits, immediately after the user-signed commitment is +/// broadcast. +/// +/// The symmetry matters: before this helper existed, the send path +/// relied exclusively on the async scanner to observe the commit on +/// chain and run `state.update` itself. That left a race window in +/// which a wallet could chain `/api/send` + `/api/commit` and then +/// issue a second `/api/send` whose proof-build walks the SMT for the +/// first send's commitment — and finds it missing because the scanner +/// hadn't yet observed the new inscription (especially on Mutinynet +/// where reveal-broadcast → scanner-observe sits at tens of seconds). +/// Running Phase E synchronously here closes that window: by the time +/// the handler responds 200, the SMT entry for the just-broadcast +/// commitment is committed in memory AND on disk, and the scanner +/// will skip its redundant integration via +/// `should_skip_scanner_state_update`. The scanner remains the +/// authoritative path for external / recovery inscriptions. +/// +/// ## Lock topology (preserved across both callers) +/// The function acquires `state.account_node` only to clone its +/// `Arc>` reference, then drops the account-node guard +/// **before** acquiring the state guard. `std::sync::Mutex` is held +/// only across the synchronous `update_and_snapshot_for_persist` call +/// and is released before the async `persist_state_and_mark_complete_tx` +/// — keeping a `std::sync::Mutex` off any `.await` boundary. +/// +/// ## Error handling (no fallbacks) +/// On Err the caller logs and converts to 503. There is **no in-process +/// retry, no spawn-async-retry, no half-state cleanup attempt** — the +/// scanner-replay path is the single source of repair, identical for +/// mint and send. See the memory rule on no-fallbacks for why this is +/// not a robustness gap. +pub(crate) async fn apply_commit_and_persist_phase_e( + state: &AppState, + commitment: &Commitment, + commit_txid_bytes: &[u8; 32], + flow_label: &'static str, +) -> Result { + // Test-only deterministic hold between the broadcast result and + // the phase-3b state advance. Pre-unlocked in all `test_state` + // constructors so production-shaped tests acquire + drop in one + // step. Holding the guard across a colliding SMT injection lets + // the in-process state.update Err test observe the collision when + // the handler's `state.update` finally runs. Production builds + // compile this out entirely (the field does not exist). + #[cfg(test)] + drop(state.state_advance_release_lock.lock().await); + + let state_advance_outcome = { + let state_arc_for_advance = { + let account_node_guard = lock_or_recover(&state.account_node); + account_node_guard.state().clone() + }; + let mut state_guard = lock_or_recover(&state_arc_for_advance); + state_guard.update_and_snapshot_for_persist(std::slice::from_ref(commitment)) + }; + let (new_root, smt_bytes, mmr_bytes, root_index_entry) = match state_advance_outcome { + Ok(snapshot) => snapshot, + Err(e) => { + // The in-process SMT/MMR could not be advanced — typically + // an SMT key-collision-with-different-value. The broadcast + // already landed on chain; the publisher already advanced + // the row to `reveal_broadcast` BEFORE the broadcast call, + // so the scanner-replay path will pick the inscription up + // from chain and run state.update against the un-mutated + // SMT. + eprintln!( + "{}: in-process state.update failed: {} (broadcast already landed; scanner-replay will reconcile)", + flow_label, e + ); + return Err(PhaseEFailure::StateUpdate); + } + }; + let root_index_ref = root_index_entry.as_ref().map(|(p, s, i)| (p, s, *i as u64)); + match db::persist_state_and_mark_complete_tx( + &state.pool, + &smt_bytes, + &mmr_bytes, + root_index_ref, + &commit_txid_bytes[..], + ) + .await + { + Ok(()) => { + println!( + "{}: state.update persisted + row marked complete. New MMR root: {}", + flow_label, + hex::encode(zkcoins_program::hash::digest_to_bytes(&new_root)) + ); + Ok(new_root) + } + Err(e) => { + // The atomic tx rolled back: SMT/MMR/root_index AND the + // row advance all stayed at their pre-call values on disk. + // The in-memory SMT/MMR HAVE already been mutated (that + // happened above before the await), so they are now ahead + // of disk by exactly one leaf. On restart, + // `State::load_from_pg` returns the pre-update on-disk + // state and the scanner-replay path walks the block, + // observes the row at `reveal_broadcast`, and integrates + // the inscription itself — a clean heal. + eprintln!( + "{}: atomic persist + mark-complete failed: {} (scanner-replay will heal)", + flow_label, e + ); + Err(PhaseEFailure::DurablePersist) + } + } +} + // Define a struct for our application state #[derive(Clone)] pub(crate) struct AppState { @@ -1192,122 +1336,27 @@ async fn mint_handler( // Apply the freshly-broadcast commitment to the in-memory SMT + MMR // and persist the resulting snapshot — together with the // `pending_inscriptions.status = 'complete'` row advance — in ONE - // atomic Postgres transaction (`persist_state_and_mark_complete_tx`). - // The scanner's pre-state.update lookup uses that `complete` marker - // to skip its own redundant integration when it later observes the - // same commit on chain. - // - // Rationale (this is the regression Phase E fixes): the scanner - // observed a mint's commit ~20-30 s after `/api/mint` returned 200. - // A wallet that issued a second mint inside that window walked - // `derive_num_pubkeys_from_smt` against the un-updated SMT, signed - // with the same pubkey index as the first mint, and surfaced - // `Unable to get mmr inclusion proof for the previous root` at the - // prover. Advancing `state.update` synchronously here closes the - // window: the second mint's SMT walk sees the first mint's entry - // immediately. The scanner becomes a redundant observer for our - // own inscriptions and remains the authoritative path for external - // recovery inscriptions and out-of-band commits. - // - // Lock topology: the state lock is acquired AFTER the broadcast - // completes (broadcasting is slow and would otherwise serialize - // all `/api/mint` requests behind a single in-flight inscription). - // - // Crash-recovery contract (the BLOCKER this commit fixed): the - // previous two-step shape (persist SMT/MMR/root_index, then a - // standalone UPDATE to `complete`) opened a window where the - // SMT/MMR/root_index could land on disk while the row stayed at - // `reveal_broadcast`. On restart, `State::load_from_pg` rebuilt the - // in-memory state WITH the new leaf, the scanner re-scanned the - // block, observed `reveal_broadcast` → `should_skip_scanner_state_update` - // returned `false`, and `state.update` ran a second time — the SMT - // insert was an idempotent no-op (same key+value) but - // `mmr.append(leaf)` appended a DUPLICATE leaf, diverging the MMR - // root. The atomic single-tx persist + mark-complete below - // guarantees that on success, the scanner-skip predicate will - // correctly fire on replay. On tx failure, the row stays at - // `reveal_broadcast` and the in-memory state advance was NOT - // persisted to disk (transaction atomicity); the scanner will - // replay cleanly. - // Test-only deterministic hold between the broadcast result and - // the phase-3b state advance. Pre-unlocked in all `test_state` - // constructors so production-shaped tests acquire + drop in one - // step. The in-process state.update Err test holds the guard - // across a colliding SMT injection so the handler observes the - // collision when its `state.update` finally runs. Production - // builds compile this out entirely (the field does not exist). - #[cfg(test)] - drop(state.state_advance_release_lock.lock().await); - - let state_advance_outcome = { - let state_arc_for_advance = { - let account_node_guard = lock_or_recover(&state.account_node); - account_node_guard.state().clone() - }; - let mut state_guard = lock_or_recover(&state_arc_for_advance); - state_guard.update_and_snapshot_for_persist(std::slice::from_ref(&commitment)) - }; - let (new_root, smt_bytes, mmr_bytes, root_index_entry) = match state_advance_outcome { - Ok(snapshot) => snapshot, - Err(e) => { - // The in-process SMT/MMR could not be advanced — typically - // an SMT key-collision-with-different-value (a concurrent - // mint race that slipped the phase-2 re-derive gate, or a - // genuine bug). The broadcast already landed on chain, but - // the caller's mint was NOT integrated synchronously. The - // publisher already advanced the row to `reveal_broadcast` - // BEFORE the broadcast call; we keep it there so the - // scanner-replay path will pick the inscription up from - // chain and run state.update against the un-mutated SMT. - // Return 503 so the wallet knows the mint did NOT land - // synchronously and can poll for completion. - eprintln!( - "mint_handler: in-process state.update failed: {} (broadcast already landed; scanner-replay will reconcile)", - e - ); - return handler_error_response( - StatusCode::SERVICE_UNAVAILABLE, - "mint broadcast landed on chain but in-process state advance failed; scanner will reconcile", - ); - } - }; - let root_index_ref = root_index_entry.as_ref().map(|(p, s, i)| (p, s, *i as u64)); - match db::persist_state_and_mark_complete_tx( - &state.pool, - &smt_bytes, - &mmr_bytes, - root_index_ref, - &commit_txid_bytes, - ) - .await + // atomic Postgres transaction. The shared implementation lives in + // [`apply_commit_and_persist_phase_e`], which is also invoked from + // the send path in [`crate::runtime::broadcast_commit_and_deliver`] + // so the two flows that originate inscriptions on this node both + // integrate them synchronously and the scanner becomes a redundant + // observer for our own commits. See the helper's docstring for the + // full rationale (race window, lock topology, crash-recovery + // contract). + if let Err(failure) = + apply_commit_and_persist_phase_e(&state, &commitment, &commit_txid_bytes, "mint_handler") + .await { - Ok(()) => { - println!( - "mint_handler: state.update persisted + row marked complete. New MMR root: {}", - hex::encode(zkcoins_program::hash::digest_to_bytes(&new_root)) - ); - } - Err(e) => { - // The atomic tx rolled back: SMT/MMR/root_index AND - // the row advance all stayed at their pre-call values - // on disk. The in-memory SMT/MMR HAVE already been - // mutated (that happened above before the await), so - // they are now ahead of disk by exactly one leaf. - // On restart, `State::load_from_pg` returns the - // pre-update on-disk state and the scanner-replay path - // walks the block, observes the row at - // `reveal_broadcast`, and integrates the inscription - // itself — a clean heal. Return 503 so the caller - // knows the durable state did not advance. - eprintln!( - "mint_handler: atomic persist + mark-complete failed: {} (scanner-replay will heal)", - e - ); - return handler_error_response( - StatusCode::SERVICE_UNAVAILABLE, - "mint broadcast landed on chain but durable state advance failed; scanner will reconcile", - ); - } + let msg: &'static str = match failure { + PhaseEFailure::StateUpdate => { + "mint broadcast landed on chain but in-process state advance failed; scanner will reconcile" + } + PhaseEFailure::DurablePersist => { + "mint broadcast landed on chain but durable state advance failed; scanner will reconcile" + } + }; + return handler_error_response(StatusCode::SERVICE_UNAVAILABLE, msg); } // ---- 4. COMMIT phase (broadcast OK) --------------------------------- @@ -1454,18 +1503,29 @@ async fn get_proof_handler( /// Accepts a client-signed commitment for a previously generated proof. /// Broadcasts the commitment as a Taproot inscription and delivers the coin to the recipient. /// -/// **Broadcast-then-deliver invariant (zk-coins/node#89).** Unlike -/// the mint flow, the `/api/commit` endpoint receives a *proof_id* the -/// node already generated (in an earlier `/api/send` call), looks up -/// the persisted `CoinProof`, broadcasts its commitment, and only then -/// hands the proof to `receive_coin` for the recipient mutation. The -/// in-memory mutation lives in [`broadcast_commit_and_deliver`] in -/// `runtime.rs`; the broadcast call sits at the very top of -/// that function and returns 503 on failure with NO subsequent state -/// mutation, so there is no analogue of the mint state-desync class -/// here. DO NOT reorder the broadcast and the `receive_coin` call — -/// the audit in zk-coins/node#89 verified this ordering is correct -/// and any future refactor must preserve it. +/// **Broadcast-then-deliver invariant (zk-coins/node#89).** The +/// `/api/commit` endpoint receives a *proof_id* the node already +/// generated (in an earlier `/api/send` call), looks up the persisted +/// `CoinProof`, broadcasts its commitment, advances the SMT/MMR via +/// the shared Phase E helper synchronously, and only then hands the +/// proof to `receive_coin` for the recipient mutation. The in-memory +/// mutation + persistence lives in [`broadcast_commit_and_deliver`] in +/// `runtime.rs`; the broadcast call sits at the very top of that +/// function and returns 503 on failure with NO subsequent state +/// mutation. DO NOT reorder the broadcast and the `receive_coin` call. +/// +/// **Phase E symmetry (this branch).** The send-commit path now runs +/// [`apply_commit_and_persist_phase_e`] synchronously between the +/// broadcast and `receive_coin`, matching `mint_handler`. Before this +/// change the send-commit SMT integration relied exclusively on the +/// async scanner, which left a race window where a wallet that +/// followed `/api/send` + `/api/commit` with a second `/api/send` +/// would walk the SMT for the first commit's pubkey and find it +/// missing — surfacing as 422 `"Unable to get merkle proofs for +/// provided public key"`. The synchronous Phase E call closes that +/// window; the scanner remains the authoritative path for external +/// recovery inscriptions but is now a redundant observer for our own +/// send commits, exactly as for mint commits. async fn commit_handler( State(state): State, Json(request): Json, diff --git a/node/src/router_tests.rs b/node/src/router_tests.rs index 12a3a880..731672d3 100644 --- a/node/src/router_tests.rs +++ b/node/src/router_tests.rs @@ -5864,3 +5864,446 @@ async fn r2_probe_history_limit_clamped_to_max() { let arr: Vec = serde_json::from_str(&body).expect("valid JSON"); assert!(arr.is_empty()); } + +// --------------------------------------------------------------------------- +// Phase E (send-commit branch) — mirrors the mint Phase E tests above. +// +// `broadcast_commit_and_deliver` runs the shared +// `apply_commit_and_persist_phase_e` helper synchronously after the +// Bitcoin broadcast. The tests below assert the two load-bearing +// observable properties from outside the handler: +// +// 1. Happy path: after a 200 response the SMT contains the commit's +// pubkey, the MMR has advanced by one leaf, the matching +// `mmr_root_index` row is present, and the `pending_inscriptions` +// row sits at `complete` — so a scanner re-observation hits +// `should_skip_scanner_state_update`. +// +// 2. Atomic rollback (`PhaseEFailure::DurablePersist`): a trigger that +// blocks the in-tx UPDATE to `complete` rolls the whole transaction +// back. The handler surfaces 503; on-disk SMT/MMR/root_index stays +// unchanged; the row stays at `reveal_broadcast` so scanner-replay +// will integrate the inscription from chain. +// --------------------------------------------------------------------------- + +/// End-to-end mirror of `mint_handler_advances_state_synchronously_with_broadcast` +/// for the send-commit path. Runs `/api/send` (real prover) followed by +/// `/api/commit` (real broadcast against a wiremock Esplora that +/// accepts both the UTXO lookup and the `POST /tx`), then asserts the +/// in-memory and on-disk Phase E aftermath that closes the second-send +/// race (the regression that `second_send_roundtrip_succeeds_without_prev_commitment_pubkey_field` +/// surfaced against Mutinynet). +#[tokio::test] +async fn commit_handler_advances_state_synchronously_with_broadcast() { + use bitcoin::bip32::{ChildNumber, Xpriv, Xpub}; + use bitcoin::hashes::Hash as _; + use bitcoin::secp256k1::{Keypair, PublicKey, SecretKey}; + use testcontainers::{runners::AsyncRunner, ImageExt}; + use testcontainers_modules::postgres::Postgres; + + let pg_container = Postgres::default() + .with_tag("17") + .start() + .await + .expect("failed to start postgres container"); + let host = pg_container.get_host().await.unwrap(); + let port = pg_container.get_host_port_ipv4(5432).await.unwrap(); + let url = format!("postgres://postgres:postgres@{}:{}/postgres", host, port); + let pool = Arc::new( + crate::db::connect_and_migrate(&url) + .await + .expect("connect_and_migrate failed"), + ); + + // `mint_broadcast_mock_server` is publisher-key-agnostic — same + // `0x01` SecretKey used for every test-mode broadcast. It accepts + // the UTXO GET and the `POST /tx` so the commit-side + // `create_and_broadcast_inscription` succeeds. + let mock_server = mint_broadcast_mock_server().await; + + // `test_state()` carries `dead_pool`; swap in the live pool and the + // mock Esplora before exercising the handler. + let mut state = test_state(); + state.pool = Arc::clone(&pool); + state.esplora_config = Arc::new(crate::publisher::EsploraConfig { + url: mock_server.uri(), + is_mainnet: false, + network_name: "Mutinynet".to_string(), + ws_url: None, + }); + + // Derive the same BIP-32 child keys the other commit tests use. + let secret_bytes = include_bytes!("../minting_secret.bin"); + let xpriv = Xpriv::new_master(bitcoin::Network::Signet, secret_bytes).unwrap(); + let secp = secp::Secp256k1::new(); + let pk_0: PublicKey = Xpub::from_priv(&secp, &xpriv) + .derive_pub(&secp, &[ChildNumber::Normal { index: 0 }]) + .unwrap() + .public_key; + let pk_1: PublicKey = Xpub::from_priv(&secp, &xpriv) + .derive_pub(&secp, &[ChildNumber::Normal { index: 1 }]) + .unwrap() + .public_key; + let sk_0: SecretKey = xpriv + .derive_priv(&secp, &[ChildNumber::Normal { index: 0 }]) + .unwrap() + .private_key; + + // ---- /api/send (real prover, returns the post-state hashes) ---- + let account_address = "0x".to_string() + + &hex::encode(zkcoins_program::hash::digest_to_bytes( + &zkcoins_program::types::MINTING_ADDRESS, + )); + let recipient = "0x".to_string() + &hex::encode([0xa1u8; 32]); + let amount: u64 = 1; + let now = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_secs(); + let mut hasher = Sha256::new(); + hasher.update(account_address.as_bytes()); + hasher.update(recipient.as_bytes()); + hasher.update(amount.to_le_bytes()); + hasher.update(now.to_le_bytes()); + let hash: [u8; 32] = hasher.finalize().into(); + let msg = Message::from_digest(hash); + let kp = Keypair::from_secret_key(&secp, &sk_0); + let sig = secp.sign_schnorr(&msg, &kp); + + let send_body = serde_json::json!({ + "account_address": account_address, + "recipient": recipient, + "amount": amount, + "public_key": hex::encode(pk_0.serialize()), + "next_public_key": hex::encode(pk_1.serialize()), + "signature": hex::encode(sig.serialize()), + "timestamp": now, + }); + let send_req = Request::post("/api/send") + .header("content-type", "application/json") + .body(Body::from(send_body.to_string())) + .unwrap(); + let (send_status, send_body_text) = send_request_with_state(state.clone(), send_req).await; + assert_eq!(send_status, StatusCode::OK, "send failed: {send_body_text}"); + let send_resp: serde_json::Value = serde_json::from_str(&send_body_text).unwrap(); + let proof_id = send_resp["proof_id"].as_u64().unwrap(); + let ash_hex = send_resp["account_state_hash"] + .as_str() + .unwrap() + .to_string(); + let ocr_hex = send_resp["output_coins_root"].as_str().unwrap().to_string(); + + // Pre-commit sanity: pk_0 is NOT yet in the SMT (send_coin_handler + // does not advance the SMT; that is exactly the Phase E gap this + // commit closes for the send branch). + let pk0_smt_key = bitcoin::hashes::sha256::Hash::hash(&pk_0.serialize()).to_byte_array(); + { + let node_guard = state.account_node.lock().unwrap(); + let state_arc = node_guard.state().clone(); + let state_guard = state_arc.lock().unwrap(); + assert!( + state_guard.smt.get(&pk0_smt_key).is_none(), + "post-send / pre-commit: pk_0 must NOT be in SMT yet (commit's Phase E inserts it)" + ); + assert_eq!( + state_guard.mmr.leaf_count(), + 0, + "post-send / pre-commit: MMR must be empty" + ); + } + + // ---- /api/commit (broadcast OK → Phase E runs synchronously) ---- + let ash_bytes = hex::decode(&ash_hex).unwrap(); + let ocr_bytes = hex::decode(&ocr_hex).unwrap(); + let mut commit_message = Vec::with_capacity(ash_bytes.len() + ocr_bytes.len()); + commit_message.extend_from_slice(&ash_bytes); + commit_message.extend_from_slice(&ocr_bytes); + let commitment = shared::commitment::Commitment::new(&sk_0, commit_message.clone()) + .expect("commitment creation"); + assert!(commitment.verify(), "test commitment must verify locally"); + + let commit_body = serde_json::json!({ + "proof_id": proof_id, + "public_key": hex::encode(commitment.public_key.serialize()), + "signature": hex::encode(commitment.signature.serialize()), + "message": hex::encode(&commitment.message), + }); + let commit_req = Request::post("/api/commit") + .header("content-type", "application/json") + .body(Body::from(commit_body.to_string())) + .unwrap(); + let (commit_status, commit_resp_body) = + send_request_with_state(state.clone(), commit_req).await; + assert_eq!( + commit_status, + StatusCode::OK, + "commit must succeed against accepting Esplora + live pool: {}", + commit_resp_body + ); + + // ---- Phase E aftermath: SMT/MMR/root_indices reflect the commit ---- + let state_arc = { + let node_guard = state.account_node.lock().unwrap(); + node_guard.state().clone() + }; + { + let state_guard = state_arc.lock().unwrap(); + assert!( + state_guard.smt.get(&pk0_smt_key).is_some(), + "Phase E regression: commit_handler must advance SMT with pk_0 before returning 200" + ); + assert_eq!( + state_guard.mmr.leaf_count(), + 1, + "Phase E: MMR must hold exactly one new leaf after the commit" + ); + assert!( + state_guard + .root_indices + .contains_key(&state_guard.prev_mmr_root), + "Phase E: root_indices must hold the freshly written prev_mmr_root" + ); + } + + // ---- pending_inscriptions row marked `complete` atomically ---- + let (pending_status,): (String,) = + sqlx::query_as("SELECT status FROM pending_inscriptions ORDER BY id DESC LIMIT 1") + .fetch_one(&*pool) + .await + .expect("a pending row must exist for the broadcast commit"); + assert_eq!( + pending_status, + crate::db::PENDING_STATUS_COMPLETE, + "Phase E: commit_handler must mark pending_inscriptions complete after state.update" + ); + let (commit_txid_bytes,): (Vec,) = + sqlx::query_as("SELECT commit_txid FROM pending_inscriptions ORDER BY id DESC LIMIT 1") + .fetch_one(&*pool) + .await + .expect("commit_txid column must populate"); + assert!( + crate::scanner::should_skip_scanner_state_update( + crate::db::pending_inscription_status_by_commit_txid(&pool, &commit_txid_bytes) + .await + .unwrap() + .as_deref() + ), + "scanner must skip its redundant state.update for a Phase-E-completed send commit" + ); +} + +/// Mirror of `mint_handler_atomic_tx_rollback_leaves_state_and_row_consistent` +/// for the send-commit path. Installs a `BEFORE UPDATE` trigger on +/// `pending_inscriptions` that raises an exception when the new +/// `status` value is `complete`. The trigger fires inside the atomic +/// `persist_state_and_mark_complete_tx` envelope so the SMT/MMR/ +/// root_index UPSERTs and the mark-complete UPDATE all roll back +/// together. `broadcast_commit_and_deliver` converts the failure to +/// 503; on-disk durable state is unchanged; the row stays at +/// `reveal_broadcast` so the scanner-replay path can integrate the +/// inscription from chain without doubling up the MMR leaf. +#[tokio::test] +async fn commit_handler_atomic_tx_rollback_leaves_state_and_row_consistent() { + use bitcoin::bip32::{ChildNumber, Xpriv, Xpub}; + use bitcoin::secp256k1::{Keypair, PublicKey, SecretKey}; + use testcontainers::{runners::AsyncRunner, ImageExt}; + use testcontainers_modules::postgres::Postgres; + + let pg_container = Postgres::default() + .with_tag("17") + .start() + .await + .expect("failed to start postgres container"); + let host = pg_container.get_host().await.unwrap(); + let port = pg_container.get_host_port_ipv4(5432).await.unwrap(); + let url = format!("postgres://postgres:postgres@{}:{}/postgres", host, port); + let pool = Arc::new( + crate::db::connect_and_migrate(&url) + .await + .expect("connect_and_migrate failed"), + ); + + // Trigger raises on every UPDATE that sets `status = 'complete'`. + sqlx::query( + "CREATE OR REPLACE FUNCTION fail_complete_commit() RETURNS trigger AS $$ + BEGIN + IF NEW.status = 'complete' THEN + RAISE EXCEPTION 'simulated mark-complete failure (commit)'; + END IF; + RETURN NEW; + END; + $$ LANGUAGE plpgsql", + ) + .execute(&*pool) + .await + .unwrap(); + sqlx::query( + "CREATE TRIGGER block_complete_commit BEFORE UPDATE ON pending_inscriptions \ + FOR EACH ROW EXECUTE FUNCTION fail_complete_commit()", + ) + .execute(&*pool) + .await + .unwrap(); + + let mock_server = mint_broadcast_mock_server().await; + let mut state = test_state(); + state.pool = Arc::clone(&pool); + state.esplora_config = Arc::new(crate::publisher::EsploraConfig { + url: mock_server.uri(), + is_mainnet: false, + network_name: "Mutinynet".to_string(), + ws_url: None, + }); + + let secret_bytes = include_bytes!("../minting_secret.bin"); + let xpriv = Xpriv::new_master(bitcoin::Network::Signet, secret_bytes).unwrap(); + let secp = secp::Secp256k1::new(); + let pk_0: PublicKey = Xpub::from_priv(&secp, &xpriv) + .derive_pub(&secp, &[ChildNumber::Normal { index: 0 }]) + .unwrap() + .public_key; + let pk_1: PublicKey = Xpub::from_priv(&secp, &xpriv) + .derive_pub(&secp, &[ChildNumber::Normal { index: 1 }]) + .unwrap() + .public_key; + let sk_0: SecretKey = xpriv + .derive_priv(&secp, &[ChildNumber::Normal { index: 0 }]) + .unwrap() + .private_key; + + let account_address = "0x".to_string() + + &hex::encode(zkcoins_program::hash::digest_to_bytes( + &zkcoins_program::types::MINTING_ADDRESS, + )); + let recipient = "0x".to_string() + &hex::encode([0xa2u8; 32]); + let amount: u64 = 1; + let now = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_secs(); + let mut hasher = Sha256::new(); + hasher.update(account_address.as_bytes()); + hasher.update(recipient.as_bytes()); + hasher.update(amount.to_le_bytes()); + hasher.update(now.to_le_bytes()); + let hash: [u8; 32] = hasher.finalize().into(); + let msg = Message::from_digest(hash); + let kp = Keypair::from_secret_key(&secp, &sk_0); + let sig = secp.sign_schnorr(&msg, &kp); + + let send_body = serde_json::json!({ + "account_address": account_address, + "recipient": recipient, + "amount": amount, + "public_key": hex::encode(pk_0.serialize()), + "next_public_key": hex::encode(pk_1.serialize()), + "signature": hex::encode(sig.serialize()), + "timestamp": now, + }); + let send_req = Request::post("/api/send") + .header("content-type", "application/json") + .body(Body::from(send_body.to_string())) + .unwrap(); + let (send_status, send_body_text) = send_request_with_state(state.clone(), send_req).await; + assert_eq!(send_status, StatusCode::OK, "send failed: {send_body_text}"); + let send_resp: serde_json::Value = serde_json::from_str(&send_body_text).unwrap(); + let proof_id = send_resp["proof_id"].as_u64().unwrap(); + let ash_hex = send_resp["account_state_hash"] + .as_str() + .unwrap() + .to_string(); + let ocr_hex = send_resp["output_coins_root"].as_str().unwrap().to_string(); + + let ash_bytes = hex::decode(&ash_hex).unwrap(); + let ocr_bytes = hex::decode(&ocr_hex).unwrap(); + let mut commit_message = Vec::with_capacity(ash_bytes.len() + ocr_bytes.len()); + commit_message.extend_from_slice(&ash_bytes); + commit_message.extend_from_slice(&ocr_bytes); + let commitment = shared::commitment::Commitment::new(&sk_0, commit_message.clone()) + .expect("commitment creation"); + assert!(commitment.verify(), "test commitment must verify locally"); + + let commit_body = serde_json::json!({ + "proof_id": proof_id, + "public_key": hex::encode(commitment.public_key.serialize()), + "signature": hex::encode(commitment.signature.serialize()), + "message": hex::encode(&commitment.message), + }); + let commit_req = Request::post("/api/commit") + .header("content-type", "application/json") + .body(Body::from(commit_body.to_string())) + .unwrap(); + let (commit_status, commit_resp_body) = send_request_with_state(state, commit_req).await; + + // Trigger fires inside the atomic tx → handler converts to 503. + assert_eq!( + commit_status, + StatusCode::SERVICE_UNAVAILABLE, + "atomic tx rollback must surface 503, body: {}", + commit_resp_body + ); + let v: serde_json::Value = serde_json::from_str(&commit_resp_body).expect("valid JSON"); + assert_eq!(v["success"], false); + assert!( + v["error"] + .as_str() + .unwrap_or("") + .contains("durable state advance failed"), + "response error must explain the durable-persist failure, got: {}", + v["error"] + ); + + // On-disk SMT/MMR/root_index did NOT advance — the atomic + // envelope rolled them back together with the failed UPDATE. + assert_eq!( + crate::db::load_smt(&pool).await.unwrap(), + None, + "atomic-tx rollback must leave smt_state untouched" + ); + assert_eq!( + crate::db::load_mmr(&pool).await.unwrap(), + None, + "atomic-tx rollback must leave mmr_state untouched" + ); + assert!( + crate::db::load_root_indices(&pool) + .await + .unwrap() + .is_empty(), + "atomic-tx rollback must leave mmr_root_index untouched" + ); + + // Pending row stays at `reveal_broadcast`: publisher set it there + // before the broadcast, mark-complete was the call the trigger + // blocked. Scanner-replay on next boot picks up the inscription + // and integrates it via its own state.update path. + let (pending_status,): (String,) = + sqlx::query_as("SELECT status FROM pending_inscriptions ORDER BY id DESC LIMIT 1") + .fetch_one(&*pool) + .await + .expect("a pending row must exist for the broadcasted commitment"); + assert_eq!( + pending_status, + crate::db::PENDING_STATUS_REVEAL_BROADCAST, + "atomic-tx rollback: pending row must stay at reveal_broadcast for scanner-replay to pick up" + ); + let (commit_txid_bytes,): (Vec,) = + sqlx::query_as("SELECT commit_txid FROM pending_inscriptions ORDER BY id DESC LIMIT 1") + .fetch_one(&*pool) + .await + .unwrap(); + assert!( + !crate::scanner::should_skip_scanner_state_update( + crate::db::pending_inscription_status_by_commit_txid(&pool, &commit_txid_bytes) + .await + .unwrap() + .as_deref() + ), + "scanner must NOT skip its state.update for a send commit whose mark-complete failed" + ); + + sqlx::query("DROP TRIGGER block_complete_commit ON pending_inscriptions") + .execute(&*pool) + .await + .unwrap(); +} diff --git a/node/src/runtime.rs b/node/src/runtime.rs index 5739532c..fc58e2f2 100644 --- a/node/src/runtime.rs +++ b/node/src/runtime.rs @@ -22,7 +22,10 @@ use tokio::net::TcpListener; use crate::account_node::{persist_account, CoinProof}; use crate::db; use crate::publisher::{create_and_broadcast_inscription, resume_pending_inscriptions}; -use crate::router::{lock_or_recover, SendCoinResponse}; +use crate::router::{ + apply_commit_and_persist_phase_e, handler_error_response, lock_or_recover, PhaseEFailure, + SendCoinResponse, +}; use crate::NETWORK_CONFIG; use shared::ProofData; use zkcoins_program::hash::digest_to_bytes; @@ -231,12 +234,12 @@ pub async fn start_rest_node( Ok(()) } -/// Broadcast the commit inscription and, on success, deliver the coin -/// to the recipient and persist the account state. This contains the -/// network call (Bitcoin broadcast) and the post-broadcast bookkeeping, -/// plus the success/failure response dispatch — all of which cannot be -/// exercised by unit tests, so the whole function lives in the runtime -/// module that is excluded from the coverage scope. +/// Broadcast the commit inscription and, on success, run the shared +/// Phase E (SMT/MMR advance + atomic persist + `pending_inscriptions` +/// row marked `complete`), then deliver the coin to the recipient and +/// persist the account state. This contains the network call (Bitcoin +/// broadcast) and the post-broadcast bookkeeping, plus the +/// success/failure response dispatch. /// /// **Invariant (zk-coins/node#89).** The broadcast `if let Err(...) /// { return 503 }` MUST stay above every `receive_coin`/`upsert_account` @@ -245,6 +248,20 @@ pub async fn start_rest_node( /// function does not have that bug because its broadcast is already /// the first effect. Any future refactor that moves a state mutation /// above the broadcast re-introduces the state-desync class — do not. +/// +/// **Phase E symmetry.** Between the broadcast and the recipient +/// `receive_coin` mutation, we invoke +/// [`apply_commit_and_persist_phase_e`] synchronously — identical +/// shape to `mint_handler`. Prior to this, the send-commit SMT +/// integration ran only via the async scanner, which surfaced as a +/// race for back-to-back `/api/send` + `/api/commit` + `/api/send` +/// flows: the second send walked the SMT for the first commit's +/// pubkey and found no entry, returning 422 `"Unable to get merkle +/// proofs for provided public key"`. Running Phase E inline closes +/// that window. The scanner remains the recovery path for external +/// inscriptions and re-scans of our own commits hit +/// `should_skip_scanner_state_update` because the `complete` row +/// advance lands atomically here. pub(crate) async fn broadcast_commit_and_deliver( state: &AppState, commitment: Commitment, @@ -256,19 +273,58 @@ pub(crate) async fn broadcast_commit_and_deliver( "Broadcasting user commitment ({} bytes)", commitment_data.len() ); - if let Err(err) = create_and_broadcast_inscription( + // Use `state.esplora_config` (instead of the process-wide + // `NETWORK_CONFIG` lazy_static) so tests can redirect Esplora calls + // at a `wiremock::MockServer`, matching the testability shape + // already in place for `mint_handler`. In production + // `start_rest_node` clones `NETWORK_CONFIG` into this slot so the + // runtime behaviour is unchanged. + let broadcast_outcome = create_and_broadcast_inscription( &commitment_data, crate::db::InscriptionKind::Send, - &NETWORK_CONFIG, + &state.esplora_config, Some(&state.pool), ) + .await; + let commit_txid_bytes: [u8; 32] = match broadcast_outcome { + Ok((commit_txid, _reveal_txid)) => { + use bitcoin::hashes::Hash as _; + commit_txid.to_byte_array() + } + Err(err) => { + eprintln!("Error broadcasting commit inscription: {}", err); + return handler_error_response( + StatusCode::SERVICE_UNAVAILABLE, + "Failed to broadcast commitment inscription on-chain", + ); + } + }; + + // ---- Phase E (broadcast OK) ----------------------------------------- + // Run the shared SMT/MMR advance + atomic persist + mark-complete + // BEFORE the recipient `receive_coin` mutation. Locked-step with + // `mint_handler::Phase E`; see [`apply_commit_and_persist_phase_e`] + // for the full rationale, lock topology, and crash-recovery + // contract. On failure the broadcast already landed on chain — we + // surface 503 (no fallback, no retry) and the scanner-replay path + // is the single source of repair. + if let Err(failure) = apply_commit_and_persist_phase_e( + state, + &commitment, + &commit_txid_bytes, + "broadcast_commit_and_deliver", + ) .await { - eprintln!("Error broadcasting commit inscription: {}", err); - return crate::router::handler_error_response( - StatusCode::SERVICE_UNAVAILABLE, - "Failed to broadcast commitment inscription on-chain", - ); + let msg: &'static str = match failure { + PhaseEFailure::StateUpdate => { + "commit broadcast landed on chain but in-process state advance failed; scanner will reconcile" + } + PhaseEFailure::DurablePersist => { + "commit broadcast landed on chain but durable state advance failed; scanner will reconcile" + } + }; + return handler_error_response(StatusCode::SERVICE_UNAVAILABLE, msg); } let mut updated_proof = coin_proof;