Reduce `Box`ing using `impl Trait` in trait methods post-MSRV-bump #4175

TheBlueMatt · 2025-10-25T18:28:13Z

A handful of cleanups now that we bumped the MSRV and can use impl Trait in trait methods.

ldk-reviews-bot · 2025-10-25T18:28:15Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

TheBlueMatt · 2025-10-25T18:29:16Z

There's still a few Box::pins in lightning-background-processor that we really should be able to get rid of, but its not entirely trivial so I didn't do it here.

codecov · 2025-10-25T21:05:54Z

Codecov Report

❌ Patch coverage is 73.94636% with 68 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.29%. Comparing base (2d6e017) to head (8366554).

Files with missing lines	Patch %	Lines
lightning-background-processor/src/lib.rs	0.00%	20 Missing ⚠️
lightning-block-sync/src/rest.rs	27.77%	13 Missing ⚠️
lightning-block-sync/src/rpc.rs	27.77%	13 Missing ⚠️
lightning-block-sync/src/gossip.rs	0.00%	9 Missing ⚠️
lightning/src/util/test_utils.rs	25.00%	6 Missing ⚠️
lightning-persister/src/fs_store.rs	90.90%	4 Missing ⚠️
lightning-net-tokio/src/lib.rs	60.00%	2 Missing ⚠️
lightning-liquidity/src/lsps2/service.rs	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4175      +/-   ##
==========================================
- Coverage   89.29%   89.29%   -0.01%     
==========================================
  Files         180      180              
  Lines      137913   137934      +21     
  Branches   137913   137934      +21     
==========================================
+ Hits       123144   123162      +18     
- Misses      12154    12168      +14     
+ Partials     2615     2604      -11

