Fix to_remote SpendableOutputs generation in rare reorg cases #1022

TheBlueMatt · 2021-07-29T19:51:58Z

If we first see a local commitment transaction, and then a reorg
causes the confirmed channel close transaction to instead be a
remote commitment transaction, we would fail a spurious if else
check, resulting in us not generating the correct SpendableOutput
event for the to_remote output now confirmed on chain.

This resolves the incorrect logic and adds a regression test.

TheBlueMatt · 2021-07-29T19:52:42Z

Obviously a pretty important bugfix, so tagging 0.0.100.

valentinewallace

LGTM

valentinewallace · 2021-07-30T22:24:00Z

lightning/src/chain/channelmonitor.rs

+			if let Some(ref broadcasted_holder_revokable_script) = self.broadcasted_holder_revokable_script {
 				if broadcasted_holder_revokable_script.0 == outp.script_pubkey {
 					spendable_output =  Some(SpendableOutputDescriptor::DelayedPaymentOutput(DelayedPaymentOutputDescriptor {


Added some print statements and it seems like the issue was: it'd enter this else if statement, but wouldn't pass the check on the second highlighted line, and then naturally fail to even consider the next else if check

Given the second if clause overwrites spendable_output, it seems like essentially a reordering of the clauses?

That might have worked too, but it's really the if else blocks miss that one in the middle isn't fully captured by the check. Because the check is split across the if let and the inner if, we sometimes incorrectly short-circuit the if else tree.

Yeah, I was wondering if they could have possibly been merge-able into one condition.

No, rather obnoxiously Rust doesn't let you merge if let into any additional constraints.

Here my understanding of the fixed issue.

If a holder commitment is broadcast, we set ChannelMonitor::broadcasted_holder_revokable_script to Some in check_spend_holder_transaction, from then always marking the spendable output as a DelayedPaymentOutput in is_paying_spendable_output.

After this PR, the spendable output type detection is moved from a "else if" control flow to a "if" one, thus preventing the accidental jump over StaticPaymentOutput detection.

arik-so · 2021-08-03T21:34:58Z

lightning/src/ln/reorg_tests.rs

+		assert_eq!(outputs.len(), 1);
+		let spend_tx = nodes[0].keys_manager.backing.spend_spendable_outputs(&[&outputs[0]], Vec::new(),
+			Builder::new().push_opcode(opcodes::all::OP_RETURN).into_script(), 253, &Secp256k1::new()).unwrap();
+		check_spends!(spend_tx, remote_txn_b[0]);


admittedly not really the place for this remark, but check_spends sounds awfully vague. Especially with the macro not being defined in this file, a more descriptive name might be helpful for readers.

Can't say I disagree. There's a lot of open PRs right now, however, so if we want to change it maybe we should wait until things are a bit calmer in a month or two.

arik-so · 2021-08-03T21:36:59Z

lightning/src/ln/reorg_tests.rs

+	// nodes[1] is waiting for the to_self_delay to expire, which is many more than
+	// ANTI_REORG_DELAY. Instead, walk it back and confirm the original remote_txn_a commitment
+	// again and check that nodes[1] generates a similar spendable output.
+	// Technically a reorg of ANTI_REORG_DELAY violates our assumptions, so this is undefined by


Do we have a list for which events we wouldn't know how to handle correctly based on max possible reorg size assumptions? Might be useful to link from this comment.

I think probably roughly all of them. In general we don't bother to correctly handle anything that includes a reorg of ANTI_REORG_DELAY. Many things are likely to work just fine, probably, but it's outside our security model entirely. This could maybe be better documented.

arik-so · 2021-08-03T21:39:22Z

lightning/src/chain/channelmonitor.rs

+			if let Some(ref broadcasted_holder_revokable_script) = self.broadcasted_holder_revokable_script {
 				if broadcasted_holder_revokable_script.0 == outp.script_pubkey {
 					spendable_output =  Some(SpendableOutputDescriptor::DelayedPaymentOutput(DelayedPaymentOutputDescriptor {


Given the second if clause overwrites spendable_output, it seems like essentially a reordering of the clauses?

ariard · 2021-08-03T23:21:16Z

Nit, can you take this : ariard@6747827 ?

ariard

Otherwise ACK 8a9b7e5.

I did verify the presence of the current issue and effectiveness of the fix. Mea culpa for this, iirc i introduced is_paying_spendable_output

ariard · 2021-08-03T23:26:49Z

lightning/src/chain/channelmonitor.rs

@@ -2451,7 +2451,8 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 					output: outp.clone(),
 				});
 				break;
-			} else if let Some(ref broadcasted_holder_revokable_script) = self.broadcasted_holder_revokable_script {
+			}
+			if let Some(ref broadcasted_holder_revokable_script) = self.broadcasted_holder_revokable_script {


One step further to minimize unseen interactions like this could be to unset self.broadcasted_holder_revokeable_script in block_disconnected/transaction_unconfirmed's onchain_events_awaiting_threshold_conf. If the removed event is of type MaturingOutput and SpendableOutputDescriptor == DelayedPaymentOutput, self.broadcasted_holder_revokeable_script==None I think ?

I mean with this no longer being an if else branch list it should "always work", and unsettnig scripts to match seems risky.

lightning/src/ln/reorg_tests.rs

If we first see a local commitment transaction, and then a reorg causes the confirmed channel close transaction to instead be a remote commitment transaction, we would fail a spurious `if else` check, resulting in us not generating the correct `SpendableOutput` event for the to_remote output now confirmed on chain. This resolves the incorrect logic and adds a regression test.

TheBlueMatt · 2021-08-04T02:36:21Z

Nit, can you take this : ariard/rust-lightning@6747827 ?

I have several more PRs coming in this code area, I'll throw it in the next one.

Otherwise, added a mut which is needed for older rustc support and condensed a comment as @ariard suggested. Diff from previous ACKs is trivial so will take after CI:

$ git diff-tree -U1 8a9b7e5a ad445908
diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs
index d4dce6d5..7845c089 100644
--- a/lightning/src/ln/reorg_tests.rs
+++ b/lightning/src/ln/reorg_tests.rs
@@ -435,6 +435,6 @@ fn test_set_outpoints_partial_claiming() {
 fn do_test_to_remote_after_local_detection(style: ConnectStyle) {
-	// In previous code, detection of commitment transaction to_remote outputs in a remote
-	// commitment transaction was dependent on whether a local commitment transaction had been seen
-	// on-chain previously. This resulted in some edge cases around not being able to generate a
-	// SpendableOutput event after a reorg.
+	// In previous code, detection of to_remote outputs in a counterparty commitment transaction
+	// was dependent on whether a local commitment transaction had been seen on-chain previously.
+	// This resulted in some edge cases around not being able to generate a SpendableOutput event
+	// after a reorg.
 	//
@@ -446,3 +446,3 @@ fn do_test_to_remote_after_local_detection(style: ConnectStyle) {
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
-	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 
$

codecov · 2021-08-04T02:57:10Z

Codecov Report

Merging #1022 (ad44590) into main (e26c9b0) will increase coverage by 0.67%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1022      +/-   ##
==========================================
+ Coverage   90.82%   91.49%   +0.67%     
==========================================
  Files          60       61       +1     
  Lines       31455    39398    +7943     
==========================================
+ Hits        28568    36046    +7478     
- Misses       2887     3352     +465

Impacted Files	Coverage Δ
lightning/src/chain/channelmonitor.rs	`90.80% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/reorg_tests.rs	`99.70% <100.00%> (+0.07%)`	⬆️
lightning/src/util/events.rs	`12.50% <0.00%> (-2.64%)`	⬇️
lightning/src/ln/functional_tests.rs	`96.78% <0.00%> (-0.60%)`	⬇️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.25% <0.00%> (-0.49%)`	⬇️
lightning/src/lib.rs	`100.00% <0.00%> (ø)`
lightning/src/ln/features.rs	`99.56% <0.00%> (+0.12%)`	⬆️
lightning/src/ln/onion_utils.rs	`95.21% <0.00%> (+0.29%)`	⬆️
lightning-block-sync/src/http.rs	`94.04% <0.00%> (+0.33%)`	⬆️
lightning/src/chain/onchaintx.rs	`94.66% <0.00%> (+0.47%)`	⬆️
... and 15 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 e26c9b0...ad44590. Read the comment docs.

TheBlueMatt added this to the 0.0.100 milestone Jul 29, 2021

TheBlueMatt force-pushed the 2021-07-to-remote-reorg branch from 7282abd to 8a9b7e5 Compare July 29, 2021 20:47

valentinewallace approved these changes Jul 30, 2021

View reviewed changes

arik-so reviewed Aug 3, 2021

View reviewed changes

arik-so approved these changes Aug 3, 2021

View reviewed changes

ariard reviewed Aug 3, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-07-to-remote-reorg branch from 8a9b7e5 to 0545842 Compare August 4, 2021 02:33

TheBlueMatt force-pushed the 2021-07-to-remote-reorg branch from 0545842 to ad44590 Compare August 4, 2021 02:35

TheBlueMatt merged commit 09e1670 into lightningdevkit:main Aug 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to_remote SpendableOutputs generation in rare reorg cases #1022

Fix to_remote SpendableOutputs generation in rare reorg cases #1022

TheBlueMatt commented Jul 29, 2021

TheBlueMatt commented Jul 29, 2021

valentinewallace left a comment

valentinewallace Jul 30, 2021

arik-so Aug 3, 2021

TheBlueMatt Aug 3, 2021

arik-so Aug 3, 2021

TheBlueMatt Aug 3, 2021

ariard Aug 3, 2021 •

edited

Loading

arik-so Aug 3, 2021

TheBlueMatt Aug 3, 2021

arik-so Aug 3, 2021

TheBlueMatt Aug 3, 2021

arik-so Aug 3, 2021

ariard commented Aug 3, 2021

ariard left a comment

ariard Aug 3, 2021

TheBlueMatt Aug 4, 2021

TheBlueMatt commented Aug 4, 2021

codecov bot commented Aug 4, 2021 •

edited

Loading

Fix to_remote SpendableOutputs generation in rare reorg cases #1022

Fix to_remote SpendableOutputs generation in rare reorg cases #1022

Conversation

TheBlueMatt commented Jul 29, 2021

TheBlueMatt commented Jul 29, 2021

valentinewallace 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

ariard Aug 3, 2021 • edited Loading

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

ariard commented Aug 3, 2021

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

TheBlueMatt commented Aug 4, 2021

codecov bot commented Aug 4, 2021 • edited Loading

Codecov Report

ariard Aug 3, 2021 •

edited

Loading

codecov bot commented Aug 4, 2021 •

edited

Loading