From e9c699570669e9723bec47b1fcd6df66257a8128 Mon Sep 17 00:00:00 2001 From: Michael Mallan Date: Fri, 8 Nov 2024 15:07:57 +0000 Subject: [PATCH 1/3] qa: add threshold paramater for multisig descs --- tests/fixtures.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/tests/fixtures.py b/tests/fixtures.py index 4c362c835..40c26c547 100644 --- a/tests/fixtures.py +++ b/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,9 +239,7 @@ 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, From cf88aca3aee0fed9a817dd4946db7ad3c1b080a7 Mon Sep 17 00:00:00 2001 From: Michael Mallan Date: Fri, 8 Nov 2024 09:41:00 +0000 Subject: [PATCH 2/3] descriptors: fix satisfaction size for Taproot This includes the sizes of the script and control block elements. --- src/descriptors/mod.rs | 57 ++++++++++++++++++++++++++++++++++-------- tests/fixtures.py | 27 ++++++++++++++++++++ tests/test_spend.py | 47 ++++++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 11 deletions(-) diff --git a/src/descriptors/mod.rs b/src/descriptors/mod.rs index 818ecb19c..2456f32e3 100644 --- a/src/descriptors/mod.rs +++ b/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,10 +260,11 @@ 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 @@ -270,15 +272,23 @@ impl LianaDescriptor { .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(); diff --git a/tests/fixtures.py b/tests/fixtures.py index 40c26c547..dc2860a88 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -258,6 +258,33 @@ def lianad_multisig(bitcoin_backend, directory): 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, + main_desc, + bitcoin_backend, + ) + + try: + lianad.start() + yield lianad + except Exception: + lianad.cleanup() + raise + + lianad.cleanup() + + def multipath_desc(multi_signer, csv_values, is_taproot): prim_multi = multi_expression(3, multi_signer.prim_hds, is_taproot) first_recov_multi = multi_expression( diff --git a/tests/test_spend.py b/tests/test_spend.py index 90680e1af..3f11874ec 100644 --- a/tests/test_spend.py +++ b/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 ( @@ -583,3 +585,48 @@ 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 + + # Due to https://github.com/wizardsardine/liana/pull/1323 we currently + # add 10 sats if feerate is 1. + extra = 10 if feerate == 1 else 0 + + # 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) + extra From 6e74a96e12feb00fcf0f6974f56002c0356e0960 Mon Sep 17 00:00:00 2001 From: Michael Mallan Date: Fri, 8 Nov 2024 15:09:47 +0000 Subject: [PATCH 3/3] Revert "spend: add 10 sats to fee for 1 sat/vb txs" This reverts commit 74a53baa39c409d8a39a27affc5ebac34128cb24. I also removed the extra sats being added in the functional test. --- src/commands/mod.rs | 18 +++++++++--------- src/spend.rs | 15 +-------------- tests/test_spend.py | 10 +++------- 3 files changed, 13 insertions(+), 30 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 3eca39acb..ae17e0ea8 100644 --- a/src/commands/mod.rs +++ b/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,7 +1606,7 @@ 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 }), @@ -1614,7 +1614,7 @@ mod tests { // 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() diff --git a/src/spend.rs b/src/spend.rs index 2c7aad3b8..9465b775e 100644 --- a/src/spend.rs +++ b/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, diff --git a/tests/test_spend.py b/tests/test_spend.py index 3f11874ec..19081dd80 100644 --- a/tests/test_spend.py +++ b/tests/test_spend.py @@ -136,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 ( @@ -155,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 ( @@ -623,10 +623,6 @@ def test_tr_multisig_2_of_2_feerate_is_met(feerate, lianad_multisig_2_of_2, bitc spend_weight = res["weight"] assert spend_weight == 646 - # Due to https://github.com/wizardsardine/liana/pull/1323 we currently - # add 10 sats if feerate is 1. - extra = 10 if feerate == 1 else 0 - # 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) + extra + assert spend_fee == math.ceil((646.0 / 4.0) * feerate)