Publish an SSE update from the bet/withdrawal rollback paths (B-49)
_release_failed_bet and _release_failed_withdrawal restored the balance, freed the reserved UTXOs and (for a bet) removed the participant without calling broadcaster.publish(), so every dashboard kept showing the phantom bet and the reduced balance until its next poll — while the success path and the reconciler's own abandon path both published. The two regression tests pre-open the round before subscribing: place_bet opens one itself, and that publish() would otherwise satisfy the assertion whether or not the rollback published anything.
This commit is contained in:
@@ -1,30 +1,19 @@
|
||||
# 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-49. B-25 through B-48 are fixed (see "Previously
|
||||
fixed" below) — no Critical-, High- or Medium-severity finding remains open; the remaining 1 is
|
||||
Low/hygiene. 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 twenty-four fixes so far
|
||||
brought the suite from 139 to 251).
|
||||
7 medium, 8 low). B-25 through B-49 are now **all fixed** (see "Previously fixed" below) —
|
||||
nothing from either audit remains open. The 139-test suite was green at the time of the audit,
|
||||
so none of these were caught by existing coverage — every fix landed with a regression test,
|
||||
which brought the suite from 139 to 253.
|
||||
|
||||
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.
|
||||
The payout phase is now fully recoverable; the "drawing" phase (waiting on a block) is now
|
||||
observable (B-36) but still has no equivalent resume-after-restart — see "Known gaps / TODO"
|
||||
in [CLAUDE.md](CLAUDE.md), which is also where other by-design limitations (single-shared-token
|
||||
admin auth, single-process assumptions, no user-facing history, etc.) are documented.
|
||||
|
||||
---
|
||||
|
||||
## Low / hygiene
|
||||
|
||||
### B-49 — Rollback paths do not publish an SSE update
|
||||
|
||||
`bets/service.py:_release_failed_bet` and `withdrawals/service.py:_release_failed_withdrawal`
|
||||
restore the balance without calling `broadcaster.publish()`, so dashboards only find out on
|
||||
their next poll.
|
||||
**Fix:** one `broadcaster.publish()` at the end of each, as every other state-changing path
|
||||
already does.
|
||||
The recurring pattern behind those findings is worth keeping in mind for the next one: the code
|
||||
is rigorous about the failure modes that have actually been hit, and silent about the ones that
|
||||
have not. The payout phase is now fully recoverable; the "drawing" phase (waiting on a block) is
|
||||
now observable (B-36) but still has no equivalent resume-after-restart — see "Known gaps / TODO"
|
||||
in [CLAUDE.md](CLAUDE.md), which is also where the by-design limitations (single-shared-token
|
||||
admin auth, single-process assumptions, no user-facing history, etc.) are documented. This file
|
||||
stays as the record of what was found and fixed; a new finding gets the next B-nn and its own
|
||||
regression test.
|
||||
|
||||
---
|
||||
|
||||
@@ -42,6 +31,7 @@ already does.
|
||||
- **B-44** — README's Quick start documented a bare `uvicorn --reload` workflow, and `docs/running-the-server.md` still had a matching "Locale / venv" section, both contradicting CLAUDE.md's Docker-only policy
|
||||
- **B-45** — `/admin/rounds`/`/admin/audit-log`'s `limit` had no bounds (`-1` means "everything" on SQLite), and `/admin/pending-transactions` had no limit or status filter at all
|
||||
- **B-46** — `secrets.compare_digest` on a `str` raises `TypeError` on non-ASCII input, turning an invalid admin token with non-ASCII characters into a 500 instead of a 403
|
||||
- **B-49** — `_release_failed_bet`/`_release_failed_withdrawal` restored the balance without publishing an SSE update, so a rolled-back bet or withdrawal stayed on the dashboards until their next poll
|
||||
- **B-48** — `select_utxos` had no cap on input count, so a fragmented address built an ever-larger transaction whose fee (deducted from the amount being moved) ate into the bet or withdrawal, up to the point of being non-standard
|
||||
- **B-47** — `raw_tx_hex` and `payload_json` were unbounded `String` columns (`VARCHAR` with no length) — fine on SQLite/PostgreSQL, rejected by backends like MySQL that require a length
|
||||
- **B-32** — an RBF bump could retry forever below BIP125's relay-mandated minimum fee delta, with no ceiling on the fee rate either
|
||||
|
||||
@@ -8,11 +8,11 @@ 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 — 251 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 — 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.
|
||||
|
||||
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.
|
||||
|
||||
**Read [BUGS.md](BUGS.md) before trusting any behaviour here.** Two audits: 2026-07-26 found 24 bugs (5 critical), all fixed; 2026-07-27 found 25 more (B-25 … B-49), of which **1 is still open** — no Critical, High or Medium remains, only Low/hygiene: rollback paths not publishing an SSE update (B-49). BUGS.md is the live open list with a proposed fix per finding; "Known gaps" at the end of this file is for limitations accepted **by design** instead. Don't fix a BUGS.md item silently as a side effect of other work — each fix lands with its own regression test.
|
||||
**Read [BUGS.md](BUGS.md) before trusting any behaviour here.** Two audits: 2026-07-26 found 24 bugs (5 critical), all fixed; 2026-07-27 found 25 more (B-25 … B-49), all fixed as of 2026-07-27 — **nothing is open**. BUGS.md is the record of what each one was; "Known gaps" at the end of this file is for limitations accepted **by design** instead. Don't fix a BUGS.md item silently as a side effect of other work — each fix lands with its own regression test.
|
||||
|
||||
Before writing code, read the "Architecture" section below in full plus the diagrams in [flowchart/](flowchart/): [platform-overview.mmd](flowchart/platform-overview.mmd) (the 5-phase flow) and [round-lifecycle.mmd](flowchart/round-lifecycle.mmd) (the round/draw lifecycle). Every node **and edge label** (conditions, retries, loops) is a behaviour that must be implemented as described. Regenerate the companion PDFs with `flowchart/render-pdf.sh <file>.mmd` after editing either.
|
||||
|
||||
@@ -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 251 tests
|
||||
python -m pytest # all 253 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
|
||||
```
|
||||
@@ -181,7 +181,7 @@ At 120s blocks that's ~4–6 min worst case (last bet confirms right at the dead
|
||||
|
||||
`GET /rounds/stream` is **additive to** the polling loops in the two SPAs, not a replacement — a blocked or dropped stream just degrades to the old behaviour. No payload, no auth: it's a "something changed, go refetch" ping, with all personalization (e.g. `user_played`) staying in the authenticated REST endpoints. The generator re-checks `request.is_disconnected()` every 5s and sends a keep-alive comment every 20s, so neither a client that vanished without a clean close nor a proxy idle timeout breaks it silently.
|
||||
|
||||
`rounds/events.py`'s `RoundEventBroadcaster` (singleton `broadcaster`) is in-process pub/sub, one `asyncio.Queue(maxsize=1)` per client so redundant notifications coalesce. `publish()` is called on: a round opening (`rounds/service.py`), every status transition (`scheduler.py`), a bet or withdrawal broadcast, any pending tx confirming (`tx/confirmation.py`), a deposit credited (`deposits/service.py`), and a new tip arriving (`electrum/listener.py` — exactly what the drawing phase waits on). Rollback paths are the known exception (B-49).
|
||||
`rounds/events.py`'s `RoundEventBroadcaster` (singleton `broadcaster`) is in-process pub/sub, one `asyncio.Queue(maxsize=1)` per client so redundant notifications coalesce. `publish()` is called on: a round opening (`rounds/service.py`), every status transition (`scheduler.py`), a bet or withdrawal broadcast, any pending tx confirming (`tx/confirmation.py`), a deposit credited (`deposits/service.py`), and a new tip arriving (`electrum/listener.py` — exactly what the drawing phase waits on). The rollback paths (`_release_failed_bet`, `_release_failed_withdrawal`, the reconciler's abandon) publish too — a rollback moves as much state as the success path, so it must ping the dashboards the same way (B-49).
|
||||
|
||||
Deliberate scope limits, not oversights: **single-process only** (fine for one uvicorn process; a multi-worker deployment needs e.g. Redis pub/sub — don't add it speculatively); **generic broadcast, not per-user** (everyone refetches on every event; acceptable at ~100 concurrent users, and a targeted channel would need auth on the stream plus server-side knowledge of who each event affects); `MAX_SUBSCRIBERS` (500) is defensive only — past it the endpoint returns 503 and `EventSource` falls back to polling, which being global and unauthenticated makes the cap itself a cheap DoS of the realtime feature (B-38).
|
||||
|
||||
@@ -234,7 +234,7 @@ Explicit design choices, not derivable from any single file — respect them:
|
||||
|
||||
## Known gaps / TODO
|
||||
|
||||
Accepted **by design**. For actual bugs see [BUGS.md](BUGS.md) (1 open) — not duplicated here.
|
||||
Accepted **by design**. For actual bugs see [BUGS.md](BUGS.md) (none open) — not duplicated here.
|
||||
|
||||
- **`drawing` doesn't resume after a restart.** `_tick()` handles `open`, `closing` and `paying_out` (the last via `_retry_payout_if_due`); nothing re-enters `_wait_for_next_block` after a crash. That wait is unbounded by design (the draw's entropy genuinely depends on a future block) but no longer silent — past `_DRAW_STALL_THRESHOLD_SECONDS` it logs progress and writes a `draw_stalled` audit entry, and `GET /rounds/current`'s `draw_waiting_since` surfaces it live (B-36). Restart-resumption itself remains the last prerequisite for running unattended.
|
||||
- **RBF handles one shape only**: a single change output, back to the tx's own sender, big enough to absorb the increase. No extra-input fallback — an exact-amount tx or too-small change raises `RbfError`. Not permanent, though: an unbumpable tx that never confirms is eventually abandoned and its UTXOs released.
|
||||
|
||||
@@ -144,6 +144,11 @@ async def _release_failed_bet(
|
||||
user_id=user_id,
|
||||
)
|
||||
await session.commit()
|
||||
# The rollback moved as much state as the successful path did — the balance is
|
||||
# back, the participant is gone, so participant_count and jackpot shrank again.
|
||||
# Without this the dashboards kept showing the phantom bet until their next poll
|
||||
# (B-49); the reconciler's own abandon path has always published here.
|
||||
broadcaster.publish()
|
||||
|
||||
|
||||
def _pending_transaction(
|
||||
|
||||
@@ -173,3 +173,4 @@ async def _release_failed_withdrawal(
|
||||
user_id=user_id,
|
||||
)
|
||||
await session.commit()
|
||||
broadcaster.publish() # the reserved UTXOs are spendable again — refetch the balance (B-49)
|
||||
|
||||
@@ -8,6 +8,7 @@ from app.bets.service import BetError, place_bet
|
||||
from app.config import settings
|
||||
from app.db.base import Base
|
||||
from app.db.models import AuditLog, PendingTransaction, Round, RoundConfig, RoundParticipant, User, UtxoEvent
|
||||
from app.rounds.events import broadcaster
|
||||
from app.rounds.service import open_new_round_if_needed
|
||||
from app.wallet.hd import derive_user_address
|
||||
from app.wallet.psbt_builder import MAX_TX_INPUTS
|
||||
@@ -196,6 +197,33 @@ async def test_failed_broadcast_leaves_nothing_behind(session_factory):
|
||||
assert "bet_placed" not in events
|
||||
|
||||
|
||||
async def test_failed_broadcast_publishes_an_sse_update(session_factory): # B-49
|
||||
"""The rollback moves as much state as the successful path does, so it must ping
|
||||
the dashboards the same way — otherwise the phantom bet stays on screen until the
|
||||
next poll."""
|
||||
user_id = await _make_funded_user(session_factory, 21, 3_000_000_000)
|
||||
async with session_factory() as session:
|
||||
# Open the round up front: place_bet would otherwise open it itself, and that
|
||||
# publish() would satisfy the assertion below whether or not the rollback ever
|
||||
# published one of its own.
|
||||
await open_new_round_if_needed(session)
|
||||
await session.commit()
|
||||
|
||||
queue = broadcaster.subscribe()
|
||||
try:
|
||||
while not queue.empty():
|
||||
queue.get_nowait()
|
||||
|
||||
async with session_factory() as session:
|
||||
user = await session.get(User, user_id)
|
||||
with pytest.raises(BetError, match="refused"):
|
||||
await place_bet(session, RejectingElectrumClient(), user)
|
||||
|
||||
assert not queue.empty()
|
||||
finally:
|
||||
broadcaster.unsubscribe(queue)
|
||||
|
||||
|
||||
async def test_failed_broadcast_reports_the_broadcast_failed_code(session_factory):
|
||||
user_id = await _make_funded_user(session_factory, 5, 3_000_000_000)
|
||||
|
||||
|
||||
@@ -6,6 +6,7 @@ from app.bets.service import place_bet
|
||||
from app.config import settings
|
||||
from app.db.base import Base
|
||||
from app.db.models import PendingTransaction, User, UtxoEvent, Withdrawal
|
||||
from app.rounds.events import broadcaster
|
||||
from app.wallet.hd import derive_user_address
|
||||
from app.withdrawals.service import WithdrawalError, request_withdrawal
|
||||
|
||||
@@ -171,6 +172,31 @@ async def test_withdrawal_to_own_address_is_rejected(session_factory):
|
||||
assert (await session.scalars(select(UtxoEvent).where(UtxoEvent.user_id == user_id))).one().spent_txid is None
|
||||
|
||||
|
||||
async def test_failed_broadcast_publishes_an_sse_update(session_factory): # B-49
|
||||
"""The released UTXOs are spendable again and the balance changed back, so the
|
||||
rollback must nudge the dashboard to refetch instead of leaving it stale until
|
||||
its next poll."""
|
||||
user_id = await _make_funded_user(session_factory, 10, 3_000_000_000)
|
||||
|
||||
class RejectingClient:
|
||||
async def broadcast(self, raw_tx_hex: str) -> str:
|
||||
raise RuntimeError("min relay fee not met")
|
||||
|
||||
queue = broadcaster.subscribe()
|
||||
try:
|
||||
while not queue.empty():
|
||||
queue.get_nowait()
|
||||
|
||||
async with session_factory() as session:
|
||||
user = await session.get(User, user_id)
|
||||
with pytest.raises(WithdrawalError, match="refused"):
|
||||
await request_withdrawal(session, RejectingClient(), user, derive_user_address(98), 1_000_000_000)
|
||||
|
||||
assert not queue.empty()
|
||||
finally:
|
||||
broadcaster.unsubscribe(queue)
|
||||
|
||||
|
||||
async def test_failed_broadcast_marks_the_withdrawal_failed_and_frees_the_coins(session_factory):
|
||||
"""B-07/B-08: the Withdrawal row is kept (unlike a bet) so the user can see the
|
||||
instruction didn't go through, but the coins must come back."""
|
||||
|
||||
Reference in New Issue
Block a user