Add a parallel async event handler to OnionMessenger and pass it directly to BackgroundProcessor #3060

TheBlueMatt · 2024-05-10T21:31:05Z

This adds an OnionMessenger::process_pending_events_async
mirroring the same in ChannelManager. However, unlike the one in
ChannelManager, this processes the events in parallel by spawning
all futures and using the new MultiFuturePoller.

Because OnionMessenger just generates a stream of messages to
store/fetch, we first process all the events to store new messages,
await them, then process all the events to fetch stored messages,
ensuring reordering shouldn't result in lost messages (unless we
race with a peer disconnection, which could happen anyway).

This allows us to have a bound on any `OnionMessenger` without having to create bounds for every bound in `OnionMessenger`.

If we have a handful of futures we want to make progress on simultaneously we need some way to poll all of them in series, which we add here in the form of `MultiFuturePoller`. Its probably not as effecient as some of the options in the `futures` crate, but it is very trivial and not that bad.

codecov-commenter · 2024-05-10T23:02:44Z

Codecov Report

Attention: Patch coverage is 78.07018% with 25 lines in your changes missing coverage. Please review.

Project coverage is 92.08%. Comparing base (0ffa4b3) to head (fadb268).
Report is 76 commits behind head on main.

Files	Patch %	Lines
lightning/src/util/async_poll.rs	29.41%	11 Missing and 1 partial ⚠️
lightning/src/onion_message/messenger.rs	83.82%	7 Missing and 4 partials ⚠️
lightning-background-processor/src/lib.rs	93.10%	0 Missing and 2 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3060      +/-   ##
==========================================
+ Coverage   89.83%   92.08%   +2.25%     
==========================================
  Files         116      118       +2     
  Lines       96472   113057   +16585     
  Branches    96472   113057   +16585     
==========================================
+ Hits        86663   104113   +17450     
+ Misses       7244     6726     -518     
+ Partials     2565     2218     -347

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

valentinewallace · 2024-05-20T15:36:33Z

Looks like 1.63 builds are sad

valentinewallace · 2024-05-20T15:57:54Z

lightning/src/onion_message/messenger.rs

+	/// A type implementing [`EntropySource`]
+	type EntropySource: EntropySource + ?Sized;
+	/// A type that may be dereferenced to [`Self::EntropySource`]
+	type ES: Deref<Target = Self::EntropySource>;
+	/// A type implementing [`NodeSigner`]
+	type NodeSigner: NodeSigner + ?Sized;
+	/// A type that may be dereferenced to [`Self::NodeSigner`]
+	type NS: Deref<Target = Self::NodeSigner>;
+	/// A type implementing [`Logger`]
+	type Logger: Logger + ?Sized;
+	/// A type that may be dereferenced to [`Self::Logger`]
+	type L: Deref<Target = Self::Logger>;
+	/// A type implementing [`NodeIdLookUp`]
+	type NodeIdLookUp: NodeIdLookUp + ?Sized;
+	/// A type that may be dereferenced to [`Self::NodeIdLookUp`]
+	type NL: Deref<Target = Self::NodeIdLookUp>;
+	/// A type implementing [`MessageRouter`]
+	type MessageRouter: MessageRouter + ?Sized;
+	/// A type that may be dereferenced to [`Self::MessageRouter`]
+	type MR: Deref<Target = Self::MessageRouter>;
+	/// A type implementing [`OffersMessageHandler`]
+	type OffersMessageHandler: OffersMessageHandler + ?Sized;
+	/// A type that may be dereferenced to [`Self::OffersMessageHandler`]
+	type OMH: Deref<Target = Self::OffersMessageHandler>;
+	/// A type implementing [`CustomOnionMessageHandler`]
+	type CustomOnionMessageHandler: CustomOnionMessageHandler + ?Sized;
+	/// A type that may be dereferenced to [`Self::CustomOnionMessageHandler`]


nit: end sentences in periods

I don't think they're full sentences?

AChannelManager's docs have the same sentence structure and use periods. Feel free to ignore, though.

lightning/src/util/async_poll.rs

valentinewallace · 2024-05-20T18:20:15Z

lightning/src/onion_message/messenger.rs

+				Some(Event::OnionMessageIntercepted { .. }) => {
+					futures.push(Some(handler(next_event.unwrap())));
+				},


Could this parallelism affect how users structure their database storage for intercepted OMs? Just wondering if some extra docs could be helpful.

Hmm, not sure why it would matter? DBs should generally have no issue with parallel insertions, no matter the structure really.

valentinewallace

LGTM pending a second reviewer.

jkczyz · 2024-06-03T16:59:03Z

lightning/src/onion_message/messenger.rs

+		// We process events in parallel, but we want to complete `OnionMessageIntercepted` events
+		// prior to `OnionMessagePeerConnected` ones.


