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

[Anyitechs]

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

Fixes #3612

[ldk-reviews-bot]

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

[ldk-reviews-bot]

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

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]

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]

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]

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]

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]

This PR is ready for another round of review.

[tnull]

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

[Anyitechs]

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]

🔔 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]

🔔 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]

CI still failing, fixing it.

[ldk-reviews-bot]

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

[Anyitechs]

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.

[Anyitechs]

This needs a rebase now that #3509 was merged.

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

[Anyitechs]

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

[codecov[bot]]

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 :warning:

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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Anyitechs]

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.

[Anyitechs]

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.

[Anyitechs]

@tnull Force-pushed to address all your feedbacks. Also introduced BackgroundProcessorConfigAsync and BackgroundProcessorConfigAsyncBuilder for the async counterpart of the BackgroundProcessor.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tnull! 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]

No major changes. Force-pushed to fix CI that was failing due to the update on the ChainMonitor generic arguments.

[joostjager]

I tried out passing None (pre this PR) for the onion messenger, and it required the following. Horror 😅

None::<Arc<lightning::onion_message::messenger::OnionMessenger<Arc<wallet::WalletKeysManager<Arc<tx_broadcaster::TransactionBroadcaster<Arc<Logger>>>, Arc<fee_estimator::OnchainFeeEstimator>, Arc<Logger>>>, Arc<wallet::WalletKeysManager<Arc<tx_broadcaster::TransactionBroadcaster<Arc<Logger>>>, Arc<fee_estimator::OnchainFeeEstimator>, Arc<Logger>>>, Arc<Logger>, Arc<lightning::ln::channelmanager::ChannelManager<Arc<lightning::chain::chainmonitor::ChainMonitor<lightning::sign::InMemorySigner, Arc<ChainSource>, Arc<tx_broadcaster::TransactionBroadcaster<Arc<Logger>>>, Arc<fee_estimator::OnchainFeeEstimator>, Arc<Logger>, Arc<dyn KVStore + Send + Sync + 'static>>>, Arc<tx_broadcaster::TransactionBroadcaster<Arc<Logger>>>, Arc<wallet::WalletKeysManager<Arc<tx_broadcaster::TransactionBroadcaster<Arc<Logger>>>, Arc<fee_estimator::OnchainFeeEstimator>, Arc<Logger>>>, Arc<wallet::WalletKeysManager<Arc<tx_broadcaster::TransactionBroadcaster<Arc<Logger>>>, Arc<fee_estimator::OnchainFeeEstimator>, Arc<Logger>>>, Arc<wallet::WalletKeysManager<Arc<tx_broadcaster::TransactionBroadcaster<Arc<Logger>>>, Arc<fee_estimator::OnchainFeeEstimator>, Arc<Logger>>>, Arc<fee_estimator::OnchainFeeEstimator>, Arc<lightning::routing::router::DefaultRouter<Arc<lightning::routing::gossip::NetworkGraph<Arc<Logger>>>, Arc<Logger>, Arc<wallet::WalletKeysManager<Arc<tx_broadcaster::TransactionBroadcaster<Arc<Logger>>>, Arc<fee_estimator::OnchainFeeEstimator>, Arc<Logger>>>, Arc<Mutex<lightning::routing::scoring::ProbabilisticScorer<Arc<lightning::routing::gossip::NetworkGraph<Arc<Logger>>>, Arc<Logger>>>>, lightning::routing::scoring::ProbabilisticScoringFeeParameters, lightning::routing::scoring::ProbabilisticScorer<Arc<lightning::routing::gossip::NetworkGraph<Arc<Logger>>>, Arc<Logger>>>>, Arc<lightning::onion_message::messenger::DefaultMessageRouter<Arc<lightning::routing::gossip::NetworkGraph<Arc<Logger>>>, Arc<Logger>, Arc<wallet::WalletKeysManager<Arc<tx_broadcaster::TransactionBroadcaster<Arc<Logger>>>, Arc<fee_estimator::OnchainFeeEstimator>, Arc<Logger>>>>>, Arc<Logger>>>, Arc<lightning::onion_message::messenger::DefaultMessageRouter<Arc<lightning::routing::gossip::NetworkGraph<Arc<Logger>>>, Arc<Logger>, Arc<wallet::WalletKeysManager<Arc<tx_broadcaster::TransactionBroadcaster<Arc<Logger>>>, Arc<fee_estimator::OnchainFeeEstimator>, Arc<Logger>>>>>, Arc<lightning::ln::channelmanager::ChannelManager<Arc<lightning::chain::chainmonitor::ChainMonitor<lightning::sign::InMemorySigner, Arc<ChainSource>, Arc<tx_broadcaster::TransactionBroadcaster<Arc<Logger>>>, Arc<fee_estimator::OnchainFeeEstimator>, Arc<Logger>, Arc<dyn KVStore + Send + Sync + 'static>>>, Arc<tx_broadcaster::TransactionBroadcaster<Arc<Logger>>>, Arc<wallet::WalletKeysManager<Arc<tx_broadcaster::TransactionBroadcaster<Arc<Logger>>>, Arc<fee_estimator::OnchainFeeEstimator>, Arc<Logger>>>, Arc<wallet::WalletKeysManager<Arc<tx_broadcaster::TransactionBroadcaster<Arc<Logger>>>, Arc<fee_estimator::OnchainFeeEstimator>, Arc<Logger>>>, Arc<wallet::WalletKeysManager<Arc<tx_broadcaster::TransactionBroadcaster<Arc<Logger>>>, Arc<fee_estimator::OnchainFeeEstimator>, Arc<Logger>>>, Arc<fee_estimator::OnchainFeeEstimator>, Arc<lightning::routing::router::DefaultRouter<Arc<lightning::routing::gossip::NetworkGraph<Arc<Logger>>>, Arc<Logger>, Arc<wallet::WalletKeysManager<Arc<tx_broadcaster::TransactionBroadcaster<Arc<Logger>>>, Arc<fee_estimator::OnchainFeeEstimator>, Arc<Logger>>>, Arc<Mutex<lightning::routing::scoring::ProbabilisticScorer<Arc<lightning::routing::gossip::NetworkGraph<Arc<Logger>>>, Arc<Logger>>>>, lightning::routing::scoring::ProbabilisticScoringFeeParameters, lightning::routing::scoring::ProbabilisticScorer<Arc<lightning::routing::gossip::NetworkGraph<Arc<Logger>>>, Arc<Logger>>>>, Arc<lightning::onion_message::messenger::DefaultMessageRouter<Arc<lightning::routing::gossip::NetworkGraph<Arc<Logger>>>, Arc<Logger>, Arc<wallet::WalletKeysManager<Arc<tx_broadcaster::TransactionBroadcaster<Arc<Logger>>>, Arc<fee_estimator::OnchainFeeEstimator>, Arc<Logger>>>>>, Arc<Logger>>>, lightning::ln::peer_handler::IgnoringMessageHandler, lightning::ln::peer_handler::IgnoringMessageHandler, lightning::ln::peer_handler::IgnoringMessageHandler>>>,

[tnull]

@Anyitechs let us know when you rebased and this is ready for another (hopefully final) look!

[Anyitechs]

@Anyitechs let us know when you rebased and this is ready for another (hopefully final) look!

Thanks! This PR is ready for another look. Rebased and pushed 2b83ad2 to address @joostjager's comment on the docs update (will squash it in if the changes looks good).

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tnull @joostjager! 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]

🔔 1st Reminder

Hey @tnull @joostjager! 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]

But the main motivation for this change in the first place is to provide users a more reasonable API that allows them to not provide all fields, no? I think this means we'd still need to provide internal dummy values, and have them only overridden with the with_ methods, as otherwise the changes in this PR just add a bunch boilerplate without fixing the actual issue we wanted to address.

Could you look into this and see whether we find a way forward here? While we do this, could we also align the GossipSync case, which already did it with a dummy value before and hence was somewhat the odd one out in the new builder API?

Thank you for catching this. I'm working on a fix already.

[joostjager]

If we still need dummy implementations for everything, I'd feel more strongly than before about just dropping the builder.