rust-lightning Sweeper async change destination source fetching

This PR converts OutputSweeper to take an async ChangeDestinationSource implementation. This allows a (remote) address fetch call to run without blocking chain notifications.

Furthermore the changes demonstrates how LDK could be written in a natively async way, and still allow usage from a sync context using wrappers.

Part of #3540

Apr 14 '25 11:04 joostjager

👋 Thanks for assigning @tnull as a reviewer! I'll wait for their review and will help manage the review process. Once they submit their review, I'll check if a second reviewer would be helpful.

Apr 14 '25 11:04 ldk-reviews-bot

Codecov Report

Attention: Patch coverage is 81.65939% with 42 lines in your changes missing coverage. Please review.

Project coverage is 90.61%. Comparing base (f507778) to head (1a98158). Report is 68 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/util/sweep.rs	77.16%	25 Missing and 12 partials :warning:
lightning/src/util/async_poll.rs	57.14%	3 Missing :warning:
lightning-background-processor/src/lib.rs	96.22%	1 Missing and 1 partial :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3734      +/-   ##
==========================================
+ Coverage   89.13%   90.61%   +1.47%     
==========================================
  Files         157      157              
  Lines      123851   134826   +10975     
  Branches   123851   134826   +10975     
==========================================
+ Hits       110395   122170   +11775     
+ Misses      10779    10016     -763     
+ Partials     2677     2640      -37

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

Apr 21 '25 09:04 codecov[bot]

Basically LGTM, were there any major questions remaining you wanted resolved?

Few things:

We are breaking the sweeper api in this PR. Are there specific things to check/double-check now? Update ldk-node perhaps to see if it works? Not sure if much more can be done, given that rust-lightning is an open-source library.
The address inflation risk should have remained the same as before. We are calling sweeper on a timer now, but the filter function is still checking latest_broadcast_height. Or did I miss anything there?
Futures flag: https://github.com/lightningdevkit/rust-lightning/pull/3734#discussion_r2052470771

Apr 22 '25 06:04 joostjager

We are breaking the sweeper api in this PR. Are there specific things to check/double-check now? Update ldk-node perhaps to see if it works? Not sure if much more can be done, given that rust-lightning is an open-source library.

In general we don't care too much about API breaks between versions. Obviously we don't want to require downstream projects completely overhaul their integration with LDK just for the sake of it, but this change should basically just require adding "Sync" in a few places, so it should be trivial.

The address inflation risk should have remained the same as before. We are calling sweeper on a timer now, but the filter function is still checking latest_broadcast_height. Or did I miss anything there?

Sounds correct to me.

Futures flag: Sweeper async change destination source fetching #3734 (comment)

Indeed, I see no reason not to remove the futures feature from BP.