Disconnect announcement_signatures sending from funding_locked #1179

TheBlueMatt · 2021-11-19T03:12:19Z

The spec actually requires we never send announcement_signatures
(and, thus, channel_announcements) until after six confirmations.
However, we would happily have sent them prior to that as long as
we exchange funding_locked messages with our countarparty. Thanks
to re-broadcasting this issue is largely harmless, however it could
have some negative interactions with less-robust peers. Much more
importantly, this represents an important step towards supporting
0-conf channels, where funding_locked messages may be exchanged
before we even have an SCID to construct the messages with.

Because there is no ACK mechanism for announcement_signatures we
rely on existing channel updates to stop rebroadcasting them - if
we sent a commitment_signed after an announcement_signatures
and later receive a revoke_and_ack, we know our counterparty also
received our announcement_signatures. This may resolve some rare
edge-cases where we send a funding_locked which our counterparty
receives, but lose connection before the announcement_signatures
(usually the very next message) arrives.

Sadly, because the set of places where an announcement_signatures
may now be generated more closely mirrors where funding_locked
messages may be generated, but they are now separate, there is a
substantial amount of code motion providing relevant parameters
about current block information and ensuring we can return new
announcement_signatures messages.

codecov · 2021-11-19T03:37:57Z

Codecov Report

Merging #1179 (cc7a03a) into main (8d886ee) will increase coverage by 0.01%.
The diff coverage is 89.05%.

@@            Coverage Diff             @@
##             main    #1179      +/-   ##
==========================================
+ Coverage   90.36%   90.38%   +0.01%     
==========================================
  Files          69       69              
  Lines       36794    36980     +186     
==========================================
+ Hits        33249    33424     +175     
- Misses       3545     3556      +11

Impacted Files	Coverage Δ
lightning/src/ln/functional_test_utils.rs	`95.06% <78.57%> (-0.07%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.19% <84.09%> (-0.18%)`	⬇️
lightning/src/ln/channelmanager.rs	`83.94% <85.10%> (+0.25%)`	⬆️
lightning/src/chain/keysinterface.rs	`95.18% <87.50%> (+0.04%)`	⬆️
lightning/src/ln/channel.rs	`88.55% <92.10%> (+0.13%)`	⬆️
lightning/src/chain/channelmonitor.rs	`91.12% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.67% <100.00%> (+0.02%)`	⬆️
lightning/src/util/enforcing_trait_impls.rs	`89.38% <100.00%> (ø)`
lightning/src/util/test_utils.rs	`81.96% <100.00%> (+0.04%)`	⬆️
... 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 8d886ee...cc7a03a. Read the comment docs.

lightning/src/ln/channel.rs

valentinewallace · 2021-12-06T20:52:42Z

lightning/src/ln/channelmanager.rs

@@ -3604,8 +3596,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				return;
 			}

