Add flag to disable broadcasting when it's dangerous due to information loss #1593

QPotato · 2022-07-04T16:14:51Z

Work in progress PR. The functionality should be ready, but I'm still figuring out how to write tests for this. I'm opening the PR to run CI and use it as reference to ask questions about the tests on the issue.

This PR adds a new field to ChannelMonitor to track when a channel was force closed with should_broadcast: false. We then use this flag to disabled automatic braodcasting of the latest holder commitment transaction in cases where doing so could be dangerous due to missing information. In particular, adding this solves the case where the transaction would have been broadcast on deserialization of the monitor.

I also fixed some minor typos I found on docstrings while reading through the code.

Closes #1563

TheBlueMatt

Generally looks good. I do wonder why CI didn't run...sometimes GH Actions is just broken, I guess - maybe try git commit --amend && git push -f to push a new commit and see if it works. As for testing, you should be able to write a functional test to call force_close_without_broadcasting_txn and then reload the channelmanager and see if the ChannelMonitor automatically rebroadcasts (see test_manager_serialize_deserialize_inconsistent_monitor for a reload test that hits the rebroadcast case).

TheBlueMatt · 2022-07-05T00:44:21Z

lightning/src/chain/channelmonitor.rs

@@ -1934,6 +1966,8 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 		self.pending_monitor_events.push(MonitorEvent::CommitmentTxConfirmed(self.funding_info.0));
 	}

+
+


spurious newlines here?

Fixed in 84d8de6

TheBlueMatt · 2022-07-05T00:44:46Z

lightning/src/chain/channelmonitor.rs

+		}
+	}
+
+	pub(crate) fn force_broadcast_latest_holder_commitment_txn_unsafe<B: Deref, L: Deref>(&mut self, broadcaster: &B, logger: &L)


Does this need to be pub(crate)?

I don't think so, I mostly followed the signature for the previous function which was pub(crate). But since both are supposed to be used through their respective wrappers, I guess we can just remove it? I did so in c262be2

Oops, to "drop" pub(crate) I meant make it module-visible (ie no visibility specifier present at all), not pub :)

QPotato · 2022-07-11T00:17:25Z

lightning/src/ln/functional_tests.rs

+	assert!(nodes[0].chain_monitor.watch_channel(deserialized_chanmon.get_funding_txo().0, deserialized_chanmon).is_ok());
+	check_added_monitors!(nodes[0], 1);
+	nodes[0].node.get_and_clear_pending_msg_events();
+	nodes[0].node.get_and_clear_pending_events();


TBH, I don't know why I need this in my test. I only know that without these lines my test would panic while trying to drop() the monitor or the node.

Right, so we use that to ensure that tests are always complete in checking for all the events that are generated during the course of the test - in general we should strive to that rather than simply clearing events. In this case it looks like you're missing a check_closed_event in the test body when the channel closes.

Yeap, that worked without needing to clear the messages. Fixed in bec767b.

QPotato · 2022-07-11T00:21:01Z

lightning/src/chain/channelmonitor.rs

@@ -948,6 +952,7 @@ impl<Signer: Sign> Writeable for ChannelMonitorImpl<Signer> {

 		self.lockdown_from_offchain.write(writer)?;
 		self.holder_tx_signed.write(writer)?;
+		self.allow_automated_broadcast.write(writer)?;


At first I thought I didn't need to write and read the new field. My first understanding was that monitor updates are replayed on deserialization. After some hours trying to make the test work and reading through the code, I realized that probably not the case and we actually serialize and deserialize the whole state so I added it here too and it seems to work.

Yes, we need to write it, but we cannot add new fields to the "normal" write sequence, they have to go in (odd) TLVs so that old nodes can tell what they are and ignore them.

Makes sense. I changed this in bec767b.

QPotato · 2022-07-11T16:31:39Z

Huh, no idea why CI is not running. I might be doing something wrong?

TheBlueMatt · 2022-07-11T18:57:27Z

lightning/src/chain/channelmonitor.rs

+			}
+			self.pending_monitor_events.push(MonitorEvent::CommitmentTxConfirmed(self.funding_info.0));
+		} else {
+			log_error!(logger, "You have a toxic holder commitment transaction avaible in channel monitor, read comment in ChannelMonitor::get_latest_holder_commitment_txn to be informed of manual action to take");


Actually, lets remove reference to "You have a toxic holder commitment transaction available" - since #1564 that's no longer actually true, if there's some toxic holder commitment transaction we panic instead of updating any state. Rather, the bool is now generally a result of the user deliberately calling "force_close_without_broadcasting". Its not really your bug but would be good to fix it up here.

Sure. I downgraded this to an info log in cc2e3c5.
I also removed the other instance of this message in update_monitor on de5266c simplifying the logic there a bit and keeping the info log in the case the case were we have both !*should_broadcast && self.holder_tx_signed. Though let me know if I didn't get this right and need to further change it.

TheBlueMatt · 2022-07-11T19:02:51Z

lightning/src/chain/channelmonitor.rs

+		self.inner.lock().unwrap().maybe_broadcast_latest_holder_commitment_txn(broadcaster, logger)
+	}
+
+	pub(crate) fn force_broadcast_latest_holder_commitment_txn_unsafe<B: Deref, L: Deref>(


My compiler says this is never used.

Right. We talked before about adding this method so that the users still has an option to broadcast if they think they know what they are doing, but I think in that case we want it to be plain pub, right? Changed this in 9084457

Actually, I think you also suggested this in another comment above. I rediscovered visibility rules, lol.

TheBlueMatt · 2022-07-11T19:05:36Z

lightning/src/ln/functional_tests.rs

+	assert!(nodes[0].chain_monitor.watch_channel(deserialized_chanmon.get_funding_txo().0, deserialized_chanmon).is_ok());
+	check_added_monitors!(nodes[0], 1);
+	nodes[0].node.get_and_clear_pending_msg_events();
+	nodes[0].node.get_and_clear_pending_events();


Right, so we use that to ensure that tests are always complete in checking for all the events that are generated during the course of the test - in general we should strive to that rather than simply clearing events. In this case it looks like you're missing a check_closed_event in the test body when the channel closes.

TheBlueMatt · 2022-07-20T22:26:40Z

I think the reason CI may not be running is you need to rebase this onto rust-lightning git, ie remove the merge commits. If you're not sure how to do that, Bitcoin Core's contributor guide has some instructions.

lightning/src/chain/channelmonitor.rs

QPotato · 2022-07-25T14:01:46Z

Still trying to figure out the rebase thing. It's kinda different to what I'm used to do with git. I think I messed up the branch a bit when trying it. Will give it another shot tomorrow. Thanks.

TheBlueMatt · 2022-07-25T18:23:11Z

No, the branch looks good, for the life of me I have no idea why Github isn't even trying to run CI...

TheBlueMatt · 2022-07-25T18:25:46Z

lightning/src/chain/channelmonitor.rs

+		self.inner.lock().unwrap().maybe_broadcast_latest_holder_commitment_txn(broadcaster, logger)
+	}
+
+	// Broadcasts the latest commitment transaction, even if we can't ensure it's safe to do so


