LSPS2: JIT Channels #11

johncantrell97 · 2023-07-19T02:00:06Z

This implements LSPS2: JIT Channels on top of LSPS0 transport layer.

tnull · 2023-08-16T10:42:46Z

@johncantrell97 Do you mind rebasing this? Will give it a proper review round afterwards, let's land this initial draft soon then to have a common ground to iterate on.

johncantrell97 · 2023-08-16T18:25:16Z

@johncantrell97 Do you mind rebasing this? Will give it a proper review round afterwards, let's land this initial draft soon then to have a common ground to iterate on.

Will do shortly. Have a few changes from implementing this into Mutiny I want to get in.

src/events.rs

src/jit_channel/channel_manager.rs

tnull · 2023-08-29T13:03:41Z

src/jit_channel/channel_manager.rs

+	pending_messages: Arc<Mutex<Vec<(PublicKey, LSPSMessage)>>>,
+	pending_events: Arc<EventQueue>,
+	per_peer_state: RwLock<HashMap<PublicKey, Mutex<PeerState>>>,
+	channels_by_scid: RwLock<HashMap<u64, JITChannel>>,


I'm confused: which channels are owned here and which ones are owned by the PeerState? Why do we have two objects holding JITChannel objects?

tnull · 2023-08-29T13:08:36Z

src/jit_channel/channel_manager.rs

+		&self, counterparty_node_id: PublicKey, payment_size_msat: Option<u64>,
+		token: Option<String>, user_channel_id: u128,
+	) {
+		let channel_id = self.generate_channel_id();


This is really confusing as these are not 'regular'/BOLT channel_ids. Can we name them differently here and elsewhere to avoid confusion?

tnull · 2023-08-29T13:10:26Z

src/jit_channel/channel_manager.rs

+			token,
+		);
+
+		let mut per_peer_state = self.per_peer_state.write().unwrap();


nit: Could be a bit more readable to refer to these as outer_state_lock/inner_state_lock?

tnull · 2023-08-29T13:25:56Z

src/jit_channel/msgs.rs

