Expose temporary channel ID and user channel ID pre-funding by TheBlueMatt · Pull Request #1121 · lightningdevkit/rust-lightning

TheBlueMatt · 2021-10-13T21:46:01Z

Its pretty hard for users to track a channel pre-funding to ChannelClosed especially when our counterparty closed the channel due to a parameter mismatch. This resolves that with more data in the relevant events.

This makes it more practical for users to track channels prior to funding, especially if the channel fails because the peer rejects it for a parameter mismatch.

This makes it more practical for users to track channels using their own IDs, especially across funding.

codecov · 2021-10-13T22:15:42Z

Codecov Report

Merging #1121 (347c49e) into main (2144166) will increase coverage by 1.16%.
The diff coverage is 63.63%.

❗ Current head 347c49e differs from pull request most recent head bb7ef6c. Consider uploading reports for the commit bb7ef6c to get more accurate results

@@            Coverage Diff             @@
##             main    #1121      +/-   ##
==========================================
+ Coverage   90.67%   91.83%   +1.16%     
==========================================
  Files          66       66              
  Lines       34732    39174    +4442     
==========================================
+ Hits        31492    35977    +4485     
+ Misses       3240     3197      -43

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`96.04% <ø> (ø)`
lightning/src/ln/channelmanager.rs	`85.24% <53.84%> (+0.08%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`96.92% <100.00%> (+1.83%)`	⬆️
lightning/src/util/events.rs	`33.33% <100.00%> (+0.92%)`	⬆️
lightning/src/chain/mod.rs	`50.00% <0.00%> (-8.83%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`96.52% <0.00%> (-0.12%)`	⬇️
lightning/src/ln/monitor_tests.rs	`100.00% <0.00%> (ø)`
lightning-background-processor/src/lib.rs	`94.38% <0.00%> (+0.15%)`	⬆️
lightning-persister/src/lib.rs	`94.96% <0.00%> (+0.74%)`	⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2144166...bb7ef6c. Read the comment docs.

arik-so

If we're gonna keep a user id around, it might be helpful to have the abilitty to set a channel's user id (just once), so once an iincoming channel is persisted to some local database, it's easier to keep track of.

arik-so · 2021-10-14T20:18:53Z

lightning/src/ln/channelmanager.rs

+	/// [`Event::FundingGenerationReady`]: events::Event::FundingGenerationReady
+	/// [`Event::FundingGenerationReady::temporary_channel_id`]: events::Event::FundingGenerationReady::temporary_channel_id
+	/// [`Event::ChannelClosed::channel_id`]: events::Event::ChannelClosed::channel_id
+	pub fn create_channel(&self, their_network_key: PublicKey, channel_value_satoshis: u64, push_msat: u64, user_id: u64, override_config: Option<UserConfig>) -> Result<[u8; 32], APIError> {


Would it make sense to create a type alias for the channel id here instead of just [u8; 32]?

Yea, maybe lets do that as a part of addressing #105? We'll want to separate the types out between temporary channel ids and permanent ones, I think.

TheBlueMatt · 2021-10-14T22:50:32Z

If we're gonna keep a user id around, it might be helpful to have the abilitty to set a channel's user id (just once), so once an iincoming channel is persisted to some local database, it's easier to keep track of.

Yea, there's a few things we should probably consider for the user id, since its not new or (materially) changed in this PR, lets leave it for a followup and figure out what we want to do with it in #1125.

valentinewallace

Minor suggestions only

lightning/src/ln/channelmanager.rs

jkczyz

Fine with holding off on decision on channel_user_id as per #1125.

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2021-10-15T23:03:54Z

Oops, sorry, fixed missing renames for user_id.

TheBlueMatt · 2021-10-16T00:28:23Z

Squashed the fixup commit down, diff since Jeff's ACK:

$ git diff-tree -U1 347c49e1 bb7ef6c29
diff --git a/fuzz/src/router.rs b/fuzz/src/router.rs
index 6c792916f..b80a8284e 100644
--- a/fuzz/src/router.rs
+++ b/fuzz/src/router.rs
@@ -218,3 +218,3 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 								channel_value_satoshis: slice_to_be64(get_slice!(8)),
-								user_id: 0, inbound_capacity_msat: 0,
+								user_channel_id: 0, inbound_capacity_msat: 0,
 								unspendable_punishment_reserve: None,
$

TheBlueMatt · 2021-10-16T02:14:59Z

Diff since Val's ACK is also just one trivial variable rename, so gonna take this. Gotta wait on #1127 anyway:

$ git diff-tree -U1 539bb6d0aed0c0cc273f78ec3c29b7aeaec64ac0 bb7ef6c2901a9582e19511ab0a974d1de3ef1205
diff --git a/fuzz/src/router.rs b/fuzz/src/router.rs
index 6c792916f..b80a8284e 100644
--- a/fuzz/src/router.rs
+++ b/fuzz/src/router.rs
@@ -218,3 +218,3 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 								channel_value_satoshis: slice_to_be64(get_slice!(8)),
-								user_id: 0, inbound_capacity_msat: 0,
+								user_channel_id: 0, inbound_capacity_msat: 0,
 								unspendable_punishment_reserve: None,
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 7f4caacfc..0106fe51d 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -778,4 +778,4 @@ pub struct ChannelDetails {
 	pub unspendable_punishment_reserve: Option<u64>,
-	/// The user_id passed in to create_channel, or 0 if the channel was inbound.
-	pub user_id: u64,
+	/// The `user_channel_id` passed in to create_channel, or 0 if the channel was inbound.
+	pub user_channel_id: u64,
 	/// The available outbound capacity for sending HTLCs to the remote peer. This does not include
@@ -1363,3 +1363,3 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					outbound_capacity_msat,
-					user_id: channel.get_user_id(),
+					user_channel_id: channel.get_user_id(),
 					confirmations_required: channel.minimum_depth(),
diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index d6adb889d..9e02e10f9 100644
--- a/lightning/src/routing/router.rs
+++ b/lightning/src/routing/router.rs
@@ -1329,3 +1329,3 @@ mod tests {
 			channel_value_satoshis: 0,
-			user_id: 0,
+			user_channel_id: 0,
 			outbound_capacity_msat,
diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs
index 8502f9be4..7ea369f55 100644
--- a/lightning/src/util/events.rs
+++ b/lightning/src/util/events.rs
@@ -148,4 +148,4 @@ pub enum Event {
 		output_script: Script,
-		/// The `user_id` value passed in to [`ChannelManager::create_channel`], or 0 for an
-		/// inbound channel.
+		/// The `user_channel_id` value passed in to [`ChannelManager::create_channel`], or 0 for
+		/// an inbound channel.
 		///
@@ -267,4 +267,4 @@ pub enum Event {
 		channel_id: [u8; 32],
-		/// The `user_id` value passed in to [`ChannelManager::create_channel`], or 0 for an
-		/// inbound channel. This will always be zero for objects serialized with LDK versions
+		/// The `user_channel_id` value passed in to [`ChannelManager::create_channel`], or 0 for
+		/// an inbound channel. This will always be zero for objects serialized with LDK versions
 		/// prior to 0.0.102.
$

TheBlueMatt added 2 commits October 13, 2021 21:44

Return the temporary channel id in success from create_channel

be82239

This makes it more practical for users to track channels prior to funding, especially if the channel fails because the peer rejects it for a parameter mismatch.

Include the user channel id counter in Event::ChannelClosed

9a5c9b3

This makes it more practical for users to track channels using their own IDs, especially across funding.

arik-so reviewed Oct 14, 2021

View reviewed changes

valentinewallace approved these changes Oct 15, 2021

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

TheBlueMatt added this to the 0.0.102 milestone Oct 15, 2021

valentinewallace approved these changes Oct 15, 2021

View reviewed changes

jkczyz approved these changes Oct 15, 2021

View reviewed changes

lightning/src/ln/channelmanager.rs Show resolved Hide resolved

jkczyz approved these changes Oct 15, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-10-return-temp-id branch from 347c49e to 656a4a1 Compare October 16, 2021 00:25

Rename create_channel param to user_channel_id to standardize it

bb7ef6c

TheBlueMatt force-pushed the 2021-10-return-temp-id branch from 656a4a1 to bb7ef6c Compare October 16, 2021 00:27

TheBlueMatt merged commit 001bc71 into lightningdevkit:main Oct 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose temporary channel ID and user channel ID pre-funding#1121

Expose temporary channel ID and user channel ID pre-funding#1121
TheBlueMatt merged 3 commits intolightningdevkit:mainfrom
TheBlueMatt:2021-10-return-temp-id

TheBlueMatt commented Oct 13, 2021

Uh oh!

codecov bot commented Oct 13, 2021 •

edited

Loading

Uh oh!

arik-so left a comment

Uh oh!

arik-so Oct 14, 2021

Uh oh!

TheBlueMatt Oct 14, 2021

Uh oh!

TheBlueMatt commented Oct 14, 2021

Uh oh!

valentinewallace left a comment

Uh oh!

Uh oh!

Uh oh!

jkczyz left a comment

Uh oh!

Uh oh!

TheBlueMatt commented Oct 15, 2021

Uh oh!

TheBlueMatt commented Oct 16, 2021

Uh oh!

TheBlueMatt commented Oct 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

TheBlueMatt commented Oct 13, 2021

Uh oh!

codecov bot commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

arik-so left a comment

Choose a reason for hiding this comment

Uh oh!

arik-so Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Oct 14, 2021

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt commented Oct 15, 2021

Uh oh!

TheBlueMatt commented Oct 16, 2021

Uh oh!

TheBlueMatt commented Oct 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Oct 13, 2021 •

edited

Loading