Add counterparty_node_id & channel_capacity to ChannelClosed event #2387

vladimirfomene · 2023-06-30T10:49:14Z

The current ChannelClosed event does not let
you know the counterparty during a channel close
event. This change adds the counterparty_node_id
and the channel_capacity to the ChannelClosed event. This helps users to have more context during a
channel close event. Solves #2343

lightning/src/events/mod.rs

lightning/src/ln/channelmanager.rs

valentinewallace

Could you add some test coverage? Otherwise LGTM.

lightning/src/events/mod.rs

vladimirfomene · 2023-07-06T17:34:32Z

@valentinewallace, let me do that and open it up for reviews.

codecov-commenter · 2023-07-11T19:33:07Z

Codecov Report

Patch coverage: 98.43% and project coverage change: -0.02% ⚠️

Comparison is base (6f58072) 90.34% compared to head (ca02769) 90.32%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2387      +/-   ##
==========================================
- Coverage   90.34%   90.32%   -0.02%     
==========================================
  Files         106      106              
  Lines       55784    55850      +66     
  Branches    55784    55850      +66     
==========================================
+ Hits        50398    50447      +49     
- Misses       5386     5403      +17

Files Changed	Coverage Δ
lightning/src/events/mod.rs	`42.28% <80.00%> (+0.17%)`	⬆️
lightning/src/ln/channelmanager.rs	`85.64% <82.35%> (-0.08%)`	⬇️
lightning-persister/src/lib.rs	`89.16% <100.00%> (ø)`
lightning/src/chain/chainmonitor.rs	`95.03% <100.00%> (+0.02%)`	⬆️
lightning/src/chain/channelmonitor.rs	`94.77% <100.00%> (+0.05%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`98.69% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`88.46% <100.00%> (-0.38%)`	⬇️
lightning/src/ln/functional_tests.rs	`98.26% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/monitor_tests.rs	`98.52% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/payment_tests.rs	`97.67% <100.00%> (+<0.01%)`	⬆️
... and 4 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dunxen · 2023-07-12T19:37:25Z

The fix for the CI breakage is merged now so this just needs a rebase :)

dunxen

Thanks! Just a few comments.

lightning/src/events/mod.rs

lightning/src/ln/channelmanager.rs

benthecarman · 2023-07-15T15:51:58Z

If possible getting the closing txid would be great here too

vladimirfomene · 2023-07-20T03:35:12Z

@benthecarman in most cases when this event is fired, a closing tx has not yet been confirmed on chain so it is hard to figure out the closing txid at this point. I asked @TheBlueMatt on discord and here is what he had to say about it.

As you note, the issue is we don't know for sure what the closing txid will be when we generate ChannelClosed (because it hasn't confirmed and could change - even if we have a coop close txid we could still, or our counterparty could still, force-close and replace it.)

lightning/src/events/mod.rs

TheBlueMatt · 2023-07-30T20:47:33Z

lightning/src/ln/channelmanager.rs

-							reason: ClosureReason::ProcessingError { err: err.err.clone() }
+							reason: ClosureReason::ProcessingError { err: err.err.clone() },
+							counterparty_node_id: Some($counterparty_node_id),
+							channel_capacity_sats: $channel_capacity_sats,


If we think this may be set to None sometimes (due to dual funding?) then we should update the docs to indicate when the field can be None even for users of LDK 0.0.117, if not, let's make the macro take a non-option and wrap it in Some here so that its a compile error to pass None.

When might it be considered None for the dual funding case? If we get a close event before accept_channel2 is sent / received?

Thought so as well. I don’t know if we need worry about that with the new fix. @TheBlueMatt thanks for the pointer to look at the internal object

lightning/src/ln/channelmanager.rs

dunxen

Looking good. I think you don't need to worry about addressing latest feedback with fixups commits. Can just edit the relevant commits and then we're ready for accept and merge unless any final things caught in review after.

lightning/src/events/mod.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/functional_test_utils.rs

The current ChannelClosed event does not let you know the counterparty during a channel close event. This change adds the counterparty_node_id and the channel_capacity to the ChannelClosed event. This helps users to have more context during a channel close event. Solves lightningdevkit#2343

dunxen

CI failure unrelated.
LGTM

TheBlueMatt · 2023-08-10T22:28:38Z

lightning/src/events/mod.rs

