Merge #30: Spend transactions listing and deletion

c73e8a42dd050c1e24daf295c666f6aa625f3bee commands: add a new 'delspendtx' command (Antoine Poinsot)
d5bd10add8270c1e0aae1855e6f8a96649323e24 commands: add a change_index field to listspendtxs entries (Antoine Poinsot)
c6e004806aecef54c640119439406b5308fbacd0 commands: add a 'list_spend' command (and 'listspendtxs' RPC) (Antoine Poinsot)

Pull request description:

  Based on #27 this implements listing and deletion of Spend transaction from database. The API was based off revaultd's one and @edouardparis' suggestions in #1. We could be smarter in various places but at the moment we KISS.

ACKs for top commit:
  darosior:
    ACK c73e8a42dd050c1e24daf295c666f6aa625f3bee

Tree-SHA512: d4a13b9cbc9a6d48f698ba2f113c5de116b17cc51ed1f8dba578c041d9a70249699627729562ea199b32db6aaa9d2eb973d2b890f35a5e398e9c34940701ea86

doc/API.md

@@ -10,6 +10,8 @@ Commands must be sent as valid JSONRPC 2.0 requests, ending with a `\n`.
 | [`stop`](#stop)                                             | Stops the minisafe daemon                            |
 | [`getinfo`](#getinfo)                                       | Get general information about the daemon             |
 | [`getnewaddress`](#getnewaddress)                           | Get a new receiving address                          |
+| [`listspendtxs`](#listspendtxs)                             | List all stored Spend transactions                   |
+| [`delspendtx`](#delspendtx)                                 | Delete a stored Spend transaction                    |
 
 # Reference
 
@@ -130,3 +132,44 @@ This command does not return anything for now.
 
 | Field          | Type      | Description                                          |
 | -------------- | --------- | ---------------------------------------------------- |
+
+
+### `listspendtxs`
+
+List stored Spend transactions.
+
+#### Request
+
+This command does not take any parameter for now.
+
+| Field         | Type              | Description                                                 |
+| ------------- | ----------------- | ----------------------------------------------------------- |
+
+#### Response
+
+| Field          | Type          | Description                                                      |
+| -------------- | ------------- | ---------------------------------------------------------------- |
+| `spend_txs`    | array         | Array of Spend tx entries                                        |
+
+##### Spend tx entry
+
+| Field          | Type              | Description                                                             |
+| -------------- | ----------------- | ----------------------------------------------------------------------- |
+| `psbt`         | string            | Base64-encoded PSBT of the Spend transaction.                           |
+| `change_index` | int or null       | Index of the change output in the transaction outputs, if there is one. |
+
+
+### `delspendtx`
+
+#### Request
+
+| Field    | Type   | Description                                         |
+| -------- | ------ | --------------------------------------------------- |
+| `txid`   | string | Hex encoded txid of the Spend transaction to delete |
+
+#### Response
+
+This command does not return anything for now.
+
+| Field          | Type      | Description                                          |
+| -------------- | --------- | ---------------------------------------------------- |

src/commands/mod.rs

@@ -9,7 +9,7 @@ use crate::{
     database::{Coin, DatabaseInterface},
     descriptors, DaemonControl, VERSION,
 };
-use utils::{deser_amount_from_sats, deser_psbt_base64, ser_amount, ser_base64};
+use utils::{change_index, deser_amount_from_sats, deser_psbt_base64, ser_amount, ser_base64};
 
 use std::{
     collections::{BTreeMap, HashMap},
@@ -287,7 +287,7 @@ impl DaemonControl {
                 script_pubkey: address.script_pubkey(),
             });
             // TODO: if it's an address of ours, signal it as change to signing devices by adding
-            // the BIP32 derivation path to the PSBT input.
+            // the BIP32 derivation path to the PSBT output.
             psbt_outs.push(PsbtOut::default());
         }
 
@@ -418,6 +418,24 @@ impl DaemonControl {
 
         Ok(())
     }
+
+    pub fn list_spend(&self) -> ListSpendResult {
+        let mut db_conn = self.db.connection();
+        let spend_txs = db_conn
+            .list_spend()
+            .into_iter()
+            .map(|psbt| {
+                let change_index = change_index(&psbt).map(|i| i.try_into().expect("insane usize"));
+                ListSpendEntry { psbt, change_index }
+            })
+            .collect();
+        ListSpendResult { spend_txs }
+    }
+
+    pub fn delete_spend(&self, txid: &bitcoin::Txid) {
+        let mut db_conn = self.db.connection();
+        db_conn.delete_spend(txid);
+    }
 }
 
 #[derive(Debug, Clone, Serialize, Deserialize)]
@@ -462,6 +480,18 @@ pub struct CreateSpendResult {
     pub psbt: Psbt,
 }
 
+#[derive(Debug, Clone, Serialize, Deserialize)]
+pub struct ListSpendEntry {
+    #[serde(serialize_with = "ser_base64", deserialize_with = "deser_psbt_base64")]
+    pub psbt: Psbt,
+    pub change_index: Option<u32>,
+}
+
+#[derive(Debug, Clone, Serialize, Deserialize)]
+pub struct ListSpendResult {
+    pub spend_txs: Vec<ListSpendEntry>,
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

src/commands/utils.rs

@@ -32,3 +32,27 @@ where
     let psbt = consensus::deserialize(&s).map_err(de::Error::custom)?;
     Ok(psbt)
 }
+
+// Utility to gather the index of a change output in a Psbt, if there is one.
+// FIXME: this is temporary! This is based on create_spend's behaviour that reuses the
+// first coin address and doesn't shuffle the outputs!
+pub fn change_index(psbt: &Psbt) -> Option<usize> {
+    // We always set the witness UTxO in the PSBTs we create.
+    let first_coin_spk = match psbt.inputs[0]
+        .witness_utxo
+        .as_ref()
+        .map(|o| &o.script_pubkey)
+    {
+        Some(spk) => spk,
+        None => return None,
+    };
+
+    let tx = &psbt.global.unsigned_tx;
+    for i in (0..tx.output.len()).rev() {
+        if &tx.output[i].script_pubkey == first_coin_spk {
+            return Some(i);
+        }
+    }
+
+    None
+}

src/database/mod.rs

@@ -76,6 +76,12 @@ pub trait DatabaseConnection {
 
     /// Insert a new Spend transaction or replace an existing one.
     fn store_spend(&mut self, psbt: &Psbt);
+
+    /// List all existing Spend transactions.
+    fn list_spend(&mut self) -> Vec<Psbt>;
+
+    /// Delete a Spend transaction from database.
+    fn delete_spend(&mut self, txid: &bitcoin::Txid);
 }
 
 // FIXME: if possible, avoid reallocating.
@@ -171,6 +177,17 @@ impl DatabaseConnection for SqliteConn {
     fn store_spend(&mut self, psbt: &Psbt) {
         self.store_spend(psbt)
     }
+
+    fn list_spend(&mut self) -> Vec<Psbt> {
+        self.list_spend()
+            .into_iter()
+            .map(|db_spend| db_spend.psbt)
+            .collect()
+    }
+
+    fn delete_spend(&mut self, txid: &bitcoin::Txid) {
+        self.delete_spend(txid)
+    }
 }
 
 #[derive(Debug, Clone, PartialEq, Eq)]

src/database/sqlite/mod.rs

@@ -382,6 +382,27 @@ impl SqliteConn {
         })
         .expect("Db must not fail");
     }
+
+    pub fn list_spend(&mut self) -> Vec<DbSpendTransaction> {
+        db_query(
+            &mut self.conn,
+            "SELECT * FROM spend_transactions",
+            rusqlite::params![],
+            |row| row.try_into(),
+        )
+        .expect("Db must not fail")
+    }
+
+    pub fn delete_spend(&mut self, txid: &bitcoin::Txid) {
+        db_exec(&mut self.conn, |db_tx| {
+            db_tx.execute(
+                "DELETE FROM spend_transactions WHERE txid = ?1",
+                rusqlite::params![txid.to_vec()],
+            )?;
+            Ok(())
+        })
+        .expect("Db must not fail");
+    }
 }
 
 #[cfg(test)]

