fix: remove deprecated max_satisfaction_weight

[realeinherjar]

Description

Closes #1036.

• Change deprecated max_satisfaction_weight to max_weight_to_satisfy
• Remove #[allow(deprecated)] flags

Notes to the reviewers

I've changed all max_satisfaction_weight() to max_weight_to_satisfy()
in Wallet.get_available_utxo() and Wallet.build_fee_bump().
Checking the docs on the miniscript crate for max_weight_to_satisfy has the following note:

Since this method uses segwit_weight instead of legacy_weight,
if you want to include only legacy inputs in your transaction,
you should remove 1WU from each input's max_weight_to_satisfy
for a more accurate estimate.

Hence, I also added 1WU (4) to the max_weight_to_satisfy() calls.
But I think that there is a better way to do this instead of hardcoding 4.
Please advise.

Changelog notice

Fixed

Replace the deprecated max_satisfaction_weight from rust-miniscript to max_weight_to_satisfy.

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

[danielabrozzoni]

I'm pretty sure that adding a +4 is not enough to make the code correct, but proving it isn't as easy as I thought.

Adding +4 is fine for segwit transactions, but it's not for legacy ones. Looking at the bdk txbuilding tests, it seems to me that we never test with legacy descriptors:

bdk/crates/bdk/tests/common.rs

Lines 111 to 156 in 43fb0b2

	pub fn get_test_wpkh() -> &'static str {
	"wpkh(cVpPVruEDdmutPzisEsYvtST1usBR3ntr8pXSyt6D2YYqXRyPcFW)"
	}
	pub fn get_test_single_sig_csv() -> &'static str {
	// and(pk(Alice),older(6))
	"wsh(and_v(v:pk(cVpPVruEDdmutPzisEsYvtST1usBR3ntr8pXSyt6D2YYqXRyPcFW),older(6)))"
	}
	pub fn get_test_a_or_b_plus_csv() -> &'static str {
	// or(pk(Alice),and(pk(Bob),older(144)))
	"wsh(or_d(pk(cRjo6jqfVNP33HhSS76UhXETZsGTZYx8FMFvR9kpbtCSV1PmdZdu),and_v(v:pk(cMnkdebixpXMPfkcNEjjGin7s94hiehAH4mLbYkZoh9KSiNNmqC8),older(144))))"
	}
	pub fn get_test_single_sig_cltv() -> &'static str {
	// and(pk(Alice),after(100000))
	"wsh(and_v(v:pk(cVpPVruEDdmutPzisEsYvtST1usBR3ntr8pXSyt6D2YYqXRyPcFW),after(100000)))"
	}
	pub fn get_test_tr_single_sig() -> &'static str {
	"tr(cNJmN3fH9DDbDt131fQNkVakkpzawJBSeybCUNmP1BovpmGQ45xG)"
	}
	pub fn get_test_tr_with_taptree() -> &'static str {
	"tr(b511bd5771e47ee27558b1765e87b541668304ec567721c7b880edc0a010da55,{pk(cPZzKuNmpuUjD1e8jUU4PVzy2b5LngbSip8mBsxf4e7rSFZVb4Uh),pk(8aee2b8120a5f157f1223f72b5e62b825831a27a9fdf427db7cc697494d4a642)})"
	}
	pub fn get_test_tr_with_taptree_both_priv() -> &'static str {
	"tr(b511bd5771e47ee27558b1765e87b541668304ec567721c7b880edc0a010da55,{pk(cPZzKuNmpuUjD1e8jUU4PVzy2b5LngbSip8mBsxf4e7rSFZVb4Uh),pk(cNaQCDwmmh4dS9LzCgVtyy1e1xjCJ21GUDHe9K98nzb689JvinGV)})"
	}
	pub fn get_test_tr_repeated_key() -> &'static str {
	"tr(b511bd5771e47ee27558b1765e87b541668304ec567721c7b880edc0a010da55,{and_v(v:pk(cVpPVruEDdmutPzisEsYvtST1usBR3ntr8pXSyt6D2YYqXRyPcFW),after(100)),and_v(v:pk(cVpPVruEDdmutPzisEsYvtST1usBR3ntr8pXSyt6D2YYqXRyPcFW),after(200))})"
	}
	pub fn get_test_tr_single_sig_xprv() -> &'static str {
	"tr(tprv8ZgxMBicQKsPdDArR4xSAECuVxeX1jwwSXR4ApKbkYgZiziDc4LdBy2WvJeGDfUSE4UT4hHhbgEwbdq8ajjUHiKDegkwrNU6V55CxcxonVN/*)"
	}
	pub fn get_test_tr_with_taptree_xprv() -> &'static str {
	"tr(cNJmN3fH9DDbDt131fQNkVakkpzawJBSeybCUNmP1BovpmGQ45xG,{pk(tprv8ZgxMBicQKsPdDArR4xSAECuVxeX1jwwSXR4ApKbkYgZiziDc4LdBy2WvJeGDfUSE4UT4hHhbgEwbdq8ajjUHiKDegkwrNU6V55CxcxonVN/*),pk(8aee2b8120a5f157f1223f72b5e62b825831a27a9fdf427db7cc697494d4a642)})"
	}
	pub fn get_test_tr_dup_keys() -> &'static str {
	"tr(cNJmN3fH9DDbDt131fQNkVakkpzawJBSeybCUNmP1BovpmGQ45xG,{pk(8aee2b8120a5f157f1223f72b5e62b825831a27a9fdf427db7cc697494d4a642),pk(8aee2b8120a5f157f1223f72b5e62b825831a27a9fdf427db7cc697494d4a642)})"
	}

