Decouple the RBF abandon clock from the bump clock (B-27)
bump_fee (tx/broadcast.py) used to overwrite PendingTransaction. broadcast_at on every fee bump, but reconcile.py's abandon-after-N- hours grace period is measured from that same column. A transaction successfully bumped every rbf_timeout_seconds (900s by default) but never mined reset that clock before it could ever reach the 6-hour abandon window, so it was never abandoned: its UTXOs never returned to the user, and if it was a bet the round stayed in "closing" indefinitely. PendingTransaction gains a last_broadcast_at column (migration 861e76aaf34c, backfilled from broadcast_at for existing rows before the NOT NULL constraint is applied). broadcast_at is now never rewritten after creation, so reconcile.py's _is_due keeps measuring from the first broadcast unchanged. bump_fee updates last_broadcast_at instead, and should_bump now reads last_broadcast_at rather than broadcast_at — correct, since whether another bump is due should reset after every bump, unlike the reconciler's abandon check, which must not. BUGS.md moves B-27 to "Previously fixed" with the fix description; the suite grows from 148 to 151 tests, including a direct proof that a tx bumped a minute ago but first broadcast 7 hours ago still gets abandoned. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
+16
-5
@@ -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()
|
||||
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
@@ -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')
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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."""
|
||||
|
||||
Reference in New Issue
Block a user