Drop `Deref` indirection for most trait parameters #4311

valentinewallace · 2026-01-13T19:16:02Z

Reduces generics and verbosity across the codebase, should provide equivalent behavior.

Inspired by #4309 (comment). Basically, instead of Deref indirection we instead impl<T: Trait, D: Deref<Target = Trait>> Trait for D for all our traits.

The majority of the codebase's traits are done in this PR, but we do skip a good amount still. In a lot of cases traits are skipped because of compiler complaints about (potential-) conflicting trait implementations, but also ran into some lifetime issues and some c_bindings issues.

ldk-reviews-bot · 2026-01-13T19:16:05Z

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

valentinewallace · 2026-01-13T19:16:30Z

If this looks good and works for language bindings, I/claude can do it for all the other traits. I feel like this is a solid improvement.

valentinewallace · 2026-01-13T19:17:01Z

@TheBlueMatt would appreciate conceptual review to make sure this works for the language bindings.

valentinewallace · 2026-01-13T19:19:56Z

Actually, it looks like this might not work for 1.75 (associated type bounds are unstable). Not sure if we plan on bumping that anytime soon?

TheBlueMatt · 2026-01-13T20:32:05Z

@TheBlueMatt would appreciate conceptual review to make sure this works for the language bindings.

It means updating the generation logic to support it, but it certainly isn't directly problematic.

Actually, it looks like this might not work for 1.75 (associated type bounds are unstable). Not sure if we plan on bumping that anytime soon?

You can write it as impl<T: BroadcasterInterface, B: Deref<Target = T>> BroadcasterInterface for B { instead of impl<B: Deref<Target: BroadcasterInterface>> BroadcasterInterface for B {.

TheBlueMatt · 2026-01-13T20:32:28Z

Conceptually tho yea imo we should do this. Its "the right way" to do it in rust.

lightning/src/chain/chaininterface.rs

codecov · 2026-01-13T23:55:54Z

Codecov Report

❌ Patch coverage is 83.36013% with 207 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.53%. Comparing base (b4fb555) to head (db87b32).
⚠️ Report is 25 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/msgs.rs	46.90%	103 Missing ⚠️
lightning/src/util/persist.rs	45.94%	20 Missing ⚠️
lightning/src/ln/channelmanager.rs	83.47%	13 Missing and 6 partials ⚠️
lightning/src/ln/channel.rs	93.91%	14 Missing ⚠️
lightning-block-sync/src/gossip.rs	0.00%	10 Missing ⚠️
lightning-background-processor/src/lib.rs	68.18%	7 Missing ⚠️
lightning/src/chain/channelmonitor.rs	90.54%	7 Missing ⚠️
lightning/src/util/anchor_channel_reserves.rs	0.00%	6 Missing ⚠️
lightning/src/chain/chainmonitor.rs	77.77%	4 Missing ⚠️
lightning/src/ln/peer_handler.rs	88.23%	3 Missing and 1 partial ⚠️
... and 6 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4311      +/-   ##
==========================================
- Coverage   86.58%   86.53%   -0.06%     
==========================================
  Files         158      158              
  Lines      102367   102571     +204     
  Branches   102367   102571     +204     
==========================================
+ Hits        88636    88757     +121     
- Misses      11316    11398      +82     
- Partials     2415     2416       +1

Flag	Coverage Δ
fuzzing	`37.06% <43.29%> (+0.12%)`	⬆️
tests	`85.79% <81.02%> (-0.08%)`	⬇️

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

Note that you'll need to bump lightning-liquidity's version to 0.3.0+git to get the SemVer checks to pass, as it's an API-breaking change.

Useful for upcoming commits where we otherwise break SemVer checks by changing the ALiquidityManager associated types.

joostjager · 2026-01-15T18:42:15Z

Is the Logger a candidate too for this?

valentinewallace · 2026-01-15T18:49:01Z

Is the Logger a candidate too for this?

Yes, I will un-draft the PR once all the traits are complete :)

ldk-reviews-bot · 2026-01-15T19:18:08Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

joostjager · 2026-01-15T22:26:38Z

lightning/src/ln/msgs.rs

 	fn message_received(&self);
 }

