Merge pull request #4636 from BitGo/BTC-1225

feat(abstract-utxo): add internal/external info when known

modules/abstract-utxo/src/abstractUtxoCoin.ts

@@ -33,7 +33,6 @@ import {
   IBaseCoin,
   InvalidAddressDerivationPropertyError,
   InvalidAddressError,
-  InvalidAddressVerificationObjectPropertyError,
   IRequestTracer,
   isTriple,
   ITransactionExplanation as BaseTransactionExplanation,
@@ -112,11 +111,24 @@ export interface VerifyAddressOptions extends BaseVerifyAddressOptions {
   index: number;
 }
 
-export interface Output {
+export interface BaseOutput {
   address: string;
   amount: string | number;
+  // Even though this external flag is redundant with the chain property, it is necessary for backwards compatibility
+  // with legacy transaction format.
   external?: boolean;
+}
+
+export interface WalletOutput extends BaseOutput {
   needsCustomChangeKeySignatureVerification?: boolean;
+  chain: number;
+  index: number;
+}
+
+export type Output = BaseOutput | WalletOutput;
+
+export function isWalletOutput(output: Output): output is WalletOutput {
+  return (output as WalletOutput).chain !== undefined && (output as WalletOutput).index !== undefined;
 }
 
 export interface TransactionExplanation extends BaseTransactionExplanation<string, string> {
@@ -150,6 +162,11 @@ export interface ExplainTransactionOptions<TNumber extends number | bigint = num
   pubs?: Triple<string>;
 }
 
+export interface DecoratedExplainTransactionOptions<TNumber extends number | bigint = number>
+  extends ExplainTransactionOptions<TNumber> {
+  changeInfo?: { address: string; chain: number; index: number }[];
+}
+
 export type UtxoNetwork = utxolib.Network;
 
 export interface TransactionPrebuild<TNumber extends number | bigint = number> extends BaseTransactionPrebuild {
@@ -592,7 +609,7 @@ export abstract class AbstractUtxoCoin extends BaseCoin {
     );
 
     const needsCustomChangeKeySignatureVerification = allOutputDetails.some(
-      (output) => output.needsCustomChangeKeySignatureVerification
+      (output) => (output as WalletOutput)?.needsCustomChangeKeySignatureVerification
     );
 
     const changeOutputs = _.filter(allOutputDetails, { external: false });
@@ -949,7 +966,7 @@ export abstract class AbstractUtxoCoin extends BaseCoin {
    * @throws {UnexpectedAddressError}
    */
   async isWalletAddress(params: VerifyAddressOptions): Promise<boolean> {
-    const { address, addressType, keychains, coinSpecific, chain, index } = params;
+    const { address, addressType, keychains, chain, index } = params;
 
     if (!this.isValidAddress(address)) {
       throw new InvalidAddressError(`invalid address: ${address}`);
@@ -961,12 +978,6 @@ export abstract class AbstractUtxoCoin extends BaseCoin {
       );
     }
 
-    if (!_.isObject(coinSpecific)) {
-      throw new InvalidAddressVerificationObjectPropertyError(
-        'address validation failure: coinSpecific field must be an object'
-      );
-    }
-
     if (!keychains) {
       throw new Error('missing required param keychains');
     }

modules/abstract-utxo/src/parseOutput.ts

@@ -14,7 +14,7 @@ import {
   UnexpectedAddressError,
   VerificationOptions,
 } from '@bitgo/sdk-core';
-import { AbstractUtxoCoin, Output, TransactionParams } from './abstractUtxoCoin';
+import { AbstractUtxoCoin, Output, TransactionParams, isWalletOutput } from './abstractUtxoCoin';
 
 const debug = debugLib('bitgo:v2:parseoutput');
 
@@ -222,6 +222,32 @@ export async function parseOutput({
   let currentAddressType: string | undefined = undefined;
   const RECIPIENT_THRESHOLD = 1000;
   try {
+    // In the case of PSBTs, we can already determine the internal/external status of the output addresses
+    // based on the derivation information being included in the PSBT. We can short circuit GET v2.wallet.address
+    // and save on network requests. Since we have the derivation information already, we can still verify the address
+    if (currentOutput.external !== undefined) {
+      // In the case that we have a custom change wallet, we need to verify the address against the custom change keys
+      // and not the wallet keys. This check is done in the handleVerifyAddressError function if this error is thrown.
+      if (customChange !== undefined) {
+        throw new UnexpectedAddressError('`address validation failure');
+      }
+      // If it is an internal address, we can skip the network request and just verify the address locally with the
+      // derivation information we have. Otherwise, if the address is external, which is the only remaining case, we
+      // can just return the current output as is without contacting the server.
+      if (isWalletOutput(currentOutput)) {
+        const res = await coin.isWalletAddress({
+          addressType: AbstractUtxoCoin.inferAddressType({ chain: currentOutput.chain }) || undefined,
+          keychains: keychainArray as { pub: string; commonKeychain?: string | undefined }[],
+          address: currentAddress,
+          chain: currentOutput.chain,
+          index: currentOutput.index,
+        });
+        if (!res) {
+          throw new UnexpectedAddressError();
+        }
+      }
+      return currentOutput;
+    }
     /**
      * The only way to determine whether an address is known on the wallet is to initiate a network request and
      * fetch it. Should the request fail and return a 404, it will throw and therefore has to be caught. For that
@@ -240,7 +266,7 @@ export async function parseOutput({
       );
 
       if (isCurrentAddressInRecipients) {
-        return { ...currentOutput, external: true };
+        return { ...currentOutput };
       }
     }
 

modules/abstract-utxo/src/transaction.ts

@@ -2,10 +2,12 @@ import * as utxolib from '@bitgo/utxo-lib';
 import { BitGoBase, IRequestTracer, Triple } from '@bitgo/sdk-core';
 import {
   AbstractUtxoCoin,
+  DecoratedExplainTransactionOptions,
+  WalletOutput,
   ExplainTransactionOptions,
-  Output,
   TransactionExplanation,
   TransactionPrebuild,
+  Output,
 } from './abstractUtxoCoin';
 import { bip32, BIP32Interface, bitgo } from '@bitgo/utxo-lib';
 
@@ -91,27 +93,35 @@ export async function getTxInputs<TNumber extends number | bigint>(params: {
 
 function explainCommon<TNumber extends number | bigint>(
   tx: bitgo.UtxoTransaction<TNumber>,
-  params: ExplainTransactionOptions<TNumber>,
+  params: DecoratedExplainTransactionOptions<TNumber>,
   network: utxolib.Network
 ) {
   const displayOrder = ['id', 'outputAmount', 'changeAmount', 'outputs', 'changeOutputs'];
   let spendAmount = BigInt(0);
   let changeAmount = BigInt(0);
-  const changeOutputs: Output[] = [];
+  const changeOutputs: WalletOutput[] = [];
   const outputs: Output[] = [];
 
-  const { changeAddresses = [] } = params.txInfo ?? {};
+  const { changeInfo } = params;
+  const changeAddresses = changeInfo?.map((info) => info.address) ?? [];
 
-  tx.outs.forEach((currentOutput) => {
+  tx.outs.forEach((currentOutput, outputIndex) => {
     const currentAddress = utxolib.address.fromOutputScript(currentOutput.script, network);
     const currentAmount = BigInt(currentOutput.value);
 
     if (changeAddresses.includes(currentAddress)) {
       // this is change
       changeAmount += currentAmount;
+      const change = changeInfo?.[outputIndex];
+      if (!change) {
+        throw new Error('changeInfo must have change information for all change outputs');
+      }
       changeOutputs.push({
         address: currentAddress,
         amount: currentAmount.toString(),
+        chain: change.chain,
+        index: change.index,
+        external: false,
       });
       return;
     }
@@ -120,6 +130,12 @@ function explainCommon<TNumber extends number | bigint>(
     outputs.push({
       address: currentAddress,
       amount: currentAmount.toString(),
+      // If changeInfo has a length greater than or equal to zero, it means that the change information
+      // was provided to the function but the output was not identified as change. In this case,
+      // the output is external, and we can set it as so. If changeInfo is undefined, it means we were
+      // given no information about change outputs, so we can't determine anything about the output,
+      // so we leave it undefined.
+      external: changeInfo ? true : undefined,
     });
   });
 
@@ -207,21 +223,53 @@ export function explainPsbt<TNumber extends number | bigint>(
     throw new Error('failed to parse psbt hex');
   }
   const txOutputs = psbt.txOutputs;
-  function getChangeAddresses() {
+
+  function getChainAndIndexFromBip32Derivations(output: bitgo.PsbtOutput) {
+    const derivations = output.bip32Derivation ?? output.tapBip32Derivation ?? undefined;
+    if (!derivations) {
+      return undefined;
+    }
+    const paths = derivations.map((d) => d.path);
+    if (!paths || paths.length !== 3) {
+      throw new Error('expected 3 paths in bip32Derivation or tapBip32Derivation');
+    }
+    if (!paths.every((p) => paths[0] === p)) {
+      throw new Error('expected all paths to be the same');
+    }
+
+    paths.forEach((path) => {
+      if (paths[0] !== path) {
+        throw new Error(
+          'Unable to get a single chain and index on the output because there are different paths for different keys'
+        );
+      }
+    });
+    return utxolib.bitgo.getChainAndIndexFromPath(paths[0]);
+  }
+
+  function getChangeInfo() {
     try {
-      return utxolib.bitgo
-        .findInternalOutputIndices(psbt)
-        .map((i) => utxolib.address.fromOutputScript(txOutputs[i].script, network));
+      return utxolib.bitgo.findInternalOutputIndices(psbt).map((i) => {
+        const derivationInformation = getChainAndIndexFromBip32Derivations(psbt.data.outputs[i]);
+        if (!derivationInformation) {
+          throw new Error('could not find derivation information on bip32Derivation or tapBip32Derivation');
+        }
+        return {
+          address: utxolib.address.fromOutputScript(txOutputs[i].script, network),
+          external: false,
+          ...derivationInformation,
+        };
+      });
     } catch (e) {
       if (e instanceof utxolib.bitgo.ErrorNoMultiSigInputFound) {
-        return [];
+        return undefined;
       }
       throw e;
     }
   }
-  const changeAddresses = getChangeAddresses();
+  const changeInfo = getChangeInfo();
   const tx = psbt.getUnsignedTx() as bitgo.UtxoTransaction<TNumber>;
-  const common = explainCommon(tx, { ...params, txInfo: { ...params.txInfo, changeAddresses } }, network);
+  const common = explainCommon(tx, { ...params, txInfo: params.txInfo, changeInfo }, network);
   const inputSignaturesCount = getPsbtInputSignaturesCount(psbt, params);
 
   // Set fee from subtracting inputs from outputs

modules/bitgo/test/v2/unit/coins/utxo/prebuildAndSign.ts

@@ -81,6 +81,7 @@ function run(coin: AbstractUtxoCoin, inputScripts: ScriptType[], txFormat: TxFor
     rbfTxIds?: string[];
     feeMultiplier?: number;
     selfSend?: boolean;
+    nockOutputAddresses?: boolean;
   }): nock.Scope[] {
     const nocks: nock.Scope[] = [];
 
@@ -104,11 +105,13 @@ function run(coin: AbstractUtxoCoin, inputScripts: ScriptType[], txFormat: TxFor
     });
 
     // nock the address info fetch
-    nocks.push(
-      nock(params.bgUrl)
-        .get(`/api/v2/${coin.getChain()}/wallet/${params.wallet.id()}/address/${params.addressInfo.address}`)
-        .reply(200, params.addressInfo)
-    );
+    if (params.nockOutputAddresses) {
+      nocks.push(
+        nock(params.bgUrl)
+          .get(`/api/v2/${coin.getChain()}/wallet/${params.wallet.id()}/address/${params.addressInfo.address}`)
+          .reply(200, params.addressInfo)
+      );
+    }
 
     if (params.rbfTxIds) {
       nocks.push(
@@ -202,6 +205,7 @@ function run(coin: AbstractUtxoCoin, inputScripts: ScriptType[], txFormat: TxFor
           prebuild,
           recipient,
           addressInfo,
+          nockOutputAddresses: txFormat !== 'psbt',
         });
 
         // call prebuild and sign, nocks should be consumed
@@ -238,6 +242,7 @@ function run(coin: AbstractUtxoCoin, inputScripts: ScriptType[], txFormat: TxFor
           prebuild,
           recipient,
           addressInfo,
+          nockOutputAddresses: txFormat !== 'psbt',
         });
 
         await wallet
@@ -263,6 +268,7 @@ function run(coin: AbstractUtxoCoin, inputScripts: ScriptType[], txFormat: TxFor
           rbfTxIds,
           feeMultiplier,
           selfSend,
+          nockOutputAddresses: txFormat !== 'psbt',
         });
 
         // call prebuild and sign, nocks should be consumed
@@ -299,18 +305,24 @@ function run(coin: AbstractUtxoCoin, inputScripts: ScriptType[], txFormat: TxFor
 utxoCoins
   .filter((coin) => utxolib.getMainnet(coin.network) !== utxolib.networks.bitcoinsv)
   .forEach((coin) => {
-    scriptTypes.forEach((inputScript) => {
-      const inputScriptCleaned = (
-        inputScript === 'taprootKeyPathSpend' ? 'p2trMusig2' : inputScript
-      ) as utxolib.bitgo.outputScripts.ScriptType2Of3;
-
-      if (!coin.supportsAddressType(inputScriptCleaned)) {
-        return;
-      }
-
-      run(coin, [inputScript, inputScript], 'psbt');
-      if (getReplayProtectionAddresses(coin.network).length) {
-        run(coin, ['p2shP2pk', inputScript], 'psbt');
-      }
-    });
+    scriptTypes
+      // Don't iterate over p2shP2pk - in no scenario would a wallet spend two p2shP2pk inputs as these
+      // are single signature inputs that are used for replay protection and are added to the transaction
+      // by our system from a separate wallet. We do run tests below where one of the inputs is a p2shP2pk and
+      // the other is an input spent by the user.
+      .filter((scriptType) => scriptType !== 'p2shP2pk')
+      .forEach((inputScript) => {
+        const inputScriptCleaned = (
+          inputScript === 'taprootKeyPathSpend' ? 'p2trMusig2' : inputScript
+        ) as utxolib.bitgo.outputScripts.ScriptType2Of3;
+
+        if (!coin.supportsAddressType(inputScriptCleaned)) {
+          return;
+        }
+
+        run(coin, [inputScript, inputScript], 'psbt');
+        if (getReplayProtectionAddresses(coin.network).length) {
+          run(coin, ['p2shP2pk', inputScript], 'psbt');
+        }
+      });
   });

modules/utxo-lib/src/bitgo/parseInput.ts

@@ -698,3 +698,20 @@ export function parsePubScript(
 
   return result;
 }
+
+export function getChainAndIndexFromPath(path: string): { chain: number; index: number } {
+  const parts = path.split('/');
+  if (parts.length <= 2) {
+    throw new Error(`invalid path "${path}"`);
+  }
+  const chain = Number(parts[parts.length - 2]);
+  const index = Number(parts[parts.length - 1]);
+  if (isNaN(chain) || isNaN(index)) {
+    throw new Error(`Could not parse chain and index into numbers from path ${path}`);
+  }
+  if (chain < 0 || index < 0) {
+    throw new Error(`chain and index must be non-negative`);
+  }
+
+  return { chain, index };
+}