Merge #1451: descriptors: fix max satisfaction weight for Taproot mul…

…tisig primary path spends

6e74a96 Revert "spend: add 10 sats to fee for 1 sat/vb txs" (Michael Mallan)
cf88aca descriptors: fix satisfaction size for Taproot (Michael Mallan)
e9c6995 qa: add threshold paramater for multisig descs (Michael Mallan)

Pull request description:

  This is a fix to #1371 and the related #1322.

  As per #1371 (comment), this adds the varint length for the script and control block elements of a Taproot primary path spend.

  I think this is what caused #1322 and so I have reverted the change from #1323.

ACKs for top commit:
  edouardparis:
    utACK 6e74a96

Tree-SHA512: 754e5c2186de853cde9006aeab7989141a4d363b9e3d8adb3f0264bcbca7e3b56a94822d434f6f7d8108e7c0e3023959fa407643e5868082f8981c5ce10b9a1e

src/commands/mod.rs

@@ -1497,7 +1497,7 @@ mod tests {
 
         // 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_829);
+        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()
@@ -1565,18 +1565,18 @@ mod tests {
             dummy_addr.payload().script_pubkey()
         );
         assert_eq!(tx.output[0].value.to_sat(), 95_000);
-        // change = 100_000 - 95_000 - /* fee without change */ 128 - /* extra fee for change output */ 43 = 4829
+        // change = 100_000 - 95_000 - /* fee without change */ 127 - /* extra fee for change output */ 43 = 4830
         assert_eq!(
             warnings,
             vec![
                 "Dust UTXO. The minimal change output allowed by Liana is 5000 sats. \
-                Instead of creating a change of 4829 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_829;
+        *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()
@@ -1591,7 +1591,7 @@ mod tests {
 
         // Now increase target also by the extra fee that was paying for change and we can still create the spend.
         *destinations.get_mut(&dummy_addr).unwrap() =
-            95_000 + 4_829 + /* fee for change output */ 43;
+            95_000 + 4_830 + /* fee for change output */ 43;
         let (psbt, warnings) = if let CreateSpendResult::Success { psbt, warnings } = control
             .create_spend(&destinations, &[dummy_op], 1, None)
             .unwrap()
@@ -1606,15 +1606,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_829 + /* 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 */ 128 - /* 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()
@@ -1635,7 +1635,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 */ 128 - /* 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()
@@ -1651,7 +1651,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 */ 128 - /* 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()

src/descriptors/mod.rs

@@ -6,6 +6,7 @@ use miniscript::{
         secp256k1,
     },
     descriptor,
+    miniscript::satisfy::Placeholder,
     plan::{Assets, CanSign},
     psbt::{PsbtInputExt, PsbtOutputExt},
     translate_hash_clone, ForEachKey, TranslatePk, Translator,
@@ -259,26 +260,35 @@ impl LianaDescriptor {
             };
 
             // 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.
+            // Taproot it considers the whole witness (except the control block size + the
+            // script size), while under P2WSH it does not consider the witscript! Therefore we
+            // manually add the size of the witscript under P2WSH by means of the
+            // `explicit_script()` helper, which gives an error for Taproot, and for Taproot
+            // we add the sizes of the control block and script.
             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);
+                .map(|s| varint_len(s.len()) + s.len());
 
             // Finally, compute the satisfaction template for the primary path and get its size.
-            der_desc
-                .plan(&assets)
-                .expect("Always satisfiable")
-                .witness_size()
-                + witscript_size
+            let plan = der_desc.plan(&assets).expect("Always satisfiable");
+            plan.witness_size()
+                + witscript_size.unwrap_or_else(|_| {
+                    plan.witness_template()
+                        .iter()
+                        .map(|elem| match elem {
+                            // We need to calculate the size manually before calculating the varint length.
+                            // See https://docs.rs/miniscript/11.0.0/src/miniscript/util.rs.html#35-36.
+                            Placeholder::TapScript(s) => varint_len(s.len()),
+                            Placeholder::TapControlBlock(cb) => varint_len(cb.serialize().len()),
+                            _ => 0,
+                        })
+                        .sum()
+                })
         } 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
@@ -1187,6 +1197,31 @@ mod tests {
         );
     }
 
+    #[test]
+    fn taproot_multisig_descriptor_sat_weight() {
+        // See https://mempool.space/signet/tx/84f09bddfe0f036d0390edf655636ad6092c3ab8f09b2bb1503caa393463f241
+        // for an example spend from this descriptor.
+        let desc = LianaDescriptor::from_str("tr(tpubD6NzVbkrYhZ4WUdbVsXDYBCXS8EPSYG1cAN9g4uP6uLQHMHXRvHSFkQBXy7MBeAvV8PDVJJ4o3AwYMKJHp45ci2g69UCAKteVSAJ61CnGEV/<0;1>/*,{and_v(v:pk([9e1c1983/48'/1'/0'/2']tpubDEWCLCMncbStq4BLXkQUAPqzzrh2tQUgYeQPt4NrB5D7gRraMyGbRqzPTmQGvqfdaFsXDVGSQBRgfXuNjDyfU626pxSjpQZszFNY6CzogxK/<2;3>/*),older(65535)),multi_a(2,[9e1c1983/48'/1'/0'/2']tpubDEWCLCMncbStq4BLXkQUAPqzzrh2tQUgYeQPt4NrB5D7gRraMyGbRqzPTmQGvqfdaFsXDVGSQBRgfXuNjDyfU626pxSjpQZszFNY6CzogxK/<0;1>/*,[3b1913e1/48'/1'/0'/2']tpubDFeZ2ezf4VUuTnjdhxJ1DKhLa2t6vzXZNz8NnEgeT2PN4pPqTCTeWUcaxKHPJcf1C8WzkLA71zSjDwuo4zqu4kkiL91ZUmJydC8f1gx89wM/<0;1>/*)})#ee0r4tw5").unwrap();
+        // varint_len(num witness elements) = 1
+        // varint_len(signature) + signature = 1 + 64
+        // varint_len(script) + script = 1 + 70
+        // varint_len(control block) + control block = 1 + 65
+        assert_eq!(
+            desc.max_sat_weight(true),
+            1 + (1 + 64) + (1 + 64) + (1 + 70) + (1 + 65)
+        );
+
+        // See https://mempool.space/signet/tx/63095cf6b5a57e5f3a7f0af0e22c8234cc4a4c1531c3236b00bd2a009f70e801
+        // for an example of a recovery transaction from the following descriptor:
+        // tr(tpubD6NzVbkrYhZ4XcC4HC7TDGrhraymFg9xo31hVtc5sh3dtsXbB5ZXewwMXi6HSmR2PyLeG8VwD3anqavSJVtXYJAAJcaEGCZdkBnnWTmhz3X/<0;1>/*,{and_v(v:pk([9e1c1983/48'/1'/0'/2']tpubDEWCLCMncbStq4BLXkQUAPqzzrh2tQUgYeQPt4NrB5D7gRraMyGbRqzPTmQGvqfdaFsXDVGSQBRgfXuNjDyfU626pxSjpQZszFNY6CzogxK/<2;3>/*),older(1)),multi_a(2,[88d8b4b9/48'/1'/0'/2']tpubDENzCJsHPDzX1EAP9eUPumw2hFUyjuUtBK8CWNPkudZTQ1mchX1hiAwog3fd6BKbq1rdZbLW3Q1d79AcvQCCMdehuSZ8GcShDcHaYTosCRa/<0;1>/*,[9e1c1983/48'/1'/0'/2']tpubDEWCLCMncbStq4BLXkQUAPqzzrh2tQUgYeQPt4NrB5D7gRraMyGbRqzPTmQGvqfdaFsXDVGSQBRgfXuNjDyfU626pxSjpQZszFNY6CzogxK/<0;1>/*)})#pepfj0gd
+        // Recovery path would use 1 + (1+64) + (1+36) + (1+65), but `max_sat_weight` considers all
+        // spending paths when passing `false`. So it currently gives the same as passing `true`.
+        // This `true` branch assumes a Schnorr signature of size 1+64+1, where the final +1 is for the sighash suffix:
+        // https://docs.rs/miniscript/11.0.0/src/miniscript/descriptor/tr.rs.html#254-301
+        // So we need to add 2, 1 for each signature.
+        assert_eq!(desc.max_sat_weight(false), desc.max_sat_weight(true) + 2);
+    }
+
     #[test]
     fn liana_desc_keys() {
         let secp = secp256k1::Secp256k1::signing_only();

src/spend.rs

@@ -12,7 +12,6 @@ use bdk_coin_select::{
     metrics::LowestFee, Candidate, ChangePolicy, CoinSelector, DrainWeights, FeeRate, Replace,
     Target, TargetFee, TargetOutputs, TXIN_BASE_WEIGHT,
 };
-use log::info;
 use miniscript::bitcoin::{
     self,
     absolute::{Height, LockTime},
@@ -385,21 +384,9 @@ fn select_coins_for_spend(
         long_term_feerate,
     );
 
-    // FIXME: This is a quick change to avoid going below the min relay fee:
-    // If the required feerate is 1 sat/vb and this is not a replacement tx,
-    // use the replaced_fee parameter to ensure we pay at least 10 sats more
-    // than the size of the tx.
-    // E.g. if vsize = 186, the fee will be pay >= 196 sats.
-    let replaced_fee_modified = if replaced_fee.is_none() && feerate_vb_u32 == 1 {
-        info!("setting replaced fee to 10");
-        Some(10)
-    } else {
-        replaced_fee
-    };
-
     // Finally, run the coin selection algorithm. We use an opportunistic BnB and if it couldn't
     // find any solution we fall back to selecting coins by descending value.
-    let replace = replaced_fee_modified.map(Replace::new);
+    let replace = replaced_fee.map(Replace::new);
     let target_fee = TargetFee {
         rate: feerate,
         replace,

tests/fixtures.py

@@ -216,10 +216,10 @@ def multi_expression(thresh, keys, is_taproot):
     return exp + ")"
 
 
-def multisig_desc(multi_signer, csv_value, is_taproot):
+def multisig_desc(multi_signer, csv_value, is_taproot, prim_thresh, recov_thresh):
     prim_multi, recov_multi = (
-        multi_expression(3, multi_signer.prim_hds, is_taproot),
-        multi_expression(2, multi_signer.recov_hds[csv_value], is_taproot),
+        multi_expression(prim_thresh, multi_signer.prim_hds, is_taproot),
+        multi_expression(recov_thresh, multi_signer.recov_hds[csv_value], is_taproot),
     )
     if is_taproot:
         all_xpubs = [
@@ -239,10 +239,35 @@ def lianad_multisig(bitcoin_backend, directory):
     # A 3-of-4 that degrades into a 2-of-5 after 10 blocks
     csv_value = 10
     signer = MultiSigner(4, {csv_value: 5}, is_taproot=USE_TAPROOT)
-    main_desc = Descriptor.from_str(
-        multisig_desc(signer, csv_value, is_taproot=USE_TAPROOT)
+    main_desc = Descriptor.from_str(multisig_desc(signer, csv_value, USE_TAPROOT, 3, 2))
+
+    lianad = Lianad(
+        datadir,
+        signer,
+        main_desc,
+        bitcoin_backend,
     )
 
+    try:
+        lianad.start()
+        yield lianad
+    except Exception:
+        lianad.cleanup()
+        raise
+
+    lianad.cleanup()
+
+
+@pytest.fixture
+def lianad_multisig_2_of_2(bitcoin_backend, directory):
+    datadir = os.path.join(directory, "lianad")
+    os.makedirs(datadir, exist_ok=True)
+
+    # A 2-of-2 that degrades into a 1-of-1 after 10 blocks
+    csv_value = 10
+    signer = MultiSigner(2, {csv_value: 1}, is_taproot=USE_TAPROOT)
+    main_desc = Descriptor.from_str(multisig_desc(signer, csv_value, USE_TAPROOT, 2, 1))
+
     lianad = Lianad(
         datadir,
         signer,

tests/test_spend.py

@@ -1,3 +1,5 @@
+import math
+
 from fixtures import *
 from test_framework.serializations import PSBT, uint256_from_str
 from test_framework.utils import (
@@ -134,7 +136,7 @@ def sign_and_broadcast(psbt):
     res = lianad.rpc.createspend(destinations, [outpoint], 1)
     psbt = PSBT.from_base64(res["psbt"])
     sign_and_broadcast(psbt)
-    change_amount = 848 if USE_TAPROOT else 829
+    change_amount = 858 if USE_TAPROOT else 839
     assert len(psbt.o) == 1
     assert len(res["warnings"]) == 1
     assert (
@@ -153,7 +155,7 @@ def sign_and_broadcast(psbt):
     res = lianad.rpc.createspend(destinations, [outpoint_3], 1)
     psbt = PSBT.from_base64(res["psbt"])
     sign_and_broadcast(psbt)
-    change_amount = 836 if USE_TAPROOT else 817
+    change_amount = 846 if USE_TAPROOT else 827
     assert len(psbt.o) == 1
     assert len(res["warnings"]) == 1
     assert (
@@ -583,3 +585,44 @@ def test_sweep(lianad, bitcoind):
         c["amount"] for c in lianad.rpc.listcoins(["unconfirmed", "confirmed"])["coins"]
     )
     assert balance == int((0.2 + 0.1 + 0.3) * COIN)
+
+
+@pytest.mark.parametrize("feerate", [1, 2])
+@pytest.mark.skipif(not USE_TAPROOT, reason="This tests a Taproot-specific bug.")
+def test_tr_multisig_2_of_2_feerate_is_met(feerate, lianad_multisig_2_of_2, bitcoind):
+    """
+    A regression test for https://github.com/wizardsardine/liana/issues/1371.
+
+    Test that a 2-of-2 Taproot primary path spend pays a high enough fee for
+    the target feerate.
+
+    See https://mempool.space/signet/tx/a63c4a69be71fcba0e16f742a2697401fbc47ad7dff10a790b8f961004aa0ab4
+    for an example of such a transaction.
+    """
+    # Get a coin.
+    deposit_txid = bitcoind.rpc.sendtoaddress(
+        lianad_multisig_2_of_2.rpc.getnewaddress()["address"], 0.001
+    )
+    bitcoind.generate_block(1, wait_for_mempool=deposit_txid)
+    wait_for(
+        lambda: len(lianad_multisig_2_of_2.rpc.listcoins(["confirmed"])["coins"]) == 1
+    )
+    coin = lianad_multisig_2_of_2.rpc.listcoins(["confirmed"])["coins"][0]
+
+    # Refresh the coin at the given feerate.
+    res = lianad_multisig_2_of_2.rpc.createspend({}, [coin["outpoint"]], feerate)
+    spend_psbt = PSBT.from_base64(res["psbt"])
+    signed_psbt = lianad_multisig_2_of_2.signer.sign_psbt(spend_psbt, [0, 1])
+    lianad_multisig_2_of_2.rpc.updatespend(signed_psbt.to_base64())
+    spend_txid = signed_psbt.tx.txid().hex()
+    lianad_multisig_2_of_2.rpc.broadcastspend(spend_txid)
+
+    # Check the tx fee and weight in mempool are as expected.
+    res = bitcoind.rpc.getmempoolentry(spend_txid)
+    spend_fee = res["fees"]["base"] * COIN
+    spend_weight = res["weight"]
+    assert spend_weight == 646
+
+    # Note that due to https://github.com/wizardsardine/liana/issues/1132
+    # we do not round up vbytes before multiplying by feerate.
+    assert spend_fee == math.ceil((646.0 / 4.0) * feerate)