spend: feerate can be lower than target value

[jp1ac4]

The fee of a Taproot (only affects Taproot?) transaction can sometimes be 1 sat lower than required to achieve the target feerate as defined by the coin selection algorithm, i.e. feerate = fee / (weight/4).

Note that #1132 is similar but relates to whether or not the vbytes are rounded up when calculating the feerate.

If we fix this, then we should revert the change from #1323.

[jp1ac4]

This is possibly related to #1322.

[jp1ac4]

I was able to reproduce this on Signet with txid a63c4a69be71fcba0e16f742a2697401fbc47ad7dff10a790b8f961004aa0ab4.

It's a spend from the primary path of a Taproot descriptor, where the primary path is a 2-of-2 multisig, with target feerate 2 sat/vb.

The tx weight is 646 and fee 322. Even if calculating the feerate without rounding up the vbytes, this gives 322 / (646/4) = 1.99.

[jp1ac4]

I think we're currently missing the varint length for TapControlBlock and TapScript, which is why this issue doesn't affect a key spend:
https://docs.rs/miniscript/11.0.0/src/miniscript/util.rs.html#19-39

This is also mentioned here:
rust-bitcoin/rust-miniscript#481 (comment)

[jp1ac4]

Perhaps something like this could work:

diff --git a/src/descriptors/mod.rs b/src/descriptors/mod.rs
index 818ecb19..6ee1034a 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,
@@ -270,15 +271,21 @@ 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 {
+                            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

I did a test with this change using the same type of tx as before, and now the fee correctly came to 323 sats.

[jp1ac4]

I think the change above may be enough to fix the fee issue.

There's a separate, less severe issue to do with the estimated satisfied weight of a transaction here:

liana/src/descriptors/mod.rs

Lines 541 to 557 in cbc46e9

	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(use_primary_path).try_into().unwrap();
	// Add weights together before converting to vbytes to avoid rounding up multiple times.
	let tx_wu = tx
	.weight()
	.to_wu()
	.checked_add(max_sat_weight.checked_mul(num_inputs).unwrap())
	.unwrap();
	tx_wu
	.checked_add(witness_factor.checked_sub(1).unwrap())
	.unwrap()
	.checked_div(witness_factor)
	.unwrap()
	}
	}

This is used to display the feerate in the GUI. It currently gives the satisfied weight as 642, which, even if we add 2 with the change above, will leave us 2 short. I think this discrepancy may be because when we get the weight of the unsigned tx, it doesn't include the Segwit marker and flag. I think this is because all inputs have empty witnesses, and so the transaction is serialized as non-Segwit:
https://docs.rs/bitcoin/0.31.0/src/bitcoin/blockdata/transaction.rs.html#968-979