Event handling races with channelmanager storage

[TheBlueMatt]

Currently if you call ChannelManager::get_and_clear_pending_events while serializing out the ChannelManager you can race - clearing the pending events, writing the channelmanager out, then crashing before having handled the events, thus potentially losing them.

Previously, I'd thought about adding an event handling function in lightning_background_processor::BackgroundProcessor so that we default to a single loop which handles events and serializes the ChannelManager, avoiding many of the issues. See also the issue on the sample client about this - lightningdevkit/ldk-sample#4 (comment), one thing we'd ideally like to support is for a tokio client to process many of the events in an async context, allowing them to use async functions instead of relying on our sync API. It may be possible to keep our synchronous API, but have users call some method to spawn futures in a tokio context which exists outside of the background processor thread, and then have the background processor thread block_on those futures completing, but I'm not sure if tokio supports it.

Still, while this approach should help significantly, but there is a second race that is much trickier to fix - we can end up where users process an event, store something about it to disk, then crash before re-serializing the ChannelManger, resulting in duplicate processing.

[jkczyz]

Still, while this approach should help significantly, but there is a second race that is much trickier to fix - we can end up where users process an event, store something about it to disk, then crash before re-serializing the ChannelManger, resulting in duplicate processing.

One possible solution to this is to expand the traits in #920 as following:

• Add an event_id: u64 parameter to EventHandler::handle_event
• Add a method fn last_processed_event(&self) -> u64 to EventsProvider

Implementations of EventsProvider would use a strictly increasing id for each processed event. It would need to serialize this just as it serializes unprocessed events.

When handling events, an EventHandler would track the events it processed in some manner. For instance, it may be implemented in a multiprocess system where the handler writes the event to a database along with the id and a second process picks up the events. The handler would use the id to determine if it has seen the event before. (Actually, the last_processed_event method may not be necessary to implement this.)

The problem that I see with this approach is that it wouldn't work as stated if an EventHandler is used to process events from more than one EventsProviders. Currently, this is the case with ChannelManager and `ChainMonitor.

[TheBlueMatt]

A bigger issue with event_ids is creating them - imagine a ChannelManager with two outbound payments pending. We receive an update_fulfill_htlc message from peer A, generating a PaymentSent event. The user handles it, but before persisting the ChannelManager again we crash. Then, upon restart, we receive an update_fulfill_htlc message from peer B before we manage to reconnect to peer A, generating a different PaymentSent. Naively, without any other information and just a simple counter, those two events (despite being different) would have the same event_id.

We could resolve this in a few ways - we could separate the event_id namespace into per-event-type namespaces and ensure the event_ids are somehow calculable in advance. Alternatively, we could wait until an event is persisted to disk before ever giving it to the user, at worst doubling our persistence calls. Both kinda suck, but I'm not sure how else you could do it.

[jkczyz]

A bigger issue with event_ids is creating them - imagine a ChannelManager with two outbound payments pending. We receive an update_fulfill_htlc message from peer A, generating a PaymentSent event. The user handles it, but before persisting the ChannelManager again we crash. Then, upon restart, we receive an update_fulfill_htlc message from peer B before we manage to reconnect to peer A, generating a different PaymentSent. Naively, without any other information and just a simple counter, those two events (despite being different) would have the same event_id.

Hmmm, right, that would be a problem. To fill in my lack of understanding, when reconnecting to peer A does said peer resend an update_fulfill_htlc message?

Related, I noticed that our docs say users must be able to handle duplicative events:

https://github.com/rust-bitcoin/rust-lightning/blob/3b67be235a46d8f9a9a6faf7ca7c5872ffbd8646/lightning/src/util/events.rs#L98-L99

We could resolve this in a few ways - we could separate the event_id namespace into per-event-type namespaces and ensure the event_ids are somehow calculable in advance.

When you say "calculable in advance" do you mean to ensure ordering (which given the above, probably not possible)? Or do you mean to uniquely identify them based off of their fields (e.g., payment_preimage for PaymentSent events).

Alternatively, we could wait until an event is persisted to disk before ever giving it to the user, at worst doubling our persistence calls. Both kinda suck, but I'm not sure how else you could do it.

Could get a little ugly since you'd have queues for "not persisted" and "pending" events, but you can't guarantee that events were persisted before moving them to pending. You could only know the persister has been notified.

[TheBlueMatt]

Hmmm, right, that would be a problem. To fill in my lack of understanding, when reconnecting to peer A does said peer resend an update_fulfill_htlc message?

Correct.

Related, I noticed that our docs say users must be able to handle duplicative events:

Right, that's at least somewhat independent, at least the cases where we generate duplicate events are fixed by #918. Though maybe given this we should keep the docs there?

When you say "calculable in advance" do you mean to ensure ordering (which given the above, probably not possible)? Or do you mean to uniquely identify them based off of their fields (e.g., payment_preimage for PaymentSent events).

Right, I mean "assign a future event id to the PaymentSent event long before we generate it, presumably when we go to send the payment".

Could get a little ugly since you'd have queues for "not persisted" and "pending" events, but you can't guarantee that events were persisted before moving them to pending. You could only know the persister has been notified.

Right. We could also do it at serialize time, so you at least know the ChannelManager bytes were handed to the user, but, indeed, they may not be on disk.

[TheBlueMatt]

The immediately-solvable part was addressed with #920, so removing the milestone.

[TheBlueMatt]

The second part of this issue which is still unresolved is:

Still, while this approach should help significantly, but there is a second race that is much trickier to fix - we can end up where users process an event, store something about it to disk, then crash before re-serializing the ChannelManger, resulting in duplicate processing.

However, I don't think its worth keeping open for this. Ultimately we cannot fundamentally address this and we rely on our users to handle it. We include unique IDs in many events to make this easy.