davideandClaude Sonnet 5
7fa26df104
_wait_for_next_block had no timeout, no log, and no audit entry: a connection that stopped advancing the tip left a round silently frozen in "drawing" with nothing in /admin to explain why. Log progress periodically, write a draw_stalled audit entry past a threshold (a few block-time multiples), and surface the wait via a new Round.drawing_started_at column, exposed as draw_waiting_since in GET /rounds/current. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
189 lines
11 KiB
Markdown
189 lines
11 KiB
Markdown
# Known bugs
|
|
|
|
A second full-codebase audit on 2026-07-27 found **25 further issues** (4 critical, 6 high,
|
|
7 medium, 8 low), listed below as B-33 … B-49. B-25 through B-36 are fixed (see "Previously
|
|
fixed" below) — no Critical-severity finding remains open; the other 13 are High/Medium/Low.
|
|
The 139-test suite was green at the time of the audit, so none of these were caught by existing
|
|
coverage — every fix lands with a regression test (the twelve fixes so far brought the suite
|
|
from 139 to 200).
|
|
|
|
The recurring pattern across the open findings is worth stating once: the code is rigorous
|
|
about the failure modes that have actually been hit, and silent about the ones that have not.
|
|
The payout phase is now fully recoverable; the "drawing" phase (waiting on a block) is now
|
|
observable (B-36) but still has no equivalent resume-after-restart — see "Known gaps / TODO"
|
|
in [CLAUDE.md](CLAUDE.md).
|
|
|
|
For limitations that are accepted by design rather than bugs (single-shared-token admin auth,
|
|
single-process assumptions, no user-facing history, etc.), see "Known gaps / TODO" in
|
|
[CLAUDE.md](CLAUDE.md).
|
|
|
|
---
|
|
|
|
## Medium
|
|
|
|
### B-37 — Displayed balance and spendable balance diverge, and the error does not explain it
|
|
|
|
After a bet the change is unconfirmed, so `cached_balance_sats` ≈ 0 while the UI shows
|
|
`pending_balance_sats` (the real figure). A withdrawal attempted right after validates against
|
|
**confirmed** UTXOs (`withdrawals/service.py:54-60`) and answers `insufficient_balance`.
|
|
|
|
The user sees "1.000 PLM" on screen and is told they have no funds. The mechanism is a
|
|
documented design decision, but the error does not distinguish "you don't have the money" from
|
|
"your money is waiting to confirm" — two very different situations for whoever reads it.
|
|
|
|
**Proposed fix.** A distinct error code (e.g. `balance_pending_confirmation`) raised when the
|
|
requested amount is covered by `pending_balance_sats` but not by the confirmed balance,
|
|
carrying the pending amount in `params`, plus its `error.*` entry in all 7 languages. The
|
|
withdrawal form should also cap/hint the max against the confirmed balance rather than the
|
|
displayed one.
|
|
|
|
### B-38 — The 500-subscriber SSE cap is a zero-cost DoS of the realtime feature
|
|
|
|
`GET /rounds/stream` requires no authentication and each connection takes a slot on a
|
|
**global** counter (`rounds/events.py:33-38`). Anyone opening 500 connections degrades every
|
|
real user to polling. The comment describes it as a defensive cap; it is in fact the vector,
|
|
not the defence.
|
|
|
|
**Proposed fix.** Cap per client IP (and, once available, per authenticated user) rather than
|
|
globally, and evict the oldest idle subscriber instead of refusing new ones. The reverse proxy
|
|
is the right place for the connection-count limit — Caddy can enforce it before the request
|
|
reaches the app.
|
|
|
|
### B-39 — SQLite with no WAL, no `busy_timeout`, and five concurrent writer tasks
|
|
|
|
`db/base.py:6` calls `create_async_engine(settings.database_url)` with no `connect_args`, and
|
|
there is no `PRAGMA` anywhere in the repo (verified by grep). Without `journal_mode=WAL`
|
|
readers block writers, and the concurrent writers are five background tasks plus every HTTP
|
|
handler. `database is locked` under load is realistic, and nothing handles it.
|
|
|
|
**Proposed fix.** Set `journal_mode=WAL`, `synchronous=NORMAL` and a `busy_timeout` of a few
|
|
seconds on connect (a `connect` event listener on the engine, applied only for the SQLite
|
|
dialect), and retry `OperationalError: database is locked` in the background loops. Longer
|
|
term this is an argument for PostgreSQL, which the single-process constraints in CLAUDE.md
|
|
also point at.
|
|
|
|
### B-40 — `bump_fee` holds a DB session open across N network calls
|
|
|
|
`tx/broadcast.py:80` issues one `get_transaction` **per input** (up to 15s each) and then a
|
|
`broadcast`, all with the session open. This is precisely the pattern B-18 removed from
|
|
`_trigger_payout` via its three-phase structure; it survives here.
|
|
|
|
Side note in the same function: `_prevout_amount` does `round(value_coins * 100_000_000)` on a
|
|
float from the server — acceptable at these magnitudes, but it is floating-point money
|
|
arithmetic in a codebase that is otherwise strictly integer-satoshi.
|
|
|
|
**Proposed fix.** Restructure into the same three phases: read what is needed and close the
|
|
session, do the chain work, then reopen to persist. For the float: prefer the raw (non-verbose)
|
|
transaction and parse the output value as an integer with `embit`, which is what
|
|
`reconcile.py:_release_inputs` already does for inputs.
|
|
|
|
### B-41 — All confirmation logic depends on `verbose=True`, which is not universally supported
|
|
|
|
`poll_once`, `reconcile._tx_exists_on_chain` and `bump_fee` all call
|
|
`blockchain.transaction.get(txid, True)`. Several Electrum server implementations and versions
|
|
reject the verbose flag ("verbose transactions are currently unsupported"). Falling back onto
|
|
such a server means **no confirmations, no reconciliation, no bumps** — and the code would read
|
|
that as a transport error and stay silent.
|
|
|
|
Related: `reconcile.py:83` decides whether to **abandon a transaction** by substring-matching
|
|
the error text (`"missing"`, `"not found"`, `"no such"`, `"unknown"`). It works against
|
|
ElectrumX; it is fragile as the basis for a decision that releases funds.
|
|
|
|
**Proposed fix.** Use `blockchain.transaction.get_merkle` (or the scripthash history) for
|
|
confirmation and existence checks — both are portable and give the confirming height directly.
|
|
Probe verbose support once at connect time and record it on the client, so an unsupported
|
|
server is detected loudly at session start rather than silently mid-operation.
|
|
|
|
---
|
|
|
|
## Low / hygiene
|
|
|
|
### B-42 — `/docs` exposed in production
|
|
|
|
FastAPI mounts Swagger by default, so the entire API surface — `/admin` included — is publicly
|
|
enumerable. The README advertises it.
|
|
**Fix:** `docs_url=None, redoc_url=None, openapi_url=None` in production (env-gated), or place
|
|
them behind `require_admin`.
|
|
|
|
### B-43 — No HTTP security headers
|
|
|
|
The [Caddyfile](Caddyfile) sets no CSP, no `X-Frame-Options`/`frame-ancestors`, and no HSTS
|
|
(Caddy does not add it on its own). The JWT lives in `localStorage`, so any XSS exfiltrates
|
|
it, and the page is iframeable.
|
|
**Fix:** a `header` block in the Caddyfile with `Strict-Transport-Security`,
|
|
`X-Content-Type-Options: nosniff`, `Referrer-Policy` and a CSP tight enough for two static
|
|
pages with no external assets (`default-src 'self'`).
|
|
|
|
### B-44 — README and CLAUDE.md contradict each other
|
|
|
|
The README says to run `uvicorn --reload` directly and
|
|
`docker compose run --rm app python scripts/generate_master_key.py`; CLAUDE.md says explicitly
|
|
that neither is supported. Whoever opens the repo reads the README first.
|
|
**Fix:** align the README's Quick start with the Docker-only workflow documented in
|
|
CLAUDE.md and `docs/setup.md`.
|
|
|
|
### B-45 — Unvalidated and unpaginated admin list endpoints
|
|
|
|
`limit: int = 50` on `/admin/rounds` and `/admin/audit-log` has no bounds (`-1` means
|
|
"everything" on SQLite), and `/admin/pending-transactions` has no limit at all — it grows
|
|
without end.
|
|
**Fix:** `Query(default=50, ge=1, le=500)` on both, and the same treatment plus a status filter
|
|
on the pending-transaction list.
|
|
|
|
### B-46 — `secrets.compare_digest` on a `str` raises on non-ASCII input
|
|
|
|
`api/routes/admin.py:27` raises `TypeError` — a 500 instead of a 403 — when the header contains
|
|
non-ASCII characters.
|
|
**Fix:** compare the UTF-8 encoded bytes of both sides.
|
|
|
|
### B-47 — Unbounded `String` columns for large text
|
|
|
|
`raw_tx_hex` (`db/models.py:146`) and `payload_json` (`:178`) should be `Text`. It works on
|
|
SQLite and PostgreSQL and breaks elsewhere.
|
|
**Fix:** switch both to `Text` in a migration.
|
|
|
|
### B-48 — No cap on input count in `select_utxos`
|
|
|
|
A user with hundreds of small UTXOs builds a huge transaction whose fee — deducted from the bet
|
|
amount — materially erodes their contribution to the pool, and it can exceed standardness
|
|
limits.
|
|
**Fix:** cap the selected inputs (e.g. 50) and fail with a translatable error suggesting a
|
|
consolidation, or consolidate the address automatically when the count crosses a threshold.
|
|
|
|
### B-49 — Rollback paths do not publish an SSE update
|
|
|
|
`bets/service.py:_release_failed_bet` and `withdrawals/service.py:_release_failed_withdrawal`
|
|
restore the balance without calling `broadcaster.publish()`, so dashboards only find out on
|
|
their next poll.
|
|
**Fix:** one `broadcaster.publish()` at the end of each, as every other state-changing path
|
|
already does.
|
|
|
|
---
|
|
|
|
## Previously fixed
|
|
|
|
- **B-25** — the payout had no two-phase write, unlike bets and withdrawals
|
|
- **B-26** — a payout failure or a process restart could wedge a round in `paying_out` forever
|
|
- **B-27** — an RBF bump reset the reconciler's own abandon clock, so a repeatedly-bumped tx was never abandoned
|
|
- **B-28** — a hostile Electrum server (or a MITM) could single-handedly pick the round's winner
|
|
- **B-29** — a UTXO absent from one server's `listunspent` was marked spent immediately, irreversibly, on a single unauthenticated reply
|
|
- **B-30** — a lost scripthash subscription meant a user's deposits were never credited, with no periodic safety net
|
|
- **B-31** — resubscribing on reconnect ran serially before anything else started, freezing the chain tip (and so an in-flight draw) for the whole sweep
|
|
- **B-32** — an RBF bump could retry forever below BIP125's relay-mandated minimum fee delta, with no ceiling on the fee rate either
|
|
- **B-33** — `POST /auth/login` had no rate limiting, so a password could be brute-forced against an enumerable username list
|
|
- **B-34** — password change/reset didn't invalidate already-issued JWTs, so a stolen token survived a change meant to lock it out
|
|
- **B-35** — API timestamps round-tripped as naive datetimes, so the frontend parsed them as local time instead of UTC
|
|
- **B-36** — a stalled draw wait had no timeout, no log, and no audit trail, so a frozen round showed nothing in `/admin`
|
|
|
|
See git history for the fix-by-fix breakdown (commits `f13f685`, `50a43ae`, `933760e`, and the
|
|
B-28/B-29/B-30/B-31/B-32/B-33/B-34/B-35/B-36 fixes). Suite grew from 139 to 200 tests over the twelve.
|
|
|
|
A full-codebase audit on 2026-07-26 (commit `d4e0974`) found 24 bugs across every Python
|
|
module under `app/`, both static frontends, and the Docker/Caddy deployment — 5 critical,
|
|
7 high, 7 medium, 5 low. All 24 were fixed and verified against the current code on
|
|
2026-07-27; the fixes are covered by the regression suite (grew from 79 to 139 tests) and
|
|
five of them were additionally confirmed against a real mainnet deployment (see git history
|
|
between `fb734bb` (documenting the findings) and `845ba98` (recording the audit outcome) for
|
|
the fix-by-fix breakdown — each commit message names the bugs it closes and where their
|
|
tests live).
|