From 44d1dfa23dcb19be6f3a02f7e18b3b905c1d4683 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 17 May 2022 00:12:20 +0000 Subject: [PATCH] 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. --- lightning/src/chain/channelmonitor.rs | 34 +++++++++---- lightning/src/ln/reorg_tests.rs | 71 +++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 10 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 512860016..a61e6afbf 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1659,7 +1659,8 @@ impl ChannelMonitor { /// 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 ChannelMonitorImpl { } 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 ChannelMonitorImpl { 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,7 +2278,9 @@ impl ChannelMonitorImpl { 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; @@ -2280,7 +2288,9 @@ impl ChannelMonitorImpl { 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 ChannelMonitorImpl { 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 ChannelMonitorImpl { 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); } diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs index 0025598e4..a7090e4e7 100644 --- a/lightning/src/ln/reorg_tests.rs +++ b/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::()); + + // 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 -- 2.39.5