Why not have OnionMessenger store the events in two separate Vecs? Seems like the code would be a lot simpler.

Uhhh, right, duh, that would be simpler.

In the next commit, `OnionMessenger` events are handled in parallel using rust async. When we do that, we'll want to handle `OnionMessageIntercepted` events prior to `OnionMessagePeerConnected` ones. While we'd generally prefer to handle all events in the order they were generated, if we want to handle them in parallel, we don't want a `OnionMessageIntercepted` event to start being processed, then handle an `OnionMessagePeerConnected` prior to the first completing. This could cause us to store a freshly-intercepted message for a peer in a DB that was just wiped because the peer is now connected. This does run the risk of processing a `OnionMessagePeerConnected` event prior to an `OnionMessageIntercepted` event (because a peer connected, then disconnected, then we received a message for that peer all before any events were handled), that is somewhat less likely and discarding a message in a rare race is better than leaving a message lying around undelivered. Thus, here, we store `OnionMessenger` events in separate `Vec`s which we can pull from in message-type-order.

This adds an `OnionMessenger::process_pending_events_async` mirroring the same in `ChannelManager`. However, unlike the one in `ChannelManager`, this processes the events in parallel by spawning all futures and using the new `MultiFuturePoller`. Because `OnionMessenger` just generates a stream of messages to store/fetch, we first process all the events to store new messages, `await` them, then process all the events to fetch stored messages, ensuring reordering shouldn't result in lost messages (unless we race with a peer disconnection, which could happen anyway).

When `OnionMessenger` first developed a timer and events interface, we accessed the `OnionMessenger` indirectly via the `PeerManager`. While this is a fairly awkward interface, it avoided a large pile of generics on the background processor interfaces. However, since we now have an `AOnionMessenger` trait, this concern is no longer significant. Further, because we now want to use the built-in `OnionMessenger` async event processing method, we really need a direct referene to the `OnionMessenger` in the background processor, which we add here optionally.

This never really made a lot of sense from an API perspective, but was required to avoid handing the background processor an explicit `OnionMessegner`, which we are now doing. Thus, we can simply drop these bounds as unnecessary.

jkczyz

LGTM

lightning-background-processor/src/lib.rs

TheBlueMatt · 2024-06-03T20:33:41Z

Addressed @jkczyz's point and squashed:

$ git diff-tree -U1 fadb26875 21aebd2d7
diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs
index 6b7cd5b05..a17f514a5 100644
--- a/lightning-background-processor/src/lib.rs
+++ b/lightning-background-processor/src/lib.rs
@@ -57,5 +57,2 @@ use std::time::Instant;

-#[cfg(not(feature = "std"))]
-use alloc::boxed::Box;
-
 /// `BackgroundProcessor` takes care of tasks that (1) need to happen periodically to keep
@@ -696,3 +693,3 @@ where
 		let fetch_time = &fetch_time;
-		Box::pin(async move { // We should be able to drop the Box once our MSRV is 1.68
+		async move { // We should be able to drop the Box once our MSRV is 1.68
 			if let Some(network_graph) = network_graph {
@@ -711,3 +708,3 @@ where
 			event_handler(event).await;
-		})
+		}
 	};

valentinewallace · 2024-06-03T21:11:45Z

LGTM after the comment about Box is removed.

TheBlueMatt · 2024-06-03T21:26:04Z

🤦

$ git diff-tree -U1 21aebd2d eedceeb3
diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs
index a17f514a5..a5d56c459 100644
--- a/lightning-background-processor/src/lib.rs
+++ b/lightning-background-processor/src/lib.rs
@@ -693,3 +693,3 @@ where
 		let fetch_time = &fetch_time;