src/jsonrpc/api.rs

@@ -60,6 +60,18 @@ fn update_spend(control: &DaemonControl, params: Params) -> Result<serde_json::V
     Ok(serde_json::json!({}))
 }
 
+fn delete_spend(control: &DaemonControl, params: Params) -> Result<serde_json::Value, Error> {
+    let txid = params
+        .get(0, "txid")
+        .ok_or(Error::invalid_params("Missing 'txid' parameter."))?
+        .as_str()
+        .and_then(|s| bitcoin::Txid::from_str(&s).ok())
+        .ok_or(Error::invalid_params("Invalid 'feerate' parameter."))?;
+    control.delete_spend(&txid);
+
+    Ok(serde_json::json!({}))
+}
+
 /// Handle an incoming JSONRPC2 request.
 pub fn handle_request(control: &DaemonControl, req: Request) -> Result<Response, Error> {
     let result = match req.method.as_str() {
@@ -69,9 +81,16 @@ pub fn handle_request(control: &DaemonControl, req: Request) -> Result<Response,
             ))?;
             create_spend(control, params)?
         }
+        "delspendtx" => {
+            let params = req
+                .params
+                .ok_or(Error::invalid_params("Missing 'txid' parameter."))?;
+            delete_spend(control, params)?
+        }
         "getinfo" => serde_json::json!(&control.get_info()),
         "getnewaddress" => serde_json::json!(&control.get_new_address()),
         "listcoins" => serde_json::json!(&control.list_coins()),
