From f13f6850b7449676701183597542acb2bbd72604 Mon Sep 17 00:00:00 2001 From: Davide Grilli Date: Mon, 27 Jul 2026 09:19:53 +0200 Subject: [PATCH] Persist the payout before broadcasting it (B-25) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _trigger_payout used to broadcast the payout transaction and only afterwards write payout_txid and its PendingTransaction. A crash in that window (docker-compose.yml auto-restarts on crash) left a payout on-chain with zero record: the round stuck in paying_out, nothing for the reconciler to resolve, and a manual retry that would have paid the winner a second time. This mirrors B-08, which already fixed the same gap for place_bet/request_withdrawal. _trigger_payout now has four phases: read, build (network read only, no write), persist the intent as a PendingTransaction(kind="payout", status="building") and commit, then broadcast and promote to "pending". A rejected broadcast now leaves that "building" row for tx/reconcile.py to resolve — its existing building/pending handling already covers a payout kind correctly, including clearing payout_txid on abandonment, so reconcile.py needed no changes. Since pool UTXOs aren't tracked in utxo_events and so can never be reserved/released the way a user's own UTXOs are, two guards go along with the two-phase write: _trigger_payout now refuses to build a second payout for a round that already has a non-terminal PendingTransaction, and the payout builder excludes any UTXO already referenced by any non-terminal payout transaction (_reserved_payout_outpoints) so a stale payout from an earlier round the reconciler hasn't abandoned yet can't be double-spent by a fresh attempt. This makes a payout retry safe; making one happen automatically is B-26, still open. BUGS.md moves B-25 to "Previously fixed" with the fix description; the suite grows from 139 to 143 tests. Co-Authored-By: Claude Sonnet 5 --- BUGS.md | 82 +++++++++------ app/rounds/scheduler.py | 118 +++++++++++++++++---- tests/unit/test_scheduler.py | 199 ++++++++++++++++++++++++++++++++++- 3 files changed, 342 insertions(+), 57 deletions(-) diff --git a/BUGS.md b/BUGS.md index 765073d..119ebbf 100644 --- a/BUGS.md +++ b/BUGS.md @@ -1,20 +1,21 @@ # 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. **All of them are open.** The 139-test suite -is green, so none of these are caught by existing coverage — every fix should land with a -regression test. +7 medium, 8 low), listed below as B-25 … B-49. B-25 is fixed as of 2026-07-27; the other 24 +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's four tests brought the +suite to 143). -The recurring pattern across B-25, B-26, B-27, 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. Bets and withdrawals write -their intent before broadcasting; the payout does not. Broadcast failures are audit-logged; -*pre*-broadcast failures (no client, insufficient pool funds) are not. +The recurring pattern across B-26, B-27, 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. Broadcast failures are +audit-logged; *pre*-broadcast failures (no client, insufficient pool funds) are not. -**Single highest priority: make `paying_out` a recoverable, idempotent state** (record before -broadcast + startup resume + retry). That closes B-25, B-26 and half of the already-known -"scheduler doesn't resume" gap — i.e. every way the lottery currently stops and cannot -restart on its own. +**Highest remaining priority: make `paying_out` fully recoverable, not just idempotent.** +B-25 made a payout retry *safe* (persisted before broadcast, guarded against double-spend); +B-26 is what would make a retry actually *happen* automatically. Together with the +already-known "scheduler doesn't resume" gap, that's every way the lottery currently stops +and cannot restart on its own. 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 @@ -24,27 +25,6 @@ single-process assumptions, no user-facing history, etc.), see "Known gaps / TOD ## Critical -### B-25 — The payout has no two-phase write, unlike bets and withdrawals - -`rounds/scheduler.py:214` broadcasts, and only afterwards (`:229-251`) writes `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 — leaves a payout on-chain -with **no record at all**: the round is stuck in `paying_out`, the reconciler has nothing to -resolve, and a manual retry would pay the winner a second time (pool UTXOs are not tracked in -`utxo_events`, so nothing reserves them). - -This is exactly what B-08 fixed for `place_bet`/`request_withdrawal`; the same fix was never -applied to the path that moves the most money. - -**Proposed fix.** Mirror the bet path: write `Round.payout_txid` plus a -`PendingTransaction(kind="payout", status="building")` and commit *before* -`client.broadcast()`, then promote to `"pending"` after. Teach -`reconcile.py:_promote`/`_abandon` to handle a `building` payout (promote if the tx is on -chain, otherwise clear `payout_txid` and leave the round for the retry routine). Since pool -UTXOs are invisible to `utxo_events`, `_abandon` cannot release them — so the payout builder -must additionally refuse to spend an outpoint already referenced by a non-terminal payout -`PendingTransaction`, which is what makes a retry safe against double-paying. - ### B-26 — A transient failure at payout time wedges the lottery permanently `rounds/scheduler.py:166-169`: if `listener.client is None` when `_trigger_payout` starts, it @@ -383,6 +363,42 @@ 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 makes a payout retry *safe*; it does not yet make one *automatic* — that is B-26, still +open. 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`). + 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, 7 high, 7 medium, 5 low. All 24 were fixed and verified against the current code on diff --git a/app/rounds/scheduler.py b/app/rounds/scheduler.py index 614eec7..0a3dcf0 100644 --- a/app/rounds/scheduler.py +++ b/app/rounds/scheduler.py @@ -3,8 +3,9 @@ import logging from datetime import datetime, timedelta, timezone from embit import script +from embit.transaction import Transaction from sqlalchemy import func, select -from sqlalchemy.ext.asyncio import async_sessionmaker +from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker from app.audit.log import write_audit_log from app.db.models import PendingTransaction, Round, RoundParticipant, User @@ -161,8 +162,18 @@ class RoundScheduler: return self._listener.tip_height, header_hex_to_block_hash(self._listener.tip_header_hex) async def _trigger_payout(self, round_id: int) -> None: - """Three phases, so no DB session is held across a network call (B-18): read - what's needed, do the chain work, then persist the outcome.""" + """Four phases, so no DB session is held across a network call (B-18): read + what's needed, build the tx, persist the intent, then broadcast. + + The persist happens *before* the broadcast (B-25) — the same two-phase shape + as place_bet/request_withdrawal (B-08): a crash between building the payout + and recording it used to leave money on-chain with zero trace in the DB (no + payout_txid, no PendingTransaction), so a manual retry would have paid the + winner a second time. Now the worst case is a "building" PendingTransaction + the reconciler (app/tx/reconcile.py) can resolve either way by asking the + chain whether the tx exists, exactly like it already does for bets and + withdrawals. + """ client = self._listener.client if client is None: logger.error("round %s payout deferred: not connected", round_id) @@ -171,6 +182,25 @@ class RoundScheduler: # --- Phase 1: read (session closed before any network I/O) --------------- async with self._session_factory() as session: round_ = await session.get(Round, round_id) + already_in_flight = await session.scalar( + select(PendingTransaction).where( + PendingTransaction.round_id == round_id, + PendingTransaction.kind == "payout", + PendingTransaction.status.in_(("building", "pending")), + ) + ) + if already_in_flight is not None: + # A payout for this round is already building or broadcast — this + # must not build a second one, or a retry (manual, or a future + # automatic one) would pay the winner twice. Confirmation/ + # reconciliation already owns resolving that row. + logger.info( + "round %s payout already in flight (pending_transaction %s), skipping", + round_id, + already_in_flight.id, + ) + return + reserved_outpoints = await _reserved_payout_outpoints(session) config = await get_round_config(session) fee_address = config.fee_address fee_rate = config.fee_rate_sat_vb @@ -192,13 +222,17 @@ class RoundScheduler: winner_share = pool_amount_sats * 70 // 100 commission_share = pool_amount_sats - winner_share # remainder from rounding goes to fees - # --- Phase 2: build and broadcast ---------------------------------------- + # --- Phase 2: build (network read only, no DB write yet) ----------------- try: pool_key = derive_pool_key() pool_script_obj = script.p2wpkh(pool_key.to_public()) pool_address = pool_script_obj.address(network=PLM_MAINNET) entries = await client.listunspent(address_to_scripthash(pool_address)) - utxos = [Utxo(e["tx_hash"], e["tx_pos"], e["value"]) for e in entries if e["height"] > 0] + utxos = [ + Utxo(e["tx_hash"], e["tx_pos"], e["value"]) + for e in entries + if e["height"] > 0 and (e["tx_hash"], e["tx_pos"]) not in reserved_outpoints + ] built = build_payout_transaction( signing_key=pool_key, @@ -211,36 +245,54 @@ class RoundScheduler: change_address=pool_address, fee_rate_sat_vb=fee_rate, ) - await client.broadcast(built.raw_hex) except InsufficientFundsError: logger.exception("round %s payout failed: insufficient pool UTXOs", round_id) return except Exception: - # Anything else — a malformed fee_address (EmbitError), a rejected - # broadcast, a dead connection. This used to escape all the way to - # run()'s catch-all, which logged it without recording anything, leaving - # no trace of *why* the round was stuck (B-05). The round stays in - # "paying_out" either way: automatic payout retry is still an open gap. - logger.exception("round %s payout failed", round_id) + # Anything else — a malformed fee_address (EmbitError) or similar. This + # used to escape all the way to run()'s catch-all, which logged it + # without recording anything, leaving no trace of *why* the round was + # stuck (B-05). The round stays in "paying_out" either way: automatic + # payout retry is still an open gap. + logger.exception("round %s payout build failed", round_id) await self._log_payout_failure(round_id, winner_user_id) return - # --- Phase 3: persist ----------------------------------------------------- + # --- Phase 3: persist the intent, *then* broadcast (B-25) ----------------- async with self._session_factory() as session: round_ = await session.get(Round, round_id) round_.winner_amount_sats = built.winner_sats round_.fee_amount_sats = built.commission_sats round_.payout_txid = built.txid - session.add( - PendingTransaction( - kind="payout", - round_id=round_id, - current_txid=built.txid, - fee_rate_sat_vb=fee_rate, - raw_tx_hex=built.raw_hex, - status="pending", - ) + pending = PendingTransaction( + kind="payout", + round_id=round_id, + current_txid=built.txid, + fee_rate_sat_vb=fee_rate, + raw_tx_hex=built.raw_hex, + # "building" until the broadcast succeeds, exactly like place_bet's + # two phases — see the reconciler, which gives this a short grace + # period before asking the chain whether it made it out after all. + status="building", ) + session.add(pending) + await session.commit() + pending_id = pending.id + + # --- Phase 4: broadcast, then promote the pending row -------------------- + try: + await client.broadcast(built.raw_hex) + except Exception: + # The row stays "building": the reconciler will ask the chain about it + # and, finding nothing, abandon it and clear payout_txid (B-25) — instead + # of the round being stuck with a payout_txid that never went anywhere. + logger.exception("round %s payout broadcast failed", round_id) + await self._log_payout_failure(round_id, winner_user_id) + return + + async with self._session_factory() as session: + pending = await session.get(PendingTransaction, pending_id) + pending.status = "pending" await write_audit_log( session, "payout_sent", @@ -267,3 +319,25 @@ class RoundScheduler: await session.commit() except Exception: logger.exception("could not record the payout failure of round %s", round_id) + + +async def _reserved_payout_outpoints(session: AsyncSession) -> set[tuple[str, int]]: + """Pool UTXOs already claimed by a payout that hasn't resolved yet — this + round's own in-flight payout (guarded against separately in _trigger_payout) or + a stale one from an earlier round the reconciler hasn't abandoned yet (B-25). + These must be excluded from selection, or a retry would double-spend the same + coins into two payouts before the reconciler gets a chance to release them.""" + rows = ( + await session.scalars( + select(PendingTransaction).where( + PendingTransaction.kind == "payout", + PendingTransaction.status.in_(("building", "pending")), + ) + ) + ).all() + reserved: set[tuple[str, int]] = set() + for row in rows: + tx = Transaction.parse(bytes.fromhex(row.raw_tx_hex)) + for vin in tx.vin: + reserved.add((vin.txid.hex(), vin.vout)) + return reserved diff --git a/tests/unit/test_scheduler.py b/tests/unit/test_scheduler.py index 1aa4a9e..7bcf771 100644 --- a/tests/unit/test_scheduler.py +++ b/tests/unit/test_scheduler.py @@ -4,9 +4,10 @@ import pytest from sqlalchemy import select from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine +from app.config import settings from app.db.base import Base -from app.db.models import Round, RoundConfig -from app.rounds.scheduler import RoundScheduler +from app.db.models import AuditLog, PendingTransaction, Round, RoundConfig, User +from app.rounds.scheduler import RoundScheduler, _reserved_payout_outpoints class FakeListener: @@ -52,3 +53,197 @@ async def test_tick_closes_round_with_no_participants_once_due(session_factory, async with session_factory() as session: round_ = (await session.scalars(select(Round))).one() assert round_.status == "closed" + + +# --- B-25: the payout must be persisted before it is broadcast, like bets/withdrawals --- + +# A real, reusable PLM bech32 address so build_payout_transaction's +# script.Script.from_address(...) succeeds — this is not a value the scheduler +# validates itself (that's the admin panel's job for fee_address), it just needs to +# actually decode. +_WINNER_ADDRESS = "plm1q0xcqpzrky6eff2g52qdye53xkk9jxkvraxkkwd" +_FEE_ADDRESS = "plm1q5x25wd6q463mfhckjraaedgjg0lyu73qfcj43n" + +_POOL_AMOUNT_SATS = 10_000_000_000 # 100 PLM + + +class FakePayoutClient: + def __init__(self, entries, *, fail_broadcast=False): + self._entries = entries + self._fail_broadcast = fail_broadcast + self.broadcasted: list[str] = [] + + async def listunspent(self, scripthash): + return self._entries + + async def broadcast(self, raw_tx_hex): + if self._fail_broadcast: + raise RuntimeError("node rejected the transaction") + self.broadcasted.append(raw_tx_hex) + return "network-txid" + + +class FakePayoutListener: + def __init__(self, client): + self.client = client + + +@pytest.fixture +async def payout_session_factory(tmp_path, monkeypatch): + """Same master-key bootstrap as test_broadcast.py's fixture: _trigger_payout + needs a real pool key to sign with.""" + monkeypatch.setattr(settings, "master_key_path", str(tmp_path / "master.xprv.enc")) + monkeypatch.setattr( + settings, + "xprv_encryption_key", + __import__("cryptography.fernet", fromlist=["Fernet"]).Fernet.generate_key().decode(), + ) + from app.wallet import hd + + hd._account_key = None + hd.generate_master_key() + + engine = create_async_engine("sqlite+aiosqlite:///:memory:") + async with engine.begin() as conn: + await conn.run_sync(Base.metadata.create_all) + yield async_sessionmaker(engine, expire_on_commit=False) + await engine.dispose() + hd._account_key = None + + +async def _seed_paying_out_round(session_factory, round_id: int = 1) -> int: + async with session_factory() as session: + winner = User(username="winner", password_hash="x", derivation_index=0, address=_WINNER_ADDRESS) + session.add(winner) + await session.flush() + session.add(RoundConfig(fee_address=_FEE_ADDRESS, fee_rate_sat_vb=1)) + session.add( + Round( + id=round_id, + status="paying_out", + pool_amount_sats=_POOL_AMOUNT_SATS, + winner_user_id=winner.id, + ) + ) + await session.commit() + return winner.id + + +async def test_trigger_payout_persists_before_broadcasting(payout_session_factory): + """The happy path: payout_txid and a PendingTransaction must exist once the + broadcast succeeds, promoted from "building" to "pending" — the two-phase write + that used to be missing entirely (B-25).""" + await _seed_paying_out_round(payout_session_factory) + entries = [{"tx_hash": "33" * 32, "tx_pos": 0, "height": 10, "value": _POOL_AMOUNT_SATS + 100_000}] + client = FakePayoutClient(entries) + scheduler = RoundScheduler(payout_session_factory, FakePayoutListener(client)) + + await scheduler._trigger_payout(1) + + assert client.broadcasted + async with payout_session_factory() as session: + round_ = await session.get(Round, 1) + assert round_.payout_txid is not None + assert round_.winner_amount_sats and round_.fee_amount_sats + + pending = (await session.scalars(select(PendingTransaction))).one() + assert pending.kind == "payout" + assert pending.status == "pending" + assert pending.current_txid == round_.payout_txid + + events = [e.event_type for e in (await session.scalars(select(AuditLog))).all()] + assert "payout_sent" in events + + +async def test_trigger_payout_broadcast_failure_leaves_a_recoverable_row(payout_session_factory): + """Before B-25, a broadcast rejection here left nothing behind — no payout_txid, + no PendingTransaction — because everything was persisted only after the + broadcast. Now the intent is already durable, so the reconciler has something to + resolve instead of the round being stuck with zero trace of what was attempted.""" + await _seed_paying_out_round(payout_session_factory) + entries = [{"tx_hash": "44" * 32, "tx_pos": 0, "height": 10, "value": _POOL_AMOUNT_SATS + 100_000}] + client = FakePayoutClient(entries, fail_broadcast=True) + scheduler = RoundScheduler(payout_session_factory, FakePayoutListener(client)) + + await scheduler._trigger_payout(1) + + assert not client.broadcasted + async with payout_session_factory() as session: + round_ = await session.get(Round, 1) + assert round_.payout_txid is not None # durable, even though the broadcast failed + + pending = (await session.scalars(select(PendingTransaction))).one() + assert pending.kind == "payout" + assert pending.status == "building" # not lost — the reconciler resolves this + assert pending.current_txid == round_.payout_txid + + events = [e.event_type for e in (await session.scalars(select(AuditLog))).all()] + assert "payout_failed" in events + + +async def test_trigger_payout_skips_when_already_in_flight(payout_session_factory): + """A second call for a round that already has a non-terminal payout + PendingTransaction must not build (and broadcast) another one — that would pay + the winner twice.""" + winner_id = await _seed_paying_out_round(payout_session_factory) + async with payout_session_factory() as session: + round_ = await session.get(Round, 1) + round_.payout_txid = "already-sent-txid" + session.add( + PendingTransaction( + kind="payout", + round_id=1, + current_txid="already-sent-txid", + fee_rate_sat_vb=1, + raw_tx_hex="00", + status="pending", + ) + ) + await session.commit() + + entries = [{"tx_hash": "55" * 32, "tx_pos": 0, "height": 10, "value": _POOL_AMOUNT_SATS + 100_000}] + client = FakePayoutClient(entries) + scheduler = RoundScheduler(payout_session_factory, FakePayoutListener(client)) + + await scheduler._trigger_payout(1) + + assert not client.broadcasted + async with payout_session_factory() as session: + assert (await session.scalars(select(PendingTransaction))).all() # still just the one seeded + rows = (await session.scalars(select(PendingTransaction))).all() + assert len(rows) == 1 + assert rows[0].current_txid == "already-sent-txid" + + +async def test_reserved_payout_outpoints_excludes_utxos_claimed_by_a_stale_payout(payout_session_factory): + """A payout still "building"/"pending" for some round — most plausibly a stale + one the reconciler hasn't abandoned yet — must keep its inputs off the table for + a fresh payout attempt, or the same pool coins could be spent twice.""" + from embit import script + from embit.transaction import Transaction, TransactionInput, TransactionOutput + + reserved_txid = "66" * 32 + raw_tx = ( + Transaction( + vin=[TransactionInput(bytes.fromhex(reserved_txid), 2)], + vout=[TransactionOutput(1_000_000, script.Script.from_address(_WINNER_ADDRESS))], + ) + .serialize() + .hex() + ) + async with payout_session_factory() as session: + session.add( + PendingTransaction( + kind="payout", + round_id=99, + current_txid="stale-payout-txid", + fee_rate_sat_vb=1, + raw_tx_hex=raw_tx, + status="building", + ) + ) + await session.commit() + + reserved = await _reserved_payout_outpoints(session) + + assert reserved == {(reserved_txid, 2)}