Expose more info in `PaymentForwarded` event #1419

atalw · 2022-04-12T11:21:34Z

This PR closes #1391.

This adds source_node_id and channel_id keys in the PaymentForwarded event.

There are 2 pending blocking issues after which the PR will be open to be merged.

…ail)

atalw · 2022-04-16T09:11:25Z

lightning/src/ln/channelmanager.rs

+								panic!("Channel doesn't exist")
+							}
+						};
+						let source_node_id = channel.get().get_counterparty_node_id();


@TheBlueMatt Getting the correct source_node_id and channel_id now. Only case remaining is when the channel has been closed. How do we get the source_node_id in that case? And should we still emit the id of the closed channel?

Hmmmm, maybe lets just drop the source node_id entirely for now? Its kinda annoying to do a channel lookup just to get it, especially given we can't find it if we went onchain. Users can do the lookup manually, if they want.

atalw · 2022-04-16T09:13:30Z

lightning/src/ln/functional_test_utils.rs

+				let next_channel_id = match node.node.list_usable_channels().iter().find(|&x| x.counterparty.node_id == next_node.node.get_our_node_id()) {
+					Some(channel) => channel.channel_id,
+					None => panic!("Uh oh! Coudn't find the channel")
+				};


@TheBlueMatt Do we need to handle the case where there could be multiple channels opened within the same 2 nodes? Also there are cases where the channel doesn't exist (eg channel_monitor_network_test).

Do we need to handle the case where there could be multiple channels opened within the same 2 nodes?

Yea, maybe lets make the check "is the channel listed in the event one of the ones between the two given nodes".

Also there are cases where the channel doesn't exist (eg channel_monitor_network_test

I assume those are cases when the channel is already closed? Can we detect that based on fee_earned_msat being None in the event?

Cool, done.

codecov-commenter · 2022-04-17T11:04:41Z

Codecov Report

Merging #1419 (036a988) into main (7671ae5) will increase coverage by 0.09%.
The diff coverage is 86.11%.

❗ Current head 036a988 differs from pull request most recent head e53c5bd. Consider uploading reports for the commit e53c5bd to get more accurate results

@@            Coverage Diff             @@
##             main    #1419      +/-   ##
==========================================
+ Coverage   90.76%   90.86%   +0.09%     
==========================================
  Files          73       74       +1     
  Lines       41195    41423     +228     
  Branches    41195    41423     +228     
==========================================
+ Hits        37392    37638     +246     
+ Misses       3803     3785      -18

Impacted Files	Coverage Δ
lightning/src/util/events.rs	`33.56% <25.00%> (+0.11%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.07% <87.50%> (-0.01%)`	⬇️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.76% <100.00%> (ø)`
lightning/src/ln/channelmanager.rs	`84.70% <100.00%> (-0.05%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`95.56% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/payment_tests.rs	`99.23% <100.00%> (ø)`
lightning/src/ln/reorg_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/shutdown_tests.rs	`96.50% <100.00%> (ø)`
lightning-invoice/src/lib.rs	`87.39% <0.00%> (-0.85%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`97.45% <0.00%> (-0.17%)`	⬇️
... and 18 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 7671ae5...e53c5bd. Read the comment docs.

TheBlueMatt · 2022-04-17T17:01:33Z

lightning/src/util/events.rs

 				7u8.write(writer)?;
 				write_tlv_fields!(writer, {
-					(0, fee_earned_msat, option),
-					(2, claim_from_onchain_tx, required),
+					(0, channel_id, option),


We can't change the indexes of existing fields or we'll fail to read old data. For new fields, we want to use odd field IDs (which tells old versions to simply ignore the field if they don't understand it) and options (which mean we set the field to None if its not included, ie because the object was serialized by an old version of LDK.

Changed channel_id to 1

TheBlueMatt · 2022-04-17T17:03:02Z

lightning/src/ln/channelmanager.rs

@@ -4291,12 +4290,16 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 						} else { None };

 						let mut pending_events = self.pending_events.lock().unwrap();
+
+						let channel_id = if fee_earned_msat.is_some() { Some(prev_outpoint.to_channel_id()) } else { None };


Heh, oops, I didn't mean "set it to None if the channel is gone" I meant "dont test for it if the fee_earned_msat field is None in tests", we should include it anyways because we have it and users may care.

TheBlueMatt · 2022-04-17T17:03:12Z

lightning/src/util/events.rs

@@ -343,6 +343,9 @@ pub enum Event {
 	/// This event is generated when a payment has been successfully forwarded through us and a
 	/// forwarding fee earned.
 	PaymentForwarded {
+		/// The channel between the source node and us. Optional because PaymentForwarded event can 
+		/// be emitted even after the channel has been closed.


This should mention something about old versions of LDK.

I'll just say it's optional because old versions of LDK do not serialize this field. And I think we should mention a version number to make it more concrete. If yes, is it v0.0.106?

Yep, best to say "versions prior to 0.0.107" or "versions up to 0.0.106" or whatever.

TheBlueMatt

This basically looks good to me, I think. Will get you a second reviewer when you unmark-draft.

valentinewallace

I believe 4 warnings were introduced in CI: https://github.com/lightningdevkit/rust-lightning/runs/6065172483?check_suite_focus=true

Otherwise, this basically LGTM

lightning/src/ln/functional_test_utils.rs

TheBlueMatt

A few minor nits, but I think that's about it.

TheBlueMatt · 2022-04-19T13:06:24Z

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -2073,7 +2073,7 @@ fn test_fail_htlc_on_broadcast_after_claim() {
 	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
 	let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);

-	create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
+	let _ = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).2;


No need to change these lines at all now.

TheBlueMatt · 2022-04-19T13:07:47Z

lightning/src/util/events.rs

@@ -343,6 +343,9 @@ pub enum Event {
 	/// This event is generated when a payment has been successfully forwarded through us and a
 	/// forwarding fee earned.
 	PaymentForwarded {
+		/// The channel between the source node and us. Optional because versions prior to 0.0.107
+		/// do not serialize this field.
+		channel_id: Option<[u8; 32]>,


Lets call this source_channel_id or so? I imagine eventually we'll add a sink_channel_id field as well.

Updated to source_channel_id.

TheBlueMatt · 2022-04-19T18:00:43Z

Nice! Okay, can you squash the commits down into a clean git history? Take a look at https://github.com/bitcoin/bitcoin/blob/master/doc/productivity.md#interactive-dummy-rebases-for-fixups-and-execs-with-git-merge-base if you aren't sure how to go about doing it.

atalw · 2022-04-20T05:31:07Z

Squashed.

atalw referenced this pull request in atalw/rust-lightning-1 Apr 12, 2022

Add channel_id key in PaymentForwarded event (FIXME: some tests f…

8107f9b

…ail)

atalw commented Apr 16, 2022

View reviewed changes

atalw force-pushed the 2022-03-paymentforwarded-event branch from a295722 to 95bf287 Compare April 17, 2022 11:04

TheBlueMatt reviewed Apr 17, 2022

View reviewed changes

TheBlueMatt added the Seeking Code Review label Apr 18, 2022

TheBlueMatt reviewed Apr 18, 2022

View reviewed changes

atalw marked this pull request as ready for review April 18, 2022 16:28

valentinewallace reviewed Apr 18, 2022

View reviewed changes

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

TheBlueMatt removed the Seeking Code Review label Apr 18, 2022

TheBlueMatt reviewed Apr 19, 2022

View reviewed changes

valentinewallace previously approved these changes Apr 19, 2022

View reviewed changes

Add source_channel_id in PaymentForwarded event

e53c5bd

atalw dismissed valentinewallace’s stale review via e53c5bd April 20, 2022 05:28

atalw force-pushed the 2022-03-paymentforwarded-event branch from 036a988 to e53c5bd Compare April 20, 2022 05:28

TheBlueMatt approved these changes Apr 20, 2022

View reviewed changes

TheBlueMatt assigned valentinewallace Apr 20, 2022

valentinewallace approved these changes Apr 20, 2022

View reviewed changes

valentinewallace merged commit 742f5e5 into lightningdevkit:main Apr 20, 2022

This was referenced Apr 20, 2022

Expose more info in PaymentForwarded #1391

Closed

expose from/to_channel_id in PaymentForwarded Event #1231

Closed

atalw deleted the 2022-03-paymentforwarded-event branch April 22, 2022 05:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose more info in `PaymentForwarded` event #1419

Expose more info in `PaymentForwarded` event #1419

atalw commented Apr 12, 2022 •

edited

Loading

atalw Apr 16, 2022

TheBlueMatt Apr 16, 2022

atalw Apr 16, 2022 •

edited

Loading

TheBlueMatt Apr 16, 2022

atalw Apr 17, 2022

codecov-commenter commented Apr 17, 2022 •

edited

Loading

TheBlueMatt Apr 17, 2022

atalw Apr 18, 2022

TheBlueMatt Apr 17, 2022

TheBlueMatt Apr 17, 2022

atalw Apr 18, 2022

TheBlueMatt Apr 18, 2022

TheBlueMatt left a comment

valentinewallace left a comment

TheBlueMatt left a comment

TheBlueMatt Apr 19, 2022

atalw Apr 19, 2022

TheBlueMatt Apr 19, 2022

atalw Apr 19, 2022

TheBlueMatt commented Apr 19, 2022

atalw commented Apr 20, 2022

Expose more info in PaymentForwarded event #1419

Expose more info in PaymentForwarded event #1419

Conversation

atalw commented Apr 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atalw Apr 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 17, 2022 • edited Loading

Codecov Report

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

TheBlueMatt left a comment

Choose a reason for hiding this comment

valentinewallace left a comment

Choose a reason for hiding this comment

TheBlueMatt 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

TheBlueMatt commented Apr 19, 2022

atalw commented Apr 20, 2022

Expose more info in `PaymentForwarded` event #1419

Expose more info in `PaymentForwarded` event #1419

atalw commented Apr 12, 2022 •

edited

Loading

atalw Apr 16, 2022 •

edited

Loading

codecov-commenter commented Apr 17, 2022 •

edited

Loading