Documentation needs to start with three "/"s to count. Also, we should add a bunch more detail here - why would a user want to use this, in what context is it safe, when is it not safe, etc.

TheBlueMatt · 2022-07-25T18:26:06Z

lightning/src/chain/channelmonitor.rs

@@ -1938,7 +1959,25 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 		}
 	}

-	pub(crate) fn broadcast_latest_holder_commitment_txn<B: Deref, L: Deref>(&mut self, broadcaster: &B, logger: &L)
+	// Broadcasts the latest commitment transaction only if it's safe to do so.


Same as above, needs three "/"s, needs to describe when a user wants to use this, not just what it does.

TheBlueMatt · 2022-07-25T18:26:28Z

lightning/src/chain/channelmonitor.rs

@@ -726,6 +726,10 @@ pub(crate) struct ChannelMonitorImpl<Signer: Sign> {
 	counterparty_node_id: Option<PublicKey>,

 	secp_ctx: Secp256k1<secp256k1::All>, //TODO: dedup this a bit...
+
+	// Used to track that the channel was updated with ChannelForceClosed {should_broadcast: false}


It doesn't matter so much for internal fields, but good to make this a doc comment with three "/"s rather than two.

TheBlueMatt · 2022-07-25T18:27:27Z

Ah, somehow github has flagged you as a spammer? Can you reach out to github via the link at https://github.com/QPotato/rust-lightning/actions ?

TheBlueMatt

Hey, it looks like I missed that you pushed a commit here, sadly github doesn't generate notifications for simple pushes, so I don't always see it. I left one further comment here, but it looks like a few of the comments from my previous review weren't yet addressed. Do you still have a desire to land this PR?

TheBlueMatt · 2022-08-15T23:38:07Z

lightning/src/chain/channelmonitor.rs

@@ -3417,6 +3453,8 @@ impl<'a, Signer: Sign, K: KeysInterface<Signer = Signer>> ReadableArgs<&'a K>
 			counterparty_node_id,

 			secp_ctx,
+
+			allow_automated_broadcast: allow_automated_broadcast.unwrap(),


This should be unwrap_or(true) as old data won't have the field in it, so unwrap could panic.

QPotato · 2022-08-22T13:15:46Z

Sorry for the absence, I've had very few free time lately. I'll try to push a new commit this week but feel free to take over if ai'm delaying a release or something. Thanks for all the review so far.

TheBlueMatt · 2022-08-22T23:14:10Z

No particular rush currently as long as you're going to pick it back up. If it becomes higher priority I'll take it and open a new PR, but will comment here before I do.

…on loss

- Add a functional test - Fix some typos I found while reading the code

- Use `check_closed_event` instead of clearing pending events - Write new property and read it from TLVs

…hat it's exposed to library users and not an unreferenced symbol.

…e info log in the case the txn is already signed.

QPotato · 2022-09-05T19:55:00Z

Rebased to latest master to try to make CI pass. I now understand why we use rebase instead of merge.
Still need to expand docs a bit, as requested. I'm doing some reading so that I can come up with something useful.

TheBlueMatt · 2022-09-06T18:59:20Z

lightning/src/chain/channelmonitor.rs

@@ -2148,7 +2169,25 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 		}
 	}