-		async move { // We should be able to drop the Box once our MSRV is 1.68
+		async move {
 			if let Some(network_graph) = network_graph {

tnull · 2024-06-04T07:38:55Z

Unfortunately tests are still failing due to !Unpin issues.

TheBlueMatt · 2024-06-04T12:28:01Z

Doh! That's why the Box was there...

TheBlueMatt · 2024-06-04T13:06:44Z

Reverted to include the Box again.

TheBlueMatt · 2024-06-04T19:14:19Z

lol ffs merged without noticing there were still fixup commits...oh well, they were just doc fixes so not really a big deal.

TheBlueMatt added 2 commits May 10, 2024 21:29

Create an AOnionMessenger trait similar to our other AStructs

20f2dab

This allows us to have a bound on any `OnionMessenger` without having to create bounds for every bound in `OnionMessenger`.

TheBlueMatt force-pushed the 2024-05-parallel-async-om-events branch 2 times, most recently from e5106f9 to a915c6f Compare May 10, 2024 22:54

TheBlueMatt force-pushed the 2024-05-parallel-async-om-events branch 2 times, most recently from 62317c8 to 15d1135 Compare May 13, 2024 19:00

valentinewallace reviewed May 20, 2024

View reviewed changes

f docs

c1ea761

TheBlueMatt force-pushed the 2024-05-parallel-async-om-events branch from 15d1135 to 8e6dcaa Compare May 28, 2024 15:26

valentinewallace reviewed May 28, 2024

View reviewed changes

TheBlueMatt added the Seeking Code Review label May 30, 2024

jkczyz self-requested a review June 3, 2024 16:36

jkczyz reviewed Jun 3, 2024

View reviewed changes

TheBlueMatt added 5 commits June 3, 2024 18:28

f fix imports in BP

f96fbdd

Drop EventsProvider bounds on OnionMessageHandlers

fadb268

This never really made a lot of sense from an API perspective, but was required to avoid handing the background processor an explicit `OnionMessegner`, which we are now doing. Thus, we can simply drop these bounds as unnecessary.

TheBlueMatt force-pushed the 2024-05-parallel-async-om-events branch from 8e6dcaa to fadb268 Compare June 3, 2024 18:51

jkczyz previously approved these changes Jun 3, 2024

View reviewed changes

lightning-background-processor/src/lib.rs Show resolved Hide resolved

TheBlueMatt dismissed jkczyz’s stale review via 21aebd2 June 3, 2024 20:31

TheBlueMatt force-pushed the 2024-05-parallel-async-om-events branch from fadb268 to 21aebd2 Compare June 3, 2024 20:31

TheBlueMatt force-pushed the 2024-05-parallel-async-om-events branch from 21aebd2 to eedceeb Compare June 3, 2024 21:25

valentinewallace previously approved these changes Jun 3, 2024

View reviewed changes

jkczyz previously approved these changes Jun 3, 2024

View reviewed changes

TheBlueMatt dismissed stale reviews from jkczyz and valentinewallace via fadb268 June 4, 2024 13:06

TheBlueMatt force-pushed the 2024-05-parallel-async-om-events branch from eedceeb to fadb268 Compare June 4, 2024 13:06

jkczyz approved these changes Jun 4, 2024

View reviewed changes

valentinewallace approved these changes Jun 4, 2024

View reviewed changes

TheBlueMatt merged commit 3cd1cb5 into lightningdevkit:main Jun 4, 2024
25 of 32 checks passed

tnull mentioned this pull request Jun 5, 2024

Make event handling fallible #2995

Merged

1 task

valentinewallace removed the Seeking Code Review label Jun 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a parallel async event handler to OnionMessenger and pass it directly to BackgroundProcessor #3060

Add a parallel async event handler to OnionMessenger and pass it directly to BackgroundProcessor #3060

TheBlueMatt commented May 10, 2024

codecov-commenter commented May 10, 2024 •

edited

Loading

valentinewallace commented May 20, 2024

valentinewallace May 20, 2024

TheBlueMatt May 28, 2024

valentinewallace May 28, 2024

valentinewallace May 20, 2024

TheBlueMatt May 28, 2024

valentinewallace left a comment

jkczyz Jun 3, 2024

TheBlueMatt Jun 3, 2024

jkczyz left a comment

TheBlueMatt commented Jun 3, 2024

valentinewallace commented Jun 3, 2024

TheBlueMatt commented Jun 3, 2024

tnull commented Jun 4, 2024

TheBlueMatt commented Jun 4, 2024

TheBlueMatt commented Jun 4, 2024

TheBlueMatt commented Jun 4, 2024

		// We process events in parallel, but we want to complete `OnionMessageIntercepted` events
		// prior to `OnionMessagePeerConnected` ones.

Add a parallel async event handler to OnionMessenger and pass it directly to BackgroundProcessor #3060

Add a parallel async event handler to OnionMessenger and pass it directly to BackgroundProcessor #3060

Conversation

TheBlueMatt commented May 10, 2024

codecov-commenter commented May 10, 2024 • edited Loading

Codecov Report

valentinewallace commented May 20, 2024

valentinewallace May 20, 2024

Choose a reason for hiding this comment

TheBlueMatt May 28, 2024

Choose a reason for hiding this comment

valentinewallace May 28, 2024

Choose a reason for hiding this comment

valentinewallace May 20, 2024

Choose a reason for hiding this comment

TheBlueMatt May 28, 2024

Choose a reason for hiding this comment

valentinewallace left a comment

Choose a reason for hiding this comment

jkczyz Jun 3, 2024

Choose a reason for hiding this comment

TheBlueMatt Jun 3, 2024

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Jun 3, 2024

valentinewallace commented Jun 3, 2024

TheBlueMatt commented Jun 3, 2024

tnull commented Jun 4, 2024

TheBlueMatt commented Jun 4, 2024

TheBlueMatt commented Jun 4, 2024

TheBlueMatt commented Jun 4, 2024

codecov-commenter commented May 10, 2024 •

edited

Loading