Record the audit outcome and the architecture it changed

BUGS.md keeps every finding's original description and gains, per entry, what was
actually done and where its regression test lives — including the two entries
fixed differently from the plan (B-15 validates at startup, B-09 kept both
callers plus a bounded retry) and the one only partially fixed by decision (B-16,
where shipping the guide was deferred).

It also gains a Runtime verification section, which is the part worth reading:
what the live Docker deployment actually demonstrated (startup validation on the
real .env, the listener connecting and holding, rounds cycling, the migration
applied, and the reconciler's missing-tx heuristic checked against the real
server's error message) separated from what has no runtime evidence at all —
nothing has spent money since the restart, so the two-phase write, the RBF
retargeting, the reconciler's actual behaviour and the dust path are unit-tested
only. A green suite is not a working deployment, and the file now says so.

CLAUDE.md documents the two things a reader would otherwise have to reverse-
engineer: the transaction lifecycle (why rows are written before broadcasting,
what each PendingTransaction status means, why spent_txid must track the current
txid, and that one-active-round is now a DB invariant) and the Electrum
connection's rotation/keepalive/timeout behaviour. Its Known gaps list is rewritten
to say what is still open now that transaction-level state self-heals but
round-level state does not.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
2026-07-27 00:35:28 +02:00
co-authored by Claude Opus 5
parent dc4d5761df
commit 845ba98409
2 changed files with 338 additions and 22 deletions
+74 -9
View File
@@ -8,10 +8,17 @@ The user communicates in Italian in chat — reply to them in Italian. Everythin
## Project status
All 10 build-order stages from `/home/davide/.claude/plans/scalable-mixing-sloth.md` are code-complete and unit-tested (76 tests green): project skeleton, DB schema + Alembic migrations, auth, HD wallet derivation, Electrum client, deposit detection, bet flow, round/draw engine, payout, withdrawal, RBF fee-bump, admin config + audit log. Beyond the original 10 stages: a Docker + Caddy deployment (see below), a full admin dashboard (`/admin`), a static test UI for the user-facing flow (`/`), a pending-inclusive balance display (see "Balance display" below), and a Server-Sent Events push channel layered on top of the original polling (see "Real-time updates" below).
All 10 build-order stages from `/home/davide/.claude/plans/scalable-mixing-sloth.md` are code-complete and unit-tested (137 tests green): project skeleton, DB schema + Alembic migrations, auth, HD wallet derivation, Electrum client, deposit detection, bet flow, round/draw engine, payout, withdrawal, RBF fee-bump, admin config + audit log. Beyond the original 10 stages: a Docker + Caddy deployment (see below), a full admin dashboard (`/admin`), a static test UI for the user-facing flow (`/`), a pending-inclusive balance display (see "Balance display" below), and a Server-Sent Events push channel layered on top of the original polling (see "Real-time updates" below).
Real-money verification on mainnet, done so far: registration + address derivation, deposit crediting (1-conf), a real 10 PLM bet (broadcast, confirmed, change credited back), and a full round cycle — close → draw (real block hash) → payout (70/30 split, exact sat math verified against the broadcast tx) → confirmation → round closed → next round auto-opened. Withdrawal and the RBF bump path are unit-tested but have never been exercised against a live broadcast. See "Known gaps" below before treating this as production-ready.
A full-codebase audit on 2026-07-26 found 24 bugs — five of them critical, including a
dropped Electrum connection that hung the whole server with no reconnect, an RBF fee bump
that wedged a round forever, and no way for the system to recover a broadcast that never
confirmed (funds frozen). All 24 are fixed; [BUGS.md](BUGS.md) is the record, with each
one's root cause, what was actually done, and where its regression test lives. Read it
before assuming any behaviour here predates those fixes.
Before writing code, always read the "Architecture" section below in full, plus the diagrams in [flowchart/](flowchart/): [platform-overview.mmd](flowchart/platform-overview.mmd) for the whole 5-phase flow, and [round-lifecycle.mmd](flowchart/round-lifecycle.mmd) for the round/draw lifecycle in detail. Every node in these diagrams corresponds to a behavior that must be implemented exactly as described, including the labels on the edges (conditions, retries, loops). Regenerate their companion PDFs with `flowchart/render-pdf.sh <file>.mmd` after editing either one.
Human-facing guides live in [docs/](docs/) (Italian, per explicit request — an exception to this file's English-only rule below): [setup.md](docs/setup.md), [running-the-server.md](docs/running-the-server.md), [guida-utente.md](docs/guida-utente.md), [guida-admin.md](docs/guida-admin.md).
@@ -81,6 +88,26 @@ Mainnet:
- Block time: 120s
- BIP32 extended key headers (Legacy/native-segwit `zprv`/`zpub` etc.): see `ExtKeyHeaders` in `ChainProfiles.cs`
## Electrum connection (rotation, keepalive, timeouts)
One connection serves everything — deposit credits, broadcasts, confirmations, the chain
tip the draw waits on — which makes it the platform's biggest single point of failure.
Three things keep it honest:
- **Server rotation.** `ELECTRUM_HOST`/`ELECTRUM_PORT` is the primary;
`ELECTRUM_FALLBACK_SERVERS` is a comma-separated list of `host:port[:notls]` extras
(parsed by `electrum/client.py:parse_endpoints`, which rejects malformed entries at
startup rather than during the outage when the fallback is needed). The listener tries
the next server after any failed or dropped session, and only sleeps on the backoff once
every server has had a turn — so one dead server costs a single attempt, not an outage.
- **Every request is bounded** (`_REQUEST_TIMEOUT_SECONDS`, 15s) and a timeout tears the
connection down. Unbounded waits used to hang a `POST /bets` *while holding the per-user
lock*, and could stop the confirmation poller permanently.
- **The drop is observable.** `client.wait_closed()` resolves when the read loop dies, and
`listener._run_once` races it against the notification consumers and a 60s `server.ping`
keepalive. Without this the listener sat on queues nobody would ever fill again and never
reconnected — while `listener.client` still looked alive to everything else.
## Balance display
`place_bet`/`request_withdrawal` (`app/bets/service.py`, `app/withdrawals/service.py`) select whole UTXOs to cover the amount (`select_utxos`, largest-first) and mark every selected UTXO `spent_txid` immediately at broadcast time — well before the tx has any confirmations. `User.cached_balance_sats` (`recompute_balance`, `app/wallet/balance.py`) only sums confirmed, unspent UTXOs, so right after a bet/withdrawal it understates the user's real balance by the entire unconfirmed change amount, which is often far larger than the amount actually moving.
@@ -160,16 +187,54 @@ These choices were made explicitly during design (not derivable from reading a s
- The admin panel can export any user's raw WIF private key (`GET /admin/users/{id}/privkey`, `app/wallet/hd.py:derive_user_wif`). This is intentional, not a vulnerability to fix: the server already holds the master key everything derives from (custodial by design, see above), so this only exposes through the API something an operator could already do via a script. Every access is written to `audit_log` (`admin_privkey_accessed`) — don't remove that logging when touching this endpoint.
- RBF fee bumps are paid by whoever's change output the tx pays back to — the user for bets/withdrawals, the pool for payouts — never by the fixed counterparty amount (recipient/winner/fee-address outputs are untouched; only the sender's own change shrinks). See `bump_fee` in `app/tx/broadcast.py`.
## Transaction reconciliation and the tx lifecycle
Everything that spends money is written **before** it is broadcast, and resolved against
the chain afterwards. This is what makes the system recover on its own instead of needing
manual DB edits (BUGS.md B-04/B-08).
`PendingTransaction.status` is the lifecycle: `building``pending``confirmed`, or
`failed`.
- `building` is written first, with the UTXOs already marked `spent_txid`, and committed
*before* the broadcast (`bets/service.py:place_bet`, `withdrawals/service.py`). A crash
in that window therefore leaves evidence rather than coins spent on-chain with no record.
- If the broadcast is refused, the service releases the reserved UTXOs, restores the
balance, removes the participant (or marks the withdrawal `failed`), audit-logs it, and
raises `broadcast_failed` — answered as **502**, since the network refused it, not the
caller.
- `app/tx/reconcile.py` (`PendingTransactionReconciler`, every 120s and once at startup)
asks the chain about anything still `building`/`pending`. Tx present → promote; tx gone →
mark `failed` with a `failure_reason`, release the inputs, roll the domain row back,
audit-log `pending_tx_abandoned`. Grace periods differ by state (120s for `building`,
6h for `pending`, so the RBF bumper gets its attempts first), and a *transport* failure
never abandons anything — only a server that positively doesn't know the tx does.
Because of this, `UtxoEvent.spent_txid` must always equal the *current* txid of the tx
reserving it: `bump_fee` retargets it (along with `RoundParticipant.bet_txid`,
`Withdrawal.txid` and `Round.payout_txid`) on every fee bump. Confirmation handlers
deliberately key off immutable ids (`round_id`/`user_id`, `withdrawal_id`) rather than the
txid, which changes under them.
**At most one active round is a database invariant**, not just a code convention:
`ix_rounds_single_active` (a unique index over the constant expression `(1)`, restricted to
the active statuses) makes a concurrent second insert fail cleanly, and
`open_new_round_if_needed` recovers by using the winner's round.
## Known gaps / TODO
Not blockers for reading the code, but must be addressed before this is production-ready:
Not blockers for reading the code, but must be addressed before this is production-ready.
The 24 findings of the 2026-07-26 full-codebase audit are **all fixed** — see
[BUGS.md](BUGS.md), which keeps each one's root cause, fix and regression test as the
record. What remains open:
- **Scheduler doesn't resume mid-flight rounds after a restart.** `rounds/scheduler.py`'s `_tick()` only acts on rounds with `status == "open"`. If the process restarts while a round is `closing`/`drawing`/`paying_out`, it's permanently stuck — nothing re-enters `_wait_for_next_block` or retries `_trigger_payout`. Needs a startup routine that inspects in-progress rounds and resumes (or a periodic "unstick" check) before this can run unattended.
- **RBF bump only handles one case**: a single change output, paying back to the tx's own sender address, large enough to absorb the fee increase. No additional-input selection fallback — an exact-amount tx (no change) or a change output too small to absorb the bump raises `RbfError` and needs manual operator intervention. Documented in `tx/broadcast.py`.
- **Payout retry**: if `_trigger_payout` fails (e.g. insufficient pool UTXOs, Electrum disconnected), it just logs and returns — the round stays stuck in `paying_out` with no automatic retry.
- **Withdrawal and RBF bump have never been exercised against a live broadcast** — only deposit and bet flow are verified end-to-end with real PLM as of this commit.
- **No general user-facing history endpoints** (list my own bets / withdrawals / past rounds) — `GET /users/me/last-round-result` covers exactly one case (the outcome of the most recent *closed* round the user played in, as a reveal-persistence backstop; see DRAW above), not a real history. The admin side has more (`/admin/rounds`, `/admin/pending-transactions`, `/admin/audit-log`), but there's still no "my own full history" equivalent for a logged-in user.
- **Admin auth is a single shared bearer token** (`ADMIN_TOKEN`, `X-Admin-Token` header) — no per-admin identity or audit trail of *who* changed config (the `audit_log` table records *what* changed, not which operator did it). This token now gates a lot more than config (user list, private key export, round/audit history), so its blast radius if leaked is correspondingly larger.
- **Scheduler doesn't resume mid-flight rounds after a restart.** `rounds/scheduler.py`'s `_tick()` only acts on rounds with `status == "open"`. If the process restarts while a round is `closing`/`drawing`/`paying_out`, it's permanently stuck — nothing re-enters `_wait_for_next_block` or retries `_trigger_payout`. Needs a startup routine that inspects in-progress rounds and resumes (or a periodic "unstick" check) before this can run unattended. Note this is *round*-level state: in-flight *transactions* do now recover on their own (see "Transaction reconciliation" below).
- **RBF bump only handles one case**: a single change output, paying back to the tx's own sender address, large enough to absorb the fee increase. No additional-input selection fallback — an exact-amount tx (no change) or a change output too small to absorb the bump raises `RbfError`. The consequence is no longer permanent, though: a tx that can't be bumped and never confirms is eventually abandoned and its UTXOs released (see "Transaction reconciliation"), so the funds come back instead of being frozen.
- **Payout retry**: if `_trigger_payout` fails (insufficient pool UTXOs, a bad `fee_address`, Electrum disconnected), it logs, writes a `payout_failed` audit entry, and returns — the round stays in `paying_out` with no automatic retry. The audit entry makes it visible in `/admin`; acting on it is still manual.
- **Withdrawal and RBF bump have never been exercised against a live broadcast** — only deposit and bet flow are verified end-to-end with real PLM. Both paths have unit coverage, including their failure and rollback branches, but unit tests are not a live network.
- **No general user-facing history endpoints** (list my own bets / withdrawals / past rounds) — `GET /users/me/last-round-result` covers exactly one case (the outcome of the most recent *closed* round the user played in, as a reveal-persistence backstop; see DRAW above), not a real history. The admin side has more (`/admin/rounds`, `/admin/pending-transactions`, `/admin/audit-log`), but there's still no "my own full history" equivalent for a logged-in user. A failed withdrawal now leaves a `status="failed"` row the user cannot see anywhere — an argument for closing this gap.
- **Admin auth is a single shared bearer token** (`ADMIN_TOKEN`, `X-Admin-Token` header) — no per-admin identity: `audit_log` records *what* changed (config edits are now logged too, as `config_updated`, with before/after values) but never *which operator* did it. This token gates the user list, private key export and round/audit history, so its blast radius if leaked is large.
- **No rate limiting / abuse protection** on any endpoint (register, bet, withdrawal, admin).
- **`/guida` is not served in Docker.** `GET /guida` reads `docs/guida-utente.md`, and the `Dockerfile` deliberately does not `COPY docs` — the guide is pending a rewrite, so it isn't shipped yet. The endpoint answers a clean 404 (`guide_unavailable`, translated) and logs an error rather than crashing, but the navbar help link leads nowhere until `COPY docs ./docs` is added back.
- No automated integration tests against a live Electrum connection — all live-network verification so far has been manual (ad hoc scripts + real mainnet transactions), not part of the `pytest` suite.
- **`docker-compose.yml`'s `restart: unless-stopped`** on the app container means a crash mid-round auto-restarts straight into the scheduler-resume gap above — see the Deployment section.
- **Single-process assumptions**: the SSE broadcaster (`rounds/events.py`) and the per-user locks (`tx/locks.py`) are both in-process only. Fine for the current one-uvicorn-process deployment; a multi-worker one needs a shared channel and a DB/Redis lock. Note the round-uniqueness invariant is *not* in this category any more — it's enforced by a DB index (see below).