feat(background-processor): add BackgroundProcessorBuilder for optional components #3688

Anyitechs · 2025-03-28T08:01:55Z

This PR introduces two main improvements to the BackgroundProcessor:

Adds a BackgroundProcessorBuilder for a more ergonomic way to construct a BackgroundProcessor, and supports optional parameters through builder methods
Introduces BackgroundProcessorConfig to standardize configuration across sync (BackgroundProcessor) and async variants (process_events_async), enabling same configuration to be used for both variants. The Builder returns this config object, which can be used to construct a processor via BackgroundProcessor::from_config

ldk-reviews-bot · 2025-03-28T08:01:58Z

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

tnull

Already looks pretty good, one question though.

Also, do you have any idea how we could solve the same issue for the async variant of the background processor, i.e., for the optional arguments of process_events_async?

tnull · 2025-03-28T08:14:17Z

lightning-background-processor/src/lib.rs

@@ -1046,6 +1049,171 @@ impl BackgroundProcessor {
 			None => Ok(()),
 		}
 	}
+
+	/// Creates a new [`BackgroundProcessorBuilder`] to construct a   [`BackgroundProcessor`] with optional components.
+	pub fn builder<'a, PS, EH, M, CM, PGS, RGS, G, UL, L, PM>(


I'm a bit confused on the purpose of this method: in which scenario would we already have a working BackgroundProcessor to call builder on, just to then use the BackgroundProcessorBuilder to create yet another BackgroundProcessor?

This is confusing indeed. I was thinking the BackgroundProcessorBuilder::new() can be an internal method and accessible via builder, but it doesn't make sense. I will remove the builder method here and allow users use BackgroundProcessorBuilder::new() directly.

@tnull this doesn't take self, though?

Either way, normally the builder pattern has the builder() associated function, but works something like:
BackgroundProcessor::builder() constructs a BackgroundProcessorBuilder with "default" fields (which might be hard in this case), and you would not have any parameters for builder().

So yeah, probably removing this is best.

@tnull this doesn't take self, though?

Ah true.

Either way, normally the builder pattern has the builder() associated function, but works something like: BackgroundProcessor::builder() constructs a BackgroundProcessorBuilder with "default" fields (which might be hard in this case), and you would not have any parameters for builder().

Yeah, unclear what defaults would be.

ldk-reviews-bot · 2025-03-28T08:16:53Z

👋 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.

Anyitechs · 2025-03-28T09:00:13Z

Already looks pretty good, one question though.

Also, do you have any idea how we could solve the same issue for the async variant of the background processor, i.e., for the optional arguments of process_events_async?

Yea, I think we can extend the new BackgroundProcessorBuilder or provide a new builder for the async variant. But extending the new builder might be the better approach as we can avoid duplicating the builder logic since the sync and async variants share the same optional components.

tnull · 2025-03-28T09:07:32Z

Yea, I think we can extend the new BackgroundProcessorBuilder or provide a new builder for the async variant. But extending the new builder might be the better approach as we can avoid duplicating the builder logic since the sync and async variants share the same optional components.

The question is how this would work? process_events_async is just a method that users need to call regularly, so to 'build' it with different configuration you'd need to provide multiple versions of it, which can get messy real quick, essentially exactly the thing we tried to avoid with the builder pattern. It makes me wonder if we should introduce a BackgroundProcessorParams/BackgroundProcessorConfig object that process_events_async takes as its only argument. And if we do so, maybe we should/could reuse the same object for the non-async BackgroundProcessor, also? Meaning, this object could either replace the builder or we could have the builder return the new config object.

What do you think?

Anyitechs · 2025-03-28T09:53:33Z

Yea, I think we can extend the new BackgroundProcessorBuilder or provide a new builder for the async variant. But extending the new builder might be the better approach as we can avoid duplicating the builder logic since the sync and async variants share the same optional components.

