fix(server): prepare-then-commit mint to prevent state desync - #90
Merged
Merged
Conversation
TaprootFreak
marked this pull request as ready for review
May 23, 2026 19:04
PR #87 merged with the nested `if let Some(block) { if let Some(hash) { if let Ok(h) = BlockHash::from_str(hash) { ... } } }` shape. The closing brace at line 37 (the path where `block.id` is a string but not a valid hex) reads as covered by the `parse_ws_frame_returns_empty_when_block_id_is_invalid_hex` test in local `cargo test`, but llvm-cov's region tracking reports the closing-brace region as untaken — the Coverage Gate flags `server/src/scanner_ws_parse.rs:37` as the only uncovered line. Flatten the block arm to a single Option chain (`block.get("id").and_then(...).and_then(...).map(...).unwrap_or_default()`). Behavior is identical across every input shape the existing tests cover; LLVM's region tracking collapses cleanly because the closing brace no longer exists as a distinct sub-region. Affects PR #18 (Release: develop -> main) and any open PR based on develop (e.g. #90).
2 tasks
TaprootFreak
force-pushed
the
fix/mint-state-desync-prepare-then-commit
branch
from
May 23, 2026 19:32
00ab010 to
ec4ce59
Compare
TaprootFreak
changed the base branch from
develop
to
fix/scanner-ws-parse-coverage
May 23, 2026 19:32
TaprootFreak
added a commit
that referenced
this pull request
May 23, 2026
) PR #87 merged with the nested `if let Some(block) { if let Some(hash) { if let Ok(h) = BlockHash::from_str(hash) { ... } } }` shape. The closing brace at line 37 (the path where `block.id` is a string but not a valid hex) reads as covered by the `parse_ws_frame_returns_empty_when_block_id_is_invalid_hex` test in local `cargo test`, but llvm-cov's region tracking reports the closing-brace region as untaken — the Coverage Gate flags `server/src/scanner_ws_parse.rs:37` as the only uncovered line. Flatten the block arm to a single Option chain (`block.get("id").and_then(...).and_then(...).map(...).unwrap_or_default()`). Behavior is identical across every input shape the existing tests cover; LLVM's region tracking collapses cleanly because the closing brace no longer exists as a distinct sub-region. Affects PR #18 (Release: develop -> main) and any open PR based on develop (e.g. #90).
TaprootFreak
force-pushed
the
fix/mint-state-desync-prepare-then-commit
branch
from
May 23, 2026 20:59
ec4ce59 to
8636dcf
Compare
TaprootFreak
changed the base branch from
fix/scanner-ws-parse-coverage
to
develop
May 23, 2026 21:03
TaprootFreak
force-pushed
the
fix/mint-state-desync-prepare-then-commit
branch
from
May 23, 2026 21:51
8636dcf to
a414fdf
Compare
mint_handler advanced minting_meta.num_pubkeys, mutated the in-memory minting account, and persisted recipient state BEFORE attempting the on-chain inscription broadcast. When the broadcast failed (publisher empty, Esplora 5xx, WS timeout, etc.) the server's bookkeeping had already moved on, but the SMT/MMR never received the commitment — every subsequent mint and send for the same minting-account-pubkey-N then returned 422 with either "Unable to get merkle proofs for provided public key" or "Unable to get mmr inclusion proof for the previous root". Once tripped, the only known recovery was a full DEV-state wipe. Refactor into prepare -> broadcast -> commit: 1. Snapshot: under minting_account guard, read N + derive pubkeys. 2. Proof: clone the minting account and run send_coins against the clone — no mutation of self.accounts. 3. Broadcast: create_and_broadcast_inscription. Err -> 503, no state advanced anywhere. 4. Commit (broadcast OK): single sqlx tx with an optimistic UPDATE minting_meta SET num_pubkeys = N+1 WHERE id = 1 AND num_pubkeys = N so concurrent mints can't both commit; on row count 0 -> 503 "Concurrent mint detected". After tx commit, swap the mutated snapshot into account_server, advance num_pubkeys in-memory, persist the MintProof. Return 200. Startup invariant check (server_runtime): for i in 0..num_pubkeys assert get_commitment_proof(derive_public_key(i)) is Ok. Refuse to start on first miss with a CRITICAL log line that names the recovery procedure (reset_state workflow). No flag override. Tests: four new mint-broadcast-failure tests assert num_pubkeys unchanged + in-memory state unchanged + retry succeeds + concurrent mints serialize. The existing mint_broadcast_failure_returns_503 test gained the missing state-unchanged assertions it was missing. A new server_runtime test asserts the startup check rejects a desynced state. commit_handler audit: broadcast-first-then-receive_coin pattern already correct; documented as an invariant in code. Closes #89.
TaprootFreak
force-pushed
the
fix/mint-state-desync-prepare-then-commit
branch
from
May 23, 2026 22:12
a414fdf to
f17d8f2
Compare
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #89.
Problem
mint_handleradvancedminting_meta.num_pubkeys, mutated the in-memory minting account, and persisted recipient state before attempting the on-chain inscription broadcast. When the broadcast failed (publisher empty, Esplora 5xx, WS timeout, etc.) the server's bookkeeping had already moved on, but the SMT/MMR never received the commitment — every subsequent mint and send for the same minting-account-pubkey-N then returned 422 with either:Unable to get merkle proofs for provided public key, orUnable to get mmr inclusion proof for the previous rootOnce tripped, the only known recovery was a full DEV-state wipe.
Fix — prepare → broadcast → commit
minting_accountguard, readN = num_pubkeysand derive(pubkey_N, pubkey_{N+1}, pubkey_{N-1}). Release.prepare_mint(a new clone-based wrapper around the existingsend_coinslogic) against the clone — no mutation ofself.accounts.create_and_broadcast_inscription. OnErr→503, NO state advanced anywhere.sqlx::Transactionwith an optimistic UPDATE:UPDATE minting_meta SET num_pubkeys = N+1 WHERE id = 1 AND num_pubkeys = N. Onrows_affected() == 0→503 "Concurrent mint detected". After tx commit: re-acquire the lock, atomically swap the mutated minting snapshot intoaccount_serverviacommit_mint(...), and applyreceive_coin(additive) on each recipient — noimport_accountthat would overwrite concurrent updates. Persist each recipient with an independentdb::upsert_account(mirrors thebroadcast_commit_and_delivershape for/api/send). Return200.Startup invariant check
In
server_runtime::start_rest_server, before binding the listener:For
i in 0..num_pubkeys, assertstate.get_commitment_proof(derive_public_key(i))isOk. Refuse to start on first miss with a CRITICAL log line naming the recovery procedure (reset_stateworkflow).To avoid a false-positive when a recent mint hasn't yet been ingested by the scanner, the check
awaits a bounded scanner-settle window: up toSCANNER_INITIAL_SETTLE_TIMEOUT_MS(default 90 s, env-overridable) for the scanner to make at least one successfulstate.updatecall. The signal is anArc<AtomicU64>incremented on every successful state.update from the scanner callback (main.rs). On timeout, the SMT check runs against the current state anyway — better a loud false-positive than a silent zombie.On bootstrap failure,
main.rsnow callsstd::process::exit(1)so the orchestrator crash-loops cleanly (matches the existing panic-hook semantics).What's not changed
commit_handlerandbroadcast_commit_and_deliveralready do broadcast-first-then-receive_coin. Audited and confirmed. Documented as an invariant in code comments.send_handlerhas no on-chain broadcast — it returns aCoinProofthat clients submit via/api/commit. Nothing to refactor.Tests
mint_broadcast_failure_does_not_advance_num_pubkeys— failed broadcast →num_pubkeysstays atSome(0).mint_broadcast_failure_does_not_mutate_minting_account— in-memory state unchanged after failed broadcast.mint_retry_after_broadcast_failure_succeeds— first call: dead Esplora → 503; second call: working Esplora → 200,num_pubkeys == 1, single inscription on mock.concurrent_mints_only_one_commits— deterministic: pre-seeded staleminting_metarow triggers the loser branch with 503 "Concurrent mint detected".startup_invariant_rejects_when_num_pubkeys_exceeds_smt— bootstraps withnum_pubkeys = 5+ empty SMT, assertsstart_rest_servererrors with the CRITICAL message.mint_broadcast_failure_returns_503was updated with state-unchanged assertions it was previously missing.mint_upsert_account_failure_logs_and_returns_okwas renamed/rewritten tomint_commit_tx_failure_returns_503to reflect the new hard-fail semantics on commit-tx errors.Companion change
DFXServer/server@6b0506c removes the orphan
DEV_SKIP_BROADCAST_FAILURE=trueenv var frominfrastructure/dfxdev/zkcoins/docker-compose.yamland adds areset_staterunbook entry to the README. The env var was no longer read by the binary (since PR #73); this PR closes the desync class underneath, so the orphan line can finally go.Test plan
mint_roundtrip+send_commit_roundtripagainst DEV pass cleanly. No retries onUnable to get merkle proofsorUnable to get mmr inclusion proofbecause the underlying desync can no longer happen.num_pubkeysadvanced by exactly one.