This should be improved. I tried to quickly add a get_test_pkh to (hopefully) make test_bump_fee_add_input fail, but assert_fee_rate! works for segwit inputs only (it uses the witness_utxo).

I would tackle this issue in steps:

1. Improve the current testing environment to also test for legacy inputs (as stated above, this would include improving the current asset_fee_rate)
2. If I'm correct, step 1 would introduce some tests that currently fail
3. Improve the current code. I would start with renaming the satisfaction_weight in WeightedUtxo to weight_to_satisfy for clarity, and adding a small description copy-pasting the meaning of weight_to_satisfy (can be copied from rust-ms). Then, change the code that uses WeightedUtxo::weight_to_satisfy. At the moment we have a TXIN_BASE_WEIGHT constant that we would add to satisfaction_weight, for example, see:

bdk/crates/bdk/src/wallet/coin_selection.rs

Line 311 in 43fb0b2

(TXIN_BASE_WEIGHT + weighted_utxo.satisfaction_weight) as u64,

We should instead have different constants (I think you can just use Txin::default().legacy_weight() and Txin::default().segwit_weight() and add the weight_to_satisfy to those constants based on whether the tx is segwit or not.

Just a small note: I understand that this issue is much bigger and more complicated than what it seemed. Feel free to ask for help when/if you need it, and to solve only parts of it (ex: only improve the testing framework) if you think it's best :)

[realeinherjar]

Adding +4 is fine for segwit transactions, but it's not for legacy ones

I totally agree with you.
The issue for me is not what needs to be done, but how are we gonna do it...
I know that this is urgent in the 1.0 roadmap

1. Improve the current testing environment to also test for legacy inputs (as stated above, this would include improving the current asset_fee_rate)

Yes, also agree that this is step 1.
I will work on that straight away.

Just a small note: I understand that this issue is much bigger and more complicated than what it seemed. Feel free to ask for help when/if you need it, and to solve only parts of it (ex: only improve the testing framework) if you think it's best :)

Yes, sure thanks. This PR could also be more than 2 hands.

TxIn::default().legacy_weight() and TxIn::default().segwit_weight()

I was exactly thinking that.
My early drafts used it.
I have one question about implementation:
What would be a good approach to figure it out which weight constant to add?
I was trying to do a match on Descriptor from miniscript.
However, I was trying to do it inside Wallet.build_fee_bump() and TxBuilder.add_utxos().
But, seeing your comments it seems that a better place would be CoinSelectionAlgorithm.select_sorted_utxos()?

[danielabrozzoni]

This PR could also be more than 2 hands.

I think the more people work on it, the harder it gets to communicate what needs to be done. Ofc many can help with reviews, but I'll let you do the code if you feel up for it :)

But, seeing your comments it seems that a better place would be CoinSelectionAlgorithm.select_sorted_utxos()?

I was initially thinking of some kind of is_segwit parameter in coin_select, but I now realize this isn't the clean way to go. The coin selection should only select utxos, the logic for calculating the weight should be done outside.

Matching on the DescriptorType is a good start (see https://docs.rs/miniscript/latest/miniscript/descriptor/enum.DescriptorType.html#method.segwit_version), but unfortunately not enough, as you also need to consider foreign utxos (consider a user with a legacy descriptor, adding a segwit foreign utxo).

One thing we might want to do is to consider all txs to be segwit by default, but then the user can customize this using a set_segwit method in the tx builder. We would then modify WeightedUtxo::satisfaction_weight to include the txin base weight (instead of it being added while selecting utxos).

However, I don't think this is a clean solution, it seems quite hacky and not really clear for the user.

We could also:

• Reconsider our priorities and push this issue to bdk 2.0 (or 3.0? I can't remember), when we'll have a decent interface for coin selection and tx building. Our fee estimation is quite broken anyways... We can keep using max_satisfaction_weight and calling it a day
• (Similarly to what it's done in this PR), assume segwit transactions, and overestimate for legacy transactions.

Thoughts? cc @evanlinjin @LLFourn

[realeinherjar]

• Reconsider our priorities and push this issue to bdk 2.0 (or 3.0? I can't remember), when we'll have a decent interface for coin selection and tx building. Our fee estimation is quite broken anyways... We can keep using max_satisfaction_weight and calling it a day

That is also valid, this was deprecated in Jan 2023 (see rust-bitcoin/rust-miniscript#476).
However, even if we close this by pushing this to 2.0+, I would still open a new PR to test more corner cases in fee bump and tx build (I didn't like that my hardcoded 4 passed all tests).

[notmandatory]

This one needs a rebase after alpha.5 release.

[storopoli]

I'm taking over this in a new PR with some tweaks and a rebase