The question is how this would work? process_events_async is just a method that users need to call regularly, so to 'build' it with different configuration you'd need to provide multiple versions of it, which can get messy real quick, essentially exactly the thing we tried to avoid with the builder pattern. It makes me wonder if we should introduce a BackgroundProcessorParams/BackgroundProcessorConfig object that process_events_async takes as its only argument. And if we do so, maybe we should/could reuse the same object for the non-async BackgroundProcessor, also? Meaning, this object could either replace the builder or we could have the builder return the new config object.

What do you think?

Thank you for the suggestion. I agree that introducing a BackgroundProcessorConfig object would be a better option. I'm thinking we can reuse the same object for both variants and have the builder return the new config object.

What do you think about that?

tnull · 2025-03-28T10:02:54Z

What do you think about that?

Sounds good to me, although we might need to account for the fact that the two variants have slightly different type requirements (i.e., trait bounds). But I think we should be able to accommodate that via feature-gates on std and/or futures.

Anyitechs · 2025-03-28T10:17:20Z

Sounds good to me, although we might need to account for the fact that the two variants have slightly different type requirements (i.e., trait bounds). But I think we should be able to accommodate that via feature-gates on std and/or futures.

Yes, that can be handled conditionally using feature-gates.

I guess I can go ahead with the implementation, right?

Anyitechs · 2025-04-09T17:11:46Z

This PR is ready for another round of review.

tnull · 2025-04-10T09:23:21Z

lightning-background-processor/src/lib.rs

-	onion_messenger: Option<OM>, gossip_sync: GossipSync<PGS, RGS, G, UL, L>, peer_manager: PM,
-	logger: L, scorer: Option<S>, sleeper: Sleeper, mobile_interruptable_platform: bool,
-	fetch_time: FetchTime,
+	config: BackgroundProcessorConfig<


nit: I think we might want to add a #[rustfmt::skip] here/above to avoid having rustfmt making this that vertical.

tnull · 2025-04-10T09:26:18Z

lightning-background-processor/src/lib.rs

@@ -1048,6 +1160,227 @@ impl BackgroundProcessor {
 	}
 }

+/// Configuration for both synchronous [`BackgroundProcessor`] and asynchronous [`process_events_async`] event processing.


I'm not sure these doc links will work under all circumstances as process_events_async will only be exposed when the futures feature is set. You might need to alter the docs based on the set feature via cfg_attr, see the changes to the background processor docs in #3509 for reference how to do this.

Ah true! Thank you.

tnull · 2025-04-10T09:29:09Z

lightning-background-processor/src/lib.rs

+	_phantom: PhantomData<(&'a (), CF, T, F, P)>,
+}
+
+/// A builder for constructing a [`BackgroundProcessor`] with optional components.


This should say BackgroundProcessorConfig now, no? And do we want to rename the builder to BackgroundProcessorConfigBuilder?

I agree. The builder builds a BackgroundProcessorConfig object, so renaming that to BackgroundProcessorConfigBuilder might be more suitable.

tnull · 2025-04-10T09:29:32Z

lightning-background-processor/src/lib.rs

