Expose more info in `PaymentForwarded` event by atalw · Pull Request #1419 · lightningdevkit/rust-lightning

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

+ 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

+ 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

 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


 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

 	/// 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

TheBlueMatt

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

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

 	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

 	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 · 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

+ 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

+ 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

 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


 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

 	/// 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

TheBlueMatt

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

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

 	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

 	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 · 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

+ 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

+ 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

 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


 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

 	/// 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

TheBlueMatt

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

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

 	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

 	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 · 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

+ 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

+ 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

 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


 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

 	/// 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

TheBlueMatt

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

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

 	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

 	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 · 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

+ 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

+ 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

 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


 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

 	/// 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

TheBlueMatt

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

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

 	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

 	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 · 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

+ 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

+ 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

 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


 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

 	/// 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

TheBlueMatt

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

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

 	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

 	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 · 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

+ 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

+ 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

 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


 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

 	/// 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

TheBlueMatt

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

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

 	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

 	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 · 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

+ 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

+ 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

 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


 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

 	/// 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

TheBlueMatt

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

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

 	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

 	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 95bf287Compare 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

Comment threadlightning/src/ln/functional_test_utils.rs Outdated

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 e53c5bdApril 20, 2022 05:28

atalw force-pushed the 2022-03-paymentforwarded-event branch from 036a988 to e53c5bdCompare 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:mainApr 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

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 95bf287Compare 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

Comment threadlightning/src/ln/functional_test_utils.rs Outdated

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 e53c5bdApril 20, 2022 05:28

atalw force-pushed the 2022-03-paymentforwarded-event branch from 036a988 to e53c5bdCompare 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:mainApr 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

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 95bf287Compare 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

Comment threadlightning/src/ln/functional_test_utils.rs Outdated

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 e53c5bdApril 20, 2022 05:28

atalw force-pushed the 2022-03-paymentforwarded-event branch from 036a988 to e53c5bdCompare 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:mainApr 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

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 95bf287Compare 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

Comment threadlightning/src/ln/functional_test_utils.rs Outdated

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 e53c5bdApril 20, 2022 05:28

atalw force-pushed the 2022-03-paymentforwarded-event branch from 036a988 to e53c5bdCompare 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:mainApr 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

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 95bf287Compare 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

Comment threadlightning/src/ln/functional_test_utils.rs Outdated

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 e53c5bdApril 20, 2022 05:28

atalw force-pushed the 2022-03-paymentforwarded-event branch from 036a988 to e53c5bdCompare 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:mainApr 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

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 95bf287Compare 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

Comment threadlightning/src/ln/functional_test_utils.rs Outdated

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 e53c5bdApril 20, 2022 05:28

atalw force-pushed the 2022-03-paymentforwarded-event branch from 036a988 to e53c5bdCompare 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:mainApr 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

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 95bf287Compare 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

Comment threadlightning/src/ln/functional_test_utils.rs Outdated

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 e53c5bdApril 20, 2022 05:28

atalw force-pushed the 2022-03-paymentforwarded-event branch from 036a988 to e53c5bdCompare 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:mainApr 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

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 95bf287Compare 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

Comment threadlightning/src/ln/functional_test_utils.rs Outdated

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 e53c5bdApril 20, 2022 05:28

atalw force-pushed the 2022-03-paymentforwarded-event branch from 036a988 to e53c5bdCompare 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:mainApr 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


		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 };

Uh oh!

Conversation

atalw commented Apr 12, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atalwApr 16, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Apr 17, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Apr 19, 2022

Uh oh!

atalw commented Apr 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

atalw commented Apr 12, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atalwApr 16, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atalw commented Apr 12, 2022•
edited
Loading

atalwApr 16, 2022•
edited
Loading

codecov-commenter commented Apr 17, 2022•
edited
Loading

atalw commented Apr 12, 2022•
edited
Loading

atalwApr 16, 2022•
edited
Loading

codecov-commenter commented Apr 17, 2022•
edited
Loading