Pass `counterparty_node_id` to `ChannelManager` functions #1479

ViktorTigerstrom · 2022-05-12T23:28:11Z

After #105 and #422, channels will be stored in the ChannelManager::per_peer_state which requires the counterparty_node_id to access them. #1278 also requires this updated storage design. This will require that the user passes the counterparty_node_id to some of the public API functions of the ChannelManager for which only the channel_id is passed currently.

This PR updates the parameters required in those functions, to include the counterparty_node_id.

I figured it's probably better to separate the API changes from rest of the channel storage changes. Let me know if you're of a different opinion, and I'll close this PR.

Pushing this as a draft for now, but let me know if you think that better to merge this before the other functionality is ready (#105 and #422), which I will base on this.

Finally, when updating these functions, I also noticed that lots of other functions that has the counterparty_node_id as an input parameter, name the counterparty_node_id differently. That could be a bit confusing in my opinion. Examples that I have found are: their_node_id, their_network_key, peer_node_id. Let me know if you'd like me to update the name this PR is using, or if I should update the name in the other functions to make it uniform.

codecov-commenter · 2022-05-12T23:49:29Z

Codecov Report

Merging #1479 (e9a5407) into main (b5a6307) will decrease coverage by 0.01%.
The diff coverage is 98.03%.

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

@@            Coverage Diff             @@
##             main    #1479      +/-   ##
==========================================
- Coverage   90.89%   90.88%   -0.02%     
==========================================
  Files          76       76              
  Lines       42067    42073       +6     
  Branches    42067    42073       +6     
==========================================
+ Hits        38238    38239       +1     
- Misses       3829     3834       +5

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	`84.75% <91.30%> (-0.03%)`	⬇️
lightning-background-processor/src/lib.rs	`94.37% <100.00%> (ø)`
lightning-persister/src/lib.rs	`93.45% <100.00%> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`97.76% <100.00%> (ø)`
lightning/src/ln/functional_test_utils.rs	`95.54% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.09% <100.00%> (-0.08%)`	⬇️
lightning/src/ln/monitor_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/payment_tests.rs	`99.23% <100.00%> (ø)`
lightning/src/ln/priv_short_conf_tests.rs	`97.95% <100.00%> (ø)`
lightning/src/ln/shutdown_tests.rs	`96.50% <100.00%> (ø)`
... and 4 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 b5a6307...c179844. Read the comment docs.

ViktorTigerstrom · 2022-05-12T23:53:38Z

Sorry for the failed checks, will address them tomorrow.

ViktorTigerstrom · 2022-05-13T00:12:15Z

lightning/src/ln/channelmanager.rs

@@ -2757,7 +2756,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	///
 	/// [`Event::FundingGenerationReady`]: crate::util::events::Event::FundingGenerationReady
 	/// [`Event::ChannelClosed`]: crate::util::events::Event::ChannelClosed
-	pub fn funding_transaction_generated(&self, temporary_channel_id: &[u8; 32], funding_transaction: Transaction) -> Result<(), APIError> {
+	pub fn funding_transaction_generated(&self, temporary_channel_id: &[u8; 32], counterparty_node_id: &PublicKey, funding_transaction: Transaction) -> Result<(), APIError> {


Should we add the counterparty_node_id to the FundingGenerationReady event?

TheBlueMatt · 2022-05-13T02:56:13Z

lightning/src/ln/channelmanager.rs

@@ -1769,7 +1769,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		});
 	}

-	fn close_channel_internal(&self, channel_id: &[u8; 32], target_feerate_sats_per_1000_weight: Option<u32>) -> Result<(), APIError> {
+	fn close_channel_internal(&self, channel_id: &[u8; 32], counterparty_node_id: &PublicKey, target_feerate_sats_per_1000_weight: Option<u32>) -> Result<(), APIError> {


nit: add a _ to the variable name so rustc doesn't complain its unused.

ViktorTigerstrom · 2022-05-13T22:01:39Z

Addressed the feedback and the failed checks. Let me know if you think it's worth adding the extra counterparty_node_id check to create_funding_transaction (commit a85a63e), and I'll squash it with the FundingGenerationReady commit! Else I will drop that specific commit.

ViktorTigerstrom · 2022-05-13T22:07:41Z

lightning/src/ln/channelmanager.rs

@@ -4130,6 +4131,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				if !channel.get().inbound_is_awaiting_accept() {
 					return Err(APIError::APIMisuseError { err: "The channel isn't currently awaiting to be accepted.".to_owned() });
 				}
+				if *counterparty_node_id != channel.get().get_counterparty_node_id() {


Let me know if you want me to add a test case for this. Though given that it shouldn't be a thing after #105, perhaps it isn't really worth it.

Yea don't worry about it, then.

TheBlueMatt · 2022-05-13T23:57:46Z

Feel free to squash the fix up commits into the corresponding commits.

ViktorTigerstrom · 2022-05-14T00:20:43Z

Thanks @TheBlueMatt! Squashed the commits without changes. The only one I'm not entirely sure about is ded76da. As it's an update to the docs of the OpenChannelRequest, do you prefer it to be a separate commit, or should I squash it with the Pass counterparty_node_id to accept_inbound_channel commit as that's the reason for the doc update?

TheBlueMatt · 2022-05-14T03:27:50Z

lightning/src/ln/channelmanager.rs

@@ -1769,7 +1769,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		});
 	}

-	fn close_channel_internal(&self, channel_id: &[u8; 32], target_feerate_sats_per_1000_weight: Option<u32>) -> Result<(), APIError> {
+	fn close_channel_internal(&self, channel_id: &[u8; 32], _counterparty_node_id: &PublicKey, target_feerate_sats_per_1000_weight: Option<u32>) -> Result<(), APIError> {


I mean if you want to you can replace the existing counterparty_node_id variable in the function, but of course it doesn't matter it'll come in the next PR.

Yeah that was my reasoning behind that choice, but changed that now just in case you'd like to merge this PR before the next PR is completed :).

As the `counterparty_node_id` is now required to be passed back to the `ChannelManager` to accept or reject an inbound channel request, the documentation is updated to reflect that.

ViktorTigerstrom · 2022-05-14T18:41:23Z

Addressed the latest feedback, and marked this as an open PR in case you think it's worth merging this before the functionality for #105 is completed :)

ViktorTigerstrom commented May 13, 2022

View reviewed changes

TheBlueMatt reviewed May 13, 2022

View reviewed changes

ViktorTigerstrom commented May 13, 2022

View reviewed changes

Add counterparty_node_id to FundingGenerationReady

7893ddc

ViktorTigerstrom force-pushed the 2022-05-pass-counterparty-id-to-functions branch from e9a5407 to ded76da Compare May 14, 2022 00:17

TheBlueMatt previously approved these changes May 14, 2022

View reviewed changes

ViktorTigerstrom added 5 commits May 14, 2022 20:32

Pass counterparty_node_id to close_channel functions

84a6e7b

Pass counterparty_node_id to force_close_channel

14e52cd

Pass counterparty_node_id to funding_transaction_generated

c581bab

Pass counterparty_node_id to accept_inbound_channel

70fa465

Update OpenChannelRequest documentation

c179844

As the `counterparty_node_id` is now required to be passed back to the `ChannelManager` to accept or reject an inbound channel request, the documentation is updated to reflect that.

ViktorTigerstrom dismissed TheBlueMatt’s stale review via c179844 May 14, 2022 18:35

ViktorTigerstrom force-pushed the 2022-05-pass-counterparty-id-to-functions branch from ded76da to c179844 Compare May 14, 2022 18:35

ViktorTigerstrom marked this pull request as ready for review May 14, 2022 18:38

TheBlueMatt added the Seeking Code Review label May 14, 2022

TheBlueMatt approved these changes May 14, 2022

View reviewed changes

arik-so approved these changes May 16, 2022

View reviewed changes

arik-so merged commit a5629e5 into lightningdevkit:main May 16, 2022

ViktorTigerstrom mentioned this pull request May 16, 2022

Add missing counterparty_node_id in force_close_channel calls #1485

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass `counterparty_node_id` to `ChannelManager` functions #1479

Pass `counterparty_node_id` to `ChannelManager` functions #1479

ViktorTigerstrom commented May 12, 2022

codecov-commenter commented May 12, 2022 •

edited

Loading

ViktorTigerstrom commented May 12, 2022

ViktorTigerstrom May 13, 2022

TheBlueMatt May 13, 2022

TheBlueMatt May 13, 2022

ViktorTigerstrom commented May 13, 2022

ViktorTigerstrom May 13, 2022

TheBlueMatt May 13, 2022

TheBlueMatt commented May 13, 2022

ViktorTigerstrom commented May 14, 2022 •

edited

Loading

TheBlueMatt May 14, 2022

ViktorTigerstrom May 14, 2022

ViktorTigerstrom commented May 14, 2022

Pass counterparty_node_id to ChannelManager functions #1479

Pass counterparty_node_id to ChannelManager functions #1479

Conversation

ViktorTigerstrom commented May 12, 2022

codecov-commenter commented May 12, 2022 • edited Loading

Codecov Report

ViktorTigerstrom commented May 12, 2022

ViktorTigerstrom May 13, 2022

Choose a reason for hiding this comment

TheBlueMatt May 13, 2022

Choose a reason for hiding this comment

TheBlueMatt May 13, 2022

Choose a reason for hiding this comment

ViktorTigerstrom commented May 13, 2022

ViktorTigerstrom May 13, 2022

Choose a reason for hiding this comment

TheBlueMatt May 13, 2022

Choose a reason for hiding this comment

TheBlueMatt commented May 13, 2022

ViktorTigerstrom commented May 14, 2022 • edited Loading

TheBlueMatt May 14, 2022

Choose a reason for hiding this comment

ViktorTigerstrom May 14, 2022

Choose a reason for hiding this comment

ViktorTigerstrom commented May 14, 2022

Pass `counterparty_node_id` to `ChannelManager` functions #1479

Pass `counterparty_node_id` to `ChannelManager` functions #1479

codecov-commenter commented May 12, 2022 •

edited

Loading

ViktorTigerstrom commented May 14, 2022 •

edited

Loading