refactor(wallet)!: Use Weight type instead of usize

[oleonardolima]

fixes #1466
depends on #1448

Description

This PR is a follow-up on top of #1448, and should be rebased and merged after it, it uses the rust-bitcoin Weight type instead of the current usize usage.

NOTE: It also adds a new MiniscriptError variant, and remove the .unwrap() usage. As suggested I'll address this on another issue #1485, trying to reproduce the error first.

Notes to the reviewers

It should be ready to review after #1448 gets merged.

Changelog notice

• Change WeightedUtxo satisfaction_weight has been changed to use Weight type.
• Change TxBuilder methods add_foreign_utxo and add_foreign_utxo_with_sequence to expect Weight as satisfaction_weight type.

Checklists

All Submissions:

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

[oleonardolima]

Okay we need to create a ticket for what we should do when a caller creates a wallet with descriptors that are insatiable. Panicing with no clear reason when they try construct a tx is not the right behavior imo. Out of scope for this PR.

@evanlinjin mentioned it the #1448 (comment). I'm moving the discussion to here as I've tried to address it here at 51e6f6d.

edit: also refer to upstream issue rust-bitcoin/rust-miniscript#695

[apoelstra]

Ok, moving my comment here:

Regarding the error path of max_weight_to_satisfy when you try to give it a (literally) unspendable descriptor like wsh(0) or something, I see a couple ways you can handle this:

• Try to propagate the error from the max_weight_to_satisfy call, treating it similarly to a transient "cannot spend" condition.
• Use .unwrap_or(Weight::MAX) which will have roughly the correct semantics without causing errors (though it could cause panics down the line if you add with it)
• Petition upstream to add a flag to ExtParams that would refuse to parse such scripts, then you can unwrap. Then users would be unable to import such scripts. Maybe you want them to be able to, though, for watch-only wallets?

[oleonardolima]

Thanks for the suggestions! Although I've tried to do something similar to option (1), I'm now wondering about option (2), still need to think about the possible panics down the line 🤔

[ValuedMammal]

I'm wondering whether it's even possible to create a bdk Wallet with an unsatisfiable descriptor

[storopoli]

I think that what idea (2) meant was to use the Weight::MAX which is actually u64::MAX (Ref: https://docs.rs/bitcoin-units/0.1.1/src/bitcoin_units/weight.rs.html#35).
That might hit the DEFAULT_MAX_FEE_RATE which is 25_000 sats/vB (Ref: https://docs.rs/bitcoin/latest/src/bitcoin/psbt/mod.rs.html#133.

If we hit that, then we get a ExtractTxError.

I think that's what it is meant as could cause panics.

What am I missing? I probably could be wrong here.
I am assuming that Psbt::extract_tx_fee_rate_limit() is being called somewhere down the line, right?

[ValuedMammal]

I'm also ok with just fixing the usize to Weight stuff in this PR and creating a separate issue to decide how to handle an unsatisfiable descriptor

[oleonardolima]

I'm also ok with just fixing the usize to Weight stuff in this PR and creating a separate issue to decide how to handle an unsatisfiable descriptor

Yes, I removed the other commit. I only fixes the usage of usize now.

[ValuedMammal]

ACK 729fec6

I know this is picky but since this change is API breaking, the commit should tell us that in conventional commits style, so something like

refactor(wallet)!: change WeightedUtxo to use Weight type

In general the changelog should communicate any API changes so in this case the fact that WeightedUtxo is changed as well as the function signatures for add_foreign_utxo{_with_sequence}

Note to reviewers we didn't fix the unwrap for max_weight_to_satisfy but an issue has been created to discuss further.

[oleonardolima]

ACK 729fec6

I know this is picky but since this change is API breaking, the commit should tell us that in conventional commits style, so something like

refactor(wallet)!: change WeightedUtxo to use Weight type

In general the changelog should communicate any API changes so in this case the fact that WeightedUtxo is changed as well as the function signatures for add_foreign_utxo{_with_sequence}

Note to reviewers we didn't fix the unwrap for max_weight_to_satisfy but an issue has been created to discuss further.

No worries, thanks for the suggestions. 🚀

[storopoli]

I'm also ok with just fixing the usize to Weight stuff in this PR and creating a separate issue to decide how to handle an unsatisfiable descriptor

I second this. Do we have the issue open? I did not see linked to this PR.

Anyways, ACK 438cd46

[oleonardolima]

I second this. Do we have the issue open? I did not see linked to this PR.

Yes, it's on the PR description, issue: #1485

[notmandatory]

ACK 438cd46