+impl<T: ChannelMessageHandler + ?Sized, C: Deref<Target = T>> ChannelMessageHandler for C {


Is this really better than using Arc instead of Deref, and accepting that for no-std the API might look slightly different?

There aren't that many no-std projects.

Yeah maybe we should resolve this here #4309 (comment) before moving forward with this direction? Is it still difficult to add new trait params now that we have Claude?

Ask Claude to add a logger to ChannelMonitor, and then report back 😂

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

valentinewallace · 2026-01-20T17:26:20Z

@tnull do you have any idea if the LDK Node API CI failure is easy to fix on the LDK Node end? Otherwise I can drop the KVStore update commit.

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Unfortunately the same improvement can't be made for KVStoreSync and Persist, due to the blanket implementation where all KVStoreSync traits implement Persist resulting in conflicting implementations. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Reduces generics and verbosity across the codebase, should provide equivalent behavior. This could be split into multiple commits (e.g. one for OnionMessenger message handler types, one for PeerManager message handler types) but it would require adding a new IgnoringOnionMessageHandler type for the messenger handlers, due to the IgnoringMessageHandler's Deref implementation conflicting otherwise. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

TheBlueMatt · 2026-01-20T19:18:34Z

@tnull do you have any idea if the LDK Node API CI failure is easy to fix on the LDK Node end? Otherwise I can drop the KVStore update commit.

It is now the responsibility of rust-lightning PR authors to go fix ldk-node if your PR breaks it :p

valentinewallace · 2026-01-20T19:24:26Z

@tnull do you have any idea if the LDK Node API CI failure is easy to fix on the LDK Node end? Otherwise I can drop the KVStore update commit.

It is now the responsibility of rust-lightning PR authors to go fix ldk-node if your PR breaks it :p

I'm aware, but I'm not sure if it's something that should be fixed or whether the new rust-lightning API isn't good. Thought he might know off the top of his head :p

TheBlueMatt · 2026-01-20T19:31:20Z

Fair enough. It does look like its just a conflict cause there's a deref-bounded auto-impl, which shouldn't be crazy to fix. Would have to go actually try to be sure though.

tankyleo · 2026-01-20T20:13:03Z

@valentinewallace we just merged this PR #4305 which has a Deref indirection on EntropySource

jkczyz · 2026-01-20T21:23:13Z

lightning/src/sign/mod.rs

 	fn sign_message(&self, msg: &[u8]) -> Result<String, ()>;
 }

