From 215995cc2c31700434ebb92bde3c89ff75d2eec3 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 27 Jan 2025 18:07:11 +0000 Subject: [PATCH] Mark counterparty revoked offered HTLCs as `Unpinnable` If the counterparty broadcasts a revoked transaction with offered HTLCs, the output is not immediately pinnable as the counterparty cannot claim the HTLC until the CLTV expires and they use an HTLC-Timeout path. Here we properly set these packages as `Unpinnable`, changing some transaction generation during tests. --- lightning/src/chain/package.rs | 9 +++- lightning/src/ln/functional_tests.rs | 78 ++++++++++++++++------------ lightning/src/ln/monitor_tests.rs | 10 ++-- 3 files changed, 59 insertions(+), 38 deletions(-) diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index a5fd51acc61..dd7383695cf 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -699,8 +699,13 @@ impl PackageSolvingData { match self { PackageSolvingData::RevokedOutput(RevokedOutput { .. }) => PackageMalleability::Malleable(AggregationCluster::Unpinnable), - PackageSolvingData::RevokedHTLCOutput(..) => - PackageMalleability::Malleable(AggregationCluster::Pinnable), + PackageSolvingData::RevokedHTLCOutput(RevokedHTLCOutput { htlc, .. }) => { + if htlc.offered { + PackageMalleability::Malleable(AggregationCluster::Unpinnable) + } else { + PackageMalleability::Malleable(AggregationCluster::Pinnable) + } + }, PackageSolvingData::CounterpartyOfferedHTLCOutput(..) => PackageMalleability::Malleable(AggregationCluster::Unpinnable), PackageSolvingData::CounterpartyReceivedHTLCOutput(..) => diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 177306cfd7b..7a0e5138eaf 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -15,7 +15,7 @@ use crate::chain; use crate::chain::{ChannelMonitorUpdateStatus, Confirm, Listen, Watch}; use crate::chain::chaininterface::LowerBoundedFeeEstimator; use crate::chain::channelmonitor; -use crate::chain::channelmonitor::{Balance, ChannelMonitorUpdateStep, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY}; +use crate::chain::channelmonitor::{Balance, ChannelMonitorUpdateStep, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY, COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE}; use crate::chain::transaction::OutPoint; use crate::sign::{ecdsa::EcdsaChannelSigner, EntropySource, OutputSpender, SignerProvider}; use crate::events::bump_transaction::WalletSource; @@ -2509,14 +2509,12 @@ fn test_justice_tx_htlc_timeout() { mine_transaction(&nodes[1], &revoked_local_txn[0]); { let mut node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap(); - // The unpinnable, revoked to_self output, and the pinnable, revoked htlc output will - // be claimed in separate transactions. - assert_eq!(node_txn.len(), 2); - for tx in node_txn.iter() { - assert_eq!(tx.input.len(), 1); - check_spends!(tx, revoked_local_txn[0]); - } - assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output); + // The revoked HTLC output is not pinnable for another `TEST_FINAL_CLTV` blocks, and is + // thus claimed in the same transaction with the revoked to_self output. + assert_eq!(node_txn.len(), 1); + assert_eq!(node_txn[0].input.len(), 2); + check_spends!(node_txn[0], revoked_local_txn[0]); + assert_ne!(node_txn[0].input[0].previous_output, node_txn[0].input[1].previous_output); node_txn.clear(); } check_added_monitors!(nodes[1], 1); @@ -2736,28 +2734,26 @@ fn claim_htlc_outputs() { assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); - assert_eq!(node_txn.len(), 2); // Two penalty transactions: - assert_eq!(node_txn[0].input.len(), 1); // Claims the unpinnable, revoked output. - assert_eq!(node_txn[1].input.len(), 2); // Claims both pinnable, revoked HTLC outputs separately. - check_spends!(node_txn[0], revoked_local_txn[0]); - check_spends!(node_txn[1], revoked_local_txn[0]); - assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output); - assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[1].previous_output); - assert_ne!(node_txn[1].input[0].previous_output, node_txn[1].input[1].previous_output); + assert_eq!(node_txn.len(), 2); // ChannelMonitor: penalty txn + + // The ChannelMonitor should claim the accepted HTLC output separately from the offered + // HTLC and to_self outputs. + let accepted_claim = node_txn.iter().filter(|tx| tx.input.len() == 1).next().unwrap(); + let offered_to_self_claim = node_txn.iter().filter(|tx| tx.input.len() == 2).next().unwrap(); + check_spends!(accepted_claim, revoked_local_txn[0]); + check_spends!(offered_to_self_claim, revoked_local_txn[0]); + assert_eq!(accepted_claim.input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT); let mut witness_lens = BTreeSet::new(); - witness_lens.insert(node_txn[0].input[0].witness.last().unwrap().len()); - witness_lens.insert(node_txn[1].input[0].witness.last().unwrap().len()); - witness_lens.insert(node_txn[1].input[1].witness.last().unwrap().len()); - assert_eq!(witness_lens.len(), 3); + witness_lens.insert(offered_to_self_claim.input[0].witness.last().unwrap().len()); + witness_lens.insert(offered_to_self_claim.input[1].witness.last().unwrap().len()); + assert_eq!(witness_lens.len(), 2); 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 + assert_eq!(*witness_lens.iter().skip(1).next().unwrap(), OFFERED_HTLC_SCRIPT_WEIGHT); - // Finally, mine the penalty transactions and check that we get an HTLC failure after + // 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]); - mine_transaction(&nodes[1], &node_txn[1]); + mine_transaction(&nodes[1], accepted_claim); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); expect_payment_failed!(nodes[1], payment_hash_2, false); } @@ -4920,8 +4916,7 @@ fn test_static_spendable_outputs_timeout_tx() { check_spends!(spend_txn[2], node_txn[0], commitment_tx[0]); // All outputs } -#[test] -fn test_static_spendable_outputs_justice_tx_revoked_commitment_tx() { +fn do_test_static_spendable_outputs_justice_tx_revoked_commitment_tx(split_tx: bool) { 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]); @@ -4937,20 +4932,27 @@ fn test_static_spendable_outputs_justice_tx_revoked_commitment_tx() { claim_payment(&nodes[0], &vec!(&nodes[1])[..], payment_preimage); + if split_tx { + connect_blocks(&nodes[1], TEST_FINAL_CLTV - COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE + 1); + } + mine_transaction(&nodes[1], &revoked_local_txn[0]); check_closed_broadcast!(nodes[1], true); check_added_monitors!(nodes[1], 1); check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed, [nodes[0].node.get_our_node_id()], 100000); - // The unpinnable, revoked to_self output and the pinnable, revoked HTLC output will be claimed - // in separate transactions. + // If the HTLC expires within COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE, we'll claim both + // the revoked and HTLC outputs in one transaction, otherwise we'll split them as we consider + // the HTLC output as pinnable and want to claim pinnable and unpinnable outputs separately. let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().clone(); - assert_eq!(node_txn.len(), 2); + assert_eq!(node_txn.len(), if split_tx { 2 } else { 1 }); for tx in node_txn.iter() { - assert_eq!(tx.input.len(), 1); + assert_eq!(tx.input.len(), if split_tx { 1 } else { 2 }); check_spends!(tx, revoked_local_txn[0]); } - assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output); + if split_tx { + assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output); + } mine_transaction(&nodes[1], &node_txn[0]); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); @@ -4960,6 +4962,12 @@ fn test_static_spendable_outputs_justice_tx_revoked_commitment_tx() { check_spends!(spend_txn[0], node_txn[0]); } +#[test] +fn test_static_spendable_outputs_justice_tx_revoked_commitment_tx() { + do_test_static_spendable_outputs_justice_tx_revoked_commitment_tx(true); + do_test_static_spendable_outputs_justice_tx_revoked_commitment_tx(false); +} + #[test] fn test_static_spendable_outputs_justice_tx_revoked_htlc_timeout_tx() { let mut chanmon_cfgs = create_chanmon_cfgs(2); @@ -4992,6 +5000,10 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_timeout_tx() { check_spends!(revoked_htlc_txn[0], revoked_local_txn[0]); assert_ne!(revoked_htlc_txn[0].lock_time, LockTime::ZERO); // HTLC-Timeout + // In order to connect `revoked_htlc_txn[0]` we must first advance the chain by + // `TEST_FINAL_CLTV` blocks as otherwise the transaction is consensus-invalid due to its + // locktime. + connect_blocks(&nodes[1], TEST_FINAL_CLTV); // B will generate justice tx from A's revoked commitment/HTLC tx connect_block(&nodes[1], &create_dummy_block(nodes[1].best_block_hash(), 42, vec![revoked_local_txn[0].clone(), revoked_htlc_txn[0].clone()])); check_closed_broadcast!(nodes[1], true); diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index e2c76643348..7a20a79159a 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -10,7 +10,7 @@ //! Further functional tests which test blockchain reorganizations. use crate::sign::{ecdsa::EcdsaChannelSigner, OutputSpender, SpendableOutputDescriptor}; -use crate::chain::channelmonitor::{ANTI_REORG_DELAY, ARCHIVAL_DELAY_BLOCKS,LATENCY_GRACE_PERIOD_BLOCKS, Balance, BalanceSource, ChannelMonitorUpdateStep}; +use crate::chain::channelmonitor::{ANTI_REORG_DELAY, ARCHIVAL_DELAY_BLOCKS,LATENCY_GRACE_PERIOD_BLOCKS, COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE, Balance, BalanceSource, ChannelMonitorUpdateStep}; use crate::chain::transaction::OutPoint; use crate::chain::chaininterface::{ConfirmationTarget, LowerBoundedFeeEstimator, compute_feerate_sat_per_1000_weight}; use crate::events::bump_transaction::{BumpTransactionEvent, WalletSource}; @@ -1734,6 +1734,12 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) { assert_eq!(revoked_htlc_success.lock_time, LockTime::ZERO); assert_ne!(revoked_htlc_timeout.lock_time, LockTime::ZERO); + // First connect blocks until the HTLC expires with + // `COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE` blocks, making us consider all the HTLCs + // pinnable claims, which the remainder of the test assumes. + connect_blocks(&nodes[0], TEST_FINAL_CLTV - COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE); + expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!(&nodes[0], + [HTLCDestination::FailedPayment { payment_hash: failed_payment_hash }]); // A will generate justice tx from B's revoked commitment/HTLC tx mine_transaction(&nodes[0], &revoked_local_txn[0]); check_closed_broadcast!(nodes[0], true); @@ -1846,8 +1852,6 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) { sorted_vec(nodes[0].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances())); connect_blocks(&nodes[0], revoked_htlc_timeout.lock_time.to_consensus_u32() - nodes[0].best_block_info().1); - expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!(&nodes[0], - [HTLCDestination::FailedPayment { payment_hash: failed_payment_hash }]); // As time goes on A may split its revocation claim transaction into multiple. let as_fewer_input_rbf = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); for tx in as_fewer_input_rbf.iter() {