@@ -755,7 +755,15 @@ pub enum Event {
 		/// [`UserConfig::manually_accept_inbound_channels`]: crate::util::config::UserConfig::manually_accept_inbound_channels
 		user_channel_id: u128,
 		/// The reason the channel was closed.
-		reason: ClosureReason
+		reason: ClosureReason,
+		/// Counterparty in the closed channel. 


You have quite a bit of whitespace at the ends of lines in your patch, a local git show should highlight them for you.

TheBlueMatt · 2023-08-10T22:39:19Z

lightning/src/ln/monitor_tests.rs

@@ -1655,7 +1655,8 @@ fn do_test_restored_packages_retry(check_old_monitor_retries_after_upgrade: bool
 	nodes[0].node.force_close_broadcasting_latest_txn(&chan_id, &nodes[1].node.get_our_node_id()).unwrap();
 	check_added_monitors(&nodes[0], 1);
 	check_closed_broadcast(&nodes[0], 1, true);
-	check_closed_event(&nodes[0], 1, ClosureReason::HolderForceClosed, false);
+	check_closed_event!(&nodes[0], 1, ClosureReason::HolderForceClosed, false,


We're trying to move away from the macros slowly towards the functions, please don't re-introduce macro calls where we previously had functions.

wpaulino reviewed Jun 30, 2023

View reviewed changes

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

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

vladimirfomene force-pushed the add_extra_fields_to_ChannelClosed_event branch from 04952ca to 01e3ef1 Compare July 6, 2023 13:55

valentinewallace reviewed Jul 6, 2023

View reviewed changes

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

vladimirfomene force-pushed the add_extra_fields_to_ChannelClosed_event branch 5 times, most recently from 9be7828 to ef131e3 Compare July 11, 2023 19:32

vladimirfomene force-pushed the add_extra_fields_to_ChannelClosed_event branch 2 times, most recently from a54a709 to ee61e38 Compare July 12, 2023 12:01

vladimirfomene marked this pull request as ready for review July 12, 2023 12:34

vladimirfomene force-pushed the add_extra_fields_to_ChannelClosed_event branch from ee61e38 to 1052ac6 Compare July 13, 2023 03:22

dunxen reviewed Jul 13, 2023

View reviewed changes

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

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

vladimirfomene force-pushed the add_extra_fields_to_ChannelClosed_event branch from 1052ac6 to 63614a1 Compare July 14, 2023 07:54

dunxen self-requested a review July 20, 2023 09:11

TheBlueMatt reviewed Jul 30, 2023

View reviewed changes

vladimirfomene marked this pull request as draft July 31, 2023 06:50

vladimirfomene force-pushed the add_extra_fields_to_ChannelClosed_event branch 2 times, most recently from 854b742 to ca02769 Compare August 4, 2023 12:10

vladimirfomene marked this pull request as ready for review August 4, 2023 14:04

dunxen reviewed Aug 8, 2023

View reviewed changes

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

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

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

vladimirfomene added 2 commits August 8, 2023 13:58

Add test coverage ChannelClosed event fields

7cfafc9

vladimirfomene force-pushed the add_extra_fields_to_ChannelClosed_event branch from ca02769 to 7cfafc9 Compare August 8, 2023 11:12

dunxen approved these changes Aug 9, 2023

View reviewed changes

TheBlueMatt approved these changes Aug 10, 2023

View reviewed changes

TheBlueMatt merged commit 131560e into lightningdevkit:main Aug 10, 2023

wpaulino mentioned this pull request Aug 15, 2023

Add counterparty_node_id to channel closed events. #2343

Closed

benthecarman mentioned this pull request Dec 4, 2023

Add channel's funding txo to Channel Event::ChannelClosed #2772

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add counterparty_node_id & channel_capacity to ChannelClosed event #2387

Add counterparty_node_id & channel_capacity to ChannelClosed event #2387

vladimirfomene commented Jun 30, 2023

valentinewallace left a comment

vladimirfomene commented Jul 6, 2023

codecov-commenter commented Jul 11, 2023 •

edited

Loading

dunxen commented Jul 12, 2023

dunxen left a comment

benthecarman commented Jul 15, 2023

vladimirfomene commented Jul 20, 2023

TheBlueMatt Jul 30, 2023

dunxen Aug 1, 2023

vladimirfomene Aug 4, 2023

dunxen left a comment

dunxen left a comment

TheBlueMatt Aug 10, 2023

TheBlueMatt Aug 10, 2023

Add counterparty_node_id & channel_capacity to ChannelClosed event #2387

Add counterparty_node_id & channel_capacity to ChannelClosed event #2387

Conversation

vladimirfomene commented Jun 30, 2023

valentinewallace left a comment

Choose a reason for hiding this comment

vladimirfomene commented Jul 6, 2023

codecov-commenter commented Jul 11, 2023 • edited Loading

Codecov Report

dunxen commented Jul 12, 2023

dunxen left a comment

Choose a reason for hiding this comment

benthecarman commented Jul 15, 2023

vladimirfomene commented Jul 20, 2023

TheBlueMatt Jul 30, 2023

Choose a reason for hiding this comment

dunxen Aug 1, 2023

Choose a reason for hiding this comment

vladimirfomene Aug 4, 2023

Choose a reason for hiding this comment

dunxen left a comment

Choose a reason for hiding this comment

dunxen left a comment

Choose a reason for hiding this comment

TheBlueMatt Aug 10, 2023

Choose a reason for hiding this comment

TheBlueMatt Aug 10, 2023

Choose a reason for hiding this comment

codecov-commenter commented Jul 11, 2023 •

edited

Loading