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.