Merge #391: Merging in update_spend_tx.

0e80faf Merging functionality in `update_spend_tx`. Along with doc change & the required test. (Zshan0)

Pull request description:

  Currently, `update_spend_tx` over-writes the partial signatures that are present in the database with the signatures of the given `spend_tx`. To avoid this, the suggested change in #312  was to merge the older and the newer signatures together and save it in the database.

  The corresponding test and the API documentation have been updated with the same

ACKs for top commit:
  darosior:
    ACK 0e80faf

Tree-SHA512: 2dabe11bf1949fd3e62a815d0c30c5603dbeebc26eaa9458e92c6d88be4f53b1a59b9980616650c199906633598fab11eedc3a94c181a36b48041e1b7d31f543

doc/API.md

@@ -349,7 +349,7 @@ feerate.
 ### `updatespendtx`
 
 The `updatespendtx` RPC Command stores or update the stored Spend transaction with the
-given one.
+given one. The signatures from both the old & the new transactions will be merged.
 
 #### Request
 

src/commands/mod.rs

@@ -78,6 +78,7 @@ pub enum CommandError {
     /// (Got, Expected)
     SpendNotEnoughSig(usize, usize),
     SpendInvalidSig(Vec<u8>),
+    SpendSignaturesUpdate(revault_tx::error::InputSatisfactionError),
     MissingCpfpKey,
     ManagerOnly,
     StakeholderOnly,
@@ -148,6 +149,9 @@ impl fmt::Display for CommandError {
             }
             Self::Race => write!(f, "Internal error due to a race. Please try again."),
             Self::UnknownCancel(txid) => write!(f, "Unknown Cancel transaction: '{}'", txid),
+            Self::SpendSignaturesUpdate(error) => {
+                write!(f, "Error in updating the signature '{}'", error)
+            }
         }
     }
 }
@@ -190,6 +194,7 @@ impl CommandError {
             CommandError::Bitcoind(_) => ErrorCode::BITCOIND_ERROR,
             CommandError::Tx(_) => ErrorCode::INTERNAL_ERROR,
             CommandError::SpendFeerateTooLow(_, _) => ErrorCode::INVALID_PARAMS,
+            CommandError::SpendSignaturesUpdate(_) => ErrorCode::INVALID_PARAMS,
             // TODO: some of these probably need specific error codes
             CommandError::SpendTooLarge
             | CommandError::SpendUnknownUnVault(_)
@@ -1110,15 +1115,32 @@ impl DaemonControl {
 
             db_unvaults.push(db_unvault);
         }
+        let mut old_tx =
+            db_spend_transaction(&db_path, &spend_txid).expect("Database must be available");
+
+        if let Some(unwrapped_tx) = old_tx.as_mut() {
+            // Merging the signatures of `old_tx` into `spend_tx` and saving it.
+
+            let psbt_ins = spend_tx.into_psbt().inputs;
+
+            for (i, input) in psbt_ins.into_iter().enumerate() {
+                for (pubkey, raw_sig) in input.partial_sigs {
+                    let sig = secp256k1::Signature::from_der(&raw_sig[..raw_sig.len() - 1])
+                        .map_err(|_| CommandError::SpendInvalidSig(raw_sig.clone()))?;
+
+                    // i remains in bound since the transaction is taken based on txid.
+                    unwrapped_tx
+                        .psbt
+                        .add_signature(i, pubkey.key, sig, &revaultd.secp_ctx)
+                        .map_err(|e| CommandError::SpendSignaturesUpdate(e))?;
+                }
+            }
 
-        // The user has the ability to set priority to the transaction in
-        // setspendtx, here we always set it to false.
-        if db_spend_transaction(&db_path, &spend_txid)
-            .expect("Database must be available")
-            .is_some()
-        {
             log::debug!("Updating Spend transaction '{}'", spend_txid);
-            db_update_spend(&db_path, &spend_tx, false).expect("Database must be available");
+            // The user has the ability to set priority to the transaction in
+            // setspendtx, here we always set it to false.
+            db_update_spend(&db_path, &unwrapped_tx.psbt, false)
+                .expect("Database must be available");
         } else {
             log::debug!("Storing new Spend transaction '{}'", spend_txid);
             db_insert_spend(&db_path, &db_unvaults, &spend_tx).expect("Database must be available");

tests/test_spend.py

@@ -17,6 +17,61 @@
 )
 
 
+@pytest.mark.skipif(not POSTGRES_IS_SETUP, reason="Needs Postgres for servers db")
+def test_update_spend_tx(revault_network, bitcoind):
+    CSV = 12
+    revault_network.deploy(
+        n_stakeholders=2, n_managers=3, managers_threshold=3, csv=CSV
+    )
+    managers = [revault_network.man(i) for i in range(3)]
+    amount = 0.24
+    vault = revault_network.fund(amount)
+    deposit = f"{vault['txid']}:{vault['vout']}"
+    derivation_indices = [vault["derivation_index"]]
+
+    addr = bitcoind.rpc.getnewaddress()
+    spent_vaults = [deposit]
+    feerate = 2
+    fees = revault_network.compute_spendtx_fees(feerate, len(spent_vaults), 1)
+    destination = {addr: vault["amount"] - fees}
+
+    revault_network.secure_vault(vault)
+    revault_network.activate_vault(vault)
+
+    spend_tx = managers[0].rpc.getspendtx(spent_vaults, destination, feerate)[
+        "spend_tx"
+    ]["psbt"]
+    spend_tx_a = spend_tx
+    spend_tx_b = spend_tx
+
+    # Transaction with only the first signature
+    spend_tx_a = managers[0].man_keychain.sign_spend_psbt(
+        spend_tx_a, derivation_indices
+    )
+
+    # Transaction with other 2 signatures.
+    for man in managers[1:]:
+        spend_tx_b = man.man_keychain.sign_spend_psbt(spend_tx_b, derivation_indices)
+
+    # Storing the transaction with first signature.
+    managers[0].rpc.updatespendtx(spend_tx_a)
+    managers[0].wait_for_log("Storing new Spend transaction")
+
+    # Updating the transaction with the last 2 signatures.
+    managers[0].rpc.updatespendtx(spend_tx_b)
+    managers[0].wait_for_log("Updating Spend transaction")
+
+    spend_txs = managers[0].rpc.listspendtxs()["spend_txs"]
+    assert len(spend_txs) == 1
+
+    serialized_psbt = spend_txs[0]["psbt"]
+    psbt = serializations.PSBT()
+    psbt.deserialize(serialized_psbt)
+
+    # Must have 3 signatures if they are merged.
+    assert len(psbt.inputs[0].partial_sigs) == 3
+
+
 @pytest.mark.skipif(not POSTGRES_IS_SETUP, reason="Needs Postgres for servers db")
 def test_spendtx_management(revault_network, bitcoind):
     CSV = 12