- 
                Notifications
    You must be signed in to change notification settings 
- Fork 412
          chore(msrv): bump MSRV to 1.85.0
          #2055
        
          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.
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
  
    chore(msrv): bump MSRV to 1.85.0
  
  #2055
              Conversation
b04bbed    to
    b6e75cb      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK b6e75cb
Ran cargo test --all-features with:
❯ cargo --version
cargo 1.85.0 (d73d2caf9 2024-12-31)
8bb3255    to
    93c2172      
    Compare
  
    3e626f3    to
    adc99e7      
    Compare
  
    | I've added new commits bumping the criterion version and updating the toolchain action to match the one used in bdk_wallet. Still need to debug the CI failure now. | 
| 
 
 Scratch that, I didn't notice it was only in the test module. | 
b4f3c30    to
    0f5ac52      
    Compare
  
    | cACK, will do a deeper review when I get a chance but don't wait for me if you think it's ready to go. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 0f5ac52
- update all references to previous 1.63.0 MSRV. - update the rust-version to 1.85.0. - update the CI to not exclude bdk_electrum anymore. - update the ci/pin-msrv.sh to 1.85.0.
- use `is_non_or` instead of `map_or` - update lifetime on `find_direct_anchor`
0f5ac52    to
    9af94e0      
    Compare
  
    | I had to do a rebase due to the ci/pin-msrv.sh conflict introduced by #2058. | 
9af94e0    to
    81ed4d0      
    Compare
  
    | I've also updated the zizmor.yml as @ValuedMammal commented here: #2056 (comment) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 81ed4d0
| There's a few additional cleanups: oleonardolima/bdk@chore/bump-msrv-to-1.85.0...ValuedMammal:review_2055 
 📌 Look into bumping rust stable version to 1.90.0 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small nit: the actions-rust-lang/setup-rust-toolchain@v1 sets the config value cache to true by default, but you need to know this about the action to understand that the cache is activated if the value is not set explicitly.
In some of your steps you configure it explicitly, and that's my preferred approach. It doesn't change anything but signals to contributors that the cache is active without requiring they know in depth the action.
I would personally set the field to true on all uses of the action (or remove it entirely if you prefer the implicit way), but at the moment you set it to true on some and don't set it on others, making it less obvious why/when the cache is active.
| ACK changes added by @ValuedMammal here: oleonardolima/bdk@chore/bump-msrv-to-1.85.0...ValuedMammal:review_2055 Confirmed that it achieves the following: 
 
 ACK on this idea. 1.90.0 will probably make our CI faster as well. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self-ACK fdafe86
I cherry-picked @ValuedMammal's changes and also bumped rust-version to 1.90.
Question: Does CI changes need to appear in the changelog?
To do: Update branch protection rules before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK fdafe86. Looks good.
| ACK fdafe86 | 
0bd93da fix(core): Memory leak bugs in `CheckPoint::drop` impl (志宇) Pull request description: based on #1997, depends on #2055 ### Description Fix memory leak bug in CheckPoint::drop by using Arc::into_inner. Add tests (from the old PR) for memory leak + stack overflow when dropping CheckPoint. ### Notes to the reviewers It should be merged after #2055. ### Changelog notice ``` ### Fix: - Fix memory leak bug in CheckPoint::drop by using `Arc::into_inner`. ``` ### Checklists #### All Submissions: * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) #### Bugfixes: * [x] This pull request breaks the existing API * [x] I've added tests to reproduce the issue which are now passing * [x] I'm linking the issue being fixed by this PR ACKs for top commit: evanlinjin: ACK 0bd93da Tree-SHA512: f61bd7ee19a6eb4e0f0275882d8c7b9f0e050e868e1e7a51b4089c8c71d14478905d900c619bebe71e4ffe7117400d99e4604d6a16e3b9a7515755d3f3250c90
fixes #2009
Description
It bumps the project MSRV to 1.85.0, as the latest Debian trixie release establishes that as stable, see: https://tracker.debian.org/pkg/rustc.
This PR does:
bdk_electrumanymore.Notes to the reviewers
Should we update something in the GitHub repository configuration to get rid of old mandatory CI jobs ?
Changelog notice
Checklists
All Submissions: