Support new impl traits upstream #696
Conversation
|
👋 I see @joostjager was un-assigned. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
I have to say I'm not the biggest fan of all this additional boilerplate, especially since that will get even bigger when we finally support and make use of async versions of the MigratableKVStore and PaginatedKVStore. I also don't quite buy that the added complexity is worth saving some boxing, especially given that tokio will auto-box all read/write tasks as they are simply too big for the stack (according to tokio at least).
Note this already needs a rebase, and also doesn't compile under cargo build --features uniffi currently.
Given that this will likely introduce a lot of conflicts with #692 and make the changes there even more complex, I might be good to hold off on this (and by extension the upstream API-breaking changes until after #692 lands).
I think we'll survive three or four more methods :)
No, tokio will auto-box the top-of-task future, but everything below that gets compiled into a single object when you have concrete future types. This should nontrivially reduce boxing, which I imagine are a decent chunk of our allocations, or at least on ldk-sample write-related allocations were so I have to imagine still are. Of course we don't reduce allocations in
Seems reasonable to hold off on landing this, but I don't see why that has to delay landing the PR upstream? Its somewhat annoying to rebase a PR that touches a bunch of code across |
TheBlueMatt
force-pushed
the
2025-11-impl-traits
branch
from
d650350 to
04fd0a6
Compare
In the next commit we'll update `rust-lightning` with the `Pin<Box<dyn ...>>` pattern dropped in favor of native async traits (with `impl Future...`). In anticipation of those traits becoming not-object-safe here we drop the dyn indirection that exists in our gossip validation.
TheBlueMatt
force-pushed
the
2025-11-impl-traits
branch
2 times, most recently
from
ba5c9b7 to
1f9c6b7
Compare
|
Rebased and updated the LDK commit to latest git from today. |
In `rust-lightning` we dropped the `Pin<Box<dyn ...>>` in favor of `impl Future`s on most traits to make the traits native-async-ish. Here we update to implement that change here. However, because we avoid generics in ldk-node through concrete dyn indirections, and having implict associated types on `rust-lightning` traits makes them not-object-safe, we have to do a bit of indirection. Instead, we define a `DynStoreTrait` which re-concretizes the `rust-lightning` storage traits, allowing us to use them `dyn`.
TheBlueMatt
force-pushed
the
2025-11-impl-traits
branch
from
1f9c6b7 to
30c3fba
Compare
3 participants
Updates to support lightningdevkit/rust-lightning#4175 as described at lightningdevkit/rust-lightning#4175 (comment)
Commit messages need cleaning up and rustfmt, but otherwise its probably good.