From 7224ca0e6606b2bcc25528691e1c3229197a7e47 Mon Sep 17 00:00:00 2001 From: Davide Grilli Date: Mon, 27 Jul 2026 09:42:24 +0200 Subject: [PATCH] Shorten the fixed B-25/26/27 entries in BUGS.md Same as before: once a bug is fixed, its long write-up collapses into a short paragraph pointing at the fix commits instead of repeating what the code and commit messages already say. File goes from 442 to 343 lines. --- BUGS.md | 142 +++++++++----------------------------------------------- 1 file changed, 22 insertions(+), 120 deletions(-) diff --git a/BUGS.md b/BUGS.md index 495b281..8f47162 100644 --- a/BUGS.md +++ b/BUGS.md @@ -1,23 +1,16 @@ # Known bugs A second full-codebase audit on 2026-07-27 found **25 further issues** (4 critical, 6 high, -7 medium, 8 low), listed below as B-25 … B-49. B-25, B-26 and B-27 are fixed as of 2026-07-27; -the other 22 are open. The 139-test suite was green at the time of the audit, so none of these -were caught by existing coverage — every fix lands with a regression test (B-25, B-26 and B-27 -together brought the suite from 139 to 151). +7 medium, 8 low), listed below as B-25 … B-49. B-25, B-26 and B-27 are fixed (see "Previously +fixed" below); the other 22 are open. The 139-test suite was green at the time of the audit, so +none of these were caught by existing coverage — every fix lands with a regression test (the +three fixes so far brought the suite from 139 to 151). -The recurring pattern across B-29 and B-36 is worth stating once: the code is rigorous about -the failure modes that have actually been hit, and silent about the ones that have not. -Outgoing transactions reconcile; deposits do not. - -**`paying_out` is now fully recoverable, not just idempotent.** B-25 made a payout retry -*safe* (persisted before broadcast, guarded against double-spend); B-26 made it *automatic* -(the scheduler retries a stuck `paying_out` round on its own, throttled, and every failure — -including ones that used to fail silently — is now audit-logged with a reason). Together -these close every way a payout specifically could wedge the lottery forever. What's still open -in the same family is narrower: the "drawing" phase (waiting on a block) has no equivalent -resume-after-restart or stall visibility — see B-36 and the "scheduler doesn't resume" entry -in CLAUDE.md's Known gaps, which this doesn't touch. +The recurring pattern across the open findings is worth stating once: the code is rigorous +about the failure modes that have actually been hit, and silent about the ones that have not. +Outgoing transactions reconcile; deposits do not (B-29). The payout phase is now fully +recoverable; the "drawing" phase (waiting on a block) still has no equivalent +resume-after-restart or stall visibility (B-36). For limitations that are accepted by design rather than bugs (single-shared-token admin auth, single-process assumptions, no user-facing history, etc.), see "Known gaps / TODO" in @@ -126,8 +119,8 @@ ignores the `le=10_000` bound the admin panel enforces on the config field. **Proposed fix.** Compute the delta from the actual replacement vsize (`fee_delta = max(new_fee - old_fee, ceil(vsize * incremental_relay_rate))`) so the bump is always relay-valid. Cap `new_fee_rate` at the configured maximum and raise `RbfError` once -reached, so the transaction falls through to the reconciler (which needs B-27 fixed to -actually act on it) rather than being retried indefinitely. +reached, so the transaction falls through to the reconciler (which, since B-27, correctly +abandons it) rather than being retried indefinitely. ### B-33 — No brute-force protection on a custodial wallet @@ -328,108 +321,17 @@ already does. ## Previously fixed -### B-25 — The payout has no two-phase write, unlike bets and withdrawals - -`rounds/scheduler.py` used to broadcast the payout and only afterwards write `payout_txid` -and the `PendingTransaction`. A crash in that window — and `docker-compose.yml` sets -`restart: unless-stopped`, so a crash means an automatic restart — left a payout on-chain -with **no record at all**: the round stuck in `paying_out`, the reconciler with nothing to -resolve, and a manual retry that would pay the winner a second time (pool UTXOs are not -tracked in `utxo_events`, so nothing reserved them). - -This was exactly what B-08 fixed for `place_bet`/`request_withdrawal`; the same fix had never -been applied to the path that moves the most money. - -**Fixed:** `_trigger_payout` (`rounds/scheduler.py`) now has four phases instead of three — -read, *build* (network read only, no write), *persist the intent as `PendingTransaction(kind= -"payout", status="building")` and commit*, then broadcast and promote to `"pending"`. A -broadcast rejection now leaves that `"building"` row behind for the existing reconciler -(`tx/reconcile.py`) to resolve — its generic `building`/`pending` handling already covered a -`payout` kind correctly (including clearing `payout_txid` on abandonment), so no changes were -needed there. - -Two guards were added alongside the two-phase write, since pool UTXOs are invisible to -`utxo_events` and so can never be released/reserved the way a user's own UTXOs are: -`_trigger_payout` now refuses to build a second payout for a round that already has a -non-terminal `PendingTransaction(kind="payout")`, and the payout builder excludes any UTXO -already referenced by *any* non-terminal payout transaction (`_reserved_payout_outpoints`) — -not just this round's — so a stale payout from an earlier round that the reconciler hasn't -abandoned yet can't be double-spent by a fresh attempt. `should_bump`/reconciler retry timing -around a fee-bumped payout is unaffected by this fix (see B-27, still open). - -This made a payout retry *safe*; B-26 (below) is what makes one *automatic*. Regression -tests: `tests/unit/test_scheduler.py` -(`test_trigger_payout_persists_before_broadcasting`, -`test_trigger_payout_broadcast_failure_leaves_a_recoverable_row`, -`test_trigger_payout_skips_when_already_in_flight`, -`test_reserved_payout_outpoints_excludes_utxos_claimed_by_a_stale_payout`). - -### B-26 — A transient failure at payout time wedges the lottery permanently - -`rounds/scheduler.py`: if `listener.client is None` when `_trigger_payout` starts, it returned -without recording anything. `_trigger_payout` was called exactly once, from `_close_and_draw`, -and `_tick` ignored any round not in `open`/`closing`. The round stayed in `paying_out`, no new -round could open, and — unlike the generic `except Exception` branch — nothing was written to -`audit_log`, so `/admin` showed a stalled state with no explanation. - -The payout runs immediately after a ~2-minute wait on a block, so an Electrum drop in that -window is entirely plausible. Same shape applied to `InsufficientFundsError`, a missing -`fee_address` and a missing winner user — none of them logged anything either. - -CLAUDE.md listed "payout retry" as an accepted gap, but treated it as an operational -inconvenience; in practice it was a single point of failure that stopped the whole platform, -including across a process restart while a round was `paying_out`. - -**Fixed:** two changes, matching the proposed fix exactly. (a) Every early return in -`_trigger_payout` — not connected, no `fee_address`, winner not found, insufficient pool -UTXOs, a build error, a rejected broadcast — now calls `_log_payout_failure` with a `reason` -string in the payload, so `/admin`'s audit log always shows *why* a round is stuck, not just -that it is. (b) `_tick` now handles `status == "paying_out"` by calling the new -`_retry_payout_if_due`, which re-invokes `_trigger_payout` unless the most recent -`payout_failed` audit entry for this round is younger than `_PAYOUT_RETRY_INTERVAL_SECONDS` -(60s) — throttled so a persistently-broken payout (e.g. an operator hasn't set `fee_address` -yet) doesn't retry, and re-log a failure, on every 5-second tick. - -Because B-25 already made `_trigger_payout` idempotent (it no-ops if a non-terminal payout -`PendingTransaction` already exists for the round) and persists before broadcasting, this -retry is safe to fire on a process restart too: a round found `paying_out` at startup — whose -payout may have already broadcast, may never have been attempted, or may have been abandoned -by the reconciler — is retried the same way, closing the `paying_out` half of the "scheduler -doesn't resume mid-flight rounds after a restart" gap in CLAUDE.md (the "drawing"/block-wait -half is unrelated and still open, see B-36). - -Regression tests: `tests/unit/test_scheduler.py` -(`test_trigger_payout_logs_a_failure_when_not_connected`, -`test_trigger_payout_logs_a_failure_when_fee_address_missing`, -`test_tick_retries_a_stuck_paying_out_round_with_no_recent_failure`, -`test_tick_throttles_retry_after_a_recent_payout_failure`, -`test_tick_retries_once_the_throttle_window_has_elapsed`). - -### B-27 — Every RBF bump resets the reconciler's abandon clock, so it never fires - -`tx/broadcast.py` used to set `pending.broadcast_at = now` on each bump, but -`tx/reconcile.py`'s `_is_due` computes the 6-hour abandon deadline **from that same field**. - -With the default `rbf_timeout_seconds = 900`, a transaction that is successfully bumped every -15 minutes but never mined reset the counter long before it could reach 6 hours: it was -**never abandoned**, its UTXOs never returned to the user, and if it was a bet the round stayed -in `closing` indefinitely (`scheduler.py:90-91`). `reconcile.py` exists precisely to prevent -this, and the bumper disarmed it. - -**Fixed:** the field is split, exactly as proposed. `PendingTransaction` gained a -`last_broadcast_at` column (Alembic migration `861e76aaf34c`, backfilled from the existing -`broadcast_at` for every pre-existing row, then made `NOT NULL`). `broadcast_at` is now never -rewritten after creation — it stays the *first* broadcast, which is what `reconcile.py:_is_due` -already read and continues to read unchanged. `bump_fee` (`tx/broadcast.py`) now updates -`last_broadcast_at` instead, and `should_bump` reads `last_broadcast_at` rather than -`broadcast_at` — correctly, since *that* decision (is another bump due?) should reset after -every bump, unlike the reconciler's abandon check, which must not. - -Regression tests: `tests/unit/test_broadcast.py` -(`test_should_bump_measures_from_last_broadcast_not_first`, -`test_bump_fee_leaves_broadcast_at_untouched`) and `tests/unit/test_reconcile.py` -(`test_abandons_a_repeatedly_bumped_tx_despite_a_recent_last_broadcast`, the direct proof that -a tx bumped minutes ago but first broadcast 7 hours ago is still abandoned). +B-25 (the payout had no two-phase write, unlike bets and withdrawals — a crash or rejected +broadcast could leave money on-chain with no DB record, or leave the round wedged with no way +to retry safely), B-26 (a transient failure or a process restart at payout time wedged the +round in `paying_out` forever, with most failure paths logging nothing) and B-27 (every RBF +bump reset the reconciler's own abandon clock, so a repeatedly-bumped-but-never-mined +transaction was never abandoned) are fixed as of 2026-07-27. Together B-25 and B-26 make +`paying_out` fully recoverable — a payout retry is now safe (persisted before broadcast, +guarded against double-spend) and automatic (the scheduler retries a stuck round on its own, +throttled, with every failure audit-logged with a reason, including across a process restart). +See git history (commits `f13f685`, `50a43ae`, `933760e`) for the fix-by-fix breakdown; the +regression suite grew from 139 to 151 tests over the three. A full-codebase audit on 2026-07-26 (commit `d4e0974`) found 24 bugs across every Python module under `app/`, both static frontends, and the Docker/Caddy deployment — 5 critical,