Initial event handling implementation #10

tnull · 2022-09-01T07:17:48Z

~~Based on #8~~. Dependent on #9.

In this PR, we provide the event handling for LDK events and implement the structures and logic for emitting LdkLiteEvents.

tnull · 2022-09-19T16:53:54Z

Rebased on main.

wpaulino

Didn't review the event handling itself, will need to look at some of the dependent PRs for more context. The PR description should be updated to note what this currently depends on.

src/event.rs

tnull · 2022-09-27T08:11:54Z

The PR description should be updated to note what this currently depends on.

Hum, this doesn't really depend on anything now that #8 has been merged. However, essentially everything after (as in "issue number is higher") is dependent on this PR.

wpaulino · 2022-09-27T17:21:07Z

Hum, this doesn't really depend on anything now that #8 has been merged. However, essentially everything after (as in "issue number is higher") is dependent on this PR.

Some of the components used within event handling don't exist in main yet, e.g., LdkLiteChainAccess.

tnull · 2022-09-27T17:46:48Z

Some of the components used within event handling don't exist in main yet, e.g., LdkLiteChainAccess.

Ah, right, missed that one. Will update the description.

src/event.rs

TheBlueMatt · 2022-10-17T19:52:55Z

src/event.rs

+
+impl Writeable for PaymentSuccessfulEvent {
+	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), lightning::io::Error> {
+		self.payment_hash.write(writer)?;


We need some kind of upgradability story for all the events. IMO just do TLV-encoding for everything from day one, its simpler (but a bit slower...I really wish TLV encoding had a fixed-length type/length).

So, IIUC, we'd follow the same approach as currently (as we can't yet make use of the macros), but just also write out the length prefix for each event?

Right, we could also expose the macro upstream? But, I mean, exact compat semantics is kinda up to you. #10 (comment)

src/event.rs

TheBlueMatt · 2022-10-17T19:55:38Z

src/event.rs

