From 70ae45fea030ed1d2064918c7b023aa142387bc8 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 30 Apr 2022 20:30:00 +0000 Subject: [PATCH] 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. --- lightning/src/chain/channelmonitor.rs | 22 ++++++-- lightning/src/ln/functional_tests.rs | 76 ++++++++++++++------------- lightning/src/ln/monitor_tests.rs | 49 +++++++++++++++++ 3 files changed, 107 insertions(+), 40 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index a61e6afbf..3f7a1055b 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -869,6 +869,9 @@ impl Writeable for ChannelMonitorImpl { 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 revoked commitment transactions should be none!"); serialize_htlc_in_commitment!(htlc_output); htlc_source.as_ref().map(|b| b.as_ref()).write(writer)?; } @@ -1676,9 +1679,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)> = &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; } @@ -2102,8 +2110,16 @@ impl ChannelMonitorImpl { } 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(|(htlc, htlc_source)| + (htlc, htlc_source.as_ref().map(|htlc_source| htlc_source.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(|reference| *reference), 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 diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index a716d8067..ddab3353e 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -2515,10 +2515,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); @@ -2541,9 +2541,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 @@ -2560,7 +2560,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); @@ -2579,11 +2585,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); @@ -2605,9 +2611,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); // Check the pair local commitment and HTLC-timeout broadcast due to HTLC expiration @@ -2635,6 +2641,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); @@ -7282,37 +7296,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); } } diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index 492a65537..7c46f7b6d 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -94,6 +94,55 @@ fn test_spendable_output<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, spendable_t } else { panic!(); } } +#[test] +fn revoked_output_htlc_resolution_timing() { + // Tests that 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. -- 2.39.5