Merge pull request #8335 from BitGo/BTC-0.deprecate-legacy-tx-signing

feat(abstract-utxo): deprecate legacy transaction signing

Author: OttoAllmendinger

modules/abstract-utxo/src/abstractUtxoCoin.ts

@@ -338,6 +338,11 @@ type UtxoBaseSignTransactionOptions<TNumber extends number | bigint = number> =
    */
   returnLegacyFormat?: boolean;
   wallet?: UtxoWallet;
+  /**
+   * When true (default), extract finalized PSBT to legacy transaction format.
+   * When false, return finalized PSBT. Useful for testing to keep transactions in PSBT format.
+   */
+  extractTransaction?: boolean;
 };
 
 export type SignTransactionOptions<TNumber extends number | bigint = number> = UtxoBaseSignTransactionOptions<TNumber> &
@@ -406,6 +411,11 @@ export abstract class AbstractUtxoCoin
 
   public readonly amountType: 'number' | 'bigint';
 
+  protected readonly supportedTxFormats: { readonly psbt: boolean; readonly legacy: boolean } = {
+    psbt: true,
+    legacy: false,
+  };
+
   protected constructor(bitgo: BitGoBase, amountType: 'number' | 'bigint' = 'number') {
     super(bitgo);
     this.amountType = amountType;
@@ -582,8 +592,15 @@ export abstract class AbstractUtxoCoin
     }
 
     if (utxolib.bitgo.isPsbt(input)) {
+      if (!this.supportedTxFormats.psbt) {
+        throw new ErrorDeprecatedTxFormat('psbt');
+      }
       return decodePsbtWith(input, this.name, decodeWith);
     } else {
+      // Legacy format transactions are deprecated. This will be an unconditional error in the future.
+      if (!this.supportedTxFormats.legacy) {
+        throw new ErrorDeprecatedTxFormat('legacy');
+      }
       if (decodeWith !== 'utxolib') {
         console.error('received decodeWith hint %s, ignoring for legacy transaction', decodeWith);
       }

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

@@ -67,6 +67,8 @@ export async function signTransaction<
     allowNonSegwitSigningWithoutPrevTx: boolean;
     pubs: string[] | undefined;
     cosignerPub: string | undefined;
+    /** When true (default), extract finalized PSBT to legacy transaction format. When false, return finalized PSBT. */
+    extractTransaction?: boolean;
   }
 ): Promise<
   utxolib.bitgo.UtxoPsbt | utxolib.bitgo.UtxoTransaction<bigint | number> | fixedScriptWallet.BitGoPsbt | Buffer
@@ -77,6 +79,8 @@ export async function signTransaction<
     isLastSignature = params.isLastSignature;
   }
 
+  const { extractTransaction = true } = params;
+
   if (tx instanceof bitgo.UtxoPsbt) {
     const signedPsbt = await signPsbtWithMusig2ParticipantUtxolib(
       coin as Musig2Participant<utxolib.bitgo.UtxoPsbt>,
@@ -88,8 +92,12 @@ export async function signTransaction<
       }
     );
     if (isLastSignature) {
-      signedPsbt.finalizeAllInputs();
-      return signedPsbt.extractTransaction();
+      if (extractTransaction) {
+        signedPsbt.finalizeAllInputs();
+        return signedPsbt.extractTransaction();
+      }
+      // Return signed PSBT without finalizing to preserve derivation info
+      return signedPsbt;
     }
     return signedPsbt;
   } else if (tx instanceof fixedScriptWallet.BitGoPsbt) {
@@ -110,8 +118,12 @@ export async function signTransaction<
       }
     );
     if (isLastSignature) {
-      signedPsbt.finalizeAllInputs();
-      return Buffer.from(signedPsbt.extractTransaction().toBytes());
+      if (extractTransaction) {
+        signedPsbt.finalizeAllInputs();
+        return Buffer.from(signedPsbt.extractTransaction().toBytes());
+      }
+      // Return finalized PSBT without extracting to legacy format
+      return signedPsbt;
     }
     return signedPsbt;
   }

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

@@ -78,6 +78,7 @@ export async function signTransaction<TNumber extends number | bigint>(
       allowNonSegwitSigningWithoutPrevTx: params.allowNonSegwitSigningWithoutPrevTx ?? false,
       pubs: params.pubs,
       cosignerPub: params.cosignerPub,
+      extractTransaction: params.extractTransaction,
     });
 
     // Convert half-signed PSBT to legacy format when the caller explicitly requested txFormat: 'legacy'

modules/abstract-utxo/test/unit/customSigner.ts

@@ -83,20 +83,4 @@ describe('UTXO Custom Signer Function', function () {
     sinon.assert.calledOnce(customSigningFunction as sinon.SinonStub);
     scope.done();
   });
-
-  it('should use a custom signing function if provided for Tx without taprootKeyPathSpend input', async function () {
-    const tx = utxoLib.testutil.constructTxnBuilder(
-      [{ scriptType: 'p2wsh', value: BigInt(1000) }],
-      [{ scriptType: 'p2sh', value: BigInt(900) }],
-      basecoin.network,
-      rootWalletKey,
-      'unsigned'
-    );
-    const scope = nocks({ txHex: tx.buildIncomplete().toHex() });
-    const result = await wallet.sendMany({ recipients, customSigningFunction });
-
-    assertHasProperty(result, 'ok', true);
-    sinon.assert.calledOnce(customSigningFunction as sinon.SinonStub);
-    scope.done();
-  });
 });

modules/abstract-utxo/test/unit/signTransaction.ts

@@ -6,7 +6,7 @@ import nock = require('nock');
 import { testutil } from '@bitgo/utxo-lib';
 import { common, Triple } from '@bitgo/sdk-core';
 
-import { getReplayProtectionPubkeys } from '../../src';
+import { getReplayProtectionPubkeys, ErrorDeprecatedTxFormat } from '../../src';
 import type { Unspent } from '../../src/unspent';
 
 import { getUtxoWallet, getDefaultWalletKeys, getUtxoCoin, keychainsBase58, defaultBitGo } from './util';
@@ -170,7 +170,7 @@ describe('signTransaction', function () {
     }
   });
 
-  it('customSigningFunction flow - Network Tx', async function () {
+  it('customSigningFunction flow - Network Tx should reject legacy format', async function () {
     const inputs: testutil.TxnInput<bigint>[] = testutil.txnInputScriptTypes
       .filter((v) => v !== 'p2shP2pk')
       .map((scriptType) => ({
@@ -182,9 +182,10 @@ describe('signTransaction', function () {
     const txBuilder = testutil.constructTxnBuilder(inputs, outputs, coin.network, rootWalletKeys, 'unsigned');
     const unspents = inputs.map((v, i) => testutil.toTxnUnspent(v, i, coin.network, rootWalletKeys));
 
-    for (const v of [false, true]) {
-      await signTransaction(txBuilder.buildIncomplete(), v, unspents);
-    }
+    // Legacy format transactions are now deprecated and should throw ErrorDeprecatedTxFormat
+    await assert.rejects(async () => {
+      await signTransaction(txBuilder.buildIncomplete(), false, unspents);
+    }, ErrorDeprecatedTxFormat);
   });
 
   it('fails on PSBT cache miss', async function () {

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

@@ -5,7 +5,7 @@ import * as _ from 'lodash';
 import * as utxolib from '@bitgo/utxo-lib';
 import nock = require('nock');
 import { BIP32Interface, bitgo, testutil } from '@bitgo/utxo-lib';
-import { address as wasmAddress } from '@bitgo/wasm-utxo';
+import { address as wasmAddress, fixedScriptWallet } from '@bitgo/wasm-utxo';
 import {
   common,
   FullySignedTransaction,
@@ -108,6 +108,7 @@ function run<TNumber extends number | bigint = number>(
         prv: signer.toBase58(),
         pubs: walletKeys.triple.map((k) => k.neutered().toBase58()),
         cosignerPub: cosigner.neutered().toBase58(),
+        extractTransaction: false,
       } as WalletSignTransactionOptions;
     }
 
@@ -223,7 +224,7 @@ function run<TNumber extends number | bigint = number>(
 
       function toTransactionStagesObj(stages: TransactionStages): TransactionObjStages {
         return _.mapValues(stages, (v) =>
-          v === undefined || v instanceof utxolib.bitgo.UtxoPsbt
+          v === undefined || v instanceof utxolib.bitgo.UtxoPsbt || v instanceof fixedScriptWallet.BitGoPsbt
             ? undefined
             : v instanceof utxolib.bitgo.UtxoTransaction
             ? transactionToObj<TNumber>(v)
@@ -266,14 +267,11 @@ function run<TNumber extends number | bigint = number>(
       signedBy: BIP32Interface[],
       sign: 'halfsigned' | 'fullsigned'
     ) {
-      if (txFormat === 'psbt' && sign === 'halfsigned') {
+      if (txFormat === 'psbt') {
         testPsbtValidSignatures(tx, signedBy);
         return;
       }
-      const unspents =
-        txFormat === 'psbt'
-          ? getUnspentsForPsbt().map((u) => ({ ...u, value: bitgo.toTNumber(u.value, amountType) as TNumber }))
-          : getUnspents();
+      const unspents = getUnspents();
       const prevOutputs = unspents.map(
         (u): utxolib.TxOutput<TNumber> => ({
           script: Buffer.from(wasmAddress.toOutputScriptWithCoin(u.address, coin.name)),
@@ -398,6 +396,8 @@ function run<TNumber extends number | bigint = number>(
         const txHex =
           stageTx instanceof utxolib.bitgo.UtxoPsbt || stageTx instanceof utxolib.bitgo.UtxoTransaction
             ? stageTx.toBuffer().toString('hex')
+            : stageTx instanceof fixedScriptWallet.BitGoPsbt
+            ? Buffer.from(stageTx.serialize()).toString('hex')
             : stageTx.txHex;
 
         const pubs = walletKeys.triple.map((k) => k.neutered().toBase58()) as Triple<string>;
@@ -415,9 +415,7 @@ function run<TNumber extends number | bigint = number>(
 }
 
 function runTestForCoin(coin: AbstractUtxoCoin) {
-  (['legacy', 'psbt'] as const).forEach((txFormat) => {
-    run(coin, getScriptTypes(coin, txFormat), txFormat, { decodeWith: 'wasm-utxo' });
-  });
+  run(coin, getScriptTypes(coin, 'psbt'), 'psbt', { decodeWith: 'wasm-utxo' });
 }
 
 describe('Transaction Suite', function () {