-	pub(crate) fn broadcast_latest_holder_commitment_txn<B: Deref, L: Deref>(&mut self, broadcaster: &B, logger: &L)
+	/// Broadcasts the latest commitment transaction only if it's safe to do so.
+	pub fn maybe_broadcast_latest_holder_commitment_txn<B: Deref, L: Deref>(&mut self, broadcaster: &B, logger: &L)


The entire type is private, so there's no reason to make these pub. If anything its a bit confusing because it looks like users/other things can call this (and the next function) but they cant.

TheBlueMatt · 2022-09-06T19:00:19Z

lightning/src/chain/channelmonitor.rs

-						// If we generated a MonitorEvent::CommitmentTxConfirmed, the ChannelManager
-						// will still give us a ChannelForceClosed event with !should_broadcast, but we
-						// shouldn't print the scary warning above.
+					self.allow_automated_broadcast = *should_broadcast;


If its already set to false we should probably leave it at false and not update it. I dont know that it actually matters, but it seems safer.

TheBlueMatt · 2022-09-06T19:07:35Z

lightning/src/chain/channelmonitor.rs

+	}
+
+	/// Broadcasts the latest commitment transaction, even if we can't ensure it's safe to do so
+	/// due to missing information.


I think we should just list the reasons why we force-closed without broadcast here, which I think are just -

channel monitor update(s) failed to be applied, likely because of a persistence error

user called the ChannelManager function to force-close without broadcast.

TheBlueMatt · 2022-11-30T19:28:37Z

Looks like this needs rebase again.

TheBlueMatt · 2024-06-13T14:49:44Z

Closing as abandoned.

TheBlueMatt reviewed Jul 5, 2022

View reviewed changes

QPotato force-pushed the track_should_broadcast branch from 558a431 to ef76dda Compare July 6, 2022 17:28

QPotato changed the title ~~[WIP] Add flag to disable broadcasting when it's dangerous due to information loss~~ Add flag to disable broadcasting when it's dangerous due to information loss Jul 11, 2022

QPotato commented Jul 11, 2022

View reviewed changes

QPotato requested a review from TheBlueMatt July 11, 2022 00:24

TheBlueMatt reviewed Jul 11, 2022

View reviewed changes

wpaulino self-requested a review July 11, 2022 22:12

QPotato requested a review from TheBlueMatt July 20, 2022 04:01

TheBlueMatt reviewed Jul 20, 2022

View reviewed changes

lightning/src/chain/channelmonitor.rs Show resolved Hide resolved

QPotato force-pushed the track_should_broadcast branch from de5266c to 2d69c47 Compare July 22, 2022 00:15

TheBlueMatt reviewed Jul 25, 2022

View reviewed changes

QPotato force-pushed the track_should_broadcast branch from 2d69c47 to 0bc9345 Compare August 9, 2022 20:08

TheBlueMatt reviewed Aug 15, 2022

View reviewed changes

TheBlueMatt mentioned this pull request Aug 16, 2022

Do not broadcast commitment txn on Permanent mon update failure #1106

Merged

QPotato added 9 commits September 5, 2022 16:49

Add flag to disable broadcasting when it's dangerous due to informati…

9445382

…on loss

- Add the new flag on de/serialization

f7c2023

- Add a functional test - Fix some typos I found while reading the code

remove pub(crate)

dff01e1

Partially address review

7342d8c

- Use `check_closed_event` instead of clearing pending events - Write new property and read it from TLVs

Use pub on force_broadcast_latest_holder_commitment_txn_unsafe so t…

0da0f93

…hat it's exposed to library users and not an unreferenced symbol.

Remove a reference to toxic holder commitment transaction

f0f8c83

Remove second reference to toxic holder commitment txn but keeping th…

140152b

…e info log in the case the txn is already signed.

Re-add changes lost on rebase

2971adc

Add documentation on public method

214c222

Use triple slash on public method docs

616e69b

QPotato force-pushed the track_should_broadcast branch from 2aa3787 to 616e69b Compare September 5, 2022 19:53

TheBlueMatt reviewed Sep 6, 2022

View reviewed changes

wpaulino removed their request for review July 28, 2023 18:45

TheBlueMatt closed this Jun 13, 2024

		@@ -1934,6 +1966,8 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
		self.pending_monitor_events.push(MonitorEvent::CommitmentTxConfirmed(self.funding_info.0));
		}

Add flag to disable broadcasting when it's dangerous due to information loss #1593

Add flag to disable broadcasting when it's dangerous due to information loss #1593

Conversation

QPotato commented Jul 4, 2022 • edited Loading

TheBlueMatt 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

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

QPotato commented Jul 11, 2022

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 Jul 20, 2022

QPotato commented Jul 25, 2022

TheBlueMatt commented Jul 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Jul 25, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QPotato commented Aug 22, 2022

TheBlueMatt commented Aug 22, 2022

QPotato commented Sep 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Nov 30, 2022

TheBlueMatt commented Jun 13, 2024

QPotato commented Jul 4, 2022 •

edited

Loading