Skip to content

Stage 5d-next-5 / Step 7 housekeeping: HTTP errors + CI coverage + CI cyclic-tests + doc fold #28

Closed
Closed
Stage 5d-next-5 / Step 7 housekeeping: HTTP errors + CI coverage + CI cyclic-tests + doc fold#28

Description

@TaprootFreak
TaprootFreak
opened on May 18, 2026

Stage 5d-next-5 / Step 7 housekeeping follow-ups: HTTP errors + CI coverage + CI cyclic-tests + doc fold

Self-contained brief for a fresh Claude session. Do not assume any prior context — read the linked PRs and source paths fresh before proposing changes. Modeled on the structure of the now-closed #25 (also a self-contained brief).

TL;DR

After PR #23 (Stage 5d-next-5 Phases 1+2a+2b+3) and PR #26 (send_coins switched to in-circuit _and_sources API + off-circuit shim retained as defense-in-depth) landed, four housekeeping follow-ups were deliberately deferred. They're separable: each can be its own PR; none of them blocks the others. List in priority order:

  1. HTTP error-mapping — replace the current 200 OK + {success: false} failure path of /api/send + /api/mint with proper 4xx + structured error body. (Medium-High risk: this is an API-contract change that wallet clients consume.)
  2. CI coverage exclusions drop — remove account_server.rs + server.rs from the --ignore-filename-regex set in .github/workflows/ci.yaml. (Medium risk: the --fail-under-lines 100 gate must actually hit 100% for those files; needs local cargo llvm-cov verification.)
  3. CI cyclic-tests inclusion — remove --skip stage_5d --skip stage_5e from .github/workflows/ci.yaml and bump timeout-minutes from 30 to ~120. (High risk: GH ubuntu-latest has 7 GB RAM and ~2 GB per cyclic test; only verifiable via an actual CI run.)
  4. Doc fold — move program-plonky2/STAGE_5D_NEXT_5_AGGREGATOR.md into program-plonky2/MIGRATION_RESEARCH.md §7.22 and update cross-references. (Low risk: pure documentation movement.)

Working environment

Context (must read)

What's already landed

Why these were deferred from #25 / PR #26

Each carries a different risk profile that warrants its own decision-pass instead of expanding PR #26's scope. The defense-in-depth comment block at server/src/account_server.rs:377-389 already records the rationale for the off-circuit shim, and PR #26's body explicitly lists these four items under "Out of scope".

Item 1 — HTTP error mapping (Medium-High risk; API-contract change)

Goal

Replace the current 200 OK + {success: false} failure path of the HTTP handlers with proper status codes and a structured error body, so wallet clients can distinguish "user error" (4xx) from "server error" (5xx) and surface specific error strings.

Current state

  • server/src/server.rs::send_coin_handler (line ~370): on send_coins error, returns (StatusCode::OK, Json(SendCoinResponse { success: false, … })) (lines 495-503). The actual error string from send_coins is eprintln!-logged but never reaches the client.
  • server/src/server.rs::mint_handler (line ~507, feature-gated faucet): same pattern.
  • server/src/account_server.rs::send_coins returns Result<Vec<CoinProof>, &'static str> with these failure strings:
    • "Unknown account address"
    • "Insufficient funds"
    • "Coin is missing commitment"
    • "Should provide an inclusion proof"
    • "Coin should not exist in coin history tree"
    • "Coin should not exist in tree yet"
    • "In-coin not present in source's output_coins_root" (Phase 2b defense-in-depth shim)
    • "Source commitment not present in history MMR" (Phase 2b defense-in-depth shim)
    • "Too many in-coins for one transition"
    • "Too many out-coins for one transition"
    • "prev_commitment_pubkey required for account update"
    • "prove_initial_with_in_and_out_coins_and_sources failed"
    • "prove_account_update_with_in_and_out_coins_and_sources failed"

Implementation plan

Option A — String-based mapping (smaller change)

Map error strings to status codes in the HTTP handler:

fn map_send_coins_error(err: &str) -> (StatusCode, &str) {
    match err {
        "Unknown account address" => (StatusCode::NOT_FOUND, err),
        "Insufficient funds" => (StatusCode::UNPROCESSABLE_ENTITY, err),
        // Defense-in-depth shim rejections — caller witness is malformed
        "In-coin not present in source's output_coins_root"
        | "Source commitment not present in history MMR"
        | "Coin is missing commitment"
        | "Coin should not exist in coin history tree"
        | "Coin should not exist in tree yet"
        | "Should provide an inclusion proof"
        | "Too many in-coins for one transition"
        | "Too many out-coins for one transition" => (StatusCode::UNPROCESSABLE_ENTITY, err),
        "prev_commitment_pubkey required for account update" => (StatusCode::BAD_REQUEST, err),
        s if s.ends_with("failed") => (StatusCode::INTERNAL_SERVER_ERROR, "prove failed"),
        _ => (StatusCode::INTERNAL_SERVER_ERROR, "internal error"),
    }
}

Extend SendCoinResponse with error: Option<String>:

pub struct SendCoinResponse {
    pub success: bool,
    pub error: Option<String>,
    pub proof_id: Option<String>,
    pub account_state_hash: Option<String>,
    pub output_coins_root: Option<String>,
}

Update the Err(_) => … branches of send_coin_handler + mint_handler to use the map.

Option B — Proper error enum (cleaner, bigger change)

Refactor account_server::send_coins to return Result<Vec<CoinProof>, SendCoinsError> where SendCoinsError is an enum. Each variant maps to a status code via impl IntoResponse. Tests gain compiler-checked exhaustiveness instead of string-matching.

Memory feedback_threat_model_over_checklist: the threat model is "user sent malformed witness" vs "server crashed". Both options achieve the same external behavior; Option A is the minimum viable change. Option B is the right long-term shape but slips scope. Recommend Option A unless the user explicitly asks for B.

Tests

For each error string, a handler-level test that asserts the status code + body. Tests live in server/src/server_tests.rs. Pattern:

#[test]
fn test_send_coin_handler_maps_insufficient_funds_to_422() {
    // Spin up state with mint account at balance 0; client sends 1.
    // Assert response.status() == StatusCode::UNPROCESSABLE_ENTITY
    // Assert body.error == Some("Insufficient funds")
}

Cover at least: Unknown account address → 404, Insufficient funds → 422, the two Phase 2b shim rejections → 422, the prove_*_failed paths → 500.

Verification

  • cargo test -p server --release --all-features -- --test-threads=1 adds ~8-10 new tests, runtime impact minimal (handler tests are non-cyclic and fast).
  • Wallet client (in zk-coins/app) needs awareness — but this issue is about the server side; the client update is its own follow-up.

