Skip to content
This repository was archived by the owner on Jan 6, 2025. It is now read-only.

Jit channels clean#20

Merged
tnull merged 16 commits into
lightningdevkit:mainfrom
johncantrell97:jit-channels-clean
Oct 30, 2023
Merged

Jit channels clean#20
tnull merged 16 commits into
lightningdevkit:mainfrom
johncantrell97:jit-channels-clean

Conversation

@johncantrell97
Copy link
Copy Markdown
Contributor

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

@tnull tnull mentioned this pull request Oct 5, 2023
Comment thread .github/workflows/build.yml Outdated
@tnull
Copy link
Copy Markdown
Collaborator

tnull commented Oct 12, 2023

This needs a rebase.

@johncantrell97 johncantrell97 force-pushed the jit-channels-clean branch 2 times, most recently from f28ea02 to 00fa488 Compare October 12, 2023 14:43
Copy link
Copy Markdown
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Started a thorough pass, still have to take a closer look at message handler commit and following.

Comment thread Cargo.toml Outdated
Comment thread Cargo.toml Outdated
Comment thread src/jit_channel/mod.rs Outdated
Comment thread src/jit_channel/mod.rs
Comment thread src/jit_channel/msgs.rs Outdated
Comment thread src/jit_channel/event.rs Outdated
Comment thread src/lib.rs Outdated
{
entropy_source: ES,
peer_manager: Mutex<Option<Arc<PeerManager<Descriptor, CM, RM, OM, L, CMH, NS>>>>,
channel_manager: Arc<ChannelManager<M, T, ES, NS, SP, F, R, L>>,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since LDK 0.0.117, we can use the AChannelManager trait here now.

Comment thread src/jit_channel/channel_manager.rs Outdated
Comment thread src/jit_channel/scid_utils.rs Outdated
Copy link
Copy Markdown
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completed the pass.

Looks pretty good to me, but I have yet to do another pass comparing the logic once more to the spec.

Comment thread src/jit_channel/scid_utils.rs Outdated
Comment thread src/utils.rs Outdated
Comment thread src/jit_channel/channel_manager.rs Outdated
Comment thread src/jit_channel/channel_manager.rs Outdated
Comment thread src/jit_channel/channel_manager.rs Outdated
Comment thread src/transport/message_handler.rs Outdated
Comment thread src/transport/message_handler.rs Outdated
Comment thread src/transport/message_handler.rs Outdated
Comment thread src/transport/message_handler.rs Outdated
Comment thread src/transport/message_handler.rs Outdated
@johncantrell97 johncantrell97 force-pushed the jit-channels-clean branch 2 times, most recently from 301174e to dc72629 Compare October 19, 2023 16:28
@johncantrell97 johncantrell97 force-pushed the jit-channels-clean branch 3 times, most recently from bf7e14b to 6291be8 Compare October 19, 2023 17:12
Comment thread src/jit_channel/channel_manager.rs
@johncantrell97 johncantrell97 force-pushed the jit-channels-clean branch 5 times, most recently from 278e730 to f03a1aa Compare October 21, 2023 01:27
per_peer_state: RwLock<HashMap<PublicKey, Mutex<PeerState>>>,
peer_by_scid: RwLock<HashMap<u64, PublicKey>>,
promise_secret: [u8; 32],
min_payment_size_msat: u64,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will probably need all of the config options, so might be worth holding a JitChannelConfig here?

Comment thread src/jit_channel/msgs.rs
}

impl RawOpeningFeeParams {
pub(crate) fn into_opening_fee_params(self, promise_secret: &[u8; 32]) -> OpeningFeeParams {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still would be nice to have this be a util method analogous to is_valid_opening_fee (could also DRY this one up then) to keep LSP-specific logic separate from the messaging/type serialization.

Happy to have that happen in a follow-up though.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread Cargo.toml

bitcoin = "0.29.0"

chrono = { version = "0.4", default-features = false, features = ["std", "serde"] }
Copy link
Copy Markdown
Collaborator

@tnull tnull Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems we don't actually require full std here, alloc should be enough.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now tracking here: #29 (comment)

}

/// Configuration options for JIT channels.
pub struct JITChannelsConfig {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably should be renamed OutboundJitChannelConfig or LSPS2OutboundChannelConfig and get moved to the jit_channel/lsps2 module.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now tracking here #31

Copy link
Copy Markdown
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright. I had another look at the spec and poked around a bit more.

While nothing in particular stood out, we'll def. want want to increase our input validation, error handling and test coverage going forward, i.e., before this is deemed stable and usable in production.

I had some minor comments which however can be addressed as follow ups. To this end, and to generally track next steps, I now opened a bunch of issues.

That said, for now, I'm merging this first draft 🎉

@tnull tnull merged commit 40cc652 into lightningdevkit:main Oct 30, 2023
@tnull tnull mentioned this pull request Oct 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants