Merge #1125: descriptors: improve satisfaction size estimate for sing…

…le key

0eda557 descriptors: improve satisfaction size estimate for primary path (jp1ac4)

Pull request description:

  This is to resolve #1118.

  It improves the satisfaction size estimate in the case of a primary path spend involving a single key.

  Tests have been updated to account for transaction fees now being lower in some cases due to better estimates.

  It was already known that we sometimes end up with a transaction feerate (in sat/vb) lower than our target value, since our fee calculation from coin selection is based on sat/weight, which can give a sat/vb value lower than our target due to rounding. This will probably happen more often now given that we'll pay lower fees due to better size estimates.

ACKs for top commit:
  darosior:
    ACK 0eda557

Tree-SHA512: 588ded7d69e937dd32ee66ca0d59daad405a1bc4605eef1b6342788625f07a6b9d2c166fb35722d0f002e10104d6e57c13abfa7a7fbbc0fd84391b91c766b2aa

src/commands/mod.rs

@@ -1488,9 +1488,9 @@ mod tests {
         // rust-bitcoin's serialization of transactions with no input silently affected our fee
         // calculation.
 
-        // Transaction is 1 in (P2WSH satisfaction), 2 outs. At 1sat/vb, it's 170 sats fees.
+        // Transaction is 1 in (P2WSH satisfaction), 2 outs. At 1sat/vb, it's 161 sats fees.
         // At 2sats/vb, it's twice that.
-        assert_eq!(tx.output[1].value.to_sat(), 89_830);
+        assert_eq!(tx.output[1].value.to_sat(), 89_839);
         let psbt = if let CreateSpendResult::Success { psbt, .. } = control
             .create_spend(&destinations, &[dummy_op], 2, None)
             .unwrap()
@@ -1500,7 +1500,7 @@ mod tests {
             panic!("expect successful spend creation")
         };
         let tx = psbt.unsigned_tx;
-        assert_eq!(tx.output[1].value.to_sat(), 89_660);
+        assert_eq!(tx.output[1].value.to_sat(), 89_678);
 
         // A feerate of 555 won't trigger the sanity checks (they were previously not taking the
         // satisfaction size into account and overestimating the feerate).
@@ -1563,13 +1563,13 @@ mod tests {
             warnings,
             vec![
                 "Dust UTXO. The minimal change output allowed by Liana is 5000 sats. \
-                Instead of creating a change of 4830 sats, it was added to the \
+                Instead of creating a change of 4839 sats, it was added to the \
                 transaction fee. Select a larger input to avoid this from happening."
             ]
         );
 
         // Increase the target value by the change amount and the warning will disappear.
-        *destinations.get_mut(&dummy_addr).unwrap() = 95_000 + 4_830;
+        *destinations.get_mut(&dummy_addr).unwrap() = 95_000 + 4_839;
         let (psbt, warnings) = if let CreateSpendResult::Success { psbt, warnings } = control
             .create_spend(&destinations, &[dummy_op], 1, None)
             .unwrap()
@@ -1599,15 +1599,15 @@ mod tests {
 
         // Now increase the target by 1 more sat and we will have insufficient funds.
         *destinations.get_mut(&dummy_addr).unwrap() =
-            95_000 + 4_830 + /* fee for change output */ 43 + 1;
+            95_000 + 4_839 + /* fee for change output */ 43 + 1;
         assert_eq!(
             control.create_spend(&destinations, &[dummy_op], 1, None),
             Ok(CreateSpendResult::InsufficientFunds { missing: 1 }),
         );
 
         // Now decrease the target so that the lost change is just 1 sat.
         *destinations.get_mut(&dummy_addr).unwrap() =
-            100_000 - /* fee without change */ 127 - /* extra fee for change output */ 43 - 1;
+            100_000 - /* fee without change */ 118 - /* extra fee for change output */ 43 - 1;
         let warnings = if let CreateSpendResult::Success { warnings, .. } = control
             .create_spend(&destinations, &[dummy_op], 1, None)
             .unwrap()
@@ -1628,7 +1628,7 @@ mod tests {
 
         // Now decrease the target value so that we have enough for a change output.
         *destinations.get_mut(&dummy_addr).unwrap() =
-            95_000 - /* fee without change */ 127 - /* extra fee for change output */ 43;
+            95_000 - /* fee without change */ 118 - /* extra fee for change output */ 43;
         let (psbt, warnings) = if let CreateSpendResult::Success { psbt, warnings } = control
             .create_spend(&destinations, &[dummy_op], 1, None)
             .unwrap()
@@ -1644,7 +1644,7 @@ mod tests {
 
         // Now increase the target by 1 and we'll get a warning again, this time for 1 less than the dust threshold.
         *destinations.get_mut(&dummy_addr).unwrap() =
-            95_000 - /* fee without change */ 127 - /* extra fee for change output */ 43 + 1;
+            95_000 - /* fee without change */ 118 - /* extra fee for change output */ 43 + 1;
         let warnings = if let CreateSpendResult::Success { warnings, .. } = control
             .create_spend(&destinations, &[dummy_op], 1, None)
             .unwrap()
@@ -1697,12 +1697,8 @@ mod tests {
             spend_txid: None,
             spend_block: None,
         }]);
-        // Even though 1_000 is the max feerate allowed by our sanity check, we need to
-        // use 1_003 in order to exceed it and fail this test since coin selection is
-        // based on a minimum feerate of `feerate_vb / 4.0` sats/wu, which can result in
-        // the sats/vb feerate being lower than `feerate_vb`.
         assert_eq!(
-            control.create_spend(&destinations, &[dummy_op_dup], 1_003, None),
+            control.create_spend(&destinations, &[dummy_op_dup], 1_001, None),
             Err(CommandError::SpendCreation(SpendCreationError::InsaneFees(
                 InsaneFeeInfo::TooHighFeerate(1_001)
             )))

src/descriptors/mod.rs

@@ -6,12 +6,13 @@ use miniscript::{
         secp256k1,
     },
     descriptor,
+    plan::{Assets, CanSign},
     psbt::{PsbtInputExt, PsbtOutputExt},
     translate_hash_clone, ForEachKey, TranslatePk, Translator,
 };
 
 use std::{
-    collections::{BTreeMap, HashMap, HashSet},
+    collections::{BTreeMap, BTreeSet, HashMap, HashSet},
     convert::TryInto,
     error, fmt,
     str::{self, FromStr},
@@ -56,6 +57,10 @@ impl From<LianaPolicyError> for LianaDescError {
     }
 }
 
+fn varint_len(n: usize) -> usize {
+    bitcoin::VarInt(n as u64).size()
+}
+
 // Whether the key identified by its fingerprint+derivation path was derived from one of the xpubs
 // for this spending path.
 fn key_is_for_path(
@@ -228,32 +233,73 @@ impl LianaDescriptor {
             .0
     }
 
-    // TODO: on Taproot we should use this for recovery but keyspend size if there is a spendable
-    // internal key.
     /// Get the maximum size difference of a transaction input spending a Script derived from this
     /// descriptor before and after satisfaction. The returned value is in weight units.
     /// Callers are expected to account for the Segwit marker (2 WU). This takes into account the
     /// size of the witness stack length varint.
-    pub fn max_sat_weight(&self) -> usize {
-        // We add one to account for the witness stack size, as the `max_weight_to_satisfy` method
-        // computes the difference in size for a satisfied input that was *already* in a
-        // transaction that spent one or more Segwit coins (and thus already have 1 WU accounted
-        // for the emtpy witness). But this method is used to account between a completely "nude"
-        // transaction (and therefore no Segwit marker nor empty witness in inputs) and a satisfied
-        // transaction.
-        self.multi_desc
-            .max_weight_to_satisfy()
-            .expect("Always satisfiable")
-            + 1
+    pub fn max_sat_weight(&self, use_primary_path: bool) -> usize {
+        if use_primary_path {
+            // Get the keys from the primary path, to get a satisfaction size estimation only
+            // considering those.
+            let keys = self
+                .policy()
+                .primary_path
+                .thresh_origins()
+                .1
+                .into_iter()
+                .fold(BTreeSet::new(), |mut keys, (fg, der_paths)| {
+                    for der_path in der_paths {
+                        keys.insert(((fg, der_path), CanSign::default()));
+                    }
+                    keys
+                });
+            let assets = Assets {
+                keys,
+                ..Default::default()
+            };
+
+            // Unfortunately rust-miniscript satisfaction size estimation is inconsistent. For
+            // Taproot it considers the whole witness (including the control block size + the
+            // script size) but under P2WSH it does not consider the witscript! Therefore we
+            // manually add the size of the witscript, but only under P2WSH by the mean of the
+            // `explicit_script()` helper.
+            let der_desc = self
+                .receive_desc
+                .0
+                .at_derivation_index(0)
+                .expect("unhardened index");
+            let witscript_size = der_desc
+                .explicit_script()
+                .map(|s| varint_len(s.len()) + s.len())
+                .unwrap_or(0);
+
+            // Finally, compute the satisfaction template for the primary path and get its size.
+            der_desc
+                .plan(&assets)
+                .expect("Always satisfiable")
+                .witness_size()
+                + witscript_size
+        } else {
+            // We add one to account for the witness stack size, as the values above give the
+            // difference in size for a satisfied input that was *already* in a transaction
+            // that spent one or more Segwit coins (and thus already have 1 WU accounted for the
+            // emtpy witness). But this method is used to account between a completely "nude"
+            // transaction (and therefore no Segwit marker nor empty witness in inputs) and a
+            // satisfied transaction.
+            self.multi_desc
+                .max_weight_to_satisfy()
+                .expect("Always satisfiable")
+                + 1
+        }
     }
 
     /// Get the maximum size difference of a transaction input spending a Script derived from this
     /// descriptor before and after satisfaction. The returned value is in (rounded up) virtual
     /// bytes.
     /// Callers are expected to account for the Segwit marker (2 WU). This takes into account the
     /// size of the witness stack length varint.
-    pub fn max_sat_vbytes(&self) -> usize {
-        self.max_sat_weight()
+    pub fn max_sat_vbytes(&self, use_primary_path: bool) -> usize {
+        self.max_sat_weight(use_primary_path)
             .checked_add(WITNESS_SCALE_FACTOR - 1)
             .unwrap()
             .checked_div(WITNESS_SCALE_FACTOR)
@@ -262,9 +308,9 @@ impl LianaDescriptor {
 
     /// Get the maximum size in virtual bytes of the whole input in a transaction spending
     /// a coin with this Script.
-    pub fn spender_input_size(&self) -> usize {
+    pub fn spender_input_size(&self, use_primary_path: bool) -> usize {
         // txid + vout + nSequence + empty scriptSig + witness
-        32 + 4 + 4 + 1 + self.max_sat_vbytes()
+        32 + 4 + 4 + 1 + self.max_sat_vbytes(use_primary_path)
     }
 
     /// Whether this is a Taproot descriptor.
@@ -492,10 +538,10 @@ impl LianaDescriptor {
     /// Maximum possible size in vbytes of an unsigned transaction, `tx`,
     /// after satisfaction, assuming all inputs of `tx` are from this
     /// descriptor.
-    pub fn unsigned_tx_max_vbytes(&self, tx: &bitcoin::Transaction) -> u64 {
+    pub fn unsigned_tx_max_vbytes(&self, tx: &bitcoin::Transaction, use_primary_path: bool) -> u64 {
         let witness_factor: u64 = WITNESS_SCALE_FACTOR.try_into().unwrap();
         let num_inputs: u64 = tx.input.len().try_into().unwrap();
-        let max_sat_weight: u64 = self.max_sat_weight().try_into().unwrap();
+        let max_sat_weight: u64 = self.max_sat_weight(use_primary_path).try_into().unwrap();
         // Add weights together before converting to vbytes to avoid rounding up multiple times.
         let tx_wu = tx
             .weight()
@@ -1055,27 +1101,83 @@ mod tests {
     #[test]
     fn inheritance_descriptor_sat_size() {
         let desc = LianaDescriptor::from_str("wsh(or_d(pk([92162c45]tpubD6NzVbkrYhZ4WzTf9SsD6h7AH7oQEippXK2KP8qvhMMqFoNeN5YFVi7vRyeRSDGtgd2bPyMxUNmHui8t5yCgszxPPxMafu1VVzDpg9aruYW/<0;1>/*),and_v(v:pkh([abcdef01]tpubD6NzVbkrYhZ4Wdgu2yfdmrce5g4fiH1ZLmKhewsnNKupbi4sxjH1ZVAorkBLWSkhsjhg8kiq8C4BrBjMy3SjAKDyDdbuvUa1ToAHbiR98js/<0;1>/*),older(2))))#ravw7jw5").unwrap();
-        assert_eq!(desc.max_sat_vbytes(), (1 + 66 + 1 + 34 + 73 + 3) / 4); // See the stack details below.
+        // See the stack details below.
+        assert_eq!(desc.max_sat_vbytes(true), (1 + 66 + 73 + 3) / 4);
+        assert_eq!(desc.max_sat_vbytes(false), (1 + 66 + 1 + 34 + 73 + 3) / 4);
 
         // Maximum input size is (txid + vout + scriptsig + nSequence + max_sat).
         // Where max_sat is:
         // - Push the witness stack size
         // - Push the script
+        // If recovery:
         // - Push an empty vector for using the recovery path
         // - Push the recovery key
-        // - Push a signature for the recovery key
+        // EndIf
+        // - Push a signature for the primary/recovery key
         // NOTE: The specific value is asserted because this was tested against a regtest
         // transaction.
+        let stack = vec![vec![0; 65], vec![0; 72]];
+        let witness_size = bitcoin::VarInt(stack.len() as u64).size()
+            + stack
+                .iter()
+                .map(|item| bitcoin::VarInt(item.len() as u64).size() + item.len())
+                .sum::<usize>();
+        assert_eq!(
+            desc.spender_input_size(true),
+            32 + 4 + 1 + 4 + wu_to_vb(witness_size),
+        );
         let stack = vec![vec![0; 65], vec![0; 0], vec![0; 33], vec![0; 72]];
         let witness_size = bitcoin::VarInt(stack.len() as u64).size()
             + stack
                 .iter()
                 .map(|item| bitcoin::VarInt(item.len() as u64).size() + item.len())
                 .sum::<usize>();
         assert_eq!(
-            desc.spender_input_size(),
+            desc.spender_input_size(false),
             32 + 4 + 1 + 4 + wu_to_vb(witness_size),
         );
+
+        // Now perform the sanity checks under Taproot.
+        let owner_key = PathInfo::Single(descriptor::DescriptorPublicKey::from_str("[abcdef01]xpub6Eze7yAT3Y1wGrnzedCNVYDXUqa9NmHVWck5emBaTbXtURbe1NWZbK9bsz1TiVE7Cz341PMTfYgFw1KdLWdzcM1UMFTcdQfCYhhXZ2HJvTW/<0;1>/*").unwrap());
+        let heir_key = PathInfo::Single(descriptor::DescriptorPublicKey::from_str("[abcdef01]xpub688Hn4wScQAAiYJLPg9yH27hUpfZAUnmJejRQBCiwfP5PEDzjWMNW1wChcninxr5gyavFqbbDjdV1aK5USJz8NDVjUy7FRQaaqqXHh5SbXe/<0;1>/*").unwrap());
+        let timelock = 52560;
+        let desc = LianaDescriptor::new(
+            LianaPolicy::new(
+                owner_key.clone(),
+                [(timelock, heir_key.clone())].iter().cloned().collect(),
+            )
+            .unwrap(),
+        );
+
+        // If using the primary path, it's a keypath spend.
+        assert_eq!(desc.max_sat_vbytes(true), (1 + 65 + 3) / 4);
+        // If using the recovery path, it's a script path spend. The script is 40 bytes long. The
+        // control block is just the internal key and parity, so 33 bytes long.
+        assert_eq!(
+            desc.max_sat_vbytes(false),
+            (1 + 65 + 1 + 40 + 1 + 33 + 3) / 4
+        );
+
+        // The same against the spender_input_size() helper, adding the size of the txin and
+        // checking against a dummy witness stack.
+        fn wit_size(stack: &[Vec<u8>]) -> usize {
+            varint_len(stack.len())
+                + stack
+                    .iter()
+                    .map(|item| varint_len(item.len()) + item.len())
+                    .sum::<usize>()
+        }
+        let txin_boilerplate = 32 + 4 + 1 + 4;
+        let stack = vec![vec![0; 64]];
+        assert_eq!(
+            desc.spender_input_size(true),
+            txin_boilerplate + wu_to_vb(wit_size(&stack)),
+        );
+        let stack = vec![vec![0; 33], vec![0; 40], vec![0; 64]];
+        assert_eq!(
+            desc.spender_input_size(false),
+            txin_boilerplate + wu_to_vb(wit_size(&stack)),
+        );
     }
 
     #[test]

src/spend.rs

@@ -99,6 +99,7 @@ fn check_output_value(value: bitcoin::Amount) -> Result<(), SpendCreationError>
 fn sanity_check_psbt(
     spent_desc: &descriptors::LianaDescriptor,
     psbt: &Psbt,
+    use_primary_path: bool,
 ) -> Result<(), SpendCreationError> {
     let tx = &psbt.unsigned_tx;
 
@@ -137,7 +138,7 @@ fn sanity_check_psbt(
     }
 
     // Check the feerate isn't insane.
-    let tx_vb = spent_desc.unsigned_tx_max_vbytes(tx);
+    let tx_vb = spent_desc.unsigned_tx_max_vbytes(tx, use_primary_path);
     let feerate_sats_vb = abs_fee
         .checked_div(tx_vb)
         .ok_or(SpendCreationError::InsaneFees(
@@ -663,6 +664,13 @@ pub fn create_spend(
         value: bitcoin::Amount::MAX,
         script_pubkey: change_addr.addr.script_pubkey(),
     };
+    // If no candidates have relative locktime, then we should use the primary spending path.
+    // Note we set this value before actually selecting the coins, but we expect either all
+    // candidates or none to have relative locktime sequence so this is fine.
+    let use_primary_path = !candidate_coins
+        .iter()
+        .filter_map(|cand| cand.sequence)
+        .any(|seq| seq.is_relative_lock_time());
     // Now select the coins necessary using the provided candidates and determine whether
     // there is any leftover to create a change output.
     let CoinSelectionRes {
@@ -684,7 +692,7 @@ pub fn create_spend(
         }
         .into();
         let max_sat_wu = main_descriptor
-            .max_sat_weight()
+            .max_sat_weight(use_primary_path)
             .try_into()
             .expect("Weight must fit in a u32");
         select_coins_for_spend(
@@ -771,7 +779,7 @@ pub fn create_spend(
         inputs: psbt_ins,
         outputs: psbt_outs,
     };
-    sanity_check_psbt(main_descriptor, &psbt)?;
+    sanity_check_psbt(main_descriptor, &psbt, use_primary_path)?;
     // TODO: maybe check for common standardness rules (max size, ..)?
 
     Ok(CreateSpendRes {

tests/test_chain.py

@@ -446,10 +446,10 @@ def test_spend_replacement(lianad, bitcoind):
     destinations = {
         bitcoind.rpc.getnewaddress(): 650_000,
     }
-    second_res = lianad.rpc.createspend(destinations, second_outpoints, 2)
+    second_res = lianad.rpc.createspend(destinations, second_outpoints, 3)
     second_psbt = PSBT.from_base64(second_res["psbt"])
     destinations = {}
-    third_res = lianad.rpc.createspend(destinations, second_outpoints, 4)
+    third_res = lianad.rpc.createspend(destinations, second_outpoints, 5)
     third_psbt = PSBT.from_base64(third_res["psbt"])
 
     # Broadcast the first transaction. Make sure it's detected.