diff --git a/BUGS.md b/BUGS.md index 33e289a..6003d14 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-32 … B-49. B-25 through B-31 are fixed (see "Previously -fixed" below) — no Critical-severity finding remains open; the other 18 are High/Medium/Low. +7 medium, 8 low), listed below as B-33 … B-49. B-25 through B-32 are fixed (see "Previously +fixed" below) — no Critical-severity finding remains open; the other 17 are High/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 seven fixes so far brought the suite -from 139 to 185). +coverage — every fix lands with a regression test (the eight fixes so far brought the suite +from 139 to 187). 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. @@ -20,28 +20,6 @@ single-process assumptions, no user-facing history, etc.), see "Known gaps / TOD ## High -### B-32 — `bump_fee` can loop forever on rebroadcasts the node always rejects - -`tx/broadcast.py:87-88` forces `fee_delta = 1` when `fee_delta <= 0`. A **one-satoshi** total -fee increase violates BIP125 rule 4 (a replacement must pay at least the incremental relay fee -times its own size), so the node rejects it. `bump_fee` raises before updating `pending`, so -`fee_rate_sat_vb` never advances and the next tick **retries with identical parameters, every -30 seconds, forever**. - -This triggers whenever the real fee exceeds the estimate — i.e. whenever dust change was -absorbed into the fee, which is an explicitly supported path -(`wallet/psbt_builder.py:105-107`). - -Related, same function: `new_fee_rate = pending.fee_rate_sat_vb + 1` on every bump, with **no -ceiling**. A transaction stuck for a day reaches ~96 sat/vB, eating the user's change, and it -ignores the `le=10_000` bound the admin panel enforces on the config field. - -**Proposed fix.** Compute the delta from the actual replacement vsize -(`fee_delta = max(new_fee - old_fee, ceil(vsize * incremental_relay_rate))`) so the bump is -always relay-valid. Cap `new_fee_rate` at the configured maximum and raise `RbfError` once -reached, so the transaction falls through to the reconciler (which, since B-27, correctly -abandons it) rather than being retried indefinitely. - ### B-33 — No brute-force protection on a custodial wallet `POST /auth/login` (`auth/routes.py:83`) has no rate limiting, no lockout, no delay and no @@ -248,9 +226,10 @@ already does. - **B-29** — a UTXO absent from one server's `listunspent` was marked spent immediately, irreversibly, on a single unauthenticated reply - **B-30** — a lost scripthash subscription meant a user's deposits were never credited, with no periodic safety net - **B-31** — resubscribing on reconnect ran serially before anything else started, freezing the chain tip (and so an in-flight draw) for the whole sweep +- **B-32** — an RBF bump's fee delta could fall below BIP125's relay-mandated minimum, so the node rejected it and the same tick retried identically forever; also had no ceiling on how high the fee rate could climb See git history for the fix-by-fix breakdown (commits `f13f685`, `50a43ae`, `933760e`, and the -B-28/B-29/B-30/B-31 fixes). Suite grew from 139 to 185 tests over the seven. +B-28/B-29/B-30/B-31/B-32 fixes). Suite grew from 139 to 187 tests over the eight. 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/api/routes/admin.py b/app/api/routes/admin.py index 11b3db5..d4f036c 100644 --- a/app/api/routes/admin.py +++ b/app/api/routes/admin.py @@ -14,6 +14,7 @@ from app.db.session import get_session from app.rounds.config import get_round_config from app.wallet.address import is_valid_plm_address from app.wallet.hd import derive_user_wif +from app.wallet.psbt_builder import MAX_FEE_RATE_SAT_VB router = APIRouter(prefix="/admin", tags=["admin"]) @@ -68,7 +69,7 @@ class RoundConfigUpdate(BaseModel): bet_amount_sats: int | None = Field(default=None, gt=0, le=100_000 * 100_000_000) round_duration_seconds: int | None = Field(default=None, ge=30, le=7 * 24 * 3600) round_cooldown_seconds: int | None = Field(default=None, ge=0, le=24 * 3600) - fee_rate_sat_vb: int | None = Field(default=None, ge=1, le=10_000) + fee_rate_sat_vb: int | None = Field(default=None, ge=1, le=MAX_FEE_RATE_SAT_VB) rbf_timeout_seconds: int | None = Field(default=None, ge=60, le=7 * 24 * 3600) draw_animation_seconds: int | None = Field(default=None, ge=0, le=600) diff --git a/app/tx/broadcast.py b/app/tx/broadcast.py index d2fa708..c31ea60 100644 --- a/app/tx/broadcast.py +++ b/app/tx/broadcast.py @@ -14,12 +14,18 @@ from app.electrum.client import ElectrumClient from app.rounds.config import get_round_config from app.wallet.hd import derive_pool_key, derive_user_key from app.wallet.plm_network import PLM_MAINNET -from app.wallet.psbt_builder import RBF_SEQUENCE, estimate_vsize +from app.wallet.psbt_builder import MAX_FEE_RATE_SAT_VB, RBF_SEQUENCE, estimate_vsize logger = logging.getLogger(__name__) _POLL_INTERVAL_SECONDS = 30 -_FEE_RATE_INCREMENT = 1 # minimum relay-policy-friendly bump per BIP125 +_FEE_RATE_INCREMENT = 1 # how much pending.fee_rate_sat_vb's *target* rises by per bump + +# BIP125 rule 4: a replacement transaction must pay at least this much more, in +# total, per vbyte of its own size, than the transaction it replaces — Bitcoin +# Core's default incremental relay fee. bump_fee's delta must never fall below +# this regardless of what the target-rate arithmetic comes out to (B-32). +_INCREMENTAL_RELAY_FEE_RATE_SAT_VB = 1 class RbfError(Exception): @@ -79,20 +85,38 @@ async def bump_fee(session: AsyncSession, client: ElectrumClient, pending: Pendi 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. + 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 + (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" + ) + old_tx = Transaction.parse(bytes.fromhex(pending.raw_tx_hex)) signing_key, own_script, own_address = await _signing_context(session, pending) 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)) - new_fee_rate = pending.fee_rate_sat_vb + _FEE_RATE_INCREMENT - new_fee = estimate_vsize(len(old_tx.vin), len(old_tx.vout)) * new_fee_rate - fee_delta = new_fee - old_fee - if fee_delta <= 0: - fee_delta = _FEE_RATE_INCREMENT # already above the new target vsize*rate; bump by a token amount + target_fee_rate = min(pending.fee_rate_sat_vb + _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 + # out to. That naive difference used to go to zero or negative whenever + # old_fee already exceeded target_fee (e.g. a dust change amount folded into + # the original fee — wallet/psbt_builder.py's DUST_LIMIT_SATS handling), and + # the previous fallback — a flat 1-satoshi total bump — was nowhere near this + # relay-mandated minimum, so the node rejected it every time. Because bump_fee + # raised before touching `pending`, the next tick retried with identical + # parameters every 30 seconds, forever (B-32). + min_valid_delta = vsize * _INCREMENTAL_RELAY_FEE_RATE_SAT_VB + fee_delta = max(target_fee - old_fee, min_valid_delta) change_index = _find_change_output(old_tx, own_address) if change_index is None or old_tx.vout[change_index].value <= fee_delta: @@ -124,7 +148,11 @@ async def bump_fee(session: AsyncSession, client: ElectrumClient, pending: Pendi pending.replaced_by_txid = old_txid # points backwards: what current_txid replaced pending.current_txid = new_txid pending.raw_tx_hex = raw_hex - pending.fee_rate_sat_vb = new_fee_rate + # 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 diff --git a/app/wallet/psbt_builder.py b/app/wallet/psbt_builder.py index 6c9ebf8..b822617 100644 --- a/app/wallet/psbt_builder.py +++ b/app/wallet/psbt_builder.py @@ -25,6 +25,14 @@ RBF_SEQUENCE = 0xFFFFFFFD # withdrawal fail at broadcast with an opaque error (B-06). DUST_LIMIT_SATS = 294 +# Sanity ceiling on any transaction's fee rate — shared by RoundConfig.fee_rate_sat_vb's +# admin-facing bound (app/api/routes/admin.py, so the two can't drift apart, the same +# reason MIN_PASSWORD_LENGTH is shared in auth/security.py) and tx/broadcast.py's RBF +# bump escalation, which refuses to bump a pending_transaction past this rate (B-32) — +# without a ceiling, a stuck transaction's fee climbed by 1 sat/vB every bump forever, +# eating further and further into the sender's change with no limit. +MAX_FEE_RATE_SAT_VB = 10_000 + class InsufficientFundsError(Exception): """`code` is the machine-readable identifier the API layer forwards to the diff --git a/tests/unit/test_broadcast.py b/tests/unit/test_broadcast.py index d7dee34..e4ea562 100644 --- a/tests/unit/test_broadcast.py +++ b/tests/unit/test_broadcast.py @@ -11,7 +11,7 @@ from app.db.base import Base from app.db.models import PendingTransaction, User from app.tx.broadcast import RbfError, bump_fee, should_bump from app.wallet.plm_network import PLM_MAINNET -from app.wallet.psbt_builder import Utxo, build_signed_transaction +from app.wallet.psbt_builder import MAX_FEE_RATE_SAT_VB, Utxo, build_signed_transaction, estimate_vsize def _key(seed_byte: int) -> HDKey: @@ -319,3 +319,132 @@ async def test_bump_fee_retargets_every_stored_txid(session_factory): utxo = (await session.scalars(select(UtxoEvent))).one() assert utxo.spent_txid == new_txid + + +# --- B-32: the bump delta must always meet BIP125's relay-mandated minimum, and +# escalation must stop at a ceiling instead of retrying forever. ------------------ + + +async def test_bump_fee_meets_bip125_minimum_when_old_fee_already_exceeds_target(session_factory): + """old_fee (as bump_fee computes it from the actual prevout amounts) can end + up higher than vsize * target_fee_rate — e.g. because dust change was folded + into the original fee (wallet/psbt_builder.py's DUST_LIMIT_SATS handling). + The naive `target_fee - old_fee` goes negative in that case; the previous + fallback was a flat 1-satoshi total bump, nowhere near BIP125 rule 4's + required minimum, so the node rejected it every time and — since bump_fee + raised before touching `pending` — the next tick retried identically every + 30 seconds, forever. Simulated here by reporting a prevout inflated beyond + what was actually spent, which has the same effect on old_fee as dust + absorption would.""" + 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(96).to_public()).address(network=PLM_MAINNET) + + utxo_amount = 150_000_000 + utxo_txid = "55" * 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="dave", 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 + + # Reports a prevout inflated well beyond what was actually spent — has the + # same effect on old_fee as dust absorption would have: old_fee ends up far + # above vsize * target_fee_rate (target_fee_rate = 2 here). + 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) + + assert client.broadcasted + new_tx = Transaction.parse(bytes.fromhex(client.broadcasted[0])) + old_tx = Transaction.parse(bytes.fromhex(built.raw_hex)) + old_change = next(o.value for o in old_tx.vout if o.script_pubkey.address(network=PLM_MAINNET) == my_address) + new_change = next(o.value for o in new_tx.vout if o.script_pubkey.address(network=PLM_MAINNET) == my_address) + + vsize = estimate_vsize(len(old_tx.vin), len(old_tx.vout)) + min_valid_delta = vsize * 1 # BIP125 rule 4's floor at a 1 sat/vB incremental relay fee + assert min_valid_delta > 1 # meaningfully more than the old flat "1 satoshi" fallback + assert old_change - new_change == min_valid_delta + + async with session_factory() as session: + row = await session.get(PendingTransaction, pending_id) + old_fee_as_bump_fee_computed_it = built.fee_sats + inflated_excess + expected_rate = (old_fee_as_bump_fee_computed_it + min_valid_delta) // vsize + assert row.fee_rate_sat_vb == expected_rate + assert row.fee_rate_sat_vb > 2 # the actual rate, not the naive (and too-low) target + + +async def test_bump_fee_refuses_once_at_the_max_fee_rate(session_factory): + """Without a ceiling, a stuck transaction's fee rate climbed by 1 sat/vB every + 30 seconds forever, eating further and further into the user's change.""" + 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(95).to_public()).address(network=PLM_MAINNET) + + utxo_amount = 150_000_000 + utxo_txid = "66" * 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="erin", 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=MAX_FEE_RATE_SAT_VB, + 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 + + 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) + + assert not client.broadcasted