-			let updates = channel.get_mut().monitor_updating_restored(&self.logger);
-			let channel_update = if updates.funding_locked.is_some() && channel.get().is_usable() && !channel.get().should_announce() {


I think since we dropped the !channel.get().should_announce() (why did we drop that?) the comment below needs amending (i.e. s/only generating a unicast channel_update if this is a private channel//)

Yep, good point. Updated the comment. There's not really a good reason for or against sending it twice here, its kinda nice to just send it and remove the check to simplify things down, though, IMO.

lightning/src/ln/channelmanager.rs

lightning/src/ln/functional_tests.rs

valentinewallace

Shaping up!

valentinewallace · 2021-12-08T18:11:54Z

lightning/src/ln/channel.rs

+			let announcement_sigs = if let Some((genesis_block_hash, node_pk)) = genesis_node_pk {
+				self.get_announcement_sigs(node_pk, genesis_block_hash, height, logger)
+			} else { None };


To simplify, think you can move this call to get_announcement_sigs outside the if let Some(funding_locked) .. and get rid of the second call below

get_announcement_sigs updates our "message sent" state, so we really can't call it unless we're definitely about to return Ok.

valentinewallace · 2021-12-08T18:17:17Z

lightning/src/ln/channel.rs

@@ -4182,7 +4296,8 @@ impl<Signer: Sign> Channel<Signer> {
 					// may have already happened for this block).
 					if let Some(funding_locked) = self.check_get_funding_locked(height) {
 						log_info!(logger, "Sending a funding_locked to our peer for channel {}", log_bytes!(self.channel_id));
-						return Ok(Some(funding_locked));
+						let announcement_sigs = self.get_announcement_sigs(node_pk, genesis_block_hash, height, logger);


To me it'd make more sense if we always called get_announcement_sigs at the beginning and always return its result (i.e. we'd also return its result at L4311, not just here)

check_get_funding_locked updates our local state flags, though, so it has to come after that.

valentinewallace · 2021-12-08T18:45:07Z

lightning/src/ln/channel.rs

@@ -3192,6 +3252,8 @@ impl<Signer: Sign> Channel<Signer> {
 			})


Preexisting, but I think it'd overall be nicer if we replaced each time we construct msgs::FundingLocked (e.g. 4 lines up and elsewhere) with a call to check_get_funding_locked(..). Then that method could be responsible for setting self.monitor_pending_funding_locked = false, and it's DRYer/more encapsulated

check_get_funding_locked is a bit of a misnomer - it updates state and isn't really just a getter, its really just part of the block/transaction-connection logic refactored out into a poorly-named function :/

TheBlueMatt · 2022-01-20T21:28:13Z

Squashed.

This removes one more place where we directly access the node_id secret key in `ChannelManager`, slowly marching towards allowing the node_id secret key to be offline in the signer. More importantly, it allows more ChannelAnnouncement logic to move into the `Channel` without having to pass the node secret key around, avoiding the announcement logic being split across two files.

TheBlueMatt · 2022-01-25T18:27:06Z

Rebased to update the TLV numbering due to conflicts.

lightning/src/ln/channel.rs

arik-so · 2022-01-26T07:09:35Z

lightning/src/ln/channel.rs

-	/// for both loose and in response to an AnnouncementSignatures message from the remote peer.
+	/// Gets an UnsignedChannelAnnouncement for this channel. The channel must be publicly
+	/// announceable and available for use (have exchanged FundingLocked messages in both
+	/// directions). Should be used for both loose and in response to an AnnouncementSignatures


what does "should be used for loose" mean?

Was this resolved...?

I reworked the documentation to no longer say "loose".

Ah, it looks like the updated comment was put in ed1163a

codecov-commenter · 2022-01-26T18:19:43Z

Codecov Report

Merging #1179 (c4b22c3) into main (1a24dcc) will increase coverage by 0.03%.
The diff coverage is 92.10%.

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

@@            Coverage Diff             @@
##             main    #1179      +/-   ##
==========================================
+ Coverage   90.38%   90.41%   +0.03%     
==========================================
  Files          71       70       -1     
  Lines       38182    38315     +133     
==========================================
+ Hits        34509    34643     +134     
+ Misses       3673     3672       -1

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	`84.49% <85.29%> (+0.26%)`	⬆️
lightning/src/chain/keysinterface.rs	`95.79% <85.71%> (-0.03%)`	⬇️
lightning/src/ln/channel.rs	`89.42% <90.71%> (+0.24%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.17% <93.90%> (-0.16%)`	⬇️
lightning/src/chain/channelmonitor.rs	`91.56% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.72% <100.00%> (+0.02%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`95.22% <100.00%> (-0.08%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`89.47% <100.00%> (ø)`
lightning/src/util/test_utils.rs	`82.26% <100.00%> (+0.04%)`	⬆️
lightning-background-processor/src/lib.rs	`92.54% <0.00%> (-0.50%)`	⬇️
... and 12 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 1a24dcc...ed1163a. Read the comment docs.

This improves readability and makes it easier to add additional return fields.

While its generally harmless to do so (the messages will simply be dropped in `PeerManager`) there is a potential race condition where the FundingLocked message enters the outbound message queue, then the peer reconnects, and then the FundingLocked message is delivered prior to the normal ChannelReestablish flow. We also take this opportunity to rewrite `test_funding_peer_disconnect` to be explicit instead of using `reconnect_peers`. This allows it to check each message being sent carefully, whereas `reconnect_peers` is rather lazy and accepts that sometimes signatures will be exchanged, and sometimes not.

If we have not yet sent `funding_locked` only because of a pending channel monitor update, we shouldn't consider a channel `is_usable`. This has a number of downstream effects, including not attempting to route payments through the channel, not sending private `channel_update` messages to our counterparty, or sending channel_announcement messages if our couterparty has already signed for it. We further gate generation of `node_announcement`s on `is_usable`, preventing generation of those or `announcement_signatures` until we've sent our `funding_locked`. Finally, `during_funding_monitor_fail` is updated to test a case where we see the funding transaction lock in but have a pending monitor update failure, then receive `funding_locked` from our counterparty and ensure we don't generate the above messages until after the monitor update completes.

The spec actually requires we never send `announcement_signatures` (and, thus, `channel_announcement`s) until after six confirmations. However, we would happily have sent them prior to that as long as we exchange `funding_locked` messages with our countarparty. Thanks to re-broadcasting this issue is largely harmless, however it could have some negative interactions with less-robust peers. Much more importantly, this represents an important step towards supporting 0-conf channels, where `funding_locked` messages may be exchanged before we even have an SCID to construct the messages with. Because there is no ACK mechanism for `announcement_signatures` we rely on existing channel updates to stop rebroadcasting them - if we sent a `commitment_signed` after an `announcement_signatures` and later receive a `revoke_and_ack`, we know our counterparty also received our `announcement_signatures`. This may resolve some rare edge-cases where we send a `funding_locked` which our counterparty receives, but lose connection before the `announcement_signatures` (usually the very next message) arrives. Sadly, because the set of places where an `announcement_signatures` may now be generated more closely mirrors where `funding_locked` messages may be generated, but they are now separate, there is a substantial amount of code motion providing relevant parameters about current block information and ensuring we can return new `announcement_signatures` messages.

Channel::get_announcement_sigs is only used in contexts where we have a logger already, and the error returned is always ignored, so instead of returning an ignored error message we return an `Option` directly and log when it won't be too verbose.

arik-so

re-ACKing

arik-so · 2022-01-26T20:15:39Z

One aside: it's "Disconnect" with two _n_s.

valentinewallace

Some nits. ACK ed1163a

valentinewallace · 2022-01-26T19:15:33Z

lightning/src/ln/channel.rs

 	/// Will only fail if we're not in a state where channel_announcement may be sent (including
 	/// closing).
+	///
 	/// Note that the "channel must be funded" requirement is stricter than BOLT 7 requires - see
 	/// https://github.com/lightningnetwork/lightning-rfc/issues/468


This issue is closed, does this need updating?

Will fix in a followup PR, since this has two acks.

valentinewallace · 2022-01-26T20:36:46Z

lightning/src/ln/channel.rs

-	/// for both loose and in response to an AnnouncementSignatures message from the remote peer.
+	/// Gets an UnsignedChannelAnnouncement for this channel. The channel must be publicly
+	/// announceable and available for use (have exchanged FundingLocked messages in both
+	/// directions). Should be used for both loose and in response to an AnnouncementSignatures


Ah, it looks like the updated comment was put in ed1163a

TheBlueMatt force-pushed the 2021-11-fix-announce-sigs-broadcast-time branch 2 times, most recently from 76e84b4 to cc7a03a Compare November 19, 2021 04:33

TheBlueMatt added the Seeking Code Review label Nov 22, 2021

valentinewallace self-requested a review December 3, 2021 19:02

valentinewallace reviewed Dec 6, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-11-fix-announce-sigs-broadcast-time branch from cc7a03a to f4d944f Compare December 7, 2021 18:03

valentinewallace reviewed Dec 8, 2021

View reviewed changes

jkczyz self-requested a review January 20, 2022 21:26

TheBlueMatt force-pushed the 2021-11-fix-announce-sigs-broadcast-time branch from 94243a8 to c4b22c3 Compare January 20, 2022 21:28

TheBlueMatt force-pushed the 2021-11-fix-announce-sigs-broadcast-time branch from c4b22c3 to a11bee9 Compare January 25, 2022 18:26

arik-so previously approved these changes Jan 26, 2022

View reviewed changes

TheBlueMatt dismissed arik-so’s stale review via 91b7b1f January 26, 2022 18:18

TheBlueMatt force-pushed the 2021-11-fix-announce-sigs-broadcast-time branch from a11bee9 to 91b7b1f Compare January 26, 2022 18:18

TheBlueMatt added 6 commits January 26, 2022 18:20

Return struct, not long tuple, from Channel::channel_reestablish

a6ddb97

This improves readability and makes it easier to add additional return fields.

Swap loop and condition order to avoid looping unnecessarily

ee7cfa5

TheBlueMatt force-pushed the 2021-11-fix-announce-sigs-broadcast-time branch from 91b7b1f to ed1163a Compare January 26, 2022 18:20

arik-so approved these changes Jan 26, 2022

View reviewed changes

TheBlueMatt changed the title ~~Disconect announcement_signatures sending from funding_locked~~ Disconnect announcement_signatures sending from funding_locked Jan 26, 2022

valentinewallace approved these changes Jan 26, 2022

View reviewed changes

TheBlueMatt merged commit 457e48e into lightningdevkit:main Jan 26, 2022

TheBlueMatt mentioned this pull request Jan 26, 2022

Remove stale reference to incomplete BOLT compliance #1285

Merged

		@@ -3192,6 +3252,8 @@ impl<Signer: Sign> Channel<Signer> {
		})

Disconnect announcement_signatures sending from funding_locked #1179

Disconnect announcement_signatures sending from funding_locked #1179

Uh oh!

Conversation

TheBlueMatt commented Nov 19, 2021

Uh oh!

codecov bot commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

valentinewallace 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!

TheBlueMatt commented Jan 20, 2022

Uh oh!

TheBlueMatt commented Jan 25, 2022

Uh oh!

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!

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!

arik-so left a comment

Choose a reason for hiding this comment

Uh oh!

arik-so commented Jan 26, 2022

Uh oh!

valentinewallace 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!

Uh oh!

codecov bot commented Nov 19, 2021 •

edited

Loading

codecov-commenter commented Jan 26, 2022 •

edited

Loading