From 4582b201ea7a2880d452ef06354a953979e75dbd Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 10 Oct 2024 19:42:16 +0000 Subject: [PATCH] Avoid startup `PeerState` entries for peers with unfunded channels If a peer creates a channel with us which never reaches the funding stage (or never gets any commitment updates after creation), we'll avoid inserting the `update_id` into `closed_channel_monitor_update_ids` at runtime to avoid keeping a `PeerState` entry around for no reason. However, on startup we still create a `ChannelMonitorUpdate` with a `ChannelForceClosed` update step to ensure the `ChannelMonitor` is locked and shut down. This is pretty redundant, and results in a bunch of on-startup `ChannelMonitorUpdate`s for any old but non-archived `ChannelMonitor`s. Instead, here, we check if a `ChannelMonitor` already saw a `ChannelForceClosed` update step before we generate the on-startup `ChannelMonitorUpdate`. This also allows us to skip the `closed_channel_monitor_update_ids` insertion as we can be confident we'll never have a `ChannelMonitorUpdate` for this channel at all. --- lightning/src/chain/channelmonitor.rs | 6 ++ lightning/src/ln/channelmanager.rs | 86 +++++++-------------------- lightning/src/ln/monitor_tests.rs | 4 -- lightning/src/ln/payment_tests.rs | 4 -- 4 files changed, 28 insertions(+), 72 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 8653dc827..01698aab6 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1711,6 +1711,12 @@ impl ChannelMonitor { self.inner.lock().unwrap().get_cur_holder_commitment_number() } + /// Gets whether we've been notified that this channel is closed by the `ChannelManager` (i.e. + /// via a [`ChannelMonitorUpdateStep::ChannelForceClosed`]). + pub(crate) fn offchain_closed(&self) -> bool { + self.inner.lock().unwrap().lockdown_from_offchain + } + /// Gets the `node_id` of the counterparty for this channel. /// /// Will be `None` for channels constructed on LDK versions prior to 0.0.110 and always `Some` diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index f06a3af52..7cb484d4f 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -7253,8 +7253,6 @@ where let prev_channel_id = hop_data.channel_id; let prev_user_channel_id = hop_data.user_channel_id; let completed_blocker = RAAMonitorUpdateBlockingAction::from_prev_hop_data(&hop_data); - #[cfg(debug_assertions)] - let claiming_chan_funding_outpoint = hop_data.outpoint; self.claim_funds_from_hop(hop_data, payment_preimage, None, |htlc_claim_value_msat, definitely_duplicate| { let chan_to_release = @@ -7279,61 +7277,6 @@ where // monitor updates still in flight. In that case, we shouldn't // immediately free, but instead let that monitor update complete // in the background. - #[cfg(debug_assertions)] { - let background_events = self.pending_background_events.lock().unwrap(); - // There should be a `BackgroundEvent` pending... - assert!(background_events.iter().any(|ev| { - match ev { - BackgroundEvent::MonitorUpdateRegeneratedOnStartup { - funding_txo, update, .. - } => { - if *funding_txo == claiming_chan_funding_outpoint { - // to apply a monitor update that blocked the claiming channel, - assert!(update.updates.iter().any(|upd| - if let ChannelMonitorUpdateStep::PaymentPreimage { - payment_preimage: update_preimage, .. - } = upd { - payment_preimage == *update_preimage - } else { - false - } - ), "{:?}", update); - true - } else if *funding_txo == next_channel_outpoint { - // or the channel we'd unblock is already closed, - assert!(update.updates.iter().any(|upd| - if let ChannelMonitorUpdateStep::ChannelForceClosed { .. } = upd { - true - } else { - false - } - ), "{:?}", update); - true - } else { false } - }, - // or the channel we'd unblock is already closed (for an - // old channel), - BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup( - (funding_txo, _channel_id, monitor_update) - ) => { - if *funding_txo == next_channel_outpoint { - assert_eq!(monitor_update.updates.len(), 1); - assert!(matches!( - monitor_update.updates[0], - ChannelMonitorUpdateStep::ChannelForceClosed { .. } - )); - true - } else { false } - }, - // or the monitor update has completed and will unblock - // immediately once we get going. - BackgroundEvent::MonitorUpdatesComplete { - channel_id, .. - } => - *channel_id == prev_channel_id, - } - }), "{:?}", *background_events); - } (None, None) } else if definitely_duplicate { if let Some(other_chan) = chan_to_release { @@ -12632,6 +12575,28 @@ where for (funding_txo, monitor) in args.channel_monitors.iter() { if !funding_txo_set.contains(funding_txo) { + if let Some(counterparty_node_id) = monitor.get_counterparty_node_id() { + // If the ChannelMonitor had any updates, we may need to update it further and + // thus track it in `closed_channel_monitor_update_ids`. If the channel never + // had any updates at all, there can't be any HTLCs pending which we need to + // claim. + // Note that a `ChannelMonitor` is created with `update_id` 0 and after we + // provide it with a closure update its `update_id` will be at 1. + if !monitor.offchain_closed() || monitor.get_latest_update_id() > 1 { + per_peer_state.entry(counterparty_node_id) + .or_insert_with(|| Mutex::new(empty_peer_state())) + .lock().unwrap() + .closed_channel_monitor_update_ids.entry(monitor.channel_id()) + .and_modify(|v| *v = cmp::max(monitor.get_latest_update_id(), *v)) + .or_insert(monitor.get_latest_update_id()); + } + } + + if monitor.offchain_closed() { + // We already appled a ChannelForceClosed update. + continue; + } + let logger = WithChannelMonitor::from(&args.logger, monitor, None); let channel_id = monitor.channel_id(); log_info!(logger, "Queueing monitor update to ensure missing channel {} is force closed", @@ -12650,13 +12615,6 @@ where update: monitor_update, }; close_background_events.push(update); - - per_peer_state.entry(counterparty_node_id) - .or_insert_with(|| Mutex::new(empty_peer_state())) - .lock().unwrap() - .closed_channel_monitor_update_ids.entry(monitor.channel_id()) - .and_modify(|v| *v = cmp::max(monitor.get_latest_update_id(), *v)) - .or_insert(monitor.get_latest_update_id()); } else { // This is a fairly old `ChannelMonitor` that hasn't seen an update to its // off-chain state since LDK 0.0.118 (as in LDK 0.0.119 any off-chain diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index 4aad8f569..99ad31d35 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -2302,9 +2302,6 @@ fn do_test_restored_packages_retry(check_old_monitor_retries_after_upgrade: bool // Connecting more blocks should result in the HTLC transactions being rebroadcast. connect_blocks(&nodes[0], crate::chain::package::LOW_FREQUENCY_BUMP_INTERVAL); - if check_old_monitor_retries_after_upgrade { - check_added_monitors(&nodes[0], 1); - } { let txn = nodes[0].tx_broadcaster.txn_broadcast(); assert_eq!(txn.len(), 1); @@ -3014,7 +3011,6 @@ fn do_test_anchors_monitor_fixes_counterparty_payment_script_on_reload(confirm_c // If we saw the commitment before our `counterparty_payment_script` was fixed, we'll never // get the spendable output event for the `to_remote` output, so we'll need to get it // manually via `get_spendable_outputs`. - check_added_monitors(&nodes[1], 1); let outputs = get_monitor!(nodes[1], chan_id).get_spendable_outputs(&commitment_tx, commitment_tx_conf_height); assert_eq!(outputs.len(), 1); let spend_tx = nodes[1].keys_manager.backing.spend_spendable_outputs( diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 7839b49be..46a6e3026 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -993,7 +993,6 @@ fn do_test_completed_payment_not_retryable_on_reload(use_dust: bool) { nodes[1].node.peer_disconnected(nodes[0].node.get_our_node_id()); nodes[0].node.test_process_background_events(); - check_added_monitors(&nodes[0], 1); let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]); reconnect_args.send_channel_ready = (true, true); @@ -1023,7 +1022,6 @@ fn do_test_completed_payment_not_retryable_on_reload(use_dust: bool) { nodes[1].node.peer_disconnected(nodes[0].node.get_our_node_id()); nodes[0].node.test_process_background_events(); - check_added_monitors(&nodes[0], 1); reconnect_nodes(ReconnectArgs::new(&nodes[0], &nodes[1])); @@ -1162,7 +1160,6 @@ fn do_test_dup_htlc_onchain_doesnt_fail_on_reload(persist_manager_post_event: bo let height = nodes[0].blocks.lock().unwrap().len() as u32 - 1; nodes[0].chain_monitor.chain_monitor.block_connected(&claim_block, height); assert!(nodes[0].node.get_and_clear_pending_events().is_empty()); - check_added_monitors(&nodes[0], 1); } #[test] @@ -3522,7 +3519,6 @@ fn do_no_missing_sent_on_reload(persist_manager_with_payment: bool, at_midpoint: reload_node!(nodes[0], test_default_channel_config(), &nodes[0].node.encode(), &[&chan_0_monitor_serialized], persister_c, chain_monitor_c, nodes_0_deserialized_c); let events = nodes[0].node.get_and_clear_pending_events(); assert!(events.is_empty()); - check_added_monitors(&nodes[0], 1); } #[test] -- 2.39.5