From fb734bb8185a8cac878db92de533ebad709f3c31 Mon Sep 17 00:00:00 2001 From: Davide Grilli Date: Sun, 26 Jul 2026 22:10:50 +0200 Subject: [PATCH] Document the 24 bugs found in the full-codebase audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds BUGS.md: a severity-ordered backlog from the 2026-07-26 audit of every Python module, both static frontends and the Docker/Caddy deployment. Each entry carries symptom, root cause with file references, reproduction and a proposed fix, plus the regression test it needs — none of these are covered by the current suite. The five critical ones share a root cause worth stating up front: the code treats a broadcast as final and the Electrum connection as never failing, so there is no reconciliation between the DB's view and the chain's. That leaves several states the system only leaves via manual DB edits. Co-Authored-By: Claude Opus 5 --- BUGS.md | 613 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 613 insertions(+) create mode 100644 BUGS.md diff --git a/BUGS.md b/BUGS.md new file mode 100644 index 0000000..cdf6364 --- /dev/null +++ b/BUGS.md @@ -0,0 +1,613 @@ +# 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 `, 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.