Add option accept or reject channel requests #1281

ViktorTigerstrom · 2022-01-26T00:03:33Z

Adds support for manually accepting or rejecting a request to open a channel. Issue #1242

codecov-commenter · 2022-01-26T00:26:15Z

Codecov Report

Merging #1281 (6517c62) into main (b8e9e8b) will decrease coverage by 0.01%.
The diff coverage is 93.08%.

❗ Current head 6517c62 differs from pull request most recent head 1891b37. Consider uploading reports for the commit 1891b37 to get more accurate results

@@            Coverage Diff             @@
##             main    #1281      +/-   ##
==========================================
- Coverage   90.50%   90.48%   -0.02%     
==========================================
  Files          71       71              
  Lines       39042    39177     +135     
==========================================
+ Hits        35336    35451     +115     
- Misses       3706     3726      +20

Impacted Files	Coverage Δ
lightning/src/util/config.rs	`44.68% <0.00%> (-3.15%)`	⬇️
lightning/src/util/events.rs	`33.45% <66.66%> (+0.71%)`	⬆️
lightning/src/ln/channelmanager.rs	`84.59% <88.23%> (+0.12%)`	⬆️
lightning/src/ln/channel.rs	`89.29% <95.23%> (-0.02%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.02% <96.03%> (-0.21%)`	⬇️

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 b8e9e8b...1891b37. Read the comment docs.

TheBlueMatt · 2022-01-26T01:37:16Z

lightning/src/ln/channelmanager.rs

+		return self.internal_create_requested_channel(counterparty_node_id, their_features, msg);
+	}
+
+	fn internal_create_requested_channel(&self, counterparty_node_id: &PublicKey, their_features: InitFeatures, msg: &msgs::OpenChannel) -> Result<(), MsgHandleErrInternal>{


Instead of splitting the function here, we can simplify things a bit by splitting the function a few lines down and moving only the SendAcceptChannel message generation out. There's no harm in having a channel be inserted into the channel_state.by_id map even if its not accepted yet, we just have to make sure we reject a FundingCreated message in Channel::funding_created if the sender is trying to be too clever and skip waiting for our accept. This will require a new flag in the Channel which is updated when we (eventually) call get_accept_channel and checked in funding_created.

From there, Event::OpenChannelRequest can be tweaked to just include relevant info instead of doing it via a struct, and respond_to_open_channel_request can just take the temporary_channel_id. All that said, I'd suggest we err on the side of including too much data rather than too little, as the node operator may need extra data to decide if they want to accept a channel.

Thanks a lot for the detailed input! I'll start working on implementing your suggestions.

Just to clarify though, how much/which data is enough for node operator to aid them in the decision to accept or reject the channel? Including all fields in the OpenChannel msg is too excessive, but given that I'd really appreciate if I'd could get some feedback on which fields should be included instead of selecting myself.

I'm not sure there's necessarily a clear answer, but maybe include, for now (we can always add more later): counterparty node id, chain_hash, temporary_channel_id, funding_satoshis, push_msat?

Thanks, I'll start with those.

TheBlueMatt · 2022-01-26T01:41:16Z

lightning/src/util/events.rs

@@ -515,6 +535,12 @@ impl Writeable for Event {
 					(2, payment_hash, required),
 				})
 			},
+			&Event::OpenChannelRequest { ref open_channel_request_info } => {
+				11u8.write(writer)?;


11 is already used for DiscardFunding. We need a new value here. That said, we shouldn't actually ever load this object from disk, similar to how we handle PendingHTLCsForwardable and FundingGenerationReady - if we have such an object when we go to serialize, it won't make sense when we go to reload it. In lightning, if a channel has not exchanged funding messages, it is dropped on reconnect (ie restart), so the peer will have forgotten about the channel entirely anyway.

Thanks for the feedback! I'll make sure to fix that.

ViktorTigerstrom · 2022-01-31T00:52:14Z

I've now updated the PR to hopefully resolve the issues raised in the feedback. I can of course squash the 2 commits into 1 if that's more suitable.

TheBlueMatt

Basically looks good.

TheBlueMatt · 2022-01-31T22:27:24Z

lightning/src/util/events.rs

@@ -515,6 +540,11 @@ impl Writeable for Event {
 					(2, payment_hash, required),
 				})
 			},
