From e93622b843780735eccde214695629cc666a6fe8 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 27 Jan 2025 18:04:38 +0000 Subject: [PATCH 1/3] Drop `counterparty_spendable_height` accessor `counterparty_spendable_height` is not used outside of `package.rs` so there's not much reason to have an accessor for it. Also, in the next commit an issue with setting the correct value for revoked counterparty HTLC outputs is fixed, and the upgrade path causes the value to be 0 in some cases, making using the value in too many places somewhat fraught. --- lightning/src/chain/package.rs | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 52ccf03f1..d3c1e66ed 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -834,17 +834,17 @@ impl PackageTemplate { // Now check that we only merge packages if they are both unpinnable or both // pinnable. let self_pinnable = self_cluster == AggregationCluster::Pinnable || - self.counterparty_spendable_height() <= cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE; + self.counterparty_spendable_height <= cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE; let other_pinnable = other_cluster == AggregationCluster::Pinnable || - other.counterparty_spendable_height() <= cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE; + other.counterparty_spendable_height <= cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE; if self_pinnable && other_pinnable { return true; } let self_unpinnable = self_cluster == AggregationCluster::Unpinnable && - self.counterparty_spendable_height() > cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE; + self.counterparty_spendable_height > cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE; let other_unpinnable = other_cluster == AggregationCluster::Unpinnable && - other.counterparty_spendable_height() > cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE; + other.counterparty_spendable_height > cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE; if self_unpinnable && other_unpinnable { return true; } @@ -855,13 +855,6 @@ impl PackageTemplate { pub(crate) fn is_malleable(&self) -> bool { matches!(self.malleability, PackageMalleability::Malleable(..)) } - /// The height at which our counterparty may be able to spend this output. - /// - /// This is an important limit for aggregation as after this height our counterparty may be - /// able to pin transactions spending this output in the mempool. - pub(crate) fn counterparty_spendable_height(&self) -> u32 { - self.counterparty_spendable_height - } pub(crate) fn previous_feerate(&self) -> u64 { self.feerate_previous } From 5eb0a34ca4adbdc7699bea2a09520cb886cad2db Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 27 Jan 2025 18:07:05 +0000 Subject: [PATCH 2/3] Set correct `counterparty_spendable_height` on c.p. revoked HTLCs 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 fix the `counterparty_spendable_height` value we set on counterparty revoked HTLC claims to match reality. Note that because we still consider these outputs `Pinnable` the value is not used. In the next commit we'll start making them `Unpinnable` which will actually change behavior. Note that when upgrading we have to wipe the `counterparty_spendable_height` value for non-offered HTLCs as otherwise we'd consider them `Unpinnable` when they are, in fact, `Pinnable`. --- lightning/src/chain/channelmonitor.rs | 7 ++++++- lightning/src/chain/package.rs | 22 ++++++++++++++++++---- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 2d5667a15..a81d0e04e 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -3556,11 +3556,16 @@ impl ChannelMonitorImpl { return (claimable_outpoints, to_counterparty_output_info); } let revk_htlc_outp = RevokedHTLCOutput::build(per_commitment_point, self.counterparty_commitment_params.counterparty_delayed_payment_base_key, self.counterparty_commitment_params.counterparty_htlc_base_key, per_commitment_key, htlc.amount_msat / 1000, htlc.clone(), &self.onchain_tx_handler.channel_transaction_parameters.channel_type_features); + let counterparty_spendable_height = if htlc.offered { + htlc.cltv_expiry + } else { + height + }; let justice_package = PackageTemplate::build_package( commitment_txid, transaction_output_index, PackageSolvingData::RevokedHTLCOutput(revk_htlc_outp), - htlc.cltv_expiry, + counterparty_spendable_height, ); claimable_outpoints.push(justice_package); } diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index d3c1e66ed..bd74dcd6f 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -771,10 +771,12 @@ pub struct PackageTemplate { /// Block height at which our counterparty can potentially claim this output as well (assuming /// they have the keys or information required to do so). /// - /// This is used primarily by external consumers to decide when an output becomes "pinnable" - /// because the counterparty can potentially spend it. It is also used internally by - /// [`Self::get_height_timer`] to identify when an output must be claimed by, depending on the - /// type of output. + /// This is used primarily to decide when an output becomes "pinnable" because the counterparty + /// can potentially spend it. It is also used internally by [`Self::get_height_timer`] to + /// identify when an output must be claimed by, depending on the type of output. + /// + /// Note that for revoked counterparty HTLC outputs the value may be zero in some cases where + /// we upgraded from LDK 0.1 or prior. counterparty_spendable_height: u32, // Cache of package feerate committed at previous (re)broadcast. If bumping resources // (either claimed output value or external utxo), it will keep increasing until holder @@ -1218,6 +1220,18 @@ impl Readable for PackageTemplate { (4, _height_original, option), // Written with a dummy value since 0.1 (6, height_timer, option), }); + for (_, input) in &inputs { + if let PackageSolvingData::RevokedHTLCOutput(RevokedHTLCOutput { htlc, .. }) = input { + // LDK versions through 0.1 set the wrong counterparty_spendable_height for + // non-offered revoked HTLCs (ie HTLCs we sent to our counterparty which they can + // claim with a preimage immediately). Here we detect this and reset the value to + // zero, as the value is unused except for merging decisions which doesn't care + // about any values below the current height. + if !htlc.offered && htlc.cltv_expiry == counterparty_spendable_height { + counterparty_spendable_height = 0; + } + } + } Ok(PackageTemplate { inputs, malleability, From 6c57a1fb429e502c43ececdaee5d158fd43520a6 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 27 Jan 2025 18:07:11 +0000 Subject: [PATCH 3/3] 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 | 79 ++++++++++++++++------------ lightning/src/ln/monitor_tests.rs | 10 ++-- 3 files changed, 60 insertions(+), 38 deletions(-) diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index bd74dcd6f..0686d44d5 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 177306cfd..47910febe 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,28 @@ 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 in more than COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE blocks, 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 +4963,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 +5001,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 e2c766433..7a20a7915 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() {