From 08c566d54787380b6cc5891d93c850206040f85e Mon Sep 17 00:00:00 2001 From: Davide Grilli Date: Mon, 27 Jul 2026 15:14:58 +0200 Subject: [PATCH] Restructure bump_fee into three phases, drop float fee math (B-40) bump_fee issued one get_transaction per input (up to 15s each) and then a broadcast, all with the caller's DB session held open - exactly the pattern already fixed elsewhere for the same reason (B-18's _trigger_payout, B-31's refresh_user). Also, _prevout_amount computed a prevout's satoshi value via round(value_coins * 100_000_000) on a float the server reported, in a codebase that is otherwise strictly integer-satoshi. bump_fee now takes a session_factory and a pending_id instead of a live session and row, with three phases: read what's needed (the signing key, current fee rate, raw tx) and close the session before any network call; do the chain reads, signing and broadcast with no session open; reopen a session only to persist the outcome. _prevout_amount now asks for the raw (non-verbose) transaction and reads embit's parsed TransactionOutput.value directly - already an exact integer, no float conversion involved at all. A pending_transaction that's no longer "pending" by the time bump_fee actually runs (it confirmed in the meantime, a normal race) is now a quiet no-op returning None, rather than being folded into RbfBumper's error-logging path. Suite grows from 214 to 217 tests. BUGS.md moves B-40 to Previously fixed, and trims its own now-stale claim that bump_fee still depended on verbose=True (B-41) - it no longer does. --- BUGS.md | 40 +++------ app/tx/broadcast.py | 128 ++++++++++++++++----------- tests/unit/test_broadcast.py | 162 ++++++++++++++++++++++++++++++----- 3 files changed, 231 insertions(+), 99 deletions(-) diff --git a/BUGS.md b/BUGS.md index e738c01..628ad08 100644 --- a/BUGS.md +++ b/BUGS.md @@ -1,11 +1,11 @@ # 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-40 … B-49. B-25 through B-39 are fixed (see "Previously -fixed" below) — no Critical-severity finding remains open; the other 10 are Medium/Low. +7 medium, 8 low), listed below as B-41 … B-49. B-25 through B-40 are fixed (see "Previously +fixed" below) — no Critical-severity finding remains open; the other 9 are Medium/Low. 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 fifteen fixes so far brought the suite -from 139 to 214). +coverage — every fix lands with a regression test (the sixteen fixes so far brought the suite +from 139 to 217). 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. @@ -18,28 +18,15 @@ admin auth, single-process assumptions, no user-facing history, etc.) are docume ## Medium -### B-40 — `bump_fee` holds a DB session open across N network calls +### B-41 — Confirmation/reconciliation logic depends on `verbose=True`, which is not universally supported -`tx/broadcast.py:80` issues one `get_transaction` **per input** (up to 15s each) and then a -`broadcast`, all with the session open. This is precisely the pattern B-18 removed from -`_trigger_payout` via its three-phase structure; it survives here. - -Side note in the same function: `_prevout_amount` does `round(value_coins * 100_000_000)` on a -float from the server — acceptable at these magnitudes, but it is floating-point money -arithmetic in a codebase that is otherwise strictly integer-satoshi. - -**Proposed fix.** Restructure into the same three phases: read what is needed and close the -session, do the chain work, then reopen to persist. For the float: prefer the raw (non-verbose) -transaction and parse the output value as an integer with `embit`, which is what -`reconcile.py:_release_inputs` already does for inputs. - -### B-41 — All confirmation logic depends on `verbose=True`, which is not universally supported - -`poll_once`, `reconcile._tx_exists_on_chain` and `bump_fee` all call -`blockchain.transaction.get(txid, True)`. Several Electrum server implementations and versions -reject the verbose flag ("verbose transactions are currently unsupported"). Falling back onto -such a server means **no confirmations, no reconciliation, no bumps** — and the code would read -that as a transport error and stay silent. +`poll_once` and `reconcile._tx_exists_on_chain` call `blockchain.transaction.get(txid, True)`. +Several Electrum server implementations and versions reject the verbose flag ("verbose +transactions are currently unsupported"). Falling back onto such a server means **no +confirmations, no reconciliation** — and the code would read that as a transport error and stay +silent. (`bump_fee`'s own `verbose=True` call was removed as part of the B-40 fix — it now reads +the raw transaction and parses the output value with `embit` instead, so bumps are unaffected by +this finding.) Related: `reconcile.py:83` decides whether to **abandon a transaction** by substring-matching the error text (`"missing"`, `"not found"`, `"no such"`, `"unknown"`). It works against @@ -133,9 +120,10 @@ already does. - **B-37** — a withdrawal covered by unconfirmed change answered "insufficient balance" instead of distinguishing it from actually having no funds - **B-38** — the SSE subscriber cap was global, so one client opening enough connections degraded every other user to polling - **B-39** — SQLite ran without WAL or a `busy_timeout`, so a writer could block every reader and a second writer failed immediately instead of waiting +- **B-40** — `bump_fee` held a DB session open across N slow network calls, and computed a prevout's value from a server-reported float instead of an exact integer See git history for the fix-by-fix breakdown (commits `f13f685`, `50a43ae`, `933760e`, and the -B-28/B-29/B-30/B-31/B-32/B-33/B-34/B-35/B-36/B-37/B-38/B-39 fixes). Suite grew from 139 to 214 tests over the fifteen. +B-28/B-29/B-30/B-31/B-32/B-33/B-34/B-35/B-36/B-37/B-38/B-39/B-40 fixes). Suite grew from 139 to 217 tests over the sixteen. 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, diff --git a/app/tx/broadcast.py b/app/tx/broadcast.py index c31ea60..4540a99 100644 --- a/app/tx/broadcast.py +++ b/app/tx/broadcast.py @@ -48,14 +48,14 @@ def should_bump(pending: PendingTransaction, now: datetime, timeout_seconds: int return now >= pending.last_broadcast_at.replace(tzinfo=timezone.utc) + timedelta(seconds=timeout_seconds) -async def _signing_context(session: AsyncSession, pending: PendingTransaction) -> tuple: +async def _signing_context(session: AsyncSession, kind: str, user_id: int | None) -> tuple: """Returns (signing_key, own_script, own_address) for the single sender that controls every input of this tx — a user for bet/withdrawal, the pool for payout. All our builders only ever spend one address's UTXOs per tx.""" - if pending.kind == "payout": + if kind == "payout": key = derive_pool_key() else: - user = await session.get(User, pending.user_id) + user = await session.get(User, user_id) key = derive_user_key(user.derivation_index) own_script = script.p2wpkh(key.to_public()) own_address = own_script.address(network=PLM_MAINNET) @@ -63,10 +63,19 @@ async def _signing_context(session: AsyncSession, pending: PendingTransaction) - async def _prevout_amount(client: ElectrumClient, vin: TransactionInput) -> int: + """The exact integer satoshi value of the output this input spends. + + Parsed directly from the raw transaction via embit rather than asking the + server for its own float, whole-coin-denominated "value" field (verbose=True) + and converting with `* 100_000_000` — embit's TransactionOutput.value is + already an integer number of satoshis straight from the tx's binary + encoding, so this never touches floating point in a codebase that is + otherwise strictly integer-satoshi (B-40). + """ txid_hex = vin.txid.hex() - tx = await client.get_transaction(txid_hex, verbose=True) - value_coins = tx["vout"][vin.vout]["value"] - return round(value_coins * 100_000_000) + raw_hex = await client.get_transaction(txid_hex, verbose=False) + prevout_tx = Transaction.parse(bytes.fromhex(raw_hex)) + return prevout_tx.vout[vin.vout].value def _find_change_output(tx: Transaction, change_address: str) -> int | None: @@ -76,35 +85,55 @@ def _find_change_output(tx: Transaction, change_address: str) -> int | None: return None -async def bump_fee(session: AsyncSession, client: ElectrumClient, pending: PendingTransaction) -> str: - """Rebuild `pending`'s transaction with a higher fee (same inputs, same - recipient outputs, the extra fee taken from the change output) and - rebroadcast. Returns the new txid. +async def bump_fee( + session_factory: async_sessionmaker, client: ElectrumClient, pending_id: int +) -> str | None: + """Rebuild pending_transaction `pending_id`'s transaction with a higher fee + (same inputs, same recipient outputs, the extra fee taken from the change + output) and rebroadcast. Returns the new txid, or None if there was nothing + to do (the row is gone or already left "pending" — a normal race with + confirmation, not an error). + + Three phases, so no DB session is held across the network calls this needs + (one get_transaction per input, then a broadcast) — the same shape used + elsewhere for exactly this reason (B-18, rounds/scheduler.py:_trigger_payout; + B-31, electrum/listener.py:refresh_user) and now here too (B-40): read what's + needed and close the session, do the chain work, then reopen to persist. Only handles the common case: exactly one change output paying back to the tx's own sender address, large enough to absorb the increase. If there's no such output (e.g. an exact-amount bet with no change), this raises RbfError — bumping such a tx would require selecting additional inputs, which isn't implemented for the MVP; it needs manual operator intervention. Also raises - RbfError, rather than bumping, once `pending` is already at MAX_FEE_RATE_SAT_VB + RbfError, rather than bumping, once the row is already at MAX_FEE_RATE_SAT_VB (B-32) — the reconciler abandons it if it never confirms (B-27), instead of this retrying an ever-higher fee forever. """ - if pending.fee_rate_sat_vb >= MAX_FEE_RATE_SAT_VB: - raise RbfError( - f"pending_transaction {pending.id}: already at the maximum fee rate " - f"({MAX_FEE_RATE_SAT_VB} sat/vB) — refusing to bump further" - ) + # --- Phase 1: read what's needed, close the session before any network call --- + async with session_factory() as session: + pending = await session.get(PendingTransaction, pending_id) + if pending is None or pending.status != "pending": + logger.info("pending_transaction %s no longer pending; skipping bump", pending_id) + return None + if pending.fee_rate_sat_vb >= MAX_FEE_RATE_SAT_VB: + raise RbfError( + f"pending_transaction {pending_id}: already at the maximum fee rate " + f"({MAX_FEE_RATE_SAT_VB} sat/vB) — refusing to bump further" + ) - old_tx = Transaction.parse(bytes.fromhex(pending.raw_tx_hex)) - signing_key, own_script, own_address = await _signing_context(session, pending) + kind = pending.kind + current_fee_rate = pending.fee_rate_sat_vb + raw_tx_hex = pending.raw_tx_hex + signing_key, own_script, own_address = await _signing_context(session, kind, pending.user_id) + # --- Phase 2: chain reads, signing, and the broadcast — no DB session open ---- + old_tx = Transaction.parse(bytes.fromhex(raw_tx_hex)) input_amounts = [await _prevout_amount(client, vin) for vin in old_tx.vin] total_in = sum(input_amounts) old_fee = total_in - sum(o.value for o in old_tx.vout) vsize = estimate_vsize(len(old_tx.vin), len(old_tx.vout)) - target_fee_rate = min(pending.fee_rate_sat_vb + _FEE_RATE_INCREMENT, MAX_FEE_RATE_SAT_VB) + target_fee_rate = min(current_fee_rate + _FEE_RATE_INCREMENT, MAX_FEE_RATE_SAT_VB) target_fee = vsize * target_fee_rate # BIP125 rule 4's minimum, in absolute sats for this tx's size — the floor # `fee_delta` must never go below, no matter what `target_fee - old_fee` comes @@ -120,7 +149,7 @@ async def bump_fee(session: AsyncSession, client: ElectrumClient, pending: Pendi change_index = _find_change_output(old_tx, own_address) if change_index is None or old_tx.vout[change_index].value <= fee_delta: - raise RbfError(f"pending_transaction {pending.id}: no change output large enough to absorb a fee bump") + raise RbfError(f"pending_transaction {pending_id}: no change output large enough to absorb a fee bump") new_vout = list(old_tx.vout) bumped_change = new_vout[change_index].value - fee_delta @@ -144,25 +173,28 @@ async def bump_fee(session: AsyncSession, client: ElectrumClient, pending: Pendi new_txid = final_tx.txid().hex() await client.broadcast(raw_hex) - old_txid = pending.current_txid - pending.replaced_by_txid = old_txid # points backwards: what current_txid replaced - pending.current_txid = new_txid - pending.raw_tx_hex = raw_hex - # The *actual* resulting rate, not target_fee_rate: when the BIP125-minimum - # floor above raised fee_delta past the naive target, the tx now pays more - # than target_fee_rate implied. Recording the true rate keeps the next bump's - # arithmetic honest instead of drifting from what's really being paid. - pending.fee_rate_sat_vb = (old_fee + fee_delta) // vsize - pending.attempt_count += 1 - # last_broadcast_at, not broadcast_at (B-27): broadcast_at must stay the *first* - # broadcast, since reconcile.py's abandon-after-N-hours grace period is measured - # from it — overwriting it here used to reset that clock on every bump, so a - # repeatedly-bumped-but-never-mined tx was never abandoned. - pending.last_broadcast_at = datetime.now(timezone.utc) - await _retarget_txid_references(session, pending, old_txid, new_txid) - await session.commit() + # --- Phase 3: persist the outcome ---------------------------------------------- + async with session_factory() as session: + pending = await session.get(PendingTransaction, pending_id) + old_txid = pending.current_txid + pending.replaced_by_txid = old_txid # points backwards: what current_txid replaced + pending.current_txid = new_txid + pending.raw_tx_hex = raw_hex + # The *actual* resulting rate, not target_fee_rate: when the BIP125-minimum + # floor above raised fee_delta past the naive target, the tx now pays more + # than target_fee_rate implied. Recording the true rate keeps the next bump's + # arithmetic honest instead of drifting from what's really being paid. + pending.fee_rate_sat_vb = (old_fee + fee_delta) // vsize + pending.attempt_count += 1 + # last_broadcast_at, not broadcast_at (B-27): broadcast_at must stay the *first* + # broadcast, since reconcile.py's abandon-after-N-hours grace period is measured + # from it — overwriting it here used to reset that clock on every bump, so a + # repeatedly-bumped-but-never-mined tx was never abandoned. + pending.last_broadcast_at = datetime.now(timezone.utc) + await _retarget_txid_references(session, pending, old_txid, new_txid) + await session.commit() - logger.info("bumped %s pending_transaction %s: %s -> %s", pending.kind, pending.id, old_txid, new_txid) + logger.info("bumped %s pending_transaction %s: %s -> %s", kind, pending_id, old_txid, new_txid) return new_txid @@ -230,16 +262,12 @@ class RbfBumper: candidates = ( await session.scalars(select(PendingTransaction).where(PendingTransaction.status == "pending")) ).all() - due = [p for p in candidates if should_bump(p, now, timeout_seconds=timeout_seconds)] + due_ids = [p.id for p in candidates if should_bump(p, now, timeout_seconds=timeout_seconds)] - for pending in due: - async with self._session_factory() as session: - row = await session.get(PendingTransaction, pending.id) - if row is None or row.status != "pending": - continue - try: - await bump_fee(session, client, row) - except RbfError: - logger.exception("could not bump pending_transaction %s", row.id) - except Exception: - logger.exception("unexpected error bumping pending_transaction %s", row.id) + for pending_id in due_ids: + try: + await bump_fee(self._session_factory, client, pending_id) + except RbfError: + logger.exception("could not bump pending_transaction %s", pending_id) + except Exception: + logger.exception("unexpected error bumping pending_transaction %s", pending_id) diff --git a/tests/unit/test_broadcast.py b/tests/unit/test_broadcast.py index e4ea562..73bc59f 100644 --- a/tests/unit/test_broadcast.py +++ b/tests/unit/test_broadcast.py @@ -3,7 +3,7 @@ from datetime import datetime, timedelta, timezone import pytest from embit import script from embit.bip32 import HDKey -from embit.transaction import Transaction +from embit.transaction import Transaction, TransactionInput, TransactionOutput from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine from app.config import settings @@ -58,12 +58,23 @@ def test_should_bump_measures_from_last_broadcast_not_first(monkeypatch): class FakeClient: + """B-40: _prevout_amount now asks for the raw (non-verbose) transaction and + reads its output value as an integer via embit, rather than a verbose reply's + float "value" field — so this fake must hand back a real, parseable raw tx + whose vout[0] carries the requested amount (every test here spends vout 0 of + its fixture UTXO).""" + def __init__(self, prevout_values: dict[str, int]): self._prevout_values = prevout_values self.broadcasted: list[str] = [] - async def get_transaction(self, txid: str, verbose: bool = False) -> dict: - return {"vout": {0: {"value": self._prevout_values[txid] / 100_000_000}}} + async def get_transaction(self, txid: str, verbose: bool = False) -> str: + assert verbose is False + fake_prevout_tx = Transaction( + vin=[TransactionInput(b"\x00" * 32, 0)], + vout=[TransactionOutput(self._prevout_values[txid], script.Script(b"\x00\x14" + b"\x00" * 20))], + ) + return fake_prevout_tx.serialize().hex() async def broadcast(self, raw_tx_hex: str) -> str: self.broadcasted.append(raw_tx_hex) @@ -130,9 +141,7 @@ async def test_bump_fee_shrinks_change_and_rebroadcasts(session_factory): client = FakeClient({utxo_txid: utxo_amount}) - async with session_factory() as session: - row = await session.get(PendingTransaction, pending_id) - new_txid = await bump_fee(session, client, row) + new_txid = await bump_fee(session_factory, client, pending_id) assert client.broadcasted assert new_txid != built.txid @@ -197,9 +206,7 @@ async def test_bump_fee_leaves_broadcast_at_untouched(session_factory): client = FakeClient({utxo_txid: utxo_amount}) before_bump = datetime.now(timezone.utc) - async with session_factory() as session: - row = await session.get(PendingTransaction, pending_id) - await bump_fee(session, client, row) + await bump_fee(session_factory, client, pending_id) async with session_factory() as session: row = await session.get(PendingTransaction, pending_id) @@ -246,10 +253,8 @@ async def test_bump_fee_raises_when_no_change_output(session_factory): client = FakeClient({utxo_txid: utxo_amount}) - async with session_factory() as session: - row = await session.get(PendingTransaction, pending_id) - with pytest.raises(RbfError): - await bump_fee(session, client, row) + with pytest.raises(RbfError): + await bump_fee(session_factory, client, pending_id) async def test_bump_fee_retargets_every_stored_txid(session_factory): @@ -303,9 +308,7 @@ async def test_bump_fee_retargets_every_stored_txid(session_factory): await session.commit() pending_id = pending.id - async with session_factory() as session: - row = await session.get(PendingTransaction, pending_id) - new_txid = await bump_fee(session, FakeClient({utxo_txid: utxo_amount}), row) + new_txid = await bump_fee(session_factory, FakeClient({utxo_txid: utxo_amount}), pending_id) async with session_factory() as session: from sqlalchemy import select @@ -378,9 +381,7 @@ async def test_bump_fee_meets_bip125_minimum_when_old_fee_already_exceeds_target inflated_excess = 50_000 client = FakeClient({utxo_txid: utxo_amount + inflated_excess}) - async with session_factory() as session: - row = await session.get(PendingTransaction, pending_id) - new_txid = await bump_fee(session, client, row) + new_txid = await bump_fee(session_factory, client, pending_id) assert client.broadcasted new_tx = Transaction.parse(bytes.fromhex(client.broadcasted[0])) @@ -442,9 +443,124 @@ async def test_bump_fee_refuses_once_at_the_max_fee_rate(session_factory): client = FakeClient({utxo_txid: utxo_amount}) - async with session_factory() as session: - row = await session.get(PendingTransaction, pending_id) - with pytest.raises(RbfError): - await bump_fee(session, client, row) + with pytest.raises(RbfError): + await bump_fee(session_factory, client, pending_id) assert not client.broadcasted + + +# --- B-40: bump_fee must not hold a DB session open across its network calls, +# and a row that's no longer pending by the time it runs is a quiet no-op. ------- + + +async def test_bump_fee_holds_no_session_open_during_network_calls(session_factory): + """The get_transaction-per-input reads and the broadcast must happen with no + DB session held open — the same shape used elsewhere for this reason (B-18, + electrum/listener.py's refresh_user for B-31) — otherwise a session sits + idle in the pool for the whole duration of what can be several slow network + round-trips.""" + from app.wallet.hd import derive_user_address, derive_user_key + + signer = derive_user_key(0) + my_address = derive_user_address(0) + from_script = script.p2wpkh(signer.to_public()) + to_address = script.p2wpkh(_key(94).to_public()).address(network=PLM_MAINNET) + + utxo_amount = 150_000_000 + utxo_txid = "77" * 32 + built = build_signed_transaction( + signing_key=signer, + from_script=from_script, + utxos=[Utxo(utxo_txid, 0, utxo_amount)], + to_address=to_address, + amount_sats=10_000_000, + change_address=my_address, + fee_rate_sat_vb=1, + ) + + async with session_factory() as session: + user = User(username="frank", password_hash="x", derivation_index=0, address=my_address) + session.add(user) + await session.commit() + pending = PendingTransaction( + kind="bet", + user_id=user.id, + current_txid=built.txid, + fee_rate_sat_vb=1, + raw_tx_hex=built.raw_hex, + status="pending", + broadcast_at=datetime.now(timezone.utc) - timedelta(seconds=1000), + ) + session.add(pending) + await session.commit() + pending_id = pending.id + + open_count = {"n": 0} + + class _TrackedSession: + def __init__(self, inner): + self._inner = inner + + async def __aenter__(self): + result = await self._inner.__aenter__() + open_count["n"] += 1 + return result + + async def __aexit__(self, *exc): + open_count["n"] -= 1 + return await self._inner.__aexit__(*exc) + + def tracking_session_factory(): + return _TrackedSession(session_factory()) + + class TrackingClient(FakeClient): + async def get_transaction(self, txid, verbose=False): + assert open_count["n"] == 0, "a session was held open during a network call" + return await super().get_transaction(txid, verbose) + + async def broadcast(self, raw_tx_hex): + assert open_count["n"] == 0, "a session was held open during the broadcast" + return await super().broadcast(raw_tx_hex) + + client = TrackingClient({utxo_txid: utxo_amount}) + await bump_fee(tracking_session_factory, client, pending_id) + + assert client.broadcasted + assert open_count["n"] == 0 # nothing left open afterwards either + + +async def test_bump_fee_is_a_noop_when_no_longer_pending(session_factory): + """A row can legitimately confirm (or otherwise leave "pending") between + being read as due and RbfBumper actually attempting the bump — a normal + race, not an error. Must return quietly rather than raising or touching + the network.""" + async with session_factory() as session: + user = User(username="grace", password_hash="x", derivation_index=0, address="plm1qxxx") + session.add(user) + await session.commit() + pending = PendingTransaction( + kind="bet", + user_id=user.id, + current_txid="already-confirmed-txid", + fee_rate_sat_vb=1, + raw_tx_hex="00", + status="confirmed", + broadcast_at=datetime.now(timezone.utc) - timedelta(seconds=1000), + ) + session.add(pending) + await session.commit() + pending_id = pending.id + + client = FakeClient({}) + + result = await bump_fee(session_factory, client, pending_id) + + assert result is None + assert not client.broadcasted + + +async def test_bump_fee_is_a_noop_when_the_row_is_gone(session_factory): + client = FakeClient({}) + result = await bump_fee(session_factory, client, 999_999) + assert result is None + assert not client.broadcasted