davideandClaude Opus 5
c4b2dc3ea2
participant_count and jackpot_sats were computed over every round_participants row, while the draw only picks from confirmed participants and the payout only spends their sats. So the advertised jackpot could exceed the one paid out, and a player whose bet was later abandoned appeared in the count and then vanished again. Counting only confirmed rows would have fixed the arithmetic and broken something else: the player who just bet would see neither themselves nor their money for a whole block. So this is the same confirmed/in-flight split the balance already exposes (balance_sats vs pending_balance_sats): participant_count and jackpot_sats are now the confirmed, authoritative figures, and pending_participant_count/pending_jackpot_sats/has_pending_bets report what is in flight — inclusive figures, not deltas, matching the balance pair's convention. /'s round card shows the confirmed numbers big and the difference as an amber "+N in attesa" suffix, reusing .balance-pending's colour for the same "not settled yet" meaning. The two new spans render from server data through t(), so they carry no data-i18n and onLanguageChange() repaints them from the last response — the one-mechanism-per-element rule. Both strings are in all 7 languages. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
134 lines
6.4 KiB
Markdown
134 lines
6.4 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-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.
|