From 6246b13247b5b6aa8de783369d428f6b510178cf Mon Sep 17 00:00:00 2001 From: Davide Grilli Date: Mon, 3 Aug 2026 22:40:15 +0200 Subject: [PATCH] Corroborate deposit credits, not just external spends (B-59) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A candidate external spend has needed a quorum since B-29, but `value` and `height` for a *credit* came from the single active connection and went straight into utxo_events. One hostile or broken server could therefore inflate a user's displayed balance with outpoints that don't exist. It never spends anyone else's coins — a bet or withdrawal built on a phantom UTXO is refused at broadcast and rolled back — but it wedges the balance display and burns build attempts, and on a custodial platform a balance that isn't real is a support incident either way. Balances move in both directions; both directions now need the same quorum. corroborate_utxo_credit asks the other configured servers whether they report the same outpoint, for the same amount, confirmed. The height itself isn't compared: a server still catching up reports height 0 and simply doesn't agree, which is the same answer, while two honest servers can't disagree on the height of a genuinely confirmed outpoint. refresh_user gains the phase that shape already implied: find_new_credit_ candidates (new, confirmed, not already held) inside the first session, corroboration outside any session, then credit_confirmed_utxos over what survived. Only new outpoints are corroborated — re-checking what we already hold would open a connection to every other server on every refresh for an answer that can no longer change anything. A failed corroboration delays a credit, it never loses one: the next scripthash notification or DepositReconciler sweep (300s) re-offers the same outpoint, and the withholding is logged. Co-Authored-By: Claude Opus 5 --- BUGS.md | 15 ---- CLAUDE.md | 8 +-- app/deposits/service.py | 26 +++++++ app/electrum/listener.py | 73 ++++++++++++++++++- tests/unit/test_deposits.py | 23 ++++++ tests/unit/test_electrum_listener.py | 102 ++++++++++++++++++++++++++- 6 files changed, 223 insertions(+), 24 deletions(-) diff --git a/BUGS.md b/BUGS.md index 597c9bd..02453c2 100644 --- a/BUGS.md +++ b/BUGS.md @@ -40,21 +40,6 @@ remains the last prerequisite for running unattended. ## High — security -### B-59 — deposit crediting is not corroborated, unlike external-spend detection - -`app/deposits/service.py:31-45`, `app/electrum/listener.py:401-433`. - -A candidate external spend is corroborated across the other configured servers -before it can reduce a balance (B-29), but `value` and `height` for a *credit* -are taken from the single active connection with no cross-check. A hostile or -broken server can inflate a user's displayed balance with outpoints that do not -exist. The blast radius is bounded — a bet or withdrawal built on a phantom UTXO -is refused at broadcast and the rollback releases it — but it wedges the user's -balance display and burns build attempts. - -Fix: either corroborate credits the same way (symmetry with B-29), or record the -asymmetry explicitly as an accepted risk in CLAUDE.md with its bound stated. - ### B-60 — `POST /admin/bug-reports/{id}/status` writes no audit entry `app/api/routes/admin.py:404-419`. diff --git a/CLAUDE.md b/CLAUDE.md index 8d0f0f9..08c0906 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 — 296 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 — 302 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 296 tests +python -m pytest # all 302 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 ``` @@ -140,7 +140,7 @@ One connection serves everything — deposit credits, broadcasts, confirmations, - **Bounded requests** (`_REQUEST_TIMEOUT_SECONDS` = 15s); a timeout tears the connection down. Unbounded waits used to hang `POST /bets` *while holding the per-user lock*, and could stall the confirmation poller permanently. - **The drop is observable**: `client.wait_closed()` resolves when the read loop dies, and `_run_once` races it against the notification consumers and a 60s `server.ping`. Without it the listener sat on queues nobody would ever fill while `listener.client` still looked alive. - **Headers are validated, not trusted** (`_apply_header`): the tip never regresses, a header must meet the difficulty target it claims, and a single-block advance must chain from the current tip's hash. Failure raises `HeaderValidationError`, which ends the session like a dropped connection and rotates away — that header is the draw's only entropy, so a fabricated one picks the winner. -- **A quorum corroborates the two money-moving decisions** (`_corroborate_majority`, 10s per server, asking only the *other* endpoints — never the active one, which is what a MITM controls): `corroborate_header` before a block seeds the draw (B-28), `corroborate_utxo_spent` before a UTXO missing from one `listunspent` is written off as externally spent (B-29). No fallbacks configured → returns True (the accepted risk of an empty `ELECTRUM_FALLBACK_SERVERS`); nobody answers → returns **False**, since an unreachable network proves nothing. +- **A quorum corroborates every money-moving decision** (`_corroborate_majority`, 10s per server, asking only the *other* endpoints — never the active one, which is what a MITM controls): `corroborate_header` before a block seeds the draw (B-28), `corroborate_utxo_spent` before a UTXO missing from one `listunspent` is written off as externally spent (B-29), and `corroborate_utxo_credit` before a new outpoint credits a balance — same outpoint, same amount, confirmed (B-59). Balances move in both directions, so both directions need the same quorum. No fallbacks configured → returns True (the accepted risk of an empty `ELECTRUM_FALLBACK_SERVERS`); nobody answers → returns **False**, since an unreachable network proves nothing. A failed credit corroboration only *delays*: `find_new_credit_candidates` re-offers the outpoint on the next refresh or `DepositReconciler` sweep. On reconnect `_subscribe_all_users` runs as its own task with bounded concurrency (`_RESUBSCRIBE_CONCURRENCY` = 20) rather than inline and serially — otherwise a large user base froze `tip_height`, and with it an in-flight draw, for the whole sweep (B-31); one user's failure is logged and skipped. `address_for_new_user` (called right after registration) is best-effort by design: on failure that address stays unsubscribed until the next reconnect or `DepositReconciler` sweep. @@ -150,7 +150,7 @@ Diagrams: [platform-overview.mmd](flowchart/platform-overview.mmd), [round-lifec **REG** — on signup the server derives a P2WPKH address via BIP84 (`m/84'/coin'/0'/0/index`, one index per user) from the encrypted master xprv. Permanent, and doubles as deposit address, winnings address and withdrawal change address. -**DEP** — the listener subscribes to the user's scripthash; balance is credited after **1 confirmation**, with the 1-conf reorg risk knowingly accepted and no rollback logic. `deposits/service.py` also detects UTXOs that vanished (spent outside the platform — corroborated per B-29 first) and *reinstates* ones that reappear. +**DEP** — the listener subscribes to the user's scripthash; balance is credited after **1 confirmation** and only once the other servers corroborate the outpoint and its amount (B-59), with the 1-conf reorg risk knowingly accepted and no rollback logic. `deposits/service.py` also detects UTXOs that vanished (spent outside the platform — corroborated per B-29 first) and *reinstates* ones that reappear. **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. diff --git a/app/deposits/service.py b/app/deposits/service.py index ef9a037..3dde2dd 100644 --- a/app/deposits/service.py +++ b/app/deposits/service.py @@ -11,6 +11,32 @@ from app.wallet.balance import recompute_balance logger = logging.getLogger(__name__) +async def find_new_credit_candidates(session: AsyncSession, user_id: int, entries: list[dict]) -> list[dict]: + """The subset of `entries` that would actually credit something: confirmed + (height > 0, per the Electrum convention where <= 0 means mempool) and not + already recorded. + + Split out from credit_confirmed_utxos so the caller + (electrum/listener.py:refresh_user) can corroborate each *new* outpoint + against the other configured servers before any of it is written (B-59) — + the mirror image of what B-29 already required before a balance may go + *down*. Only new ones: corroborating outpoints already credited would open a + connection to every other server on every refresh, for an answer that can no + longer change what we hold. + """ + existing_keys = { + (txid, vout) + for txid, vout in ( + await session.execute(select(UtxoEvent.txid, UtxoEvent.vout).where(UtxoEvent.user_id == user_id)) + ).all() + } + return [ + entry + for entry in entries + if entry["height"] > 0 and (entry["tx_hash"], entry["tx_pos"]) not in existing_keys + ] + + async def credit_confirmed_utxos(session: AsyncSession, user_id: int, entries: list[dict]) -> int: """Insert utxo_events for newly-confirmed entries from an Electrum `listunspent` response (idempotent on txid+vout), refresh the user's cached diff --git a/app/electrum/listener.py b/app/electrum/listener.py index dab78bc..189a1ab 100644 --- a/app/electrum/listener.py +++ b/app/electrum/listener.py @@ -8,6 +8,7 @@ from sqlalchemy.ext.asyncio import async_sessionmaker from app.db.models import User from app.deposits.service import ( credit_confirmed_utxos, + find_new_credit_candidates, find_utxos_missing_from, mark_utxos_spent_externally, reinstate_reappeared_utxos, @@ -381,6 +382,51 @@ class ElectrumListener: return await self._corroborate_majority(_ask, lambda agrees: agrees, f"outpoint {txid}:{vout}") + async def corroborate_utxo_credit(self, scripthash: str, txid: str, vout: int, value: int) -> bool: + """B-59: the mirror of corroborate_utxo_spent, for money going the other + way. A candidate external spend could not reduce a balance without a + quorum, but `value` and `height` for a *credit* were taken from the single + active connection and written straight to the DB — so one hostile or broken + server could inflate a user's displayed balance with outpoints that don't + exist. That never spends anyone else's coins (a bet or withdrawal built on + a phantom UTXO is refused at broadcast and rolled back), but it wedges the + balance display and burns build attempts, and on a custodial platform a + balance that isn't real is a support incident either way. + + Agreement means: this server also reports the outpoint as unspent, for the + same amount, and considers it confirmed. The height itself is not compared — + a server still catching up reports the entry at height 0 and simply doesn't + agree, which is the same answer, while for a genuinely confirmed outpoint + two honest servers cannot disagree on the height anyway. + + A failure here delays a credit, it never loses one: the next scripthash + notification or DepositReconciler sweep (300s) retries it, and a deposit is + only credited once the quorum agrees. + """ + + async def _ask(endpoint: ElectrumEndpoint) -> bool | None: + client = self._client_factory(endpoint) + try: + await asyncio.wait_for(client.connect(), timeout=_CORROBORATION_TIMEOUT_SECONDS) + entries = await asyncio.wait_for( + client.listunspent(scripthash), timeout=_CORROBORATION_TIMEOUT_SECONDS + ) + return any( + e.get("tx_hash") == txid + and e.get("tx_pos") == vout + and e.get("value") == value + and (e.get("height") or 0) > 0 + for e in entries + ) + except Exception: + return None + finally: + await client.close() + + return await self._corroborate_majority( + _ask, lambda agrees: agrees, f"credit of {value} sats at {txid}:{vout}" + ) + async def _consume_headers(self, queue: asyncio.Queue) -> None: while True: params = await queue.get() @@ -400,20 +446,41 @@ class ElectrumListener: async def refresh_user(self, user_id: int, scripthash: str) -> None: """Three phases, so no DB session is held across a network call (B-18), - same shape as _trigger_payout: read what's needed, corroborate any - candidate external spends against other servers (B-29), then persist. + same shape as _trigger_payout: read what's needed, corroborate every + balance-moving candidate against other servers — new credits (B-59) as + well as candidate external spends (B-29) — then persist. """ assert self.client is not None entries = await self.client.listunspent(scripthash) async with self._session_factory() as session: - credited = await credit_confirmed_utxos(session, user_id, entries) + credit_candidates = await find_new_credit_candidates(session, user_id, entries) reinstated = await reinstate_reappeared_utxos(session, user_id, entries) candidates = [ (row.id, row.txid, row.vout) for row in await find_utxos_missing_from(session, user_id, entries) ] + corroborated_credits = [ + entry + for entry in credit_candidates + if await self.corroborate_utxo_credit( + scripthash, entry["tx_hash"], entry["tx_pos"], entry["value"] + ) + ] + + credited = 0 + if corroborated_credits: + async with self._session_factory() as session: + credited = await credit_confirmed_utxos(session, user_id, corroborated_credits) + if len(corroborated_credits) < len(credit_candidates): + logger.warning( + "%s new UTXO(s) for user_id=%s not corroborated by the other servers — " + "not credited yet, will retry on the next refresh", + len(credit_candidates) - len(corroborated_credits), + user_id, + ) + confirmed_ids = [ utxo_id for utxo_id, txid, vout in candidates diff --git a/tests/unit/test_deposits.py b/tests/unit/test_deposits.py index d744916..9c43ebe 100644 --- a/tests/unit/test_deposits.py +++ b/tests/unit/test_deposits.py @@ -184,3 +184,26 @@ async def test_reinstate_reappeared_utxos_ignores_unmarked_rows(session_factory, async with session_factory() as session: reinstated = await reinstate_reappeared_utxos(session, user_id, entries) assert reinstated == 0 + + +async def test_find_new_credit_candidates_skips_unconfirmed_and_already_known(session_factory, user_id): # B-59 + """What the caller has to corroborate before crediting: only entries that would + actually write something. Re-corroborating what we already hold would open a + connection to every other server on every refresh, for an answer that can no + longer change anything.""" + from app.deposits.service import find_new_credit_candidates + + async with session_factory() as session: + await credit_confirmed_utxos( + session, user_id, [{"tx_hash": "aa" * 32, "tx_pos": 0, "height": 100, "value": 1_000}] + ) + + entries = [ + {"tx_hash": "aa" * 32, "tx_pos": 0, "height": 100, "value": 1_000}, # already credited + {"tx_hash": "bb" * 32, "tx_pos": 0, "height": 0, "value": 2_000}, # still in the mempool + {"tx_hash": "cc" * 32, "tx_pos": 1, "height": 101, "value": 3_000}, # genuinely new + ] + async with session_factory() as session: + candidates = await find_new_credit_candidates(session, user_id, entries) + + assert [(c["tx_hash"], c["tx_pos"]) for c in candidates] == [("cc" * 32, 1)] diff --git a/tests/unit/test_electrum_listener.py b/tests/unit/test_electrum_listener.py index 2270254..d7823ac 100644 --- a/tests/unit/test_electrum_listener.py +++ b/tests/unit/test_electrum_listener.py @@ -446,8 +446,14 @@ _UNRELATED_ENTRY = [{"tx_hash": "99" * 32, "tx_pos": 0, "height": 100, "value": async def test_refresh_user_marks_a_utxo_spent_once_others_corroborate_it(session_factory): user_id = await _seed_funded_user(session_factory, username="bob", address="plm1q0xcqpzrky6eff2g52qdye53xkk9jxkvraxkkwd") + # The others agree the tracked UTXO is gone, and agree about the unrelated + # entry — which B-59 now requires before that one may be credited. others_factory = await _listunspent_client_factory( - {"first.example": [], "second.example": [], "third.example": []} + { + "first.example": _UNRELATED_ENTRY, + "second.example": _UNRELATED_ENTRY, + "third.example": _UNRELATED_ENTRY, + } ) listener = ElectrumListener(others_factory, session_factory, _ENDPOINTS) listener.client = _ActiveClient(_UNRELATED_ENTRY) # our own connection no longer sees the UTXO either @@ -471,7 +477,7 @@ async def test_refresh_user_does_not_mark_when_corroboration_fails(session_facto bad reply.""" user_id = await _seed_funded_user(session_factory, username="carol", address="plm1qtest2") - still_there = [{"tx_hash": "dd" * 32, "tx_pos": 0}] + still_there = [{"tx_hash": "dd" * 32, "tx_pos": 0}] + _UNRELATED_ENTRY others_factory = await _listunspent_client_factory( {"first.example": [], "second.example": still_there, "third.example": still_there} ) @@ -630,3 +636,95 @@ async def test_run_once_keeps_consuming_headers_while_resubscribing(session_fact finally: await client.close() await run_once_task + + +# --- B-59: a credit must clear the same quorum a debit already had to (B-29) ----- + +_PHANTOM = [{"tx_hash": "77" * 32, "tx_pos": 0, "height": 200, "value": 5_000_000}] + + +async def test_corroborate_utxo_credit_true_when_others_report_the_same_outpoint(session_factory): + factory = await _listunspent_client_factory( + {"first.example": _PHANTOM, "second.example": _PHANTOM, "third.example": _PHANTOM} + ) + listener = ElectrumListener(factory, session_factory, _ENDPOINTS) + + assert await listener.corroborate_utxo_credit("scripthash", "77" * 32, 0, 5_000_000) is True + + +async def test_corroborate_utxo_credit_false_when_the_amount_differs(session_factory): + """Agreement is on the amount too, not just on the outpoint existing — the + inflated number is the whole point of the attack.""" + smaller = [{"tx_hash": "77" * 32, "tx_pos": 0, "height": 200, "value": 1_000}] + factory = await _listunspent_client_factory( + {"first.example": smaller, "second.example": smaller, "third.example": _PHANTOM} + ) + listener = ElectrumListener(factory, session_factory, _ENDPOINTS) + + assert await listener.corroborate_utxo_credit("scripthash", "77" * 32, 0, 5_000_000) is False + + +async def test_corroborate_utxo_credit_false_when_others_call_it_unconfirmed(session_factory): + """height <= 0 is Electrum's "still in the mempool" — a server that hasn't seen + the block yet doesn't corroborate a 1-conf credit.""" + mempool = [{"tx_hash": "77" * 32, "tx_pos": 0, "height": 0, "value": 5_000_000}] + factory = await _listunspent_client_factory( + {"first.example": mempool, "second.example": mempool, "third.example": _PHANTOM} + ) + listener = ElectrumListener(factory, session_factory, _ENDPOINTS) + + assert await listener.corroborate_utxo_credit("scripthash", "77" * 32, 0, 5_000_000) is False + + +async def test_corroborate_utxo_credit_false_when_nobody_responds(session_factory): + factory = await _listunspent_client_factory( + {"first.example": None, "second.example": None, "third.example": ConnectionRefusedError("down")} + ) + listener = ElectrumListener(factory, session_factory, _ENDPOINTS) + + assert await listener.corroborate_utxo_credit("scripthash", "77" * 32, 0, 5_000_000) is False + + +async def test_refresh_user_does_not_credit_a_utxo_only_our_own_server_reports(session_factory): + """The mirror of B-29's headline case: before this, one hostile or broken + server could inflate a user's displayed balance with an outpoint that doesn't + exist. The credit is withheld, not lost — the next refresh retries it.""" + user_id = await _seed_funded_user(session_factory, username="dave", address="plm1qtest3") + + others_factory = await _listunspent_client_factory( + {"first.example": [], "second.example": [], "third.example": []} + ) + listener = ElectrumListener(others_factory, session_factory, _ENDPOINTS) + # Our own connection reports the tracked UTXO plus a phantom one nobody else has. + listener.client = _ActiveClient( + [{"tx_hash": "dd" * 32, "tx_pos": 0, "height": 100, "value": 20_000_000}] + _PHANTOM + ) + + await listener.refresh_user(user_id, "scripthash") + + async with session_factory() as session: + assert ( + await session.scalars(select(UtxoEvent).where(UtxoEvent.txid == "77" * 32)) + ).all() == [] + user = await session.get(User, user_id) + assert user.cached_balance_sats == 20_000_000 # unchanged, not inflated + + +async def test_refresh_user_credits_once_the_others_corroborate(session_factory): + user_id = await _seed_funded_user(session_factory, username="erin", address="plm1qtest4") + + # first.example is the active endpoint, which _corroborate_majority never asks — + # the quorum here is second + third. + others_factory = await _listunspent_client_factory( + {"first.example": [], "second.example": _PHANTOM, "third.example": _PHANTOM} + ) + listener = ElectrumListener(others_factory, session_factory, _ENDPOINTS) + listener.client = _ActiveClient( + [{"tx_hash": "dd" * 32, "tx_pos": 0, "height": 100, "value": 20_000_000}] + _PHANTOM + ) + + await listener.refresh_user(user_id, "scripthash") + + async with session_factory() as session: + user = await session.get(User, user_id) + assert user.cached_balance_sats == 25_000_000