From 4c80c1c5bf601b5a78f5b56aaa05a2deb2a1dd9a Mon Sep 17 00:00:00 2001 From: Davide Grilli Date: Mon, 27 Jul 2026 23:30:06 +0200 Subject: [PATCH] Cap the number of inputs a transaction may spend (B-48) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit select_utxos had no ceiling on input count, so an address fragmented into many small deposits built an ever-larger transaction whose fee — deducted from the amount being moved — eroded the bet's share of the pool or the withdrawn amount, and past a few hundred inputs stopped being standard at all. MAX_TX_INPUTS (50) now bounds the selection. Reaching the cap without covering the target is reported as its own "too_many_inputs" code, distinct from having no funds, with the cap carried in the error params for the 7 translations. The payout path records the same distinction in its payout_failed audit reason. --- BUGS.md | 17 +++++------------ CLAUDE.md | 9 +++++---- app/bets/service.py | 2 +- app/rounds/scheduler.py | 9 ++++++--- app/static/i18n.js | 7 +++++++ app/wallet/psbt_builder.py | 32 +++++++++++++++++++++++++++++--- app/withdrawals/service.py | 2 +- tests/unit/test_bets.py | 30 ++++++++++++++++++++++++++++++ tests/unit/test_psbt_builder.py | 17 +++++++++++++++++ 9 files changed, 101 insertions(+), 24 deletions(-) diff --git a/BUGS.md b/BUGS.md index d0a765b..602cd15 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-48 … B-49. B-25 through B-47 are fixed (see "Previously -fixed" below) — no Critical-, High- or Medium-severity finding remains open; the remaining 2 are +7 medium, 8 low), listed below as B-49. B-25 through B-48 are fixed (see "Previously +fixed" below) — no Critical-, High- or Medium-severity finding remains open; the remaining 1 is 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 twenty-three fixes so far -brought the suite from 139 to 248). +by existing coverage — every fix lands with a regression test (the twenty-four fixes so far +brought the suite from 139 to 251). 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,14 +18,6 @@ admin auth, single-process assumptions, no user-facing history, etc.) are docume ## Low / hygiene -### B-48 — No cap on input count in `select_utxos` - -A user with hundreds of small UTXOs builds a huge transaction whose fee — deducted from the bet -amount — materially erodes their contribution to the pool, and it can exceed standardness -limits. -**Fix:** cap the selected inputs (e.g. 50) and fail with a translatable error suggesting a -consolidation, or consolidate the address automatically when the count crosses a threshold. - ### B-49 — Rollback paths do not publish an SSE update `bets/service.py:_release_failed_bet` and `withdrawals/service.py:_release_failed_withdrawal` @@ -50,6 +42,7 @@ already does. - **B-44** — README's Quick start documented a bare `uvicorn --reload` workflow, and `docs/running-the-server.md` still had a matching "Locale / venv" section, both contradicting CLAUDE.md's Docker-only policy - **B-45** — `/admin/rounds`/`/admin/audit-log`'s `limit` had no bounds (`-1` means "everything" on SQLite), and `/admin/pending-transactions` had no limit or status filter at all - **B-46** — `secrets.compare_digest` on a `str` raises `TypeError` on non-ASCII input, turning an invalid admin token with non-ASCII characters into a 500 instead of a 403 +- **B-48** — `select_utxos` had no cap on input count, so a fragmented address built an ever-larger transaction whose fee (deducted from the amount being moved) ate into the bet or withdrawal, up to the point of being non-standard - **B-47** — `raw_tx_hex` and `payload_json` were unbounded `String` columns (`VARCHAR` with no length) — fine on SQLite/PostgreSQL, rejected by backends like MySQL that require a length - **B-32** — an RBF bump could retry forever below BIP125's relay-mandated minimum fee delta, with no ceiling on the fee rate either - **B-33** — `POST /auth/login` had no rate limiting, so a password could be brute-forced against an enumerable username list diff --git a/CLAUDE.md b/CLAUDE.md index b95d1e9..3838ddb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,11 +8,11 @@ The user communicates in Italian in chat — reply to them in Italian. Everythin ## Project status -All 10 stages of the original build order are code-complete and unit-tested — 185 tests, all under `tests/unit/` (`tests/integration/` is an empty package). Beyond them: Docker + Caddy deployment, admin dashboard (`/admin`), static test UI (`/`), pending-inclusive balance display, an SSE push channel layered over the original polling, self-service password change + admin password reset, and the reconciliation/corroboration machinery below. +All 10 stages of the original build order are code-complete and unit-tested — 251 tests, all under `tests/unit/` (`tests/integration/` is an empty package). Beyond them: Docker + Caddy deployment, admin dashboard (`/admin`), static test UI (`/`), pending-inclusive balance display, an SSE push channel layered over the original polling, self-service password change + admin password reset, and the reconciliation/corroboration machinery below. Verified on mainnet with real money: registration + address derivation, deposit crediting (1-conf), a real 10 PLM bet (broadcast → confirmed → change credited back), and one full round cycle (close → draw on a real block hash → 70/30 payout with sat math checked against the broadcast tx → confirmation → close → next round auto-opened). **Withdrawal and the RBF bump path have never been exercised against a live broadcast** — unit-tested only. -**Read [BUGS.md](BUGS.md) before trusting any behaviour here.** Two audits: 2026-07-26 found 24 bugs (5 critical), all fixed; 2026-07-27 found 25 more (B-25 … B-49), of which **2 are still open** — no Critical, High or Medium remains, only Low/hygiene: no cap on input count in `select_utxos` (B-48) and rollback paths not publishing an SSE update (B-49). BUGS.md is the live open list with a proposed fix per finding; "Known gaps" at the end of this file is for limitations accepted **by design** instead. Don't fix a BUGS.md item silently as a side effect of other work — each fix lands with its own regression test. +**Read [BUGS.md](BUGS.md) before trusting any behaviour here.** Two audits: 2026-07-26 found 24 bugs (5 critical), all fixed; 2026-07-27 found 25 more (B-25 … B-49), of which **1 is still open** — no Critical, High or Medium remains, only Low/hygiene: rollback paths not publishing an SSE update (B-49). BUGS.md is the live open list with a proposed fix per finding; "Known gaps" at the end of this file is for limitations accepted **by design** instead. Don't fix a BUGS.md item silently as a side effect of other work — each fix lands with its own regression test. Before writing code, read the "Architecture" section below in full plus the diagrams in [flowchart/](flowchart/): [platform-overview.mmd](flowchart/platform-overview.mmd) (the 5-phase flow) and [round-lifecycle.mmd](flowchart/round-lifecycle.mmd) (the round/draw lifecycle). Every node **and edge label** (conditions, retries, loops) is a behaviour that must be implemented as described. Regenerate the companion PDFs with `flowchart/render-pdf.sh .mmd` after editing either. @@ -33,7 +33,7 @@ PYTHONPATH=. python scripts/decrypt_master_key.py # ops recovery: decrypt+pr PYTHONPATH=. python scripts/encrypt_master_key.py # ops bootstrap: import an externally-generated xprv (--overwrite to replace) PYTHONPATH=. python scripts/electrum_smoke_test.py # manual check: connect, handshake, subscribe to headers, print the tip -python -m pytest # all 185 tests +python -m pytest # all 251 tests python -m pytest tests/unit/test_hd.py # one file python -m pytest tests/unit/test_hd.py::test_derivation_is_deterministic # one test ``` @@ -91,6 +91,7 @@ Source of truth: the `PalladiumWallet` repo — [ChainProfiles.cs](../PalladiumW | Min deposit | none | — | | Min password length | 8 | `auth/security.py:MIN_PASSWORD_LENGTH` | | Confirmations, every tx kind | **1** | hardcoded in `tx/confirmation.py` | +| Max inputs per tx | 50 (`MAX_TX_INPUTS`, B-48) — over it the build fails with `too_many_inputs`, it never spends more | hardcoded in `wallet/psbt_builder.py` | `GET /rounds/current`'s `jackpot_sats` is the winner's 70% share, not the whole pool, and the pool is summed from the participants' actual `bet_amount_sats` (each already net of its own bet fee) rather than `count × current bet amount` — editing the bet amount mid-round must not move an in-progress round's advertised jackpot (B-11). @@ -233,7 +234,7 @@ Explicit design choices, not derivable from any single file — respect them: ## Known gaps / TODO -Accepted **by design**. For actual bugs see [BUGS.md](BUGS.md) (2 open) — not duplicated here. +Accepted **by design**. For actual bugs see [BUGS.md](BUGS.md) (1 open) — not duplicated here. - **`drawing` doesn't resume after a restart.** `_tick()` handles `open`, `closing` and `paying_out` (the last via `_retry_payout_if_due`); nothing re-enters `_wait_for_next_block` after a crash. That wait is unbounded by design (the draw's entropy genuinely depends on a future block) but no longer silent — past `_DRAW_STALL_THRESHOLD_SECONDS` it logs progress and writes a `draw_stalled` audit entry, and `GET /rounds/current`'s `draw_waiting_since` surfaces it live (B-36). Restart-resumption itself remains the last prerequisite for running unattended. - **RBF handles one shape only**: a single change output, back to the tx's own sender, big enough to absorb the increase. No extra-input fallback — an exact-amount tx or too-small change raises `RbfError`. Not permanent, though: an unbumpable tx that never confirms is eventually abandoned and its UTXOs released. diff --git a/app/bets/service.py b/app/bets/service.py index 572f7ef..28070d9 100644 --- a/app/bets/service.py +++ b/app/bets/service.py @@ -62,7 +62,7 @@ async def place_bet(session: AsyncSession, client: ElectrumClient, user: User) - fee_rate_sat_vb=config.fee_rate_sat_vb, ) except InsufficientFundsError as exc: - raise BetError(exc.code, str(exc)) from exc + raise BetError(exc.code, str(exc), **exc.params) from exc # --- Phase 1: record the intent, *then* broadcast (B-08) -------------------- # Broadcasting first meant a failure (or a crash) between the broadcast and the diff --git a/app/rounds/scheduler.py b/app/rounds/scheduler.py index f7f93d4..28f5647 100644 --- a/app/rounds/scheduler.py +++ b/app/rounds/scheduler.py @@ -364,9 +364,12 @@ class RoundScheduler: change_address=pool_address, fee_rate_sat_vb=fee_rate, ) - except InsufficientFundsError: - logger.exception("round %s payout failed: insufficient pool UTXOs", round_id) - await self._log_payout_failure(round_id, winner_user_id, "insufficient pool UTXOs") + except InsufficientFundsError as exc: + # Includes the B-48 "too_many_inputs" case: the pool holds enough, but spread + # over more UTXOs than one transaction may spend, so /admin has to say which. + reason = "insufficient pool UTXOs" if exc.code == "insufficient_balance" else exc.code + logger.exception("round %s payout failed: %s", round_id, reason) + await self._log_payout_failure(round_id, winner_user_id, reason) return except Exception: # Anything else — a malformed fee_address (EmbitError) or similar. This diff --git a/app/static/i18n.js b/app/static/i18n.js index ae25689..38fa014 100644 --- a/app/static/i18n.js +++ b/app/static/i18n.js @@ -135,6 +135,7 @@ const TRANSLATIONS = { 'error.invalid_amount': 'Enter an amount greater than zero.', 'error.broadcast_failed': 'The network refused the transaction. Please try again shortly.', 'error.amount_below_dust_limit': 'The amount is too small to be sent.', + 'error.too_many_inputs': 'Your balance is split across too many small deposits to be spent in a single transaction (max {max_inputs}). Please contact support to consolidate it.', 'error.withdrawal_to_own_address': 'That is your own deposit address — withdraw to an external wallet.', 'error.internal_error': 'Unexpected server error. Please try again shortly.', 'error.guide_unavailable': 'The guide is not available right now.', @@ -274,6 +275,7 @@ const TRANSLATIONS = { 'error.invalid_amount': 'Inserisci un importo maggiore di zero.', 'error.broadcast_failed': 'La rete ha rifiutato la transazione. Riprova tra poco.', 'error.amount_below_dust_limit': "L'importo è troppo basso per essere inviato.", + 'error.too_many_inputs': 'Il tuo saldo è suddiviso in troppi piccoli depositi per essere speso in una sola transazione (max {max_inputs}). Contatta l\'assistenza per consolidarlo.', 'error.withdrawal_to_own_address': 'Questo è il tuo indirizzo di deposito — preleva verso un wallet esterno.', 'error.internal_error': 'Errore inatteso del server. Riprova tra poco.', 'error.guide_unavailable': 'La guida non è disponibile in questo momento.', @@ -413,6 +415,7 @@ const TRANSLATIONS = { 'error.invalid_amount': 'Introduce un importe mayor que cero.', 'error.broadcast_failed': 'La red rechazó la transacción. Inténtalo de nuevo en un momento.', 'error.amount_below_dust_limit': 'El importe es demasiado pequeño para enviarse.', + 'error.too_many_inputs': 'Tu saldo está repartido en demasiados depósitos pequeños para gastarse en una sola transacción (máx. {max_inputs}). Contacta con soporte para consolidarlo.', 'error.withdrawal_to_own_address': 'Esa es tu propia dirección de depósito — retira a una cartera externa.', 'error.internal_error': 'Error inesperado del servidor. Inténtalo de nuevo en un momento.', 'error.guide_unavailable': 'La guía no está disponible en este momento.', @@ -552,6 +555,7 @@ const TRANSLATIONS = { 'error.invalid_amount': 'Saisissez un montant supérieur à zéro.', 'error.broadcast_failed': 'Le réseau a refusé la transaction. Veuillez réessayer dans un instant.', 'error.amount_below_dust_limit': "Le montant est trop faible pour être envoyé.", + 'error.too_many_inputs': 'Votre solde est réparti sur trop de petits dépôts pour être dépensé en une seule transaction (max {max_inputs}). Contactez le support pour le consolider.', 'error.withdrawal_to_own_address': "C'est votre propre adresse de dépôt — retirez vers un portefeuille externe.", 'error.internal_error': 'Erreur inattendue du serveur. Veuillez réessayer dans un instant.', 'error.guide_unavailable': "Le guide n'est pas disponible pour le moment.", @@ -691,6 +695,7 @@ const TRANSLATIONS = { 'error.invalid_amount': 'Gib einen Betrag größer als null ein.', 'error.broadcast_failed': 'Das Netzwerk hat die Transaktion abgelehnt. Bitte versuche es in Kürze erneut.', 'error.amount_below_dust_limit': 'Der Betrag ist zu klein, um gesendet zu werden.', + 'error.too_many_inputs': 'Ihr Guthaben ist auf zu viele kleine Einzahlungen verteilt, um in einer einzigen Transaktion ausgegeben zu werden (max. {max_inputs}). Bitte wenden Sie sich an den Support, um es zusammenzufassen.', 'error.withdrawal_to_own_address': 'Das ist deine eigene Einzahlungsadresse — zahle auf eine externe Wallet aus.', 'error.internal_error': 'Unerwarteter Serverfehler. Bitte versuche es in Kürze erneut.', 'error.guide_unavailable': 'Die Anleitung ist derzeit nicht verfügbar.', @@ -830,6 +835,7 @@ const TRANSLATIONS = { 'error.invalid_amount': 'Введите сумму больше нуля.', 'error.broadcast_failed': 'Сеть отклонила транзакцию. Попробуйте ещё раз через минуту.', 'error.amount_below_dust_limit': 'Сумма слишком мала для отправки.', + 'error.too_many_inputs': 'Ваш баланс разбит на слишком много мелких депозитов, чтобы потратить его одной транзакцией (максимум {max_inputs}). Обратитесь в поддержку для консолидации.', 'error.withdrawal_to_own_address': 'Это ваш собственный адрес для депозита — выводите на внешний кошелёк.', 'error.internal_error': 'Непредвиденная ошибка сервера. Попробуйте ещё раз через минуту.', 'error.guide_unavailable': 'Руководство сейчас недоступно.', @@ -969,6 +975,7 @@ const TRANSLATIONS = { 'error.invalid_amount': '请输入大于零的金额。', 'error.broadcast_failed': '网络拒绝了该交易，请稍后重试。', 'error.amount_below_dust_limit': '金额过小，无法发送。', + 'error.too_many_inputs': '您的余额分散在过多的小额存款中，无法在一笔交易中花费（最多 {max_inputs} 笔）。请联系客服进行归集。', 'error.withdrawal_to_own_address': '这是你自己的充值地址 — 请提现到外部钱包。', 'error.internal_error': '服务器发生意外错误，请稍后重试。', 'error.guide_unavailable': '指南当前不可用。', diff --git a/app/wallet/psbt_builder.py b/app/wallet/psbt_builder.py index b822617..26f89f7 100644 --- a/app/wallet/psbt_builder.py +++ b/app/wallet/psbt_builder.py @@ -33,15 +33,30 @@ DUST_LIMIT_SATS = 294 # eating further and further into the sender's change with no limit. MAX_FEE_RATE_SAT_VB = 10_000 +# Ceiling on how many UTXOs one transaction may spend (B-48). Every extra input costs +# ~68 vbytes of fee, and that fee comes out of the amount being moved — so an address +# fragmented into hundreds of small deposits would silently erode its own bet (shrinking +# the user's share of the pool) or withdrawal, and past a few hundred inputs the tx also +# stops being standard and gets refused at broadcast. Failing the build with a +# translatable error is the honest outcome; consolidating the address is the way out. +MAX_TX_INPUTS = 50 + class InsufficientFundsError(Exception): """`code` is the machine-readable identifier the API layer forwards to the client so it can translate the failure (see app/api/errors.py); the message - itself stays English.""" + itself stays English, and `params` carries the values it interpolates so the + translation can place them wherever its own grammar needs them.""" - def __init__(self, message: str, code: str = "insufficient_balance") -> None: + def __init__( + self, + message: str, + code: str = "insufficient_balance", + **params: int | str, + ) -> None: super().__init__(message) self.code = code + self.params = params @dataclass @@ -68,11 +83,22 @@ def estimate_vsize(n_inputs: int, n_outputs: int) -> int: def select_utxos(utxos: list[Utxo], target_sats: int) -> tuple[list[Utxo], int]: """Greedily select UTXOs (largest first, to minimize input count) covering target_sats — the amount deducted from the sender's balance. The fee is paid - out of target_sats (see build_signed_transaction), not added on top of it.""" + out of target_sats (see build_signed_transaction), not added on top of it. + + At most MAX_TX_INPUTS are ever selected (B-48): if the largest MAX_TX_INPUTS + UTXOs don't cover the target, the balance is there but too fragmented to spend + in one transaction, which is a different failure from having no funds at all + and gets its own code.""" ordered = sorted(utxos, key=lambda u: u.amount_sats, reverse=True) selected: list[Utxo] = [] total = 0 for utxo in ordered: + if len(selected) == MAX_TX_INPUTS: + raise InsufficientFundsError( + f"balance too fragmented: more than {MAX_TX_INPUTS} inputs would be needed", + code="too_many_inputs", + max_inputs=MAX_TX_INPUTS, + ) selected.append(utxo) total += utxo.amount_sats if total >= target_sats: diff --git a/app/withdrawals/service.py b/app/withdrawals/service.py index 84a7b6c..7cf8678 100644 --- a/app/withdrawals/service.py +++ b/app/withdrawals/service.py @@ -89,7 +89,7 @@ async def request_withdrawal( fee_rate_sat_vb=config.fee_rate_sat_vb, ) except InsufficientFundsError as exc: - raise WithdrawalError(exc.code, str(exc)) from exc + raise WithdrawalError(exc.code, str(exc), **exc.params) from exc # Persist the intent before broadcasting, and only promote the rows once the # network has accepted the tx — same two-phase shape as place_bet (B-08). diff --git a/tests/unit/test_bets.py b/tests/unit/test_bets.py index aa2886a..8567b43 100644 --- a/tests/unit/test_bets.py +++ b/tests/unit/test_bets.py @@ -10,6 +10,7 @@ from app.db.base import Base from app.db.models import AuditLog, PendingTransaction, Round, RoundConfig, RoundParticipant, User, UtxoEvent from app.rounds.service import open_new_round_if_needed from app.wallet.hd import derive_user_address +from app.wallet.psbt_builder import MAX_TX_INPUTS class FakeElectrumClient: @@ -91,6 +92,35 @@ async def test_place_bet_rejects_insufficient_balance(session_factory): await place_bet(session, client, user) +async def test_place_bet_reports_a_too_fragmented_balance_distinctly(session_factory): # B-48 + # 100 x 0.15 PLM = 15 PLM, plenty for a 10 PLM bet, but the 50 largest inputs + # only add up to 7.5 PLM — so the build must fail with its own code, not with + # the "you have no funds" one, and must carry the cap for the translation. + user_id = await _make_funded_user(session_factory, 20, 15_000_000) + async with session_factory() as session: + for i in range(99): + session.add( + UtxoEvent( + user_id=user_id, + txid=f"{i:064x}", + vout=0, + amount_sats=15_000_000, + confirmed_height=100, + ) + ) + await session.commit() + client = FakeElectrumClient() + + async with session_factory() as session: + user = await session.get(User, user_id) + with pytest.raises(BetError) as excinfo: + await place_bet(session, client, user) + + assert excinfo.value.code == "too_many_inputs" + assert excinfo.value.params == {"max_inputs": MAX_TX_INPUTS} + assert not client.broadcasted + + async def test_place_bet_rejects_second_bet_same_round(session_factory): user_id = await _make_funded_user(session_factory, 2, 3_000_000_000) client = FakeElectrumClient() diff --git a/tests/unit/test_psbt_builder.py b/tests/unit/test_psbt_builder.py index fcd0686..4f8eb6e 100644 --- a/tests/unit/test_psbt_builder.py +++ b/tests/unit/test_psbt_builder.py @@ -5,6 +5,7 @@ from embit.transaction import Transaction from app.wallet.plm_network import PLM_MAINNET from app.wallet.psbt_builder import ( + MAX_TX_INPUTS, InsufficientFundsError, Utxo, build_signed_transaction, @@ -36,6 +37,22 @@ def test_select_utxos_raises_when_insufficient(): select_utxos(utxos, target_sats=10_000_000) +def test_select_utxos_never_exceeds_the_input_cap(): # B-48 + # 200 dust-ish UTXOs that together cover the target, but only past the cap. + utxos = [Utxo(f"{i:064x}", 0, 100_000) for i in range(200)] + with pytest.raises(InsufficientFundsError) as excinfo: + select_utxos(utxos, target_sats=100_000 * MAX_TX_INPUTS + 1) + assert excinfo.value.code == "too_many_inputs" + assert excinfo.value.params == {"max_inputs": MAX_TX_INPUTS} + + +def test_select_utxos_allows_exactly_the_input_cap(): + utxos = [Utxo(f"{i:064x}", 0, 100_000) for i in range(200)] + selected, total = select_utxos(utxos, target_sats=100_000 * MAX_TX_INPUTS) + assert len(selected) == MAX_TX_INPUTS + assert total == 100_000 * MAX_TX_INPUTS + + def test_build_signed_transaction_deducts_fee_from_amount_not_change(): signer = _key(1) from_script = script.p2wpkh(signer.to_public())