+}
+
+impl RawOpeningFeeParams {
+	pub(crate) fn into_opening_fee_params(self, promise_secret: &[u8; 32]) -> OpeningFeeParams {


Mh, it's kind of weird that we do the HMAC stuff at two different places. Couldn't we DRY this up by having the HMAC calculation be a helper function and to verify just compare that with the promise we got?

src/jit_channel/msgs.rs

src/transport/msgs.rs

tnull · 2023-08-29T13:34:39Z

src/jit_channel/channel_manager.rs

+				match peer_state.pending_requests.remove(&request_id) {
+					Some(Request::Buy(buy_request)) => {
+						let mut channels_by_scid = self.channels_by_scid.write().unwrap();
+						channels_by_scid.insert(


We insert here, but never remove this channel state? How does it end up in PeerState::channels_by_id again, if this is the plan?

tnull

Nice refactor! Did a high-level pass.

tnull · 2023-09-28T12:48:55Z

src/jit_channel/channel_manager.rs

 }

-struct JITChannel {
+impl JITChannelState {
+	fn versions_received(&self, versions: Vec<u16>) -> Self {


Rather than having a dedicated failed state, should we have these state transitions methods return Result<Self, JITChannelError> and bubble up handleable/loggable errors? Also/alternatively, we could enforce that the most relevant state values are moved by taking self.

tnull · 2023-09-28T12:52:34Z

src/jit_channel/channel_manager.rs

@@ -43,17 +40,236 @@ use super::msgs::{

 const SUPPORTED_SPEC_VERSION: u16 = 1;

-#[derive(PartialEq)]
+#[derive(PartialEq, Debug)]
 enum JITChannelState {


As the other one is Outbound, we should rename this InboundJITChannelState

tnull · 2023-09-28T12:54:43Z

src/jit_channel/channel_manager.rs

@@ -62,67 +278,111 @@ struct JITChannel {
 	min_payment_size_msat: Option<u64>,


We might want to group all the user-settable parameters into some kind of InboundJITChannelConfig object (and corresponding OutboundJITChannelConfig)?

tnull · 2023-09-28T12:56:57Z

src/jit_channel/channel_manager.rs

+		}
+	}
+
+	pub fn htlc_intercepted(


I think none of the state-transition functions should be pub?

tnull · 2023-09-28T12:58:39Z

src/jit_channel/channel_manager.rs

 }

 #[derive(Default)]
 struct PeerState {
-	channels_by_id: HashMap<u128, JITChannel>,
+	inbound_channels_by_id: HashMap<u128, InboundJITChannel>,
+	outbound_channels_by_scid: HashMap<u64, OutboundJITChannel>,


Why is this u64 and the other one u128?

tnull · 2023-09-28T12:59:10Z

src/jit_channel/channel_manager.rs

 }

 #[derive(Default)]
 struct PeerState {
-	channels_by_id: HashMap<u128, JITChannel>,
+	inbound_channels_by_id: HashMap<u128, InboundJITChannel>,
+	outbound_channels_by_scid: HashMap<u64, OutboundJITChannel>,
 	request_to_cid: HashMap<RequestId, u128>,


If we need such a map at all, don't we need one for each of inbound/outbound? Or is it just needed for inbound channels? If so, can we include inbound in the field name?

tnull · 2023-09-28T13:00:34Z

src/jit_channel/channel_manager.rs

 }

 #[derive(Default)]
 struct PeerState {
-	channels_by_id: HashMap<u128, JITChannel>,
+	inbound_channels_by_id: HashMap<u128, InboundJITChannel>,
+	outbound_channels_by_scid: HashMap<u64, OutboundJITChannel>,
 	request_to_cid: HashMap<RequestId, u128>,
 	pending_requests: HashMap<RequestId, Request>,


These are just inbound, right? Would be nice to now include inbound/outbound in such state fields to discern what they belong to.

tnull · 2023-09-28T13:04:54Z

src/jit_channel/channel_manager.rs

 	request_to_cid: HashMap<RequestId, u128>,
 	pending_requests: HashMap<RequestId, Request>,
 }

 impl PeerState {
-	pub fn insert_channel(&mut self, channel_id: u128, channel: JITChannel) {
-		self.channels_by_id.insert(channel_id, channel);
+	pub fn insert_inbound_channel(&mut self, jit_channel_id: u128, channel: InboundJITChannel) {


Maybe add_? Also, can leave out the superfluousjit_channel_id and just use channel.id which would prevent we'd ever insert a channel under the wrong id.

tnull · 2023-09-28T13:25:35Z

src/jit_channel/channel_manager.rs

 			peer_manager: Mutex::new(None),
 			channel_manager,
 		}
 	}

+	fn map_scid_to_peer(&self, scid: u64, counterparty_node_id: PublicKey) {


Not sure this helper methods with less than 5 LoC improve readability. Maybe just inline it?

tnull · 2023-10-05T18:15:36Z

Closing as superseded by #20.

johncantrell97 force-pushed the jit-channels branch 4 times, most recently from 419f445 to 31c650c Compare July 24, 2023 00:35

johncantrell97 force-pushed the jit-channels branch 2 times, most recently from 31c650c to b58f614 Compare July 26, 2023 19:22

LSPS2: JIT Channels

b9cd076

johncantrell97 force-pushed the jit-channels branch from b58f614 to b9cd076 Compare July 27, 2023 15:36

johncantrell97 added 2 commits August 24, 2023 16:10

changes to make mutiny work

45d9be8

Merge branch 'main' into jit-channels

534ea13

tnull reviewed Aug 29, 2023

View reviewed changes

tnull reviewed Sep 28, 2023

View reviewed changes

tnull mentioned this pull request Sep 28, 2023

LSPS1- Channel request #8

Merged

[wip] split inbound/outbound channels, use state transitions, cleanup

c18f353

johncantrell97 force-pushed the jit-channels branch from f8982d8 to c18f353 Compare September 29, 2023 20:42

wip

c1b8114

tnull closed this Oct 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LSPS2: JIT Channels #11

LSPS2: JIT Channels #11

johncantrell97 commented Jul 19, 2023

tnull commented Aug 16, 2023 •

edited

Loading

johncantrell97 commented Aug 16, 2023

tnull Aug 29, 2023

tnull Aug 29, 2023

tnull Aug 29, 2023

tnull Aug 29, 2023

tnull Aug 29, 2023

tnull left a comment

tnull Sep 28, 2023

tnull Sep 28, 2023

tnull Sep 28, 2023

tnull Sep 28, 2023

tnull Sep 28, 2023

tnull Sep 28, 2023

tnull Sep 28, 2023

tnull Sep 28, 2023

tnull Sep 28, 2023

tnull commented Oct 5, 2023

		@@ -62,67 +278,111 @@ struct JITChannel {
		min_payment_size_msat: Option<u64>,

LSPS2: JIT Channels #11

LSPS2: JIT Channels #11

Conversation

johncantrell97 commented Jul 19, 2023

tnull commented Aug 16, 2023 • edited Loading

johncantrell97 commented Aug 16, 2023

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

tnull 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

tnull commented Oct 5, 2023

tnull commented Aug 16, 2023 •

edited

Loading