diff --git a/BUGS.md b/BUGS.md index a7bef1a..e5a0f91 100644 --- a/BUGS.md +++ b/BUGS.md @@ -40,21 +40,6 @@ remains the last prerequisite for running unattended. ## Medium — correctness and robustness -### B-64 — `_apply_header` accepts a same-height header without the chaining check - -`app/electrum/listener.py:271-296`. - -The chain check only runs for `height == self.tip_height + 1`. A header at exactly -the current tip height replaces `tip_header_hex` after passing only the -self-target check — which, as the docstring of `header_meets_its_own_target` -already notes, a server can satisfy with a self-declared easy target. Separately, -a header with no `hex` field sets `tip_header_hex = None`, discarding a tip we -otherwise accepted. - -Fix: treat a same-height header as either ignorable or as a reorg signal rather -than silently replacing the entropy source, and don't clear `tip_header_hex` on a -hex-less notification. - ### B-65 — `/rounds/current` counts unconfirmed participants; the draw and payout do not `app/api/routes/rounds.py:131-143` vs `app/rounds/scheduler.py:126`. diff --git a/CLAUDE.md b/CLAUDE.md index f49e0a3..e6138ef 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -139,7 +139,7 @@ One connection serves everything — deposit credits, broadcasts, confirmations, - **Rotation.** `ELECTRUM_HOST`/`PORT` is primary, `ELECTRUM_FALLBACK_SERVERS` a comma-separated `host:port[:notls]` list (`client.py:parse_endpoints` rejects malformed entries at startup, not during the outage when the fallback is needed). After any failed or dropped session the next server is tried immediately; the backoff (1s doubling to 30s) only kicks in once every server has had a turn. - **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. +- **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. Two more headers are refused *without* ending the session, since neither implies a hostile server (B-64): one at a height we already hold a header for (a reorg at the tip, or one server disagreeing — the hash committed to for a height is never swapped under us, and `corroborate_header` is what catches us holding an orphan), and one carrying no `hex` at all (nothing to validate or draw from, and applying the height alone would break the `tip_height`/`tip_header_hex` pairing). `_run_once` separately refuses to publish the client while *no* tip is known, so the ignore-don't-kill choice can't reopen B-63. - **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. diff --git a/app/electrum/listener.py b/app/electrum/listener.py index 0611cca..1c9246e 100644 --- a/app/electrum/listener.py +++ b/app/electrum/listener.py @@ -165,6 +165,15 @@ class ElectrumListener: try: header = await client.subscribe_headers() self._apply_header(header) + if self.tip_header_hex is None: + # B-64: the header was unusable (no hex) and we have never had a tip, + # so publishing this client would hand every consumer a connection + # whose chain position is unknown — B-63 all over again. A server at + # or behind a tip we already know is fine and doesn't come through + # here: the point is only that *some* tip is established. + raise HeaderValidationError( + f"{endpoint} announced an unusable initial header ({header!r}) and no tip is known" + ) # B-63: published only now, never before the first header has been # applied. `self.client is not None` is what every consumer treats as # "the chain is reachable" — including RoundScheduler._tick, which then @@ -260,7 +269,7 @@ class ElectrumListener: await self.refresh_user(user_id, scripthash) def _apply_header(self, header: dict) -> None: - """Record a new chain tip, refusing to move backwards. + """Record a new chain tip, refusing to move backwards or sideways. Full reorg handling is out of scope for v1 by explicit design decision, but the tip must never regress: `_wait_for_next_block` waits for @@ -278,9 +287,29 @@ class ElectrumListener: silently ignoring the header, which (via _consume_headers/_run_once) ends this session the same way a dropped connection would, so run() rotates to the next configured server instead of continuing to trust this one. + + Two more headers are refused without ending the session, since neither is + evidence of a hostile server the way the above are (B-64): one carrying no + hex, and one at the height we already hold a header for. """ height = header["height"] header_hex = header.get("hex") + + if not header_hex: + # Nothing to validate and nothing to draw from — and applying the height + # alone would break exactly the pairing this function exists to keep: + # tip_height would describe a block tip_header_hex doesn't (and on a + # session's first header, would publish a client whose tip is unknown, + # which is B-63). Ignored rather than fatal: a server that only ever + # pushed heights would freeze the draw — visibly, via B-36's + # draw_stalled — instead of costing us the one connection that also + # credits deposits and broadcasts transactions. _run_once separately + # refuses to publish a client while the tip is still unknown. + logger.warning( + "ignoring Electrum header at height %s: no header hex to validate or to draw from", height + ) + return + if height < self.tip_height: logger.warning( "ignoring Electrum header at height %s, below the current tip %s (reorg or server switch?)", @@ -289,19 +318,35 @@ class ElectrumListener: ) return - if header_hex: - if not header_meets_its_own_target(header_hex): - raise HeaderValidationError( - f"header at height {height} does not satisfy its own claimed difficulty target" - ) - if ( - self.tip_header_hex - and height == self.tip_height + 1 - and header_prev_hash(header_hex) != header_hex_to_block_hash(self.tip_header_hex) - ): - raise HeaderValidationError( - f"header at height {height} does not chain from the current tip (height {self.tip_height})" + if height == self.tip_height and self.tip_header_hex: + # B-64: a second header for the height we already hold one for. Either the + # same block re-announced (nothing to do) or a competing one — a reorg at + # the tip, or a server swapping out the very hash a draw may be about to + # use. The linkage check above cannot speak to this case at all, since + # there is no height advance to check. Whichever it is, the hash committed + # to for a height is not replaced under us: if ours turns out to be the + # orphan, corroborate_header (B-28) refuses to seed a draw from it and the + # draw waits for a further block instead. + if header_hex != self.tip_header_hex: + logger.warning( + "ignoring a competing header at the current tip height %s " + "(reorg at the tip, or a server disagreeing with the rest)", + height, ) + return + + if not header_meets_its_own_target(header_hex): + raise HeaderValidationError( + f"header at height {height} does not satisfy its own claimed difficulty target" + ) + if ( + self.tip_header_hex + and height == self.tip_height + 1 + and header_prev_hash(header_hex) != header_hex_to_block_hash(self.tip_header_hex) + ): + raise HeaderValidationError( + f"header at height {height} does not chain from the current tip (height {self.tip_height})" + ) self.tip_height = height self.tip_header_hex = header_hex diff --git a/tests/unit/test_electrum_listener.py b/tests/unit/test_electrum_listener.py index a12a225..f6b0aae 100644 --- a/tests/unit/test_electrum_listener.py +++ b/tests/unit/test_electrum_listener.py @@ -247,6 +247,52 @@ def test_apply_header_rejects_one_that_does_not_chain_from_the_tip(session_facto assert (listener.tip_height, listener.tip_header_hex) == (100, header_100) # untouched +def test_apply_header_ignores_a_competing_header_at_the_current_tip_height(session_factory, caplog): # B-64 + """The linkage check only fires on a single-block advance, so a header at the + height we already hold one for used to be applied on nothing but its own + self-consistency — replacing the very hash a draw may be about to be seeded + with. Whichever it is (a reorg at the tip, or a server disagreeing with the + rest), the hash committed to for a height is not swapped under us; if ours is + the orphan, corroborate_header refuses to draw from it anyway.""" + listener = ElectrumListener(lambda endpoint: None, session_factory, _ENDPOINTS) + header_100 = _mine_header("00" * 32) + listener._apply_header({"height": 100, "hex": header_100}) + + competing_100 = _mine_header("11" * 32) # same height, well-formed, different block + assert competing_100 != header_100 + with caplog.at_level(logging.WARNING): + listener._apply_header({"height": 100, "hex": competing_100}) + + assert (listener.tip_height, listener.tip_header_hex) == (100, header_100) # untouched + assert "competing header" in caplog.text + + +def test_apply_header_treats_the_same_header_re_announced_as_a_no_op(session_factory): # B-64 + listener = ElectrumListener(lambda endpoint: None, session_factory, _ENDPOINTS) + header_100 = _mine_header("00" * 32) + listener._apply_header({"height": 100, "hex": header_100}) + + listener._apply_header({"height": 100, "hex": header_100}) # must not raise + + assert (listener.tip_height, listener.tip_header_hex) == (100, header_100) + + +def test_apply_header_ignores_one_carrying_no_hex(session_factory, caplog): # B-64 + """A hex-less header can be neither validated nor drawn from, and applying its + height alone used to *clear* the hex we already had — leaving tip_height and + tip_header_hex describing different blocks, which is the one thing this function + exists to prevent.""" + listener = ElectrumListener(lambda endpoint: None, session_factory, _ENDPOINTS) + header_100 = _mine_header("00" * 32) + listener._apply_header({"height": 100, "hex": header_100}) + + with caplog.at_level(logging.WARNING): + listener._apply_header({"height": 101}) # height only, no hex + + assert (listener.tip_height, listener.tip_header_hex) == (100, header_100) # pair intact + assert "no header hex" in caplog.text + + def test_apply_header_skips_linkage_check_across_a_height_gap(session_factory): """A reconnect (or the very first header of a session) hands us whatever the server's current tip is — which is legitimately not a single-block advance @@ -634,6 +680,24 @@ async def test_run_once_publishes_the_client_only_once_the_tip_is_known(session_ await run_once_task +async def test_run_once_refuses_a_session_whose_initial_header_carries_no_hex(session_factory): # B-64 + """A hex-less header is ignored rather than fatal (a server that only pushes + heights must not cost us the connection that also credits deposits) — but on the + *first* header of a process there is no tip to fall back on, and publishing the + client anyway would hand consumers a connection whose chain position is unknown, + which is exactly what B-63 closed.""" + client = _FakeConnectClient({"height": 100}) # no "hex" + listener = ElectrumListener(lambda endpoint: client, session_factory, _ENDPOINTS) + + # Bounded: without the guard _run_once goes on to wait on the session's tasks, + # which nothing in this test ever ends — a regression must fail, not hang. + with pytest.raises(HeaderValidationError): + await asyncio.wait_for(listener._run_once(_ENDPOINTS[0]), timeout=5) + + assert listener.client is None + assert (listener.tip_height, listener.tip_header_hex) == (0, None) + + async def test_run_once_leaves_no_client_published_when_the_first_header_is_rejected( session_factory, ): # B-63 @@ -645,7 +709,7 @@ async def test_run_once_leaves_no_client_published_when_the_first_header_is_reje listener = ElectrumListener(lambda endpoint: client, session_factory, _ENDPOINTS) with pytest.raises(HeaderValidationError): - await listener._run_once(_ENDPOINTS[0]) + await asyncio.wait_for(listener._run_once(_ENDPOINTS[0]), timeout=5) assert listener.client is None assert (listener.tip_height, listener.tip_header_hex) == (0, None)