From bc06fbb2a52e3118392b5d82d54ab63312b31cb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Tue, 4 Aug 2026 20:25:49 +0000 Subject: [PATCH] honour frozen coins in automatic selection, not just coin control MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit freezing a coin promised it would not be spent, and the promise only held when the caller named outpoints explicitly. an ordinary send picked its own inputs from a snapshot in which every utxo said frozen: false — the chain has no idea what officer froze — so selectCoins, which has always filtered on that flag, never saw one set. sendAll was the worst case: "send everything" swept the frozen coin too. the frozen list now travels with the request, set by the route and overwritten if a caller supplies one. it can only ever restrict what is spendable, so smuggling a value in gains nothing. the backend still reads no officer table. three tests pin it, including a control that sends successfully once the coin is unfrozen — without it the other two would pass on a wallet that could not spend at all. Co-Authored-By: Claude Opus 5 --- .../sidecar/wallet/backends/onchain.ts | 10 +++- .../wallet/chain-source-esplora.test.ts | 57 ++++++++++++++++++- src/servers/sidecar/wallet/routes.ts | 7 ++- src/servers/sidecar/wallet/types.ts | 9 +++ 4 files changed, 77 insertions(+), 6 deletions(-) diff --git a/src/servers/sidecar/wallet/backends/onchain.ts b/src/servers/sidecar/wallet/backends/onchain.ts index 35fce413..0463a3eb 100644 --- a/src/servers/sidecar/wallet/backends/onchain.ts +++ b/src/servers/sidecar/wallet/backends/onchain.ts @@ -585,8 +585,16 @@ export class OnchainBackend extends BaseBackend { // that may already be gone, and the failure would arrive as a broadcast rejection after signing. const snap = await this.snapshot({ fresh: true }); + // The snapshot comes off the chain, which knows nothing about freezing, so every UTXO in it claims + // `frozen: false`. selectCoins has always honoured the flag — nothing ever set it, which is what made + // a plain send happily spend a coin the owner had frozen, and made "send everything" sweep it. + const frozen = new Set(req.frozenOutpoints ?? []); + const utxos = frozen.size + ? snap.utxos.map((u) => (frozen.has(`${u.txid}:${u.vout}`) ? { ...u, frozen: true } : u)) + : snap.utxos; + const selection = selectCoins({ - utxos: snap.utxos, + utxos, targetSats: req.amountSats ?? 0, sendAll: req.sendAll === true, satPerVbyte: req.satPerVbyte, diff --git a/src/servers/sidecar/wallet/chain-source-esplora.test.ts b/src/servers/sidecar/wallet/chain-source-esplora.test.ts index 69ecaa07..c0b6cd12 100644 --- a/src/servers/sidecar/wallet/chain-source-esplora.test.ts +++ b/src/servers/sidecar/wallet/chain-source-esplora.test.ts @@ -114,8 +114,10 @@ class StubEsplora { throw new Error('not needed'); } + /** Accepts anything and reports a fixed txid — the spend tests care what got selected, not what got sent. */ async broadcast(): Promise { - throw new Error('not needed'); + this.calls.push('broadcast'); + return 'd'.repeat(64); } } @@ -134,7 +136,16 @@ const LOCKED_SIGNER: WalletSigner = { }, }; -function backendFor(world: Map) { +/** + * A signer holding the fixture's own root key. Only the spend tests need one — everything else runs + * locked, which is the state the wallet is in almost all of the time. + */ +const UNLOCKED_SIGNER: WalletSigner = { + isUnlocked: () => true, + withRoot: (fn: (root: HDKey) => T): T => fn(HDKey.fromMasterSeed(mnemonicToSeedSync(PUBLIC_TEST_MNEMONIC))), +}; + +function backendFor(world: Map, signer: WalletSigner = LOCKED_SIGNER) { const stub = new StubEsplora(world); const source = new EsploraChainSource({ chain: stub as unknown as EsploraChain, @@ -144,7 +155,7 @@ function backendFor(world: Map) { chain: source, network: 'bitcoin', accountXpub: { p2wpkh: ACCOUNT_XPUB }, - signer: LOCKED_SIGNER, + signer, }); return { backend, stub }; } @@ -251,6 +262,46 @@ describe('EsploraChainSource through OnchainBackend', () => { expect(tx.feeSats).toBeNull(); }); + // Freezing a coin is a promise that it will not be spent. It was not being kept: the route checked the + // frozen list only against outpoints the caller named explicitly, so coin control was safe while a + // plain send — the ordinary case — picked inputs from a snapshot on which every UTXO claimed + // `frozen: false`, because the chain has no idea what Officer froze. Sweeping was the worst of it: a + // "send everything" emptied the frozen coin too. + // + // These run unlocked and reach the signer, which is why the fixture key had to be the published test + // vector and nothing else. + describe('frozen coins', () => { + const fundedWorld = () => new Map([[addressAt(0, 0), { confirmed: 50_000 }]]); + /** The single outpoint the stub serves for a funded address. */ + const OUTPOINT = `${'a'.repeat(64)}:0`; + const TO = 'bc1qcr8te4kr609gcawutmrza0j4xv80jy8z306fyu'; + + test('automatic selection will not spend one', async () => { + const { backend, stub } = backendFor(fundedWorld(), UNLOCKED_SIGNER); + await expect( + backend.sendCoins({ address: TO, amountSats: 10_000, satPerVbyte: 5, frozenOutpoints: [OUTPOINT] }), + ).rejects.toThrow(/no spendable UTXOs/); + // Nothing was signed and nothing left the machine. + expect(stub.calls).not.toContain('broadcast'); + }); + + test('sendAll will not sweep one', async () => { + const { backend, stub } = backendFor(fundedWorld(), UNLOCKED_SIGNER); + await expect( + backend.sendCoins({ address: TO, sendAll: true, satPerVbyte: 5, frozenOutpoints: [OUTPOINT] }), + ).rejects.toThrow(/no spendable UTXOs/); + expect(stub.calls).not.toContain('broadcast'); + }); + + test('the same send succeeds once the coin is not frozen', async () => { + // The control. Without it the two tests above would still pass if sending were broken outright. + const { backend, stub } = backendFor(fundedWorld(), UNLOCKED_SIGNER); + const result = await backend.sendCoins({ address: TO, amountSats: 10_000, satPerVbyte: 5 }); + expect(result.txid).toBe('d'.repeat(64)); + expect(stub.calls).toContain('broadcast'); + }); + }); + test('fee estimates pass through untouched', async () => { const { backend } = backendFor(new Map()); expect(await backend.estimateFees()).toEqual({ diff --git a/src/servers/sidecar/wallet/routes.ts b/src/servers/sidecar/wallet/routes.ts index 6ad38a29..4780b7fb 100644 --- a/src/servers/sidecar/wallet/routes.ts +++ b/src/servers/sidecar/wallet/routes.ts @@ -308,9 +308,12 @@ async function handleWallets(ctx: OfficerContext, seg: string[]): Promise frozen.has(o))) return badRequest('refusing to spend a frozen UTXO'); - const result = await backend.sendCoins(req2); + // Spread last so a caller cannot supply its own frozen list: the check above only ever covered + // outpoints the caller named explicitly, which left automatic selection free to pick a frozen coin. + const result = await backend.sendCoins({ ...req2, frozenOutpoints: frozenList }); if (req2.label) await setWalletLabel(walletId, 'tx', result.txid, req2.label); return json(result); } diff --git a/src/servers/sidecar/wallet/types.ts b/src/servers/sidecar/wallet/types.ts index 6ab9ed3d..5e0e91f3 100644 --- a/src/servers/sidecar/wallet/types.ts +++ b/src/servers/sidecar/wallet/types.ts @@ -141,6 +141,15 @@ export type SendCoinsRequest = { /** Opt out of RBF signalling. Default is replaceable. */ rbf?: boolean; label?: string; + /** + * Outpoints automatic selection must not touch. SET BY THE ROUTE FROM OFFICER'S FROZEN LIST, never by + * the caller — whatever arrives in the request body is overwritten. + * + * Freezing is Officer policy stored in Postgres, and a backend has no business reading that table; so + * the frozen set has to travel with the request. Note it only ever restricts what may be spent, so + * even a caller who did smuggle a value in could not widen the selection. + */ + frozenOutpoints?: readonly string[]; }; export type SendCoinsResult = {