From dbf17681a432791f58ef4cdbb3fb2ebfa4ba0742 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 17 Nov 2022 14:52:27 +0100 Subject: [PATCH 1/3] commands: signal for opt-in RBF in Spend transactions --- src/commands/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 7fbab980..20b39607 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -298,6 +298,7 @@ impl DaemonControl { in_value += coin.amount; txins.push(bitcoin::TxIn { previous_output: *op, + sequence: bitcoin::Sequence::ENABLE_RBF_NO_LOCKTIME, // TODO: once we move to Taproot, anti-fee-sniping using nSequence ..bitcoin::TxIn::default() }); From 838daa0c93ad72adde9b5c502796f87571fb5efd Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 17 Nov 2022 15:01:34 +0100 Subject: [PATCH 2/3] commands: set the BIP32 derivation in PSBT for change outputs So the signing devices (or even the GUI) can view them as such. --- src/commands/mod.rs | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 20b39607..3fd0307a 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -333,9 +333,23 @@ impl DaemonControl { value: amount.to_sat(), 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 output. - psbt_outs.push(PsbtOut::default()); + // If it's an address of ours, signal it as change to signing devices by adding the + // BIP32 derivation path to the PSBT output. + let bip32_derivation = + if let Some((index, is_change)) = db_conn.derivation_index_by_address(address) { + let desc = if is_change { + self.config.main_descriptor.change_descriptor() + } else { + self.config.main_descriptor.receive_descriptor() + }; + desc.derive(index, &self.secp).bip32_derivations() + } else { + Default::default() + }; + psbt_outs.push(PsbtOut { + bip32_derivation, + ..PsbtOut::default() + }); } // Now create the transaction, compute its fees and already sanity check if its feerate @@ -398,7 +412,10 @@ impl DaemonControl { // TODO: shuffle once we have Taproot change_txo.value = change_amount.to_sat(); tx.output.push(change_txo); - psbt_outs.push(PsbtOut::default()); + psbt_outs.push(PsbtOut { + bip32_derivation: change_desc.bip32_derivations(), + ..PsbtOut::default() + }); } } } From 4bd65213e2561453f7c1fee16acccdd128296406 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 17 Nov 2022 15:15:22 +0100 Subject: [PATCH 3/3] commands: swap the outpoints and destinations arguments to createspend The outpoints argument could eventually be made optional in order to introduce some automated coin selection. So it makes more sense for it to be after a required parameter, the destinations. --- src/commands/mod.rs | 30 +++++++++++++++--------------- src/jsonrpc/api.rs | 32 ++++++++++++++++---------------- tests/test_framework/utils.py | 2 +- tests/test_rpc.py | 12 ++++++------ tests/test_spend.py | 4 ++-- 5 files changed, 40 insertions(+), 40 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 3fd0307a..9f54de36 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -265,8 +265,8 @@ impl DaemonControl { pub fn create_spend( &self, - coins_outpoints: &[bitcoin::OutPoint], destinations: &HashMap, + coins_outpoints: &[bitcoin::OutPoint], feerate_vb: u64, ) -> Result { if coins_outpoints.is_empty() { @@ -677,22 +677,22 @@ mod tests { .cloned() .collect(); assert_eq!( - control.create_spend(&[], &destinations, 1), + control.create_spend(&destinations, &[], 1), Err(CommandError::NoOutpoint) ); assert_eq!( - control.create_spend(&[dummy_op], &HashMap::new(), 1), + control.create_spend(&HashMap::new(), &[dummy_op], 1), Err(CommandError::NoDestination) ); assert_eq!( - control.create_spend(&[dummy_op], &destinations, 0), + control.create_spend(&destinations, &[dummy_op], 0), Err(CommandError::InvalidFeerate(0)) ); // The coin doesn't exist. If we create a new unspent one at this outpoint with a much // higher value, we'll get a Spend transaction with a change output. assert_eq!( - control.create_spend(&[dummy_op], &destinations, 1), + control.create_spend(&destinations, &[dummy_op], 1), Err(CommandError::UnknownOutpoint(dummy_op)) ); let mut db_conn = control.db().lock().unwrap().connection(); @@ -706,7 +706,7 @@ mod tests { spend_txid: None, spend_block: None, }]); - let res = control.create_spend(&[dummy_op], &destinations, 1).unwrap(); + let res = control.create_spend(&destinations, &[dummy_op], 1).unwrap(); let tx = res.psbt.unsigned_tx; assert_eq!(tx.input.len(), 1); assert_eq!(tx.input[0].previous_output, dummy_op); @@ -717,13 +717,13 @@ mod tests { // Transaction is 1 in (P2WSH satisfaction), 2 outs. At 1sat/vb, it's 170 sats fees. // At 2sats/vb, it's twice that. assert_eq!(tx.output[1].value, 89_830); - let res = control.create_spend(&[dummy_op], &destinations, 2).unwrap(); + let res = control.create_spend(&destinations, &[dummy_op], 2).unwrap(); let tx = res.psbt.unsigned_tx; assert_eq!(tx.output[1].value, 89_660); // If we ask for a too high feerate, or a too large/too small output, it'll fail. assert_eq!( - control.create_spend(&[dummy_op], &destinations, 10_000), + control.create_spend(&destinations, &[dummy_op], 10_000), Err(CommandError::InsufficientFunds( bitcoin::Amount::from_sat(100_000), bitcoin::Amount::from_sat(10_000), @@ -732,7 +732,7 @@ mod tests { ); *destinations.get_mut(&dummy_addr).unwrap() = 100_001; assert_eq!( - control.create_spend(&[dummy_op], &destinations, 1), + control.create_spend(&destinations, &[dummy_op], 1), Err(CommandError::InsufficientFunds( bitcoin::Amount::from_sat(100_000), bitcoin::Amount::from_sat(100_001), @@ -741,7 +741,7 @@ mod tests { ); *destinations.get_mut(&dummy_addr).unwrap() = 4_500; assert_eq!( - control.create_spend(&[dummy_op], &destinations, 1), + control.create_spend(&destinations, &[dummy_op], 1), Err(CommandError::InvalidOutputValue(bitcoin::Amount::from_sat( 4_500 ))) @@ -750,7 +750,7 @@ mod tests { // If we ask for a large, but valid, output we won't get a change output. 95_000 because we // won't create an output lower than 5k sats. *destinations.get_mut(&dummy_addr).unwrap() = 95_000; - let res = control.create_spend(&[dummy_op], &destinations, 1).unwrap(); + let res = control.create_spend(&destinations, &[dummy_op], 1).unwrap(); let tx = res.psbt.unsigned_tx; assert_eq!(tx.input.len(), 1); assert_eq!(tx.input[0].previous_output, dummy_op); @@ -768,7 +768,7 @@ mod tests { .unwrap(), )]); assert_eq!( - control.create_spend(&[dummy_op], &destinations, 1), + control.create_spend(&destinations, &[dummy_op], 1), Err(CommandError::AlreadySpent(dummy_op)) ); @@ -836,17 +836,17 @@ mod tests { .cloned() .collect(); let mut psbt_a = control - .create_spend(&[dummy_op_a], &destinations_a, 1) + .create_spend(&destinations_a, &[dummy_op_a], 1) .unwrap() .psbt; let txid_a = psbt_a.unsigned_tx.txid(); let psbt_b = control - .create_spend(&[dummy_op_b], &destinations_b, 10) + .create_spend(&destinations_b, &[dummy_op_b], 10) .unwrap() .psbt; let txid_b = psbt_b.unsigned_tx.txid(); let psbt_c = control - .create_spend(&[dummy_op_a, dummy_op_b], &destinations_c, 100) + .create_spend(&destinations_c, &[dummy_op_a, dummy_op_b], 100) .unwrap() .psbt; let txid_c = psbt_c.unsigned_tx.txid(); diff --git a/src/jsonrpc/api.rs b/src/jsonrpc/api.rs index 6876ab8d..42761047 100644 --- a/src/jsonrpc/api.rs +++ b/src/jsonrpc/api.rs @@ -8,22 +8,8 @@ use std::{collections::HashMap, convert::TryInto, str::FromStr}; use miniscript::bitcoin::{self, consensus, util::psbt::PartiallySignedTransaction as Psbt}; fn create_spend(control: &DaemonControl, params: Params) -> Result { - let outpoints = params - .get(0, "outpoints") - .ok_or_else(|| Error::invalid_params("Missing 'outpoints' parameter."))? - .as_array() - .and_then(|arr| { - arr.iter() - .map(|entry| { - entry - .as_str() - .and_then(|e| bitcoin::OutPoint::from_str(e).ok()) - }) - .collect::>>() - }) - .ok_or_else(|| Error::invalid_params("Invalid 'outpoints' parameter."))?; let destinations = params - .get(1, "destinations") + .get(0, "destinations") .ok_or_else(|| Error::invalid_params("Missing 'destinations' parameter."))? .as_object() .and_then(|obj| { @@ -36,6 +22,20 @@ fn create_spend(control: &DaemonControl, params: Params) -> Result>>() }) .ok_or_else(|| Error::invalid_params("Invalid 'destinations' parameter."))?; + let outpoints = params + .get(1, "outpoints") + .ok_or_else(|| Error::invalid_params("Missing 'outpoints' parameter."))? + .as_array() + .and_then(|arr| { + arr.iter() + .map(|entry| { + entry + .as_str() + .and_then(|e| bitcoin::OutPoint::from_str(e).ok()) + }) + .collect::>>() + }) + .ok_or_else(|| Error::invalid_params("Invalid 'outpoints' parameter."))?; let feerate: u64 = params .get(2, "feerate") .ok_or_else(|| Error::invalid_params("Missing 'feerate' parameter."))? @@ -43,7 +43,7 @@ fn create_spend(control: &DaemonControl, params: Params) -> Result