From a9a3ddeec61d0afa3edbf829ba8ee342061f83df 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.test.ts | 66 +++++++++++++++++ .../src/infra/BdkAccountAdapter.ts | 14 ++++ .../src/infra/StoredAccountAdapter.ts | 4 ++ .../src/use-cases/AccountUseCases.test.ts | 70 +++++++++++++++++++ .../src/use-cases/AccountUseCases.ts | 10 +++ 7 files changed, 174 insertions(+) diff --git a/packages/bitcoin-wallet-snap/CHANGELOG.md b/packages/bitcoin-wallet-snap/CHANGELOG.md index 6b8303d1a..55456f116 100644 --- a/packages/bitcoin-wallet-snap/CHANGELOG.md +++ b/packages/bitcoin-wallet-snap/CHANGELOG.md @@ -32,6 +32,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Coalesce concurrent account synchronization runs so stacked triggers (the 30s cronjob, `onActive`, and background events scheduled by `setSelectedAccounts`) share one run instead of duplicating network fetches, state writes, and keyring events ([#221](https://github.com/MetaMask/internal-snaps/pull/221)) - Fix account deletion failing against keyring v2 clients by removing the `AccountDeleted` event emission from the delete flow ([#221](https://github.com/MetaMask/internal-snaps/pull/221)) - v2 clients reject v1 lifecycle events, which aborted the deletion before the account was removed from state. Deletion is client-initiated in v2, so no event is needed. +- 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 901dfc2cb..3d68b4cdc 100644 --- a/packages/bitcoin-wallet-snap/src/entities/account.ts +++ b/packages/bitcoin-wallet-snap/src/entities/account.ts @@ -98,6 +98,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.test.ts b/packages/bitcoin-wallet-snap/src/infra/BdkAccountAdapter.test.ts index 3b2425f9b..26f244060 100644 --- a/packages/bitcoin-wallet-snap/src/infra/BdkAccountAdapter.test.ts +++ b/packages/bitcoin-wallet-snap/src/infra/BdkAccountAdapter.test.ts @@ -1,7 +1,11 @@ import type { + AddressInfo, ChangeSet, DescriptorPair, + KeychainKind, Network, + ScriptBuf, + SpkIndexed, } from '@metamask/bitcoindevkit'; import { Wallet } from '@metamask/bitcoindevkit'; import { mock } from 'jest-mock-extended'; @@ -187,4 +191,66 @@ describe('BdkAccountAdapter', () => { }); }); }); + + describe('revealToScript', () => { + const mockScript = mock(); + + const indexed = (keychain: KeychainKind, index: number): SpkIndexed => + mock({ 0: keychain, 1: index }); + + const adapter = (): BdkAccountAdapter => + BdkAccountAdapter.create( + mockId, + mockDerivationPath, + mockDescriptors, + mockNetwork, + ); + + it('returns false and reveals nothing when the script is not ours', () => { + mockWallet.derivation_of_spk.mockReturnValue(undefined); + + expect(adapter().revealToScript(mockScript)).toBe(false); + expect(mockWallet.reveal_addresses_to).not.toHaveBeenCalled(); + }); + + it('returns false and reveals nothing when the index is already revealed', () => { + mockWallet.derivation_of_spk.mockReturnValue(indexed('internal', 4)); + mockWallet.derivation_index.mockReturnValue(4); + + expect(adapter().revealToScript(mockScript)).toBe(false); + expect(mockWallet.reveal_addresses_to).not.toHaveBeenCalled(); + }); + + it('reveals up to the index when the script lies beyond the revealed set', () => { + mockWallet.derivation_of_spk.mockReturnValue(indexed('internal', 7)); + mockWallet.derivation_index.mockReturnValue(4); + mockWallet.reveal_addresses_to.mockReturnValue([mock()]); + + expect(adapter().revealToScript(mockScript)).toBe(true); + expect(mockWallet.reveal_addresses_to).toHaveBeenCalledWith( + 'internal', + 7, + ); + }); + + it('reveals when the keychain has no revealed index yet', () => { + mockWallet.derivation_of_spk.mockReturnValue(indexed('external', 0)); + mockWallet.derivation_index.mockReturnValue(undefined); + mockWallet.reveal_addresses_to.mockReturnValue([mock()]); + + expect(adapter().revealToScript(mockScript)).toBe(true); + expect(mockWallet.reveal_addresses_to).toHaveBeenCalledWith( + 'external', + 0, + ); + }); + + it('returns false when the reveal produces no new addresses', () => { + mockWallet.derivation_of_spk.mockReturnValue(indexed('external', 7)); + mockWallet.derivation_index.mockReturnValue(4); + mockWallet.reveal_addresses_to.mockReturnValue([]); + + expect(adapter().revealToScript(mockScript)).toBe(false); + }); + }); }); 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 2a94fa259..fa7e9bc38 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts @@ -1092,6 +1092,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', () => { @@ -1185,6 +1253,7 @@ describe('AccountUseCases', () => { expect(mockTxBuilder.drainToByScript).toHaveBeenCalledWith( mockOutput.script_pubkey, ); + expect(mockAccount.revealToScript).not.toHaveBeenCalled(); expect(psbt).toBe(mockFilledPsbt); }); @@ -1503,6 +1572,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 e77521f51..1880bc2cf 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts @@ -487,6 +487,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);