Flag	Coverage Δ
fuzzing	`33.53% <12.41%> (+0.85%)`	⬆️
tests	`88.68% <73.94%> (-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.

tnull

Given how much this cleans up the new async traits, I do wonder if we want to ship this for 0.2 afterall?

tnull · 2025-10-27T06:57:59Z

Cargo.toml

    "lightning-macros",
    "lightning-dns-resolver",
    "lightning-liquidity",
+    "lightning-transaction-sync",


As BDK is about to bump their MSRV to 1.85, let's not bother with this if we need to move it out again shortly.

I thought they agreed to keep the utility crates to 1.75?

Actually, I thought they were gonna walk it back to 1.75...

ldk-reviews-bot · 2025-10-27T07:06:20Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

TheBlueMatt · 2025-10-27T12:26:33Z

Given how much this cleans up the new async traits, I do wonder if we want to ship this for 0.2 afterall?

Meh, I don't think they clean them up that much, they just remove a Box::pin, and actually they become a bit more annoying to work with (see the few places we have to keep the Box::pin cause of rustc mess). It becomes a bit easier to work with in edition 2024, which luckily a downstream crate could choose to use if they want.

TheBlueMatt · 2025-10-30T14:42:10Z

Rebased to pick up #4180, this should now pass CI (modulo fuzzing, which still confuses me).

TheBlueMatt · 2025-10-30T15:27:29Z

The CI failure is bitcoindevkit/rust-electrum-client#181 which is unrelated to this PR and its just that we're now testing it.

TheBlueMatt · 2025-10-31T14:39:12Z

Rebased for the restoration of the lazy flag.

TheBlueMatt · 2025-11-03T13:54:45Z

Fixed a bunk rebase.

TheBlueMatt · 2025-11-04T12:41:34Z

Oops, fixed a no-std error in the new BP stuff:

$ git diff-tree -U1 05717d0860 20697053c
diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs
index 874c82f19d..bc0d42ac19 100644
--- a/lightning-background-processor/src/lib.rs
+++ b/lightning-background-processor/src/lib.rs
@@ -43,2 +43,4 @@ use lightning::util::ser::Writeable;

+#[cfg(not(c_bindings))]
+use lightning::io::Error;
 use lightning::ln::channelmanager::AChannelManager;
@@ -53,2 +55,4 @@ use lightning::sign::{
 };
+#[cfg(not(c_bindings))]
+use lightning::util::async_poll::MaybeSend;
 use lightning::util::logger::Logger;
@@ -430,4 +434,3 @@ impl KVStore for DummyKVStore {
 		&self, _: &str, _: &str, _: &str,
-	) -> impl core::future::Future<Output = Result<Vec<u8>, lightning::io::Error>> + Send + 'static
-	{
+	) -> impl core::future::Future<Output = Result<Vec<u8>, Error>> + MaybeSend + 'static {
 		async { unimplemented!() }
@@ -437,3 +440,3 @@ impl KVStore for DummyKVStore {
 		&self, _: &str, _: &str, _: &str, _: Vec<u8>,
-	) -> impl core::future::Future<Output = Result<(), lightning::io::Error>> + Send + 'static {
+	) -> impl core::future::Future<Output = Result<(), Error>> + MaybeSend + 'static {
 		async { unimplemented!() }
@@ -443,3 +446,3 @@ impl KVStore for DummyKVStore {
 		&self, _: &str, _: &str, _: &str, _: bool,
-	) -> impl core::future::Future<Output = Result<(), lightning::io::Error>> + Send + 'static {
+	) -> impl core::future::Future<Output = Result<(), Error>> + MaybeSend + 'static {
 		async { unimplemented!() }
@@ -449,4 +452,3 @@ impl KVStore for DummyKVStore {
 		&self, _: &str, _: &str,
-	) -> impl core::future::Future<Output = Result<Vec<String>, lightning::io::Error>> + Send + 'static
-	{
+	) -> impl core::future::Future<Output = Result<Vec<String>, Error>> + MaybeSend + 'static {
 		async { unimplemented!() }

tnull · 2025-11-04T20:08:25Z

This needs another rebase unfortunately. Also, good opportunity to check whether the electrum-client release really fixed our doc builds.

Now that it has the same MSRV as everything else in the workspace, it doesn't need to live on its own.

Now that our MSRV is above 1.68 we can use the `pin!` macro to avoid having to `Box` various futures, avoiding some allocations, especially in `lightning-net-tokio`, which happens in a tight loop.

Now that our MSRV is 1.75, we can return `impl Trait` from trait methods. Here we use this to clean up `KVStore` methods, dropping the `Pin<Box<dyn ...>>` we had to use to have trait methods return a concrete type. Sadly, there's two places where we can't drop a `Box::pin` until we switch to edition 2024.

Now that our MSRV is 1.75, we can return `impl Trait` from trait methods. Here we use this to clean up `lightning-block-sync` trait methods, dropping the `Pin<Box<dyn ...>>` we had to use to have trait methods return a concrete type.

Now that our MSRV is 1.75, we can return `impl Trait` from trait methods. Here we use this to clean up `lightning` crate trait methods, dropping the `Pin<Box<dyn ...>>`/`AsyncResult` we had to use to have trait methods return a concrete type.

Now that we have an MSRV that supports returning `impl Trait` in trait methods, we can use it to avoid the `Box<dyn ...>` we had spewed all over our BOLT 11 invoice serialization.

TheBlueMatt · 2025-11-05T01:24:58Z

Oops, no big deal, done.

tnull

NACK at least on the impl Future changes here, as they break LDK Node builds and the solution would .. at least not be trivial on that end.

tnull · 2025-11-05T08:31:11Z

lightning/src/util/persist.rs

 	fn read(
 		&self, primary_namespace: &str, secondary_namespace: &str, key: &str,
-	) -> AsyncResult<'static, Vec<u8>, io::Error>;
+	) -> impl Future<Output = Result<Vec<u8>, io::Error>> + 'static + MaybeSend;


This breaks LDK Node builds, where we have to use dyn trait (and actually solved the dual-KVStore/KVStoreSync requirements with a dyn trait, see https://github.com/lightningdevkit/ldk-node/blob/eaad8f56d8a7074102f6e127798b4c987459e850/src/types.rs#L42-L52):

error[E0038]: the trait `SyncAndAsyncKVStore` is not dyn compatible --> src/lib.rs:1363:39 | 1363 | total_anchor_channels_reserve_sats(&self.channel_manager, &self.config); | ^^^^^^^^^^^^^^^^^^^^^ `SyncAndAsyncKVStore` is not dyn compatible

Please let's not do this until we're positive there is an easy fix on the LDK Node end (I doubt there is).

Same goes for example for the UtxoSource, where the only solution was to use dyn UtxoSource that is determined at runtime:

error[E0038]: the trait `UtxoSource` is not dyn compatible --> src/gossip.rs:25:16 | 25 | gossip_sync: Arc<P2PGossipSync>, | ^^^^^^^^^^^^^^^^^^ `UtxoSource` is not dyn compatible |

I'd prefer we don't touch the traits at all here.

Yes, you can, you need a trivial wrapper and it'll work just fine https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=59920ac76b2009b430618ca48074f7ed

Though, really, ldk-node shouldn't be using dyn everywhere, that's not a great design to begin with.

Well, we basically need to, as we let users set a bunch of things at compile time through the builder pattern, where generics are simply not an option. On top of that, Uniffi doesn't support any generics, so in particular our top-level Node object can't have a generic KVStore. On top of that we at some point also want to get (back to) the point where even bindings users can bring their own KVStore, and Uniffi supports traits only if dyn .. + Send + Sync.

Yes, you can, you need a trivial wrapper and it'll work just fine https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=59920ac76b2009b430618ca48074f7ed

See above, generics are not an option for us in several places/for several reasons.

as we let users set a bunch of things at compile time through the builder pattern, where generics are simply not an option.

Hmm? Builders do not at all imply you cannot have generics. Builders can simply have generics in them as well which propagate cleanly to the ultimately-built type.

On top of that, Uniffi doesn't support any generics, so in particular our top-level Node object can't have a generic KVStore.

The proposed wrappers there would not have any generics on the top-level Node object.

we at some point also want to get (back to) the point where even bindings users can bring their own KVStore, and Uniffi supports traits only if dyn .. + Send + Sync.

Sure, that would require a different top-level trait for bindings (like the suggested wrapper one in the above play link), but that's not crazy IMO.

I'm not quite clear on why that implies we have to wait to land this? I was under the impression ldk-node was willing to fall behind by a week here or there when things are in-flight as long as the ctach-up is quick.

I'm not quite clear on why that implies we have to wait to land this? I was under the impression ldk-node was willing to fall behind by a week here or there when things are in-flight as long as the ctach-up is quick.

Well, the catch up might not be as immediate as the change is rather invasive (as it's changing the types all over the codebase) and we have a large PR (lightningdevkit/ldk-node#692) in-flight that I have yet to take real look at but is similarly invasive to everything KVStore. We might be able to marry the two concepts, but all of this will need to be deferred post-release anyways.

In the meantime it would be good to unbreak LDK CI while keeping the ability to build LDK Node against LDK main, as these likely won't be the only relevant changes we want to account for.

Right, but ldk-node being in the middle of a release where upgrade-to-new-LDK stuff is not a priority doesn't really seem like a great justification for rust-lightning not being able to land things, IMO. We should discuss it later today at the dev sync tho.

I agree, let's discuss it later.

(lightningdevkit/ldk-node#692) author here. I've been following the conversation and would like to be in the loop of the outcome of your dev sync discussion, as it may inform how I approach my PR (if it isn't merged before this).

We might be able to marry the two concepts

I do agree that we might be able to marry the two concepts - I've noticed the similarity in approach between (lightningdevkit/ldk-node#696) and parts of my PR, so coordinating these changes could help minimize the overall churn.

tnull · 2025-11-05T12:38:53Z

Feel free to rebase once more as we just landed #4206 to check for breakages.

ldk-reviews-bot requested a review from wpaulino October 25, 2025 18:38

TheBlueMatt force-pushed the 2025-10-msrv-less-box branch 14 times, most recently from e8fa417 to d54050d Compare October 25, 2025 20:55

TheBlueMatt force-pushed the 2025-10-msrv-less-box branch 2 times, most recently from 9d5a3e2 to 724caa8 Compare October 26, 2025 11:50

tnull requested review from tnull and removed request for wpaulino October 27, 2025 06:56

tnull reviewed Oct 27, 2025

View reviewed changes

TheBlueMatt mentioned this pull request Oct 28, 2025

Bump electrsd to 0.36.1, fix CI #4180

Merged

TheBlueMatt force-pushed the 2025-10-msrv-less-box branch from 724caa8 to a467664 Compare October 30, 2025 14:36

TheBlueMatt mentioned this pull request Oct 31, 2025

Fix lightning-transaction-sync docs #4199

Closed

TheBlueMatt force-pushed the 2025-10-msrv-less-box branch from a467664 to 60a21c6 Compare October 31, 2025 14:38

TheBlueMatt force-pushed the 2025-10-msrv-less-box branch from 60a21c6 to 05717d0 Compare November 3, 2025 13:54

TheBlueMatt force-pushed the 2025-10-msrv-less-box branch 2 times, most recently from 5161991 to 2069705 Compare November 4, 2025 12:41

TheBlueMatt added 7 commits November 5, 2025 01:24

Move lightning-transaction-sync back into the main workspace

2151101

Now that it has the same MSRV as everything else in the workspace, it doesn't need to live on its own.

Trivially replace Box::pin with pin! in a few places

46949f4

Now that our MSRV is above 1.68 we can use the `pin!` macro to avoid having to `Box` various futures, avoiding some allocations, especially in `lightning-net-tokio`, which happens in a tight loop.

Drop required Boxing of lightning-block-sync Futures

8e02e4c

Now that our MSRV is 1.75, we can return `impl Trait` from trait methods. Here we use this to clean up `lightning-block-sync` trait methods, dropping the `Pin<Box<dyn ...>>` we had to use to have trait methods return a concrete type.

Drop Boxing of iterators during BOLT 11 invoice serialization

189192c

Now that we have an MSRV that supports returning `impl Trait` in trait methods, we can use it to avoid the `Box<dyn ...>` we had spewed all over our BOLT 11 invoice serialization.

Fix new warning-errors in 1.75 CI builds

8366554

TheBlueMatt force-pushed the 2025-10-msrv-less-box branch from 2069705 to 8366554 Compare November 5, 2025 01:24

tnull reviewed Nov 5, 2025

View reviewed changes

TheBlueMatt mentioned this pull request Nov 5, 2025

Support new impl traits upstream lightningdevkit/ldk-node#696

Draft

Reduce Boxing using impl Trait in trait methods post-MSRV-bump #4175

Are you sure you want to change the base?

Reduce Boxing using impl Trait in trait methods post-MSRV-bump #4175

Uh oh!

Conversation

TheBlueMatt commented Oct 25, 2025

Uh oh!

ldk-reviews-bot commented Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Oct 25, 2025

Uh oh!

codecov bot commented Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

tnull Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Oct 27, 2025

Uh oh!

TheBlueMatt commented Oct 27, 2025

Uh oh!

TheBlueMatt commented Oct 30, 2025

Uh oh!

TheBlueMatt commented Oct 30, 2025

Uh oh!

TheBlueMatt commented Oct 31, 2025

Uh oh!

TheBlueMatt commented Nov 3, 2025

Uh oh!

TheBlueMatt commented Nov 4, 2025

Uh oh!

tnull commented Nov 4, 2025

Uh oh!

TheBlueMatt commented Nov 5, 2025

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

tnull Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Reduce `Box`ing using `impl Trait` in trait methods post-MSRV-bump #4175

Reduce `Box`ing using `impl Trait` in trait methods post-MSRV-bump #4175

ldk-reviews-bot commented Oct 25, 2025 •

edited

Loading

codecov bot commented Oct 25, 2025 •

edited

Loading

tnull Oct 27, 2025 •

edited

Loading

tnull Nov 5, 2025 •

edited

Loading

TheBlueMatt Nov 5, 2025 •

edited

Loading

tnull Nov 6, 2025 •

edited

Loading

tnull commented Nov 5, 2025 •

edited

Loading