Make BumpTransactionEventHandler async-optional

[TheBlueMatt]

Turns out this it the only async -> sync -> (blocked) async inversion we have in ldk-sample, so making the traits async-optional (via some proc-macro?) would be really nice.

[tnull]

Turns out this it the only async -> sync -> (blocked) async inversion we have in ldk-sample,

I'm not quite sure this is true, due to ChangeDestinationSource being used in OutputSweeper's spending method which in turn is triggered by Confirm/Listen (and similar would hold for WalletSource, as the anchor spends are triggered by new best blocks?). So if we make the traits async-optional, we'd also need to propagate it up the chain syncing logic, including all the traits and connected methods, no?

I mean I'm all for more async-optional traits, but I'm not sure how we'd actually use async variants of ChangeDestinationSource/WalletSource in our code without introducing async contagion currently?

[TheBlueMatt]

Grr, right. I suppose we could fix that with OutputSweeper having a fixed payout address (or a list) that is pre-configured if users want to avoid that inversion. Still seems worth doing?

[tnull]

Grr, right. I suppose we could fix that with OutputSweeper having a fixed payout address (or a list) that is pre-configured if users want to avoid that inversion. Still seems worth doing?

That sound pretty bad privacy-wise? There would really be no use doing #1139 and/or going out of our way to do something similar while making payment key recoverable, to then just sweep everything to the same address in the end?

Another approach to solve this for the OutputSweeper would be to trigger spending/rebroadcasting via the (already optionally-async) background processor rather than have it triggered by Filter/Confirm (which might be a good idea anyways, as more and more stuff piles onto block connection, and resulting deadlocks and/or performance spikes might get harder and harder to debug).

And I guess we for the other concerns we could make the BumpTransactionEventHandler and related traits async-optional as well (the latter via proc-macros via async_trait I guess, until our MSRV hits 1.75? Essentially simply re-doing and applying what we dropped during #3330?)?

[tnull]

Just discussed offline that we try to prioritize this for 0.2, i.e., making ChangeDestinationSource/WalletSource async-optional and have the sweeper and BumpTransactionEventHandler driven by the BackgroundProcessor.

[joostjager]

After exploring optional asyncification and also (just for me) rust async in general, it seems that trying to do it all using macros isn't very pleasant.

There's the macro that duplicates and asyncifies all the 'async-contaminated' functions, similar to maybe_async. But also for example for the OutputSweeper itself that is dependent on ChangeDestinationSource, a macro would need to duplicate the whole struct with a different (async) parameter for the trait.

pub struct OutputSweeper<B: Deref, D: Deref, E: Deref, F: Deref, K: Deref, L: Deref, O: Deref>
where
    B::Target: BroadcasterInterface,
    D::Target: ChangeDestinationSource,
    E::Target: FeeEstimator,
    F::Target: Filter + Sync + Send,
    K::Target: KVStore,
    L::Target: Logger,
    O::Target: OutputSpender,

And then OutputSweeper is still relatively on the side. For other interfaces, the macros may end up duplicating large portions of LDK? ChannelManager has its own homebrewn callback mechanism, so that large part could probably avoid async duplication.

Also driving sweeping from the background processor just to be able to use it asynchronously seems like a workaround too, which may not be ideal either.

An alternative is to continue with the homebrewn callbacks for the wallet operations that this issue aims to make non-blocking. Need to see whether that introduces circular dependencies.

(mostly take aways from offline discussion with @TheBlueMatt)

[joostjager]

Update: went with the third option of providing sync-wrappers in #3734.