Correct handling of reorg'd-out revoked counterparty transactions

Previously, while processing a confirmed revoked counterparty
commitment transaction, we'd populate `OnchainEvent`s for live
HTLCs with a `txid` source of the txid of the latest counterparty
commitment transactions, not the confirmed revoked one. This meant
that, if the user is using `transaction_unconfirmed` to notify us
of reorg information, we'd end up not removing the entry if the
revoked commitment transaction was reorg'd out. This would
ultimately cause us to spuriously resolve the HTLC(s) as the chain
advanced, even though we were doing so based on a now-reorged-out
transaction.

Luckily the fix is simple - set the correct txid in the
`OnchainEventEntry`. We also take this opportunity to update
logging in a few places with the txid of the transaction causing an
event.

Author: TheBlueMatt

lightning/src/chain/channelmonitor.rs

@@ -1659,7 +1659,8 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
 /// as long as we examine both the current counterparty commitment transaction and, if it hasn't
 /// been revoked yet, the previous one, we we will never "forget" to resolve an HTLC.
 macro_rules! fail_unbroadcast_htlcs {
-	($self: expr, $commitment_tx_type: expr, $commitment_tx_conf_height: expr, $confirmed_htlcs_list: expr, $logger: expr) => { {
+	($self: expr, $commitment_tx_type: expr, $commitment_txid_confirmed: expr,
+	 $commitment_tx_conf_height: expr, $confirmed_htlcs_list: expr, $logger: expr) => { {
 		macro_rules! check_htlc_fails {
 			($txid: expr, $commitment_tx: expr) => {
 				if let Some(ref latest_outpoints) = $self.counterparty_claimable_outpoints.get($txid) {
@@ -1693,7 +1694,7 @@ macro_rules! fail_unbroadcast_htlcs {
 								}
 							});
 							let entry = OnchainEventEntry {
-								txid: *$txid,
+								txid: $commitment_txid_confirmed,
 								height: $commitment_tx_conf_height,
 								event: OnchainEvent::HTLCUpdate {
 									source: (**source).clone(),
@@ -1702,8 +1703,9 @@ macro_rules! fail_unbroadcast_htlcs {
 									commitment_tx_output_idx: None,
 								},
 							};
-							log_trace!($logger, "Failing HTLC with payment_hash {} from {} counterparty commitment tx due to broadcast of {} commitment transaction, waiting for confirmation (at height {})",
-								log_bytes!(htlc.payment_hash.0), $commitment_tx, $commitment_tx_type, entry.confirmation_threshold());
+							log_trace!($logger, "Failing HTLC with payment_hash {} from {} counterparty commitment tx due to broadcast of {} commitment transaction {}, waiting for confirmation (at height {})",
+								log_bytes!(htlc.payment_hash.0), $commitment_tx, $commitment_tx_type,
+								$commitment_txid_confirmed, entry.confirmation_threshold());
 							$self.onchain_events_awaiting_threshold_conf.push(entry);
 						}
 					}
@@ -2100,7 +2102,8 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 				}
 				self.counterparty_commitment_txn_on_chain.insert(commitment_txid, commitment_number);
 
-				fail_unbroadcast_htlcs!(self, "revoked counterparty", height, [].iter().map(|a| *a), logger);
+				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
@@ -2116,7 +2119,10 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 			self.counterparty_commitment_txn_on_chain.insert(commitment_txid, commitment_number);
 
 			log_info!(logger, "Got broadcast of non-revoked counterparty commitment transaction {}", commitment_txid);
-			fail_unbroadcast_htlcs!(self, "counterparty", height, per_commitment_data.iter().map(|(a, b)| (a, b.as_ref().map(|b| b.as_ref()))), logger);
+			fail_unbroadcast_htlcs!(self, "counterparty", commitment_txid, height,
+				per_commitment_data.iter().map(|(htlc, htlc_source)|
+					(htlc, htlc_source.as_ref().map(|htlc_source| htlc_source.as_ref()))
+				), logger);
 
 			let htlc_claim_reqs = self.get_counterparty_htlc_output_claim_reqs(commitment_number, commitment_txid, Some(tx));
 			for req in htlc_claim_reqs {
@@ -2272,15 +2278,19 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 			let res = self.get_broadcasted_holder_claims(&self.current_holder_commitment_tx, height);
 			let mut to_watch = self.get_broadcasted_holder_watch_outputs(&self.current_holder_commitment_tx, tx);
 			append_onchain_update!(res, to_watch);
-			fail_unbroadcast_htlcs!(self, "latest holder", height, self.current_holder_commitment_tx.htlc_outputs.iter().map(|(a, _, c)| (a, c.as_ref())), logger);
+			fail_unbroadcast_htlcs!(self, "latest holder", commitment_txid, height,
+				self.current_holder_commitment_tx.htlc_outputs.iter()
+				.map(|(htlc, _, htlc_source)| (htlc, htlc_source.as_ref())), logger);
 		} else if let &Some(ref holder_tx) = &self.prev_holder_signed_commitment_tx {
 			if holder_tx.txid == commitment_txid {
 				is_holder_tx = true;
 				log_info!(logger, "Got broadcast of previous holder commitment tx {}, searching for available HTLCs to claim", commitment_txid);
 				let res = self.get_broadcasted_holder_claims(holder_tx, height);
 				let mut to_watch = self.get_broadcasted_holder_watch_outputs(holder_tx, tx);
 				append_onchain_update!(res, to_watch);
-				fail_unbroadcast_htlcs!(self, "previous holder", height, holder_tx.htlc_outputs.iter().map(|(a, _, c)| (a, c.as_ref())), logger);
+				fail_unbroadcast_htlcs!(self, "previous holder", commitment_txid, height,
+					holder_tx.htlc_outputs.iter().map(|(htlc, _, htlc_source)| (htlc, htlc_source.as_ref())),
+					logger);
 			}
 		}
 
@@ -2561,7 +2571,8 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 						matured_htlcs.push(source.clone());
 					}
 
-					log_debug!(logger, "HTLC {} failure update has got enough confirmations to be passed upstream", log_bytes!(payment_hash.0));
+					log_debug!(logger, "HTLC {} failure update in {} has got enough confirmations to be passed upstream",
+						log_bytes!(payment_hash.0), entry.txid);
 					self.pending_monitor_events.push(MonitorEvent::HTLCEvent(HTLCUpdate {
 						payment_hash,
 						payment_preimage: None,
@@ -2640,7 +2651,10 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 		F::Target: FeeEstimator,
 		L::Target: Logger,
 	{
-		self.onchain_events_awaiting_threshold_conf.retain(|ref entry| entry.txid != *txid);
+		self.onchain_events_awaiting_threshold_conf.retain(|ref entry| if entry.txid == *txid {
+			log_info!(logger, "Removing onchain event with txid {}", txid);
+			false
+		} else { true });
 		self.onchain_tx_handler.transaction_unconfirmed(txid, broadcaster, fee_estimator, logger);
 	}
 

lightning/src/ln/reorg_tests.rs

@@ -185,6 +185,77 @@ fn test_onchain_htlc_timeout_delay_remote_commitment() {
 	do_test_onchain_htlc_reorg(false, false);
 }
 
+#[test]
+fn test_counterparty_revoked_reorg() {
+	// Test what happens when a revoked counterparty transaction is broadcast but then reorg'd out
+	// of the main chain. Specifically, HTLCs in the latest commitment transaction which are not
+	// included in the revoked commitment transaction should not be considered failed, and should
+	// still be claim-from-able after the reorg.
+	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());
+
+	// Get the initial commitment transaction for broadcast, before any HTLCs are added at all.
+	let revoked_local_txn = get_local_commitment_txn!(nodes[0], chan.2);
+	assert_eq!(revoked_local_txn.len(), 1);
+
+	// Now add two HTLCs in each direction, one dust and one not.
+	route_payment(&nodes[0], &[&nodes[1]], 5_000_000);
+	route_payment(&nodes[0], &[&nodes[1]], 5_000);
+	let (payment_preimage_3, payment_hash_3, ..) = route_payment(&nodes[1], &[&nodes[0]], 4_000_000);
+	let payment_hash_4 = route_payment(&nodes[1], &[&nodes[0]], 4_000).1;
+
+	nodes[0].node.claim_funds(payment_preimage_3);
+	let _ = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
+	check_added_monitors!(nodes[0], 1);
+	expect_payment_claimed!(nodes[0], payment_hash_3, 4_000_000);
+
+	let mut unrevoked_local_txn = get_local_commitment_txn!(nodes[0], chan.2);
+	assert_eq!(unrevoked_local_txn.len(), 3); // commitment + 2 HTLC txn
+	// Sort the unrevoked transactions in reverse order, ie commitment tx, then HTLC 1 then HTLC 3
+	unrevoked_local_txn.sort_unstable_by_key(|tx| 1_000_000 - tx.output.iter().map(|outp| outp.value).sum::<u64>());
+
+	// Now mine A's old commitment transaction, which should close the channel, but take no action
+	// on any of the HTLCs, at least until we get six confirmations (which we won't get).
+	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);
+
+	// Connect up to one block before the revoked transaction would be considered final, then do a
+	// reorg that disconnects the full chain and goes up to the height at which the revoked
+	// transaction would be final.
+	let theoretical_conf_height = nodes[1].best_block_info().1 + ANTI_REORG_DELAY - 1;
+	connect_blocks(&nodes[1], ANTI_REORG_DELAY - 2);
+	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+	assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
+
+	disconnect_all_blocks(&nodes[1]);
+	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], theoretical_conf_height);
+	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+	assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
+
+	// Now connect A's latest commitment transaction instead and resolve the HTLCs
+	mine_transaction(&nodes[1], &unrevoked_local_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 the HTLC claim transaction for HTLC 3
+	mine_transaction(&nodes[1], &unrevoked_local_txn[2]);
+	expect_payment_sent!(nodes[1], payment_preimage_3);
+	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+
+	// Connect blocks to confirm the unrevoked commitment transaction
+	connect_blocks(&nodes[1], ANTI_REORG_DELAY - 2);
+	expect_payment_failed!(nodes[1], payment_hash_4, true);
+}
+
 fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_unconfirmed: bool, connect_style: ConnectStyle) {
 	// After creating a chan between nodes, we disconnect all blocks previously seen to force a
 	// channel close on nodes[0] side. We also use this to provide very basic testing of logic