davideandClaude Opus 5
8a0ebecfcc
ElectrumListener._run_once assigned self.client before subscribe_headers() returned, so there was a window — one round-trip wide, at process start — where the connection looked alive while tip_height was still its initial 0. "client is not None" is what every consumer reads as "the chain is reachable", RoundScheduler._tick included, and a round closing inside that window recorded tip_at_close = 0. The very first header we then learned about — the current tip, a block mined *before* the round closed, whose hash was already public while bets were still open — satisfied tip_height > tip_at_close and became the draw's entropy. The draw's whole guarantee is that its seed did not exist yet when betting stopped. Two changes, defending different things: - The client is published only once the first header has been applied, so "client is not None" now means "reachable *and* we know where the chain is". During the window consumers see no connection, which is honest: a bet gets the same 503 it already gets while disconnected, and the background tasks skip a cycle as they already do. - _wait_for_next_block treats a baseline of 0 as *unknown*, not as height zero: it adopts the first height it learns as the baseline, waits for a block strictly after it, and records draw_baseline_tip_unknown so the extra block of waiting is explainable from /admin. Unreachable via the listener now, but it is the local statement of what the draw requires, and nothing else in that function would notice if the invariant stopped holding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
162 lines
7.8 KiB
Markdown
162 lines
7.8 KiB
Markdown
# BUGS.md — audit of 2026-08-03
|
|
|
|
Third full-codebase audit, opened after the 2026-07-26 (B-01 … B-24) and
|
|
2026-07-27 (B-25 … B-49) lists were emptied. Numbering continues from the last
|
|
fixed finding, B-51.
|
|
|
|
The list opened at B-52 … B-72 and holds only what is still **open**: a finding is
|
|
removed from this file once it is fixed, and is not listed here afterwards. Per
|
|
CLAUDE.md's convention each entry gets its own commit with its own regression test,
|
|
and the `B-nn` marker goes in a comment next to the fix, so
|
|
`git log --all --grep 'B-nn'` is the record of how any closed finding was closed.
|
|
|
|
State of the tree at audit time: 264 unit tests, all passing; `tests/integration/`
|
|
still empty; withdrawal and the RBF bump path still never live-broadcast.
|
|
|
|
Verified as *not* broken while looking for these: i18n key parity (146 identical
|
|
keys across all 7 languages), HTML escaping of every user-controlled value in
|
|
`admin.js`, the Alembic chain (linear, single head, matching `models.py`),
|
|
strictly-integer satoshi arithmetic everywhere, and `.gitignore` coverage of
|
|
secrets/DB/logs (nothing sensitive is tracked in git).
|
|
|
|
Severity is about consequence, not likelihood:
|
|
**critical** = money stuck or lost, or the lottery stops;
|
|
**high** = a security control that does not hold;
|
|
**medium** = wrong behaviour with a bounded blast radius;
|
|
**low** = drift between documentation and code.
|
|
|
|
---
|
|
|
|
## Critical
|
|
|
|
### (not new) `drawing` does not resume after a restart
|
|
|
|
Already tracked as an accepted gap in CLAUDE.md's "Known gaps", not re-numbered
|
|
here. Worth restating in context: with `restart: unless-stopped` on the app
|
|
container, this is the one state that gets stuck with money in play, and it
|
|
remains the last prerequisite for running unattended.
|
|
|
|
---
|
|
|
|
## Medium — correctness and robustness
|
|
|
|
### B-64 — `_apply_header` accepts a same-height header without the chaining check
|
|
|
|
`app/electrum/listener.py:271-296`.
|
|
|
|
The chain check only runs for `height == self.tip_height + 1`. A header at exactly
|
|
the current tip height replaces `tip_header_hex` after passing only the
|
|
self-target check — which, as the docstring of `header_meets_its_own_target`
|
|
already notes, a server can satisfy with a self-declared easy target. Separately,
|
|
a header with no `hex` field sets `tip_header_hex = None`, discarding a tip we
|
|
otherwise accepted.
|
|
|
|
Fix: treat a same-height header as either ignorable or as a reorg signal rather
|
|
than silently replacing the entropy source, and don't clear `tip_header_hex` on a
|
|
hex-less notification.
|
|
|
|
### B-65 — `/rounds/current` counts unconfirmed participants; the draw and payout do not
|
|
|
|
`app/api/routes/rounds.py:131-143` vs `app/rounds/scheduler.py:126`.
|
|
|
|
`participant_count` and `jackpot_sats` are computed over *all* `round_participants`
|
|
rows, while the draw and the payout only use `status == "confirmed"`. So the
|
|
advertised jackpot can exceed what is actually paid out, and a participant whose
|
|
bet is later abandoned appears in the count and then vanishes from it.
|
|
|
|
Fix: either count only `confirmed` (and accept that a fresh bet takes a block to
|
|
show up), or expose the two figures separately (confirmed vs in-flight) so the
|
|
number on screen and the number that gets paid agree by construction.
|
|
|
|
### B-66 — nothing stops rounds from opening with no `fee_address` configured
|
|
|
|
`app/rounds/config.py:12-17`, `app/rounds/scheduler.py:330-335`.
|
|
|
|
A fresh instance starts with `fee_address = ""`. Rounds open, bets are accepted and
|
|
confirm, and only then does the payout refuse to build — leaving the round in
|
|
`paying_out`, retrying every 60 s, with the audit log as the only signal.
|
|
|
|
Fix: refuse to open a round while `fee_address` is unset (and surface it on
|
|
`/admin` and as a maintenance-style banner), so the failure happens before anyone's
|
|
money is committed.
|
|
|
|
### B-67 — `/qr/{address}` is unauthenticated, synchronous and only shape-validated
|
|
|
|
`app/api/routes/qr.py:11-21`.
|
|
|
|
`qrcode.make` runs on the event loop, so the endpoint is a cheap CPU amplifier for
|
|
an unauthenticated caller, and the regex accepts any `plm1[a-z0-9]{10,90}` string
|
|
without validating the bech32 checksum — so it happily renders a QR for a
|
|
non-address.
|
|
|
|
Fix: validate with `is_valid_plm_address` (already used by withdrawals and by the
|
|
admin `fee_address` validator), and either offload the render or cache it per
|
|
address.
|
|
|
|
---
|
|
|
|
## Low — documentation and consistency drift
|
|
|
|
### B-68 — CLAUDE.md and README describe a state the code has moved past
|
|
|
|
- CLAUDE.md's tech-stack line still says JWT has **no revocation (B-34)**; `token_version`
|
|
implements exactly that revocation (`app/db/models.py:29`, `app/auth/dependencies.py:26`).
|
|
- "Known gaps" still says **no rate limiting anywhere (B-33)**; login and registration
|
|
are throttled (`app/auth/rate_limit.py`). What is genuinely still unthrottled is
|
|
bets, withdrawals and the admin endpoints — that is the claim worth keeping.
|
|
- "Known gaps" still says **`/report-bug` is a placeholder**; it is fully implemented
|
|
and translated, with admin triage. Only `/guida` is still a stub.
|
|
- Test counts are stale in three places: 264 actual, CLAUDE.md says 253 twice,
|
|
README says 232.
|
|
- The code map omits `app/auth/rate_limit.py`, `app/api/client_ip.py` and
|
|
`app/api/routes/bug_reports.py`.
|
|
- README links `flowchart.mmd`, which does not exist (the diagrams live in
|
|
`flowchart/`), describes `docs/running-the-server.md` as covering "local venv vs.
|
|
Docker" (that workflow was removed in B-44), and links the anchor
|
|
`CLAUDE.md#tech-stack-mvp`, which no longer exists.
|
|
|
|
### B-69 — stale in-code comments
|
|
|
|
- `app/tx/reconcile.py:206` calls the payout retry "a future payout-retry routine —
|
|
still an open gap"; it exists (B-26).
|
|
- `app/db/base.py:9` says "five concurrent background tasks"; there are six.
|
|
- `app/auth/routes.py:31` cites B-31 where it means B-33 (see B-58).
|
|
|
|
### B-70 — the flowchart's WITHDRAW precondition is not implemented as written
|
|
|
|
`flowchart/platform-overview.mmd:39` (node E1) states a withdrawal cannot happen
|
|
together with a bet in progress. The code only serializes the *builds* through the
|
|
per-user lock (`app/tx/locks.py`): a withdrawal is accepted while a bet is still
|
|
unconfirmed, as long as confirmed UTXOs cover it.
|
|
|
|
CLAUDE.md declares every node and edge label of the diagrams a behaviour that must
|
|
be implemented as described, so one of the two has to move — most likely the
|
|
diagram, since the lock already prevents the actual double-spend hazard, but that
|
|
is a decision, not a cleanup.
|
|
|
|
### B-71 — `.env` points `MASTER_KEY_PATH` at a second copy of the master key
|
|
|
|
CLAUDE.md's deployment section prescribes pointing `MASTER_KEY_PATH` at the
|
|
host-side `./data/keys/master.xprv.enc` so the venv scripts and the container read
|
|
one file. `.env` instead sets `./master.xprv.enc`, and both files now exist in the
|
|
working tree (both gitignored). They were verified during this audit to decrypt to
|
|
the *same* xprv, so nothing has diverged yet — but `scripts/decrypt_master_key.py`
|
|
reads a different file from the one the container uses, and a future
|
|
`generate_master_key.py --overwrite` would split them silently, with an ops
|
|
recovery path that then reports the wrong key.
|
|
|
|
The same duplication exists for the database (`./plm_lottery.db` next to
|
|
`data/db/plm_lottery.db`), which is less dangerous but equally confusing.
|
|
|
|
Fix: set `MASTER_KEY_PATH=./data/keys/master.xprv.enc` in `.env`, delete the stray
|
|
root copy once confirmed redundant, and state the same for `DATABASE_URL`.
|
|
|
|
### B-72 — `docs/setup.md` still frames setup as "locally or via Docker"
|
|
|
|
`docs/setup.md:1-10` lists Python as a prerequisite "for the local/venv workflow"
|
|
and describes the master-key step as local *or* Docker, while B-44 made Docker the
|
|
only supported way to run the server (`docs/running-the-server.md` and README were
|
|
updated, this file was not). The venv genuinely is needed for tests, migrations and
|
|
the key scripts — the wording just needs to say that instead of implying a second
|
|
way to run the server.
|