Acceptance criteria

  1. SendCoinResponse includes an error: Option<String> field; on success it is None.
  2. Each documented send_coins error string maps to a deterministic (StatusCode, error_body) via map_send_coins_error (or the enum equivalent under Option B).
  3. New handler-level tests cover at minimum the 5 cases listed above.
  4. The defense-in-depth shim rejection paths surface specific 422 + the error string within milliseconds (as the shim already runs in microseconds; the HTTP layer doesn't add latency).
  5. cargo test -p server --release --all-features -- --test-threads=1 green.

Risks

  • API-contract change: wallet clients consuming the API today see 200 OK + success:false. After this change they see 4xx + body.error. If wallet clients hard-code the 200 check, they break. Mitigation: coordinate with zk-coins/app maintainer (= user); explicitly call out the contract change in the PR title + body.
  • 5xx leakage: prove_*_failed is the catch-all for cyclic-recursion prover failure. If the body echoes the raw error string, an attacker could probe for prover internal state. Mitigation: collapse all *_failed strings to a generic "prove failed" body (per the Option A snippet above).

Item 2 — CI coverage exclusions drop (Medium risk; needs local llvm-cov verification)

Goal

Remove the temporary account_server.rs + server.rs exclusions from the cargo llvm-cov invocation in .github/workflows/ci.yaml. Those files were excluded while the in-circuit send_coins wiring was a Step 7 follow-up (their test modules were disabled at the include-point). PR #23 + PR #26 restored those tests; the gate can now run against the full surface.

Current state

.github/workflows/ci.yaml line 177:

- name: Run cargo-llvm-cov (MVP scope, regression guard)
  run: |
    cargo llvm-cov -p server --show-missing-lines \
      --ignore-filename-regex 'main\.rs|publisher\.rs|server_runtime\.rs|scanner_runtime\.rs|account_server\.rs|server\.rs|_tests\.rs$' \
      --fail-under-lines 100 \
      --fail-under-functions 100 \
      -- --test-threads=1

Target shape:

      --ignore-filename-regex 'main\.rs|publisher\.rs|server_runtime\.rs|scanner_runtime\.rs|_tests\.rs$' \

Implementation plan

  1. Run the proposed coverage gate locally: 
    cargo llvm-cov -p server --show-missing-lines \
  --ignore-filename-regex 'main\.rs|publisher\.rs|server_runtime\.rs|scanner_runtime\.rs|_tests\.rs$' \
  --fail-under-lines 100 \
  --fail-under-functions 100 \
  -- --test-threads=1
    Wall time: ~30-60 min on M3.
  2. Read the --show-missing-lines output for each unreached line/function in account_server.rs + server.rs.
  3. For each uncovered region:
    • If a real test path can exercise it (likely most error branches): add the test. Pattern matches server/src/account_server_tests.rs.
    • If the region is genuinely unreachable in normal operation (e.g. a Mutex::PoisonError recovery branch, a logging fallback): annotate with #[cfg_attr(coverage_nightly, coverage(off))] on the function or // LCOV_EXCL_LINE on the line. Mention WHY in the comment so future readers don't think it's slop.
  4. Re-run until cargo llvm-cov ... --fail-under-lines 100 --fail-under-functions 100 returns 0.
  5. Update .github/workflows/ci.yaml to drop the exclusions.

Verification

  • Local cargo llvm-cov pass at 100% lines / 100% functions.
  • CI run on a PR targeting develop (because the workflow only triggers there) confirms the gate still passes.

Acceptance criteria

  1. cargo llvm-cov -p server --show-missing-lines --ignore-filename-regex 'main\.rs|publisher\.rs|server_runtime\.rs|scanner_runtime\.rs|_tests\.rs$' --fail-under-lines 100 --fail-under-functions 100 -- --test-threads=1 passes locally.
  2. .github/workflows/ci.yaml --ignore-filename-regex argument no longer contains account_server\.rs or server\.rs.
  3. Any coverage(off) / LCOV_EXCL annotations added in step 3 are justified with an in-source comment explaining why the region is unreachable.

Risks

  • Coverage gap: if some lines/functions can't reasonably be tested without major refactoring, the gate may resist 100%. Mitigation: tactical coverage(off) annotations on the truly-unreachable regions (e.g. unwrap_or_else(PoisonError::into_inner) fallbacks, unreachable!() arms). Memory feedback_no_fallbacks: don't use coverage(off) as a "fallback" for real coverage gaps — only for genuinely unreachable code.
  • Local run cost: the 30-60 min llvm-cov run is non-trivial wall time. Plan to run it once, fix all gaps in a single pass, then re-run for confirmation.

Item 3 — CI cyclic-tests inclusion + timeout bump (High risk; only verifiable via CI run)

Goal

Make the GitHub-hosted CI actually run the Stage 5d / Stage 5e cyclic-recursion tests in program-plonky2, instead of --skip-ing them. Bump the tests job's timeout-minutes so the longer wall fits.

Current state

.github/workflows/ci.yaml lines 121-124:

- name: Run tests (program-plonky2, off-circuit + non-cyclic gadgets)
  run: |
    cargo test -p zkcoins-program-plonky2 --release --lib -- --test-threads=1 \
      --skip stage_5b --skip stage_5c --skip stage_5d --skip stage_5e

And line 81:

  tests:
    name: Tests
    runs-on: ubuntu-latest
    timeout-minutes: 30

Local single-threaded wall time anchors for the cyclic tests at the current production parameters (MAX_IN_COINS = MAX_OUT_COINS = 8, INNER_PAD_BITS_STAGE_5D_NEXT_5 = 15):

Test class Per-test wall (M3)
stage_5c_plus_* smokes ~40-55 s
stage_5d_next_3_* integration ~45-80 s
stage_5d_next_5_phase_2b_* positives ~99-154 s
stage_5d_next_5_phase_3_* negatives ~35-55 s
Aggregator unit tests ~25 s each

Full program-plonky2 lib sweep ~42 min wall on M3 with --test-threads=2. Single-threaded on ubuntu-latest is ~80-120 min (CPU is ~2.5× slower; no GPU acceleration available).

Implementation plan

  1. Bump timeout-minutes from 30 → 120 for the tests job (line 81).
  2. Remove --skip stage_5b --skip stage_5c --skip stage_5d --skip stage_5e (or keep --skip stage_5b --skip stage_5c if those are deprecated; verify with cargo test ... -- --list).
  3. Push to a branch targeting develop (the workflow trigger is pull_request: branches: [develop] — feat/plonky2-migration-targeted PRs do NOT trigger CI). The cleanest path: rebase this fix onto develop-after-feat/plonky2-migration-merges, or use a temporary branch off develop for verification.
  4. Watch the CI run; if it OOMs, drop the cyclic-test count or bump runner size; if it times out at 120 min, bump further.

Verification

  • CI run on a develop-targeted PR completes within 120 min with all cyclic tests green.
  • No OOM-kill (exit code 143) in any job step.

Acceptance criteria

  1. .github/workflows/ci.yaml tests job no longer --skips stage_5d / stage_5e (and optionally stage_5b / stage_5c).
  2. tests job timeout-minutes is ≥ 120.
  3. A real CI run on a develop-targeted PR completes green within the bumped timeout.

Risks

Item 4 — Doc fold: STAGE_5D_NEXT_5_AGGREGATOR.md → MIGRATION_RESEARCH.md §7.22 (Low risk; pure doc move)

Goal

Fold the standalone aggregator architecture write-up into the MIGRATION_RESEARCH.md §7.x lessons-learned series (where it logically belongs as "§7.22 — Stage 5d-next-5 source-side verification: aggregator-pattern + ConstantGate + pad-bits"), then delete the standalone file and update cross-references.

Current state

  • program-plonky2/STAGE_5D_NEXT_5_AGGREGATOR.md — 612 lines, full Phase 2a/2b architecture, empirical insights (ConstantGate::new(2) injection + helper_degree = pad_bits + 1), test coverage matrix, "How to verify Phase 2a from scratch" runbook.
  • program-plonky2/MIGRATION_RESEARCH.md — has §7.0 through §7.21 sections capturing lessons from each migration phase. §7.21 is "Stage 5d-next-4 source-side verification: blocked on Plonky2 1.1.0, deferred to Stage 5d-next-5 (post-MVP)". §7.22 would naturally be the resolution.
  • Cross-references to STAGE_5D_NEXT_5_AGGREGATOR.md exist in:

Implementation plan

  1. Open program-plonky2/MIGRATION_RESEARCH.md, find the §7.21 section, append §7.22 below it with:
    • Title: ## 7.22 Stage 5d-next-5: source-side verification via aggregator pattern (resolves §7.21)
    • Status snapshot table from STAGE_5D_NEXT_5_AGGREGATOR.md (Phases 1/2a/2b/3 all done).
    • Architecture summary — copy the ASCII box diagram.
    • Two empirical insights — copy the ConstantGate::new(2) injection and helper_degree = pad_bits + 1 sections with their probe-verified tables. These are the lessons the MIGRATION_RESEARCH doc is meant to capture.
    • Test coverage matrix — Phase 3 positives + negatives.
    • Benchmark table.
    • Verification runbook.
  2. Update cross-references. Replace each occurrence of STAGE_5D_NEXT_5_AGGREGATOR.md with MIGRATION_RESEARCH.md#722-stage-5d-next-5-source-side-verification-via-aggregator-pattern-resolves-721 (or whatever exact GitHub-anchor heading you write).
  3. Delete program-plonky2/STAGE_5D_NEXT_5_AGGREGATOR.md.
  4. Update §7.21's status note to reference §7.22 as the resolution.

Verification

git grep STAGE_5D_NEXT_5_AGGREGATOR.md           # expect: 0 hits (outside git history)
git grep "MIGRATION_RESEARCH.md.*7.22\|§7.22"    # expect: appears in the doc + each updated cross-ref
cargo doc -p zkcoins-program-plonky2 --no-deps   # expect: no broken intra-doc links

Acceptance criteria

  1. program-plonky2/STAGE_5D_NEXT_5_AGGREGATOR.md deleted.
  2. program-plonky2/MIGRATION_RESEARCH.md §7.22 contains the architecture summary, two empirical insights, test coverage matrix, benchmark, and verification runbook.
  3. git grep STAGE_5D_NEXT_5_AGGREGATOR.md outside git log returns 0 results.
  4. cargo doc succeeds with no broken intra-doc links.

Risks

  • Lost content: the standalone doc is 612 lines; the §7.22 section may need to be more compressed than a verbatim copy to fit the MIGRATION_RESEARCH.md style (which is "lessons" not "tutorials"). The full architecture / runbook content could land in §7.22 verbatim, but if §7.x sections are normally tighter, abbreviate and link to PR Stage 5d-next-5: Phases 2a + 2b + 3 LANDED (refs #19; closes the source-verification work) #23 for full detail. Mitigation: read §7.18 / §7.19 / §7.21 first to calibrate the section-length norm.

Acceptance criteria (issue-level)

A reviewer should be able to verify, in order:

  1. Each of the four items above lands in its own PR (or, if the user prefers, two PRs grouped by risk: item 4 in one, items 1+2+3 in another).
  2. cargo check --workspace --all-targets + cargo fmt --all --check + cargo clippy --workspace --all-targets --all-features -- -D warnings green after each PR.
  3. cargo test -p server --release --all-features -- --test-threads=1 green after Item 1 (with the new handler-mapping tests) and after Item 2 (with any added coverage-gap tests).
  4. cargo llvm-cov -p server --show-missing-lines --fail-under-lines 100 --fail-under-functions 100 -- --test-threads=1 green at the relaxed exclusion list after Item 2.
  5. A CI run on a develop-targeted PR completes within 120 min after Item 3.
  6. git grep STAGE_5D_NEXT_5_AGGREGATOR.md returns 0 hits outside git history after Item 4.

House rules (from caller's memories — read before pushing)

  • feedback_zkcoins_direct_develop: zkCoins repos push directly to develop, no PRs targeting develop directly. But this work is on top of feat/plonky2-migration (which is itself en route to develop via PR feat: Plonky2 / Poseidon proof-system migration #17). Add commits to a follow-up branch off feat/plonky2-migration AND open a Draft PR targeting feat/plonky2-migration. Exception for Item 3 verification: that one needs a develop-targeted PR because CI only triggers there.
  • feedback_pr_as_draft + feedback_gh_pr_draft_status: PRs always Draft; after every gh pr edit or push, re-verify isDraft: true via gh pr view N --json isDraft because the GH CLI sometimes flips it back to ready.
  • feedback_no_force_push: never git push --force except after a rebase.
  • feedback_no_squash: don't squash commits on the feature branch; the user squashes on merge.
  • feedback_local_tests_before_push: run cargo fmt --all --check, cargo clippy --workspace --all-targets --all-features -- -D warnings, and the relevant cargo test lane locally before every push.
  • feedback_ci_monitor_after_push: after every push, monitor CI; if any job is red, intervene. NOTE: PR-targeting feat/plonky2-migration doesn't trigger CI; only develop-targeted PRs do. Items 1/2/4 only get CI feedback once the parent PR feat: Plonky2 / Poseidon proof-system migration #17 merges to develop and these changes land there.
  • feedback_merge_self: never merge a PR yourself — the user does that.
  • Commit-message style: no Claude / Anthropic / AI / Generated with … / Co-Authored-By: Claude footers. Plain human-developer style. Match the existing commit-message tone in the repo (git log --oneline).
  • feedback_no_fallbacks: no ?? default, || default, empty catch, default parameters added without explicit instruction. Applies especially to Item 2's coverage-gap fixes (don't coverage(off) real gaps; only annotate genuinely unreachable regions).
  • feedback_threat_model_over_checklist: findings against the real threat model, not generic hardening. Applies to Item 1's status-code mapping (the threat is "client sends malformed witness" → 4xx vs "server crashes during prove" → 5xx; don't invent new 4xx codes for hypothetical attacks).

Reference reading order

  1. program-plonky2/STAGE_5D_NEXT_5_AGGREGATOR.md (will be folded by Item 4, but it's the canonical architecture write-up — read before doing anything).
  2. PR Stage 5d-next-5: Phases 2a + 2b + 3 LANDED (refs #19; closes the source-verification work) #23 + PR Stage 5d-next-5 follow-ups: in-circuit send_coins (defense-in-depth) + doc parity (closes #25) #26 bodies on https://github.com/zk-coins/server/pulls (full context of what just landed).
  3. server/src/account_server.rs lines 278-534 (send_coins function — the off-circuit shim at 377-401, the sources Vec build at 425-445, the prover calls at 478-501).
  4. server/src/server.rs lines 370-505 (send_coin_handler — Item 1's target).
  5. .github/workflows/ci.yaml (Items 2 + 3).
  6. program-plonky2/MIGRATION_RESEARCH.md §7.x sections (Item 4's target shape — read §7.18 + §7.19 + §7.21 to calibrate the section style).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions