614 lines
30 KiB
Markdown
614 lines
30 KiB
Markdown
 
|
# Known bugs and required fixes
|
||
|
|
|
||
|
|
Full-codebase audit performed on 2026-07-26 against commit `d4e0974` (branch `main`),
|
||
|
|
covering every Python module under `app/`, both static frontends, and the Docker/Caddy
|
||
|
|
deployment. The test suite was green at the time of the audit (79 passed), so **none of
|
||
|
|
the findings below are caught by the existing tests** — every one of them needs a
|
||
|
|
regression test alongside its fix.
|
||
|
|
|
||
|
|
Findings are ordered by severity. Each entry is self-contained: symptom, root cause with
|
||
|
|
file references, how to reproduce, and the proposed fix. Items already listed under
|
||
|
|
"Known gaps / TODO" in `CLAUDE.md` are cross-referenced rather than repeated, except
|
||
|
|
where this audit found the gap to be worse than documented.
|
||
|
|
|
||
|
|
## Summary
|
||
|
|
|
||
|
|
| ID | Severity | Area | One-line |
|
||
|
|
|----|----------|------|----------|
|
||
|
|
| [B-01](#b-01) | Critical | Electrum client/listener | A dropped connection hangs the whole server permanently — no reconnect, no request timeout, no keepalive |
|
||
|
|
| [B-02](#b-02) | Critical | RBF / bets | A fee bump on a bet tx orphans `RoundParticipant.bet_txid`, wedging the round in `closing` forever |
|
||
|
|
| [B-03](#b-03) | Critical | Confirmation poller | One unresolvable txid aborts confirmation detection for every other pending tx |
|
||
|
|
| [B-04](#b-04) | Critical | Balance / UTXO state | No reconciliation path: a dropped tx freezes the spent UTXOs (and the user's funds) forever |
|
||
|
|
| [B-05](#b-05) | Critical | Admin config / payout | `fee_address` and numeric params are unvalidated; a bad value either wedges the payout or sends the commission to an unspendable script |
|
||
|
|
| [B-06](#b-06) | High | PSBT builder | Change outputs below the dust limit are created, making the transaction unrelayable |
|
||
|
|
| [B-07](#b-07) | High | Bets / withdrawals | A broadcast rejection surfaces as HTTP 500, bypassing the structured error contract |
|
||
|
|
| [B-08](#b-08) | High | Bets | Broadcast happens before the bet is persisted: a commit failure loses the money with no record |
|
||
|
|
| [B-09](#b-09) | High | Rounds | Race allows two simultaneously-`open` rounds, which permanently blocks all future rounds |
|
||
|
|
| [B-10](#b-10) | High | Admin / audit | `PUT /admin/config` writes no audit-log entry |
|
||
|
|
| [B-11](#b-11) | High | Rounds API | The displayed jackpot is not the amount the winner receives |
|
||
|
|
| [B-12](#b-12) | High | Auth | Registration enforces no password or username validation, unlike change-password |
|
||
|
|
| [B-13](#b-13) | Medium | Auth | `verify_password` turns a malformed stored hash into a 500 instead of a 401 |
|
||
|
|
| [B-14](#b-14) | Medium | Admin auth | Admin token compared with `!=` instead of a constant-time comparison |
|
||
|
|
| [B-15](#b-15) | Medium | Config | Empty `JWT_SECRET` / `XPRV_ENCRYPTION_KEY` are not rejected at startup |
|
||
|
|
| [B-16](#b-16) | Medium | Deployment | `GET /guida` is broken in Docker: `docs/` is never copied into the image |
|
||
|
|
| [B-17](#b-17) | Medium | RBF | `_find_change_output` can shrink the recipient output instead of the change |
|
||
|
|
| [B-18](#b-18) | Medium | Scheduler | DB session held open across Electrum network calls during payout |
|
||
|
|
| [B-19](#b-19) | Medium | Electrum listener | `tip_height` is assigned without a monotonicity check (reorg) |
|
||
|
|
| [B-20](#b-20) | Low | RBF | `PendingTransaction.replaced_by_txid` is never written — dead column shown in the admin UI |
|
||
|
|
| [B-21](#b-21) | Low | Confirmation poller | Dead local `pending_ids`, plus attribute access on detached ORM objects |
|
||
|
|
| [B-22](#b-22) | Low | Frontend | Amounts rendered by raw division — floating-point artefacts visible to users |
|
||
|
|
| [B-23](#b-23) | Low | Frontend | Concurrent `withLoading` on the same button can leave it stuck on its loading label |
|
||
|
|
| [B-24](#b-24) | Low | API contract | The global exception handler answers with a bare-string `detail` |
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Critical
|
||
|
|
|
||
|
|
### B-01
|
||
|
|
|
||
|
|
**A dropped Electrum connection hangs the entire server permanently**
|
||
|
|
|
||
|
|
*Files:* `app/electrum/client.py:31-63`, `app/electrum/client.py:85-99`, `app/electrum/listener.py:58-77`
|
||
|
|
|
||
|
|
Three independent defects compose into a single absorbing failure state:
|
||
|
|
|
||
|
|
1. **The read loop's death never propagates.** `connect()` spawns `_read_loop` as a
|
||
|
|
detached task. When the socket closes, `readline()` returns `b""`, the loop breaks and
|
||
|
|
the task completes — but `_run_once` is blocked on
|
||
|
|
`asyncio.gather(self._consume_headers(...), self._consume_scripthash(...))`, and both
|
||
|
|
consumers are awaiting `asyncio.Queue.get()` on queues nobody will ever fill again.
|
||
|
|
The `gather` never returns and never raises, so the `except Exception` +
|
||
|
|
backoff-reconnect logic in `ElectrumListener.run()` is never reached.
|
||
|
|
2. **`self.client` stays non-`None`.** Every consumer of the client
|
||
|
|
(`RoundScheduler`, `ConfirmationPoller`, `RbfBumper`, `place_bet`,
|
||
|
|
`request_withdrawal`, and the `listener.client is None` guards in the bet/withdrawal
|
||
|
|
routes) therefore keeps treating a dead connection as live.
|
||
|
|
3. **`request()` has no timeout.** It registers a future in `self._pending`, writes to a
|
||
|
|
closed writer (`drain()` frequently does not raise on a half-closed socket) and then
|
||
|
|
`await future` — which nothing will ever resolve, because `_read_loop` is gone. Note
|
||
|
|
that `_read_loop`'s `finally` only fails the futures that existed at the moment it
|
||
|
|
died; every future created afterwards hangs forever.
|
||
|
|
|
||
|
|
There is also **no keepalive** (`server.ping`) anywhere. Electrum servers routinely drop
|
||
|
|
idle connections after ~10 minutes, so on a low-traffic instance this is close to
|
||
|
|
guaranteed rather than hypothetical.
|
||
|
|
|
||
|
|
*Observable consequences:* deposits stop being credited; the tip stops advancing, so a
|
||
|
|
round in `drawing` waits in `_wait_for_next_block` forever; `ConfirmationPoller` blocks
|
||
|
|
on its first `get_transaction` and stops polling entirely; a `POST /bets` request hangs
|
||
|
|
indefinitely **while holding that user's `UserLocks` entry**, so every later bet or
|
||
|
|
withdrawal from the same user deadlocks behind it.
|
||
|
|
|
||
|
|
*Reproduction:* start the app, kill the TCP connection out from under it
|
||
|
|
(`ss -K dst <electrum-host>`, or block the port with a firewall rule). No reconnect log
|
||
|
|
line is emitted, `/rounds/current` keeps reporting the last known `chain_tip_height`, and
|
||
|
|
a bet request never returns.
|
||
|
|
|
||
|
|
*Proposed fix:*
|
||
|
|
- Wrap every `request()` in `asyncio.wait_for(...)` with an explicit timeout (10-15s) and
|
||
|
|
pop the future from `self._pending` on timeout.
|
||
|
|
- Add a keepalive task issuing `server.ping` every ~60s; a failed ping tears the
|
||
|
|
connection down.
|
||
|
|
- Make the read loop's termination observable: either include `self._read_task` in
|
||
|
|
`_run_once`'s `gather`, or set an `asyncio.Event` in `_read_loop`'s `finally` that
|
||
|
|
`_run_once` awaits alongside the consumers. On teardown, set `self.client = None`
|
||
|
|
*before* awaiting the reconnect backoff.
|
||
|
|
|
||
|
|
*Test:* a unit test with a fake stream that closes mid-session must observe
|
||
|
|
`listener.client is None` and a reconnect attempt; a test that `request()` raises
|
||
|
|
`ElectrumError`/`TimeoutError` rather than hanging when no response arrives.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### B-02
|
||
|
|
|
||
|
|
**An RBF fee bump on a bet permanently wedges the round**
|
||
|
|
|
||
|
|
*Files:* `app/tx/broadcast.py:116-121`, `app/bets/confirmation.py:10-16`,
|
||
|
|
`app/rounds/scheduler.py:77-86`, `app/rounds/scheduler.py:92-98`
|
||
|
|
|
||
|
|
`bump_fee` rebuilds the transaction, which changes its txid, and updates
|
||
|
|
`PendingTransaction.current_txid` accordingly. Nothing updates the domain row's copy of
|
||
|
|
that txid: `RoundParticipant.bet_txid` is written exactly once, at
|
||
|
|
`app/bets/service.py:80`, and never again (verified by grepping every write to
|
||
|
|
`bet_txid`).
|
||
|
|
|
||
|
|
When the bumped transaction confirms, `_on_bet_confirmed` looks the participant up by
|
||
|
|
`RoundParticipant.bet_txid == pending.current_txid`, finds nothing, and returns silently.
|
||
|
|
Consequences, in order:
|
||
|
|
|
||
|
|
- the participant stays `status == "broadcast"` forever;
|
||
|
|
- `RoundScheduler._tick` counts participants in `"broadcast"` before allowing the round to
|
||
|
|
close, so the round stays in `closing` **indefinitely**;
|
||
|
|
- since a new round cannot open while one is active (`get_active_round`), the entire
|
||
|
|
lottery stops;
|
||
|
|
- even if the round were forced closed, `_close_and_draw` only selects
|
||
|
|
`status == "confirmed"` participants, so that player would have paid the bet without
|
||
|
|
entering the draw.
|
||
|
|
|
||
|
|
The same staleness affects `Withdrawal.txid`. It is less severe there, because
|
||
|
|
`_on_withdrawal_confirmed` keys off `pending.withdrawal_id`, so the status does update —
|
||
|
|
but the txid stored and shown to the user is the replaced one.
|
||
|
|
|
||
|
|
*Reproduction:* place a bet, let `rbf_timeout_seconds` elapse without confirmation so
|
||
|
|
`RbfBumper` bumps it, then let the bumped tx confirm. `round_participants.status` remains
|
||
|
|
`broadcast`; the round never leaves `closing`.
|
||
|
|
|
||
|
|
*Proposed fix:* make the bump update the domain row too. Either have `bump_fee` write
|
||
|
|
through to `RoundParticipant.bet_txid` / `Withdrawal.txid` (it already knows `kind`,
|
||
|
|
`user_id`, `round_id`, `withdrawal_id`), or — cleaner — stop keying confirmation handlers
|
||
|
|
on the txid at all: `_on_bet_confirmed` should resolve the participant via
|
||
|
|
`pending.round_id` + `pending.user_id`, which are immutable. Also set
|
||
|
|
`replaced_by_txid` (see [B-20](#b-20)) so the chain of replacements is auditable.
|
||
|
|
|
||
|
|
*Test:* bump a pending bet, then run the confirmation handler with the new txid and assert
|
||
|
|
the participant flips to `confirmed`.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### B-03
|
||
|
|
|
||
|
|
**One unresolvable txid stops confirmation detection for everything else**
|
||
|
|
|
||
|
|
*File:* `app/tx/confirmation.py:35-37`
|
||
|
|
|
||
|
|
```python
|
||
|
|
for pending_id, txid, kind in [...]:
|
||
|
|
tx = await client.get_transaction(txid, verbose=True) # not guarded
|
||
|
|
```
|
||
|
|
|
||
|
|
If a txid is no longer knowable by the server — dropped from the mempool, replaced by an
|
||
|
|
RBF bump whose old entry is still `pending`, or simply an Electrum server that answers
|
||
|
|
with an error — `ElectrumError` propagates out of `poll_once` and **aborts the loop before
|
||
|
|
the remaining pending transactions are checked**. `ConfirmationPoller.run` logs it and
|
||
|
|
retries every 10s, failing at the same row every time.
|
||
|
|
|
||
|
|
Because the poller is the single mechanism that confirms bets, payouts and withdrawals,
|
||
|
|
one stuck row means: no bet ever confirms again (so no round ever closes — see
|
||
|
|
[B-02](#b-02)), no payout ever completes, no withdrawal ever settles. Recovery requires
|
||
|
|
manual DB surgery.
|
||
|
|
|
||
|
|
*Proposed fix:* wrap the per-transaction lookup in `try/except Exception`, log at
|
||
|
|
`warning` with the txid, and `continue`. Optionally track consecutive failures per row so
|
||
|
|
a permanently-unknown tx can be escalated to an operator (which is also the hook
|
||
|
|
[B-04](#b-04) needs).
|
||
|
|
|
||
|
|
*Test:* `poll_once` with two pending rows where the first raises must still confirm the
|
||
|
|
second and return `1`.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### B-04
|
||
|
|
|
||
|
|
**No reconciliation: a dropped transaction freezes the spent UTXOs forever**
|
||
|
|
|
||
|
|
*Files:* `app/bets/service.py:69-73`, `app/withdrawals/service.py:65-68`,
|
||
|
|
`app/wallet/balance.py:9-18`
|
||
|
|
|
||
|
|
`spent_txid` is set optimistically at broadcast time and is **never cleared anywhere in
|
||
|
|
the codebase**. `recompute_balance` only sums UTXOs with `spent_txid IS NULL`, so those
|
||
|
|
coins are permanently excluded from the user's balance.
|
||
|
|
|
||
|
|
If the transaction never confirms and eventually disappears from the mempool — which
|
||
|
|
`CLAUDE.md` already acknowledges is a realistic outcome, since the RBF path raises
|
||
|
|
`RbfError` and gives up whenever there is no change output large enough to absorb the
|
||
|
|
bump — the UTXOs remain perfectly spendable on-chain while the database considers them
|
||
|
|
gone. The user's funds are silently lost from their point of view.
|
||
|
|
|
||
|
|
More broadly, nothing in the system ever writes a terminal failure state: there is no
|
||
|
|
`status = "failed"` on `PendingTransaction`, `Withdrawal` or `RoundParticipant`, and no
|
||
|
|
startup routine that re-scans in-flight transactions against the chain. Combined with
|
||
|
|
`restart: unless-stopped` and the already-documented scheduler-resume gap, the system has
|
||
|
|
several states it can only be pulled out of by hand.
|
||
|
|
|
||
|
|
*Proposed fix:* add a reconciliation task (and run it once at startup) that, for every
|
||
|
|
`PendingTransaction` in `pending` older than some threshold, asks the chain whether the
|
||
|
|
tx exists. If it is gone:
|
||
|
|
- mark the row `failed`, and set `replaced_by_txid`/a failure reason;
|
||
|
|
- clear `spent_txid` on the UTXOs it consumed (they are identifiable by parsing
|
||
|
|
`raw_tx_hex`'s inputs);
|
||
|
|
- call `recompute_balance`;
|
||
|
|
- roll the domain row back (delete the `RoundParticipant`, mark the `Withdrawal`
|
||
|
|
`failed`) and audit-log the event.
|
||
|
|
|
||
|
|
*Test:* given a pending bet whose tx is unknown to the chain, the reconciler must restore
|
||
|
|
the user's balance and remove the participant.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### B-05
|
||
|
|
|
||
|
|
**`fee_address` and the numeric config params are unvalidated**
|
||
|
|
|
||
|
|
*File:* `app/api/routes/admin.py:70-80`, consumed at `app/rounds/scheduler.py:162-198`
|
||
|
|
|
||
|
|
`PUT /admin/config` assigns whatever it is given straight onto the `RoundConfig` row. Two
|
||
|
|
distinct failure modes:
|
||
|
|
|
||
|
|
1. **Malformed `fee_address`** → `build_payout_transaction` calls
|
||
|
|
`script.Script.from_address(fee_address)`, which raises `EmbitError`. `_trigger_payout`
|
||
|
|
only catches `InsufficientFundsError`, so the exception escapes through
|
||
|
|
`_close_and_draw` up to the `except Exception` in `RoundScheduler.run()`. The round is
|
||
|
|
left in `paying_out` with no retry (the payout-retry gap in `CLAUDE.md`), i.e. wedged.
|
||
|
|
2. **Well-formed but foreign `fee_address`** (e.g. a Bitcoin `bc1...`) → this is *worse*,
|
||
|
|
because it parses fine into a valid witness program. The payout is built, signed and
|
||
|
|
broadcast, and the 30 % commission of every round lands on a script nobody holds the
|
||
|
|
key for. This is exactly the failure mode `app/wallet/address.py` was written to
|
||
|
|
prevent for user withdrawals; the check was never applied to the operator-supplied
|
||
|
|
address.
|
||
|
|
|
||
|
|
Numeric fields are equally unguarded: `round_duration_seconds = 0` (round expires the
|
||
|
|
instant it opens), `fee_rate_sat_vb = 0` (fee-less transactions are never relayed, so
|
||
|
|
every bet/payout hangs and the whole pipeline stalls), negative `bet_amount_sats`,
|
||
|
|
`draw_animation_seconds` far larger than the round itself.
|
||
|
|
|
||
|
|
*Proposed fix:* validate in `RoundConfigUpdate` (Pydantic `field_validator` /
|
||
|
|
`Field(gt=0)`):
|
||
|
|
- `fee_address` must pass `is_valid_plm_address`;
|
||
|
|
- `bet_amount_sats`, `round_duration_seconds`, `fee_rate_sat_vb`, `rbf_timeout_seconds`
|
||
|
|
strictly positive, with sane upper bounds;
|
||
|
|
- `round_cooldown_seconds`, `draw_animation_seconds` non-negative.
|
||
|
|
|
||
|
|
Independently, `_trigger_payout` should catch `Exception` around the build/broadcast,
|
||
|
|
audit-log the failure, and leave the round in a state a retry routine can pick up.
|
||
|
|
|
||
|
|
*Test:* `PUT /admin/config` with a `bc1...` fee address and with `fee_rate_sat_vb = 0`
|
||
|
|
must both return 422/400 and leave the stored config untouched.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## High
|
||
|
|
|
||
|
|
### B-06
|
||
|
|
|
||
|
|
**Dust change outputs are created**
|
||
|
|
|
||
|
|
*File:* `app/wallet/psbt_builder.py:98-99` and `:166-167`
|
||
|
|
|
||
|
|
Both builders use `if change > 0:` as the sole condition for adding the change output. Any
|
||
|
|
leftover below the P2WPKH dust threshold (~294 sat at the standard 3000 sat/kvB dust
|
||
|
|
relay fee) produces a transaction that relaying nodes reject outright as `dust`. The
|
||
|
|
resulting `ElectrumError` is unhandled (see [B-07](#b-07)), so the user gets an opaque
|
||
|
|
HTTP 500 and cannot bet or withdraw at all until their UTXO set happens to change.
|
||
|
|
|
||
|
|
*Proposed fix:* introduce a `DUST_LIMIT_SATS` constant; add the change output only when
|
||
|
|
`change >= DUST_LIMIT_SATS`, otherwise leave the remainder to the fee (and reflect that in
|
||
|
|
the returned `fee_sats`/`change_sats` so the accounting stays honest). Note the fee
|
||
|
|
estimate at `psbt_builder.py:86` already assumes two outputs unconditionally, so dropping
|
||
|
|
change does not underpay.
|
||
|
|
|
||
|
|
*Test:* build a transaction whose change lands at 100 sat and assert a single-output tx
|
||
|
|
with the remainder folded into the fee.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### B-07
|
||
|
|
|
||
|
|
**Broadcast failures surface as HTTP 500**
|
||
|
|
|
||
|
|
*Files:* `app/bets/service.py:67`, `app/withdrawals/service.py:63`
|
||
|
|
|
||
|
|
`await client.broadcast(built.raw_hex)` is not guarded. Every node-side rejection —
|
||
|
|
fee below the relay minimum, dust output ([B-06](#b-06)), mempool conflict, non-standard
|
||
|
|
tx — raises `ElectrumError`, which the global handler in `app/main.py:74-77` turns into
|
||
|
|
`{"detail": "internal server error"}`. That bypasses the whole error contract documented
|
||
|
|
in `app/api/errors.py`: no `code`, nothing translatable, no actionable message.
|
||
|
|
|
||
|
|
*Proposed fix:* wrap the broadcast, and raise
|
||
|
|
`BetError("broadcast_failed", ...)` / `WithdrawalError("broadcast_failed", ...)` carrying
|
||
|
|
the node's message in `params`. Add `error.broadcast_failed` to all 7 languages in
|
||
|
|
`app/static/i18n.js` (per the i18n contract in `CLAUDE.md`). Answer 502/503 rather than
|
||
|
|
400, since the failure is not the client's fault.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### B-08
|
||
|
|
|
||
|
|
**Bets are broadcast before they are persisted**
|
||
|
|
|
||
|
|
*File:* `app/bets/service.py:67-94`
|
||
|
|
|
||
|
|
The order today is: broadcast → mutate `spent_txid` → insert `RoundParticipant` +
|
||
|
|
`PendingTransaction` → `commit()`. If the commit fails, or the process dies in that
|
||
|
|
window, the transaction is already irreversibly on the chain: the money has moved to the
|
||
|
|
pool address, but **no record of the bet exists** — no participant (so no entry in the
|
||
|
|
draw), no pending row (so no RBF, no confirmation tracking), and the UTXOs are not even
|
||
|
|
marked spent, so the next bet attempt will try to double-spend them.
|
||
|
|
|
||
|
|
The same shape exists in `request_withdrawal`, where the `Withdrawal` row is flushed
|
||
|
|
before broadcast but only committed after.
|
||
|
|
|
||
|
|
*Proposed fix:* persist the intent first. Insert the `PendingTransaction` (and the
|
||
|
|
`RoundParticipant`) in a `building`/`unbroadcast` state, commit, then broadcast, then flip
|
||
|
|
to `broadcast` in a second commit. A crash between the two leaves a row the
|
||
|
|
reconciliation task from [B-04](#b-04) can resolve against the chain in either direction.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### B-09
|
||
|
|
|
||
|
|
**Two rounds can be opened concurrently, blocking every future round**
|
||
|
|
|
||
|
|
*File:* `app/rounds/service.py:44-60`
|
||
|
|
|
||
|
|
`open_new_round_if_needed` performs a read (`get_active_round`) followed by an insert with
|
||
|
|
no lock and no database-level uniqueness guarantee, and it is called both from
|
||
|
|
`RoundScheduler._tick` and from **every** `place_bet`. Two concurrent callers can both
|
||
|
|
observe "no active round" and both insert a row with `status="open"`.
|
||
|
|
|
||
|
|
`get_active_round` then quietly hides the problem — `select(...).order_by(Round.id.desc())`
|
||
|
|
+ `scalar()` returns only the newest — while the older round stays `open` forever, never
|
||
|
|
ticked, never closed. Since `get_active_round` matches on status, no further round can
|
||
|
|
ever open once the newer one closes: the lottery stops for good.
|
||
|
|
|
||
|
|
*Proposed fix:* the simplest robust option is to make the `RoundScheduler` the only writer
|
||
|
|
and have `place_bet` merely read the active round (rejecting the bet if there is none —
|
||
|
|
the scheduler opens one within 5s anyway). Additionally add a DB guard: a partial unique
|
||
|
|
index over `status IN ('open','closing','drawing','paying_out')`, or an advisory/serialized
|
||
|
|
transaction around the read-then-insert. `get_active_round` should also log loudly if it
|
||
|
|
ever sees more than one row.
|
||
|
|
|
||
|
|
*Test:* two concurrent `open_new_round_if_needed` calls on separate sessions must yield
|
||
|
|
exactly one round.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### B-10
|
||
|
|
|
||
|
|
**`PUT /admin/config` writes no audit-log entry**
|
||
|
|
|
||
|
|
*File:* `app/api/routes/admin.py:70-80`
|
||
|
|
|
||
|
|
`/admin/pause` and `/admin/resume` both call `write_audit_log`; the config update — which
|
||
|
|
can change `fee_address` (where 30 % of every pool goes), `bet_amount_sats` and the fee
|
||
|
|
rate — writes nothing. This contradicts `CLAUDE.md`, which states that `audit_log`
|
||
|
|
records *what* changed (only the *who* is documented as missing).
|
||
|
|
|
||
|
|
Secondary issue: `paused` is part of `_CONFIG_FIELDS`, so it can be toggled through
|
||
|
|
`PUT /config`, silently bypassing the deliberately audit-logged `pause`/`resume`
|
||
|
|
endpoints.
|
||
|
|
|
||
|
|
*Proposed fix:* write a `config_updated` audit entry containing the changed fields as
|
||
|
|
`{field: {"from": old, "to": new}}` — computed before assignment. Remove `paused` from
|
||
|
|
`_CONFIG_FIELDS` (keep it in the response model) so the pause switch has exactly one
|
||
|
|
audited path.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### B-11
|
||
|
|
|
||
|
|
**The displayed jackpot is not what the winner receives**
|
||
|
|
|
||
|
|
*File:* `app/api/routes/rounds.py:118-144`
|
||
|
|
|
||
|
|
```python
|
||
|
|
pool_amount_sats = participant_count * config.bet_amount_sats
|
||
|
|
jackpot_sats = pool_amount_sats * 70 // 100
|
||
|
|
```
|
||
|
|
|
||
|
|
Three independent divergences from the amount actually paid out:
|
||
|
|
|
||
|
|
1. The real pool is `sum(p.bet_amount_sats)`, and each participant's `bet_amount_sats` is
|
||
|
|
`recipient_sats` — the bet amount **minus that bet's network fee**
|
||
|
|
(`psbt_builder.build_signed_transaction`).
|
||
|
|
2. The payout deducts its own network fee from the winner's 70 % share
|
||
|
|
(`build_payout_transaction`), so the winner receives strictly less than 70 % of the
|
||
|
|
pool.
|
||
|
|
3. `participant_count` counts every `RoundParticipant` row, including bets still in
|
||
|
|
`broadcast` that may never confirm — and those are excluded from the draw and the pool
|
||
|
|
by `_close_and_draw`.
|
||
|
|
|
||
|
|
Plus: it multiplies by the *current* `bet_amount_sats`, which an operator can change
|
||
|
|
mid-round, retroactively changing the advertised jackpot of a round already in progress.
|
||
|
|
|
||
|
|
The in-code comment explicitly promises "what's displayed should match what the winner
|
||
|
|
actually receives", which is the opposite of the behaviour.
|
||
|
|
|
||
|
|
*Proposed fix:* compute the pool from the participants' stored `bet_amount_sats`
|
||
|
|
(optionally restricted to `confirmed`), derive the 70 % share from that, and either
|
||
|
|
subtract an estimated payout fee or relabel the field as an estimate in the UI. Keep
|
||
|
|
`participant_count` as-is for display but consider exposing `confirmed_count` separately.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### B-12
|
||
|
|
|
||
|
|
**Registration has no password or username validation**
|
||
|
|
|
||
|
|
*File:* `app/auth/routes.py:18-56`
|
||
|
|
|
||
|
|
`POST /users/me/change-password` enforces `_MIN_PASSWORD_LENGTH = 8`
|
||
|
|
(`app/api/routes/users.py:15`), and the frontend re-checks it. `POST /auth/register`
|
||
|
|
enforces nothing: an empty username and a one-character password are both accepted, and
|
||
|
|
there is no `minlength` on the registration inputs in `app/static/index.html` either. On a
|
||
|
|
custodial system holding real funds, this is the wrong default.
|
||
|
|
|
||
|
|
Secondary defect in the same handler: the `_MAX_REGISTER_RETRIES` loop catches *any*
|
||
|
|
`IntegrityError`. A username collision that slips past the pre-check (concurrent
|
||
|
|
registration) is therefore retried five times and finally reported as
|
||
|
|
`derivation_index_conflict`, which is misleading for both the user and the operator.
|
||
|
|
|
||
|
|
*Proposed fix:* validate `username` (length, allowed character set) and `password`
|
||
|
|
(shared minimum-length constant, reused by `users.py`) in `RegisterRequest`; mirror the
|
||
|
|
constraints in the HTML form. Inspect the `IntegrityError` and re-raise `username_taken`
|
||
|
|
when it is the username constraint that failed.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Medium
|
||
|
|
|
||
|
|
### B-13
|
||
|
|
|
||
|
|
**A malformed stored hash becomes a 500 instead of a 401**
|
||
|
|
|
||
|
|
*File:* `app/auth/security.py:16-20`
|
||
|
|
|
||
|
|
`verify_password` catches only `VerifyMismatchError`. Argon2 raises `InvalidHashError` for
|
||
|
|
a hash it cannot parse and `VerificationError` for other verification failures, both of
|
||
|
|
which escape as an unhandled 500. Catch `argon2.exceptions.VerificationError` (the
|
||
|
|
superclass of `VerifyMismatchError`) plus `InvalidHashError` and return `False`.
|
||
|
|
|
||
|
|
### B-14
|
||
|
|
|
||
|
|
**Admin token compared non-constant-time**
|
||
|
|
|
||
|
|
*File:* `app/api/routes/admin.py:20-22`
|
||
|
|
|
||
|
|
`x_admin_token != settings.admin_token` is a short-circuiting comparison. This token gates
|
||
|
|
the private-key export endpoint, so it deserves `secrets.compare_digest`. Keep the
|
||
|
|
existing "empty token means always deny" behaviour.
|
||
|
|
|
||
|
|
### B-15
|
||
|
|
|
||
|
|
**Empty secrets are not rejected at startup**
|
||
|
|
|
||
|
|
*File:* `app/config.py:13-18`
|
||
|
|
|
||
|
|
`jwt_secret` and `xprv_encryption_key` both default to `""`. With an empty `JWT_SECRET`,
|
||
|
|
PyJWT raises `InvalidKeyError: HMAC key must not be empty` on every login and
|
||
|
|
registration — a 500 with no hint about the real cause (verified locally). With an empty
|
||
|
|
`XPRV_ENCRYPTION_KEY`, Fernet fails on the first key derivation instead. Both should fail
|
||
|
|
fast: a Pydantic validator that rejects empty values (and, for `jwt_secret`, a minimum
|
||
|
|
length) so the container refuses to start rather than half-working.
|
||
|
|
|
||
|
|
### B-16
|
||
|
|
|
||
|
|
**`GET /guida` is broken in Docker**
|
||
|
|
|
||
|
|
*Files:* `Dockerfile:38-42`, `app/main.py:98-103`
|
||
|
|
|
||
|
|
The image copies `pyproject.toml`, `app/`, `migrations/`, `alembic.ini` and `scripts/`.
|
||
|
|
`docs/` is never copied (it is not in `.dockerignore` — it is simply not `COPY`-ed), so
|
||
|
|
`FileResponse("docs/guida-utente.md")` raises inside the container and the help link in
|
||
|
|
the navbar returns a 500 in every real deployment. Add `COPY docs ./docs` to the
|
||
|
|
Dockerfile (and treat the guide as a shipped asset), or serve the guide from `app/static/`.
|
||
|
|
|
||
|
|
### B-17
|
||
|
|
|
||
|
|
**`_find_change_output` can shrink the recipient output**
|
||
|
|
|
||
|
|
*File:* `app/tx/broadcast.py:59-63`
|
||
|
|
|
||
|
|
It returns the *first* output whose address matches the sender's own address. Nothing
|
||
|
|
prevents a withdrawal to the user's own address (`request_withdrawal` never compares
|
||
|
|
`external_address` with `user.address`), in which case output 0 is the recipient and
|
||
|
|
output 1 is the change — and a fee bump would reduce the recipient output. The same case
|
||
|
|
makes `compute_pending_balance` (`app/wallet/balance.py:55-59`) count both outputs, so the
|
||
|
|
displayed pending balance double-counts the withdrawn amount.
|
||
|
|
|
||
|
|
*Proposed fix:* reject a withdrawal whose destination is the user's own address (a
|
||
|
|
distinct error code — it is a user mistake, not a system limit), and/or identify the
|
||
|
|
change output by index rather than by address, recording it on `PendingTransaction` at
|
||
|
|
build time.
|
||
|
|
|
||
|
|
### B-18
|
||
|
|
|
||
|
|
**DB session held open across Electrum network calls during payout**
|
||
|
|
|
||
|
|
*File:* `app/rounds/scheduler.py:162-220`
|
||
|
|
|
||
|
|
`_trigger_payout` opens a session and, inside it, awaits `client.listunspent(...)` and
|
||
|
|
`client.broadcast(...)` before committing. On SQLite that holds the write lock for the
|
||
|
|
duration of two network round-trips (unbounded, per [B-01](#b-01)); on Postgres it becomes
|
||
|
|
a long-running transaction. Restructure as: read config/winner and close the session →
|
||
|
|
do the network work → reopen a session to persist the result.
|
||
|
|
|
||
|
|
### B-19
|
||
|
|
|
||
|
|
**`tip_height` is assigned without a monotonicity check**
|
||
|
|
|
||
|
|
*File:* `app/electrum/listener.py:92-101`
|
||
|
|
|
||
|
|
`self.tip_height = header["height"]` accepts a lower height on a reorg. Since
|
||
|
|
`_wait_for_next_block` compares `self._listener.tip_height <= tip_at_close`, a regression
|
||
|
|
silently extends the wait by a block. Reorg handling is out of scope for v1 by explicit
|
||
|
|
design decision, but the assignment should at least be `max(...)`, or log a warning when
|
||
|
|
the height goes backwards.
|
||
|
|
|
||
|
|
## Low / hygiene
|
||
|
|
|
||
|
|
### B-20
|
||
|
|
|
||
|
|
**`replaced_by_txid` is never written**
|
||
|
|
|
||
|
|
*Files:* `app/db/models.py:119`, `app/tx/broadcast.py:116-120`,
|
||
|
|
`app/api/routes/admin.py:261`
|
||
|
|
|
||
|
|
`bump_fee` overwrites `current_txid` in place and never records what the old txid was. The
|
||
|
|
column exists, is selected, and is rendered in the admin "Transazioni pendenti" table,
|
||
|
|
where it therefore always shows `—`. Set it in `bump_fee` (part of the [B-02](#b-02) fix)
|
||
|
|
so the replacement chain is auditable.
|
||
|
|
|
||
|
|
### B-21
|
||
|
|
|
||
|
|
**Dead code and detached-object access in the confirmation poller**
|
||
|
|
|
||
|
|
*File:* `app/tx/confirmation.py:29-35`
|
||
|
|
|
||
|
|
`pending_ids` (line 32) is computed and never used. Line 35 rebuilds the same tuples from
|
||
|
|
ORM objects whose session has already been closed; it works today only because
|
||
|
|
`expire_on_commit=False` and no commit intervened, so flipping that engine setting would
|
||
|
|
break it silently. Select the plain columns in the query
|
||
|
|
(`select(PendingTransaction.id, ..., PendingTransaction.kind)`) instead of hydrating
|
||
|
|
entities.
|
||
|
|
|
||
|
|
### B-22
|
||
|
|
|
||
|
|
**Amounts rendered by raw division**
|
||
|
|
|
||
|
|
*Files:* `app/static/app.js` (multiple: `:295`, `:424`, `:660`, `:673`, `:680`),
|
||
|
|
`app/static/admin.js` (table cells)
|
||
|
|
|
||
|
|
Every amount is displayed as `sats / SATS_PER_PLM` with no formatting, so values like
|
||
|
|
`0.7000000000000001` are reachable in the balance, the jackpot and the admin tables. Add a
|
||
|
|
single `formatPlm(sats)` helper (fixed decimals, locale-aware grouping) and route every
|
||
|
|
display through it.
|
||
|
|
|
||
|
|
### B-23
|
||
|
|
|
||
|
|
**Concurrent `withLoading` on the same button**
|
||
|
|
|
||
|
|
*File:* `app/static/app.js:20-31`
|
||
|
|
|
||
|
|
`withLoading` snapshots `button.innerHTML` and restores it in `finally`. `refreshMe()` is
|
||
|
|
invoked from the SSE handler, the poll chain, `placeBet`, `withdraw` and `showDashboard`,
|
||
|
|
all sharing `#refresh-btn`. Two overlapping calls make the second snapshot the *loading*
|
||
|
|
label, which it then restores permanently — leaving the button stuck on
|
||
|
|
"Aggiornamento…". Guard with a per-button in-flight flag (bail out, or await the existing
|
||
|
|
promise).
|
||
|
|
|
||
|
|
### B-24
|
||
|
|
|
||
|
|
**The global exception handler breaks the error contract**
|
||
|
|
|
||
|
|
*File:* `app/main.py:74-77`
|
||
|
|
|
||
|
|
It answers `{"detail": "internal server error"}` — a bare string, while
|
||
|
|
`app/api/errors.py` documents `detail` as `{"code", "message", "params"}`. The frontend
|
||
|
|
tolerates a string (`apiErrorMessage` handles that case), but the contract should be
|
||
|
|
uniform: return `ApiError("internal_error", "internal server error").as_detail()` and add
|
||
|
|
`error.internal_error` to the i18n table.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Cross-cutting observation
|
||
|
|
|
||
|
|
[B-01](#b-01) through [B-04](#b-04) share one root cause: **the code treats a broadcast as
|
||
|
|
final and the network connection as never failing.** There is no reconciliation between
|
||
|
|
the database's view of the world and the chain's — no re-scan at startup, no terminal
|
||
|
|
`failed` state, no request timeouts, no "unstick" routine. Combined with the already
|
||
|
|
documented scheduler-resume gap and `restart: unless-stopped` in `docker-compose.yml`,
|
||
|
|
the system has several absorbing states from which only manual database edits recover.
|
||
|
|
|
||
|
|
Fixing that class of problem is more valuable than any individual item above. Suggested
|
||
|
|
order of work, by expected time-to-first-occurrence in production:
|
||
|
|
|
||
|
|
1. [B-01](#b-01) — will happen within hours of deployment.
|
||
|
|
2. [B-03](#b-03) — one bad row is enough, and it is silent.
|
||
|
|
3. [B-02](#b-02) — happens on the first bump the RBF loop actually performs.
|
||
|
|
4. [B-06](#b-06) — depends only on the user's UTXO shape.
|
||
|
|
5. [B-05](#b-05) — happens on the first operator typo, and is unrecoverable in the
|
||
|
|
foreign-address variant.
|
||
|
|
6. [B-04](#b-04) + [B-08](#b-08) — the reconciliation layer both of them need.
|
||
|
|
7. Everything else.
|