+		locked_queue.0.front().unwrap().clone()
+	}
+
+	pub(crate) fn event_handled(&self) -> Result<(), Error> {


What if events are handled async? Dont we need some kind of EventId here?

Mh, I though we had discussed that through this (blocking) interface we want to force users to handle events sequentially, i.e., allow them to only make progress once they handled an event? IIUC, async event handling would mean we'd keep them in a persisted hash map and allow to query for a list of all unhandled events?

Hmm, fair enough. Shouldn't that mean that next_event shouldn't return a new event unless all pending events have been handled?

Yes. It would currently always return the front of the queue until it is popped in event_handled.

Ah, hmm, that's a confusing API IMO. I guess it depends on how this is exposed publicly, but needs good docs.

Yes, def. need to document that behavior nicely. However, if the app crashes before the event is handled and we don't return the event again, we'd need yet another method to allow the user to look it up. I therefore think just returning the same event until handled is probably the most straightforward approach towards ensuring the user has to handle all events and can recover/resume after a failure.

Tests are a nice way to document. 🙂 Could you add some either here or possibly as doc tests, if that makes sense.

Now added a unit test here for the EventQueue with 1566dcb. (Also rebased #11 on top of this, so you can have a look at https://github.com/lightningdevkit/ldk-lite/actions/runs/3413412794/jobs/5680088292 to check the CI run)

As the documentation of the top-level API will be in lib.rs I took a TODO to possibly add a doctest/example there.

TheBlueMatt · 2022-10-17T19:57:50Z

src/event.rs

+				let forwarding_channel_manager = self.channel_manager.clone();
+				let min = time_forwardable.as_millis() as u64;
+
+				// TODO: any way we still can use tokio here?


Yes, please. Tokio has a nice simple timer interface to use here. I guess its not clear to me how these types are used so its less clear to me if its doable, but I assume we'll have a tokio runtime we can pass into the handler struct.

Yup, will address this in an upcoming "make everything async" PR in which I'll also switch the calls to BDK over to async. Should be noted thought that this is not entirely trivial since the tokio runtime won't be available when we initialize the event handler, so we'll have to pass it around after the fact, i.e., when start() gets called and the runtime is setup.

And here's the upcoming PR: #26

tnull · 2022-10-20T09:38:39Z

So, turns out that UniFFI doesn't support tuple structs in enum variants in an easy manner. Rather than writing a lot of boilerplate code for bindings, I now reverted fdef833 with 0a0d8b3.

Let me know if history starts getting confusing here, happy to squash then.

TheBlueMatt · 2022-11-01T00:24:56Z

src/event.rs

+			//6u8 => {
+			// TODO OnChainPaymentReceived
+			//}
+			_ => Err(lightning::ln::msgs::DecodeError::InvalidValue),


Do you want to do the thing LDK/lightning does and have the ability to ignore some types for forwards compatibility?

Actually, I'm currently reconsidering whether I want to re-use upstream serialization at all. I currently considering introducing a new abstraction in which every field/value would need to have explicit TYPE/SERIALIZATION_TYPE/VERSION: u8 given and to also enforce that converting from one VERSION to another has to be explicitly implemented. I hope this way I can escape the mess that is rust-lightning's serialization logic, where we have to remember any caveats for backwards/forwards compat at all times when introducing changes. Any thoughts on that?

I dunno, all options are terrible. Some old rust-lightning structs still have a version:min_version two-byte prefix which goes "the version I am is X, the minimum version that can read this is Y", but that still requires the same level of awareness from devs. You could go the "no forwards-compatibility at all" route, which of course simplifies everything a ton, but then users can't run two copies of LDK on different devices and upgrade them out of lock-step, but maybe that's okay.

src/event.rs

TheBlueMatt · 2022-11-01T00:27:39Z

src/event.rs

+		locked_queue.0.front().unwrap().clone()
+	}
+
+	pub(crate) fn event_handled(&self) -> Result<(), Error> {


Hmm, fair enough. Shouldn't that mean that next_event shouldn't return a new event unless all pending events have been handled?

src/event.rs

tnull · 2022-11-07T11:52:45Z

If you want to land this, feel free to squash (with some TODO: figure out serialization more concretely - #30 notes) and I'll give another once-over.

Alright, would be great to keep moving forward to escape rebase-hell. 😅

Squashed commits now:

$ git diff-tree -U2 cc83be6 799df1e
diff --git a/src/event.rs b/src/event.rs
index c8b807b..468af37 100644
--- a/src/event.rs
+++ b/src/event.rs
@@ -73,4 +73,5 @@ pub enum LdkLiteEvent {
 }

+// TODO: Figure out serialization more concretely - see issue #30
 impl Readable for LdkLiteEvent {
        fn read<R: lightning::io::Read>(

src/event.rs

jkczyz · 2022-11-17T04:21:49Z

src/event.rs

+	//}
+}
+
+// TODO: Figure out serialization more concretely - see issue #30


Also consider using impl_writeable_tlv_based_enum for enums.

src/event.rs

jkczyz · 2022-11-17T04:30:07Z

src/event.rs

+			.notifier
+			.wait_while(self.queue.lock().unwrap(), |queue| queue.0.is_empty())
+			.unwrap();
+		locked_queue.0.front().unwrap().clone()


Should we store / return an Arc to avoid cloning?

Done with a45eb9e.

Now reverted this with 3b6ded0 for two reasons: a) I find using a plain Event preferable in the interface but more importantly b) turns out that UniFFI (see #25) doesn't support Arc'ed enum return values for the time being. However, it will itself wrap the Event in an Arc for the exposed interface, but sadly can't re-use the Arc in the case of an enum.

jkczyz · 2022-11-17T04:31:55Z

src/event.rs

+	}
+}
+
+impl<K: KVStorePersister> ReadableArgs<Arc<K>> for LdkLiteEventQueue<K> {


We'll want a Writeable implementation, too.

Why do we need Writeable for EventQueue? Readable/Writeable are already implemented for EventQueueSerWrapper and the ReadableArgs implementation of EventQueue is merely a short form of reading the queue separately an passing it to something like EventQueue::new, which allows us to never expose EventQueueSerWrapper.

src/event.rs

jkczyz · 2022-11-21T19:51:46Z

src/event.rs

+	pub(crate) fn next_event(&self) -> Arc<LdkLiteEvent> {
+		let locked_queue = self
+			.notifier
+			.wait_while(self.queue.lock().unwrap(), |queue| queue.is_empty())
+			.unwrap();
+		Arc::clone(&locked_queue.front().unwrap())
+	}


Curious how this would work with shutdown. If a user has a loop calling next_event, do they need to wait for an event comes through to exit the loop? Would using wait_timeout_while be preferable?

Hm, yeah, that is a good question regarding how we imagine the overall API to be used (i.e., if we expect the user side to entirely run single-threaded or if there would be a background process blocking on this). I'm not the biggest fan of timeouts, but maybe we should expose multiple versions of this via the main API? One blocking, one blocking for a given timeout, and one returning a Future?

tnull · 2022-11-30T12:37:53Z

Alright, after I had to make #11 async-based already, this now also includes the necessary changes to event.rs. Had to squash fixups in the process.

jkczyz

Looks like the commits are a little mixed up. Might as well squash them.

src/event.rs

tnull · 2022-12-12T13:36:01Z

Looks like the commits are a little mixed up. Might as well squash them.

Squashed and addressed latest feedback.

jkczyz

Pretty much LGTM if you want to squash

jkczyz · 2023-01-06T04:45:55Z

src/event.rs

+				if locked_runtime.as_ref().is_none() {
+					return;
+				}


Yeah, likely. Tagging @TheBlueMatt to confirm.

src/event.rs

tnull · 2023-01-06T09:13:04Z

Pretty much LGTM if you want to squash

Squashed fixups without further changes.

src/event.rs

This was referenced Sep 1, 2022

Initial implementation of LDK Node #11

Merged

Initial payment store implementation #13

Merged

Make wallet entropy source configurable #14

Closed

tnull force-pushed the 2022-09-initial-event-handling branch 3 times, most recently from fb52b66 to 562ae2a Compare September 8, 2022 10:38

tnull mentioned this pull request Sep 9, 2022

Implement chain::Access trait #21

Closed

tnull force-pushed the 2022-09-initial-event-handling branch 4 times, most recently from 4815eba to 3c4db1b Compare September 14, 2022 16:52

tnull force-pushed the 2022-09-initial-event-handling branch from 3c4db1b to 77940d9 Compare September 19, 2022 16:51

wpaulino reviewed Sep 26, 2022

View reviewed changes

src/event.rs Outdated Show resolved Hide resolved

src/event.rs Show resolved Hide resolved

src/event.rs Outdated Show resolved Hide resolved

src/event.rs Outdated Show resolved Hide resolved

tnull force-pushed the 2022-09-initial-event-handling branch from 8312428 to fdef833 Compare September 30, 2022 08:25

jkczyz reviewed Oct 4, 2022

View reviewed changes

src/event.rs Outdated Show resolved Hide resolved

src/event.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Oct 5, 2022

View reviewed changes

src/event.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Oct 17, 2022

View reviewed changes

tnull force-pushed the 2022-09-initial-event-handling branch 2 times, most recently from a9fd684 to a4922da Compare October 20, 2022 06:54

This was referenced Oct 20, 2022

Make everything async #26

Closed

UniFFI bindings #25

Merged

TheBlueMatt reviewed Nov 1, 2022

View reviewed changes

tnull force-pushed the 2022-09-initial-event-handling branch 3 times, most recently from 3c705d4 to a834254 Compare November 1, 2022 14:21

tnull force-pushed the 2022-09-initial-event-handling branch from cc83be6 to 799df1e Compare November 7, 2022 11:51

tnull force-pushed the 2022-09-initial-event-handling branch from aff85ae to 1566dcb Compare November 7, 2022 19:21

TheBlueMatt approved these changes Nov 15, 2022

View reviewed changes

src/event.rs Outdated Show resolved Hide resolved

src/event.rs Show resolved Hide resolved

tnull requested review from wpaulino and jkczyz November 16, 2022 09:05

jkczyz reviewed Nov 17, 2022

View reviewed changes

jkczyz reviewed Nov 21, 2022

View reviewed changes

tnull force-pushed the 2022-09-initial-event-handling branch from c2ae588 to 9d6324b Compare November 30, 2022 12:34

jkczyz reviewed Dec 7, 2022

View reviewed changes

tnull force-pushed the 2022-09-initial-event-handling branch from 1ff2855 to 417eb55 Compare December 12, 2022 13:35

tnull requested review from jkczyz and TheBlueMatt January 4, 2023 12:51

jkczyz reviewed Jan 6, 2023

View reviewed changes

TheBlueMatt reviewed Jan 6, 2023

View reviewed changes

src/event.rs Outdated Show resolved Hide resolved

src/event.rs Show resolved Hide resolved

src/event.rs Show resolved Hide resolved

tnull force-pushed the 2022-09-initial-event-handling branch from 418dc62 to e0bfb64 Compare January 6, 2023 09:12

Add initial event handling structures and logic

e775a6b

tnull force-pushed the 2022-09-initial-event-handling branch from b2f3872 to e775a6b Compare January 9, 2023 09:50

jkczyz reviewed Jan 11, 2023

View reviewed changes

src/event.rs Show resolved Hide resolved

src/event.rs Outdated Show resolved Hide resolved

jkczyz mentioned this pull request Jan 12, 2023

Initial implementation of wallet functionality #33

Merged

tnull added 2 commits January 16, 2023 07:07

f Remove {drop, set}_runtime()

2141ba6

f Event clippy lints

4e46299

tnull requested a review from jkczyz January 16, 2023 14:01

f Don't call create_funding_transaction by ref

88a2c78

jkczyz approved these changes Jan 17, 2023

View reviewed changes

tnull merged commit 053a5ed into lightningdevkit:main Jan 17, 2023

tnull mentioned this pull request May 24, 2024

Add BOLT12 support #256

Merged

Initial event handling implementation #10

Initial event handling implementation #10

Conversation

tnull commented Sep 1, 2022 • edited Loading

tnull commented Sep 19, 2022

wpaulino left a comment

Choose a reason for hiding this comment

tnull commented Sep 27, 2022

wpaulino commented Sep 27, 2022

tnull commented Sep 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Nov 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented Nov 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

tnull commented Nov 30, 2022

jkczyz left a comment • edited Loading

Choose a reason for hiding this comment

tnull commented Dec 12, 2022

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented Jan 6, 2023 • edited Loading

tnull commented Sep 1, 2022 •

edited

Loading

tnull commented Sep 27, 2022 •

edited

Loading

tnull Oct 19, 2022 •

edited

Loading

TheBlueMatt Nov 2, 2022 •

edited

Loading

tnull Nov 7, 2022 •

edited

Loading

tnull commented Oct 20, 2022 •

edited

Loading

tnull commented Nov 7, 2022 •

edited

Loading

tnull Dec 13, 2022 •

edited

Loading

tnull Nov 17, 2022 •

edited

Loading

tnull Nov 22, 2022 •

edited

Loading

jkczyz left a comment •

edited

Loading

tnull commented Jan 6, 2023 •

edited

Loading