Add LoggerScope for a thread-local Logger instance

[joostjager]

No description provided.

[ldk-reviews-bot]

👋 Hi! I see this is a draft PR.
I'll wait to assign reviewers until you mark it as ready for review.
Just convert it out of draft status when you're ready for review!

[codecov]

Codecov Report

❌ Patch coverage is 96.55172% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.33%. Comparing base (ec0b969) to head (ba31105).

Files with missing lines	Patch %	Lines
lightning/src/util/logger.rs	95.45%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4223      +/-   ##
==========================================
- Coverage   89.34%   89.33%   -0.01%     
==========================================
  Files         180      180              
  Lines      138166   138195      +29     
  Branches   138166   138195      +29     
==========================================
+ Hits       123438   123453      +15     
- Misses      12123    12137      +14     
  Partials     2605     2605              

Flag	Coverage Δ
fuzzing	35.86% <91.66%> (+0.04%)	⬆️
tests	88.71% <96.55%> (-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]

I played with this a bit, sadly Rust doesn't allow you to convert an arbitrary (?Sized) trait impl to dyn Trait (as it could lead to building a vtable that points to a type that has another vtable, which they could support by copying the vtable but do not). Thus, the real way to achieve this would be to convert the Deref<Target=Logger> bounds with Deref<Target=dyn Logger> (allowing us to simply deref the passed logger to get a dyn Logger). That's an annoying repetitive task so I asked claude to do it, but after fighting with it for a while it found that bounding on the dyn Logger results in objects that aren't Send/Sync making them not-multi-threaded, and then it immediately reset all its work and told me that I shouldn't want to do that:

The original design is actually more flexible and correct for Rust's type system. The generic bound doesn't prevent users from passing trait objects - it just doesn't require them, which is necessary for compatibility with different trait object variants.

(of course the log::Log trait is always Send + Sync-bounded to work around this issue). Luckily we have MaybeSend + MaybeSync for this purpose, so I made claude do that [1]. Anyway, once you get that far using the new LoggerScope is pretty doable (even in a trivial proc-macro [2], which could eventually push a scope onto the logger).

[1] https://git.bitcoin.ninja/?p=rust-lightning;a=shortlog;h=refs/heads/claude/2025-11-dyn-logger

[2]

/// Adds a logging scope at the top of a method.
#[proc_macro_attribute]
pub fn log_scope(_attrs: TokenStream, meth: TokenStream) -> TokenStream {
	let mut meth = if let Ok(parsed) = parse::<syn::ItemFn>(meth) {
		parsed
	} else {
		return (quote! {
			compile_error!("log_scope can only be set on methods")
		})
		.into();
	};
	let init_context = quote! { let _logging_context = crate::util::logger::LoggerScope::new(&*self.logger); };
	meth.block.stmts.insert(0, parse(init_context.into()).unwrap());
	quote! { #meth }.into()
}

[joostjager]

I've tried to make it work on top of your Maybe* commit, but failed. I can't get rid of

1066 |             let _scope = LoggerScope::new(&*self.logger); // DOES NOT WORK
     |                          ---------------- ^^^^^^^^^^^^^ doesn't have a size known at compile-time
     |                          |
     |                          required by a bound introduced by this call

First I thought you wanted to bound everything on dyn Logger to prevent the nested vtable, but that's not in your commit.

Proc macro for logger scope is interesting, and can indeed be extended with a node id in testing.

[TheBlueMatt]

The type passed to LoggerScope::new needs to change to take a &'a dyn Logger rather than taking a concrete Logger.

[joostjager]

I had tried that, but it didn't work. Same sized error. Pushed the commit. Maybe I am doing something wrong.

[joostjager]

I am wondering what the performance implications are of the logger scope. Just adding a node id in TLS would test-only, but storing the logger instance itself is production code that wouldn't be running if there was a global logger instead.

[TheBlueMatt]

I don't think its going to be material in either direct. Storing a dyn Logger in TLS shouldn't be more than like an extra 2 or 3 pointers (just 1 for the vtable, one for the self?) per thread. The existing logic was an extra ~1 pointer per logger object stored in various places (generally as Deref impls are usually just a single pointer). The dyn version requires jumping through the vtable indirection (and reading the vtable jump pointer from memory) so there's a tiny hit, but it shouldn't be substantial on any Real Hardware, really. TLS is a bit screwy on older rustc (and in C) on MacOS IIRC, but I assume by now that's all resolved.

[TheBlueMatt]

I had tried that, but it didn't work. Same sized error. Pushed the commit. Maybe I am doing something wrong.

Right, looks like claude just forgot to update the NetworkGraph logger bound to dyn.

[TheBlueMatt]

Oh, claude didn't update any of the references to dyn lol. Dumb LLMs just ignore half of what you tell them, I hadn't bothered to look at what it did, either. Anyway, gotta update all those bounds!