Don't fail HTLCs in revoked commitment txn until we spend them

When we see a counterparty revoked commitment transaction on-chain
we shouldn't immediately queue up HTLCs present in it for
resolution until we have spent the HTLC outputs in some kind of
claim transaction.

In order to do so, we first have to change the
`fail_unbroadcast_htlcs!()` call to provide it with the HTLCs which
are present in the (revoked) commitment transaction which was
broadcast. However, this is not sufficient - because all of those
HTLCs had their `HTLCSource` removed when the commitment
transaction was revoked, we also have to update
`fail_unbroadcast_htlcs` to check the payment hash and amount when
the `HTLCSource` is `None`.

Somewhat surprisingly, several tests actually explicitly tested for
the old behavior, which required amending to pass with the new
changes.

Finally, this adds a debug assertion when writing `ChannelMonitor`s
to ensure `HTLCSource`s do not leak.

Author: TheBlueMatt

lightning/src/chain/channelmonitor.rs

@@ -865,6 +865,9 @@ impl<Signer: Sign> Writeable for ChannelMonitorImpl<Signer> {
 			writer.write_all(&txid[..])?;
 			writer.write_all(&byte_utils::be64_to_array(htlc_infos.len() as u64))?;
 			for &(ref htlc_output, ref htlc_source) in htlc_infos.iter() {
+				debug_assert!(htlc_source.is_none() || Some(**txid) == self.current_counterparty_commitment_txid
+						|| Some(**txid) == self.prev_counterparty_commitment_txid,
+					"HTLC Sources for all but revoked commitment transactions should be none!");
 				serialize_htlc_in_commitment!(htlc_output);
 				htlc_source.as_ref().map(|b| b.as_ref()).write(writer)?;
 			}
@@ -1666,9 +1669,14 @@ macro_rules! fail_unbroadcast_htlcs {
 							// cannot currently change after channel initialization, so we don't
 							// need to here.
 							let confirmed_htlcs_iter: &mut Iterator<Item = (&HTLCOutputInCommitment, Option<&HTLCSource>)> = &mut $confirmed_htlcs_list;
+
 							let mut matched_htlc = false;
 							for (ref broadcast_htlc, ref broadcast_source) in confirmed_htlcs_iter {
-								if broadcast_htlc.transaction_output_index.is_some() && Some(&**source) == *broadcast_source {
+								if broadcast_htlc.transaction_output_index.is_some() &&
+									(Some(&**source) == *broadcast_source ||
+									 (broadcast_source.is_none() &&
+									  broadcast_htlc.payment_hash == htlc.payment_hash &&
+									  broadcast_htlc.amount_msat == htlc.amount_msat)) {
 									matched_htlc = true;
 									break;
 								}
@@ -2092,8 +2100,14 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 				}
 				self.counterparty_commitment_txn_on_chain.insert(commitment_txid, commitment_number);
 
-				fail_unbroadcast_htlcs!(self, "revoked counterparty", commitment_txid, height,
-					[].iter().map(|a| *a), logger);
+				if let Some(per_commitment_data) = per_commitment_option {
+					fail_unbroadcast_htlcs!(self, "revoked_counterparty", commitment_txid, height,
+						per_commitment_data.iter().map(|(a, b)| (a, b.as_ref().map(|b| b.as_ref()))), logger);
+				} else {
+					debug_assert!(false, "We should have per-commitment option for any recognized old commitment txn");
+					fail_unbroadcast_htlcs!(self, "revoked counterparty", commitment_txid, height,
+						[].iter().map(|a| *a), logger);
+				}
 			}
 		} else if let Some(per_commitment_data) = per_commitment_option {
 			// While this isn't useful yet, there is a potential race where if a counterparty

lightning/src/ln/functional_tests.rs

@@ -2510,10 +2510,10 @@ fn claim_htlc_outputs_shared_tx() {
 	let chan_1 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
 
 	// Rebalance the network to generate htlc in the two directions
-	send_payment(&nodes[0], &vec!(&nodes[1])[..], 8000000);
+	send_payment(&nodes[0], &[&nodes[1]], 8_000_000);
 	// node[0] is gonna to revoke an old state thus node[1] should be able to claim both offered/received HTLC outputs on top of commitment tx
-	let payment_preimage_1 = route_payment(&nodes[0], &vec!(&nodes[1])[..], 3000000).0;
-	let (_payment_preimage_2, payment_hash_2, _) = route_payment(&nodes[1], &vec!(&nodes[0])[..], 3000000);
+	let payment_preimage_1 = route_payment(&nodes[0], &[&nodes[1]], 3_000_000).0;
+	let (_payment_preimage_2, payment_hash_2, _) = route_payment(&nodes[1], &[&nodes[0]], 3_000_000);
 
 	// Get the will-be-revoked local txn from node[0]
 	let revoked_local_txn = get_local_commitment_txn!(nodes[0], chan_1.2);
@@ -2536,9 +2536,9 @@ fn claim_htlc_outputs_shared_tx() {
 		check_added_monitors!(nodes[1], 1);
 		check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed);
 		connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
-		expect_payment_failed!(nodes[1], payment_hash_2, true);
+		assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 
-		let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
+		let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
 		assert_eq!(node_txn.len(), 2); // ChannelMonitor: penalty tx, ChannelManager: local commitment
 
 		assert_eq!(node_txn[0].input.len(), 3); // Claim the revoked output + both revoked HTLC outputs
@@ -2555,7 +2555,13 @@ fn claim_htlc_outputs_shared_tx() {
 
 		// Next nodes[1] broadcasts its current local tx state:
 		assert_eq!(node_txn[1].input.len(), 1);
-		assert_eq!(node_txn[1].input[0].previous_output.txid, chan_1.3.txid()); //Spending funding tx unique txouput, tx broadcasted by ChannelManager
+		check_spends!(node_txn[1], chan_1.3);
+
+		// Finally, mine the penalty transaction and check that we get an HTLC failure after
+		// ANTI_REORG_DELAY confirmations.
+		mine_transaction(&nodes[1], &node_txn[0]);
+		connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
+		expect_payment_failed!(nodes[1], payment_hash_2, true);
 	}
 	get_announce_close_broadcast_events(&nodes, 0, 1);
 	assert_eq!(nodes[0].node.list_channels().len(), 0);
@@ -2574,11 +2580,11 @@ fn claim_htlc_outputs_single_tx() {
 	let chan_1 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
 
 	// Rebalance the network to generate htlc in the two directions
-	send_payment(&nodes[0], &vec!(&nodes[1])[..], 8000000);
+	send_payment(&nodes[0], &[&nodes[1]], 8_000_000);
 	// node[0] is gonna to revoke an old state thus node[1] should be able to claim both offered/received HTLC outputs on top of commitment tx, but this
 	// time as two different claim transactions as we're gonna to timeout htlc with given a high current height
-	let payment_preimage_1 = route_payment(&nodes[0], &vec!(&nodes[1])[..], 3000000).0;
-	let (_payment_preimage_2, payment_hash_2, _payment_secret_2) = route_payment(&nodes[1], &vec!(&nodes[0])[..], 3000000);
+	let payment_preimage_1 = route_payment(&nodes[0], &[&nodes[1]], 3_000_000).0;
+	let (_payment_preimage_2, payment_hash_2, _payment_secret_2) = route_payment(&nodes[1], &[&nodes[0]], 3_000_000);
 
 	// Get the will-be-revoked local txn from node[0]
 	let revoked_local_txn = get_local_commitment_txn!(nodes[0], chan_1.2);
@@ -2600,9 +2606,9 @@ fn claim_htlc_outputs_single_tx() {
 		}
 
 		connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
-		expect_payment_failed!(nodes[1], payment_hash_2, true);
+		assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 
-		let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
+		let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
 		assert!(node_txn.len() == 9 || node_txn.len() == 10);
 		// ChannelMonitor: justice tx revoked offered htlc, justice tx revoked received htlc, justice tx revoked to_local (3)
 		// ChannelManager: local commmitment + local HTLC-timeout (2)
@@ -2634,6 +2640,14 @@ fn claim_htlc_outputs_single_tx() {
 		assert_eq!(*witness_lens.iter().skip(0).next().unwrap(), 77); // revoked to_local
 		assert_eq!(*witness_lens.iter().skip(1).next().unwrap(), OFFERED_HTLC_SCRIPT_WEIGHT); // revoked offered HTLC
 		assert_eq!(*witness_lens.iter().skip(2).next().unwrap(), ACCEPTED_HTLC_SCRIPT_WEIGHT); // revoked received HTLC
+
+		// Finally, mine the penalty transactions and check that we get an HTLC failure after
+		// ANTI_REORG_DELAY confirmations.
+		mine_transaction(&nodes[1], &node_txn[2]);
+		mine_transaction(&nodes[1], &node_txn[3]);
+		mine_transaction(&nodes[1], &node_txn[4]);
+		connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
+		expect_payment_failed!(nodes[1], payment_hash_2, true);
 	}
 	get_announce_close_broadcast_events(&nodes, 0, 1);
 	assert_eq!(nodes[0].node.list_channels().len(), 0);
@@ -7261,37 +7275,25 @@ fn do_test_sweep_outbound_htlc_failure_update(revoked: bool, local: bool) {
 		check_added_monitors!(nodes[0], 1);
 		check_closed_event!(nodes[0], 1, ClosureReason::CommitmentTxConfirmed);
 		assert_eq!(nodes[0].node.get_and_clear_pending_events().len(), 0);
+
 		connect_blocks(&nodes[0], TEST_FINAL_CLTV - 1); // Confirm blocks until the HTLC expires
-		timeout_tx.push(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap()[1].clone());
+		timeout_tx = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().drain(..)
+			.filter(|tx| tx.input[0].previous_output.txid == bs_commitment_tx[0].txid()).collect();
+		check_spends!(timeout_tx[0], bs_commitment_tx[0]);
+		// For both a revoked or non-revoked commitment transaction, after ANTI_REORG_DELAY the
+		// dust HTLC should have been failed.
+		expect_payment_failed!(nodes[0], dust_hash, true);
+
 		if !revoked {
-			expect_payment_failed!(nodes[0], dust_hash, true);
 			assert_eq!(timeout_tx[0].input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT);
-			// We fail non-dust-HTLC 2 by broadcast of local timeout tx on remote commitment tx
-			mine_transaction(&nodes[0], &timeout_tx[0]);
-			assert_eq!(nodes[0].node.get_and_clear_pending_events().len(), 0);
-			connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
-			expect_payment_failed!(nodes[0], non_dust_hash, true);
 		} else {
-			// If revoked, both dust & non-dust HTLCs should have been failed after ANTI_REORG_DELAY confs of revoked
-			// commitment tx
-			let events = nodes[0].node.get_and_clear_pending_events();
-			assert_eq!(events.len(), 2);
-			let first;
-			match events[0] {
-				Event::PaymentPathFailed { payment_hash, .. } => {
-					if payment_hash == dust_hash { first = true; }
-					else { first = false; }
-				},
-				_ => panic!("Unexpected event"),
-			}
-			match events[1] {
-				Event::PaymentPathFailed { payment_hash, .. } => {
-					if first { assert_eq!(payment_hash, non_dust_hash); }
-					else { assert_eq!(payment_hash, dust_hash); }
-				},
-				_ => panic!("Unexpected event"),
-			}
+			assert_eq!(timeout_tx[0].lock_time, 0);
 		}
+		// We fail non-dust-HTLC 2 by broadcast of local timeout/revocation-claim tx
+		mine_transaction(&nodes[0], &timeout_tx[0]);
+		assert_eq!(nodes[0].node.get_and_clear_pending_events().len(), 0);
+		connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
+		expect_payment_failed!(nodes[0], non_dust_hash, true);
 	}
 }
 

lightning/src/ln/monitor_tests.rs

@@ -82,6 +82,55 @@ fn chanmon_fail_from_stale_commitment() {
 	expect_payment_failed_with_update!(nodes[0], payment_hash, false, update_a.contents.short_channel_id, true);
 }
 
+#[test]
+fn revoked_output_htlc_resolution_timing() {
+	// Tests that the resoluting time of HTLCs which were present in a broadcasted remote revoked
+	// commitment transaction are resolved only after a spend of the HTLC output reaches six
+	// confirmations. Preivously they would resolve after the revoked commitment transaction itself
+	// reaches six confirmations.
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 500_000_000, InitFeatures::known(), InitFeatures::known());
+
+	let payment_hash_1 = route_payment(&nodes[1], &[&nodes[0]], 1_000_000).1;
+
+	// Get a commitment transaction which contains the HTLC we care about, but which we'll revoke
+	// before forwarding.
+	let revoked_local_txn = get_local_commitment_txn!(nodes[0], chan.2);
+	assert_eq!(revoked_local_txn.len(), 1);
+
+	// Route a dust payment to revoke the above commitment transaction
+	route_payment(&nodes[0], &[&nodes[1]], 1_000);
+
+	// Confirm the revoked commitment transaction, closing the channel.
+	mine_transaction(&nodes[1], &revoked_local_txn[0]);
+	check_added_monitors!(nodes[1], 1);
+	check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed);
+	check_closed_broadcast!(nodes[1], true);
+
+	let bs_spend_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
+	assert_eq!(bs_spend_txn.len(), 2);
+	check_spends!(bs_spend_txn[0], revoked_local_txn[0]);
+	check_spends!(bs_spend_txn[1], chan.3);
+
+	// After the commitment transaction confirms, we should still wait on the HTLC spend
+	// transaction to confirm before resolving the HTLC.
+	connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
+	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+	assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
+
+	// Spend the HTLC output, generating a HTLC failure event after ANTI_REORG_DELAY confirmations.
+	mine_transaction(&nodes[1], &bs_spend_txn[0]);
+	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+	assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
+
+	connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
+	expect_payment_failed!(nodes[1], payment_hash_1, true);
+}
+
 #[test]
 fn chanmon_claim_value_coop_close() {
 	// Tests `get_claimable_balances` returns the correct values across a simple cooperative claim.