Skip to content

fix(faucet+runtime): unblock DEV bootstrap after Plonky2 migration - #36

Merged
TaprootFreak merged 4 commits into
developfrom
fix/minting-secret-and-startup-robustness
May 19, 2026
Merged

fix(faucet+runtime): unblock DEV bootstrap after Plonky2 migration#36
TaprootFreak merged 4 commits into
developfrom
fix/minting-secret-and-startup-robustness

Conversation

@TaprootFreak

@TaprootFreak TaprootFreak commented May 19, 2026

Copy link
Copy Markdown
Contributor

Summary

DEV (https://dev-api.zkcoins.app) is down with Cloudflare 502 because the container is Up (unhealthy) — the tokio worker that owns the HTTP listener panicked on every cold boot after the Plonky2 migration, while the scanner worker kept processing blocks. No restart, no monitor, no visible failure.

Root cause: the Plonky2 migration (D11) moved MINTING_ADDRESS to a well-known constant (hash_bytes(b"zkcoins:minting-address:placeholder:v1")). ClientAccount::new still derives address from the privkey's first child pubkey; the assert_eq! in start_rest_server between the two could never hold again. Plus, a panic in a spawned tokio task by default only kills that task — the process happily continued in zombie state for 8 h.

What changes

  • fix(faucet) — Replace the assertion with an explicit address override on the minting ClientAccount and document the rationale. Matches the pattern already used in server_tests.rs::TestAccountData::new_minting_account.
  • feat(runtime) — Install a global panic hook that runs the default reporter and then exit(1). Any future tokio worker panic now crash-loops the container via restart: unless-stopped instead of becoming a silent zombie.
  • test(runtime) — New integration smoke test that spawns start_rest_server against an ephemeral port and probes /health over real TCP. server_runtime.rs was excluded from the coverage scope, so the bootstrap path that exploded had no test at all. ~22 s warm; runs in the existing pre-push test sweep via cargo test -p server --release --all-features.
  • ci(deploy-dev) — Post-deploy curl-retry on https://dev-api.zkcoins.app/api/info. Up to 30 × 10 s. A green "Build and deploy to DEV" with a broken upstream is no longer possible — the workflow fails, the auto-release PR loses its green check, and the regression surfaces immediately instead of hours later.

Test plan

  • CI lint+build green
  • New start_rest_server_binds_and_serves_health integration test passes (verified locally in 22 s)
  • Deploy-DEV smoke step turns red on a broken deploy (will only verify on the merge to develop)
  • After merge: https://dev-api.zkcoins.app/api/info returns 200 within ~5 min of deploy completion

@TaprootFreak
fix(faucet): force minting ClientAccount address to MINTING_ADDRESS
fc6fc11 
The Plonky2 migration (D11 in MIGRATION_RESEARCH.md) moved MINTING_ADDRESS
from a privkey-derived value to a well-known constant `hash_bytes(b"zkcoins:
minting-address:placeholder:v1")`. ClientAccount::new still derives `address`
from the privkey's first child pubkey — for the faucet wallet that derivation
is meaningless, only the commitment-signing side is used. The assert_eq! in
start_rest_server therefore panicked the tokio-rt-worker on every cold boot
after the migration, leaving the container Up-but-unresponsive on DEV.

Replace the assertion with an explicit address override and document the
rationale. Matches the pattern the test harness already uses in
server_tests.rs::TestAccountData::new_minting_account.
@TaprootFreak
feat(runtime): abort the process on any tokio panic
338cc5f 
By default a panic in a spawned tokio task only kills that task. The
chain scanner runs in one task, the HTTP bootstrap in another; when the
HTTP task panicked during the Plonky2 migration the scanner kept
processing blocks, the container stayed Up, but port 4242 was never
bound. Cloudflare served 502s for hours because there is no upstream
signal that distinguishes "alive" from "alive-but-cannot-accept".

Install a global panic hook that runs the default reporter (so the
stack trace is still logged) and then exits the process. `restart:
unless-stopped` in compose then crash-loops the container, which is
trivially visible in `docker compose ps` and surfaces immediately in
the post-deploy smoke test added in a follow-up commit.
@TaprootFreak
test(runtime): smoke-test start_rest_server bootstrap end-to-end
fb61cb5 
server_runtime.rs is excluded from the coverage scope (it binds a real
socket and owns the process lifecycle), so the bootstrap path that
exploded in the Plonky2 migration was never exercised by any test. This
adds an integration smoke test that spawns start_rest_server against an
ephemeral port and probes /health over real TCP — a bootstrap panic
manifests as a connect timeout and the test fails with a clear message.

Runs in ~22 s with a warm cargo cache and is included in the suite the
pre-push hook runs via `cargo test -p server --release --all-features`.
@TaprootFreak
ci(deploy-dev): smoke-test /api/info before reporting deploy success
f83d129 
A green "Build and deploy to DEV" was historically misleading — when
the runtime panicked during bootstrap the container stayed Up-but-
unresponsive while the workflow reported success. Curl /api/info up to
30 times with 10 s spacing after the deploy ssh command; fail the
workflow if it never returns 200. This blocks the auto-release PR from
collecting a green check on a broken deploy.
@TaprootFreak
TaprootFreak marked this pull request as ready for review May 19, 2026 06:29
@TaprootFreak TaprootFreak mentioned this pull request May 19, 2026
Merged
5 tasks
@TaprootFreak
TaprootFreak merged commit d5cb60c into develop May 19, 2026
4 checks passed
@TaprootFreak TaprootFreak mentioned this pull request May 19, 2026
Merged
4 tasks
TaprootFreak added a commit that referenced this pull request May 19, 2026
@TaprootFreak
ci: harden deploy workflows — concurrency guards + PRD smoke test (#51)
5c5b963 
* ci(deploy-dev): serialize deploys to prevent docker-compose race

Three develop pushes in quick succession (the issue #40 rollout: PRs
#39, #41, #42 merged back-to-back) fired three parallel Deploy DEV
runs. Without a concurrency group they all hit `docker compose
recreate` on the host at the same time, the second one left the
zkcoins-server container half-renamed in `Created` state, and the
third failed with `Error when allocating new name: Conflict. The
container name "/zkcoins-server" is already in use`. Required a
manual `docker rm -f` + `compose up -d` on dfxdev to recover.

`cancel-in-progress: true` because develop is the moving target — if
a new commit arrives the old commit's deploy is already stale; better
to cancel and deploy the newest than to queue stale builds.

* ci(deploy-prd): add concurrency guard + post-deploy smoke test

Symmetry with Deploy DEV. Two missing pieces, both real production
risks:

1. No concurrency guard — a back-to-back release-PR-to-main merge
   would hit the same `docker compose recreate` race that took DEV
   down. Use `cancel-in-progress: false` (queue, never kill mid-
   flight) — PRD deploys must complete cleanly; a cancelled
   `compose recreate` is exactly what produces the half-renamed
   container state.

2. No smoke test after the SSH deploy. If the runtime panics during
   bootstrap (the MINTING_ADDRESS-mismatch class of bug fixed in
   #36, but new instances of the same class are always one
   migration away) the container stays Up-but-unresponsive and the
   workflow reports success. Curl /api/info up to 30 x 10 s on the
   public PRD URL after the deploy ssh command, mirror of the DEV
   workflow.
This was referenced May 20, 2026
Merged
Open
TaprootFreak added a commit that referenced this pull request May 20, 2026
@TaprootFreak
docs: post-Plonky2 consistency cleanup (round 2)
61a35cb 
PR #49 (2026-05-19) did the first pass of post-migration doc cleanup.
This second pass cleans up the remaining stale references that the
follow-up work (PR #36, PRs #41-#43, #48, #51) made visible:

ROADMAP.md
- Header: drop the `feat/plonky2-migration` branch framing — branch
  is merged, ROADMAP lives on develop.
- Step 5: 🟡 in progress → ✅ done in both the table and the "Next"
  section, with the In Progress block re-framed as historical.
- Step 9: 🟡 infra ready → 🟡 DEV live, e2e + R2 pending. Cite the
  PR #17 + #36 + #51 timeline and the live `/health` + `/api/info`
  verification. Move the e2e + R2 items into a sharper 3-item list
  and replace the "pre-push is the unit-coverage authority" sentence
  with the post-#43 reality (Coverage Gate runs in CI on the
  self-hosted M3 Ultra behind the `ci:full` label).
- MVP-status line: 2-4 d ops effort remaining, not 3-5.

CONTRIBUTING.md
- Drop the `feat/plonky2-migration` branch framing throughout.
- Update the 100% coverage paragraph: 72 → 115 tests on
  `program-plonky2`, point at the self-hosted Coverage Gate job
  for `server`.
- Replace `SP1_PROVER=mock` in the Quick Start with USERNAME_DOMAIN.
- Pre-push checklist: refresh the "open question 4 in #40" pointer
  to issue #50 (the current decision tracker).
- Prerequisites: Rust 1.81+ → nightly.
- Project Structure: drop deleted `program/` + `script/`, add
  `program-plonky2/` + `script-plonky2/` with the actual file tree.
- Architecture: drop the SP1/stub Prover trait paragraph; describe
  the single Plonky2 prover.
- SP1 zkVM Circuit section → Plonky2 State-Transition Circuit
  section, citing main.rs + MAX_IN_COINS + §7.22.
- Environment Variables: drop `SP1_PROVER`, add `USERNAME_DOMAIN`
  as the required env that panics on missing.
- Docker example: drop `SP1_PROVER=mock`, add USERNAME_DOMAIN, and
  drop the dead "pre-built ELF" sentence.

MIGRATION_RESEARCH.md
- Add §7.23 documenting the MINTING_ADDRESS panic-in-tokio-spawn
  bug + fix from PR #36. Per the "Where to put new knowledge" rule
  this Plonky2-era runtime gotcha belongs in §7.

README.md
- Docker section: the Dockerfile is no longer "being re-introduced",
  it's landed and auto-builds via deploy-dev.yaml.
- Open Tasks: drop Steps 7/8/9 (done), replace with the two Step 9
  closeout items + pre-mainnet hardening pointer.
- Design Documents trailer: drop the `feat/plonky2-migration`
  reference, point at the merged PR.

SPEC.md
- L3: "currently implemented for SP1 in `program/src/main.rs`" →
  "currently implemented in Plonky2 + Poseidon in
  `program-plonky2/src/circuit/main.rs`", + cite v0.last-sp1 tag.
- Reference impl path list: program/* → program-plonky2/*.
- §6 MINTING_ADDRESS: describe the runtime override from PR #36,
  not the deferred Step-7 plan.
- §12 stale Step-7 pointer: same fix.
- §13 "Plonky2 circuit MUST also re-check it" → "in-circuit
  predicate re-checks it" (done since Stage 5c+ / 5d-next-5).
- References block: replace "Current SP1 implementation" pointer
  with the Plonky2 path + the v0.last-sp1 tag for the SP1 history.

program-plonky2/CONTRIBUTING.md
- "Why this crate is standalone" → "Toolchain": the workspace is
  unified to nightly now, this crate is a regular workspace member.
- Project layout: add main.rs, source_aggregator.rs,
  recursion_shape_probe.rs (the files that landed since).
- CI integration: clippy IS in CI; the cyclic-recursion sweep is
  not yet — decision tracked in issue #50.

STEP4_REVIEW.md, SESSION_STATE.md, STAGE_5D_NEXT_4_DESIGN.md
- Add explicit "STATUS — DONE / HISTORICAL" banners so a future
  agent reads them as records, not as still-pending plans.

No content change to `.githooks/pre-push` or `.github/workflows/ci.yaml`
— their header comments already match the post-#42 / #48 reality.
TaprootFreak added a commit that referenced this pull request May 20, 2026
@TaprootFreak
docs: address review findings on round-2 cleanup
7c06370 
Independent review of 462ed4d / 61a35cb surfaced four factual / link
errors. Fix in this commit so the cleanup PR doesn't ship with broken
deep-links and a misdescribed bootstrap fix:

1. **Broken §7.23 anchor in 3 places.** ROADMAP.md (2×) and SPEC.md
   linked into MIGRATION_RESEARCH §7.23 using the URL fragment
   `#723-...-tokiospawn-task-...-mediumcodified`. GitHub's slugger
   actually generates `#723-...-tokiospawn-ed-task-...-medium-codified`
   (preserves the `-ed-` hyphen, inserts a hyphen between `medium`
   and `codified`). Update all three call-sites.

2. **Non-existent issue #95.** CONTRIBUTING.md env-var table cited
   `+ issue #95`; the repo's highest issue is #54. The pre-existing
   README has the same broken citation but that's out of scope here.
   Replace with a self-contained note about the global panic hook
   introduced by PR #36.

3. **Wrong MINTING_ADDRESS override location** in §7.23 step 1 +
   SPEC.md §6 + SPEC.md §12 item 2. The text claimed the override
   was passed into `AccountServer::new` as a constructor parameter
   — that signature does not exist: `AccountServer::new(state)`
   takes one argument. The actual override is in
   `server_runtime.rs::start_rest_server` and mutates
   `minting_client.address` on the freshly-constructed
   `ClientAccount` (matching PR #36's body and the
   `TestAccountData::new_minting_account` pattern). Update all three
   spots to describe the real mechanism.

4. **Deleted file referenced.** program-plonky2/CONTRIBUTING.md said
   the workspace was unified by making the root `rust-toolchain`
   "match `program-plonky2/rust-toolchain.toml`". The standalone
   `rust-toolchain.toml` was removed during the workspace
   consolidation (only the root `rust-toolchain` remains). Rephrase
   to describe the actual consolidation.

Three of the four bugs were anchor / cross-link errors that would
silently 404 or scroll-to-top in the GitHub UI; the MINTING_ADDRESS
mis-description would mislead anyone tracing the bootstrap-panic fix
through the docs.
TaprootFreak added a commit that referenced this pull request May 20, 2026
@TaprootFreak
docs: post-Plonky2 consistency cleanup (round 2) (#53)
3b67aad 
* docs: post-Plonky2 consistency cleanup (round 2)

PR #49 (2026-05-19) did the first pass of post-migration doc cleanup.
This second pass cleans up the remaining stale references that the
follow-up work (PR #36, PRs #41-#43, #48, #51) made visible:

ROADMAP.md
- Header: drop the `feat/plonky2-migration` branch framing — branch
  is merged, ROADMAP lives on develop.
- Step 5: 🟡 in progress → ✅ done in both the table and the "Next"
  section, with the In Progress block re-framed as historical.
- Step 9: 🟡 infra ready → 🟡 DEV live, e2e + R2 pending. Cite the
  PR #17 + #36 + #51 timeline and the live `/health` + `/api/info`
  verification. Move the e2e + R2 items into a sharper 3-item list
  and replace the "pre-push is the unit-coverage authority" sentence
  with the post-#43 reality (Coverage Gate runs in CI on the
  self-hosted M3 Ultra behind the `ci:full` label).
- MVP-status line: 2-4 d ops effort remaining, not 3-5.

CONTRIBUTING.md
- Drop the `feat/plonky2-migration` branch framing throughout.
- Update the 100% coverage paragraph: 72 → 115 tests on
  `program-plonky2`, point at the self-hosted Coverage Gate job
  for `server`.
- Replace `SP1_PROVER=mock` in the Quick Start with USERNAME_DOMAIN.
- Pre-push checklist: refresh the "open question 4 in #40" pointer
  to issue #50 (the current decision tracker).
- Prerequisites: Rust 1.81+ → nightly.
- Project Structure: drop deleted `program/` + `script/`, add
  `program-plonky2/` + `script-plonky2/` with the actual file tree.
- Architecture: drop the SP1/stub Prover trait paragraph; describe
  the single Plonky2 prover.
- SP1 zkVM Circuit section → Plonky2 State-Transition Circuit
  section, citing main.rs + MAX_IN_COINS + §7.22.
- Environment Variables: drop `SP1_PROVER`, add `USERNAME_DOMAIN`
  as the required env that panics on missing.
- Docker example: drop `SP1_PROVER=mock`, add USERNAME_DOMAIN, and
  drop the dead "pre-built ELF" sentence.

MIGRATION_RESEARCH.md
- Add §7.23 documenting the MINTING_ADDRESS panic-in-tokio-spawn
  bug + fix from PR #36. Per the "Where to put new knowledge" rule
  this Plonky2-era runtime gotcha belongs in §7.

README.md
- Docker section: the Dockerfile is no longer "being re-introduced",
  it's landed and auto-builds via deploy-dev.yaml.
- Open Tasks: drop Steps 7/8/9 (done), replace with the two Step 9
  closeout items + pre-mainnet hardening pointer.
- Design Documents trailer: drop the `feat/plonky2-migration`
  reference, point at the merged PR.

SPEC.md
- L3: "currently implemented for SP1 in `program/src/main.rs`" →
  "currently implemented in Plonky2 + Poseidon in
  `program-plonky2/src/circuit/main.rs`", + cite v0.last-sp1 tag.
- Reference impl path list: program/* → program-plonky2/*.
- §6 MINTING_ADDRESS: describe the runtime override from PR #36,
  not the deferred Step-7 plan.
- §12 stale Step-7 pointer: same fix.
- §13 "Plonky2 circuit MUST also re-check it" → "in-circuit
  predicate re-checks it" (done since Stage 5c+ / 5d-next-5).
- References block: replace "Current SP1 implementation" pointer
  with the Plonky2 path + the v0.last-sp1 tag for the SP1 history.

program-plonky2/CONTRIBUTING.md
- "Why this crate is standalone" → "Toolchain": the workspace is
  unified to nightly now, this crate is a regular workspace member.
- Project layout: add main.rs, source_aggregator.rs,
  recursion_shape_probe.rs (the files that landed since).
- CI integration: clippy IS in CI; the cyclic-recursion sweep is
  not yet — decision tracked in issue #50.

STEP4_REVIEW.md, SESSION_STATE.md, STAGE_5D_NEXT_4_DESIGN.md
- Add explicit "STATUS — DONE / HISTORICAL" banners so a future
  agent reads them as records, not as still-pending plans.

No content change to `.githooks/pre-push` or `.github/workflows/ci.yaml`
— their header comments already match the post-#42 / #48 reality.

* docs: address review findings on round-2 cleanup

Independent review of 462ed4d / 61a35cb surfaced four factual / link
errors. Fix in this commit so the cleanup PR doesn't ship with broken
deep-links and a misdescribed bootstrap fix:

1. **Broken §7.23 anchor in 3 places.** ROADMAP.md (2×) and SPEC.md
   linked into MIGRATION_RESEARCH §7.23 using the URL fragment
   `#723-...-tokiospawn-task-...-mediumcodified`. GitHub's slugger
   actually generates `#723-...-tokiospawn-ed-task-...-medium-codified`
   (preserves the `-ed-` hyphen, inserts a hyphen between `medium`
   and `codified`). Update all three call-sites.

2. **Non-existent issue #95.** CONTRIBUTING.md env-var table cited
   `+ issue #95`; the repo's highest issue is #54. The pre-existing
   README has the same broken citation but that's out of scope here.
   Replace with a self-contained note about the global panic hook
   introduced by PR #36.

3. **Wrong MINTING_ADDRESS override location** in §7.23 step 1 +
   SPEC.md §6 + SPEC.md §12 item 2. The text claimed the override
   was passed into `AccountServer::new` as a constructor parameter
   — that signature does not exist: `AccountServer::new(state)`
   takes one argument. The actual override is in
   `server_runtime.rs::start_rest_server` and mutates
   `minting_client.address` on the freshly-constructed
   `ClientAccount` (matching PR #36's body and the
   `TestAccountData::new_minting_account` pattern). Update all three
   spots to describe the real mechanism.

4. **Deleted file referenced.** program-plonky2/CONTRIBUTING.md said
   the workspace was unified by making the root `rust-toolchain`
   "match `program-plonky2/rust-toolchain.toml`". The standalone
   `rust-toolchain.toml` was removed during the workspace
   consolidation (only the root `rust-toolchain` remains). Rephrase
   to describe the actual consolidation.

Three of the four bugs were anchor / cross-link errors that would
silently 404 or scroll-to-top in the GitHub UI; the MINTING_ADDRESS
mis-description would mislead anyone tracing the bootstrap-panic fix
through the docs.
@TaprootFreak TaprootFreak mentioned this pull request May 28, 2026
Merged
9 tasks
TaprootFreak added a commit that referenced this pull request May 29, 2026
@TaprootFreak
perf(build): release profile tune + mimalloc on node binary (#134)
7626099 
* perf(build): tune release profile + adopt mimalloc on the node binary

Cargo's default release profile uses `codegen-units = 16` and
`lto = false`. For a binary whose wall-clock is dominated by Plonky2
prove calls (3-15 min at MAX_IN_COINS=8) those defaults leave
intra-crate inlining on the table at the `node` / `script-plonky2`
boundary that calls into `plonky2_field`'s heavily-inlined hot path
through a trait surface.

- `lto = "thin"` enables cross-crate inlining without the full link-
  time cost of `lto = "fat"` (upstream Plonky2 tried fat and reverted
  in their Cargo.toml — see the inline comment).
- `codegen-units = 1` forces the whole binary into a single LLVM
  compilation unit so the inliner sees everything at once.
- `incremental = false` is the cargo default for release, restated
  here so a future `CARGO_INCREMENTAL=1` on a build host cannot
  silently fragment the codegen unit.
- `panic = "abort"` is intentionally not set: `main.rs` installs a
  global panic hook (PR #36, MINTING_ADDRESS bootstrap recovery) and
  state / account layers use RwLock poison recovery to fail one
  request without taking the process down. Abort defeats both.

mimalloc replaces the system allocator in the binary only — library
crates, integration tests, and program/prover crates keep the system
allocator. The Plonky2 prove path allocates many polynomial and
witness buffers across the rayon worker pool; glibc-malloc on the
debian-bookworm-slim runtime image is not tuned for that pattern.
Scoping the swap to `main.rs` keeps unit-test behaviour identical to
CI and avoids pulling a C build dependency into every test target.

Local release build: 1m 57s on M3 Ultra (vs ~1m 26s pre-change),
within the ~5 min CI budget documented in README.md / CONTRIBUTING.md.
Pre-push hook (fmt + 3x clippy + workspace check) clean.

Expected proof-time impact is modest (single-digit-to-low-double-digit
percent on aggregate). The authoritative measurement is the pending
R2 wall-clock probe on the M3 Ultra (ROADMAP § Step 9) — this PR
makes the build profile what that measurement should be taken from,
not a separate one-off tune.

* docs(node): correct mimalloc default-features comment

The previous comment claimed `default-features = false` strips
"secure-mode overhead" and "debug rings". Upstream
`purpleprotocol/mimalloc_rust` ships `default = []`, so there are
no default features to disable — the original justification was
fictitious.

Reword the comment to state the real reason: it is a defensive
opt-in posture so a future upstream change that turns on `secure`,
`debug`, `extended`, or `v2` via default features cannot silently
land in the node binary without an explicit code change.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@TaprootFreak