Merge pull request #19 from bitcoinerlab/fix-getDescriptor

Refactor push/addTransaction conflict handling and fix getDescriptor

Author: landabaso

package-lock.json

@@ -2,7 +2,7 @@
   "name": "@bitcoinerlab/discovery",
   "description": "A TypeScript library for retrieving Bitcoin funds from ranged descriptors, leveraging @bitcoinerlab/explorer for standardized access to multiple blockchain explorers.",
   "homepage": "https://github.com/bitcoinerlab/discovery",
-  "version": "1.4.0",
+  "version": "1.4.1",
   "author": "Jose-Luis Landabaso",
   "license": "MIT",
   "prettier": "@bitcoinerlab/configs/prettierConfig.json",

package.json

@@ -36,8 +36,14 @@ import {
   TxHex,
   TxAttribution,
   TxWithOrder,
-  TxoMap
+  TxoMap,
+  Txo
 } from './types';
+type Conflict = {
+  descriptor: Descriptor;
+  txo: Txo;
+  index?: number;
+};
 
 const now = () => Math.floor(Date.now() / 1000);
 
@@ -1190,7 +1196,7 @@ export function DiscoveryFactory(
       const split = txo.split(':');
       if (utxo && split.length !== 2)
         throw new Error(`Error: invalid utxo: ${utxo}`);
-      if (!utxo && split.length !== 2 && split.length !== 3)
+      if (!utxo && split.length !== 2 && split.length !== 4)
         throw new Error(`Error: invalid txo: ${txo}`);
       const txId = split[0];
       if (!txId) throw new Error(`Error: invalid txo: ${txo}`);
@@ -1210,9 +1216,12 @@ export function DiscoveryFactory(
             index?: number;
           }
         | undefined;
-      const { txoMap } = this.getUtxosAndBalance({ descriptors });
-      const indexedDescriptor = txoMap[txo];
+      const { utxos, txoMap } = this.getUtxosAndBalance({ descriptors });
+      const txoMapKey: Txo = `${txId}:${vout}`; //normalizes txos with 4 parts
+      const indexedDescriptor = txoMap[txoMapKey];
       if (indexedDescriptor) {
+        if (utxo && !utxos.find(currentUtxo => currentUtxo === utxo))
+          return undefined;
         const splitTxo = (str: string): [string, string] => {
           const lastIndex = str.lastIndexOf('~');
           if (lastIndex === -1)
@@ -1233,17 +1242,32 @@ export function DiscoveryFactory(
     }
 
     /**
-     * Pushes a transaction to the network and updates the internal state
-     * accordingly. This function ensures that the transaction is pushed,
-     * verifies its presence in the mempool, and updates the internal
-     * `discoveryData` to include the new transaction.
+     * Pushes a transaction to the network and updates the internal state.
      *
-     * The `gapLimit` parameter is essential for managing descriptor discovery.
-     * When pushing a transaction, there is a possibility of receiving new funds
-     * as change. If the range for that index does not exist yet, the `gapLimit`
-     * helps to update the descriptor corresponding to a new UTXO for new
-     * indices within the gap limit.
+     * This function first broadcasts the transaction using the configured explorer.
+     * It then attempts to update the internal `discoveryData` by calling
+     * `addTransaction`.
+     *
+     * If `addTransaction` reports that one or more inputs of the pushed transaction
+     * were already considered spent by other transactions in the library's records
+     * (e.g., in an RBF scenario or a double-spend attempt already known to the
+     * library), `push` will automatically attempt to synchronize the state. It
+     * does this by calling `fetch` on all unique descriptors associated with the
+     * conflicting input(s). This helps ensure the library's state reflects the
+     * most current information from the blockchain/mempool regarding these conflicts.
+     *
+     * The `gapLimit` parameter is used both when `addTransaction` is called and
+     * during any subsequent automatic `fetch` operation triggered by conflicts.
+     * It helps discover new outputs (e.g., change addresses) that might become
+     * active due to the transaction.
      *
+     * Note: The success of broadcasting the transaction via `explorer.push(txHex)`
+     * depends on the network and node policies. Even if broadcast is successful,
+     * the transaction might not be immediately visible in the mempool or might be
+     * replaced. A warning is logged if the transaction is not found in the
+     * mempool shortly after being pushed. The final state in the library will
+     * reflect the outcome of the internal `addTransaction` call and any
+     * automatic synchronization that occurred.
      */
     async push({
       txHex,
@@ -1265,24 +1289,52 @@ export function DiscoveryFactory(
       await explorer.push(txHex);
 
       //Now, make sure it made it to the mempool:
-      let found = false;
+      let foundInMempool = false;
       for (let i = 0; i < DETECT_RETRY_MAX; i++) {
         if (await explorer.fetchTx(txId)) {
-          found = true;
+          foundInMempool = true;
           break;
         }
         await new Promise(resolve => setTimeout(resolve, DETECTION_INTERVAL));
       }
 
       const txData = { irreversible: false, blockHeight: 0, txHex };
-      this.addTransaction({ txData, gapLimit });
-      if (found === false)
+      const addResult = this.addTransaction({ txData, gapLimit });
+
+      let syncPerformed = false;
+      if (
+        !addResult.success &&
+        addResult.reason === 'INPUTS_ALREADY_SPENT' &&
+        addResult.conflicts.length > 0
+      ) {
+        // A conflict occurred: one or more inputs of the pushed transaction were already
+        // considered spent by other transactions in our records.
+        // Fetch all unique descriptors that hold the conflicting TXOs to sync their state.
+        const uniqueDescriptorsToSync = Array.from(
+          new Set(addResult.conflicts.map(c => c.descriptor))
+        );
+        if (uniqueDescriptorsToSync.length > 0) {
+          await this.fetch({ descriptors: uniqueDescriptorsToSync, gapLimit });
+          syncPerformed = true;
+        }
+        // After fetching, the state for the conflicting descriptors is updated.
+        // The originally pushed transaction might or might not be the one that "won".
+      }
+
+      if (syncPerformed) {
+        console.warn(
+          `txId ${txId}: Input conflict(s) detected; state synchronization was performed for affected descriptors. The library state reflects this outcome.`
+        );
+      }
+
+      if (!foundInMempool) {
         console.warn(
-          `txId ${txId} was pushed. However, it was then not found in the mempool. It has been set as part of the discoveryData anyway.`
+          `txId ${txId}: Pushed transaction was not found in the mempool immediately after broadcasting.`
         );
+      }
     }
 
-    /*
+    /**
      * Given a transaction it updates the internal `discoveryData` state to
      * include it.
      *
@@ -1310,6 +1362,33 @@ export function DiscoveryFactory(
      * adding new funds as change (for example). If the range for that index
      * does not exist yet, the `gapLimit` helps to update the descriptor
      * corresponding to a new UTXO for new indices within the gap limit.
+     *
+     * This function updates the internal `discoveryData` state to include the
+     * provided transaction, but only if it doesn't attempt to spend outputs
+     * already considered spent by the library.
+     *
+     * It first checks all inputs of the transaction. If any input corresponds to
+     * a `txo` (a previously known output) that is not a current `utxo`
+     * (i.e., it's already considered spent by another transaction in the
+     * library's records), this function will not modify the library state.
+     * Instead, it will return an object detailing all such conflicting inputs.
+     * This allows the caller (e.g., the `push` method) to handle these
+     * conflicts, for instance, by re-fetching the state of all affected descriptors.
+     *
+     * If no such input conflicts are found, the transaction is processed:
+     * its details are added to the `txMap`, and relevant `txId`s are associated
+     * with the `OutputData` of both its inputs (if owned) and outputs (if owned).
+     *
+     * For other types of errors (e.g., invalid input data), it may still throw.
+     *
+     * Refer to the `push` method's documentation for how it utilizes this
+     * return status for automatic state synchronization.
+     *
+     * @returns An object indicating the outcome:
+     *          - `{ success: true }` if the transaction was added without conflicts.
+     *          - `{ success: false; reason: 'INPUTS_ALREADY_SPENT'; conflicts: Array<{ descriptor: Descriptor; txo: Utxo; index?: number }> }`
+     *            if one or more inputs of the transaction were already considered spent.
+     *            `conflicts` contains an array of all such detected conflicts.
      */
     addTransaction({
       txData,
@@ -1324,13 +1403,50 @@ export function DiscoveryFactory(
        * The gap limit for descriptor discovery. Defaults to 20.
        */
       gapLimit?: number;
-    }): void {
+    }):
+      | { success: true }
+      | {
+          success: false;
+          reason: 'INPUTS_ALREADY_SPENT';
+          conflicts: Array<Conflict>;
+        } {
       const txHex = txData.txHex;
       if (!txHex)
         throw new Error('txData must contain complete txHex information');
       const { tx, txId } = this.#derivers.transactionFromHex(txHex);
       const networkId = getNetworkId(network);
 
+      const conflicts: Array<Conflict> = [];
+
+      // First pass: Check all inputs for conflicts without modifying state yet.
+      for (let vin = 0; vin < tx.ins.length; vin++) {
+        const input = tx.ins[vin];
+        if (!input) throw new Error(`Error: invalid input for ${txId}:${vin}`);
+        const prevTxId = Buffer.from(input.hash).reverse().toString('hex');
+        const prevVout = input.index;
+        const prevUtxo: Utxo = `${prevTxId}:${prevVout}`;
+
+        const isSpendingKnownUtxo = this.getDescriptor({ utxo: prevUtxo });
+        if (!isSpendingKnownUtxo) {
+          // Not spending a known UTXO, check if it's spending a known TXO (already spent)
+          const txoDescriptorInfo = this.getDescriptor({ txo: prevUtxo });
+          if (txoDescriptorInfo) {
+            const conflict: Conflict = {
+              descriptor: txoDescriptorInfo.descriptor,
+              txo: prevUtxo
+            };
+            if (txoDescriptorInfo.index !== undefined)
+              conflict.index = txoDescriptorInfo.index;
+            conflicts.push(conflict);
+          }
+        }
+      }
+
+      if (conflicts.length > 0) {
+        return { success: false, reason: 'INPUTS_ALREADY_SPENT', conflicts };
+      }
+
+      // Second pass: No conflicts found, proceed to update discoveryData.
       this.#discoveryData = produce(this.#discoveryData, discoveryData => {
         const txMap = discoveryData[networkId].txMap;
         const update = (descriptor: Descriptor, index: DescriptorIndex) => {
@@ -1348,29 +1464,28 @@ export function DiscoveryFactory(
           if (!txMap[txId]) txMap[txId] = txData; //Only add it once
         };
 
-        // search for inputs
+        // Process inputs (we know they are valid UTXOs or external)
         for (let vin = 0; vin < tx.ins.length; vin++) {
           const input = tx.ins[vin];
           if (!input)
             throw new Error(`Error: invalid input for ${txId}:${vin}`);
           //Note we create a new Buffer since reverse() mutates the Buffer
           const prevTxId = Buffer.from(input.hash).reverse().toString('hex');
-          const prevVout = input.index;
+          const prevVout = input!.index;
           const prevUtxo: Utxo = `${prevTxId}:${prevVout}`;
           const extendedDescriptor = this.getDescriptor({ utxo: prevUtxo });
-          if (extendedDescriptor)
+          if (extendedDescriptor) {
             //This means this tx is spending an utxo tracked by this discovery instance
             update(
               extendedDescriptor.descriptor,
               extendedDescriptor.index === undefined
                 ? 'non-ranged'
                 : extendedDescriptor.index
             );
-          else if (this.getDescriptor({ txo: prevUtxo }))
-            throw new Error(`Tx ${txId} was already spent.`);
+          }
         }
 
-        // search for outputs
+        // Process outputs
         for (let vout = 0; vout < tx.outs.length; vout++) {
           const nextScriptPubKey = tx.outs[vout]?.script;
           if (!nextScriptPubKey)
@@ -1380,12 +1495,14 @@ export function DiscoveryFactory(
             scriptPubKey: nextScriptPubKey,
             gapLimit
           });
-          if (descriptorWithIndex)
+          if (descriptorWithIndex) {
             //This means this tx is sending funds to a scriptPubKey tracked by
             //this discovery instance
             update(descriptorWithIndex.descriptor, descriptorWithIndex.index);
+          }
         }
       });
+      return { success: true };
     }
 
     /**

src/discovery.ts

@@ -106,7 +106,7 @@ export type IndexedDescriptor = string;
  * prevtxId:vout. Hovewer, we use a different type name to denote we're dealing
  * here with tx outputs that may have been spent or not
  */
-type Txo = string;
+export type Txo = string;
 export type TxoMap = Record<Txo, IndexedDescriptor>;
 
 /**

src/types.ts

@@ -14,7 +14,7 @@
 //TODO: test the rest of methods
 import { vaultsTests } from './vaults';
 import { RegtestUtils } from 'regtest-client';
-import { Psbt, networks } from 'bitcoinjs-lib';
+import { Psbt, networks, Transaction } from 'bitcoinjs-lib';
 import * as secp256k1 from '@bitcoinerlab/secp256k1';
 import * as descriptors from '@bitcoinerlab/descriptors';
 import { mnemonicToSeedSync } from 'bip39';
@@ -503,7 +503,8 @@ describe('Discovery on regtest', () => {
       const address = output.getAddress();
 
       //Create a new utxo received in nextIndex
-      await regtestUtils.faucet(address, 100000);
+      const faucetResult = await regtestUtils.faucet(address, 100000);
+      const trackedUtxoString = `${faucetResult.txId}:${faucetResult.vout}`;
       for (let i = 0; i < 10; i++) {
         await discovery.fetch({ descriptor });
         if (discovery.getUtxos({ descriptor }).length > initialNUtxos) break;
@@ -516,6 +517,16 @@ describe('Discovery on regtest', () => {
 
       expect(nextNextIndex).toBe(nextIndex + 1);
 
+      // Test getDescriptor for the UNSPENT trackedUtxoString
+      let descriptorInfoUnspent = discovery.getDescriptor({
+        utxo: trackedUtxoString
+      });
+      expect(descriptorInfoUnspent).toEqual({ descriptor, index: nextIndex });
+      descriptorInfoUnspent = discovery.getDescriptor({
+        txo: trackedUtxoString
+      });
+      expect(descriptorInfoUnspent).toEqual({ descriptor, index: nextIndex });
+
       const { utxos, balance } = discovery.getUtxosAndBalance({ descriptor });
 
       //Create a new tx that spends all utxos and sends them to nextNextIndex
@@ -557,6 +568,22 @@ describe('Discovery on regtest', () => {
       const txHex = psbt.extractTransaction(true).toHex();
       await discovery.push({ txHex });
       const nextNextNextIndex = discovery.getNextIndex({ descriptor });
+      const spendingTx = Transaction.fromHex(txHex);
+      const spendingTxId = spendingTx.getId();
+      let spendingVin = -1;
+      for (const [i, input] of spendingTx.ins.entries()) {
+        const prevTxId = Buffer.from(input.hash).reverse().toString('hex');
+        if (
+          prevTxId === faucetResult.txId &&
+          input.index === faucetResult.vout
+        ) {
+          spendingVin = i;
+          break;
+        }
+      }
+      expect(spendingVin).not.toBe(-1); // Ensure we found the spending input
+      const fourPartTxoString = `${trackedUtxoString}:${spendingTxId}:${spendingVin}`;
+
       expect(nextNextNextIndex).toBe(nextNextIndex + 1);
       expect(discovery.getUtxosAndBalance({ descriptor }).balance).toBe(
         finalBalance
@@ -566,6 +593,19 @@ describe('Discovery on regtest', () => {
       //Now really fetch it and make sure the push routine already set all the
       //relevant data, even if a fetch was not involved:
       await discovery.fetch({ descriptor });
+
+      // Test getDescriptor for the SPENT trackedUtxoString
+      let descriptorInfoSpent = discovery.getDescriptor({
+        utxo: trackedUtxoString
+      });
+      expect(descriptorInfoSpent).toBeUndefined(); // Should be undefined as it's spent
+      descriptorInfoSpent = discovery.getDescriptor({
+        txo: trackedUtxoString
+      });
+      expect(descriptorInfoSpent).toEqual({ descriptor, index: nextIndex });
+      // Test with 4-part txo string for the spent output
+      descriptorInfoSpent = discovery.getDescriptor({ txo: fourPartTxoString });
+      expect(descriptorInfoSpent).toEqual({ descriptor, index: nextIndex });
       expect(nextNextNextIndex).toBe(nextNextIndex + 1);
       expect(discovery.getUtxosAndBalance({ descriptor }).balance).toBe(
         finalBalance