+	PM::Target: APeerManager + Send + Sync,
+{
+	/// Creates a new builder instance.
+	pub(crate) fn new(


Seems CI is failing due to this being pub(crate)?

tnull · 2025-04-10T09:30:58Z

Did another round of review, will take another look once CI passes.

tnull · 2025-04-10T09:32:18Z

lightning-background-processor/src/lib.rs

+	/// .build();
+	/// let bg_processor = BackgroundProcessor::from_config(config);
+	/// ```
+	pub fn from_config<


I wonder if this should even replace start (i.e., whether we should have start just take a BackgroundProcessorConfig)?

I thought about this too but not sure if we want to make that change on start immediately.

Okay. I think we could drop the parameters from start and really only have it start the background thread, but no strong opinion.

Makes sense. So start takes a BackgroundProcessorConfig, and we can drop the from_config method?

Yeah, it sounds reasoable to have that as the new default constructor, as it should be easier to use anyways. Not sure if somebody else has a different opinion though, may be good to run this by a second reviewer.

Anyitechs · 2025-04-10T10:10:10Z

Did another round of review, will take another look once CI passes.

Thank you very much for the review. I'll address all the feedbacks and update the PR.

ldk-reviews-bot · 2025-04-12T00:00:30Z

🔔 1st Reminder

Hey @dunxen! 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.

ldk-reviews-bot · 2025-04-14T00:00:50Z

🔔 2nd Reminder

Hey @dunxen! 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.

Anyitechs · 2025-04-14T14:57:02Z

CI still failing, fixing it.

ldk-reviews-bot · 2025-04-16T00:01:22Z

🔔 3rd Reminder

Hey @dunxen! 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.

dunxen

Thanks! You'll need to run cargo fmt on lightning-background-processor/src/lib.rs and make sure each commit builds fine. You can run this check locally with ./ci/check-each-commit.sh main. It will do an interactive rebase and stop at the first problem commit, which you can fix with appropriate changes, git commit --amend '-S', and then git rebase --continue.

Anyitechs · 2025-04-16T13:59:05Z

Thanks! You'll need to run cargo fmt on lightning-background-processor/src/lib.rs and make sure each commit builds fine. You can run this check locally with ./ci/check-each-commit.sh main. It will do an interactive rebase and stop at the first problem commit, which you can fix with appropriate changes, git commit --amend '-S', and then git rebase --continue.

Thank you so much for the pointer. The email notification was getting much and I looked at the contributing guide severally to see if I can find some info on how to run the CI checks locally, but I couldn't find it.

Happy to add that, if it's worth having.

tnull

It seems CI is still broken due to broken docstest(s) in lightning-background-processor.

tnull

This needs a rebase now that #3509 was merged.

Anyitechs · 2025-04-24T11:28:30Z

This needs a rebase now that #3509 was merged.

Yes, I see the conflicts already. I'll fix that.

Anyitechs · 2025-05-17T05:45:40Z

Sorry I've not been available. I'll be continuing work on this PR this weekend to address all the issues.

…al components

codecov · 2025-05-28T13:44:15Z

Codecov Report

Attention: Patch coverage is 99.54955% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.69%. Comparing base (101aa6f) to head (793c777).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning-background-processor/src/lib.rs	99.54%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3688      +/-   ##
==========================================
+ Coverage   89.76%   90.69%   +0.92%     
==========================================
  Files         159      159              
  Lines      128828   138540    +9712     
  Branches   128828   138540    +9712     
==========================================
+ Hits       115644   125645   +10001     
+ Misses      10503    10363     -140     
+ Partials     2681     2532     -149

☔ 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.

Anyitechs · 2025-05-28T13:54:10Z

Had to clean up and squash the commits while fixing the CI error. May still need more work, but the PR is ready for another round of review.

tnull

Looks good so far, although the async counterpart is still missing. FWIW, if we can't reuse the exact type we should just have process_events_async take a similar AsyncBackgroundProcessorConfig or BackgroundProcessorConfigAsync that can be constructed via a corresponding builder.

Also seems like the second fixup commit can be squashed in?

tnull · 2025-05-29T08:17:12Z

lightning-background-processor/src/lib.rs

+/// event processing.
+///
+/// This configuration holds all components needed for background processing,
+/// including required components (like the channel manager and peer manager) and optional


nit: Let's either drop the examples here (as they don't add much IMO), or tick and link the actual objects, which will ensure they don't become outdated in the future as cargo doc will check the links.

tnull · 2025-05-29T08:18:15Z

lightning-background-processor/src/lib.rs

+/// A builder for constructing a [`BackgroundProcessorConfig`] with optional components.
+///
+/// This builder provides a flexible and type-safe way to construct a [`BackgroundProcessorConfig`]
+/// with optional components like `onion_messenger` and `scorer`. It helps avoid specifying


nit: Same here, let's either drop the like examples, or link them properly (maybe the former is preferable).

tnull · 2025-05-29T08:20:29Z

lightning-background-processor/src/lib.rs

+	K::Target: 'static + KVStore,
+{
+	/// Creates a new builder instance.
+	pub fn new(


Hmm, still on the fence whether it makes sense to re-add the BackgroundProcessorConfig::builder counterpart to this, which however usually doesn't take any parameters. But should we maybe still do it?

That's a good suggestion. But I'm not sure how that will work since we don't have defaults for all the parameters.

Well, I guess you could require to pass all required parameters to builder, but that breaks the pattern a bit, which is why I'm on the fence still :)

Up to you, I think.

tnull · 2025-05-29T08:22:42Z

lightning-background-processor/src/lib.rs

+	/// .build();
+	/// let bg_processor = BackgroundProcessor::from_config(config);
+	/// ```
+	pub fn from_config<


Okay. I think we could drop the parameters from start and really only have it start the background thread, but no strong opinion.

Anyitechs · 2025-05-29T13:25:01Z

Also seems like the second fixup commit can be squashed in?

Yes. Thank you.

FWIW, if we can't reuse the exact type we should just have process_events_async take a similar AsyncBackgroundProcessorConfig or BackgroundProcessorConfigAsync that can be constructed via a corresponding builder.

Yes, I'll add an AsyncBackgroundProcessorConfig and a corresponding builder for the process_events_async because we can't reuse the exact type of the sync variant.

tnull self-requested a review March 28, 2025 08:11

tnull reviewed Mar 28, 2025

View reviewed changes

Anyitechs marked this pull request as ready for review April 9, 2025 17:11

ldk-reviews-bot requested a review from wpaulino April 9, 2025 17:12

Anyitechs requested review from tnull and dunxen April 9, 2025 17:12

tnull removed the request for review from wpaulino April 9, 2025 17:17

tnull reviewed Apr 10, 2025

View reviewed changes

Anyitechs force-pushed the background-processor-builder branch from b967d87 to cb7ed79 Compare April 16, 2025 11:47

dunxen reviewed Apr 16, 2025

View reviewed changes

tnull reviewed Apr 17, 2025

View reviewed changes

Anyitechs force-pushed the background-processor-builder branch from e862860 to 7e707a9 Compare April 18, 2025 13:31

tnull mentioned this pull request Apr 23, 2025

lightning-liquidity: Introduce EventQueue notifier and wake BP for message processing #3509

Merged

tnull reviewed Apr 24, 2025

View reviewed changes

Anyitechs force-pushed the background-processor-builder branch from 7e707a9 to 3445bd5 Compare May 27, 2025 14:56

Anyitechs added 2 commits May 28, 2025 13:44

feat(background-processor): add BackgroundProcessorBuilder for option…

5534915

…al components

make BackgroundProcessorConfig docs conditional on futures feature

793c777

Anyitechs force-pushed the background-processor-builder branch from 3445bd5 to 793c777 Compare May 28, 2025 13:35

tnull reviewed May 29, 2025

View reviewed changes

feat(background-processor): add BackgroundProcessorBuilder for optional components #3688

Are you sure you want to change the base?

feat(background-processor): add BackgroundProcessorBuilder for optional components #3688

Uh oh!

Conversation

Anyitechs commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

tnull left a comment

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!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Mar 28, 2025

Uh oh!

Anyitechs commented Mar 28, 2025

Uh oh!

tnull commented Mar 28, 2025

Uh oh!

Anyitechs commented Mar 28, 2025

Uh oh!

tnull commented Mar 28, 2025

Uh oh!

Anyitechs commented Mar 28, 2025

Uh oh!

Anyitechs commented Apr 9, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull commented Apr 10, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Anyitechs commented Apr 10, 2025

Uh oh!

ldk-reviews-bot commented Apr 12, 2025

Uh oh!

ldk-reviews-bot commented Apr 14, 2025

Uh oh!

Anyitechs commented Apr 14, 2025

Uh oh!

ldk-reviews-bot commented Apr 16, 2025

Uh oh!

dunxen left a comment

Choose a reason for hiding this comment

Uh oh!

Anyitechs commented Apr 16, 2025

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Anyitechs commented Mar 28, 2025 •

edited

Loading

ldk-reviews-bot commented Mar 28, 2025 •

edited

Loading

codecov bot commented May 28, 2025 •

edited

Loading