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

notmandatory · 2024-03-04T00:20:54Z

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 · 2024-03-04T02:37:38Z

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 · 2024-03-12T14:50:08Z

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 · 2024-03-12T22:53:52Z

@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.

Fix related clippy errors

notmandatory · 2024-03-15T03:18:50Z

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 · 2024-03-15T04:17:50Z

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

notmandatory · 2024-03-16T01:51:46Z

How about we just deprecate allow_shrinking() in this release (and in corresponding bdk-ffi API) in case anyone is using it, and then remove it completely in the next release (and in the 1.0.0 api)? Then we have time to redesign txbuilder for 1.1 or 2.0 milestone.

…te it test(wallet): add test_bump_fee_allow_shrinking test

danielabrozzoni

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

danielabrozzoni · 2024-04-18T10:39:33Z

.github/workflows/cont_integration.yml

@@ -18,9 +18,9 @@ jobs:
    strategy:
      matrix:
        rust:
-          - version: 1.65.0 # STABLE
+          - version: stable


This means that if a new version of rust is released, with a new version of clippy with stricter rules, our ci would immediately fail the clippy check. Are we ok with this?

I don't think so because of

bdk/clippy.toml

Line 1 in 52f3955

msrv="1.63.0"

Ref: https://doc.rust-lang.org/stable/clippy/configuration.html

I think it means that clippy won't suggest any improvement that breaks msrv, not that clippy is pinned to version 1.63.0.

From the Ref above:

Projects that intend to support old versions of Rust can disable lints pertaining to newer features by specifying the minimum supported Rust version (MSRV) in the clippy configuration file.

So I think that you worry about

if a new version of rust is released, with a new version of clippy with stricter rules, our ci would immediately fail the clippy check

would not fail right?

I think it would fail.
Let's say that rust 1.80 introduces the method pizza(), and clippy 1.80 introduces two new rules:

pizza() should always be used instead of format()

Every variable name should start with a x (this makes your code more beautiful)

If your msrv is 1.79, clippy wouldn't complain about rule 1: you can't use pizza(), as it doesn't exist in rust 1.79! But I'm kind of sure that it would still warn you about rule 2, since it's doable in earlier versions of rust too.

Anyways, I don't know how often clippy adds rules that don't pertain to new rust features, so it might not be a big deal after all.

Yes but first MSRV CI would fail because you cannot include pizza() since it does not exist in MSRV.

And even if you don't include it the clippy.toml MSRV config will make sure to not enable the pizza_instead of format lint because of

Projects that intend to support old versions of Rust can disable lints pertaining to newer features by specifying the minimum supported Rust version (MSRV) in the clippy configuration file.

Yes but first MSRV CI would fail because you cannot include pizza() since it does not exist in MSRV.

I never said I included pizza in my code. I said that clippy had that rule, but wouldn't enforce it since my MSRV wouldn't permit it.

The point is not the rule1, it is rule2 that would make the CI fail.

Rule 2 is a "newer feature" that I've mentioned above.
So CI with clippy.toml would pass.

No, rule 2 is a new clippy lint that can be applied to also older versions of rust, not a new feature.
Anyways, I don't have the time to try this out, I'd say it's ok to merge and if you ever notice that clippy breaks when a new version is released, and it's too annoying to fix, you can change the ci again.

I don't think that this is how the clippy lints are versioned.
Because if you run clippy with Rust 1.X.0 it means that you are running a snapshot of the codebase of rust, cargo, clippy, and rustfmt from the time that 1.X.0 was released.

I see you scenario happening with some backport like 1.X.1 or 1.X.2 etc.

danielabrozzoni · 2024-04-18T10:43:10Z

