Merge #408: Change getspendtx response of ListSpendtxEntry

7f388fc Change getspendtx response of ListSpendtxEntry (edouard)

Pull request description:

  for revault/revault-gui#358
  Some information are needed.
  User cannot use listspendtx because at this step of the spend tx creation, the transaction is not yet saved in the database

ACKs for top commit:
  darosior:
    utACK 7f388fc

Tree-SHA512: a6e417a7c068cedfb3a897de63afe50d185f8a343d7bfdbcbba60464b4d0317b81ab1dc588c63fd05020b62af458e4ba453ad81a33d92e000a530eb0fe1c7ea5

Author: darosior

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
 

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)

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::*;

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)

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)