Add event notifier to call `process_pending_htlcs_forwards()` without the need to poll

[MaxFangX]

In this PR where was a brief discussion about adding some sort of notification mechanism so LDK users don't have to repeatedly poll process_pending_htlcs_forwards() to ensure that HTLCs forwards are processed:

#3891 (comment)

Lexe is very interested in such a mechanism. In the meantime we're increasing our polling interval to much higher (250ms-1s) to reduce the amount of unnecessary wakes.

Reproducing the conversation here for convenience:

@MaxFangX:

Reviewed the background processor changes; appears to be a great latency improvement, and this approach works for Lexe, but we'd prefer to have an event or notification mechanism that allows the background processor to be woken only when there are pending HTLCs to forward, similar to what @joostjager is suggesting (in particular, one which doesn't require persistence). Currently, we run 10 Lightning nodes (and thus 10 background processors) per thread; at an average of one wake every 50ms that's 20 × 10 = 200 wakes per second. We want to increase to 100 nodes per thread, which quickly becomes very wasteful at 2000 wakes per second.

I don't have any strong opinions on what the API should look like; it could be as simple as a trait which LDK calls anytime the BGP should be woken to process pending forwards. Maybe something like:

trait HtlcsForwardableTrigger {
    /// Notifies the implementer that there are pending HTLCs which can be forwarded.
    fn pending_htlcs_forwardable(&self);
}

struct LexeHtlcsForwardableTrigger {
    htlcs_forwardable_tx: mpsc::Sender<()>,
}

impl HtlcsForwardableTrigger for LexeHtlcsForwardableTrigger {
    fn pending_htlcs_forwardable(&self) {
        let _ = self.htlcs_forwardable_tx.try_send(());
    }
}

// Then BGP is listening on the htlcs_forwardable_rx ...

@tnull:

Thanks for the review and feedback! Yes, I think I mentioned this somewhere above, but I still intend to look into adding a notifier as an efficiency improvement so that BP is only woken after the batch delay if there is something to process. I had planned to do this as part of the next PR/as a follow-up to keep this PR less invasive and since it might become a bit more clear then how that interacts with receiver-side delays (e.g., in case we decide to split out the receive case). I don't think we would create yet another interface just for this though, it would likely just be a notifier that only wakes the BP when needed. For anything else you'd need to use #2855 once we have it.

@TheBlueMatt:

I wonder if we can't use the existing channelmanager notification logic to avoid unnecessary wakes — the ChannelManager should always want to be persisted any time there's something available to forward, so we could skip the timer in cases where we woke for some reason other than the ChannelManager's existing event-or-persist waker.

The only issue is that if the delay hasn't passed when we wake, we'd need to re-wake once it does, basically, otherwise we might delay much longer.

@tnull (edited):

Yeah, that was also my thought. We might need to notify where we used to push the event previously, but other than that this approach might indeed suffice. The only issue is that if the delay hasn't passed when we wake, we'd need to re-wake once it does, basically, otherwise we might delay much longer.

@TheBlueMatt:

Yea, we'll have to track a have_forwarded_since_last_wake: bool and just always make sure we refresh the forwarding sleeper any time it's set.

[tnull]

Lexe is very interested in such a mechanism. In the meantime we're increasing our polling interval to much higher (250ms-1s) to reduce the amount of unnecessary wakes.

Please note that in that PR we actually implemented the discussed change in the background processor as the batch delay is only applied (and the background processor re-woken again) if we have pending HTLCs that need processing. So we're skipping wake-ups if there's nothing to be done.

We also introduced logic there to skip repersisting ChannelManager if nothing changed. So a) you should see less spurious wake-ups and repersists after #3891 and b) you'd only need to poll process_pending_htlcs_forwards if you want to 'go faster' than the 50ms mean delay, which is probably a reasonable delay for IO efficieny anyways. But we also made sure that polling in addition to the BP is safer and cheaper now.

[MaxFangX]

@tnull thanks for the tip. Looking into it now trying to understand the code changes; will come back here if I have any questions (or close the issue if all resolved)

[MaxFangX]

@tnull Thank you for the help. I've dug into the code and now understand that channel manager get_event_or_persistence_needed_future() will get woken if there are HTLCs to forward, and in the next loop iteration needs_pending_htlc_processing() will return true, which will then set the sleep delay to a maximum of 100 ms, which then forwards the HTLCs after regular event processing, so that overall there are no spurious wakes. It would be very difficult to understand the "discovered new HTLCs to forward" -> "process_pending_htlc_forwards() is called" event flow just by looking at LDK's BGP code, so your input is much appreciated. I've implemented similar logic in Lexe's BGP (should be landed and published within a week or so).