Various fixes to the fee_amount calculation in create_tx

[danielabrozzoni]

Description

This PR mainly fixes two bugs:

1. TXIN_BASE_WEIGHT wrongly included the script_len (Fixes How to calculate TXIN_BASE_WEIGHT? #160)
2. We wouldn't take into account the segwit header in the fee calculation, which could have resulted in a transaction with a lower feerate than the requested one
3. In tests we used to push 108 bytes on the witness as a fake signature, but we should have pushed 106 instead

I also add a test to reproduce the conditions of #660, to check if it's solved. Turns out it's been solved already in #630, but if you're curious about what the bug was, here it is: #660 (comment)

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[evanlinjin]

Changes seem logical (based on my limited understanding), will try give a proper review later.

[evanlinjin]

The changes in this PR seem to make sense.

However, to ensure that this PR actually fixes #660, imo it will be helpful to write a test that exactly replicates the conditions which caused the problems in #660.

I.e.

• Start with wallet with balance of 8859 sats.
• Create tx with one recipient, sending 8630 sats & fee rate 2.01 vb/sat.
• Ensure created tx's sent + fee <= starting_balance.

I'm assuming the problem pointed out in #660 is actually, what should be the correct behavior when the fee_rate suggested surpasses the available wallet balance.

[evanlinjin]

Another idea would be to create psbts in a loop with different recipient amounts and ensure each psbt created does not surplus starting balance.

In that being said, this seems like a really annoying bug to iron out, lots of moving parts and things to think about.

[danielabrozzoni]

However, to ensure that this PR actually fixes #660, imo it will be helpful to write a test that exactly replicates the conditions which caused the problems in #660.

Thanks, I've just added the test in da712c4 :)

Another idea would be to create psbts in a loop with different recipient amounts and ensure each psbt created does not surplus starting balance.

I wrote it:

diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs
index 10c8cab..2f65cb2 100644
--- a/src/wallet/mod.rs
+++ b/src/wallet/mod.rs
@@ -3967,6 +3967,33 @@ pub(crate) mod test {
         builder.finish().unwrap();
     }
 
+    #[test]
+    fn test_fee_amount_many_cases() {
+        use rand::{SeedableRng, Rng, rngs::StdRng};
+        let seed = [0; 32];
+        let mut rng: StdRng = SeedableRng::from_seed(seed);
+        let (wallet, descriptors, _) = get_funded_wallet(get_test_wpkh());
+        let addr = Address::from_str("2N1Ffz3WaNzbeLFBb51xyFMHYSEUXcbiSoX").unwrap();
+        for _ in 0..100 {
+            let input_value = rng.gen_range(46_000, 100_000);
+            let incoming_txid = crate::populate_test_db!(
+                wallet.database.borrow_mut(),
+                testutils! (@tx ( (@external descriptors, 0) => input_value ) (@confirmations 1)),
+                Some(100),
+            );
+            let mut builder = wallet.build_tx();
+            builder
+                .add_recipient(addr.script_pubkey(), 45_000)
+                .add_utxo(OutPoint::new(incoming_txid, 0))
+                .unwrap()
+                .enable_rbf();
+            let (psbt, details) = builder.finish().unwrap();
+            let fee_amount = psbt.inputs.iter().fold(0, |acc, i| acc + i.witness_utxo.as_ref().unwrap().value) -
+                psbt.unsigned_tx.output.iter().fold(0, |acc, o| acc + o.value);
+            assert_eq!(fee_amount, details.fee.unwrap());
+        }
+    }
+
     #[test]
     fn test_sign_single_xprv() {
         let (wallet, _, _) = get_funded_wallet("wpkh(tprv8ZgxMBicQKsPd3EupYiPRhaMooHKUHJxNsTfYuScep13go8QFfHdtkG9nRkFGb7busX4isf6X9dURGCoKgitaApQ6MupRhZMcELAxTBRJgS/*)");

but then I couldn't find a way to make it fail on master (I tried changing the number of iterations and the input_value values), and we already have quite some tests checking the fee amount that wouldn't catch these bugs, so I figured it was useless.

This should be ready for review. I've also added a TL;DR of the various bug fixed in the PR description.

Sorry everyone if it took a while!

[rajarshimaitra]

Initial concept ACK.. I will make another deeper pass. For now I had few comment..

It took me sometime to wrap my head around whats going on around the fee thingy.. I guess this is solving the problem of #660 but I haven't tried it out yet. So far all the tests seems to be passing. But I don't think we are adequately testing the fee situations and edge cases. One that you have already mentioned in the TODO comment about adding new outputs.

The only fee related tests happens in the wallet module are by this macro

bdk/src/wallet/mod.rs

Lines 1983 to 2010 in 3644a45

	macro_rules! assert_fee_rate {
	($tx:expr, $fees:expr, $fee_rate:expr $( ,@dust_change $( $dust_change:expr )* )* $( ,@add_signature $( $add_signature:expr )* )* ) => ({
	let mut tx = $tx.clone();
	$(
	$( $add_signature )*
	for txin in &mut tx.input {
	txin.witness.push([0x00; 108]); // fake signature
	}
	)*
	#[allow(unused_mut)]
	#[allow(unused_assignments)]
	let mut dust_change = false;
	$(
	$( $dust_change )*
	dust_change = true;
	)*
	let tx_fee_rate = FeeRate::from_wu($fees, tx.weight());
	let fee_rate = $fee_rate;
	if !dust_change {
	assert!((tx_fee_rate - fee_rate).as_sat_vb().abs() < 0.5, "Expected fee rate of {:?}, the tx has {:?}", fee_rate, tx_fee_rate);
	} else {
	assert!(tx_fee_rate >= fee_rate, "Expected fee rate of at least {:?}, the tx has {:?}", fee_rate, tx_fee_rate);
	}
	});
	}

Which is used to check weather the fee is above certain threshold or not.. But it never asserts weather the fee reported in TransactionDetails is correct for the transaction. In fact it calculates the feerate from the result detail and then performs the check. So if the rate is wrong in the result, its wrong in the tests too..

I think we need to assert that the fees reported in the create transaction process is the correct fee.

But this is just one part of the problem..

The fee calculation is quite dynamic and its happening all over the places in the create_tx function. So its hard to reason about it.. Maybe we can streamline it somehow??

Another (unrelated to this PR) doubt I had is why the returning fee rate is 0 here?

bdk/src/wallet/mod.rs

Line 708 in 2de264e

(FeeRate::from_sat_per_vb(0.0), *fee)

So then if we use a FeePolicy::FeeAmount in policy then all the calculations you added after it is multiplied with a 0 fee rate??

Overall I also feel we should have some more dedicated fee related tests like the one you already added, to adequately handle these cases.

[danielabrozzoni]

Some good news: #660 has been solved by #630! 😁
This PR is still useful tho, I promise. I've added a test to make sure that #660 doesn't happen again

But I don't think we are adequately testing the fee situations and edge cases. One that you have already mentioned in the TODO comment about adding new outputs.
The only fee related tests happens in the wallet module are by this macro
Which is used to check weather the fee is above certain threshold or not.. But it never asserts weather the fee reported in TransactionDetails is correct for the transaction. In fact it calculates the feerate from the result detail and then performs the check. So if the rate is wrong in the result, its wrong in the tests too..

Right, I didn't notice! I've added the fee check in assert_fee_rate. Regarding the test I posted above, speaking with alekos we agreed that it's better to introduce fuzzing in bdk instead. PR coming soon (two weeks ™️)

The fee calculation is quite dynamic and its happening all over the places in the create_tx function. So its hard to reason about it.. Maybe we can streamline it somehow??

Yeah, you're right in saying that it's quite complicated and scattered around, but I don't know how we could improve it honestly :(

So then if we use a FeePolicy::FeeAmount in policy then all the calculations you added after it is multiplied with a 0 fee rate??

Yep! That's a cool trick to make sure that the fee returned in the transaction is the same as the user asked - maybe not so clear tho.
All around the coin selection and the create_tx code we have a fee_rate variable, to calculate the fee for adding each new utxo etc. We're basically saying "this thing has weight x, and we are using fee rate y", and then from x and y we can calculate the fee in sats from adding it to the transaction.
So, when the user tells you to have a 200 sats fees, you set the fee rate to zero. In this way, every time we're trying to add to the fee amount the weight of something, the fee added will be 0, and at the end of the calculations we'll have a tx with the fee specified by the user.

[rajarshimaitra]

tACK 09e0445

Changes looks good to me.. :)

[rajarshimaitra]

Yep! That's a cool trick to make sure that the fee returned in the transaction is the same as the user asked - maybe not so clear tho.

Ahh that's really neat.. Yup I missed that.. Thanks for updating the test macro and the tests.. It seems we are covering #660 all ok now.. Very much liked the docs in the test, makes them reading much easier..

To me this looks good to go..

Yeah, you're right in saying that it's quite complicated and scattered around, but I don't know how we could improve it honestly :(

Yeah me neither.. I would try to think over it and see if I can see some ways.. :)

[afilini]

ACK 09e0445

[afilini]

Code looks good, I'll try running this with my fuzzer before merging just to ensure we haven't missed something obvious

[afilini]

Ok we need some time to investigate this, because with this PR it looks like we sometimes end up with a feerate lower than we ask for. It's probably caused by the lowering of TXIN_BASE_WEIGHT, but we can't understand exactly how at the moment.

[danielabrozzoni]

Everything seems to be fixed now, I just pushed three additional commits (please don't hate me ❤️ )

Turns out that a constant in tests was wrong. I also added another check to assert_fee_rate, and added a test for high-R signatures (see #695)

[afilini]

An interesting thing to note here: while rust-bitcoin serializes txs with no inputs as segwit txs (hence, including the marker and flag bytes), the Transaction::weight() method doesn't!

I opened an issue to point out the inconsistency: rust-bitcoin/rust-bitcoin#1159

This is just to say that the current code that explicitly pushes the two extra weight units is actually correct, but this may change in the future if the weight method ends up being modified.

[afilini]

ACK 419dc24