Merge pull request #7671 from BitGo/BTC-2806.add-signtx-tests

feat(utxo): refactor signing flow and improve MuSig2 support

Author: OttoAllmendinger

modules/abstract-utxo/src/abstractUtxoCoin.ts

@@ -59,6 +59,7 @@ import { isReplayProtectionUnspent } from './transaction/fixedScript/replayProte
 import { supportedCrossChainRecoveries } from './config';
 import {
   assertValidTransactionRecipient,
+  DecodedTransaction,
   explainTx,
   fromExtendedAddressFormat,
   isScriptRecipient,
@@ -178,9 +179,7 @@ function convertValidationErrorToTxIntentMismatch(
   return txIntentError;
 }
 
-export type DecodedTransaction<TNumber extends number | bigint> =
-  | utxolib.bitgo.UtxoTransaction<TNumber>
-  | utxolib.bitgo.UtxoPsbt;
+export type { DecodedTransaction } from './transaction/types';
 
 export type RootWalletKeys = bitgo.RootWalletKeys;
 
@@ -548,6 +547,14 @@ export abstract class AbstractUtxoCoin extends BaseCoin {
     }
   }
 
+  decodeTransactionAsPsbt(input: Buffer | string): utxolib.bitgo.UtxoPsbt {
+    const decoded = this.decodeTransaction(input);
+    if (!(decoded instanceof utxolib.bitgo.UtxoPsbt)) {
+      throw new Error('expected psbt but got transaction');
+    }
+    return decoded;
+  }
+
   decodeTransactionFromPrebuild<TNumber extends number | bigint>(prebuild: {
     txHex?: string;
     txBase64?: string;
@@ -712,12 +719,23 @@ export abstract class AbstractUtxoCoin extends BaseCoin {
    * @param psbtHex all MuSig2 inputs should contain user MuSig2 nonce
    * @param walletId
    */
-  async signPsbt(psbtHex: string, walletId: string): Promise<SignPsbtResponse> {
-    const params: SignPsbtRequest = { psbt: psbtHex };
-    return await this.bitgo
+  async getMusig2Nonces(psbt: utxolib.bitgo.UtxoPsbt, walletId: string): Promise<utxolib.bitgo.UtxoPsbt> {
+    const params: SignPsbtRequest = { psbt: psbt.toHex() };
+    const response = await this.bitgo
       .post(this.url('/wallet/' + walletId + '/tx/signpsbt'))
       .send(params)
       .result();
+    return this.decodeTransactionAsPsbt(response.psbt);
+  }
+
+  /**
+   * @deprecated Use getMusig2Nonces instead
+   * @returns input psbt added with deterministic MuSig2 nonce for bitgo key for each MuSig2 inputs.
+   * @param psbtHex all MuSig2 inputs should contain user MuSig2 nonce
+   * @param walletId
+   */
+  async signPsbt(psbtHex: string, walletId: string): Promise<SignPsbtResponse> {
+    return { psbt: (await this.getMusig2Nonces(this.decodeTransactionAsPsbt(psbtHex), walletId)).toHex() };
   }
 
   /**
@@ -727,9 +745,11 @@ export abstract class AbstractUtxoCoin extends BaseCoin {
   async signPsbtFromOVC(ovcJson: Record<string, unknown>): Promise<Record<string, unknown>> {
     assert(ovcJson['psbtHex'], 'ovcJson must contain psbtHex');
     assert(ovcJson['walletId'], 'ovcJson must contain walletId');
-    const psbt = (await this.signPsbt(ovcJson['psbtHex'] as string, ovcJson['walletId'] as string)).psbt;
-    assert(psbt, 'psbt not found');
-    return _.extend(ovcJson, { txHex: psbt });
+    const psbt = await this.getMusig2Nonces(
+      this.decodeTransactionAsPsbt(ovcJson['psbtHex'] as string),
+      ovcJson['walletId'] as string
+    );
+    return _.extend(ovcJson, { txHex: psbt.toHex() });
   }
 
   /**

modules/abstract-utxo/src/transaction/fixedScript/index.ts

@@ -3,6 +3,6 @@ export { explainPsbtWasm } from './explainPsbtWasm';
 export { parseTransaction } from './parseTransaction';
 export { CustomChangeOptions } from './parseOutput';
 export { verifyTransaction } from './verifyTransaction';
-export { signTransaction } from './signTransaction';
+export { signTransaction, Musig2Participant } from './signTransaction';
 export * from './sign';
 export * from './replayProtection';

modules/abstract-utxo/src/transaction/fixedScript/sign.ts

@@ -11,7 +11,7 @@ type Unspent<TNumber extends number | bigint = number> = utxolib.bitgo.Unspent<T
 
 type RootWalletKeys = utxolib.bitgo.RootWalletKeys;
 
-type PsbtParsedScriptTypes =
+type PsbtParsedScriptType =
   | 'p2sh'
   | 'p2wsh'
   | 'p2shP2wsh'
@@ -22,17 +22,24 @@ type PsbtParsedScriptTypes =
 export class InputSigningError<TNumber extends number | bigint = number> extends Error {
   static expectedWalletUnspent<TNumber extends number | bigint>(
     inputIndex: number,
+    inputType: PsbtParsedScriptType | null, // null for legacy transaction format
     unspent: Unspent<TNumber> | { id: string }
   ): InputSigningError<TNumber> {
-    return new InputSigningError(inputIndex, unspent, `not a wallet unspent, not a replay protection unspent`);
+    return new InputSigningError(
+      inputIndex,
+      inputType,
+      unspent,
+      `not a wallet unspent, not a replay protection unspent`
+    );
   }
 
   constructor(
     public inputIndex: number,
+    public inputType: PsbtParsedScriptType | null, // null for legacy transaction format
     public unspent: Unspent<TNumber> | { id: string },
     public reason: Error | string
   ) {
-    super(`signing error at input ${inputIndex}: unspentId=${unspent.id}: ${reason}`);
+    super(`signing error at input ${inputIndex}: type=${inputType} unspentId=${unspent.id}: ${reason}`);
   }
 }
 
@@ -70,7 +77,7 @@ export function signAndVerifyPsbt(
 ): utxolib.bitgo.UtxoPsbt | utxolib.bitgo.UtxoTransaction<bigint> {
   const txInputs = psbt.txInputs;
   const outputIds: string[] = [];
-  const scriptTypes: PsbtParsedScriptTypes[] = [];
+  const scriptTypes: PsbtParsedScriptType[] = [];
 
   const signErrors: InputSigningError<bigint>[] = psbt.data.inputs
     .map((input, inputIndex: number) => {
@@ -89,7 +96,7 @@ export function signAndVerifyPsbt(
         psbt.signInputHD(inputIndex, signerKeychain);
         debug('Successfully signed input %d of %d', inputIndex + 1, psbt.data.inputs.length);
       } catch (e) {
-        return new InputSigningError<bigint>(inputIndex, { id: outputId }, e);
+        return new InputSigningError<bigint>(inputIndex, scriptType, { id: outputId }, e);
       }
     })
     .filter((e): e is InputSigningError<bigint> => e !== undefined);
@@ -109,11 +116,11 @@ export function signAndVerifyPsbt(
       const outputId = outputIds[inputIndex];
       try {
         if (!psbt.validateSignaturesOfInputHD(inputIndex, signerKeychain)) {
-          return new InputSigningError(inputIndex, { id: outputId }, new Error(`invalid signature`));
+          return new InputSigningError(inputIndex, scriptType, { id: outputId }, new Error(`invalid signature`));
         }
       } catch (e) {
         debug('Invalid signature');
-        return new InputSigningError<bigint>(inputIndex, { id: outputId }, e);
+        return new InputSigningError<bigint>(inputIndex, scriptType, { id: outputId }, e);
       }
     })
     .filter((e): e is InputSigningError<bigint> => e !== undefined);
@@ -178,13 +185,13 @@ export function signAndVerifyWalletTransaction<TNumber extends number | bigint>(
         return;
       }
       if (!isWalletUnspent<TNumber>(unspent)) {
-        return InputSigningError.expectedWalletUnspent<TNumber>(inputIndex, unspent);
+        return InputSigningError.expectedWalletUnspent<TNumber>(inputIndex, null, unspent);
       }
       try {
         signInputWithUnspent<TNumber>(txBuilder, inputIndex, unspent, walletSigner);
         debug('Successfully signed input %d of %d', inputIndex + 1, unspents.length);
       } catch (e) {
-        return new InputSigningError<TNumber>(inputIndex, unspent, e);
+        return new InputSigningError<TNumber>(inputIndex, null, unspent, e);
       }
     })
     .filter((e): e is InputSigningError<TNumber> => e !== undefined);
@@ -203,18 +210,18 @@ export function signAndVerifyWalletTransaction<TNumber extends number | bigint>(
         return;
       }
       if (!isWalletUnspent<TNumber>(unspent)) {
-        return InputSigningError.expectedWalletUnspent<TNumber>(inputIndex, unspent);
+        return InputSigningError.expectedWalletUnspent<TNumber>(inputIndex, null, unspent);
       }
       try {
         const publicKey = walletSigner.deriveForChainAndIndex(unspent.chain, unspent.index).signer.publicKey;
         if (
           !utxolib.bitgo.verifySignatureWithPublicKey<TNumber>(signedTransaction, inputIndex, prevOutputs, publicKey)
         ) {
-          return new InputSigningError(inputIndex, unspent, new Error(`invalid signature`));
+          return new InputSigningError(inputIndex, null, unspent, new Error(`invalid signature`));
         }
       } catch (e) {
         debug('Invalid signature');
-        return new InputSigningError<TNumber>(inputIndex, unspent, e);
+        return new InputSigningError<TNumber>(inputIndex, null, unspent, e);
       }
     })
     .filter((e): e is InputSigningError<TNumber> => e !== undefined);

modules/abstract-utxo/src/transaction/fixedScript/signTransaction.ts

@@ -6,10 +6,16 @@ import { bitgo } from '@bitgo/utxo-lib';
 import * as utxolib from '@bitgo/utxo-lib';
 import { isTriple, Triple } from '@bitgo/sdk-core';
 
-import { AbstractUtxoCoin, DecodedTransaction, RootWalletKeys } from '../../abstractUtxoCoin';
+import { DecodedTransaction } from '../types';
 
 import { signAndVerifyPsbt, signAndVerifyWalletTransaction } from './sign';
 
+type RootWalletKeys = bitgo.RootWalletKeys;
+
+export interface Musig2Participant {
+  getMusig2Nonces(psbt: utxolib.bitgo.UtxoPsbt, walletId: string): Promise<utxolib.bitgo.UtxoPsbt>;
+}
+
 /**
  * Key Value: Unsigned tx id => PSBT
  * It is used to cache PSBTs with taproot key path (MuSig2) inputs during external express signer is activated.
@@ -21,9 +27,10 @@ import { signAndVerifyPsbt, signAndVerifyWalletTransaction } from './sign';
 const PSBT_CACHE = new Map<string, utxolib.bitgo.UtxoPsbt>();
 
 export async function signTransaction<TNumber extends number | bigint>(
-  coin: AbstractUtxoCoin,
+  coin: Musig2Participant,
   tx: DecodedTransaction<TNumber>,
   signerKeychain: BIP32Interface | undefined,
+  network: utxolib.Network,
   params: {
     walletId: string | undefined;
     txInfo: { unspents?: utxolib.bitgo.Unspent<TNumber>[] } | undefined;
@@ -59,7 +66,7 @@ export async function signTransaction<TNumber extends number | bigint>(
         return { txHex: tx.toHex() };
       case 'cosignerNonce':
         assert(params.walletId, 'walletId is required for MuSig2 bitgo nonce');
-        return { txHex: (await coin.signPsbt(tx.toHex(), params.walletId)).psbt };
+        return { txHex: (await coin.getMusig2Nonces(tx, params.walletId)).toHex() };
       case 'signerSignature':
         const txId = tx.getUnsignedTx().getId();
         const psbt = PSBT_CACHE.get(txId);
@@ -76,8 +83,8 @@ export async function signTransaction<TNumber extends number | bigint>(
         assert(params.walletId, 'walletId is required for MuSig2 bitgo nonce');
         assert(signerKeychain);
         tx.setAllInputsMusig2NonceHD(signerKeychain);
-        const response = await coin.signPsbt(tx.toHex(), params.walletId);
-        tx.combine(bitgo.createPsbtFromHex(response.psbt, coin.network));
+        const response = await coin.getMusig2Nonces(tx, params.walletId);
+        tx = tx.combine(response);
         break;
     }
   } else {

modules/abstract-utxo/src/transaction/signTransaction.ts

@@ -63,7 +63,7 @@ export async function signTransaction<TNumber extends number | bigint>(
       throw new Error('expected a UtxoPsbt object');
     }
   } else {
-    return fixedScript.signTransaction(coin, tx, getSignerKeychain(params.prv), {
+    return fixedScript.signTransaction(coin, tx, getSignerKeychain(params.prv), coin.network, {
       walletId: params.txPrebuild.walletId,
       txInfo: params.txPrebuild.txInfo,
       isLastSignature: params.isLastSignature ?? false,

modules/abstract-utxo/src/transaction/types.ts

@@ -1,7 +1,13 @@
+import * as utxolib from '@bitgo/utxo-lib';
+
 import type { UtxoNamedKeychains } from '../keychains';
 
 import type { CustomChangeOptions } from './fixedScript';
 
+export type DecodedTransaction<TNumber extends number | bigint> =
+  | utxolib.bitgo.UtxoTransaction<TNumber>
+  | utxolib.bitgo.UtxoPsbt;
+
 export interface BaseOutput<TAmount = string | number> {
   address: string;
   amount: TAmount;

modules/abstract-utxo/test/unit/transaction/fixedScript/signPsbt.ts

@@ -0,0 +1,52 @@
+import assert from 'node:assert/strict';
+
+import * as utxolib from '@bitgo/utxo-lib';
+
+import { signAndVerifyPsbt } from '../../../../src/transaction/fixedScript/sign';
+
+function describeSignAndVerifyPsbt(acidTest: utxolib.testutil.AcidTest) {
+  describe(`${acidTest.name}`, function () {
+    it('should sign unsigned psbt to halfsigned', function () {
+      // Create unsigned PSBT
+      const psbt = acidTest.createPsbt();
+
+      // Set musig2 nonces for taproot inputs before signing
+      const sessionId = Buffer.alloc(32);
+      psbt.setAllInputsMusig2NonceHD(acidTest.rootWalletKeys.user, { sessionId });
+      psbt.setAllInputsMusig2NonceHD(acidTest.rootWalletKeys.bitgo, { deterministic: true });
+
+      // Sign with user key
+      const result = signAndVerifyPsbt(psbt, acidTest.rootWalletKeys.user, {
+        isLastSignature: false,
+      });
+
+      // Result should be a PSBT (not finalized)
+      assert(result instanceof utxolib.bitgo.UtxoPsbt, 'should return UtxoPsbt when not last signature');
+
+      // Verify that all wallet inputs have been signed by user key
+      result.data.inputs.forEach((input, inputIndex) => {
+        const { scriptType } = utxolib.bitgo.parsePsbtInput(input);
+
+        // Skip replay protection inputs (p2shP2pk)
+        if (scriptType === 'p2shP2pk') {
+          return;
+        }
+
+        // Verify user signature is present
+        const isValid = result.validateSignaturesOfInputHD(inputIndex, acidTest.rootWalletKeys.user);
+        assert(isValid, `input ${inputIndex} should have valid user signature`);
+      });
+    });
+  });
+}
+
+describe('signAndVerifyPsbt', function () {
+  // Create test suite with includeP2trMusig2ScriptPath set to false
+  // p2trMusig2 script path inputs are signed by user and backup keys,
+  // which is not the typical signing pattern and makes testing more complex
+  utxolib.testutil.AcidTest.suite({ includeP2trMusig2ScriptPath: false })
+    .filter((test) => test.signStage === 'unsigned')
+    .forEach((test) => {
+      describeSignAndVerifyPsbt(test);
+    });
+});

modules/utxo-lib/src/testutil/psbt.ts

@@ -258,6 +258,17 @@ export function constructPsbt(
 export const txFormats = ['psbt', 'psbt-lite'] as const;
 export type TxFormat = (typeof txFormats)[number];
 
+type SuiteConfig = {
+  /**
+   * By default, we include p2trMusig2 script path in the inputs.
+   * This input is a bit of a weirdo because it is signed by the user and the
+   * backup key, which usually is not mixed with other inputs and outputs.
+   *
+   * This option allows to exclude this input from the inputs.
+   */
+  includeP2trMusig2ScriptPath?: boolean;
+};
+
 /**
  * Creates a valid PSBT with as many features as possible.
  *
@@ -297,7 +308,7 @@ export class AcidTest {
     this.outputs = outputs;
   }
 
-  static withDefaults(network: Network, signStage: SignStage, txFormat: TxFormat): AcidTest {
+  static withConfig(network: Network, signStage: SignStage, txFormat: TxFormat, suiteConfig: SuiteConfig): AcidTest {
     const rootWalletKeys = getDefaultWalletKeys();
 
     const otherWalletKeys = getWalletKeysForSeed('too many secrets');
@@ -307,6 +318,7 @@ export class AcidTest {
           ? isSupportedScriptType(network, 'p2trMusig2')
           : isSupportedScriptType(network, scriptType)
       )
+      .filter((scriptType) => (suiteConfig.includeP2trMusig2ScriptPath ?? true) || scriptType !== 'p2trMusig2')
       .map((scriptType) => ({ scriptType, value: BigInt(2000) }));
 
     const outputs: Output[] = outputScriptTypes
@@ -345,12 +357,12 @@ export class AcidTest {
     return psbt;
   }
 
-  static suite(): AcidTest[] {
+  static suite(suiteConfig: SuiteConfig = {}): AcidTest[] {
     return getNetworkList()
       .filter((network) => isMainnet(network) && network !== networks.bitcoinsv)
       .flatMap((network) =>
         signStages.flatMap((signStage) =>
-          txFormats.flatMap((txFormat) => AcidTest.withDefaults(network, signStage, txFormat))
+          txFormats.flatMap((txFormat) => AcidTest.withConfig(network, signStage, txFormat, suiteConfig))
         )
       );
   }