From f9c6c848916ffd15ead9ef1f00817ad4f0a1f995 Mon Sep 17 00:00:00 2001 From: jeremytsng Date: Mon, 31 Aug 2026 15:02:50 +0800 Subject: [PATCH] fix(bitcoin-wallet-snap): reveal own output scripts when signing a PSBT Partner PSBT templates may pay wallet scripts beyond the revealed set; BDK's lookahead lets them pass isMine while routine sync, which queries revealed scripts only, never watches them. Signing now reveals every own output script and persists the wallet before broadcast, so signed-away change is always covered by sync. The reveal runs only on the signing path: signing sits behind the user confirmation dialog, while fillPsbt and computeFee stay reveal-free so an unconfirmed request cannot advance the revealed index. --- packages/bitcoin-wallet-snap/CHANGELOG.md | 1 + .../src/entities/account.ts | 9 +++ .../src/infra/BdkAccountAdapter.ts | 14 ++++ .../src/infra/StoredAccountAdapter.ts | 4 ++ .../src/use-cases/AccountUseCases.test.ts | 70 +++++++++++++++++++ .../src/use-cases/AccountUseCases.ts | 10 +++ 6 files changed, 108 insertions(+) diff --git a/packages/bitcoin-wallet-snap/CHANGELOG.md b/packages/bitcoin-wallet-snap/CHANGELOG.md index 053d97aa4..1aca0c3a5 100644 --- a/packages/bitcoin-wallet-snap/CHANGELOG.md +++ b/packages/bitcoin-wallet-snap/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Reveal and persist the wallet's own output scripts when signing a PSBT, so change from partner-supplied templates is always covered by routine sync ([#225](https://github.com/MetaMask/internal-snaps/pull/225)) - Ensure certain errors are stringified correctly ([#179](https://github.com/MetaMask/internal-snaps/pull/179)) ## [2.0.1] diff --git a/packages/bitcoin-wallet-snap/src/entities/account.ts b/packages/bitcoin-wallet-snap/src/entities/account.ts index 0bb396f5f..6bef600ad 100644 --- a/packages/bitcoin-wallet-snap/src/entities/account.ts +++ b/packages/bitcoin-wallet-snap/src/entities/account.ts @@ -97,6 +97,15 @@ export type BitcoinAccount = { */ revealNextAddress(): AddressInfo; + /** + * Reveals addresses up to and including the derivation index of `script` if it belongs to + * this wallet and lies beyond the revealed set. + * + * @param script - the script to reveal up to. + * @returns true if new addresses were revealed. + */ + revealToScript(script: ScriptBuf): boolean; + /** * Start a full scan. * diff --git a/packages/bitcoin-wallet-snap/src/infra/BdkAccountAdapter.ts b/packages/bitcoin-wallet-snap/src/infra/BdkAccountAdapter.ts index 89af0a75e..7f83c3454 100644 --- a/packages/bitcoin-wallet-snap/src/infra/BdkAccountAdapter.ts +++ b/packages/bitcoin-wallet-snap/src/infra/BdkAccountAdapter.ts @@ -156,6 +156,20 @@ export class BdkAccountAdapter implements BitcoinAccount { return this.#wallet.reveal_next_address('external'); } + revealToScript(script: ScriptBuf): boolean { + const indexed = this.#wallet.derivation_of_spk(script); + if (!indexed) { + return false; + } + const keychain = indexed[0]; + const index = indexed[1]; + const lastRevealed = this.#wallet.derivation_index(keychain); + if (lastRevealed !== undefined && lastRevealed >= index) { + return false; + } + return this.#wallet.reveal_addresses_to(keychain, index).length > 0; + } + startFullScan(): FullScanRequest { return this.#wallet.start_full_scan(); } diff --git a/packages/bitcoin-wallet-snap/src/infra/StoredAccountAdapter.ts b/packages/bitcoin-wallet-snap/src/infra/StoredAccountAdapter.ts index 908e97ca6..29a451611 100644 --- a/packages/bitcoin-wallet-snap/src/infra/StoredAccountAdapter.ts +++ b/packages/bitcoin-wallet-snap/src/infra/StoredAccountAdapter.ts @@ -167,6 +167,10 @@ export class StoredAccountAdapter implements BitcoinAccount { return this.#unsupported(); } + revealToScript(_script: ScriptBuf): boolean { + return this.#unsupported(); + } + sentAndReceived(_tx: Transaction): [Amount, Amount] { return this.#unsupported(); } diff --git a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts index d4e4972c5..2f1127f5f 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts @@ -1279,6 +1279,74 @@ describe('AccountUseCases', () => { expect(txid).toBe(mockTxid); expect(psbt).toBe('mockSignedPsbt'); }); + + it('reveals and persists own output scripts after signing (broadcast: false)', async () => { + mockAccount.isMine.mockReturnValue(true); + mockAccount.revealToScript.mockReturnValue(true); + + await useCases.signPsbt('account-id', mockPsbt, 'metamask', { + fill: false, + broadcast: false, + }); + + expect(mockAccount.revealToScript).toHaveBeenCalledWith( + mockOutput.script_pubkey, + ); + expect(mockRepository.update).toHaveBeenCalledWith(mockAccount); + }); + + it('reveals and persists own output scripts after signing (broadcast: true)', async () => { + mockAccount.getTransaction.mockReturnValue(mockWalletTx); + mockAccount.isMine.mockReturnValue(true); + mockAccount.revealToScript.mockReturnValue(true); + + await useCases.signPsbt('account-id', mockPsbt, 'metamask', { + fill: false, + broadcast: true, + }); + + expect(mockAccount.revealToScript).toHaveBeenCalledWith( + mockOutput.script_pubkey, + ); + expect(mockRepository.update).toHaveBeenCalledWith(mockAccount); + }); + + it('does not persist when no output scripts get revealed (broadcast: false)', async () => { + mockAccount.isMine.mockReturnValue(true); + mockAccount.revealToScript.mockReturnValue(false); + + await useCases.signPsbt('account-id', mockPsbt, 'metamask', { + fill: false, + broadcast: false, + }); + + expect(mockRepository.update).not.toHaveBeenCalled(); + }); + + it('only persists once via #broadcast when no output scripts get revealed (broadcast: true)', async () => { + mockAccount.getTransaction.mockReturnValue(mockWalletTx); + mockAccount.isMine.mockReturnValue(true); + mockAccount.revealToScript.mockReturnValue(false); + + await useCases.signPsbt('account-id', mockPsbt, 'metamask', { + fill: false, + broadcast: true, + }); + + expect(mockRepository.update).toHaveBeenCalledTimes(1); + expect(mockRepository.update).toHaveBeenCalledWith(mockAccount); + }); + + it('does not call revealToScript for outputs that are not isMine', async () => { + mockAccount.isMine.mockReturnValue(false); + + await useCases.signPsbt('account-id', mockPsbt, 'metamask', { + fill: false, + broadcast: false, + }); + + expect(mockAccount.revealToScript).not.toHaveBeenCalled(); + }); }); describe('fillPsbt', () => { @@ -1372,6 +1440,7 @@ describe('AccountUseCases', () => { expect(mockTxBuilder.drainToByScript).toHaveBeenCalledWith( mockOutput.script_pubkey, ); + expect(mockAccount.revealToScript).not.toHaveBeenCalled(); expect(psbt).toBe(mockFilledPsbt); }); @@ -1690,6 +1759,7 @@ describe('AccountUseCases', () => { mockOutput.script_pubkey, ); expect(mockTxBuilder.addRecipientByScript).not.toHaveBeenCalled(); + expect(mockAccount.revealToScript).not.toHaveBeenCalled(); expect(fee).toBe(mockFee); }); diff --git a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts index 57a9e00b1..3f770b1b7 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts @@ -567,6 +567,16 @@ export class AccountUseCases { : psbt; const signedPsbt = account.sign(psbtToSign); + let revealed = false; + for (const txout of psbtToSign.unsigned_tx.output) { + if (account.isMine(txout.script_pubkey)) { + revealed = account.revealToScript(txout.script_pubkey) || revealed; + } + } + if (revealed) { + await this.#repository.update(account); + } + if (options.broadcast) { const psbtString = signedPsbt.toString(); const tx = account.extractTransaction(signedPsbt);