feat(lightning): Add Lightning as transport #85

mariocynicys · 2022-08-02T20:53:34Z

This PR introduces bolt13 message structs and their lightning (de)serialization, to be used by the tower (for the lightning server) and client apps.

LDK's (de)serialization traits are used to make the message structs sendable over wire (Readable and Writeable).

A small customized macro-based serialization framework similar to LDK's was added to help organize the code and reduce its size.

Will push the lightning server to this PR later on.

Addresses #31

mariocynicys · 2022-08-03T06:30:12Z

teos-common/src/lightning/ser_macros.rs

+                // Initialize the fields we will read from. An unused_assignment happens here.
+                $(init_tlv_field_var!($field_name, $fieldty);)*
+                $($field_name = $field;)*


This can be replaced with

$(let $field_name = $field;)*

and remove #[allow(unused_assignments)].

mariocynicys · 2022-08-03T06:32:26Z

teos-common/src/lightning/ser_macros.rs

+        let v = vec![1_u8, 2, 3, 6];
+        test_encode_decode_tlv!(1, v.len(), v, vec);


Test other vector types (e.g. Vec<u16>)

mariocynicys · 2022-08-03T06:34:23Z

teos-common/src/lightning/ser_macros.rs

+            // Allow unreachable patterns which happen when we have some non-unique tlv types.
+            #[allow(unreachable_patterns)]
+            // Runs `test_encode_decode_tlv_stream` inside a closure so that we don't need to
+            // return a `Result<(), DecodeError` from the test functions.


missing >

mariocynicys · 2022-08-03T06:40:25Z

teos-common/src/lightning/ser_utils.rs

+        writer.write_all(&self.to_vec())?;
+        Ok(())


Simplify to

writer.write_all(&self.to_vec())

mariocynicys · 2022-08-03T18:42:32Z

teos-common/src/lightning/messages.rs

