LSPS2: JIT Channels#11
Conversation
johncantrell97
force-pushed
the
jit-channels
branch
4 times, most recently
from
419f445 to
31c650c
Compare
johncantrell97
force-pushed
the
jit-channels
branch
2 times, most recently
from
31c650c to
b58f614
Compare
johncantrell97
force-pushed
the
jit-channels
branch
from
b58f614 to
b9cd076
Compare
|
@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. |
| 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>>, |
There was a problem hiding this comment.
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?
| &self, counterparty_node_id: PublicKey, payment_size_msat: Option<u64>, | ||
| token: Option<String>, user_channel_id: u128, | ||
| ) { | ||
| let channel_id = self.generate_channel_id(); |
There was a problem hiding this comment.
This is really confusing as these are not 'regular'/BOLT channel_ids. Can we name them differently here and elsewhere to avoid confusion?
| token, | ||
| ); | ||
|
|
||
| let mut per_peer_state = self.per_peer_state.write().unwrap(); |
There was a problem hiding this comment.
nit: Could be a bit more readable to refer to these as outer_state_lock/inner_state_lock?
| } | ||
|
|
||
| impl RawOpeningFeeParams { | ||
| pub(crate) fn into_opening_fee_params(self, promise_secret: &[u8; 32]) -> OpeningFeeParams { |
There was a problem hiding this comment.
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?
| 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( |
There was a problem hiding this comment.
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
left a comment
tnull
left a comment
There was a problem hiding this comment.
Nice refactor! Did a high-level pass.
|
|
||
| struct JITChannel { | ||
| impl JITChannelState { | ||
| fn versions_received(&self, versions: Vec<u16>) -> Self { |
There was a problem hiding this comment.
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.
|
|
||
| #[derive(PartialEq)] | ||
| #[derive(PartialEq, Debug)] | ||
| enum JITChannelState { |
There was a problem hiding this comment.
As the other one is Outbound, we should rename this InboundJITChannelState
| @@ -62,67 +278,111 @@ struct JITChannel { | |||
| min_payment_size_msat: Option<u64>, | |||
There was a problem hiding this comment.
We might want to group all the user-settable parameters into some kind of InboundJITChannelConfig object (and corresponding OutboundJITChannelConfig)?
| } | ||
| } | ||
|
|
||
| pub fn htlc_intercepted( |
There was a problem hiding this comment.
I think none of the state-transition functions should be pub?
| struct PeerState { | ||
| channels_by_id: HashMap<u128, JITChannel>, | ||
| inbound_channels_by_id: HashMap<u128, InboundJITChannel>, | ||
| outbound_channels_by_scid: HashMap<u64, OutboundJITChannel>, |
There was a problem hiding this comment.
Why is this u64 and the other one u128?
| 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>, |
There was a problem hiding this comment.
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?
| 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>, |
There was a problem hiding this comment.
These are just inbound, right? Would be nice to now include inbound/outbound in such state fields to discern what they belong to.
| 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) { |
There was a problem hiding this comment.
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.
| } | ||
| } | ||
|
|
||
| fn map_scid_to_peer(&self, scid: u64, counterparty_node_id: PublicKey) { |
There was a problem hiding this comment.
Not sure this helper methods with less than 5 LoC improve readability. Maybe just inline it?
johncantrell97
force-pushed
the
jit-channels
branch
from
f8982d8 to
c18f353
Compare
|
Closing as superseded by #20. |
2 participants
This implements LSPS2: JIT Channels on top of LSPS0 transport layer.