From 8dd913ec599e84dd9e0454c5bd335a07d325b5d1 Mon Sep 17 00:00:00 2001 From: Davide Grilli Date: Mon, 3 Aug 2026 23:36:55 +0200 Subject: [PATCH] Always keep a change output, so every tx stays fee-bumpable (B-62) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The max-amount checkbox sends amount_sats == the whole confirmed balance, so change came out at 0, the change output was dropped, and the transaction had a single output. bump_fee has nothing to shrink there: it raised RbfError every 30s until the reconciler abandoned the row six hours later. The RBF single-change-output limitation was a documented gap, but the UI made it the *default* withdrawal path. The extra-input fallback would not have helped this case: a transaction moving the entire balance already spends every UTXO the sender has. So the fix is at build time — build_signed_transaction never produces a change output below DUST_LIMIT_SATS, and never folds it into the fee either: - withdrawals pass reduce_amount_to_keep_change=True and move a dust limit less. The fee already comes out of the withdrawn amount by design, so this is the same rule applied a little harder, and Withdrawal.amount_requested_sats vs amount_sent_sats already existed to record the difference. - bets don't: the bet is a fixed price that can't be quietly reduced. A balance exactly equal to the bet is refused with balance_leaves_no_change (translated into all 7 languages, carrying required_extra_sats), which turns "a user's balance must never exactly equal the bet" from a documented assumption into an enforced one — and stops an unbumpable bet from holding a round open until the reconciler gives up on it. bump_fee's no-change guard stays: a single-output tx broadcast before this change can still be pending across the deploy, and it must fail loudly rather than start shrinking a recipient's output. Its test now hand-builds that shape, precisely because the builder no longer will. Co-Authored-By: Claude Opus 5 --- BUGS.md | 17 -------- CLAUDE.md | 10 ++--- app/static/i18n.js | 7 ++++ app/wallet/psbt_builder.py | 41 +++++++++++++++---- app/withdrawals/service.py | 7 ++++ docs/guida-utente.md | 6 +++ tests/unit/test_broadcast.py | 24 ++++++----- tests/unit/test_psbt_builder.py | 64 +++++++++++++++++++++++------ tests/unit/test_withdrawals.py | 72 +++++++++++++++++++++++++++++++++ 9 files changed, 197 insertions(+), 51 deletions(-) diff --git a/BUGS.md b/BUGS.md index 393d6e2..175ea7b 100644 --- a/BUGS.md +++ b/BUGS.md @@ -40,23 +40,6 @@ remains the last prerequisite for running unattended. ## Medium — correctness and robustness -### B-62 — "withdraw the full amount" reliably produces an unbumpable transaction - -`app/static/app.js:798-807`, `app/wallet/psbt_builder.py:138-141`, -`app/tx/broadcast.py:150-152`. - -The max-amount checkbox sends `amount_sats == myBalanceSats == total_in`, so -`change == 0`, the change output is dropped, and the tx has a single output. `bump_fee` -then finds no change output to absorb the increase and raises `RbfError` every 30 s -until the reconciler abandons the row six hours later. The RBF single-change-output -limitation is a documented gap, but the UI makes it the *default* withdrawal path -rather than a corner case (the same applies to an exact-amount bet). - -Fix directions: leave a change output above `DUST_LIMIT_SATS` when the requested -amount would consume the whole input total (i.e. reserve a little), or warn in the -UI that a full-balance withdrawal cannot be fee-bumped, or implement the -extra-input RBF fallback. - ### B-63 — `tip_height == 0` window right after connecting can seed a draw from a pre-close block `app/electrum/listener.py:160-167`, `app/rounds/scheduler.py:153`. diff --git a/CLAUDE.md b/CLAUDE.md index 09a17f9..139e923 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,7 +8,7 @@ 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 — 309 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 — 313 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. @@ -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 309 tests +python -m pytest # all 313 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 ``` @@ -152,7 +152,7 @@ Diagrams: [platform-overview.mmd](flowchart/platform-overview.mmd), [round-lifec **DEP** — the listener subscribes to the user's scripthash; balance is credited after **1 confirmation** and only once the other servers corroborate the outpoint and its amount (B-59), with the 1-conf reorg risk knowingly accepted and no rollback logic. `deposits/service.py` also detects UTXOs that vanished (spent outside the platform — corroborated per B-29 first) and *reinstates* ones that reappear. -**PLAY** — fixed cost, **at most one active bet per user**, and **at most `MAX_PARTICIPANTS_PER_ROUND` (400) players per round** — past that the bet is refused with `round_full` and the player waits for the next round (B-52: the payout must spend one pool UTXO per bet, so a round is only ever allowed to grow to what a single payout tx can drain). PSBT user-address → pool-address, always with a **change output back to the same user address** (a user's balance must never exactly equal the bet). Fee ~1 sat/vB, **deducted from the bet amount**. No confirmation within the timeout → RBF bump and rebroadcast. +**PLAY** — fixed cost, **at most one active bet per user**, and **at most `MAX_PARTICIPANTS_PER_ROUND` (400) players per round** — past that the bet is refused with `round_full` and the player waits for the next round (B-52: the payout must spend one pool UTXO per bet, so a round is only ever allowed to grow to what a single payout tx can drain). PSBT user-address → pool-address, always with a **change output back to the same user address** of at least `DUST_LIMIT_SATS` — a user's balance must never exactly equal the bet, and since B-62 that's enforced (`balance_leaves_no_change`) rather than assumed. Fee ~1 sat/vB, **deducted from the bet amount**. No confirmation within the timeout → RBF bump and rebroadcast. **DRAW** — configurable timer (default 600s): - *Bet cutoff is the round's own deadline* (`round_deadline` = `opened_at + Round.duration_seconds`, the value snapshotted at open time — B-61), **not** the DB status: `place_bet` calls `rounds/service.round_accepts_bets`, which rejects once the deadline passes even while `status` is still `"open"` (the 5s scheduler tick can lag behind it). Once a round leaves `open`, no new bets either, and no new round opens until this one is fully `closed`. The deadline is checked twice — on arrival and again after the transaction is built — and the participant row is then committed behind a **compare-and-set on the round row** (`UPDATE rounds ... WHERE status = 'open'`, B-53): the scheduler flips `open` → `closing` in a transaction of its own and only counts in-flight bets afterwards, so without the CAS a bet could commit in between, be excluded from the draw (only `confirmed` participants are drawn) and still have its sats land in the pool with no refund path. Its mirror image on the scheduler side is `_close_and_draw` re-counting in-flight bets in the same session it snapshots the participants from. @@ -162,7 +162,7 @@ Diagrams: [platform-overview.mmd](flowchart/platform-overview.mmd), [round-lifec - *UI, two independent layers.* A generic phase box ("Pagamento al vincitore in corso…") shows to **every** viewer for the whole closing/drawing/paying_out span — pure cosmetic text driven by `status`. **Additively**, a personalized "Hai vinto!/Non hai vinto" box appears only where `user_played` is true (computed via `get_optional_user`, since the endpoint is reachable logged-out) — nobody else has anything to reveal. - *Reveal timing.* Delayed by at least `draw_animation_seconds`, anchored to the server's `closes_at` so a reload can't reset the countdown, and decoupled from the real (~block-time) wait for `winner_user_id`. Once revealed it's persisted in `localStorage.plm_persisted_result`, surviving the move to `closed` — at which point `get_active_round` stops returning the round and `winner_user_id` disappears from `GET /rounds/current`. `GET /users/me/last-round-result` is the durable DB-backed backstop for a device that missed the live window entirely. Full logic: `refreshRound`/`checkLastRoundResult` in `app/static/app.js`. -**WITHDRAW** — the only way out to an external address: PSBT user-address → external + change back to the user, fee deducted from the withdrawn amount, same RBF pattern. +**WITHDRAW** — the only way out to an external address: PSBT user-address → external + change back to the user, fee deducted from the withdrawn amount, same RBF pattern. A full-balance withdrawal moves `balance - DUST_LIMIT_SATS` so the change output (and with it the ability to fee-bump) always exists — `Withdrawal.amount_requested_sats` vs `amount_sent_sats` is what records the difference (B-62). PLAY and WITHDRAW share a **per-user lock** (`tx/locks.py`): a bet-build and a withdrawal-build can never be in flight at once, since both spend the same UTXO set. @@ -241,7 +241,7 @@ Explicit design choices, not derivable from any single file — respect them: Accepted **by design** — distinct from the audit findings above (all fixed), which are 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. +- **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, and none would help the case that used to hurt (an amount equal to the whole input total leaves no other UTXO to add) — which is why `build_signed_transaction` now guarantees a change output of at least `DUST_LIMIT_SATS` instead (B-62): a withdrawal for the full balance moves a dust limit less, a bet from a balance equal to the bet is refused with `balance_leaves_no_change`. What's left is a bump whose *delta* exceeds an otherwise-fine change output, which still raises `RbfError`; that tx is eventually abandoned and its UTXOs released. - **Withdrawal and RBF bump are unit-tested but never live-broadcast** (failure and rollback branches included — still not a real network). - **No user-facing history.** `GET /users/me/last-round-result` covers exactly one case (the reveal backstop above). Admin has `/admin/rounds`, `/admin/pending-transactions`, `/admin/audit-log`; a user has no equivalent — a failed withdrawal leaves a `failed` row they can never see, which argues for closing this. - **Admin auth is one shared bearer token** (`ADMIN_TOKEN`) with no per-admin identity: `audit_log` records *what* changed (config edits as `config_updated`, with before/after) but never *who* did it. It gates the user list, privkey export, password resets and history, so a leak is high-blast-radius. diff --git a/app/static/i18n.js b/app/static/i18n.js index fd01911..b21cbfb 100644 --- a/app/static/i18n.js +++ b/app/static/i18n.js @@ -142,6 +142,7 @@ const TRANSLATIONS = { 'error.round_full': 'This round has reached its maximum of {max_participants} players — wait for the next one, it opens shortly.', 'error.already_betting': 'You already have an active bet in the current round.', 'error.insufficient_balance': 'Insufficient balance.', + 'error.balance_leaves_no_change': 'Your balance is too close to the bet amount: {required_extra_plm} PLM more are needed so the transaction keeps a change output and can be fee-bumped if the network is slow.', 'error.balance_pending_confirmation': 'You have {pending_plm} PLM pending confirmation — it is not spendable yet.', 'error.amount_below_network_fee': 'The amount is too small to cover the network fee.', 'error.invalid_address': 'Not a valid PLM address (it must start with plm1q…).', @@ -303,6 +304,7 @@ const TRANSLATIONS = { 'error.round_full': 'Questo round ha raggiunto il massimo di {max_participants} giocatori — aspetta il prossimo, si apre tra poco.', 'error.already_betting': 'Hai già una bet attiva nel round corrente.', 'error.insufficient_balance': 'Saldo insufficiente.', + 'error.balance_leaves_no_change': 'Il tuo saldo è troppo vicino all\'importo della giocata: servono {required_extra_plm} PLM in più perché la transazione mantenga un resto e possa essere rilanciata con fee più alta se la rete è lenta.', 'error.balance_pending_confirmation': 'Hai {pending_plm} PLM in attesa di conferma — non ancora disponibili per la spesa.', 'error.amount_below_network_fee': "L'importo è troppo basso per coprire la fee di rete.", 'error.invalid_address': 'Indirizzo PLM non valido (deve iniziare con plm1q…).', @@ -464,6 +466,7 @@ const TRANSLATIONS = { 'error.round_full': 'Esta ronda ha alcanzado su máximo de {max_participants} jugadores: espera la siguiente, se abre en breve.', 'error.already_betting': 'Ya tienes una apuesta activa en la ronda actual.', 'error.insufficient_balance': 'Saldo insuficiente.', + 'error.balance_leaves_no_change': 'Tu saldo está demasiado cerca del importe de la apuesta: hacen falta {required_extra_plm} PLM más para que la transacción conserve un cambio y pueda relanzarse con más comisión si la red va lenta.', 'error.balance_pending_confirmation': 'Tienes {pending_plm} PLM pendientes de confirmación — todavía no se pueden gastar.', 'error.amount_below_network_fee': 'El importe es demasiado pequeño para cubrir la comisión de red.', 'error.invalid_address': 'Dirección PLM no válida (debe empezar por plm1q…).', @@ -625,6 +628,7 @@ const TRANSLATIONS = { 'error.round_full': 'Ce tour a atteint son maximum de {max_participants} joueurs — attendez le prochain, il ouvre dans un instant.', 'error.already_betting': 'Vous avez déjà une mise active dans le round en cours.', 'error.insufficient_balance': 'Solde insuffisant.', + 'error.balance_leaves_no_change': 'Votre solde est trop proche du montant de la mise : il faut {required_extra_plm} PLM de plus pour que la transaction garde une monnaie de rendu et puisse être relancée avec des frais plus élevés si le réseau est lent.', 'error.balance_pending_confirmation': 'Vous avez {pending_plm} PLM en attente de confirmation — pas encore disponibles.', 'error.amount_below_network_fee': 'Le montant est trop faible pour couvrir les frais de réseau.', 'error.invalid_address': 'Adresse PLM invalide (elle doit commencer par plm1q…).', @@ -786,6 +790,7 @@ const TRANSLATIONS = { 'error.round_full': 'Diese Runde hat ihr Maximum von {max_participants} Spielern erreicht — warten Sie auf die nächste, sie beginnt in Kürze.', 'error.already_betting': 'Du hast bereits eine aktive Wette in der laufenden Runde.', 'error.insufficient_balance': 'Nicht genügend Guthaben.', + 'error.balance_leaves_no_change': 'Ihr Guthaben liegt zu nah am Einsatzbetrag: Es werden {required_extra_plm} PLM mehr benötigt, damit die Transaktion einen Wechselgeld-Ausgang behält und bei langsamem Netz mit höherer Gebühr neu gesendet werden kann.', 'error.balance_pending_confirmation': 'Sie haben {pending_plm} PLM, die noch auf Bestätigung warten — noch nicht verfügbar.', 'error.amount_below_network_fee': 'Der Betrag ist zu klein, um die Netzwerkgebühr zu decken.', 'error.invalid_address': 'Keine gültige PLM-Adresse (sie muss mit plm1q… beginnen).', @@ -947,6 +952,7 @@ const TRANSLATIONS = { 'error.round_full': 'В этом раунде достигнут максимум участников ({max_participants}) — дождитесь следующего, он начнётся совсем скоро.', 'error.already_betting': 'У вас уже есть активная ставка в текущем раунде.', 'error.insufficient_balance': 'Недостаточно средств.', + 'error.balance_leaves_no_change': 'Ваш баланс слишком близок к сумме ставки: нужно ещё {required_extra_plm} PLM, чтобы в транзакции остался выход сдачи и её можно было переотправить с более высокой комиссией, если сеть работает медленно.', 'error.balance_pending_confirmation': 'У вас есть {pending_plm} PLM, ожидающих подтверждения — они пока недоступны для расходования.', 'error.amount_below_network_fee': 'Сумма слишком мала, чтобы покрыть комиссию сети.', 'error.invalid_address': 'Некорректный адрес PLM (он должен начинаться с plm1q…).', @@ -1108,6 +1114,7 @@ const TRANSLATIONS = { 'error.round_full': '本轮已达到 {max_participants} 名玩家的上限 —— 请等待下一轮，很快就会开始。', 'error.already_betting': '你在当前回合已有一笔有效下注。', 'error.insufficient_balance': '余额不足。', + 'error.balance_leaves_no_change': '您的余额与投注金额过于接近：还需要 {required_extra_plm} PLM，交易才能保留找零输出，并在网络拥堵时提高手续费重新广播。', 'error.balance_pending_confirmation': '您有 {pending_plm} PLM 待确认 —— 尚不可用于支出。', 'error.amount_below_network_fee': '金额太小，不足以支付网络手续费。', 'error.invalid_address': 'PLM 地址无效（必须以 plm1q… 开头）。', diff --git a/app/wallet/psbt_builder.py b/app/wallet/psbt_builder.py index a93ade8..219ed96 100644 --- a/app/wallet/psbt_builder.py +++ b/app/wallet/psbt_builder.py @@ -146,6 +146,7 @@ def build_signed_transaction( amount_sats: int, change_address: str, fee_rate_sat_vb: int, + reduce_amount_to_keep_change: bool = False, ) -> BuiltTransaction: """Build, sign and finalize a single-recipient P2WPKH transaction with change back to change_address. @@ -155,21 +156,47 @@ def build_signed_transaction( spec everywhere a single-recipient tx is used (bet, withdrawal): "fee deducted from the amount being moved", not paid on top by the sender. - A change amount below DUST_LIMIT_SATS is dropped and left to the fee — paying it - back to ourselves would produce an unrelayable transaction. The fee estimate - already assumes two outputs, so dropping one never underpays. + B-62: the transaction always keeps a change output of at least DUST_LIMIT_SATS. + Change used to be folded into the fee whenever it came out below the dust limit, + which for an amount equal to the whole input total (the UI's "withdraw + everything" checkbox, or a bet from a balance exactly equal to the bet amount) + produced a single-output transaction — and `tx/broadcast.py:bump_fee` has nothing + to shrink there, so it raised RbfError every 30s until the reconciler abandoned + the row hours later. Adding inputs instead is no answer for this case in + particular: the transaction already spends every UTXO the sender has. + + What happens when the change would be too small depends on who's asking, hence + `reduce_amount_to_keep_change`: + + - withdrawals pass True — the amount moved is reduced just enough to leave a + dust-limit change output. The fee already comes out of the withdrawn amount by + design, so this is the same rule applied a little harder, and the caller + records what was actually sent (`Withdrawal.amount_sent_sats`). + - bets pass False (the default) and get an InsufficientFundsError instead: the + bet is a fixed price that cannot be quietly reduced, and "a user's balance must + never exactly equal the bet" is a documented invariant of the PLAY phase. The + player needs a little more than the bet amount, which is what the error says. """ selected, total_in = select_utxos(utxos, amount_sats) fee = estimate_vsize(len(selected), 2) * fee_rate_sat_vb + + change = total_in - amount_sats + if change < DUST_LIMIT_SATS: + if not reduce_amount_to_keep_change: + raise InsufficientFundsError( + f"the amount leaves no change output: {DUST_LIMIT_SATS - change} more sats are " + "needed for the transaction to stay fee-bumpable", + code="balance_leaves_no_change", + required_extra_sats=DUST_LIMIT_SATS - change, + ) + amount_sats -= DUST_LIMIT_SATS - change + change = DUST_LIMIT_SATS + recipient_amount = amount_sats - fee if recipient_amount <= 0: raise InsufficientFundsError( "amount too small to cover the network fee", code="amount_below_network_fee" ) - change = total_in - amount_sats - if change < DUST_LIMIT_SATS: - fee += change # dust change is unspendable and unrelayable — miners get it - change = 0 if recipient_amount < DUST_LIMIT_SATS: raise InsufficientFundsError( "amount too small to be sent (dust)", code="amount_below_dust_limit" diff --git a/app/withdrawals/service.py b/app/withdrawals/service.py index dc32af0..9987333 100644 --- a/app/withdrawals/service.py +++ b/app/withdrawals/service.py @@ -87,6 +87,13 @@ async def request_withdrawal( amount_sats=amount_sats, change_address=user.address, fee_rate_sat_vb=config.fee_rate_sat_vb, + # B-62: "withdraw everything" asks for the whole confirmed balance, which + # would leave no change output and therefore nothing bump_fee could + # shrink — the one tx shape RBF cannot rescue, and the UI's default + # withdrawal path at that. Move a dust limit less instead of producing an + # unbumpable transaction; amount_sent_sats below records what actually + # went out, which is already how a fee-deducted withdrawal is reported. + reduce_amount_to_keep_change=True, ) except InsufficientFundsError as exc: raise WithdrawalError(exc.code, str(exc), **exc.params) from exc diff --git a/docs/guida-utente.md b/docs/guida-utente.md index b33d927..d2d9a31 100644 --- a/docs/guida-utente.md +++ b/docs/guida-utente.md @@ -128,6 +128,12 @@ scalata dall'importo richiesto (non si aggiunge separatamente). L'importo minimo prelevabile è pari alla quota fissa di ingresso al round (mostrata nella sezione Bet). +Con "Preleva l'intero importo" restano sul tuo saldo pochi satoshi (294, cioè +0,00000294 PLM): senza quel resto la transazione non potrebbe essere +ritrasmessa con una fee più alta se la rete fosse lenta, e resterebbe bloccata +per ore. La cifra effettivamente inviata è quindi il saldo meno quei satoshi e +meno la fee di rete. + > **Nota**: attualmente è supportato solo l'indirizzo esterno in formato > **P2WPKH bech32** (quelli che iniziano con `plm1q...`). Non inserire > indirizzi legacy (quelli che iniziano con `P...`) o P2SH: al momento diff --git a/tests/unit/test_broadcast.py b/tests/unit/test_broadcast.py index 73bc59f..16b1aa7 100644 --- a/tests/unit/test_broadcast.py +++ b/tests/unit/test_broadcast.py @@ -1,4 +1,5 @@ from datetime import datetime, timedelta, timezone +from types import SimpleNamespace import pytest from embit import script @@ -215,24 +216,27 @@ async def test_bump_fee_leaves_broadcast_at_untouched(session_factory): async def test_bump_fee_raises_when_no_change_output(session_factory): + """The guard still matters after B-62 even though the builder no longer produces + this shape: a single-output transaction broadcast before that change can still be + sitting in `pending` across the deploy, and it must fail loudly rather than + silently shrink the recipient's output. Hence a hand-built tx here — the point is + exactly that build_signed_transaction won't make one any more.""" + from embit.transaction import Transaction, TransactionInput, TransactionOutput + from app.wallet.hd import derive_user_address, derive_user_key + from app.wallet.psbt_builder import RBF_SEQUENCE signer = derive_user_key(0) my_address = derive_user_address(0) - from_script = script.p2wpkh(signer.to_public()) to_address = script.p2wpkh(_key(98).to_public()).address(network=PLM_MAINNET) - utxo_amount = 10_000_000 # exact amount, no change output + utxo_amount = 10_000_000 # entirely consumed by the single recipient output utxo_txid = "22" * 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, + legacy_tx = Transaction( + vin=[TransactionInput(bytes.fromhex(utxo_txid), 0, sequence=RBF_SEQUENCE)], + vout=[TransactionOutput(utxo_amount - 141, script.Script.from_address(to_address))], ) + built = SimpleNamespace(raw_hex=legacy_tx.serialize().hex(), txid=legacy_tx.txid().hex()) async with session_factory() as session: user = User(username="bob", password_hash="x", derivation_index=0, address=my_address) diff --git a/tests/unit/test_psbt_builder.py b/tests/unit/test_psbt_builder.py index 5407394..b69c365 100644 --- a/tests/unit/test_psbt_builder.py +++ b/tests/unit/test_psbt_builder.py @@ -114,28 +114,62 @@ def test_build_signed_transaction_deducts_fee_from_amount_not_change(): assert len(parsed.vout) == 2 -def test_build_signed_transaction_omits_change_output_when_exact_amount(): +def test_build_signed_transaction_refuses_an_amount_that_would_leave_no_change(): # B-62 + """A single-output transaction is the one shape RBF cannot rescue: bump_fee has + no change to shrink, and adding inputs is no answer either since this spends + every UTXO the sender has. The bet is a fixed price, so it is refused rather + than quietly reduced.""" + from app.wallet.psbt_builder import DUST_LIMIT_SATS + signer = _key(3) from_script = script.p2wpkh(signer.to_public()) my_address = from_script.address(network=PLM_MAINNET) to_address = script.p2wpkh(_key(4).to_public()).address(network=PLM_MAINNET) utxos = [Utxo("22" * 32, 0, 10_000_000)] # exactly amount_sats, zero change + with pytest.raises(InsufficientFundsError) as excinfo: + build_signed_transaction( + signing_key=signer, + from_script=from_script, + utxos=utxos, + to_address=to_address, + amount_sats=10_000_000, + change_address=my_address, + fee_rate_sat_vb=1, + ) + + assert excinfo.value.code == "balance_leaves_no_change" + assert excinfo.value.params == {"required_extra_sats": DUST_LIMIT_SATS} + + +def test_build_signed_transaction_can_reduce_the_amount_to_keep_change(): # B-62 + """What "withdraw everything" does instead: move a dust limit less and stay + fee-bumpable. The caller records the reduced amount as what was actually sent.""" + from embit.transaction import Transaction + + from app.wallet.psbt_builder import DUST_LIMIT_SATS + + signer = _key(3) + from_script = script.p2wpkh(signer.to_public()) + my_address = from_script.address(network=PLM_MAINNET) + to_address = script.p2wpkh(_key(4).to_public()).address(network=PLM_MAINNET) + built = build_signed_transaction( signing_key=signer, from_script=from_script, - utxos=utxos, + utxos=[Utxo("22" * 32, 0, 10_000_000)], to_address=to_address, amount_sats=10_000_000, change_address=my_address, fee_rate_sat_vb=1, + reduce_amount_to_keep_change=True, ) - assert built.change_sats == 0 - - from embit.transaction import Transaction + assert built.change_sats == DUST_LIMIT_SATS + assert built.recipient_sats == 10_000_000 - DUST_LIMIT_SATS - built.fee_sats parsed = Transaction.parse(bytes.fromhex(built.raw_hex)) - assert len(parsed.vout) == 1 + assert len(parsed.vout) == 2 + assert built.recipient_sats + built.change_sats + built.fee_sats == 10_000_000 def test_build_signed_transaction_raises_when_amount_smaller_than_fee(): @@ -159,10 +193,13 @@ def test_build_signed_transaction_raises_when_amount_smaller_than_fee(): ) -def test_dust_change_is_left_to_the_fee(): +def test_a_below_dust_change_output_is_never_created(): """B-06: `if change > 0` created change outputs below the dust limit, which makes the whole transaction unrelayable — the bet or withdrawal then failed at broadcast - with an opaque error the user could do nothing about.""" + with an opaque error the user could do nothing about. B-62 changed the remedy + (the change is topped up to the dust limit by moving slightly less, instead of + being folded into the fee and leaving an unbumpable single-output tx) but not + this rule: an output below DUST_LIMIT_SATS is never produced.""" from app.wallet.psbt_builder import DUST_LIMIT_SATS signer = _key(1) @@ -180,13 +217,16 @@ def test_dust_change_is_left_to_the_fee(): amount_sats=amount, change_address=change_address, fee_rate_sat_vb=1, + reduce_amount_to_keep_change=True, ) tx = Transaction.parse(bytes.fromhex(built.raw_hex)) - assert len(tx.vout) == 1 # no dust output - assert built.change_sats == 0 - # Nothing vanishes: the dust ends up in the fee, and inputs still equal outputs+fee. - assert built.fee_sats >= dust_change + assert len(tx.vout) == 2 + assert all(o.value >= DUST_LIMIT_SATS for o in tx.vout) + assert built.change_sats == DUST_LIMIT_SATS + # Nothing vanishes: inputs still equal outputs + fee, the recipient just gets + # the one satoshi that was missing from a relayable change output. + assert built.recipient_sats == amount - 1 - built.fee_sats assert built.recipient_sats + built.change_sats + built.fee_sats == amount + dust_change diff --git a/tests/unit/test_withdrawals.py b/tests/unit/test_withdrawals.py index 8691a5c..7bc435e 100644 --- a/tests/unit/test_withdrawals.py +++ b/tests/unit/test_withdrawals.py @@ -219,3 +219,75 @@ async def test_failed_broadcast_marks_the_withdrawal_failed_and_frees_the_coins( assert (await session.scalars(select(UtxoEvent).where(UtxoEvent.user_id == user_id))).one().spent_txid is None user = await session.get(User, user_id) assert user.cached_balance_sats == 3_000_000_000 + + +# --- B-62: "withdraw everything" must not build an unbumpable transaction --------- + + +async def test_full_balance_withdrawal_keeps_a_bumpable_change_output(session_factory): + """The UI's max-amount checkbox sends the whole confirmed balance, so change came + out at 0, the change output was dropped, and the tx had a single output — + bump_fee then had nothing to shrink and raised RbfError every 30s until the + reconciler abandoned the row hours later. Adding inputs is no answer here: the tx + already spends every UTXO the user has. So a dust limit stays behind instead.""" + from embit.transaction import Transaction + + from app.wallet.psbt_builder import DUST_LIMIT_SATS + + balance = 2_000_000_000 + user_id = await _make_funded_user(session_factory, 40, balance) + client = FakeElectrumClient() + + async with session_factory() as session: + user = await session.get(User, user_id) + withdrawal = await request_withdrawal(session, client, user, EXTERNAL_ADDRESS, balance) + + tx = Transaction.parse(bytes.fromhex(client.broadcasted[0])) + assert len(tx.vout) == 2 # recipient + change: bumpable + assert all(o.value >= DUST_LIMIT_SATS for o in tx.vout) + + # The user asked for everything and is told what actually went out — the row + # already distinguishes the two, since the fee comes out of the amount anyway. + assert withdrawal.amount_requested_sats == balance + fee = balance - sum(o.value for o in tx.vout) + change = min(o.value for o in tx.vout) + assert change == DUST_LIMIT_SATS + assert withdrawal.amount_sent_sats == balance - DUST_LIMIT_SATS - fee + + +async def test_a_bet_from_a_balance_equal_to_the_bet_is_refused(session_factory): + """The same shape on the PLAY side, where reducing the amount isn't an option — + the bet is a fixed price. "A user's balance must never exactly equal the bet" is + a documented invariant of the PLAY phase; this is where it's enforced, with an + error that says how much more is needed rather than a bare "insufficient".""" + from app.bets.service import BetError + from app.wallet.psbt_builder import DUST_LIMIT_SATS + + user_id = await _make_funded_user(session_factory, 41, BET_AMOUNT_SATS) # exactly the bet + 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 == "balance_leaves_no_change" + assert excinfo.value.params == {"required_extra_sats": DUST_LIMIT_SATS} + assert not client.broadcasted + + async with session_factory() as session: + utxo = (await session.scalars(select(UtxoEvent).where(UtxoEvent.user_id == user_id))).one() + assert utxo.spent_txid is None # refused before anything moved + + +async def test_a_bet_with_a_dust_limit_of_headroom_is_accepted(session_factory): + from app.wallet.psbt_builder import DUST_LIMIT_SATS + + user_id = await _make_funded_user(session_factory, 42, BET_AMOUNT_SATS + DUST_LIMIT_SATS) + client = FakeElectrumClient() + + async with session_factory() as session: + user = await session.get(User, user_id) + participant = await place_bet(session, client, user) + + assert participant.status == "broadcast"