diff --git a/BUGS.md b/BUGS.md index 1e751d1..495b281 100644 --- a/BUGS.md +++ b/BUGS.md @@ -1,13 +1,13 @@ # 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-25 … B-49. B-25 and B-26 are fixed as of 2026-07-27; the -other 23 are open. 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 (B-25 and B-26 -together brought the suite from 139 to 148). +7 medium, 8 low), listed below as B-25 … B-49. B-25, B-26 and B-27 are fixed as of 2026-07-27; +the other 22 are open. 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 (B-25, B-26 and B-27 +together brought the suite from 139 to 151). -The recurring pattern across B-27, B-29 and B-36 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 recurring pattern across B-29 and B-36 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. Outgoing transactions reconcile; deposits do not. **`paying_out` is now fully recoverable, not just idempotent.** B-25 made a payout retry @@ -27,22 +27,6 @@ single-process assumptions, no user-facing history, etc.), see "Known gaps / TOD ## Critical -### B-27 — Every RBF bump resets the reconciler's abandon clock, so it never fires - -`tx/broadcast.py:122` sets `pending.broadcast_at = now` on each bump, but -`tx/reconcile.py:133` computes the 6-hour abandon deadline **from that same field**. - -With the default `rbf_timeout_seconds = 900`, a transaction that is successfully bumped every -15 minutes but never mined resets the counter long before it can reach 6 hours: it is **never -abandoned**, its UTXOs never return to the user, and if it is a bet the round stays in -`closing` indefinitely (`scheduler.py:90-91`). `reconcile.py` exists precisely to prevent -this, and the bumper disarms it. - -**Proposed fix.** Split the field: keep `broadcast_at` as the *first* broadcast (never -rewritten — it is what `_is_due` must use) and add `last_broadcast_at`, updated by -`bump_fee` and used by `should_bump`. Alembic migration backfilling `last_broadcast_at = -broadcast_at`. - ### B-28 — A hostile Electrum server (or a MITM) can choose the winner `electrum/listener.py:167-186` accepts any header whose `height >= tip_height`: no @@ -421,6 +405,32 @@ Regression tests: `tests/unit/test_scheduler.py` `test_tick_throttles_retry_after_a_recent_payout_failure`, `test_tick_retries_once_the_throttle_window_has_elapsed`). +### B-27 — Every RBF bump resets the reconciler's abandon clock, so it never fires + +`tx/broadcast.py` used to set `pending.broadcast_at = now` on each bump, but +`tx/reconcile.py`'s `_is_due` computes the 6-hour abandon deadline **from that same field**. + +With the default `rbf_timeout_seconds = 900`, a transaction that is successfully bumped every +15 minutes but never mined reset the counter long before it could reach 6 hours: it was +**never abandoned**, its UTXOs never returned to the user, and if it was a bet the round stayed +in `closing` indefinitely (`scheduler.py:90-91`). `reconcile.py` exists precisely to prevent +this, and the bumper disarmed it. + +**Fixed:** the field is split, exactly as proposed. `PendingTransaction` gained a +`last_broadcast_at` column (Alembic migration `861e76aaf34c`, backfilled from the existing +`broadcast_at` for every pre-existing row, then made `NOT NULL`). `broadcast_at` is now never +rewritten after creation — it stays the *first* broadcast, which is what `reconcile.py:_is_due` +already read and continues to read unchanged. `bump_fee` (`tx/broadcast.py`) now updates +`last_broadcast_at` instead, and `should_bump` reads `last_broadcast_at` rather than +`broadcast_at` — correctly, since *that* decision (is another bump due?) should reset after +every bump, unlike the reconciler's abandon check, which must not. + +Regression tests: `tests/unit/test_broadcast.py` +(`test_should_bump_measures_from_last_broadcast_not_first`, +`test_bump_fee_leaves_broadcast_at_untouched`) and `tests/unit/test_reconcile.py` +(`test_abandons_a_repeatedly_bumped_tx_despite_a_recent_last_broadcast`, the direct proof that +a tx bumped minutes ago but first broadcast 7 hours ago is still abandoned). + 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, 7 high, 7 medium, 5 low. All 24 were fixed and verified against the current code on diff --git a/app/db/models.py b/app/db/models.py index 2ea24ce..2ceb766 100644 --- a/app/db/models.py +++ b/app/db/models.py @@ -144,7 +144,14 @@ class PendingTransaction(Base): current_txid: Mapped[str] = mapped_column(String(64)) fee_rate_sat_vb: Mapped[int] raw_tx_hex: Mapped[str] = mapped_column(String) + # The *first* broadcast — never rewritten by a bump — since this is what the + # reconciler's abandon-after-N-hours grace period (app/tx/reconcile.py) measures + # from. Bumping used to overwrite this field, which reset that clock on every + # bump and meant a repeatedly-bumped-but-never-mined tx was never abandoned + # (B-27). last_broadcast_at is the one bump_fee updates, and the one should_bump + # (app/tx/broadcast.py) reads to decide whether another bump is due. broadcast_at: Mapped[datetime] = mapped_column(default=utcnow) + last_broadcast_at: Mapped[datetime] = mapped_column(default=utcnow) status: Mapped[str] = mapped_column(String(16), default="pending") # The txid this row had *before* its most recent RBF bump (bump_fee rewrites # current_txid in place). Despite the name reading forwards, it points diff --git a/app/tx/broadcast.py b/app/tx/broadcast.py index 0df974e..d2fa708 100644 --- a/app/tx/broadcast.py +++ b/app/tx/broadcast.py @@ -27,12 +27,19 @@ class RbfError(Exception): def should_bump(pending: PendingTransaction, now: datetime, timeout_seconds: int) -> bool: - """Pure decision: has this pending tx been unconfirmed for longer than the - configured timeout (RoundConfig.rbf_timeout_seconds)? Kept separate from the - I/O-heavy bump_fee() so it's trivially unit-testable.""" + """Pure decision: has this pending tx gone unconfirmed for longer than the + configured timeout (RoundConfig.rbf_timeout_seconds) *since it was last + broadcast*? Kept separate from the I/O-heavy bump_fee() so it's trivially + unit-testable. + + Deliberately measured from last_broadcast_at, not broadcast_at: this decides + whether *another* bump is due, which should reset after every bump (a tx just + rebroadcast at a higher fee deserves the same grace period again) — unlike + reconcile.py's abandon check, which must measure from the *first* broadcast so + repeated bumping can't indefinitely postpone ever giving up on a tx (B-27).""" if pending.status != "pending": return False - return now >= pending.broadcast_at.replace(tzinfo=timezone.utc) + timedelta(seconds=timeout_seconds) + return now >= pending.last_broadcast_at.replace(tzinfo=timezone.utc) + timedelta(seconds=timeout_seconds) async def _signing_context(session: AsyncSession, pending: PendingTransaction) -> tuple: @@ -119,7 +126,11 @@ async def bump_fee(session: AsyncSession, client: ElectrumClient, pending: Pendi pending.raw_tx_hex = raw_hex pending.fee_rate_sat_vb = new_fee_rate pending.attempt_count += 1 - pending.broadcast_at = datetime.now(timezone.utc) + # 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() diff --git a/app/tx/reconcile.py b/app/tx/reconcile.py index 32ddf79..26683ce 100644 --- a/app/tx/reconcile.py +++ b/app/tx/reconcile.py @@ -129,6 +129,12 @@ async def reconcile_once(session_factory: async_sessionmaker, client: ElectrumCl def _is_due(row: PendingTransaction, now: datetime) -> bool: + # Deliberately broadcast_at (the *first* broadcast), not last_broadcast_at: an + # RBF bump used to overwrite this same field, which reset this grace period on + # every bump and meant a repeatedly-bumped-but-never-mined tx was never + # abandoned (B-27). tx/broadcast.py:bump_fee now only ever touches + # last_broadcast_at, so this keeps measuring from when the tx first appeared, + # no matter how many times it's since been bumped. grace = _BUILDING_GRACE_SECONDS if row.status == "building" else _ABANDON_AFTER_SECONDS return now >= row.broadcast_at.replace(tzinfo=timezone.utc) + timedelta(seconds=grace) diff --git a/migrations/versions/861e76aaf34c_add_last_broadcast_at_to_pending_.py b/migrations/versions/861e76aaf34c_add_last_broadcast_at_to_pending_.py new file mode 100644 index 0000000..d56abc8 --- /dev/null +++ b/migrations/versions/861e76aaf34c_add_last_broadcast_at_to_pending_.py @@ -0,0 +1,41 @@ +"""add last_broadcast_at to pending_transactions + +Fixes B-27: bump_fee used to overwrite broadcast_at on every RBF bump, but +tx/reconcile.py's abandon-after-N-hours grace period is measured from that same +column — so a transaction bumped repeatedly but never mined reset that clock on +every bump and was never abandoned. broadcast_at now stays the *first* broadcast +(what the reconciler measures from); last_broadcast_at is the new column bump_fee +updates and should_bump reads to decide whether another bump is due. + +Backfilled from the existing broadcast_at (the best available approximation for +rows written before this column existed — for a row never bumped it's exact) +before the NOT NULL constraint is applied, so this is safe against any existing +data. + +Revision ID: 861e76aaf34c +Revises: 8a1c4e7b2d90 +Create Date: 2026-07-27 + +""" +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision: str = '861e76aaf34c' +down_revision: Union[str, Sequence[str], None] = '8a1c4e7b2d90' +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + op.add_column('pending_transactions', sa.Column('last_broadcast_at', sa.DateTime(), nullable=True)) + op.execute('UPDATE pending_transactions SET last_broadcast_at = broadcast_at') + with op.batch_alter_table('pending_transactions') as batch_op: + batch_op.alter_column('last_broadcast_at', nullable=False) + + +def downgrade() -> None: + op.drop_column('pending_transactions', 'last_broadcast_at') diff --git a/tests/unit/test_broadcast.py b/tests/unit/test_broadcast.py index 3ccc33e..d7dee34 100644 --- a/tests/unit/test_broadcast.py +++ b/tests/unit/test_broadcast.py @@ -22,7 +22,7 @@ def _key(seed_byte: int) -> HDKey: def test_should_bump_false_before_timeout(): pending = PendingTransaction( kind="bet", current_txid="x", fee_rate_sat_vb=1, raw_tx_hex="00", status="pending", - broadcast_at=datetime.now(timezone.utc), + broadcast_at=datetime.now(timezone.utc), last_broadcast_at=datetime.now(timezone.utc), ) assert should_bump(pending, datetime.now(timezone.utc), timeout_seconds=900) is False @@ -31,6 +31,7 @@ def test_should_bump_true_after_timeout(): pending = PendingTransaction( kind="bet", current_txid="x", fee_rate_sat_vb=1, raw_tx_hex="00", status="pending", broadcast_at=datetime.now(timezone.utc) - timedelta(seconds=1000), + last_broadcast_at=datetime.now(timezone.utc) - timedelta(seconds=1000), ) assert should_bump(pending, datetime.now(timezone.utc), timeout_seconds=900) is True @@ -39,6 +40,19 @@ def test_should_bump_false_when_not_pending(): pending = PendingTransaction( kind="bet", current_txid="x", fee_rate_sat_vb=1, raw_tx_hex="00", status="confirmed", broadcast_at=datetime.now(timezone.utc) - timedelta(seconds=1000), + last_broadcast_at=datetime.now(timezone.utc) - timedelta(seconds=1000), + ) + assert should_bump(pending, datetime.now(timezone.utc), timeout_seconds=900) is False + + +def test_should_bump_measures_from_last_broadcast_not_first(monkeypatch): + """B-27 regression: a tx first broadcast long ago, but bumped recently, must not + be due for another bump yet — should_bump has to look at last_broadcast_at, not + the original broadcast_at, or every tick would try to re-bump it.""" + pending = PendingTransaction( + kind="bet", current_txid="x", fee_rate_sat_vb=1, raw_tx_hex="00", status="pending", + broadcast_at=datetime.now(timezone.utc) - timedelta(seconds=10_000), + last_broadcast_at=datetime.now(timezone.utc), ) assert should_bump(pending, datetime.now(timezone.utc), timeout_seconds=900) is False @@ -136,6 +150,63 @@ async def test_bump_fee_shrinks_change_and_rebroadcasts(session_factory): assert row.attempt_count == 2 +async def test_bump_fee_leaves_broadcast_at_untouched(session_factory): + """B-27 regression: bump_fee must only ever update last_broadcast_at. Before + this, it overwrote broadcast_at on every bump — the same field + tx/reconcile.py's abandon-after-N-hours grace period measures from — so a + repeatedly-bumped-but-never-mined tx reset that clock forever and was never + abandoned.""" + 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(97).to_public()).address(network=PLM_MAINNET) + + utxo_amount = 150_000_000 + utxo_txid = "33" * 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, + ) + + original_broadcast_at = datetime.now(timezone.utc) - timedelta(days=1) + async with session_factory() as session: + user = User(username="carol", 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=original_broadcast_at, + last_broadcast_at=original_broadcast_at, + ) + session.add(pending) + await session.commit() + pending_id = pending.id + + 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) + + async with session_factory() as session: + row = await session.get(PendingTransaction, pending_id) + assert row.broadcast_at.replace(tzinfo=timezone.utc) == original_broadcast_at + assert row.last_broadcast_at.replace(tzinfo=timezone.utc) >= before_bump + + async def test_bump_fee_raises_when_no_change_output(session_factory): from app.wallet.hd import derive_user_address, derive_user_key diff --git a/tests/unit/test_reconcile.py b/tests/unit/test_reconcile.py index 3c757a3..2785a7b 100644 --- a/tests/unit/test_reconcile.py +++ b/tests/unit/test_reconcile.py @@ -59,7 +59,14 @@ _RAW_TX = ( ) -async def _seed_bet(session_factory, *, pending_status: str, participant_status: str, age_seconds: int): +async def _seed_bet( + session_factory, + *, + pending_status: str, + participant_status: str, + age_seconds: int, + last_broadcast_age_seconds: int | None = None, +): from datetime import datetime, timedelta, timezone async with session_factory() as session: @@ -85,6 +92,10 @@ async def _seed_bet(session_factory, *, pending_status: str, participant_status: status=participant_status, ) ) + # last_broadcast_age_seconds defaults to age_seconds (never bumped): the two + # timestamps only diverge in the B-27 regression test below, which simulates + # a tx that's been bumped recently but first appeared long ago. + last_age = age_seconds if last_broadcast_age_seconds is None else last_broadcast_age_seconds session.add( PendingTransaction( kind="bet", @@ -95,6 +106,7 @@ async def _seed_bet(session_factory, *, pending_status: str, participant_status: raw_tx_hex=_RAW_TX, status=pending_status, broadcast_at=datetime.now(timezone.utc) - timedelta(seconds=age_seconds), + last_broadcast_at=datetime.now(timezone.utc) - timedelta(seconds=last_age), ) ) await session.commit() @@ -167,6 +179,27 @@ async def test_leaves_a_recently_broadcast_pending_row_alone(session_factory): assert await reconcile_once(session_factory, UnknownTxClient()) == 0 +async def test_abandons_a_repeatedly_bumped_tx_despite_a_recent_last_broadcast(session_factory): + """B-27 regression: before last_broadcast_at existed, bump_fee overwrote + broadcast_at on every bump, which is the same field the abandon grace period is + measured from — so a tx first seen long ago but bumped minutes ago (exactly what + a stuck-but-repeatedly-bumped tx looks like) reset its own clock forever and was + never abandoned. The reconciler must still abandon it based on when it *first* + appeared, ignoring how recently it was last bumped.""" + await _seed_bet( + session_factory, + pending_status="pending", + participant_status="broadcast", + age_seconds=7 * 3600, # first broadcast 7h ago — past the 6h abandon window + last_broadcast_age_seconds=60, # bumped a minute ago + ) + + assert await reconcile_once(session_factory, UnknownTxClient()) == 1 + + async with session_factory() as session: + assert (await session.scalars(select(PendingTransaction))).one().status == "failed" + + async def test_transport_failure_never_abandons_anything(session_factory): """A dead connection says nothing about the transaction. Treating it as "gone" would release coins for transactions that are perfectly alive."""