Workaround lnd sending funding_locked before channel_reestablish #966

TheBlueMatt · 2021-06-23T16:41:12Z

lnd has a long-standing bug where, upon reconnection, if the
channel is not yet confirmed they will not send a
channel_reestablish until the channel locks in. Then, they will
send a funding_locked before sending the channel_reestablish
(which is clearly a violation of the BOLT specs). We copy
c-lightning's workaround here and simply store the funding_locked
message until we receive a channel_reestablish.

See-also lightningnetwork/lnd#4006

Fixes #963

Should work but needs tests.

codecov · 2021-06-23T16:57:54Z

Codecov Report

Merging #966 (02a0ba6) into main (073afbb) will increase coverage by 0.19%.
The diff coverage is 100.00%.

❗ Current head 02a0ba6 differs from pull request most recent head 8df1412. Consider uploading reports for the commit 8df1412 to get more accurate results

@@            Coverage Diff             @@
##             main     #966      +/-   ##
==========================================
+ Coverage   90.66%   90.85%   +0.19%     
==========================================
  Files          60       60              
  Lines       30407    31939    +1532     
==========================================
+ Hits        27568    29019    +1451     
- Misses       2839     2920      +81

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	`88.34% <100.00%> (+0.05%)`	⬆️
lightning/src/ln/channelmanager.rs	`84.49% <100.00%> (+0.63%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.18% <100.00%> (+0.01%)`	⬆️
lightning/src/routing/network_graph.rs	`91.96% <0.00%> (-0.01%)`	⬇️
lightning/src/util/events.rs	`17.46% <0.00%> (+0.16%)`	⬆️
lightning/src/util/ser_macros.rs	`97.34% <0.00%> (+0.59%)`	⬆️
lightning/src/chain/package.rs	`93.04% <0.00%> (+0.74%)`	⬆️
lightning/src/ln/chan_utils.rs	`98.23% <0.00%> (+0.79%)`	⬆️
... and 2 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 073afbb...8df1412. Read the comment docs.

TheBlueMatt · 2021-06-24T20:40:49Z

Added a test for the behavior.

lightning/src/ln/channel.rs

jkczyz · 2021-06-25T18:08:10Z

lightning/src/ln/functional_tests.rs

-	do_test_drop_messages_peer_disconnect(4);
-	do_test_drop_messages_peer_disconnect(5);
-	do_test_drop_messages_peer_disconnect(6);
+	do_test_drop_messages_peer_disconnect(3, false);


Any reason why this was moved here?

To keep the same number of test runs in each test.

jkczyz · 2021-06-25T18:12:31Z

lightning/src/ln/channel.rs

+	/// message until we receive a channel_reestablish.
+	///
+	/// See-also <https://github.com/lightningnetwork/lnd/issues/4006>
+	pub workaround_lnd_bug_4006: Option<msgs::FundingLocked>,


Does this need to be persisted in case we crash before getting channel_reestablish? Or will LND resend funding_locked?

I assume they will resend it, ultimately there's no such thing as knowing for sure that a message was delivered, so they need to support re-sending for lnd <-> lnd to work, which hopefully it does.

jkczyz · 2021-06-25T18:13:50Z

lightning/src/ln/channel.rs

+	/// message until we receive a channel_reestablish.
+	///
+	/// See-also <https://github.com/lightningnetwork/lnd/issues/4006>
+	pub workaround_lnd_bug_4006: Option<msgs::FundingLocked>,


The ChannelManager accesses it directly on L3389 where it takes from the option and sends it back in as a new message.

Ah, right Channel is not pub so no need to make it pub(crate).

jkczyz · 2021-06-25T18:20:58Z

lightning/src/ln/channel.rs

+			self.workaround_lnd_bug_4006 = Some(msg.clone());
+			return Err(ChannelError::Ignore("Peer sent funding_locked when we needed a channel_reestablish. The peer is likely lnd, see their bug #4006.".to_owned()));


Won't this mean any implementation of ChannelMessageHandler needs to implement this workaround. Not sure if there is a better way, though. Maybe cache it at the peer handling layer using a wrapper around ChannelMessageHandler? Might not be feasible if you need the channel state. :/

Yes, if you implement a whole new channel implementation then you would, but I'm not sure how likely that is. You wouldn't need to if you were implementing ChannelMessageHandler as a simple wrapper around our ChannelManager (which I do expect people to do).

lightning/src/ln/functional_tests.rs

ccdle12 · 2021-06-25T20:00:19Z

lightning/src/ln/functional_tests.rs

+			// in `reconnect_nodes` but we currently don't fail based on that.
+			//
+			// See-also <https://github.com/lightningnetwork/lnd/issues/4006>
+			nodes[1].node.handle_funding_locked(&nodes[0].node.get_our_node_id(), &as_funding_locked.as_ref().unwrap().0);


noob question: Is it worth getting the channel_id and looking up the Channel and assert workaround_lnd_bug_4006 is Some? or is it because we don't fail this check anymore, it implicitly passes?

In general I think we've tried to avoid (though there are a number of places where we do) writing functional tests where we read non-public/internal datastructures, instead opting for completely black-box testing (at least in functional tests). Ultimately, it doesn't matter a whole lot how ChannelManager and Channel handle this case, as long as they successfully do.

lnd has a long-standing bug where, upon reconnection, if the channel is not yet confirmed they will not send a channel_reestablish until the channel locks in. Then, they will send a funding_locked *before* sending the channel_reestablish (which is clearly a violation of the BOLT specs). We copy c-lightning's workaround here and simply store the funding_locked message until we receive a channel_reestablish. See-also lightningnetwork/lnd#4006 Fixes lightningdevkit#963