+        "listspendtxs" => serde_json::json!(&control.list_spend()),
         "stop" => serde_json::json!({}),
         "updatespend" => {
             let params = req

src/testutils.rs

@@ -170,6 +170,20 @@ impl DatabaseConnection for DummyDbConn {
     fn spend_tx(&mut self, txid: &bitcoin::Txid) -> Option<Psbt> {
         self.db.read().unwrap().spend_txs.get(txid).cloned()
     }
+
+    fn list_spend(&mut self) -> Vec<Psbt> {
+        self.db
+            .read()
+            .unwrap()
+            .spend_txs
+            .values()
+            .cloned()
+            .collect()
+    }
+
+    fn delete_spend(&mut self, txid: &bitcoin::Txid) {
+        self.db.write().unwrap().spend_txs.remove(txid);
+    }
 }
 
 pub struct DummyMinisafe {

tests/test_framework/serializations.py

@@ -551,6 +551,11 @@ class CTransaction(object):
             self.sha256 = uint256_from_str(hash256(self.serialize_without_witness()))
         self.hash = encode(hash256(self.serialize())[::-1], "hex_codec").decode("ascii")
 
+    def txid(self):
+        if self.sha256 is None:
+            self.calc_sha256()
+        return ser_uint256(self.sha256)[::-1]
+
     def is_valid(self):
         self.calc_sha256()
         for tout in self.vout:

tests/test_rpc.py

@@ -95,6 +95,61 @@ def test_create_spend(minisafed, bitcoind):
     bitcoind.rpc.sendrawtransaction(signed_tx_hex)
 
 
+def test_list_spend(minisafed, bitcoind):
+    # Start by creating two conflicting Spend PSBTs. The first one will have a change
+    # output but not the second one.
+    addr = minisafed.rpc.getnewaddress()["address"]
+    value_a = 0.2567
+    bitcoind.rpc.sendtoaddress(addr, value_a)
+    wait_for(lambda: len(minisafed.rpc.listcoins()["coins"]) == 1)
+    outpoints = [c["outpoint"] for c in minisafed.rpc.listcoins()["coins"]]
+    destinations = {
+        bitcoind.rpc.getnewaddress(): int(value_a * COIN // 2),
+    }
+    res = minisafed.rpc.createspend(outpoints, destinations, 6)
+    assert "psbt" in res
+
+    addr = minisafed.rpc.getnewaddress()["address"]
+    value_b = 0.0987
+    bitcoind.rpc.sendtoaddress(addr, value_b)
+    wait_for(lambda: len(minisafed.rpc.listcoins()["coins"]) == 2)
+    outpoints = [c["outpoint"] for c in minisafed.rpc.listcoins()["coins"]]
+    destinations = {
+        bitcoind.rpc.getnewaddress(): int((value_a + value_b) * COIN - 1_000),
+    }
+    res_b = minisafed.rpc.createspend(outpoints, destinations, 2)
+    assert "psbt" in res_b
+
+    # Store them both in DB.
+    assert len(minisafed.rpc.listspendtxs()["spend_txs"]) == 0
+    minisafed.rpc.updatespend(res["psbt"])
+    minisafed.rpc.updatespend(res_b["psbt"])
+
+    # Listing all Spend transactions will list them both. It'll tell us which one has
+    # change and which one doesn't.
+    list_res = minisafed.rpc.listspendtxs()["spend_txs"]
+    assert len(list_res) == 2
+    first_psbt = next(entry for entry in list_res if entry["psbt"] == res["psbt"])
+    assert first_psbt["change_index"] == 1
+    second_psbt = next(entry for entry in list_res if entry["psbt"] == res_b["psbt"])
+    assert second_psbt["change_index"] is None
+
+    # If we delete the first one, we'll get only the second one.
+    first_psbt = PSBT()
+    first_psbt.deserialize(res["psbt"])
+    minisafed.rpc.delspendtx(first_psbt.tx.txid().hex())
+    list_res = minisafed.rpc.listspendtxs()["spend_txs"]
+    assert len(list_res) == 1
+    assert list_res[0]["psbt"] == res_b["psbt"]
+
+    # If we delete the second one, result will be empty.
+    second_psbt = PSBT()
+    second_psbt.deserialize(res_b["psbt"])
+    minisafed.rpc.delspendtx(second_psbt.tx.txid().hex())
+    list_res = minisafed.rpc.listspendtxs()["spend_txs"]
+    assert len(list_res) == 0
+
+
 def test_update_spend(minisafed, bitcoind):
     # Start by creating a Spend PSBT
     addr = minisafed.rpc.getnewaddress()["address"]
@@ -108,7 +163,52 @@ def test_update_spend(minisafed, bitcoind):
     assert "psbt" in res
 
     # Now update it
+    assert len(minisafed.rpc.listspendtxs()["spend_txs"]) == 0
     minisafed.rpc.updatespend(res["psbt"])
+    list_res = minisafed.rpc.listspendtxs()["spend_txs"]
+    assert len(list_res) == 1
+    assert list_res[0]["psbt"] == res["psbt"]
 
-    # TODO: check it's stored once we implement 'listspendtxs'
-    # TODO: check with added signatures once we implement 'listspendtxs'
+    # Keep a copy for later.
+    psbt_no_sig = PSBT()
+    psbt_no_sig.deserialize(res["psbt"])
+
+    # We can add a signature and update it
+    psbt_sig_a = PSBT()
+    psbt_sig_a.deserialize(res["psbt"])
+    dummy_pk_a = bytes.fromhex(
+        "0375e00eb72e29da82b89367947f29ef34afb75e8654f6ea368e0acdfd92976b7c"
+    )
+    dummy_sig_a = bytes.fromhex(
+        "304402202b925395cfeaa0171a7a92982bb4891acc4a312cbe7691d8375d36796d5b570a0220378a8ab42832848e15d1aedded5fb360fedbdd6c39226144e527f0f1e19d5398"
+    )
+    psbt_sig_a.inputs[0].partial_sigs[dummy_pk_a] = dummy_sig_a
+    psbt_sig_a_ser = psbt_sig_a.serialize()
+    minisafed.rpc.updatespend(psbt_sig_a_ser)
+
+    # We'll get it when querying
+    list_res = minisafed.rpc.listspendtxs()["spend_txs"]
+    assert len(list_res) == 1
+    assert list_res[0]["psbt"] == psbt_sig_a_ser
+
+    # We can add another signature to the empty PSBT and update it again
+    psbt_sig_b = PSBT()
+    psbt_sig_b.deserialize(res["psbt"])
+    dummy_pk_b = bytes.fromhex(
+        "03a1b26313f430c4b15bb1fdce663207659d8cac749a0e53d70eff01874496feff"
+    )
+    dummy_sig_b = bytes.fromhex(
+        "3044022005aebcd649fb8965f0591710fb3704931c3e8118ee60dd44917479f63ceba6d4022018b212900e5a80e9452366894de37f0d02fb9c89f1e94f34fb6ed7fd71c15c41"
+    )
+    psbt_sig_b.inputs[0].partial_sigs[dummy_pk_b] = dummy_sig_b
+    psbt_sig_b_ser = psbt_sig_b.serialize()
+    minisafed.rpc.updatespend(psbt_sig_b_ser)
+
+    # It will have merged both.
+    list_res = minisafed.rpc.listspendtxs()["spend_txs"]
+    assert len(list_res) == 1
+    psbt_merged = PSBT()
+    psbt_merged.deserialize(list_res[0]["psbt"])
+    assert len(psbt_merged.inputs[0].partial_sigs) == 2
+    assert psbt_merged.inputs[0].partial_sigs[dummy_pk_a] == dummy_sig_a
+    assert psbt_merged.inputs[0].partial_sigs[dummy_pk_b] == dummy_sig_b