+impl<T: NodeSigner + ?Sized, N: Deref<Target = T>> NodeSigner for N {


IIUC, this means our types can now take their type parameterizations by value, too, instead of only by reference. Is there any concern with users misusing this?

They can already do this, of course, by instantiating two objects of the same type to parameterize two different types by references to them. But it's at least more obviously wrong (for types where this matters), but isn't as obvious now that users would be able to easily parameterize each type by instantiating the type parameter inline instead of passing an Arc or a reference.

In other words, some of our types are expected to share references. Maybe this is more of a documentation concern?

jkczyz · 2026-01-20T21:27:47Z

lightning-liquidity/src/manager.rs

 impl<
 		ES: EntropySource + Clone,
-		NS: Deref + Clone,
+		NS: NodeSigner + Clone,


Similarly, could be a footgun if a user implements both NodeSigner and Clone for a type and then passes it by value to one of our structs instead of just implementing NodeSigner and passing a clonable reference to it.

valentinewallace requested a review from TheBlueMatt January 13, 2026 19:17

valentinewallace force-pushed the 2026-01-avoid-derefs branch from 1c6cf54 to 584fec7 Compare January 13, 2026 22:34

valentinewallace commented Jan 13, 2026

View reviewed changes

lightning/src/chain/chaininterface.rs Show resolved Hide resolved

tnull reviewed Jan 14, 2026

View reviewed changes

Bump lightning-liquidity version

d545e5a

Useful for upcoming commits where we otherwise break SemVer checks by changing the ALiquidityManager associated types.

valentinewallace force-pushed the 2026-01-avoid-derefs branch from 584fec7 to ad7aaa5 Compare January 15, 2026 00:18

valentinewallace added this to Weekly Goals Jan 15, 2026

valentinewallace self-assigned this Jan 15, 2026

valentinewallace removed the request for review from TheBlueMatt January 15, 2026 19:26

valentinewallace force-pushed the 2026-01-avoid-derefs branch from ad7aaa5 to 028f1fb Compare January 15, 2026 21:49

joostjager reviewed Jan 15, 2026

View reviewed changes

Drop Deref indirection for BroadcasterInterface

93360c4

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

valentinewallace force-pushed the 2026-01-avoid-derefs branch from 028f1fb to 3630b91 Compare January 20, 2026 17:18

valentinewallace changed the title ~~Drop Deref indirection for ChannelManager, etc~~ Drop Deref indirection for most trait parameters Jan 20, 2026

valentinewallace force-pushed the 2026-01-avoid-derefs branch from 3630b91 to 2ceb510 Compare January 20, 2026 17:22

valentinewallace and others added 4 commits January 20, 2026 13:19

Drop Deref indirection for EntropySource

b2667a1

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for NodeSigner

7b20730

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for FeeEstimator

12c3d35

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for Router

989fa2e

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

valentinewallace and others added 11 commits January 20, 2026 13:20

Drop Deref indirection for MessageRouter

cdb5910

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for Logger

c5a77b0

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for SignerProvider

902ed6c

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for chain::Watch

8b5aea4

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for NodeIdLookup

b31285c

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for chain::Filter

cf7352e

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for OutputSpender

6d28678

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for UtxoLookup

580696e

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Drop Deref indirection for UtxoSource

db87b32

Reduces generics and verbosity across the codebase, should provide equivalent behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

valentinewallace force-pushed the 2026-01-avoid-derefs branch from 2ceb510 to db87b32 Compare January 20, 2026 18:47

valentinewallace marked this pull request as ready for review January 20, 2026 18:48

ldk-reviews-bot requested a review from wpaulino January 20, 2026 18:49

jkczyz reviewed Jan 20, 2026

View reviewed changes

wpaulino removed their request for review January 20, 2026 23:04

Drop Deref indirection for most trait parameters #4311

Are you sure you want to change the base?

Drop Deref indirection for most trait parameters #4311

Uh oh!

Conversation

valentinewallace commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

valentinewallace commented Jan 13, 2026

Uh oh!

valentinewallace commented Jan 13, 2026

Uh oh!

valentinewallace commented Jan 13, 2026

Uh oh!

TheBlueMatt commented Jan 13, 2026

Uh oh!

TheBlueMatt commented Jan 13, 2026

Uh oh!

Uh oh!

codecov bot commented Jan 13, 2026 • 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!

joostjager commented Jan 15, 2026

Uh oh!

valentinewallace commented Jan 15, 2026

Uh oh!

ldk-reviews-bot commented Jan 15, 2026

Uh oh!

joostjager Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

valentinewallace Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

joostjager Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

valentinewallace commented Jan 20, 2026

Uh oh!

TheBlueMatt commented Jan 20, 2026

Uh oh!

valentinewallace commented Jan 20, 2026

Uh oh!

TheBlueMatt commented Jan 20, 2026

Uh oh!

tankyleo commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkczyz Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

jkczyz Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Drop `Deref` indirection for most trait parameters #4311

Drop `Deref` indirection for most trait parameters #4311

valentinewallace commented Jan 13, 2026 •

edited

Loading

ldk-reviews-bot commented Jan 13, 2026 •

edited

Loading

codecov bot commented Jan 13, 2026 •

edited

Loading

tankyleo commented Jan 20, 2026 •

edited

Loading