+    fn test_msg<T: Debug + Readable + Writeable + PartialEq>(msg: T) {
+        // Get a writer and write the message to it.
+        let mut stream = TestVecWriter(Vec::new());
+        msg.write(&mut stream).ok().unwrap();


drop .ok().

mariocynicys · 2022-08-03T18:43:23Z

teos-common/src/lightning/messages.rs

+        msg.write(&mut stream).ok().unwrap();
+        // Create a reader out of the written buffer.
+        let mut stream = Cursor::new(stream.0);
+        let read_msg: T = Readable::read(&mut stream).ok().unwrap();


drop .ok().

mariocynicys · 2022-08-04T06:58:22Z

teos-common/src/lightning/ser_macros.rs

+*/
+
+macro_rules! encode_tlv {
+    ($stream: expr, $type: expr, $field: expr, opt) => {


In std and other crates, they leave no space between the macro variable and its type: $stream:expr.

mariocynicys · 2022-08-06T05:58:58Z

teos-common/src/lightning/ser_utils.rs

+    /// Consume the remaining bytes.
+    #[inline]
+    pub fn eat_remaining(&mut self) -> Result<(), DecodeError> {
+        copy(self, &mut sink()).unwrap();


Maybe tolerate this unwrap here and map to a decode error instead.

sr-gi · 2022-09-17T10:52:38Z

Sorry I haven't taken the time to review this yet. Will do as soon as v0.1.2 gets released.

sr-gi

First pass. Not super familiar with Rust macros, so more like an Concept / Approach ACK than a thorough review.

sr-gi · 2022-09-21T14:24:54Z

teos-common/src/lightning/ser_utils.rs

+    use crate::cryptography::get_random_bytes;
+    use bitcoin::hashes::Hash;
+    use bitcoin::Txid;
+    pub use lightning::util::test_utils::TestVecWriter;


This is imported here but not use here. I'm guessing this is because you're grouping it as part of the serialization tools and then re-importing it in messages.rs and ser_macros.rs.

Not against it but just wanted to verify that's the approach.

I'm guessing this is because you're grouping it as part of the serialization tools and then re-importing it in messages.rs and ser_macros.rs

Yup.

sr-gi · 2022-09-21T14:26:00Z

teos-common/src/lightning/messages.rs

+
+use crate::appointment::Locator;
+use crate::lightning::ser_macros::{impl_writeable_msg, set_msg_type};
+use bitcoin::secp256k1::PublicKey;


nit: line break to separate crate stuff from btc/ln stuff

sr-gi · 2022-09-21T14:33:37Z

teos-common/src/lightning/messages.rs

+/// The register message sent by the user to subscribe for the watching service.
+#[derive(Debug)]
+pub struct Register {
+    pub pubkey: PublicKey,


This can be user_id: UserId

sr-gi · 2022-09-21T14:36:02Z

teos-common/src/lightning/messages.rs

+pub struct SubscriptionDetails {
+    pub appointment_max_size: u16,
+    pub start_block: u32,
+    pub amount_msat: u32,


This won't be necessary. Payment methods can go in the TLV fields (as happens with the invoice), so we can assume that the service is altruistic if there is no amount_msat. In case there is, then this value would be redundant.

sr-gi · 2022-09-21T14:38:29Z

teos-common/src/lightning/messages.rs

+#[derive(Debug)]
+pub struct AddUpdateAppointment {
+    pub locator: Locator,
+    // NOTE: LDK will prefix varying size fields (e.g. vectors and strings) with their length.


Yeah, the length fields in BOLT13 are so it can be implemented in different langs properly. Don't thing this annotation is neeced.

sr-gi · 2022-09-21T14:52:09Z

teos-common/src/lightning/messages.rs

+/// The appointment data message sent by the tower after a get appointment message.
+#[derive(Debug)]
+pub struct AppointmentData {
+    pub locator: Locator,
+    pub encrypted_blob: Vec<u8>,
+}
+
+/// The tracker data message sent by the tower when the requested appointment has been acted upon.
+#[derive(Debug)]
+pub struct TrackerData {
+    pub dispute_txid: Txid,
+    pub penalty_txid: Txid,
+    pub penalty_rawtx: Vec<u8>,
+}


BOLT-wise this should be two different messages right? Have you put any thought on how this would look like?

BOLT-wise this should be two different messages right?

Indeed they are.

Have you put any thought on how this would look like?

Do you mean in terms of return types of methods and the typing stuff in Rust?
They are both TowerMessage at the end.

sr-gi · 2022-09-21T18:05:04Z

teos-common/src/lightning/ser_utils.rs

+        writer.write_all(&self.to_vec())?;
+        Ok(())


sr-gi

Second pass. This should be more thorough. Haven't checked test yet.

ser_macros.rs is based on lightning::util::ser_macros.rs but it significantly deviates from it. Is there any good reasoning behind it? Has lightning::util::ser_macros.rs been updated since you wrote this?

As a general question, I saw you had some discussion regarding some of the serialization functionality being exposed in lightning. Is that still on track? What parts of this would be exported and how would that affect this part of the codebase?

sr-gi · 2022-09-22T12:27:06Z

teos-common/src/lightning/ser_utils.rs

+                Ok(None) => {}
+                // If we failed to read any bytes at all, we reached the end of our TLV
+                // stream and have simply exhausted all entries.
+                Err(e) if e == DecodeError::ShortRead && !track_read.have_read => break,


Suggested change

Err(e) if e == DecodeError::ShortRead && !track_read.have_read => break,

Err(ref e) if e == &DecodeError::ShortRead && !track_read.have_read => break,

Is this more ergonomic or it has performance impact?

It implies one less allocation AFAICT

sr-gi · 2022-09-22T12:31:21Z

teos-common/src/lightning/ser_utils.rs

+impl<'a, T: Writeable> Writeable for LightningVecWriter<'a, T> {
+    #[inline]
+    fn write<W: Writer>(&self, writer: &mut W) -> Result<(), Error> {
+        for item in self.0.iter() {


Suggested change

for item in self.0.iter() {

for ref item in self.0.iter() {

And this as well?

Well, iter actually returns an iterator of references over the original data.
So no extra allocations on this one.

sr-gi · 2022-09-22T12:59:23Z

teos-common/src/lightning/ser_utils.rs

+        let locator = Self::from_slice(&buf).map_err(|_| DecodeError::InvalidValue)?;
+        Ok(locator)


Suggested change

let locator = Self::from_slice(&buf).map_err(|_| DecodeError::InvalidValue)?;

Ok(locator)

Self::from_slice(&buf).map_err(|_| DecodeError::InvalidValue)

sr-gi · 2022-09-22T13:17:41Z

teos-common/src/lightning/ser_macros.rs

+            }
+        }
+
+        #[cfg(test)]


Any reason why this is test only? Also, why not derive this for the messages? Is it so you can test it generally within ser_macros.rs?

I didn't need to impl it at the beginning for usual functionality, and saw LDK also doesn't implement it for their messages. But then I needed it for tests, that's why I made it test only.

ser_macros wouldn't use it because it tests primitive types which already implement Eq. Generally, tests in ser_macros don't test impl_writeable_msg but test the macros used inside of it.

sr-gi · 2022-09-22T13:25:03Z

teos-common/src/lightning/messages.rs

+set_msg_type!(Register, 48848);
+set_msg_type!(SubscriptionDetails, 48850);
+set_msg_type!(AddUpdateAppointment, 48852);
+set_msg_type!(AppointmentAccepted, 48854);
+set_msg_type!(AppointmentRejected, 48856);
+set_msg_type!(GetAppointment, 48858);
+set_msg_type!(AppointmentData, 48860);
+set_msg_type!(TrackerData, 48862);
+set_msg_type!(AppointmentNotFound, 48864);
+// Let these messages get odd types since they are auxiliary messages.
+set_msg_type!(GetSubscriptionInfo, 48865);
+set_msg_type!(SubscriptionInfo, 48867);


Is there any reasoning behind the values taken for the message types? (apart from them being part of the custom message range and being even/odd depending on whether they are required or not)?

Not specifically, just random numbers as placeholders.

sr-gi · 2022-09-22T13:27:26Z

teos-common/src/lightning/ser_utils.rs

+pub trait Type {
+    /// The type identifying the message payload.
+    const TYPE: u16;
+}


Does this mas to be a trait? How does LDK implement this for the standard messages?

Might work as something else, but I just followed LDK on this one. They have that trait Encode which does the some thing. Encode is a little bit of a weird name, but I think they named it that way because they have another trait already called Type.

mariocynicys · 2022-09-23T17:49:21Z

@sr-gi

ser_macros.rs is based on lightning::util::ser_macros.rs but it significantly deviates from it. Is there any good reasoning behind it? Has lightning::util::ser_macros.rs been updated since you wrote this?

I mainly wanted impl_writeable_msg! to be exposed (and of course any macros called inside of it) because it handles all the TLV stuff, but that wasn't what LDK team wanted. They wanted to expose more of the utility macros instead (e.g. encode_tlv_stream!) which then the users could use to craft their things.

Not a big deal too. Exposing the utility macros would do fine. But after going on with BOLT13, some TLV datatypes there weren't covered by LDK's macro system (like vectors and strings) as they don't use it internally nor that it is used in other BOLTs, which led me to make some tweaks to my copy till it deviated that much.

You can also find other parts trimmed out (like check_tlv_order!, check_missing_tlv!, and default macro arm option), this is mainly because these parts weren't used in BOLT13 (and I think not even in other BOLTs, I think they mainly use it for internal messaging stuff).

As a general question, I saw you had some discussion regarding some of the serialization functionality being exposed in lightning. Is that still on track? What parts of this would be exported and how would that affect this part of the codebase?

Exposing them wouldn't help much now, because LDK's macros don't support strings and vector in TLVs, which we would like to have being supported.

sr-gi · 2022-09-24T14:44:59Z

@sr-gi

ser_macros.rs is based on lightning::util::ser_macros.rs but it significantly deviates from it. Is there any good reasoning behind it? Has lightning::util::ser_macros.rs been updated since you wrote this?

I mainly wanted impl_writeable_msg! to be exposed (and of course any macros called inside of it) because it handles all the TLV stuff, but that wasn't what LDK team wanted. They wanted to expose more of the utility macros instead (e.g. encode_tlv_stream!) which then the users could use to craft their things.

Not a big deal too. Exposing the utility macros would do fine. But after going on with BOLT13, some TLV datatypes there weren't covered by LDK's macro system (like vectors and strings) as they don't use it internally nor that it is used in other BOLTs, which led me to make some tweaks to my copy till it deviated that much.

You can also find other parts trimmed out (like check_tlv_order!, check_missing_tlv!, and default macro arm option), this is mainly because these parts weren't used in BOLT13 (and I think not even in other BOLTs, I think they mainly use it for internal messaging stuff).

As a general question, I saw you had some discussion regarding some of the serialization functionality being exposed in lightning. Is that still on track? What parts of this would be exported and how would that affect this part of the codebase?

Exposing them wouldn't help much now, because LDK's macros don't support strings and vector in TLVs, which we would like to have being supported.

Right. I'd like to do one last pass on this before accepting it.

sr-gi · 2022-10-05T09:20:35Z

Ok, I think this looks good. Not super used to reviewing macros but if something is not correct we will notice it when using the macros to implement the LN API.

LGTM modulo making the pending changes.

mariocynicys · 2022-10-05T10:23:08Z

Not super used to reviewing macros but if something is not correct we will notice it when using the macros to implement the LN API.

Yeah macros are tough but so cool and powerful. It's a pain that it ends up that you need to test macros with other macros 😆.

LGTM modulo making the pending changes.

Will patch make them soon 👍.

mariocynicys · 2023-02-05T13:23:11Z

These next commits are implementing the server functionality/interface.

sr-gi · 2023-02-06T10:00:09Z

Looks like this needs rebasing

Add message (de)serilaization to teos-common This commit adds lightning messages and message (de)serialization using helper local modules (ser_utils, ser_macros) and LDK v0.0.108 instead of the attempted fork used before (https://github.com/meryacine/rust-lightning/tree/expose-tlv-macros) Most of the serialization macros and utils are copied from LDK's own serialization framework though. Also we might remove some of the locally implemented TLV stuff if they ever get exposed by LDK. But some of the macros have been edited and diverged from LDK's first implementation: for example, the `vec` arms for the macros were adjusted so that it works with `impl_writeable_msg` macro to send arrays as TLV (which is not the case for LDK ATM). Note that we could have had vectors(arrays) be inside `Option`s and put in a TLV, but this will waste extra two bytes for vector length annotation (we don't need the vector length annotated in the V(value) of the TLV since L(length) already embeds this information) We also now have another macro match arm `str_opt`, which indicates an optional string inside a TLV, this was created to avoid adding extra length field for strings just like `vec`.

This commit adds the lightning server and its port configuration option. The peer manager in charge of the lightning server doesn't know about routing and lightning channels (yet?). It can only responde to the user custom messages defined in `teos_common::lightning::messages`. A \`block_in_place\` is used in \`handle_tower_message\` to synchronously block on grpc calls.

These tests are analogous to tests in `http::tests_methods` and they test the tower message handler logic only (not the tower message handler when inside a peer manager)

These test utils include: - A test lightning client: this simulates a client's peer manager that is trying to connect to the tower and send/receive tower messages. For a real client, this should be part of a bigger application (say a lightning node) and the custom message handler adds the capabilities of speaking with the tower over Lightning. - Some utils to launch a Lightning server and connect one peer manager to another - A simple proof of concept test

This fixes a deadlock issue that appears with too many concurrent requtests. This solution was taken from devrandom's issue: lightningdevkit/rust-lightning #1367 Another solution that would probably work is that lightning net tokio wraps our sync calls with `tokio::task::spawn_blocking(|| sync_call()).await.unwarp()`, but not sure how would this affect performace of other users with no async code requirements. Note that peer_manager_tests now don't need to be anotated with `flavor = "multi_thread"`, that's because the runtime we block_on (inside lightning net tokio) is the artificial runtime created in `api::lightning::server` which is already "multi_thread"

mariocynicys · 2023-02-06T14:46:57Z

teos/src/api/lightning.rs

+        let tcp_stream = listener.accept().await.unwrap().0;
+        if shutdown_signal.is_triggered() {
+            return;
+        }


This actually waits until there is a new inbound connection then checks for if a shutdown signal was issued.
We need to listen for inbound and shutdown signal at the same time.

aruokhai · 2024-06-24T10:21:27Z

Any update regarding this ?

aruokhai · 2024-06-24T10:21:31Z

Any update regarding this ?

aruokhai · 2024-06-24T10:21:41Z

Any update regarding this feature ?

mariocynicys commented Aug 6, 2022

View reviewed changes

sr-gi self-assigned this Aug 8, 2022

sr-gi added feature request New feature or request Summer of Bitcoin Summer of Bitcoin 2023 projects Seeking Code Review review me pls labels Aug 8, 2022

mariocynicys requested a review from sr-gi September 17, 2022 10:28

sr-gi requested changes Sep 21, 2022

View reviewed changes

sr-gi requested changes Sep 22, 2022

View reviewed changes

mariocynicys force-pushed the lightning-msgs branch from e34bb53 to 75d05e3 Compare November 20, 2022 15:33

mariocynicys force-pushed the lightning-msgs branch 3 times, most recently from b6889bd to ddfd755 Compare February 5, 2023 19:04

mariocynicys force-pushed the lightning-msgs branch from ddfd755 to 1a2840b Compare February 6, 2023 11:59

mariocynicys and others added 10 commits February 6, 2023 14:09

tests: Add tests for serialization macros and tower messages

ceefb85

Add start_block to subscription_details

9f12f20

Reviews from mariocynicys

23b87e4

Removing extra spaces in macros' arms

195b325

Reviews from sr-gi 1

ab9699d

Reviews from sr-gi 2

19ff970

Add the lightning server message handler

3969d02

Refactor teos::api::http tests to reuse code for lightning tests

de3c7df

mariocynicys added 4 commits February 6, 2023 14:09

tests: Add a signing key getter method for Watcher

697be71

tests: Add lightning tower handler tests

1857e12

These tests are analogous to tests in `http::tests_methods` and they test the tower message handler logic only (not the tower message handler when inside a peer manager)

mariocynicys force-pushed the lightning-msgs branch from 1a2840b to 5206d34 Compare February 6, 2023 12:33

mariocynicys commented Feb 6, 2023

View reviewed changes

		let v = vec![1_u8, 2, 3, 6];
		test_encode_decode_tlv!(1, v.len(), v, vec);

	Err(e) if e == DecodeError::ShortRead && !track_read.have_read => break,
	Err(ref e) if e == &DecodeError::ShortRead && !track_read.have_read => break,

		let locator = Self::from_slice(&buf).map_err(\|_\| DecodeError::InvalidValue)?;
		Ok(locator)

	let locator = Self::from_slice(&buf).map_err(\|_\| DecodeError::InvalidValue)?;
	Ok(locator)
	Self::from_slice(&buf).map_err(\|_\| DecodeError::InvalidValue)

feat(lightning): Add Lightning as transport #85

Are you sure you want to change the base?

feat(lightning): Add Lightning as transport #85

Conversation

mariocynicys commented Aug 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sr-gi commented Sep 17, 2022

sr-gi left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sr-gi left a comment • 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariocynicys commented Sep 23, 2022

sr-gi commented Sep 24, 2022

sr-gi commented Oct 5, 2022

mariocynicys commented Oct 5, 2022

mariocynicys commented Feb 5, 2023

sr-gi commented Feb 6, 2023

Choose a reason for hiding this comment

aruokhai commented Jun 24, 2024

aruokhai commented Jun 24, 2024

aruokhai commented Jun 24, 2024

mariocynicys commented Aug 2, 2022 •

edited

Loading

sr-gi left a comment •

edited

Loading