+			&Event::OpenChannelRequest { .. } => {
+				16u8.write(writer)?;


This needs to be odd - our read behavior is "even, and we dont understand it -> assume we cant read anything and it is an important event, refuse to deserialize at all", but here its an unimportant (so unimportant we want to throw it away) event, so we should use an odd value to indicate that. We should also add a comment on the read-side code noting that the value we use is used for OpenChannelRequest (maybe a match branch).

Thanks again for the detailed input! I'll assign it to 17 instead.

TheBlueMatt · 2022-01-31T22:28:02Z

lightning/src/util/events.rs

+		chain_hash: BlockHash,
+		/// The channel value of the requested channel.
+		funding_satoshis: u64,
+		/// The amount to push to the counterparty if the channel request is accepted, in milli-satoshi.


That's us! Maybe say something like "the amount the counterparty will give us if the channel is opened" or talk about it as "our starting balance"

Oh my bad, should have known that!

TheBlueMatt · 2022-01-31T22:31:55Z

lightning/src/ln/channelmanager.rs

+	/// The `temporary_channel_id` parameter indicates which channel the reponse is for.
+	///
+	/// [`events::Event::OpenChannelRequest`]: crate::util::events::Event::OpenChannelRequest
+	pub fn respond_to_open_channel_request(&self, accept_channel: bool, temporary_channel_id: [u8; 32]) -> Result<(), APIError> {


nit: I'd kinda prefer an API of accept_inbound_channel with users instead calling force_close_channel to reject the channel, that way our code stays consistent and we use the same codepaths for everything.

That makes a lot of sense! I'll change to that.

TheBlueMatt · 2022-01-31T22:39:45Z

lightning/src/ln/channelmanager.rs

+	pub fn respond_to_open_channel_request(&self, accept_channel: bool, temporary_channel_id: [u8; 32]) -> Result<(), APIError> {
+		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
+
+		if let Some(counterparty_node_id) = self.internal_get_counterparty_node_id(temporary_channel_id) {


This is fundamentally race-y - you take the lock in internal_get_counterparty_node_id, check if the channel is there, and then release the lock, then go into accept_requested_channel and take the lock again. Anyway, this will be easy to remove with the above suggestion.

Thanks, I'll fix it through the above suggestion.

ViktorTigerstrom · 2022-02-01T22:27:35Z

Updated the PR once more, to address the latest feedback. Also squashed all commits as they were becoming unfocused.

TheBlueMatt

LGTM. Could really use a test (eg take a look at some of the simpler functional tests in lightning/src/ln/functional_tests.rs), but if you don't have time to get around to that I'll be happy to take a look.

TheBlueMatt · 2022-02-02T20:42:18Z

lightning/src/util/config.rs

@@ -305,6 +305,9 @@ pub struct UserConfig {
 	/// If this is set to false, we do not accept inbound requests to open a new channel.
 	/// Default value: true.
 	pub accept_inbound_channels: bool,
+	/// If this is set to true, the user needs to manually accept inbound requests to open a new channel.
+	/// Default value: false.


nit: Would be good to at least link to Event::OpenChannelRequest here.

Sure, I'll add that.

valentinewallace

One question about a field but otherwise ACK with testing either here or in follow-up

valentinewallace · 2022-02-02T21:38:55Z

lightning/src/util/events.rs

+		/// The genesis hash of the blockchain where the channel is requested to be opened.
+		chain_hash: BlockHash,


I wonder if this is a useful field? It seems a bit extraneous to me

Yea, I guess not, we don't currently support multi-chain operation so the chain hash has to match what we are on anyway. ie its always the same value by the time it gets here. I'd say remove it.

Great ok, I'll remove the field.

valentinewallace · 2022-02-02T21:44:00Z

lightning/src/ln/channel.rs

@@ -1918,6 +1933,10 @@ impl<Signer: Sign> Channel<Signer> {
 			// channel.
 			return Err(ChannelError::Close("Received funding_created after we got the channel!".to_owned()));
 		}
+		if self.inbound_awaiting_accept {
+			// The Channel must manually accepted before the FundingCreated is received.


nit: must *be manually accepted

I think we can get rid of this comment though

I agree, I'll get rid of it.

valentinewallace · 2022-02-02T21:44:24Z

lightning/src/ln/channel.rs

@@ -536,6 +536,17 @@ pub(super) struct Channel<Signer: Sign> {
 	#[cfg(not(test))]
 	closing_fee_limits: Option<(u64, u64)>,

+	/// Flag that ensures that the `get_accept_channel` function must be called before the
+	/// `funding_created` function is executed successfully. The reason for need of this flag is


nit: s/need of//

ViktorTigerstrom · 2022-02-02T23:40:32Z

Great, thanks a lot to both of you! Sure I'll look into implementing some tests. Unfortunately I've got quite much in the upcoming days, but I'll try to get it done during weekend. I could also look into implementing support for the feature in the ldk-sample repo afterwards if that's helpful.

TheBlueMatt · 2022-02-02T23:52:11Z

Great, thanks a lot to both of you! Sure I'll look into implementing some tests. Unfortunately I've got quite much in the upcoming days, but I'll try to get it done during weekend.

No rush at all. If you have the time to address the other comments and squash the commits here, that'd be appreciated too. I'm working on 0conf support which will likely be based on this, so I may just write a test for you and we can merge it without, depending on timing.

ViktorTigerstrom · 2022-02-03T00:50:31Z

No rush at all. If you have the time to address the other comments and squash the commits here, that'd be appreciated too. I'm working on 0conf support which will likely be based on this, so I may just write a test for you and we can merge it without, depending on timing.

Ok sounds great! Just pushed the suggested changes.

ViktorTigerstrom · 2022-02-08T00:23:26Z

Pushed some tests of the feature if you haven't had time to create some already. I made one test for accepting an inbound request, and one for rejecting a request.

I also wanted to make a test for the case where the counterparty node tries to be smart and send a FundingCreated msg before the channel has been accepted, but that proved to be more difficult to implement so I'll try to get that implemented at a later time.

I pushed the tests in a separate commit, but I can of course squash them if that's more suitable.

TheBlueMatt · 2022-02-08T16:00:41Z

Nice! Those tests look great. Indeed, it'd be nice to see the channel get closed if the counterparty sends a funding_generated (I don't think we need to worry about funding_created, we need funding_generated first) before we send the accept_channel.

ViktorTigerstrom · 2022-02-08T20:51:57Z

Nice! Those tests look great. Indeed, it'd be nice to see the channel get closed if the counterparty sends a funding_generated (I don't think we need to worry about funding_created, we need funding_generated first) before we send the accept_channel.

Thanks! Great, thanks for that info.

ViktorTigerstrom · 2022-02-09T18:34:22Z

Just pushed a draft of the test that rejects the msg before the channel was manually accepted. I'm not really satisfied with the solution though, so if I've misunderstood how to test the case, or if there's a simpler way to do it please let me know.

The key issue with my solution is that I need to extract the AcceptChannel msg that nodes[1] would send with the MessageSendEvent::SendAcceptChannel event, if the channel is accepted. This is because it's required that nodes[0] has called handle_accept_channel before the create_funding_transaction is called with nodes[0] passed. If that flow is altered, create_funding_transaction will not succeed. This forced me to create a new function Channel::set_is_awaiting_accept just for this test, that resets the Channel::inbound_awaiting_accept value, as extracting the AcceptChannel message sets the Channel::inbound_awaiting_accept flag to false. As the inbound_awaiting_accept flag is an internal private flag that controls the internal logic flow in the Channel struct, exposing a public function that mutates that value isn't great...

All of this is done, just for the purpose of generating the funding transaction for nodes[0], and use the tx to generate the FundingCreated message. Obviously this is isn't necessarily required by other implementations to generate the FundingCreated message, but it was the only simple way I could find with our implementation.

If I've completely misunderstood the flow, or if there's any simpler way to generate the funding transaction that requires less prerequisites, I'd be really thankful for that feedback!

TheBlueMatt · 2022-02-09T19:14:23Z

lightning/src/ln/functional_tests.rs

+	// function, which is required in order for the create_funding_transaction function not to
+	// crash when nodes[0] is passed to it.
+	{
+		let mut channel_state_lock = nodes[1].node.channel_state.lock().unwrap();


Note you can use the get_channel_ref macro here to do most of the work.

Great thanks! Hadn't found that one.

TheBlueMatt · 2022-02-09T19:15:58Z

lightning/src/ln/channel.rs

+		self.inbound_awaiting_accept
+	}
+
+	pub fn set_is_awaiting_accept(&mut self) {


This is generally fine with me, actually, but we should make this #[cfg(test)]. An alternative way to accomplish the same test would be to split get_accept_channel into an utility to get the message itself and another pub method to get the message while setting the awaiting flag.

ViktorTigerstrom · 2022-02-10T20:27:43Z

Thanks for the feedback! Pushed some changes to address it. I chose to split get_accept_channel into a utility. Please check that we're ok with exposing a function that explicitly generates an AcceptChannel msg without doing sanity checks of the channel state and such. I added the splitting in the 59a4ac69 commit, but will squash it with the first commit if it looks good.

Also added two more test I thought of related to the manually accepting of inbound channels feature.

ViktorTigerstrom · 2022-02-10T21:04:50Z

Fixed crate error. My bad!

valentinewallace

Testing looks super solid! Just nits here, if no further feedback I think it'd be fine to just merge

valentinewallace · 2022-02-11T22:41:34Z

lightning/src/ln/functional_tests.rs

+	// Get the [`AcceptChannel`] message of [`nodes[1]`] without calling the
+	// [`ChannelManager::accept_inbound_channel`] function, which generates the
+	// [`MessageSendEvent::SendAcceptChannel`] event. This is passed to [`nodes[0]`]
+	// [`handle_accept_channel`] function, which is required in order for the
+	// [`create_funding_transaction`] function to succeed when [`nodes[0]`] is passed to it.


Can remove the surrounding braces from all of these and elsewhere in the tests, I think they only have meaning when used in docs =]

Oh, I was quite unsure how to deal with those. Thanks a lot for clarifying!

valentinewallace · 2022-02-11T22:46:24Z

lightning/src/ln/channelmanager.rs

@@ -4101,6 +4101,34 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		}
 	}

+	/// Called to accept a request to open a channel after the
+	/// [`events::Event::OpenChannelRequest`] has been triggered.


nit: s/[events::Event::OpenChannelRequest]/[Event::OpenChannelRequest] + link update

valentinewallace · 2022-02-11T22:52:15Z

lightning/src/ln/channel.rs

@@ -536,6 +536,17 @@ pub(super) struct Channel<Signer: Sign> {
 	#[cfg(not(test))]
 	closing_fee_limits: Option<(u64, u64)>,

+	/// Flag that ensures that the `get_accept_channel` function must be called before the
+	/// `funding_created` function is executed successfully. The reason for this flag is that
+	/// when the util::config::UserConfig.manually_accept_inbound_channels is set to true,


nit: s/util::config::UserConfig.manually_accept_inbound_channels/UserConfig::manually_accept_inbound_channels

valentinewallace · 2022-02-11T22:59:08Z

lightning/src/ln/channel.rs

+	/// Flag that ensures that the `accept_inbound_channel` function must be called before the
+	/// `funding_created` function is executed successfully. The reason for this flag is that


nit: s/the accept_inbound_channel function/accept_inbound_channel and similar for funding_created and elsewhere

Makes sense, thanks!

valentinewallace · 2022-02-11T23:00:17Z

lightning/src/ln/channel.rs

+
+		self.inbound_awaiting_accept = false;
+
+		return self.get_accept_channel_message();


nit: s/return self.get_accept_channel_message();/self.get_accept_channel_message()

valentinewallace · 2022-02-11T23:11:00Z

lightning/src/ln/channel.rs

+	/// [`Channel::accept_inbound_channel`] function instead.
+	///
+	/// [`msgs::AcceptChannel`]: crate::ln::msgs::AcceptChannel
+	pub fn get_accept_channel_message(&self) -> msgs::AcceptChannel {


Don't think it matters too much but we could make this pub just in testing

Yea, I'd strongly prefer this be a private method with a #[cfg(test)] method that exposes it just for testing.

Ok sure, I'll make it private and expose a function for testing!

TheBlueMatt · 2022-02-12T16:07:25Z

Needs rebase, sorry about that.

Add a new config flag `UserConfig::manually_accept_inbound_channels`, which when set to true allows the node operator to accept or reject new channel requests. When set to true, `Event::OpenChannelRequest` will be triggered once a request to open a new inbound channel is received. When accepting the request, `ChannelManager::accept_inbound_channel` should be called. Rejecting the request is done through `ChannelManager::force_close_channel`.

Add functional tests for manually responding to inbound channel requests. Responding to inbound channel requests are required when the `manually_accept_inbound_channels` config flag is set to true. The tests cover the following cases: * Accepting an inbound channel request * Rejecting an inbound channel request * FundingCreated message sent by the counterparty before accepting the inbound channel request * Attempting to accept an inbound channel request twice * Attempting to accept an unkown inbound channel

ViktorTigerstrom · 2022-02-13T20:21:03Z

Thanks again for all the feedback! Just pushed some changes which I hope addresses all of it.

TheBlueMatt reviewed Jan 26, 2022

View reviewed changes

TheBlueMatt reviewed Jan 31, 2022

View reviewed changes

ViktorTigerstrom force-pushed the 2022-01-accept-or-reject-channels branch from b4d14ae to 8d20adc Compare February 1, 2022 22:23

TheBlueMatt previously approved these changes Feb 2, 2022

View reviewed changes

valentinewallace reviewed Feb 2, 2022

View reviewed changes

ViktorTigerstrom dismissed TheBlueMatt’s stale review via f38f2d5 February 3, 2022 00:45

ViktorTigerstrom force-pushed the 2022-01-accept-or-reject-channels branch from 8d20adc to f38f2d5 Compare February 3, 2022 00:45

ViktorTigerstrom force-pushed the 2022-01-accept-or-reject-channels branch from f38f2d5 to 8c30b59 Compare February 8, 2022 00:14

TheBlueMatt reviewed Feb 9, 2022

View reviewed changes

ViktorTigerstrom force-pushed the 2022-01-accept-or-reject-channels branch from df4c3dc to 5b739eb Compare February 10, 2022 20:26

ViktorTigerstrom force-pushed the 2022-01-accept-or-reject-channels branch from 5b739eb to 6517c62 Compare February 10, 2022 21:02

ViktorTigerstrom force-pushed the 2022-01-accept-or-reject-channels branch from 6517c62 to dbe587c Compare February 10, 2022 21:27

valentinewallace previously approved these changes Feb 11, 2022

View reviewed changes

ViktorTigerstrom added 2 commits February 13, 2022 21:04

ViktorTigerstrom dismissed valentinewallace’s stale review via 1891b37 February 13, 2022 20:19

ViktorTigerstrom force-pushed the 2022-01-accept-or-reject-channels branch from dbe587c to 1891b37 Compare February 13, 2022 20:19

valentinewallace approved these changes Feb 14, 2022

View reviewed changes

TheBlueMatt approved these changes Feb 14, 2022

View reviewed changes

TheBlueMatt merged commit 0df247d into lightningdevkit:main Feb 14, 2022

TheBlueMatt mentioned this pull request May 26, 2022

Add The ability to manually accept/reject each channel #1242

Closed

shamardy mentioned this pull request Jul 25, 2022

[r2r] Lightning batch transactions confirmations, spv (wip), refactors KomodoPlatform/komodo-defi-framework#1339

Merged

		/// The genesis hash of the blockchain where the channel is requested to be opened.
		chain_hash: BlockHash,

		/// Flag that ensures that the `accept_inbound_channel` function must be called before the
		/// `funding_created` function is executed successfully. The reason for this flag is that


		self.inbound_awaiting_accept = false;

		return self.get_accept_channel_message();

Add option accept or reject channel requests #1281

Add option accept or reject channel requests #1281

Uh oh!

Conversation

ViktorTigerstrom commented Jan 26, 2022

Uh oh!

codecov-commenter commented Jan 26, 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!

ViktorTigerstrom commented Jan 31, 2022

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!

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!

ViktorTigerstrom commented Feb 1, 2022

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!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Feb 2, 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!

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!

ViktorTigerstrom commented Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Feb 2, 2022

Uh oh!

ViktorTigerstrom commented Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ViktorTigerstrom commented Feb 8, 2022

codecov-commenter commented Jan 26, 2022 •

edited

Loading

TheBlueMatt Feb 2, 2022 •

edited

Loading

ViktorTigerstrom commented Feb 2, 2022 •

edited

Loading

ViktorTigerstrom commented Feb 3, 2022 •

edited

Loading

ViktorTigerstrom commented Feb 9, 2022 •

edited

Loading

ViktorTigerstrom commented Feb 10, 2022 •

edited

Loading

ViktorTigerstrom commented Feb 10, 2022 •

edited

Loading

ViktorTigerstrom Feb 13, 2022 •

edited

Loading