diff --git a/BUGS.md b/BUGS.md index 628ad08..32ef023 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-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. +7 medium, 8 low), listed below as B-42 … B-49. B-25 through B-41 are fixed (see "Previously +fixed" below) — no Critical- or Medium-severity finding remains open; the other 8 are Low/hygiene. 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 sixteen fixes so far brought the suite -from 139 to 217). +coverage — every fix lands with a regression test (the seventeen fixes so far brought the suite +from 139 to 222). 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. @@ -16,29 +16,6 @@ admin auth, single-process assumptions, no user-facing history, etc.) are docume --- -## Medium - -### B-41 — Confirmation/reconciliation logic depends on `verbose=True`, which is not universally supported - -`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 -ElectrumX; it is fragile as the basis for a decision that releases funds. - -**Proposed fix.** Use `blockchain.transaction.get_merkle` (or the scripthash history) for -confirmation and existence checks — both are portable and give the confirming height directly. -Probe verbose support once at connect time and record it on the client, so an unsupported -server is detected loudly at session start rather than silently mid-operation. - ---- - ## Low / hygiene ### B-42 — `/docs` exposed in production @@ -121,9 +98,10 @@ already does. - **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 +- **B-41** — confirmation/reconciliation depended on a verbose `blockchain.transaction.get` reply many Electrum servers reject, and abandonment relied on fragile substring-matching of an error message 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/B-40 fixes). Suite grew from 139 to 217 tests over the sixteen. +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/B-41 fixes). Suite grew from 139 to 222 tests over the seventeen. 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/electrum/client.py b/app/electrum/client.py index b72b28a..db4242b 100644 --- a/app/electrum/client.py +++ b/app/electrum/client.py @@ -169,6 +169,17 @@ class ElectrumClient: async def listunspent(self, scripthash: str) -> list[dict]: return await self.request("blockchain.scripthash.listunspent", [scripthash]) + async def get_history(self, scripthash: str) -> list[dict]: + """Every transaction touching `scripthash`, each as {"tx_hash", "height"} — + height > 0 means confirmed at that height, height <= 0 means still in the + mempool. Used instead of blockchain.transaction.get's verbose=True mode + for confirmation/existence checks (B-41): several Electrum server + implementations and versions reject the verbose flag outright ("verbose + transactions are currently unsupported"), while get_history is a plain, + universally-supported method every server must implement. + """ + return await self.request("blockchain.scripthash.get_history", [scripthash]) + async def broadcast(self, raw_tx_hex: str) -> str: return await self.request("blockchain.transaction.broadcast", [raw_tx_hex]) diff --git a/app/tx/confirmation.py b/app/tx/confirmation.py index a7fdb91..ab9a53a 100644 --- a/app/tx/confirmation.py +++ b/app/tx/confirmation.py @@ -7,7 +7,9 @@ from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker from app.db.models import PendingTransaction from app.electrum.client import ElectrumClient +from app.electrum.scripthash import address_to_scripthash from app.rounds.events import broadcaster +from app.tx.pending_address import own_address_for logger = logging.getLogger(__name__) @@ -31,24 +33,52 @@ async def poll_once(session_factory: async_sessionmaker, client: ElectrumClient) candidates = ( await session.execute( select( - PendingTransaction.id, PendingTransaction.current_txid, PendingTransaction.kind + PendingTransaction.id, + PendingTransaction.current_txid, + PendingTransaction.kind, + PendingTransaction.user_id, ).where(PendingTransaction.status == "pending") ) ).all() + # Resolved once per candidate while the session is still open, and cached + # by scripthash below — every "payout" row shares the same pool address, + # so this also avoids asking the server the same history twice per tick. + scripthash_by_id: dict[int, str] = {} + for pending_id, _txid, kind, user_id in candidates: + try: + address = await own_address_for(session, kind, user_id) + scripthash_by_id[pending_id] = address_to_scripthash(address) + except Exception: + logger.exception("could not derive the address for pending_transaction %s", pending_id) + confirmed = 0 - for pending_id, txid, kind in candidates: + history_cache: dict[str, list[dict]] = {} + for pending_id, txid, kind, _user_id in candidates: + scripthash = scripthash_by_id.get(pending_id) + if scripthash is None: + continue # address derivation failed above; already logged + try: - tx = await client.get_transaction(txid, verbose=True) + if scripthash not in history_cache: + history_cache[scripthash] = await client.get_history(scripthash) except Exception: - # One unresolvable txid must not stop the others: a tx the server no - # longer knows (dropped from the mempool, replaced) used to abort the - # whole pass, so nothing confirmed again until an operator intervened - # (B-03). Abandoning such a row is app/tx/reconcile.py's job, not ours. - logger.warning("could not check pending_transaction %s (txid %s)", pending_id, txid, exc_info=True) + # One unresolvable scripthash must not stop the others: a tx the server + # no longer knows about (dropped from the mempool, replaced) used to + # abort the whole pass via a verbose blockchain.transaction.get call + # that some servers reject outright (B-41), so nothing confirmed again + # until an operator intervened (B-03). Abandoning such a row is + # app/tx/reconcile.py's job, not ours. + logger.warning("could not fetch history for pending_transaction %s (txid %s)", pending_id, txid, exc_info=True) continue - if not tx or tx.get("confirmations", 0) < 1: + + entry = next((e for e in history_cache[scripthash] if e.get("tx_hash") == txid), None) + # height > 0 means confirmed at that height; 0 or absent means still in + # the mempool (or the server doesn't know this txid at all yet) — either + # way, not confirmed, so keep waiting. + if entry is None or entry.get("height", 0) <= 0: continue + async with session_factory() as session: row = await session.get(PendingTransaction, pending_id) if row is None or row.status != "pending": diff --git a/app/tx/pending_address.py b/app/tx/pending_address.py new file mode 100644 index 0000000..6cc2cff --- /dev/null +++ b/app/tx/pending_address.py @@ -0,0 +1,22 @@ +from sqlalchemy.ext.asyncio import AsyncSession + +from app.db.models import User +from app.wallet.hd import derive_pool_address, derive_user_address + + +async def own_address_for(session: AsyncSession, kind: str, user_id: int | None) -> str: + """The address that owns every input of a PendingTransaction of this kind — + a user's own address for a bet/withdrawal, the pool address for a payout. + All our builders only ever spend one address's UTXOs per tx (see + tx/broadcast.py:_signing_context, which derives the same address alongside + the signing key it also needs). + + Shared by tx/confirmation.py and tx/reconcile.py (B-41): both now check + blockchain.scripthash.get_history for this address instead of asking + blockchain.transaction.get for a verbose reply, so the two can't derive + different addresses for the same row. + """ + if kind == "payout": + return derive_pool_address() + user = await session.get(User, user_id) + return derive_user_address(user.derivation_index) diff --git a/app/tx/reconcile.py b/app/tx/reconcile.py index 26683ce..fcf4776 100644 --- a/app/tx/reconcile.py +++ b/app/tx/reconcile.py @@ -36,7 +36,9 @@ from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker from app.audit.log import write_audit_log from app.db.models import PendingTransaction, Round, RoundParticipant, UtxoEvent, Withdrawal from app.electrum.client import ElectrumClient +from app.electrum.scripthash import address_to_scripthash from app.rounds.events import broadcaster +from app.tx.pending_address import own_address_for from app.wallet.balance import recompute_balance logger = logging.getLogger(__name__) @@ -72,22 +74,21 @@ class PendingTransactionReconciler: await asyncio.sleep(_POLL_INTERVAL_SECONDS) -async def _tx_exists_on_chain(client: ElectrumClient, txid: str) -> bool: - """True if the server knows this txid at all (mempool or mined). An error reply - means "unknown", which is the answer we're looking for; a transport failure is - *not* — that raises, and the caller leaves the row alone until next time.""" - try: - tx = await client.get_transaction(txid, verbose=True) - except Exception as exc: - message = str(exc).lower() - if "missing" in message or "not found" in message or "no such" in message or "unknown" in message: - return False - raise - return bool(tx) - - async def reconcile_once(session_factory: async_sessionmaker, client: ElectrumClient) -> int: - """Returns how many rows were resolved (promoted or abandoned).""" + """Returns how many rows were resolved (promoted or abandoned). + + Existence is decided by checking whether a row's own address's history + (blockchain.scripthash.get_history) includes its txid at all — mempool or + mined — rather than asking blockchain.transaction.get for a verbose reply + (B-41): several Electrum server implementations and versions reject the + verbose flag outright, and the previous substring-matching on the error + text (looking for "missing", "not found", ...) was fragile as the basis for + a decision that releases funds. A transport failure fetching history still + raises and leaves the row alone until next time — get_history not + returning our txid is the only thing that means "gone". History is cached + per scripthash within one pass, since every "payout" row shares the same + pool address. + """ now = datetime.now(timezone.utc) async with session_factory() as session: rows = ( @@ -95,16 +96,25 @@ async def reconcile_once(session_factory: async_sessionmaker, client: ElectrumCl select(PendingTransaction).where(PendingTransaction.status.in_(("building", "pending"))) ) ).all() - candidates = [ - (row.id, row.status, row.current_txid) - for row in rows - if _is_due(row, now) - ] + candidates = [] + for row in rows: + if not _is_due(row, now): + continue + try: + address = await own_address_for(session, row.kind, row.user_id) + scripthash = address_to_scripthash(address) + except Exception: + logger.exception("could not derive the address for pending_transaction %s", row.id) + continue + candidates.append((row.id, row.status, row.current_txid, scripthash)) resolved = 0 - for row_id, status, txid in candidates: + history_cache: dict[str, list[dict]] = {} + for row_id, status, txid, scripthash in candidates: try: - exists = await _tx_exists_on_chain(client, txid) + if scripthash not in history_cache: + history_cache[scripthash] = await client.get_history(scripthash) + exists = any(entry.get("tx_hash") == txid for entry in history_cache[scripthash]) except Exception: # Transport/server problem — say nothing about this tx and try again on # the next pass rather than abandoning a tx that may be perfectly alive. diff --git a/tests/unit/test_confirmation.py b/tests/unit/test_confirmation.py index 056f3d4..2e9129e 100644 --- a/tests/unit/test_confirmation.py +++ b/tests/unit/test_confirmation.py @@ -1,41 +1,83 @@ import pytest -from sqlalchemy import select from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine import app.bets.confirmation # noqa: F401 (registers the "bet" handler) import app.rounds.confirmation # noqa: F401 (registers the "payout" handler) +from sqlalchemy import select + +from app.config import settings from app.db.base import Base -from app.db.models import PendingTransaction, Round, RoundParticipant +from app.db.models import PendingTransaction, Round, RoundParticipant, User +from app.electrum.scripthash import address_to_scripthash from app.tx.confirmation import poll_once +from app.wallet.hd import derive_user_address class FakeClient: - def __init__(self, confirmations_by_txid: dict[str, int]): - self._confirmations = confirmations_by_txid + """B-41: poll_once now asks blockchain.scripthash.get_history rather than a + verbose blockchain.transaction.get, so this hands back a flat history — + height > 0 means confirmed at that height, 0 (or absent) means still in the + mempool. The scripthash argument is ignored: every candidate's derived + address is looked up against the same known universe of txids, which is + fine since matching happens on tx_hash, not on which address asked.""" - async def get_transaction(self, txid: str, verbose: bool = False) -> dict: - return {"confirmations": self._confirmations.get(txid, 0)} + def __init__(self, heights_by_txid: dict[str, int]): + self._heights = heights_by_txid + + async def get_history(self, scripthash: str) -> list[dict]: + return [{"tx_hash": txid, "height": height} for txid, height in self._heights.items()] @pytest.fixture -async def session_factory(): +async def session_factory(tmp_path, monkeypatch): + # own_address_for (B-41) derives each row's address via the HD wallet, so + # poll_once now needs a real master key — same bootstrap test_broadcast.py + # and test_reconcile.py use. + monkeypatch.setattr(settings, "master_key_path", str(tmp_path / "master.xprv.enc")) + monkeypatch.setattr( + settings, + "xprv_encryption_key", + __import__("cryptography.fernet", fromlist=["Fernet"]).Fernet.generate_key().decode(), + ) + from app.wallet import hd + + hd._account_key = None + hd.generate_master_key() + engine = create_async_engine("sqlite+aiosqlite:///:memory:") async with engine.begin() as conn: await conn.run_sync(Base.metadata.create_all) yield async_sessionmaker(engine, expire_on_commit=False) await engine.dispose() + hd._account_key = None + + +async def _make_user(session, derivation_index: int) -> User: + user = User( + username=f"user{derivation_index}", + password_hash="x", + derivation_index=derivation_index, + address=derive_user_address(derivation_index), + ) + session.add(user) + await session.flush() + return user async def test_bet_confirmation_marks_participant_confirmed(session_factory): async with session_factory() as session: + user = await _make_user(session, 0) session.add(Round(id=1, status="open")) session.add( RoundParticipant( - round_id=1, user_id=1, bet_amount_sats=1_000, bet_txid="tx1", status="broadcast" + round_id=1, user_id=user.id, bet_amount_sats=1_000, bet_txid="tx1", status="broadcast" ) ) session.add( - PendingTransaction(kind="bet", round_id=1, user_id=1, current_txid="tx1", fee_rate_sat_vb=1, raw_tx_hex="00", status="pending") + PendingTransaction( + kind="bet", round_id=1, user_id=user.id, current_txid="tx1", fee_rate_sat_vb=1, + raw_tx_hex="00", status="pending", + ) ) await session.commit() @@ -53,9 +95,17 @@ async def test_bet_confirmation_marks_participant_confirmed(session_factory): async def test_unconfirmed_tx_is_left_pending(session_factory): async with session_factory() as session: + user = await _make_user(session, 0) session.add(Round(id=2, status="open")) - session.add(RoundParticipant(round_id=2, user_id=1, bet_amount_sats=1_000, bet_txid="tx2", status="broadcast")) - session.add(PendingTransaction(kind="bet", round_id=2, user_id=1, current_txid="tx2", fee_rate_sat_vb=1, raw_tx_hex="00", status="pending")) + session.add( + RoundParticipant(round_id=2, user_id=user.id, bet_amount_sats=1_000, bet_txid="tx2", status="broadcast") + ) + session.add( + PendingTransaction( + kind="bet", round_id=2, user_id=user.id, current_txid="tx2", fee_rate_sat_vb=1, + raw_tx_hex="00", status="pending", + ) + ) await session.commit() client = FakeClient({"tx2": 0}) @@ -70,7 +120,11 @@ async def test_unconfirmed_tx_is_left_pending(session_factory): async def test_payout_confirmation_closes_round(session_factory): async with session_factory() as session: session.add(Round(id=3, status="paying_out", payout_txid="tx3")) - session.add(PendingTransaction(kind="payout", round_id=3, current_txid="tx3", fee_rate_sat_vb=1, raw_tx_hex="00", status="pending")) + session.add( + PendingTransaction( + kind="payout", round_id=3, current_txid="tx3", fee_rate_sat_vb=1, raw_tx_hex="00", status="pending" + ) + ) await session.commit() client = FakeClient({"tx3": 2}) @@ -83,43 +137,52 @@ async def test_payout_confirmation_closes_round(session_factory): class ExplodingClient: - """Answers for one txid and raises for the other — a tx the server no longer - knows (dropped from the mempool, replaced by a bump).""" + """Answers for one address's history and raises for the other's — the + get_history equivalent of a server that no longer knows a particular tx + (dropped from the mempool, replaced by a bump).""" - def __init__(self, known: dict[str, int], exploding_txid: str): - self._known = known - self._exploding = exploding_txid + def __init__(self, heights_by_txid: dict[str, int], exploding_scripthash: str): + self._heights = heights_by_txid + self._exploding = exploding_scripthash - async def get_transaction(self, txid: str, verbose: bool = False) -> dict: - if txid == self._exploding: - raise RuntimeError("missing transaction") - return {"confirmations": self._known.get(txid, 0)} + async def get_history(self, scripthash: str) -> list[dict]: + if scripthash == self._exploding: + raise RuntimeError("server error") + return [{"tx_hash": txid, "height": height} for txid, height in self._heights.items()] -async def test_one_unresolvable_txid_does_not_block_the_others(session_factory): - """B-03: the lookup used to be unguarded, so a single unknown txid aborted the - whole pass — nothing confirmed again until an operator intervened, which in turn - meant no round could ever close.""" +async def test_one_unresolvable_candidate_does_not_block_the_others(session_factory): + """B-03: the lookup used to be unguarded, so a single failing candidate aborted + the whole pass — nothing confirmed again until an operator intervened, which in + turn meant no round could ever close. B-41 changed the failure unit from "one + txid" to "one address's history", but the isolation guarantee is the same.""" async with session_factory() as session: + good_user = await _make_user(session, 0) + gone_user = await _make_user(session, 1) session.add(Round(id=10, status="open")) session.add( - RoundParticipant(round_id=10, user_id=1, bet_amount_sats=1_000, bet_txid="good", status="broadcast") - ) - session.add( - PendingTransaction( - kind="bet", round_id=10, user_id=2, current_txid="gone", fee_rate_sat_vb=1, raw_tx_hex="00", - status="pending", + RoundParticipant( + round_id=10, user_id=good_user.id, bet_amount_sats=1_000, bet_txid="good", status="broadcast" ) ) session.add( PendingTransaction( - kind="bet", round_id=10, user_id=1, current_txid="good", fee_rate_sat_vb=1, raw_tx_hex="00", - status="pending", + kind="bet", round_id=10, user_id=gone_user.id, current_txid="gone", fee_rate_sat_vb=1, + raw_tx_hex="00", status="pending", + ) + ) + session.add( + PendingTransaction( + kind="bet", round_id=10, user_id=good_user.id, current_txid="good", fee_rate_sat_vb=1, + raw_tx_hex="00", status="pending", ) ) await session.commit() - confirmed = await poll_once(session_factory, ExplodingClient({"good": 1}, exploding_txid="gone")) + exploding_scripthash = address_to_scripthash(derive_user_address(1)) + confirmed = await poll_once( + session_factory, ExplodingClient({"good": 1}, exploding_scripthash=exploding_scripthash) + ) assert confirmed == 1 # the healthy one still got processed async with session_factory() as session: @@ -135,15 +198,16 @@ async def test_bet_confirms_after_an_rbf_bump_changed_the_txid(session_factory): a txid no participant carried — the participant stayed "broadcast" forever and the round could never close. It now resolves by (round_id, user_id).""" async with session_factory() as session: + user = await _make_user(session, 0) session.add(Round(id=11, status="open")) session.add( RoundParticipant( - round_id=11, user_id=7, bet_amount_sats=1_000, bet_txid="old-txid", status="broadcast" + round_id=11, user_id=user.id, bet_amount_sats=1_000, bet_txid="old-txid", status="broadcast" ) ) session.add( PendingTransaction( - kind="bet", round_id=11, user_id=7, current_txid="bumped-txid", fee_rate_sat_vb=2, + kind="bet", round_id=11, user_id=user.id, current_txid="bumped-txid", fee_rate_sat_vb=2, raw_tx_hex="00", status="pending", replaced_by_txid="old-txid", ) ) @@ -171,3 +235,35 @@ async def test_payout_confirms_after_an_rbf_bump_changed_the_txid(session_factor async with session_factory() as session: assert (await session.get(Round, 12)).status == "closed" + + +async def test_poll_once_caches_history_per_scripthash(session_factory): + """Two pending bets from the same user share one address — fetching its + history twice in one pass would be wasteful.""" + async with session_factory() as session: + user = await _make_user(session, 0) + session.add(Round(id=20, status="open")) + session.add( + PendingTransaction( + kind="bet", round_id=20, user_id=user.id, current_txid="tx-a", fee_rate_sat_vb=1, + raw_tx_hex="00", status="pending", + ) + ) + session.add( + PendingTransaction( + kind="withdrawal", user_id=user.id, current_txid="tx-b", fee_rate_sat_vb=1, + raw_tx_hex="00", status="pending", + ) + ) + await session.commit() + + call_count = {"n": 0} + + class CountingClient: + async def get_history(self, scripthash: str) -> list[dict]: + call_count["n"] += 1 + return [{"tx_hash": "tx-a", "height": 0}, {"tx_hash": "tx-b", "height": 0}] + + await poll_once(session_factory, CountingClient()) + + assert call_count["n"] == 1 diff --git a/tests/unit/test_pending_address.py b/tests/unit/test_pending_address.py new file mode 100644 index 0000000..a807f7f --- /dev/null +++ b/tests/unit/test_pending_address.py @@ -0,0 +1,55 @@ +"""B-41: own_address_for is the single place tx/confirmation.py and +tx/reconcile.py derive a PendingTransaction's own address from — a payout's +address must always be the pool's, everything else the actual user's.""" + +import pytest +from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine + +from app.config import settings +from app.db.base import Base +from app.db.models import User +from app.tx.pending_address import own_address_for + + +@pytest.fixture +async def session_factory(tmp_path, monkeypatch): + monkeypatch.setattr(settings, "master_key_path", str(tmp_path / "master.xprv.enc")) + monkeypatch.setattr( + settings, + "xprv_encryption_key", + __import__("cryptography.fernet", fromlist=["Fernet"]).Fernet.generate_key().decode(), + ) + from app.wallet import hd + + hd._account_key = None + hd.generate_master_key() + + engine = create_async_engine("sqlite+aiosqlite:///:memory:") + async with engine.begin() as conn: + await conn.run_sync(Base.metadata.create_all) + yield async_sessionmaker(engine, expire_on_commit=False) + await engine.dispose() + hd._account_key = None + + +async def test_payout_uses_the_pool_address_regardless_of_user_id(session_factory): + from app.wallet.hd import derive_pool_address + + async with session_factory() as session: + address = await own_address_for(session, "payout", None) + + assert address == derive_pool_address() + + +@pytest.mark.parametrize("kind", ["bet", "withdrawal"]) +async def test_bet_and_withdrawal_use_the_users_own_address(session_factory, kind): + from app.wallet.hd import derive_user_address + + async with session_factory() as session: + user = User(username="alice", password_hash="x", derivation_index=3, address=derive_user_address(3)) + session.add(user) + await session.flush() + + address = await own_address_for(session, kind, user.id) + + assert address == derive_user_address(3) diff --git a/tests/unit/test_reconcile.py b/tests/unit/test_reconcile.py index 2785a7b..2a47e49 100644 --- a/tests/unit/test_reconcile.py +++ b/tests/unit/test_reconcile.py @@ -1,5 +1,10 @@ """Regression tests for B-04 (and the "building" half of B-08): a transaction that -never made it onto the chain must give the coins back instead of freezing them.""" +never made it onto the chain must give the coins back instead of freezing them. + +Also covers B-41: existence/reconciliation checks go through +blockchain.scripthash.get_history rather than a verbose blockchain.transaction.get +reply, so the fake clients below implement get_history. +""" import pytest from embit import script @@ -7,37 +12,60 @@ from embit.transaction import Transaction, TransactionInput, TransactionOutput from sqlalchemy import select from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine +from app.config import settings from app.db.base import Base from app.db.models import AuditLog, PendingTransaction, RoundParticipant, User, UtxoEvent, Withdrawal from app.tx.reconcile import reconcile_once class UnknownTxClient: - """A server that doesn't know any of the txids it's asked about.""" + """A server whose history for any address never includes our txid.""" - async def get_transaction(self, txid: str, verbose: bool = False): - raise RuntimeError(f"missing transaction {txid}") + async def get_history(self, scripthash: str) -> list[dict]: + return [] class KnownTxClient: - async def get_transaction(self, txid: str, verbose: bool = False): - return {"txid": txid, "confirmations": 0} + """A server whose history for the address includes our txid — mined or + still in the mempool doesn't matter for existence, only for confirmation + (which is tx/confirmation.py's concern, not reconcile.py's).""" + + def __init__(self, txid: str = "betxid"): + self._txid = txid + + async def get_history(self, scripthash: str) -> list[dict]: + return [{"tx_hash": self._txid, "height": 100}] class BrokenClient: """A transport failure — says nothing about whether the tx exists.""" - async def get_transaction(self, txid: str, verbose: bool = False): + async def get_history(self, scripthash: str) -> list[dict]: raise ConnectionResetError("connection reset") @pytest.fixture -async def session_factory(): +async def session_factory(tmp_path, monkeypatch): + # own_address_for (B-41) derives each row's address via the HD wallet rather + # than trusting the DB's address column, so reconcile_once now needs a real + # master key set up — same bootstrap test_broadcast.py uses. + monkeypatch.setattr(settings, "master_key_path", str(tmp_path / "master.xprv.enc")) + monkeypatch.setattr( + settings, + "xprv_encryption_key", + __import__("cryptography.fernet", fromlist=["Fernet"]).Fernet.generate_key().decode(), + ) + from app.wallet import hd + + hd._account_key = None + hd.generate_master_key() + engine = create_async_engine("sqlite+aiosqlite:///:memory:") async with engine.begin() as conn: await conn.run_sync(Base.metadata.create_all) yield async_sessionmaker(engine, expire_on_commit=False) await engine.dispose() + hd._account_key = None # A real (unsigned) transaction spending one input, built rather than hand-written @@ -66,11 +94,19 @@ async def _seed_bet( participant_status: str, age_seconds: int, last_broadcast_age_seconds: int | None = None, + derivation_index: int = 0, ): from datetime import datetime, timedelta, timezone + from app.wallet.hd import derive_user_address + async with session_factory() as session: - user = User(username="u", password_hash="x", derivation_index=0, address="plm1qtest") + user = User( + username="u", + password_hash="x", + derivation_index=derivation_index, + address=derive_user_address(derivation_index), + ) session.add(user) await session.flush() session.add( @@ -145,7 +181,7 @@ async def test_promotes_a_building_row_whose_tx_did_reach_the_chain(session_fact session_factory, pending_status="building", participant_status="building", age_seconds=300 ) - resolved = await reconcile_once(session_factory, KnownTxClient()) + resolved = await reconcile_once(session_factory, KnownTxClient("betxid")) assert resolved == 1 async with session_factory() as session: @@ -219,8 +255,10 @@ async def test_abandoned_withdrawal_is_marked_failed_and_kept(session_factory): they can see it didn't go through.""" from datetime import datetime, timedelta, timezone + from app.wallet.hd import derive_user_address + async with session_factory() as session: - user = User(username="w", password_hash="x", derivation_index=1, address="plm1qtest2") + user = User(username="w", password_hash="x", derivation_index=1, address=derive_user_address(1)) session.add(user) await session.flush() session.add( @@ -264,3 +302,42 @@ async def test_abandoned_withdrawal_is_marked_failed_and_kept(session_factory): assert withdrawal.status == "failed" assert withdrawal.txid is None assert (await session.scalars(select(UtxoEvent))).one().spent_txid is None + + +# --- B-41: existence checks now use get_history and share it across candidates +# sharing the same address, instead of a per-tx verbose blockchain.transaction.get. -- + + +async def test_reconcile_once_caches_history_per_scripthash(session_factory): + """Two payout PendingTransaction rows always share the same pool address — + fetching its history twice in one pass would be wasteful and, at scale + across many candidates on one address, needlessly slow the whole tick.""" + from datetime import datetime, timedelta, timezone + + async with session_factory() as session: + old = datetime.now(timezone.utc) - timedelta(hours=7) + session.add( + PendingTransaction( + kind="payout", round_id=1, current_txid="payout-a", fee_rate_sat_vb=1, + raw_tx_hex=_RAW_TX, status="pending", broadcast_at=old, last_broadcast_at=old, + ) + ) + session.add( + PendingTransaction( + kind="payout", round_id=2, current_txid="payout-b", fee_rate_sat_vb=1, + raw_tx_hex=_RAW_TX, status="pending", broadcast_at=old, last_broadcast_at=old, + ) + ) + await session.commit() + + call_count = {"n": 0} + + class CountingClient: + async def get_history(self, scripthash: str) -> list[dict]: + call_count["n"] += 1 + return [{"tx_hash": "payout-a", "height": 100}, {"tx_hash": "payout-b", "height": 100}] + + resolved = await reconcile_once(session_factory, CountingClient()) + + assert resolved == 0 # both exist — nothing to abandon or promote (already "pending") + assert call_count["n"] == 1 # one call covered both rows sharing the pool address