Skip to content

Bump MSRV to 1.85.0 #4002

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Aug 11, 2025

We generally align our MSRV with Debian's stable channel. Debian 13 'Trixie' was just released, shipping rustc 1.85. As the 0.2 is still a bit off, we should be fine bumping our MSRV on the main branch already, which is what we do here.

@tnull tnull requested a review from TheBlueMatt August 11, 2025 09:13
@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Aug 11, 2025

👋 I see @TheBlueMatt was un-assigned.
If you'd like another reviewer assignment, please click here.

@tnull tnull force-pushed the 2025-08-bump-msrv-to-1.85 branch 2 times, most recently from 0b33c48 to 40dbf5b Compare August 11, 2025 09:16
@tnull
Copy link
Contributor Author

tnull commented Aug 11, 2025

I also addressed most MSRV-related TODOs in the code, only thing left is

short_channel_id: u64, // This should be NonZero<u64> eventually when we bump MSRV

@TheBlueMatt any opinion on whether to do it here, in a separate PR, or as part of #3973 ?

@tnull tnull force-pushed the 2025-08-bump-msrv-to-1.85 branch from 40dbf5b to 44024fc Compare August 11, 2025 09:18
We generally align our MSRV with Debian's stable channel. Debian 13
'Trixie' was just released, shipping rustc 1.85. We therefore bump our
MSRV on the `main` branch here.
@tnull tnull force-pushed the 2025-08-bump-msrv-to-1.85 branch 2 times, most recently from 8eb3354 to 17b08bc Compare August 11, 2025 11:09
Copy link

codecov bot commented Aug 11, 2025

Codecov Report

❌ Patch coverage is 66.66667% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.94%. Comparing base (192618b) to head (04836c6).

Files with missing lines Patch % Lines
lightning/src/chain/channelmonitor.rs 0.00% 14 Missing and 2 partials ⚠️
lightning-background-processor/src/lib.rs 66.66% 0 Missing and 1 partial ⚠️
lightning/src/ln/channel.rs 94.44% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4002   +/-   ##
=======================================
  Coverage   88.93%   88.94%           
=======================================
  Files         174      174           
  Lines      124593   124579   -14     
  Branches   124593   124579   -14     
=======================================
- Hits       110813   110811    -2     
+ Misses      11283    11279    -4     
+ Partials     2497     2489    -8     
Flag Coverage Δ
fuzzing 22.18% <12.24%> (+<0.01%) ⬆️
tests 88.77% <66.66%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@TheBlueMatt
Copy link
Collaborator

We generally align our MSRV with Debian's stable channel

I don't think this is entirely true :). We generally align our MSRV with rust-bitcoin and the rest of the ecosystem. IIRC rust-bitcoin usually does something like min(2-year-old-rustc, debian stable, rustc that introduces materially useful features). The last time we bumped MSRV (#2681) the rustc we bumped to was a year and a few months old, and the release containing it wasn't until Dec, so rustc 1.63 was almost a year and a half old.

1.85 is currently only around 7 months old, and while it contains the rust 2024 edition, its not clear to me what we get that's worth bumping to what, presumably, rust-bitcoin won't do. There's also async closures but async blocks seem to have done us just fine nearly everywhere.

@TheBlueMatt TheBlueMatt removed their request for review August 11, 2025 11:18
@tnull
Copy link
Contributor Author

tnull commented Aug 11, 2025

We generally align our MSRV with rust-bitcoin and the rest of the ecosystem.

Do we? #2681 happened 8-9 months before rust-bitcoin bumped in rust-bitcoin/rust-bitcoin#3100. I also recall reading a comment where they indicated they had bumped earlier if they'd known LDK already went ahead / was fine with it. BDK is also looking to bump soon (bitcoindevkit/bdk#2009) and LDK Node will bump shortly: lightningdevkit/ldk-node#606

1.85 is currently only around 7 months old, and while it contains the rust 2024 edition, its not clear to me what we get that's worth bumping to what, presumably, rust-bitcoin won't do. There's also async closures but async blocks seem to have done us just fine nearly everywhere.

What we get is a lot of little things, most importantly general reduction of friction (a period where we don't constantly have to fight with pinned-back dependencies, where all crates have the same MSRV, where cargo fmt and cargo +1.85 fmt have the exact same output, etc. etc.). Note we can then also finally switch to 'proper' async traits, which is IMO crucial since we're about to ship the async KVStore interface, which is currently pretty clunky to use (return values of Pin<Box<dyn Future<Output = Result<Vec<Utxo>, ()>> + Send + 'a>> are not exactly ergonomic to type, to say the least).

We also get some language features (GATs, let-else bindings, both stabilized with 1.65, for example), which some devs were looking forward to be able to use finally.

IMO it makes a whole lot of sense to upgrade now that we can, if just because it makes our lives easier in many little places, but also because it allows us to expose a more coherent Rust-native API.

@tnull tnull force-pushed the 2025-08-bump-msrv-to-1.85 branch from bb2c8aa to c51313f Compare August 11, 2025 11:35
@tnull tnull force-pushed the 2025-08-bump-msrv-to-1.85 branch from 812cfd6 to 04836c6 Compare August 11, 2025 11:49
@TheBlueMatt
Copy link
Collaborator

Do we? #2681 happened 8-9 months before rust-bitcoin bumped in rust-bitcoin/rust-bitcoin#3100. I also recall reading a comment where they indicated they had bumped earlier if they'd known LDK already went ahead / was fine with it. BDK is also looking to bump soon (bitcoindevkit/bdk#2009) and LDK Node will bump shortly: lightningdevkit/ldk-node#606

We don't wait for rust-bitcoin, but we definitely coordinate around specific version of rustc.

What we get is a lot of little things, most importantly general reduction of friction (a period where we don't constantly have to fight with pinned-back dependencies, where all crates have the same MSRV, where cargo fmt and cargo +1.85 fmt have the exact same output, etc. etc.).

This doesn't sound 1.85-specific?

Note we can then also finally switch to 'proper' async traits, which is IMO crucial since we're about to ship the async KVStore interface, which is currently pretty clunky to use (return values of Pin<Box<dyn Future<Output = Result<Vec, ()>> + Send + 'a>> are not exactly ergonomic to type, to say the least).

Sadly even with rustc nightly we wouldn't want to change that. Our async KVStore requires ordering, which is not possible with native rust async methods as they do not run any code at all until polled. I guess in theory we could require "ordering after the first poll" and poll once whenever we persist, but that seems even more brittle than the current version which at least exposes the concept to the implementer.

We also get some language features (GATs, let-else bindings, both stabilized with 1.65, for example), which some devs were looking forward to be able to use finally.

We should also want 1.68 for the pin macro, afair (removes allocations in each loop of the BP).

@tnull tnull moved this to Goal: Merge in Weekly Goals Aug 12, 2025
@tnull tnull self-assigned this Aug 12, 2025
@TheBlueMatt
Copy link
Collaborator

Per https://pkgs.org/download/rustc (and packages.ubuntu.com) the latest Ubuntu LTS is on rustc 1.75, which given its also the lightning-transaction-sync MSRV seems like an obvious candidate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Goal: Merge
Development

Successfully merging this pull request may close these issues.

3 participants