Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Deprecate across the board max_satisfaction_weight #638

Merged
merged 1 commit into from
Feb 17, 2024
Merged

Deprecate across the board max_satisfaction_weight #638

merged 1 commit into from
Feb 17, 2024

Conversation

storopoli
Copy link
Contributor

Adds a since=10.0.0 to all max_satisfaction_weight deprecations. Adds a note telling users to check #476 for more details.

Closes #637.

apoelstra
apoelstra previously approved these changes Feb 15, 2024
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b656b87 Thank you!!

tcharding
tcharding approved these changes Feb 15, 2024
View reviewed changes
Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b656b87

@tcharding
Copy link
Member

Thanks for following through man.

@tcharding
Copy link
Member

CI fails need you to use the new, non-deprecated versions, and there is some clippy stuff, could be unrelated to this PR but I didn't look.

@storopoli
Copy link
Contributor Author

I'm on it...

@storopoli
Deprecate across the board max_satisfaction_weight
7fc7661 
Adds a `since=10.0.0` to all `max_satisfaction_weight` deprecations.
Adds a note telling users to check #476 for more details.

Closes #637.
@storopoli
Copy link
Contributor Author

OK CI should pass now. I had to add some dead_code that were not being touched with deprecations anymore in some Psbt traits.

apoelstra
apoelstra approved these changes Feb 15, 2024
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 7fc7661

@tcharding
Copy link
Member

CI fail is unrelated to this PR.

miniscript has MSRV of 1.48 still, that won't work when its dependencies are using 1.56.1 - that should have been changed before last release.

@tcharding
Copy link
Member

FWIW the most thorough way to go about this is, IMO, to fix the clippy as a separate PR, fix the MSRV as a separate PR, then rebase this. I understand if you don't want to do all that, I can try get to it, likely won't be till next week though.

@storopoli
Copy link
Contributor Author

FWIW the most thorough way to go about this is, IMO, to fix the clippy as a separate PR, fix the MSRV as a separate PR, then rebase this. I understand if you don't want to do all that, I can try get to it, likely won't be till next week though.

I was tinkering with trying to get things to work with Rust 1.48.0.
So I am free to make other PRs changing MSRV to 1.56.1? (Of course there are a bunch of documentation and "prose" related changes with that).
Good!

About the clippy warnings it seems also very easy.

Don't worry we can hold this until I finish these tasks and rebase.

@apoelstra
Copy link
Member

So I am free to make other PRs changing MSRV to 1.56.1?

Yep, go for it. Sorry that this hasn't been done already. I'm surprised we managed to update our bitcoin dependency without doing it.

@storopoli storopoli mentioned this pull request Feb 16, 2024
Merged
@storopoli
Copy link
Contributor Author

Ok PRs are up.

Suggested order of merging:

  1. Update MSRV to 1.56.1 #639
  2. Deprecate across the board max_satisfaction_weight #638 (this)
  3. Clippy lints MSRV 1.56.1 #640

Of course I'll need to rebase once an ancestor PR is merged...

@storopoli storopoli mentioned this pull request Feb 16, 2024
Merged
@apoelstra
Copy link
Member

Honestly because these are independent (in terms of diff) and only affect the Github Actions CI (which we can't possibly bisect on) I'd be fine just merging them without them being rebased.

But see my comment on your first one.

tcharding
tcharding approved these changes Feb 16, 2024
View reviewed changes
Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 7fc7661

apoelstra added a commit that referenced this pull request Feb 17, 2024
@apoelstra
Merge #639: Update MSRV to 1.56.1
f1211c7 
c301ce3 Update MSRV to 1.56.1 (Jose Storopoli)

Pull request description:

  Following `rust-bitcoin` MSRV of 1.56.1, this pins MSRV to 1.56.1.

  Quoting a recent merge in `rust-bitcoin`

  > Rust version 1.56.0 introduced edition 2021. Shortly afterwards, on
  > October 21 2021 Rust version 1.56.1 was released.
  >
  > Debian stable is currently shipping `rustc 1.63.0`.
  >
  > Our stated MSRV policy is: In Debian stable and at least 2 years old.
  >
  > Therefore our MSRV policy is met by Rust version 1.56.1 and we can strat
  > to bump our MSRV org wide.
  >
  > Links:
  > - https://blog.rust-lang.org/2021/11/01/Rust-1.56.1.html
  > - https://blog.rust-lang.org/2021/10/21/Rust-1.56.0.html
  > - https://packages.debian.org/stable/rust/rustc

  Removing the pins in `contrib/test.sh` since they are not necessary
  in 1.56.1.

  EDIT: CI fails because of the `#[allow(dead_code)]` implemented in #638 on some Psbt traits.
  Do I port these to here? Conceptually they belong to #638 and not here.

ACKs for top commit:
  apoelstra:
    ACK c301ce3

Tree-SHA512: 9dad92068d03a648e0d16e1bf0cba34a1959be981fbcec28bc3e6265b2c82df1380777b047e68c69ed80c579638a7e67738d3cec28293e7259abae3f348c3752
@apoelstra apoelstra merged commit 3426ca5 into rust-bitcoin:master Feb 17, 2024
12 of 16 checks passed
@storopoli storopoli deleted the js/deprecate-max_satisfaction branch February 17, 2024 16:14
apoelstra added a commit that referenced this pull request Feb 17, 2024
@apoelstra
Merge #640: Clippy lints MSRV 1.56.1
18b1b30 
e2636f3 Clippy lints MSRV 1.56.1 (Jose Storopoli)

Pull request description:

  Nothing out of the ordinary.
  The most alarming was the non-inclusive range in
  `test_utils.rs`.

  In some test functions I allowed myself to use
  `#[allow(clippy::type_complexity)]` because clippy was asking to create a type instead of using the big tuple. Since these were used just once, I thought it was overkill.

  Note, depends on:

  - #639 (we can also merge this into it instead of `master`)
  - #638 (has some allows on dead_code)

ACKs for top commit:
  apoelstra:
    ACK e2636f3

Tree-SHA512: d1f38d1e2a1415da816898806377c90986072fe0bc9a9dbc3e5476026c05c53743453cbd0a237a70aa27762f1be86e44c19422575b7e62f686140accc412177c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tcharding tcharding tcharding approved these changes

@apoelstra apoelstra apoelstra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

max_satisfaction_weight is only deprecated for some descriptors
3 participants
@storopoli @tcharding @apoelstra