From c915c99150ea882453f91c483ceb2b27f907d216 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 10 Jul 2025 00:41:37 +0000 Subject: [PATCH 1/5] Add a test utility to provide nodes with anchor reserves In a number of tests we require available UTXOs to do HTLC anchor claims by bringing our own fees. We previously wrote that out in each test, which is somewhat verbose, so here we simply add a test utility that gives each node a full BTC in a single UTXO. --- lightning/src/ln/functional_test_utils.rs | 22 ++++ lightning/src/ln/monitor_tests.rs | 147 ++-------------------- 2 files changed, 35 insertions(+), 134 deletions(-) diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 4761420d9dc..dac63c8b33f 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -393,6 +393,28 @@ fn do_connect_block_without_consistency_checks<'a, 'b, 'c, 'd>( } } +pub fn provide_anchor_reserves<'a, 'b, 'c>(nodes: &[Node<'a, 'b, 'c>]) -> Transaction { + let mut output = Vec::with_capacity(nodes.len()); + for node in nodes { + output.push(TxOut { + value: Amount::ONE_BTC, + script_pubkey: node.wallet_source.get_change_script().unwrap(), + }); + } + let tx = Transaction { + version: TxVersion::TWO, + lock_time: LockTime::ZERO, + input: vec![TxIn { ..Default::default() }], + output, + }; + let height = nodes[0].best_block_info().1 + 1; + let block = create_dummy_block(nodes[0].best_block_hash(), height, vec![tx.clone()]); + for node in nodes { + do_connect_block_with_consistency_checks(node, block.clone(), false); + } + tx +} + pub fn disconnect_blocks<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, count: u32) { call_claimable_balances(node); eprintln!( diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index 49666908507..40807d867be 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -11,7 +11,6 @@ //! Further functional tests which test blockchain reorganizations. -use crate::events::bump_transaction::sync::WalletSourceSync; use crate::sign::{ecdsa::EcdsaChannelSigner, OutputSpender, SignerProvider, SpendableOutputDescriptor}; 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; @@ -465,25 +464,7 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) { let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(user_config.clone()), Some(user_config)]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); - let coinbase_tx = Transaction { - version: Version::TWO, - lock_time: LockTime::ZERO, - input: vec![TxIn { ..Default::default() }], - output: vec![ - TxOut { - value: Amount::ONE_BTC, - script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(), - }, - TxOut { - value: Amount::ONE_BTC, - script_pubkey: nodes[1].wallet_source.get_change_script().unwrap(), - }, - ], - }; - if anchors { - nodes[0].wallet_source.add_utxo(bitcoin::OutPoint { txid: coinbase_tx.compute_txid(), vout: 0 }, coinbase_tx.output[0].value); - nodes[1].wallet_source.add_utxo(bitcoin::OutPoint { txid: coinbase_tx.compute_txid(), vout: 1 }, coinbase_tx.output[1].value); - } + let coinbase_tx = provide_anchor_reserves(&nodes); let (_, _, chan_id, funding_tx) = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 1_000_000); @@ -872,25 +853,7 @@ fn do_test_balances_on_local_commitment_htlcs(anchors: bool) { let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(user_config.clone()), Some(user_config)]); let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); - let coinbase_tx = Transaction { - version: Version::TWO, - lock_time: LockTime::ZERO, - input: vec![TxIn { ..Default::default() }], - output: vec![ - TxOut { - value: Amount::ONE_BTC, - script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(), - }, - TxOut { - value: Amount::ONE_BTC, - script_pubkey: nodes[1].wallet_source.get_change_script().unwrap(), - }, - ], - }; - if anchors { - nodes[0].wallet_source.add_utxo(bitcoin::OutPoint { txid: coinbase_tx.compute_txid(), vout: 0 }, coinbase_tx.output[0].value); - nodes[1].wallet_source.add_utxo(bitcoin::OutPoint { txid: coinbase_tx.compute_txid(), vout: 1 }, coinbase_tx.output[1].value); - } + let coinbase_tx = provide_anchor_reserves(&nodes); // Create a single channel with two pending HTLCs from nodes[0] to nodes[1], one which nodes[1] // knows the preimage for, one which it does not. @@ -1655,25 +1618,7 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) { let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(user_config.clone()), Some(user_config)]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); - let coinbase_tx = Transaction { - version: Version::TWO, - lock_time: LockTime::ZERO, - input: vec![TxIn { ..Default::default() }], - output: vec![ - TxOut { - value: Amount::ONE_BTC, - script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(), - }, - TxOut { - value: Amount::ONE_BTC, - script_pubkey: nodes[1].wallet_source.get_change_script().unwrap(), - }, - ], - }; - if anchors { - nodes[0].wallet_source.add_utxo(bitcoin::OutPoint { txid: coinbase_tx.compute_txid(), vout: 0 }, coinbase_tx.output[0].value); - nodes[1].wallet_source.add_utxo(bitcoin::OutPoint { txid: coinbase_tx.compute_txid(), vout: 1 }, coinbase_tx.output[1].value); - } + let coinbase_tx = provide_anchor_reserves(&nodes); // Create some initial channels let (_, _, chan_id, funding_tx) = @@ -1956,16 +1901,7 @@ fn do_test_revoked_counterparty_aggregated_claims(anchors: bool) { let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(user_config.clone()), Some(user_config)]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); - let coinbase_tx = Transaction { - version: Version::TWO, - lock_time: LockTime::ZERO, - input: vec![TxIn { ..Default::default() }], - output: vec![TxOut { - value: Amount::ONE_BTC, - script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(), - }], - }; - nodes[0].wallet_source.add_utxo(bitcoin::OutPoint { txid: coinbase_tx.compute_txid(), vout: 0 }, coinbase_tx.output[0].value); + let coinbase_tx = provide_anchor_reserves(&nodes); let (_, _, chan_id, funding_tx) = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 100_000_000); @@ -2246,25 +2182,7 @@ fn do_test_claimable_balance_correct_while_payment_pending(outbound_payment: boo let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[Some(user_config.clone()), Some(user_config.clone()), Some(user_config)]); let nodes = create_network(3, &node_cfgs, &node_chanmgrs); - let coinbase_tx = Transaction { - version: Version::TWO, - lock_time: LockTime::ZERO, - input: vec![TxIn { ..Default::default() }], - output: vec![ - TxOut { - value: Amount::ONE_BTC, - script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(), - }, - TxOut { - value: Amount::ONE_BTC, - script_pubkey: nodes[1].wallet_source.get_change_script().unwrap(), - }, - ], - }; - if anchors { - nodes[0].wallet_source.add_utxo(bitcoin::OutPoint { txid: coinbase_tx.compute_txid(), vout: 0 }, coinbase_tx.output[0].value); - nodes[1].wallet_source.add_utxo(bitcoin::OutPoint { txid: coinbase_tx.compute_txid(), vout: 1 }, coinbase_tx.output[1].value); - } + provide_anchor_reserves(&nodes); // Create a channel from A -> B let (_, _, chan_ab_id, funding_tx_ab) = @@ -2411,6 +2329,8 @@ fn do_test_monitor_rebroadcast_pending_claims(anchors: bool) { let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(config.clone()), Some(config)]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); + let coinbase_tx = provide_anchor_reserves(&nodes); + let (_, _, _, chan_id, funding_tx) = create_chan_between_nodes_with_value( &nodes[0], &nodes[1], 1_000_000, 500_000_000 ); @@ -2429,17 +2349,6 @@ fn do_test_monitor_rebroadcast_pending_claims(anchors: bool) { false, [nodes[1].node.get_our_node_id()], 1000000); check_added_monitors(&nodes[0], 1); - let coinbase_tx = Transaction { - version: Version::TWO, - lock_time: LockTime::ZERO, - input: vec![TxIn { ..Default::default() }], - output: vec![TxOut { // UTXO to attach fees to `htlc_tx` on anchors - value: Amount::ONE_BTC, - script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(), - }], - }; - nodes[0].wallet_source.add_utxo(bitcoin::OutPoint { txid: coinbase_tx.compute_txid(), vout: 0 }, coinbase_tx.output[0].value); - // Set up a helper closure we'll use throughout our test. We should only expect retries without // bumps if fees have not increased after a block has been connected (assuming the height timer // re-evaluates at every block) or after `ChainMonitor::rebroadcast_pending_claims` is called. @@ -2543,6 +2452,8 @@ fn do_test_yield_anchors_events(have_htlcs: bool) { let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(anchors_config.clone()), Some(anchors_config)]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); + let coinbase_tx = provide_anchor_reserves(&nodes); + let (_, _, chan_id, funding_tx) = create_announced_chan_between_nodes_with_value( &nodes, 0, 1, 1_000_000, 500_000_000 ); @@ -2618,16 +2529,6 @@ fn do_test_yield_anchors_events(have_htlcs: bool) { assert_eq!(holder_events.len(), 1); let (commitment_tx, anchor_tx) = match holder_events.pop().unwrap() { Event::BumpTransaction(event) => { - let coinbase_tx = Transaction { - version: Version::TWO, - lock_time: LockTime::ZERO, - input: vec![TxIn { ..Default::default() }], - output: vec![TxOut { // UTXO to attach fees to `anchor_tx` - value: Amount::ONE_BTC, - script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(), - }], - }; - nodes[0].wallet_source.add_utxo(bitcoin::OutPoint { txid: coinbase_tx.compute_txid(), vout: 0 }, coinbase_tx.output[0].value); nodes[0].bump_tx_handler.handle_event(&event); let mut txn = nodes[0].tx_broadcaster.unique_txn_broadcast(); assert_eq!(txn.len(), 2); @@ -2743,6 +2644,8 @@ fn test_anchors_aggregated_revoked_htlc_tx() { let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); + let coinbase_tx = provide_anchor_reserves(&nodes); + let chan_a = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 20_000_000); let chan_b = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 20_000_000); @@ -2801,18 +2704,7 @@ fn test_anchors_aggregated_revoked_htlc_tx() { assert_eq!(events.len(), 2); let mut revoked_commitment_txs = Vec::with_capacity(events.len()); let mut anchor_txs = Vec::with_capacity(events.len()); - for (idx, event) in events.into_iter().enumerate() { - let utxo_value = Amount::ONE_BTC * (idx + 1) as u64; - let coinbase_tx = Transaction { - version: Version::TWO, - lock_time: LockTime::ZERO, - input: vec![TxIn { ..Default::default() }], - output: vec![TxOut { // UTXO to attach fees to `anchor_tx` - value: utxo_value, - script_pubkey: nodes[1].wallet_source.get_change_script().unwrap(), - }], - }; - nodes[1].wallet_source.add_utxo(bitcoin::OutPoint { txid: coinbase_tx.compute_txid(), vout: 0 }, utxo_value); + for event in events { match event { Event::BumpTransaction(event) => nodes[1].bump_tx_handler.handle_event(&event), _ => panic!("Unexpected event"), @@ -3130,20 +3022,7 @@ fn do_test_monitor_claims_with_random_signatures(anchors: bool, confirm_counterp let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(user_config.clone()), Some(user_config)]); let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); - let coinbase_tx = Transaction { - version: Version::TWO, - lock_time: LockTime::ZERO, - input: vec![TxIn { ..Default::default() }], - output: vec![ - TxOut { - value: Amount::ONE_BTC, - script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(), - }, - ], - }; - if anchors { - nodes[0].wallet_source.add_utxo(bitcoin::OutPoint { txid: coinbase_tx.compute_txid(), vout: 0 }, coinbase_tx.output[0].value); - } + let coinbase_tx = provide_anchor_reserves(&nodes); // Open a channel and route a payment. We'll let it timeout to claim it. let (_, _, chan_id, funding_tx) = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0); From 92fe3aa01aa36bf0724d1db251953b11902df49c Mon Sep 17 00:00:00 2001 From: Fuyin Date: Fri, 11 Jul 2025 01:13:32 +0800 Subject: [PATCH 2/5] Prune locktimed packages when inputs are spent We have to prune locktimed packages when their inputs are spent, otherwise the notification of the watched outputs might be missed. This can lead to locktimed packages with spent inputs being added back to the pending claim requests in the future, and they are never cleaned up until node restart. Resolves: #3859 --- lightning/src/chain/onchaintx.rs | 21 +++++++++++ lightning/src/ln/functional_tests.rs | 54 ++++++++-------------------- lightning/src/ln/monitor_tests.rs | 15 +++----- 3 files changed, 39 insertions(+), 51 deletions(-) diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 8e5cd0e4ceb..290d5925a17 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -269,6 +269,9 @@ pub struct OnchainTxHandler { #[cfg(not(any(test, feature = "_test_utils")))] claimable_outpoints: HashMap, + #[cfg(any(test, feature = "_test_utils"))] + pub(crate) locktimed_packages: BTreeMap>, + #[cfg(not(any(test, feature = "_test_utils")))] locktimed_packages: BTreeMap>, onchain_events_awaiting_threshold_conf: Vec, @@ -994,6 +997,17 @@ impl OnchainTxHandler { panic!("Inconsistencies between pending_claim_requests map and claimable_outpoints map"); } } + + // Also remove/split any locktimed packages whose inputs have been spent by this transaction. + self.locktimed_packages.retain(|_locktime, packages|{ + packages.retain_mut(|package| { + if let Some(p) = package.split_package(&inp.previous_output) { + claimed_outputs_material.push(p); + } + !package.outpoints().is_empty() + }); + !packages.is_empty() + }); } for package in claimed_outputs_material.drain(..) { let entry = OnchainEventEntry { @@ -1135,6 +1149,13 @@ impl OnchainTxHandler { //- resurect outpoint back in its claimable set and regenerate tx match entry.event { OnchainEvent::ContentiousOutpoint { package } => { + // We pass 0 to `package_locktime` to get the actual required locktime. + let package_locktime = package.package_locktime(0); + if package_locktime >= height { + self.locktimed_packages.entry(package_locktime).or_default().push(package); + continue; + } + if let Some(pending_claim) = self.claimable_outpoints.get(package.outpoints()[0]) { if let Some(request) = self.pending_claim_requests.get_mut(&pending_claim.0) { assert!(request.merge_package(package, height).is_ok()); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 4c7318d1cd9..9e2b7e3de20 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -1961,45 +1961,9 @@ pub fn test_htlc_on_chain_success() { _ => panic!("Unexpected event"), } - macro_rules! check_tx_local_broadcast { - ($node: expr, $htlc_offered: expr, $commitment_tx: expr) => {{ - let mut node_txn = $node.tx_broadcaster.txn_broadcasted.lock().unwrap(); - // HTLC timeout claims for non-anchor channels are only aggregated when claimed from the - // remote commitment transaction. - if $htlc_offered { - assert_eq!(node_txn.len(), 2); - for tx in node_txn.iter() { - check_spends!(tx, $commitment_tx); - assert_ne!(tx.lock_time, LockTime::ZERO); - assert_eq!( - tx.input[0].witness.last().unwrap().len(), - OFFERED_HTLC_SCRIPT_WEIGHT - ); - assert!(tx.output[0].script_pubkey.is_p2wsh()); // revokeable output - } - assert_ne!( - node_txn[0].input[0].previous_output, - node_txn[1].input[0].previous_output - ); - } else { - assert_eq!(node_txn.len(), 1); - check_spends!(node_txn[0], $commitment_tx); - assert_ne!(node_txn[0].lock_time, LockTime::ZERO); - assert_eq!( - node_txn[0].input[0].witness.last().unwrap().len(), - ACCEPTED_HTLC_SCRIPT_WEIGHT - ); - assert!(node_txn[0].output[0].script_pubkey.is_p2wpkh()); // direct payment - assert_ne!( - node_txn[0].input[0].previous_output, - node_txn[0].input[1].previous_output - ); - } - node_txn.clear(); - }}; - } - // nodes[1] now broadcasts its own timeout-claim of the output that nodes[2] just claimed via success. - check_tx_local_broadcast!(nodes[1], false, commitment_tx[0]); + // nodes[1] does not broadcast its own timeout-claim of the output as nodes[2] just claimed it + // via success. + assert!(nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty()); // Broadcast legit commitment tx from A on B's chain // Broadcast preimage tx by B on offered output from A commitment tx on A's chain @@ -2061,7 +2025,17 @@ pub fn test_htlc_on_chain_success() { _ => panic!("Unexpected event"), } } - check_tx_local_broadcast!(nodes[0], true, node_a_commitment_tx[0]); + // HTLC timeout claims for non-anchor channels are only aggregated when claimed from the + // remote commitment transaction. + let mut node_txn = nodes[0].tx_broadcaster.txn_broadcast(); + assert_eq!(node_txn.len(), 2); + for tx in node_txn.iter() { + check_spends!(tx, node_a_commitment_tx[0]); + assert_ne!(tx.lock_time, LockTime::ZERO); + assert_eq!(tx.input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT); + assert!(tx.output[0].script_pubkey.is_p2wsh()); // revokeable output + } + assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output); } fn do_test_htlc_on_chain_timeout(connect_style: ConnectStyle) { diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index 40807d867be..103ca7e378e 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -717,8 +717,9 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) { test_spendable_output(&nodes[0], &remote_txn[0], false); assert!(nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty()); - // After broadcasting the HTLC claim transaction, node A will still consider the HTLC - // possibly-claimable up to ANTI_REORG_DELAY, at which point it will drop it. + // After confirming the HTLC claim transaction, node A will no longer attempt to claim said + // HTLC, unless the transaction is reorged. However, we'll still report a + // `MaybeTimeoutClaimableHTLC` balance for it until we reach `ANTI_REORG_DELAY` confirmations. mine_transaction(&nodes[0], &b_broadcast_txn[0]); if prev_commitment_tx { expect_payment_path_successful!(nodes[0]); @@ -734,18 +735,10 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) { // When the HTLC timeout output is spendable in the next block, A should broadcast it connect_blocks(&nodes[0], htlc_cltv_timeout - nodes[0].best_block_info().1); let a_broadcast_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); - // Aggregated claim transaction. assert_eq!(a_broadcast_txn.len(), 1); check_spends!(a_broadcast_txn[0], remote_txn[0]); - assert_eq!(a_broadcast_txn[0].input.len(), 2); - assert_ne!(a_broadcast_txn[0].input[0].previous_output.vout, a_broadcast_txn[0].input[1].previous_output.vout); - // a_broadcast_txn [0] and [1] should spend the HTLC outputs of the commitment tx - assert!(a_broadcast_txn[0].input.iter().any(|input| remote_txn[0].output[input.previous_output.vout as usize].value.to_sat() == 3_000)); + assert_eq!(a_broadcast_txn[0].input.len(), 1); assert!(a_broadcast_txn[0].input.iter().any(|input| remote_txn[0].output[input.previous_output.vout as usize].value.to_sat() == 4_000)); - - // Confirm node B's claim for node A to remove that claim from the aggregated claim transaction. - mine_transaction(&nodes[0], &b_broadcast_txn[0]); - let a_broadcast_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); let a_htlc_timeout_tx = a_broadcast_txn.into_iter().next_back().unwrap(); // Once the HTLC-Timeout transaction confirms, A will no longer consider the HTLC From f916e13328c0db13e34810712cbcf4441cd4573d Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 10 Jul 2025 17:03:12 +0000 Subject: [PATCH 3/5] Track outpoint creation height in `PackageSolvingData` When we have an outpoint to claim which is lock-timed and the locktime is reached, we add it to `OnchainTxHandler::claimable_outpoints` to indicate the outpoint is now being claimed. However, `claimable_outpoints` is supposed to track when the outpoint first appeared on chain so that we can remove the claim if the outpoint is reorged out. Sadly, in the handling for lock-timed packages, we incorrectly stored the current height in `claimable_outpoints`, causing such claims to be removed in case of a reorg right after they were generated, even if the output we intend to claim isn't removed at all. Here we start tracking when the outpoint we're spending was created in `PackageSolvingData`'s constituent types. While we could have tracked this information in `PackageTemplate`, it would preclude later merging packages that are spending outpoints included in different blocks, which we don't necessarily want to do. --- lightning/src/chain/channelmonitor.rs | 51 +++++++++++++-------- lightning/src/chain/onchaintx.rs | 26 ++++++----- lightning/src/chain/package.rs | 65 ++++++++++++++++++++------- 3 files changed, 96 insertions(+), 46 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 26afad0d953..1819e82f624 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -3514,23 +3514,26 @@ impl ChannelMonitorImpl { (payment_preimage.clone(), payment_info.clone().into_iter().collect()) }); - let confirmed_spend_txid = self.funding_spend_confirmed.or_else(|| { - self.onchain_events_awaiting_threshold_conf.iter().find_map(|event| match event.event { - OnchainEvent::FundingSpendConfirmation { .. } => Some(event.txid), - _ => None, - }) - }); - let confirmed_spend_txid = if let Some(txid) = confirmed_spend_txid { - txid - } else { - return; - }; + let confirmed_spend_info = self.funding_spend_confirmed + .map(|txid| (txid, None)) + .or_else(|| { + self.onchain_events_awaiting_threshold_conf.iter().find_map(|event| match event.event { + OnchainEvent::FundingSpendConfirmation { .. } => Some((event.txid, Some(event.height))), + _ => None, + }) + }); + let (confirmed_spend_txid, confirmed_spend_height) = + if let Some((txid, height)) = confirmed_spend_info { + (txid, height) + } else { + return; + }; // If the channel is force closed, try to claim the output from this preimage. // First check if a counterparty commitment transaction has been broadcasted: macro_rules! claim_htlcs { ($commitment_number: expr, $txid: expr, $htlcs: expr) => { - let (htlc_claim_reqs, _) = self.get_counterparty_output_claim_info($commitment_number, $txid, None, $htlcs); + let (htlc_claim_reqs, _) = self.get_counterparty_output_claim_info($commitment_number, $txid, None, $htlcs, confirmed_spend_height); let conf_target = self.closure_conf_target(); self.onchain_tx_handler.update_claims_view_from_requests( htlc_claim_reqs, self.best_block.height, self.best_block.height, broadcaster, @@ -4226,6 +4229,7 @@ impl ChannelMonitorImpl { per_commitment_point, per_commitment_key, outp.value, self.funding.channel_parameters.channel_type_features.supports_anchors_zero_fee_htlc_tx(), self.funding.channel_parameters.clone(), + height, ); let justice_package = PackageTemplate::build_package( commitment_txid, idx as u32, @@ -4250,6 +4254,7 @@ impl ChannelMonitorImpl { let revk_htlc_outp = RevokedHTLCOutput::build( per_commitment_point, per_commitment_key, htlc.clone(), self.funding.channel_parameters.clone(), + height, ); let counterparty_spendable_height = if htlc.offered { htlc.cltv_expiry @@ -4304,7 +4309,7 @@ impl ChannelMonitorImpl { (htlc, htlc_source.as_ref().map(|htlc_source| htlc_source.as_ref())) ), logger); let (htlc_claim_reqs, counterparty_output_info) = - self.get_counterparty_output_claim_info(commitment_number, commitment_txid, Some(tx), per_commitment_option); + self.get_counterparty_output_claim_info(commitment_number, commitment_txid, Some(tx), per_commitment_option, Some(height)); to_counterparty_output_info = counterparty_output_info; for req in htlc_claim_reqs { claimable_outpoints.push(req); @@ -4316,8 +4321,11 @@ impl ChannelMonitorImpl { /// Returns the HTLC claim package templates and the counterparty output info #[rustfmt::skip] - fn get_counterparty_output_claim_info(&self, commitment_number: u64, commitment_txid: Txid, tx: Option<&Transaction>, per_commitment_option: Option<&Vec<(HTLCOutputInCommitment, Option>)>>) - -> (Vec, CommitmentTxCounterpartyOutputInfo) { + fn get_counterparty_output_claim_info( + &self, commitment_number: u64, commitment_txid: Txid, tx: Option<&Transaction>, + per_commitment_option: Option<&Vec<(HTLCOutputInCommitment, Option>)>>, + confirmation_height: Option, + ) -> (Vec, CommitmentTxCounterpartyOutputInfo) { let mut claimable_outpoints = Vec::new(); let mut to_counterparty_output_info: CommitmentTxCounterpartyOutputInfo = None; @@ -4374,15 +4382,19 @@ impl ChannelMonitorImpl { let counterparty_htlc_outp = if htlc.offered { PackageSolvingData::CounterpartyOfferedHTLCOutput( CounterpartyOfferedHTLCOutput::build( - *per_commitment_point, preimage.unwrap(), htlc.clone(), + *per_commitment_point, preimage.unwrap(), + htlc.clone(), self.funding.channel_parameters.clone(), + confirmation_height, ) ) } else { PackageSolvingData::CounterpartyReceivedHTLCOutput( CounterpartyReceivedHTLCOutput::build( - *per_commitment_point, htlc.clone(), + *per_commitment_point, + htlc.clone(), self.funding.channel_parameters.clone(), + confirmation_height, ) ) }; @@ -4426,6 +4438,7 @@ impl ChannelMonitorImpl { let revk_outp = RevokedOutput::build( per_commitment_point, per_commitment_key, tx.output[idx].value, false, self.funding.channel_parameters.clone(), + height, ); let justice_package = PackageTemplate::build_package( htlc_txid, idx as u32, PackageSolvingData::RevokedOutput(revk_outp), @@ -4507,7 +4520,7 @@ impl ChannelMonitorImpl { .expect("Expected transaction output index for non-dust HTLC"); PackageTemplate::build_package( tx.txid(), transaction_output_index, - PackageSolvingData::HolderHTLCOutput(HolderHTLCOutput::build(htlc_descriptor)), + PackageSolvingData::HolderHTLCOutput(HolderHTLCOutput::build(htlc_descriptor, conf_height)), counterparty_spendable_height, ) }) @@ -4687,7 +4700,7 @@ impl ChannelMonitorImpl { let txid = self.funding.current_holder_commitment_tx.trust().txid(); let vout = htlc_descriptor.htlc.transaction_output_index .expect("Expected transaction output index for non-dust HTLC"); - let htlc_output = HolderHTLCOutput::build(htlc_descriptor); + let htlc_output = HolderHTLCOutput::build(htlc_descriptor, 0); if let Some(htlc_tx) = htlc_output.get_maybe_signed_htlc_tx( &mut self.onchain_tx_handler, &::bitcoin::OutPoint { txid, vout }, ) { diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 290d5925a17..55458ae8c98 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -1379,19 +1379,21 @@ mod tests { holder_commit_txid, htlc.transaction_output_index.unwrap(), PackageSolvingData::HolderHTLCOutput(HolderHTLCOutput::build(HTLCDescriptor { - channel_derivation_parameters: ChannelDerivationParameters { - value_satoshis: tx_handler.channel_value_satoshis, - keys_id: tx_handler.channel_keys_id, - transaction_parameters: tx_handler.channel_transaction_parameters.clone(), + channel_derivation_parameters: ChannelDerivationParameters { + value_satoshis: tx_handler.channel_value_satoshis, + keys_id: tx_handler.channel_keys_id, + transaction_parameters: tx_handler.channel_transaction_parameters.clone(), + }, + commitment_txid: holder_commit_txid, + per_commitment_number: holder_commit.commitment_number(), + per_commitment_point: holder_commit.per_commitment_point(), + feerate_per_kw: holder_commit.feerate_per_kw(), + htlc: htlc.clone(), + preimage: None, + counterparty_sig: *counterparty_sig, }, - commitment_txid: holder_commit_txid, - per_commitment_number: holder_commit.commitment_number(), - per_commitment_point: holder_commit.per_commitment_point(), - feerate_per_kw: holder_commit.feerate_per_kw(), - htlc: htlc.clone(), - preimage: None, - counterparty_sig: *counterparty_sig, - })), + 0 + )), 0, )); } diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 467e2179b43..a238d8e58e9 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -132,7 +132,7 @@ const HIGH_FREQUENCY_BUMP_INTERVAL: u32 = 1; /// /// CSV and pubkeys are used as part of a witnessScript redeeming a balance output, amount is used /// as part of the signature hash and revocation secret to generate a satisfying witness. -#[derive(Clone, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq)] pub(crate) struct RevokedOutput { per_commitment_point: PublicKey, counterparty_delayed_payment_base_key: DelayedPaymentBasepoint, @@ -143,6 +143,8 @@ pub(crate) struct RevokedOutput { on_counterparty_tx_csv: u16, is_counterparty_balance_on_anchors: Option<()>, channel_parameters: Option, + // Added in LDK 0.1.4/0.2 and always set since. + outpoint_confirmation_height: Option, } impl RevokedOutput { @@ -150,6 +152,7 @@ impl RevokedOutput { pub(crate) fn build( per_commitment_point: PublicKey, per_commitment_key: SecretKey, amount: Amount, is_counterparty_balance_on_anchors: bool, channel_parameters: ChannelTransactionParameters, + outpoint_confirmation_height: u32, ) -> Self { let directed_params = channel_parameters.as_counterparty_broadcastable(); let counterparty_keys = directed_params.broadcaster_pubkeys(); @@ -166,12 +169,14 @@ impl RevokedOutput { on_counterparty_tx_csv, is_counterparty_balance_on_anchors: if is_counterparty_balance_on_anchors { Some(()) } else { None }, channel_parameters: Some(channel_parameters), + outpoint_confirmation_height: Some(outpoint_confirmation_height), } } } impl_writeable_tlv_based!(RevokedOutput, { (0, per_commitment_point, required), + (1, outpoint_confirmation_height, option), // Added in 0.1.4/0.2 and always set (2, counterparty_delayed_payment_base_key, required), (4, counterparty_htlc_base_key, required), (6, per_commitment_key, required), @@ -190,7 +195,7 @@ impl_writeable_tlv_based!(RevokedOutput, { /// /// CSV is used as part of a witnessScript redeeming a balance output, amount is used as part /// of the signature hash and revocation secret to generate a satisfying witness. -#[derive(Clone, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq)] pub(crate) struct RevokedHTLCOutput { per_commitment_point: PublicKey, counterparty_delayed_payment_base_key: DelayedPaymentBasepoint, @@ -200,12 +205,15 @@ pub(crate) struct RevokedHTLCOutput { amount: u64, htlc: HTLCOutputInCommitment, channel_parameters: Option, + // Added in LDK 0.1.4/0.2 and always set since. + outpoint_confirmation_height: Option, } impl RevokedHTLCOutput { pub(crate) fn build( per_commitment_point: PublicKey, per_commitment_key: SecretKey, htlc: HTLCOutputInCommitment, channel_parameters: ChannelTransactionParameters, + outpoint_confirmation_height: u32, ) -> Self { let weight = if htlc.offered { weight_revoked_offered_htlc(&channel_parameters.channel_type_features) @@ -225,12 +233,14 @@ impl RevokedHTLCOutput { amount: htlc.amount_msat / 1000, htlc, channel_parameters: Some(channel_parameters), + outpoint_confirmation_height: Some(outpoint_confirmation_height), } } } impl_writeable_tlv_based!(RevokedHTLCOutput, { (0, per_commitment_point, required), + (1, outpoint_confirmation_height, option), // Added in 0.1.4/0.2 and always set (2, counterparty_delayed_payment_base_key, required), (4, counterparty_htlc_base_key, required), (6, per_commitment_key, required), @@ -248,7 +258,7 @@ impl_writeable_tlv_based!(RevokedHTLCOutput, { /// The preimage is used as part of the witness. /// /// Note that on upgrades, some features of existing outputs may be missed. -#[derive(Clone, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq)] pub(crate) struct CounterpartyOfferedHTLCOutput { per_commitment_point: PublicKey, counterparty_delayed_payment_base_key: DelayedPaymentBasepoint, @@ -257,12 +267,15 @@ pub(crate) struct CounterpartyOfferedHTLCOutput { htlc: HTLCOutputInCommitment, channel_type_features: ChannelTypeFeatures, channel_parameters: Option, + // Added in LDK 0.1.4/0.2 and always set since. + outpoint_confirmation_height: Option, } impl CounterpartyOfferedHTLCOutput { pub(crate) fn build( per_commitment_point: PublicKey, preimage: PaymentPreimage, htlc: HTLCOutputInCommitment, channel_parameters: ChannelTransactionParameters, + outpoint_confirmation_height: Option, ) -> Self { let directed_params = channel_parameters.as_counterparty_broadcastable(); let counterparty_keys = directed_params.broadcaster_pubkeys(); @@ -277,6 +290,7 @@ impl CounterpartyOfferedHTLCOutput { htlc, channel_type_features, channel_parameters: Some(channel_parameters), + outpoint_confirmation_height, } } } @@ -287,6 +301,7 @@ impl Writeable for CounterpartyOfferedHTLCOutput { let legacy_deserialization_prevention_marker = chan_utils::legacy_deserialization_prevention_marker_for_channel_type_features(&self.channel_type_features); write_tlv_fields!(writer, { (0, self.per_commitment_point, required), + (1, self.outpoint_confirmation_height, option), // Added in 0.1.4/0.2, not always set (2, self.counterparty_delayed_payment_base_key, required), (4, self.counterparty_htlc_base_key, required), (6, self.preimage, required), @@ -309,9 +324,11 @@ impl Readable for CounterpartyOfferedHTLCOutput { let mut _legacy_deserialization_prevention_marker: Option<()> = None; let mut channel_type_features = None; let mut channel_parameters = None; + let mut outpoint_confirmation_height = None; read_tlv_fields!(reader, { (0, per_commitment_point, required), + (1, outpoint_confirmation_height, option), // Added in 0.1.4/0.2, not always set (2, counterparty_delayed_payment_base_key, required), (4, counterparty_htlc_base_key, required), (6, preimage, required), @@ -333,6 +350,7 @@ impl Readable for CounterpartyOfferedHTLCOutput { htlc: htlc.0.unwrap(), channel_type_features, channel_parameters, + outpoint_confirmation_height, }) } } @@ -343,7 +361,7 @@ impl Readable for CounterpartyOfferedHTLCOutput { /// witnessScript. /// /// Note that on upgrades, some features of existing outputs may be missed. -#[derive(Clone, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq)] pub(crate) struct CounterpartyReceivedHTLCOutput { per_commitment_point: PublicKey, counterparty_delayed_payment_base_key: DelayedPaymentBasepoint, @@ -351,12 +369,14 @@ pub(crate) struct CounterpartyReceivedHTLCOutput { htlc: HTLCOutputInCommitment, channel_type_features: ChannelTypeFeatures, channel_parameters: Option, + outpoint_confirmation_height: Option, } impl CounterpartyReceivedHTLCOutput { pub(crate) fn build( per_commitment_point: PublicKey, htlc: HTLCOutputInCommitment, channel_parameters: ChannelTransactionParameters, + outpoint_confirmation_height: Option, ) -> Self { let directed_params = channel_parameters.as_counterparty_broadcastable(); let counterparty_keys = directed_params.broadcaster_pubkeys(); @@ -370,6 +390,7 @@ impl CounterpartyReceivedHTLCOutput { htlc, channel_type_features, channel_parameters: Some(channel_parameters), + outpoint_confirmation_height, } } } @@ -380,6 +401,7 @@ impl Writeable for CounterpartyReceivedHTLCOutput { let legacy_deserialization_prevention_marker = chan_utils::legacy_deserialization_prevention_marker_for_channel_type_features(&self.channel_type_features); write_tlv_fields!(writer, { (0, self.per_commitment_point, required), + (1, self.outpoint_confirmation_height, option), // Added in 0.1.4/0.2, not always set (2, self.counterparty_delayed_payment_base_key, required), (4, self.counterparty_htlc_base_key, required), (6, self.htlc, required), @@ -400,9 +422,11 @@ impl Readable for CounterpartyReceivedHTLCOutput { let mut _legacy_deserialization_prevention_marker: Option<()> = None; let mut channel_type_features = None; let mut channel_parameters = None; + let mut outpoint_confirmation_height = None; read_tlv_fields!(reader, { (0, per_commitment_point, required), + (1, outpoint_confirmation_height, option), // Added in 0.1.4/0.2, not always set (2, counterparty_delayed_payment_base_key, required), (4, counterparty_htlc_base_key, required), (6, htlc, required), @@ -422,6 +446,7 @@ impl Readable for CounterpartyReceivedHTLCOutput { htlc: htlc.0.unwrap(), channel_type_features, channel_parameters, + outpoint_confirmation_height, }) } } @@ -432,7 +457,7 @@ impl Readable for CounterpartyReceivedHTLCOutput { /// Preimage is only included as part of the witness in former case. /// /// Note that on upgrades, some features of existing outputs may be missed. -#[derive(Clone, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq)] pub(crate) struct HolderHTLCOutput { preimage: Option, amount_msat: u64, @@ -440,11 +465,14 @@ pub(crate) struct HolderHTLCOutput { cltv_expiry: u32, channel_type_features: ChannelTypeFeatures, htlc_descriptor: Option, + outpoint_confirmation_height: Option, } impl HolderHTLCOutput { #[rustfmt::skip] - pub(crate) fn build(htlc_descriptor: HTLCDescriptor) -> Self { + pub(crate) fn build( + htlc_descriptor: HTLCDescriptor, outpoint_confirmation_height: u32, + ) -> Self { let amount_msat = htlc_descriptor.htlc.amount_msat; let channel_type_features = htlc_descriptor.channel_derivation_parameters .transaction_parameters.channel_type_features.clone(); @@ -462,6 +490,7 @@ impl HolderHTLCOutput { }, channel_type_features, htlc_descriptor: Some(htlc_descriptor), + outpoint_confirmation_height: Some(outpoint_confirmation_height), } } @@ -550,6 +579,7 @@ impl Writeable for HolderHTLCOutput { let legacy_deserialization_prevention_marker = chan_utils::legacy_deserialization_prevention_marker_for_channel_type_features(&self.channel_type_features); write_tlv_fields!(writer, { (0, self.amount_msat, required), + (1, self.outpoint_confirmation_height, option), // Added in 0.1.4/0.2 and always set (2, self.cltv_expiry, required), (4, self.preimage, option), (6, legacy_deserialization_prevention_marker, option), @@ -568,9 +598,11 @@ impl Readable for HolderHTLCOutput { let mut _legacy_deserialization_prevention_marker: Option<()> = None; let mut channel_type_features = None; let mut htlc_descriptor = None; + let mut outpoint_confirmation_height = None; read_tlv_fields!(reader, { (0, amount_msat, required), + (1, outpoint_confirmation_height, option), // Added in 0.1.4/0.2 and always set (2, cltv_expiry, required), (4, preimage, option), (6, _legacy_deserialization_prevention_marker, option), @@ -588,6 +620,7 @@ impl Readable for HolderHTLCOutput { preimage, channel_type_features, htlc_descriptor, + outpoint_confirmation_height, }) } } @@ -597,7 +630,7 @@ impl Readable for HolderHTLCOutput { /// witnessScript is used as part of the witness redeeming the funding utxo. /// /// Note that on upgrades, some features of existing outputs may be missed. -#[derive(Clone, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq)] pub(crate) struct HolderFundingOutput { funding_redeemscript: ScriptBuf, pub(crate) funding_amount_sats: Option, @@ -695,7 +728,7 @@ impl Readable for HolderFundingOutput { /// /// The generic API offers access to an outputs common attributes or allow transformation such as /// finalizing an input claiming the output. -#[derive(Clone, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq)] pub(crate) enum PackageSolvingData { RevokedOutput(RevokedOutput), RevokedHTLCOutput(RevokedHTLCOutput), @@ -1003,7 +1036,7 @@ impl_writeable_tlv_based_enum_legacy!(PackageSolvingData, ; /// That way we avoid claiming in too many discrete transactions while also avoiding /// unnecessarily exposing ourselves to pinning attacks or delaying claims when we could have /// claimed at least part of the available outputs quickly and without risk. -#[derive(Copy, Clone, PartialEq, Eq)] +#[derive(Copy, Clone, Debug, PartialEq, Eq)] enum AggregationCluster { /// Our counterparty can potentially claim this output. Pinnable, @@ -1014,7 +1047,7 @@ enum AggregationCluster { /// A malleable package might be aggregated with other packages to save on fees. /// A untractable package has been counter-signed and aggregable will break cached counterparty signatures. -#[derive(Copy, Clone, PartialEq, Eq)] +#[derive(Copy, Clone, Debug, PartialEq, Eq)] enum PackageMalleability { Malleable(AggregationCluster), Untractable, @@ -1029,7 +1062,7 @@ enum PackageMalleability { /// /// As packages are time-sensitive, we fee-bump and rebroadcast them at scheduled intervals. /// Failing to confirm a package translate as a loss of funds for the user. -#[derive(Clone, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq)] pub struct PackageTemplate { // List of onchain outputs and solving data to generate satisfying witnesses. inputs: Vec<(BitcoinOutPoint, PackageSolvingData)>, @@ -1690,7 +1723,7 @@ mod tests { let channel_parameters = ChannelTransactionParameters::test_dummy(0); PackageSolvingData::RevokedOutput(RevokedOutput::build( dumb_point, dumb_scalar, Amount::ZERO, $is_counterparty_balance_on_anchors, - channel_parameters, + channel_parameters, 0, )) } } @@ -1709,7 +1742,7 @@ mod tests { channel_parameters.channel_type_features = ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies(); PackageSolvingData::RevokedHTLCOutput(RevokedHTLCOutput::build( - dumb_point, dumb_scalar, htlc, channel_parameters + dumb_point, dumb_scalar, htlc, channel_parameters, 0, )) } } @@ -1727,7 +1760,7 @@ mod tests { let mut channel_parameters = ChannelTransactionParameters::test_dummy(0); channel_parameters.channel_type_features = $features; PackageSolvingData::CounterpartyReceivedHTLCOutput( - CounterpartyReceivedHTLCOutput::build(dumb_point, htlc, channel_parameters) + CounterpartyReceivedHTLCOutput::build(dumb_point, htlc, channel_parameters, None) ) } } @@ -1746,7 +1779,7 @@ mod tests { let mut channel_parameters = ChannelTransactionParameters::test_dummy(0); channel_parameters.channel_type_features = $features; PackageSolvingData::CounterpartyOfferedHTLCOutput( - CounterpartyOfferedHTLCOutput::build(dumb_point, preimage, htlc, channel_parameters) + CounterpartyOfferedHTLCOutput::build(dumb_point, preimage, htlc, channel_parameters, None) ) } } @@ -1784,6 +1817,7 @@ mod tests { preimage: Some(preimage), counterparty_sig: commitment_tx.counterparty_htlc_sigs[0].clone(), }, + 0, )) } } @@ -1820,6 +1854,7 @@ mod tests { preimage: None, counterparty_sig: commitment_tx.counterparty_htlc_sigs[0].clone(), }, + 0, )) } } From 9a9554472bf75f43c61212862cae93c25356319d Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 10 Jul 2025 17:04:07 +0000 Subject: [PATCH 4/5] Use outpoint creation height when restoring locktimed packages When we have an outpoint to claim which is lock-timed and the locktime is reached, we add it to `OnchainTxHandler::claimable_outpoints` to indicate the outpoint is now being claimed. However, `claimable_outpoints` is supposed to track when the outpoint first appeared on chain so that we can remove the claim if the outpoint is reorged out. Sadly, in the handling for lock-timed packages, we incorrectly stored the current height in `claimable_outpoints`, causing such claims to be removed in case of a reorg right after they were generated, even if the output we intend to claim isn't removed at all. Here we use the creation-height tracking added in the previous commit to actually address the issue, using the tracked height when adding a claim to `OnchainTxHandler::claimable_outpoints`. In cases where we have no information, we continue to use the current height, retaining the issue for locktimed packages on upgrades, but this simplifies cases where we actually don't have the information available anyway. --- lightning/src/chain/onchaintx.rs | 7 ++++--- lightning/src/chain/package.rs | 34 ++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 55458ae8c98..0b63f1f47f8 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -889,9 +889,10 @@ impl OnchainTxHandler { // Because fuzzing can cause hash collisions, we can end up with conflicting claim // ids here, so we only assert when not fuzzing. debug_assert!(cfg!(fuzzing) || self.pending_claim_requests.get(&claim_id).is_none()); - for k in req.outpoints() { - log_info!(logger, "Registering claiming request for {}:{}", k.txid, k.vout); - self.claimable_outpoints.insert(k.clone(), (claim_id, conf_height)); + for (k, outpoint_confirmation_height) in req.outpoints_and_creation_heights() { + let creation_height = outpoint_confirmation_height.unwrap_or(conf_height); + log_info!(logger, "Registering claiming request for {}:{}, which exists as of height {creation_height}", k.txid, k.vout); + self.claimable_outpoints.insert(k.clone(), (claim_id, creation_height)); } self.pending_claim_requests.insert(claim_id, req); } diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index a238d8e58e9..f791e1d4d36 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -810,6 +810,34 @@ impl PackageSolvingData { } } + fn input_confirmation_height(&self) -> Option { + match self { + PackageSolvingData::RevokedOutput(RevokedOutput { + outpoint_confirmation_height, + .. + }) + | PackageSolvingData::RevokedHTLCOutput(RevokedHTLCOutput { + outpoint_confirmation_height, + .. + }) + | PackageSolvingData::CounterpartyOfferedHTLCOutput(CounterpartyOfferedHTLCOutput { + outpoint_confirmation_height, + .. + }) + | PackageSolvingData::CounterpartyReceivedHTLCOutput( + CounterpartyReceivedHTLCOutput { outpoint_confirmation_height, .. }, + ) + | PackageSolvingData::HolderHTLCOutput(HolderHTLCOutput { + outpoint_confirmation_height, + .. + }) => *outpoint_confirmation_height, + // We don't bother to track `HolderFundingOutput`'s creation height as its the funding + // transaction itself and we build `HolderFundingOutput`s before we actually get the + // commitment transaction confirmed. + PackageSolvingData::HolderFundingOutput(_) => None, + } + } + #[rustfmt::skip] fn as_tx_input(&self, previous_output: BitcoinOutPoint) -> TxIn { let sequence = match self { @@ -1177,6 +1205,12 @@ impl PackageTemplate { pub(crate) fn outpoints(&self) -> Vec<&BitcoinOutPoint> { self.inputs.iter().map(|(o, _)| o).collect() } + pub(crate) fn outpoints_and_creation_heights( + &self, + ) -> impl Iterator)> { + self.inputs.iter().map(|(o, p)| (o, p.input_confirmation_height())) + } + pub(crate) fn inputs(&self) -> impl ExactSizeIterator { self.inputs.iter().map(|(_, i)| i) } From d7726eff0d9739d7e30461767f8fe07458defd53 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 10 Jul 2025 00:51:51 +0000 Subject: [PATCH 5/5] Add a test case for the issues fixed in the previous few commits This adds a single test which exercises both the ability to prune locktimed packages when inputs are spent as well as the creation-height tracking for locktimed packages. --- lightning/src/ln/reorg_tests.rs | 226 +++++++++++++++++++++++++++++++- 1 file changed, 225 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs index 9402f71dd2f..c5f512b319a 100644 --- a/lightning/src/ln/reorg_tests.rs +++ b/lightning/src/ln/reorg_tests.rs @@ -12,13 +12,14 @@ //! Further functional tests which test blockchain reorganizations. use crate::chain::chaininterface::LowerBoundedFeeEstimator; -use crate::chain::channelmonitor::{ANTI_REORG_DELAY, LATENCY_GRACE_PERIOD_BLOCKS}; +use crate::chain::channelmonitor::{ANTI_REORG_DELAY, Balance, LATENCY_GRACE_PERIOD_BLOCKS}; use crate::chain::transaction::OutPoint; use crate::chain::Confirm; use crate::events::{Event, ClosureReason, HTLCHandlingFailureType}; use crate::ln::msgs::{BaseMessageHandler, ChannelMessageHandler, Init, MessageSendEvent}; use crate::ln::types::ChannelId; use crate::sign::OutputSpender; +use crate::types::payment::PaymentHash; use crate::types::string::UntrustedString; use crate::util::ser::Writeable; @@ -899,3 +900,226 @@ fn test_retries_own_commitment_broadcast_after_reorg() { do_test_retries_own_commitment_broadcast_after_reorg(true, false); do_test_retries_own_commitment_broadcast_after_reorg(true, true); } + +fn do_test_split_htlc_expiry_tracking(use_third_htlc: bool, reorg_out: bool) { + // Previously, we had a bug where if there were two HTLCs which expired at different heights, + // and a counterparty commitment transaction confirmed spending both of them, we'd continually + // rebroadcast attempted HTLC claims against the higher-expiry HTLC forever. + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + + // This test relies on being able to consolidate HTLC claims into a single transaction, which + // requires anchors: + let mut config = test_default_channel_config(); + config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true; + config.manually_accept_inbound_channels = true; + + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(config.clone()), Some(config)]); + let nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + let coinbase_tx = provide_anchor_reserves(&nodes); + + let node_a_id = nodes[0].node.get_our_node_id(); + let node_b_id = nodes[1].node.get_our_node_id(); + + let (_, _, chan_id, funding_tx) = + create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000_000, 0); + + // Route two non-dust HTLCs with different expiry, with a third having the same expiry as the + // second if `use_third_htlc` is set. + let (preimage_a, payment_hash_a, ..) = route_payment(&nodes[0], &[&nodes[1]], 100_000_000); + connect_blocks(&nodes[0], 2); + connect_blocks(&nodes[1], 2); + let (preimage_b, payment_hash_b, ..) = route_payment(&nodes[0], &[&nodes[1]], 100_000_000); + let payment_hash_c = if use_third_htlc { + route_payment(&nodes[0], &[&nodes[1]], 100_000_000).1 + } else { + PaymentHash([0; 32]) + }; + + // First disconnect peers so that we don't have to deal with messages: + nodes[0].node.peer_disconnected(node_b_id); + nodes[1].node.peer_disconnected(node_a_id); + + // Give node B preimages so that it will claim the first two HTLCs on-chain. + nodes[1].node.claim_funds(preimage_a); + expect_payment_claimed!(nodes[1], payment_hash_a, 100_000_000); + nodes[1].node.claim_funds(preimage_b); + expect_payment_claimed!(nodes[1], payment_hash_b, 100_000_000); + check_added_monitors(&nodes[1], 2); + + let err = "Channel force-closed".to_string(); + + // Force-close and fetch node B's commitment transaction and the transaction claiming the first + // two HTLCs. + nodes[1].node.force_close_broadcasting_latest_txn(&chan_id, &node_a_id, err).unwrap(); + check_closed_broadcast(&nodes[1], 1, true); + check_added_monitors(&nodes[1], 1); + let reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) }; + check_closed_event(&nodes[1], 1, reason, false, &[node_a_id], 10_000_000); + + let mut txn = nodes[1].tx_broadcaster.txn_broadcast(); + assert_eq!(txn.len(), 1); + let commitment_tx = txn.pop().unwrap(); + check_spends!(commitment_tx, funding_tx); + + mine_transaction(&nodes[0], &commitment_tx); + check_closed_broadcast(&nodes[0], 1, true); + let reason = ClosureReason::CommitmentTxConfirmed; + check_closed_event(&nodes[0], 1, reason, false, &[node_b_id], 10_000_000); + check_added_monitors(&nodes[0], 1); + + mine_transaction(&nodes[1], &commitment_tx); + let mut bump_events = nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events(); + assert_eq!(bump_events.len(), 1); + match bump_events.pop().unwrap() { + Event::BumpTransaction(bump_event) => { + nodes[1].bump_tx_handler.handle_event(&bump_event); + }, + ev => panic!("Unexpected event {ev:?}"), + } + + let mut txn = nodes[1].tx_broadcaster.txn_broadcast(); + if nodes[1].connect_style.borrow().updates_best_block_first() { + assert_eq!(txn.len(), 2, "{txn:?}"); + check_spends!(txn[0], funding_tx); + } else { + assert_eq!(txn.len(), 1, "{txn:?}"); + } + let bs_htlc_spend_tx = txn.pop().unwrap(); + check_spends!(bs_htlc_spend_tx, commitment_tx, coinbase_tx); + + // Now connect blocks until the first HTLC expires + assert_eq!(nodes[0].tx_broadcaster.txn_broadcast().len(), 0); + connect_blocks(&nodes[0], TEST_FINAL_CLTV - 2); + let mut txn = nodes[0].tx_broadcaster.txn_broadcast(); + assert_eq!(txn.len(), 1); + let as_first_htlc_spend_tx = txn.pop().unwrap(); + check_spends!(as_first_htlc_spend_tx, commitment_tx); + + // But confirm B's dual-HTLC-claim transaction instead. A should now have nothing to broadcast + // as the third HTLC (if there is one) won't expire for another block. + mine_transaction(&nodes[0], &bs_htlc_spend_tx); + let mut txn = nodes[0].tx_broadcaster.txn_broadcast(); + assert_eq!(txn.len(), 0); + + let sent_events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(sent_events.len(), 4, "{sent_events:?}"); + let mut found_expected_events = [false, false, false, false]; + for event in sent_events { + match event { + Event::PaymentSent { payment_hash, .. }|Event::PaymentPathSuccessful { payment_hash: Some(payment_hash), .. } => { + let path_success = matches!(event, Event::PaymentPathSuccessful { .. }); + if payment_hash == payment_hash_a { + found_expected_events[0 + if path_success { 1 } else { 0 }] = true; + } else if payment_hash == payment_hash_b { + found_expected_events[2 + if path_success { 1 } else { 0 }] = true; + } else { + panic!("Wrong payment hash {event:?}"); + } + }, + _ => panic!("Wrong event {event:?}"), + } + } + assert_eq!(found_expected_events, [true, true, true, true]); + + // However if we connect one more block the third HTLC will time out and A should claim it + connect_blocks(&nodes[0], 1); + let mut txn = nodes[0].tx_broadcaster.txn_broadcast(); + if use_third_htlc { + assert_eq!(txn.len(), 1); + let as_third_htlc_spend_tx = txn.pop().unwrap(); + check_spends!(as_third_htlc_spend_tx, commitment_tx); + // Previously, node A would generate a bogus claim here, trying to claim both HTLCs B and C in + // one transaction, so we check that the single input being spent was not already spent in node + // B's HTLC claim transaction. + assert_eq!(as_third_htlc_spend_tx.input.len(), 1, "{as_third_htlc_spend_tx:?}"); + for spent_input in bs_htlc_spend_tx.input.iter() { + let third_htlc_vout = as_third_htlc_spend_tx.input[0].previous_output.vout; + assert_ne!(third_htlc_vout, spent_input.previous_output.vout); + } + + mine_transaction(&nodes[0], &as_third_htlc_spend_tx); + + assert_eq!(&nodes[0].node.get_and_clear_pending_events(), &[]); + } else { + assert_eq!(txn.len(), 0); + // Connect a block so that both cases end with the same height + connect_blocks(&nodes[0], 1); + } + + // At this point all HTLCs have been resolved and no further transactions should be generated. + // We connect blocks until one block before `bs_htlc_spend_tx` reaches `ANTI_REORG_DELAY` + // confirmations. + connect_blocks(&nodes[0], ANTI_REORG_DELAY - 4); + let mut txn = nodes[0].tx_broadcaster.txn_broadcast(); + assert_eq!(txn.len(), 0); + assert!(nodes[0].node.get_and_clear_pending_events().is_empty()); + + if reorg_out { + // Reorg out bs_htlc_spend_tx, letting node A claim all the HTLCs instead. + disconnect_blocks(&nodes[0], ANTI_REORG_DELAY - 2); + assert_eq!(nodes[0].tx_broadcaster.txn_broadcast().len(), 0); + + // As soon as bs_htlc_spend_tx is disconnected, node A should consider all HTLCs + // claimable-on-timeout. + disconnect_blocks(&nodes[0], 1); + let balances = nodes[0].chain_monitor.chain_monitor.get_claimable_balances(&[]); + assert_eq!(balances.len(), if use_third_htlc { 3 } else { 2 }); + for balance in balances { + if let Balance::MaybeTimeoutClaimableHTLC { .. } = balance { + } else { + panic!("Unexpected balance {balance:?}"); + } + } + + connect_blocks(&nodes[0], 100); + let txn = nodes[0].tx_broadcaster.txn_broadcast(); + let mut claiming_outpoints = new_hash_set(); + for tx in txn.iter() { + for input in tx.input.iter() { + claiming_outpoints.insert(input.previous_output); + } + } + assert_eq!(claiming_outpoints.len(), if use_third_htlc { 3 } else { 2 }); + } else { + // Connect a final block, which puts `bs_htlc_spend_tx` at `ANTI_REORG_DELAY` and we wipe + // the claimable balances for the first two HTLCs. + connect_blocks(&nodes[0], 1); + let balances = nodes[0].chain_monitor.chain_monitor.get_claimable_balances(&[]); + assert_eq!(balances.len(), if use_third_htlc { 1 } else { 0 }); + + // Connect two more blocks to get `as_third_htlc_spend_tx` to `ANTI_REORG_DELAY` confs. + connect_blocks(&nodes[0], 2); + if use_third_htlc { + let failed_events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(failed_events.len(), 2); + let mut found_expected_events = [false, false]; + for event in failed_events { + match event { + Event::PaymentFailed { payment_hash: Some(payment_hash), .. }|Event::PaymentPathFailed { payment_hash, .. } => { + let path_failed = matches!(event, Event::PaymentPathFailed { .. }); + if payment_hash == payment_hash_c { + found_expected_events[if path_failed { 1 } else { 0 }] = true; + } else { + panic!("Wrong payment hash {event:?}"); + } + }, + _ => panic!("Wrong event {event:?}"), + } + } + assert_eq!(found_expected_events, [true, true]); + } + + // Further, there should be no spendable balances. + assert!(nodes[0].chain_monitor.chain_monitor.get_claimable_balances(&[]).is_empty()); + } +} + +#[test] +fn test_split_htlc_expiry_tracking() { + do_test_split_htlc_expiry_tracking(true, true); + do_test_split_htlc_expiry_tracking(false, true); + do_test_split_htlc_expiry_tracking(true, false); + do_test_split_htlc_expiry_tracking(false, false); +}