src/wallet/signer.rs

 pub enum TapLeavesOptions {
    /// The signer will sign all the leaves it has a key for.
+    #[default]


Ohh this is so cool!

notmandatory added this to the 0.29.1 milestone Mar 4, 2024

notmandatory self-assigned this Mar 4, 2024

notmandatory added the documentation Improvements or additions to documentation label Mar 4, 2024

notmandatory mentioned this pull request Mar 4, 2024

allow_shrinking not working as expected #1342

Open

notmandatory added the ci label Mar 4, 2024

notmandatory marked this pull request as draft March 4, 2024 02:36

notmandatory force-pushed the docs/allow_shrinking branch from a24fd81 to bf0845c Compare March 4, 2024 02:40

notmandatory marked this pull request as ready for review March 4, 2024 04:23

notmandatory force-pushed the docs/allow_shrinking branch 7 times, most recently from 127b29c to 3b09e26 Compare March 4, 2024 07:19

notmandatory force-pushed the docs/allow_shrinking branch 2 times, most recently from 46a3c64 to 21f9d69 Compare March 15, 2024 02:16

ci: bump MSRV to 1.63.0

51f2982

Fix related clippy errors

notmandatory force-pushed the docs/allow_shrinking branch from 21f9d69 to 9ee2ce6 Compare March 15, 2024 02:21

notmandatory changed the title ~~docs(wallet): add warning on TxBuilder::allow_shrinking()~~ docs(wallet): add warning on TxBuilder::allow_shrinking() AND bump MSRV 1.63.0 Mar 15, 2024

notmandatory changed the title ~~docs(wallet): add warning on TxBuilder::allow_shrinking() AND bump MSRV 1.63.0~~ bump MSRV 1.63.0 AND add warning to docs for TxBuilder::allow_shrinking() Mar 15, 2024

notmandatory changed the title ~~bump MSRV 1.63.0 AND add warning to docs for TxBuilder::allow_shrinking()~~ Bump MSRV 1.63.0 AND add warning to docs for TxBuilder::allow_shrinking() Mar 15, 2024

notmandatory force-pushed the docs/allow_shrinking branch from 9ee2ce6 to 2e679e6 Compare March 15, 2024 02:59

notmandatory added module-wallet api A breaking API change labels Mar 16, 2024

notmandatory removed the documentation Improvements or additions to documentation label Mar 16, 2024

notmandatory force-pushed the docs/allow_shrinking branch from 2e679e6 to be9e796 Compare March 16, 2024 01:49

notmandatory force-pushed the docs/allow_shrinking branch from be9e796 to 376dcbc Compare March 16, 2024 01:57

docs(wallet): add warning on TxBuilder::allow_shrinking() and depreca…

5bc00e4

…te it test(wallet): add test_bump_fee_allow_shrinking test

notmandatory force-pushed the docs/allow_shrinking branch from 376dcbc to 5bc00e4 Compare March 16, 2024 02:10

notmandatory changed the title ~~Bump MSRV 1.63.0 AND add warning to docs for TxBuilder::allow_shrinking()~~ Bump MSRV 1.63.0 AND add deprecate TxBuilder::allow_shrinking() Mar 16, 2024

notmandatory mentioned this pull request Mar 19, 2024

(maintenance) doc(bdk): Clarify the absolute_fee docs #1159

Open

3 tasks

yanganto mentioned this pull request Mar 22, 2024

ci: move to Nix (simpler, easy to maintain version) #1320

Open

9 tasks

danielabrozzoni approved these changes Apr 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

notmandatory commented Mar 4, 2024 •

edited

notmandatory commented Mar 4, 2024 •

edited

thunderbiscuit commented Mar 12, 2024 •

edited

notmandatory commented Mar 12, 2024 •

edited

notmandatory commented Mar 15, 2024 •

edited

LLFourn commented Mar 15, 2024

notmandatory commented Mar 16, 2024 •

edited

danielabrozzoni left a comment

danielabrozzoni Apr 18, 2024

storopoli Apr 18, 2024 •

edited

danielabrozzoni Apr 18, 2024

storopoli Apr 18, 2024

danielabrozzoni Apr 18, 2024

storopoli Apr 18, 2024

danielabrozzoni Apr 18, 2024

storopoli Apr 18, 2024

danielabrozzoni Apr 19, 2024

storopoli Apr 19, 2024

danielabrozzoni Apr 18, 2024

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

Are you sure you want to change the base?

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

Conversation

notmandatory commented Mar 4, 2024 • edited

Description

Notes to the reviewers

Checklists

All Submissions:

notmandatory commented Mar 4, 2024 • edited

thunderbiscuit commented Mar 12, 2024 • edited

notmandatory commented Mar 12, 2024 • edited

notmandatory commented Mar 15, 2024 • edited

LLFourn commented Mar 15, 2024

notmandatory commented Mar 16, 2024 • edited

danielabrozzoni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

storopoli Apr 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notmandatory commented Mar 4, 2024 •

edited

notmandatory commented Mar 4, 2024 •

edited

thunderbiscuit commented Mar 12, 2024 •

edited

notmandatory commented Mar 12, 2024 •

edited

notmandatory commented Mar 15, 2024 •

edited

notmandatory commented Mar 16, 2024 •

edited

storopoli Apr 18, 2024 •

edited