TheBlueMatt · 2021-06-28T02:06:01Z

Squashed the fixup commits with no diff:

$ git diff-tree -U1 02a0ba66 8df14123
$

valentinewallace · 2021-06-28T22:26:42Z

lightning/src/ln/functional_tests.rs

+			//
+			// See-also <https://github.com/lightningnetwork/lnd/issues/4006>
+			nodes[1].node.handle_funding_locked(&nodes[0].node.get_our_node_id(), &as_funding_locked.as_ref().unwrap().0);
+		}
 		// Even if the funding_locked messages get exchanged, as long as nothing further was
 		// received on either side, both sides will need to resend them.
 		reconnect_nodes(&nodes[0], &nodes[1], (true, true), (0, 1), (0, 0), (0, 0), (0, 0), (false, false));


Just a Q: since send_funding_locked is (true, true) here, wouldn't that result in a second nodes[0] funding_locked being sent? Is that similar enough to the lnd scenario we're trying to test? 🤔

Indeed, it will, I think its close enough, but I'm happy to rework if you disagree, its just a good chunk more special-case code here.

I think it's OK. The test does fail with the previous code

ariard

utACK 8df1412

ariard · 2021-06-28T23:21:53Z

lightning/src/ln/channel.rs

+	/// lnd has a long-standing bug where, upon reconnection, if the channel is not yet confirmed
+	/// they will not send a channel_reestablish until the channel locks in. Then, they will send a
+	/// funding_locked *before* sending the channel_reestablish (which is clearly a violation of
+	/// the BOLT specs). We copy c-lightning's workaround here and simply store the funding_locked


A node: if next_commitment_number is 1 in both the channel_reestablish it sent and received: MUST retransmit funding_locked. otherwise: MUST NOT retransmit funding_locked.

I agree, it's pretty clear, though another fix for them would be to rebroadcast a redundant funding_locked after channel_reestablish.

Hmm, no, if they don't know what is in channel_reestablish they "MUST NOT retransmit funding_locked." I don't think they could do that either.

valentinewallace · 2021-06-29T16:21:54Z

lightning/src/ln/functional_tests.rs

+			//
+			// See-also <https://github.com/lightningnetwork/lnd/issues/4006>
+			nodes[1].node.handle_funding_locked(&nodes[0].node.get_our_node_id(), &as_funding_locked.as_ref().unwrap().0);
+		}
 		// Even if the funding_locked messages get exchanged, as long as nothing further was
 		// received on either side, both sides will need to resend them.
 		reconnect_nodes(&nodes[0], &nodes[1], (true, true), (0, 1), (0, 0), (0, 0), (0, 0), (false, false));


I think it's OK. The test does fail with the previous code

TheBlueMatt added this to the 0.0.99 milestone Jun 23, 2021

TheBlueMatt force-pushed the 2021-06-workaround-broken-lnd branch from 51d7a5b to 35ec74f Compare June 24, 2021 20:40

TheBlueMatt marked this pull request as ready for review June 24, 2021 20:40

jkczyz reviewed Jun 25, 2021

View reviewed changes

ccdle12 reviewed Jun 25, 2021

View reviewed changes

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

jkczyz approved these changes Jun 25, 2021

View reviewed changes

ccdle12 reviewed Jun 25, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-06-workaround-broken-lnd branch from 02a0ba6 to 8df1412 Compare June 28, 2021 02:05

valentinewallace reviewed Jun 28, 2021

View reviewed changes

ariard reviewed Jun 28, 2021

View reviewed changes

valentinewallace approved these changes Jun 29, 2021

View reviewed changes

TheBlueMatt merged commit 74f1007 into lightningdevkit:main Jun 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workaround lnd sending funding_locked before channel_reestablish #966

Workaround lnd sending funding_locked before channel_reestablish #966

TheBlueMatt commented Jun 23, 2021

codecov bot commented Jun 23, 2021 •

edited

Loading

TheBlueMatt commented Jun 24, 2021

jkczyz Jun 25, 2021

TheBlueMatt Jun 25, 2021

jkczyz Jun 25, 2021

TheBlueMatt Jun 25, 2021

jkczyz Jun 25, 2021

TheBlueMatt Jun 25, 2021

jkczyz Jun 25, 2021

jkczyz Jun 25, 2021

TheBlueMatt Jun 25, 2021

ccdle12 Jun 25, 2021

TheBlueMatt Jun 25, 2021

TheBlueMatt commented Jun 28, 2021

valentinewallace Jun 28, 2021

TheBlueMatt Jun 29, 2021

valentinewallace Jun 29, 2021

ariard left a comment

ariard Jun 28, 2021

TheBlueMatt Jun 29, 2021

valentinewallace Jun 29, 2021

		self.workaround_lnd_bug_4006 = Some(msg.clone());
		return Err(ChannelError::Ignore("Peer sent funding_locked when we needed a channel_reestablish. The peer is likely lnd, see their bug #4006.".to_owned()));

Workaround lnd sending funding_locked before channel_reestablish #966

Workaround lnd sending funding_locked before channel_reestablish #966

Conversation

TheBlueMatt commented Jun 23, 2021

codecov bot commented Jun 23, 2021 • edited Loading

Codecov Report

TheBlueMatt commented Jun 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Jun 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 23, 2021 •

edited

Loading