Change getspendtx response of ListSpendtxEntry

edouardparis · edouardparis · commit 7f388fc890a4 · 2022-05-17T16:50:24.000+02:00
diff --git a/doc/API.md b/doc/API.md
@@ -341,9 +341,9 @@ feerate.
 
 #### Response
 
-| Field      | Type   | Description                                     |
-| ---------- | ------ | ----------------------------------------------- |
-| `spend_tx` | string | Base64-encoded Spend transaction PSBT           |
+| Field      | Type                                                        | Description                    |
+| ---------- | ----------------------------------------------------------- | ------------------------------ |
+| `spend_tx` | [Spend transaction resources](#spend_transaction_resources) | Spend transaction informations |
 
 
 ### `updatespendtx`
@@ -401,7 +401,7 @@ Please note that this status refers only to the Spend transaction, with regardin
 
 | Field          | Type   | Description                                                          |
 | -------------- | ------ | -------------------------------------------------------------------- |
-| `spend_txs`    | array  | Array of [Spend transaction resources](#spend_transaction_reources)  |
+| `spend_txs`    | array  | Array of [Spend transaction resources](#spend_transaction_resources) |
 
 ##### Spend transaction resources
 
diff --git a/src/commands/mod.rs b/src/commands/mod.rs
@@ -35,23 +35,21 @@ use crate::{
 };
 use utils::{
     deser_amount_from_sats, deser_from_str, finalized_emer_txs, gethistory, listvaults_from_db,
-    presigned_txs, ser_amount, ser_to_string, unvault_tx, vaults_from_deposits,
+    presigned_txs, ser_amount, ser_to_string, spend_entry, unvault_tx, vaults_from_deposits,
 };
 
 use revault_tx::{
     bitcoin::{
         consensus::encode, secp256k1, util::bip32, Address, Amount, Network, OutPoint,
         PublicKey as BitcoinPubKey, Transaction as BitcoinTransaction, TxOut, Txid,
     },
-    miniscript::DescriptorTrait,
     scripts::{CpfpDescriptor, DepositDescriptor, UnvaultDescriptor},
     transactions::{
         spend_tx_from_deposits, transaction_chain, transaction_chain_manager, CancelTransaction,
         CpfpableTransaction, EmergencyTransaction, RevaultPresignedTransaction, RevaultTransaction,
         SpendTransaction, UnvaultEmergencyTransaction, UnvaultTransaction,
     },
-    txins::RevaultTxIn,
-    txouts::{DepositTxOut, RevaultTxOut, SpendTxOut},
+    txouts::{DepositTxOut, SpendTxOut},
 };
 
 use std::{collections::BTreeMap, fmt};
@@ -924,14 +922,16 @@ impl DaemonControl {
         outpoints: &[OutPoint],
         destinations: &BTreeMap<Address, u64>,
         feerate_vb: u64,
-    ) -> Result<SpendTransaction, CommandError> {
+    ) -> Result<ListSpendEntry, CommandError> {
         let revaultd = self.revaultd.read().unwrap();
         manager_only!(revaultd);
         let db_file = &revaultd.db_file();
 
         // FIXME: have a feerate type to avoid that
         assert!(feerate_vb > 0, "Spend feerate can't be null.");
 
+        // TODO: remove the txins vec, just use the spent_vaults one.
+        let mut spent_vaults = Vec::with_capacity(outpoints.len());
         // Reconstruct the DepositTxin s from the outpoints and the vaults informations
         let mut txins = Vec::with_capacity(outpoints.len());
         // If we need a change output, use the highest derivation index of the vaults
@@ -947,6 +947,7 @@ impl DaemonControl {
                     change_index = vault.derivation_index;
                 }
                 txins.push((*outpoint, vault.amount, vault.derivation_index));
+                spent_vaults.push(vault);
             } else {
                 return Err(CommandError::InvalidStatus(
                     vault.status,
@@ -1071,7 +1072,12 @@ impl DaemonControl {
         };
         log::debug!("Final Spend transaction: '{:?}'", tx_res);
 
-        Ok(tx_res)
+        Ok(spend_entry(
+            &revaultd,
+            tx_res,
+            spent_vaults.iter(),
+            ListSpendStatus::NonFinal,
+        ))
     }
 
     /// Store a new or update an existing Spend transaction in database.
@@ -1150,7 +1156,7 @@ impl DaemonControl {
 
         let spend_tx_map = db_list_spends(&db_path).expect("Database must be available");
         let mut listspend_entries = Vec::with_capacity(spend_tx_map.len());
-        for (_, (db_spend, deposit_outpoints)) in spend_tx_map {
+        for (_, (db_spend, _)) in spend_tx_map {
             let mut status = match db_spend.broadcasted {
                 Some(true) => ListSpendStatus::Broadcasted,
                 Some(false) => ListSpendStatus::Pending,
@@ -1182,62 +1188,12 @@ impl DaemonControl {
                 }
             }
 
-            let (deposit_amount, mut cpfp_amount) = spent_vaults.iter().fold(
-                (Amount::from_sat(0), Amount::from_sat(0)),
-                |(deposit_total, cpfp_total), (_, vault)| {
-                    let unvault = unvault_tx(&revaultd, vault)
-                        .expect("Spent vault must have a correct unvault transaction");
-
-                    let cpfp_amount = Amount::from_sat(
-                        unvault
-                            .cpfp_txin(&revaultd.cpfp_descriptor, &revaultd.secp_ctx)
-                            .expect("Unvault tx has always a cpfp output")
-                            .txout()
-                            .txout()
-                            .value,
-                    );
-
-                    (deposit_total + vault.amount, cpfp_total + cpfp_amount)
-                },
-            );
-
-            let derivation_index = spent_vaults
-                .values()
-                .map(|v| v.derivation_index)
-                .max()
-                .expect("Spent vaults should not be empty");
-            let cpfp_script_pubkey = revaultd
-                .cpfp_descriptor
-                .derive(derivation_index, &revaultd.secp_ctx)
-                .into_inner()
-                .script_pubkey();
-            let deposit_address = revaultd
-                .deposit_descriptor
-                .derive(derivation_index, &revaultd.secp_ctx)
-                .into_inner()
-                .script_pubkey();
-            let mut cpfp_index = None;
-            let mut change_index = None;
-            for (i, txout) in db_spend.psbt.tx().output.iter().enumerate() {
-                if cpfp_index.is_none() && cpfp_script_pubkey == txout.script_pubkey {
-                    cpfp_index = Some(i);
-                    cpfp_amount += Amount::from_sat(txout.value);
-                }
-
-                if deposit_address == txout.script_pubkey {
-                    change_index = Some(i);
-                }
-            }
-
-            listspend_entries.push(ListSpendEntry {
-                psbt: db_spend.psbt,
-                deposit_outpoints,
-                deposit_amount,
-                cpfp_amount,
-                cpfp_index: cpfp_index.expect("We always create a CPFP output"),
-                change_index,
+            listspend_entries.push(spend_entry(
+                &revaultd,
+                db_spend.psbt,
+                spent_vaults.values(),
                 status,
-            });
+            ));
         }
 
         Ok(listspend_entries)
diff --git a/src/commands/utils.rs b/src/commands/utils.rs
@@ -4,7 +4,8 @@
 
 use crate::{
     commands::{
-        CommandError, HistoryEvent, HistoryEventKind, ListPresignedTxEntry, ListVaultsEntry,
+        CommandError, HistoryEvent, HistoryEventKind, ListPresignedTxEntry, ListSpendEntry,
+        ListSpendStatus, ListVaultsEntry,
     },
     database::{
         interface::{
@@ -21,18 +22,20 @@ use crate::{
 
 use revault_tx::{
     bitcoin::{
-        consensus::encode, hashes::hex::FromHex, Amount, OutPoint,
+        consensus::encode, hashes::hex::FromHex, util::bip32, Amount, OutPoint,
         Transaction as BitcoinTransaction, Txid,
     },
     miniscript::DescriptorTrait,
     transactions::{
-        transaction_chain_manager, CpfpableTransaction, RevaultTransaction, UnvaultTransaction,
+        transaction_chain_manager, CpfpableTransaction, RevaultTransaction, SpendTransaction,
+        UnvaultTransaction,
     },
     txins::{DepositTxIn, RevaultTxIn},
     txouts::{DepositTxOut, RevaultTxOut},
 };
 
 use std::{
+    cmp,
     collections::{HashMap, HashSet},
     fmt,
     str::FromStr,
@@ -490,6 +493,82 @@ pub fn gethistory<T: BitcoindThread>(
     Ok(events)
 }
 
+/// Get the ListSpendEntry for a given Spend transaction.
+/// This relies on brittle assumptions about how we construct the Spend. Those might not hold if
+/// used on a Spend PSBT we did not create.
+pub fn spend_entry<'a>(
+    revaultd: &RevaultD,
+    psbt: SpendTransaction,
+    spent_vaults: impl IntoIterator<Item = &'a DbVault>,
+    status: ListSpendStatus,
+) -> ListSpendEntry {
+    // The derivation index for the change is assumed to be reusing the largest one of the inputs.
+    let (deposit_amount, mut cpfp_amount, deposit_outpoints, derivation_index) =
+        spent_vaults.into_iter().fold(
+            (
+                Amount::from_sat(0),
+                Amount::from_sat(0),
+                Vec::new(),
+                bip32::ChildNumber::from(0),
+            ),
+            |(deposit_total, cpfp_total, mut deposit_outpoints, derivation_index), vault| {
+                deposit_outpoints.push(vault.deposit_outpoint);
+
+                let unvault = unvault_tx(&revaultd, &vault)
+                    .expect("Spent vault must have a correct unvault transaction");
+                let cpfp_amount = Amount::from_sat(
+                    unvault
+                        .cpfp_txin(&revaultd.cpfp_descriptor, &revaultd.secp_ctx)
+                        .expect("Unvault tx has always a cpfp output")
+                        .txout()
+                        .txout()
+                        .value,
+                );
+
+                (
+                    deposit_total + vault.amount,
+                    cpfp_total + cpfp_amount,
+                    deposit_outpoints,
+                    cmp::max(derivation_index, vault.derivation_index),
+                )
+            },
+        );
+
+    let cpfp_script_pubkey = revaultd
+        .cpfp_descriptor
+        .derive(derivation_index, &revaultd.secp_ctx)
+        .into_inner()
+        .script_pubkey();
+    let deposit_address = revaultd
+        .deposit_descriptor
+        .derive(derivation_index, &revaultd.secp_ctx)
+        .into_inner()
+        .script_pubkey();
+    let mut cpfp_index = None;
+    let mut change_index = None;
+    for (i, txout) in psbt.tx().output.iter().enumerate() {
+        if cpfp_index.is_none() && cpfp_script_pubkey == txout.script_pubkey {
+            cpfp_index = Some(i);
+            cpfp_amount += Amount::from_sat(txout.value);
+        }
+
+        if deposit_address == txout.script_pubkey {
+            change_index = Some(i);
+        }
+    }
+
+    ListSpendEntry {
+        psbt,
+        deposit_outpoints,
+        deposit_amount,
+        cpfp_amount,
+        // FIXME: this won't hold post optional-CPFP
+        cpfp_index: cpfp_index.expect("We always have a CPFP index"),
+        change_index,
+        status,
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
diff --git a/tests/test_chain.py b/tests/test_chain.py
@@ -676,7 +676,7 @@ def test_retrieve_vault_status(revault_network, bitcoind):
     revault_network.activate_vault(vault)
     deposits = [f"{vault['txid']}:{vault['vout']}"]
     destinations = {bitcoind.rpc.getnewaddress(): vault["amount"] // 2}
-    spend_tx = mans[0].rpc.getspendtx(deposits, destinations, 1)["spend_tx"]
+    spend_tx = mans[0].rpc.getspendtx(deposits, destinations, 1)["spend_tx"]["psbt"]
     for m in [man] + mans:
         spend_tx = m.man_keychain.sign_spend_psbt(spend_tx, [vault["derivation_index"]])
         mans[0].rpc.updatespendtx(spend_tx)
@@ -719,7 +719,7 @@ def test_retrieve_vault_status(revault_network, bitcoind):
     revault_network.activate_vault(vault)
     deposits = [f"{vault['txid']}:{vault['vout']}"]
     destinations = {bitcoind.rpc.getnewaddress(): vault["amount"] // 2}
-    spend_tx = mans[0].rpc.getspendtx(deposits, destinations, 1)["spend_tx"]
+    spend_tx = mans[0].rpc.getspendtx(deposits, destinations, 1)["spend_tx"]["psbt"]
     for m in [man] + mans:
         spend_tx = m.man_keychain.sign_spend_psbt(spend_tx, [vault["derivation_index"]])
         mans[0].rpc.updatespendtx(spend_tx)
@@ -759,7 +759,7 @@ def test_retrieve_vault_status(revault_network, bitcoind):
     revault_network.activate_vault(vault)
     deposits = [f"{vault['txid']}:{vault['vout']}"]
     destinations = {bitcoind.rpc.getnewaddress(): vault["amount"] // 2}
-    spend_tx = mans[0].rpc.getspendtx(deposits, destinations, 1)["spend_tx"]
+    spend_tx = mans[0].rpc.getspendtx(deposits, destinations, 1)["spend_tx"]["psbt"]
     for m in [man] + mans:
         spend_tx = m.man_keychain.sign_spend_psbt(spend_tx, [vault["derivation_index"]])
         mans[0].rpc.updatespendtx(spend_tx)
@@ -800,7 +800,7 @@ def test_retrieve_vault_status(revault_network, bitcoind):
     revault_network.activate_vault(vault)
     deposits = [f"{vault['txid']}:{vault['vout']}"]
     destinations = {bitcoind.rpc.getnewaddress(): vault["amount"] // 2}
-    spend_tx = mans[0].rpc.getspendtx(deposits, destinations, 1)["spend_tx"]
+    spend_tx = mans[0].rpc.getspendtx(deposits, destinations, 1)["spend_tx"]["psbt"]
     for m in [man] + mans:
         spend_tx = m.man_keychain.sign_spend_psbt(spend_tx, [vault["derivation_index"]])
         mans[0].rpc.updatespendtx(spend_tx)
@@ -842,7 +842,7 @@ def test_retrieve_vault_status(revault_network, bitcoind):
     revault_network.activate_vault(vault)
     deposits = [f"{vault['txid']}:{vault['vout']}"]
     destinations = {bitcoind.rpc.getnewaddress(): vault["amount"] // 2}
-    spend_tx = mans[0].rpc.getspendtx(deposits, destinations, 1)["spend_tx"]
+    spend_tx = mans[0].rpc.getspendtx(deposits, destinations, 1)["spend_tx"]["psbt"]
     for m in [man] + mans:
         spend_tx = m.man_keychain.sign_spend_psbt(spend_tx, [vault["derivation_index"]])
         mans[0].rpc.updatespendtx(spend_tx)
@@ -876,7 +876,7 @@ def test_retrieve_vault_status(revault_network, bitcoind):
     revault_network.activate_vault(vault)
     deposits = [f"{vault['txid']}:{vault['vout']}"]
     destinations = {bitcoind.rpc.getnewaddress(): vault["amount"] // 2}
-    spend_tx = mans[0].rpc.getspendtx(deposits, destinations, 1)["spend_tx"]
+    spend_tx = mans[0].rpc.getspendtx(deposits, destinations, 1)["spend_tx"]["psbt"]
     for m in [man] + mans:
         spend_tx = m.man_keychain.sign_spend_psbt(spend_tx, [vault["derivation_index"]])
         mans[0].rpc.updatespendtx(spend_tx)
diff --git a/tests/test_framework/revault_network.py b/tests/test_framework/revault_network.py
@@ -608,7 +608,9 @@ def broadcast_unvaults(self, vaults, destinations, feerate, priority=False):
             deriv_indexes.append(v["derivation_index"])
         man.wait_for_active_vaults(deposits)
 
-        spend_tx = man.rpc.getspendtx(deposits, destinations, feerate)["spend_tx"]
+        spend_tx = man.rpc.getspendtx(deposits, destinations, feerate)["spend_tx"][
+            "psbt"
+        ]
         for man in self.mans():
             spend_tx = man.man_keychain.sign_spend_psbt(spend_tx, deriv_indexes)
             man.rpc.updatespendtx(spend_tx)
@@ -654,7 +656,9 @@ def spend_vaults_unconfirmed(self, vaults, destinations, feerate, priority=False
         for man in self.mans():
             man.wait_for_active_vaults(deposits)
 
-        spend_tx = man.rpc.getspendtx(deposits, destinations, feerate)["spend_tx"]
+        spend_tx = man.rpc.getspendtx(deposits, destinations, feerate)["spend_tx"][
+            "psbt"
+        ]
         for man in self.mans():
             spend_tx = man.man_keychain.sign_spend_psbt(spend_tx, deriv_indexes)
             man.rpc.updatespendtx(spend_tx)
diff --git a/tests/test_rpc.py b/tests/test_rpc.py
diff --git a/tests/test_spend.py b/tests/test_spend.py
