Bump MSRV 1.63.0 AND add deprecate TxBuilder::allow_shrinking()

[notmandatory]

Description

fixes #1342

• bumps MSRV to 1.63
• fixed clippy errors for new MSRV
• add warning to docs for TxBuilder::allow_shrinking().
• add wallet test to confirm behavior of TxBuilder::allow_shrinking() when not also using drain_wallet().

Notes to the reviewers

This is a quick fix, long term we should try to make this safer to use.

Checklists

All Submissions:

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

[notmandatory]

Rather than try to figure out how to get this to build with MSRV 1.57 I'm bumping it to 1.63.0.

For some reason CI isn't running at all so I still working on that.

[thunderbiscuit]

I think the docs now correctly describe what the method is doing, but it feels like a bug to me, and the docs now simply describe the bug.

The intended behaviour (as I imagine it at least) is what is described in the first line of the doc:

Explicitly tells the wallet that it is allowed to reduce the amount of the output matching this script_pubkey in order to bump the transaction fee.

In other words, if you're bumping the fee on a transaction, you can tell the wallet to not apply default behaviour (use the change output to pay for the extra fees), but instead use one of your payees' output. If the extra fee required is 1000 sat, the output should simply shrink by 1000 sat, those sats go to the transaction fee, and nothing else about the transaction change. If the method ends up triggering a switcharoo of where change goes and now throws all change into that payees' output, it's so far off what I'd expect would happen from the name of the method that I'd consider it a bug.

If the current behaviour is useful in some way (I can't think of a use case but maybe there are?) we could rename the method to better describe what it does. Otherwise I think it would be better to fix the method to match what users might expect from an allow_shrinking() method, or disable it entirely if there is no bandwidth to fix at the moment.

[notmandatory]

@thunderbiscuit I think you are right now that I'm looking at the code again it looks like coin selection WILL add more inputs to pay the fee so will drain the whole wallet to the allow_shrinking() address if enabled.. so either we need to remove allow_shrinking() or always turn on manually_selected_only() for RBF or for RBF when using allow_shrinking(). I'll update my test to confirm. Update, I was wrong about this, see below.

[notmandatory]

I updated the test to confirm that given a wallet with 2 UTXOs, if the original TX only uses one UTXO then the new fee bumped TX with allow_shrinking() will also only use that one UTXO and NOT bring in the other available in the wallet.

I think this function is doing what it says which is allow shrinking one output to pay for a new larger TX fee. The only scenario I can think of where this behavior would be unexpected is if you also manually add new inputs with your fee bump tx builder. Should I explicitly warn against this scenario? I'd hope that the warning would cover that scenario, I'd rather not change the function name for the 0.xx branch, I'd rather have that discussion for the 1.0 bdk wallet api.

[LLFourn]

I think the docs now correctly describe what the method is doing, but it feels like a bug to me, and the docs now simply describe the bug.

Yes it's a design bug IMO: #1374

[danielabrozzoni]

utACK 5bc00e4 - looks good to me, I only have one comment regarding the clippy ci

[notmandatory]

I had to disable the code coverage and blockchain integration tests. They used to work but not are failing even though there haven't been any code changes since they last run successfully. I think time is better spent on 1.0 than trouble shooting these old flaky tests.