Add a background processor which is async

[TheBlueMatt]

This adds a new Future (which implements std::future::Future) which stores a list of callbacks to call upon completion. This is exposed from ChannelManager as an alternative way to be woken up when persistence needs to happen. Then, we add a new method in the background processor crate which works the same as the backgroundprocessor but async instead of in a thread.

Fixes #1595

[jkczyz]

Wonder if there is a way to move the common type parameters to BackgroundProcessor struct so as not to repeat this long trait bounds list. Then have an "implementation" type parameter to choose between sync and async. Then the macro could be a function without repeating all the trait bounds again.

[TheBlueMatt]

We spoke a bit offline about this, but I'm not sure how. I went down this rabbit hole initially and ended up repeating all the type stuff at least 4 times before I gave up and added a macro.

[wpaulino]

Not sure if you tried this already, but would transitioning BackgroundProcessor to be a trait with all of the generic parameters listed as associated types and the common logic between sync and async be extracted into a trait method help?

[TheBlueMatt]

Hmm, I think you still end up with the same issue - you create a trait, which lists all the generic crap, then you create two public structs which implement that trait (which both also have all the generic crap) then you create two impl blocks, which both also have all the generic crap. So now you listed it 5 times, I think :(

[wpaulino]

Perhaps we can just use a trait to group all the different generic bounds of start and process_events_async together and not actually implement it on BackgroundProcessor so that we can just reference the generic bounds by the new trait?

start's full function signature would resemble something like:

pub fn start<BPP: BackgroundProcessorParams>(
    persister: BPP::PS,
    event_handler: BPP::EH,
    chain_monitor: BPP::M,
    channel_manager: BPP::CM,
    gossip_sync: GossipSync<BPP::PGS, BPP::RGS, BPP::G, BPP::CA, BPP::L>,
    peer_manager: BPP::PM,
    logger: BPP::L,
    scorer: Option<BPP::S>,
) -> Self

[TheBlueMatt]

Sadly the trait methods would have to support being both async and sync, which I don't think we could capture in a single trait?

[jkczyz]

True. That best I could find was this: https://docs.rs/maybe-async/latest/maybe_async/

[TheBlueMatt]

Heh, I suppose we could....a macro is probably more readable than a proc macro, though :(

[jkczyz]

Probably could just feature gate two different versions if they are mutually exclusive, but probably not worth doing here. Just figured we could avoid the duplicative type parameters.

[tnull]

Btw. it's probably worth mentioning here that BDK went this route to provide blocking/async chain client implementations side-by-side (cf. https://github.com/bitcoindevkit/bdk/blob/master/macros/src/lib.rs#L84-L146). IIUC, they ran into some issues when splitting off the Esplora client from the main project and are looking into providing alternatives to give the user more control which impl they want to use.

[MaxFangX]

Btw, we implemented our own async background processor. Our project isn't open-source yet but I published the background processor as a gist here.

Trying to make an lightning-background-processor that abstracts over both async + blocking seems to be causing some problems. Perhaps it might be better to have a separate background processor crate that is async native, and which looks something like a generified version of the above? The above implementation is ~150 lines before adding generics, so the core logic really is pretty simple.

We've had some papercuts using LDK's background processor where because the Persister trait methods are blocking, async persisters end up having to do some funky hacks to implement them. A purely async background processor could define an AsyncPersister trait, and allow the user to pass in a future that completes when they want to initiate a graceful shutdown (here's an example from warp). All of the const Durations at the top could also be configurable with a BackgroundConfig which has a Default impl. This implementation also returns a JoinHandle which allows users (who are probably using tokio) to .await on the background processor to gracefully shut down (good practice to manage all your spawned tasks)

[TheBlueMatt]

Btw, we implemented our own async background processor. Our project isn't open-source yet but I published the background processor as a gist here.

Cool! Hopefully the next folks after you won't have to :)

Trying to make an lightning-background-processor that abstracts over both async + blocking seems to be causing some problems. Perhaps it might be better to have a separate background processor crate that is async native, and which looks something like a generified version of the above? The above implementation is ~150 lines before adding generics, so the core logic really is pretty simple.

Given the ordering, delays, and error handling are very specific here and it'd be very, very easy to introduce a bug if the two diverged, I'm strongly opposed to maintaining two copies. It wouldn't reduce the amount of code we have, either, as we'd still need the whole same mess of generics we have here - we'd just have them twice in two different files. Ultimately for very finicky code, sticking to DRY is a very important goal. @jkczyz pointed out above that ideally we'd have to repeat ourselves as many times as we do (or avoid the macro), but it's not clear to me exactly how, I'd love any concrete suggestions!

We've had some papercuts using LDK's background processor where because the Persister trait methods are blocking, async persisters end up having to do some funky hacks to implement them.

Not really sure why you need a static here? Sensei has a similar problem, but simply stores a reference to the tokio runtime directly in relevant structs that implement persist traits, rather than making a static, but that's somewhat besides the point.

A purely async background processor could define an AsyncPersister trait, and allow the user to pass in a future that completes when they want to initiate a graceful shutdown (here's an example from warp)

Yep, we want to make persist more async, see eg #1470. As you note, in general Rust is not at all supportive of mixing async and sync stuff in the same project, Rust-async and Rust-sync are basically two separate languages. Sadly for LDK we can't drop Rust-sync support (cause most of our users ultimately use language bindings rather than Rust) so that is what got built first and the best we can do is add async wrappers, which we're slowly building, though as you've noted they're still somewhat early in places. Sensei has been a trailblazer here, and has managed to work around the papercuts for the most part, though things like this PR came about because they reported where the papercuts were, which helps us prioritize what to build.

All of the const Durations at the top could also be configurable with a BackgroundConfig which has a Default impl

I'm not sure what exactly users need to change about the durations set here? This is also a separate discussion from sync/async, so lets move to a new issue if you have some specific tuning you'd like to see supported.

This implementation also returns a JoinHandle which allows users (who are probably using tokio) to .await on the background processor to gracefully shut down (good practice to manage all your spawned tasks)

I'm not quite sure what you're suggesting here? The new method here does return a future, which is ~equivalent to a JoinHandle, though without being tokio-specific so users can use whatever async runtime they want. It also has a way to shut down the future if you can't unregister a future in your runtime, which we also use to fetch a runtime-specific sleep future.

[codecov-commenter]

Codecov Report

Merging #1657 (7ad92f4) into main (b84b53a) will increase coverage by 0.07%.
The diff coverage is 87.61%.

❗ Current head 7ad92f4 differs from pull request most recent head 2a5bac2. Consider uploading reports for the commit 2a5bac2 to get more accurate results

@@            Coverage Diff             @@
##             main    #1657      +/-   ##
==========================================
+ Coverage   90.77%   90.84%   +0.07%     
==========================================
  Files          80       86       +6     
  Lines       44763    46303    +1540     
  Branches    44763    46303    +1540     
==========================================
+ Hits        40632    42065    +1433     
- Misses       4131     4238     +107     

Impacted Files	Coverage Δ
lightning-invoice/src/payment.rs	90.86% <ø> (+3.10%)	⬆️
lightning/src/lib.rs	100.00% <ø> (ø)
lightning/src/ln/mod.rs	95.00% <ø> (ø)
lightning/src/util/config.rs	66.66% <0.00%> (-0.78%)	⬇️
lightning/src/util/ser.rs	91.41% <ø> (ø)
lightning/src/chain/keysinterface.rs	92.54% <50.00%> (-1.06%)	⬇️
lightning/src/ln/msgs.rs	86.24% <50.00%> (-0.04%)	⬇️
lightning/src/util/test_utils.rs	77.49% <66.66%> (-0.62%)	⬇️
lightning/src/onion_message/packet.rs	72.52% <74.15%> (ø)
lightning/src/onion_message/messenger.rs	89.50% <85.92%> (ø)
... and 45 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[dunxen]

So after refreshing some async knowledge this basically looks good to me with nothing else to really point out. Just that one question.

[jkczyz]

Not sure if the Executor idea is possible, but otherwise LGTM.

[jkczyz]

Feel free to squash

[wpaulino]

LGTM

[dunxen]

LGTM besides just the tests that need to be excluded for no-std.

[TheBlueMatt]

Squashed the old fixups and added two new ones.

[wpaulino]

CI is now failing with

error[E0599]: no method named `poll` found for struct `Pin<&mut wakers::Future>` in the current scope
   --> lightning/src/util/wakers.rs:325:36
    |
325 |         assert_eq!(Pin::new(&mut future).poll(&mut Context::from_waker(&waker)), Poll::Pending);
    |                                          ^^^^ method not found in `Pin<&mut wakers::Future>`
    |
    = help: items from traits can only be used if the trait is implemented and in scope
    = note: the following trait defines an item `poll`, perhaps you need to implement it:
            candidate #1: `std::future::Future`

[TheBlueMatt]

Oops sorry. I just removed the std bounds, we can impl Future even for no-std.

[dunxen]

Looks good to go after squash!

[TheBlueMatt]

Squashed without further change.

[TheBlueMatt]

Oops, added missing include:

$ git diff-tree -U1 7ad92f4c2 2a5bac22bdiff --git a/lightning/src/util/wakers.rs b/lightning/src/util/wakers.rs
index abf3f3034..b81dacbd0 100644
--- a/lightning/src/util/wakers.rs
+++ b/lightning/src/util/wakers.rs
@@ -29,2 +29,4 @@ use core::pin::Pin;
 
+use prelude::*;
+
 /// Used to signal to one of many waiters that the condition they're waiting on has happened.