descriptors: improve satisfaction size estimate for single key

[jp1ac4]

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.

[darosior]

I think we should avoid hardcoding sizes ourselves as much as we can. Also, we should consider any primary path, not only those with a single key. Here is a diff which achieves this using rust-miniscript's planning module:

Click to expand diff

diff --git a/src/descriptors/mod.rs b/src/descriptors/mod.rs
index 605b13d..3cefe9f 100644
--- a/src/descriptors/mod.rs
+++ b/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(
@@ -233,40 +238,59 @@ impl LianaDescriptor {
     /// 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, use_primary_path: bool) -> usize {
-        let max_weight_to_satisfy =
-            if use_primary_path && matches!(self.policy().primary_path, PathInfo::Single(..)) {
-                match self.multi_desc {
-                    descriptor::Descriptor::Wsh(ref wsh) => {
-                        let redeem_script_size = match wsh.as_inner() {
-                            descriptor::WshInner::Ms(ms) => ms.script_size(),
-                            descriptor::WshInner::SortedMulti(..) => {
-                                unreachable!("None of our descriptors is a sorted multi")
-                            }
-                        };
-                        // script size + script + ECDSA signature
-                        // Assumes all ECDSA signatures are 73 bytes, including push opcode and
-                        // sighash suffix.
-                        1 + redeem_script_size + 73
-                    }
-                    descriptor::Descriptor::Tr(..) => {
-                        // Assumes all Schnorr signatures are 66 bytes, including push opcode and
-                        // sighash suffix.
-                        66
+        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()));
                     }
-                    _ => unreachable!("We only allow wsh and tr descriptors"),
-                }
-            } else {
-                self.multi_desc
-                    .max_weight_to_satisfy()
-                    .expect("Always satisfiable")
+                    keys
+                });
+            let assets = Assets {
+                keys,
+                ..Default::default()
             };
-        // 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.
-        max_weight_to_satisfy + 1
+
+            // 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
@@ -1112,6 +1136,48 @@ mod tests {
             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]

It's not as straightforward as it can get since their planning module is a bit inconsistent, still i think it's much cleaner. I've also expanded the inheritance_sat_size unit test with a Taproot descriptor which sanity checks the behaviour with a primary-path-as-internal-key.

[jp1ac4]

Thanks very much for the changes! ❤️ I've included them and updated a couple of functional tests.

[darosior]

ACK modulo my concern about how we can end up with a smaller feerate than estimated.

[darosior]

ACK 0eda557

I ran the test suite a few dozens of times in parallel to trigger any potential flakiness in the hardcoded fee values and didn't get any failure.

[darosior]

Nice. Would have been good to update the documentation of this function to mention the meaning given to the presence of a specified locktime though.