diff --git a/BUGS.md b/BUGS.md index 3dc2a50..535c58c 100644 --- a/BUGS.md +++ b/BUGS.md @@ -4,10 +4,14 @@ Third full-codebase audit, opened after the 2026-07-26 (B-01 … B-24) and 2026-07-27 (B-25 … B-49) lists were emptied. Numbering continues from the last fixed finding, B-51. -Nothing in this list is fixed yet — it is the analysis pass only. Per CLAUDE.md's -convention each entry gets its own commit with its own regression test, and the -`B-nn` marker goes in a comment next to the fix so `git log --all --grep 'B-nn'` -finds it later. +The list opened at B-52 … B-72 and holds only what is still **open**: a finding is +removed from this file once it is fixed. Per CLAUDE.md's convention each entry gets +its own commit with its own regression test, and the `B-nn` marker goes in a comment +next to the fix so `git log --all --grep 'B-nn'` finds it later. + +Already fixed and removed: B-52 (a round past ~50 participants deadlocked the +payout — `025754c`), B-53 (a bet could pay into the pool of a round it was left +out of — `PLACEHOLDER`). State of the tree at audit time: 264 unit tests, all passing; `tests/integration/` still empty; withdrawal and the RBF bump path still never live-broadcast. @@ -28,70 +32,6 @@ Severity is about consequence, not likelihood: ## Critical -### B-52 — a round with more than ~50 participants deadlocks the platform permanently — **FIXED** - -`app/wallet/psbt_builder.py:42` (`MAX_TX_INPUTS = 50`), -`app/rounds/scheduler.py:349-373`. - -Every confirmed bet leaves exactly one UTXO on the pool address, and the payout's -selection target is `winner_share + commission`, i.e. the whole pool — so it needs -*all* n bet UTXOs as inputs. At n ≥ 51 `select_utxos` raises `too_many_inputs`, -`_trigger_payout` records `payout_failed`, and `_retry_payout_if_due` re-attempts -every 60 s forever. The round stays `paying_out`, so `open_new_round_if_needed` -never opens another round: the lottery halts, the pool is unspendable through the -normal path, and the only way out is a manual consolidation with the pool key. - -CLAUDE.md presents B-48's input cap purely as a fragmented *user* address problem. -The pool case is structural rather than an edge case: participant count alone -causes it, with the default bet amount and no unusual deposit pattern. - -**Fixed** by moving the limit from where it was *discovered* to where it can still be -*enforced*: - -- `select_utxos` takes the cap as a parameter. Bets and withdrawals keep - `MAX_TX_INPUTS = 50` (a user-protection limit: the fee comes out of the amount - they are moving); the payout uses the new `MAX_PAYOUT_TX_INPUTS = 500`, since the - pool's UTXO count is just the number of bets and the fee comes out of a 70% share - of the whole pool. 500 inputs is ~34 kvB, about a third of the 100 kvB relay - standardness budget; signing that many costs ~0.4 s of event loop, once per round, - in a background task. -- `place_bet` refuses the bet past `MAX_PARTICIPANTS_PER_ROUND = 400` with a new - `round_full` error (400, translated into all 7 languages), counting every - participant row rather than only the confirmed ones. The cap sits below the input - cap so the payout keeps headroom for pool change accumulated by earlier rounds. - -The invariant is now "a round can always be paid out", enforced before any of the -401st player's money moves. A round already wedged with 51–499 participants pays out -by itself on the next `_retry_payout_if_due` tick. - -Not addressed, and deliberately so: periodic pool consolidation, which is what would -be needed to go beyond this order of magnitude (see the audit discussion — it needs a -new PendingTransaction kind, must not run mid-round, and would force the payout math -to tolerate a pool short of its exact target). Raising these two constants covers -anything up to ~1400 participants first. - -### B-53 — a bet can pay into the pool and still be left out of the draw - -`app/bets/service.py:80-92`, `app/rounds/scheduler.py:101-129`. - -`place_bet` commits its participant row as `building` *before* broadcasting -(the deliberate two-phase write of B-08). The scheduler counts in-flight -participants in one session and then reads the `confirmed` participants in a -second, separate session. A bet that passed `round_accepts_bets` just before the -deadline can commit its phase-1 row *between* those two queries: the count saw -zero, so the round draws and pays out, while the new row — not yet `confirmed` — -is excluded from `participants`. The bet then confirms normally and its sats land -in the pool address, credited to no round and to no participant. There is no -refund path, and the money silently improves the *next* round's payout change. - -The window is one task switch wide, but both queries do real DB I/O, so it is -reachable rather than theoretical. - -Fix directions: re-check the in-flight count inside the same transaction that -snapshots the participants (and abort the close if it is non-zero), or make the -deadline authoritative at the row level so a bet cannot commit against a round -whose timer has expired. - ### (not new) `drawing` does not resume after a restart Already tracked as an accepted gap in CLAUDE.md's "Known gaps", not re-numbered diff --git a/CLAUDE.md b/CLAUDE.md index 3dd0819..6492379 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,7 +8,7 @@ The user communicates in Italian in chat — reply to them in Italian. Everythin ## Project status -All 10 stages of the original build order are code-complete and unit-tested — 253 tests, all under `tests/unit/` (`tests/integration/` is an empty package). Beyond them: Docker + Caddy deployment, admin dashboard (`/admin`), static test UI (`/`), pending-inclusive balance display, an SSE push channel layered over the original polling, self-service password change + admin password reset, and the reconciliation/corroboration machinery below. +All 10 stages of the original build order are code-complete and unit-tested — 275 tests, all under `tests/unit/` (`tests/integration/` is an empty package). Beyond them: Docker + Caddy deployment, admin dashboard (`/admin`), static test UI (`/`), pending-inclusive balance display, an SSE push channel layered over the original polling, self-service password change + admin password reset, and the reconciliation/corroboration machinery below. Verified on mainnet with real money: registration + address derivation, deposit crediting (1-conf), a real 10 PLM bet (broadcast → confirmed → change credited back), and one full round cycle (close → draw on a real block hash → 70/30 payout with sat math checked against the broadcast tx → confirmation → close → next round auto-opened). **Withdrawal and the RBF bump path have never been exercised against a live broadcast** — unit-tested only. @@ -33,7 +33,7 @@ PYTHONPATH=. python scripts/decrypt_master_key.py # ops recovery: decrypt+pr PYTHONPATH=. python scripts/encrypt_master_key.py # ops bootstrap: import an externally-generated xprv (--overwrite to replace) PYTHONPATH=. python scripts/electrum_smoke_test.py # manual check: connect, handshake, subscribe to headers, print the tip -python -m pytest # all 253 tests +python -m pytest # all 275 tests python -m pytest tests/unit/test_hd.py # one file python -m pytest tests/unit/test_hd.py::test_derivation_is_deterministic # one test ``` @@ -155,7 +155,7 @@ Diagrams: [platform-overview.mmd](flowchart/platform-overview.mmd), [round-lifec **PLAY** — fixed cost, **at most one active bet per user**, and **at most `MAX_PARTICIPANTS_PER_ROUND` (400) players per round** — past that the bet is refused with `round_full` and the player waits for the next round (B-52: the payout must spend one pool UTXO per bet, so a round is only ever allowed to grow to what a single payout tx can drain). PSBT user-address → pool-address, always with a **change output back to the same user address** (a user's balance must never exactly equal the bet). Fee ~1 sat/vB, **deducted from the bet amount**. No confirmation within the timeout → RBF bump and rebroadcast. **DRAW** — configurable timer (default 600s): -- *Bet cutoff is the round's own deadline* (`opened_at + round_duration_seconds`), **not** the DB status: `place_bet` calls `rounds/service.round_accepts_bets`, which rejects once the deadline passes even while `status` is still `"open"` (the 5s scheduler tick can lag behind it). Once a round leaves `open`, no new bets either, and no new round opens until this one is fully `closed`. +- *Bet cutoff is the round's own deadline* (`opened_at + round_duration_seconds`), **not** the DB status: `place_bet` calls `rounds/service.round_accepts_bets`, which rejects once the deadline passes even while `status` is still `"open"` (the 5s scheduler tick can lag behind it). Once a round leaves `open`, no new bets either, and no new round opens until this one is fully `closed`. The deadline is checked twice — on arrival and again after the transaction is built — and the participant row is then committed behind a **compare-and-set on the round row** (`UPDATE rounds ... WHERE status = 'open'`, B-53): the scheduler flips `open` → `closing` in a transaction of its own and only counts in-flight bets afterwards, so without the CAS a bet could commit in between, be excluded from the draw (only `confirmed` participants are drawn) and still have its sats land in the pool with no refund path. Its mirror image on the scheduler side is `_close_and_draw` re-counting in-flight bets in the same session it snapshots the participants from. - *"Yellow light":* closing **waits for every already-broadcast bet to confirm** before drawing, so a bet in flight at the boundary isn't lost (`building` counts as in-flight; what bounds the wait is the reconciler eventually abandoning a bet that never confirms). - *Algorithm* (deliberately simple, meant to be replaced): first block confirmed after closing — corroborated by the other servers first, and on failure the draw waits for a *further* block and writes a `draw_header_corroboration_failed` audit entry rather than stalling silently — hash as seed, `index = seed mod participant_count` over participants ordered by **broadcast timestamp** (also the tie-break when two bets land in the same block). Equal probability for everyone, regardless of amount. - *Payout* is signed with the pool key; its **fee comes out of the winner's 70%**, leaving the 30% fee share intact. Same timeout → RBF → rebroadcast pattern. diff --git a/app/bets/service.py b/app/bets/service.py index ee2b664..58a75fd 100644 --- a/app/bets/service.py +++ b/app/bets/service.py @@ -1,12 +1,13 @@ from datetime import datetime, timezone from embit import script -from sqlalchemy import func, select +from sqlalchemy import func, select, update +from sqlalchemy.exc import OperationalError from sqlalchemy.ext.asyncio import AsyncSession from app.api.errors import ApiError from app.audit.log import write_audit_log -from app.db.models import PendingTransaction, RoundParticipant, User, UtxoEvent +from app.db.models import PendingTransaction, Round, RoundParticipant, User, UtxoEvent from app.electrum.client import ElectrumClient from app.rounds.config import get_round_config from app.rounds.events import broadcaster @@ -26,6 +27,11 @@ class BetError(ApiError): pass +class _RoundClosedDuringBuild(Exception): + """Internal signal (B-53): the round stopped accepting bets while this one was + being built. Never leaves place_bet — it becomes a `round_closing` BetError.""" + + async def place_bet(session: AsyncSession, client: ElectrumClient, user: User) -> RoundParticipant: round_ = await open_new_round_if_needed(session) if round_ is None: @@ -115,7 +121,42 @@ async def place_bet(session: AsyncSession, client: ElectrumClient, user: User) - session.add(participant) pending = _pending_transaction(round_.id, user.id, built, config.fee_rate_sat_vb) session.add(pending) - await session.commit() + + # B-53: the deadline check at the top of this function happened before the UTXO + # scan and the signing above, so re-check it here against the clock as it is now — + # a slow build must not sneak a bet past the round's deadline. + # + # And then the part the clock can't cover: a compare-and-set on the round's own + # row, in the *same* transaction as the participant insert. The scheduler flips + # "open" -> "closing" in a transaction of its own and only counts in-flight + # participants afterwards, so without this a bet could commit its "building" row + # in between and be paid into the pool while the round drew and paid out without + # it — money credited to no round, no participant and no refund path. The UPDATE + # takes SQLite's write lock, so the two transactions can no longer interleave: + # either this commits first and the scheduler's subsequent in-flight count sees + # the row, or the flip commits first and this matches zero rows and refuses the + # bet before anything is broadcast. + try: + if not round_accepts_bets(round_, config.round_duration_seconds): + raise _RoundClosedDuringBuild + guard = await session.execute( + update(Round) + .where(Round.id == round_.id, Round.status == "open") + .values(status="open") + .execution_options(synchronize_session=False) + ) + if guard.rowcount != 1: + raise _RoundClosedDuringBuild + await session.commit() + except (_RoundClosedDuringBuild, OperationalError) as exc: + # OperationalError here is SQLite's write-snapshot conflict: the round row + # changed under us, which is the same situation as the guard matching nothing. + # Nothing has been broadcast yet, so the rollback undoes phase 1 entirely — + # the UTXOs stay unspent and no participant row survives. + await session.rollback() + raise BetError( + "round_closing", "the current round is closing, please try again shortly" + ) from exc # --- Phase 2: broadcast, then promote both rows to their live state --------- try: diff --git a/app/rounds/scheduler.py b/app/rounds/scheduler.py index 13306b4..e946cc5 100644 --- a/app/rounds/scheduler.py +++ b/app/rounds/scheduler.py @@ -120,6 +120,27 @@ class RoundScheduler: async with self._session_factory() as session: round_ = await session.get(Round, round_id) + # B-53: re-check for in-flight bets in the *same* session that snapshots + # the participants. _tick's check ran in a session of its own, so a bet + # committing its "building" row in between was counted by neither: the + # round drew and paid out without it, while its sats still landed in the + # pool. place_bet's compare-and-set on the round row is what makes that + # window unreachable; this is the cheap second lock on the same door, and + # it fails safe — the round stays "closing" and the next tick retries. + pending_count = await session.scalar( + select(func.count()) + .select_from(RoundParticipant) + .where( + RoundParticipant.round_id == round_id, + RoundParticipant.status.in_(("building", "broadcast")), + ) + ) + if pending_count: + logger.info( + "round %s: %s bet(s) still in flight at close time, waiting", round_id, pending_count + ) + return + participants = ( await session.scalars( select(RoundParticipant) diff --git a/tests/unit/test_bets.py b/tests/unit/test_bets.py index 24fcf23..79ccdfd 100644 --- a/tests/unit/test_bets.py +++ b/tests/unit/test_bets.py @@ -328,3 +328,103 @@ async def test_bet_is_persisted_before_it_is_broadcast(session_factory): assert seen["pending"] == [("bet", "building")] assert seen["participants"] == ["building"] + + +# --- B-53: a bet must never pay into the pool of a round it was left out of ------ + + +async def _assert_bet_left_no_trace(session_factory, user_id: int, balance_before: int) -> None: + async with session_factory() as session: + utxo = (await session.scalars(select(UtxoEvent).where(UtxoEvent.user_id == user_id))).one() + assert utxo.spent_txid is None # nothing reserved, so the user can bet next round + assert (await session.scalars(select(RoundParticipant))).all() == [] + assert (await session.scalars(select(PendingTransaction))).all() == [] + user = await session.get(User, user_id) + assert user.cached_balance_sats == balance_before # the rollback undid the recompute too + + +async def test_place_bet_refuses_when_the_round_closed_between_the_check_and_the_commit( + session_factory, monkeypatch +): # B-53 + """The scheduler flips "open" -> "closing" in a transaction of its own and only + then counts in-flight bets. A bet whose deadline check passed just before that + flip must not be able to commit its participant row afterwards: it would be + excluded from the draw (only "confirmed" participants are drawn) while its sats + still landed in the pool address — credited to no round, with no refund path. + + round_accepts_bets is forced to pass so the refusal can only come from the + compare-and-set on the round row, which is the part that survives the race the + wall-clock check cannot see.""" + user_id = await _make_funded_user(session_factory, 40, 3_000_000_000) + client = FakeElectrumClient() + + async with session_factory() as session: + round_ = await open_new_round_if_needed(session) + await session.commit() + round_id = round_.id + + monkeypatch.setattr("app.bets.service.round_accepts_bets", lambda *args, **kwargs: True) + + async with session_factory() as session: + # What the scheduler's own tick would have committed a moment earlier. + (await session.get(Round, round_id)).status = "closing" + await session.commit() + + async with session_factory() as session: + user = await session.get(User, user_id) + balance_before = user.cached_balance_sats + with pytest.raises(BetError) as excinfo: + await place_bet(session, client, user) + + assert excinfo.value.code == "round_closing" + assert not client.broadcasted # refused before any money moved + await _assert_bet_left_no_trace(session_factory, user_id, balance_before) + + +async def test_place_bet_rechecks_the_deadline_after_building_the_transaction( + session_factory, monkeypatch +): # B-53 + """The first deadline check happens before the UTXO scan and the signing, so a + slow build could carry a bet past the round's deadline. It is re-checked against + the clock as it is at commit time.""" + user_id = await _make_funded_user(session_factory, 41, 3_000_000_000) + client = FakeElectrumClient() + + async with session_factory() as session: + await open_new_round_if_needed(session) + await session.commit() + + checks: list[bool] = [] + + def _accepts_then_expires(*args, **kwargs) -> bool: + checks.append(True) + return len(checks) == 1 # open when the bet arrived, expired by the time it was built + + monkeypatch.setattr("app.bets.service.round_accepts_bets", _accepts_then_expires) + + async with session_factory() as session: + user = await session.get(User, user_id) + balance_before = user.cached_balance_sats + with pytest.raises(BetError) as excinfo: + await place_bet(session, client, user) + + assert len(checks) == 2 # the re-check really ran + assert excinfo.value.code == "round_closing" + assert not client.broadcasted + await _assert_bet_left_no_trace(session_factory, user_id, balance_before) + + +async def test_place_bet_still_succeeds_while_the_round_is_open(session_factory): # B-53 + """The guard must not refuse the normal path: an open, in-time round still takes + bets, and the round's status is left untouched by the compare-and-set.""" + user_id = await _make_funded_user(session_factory, 42, 3_000_000_000) + client = FakeElectrumClient() + + async with session_factory() as session: + user = await session.get(User, user_id) + participant = await place_bet(session, client, user) + + assert participant.status == "broadcast" + async with session_factory() as session: + round_ = (await session.scalars(select(Round))).one() + assert round_.status == "open" diff --git a/tests/unit/test_scheduler.py b/tests/unit/test_scheduler.py index 8ff6fa6..d92dc00 100644 --- a/tests/unit/test_scheduler.py +++ b/tests/unit/test_scheduler.py @@ -7,7 +7,7 @@ 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 AuditLog, PendingTransaction, Round, RoundConfig, User +from app.db.models import AuditLog, PendingTransaction, Round, RoundConfig, RoundParticipant, User from app.rounds.scheduler import RoundScheduler, _reserved_payout_outpoints from app.wallet.psbt_builder import MAX_TX_INPUTS @@ -493,3 +493,38 @@ async def test_wait_for_next_block_logs_a_stall_audit_entry_past_the_threshold(s entries = (await session.scalars(select(AuditLog).where(AuditLog.event_type == "draw_stalled"))).all() assert len(entries) == 1 assert entries[0].round_id == 1 + + +async def test_close_and_draw_waits_when_a_bet_appears_after_the_tick_check(session_factory): # B-53 + """_tick counts in-flight bets in a session of its own, so a "building" row that + commits between that count and the participant snapshot used to be invisible to + both: the round drew and paid out without the bet, while its sats still landed in + the pool. _close_and_draw re-checks in the same session it snapshots from, and + must leave the round in "closing" for the next tick rather than draw.""" + async with session_factory() as session: + session.add(RoundConfig(fee_address="")) + session.add(Round(status="closing", opened_at=datetime.now(timezone.utc))) + await session.commit() + round_ = (await session.scalars(select(Round))).one() + session.add( + RoundParticipant( + round_id=round_.id, + user_id=1, + bet_amount_sats=1_000_000_000, + bet_txid="ab" * 32, + status="building", # committed a moment after _tick counted zero + ) + ) + await session.commit() + round_id = round_.id + + scheduler = RoundScheduler(session_factory, FakeListener()) + await scheduler._close_and_draw(round_id) + + async with session_factory() as session: + round_ = await session.get(Round, round_id) + assert round_.status == "closing" # not drawn, and not closed as participant-less + assert round_.winner_user_id is None + events = [e.event_type for e in (await session.scalars(select(AuditLog))).all()] + assert "round_closed" not in events + assert "winner_drawn" not in events