From 7bd815fc6e0b50e321004469e3ad69cd6b019611 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 22 Jul 2025 21:15:56 +0000 Subject: [PATCH 01/17] Remove direct calls to `handle_monitor_update_completion!` `handle_monitor_update_completion!` (and `handle_monitor_update_completion_actions`) are a pretty annoying API as they have several preconditions. Luckily, we already have `channel_monitor_update` which does the right work, handling both the open- and closed- channel cases and correctly checking the preconditions to `handle_monitor_update_completion!`, so here we convert calls to `handle_monitor_update_completion!` (aside from `handle_new_monitor_update!`) to just calling `channelMonitor_update`. Backport of 8b9b0785869f90f92c243220d1c25addcf6a7ec5 Nontrivial conflicts resolved in: * lightning/src/ln/channelmanager.rs due to the tracking of in-flight monitor updates having moved from `OutPoint`s to `ChannelId`s in 0.2. Also note that 8f4a4d239d0440f2bfb1e425724ecaf4d94da7d9 updated some code which was introduced in this commit and had already previously been backported to 0.1. --- lightning/src/ln/channelmanager.rs | 84 +++++++++++++++++------------- 1 file changed, 48 insertions(+), 36 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index eaca6e03beb..53ea6ab2a24 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1075,6 +1075,7 @@ enum BackgroundEvent { /// on a channel. MonitorUpdatesComplete { counterparty_node_id: PublicKey, + funding_txo: OutPoint, channel_id: ChannelId, }, } @@ -3235,8 +3236,21 @@ macro_rules! emit_channel_ready_event { } } +/// Handles the completion steps for when a [`ChannelMonitorUpdate`] is applied to a live channel. +/// +/// You should not add new direct calls to this, generally, rather rely on +/// `handle_new_monitor_update` or [`ChannelManager::channel_monitor_updated`] to call it for you. +/// +/// Requires that the in-flight monitor update set for this channel is empty! macro_rules! handle_monitor_update_completion { ($self: ident, $peer_state_lock: expr, $peer_state: expr, $per_peer_state_lock: expr, $chan: expr) => { { + #[cfg(debug_assertions)] + if let Some(funding_txo) = $chan.context.get_funding_txo() { + let in_flight_updates = + $peer_state.in_flight_monitor_updates.get(&funding_txo); + assert!(in_flight_updates.map(|updates| updates.is_empty()).unwrap_or(true)); + } + debug_assert!($chan.is_awaiting_monitor_update()); let logger = WithChannelContext::from(&$self.logger, &$chan.context, None); let update_actions = $peer_state.monitor_update_blocked_actions @@ -4101,19 +4115,7 @@ where // TODO: If we do the `in_flight_monitor_updates.is_empty()` check in // `locked_close_channel` we can skip the locks here. if let Some(funding_txo) = shutdown_res.channel_funding_txo { - let per_peer_state = self.per_peer_state.read().unwrap(); - if let Some(peer_state_mtx) = per_peer_state.get(&shutdown_res.counterparty_node_id) { - let mut peer_state = peer_state_mtx.lock().unwrap(); - if peer_state.in_flight_monitor_updates.get(&funding_txo).map(|l| l.is_empty()).unwrap_or(true) { - let update_actions = peer_state.monitor_update_blocked_actions - .remove(&shutdown_res.channel_id).unwrap_or(Vec::new()); - - mem::drop(peer_state); - mem::drop(per_peer_state); - - self.handle_monitor_update_completion_actions(update_actions); - } - } + self.channel_monitor_updated(&funding_txo, &shutdown_res.channel_id, None, Some(&shutdown_res.counterparty_node_id)); } } let mut shutdown_results = Vec::new(); @@ -6411,21 +6413,8 @@ where BackgroundEvent::MonitorUpdateRegeneratedOnStartup { counterparty_node_id, funding_txo, channel_id, update } => { self.apply_post_close_monitor_update(counterparty_node_id, channel_id, funding_txo, update); }, - BackgroundEvent::MonitorUpdatesComplete { counterparty_node_id, channel_id } => { - let per_peer_state = self.per_peer_state.read().unwrap(); - if let Some(peer_state_mutex) = per_peer_state.get(&counterparty_node_id) { - let mut peer_state_lock = peer_state_mutex.lock().unwrap(); - let peer_state = &mut *peer_state_lock; - if let Some(ChannelPhase::Funded(chan)) = peer_state.channel_by_id.get_mut(&channel_id) { - handle_monitor_update_completion!(self, peer_state_lock, peer_state, per_peer_state, chan); - } else { - let update_actions = peer_state.monitor_update_blocked_actions - .remove(&channel_id).unwrap_or(Vec::new()); - mem::drop(peer_state_lock); - mem::drop(per_peer_state); - self.handle_monitor_update_completion_actions(update_actions); - } - } + BackgroundEvent::MonitorUpdatesComplete { counterparty_node_id, funding_txo, channel_id } => { + self.channel_monitor_updated(&funding_txo, &channel_id, None, Some(&counterparty_node_id)); }, } } @@ -7785,7 +7774,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ (htlc_forwards, decode_update_add_htlcs) } - fn channel_monitor_updated(&self, funding_txo: &OutPoint, channel_id: &ChannelId, highest_applied_update_id: u64, counterparty_node_id: Option<&PublicKey>) { + fn channel_monitor_updated(&self, funding_txo: &OutPoint, channel_id: &ChannelId, highest_applied_update_id: Option, counterparty_node_id: Option<&PublicKey>) { debug_assert!(self.total_consistency_lock.try_write().is_err()); // Caller holds read lock let counterparty_node_id = match counterparty_node_id { @@ -7807,16 +7796,33 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ peer_state_lock = peer_state_mutex_opt.unwrap().lock().unwrap(); let peer_state = &mut *peer_state_lock; + let logger = WithContext::from(&self.logger, Some(counterparty_node_id), Some(*channel_id), None); let remaining_in_flight = if let Some(pending) = peer_state.in_flight_monitor_updates.get_mut(funding_txo) { - pending.retain(|upd| upd.update_id > highest_applied_update_id); + if let Some(highest_applied_update_id) = highest_applied_update_id { + pending.retain(|upd| upd.update_id > highest_applied_update_id); + log_trace!( + logger, + "ChannelMonitor updated to {highest_applied_update_id}. {} pending in-flight updates.", + pending.len() + ); + } else if let Some(update) = pending.get(0) { + log_trace!( + logger, + "ChannelMonitor updated to {}. {} pending in-flight updates.", + update.update_id - 1, + pending.len() + ); + } else { + log_trace!( + logger, + "ChannelMonitor updated. {} pending in-flight updates.", + pending.len() + ); + } pending.len() } else { 0 }; - let logger = WithContext::from(&self.logger, Some(counterparty_node_id), Some(*channel_id), None); - log_trace!(logger, "ChannelMonitor updated to {}. {} pending in-flight updates.", - highest_applied_update_id, remaining_in_flight); - if remaining_in_flight != 0 { return; } @@ -9651,7 +9657,12 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ } }, MonitorEvent::Completed { funding_txo, channel_id, monitor_update_id } => { - self.channel_monitor_updated(&funding_txo, &channel_id, monitor_update_id, counterparty_node_id.as_ref()); + self.channel_monitor_updated( + &funding_txo, + &channel_id, + Some(monitor_update_id), + counterparty_node_id.as_ref(), + ); }, } } @@ -13929,6 +13940,7 @@ where pending_background_events.push( BackgroundEvent::MonitorUpdatesComplete { counterparty_node_id: $counterparty_node_id, + funding_txo: $funding_txo, channel_id: $monitor.channel_id(), }); } else { @@ -15321,7 +15333,7 @@ mod tests { let chan = create_announced_chan_between_nodes(&nodes, 0, 1); - nodes[0].node.force_close_channel_with_peer(&chan.2, &nodes[1].node.get_our_node_id(), None, true).unwrap(); + nodes[0].node.force_close_broadcasting_latest_txn(&chan.2, &nodes[1].node.get_our_node_id(), "".to_string()).unwrap(); check_added_monitors!(nodes[0], 1); check_closed_event!(nodes[0], 1, ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) }, [nodes[1].node.get_our_node_id()], 100000); From d2b100745f389d2128447238e6f8797cf8056921 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 3 Feb 2026 16:09:06 -0800 Subject: [PATCH 02/17] Hold in-flight monitor updates until background event processing We previously assumed background events would eventually be processed prior to another `ChannelManager` write, so we would immediately remove all in-flight monitor updates that completed since the last `ChannelManager` serialization. This isn't always the case, so we now keep them all around until we're ready to handle them, i.e., when `process_background_events` is called. This was discovered while fuzzing `chanmon_consistency_target` on the main branch with some changes that allow it to connect blocks. It was triggered by reloading the `ChannelManager` after a monitor update completion for an outgoing HTLC, calling `ChannelManager::best_block_updated`, and reloading the `ChannelManager` once again. A test is included that provides a minimal reproduction of this case. Backport of 7e84268505d0c72d16f4499b53bc51a32c85fe06 Conflicts resolved in: * lightning/src/ln/channelmanager.rs * lightning/src/ln/reload_tests.rs --- lightning/src/ln/channelmanager.rs | 79 ++++++++++++++++++++-------- lightning/src/ln/reload_tests.rs | 82 ++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+), 22 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 53ea6ab2a24..1dfb7450bb0 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1077,6 +1077,7 @@ enum BackgroundEvent { counterparty_node_id: PublicKey, funding_txo: OutPoint, channel_id: ChannelId, + highest_update_id_completed: u64, }, } @@ -6413,8 +6414,23 @@ where BackgroundEvent::MonitorUpdateRegeneratedOnStartup { counterparty_node_id, funding_txo, channel_id, update } => { self.apply_post_close_monitor_update(counterparty_node_id, channel_id, funding_txo, update); }, - BackgroundEvent::MonitorUpdatesComplete { counterparty_node_id, funding_txo, channel_id } => { - self.channel_monitor_updated(&funding_txo, &channel_id, None, Some(&counterparty_node_id)); + BackgroundEvent::MonitorUpdatesComplete { + counterparty_node_id, + funding_txo, + channel_id, + highest_update_id_completed, + } => { + // Now that we can finally handle the background event, remove all in-flight + // monitor updates for this channel that we've known to complete, as they have + // already been persisted to the monitor and can be applied to our internal + // state such that the channel resumes operation if no new updates have been + // made since. + self.channel_monitor_updated( + &funding_txo, + &channel_id, + Some(highest_update_id_completed), + Some(&counterparty_node_id), + ); }, } } @@ -13911,39 +13927,58 @@ where ($counterparty_node_id: expr, $chan_in_flight_upds: expr, $funding_txo: expr, $monitor: expr, $peer_state: expr, $logger: expr, $channel_info_log: expr ) => { { + // When all in-flight updates have completed after we were last serialized, we + // need to remove them. However, we can't guarantee that the next serialization + // will have happened after processing the + // `BackgroundEvent::MonitorUpdatesComplete`, so removing them now could lead to the + // channel never being resumed as the event would not be regenerated after another + // reload. At the same time, we don't want to resume the channel now because there + // may be post-update actions to handle. Therefore, we're forced to keep tracking + // the completed in-flight updates (but only when they have all completed) until we + // are processing the `BackgroundEvent::MonitorUpdatesComplete`. let mut max_in_flight_update_id = 0; - let starting_len = $chan_in_flight_upds.len(); - $chan_in_flight_upds.retain(|upd| upd.update_id > $monitor.get_latest_update_id()); - if $chan_in_flight_upds.len() < starting_len { + let num_updates_completed = $chan_in_flight_upds + .iter() + .filter(|update| { + max_in_flight_update_id = cmp::max(max_in_flight_update_id, update.update_id); + update.update_id <= $monitor.get_latest_update_id() + }) + .count(); + if num_updates_completed > 0 { log_debug!( $logger, "{} ChannelMonitorUpdates completed after ChannelManager was last serialized", - starting_len - $chan_in_flight_upds.len() + num_updates_completed, ); } - for update in $chan_in_flight_upds.iter() { - log_debug!($logger, "Replaying ChannelMonitorUpdate {} for {}channel {}", - update.update_id, $channel_info_log, &$monitor.channel_id()); - max_in_flight_update_id = cmp::max(max_in_flight_update_id, update.update_id); - pending_background_events.push( - BackgroundEvent::MonitorUpdateRegeneratedOnStartup { - counterparty_node_id: $counterparty_node_id, - funding_txo: $funding_txo, - channel_id: $monitor.channel_id(), - update: update.clone(), - }); - } - if $chan_in_flight_upds.is_empty() { - // We had some updates to apply, but it turns out they had completed before we - // were serialized, we just weren't notified of that. Thus, we may have to run - // the completion actions for any monitor updates, but otherwise are done. + let all_updates_completed = num_updates_completed == $chan_in_flight_upds.len(); + + if all_updates_completed { + log_debug!($logger, "All monitor updates completed since the ChannelManager was last serialized"); pending_background_events.push( BackgroundEvent::MonitorUpdatesComplete { counterparty_node_id: $counterparty_node_id, funding_txo: $funding_txo, channel_id: $monitor.channel_id(), + highest_update_id_completed: max_in_flight_update_id, }); } else { + $chan_in_flight_upds.retain(|update| { + let replay = update.update_id > $monitor.get_latest_update_id(); + if replay { + log_debug!($logger, "Replaying ChannelMonitorUpdate {} for {}channel {}", + update.update_id, $channel_info_log, &$monitor.channel_id()); + pending_background_events.push( + BackgroundEvent::MonitorUpdateRegeneratedOnStartup { + counterparty_node_id: $counterparty_node_id, + funding_txo: $funding_txo, + channel_id: $monitor.channel_id(), + update: update.clone(), + } + ); + } + replay + }); $peer_state.closed_channel_monitor_update_ids.entry($monitor.channel_id()) .and_modify(|v| *v = cmp::max(max_in_flight_update_id, *v)) .or_insert(max_in_flight_update_id); diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs index 16904d85758..0cdc1acab66 100644 --- a/lightning/src/ln/reload_tests.rs +++ b/lightning/src/ln/reload_tests.rs @@ -1291,3 +1291,85 @@ fn test_reload_partial_funding_batch() { // Ensure the channels don't exist anymore. assert!(nodes[0].node.list_channels().is_empty()); } + +#[test] +fn test_hold_completed_inflight_monitor_updates_upon_manager_reload() { + // Test that if a `ChannelMonitorUpdate` completes after the `ChannelManager` is serialized, + // but before it is deserialized, we hold any completed in-flight updates until background event + // processing. Previously, we would remove completed monitor updates from + // `in_flight_monitor_updates` during deserialization, relying on + // [`ChannelManager::process_background_events`] to eventually be called before the + // `ChannelManager` is serialized again such that the channel is resumed and further updates can + // be made. + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let (persister_a, persister_b); + let (chain_monitor_a, chain_monitor_b); + + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let nodes_0_deserialized_a; + let nodes_0_deserialized_b; + + let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); + let chan_id = create_announced_chan_between_nodes(&nodes, 0, 1).2; + + send_payment(&nodes[0], &[&nodes[1]], 1_000_000); + + chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); + + // Send a payment that will be pending due to an async monitor update. + let (route, payment_hash, _, payment_secret) = + get_route_and_payment_hash!(nodes[0], nodes[1], 1_000_000); + let payment_id = PaymentId(payment_hash.0); + let onion = RecipientOnionFields::secret_only(payment_secret); + nodes[0].node.send_payment_with_route(route, payment_hash, onion, payment_id).unwrap(); + check_added_monitors(&nodes[0], 1); + + assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); + assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); + + // Serialize the ChannelManager while the monitor update is still in-flight. + let node_0_serialized = nodes[0].node.encode(); + + // Now complete the monitor update by calling force_channel_monitor_updated. + // This updates the monitor's state, but the ChannelManager still thinks it's pending. + let (outpoint, latest_update, _) = nodes[0].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone(); + nodes[0].chain_monitor.chain_monitor.force_channel_monitor_updated(outpoint, latest_update); + let monitor_serialized_updated = get_monitor!(nodes[0], chan_id).encode(); + + // Reload the node with the updated monitor. Upon deserialization, the ChannelManager will + // detect that the monitor update completed (monitor's update_id >= the in-flight update_id) + // and queue a `BackgroundEvent::MonitorUpdatesComplete`. + nodes[0].node.peer_disconnected(nodes[1].node.get_our_node_id()); + nodes[1].node.peer_disconnected(nodes[0].node.get_our_node_id()); + reload_node!( + nodes[0], + test_default_channel_config(), + &node_0_serialized, + &[&monitor_serialized_updated[..]], + persister_a, + chain_monitor_a, + nodes_0_deserialized_a + ); + + // If we serialize again, even though we haven't processed any background events yet, we should + // still see the `BackgroundEvent::MonitorUpdatesComplete` be regenerated on startup. + let node_0_serialized = nodes[0].node.encode(); + reload_node!( + nodes[0], + test_default_channel_config(), + &node_0_serialized, + &[&monitor_serialized_updated[..]], + persister_b, + chain_monitor_b, + nodes_0_deserialized_b + ); + + // Reconnect the nodes. We should finally see the `update_add_htlc` go out, as the reconnection + // should first process `BackgroundEvent::MonitorUpdatesComplete, allowing the channel to be + // resumed. + let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]); + reconnect_args.pending_htlc_adds = (0, 1); + reconnect_nodes(reconnect_args); + expect_pending_htlcs_forwardable_ignore!(nodes[1]); +} From 125dd11045ccd80ce53bc0beeb2949e9854177e9 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 5 Feb 2026 08:49:22 -0800 Subject: [PATCH 03/17] Rustfmt ChannelManager::process_background_events Backport of f128b8504d1724008eab10d37ad9f619657d1a24 Conflicts resolved in: * lightning/src/ln/channelmanager.rs --- lightning/src/ln/channelmanager.rs | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 1dfb7450bb0..c32ee3a652c 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -6394,7 +6394,10 @@ where /// /// Expects the caller to have a total_consistency_lock read lock. fn process_background_events(&self) -> NotifyOption { - debug_assert_ne!(self.total_consistency_lock.held_by_thread(), LockHeldState::NotHeldByThread); + debug_assert_ne!( + self.total_consistency_lock.held_by_thread(), + LockHeldState::NotHeldByThread + ); self.background_events_processed_since_startup.store(true, Ordering::Release); @@ -6411,8 +6414,18 @@ where // monitor updating completing. let _ = self.chain_monitor.update_channel(funding_txo, &update); }, - BackgroundEvent::MonitorUpdateRegeneratedOnStartup { counterparty_node_id, funding_txo, channel_id, update } => { - self.apply_post_close_monitor_update(counterparty_node_id, channel_id, funding_txo, update); + BackgroundEvent::MonitorUpdateRegeneratedOnStartup { + counterparty_node_id, + funding_txo, + channel_id, + update, + } => { + self.apply_post_close_monitor_update( + counterparty_node_id, + channel_id, + funding_txo, + update, + ); }, BackgroundEvent::MonitorUpdatesComplete { counterparty_node_id, From 4f3174ed0ceb60ea4b879c11eebd16056807e998 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 29 Mar 2026 16:23:35 +0000 Subject: [PATCH 04/17] Attempt to unblock blocked monitor updates on startup When we make an MPP claim we push RAA blockers for each chanel to ensure we don't allow any single channel to make too much progress until all channels have the preimage durably on disk. We don't have to store those RAA blockers on disk in the ChannelManager as there's no point - if the ChannelManager gets to disk with the RAA blockers it also brought with it the pending ChannelMonitorUpdates that contain the preimages and will now be replayed, ensuring the preimage makes it to all ChannelMonitors. However, just because those RAA blockers dissapear on reload doesn't mean the implications of them does too - if a later ChannelMonitorUpdate was blocked in the channel we don't have logic to unblock it on startup. Here we add such logic, simply attempting to unblock all blocked `ChannelMonitorUpdate`s that existed on startup. Code written by Claude. Fixes #4518 Backport of b0c312dbd25816af70dc16685eec5584bd6a5822 Conflicts resolved in: * lightning/src/ln/channelmanager.rs --- lightning/src/ln/channelmanager.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index c32ee3a652c..3b10363ce92 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1079,6 +1079,11 @@ enum BackgroundEvent { channel_id: ChannelId, highest_update_id_completed: u64, }, + /// A channel had blocked monitor updates waiting on startup. If the updates were blocked on + /// an MPP claim blocker not written to disk, we may be able to unblock them now. + /// + /// This event is never written to disk. + AttemptUnblockMonitorUpdates { counterparty_node_id: PublicKey, channel_id: ChannelId }, } /// A pointer to a channel that is unblocked when an event is surfaced @@ -6445,6 +6450,12 @@ where Some(&counterparty_node_id), ); }, + BackgroundEvent::AttemptUnblockMonitorUpdates { + counterparty_node_id, + channel_id, + } => { + self.handle_monitor_update_release(counterparty_node_id, channel_id, None); + }, } } NotifyOption::DoPersist @@ -14014,6 +14025,7 @@ where // Channels that were persisted have to be funded, otherwise they should have been // discarded. let funding_txo = chan.context.get_funding_txo().ok_or(DecodeError::InvalidValue)?; + let chan_id = chan.context.channel_id(); let monitor = args.channel_monitors.get(&funding_txo) .expect("We already checked for monitor presence when loading channels"); let mut max_in_flight_update_id = monitor.get_latest_update_id(); @@ -14036,6 +14048,14 @@ where log_error!(logger, " Please ensure the chain::Watch API requirements are met and file a bug report at https://github.com/lightningdevkit/rust-lightning"); return Err(DecodeError::DangerousValue); } + if chan.blocked_monitor_updates_pending() > 0 { + pending_background_events.push( + BackgroundEvent::AttemptUnblockMonitorUpdates { + counterparty_node_id: *counterparty_id, + channel_id: chan_id, + }, + ); + } } else { // We shouldn't have persisted (or read) any unfunded channel types so none should have been // created in this `channel_by_id` map. From 72b35ffa0dc210534013856f5ee7dd652528db44 Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Wed, 6 May 2026 19:54:19 +0000 Subject: [PATCH 05/17] Add reload test for stuck MPP fulfill Add a characterization test for a claimed MPP payment whose preimage monitor updates are only partially persisted before restart. The test drives both channels through a held fee-update commitment dance, claims with async monitor persistence, reloads one fresh and one stale monitor, and verifies that we don't leave a sender-side HTLC stuck after reconnect. Backport of 01d55dc1651ceffa560cd79c8993dbc7755383e8 Conflicts due to different channel sorting and holding cell free timing resolved in: * lightning/src/ln/reload_tests.rs --- lightning/src/ln/reload_tests.rs | 353 +++++++++++++++++++++++++++++++ 1 file changed, 353 insertions(+) diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs index 0cdc1acab66..d65be70e99f 100644 --- a/lightning/src/ln/reload_tests.rs +++ b/lightning/src/ln/reload_tests.rs @@ -984,6 +984,359 @@ fn test_partial_claim_before_restart() { do_test_partial_claim_before_restart(true, true); } +#[test] +fn test_mpp_claim_htlc_fulfills_unblocked_on_reload() { + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let persister; + let new_chain_monitor; + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let nodes_1_deserialized; + let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + // Open two independent channels between the same nodes. The payment below is large enough to + // force the router to split it across both channels, which is what makes the MPP claim depend + // on both ChannelMonitors durably learning the preimage. + let chan_b = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, 0); + let chan_a = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, 0); + let chan_id_a = chan_a.2; + let chan_id_b = chan_b.2; + let scid_a = chan_a.0.contents.short_channel_id; + let scid_b = chan_b.0.contents.short_channel_id; + + // Send an MPP payment to nodes[1]. `send_along_route_with_secret` leaves the payment + // claimable but unclaimed, so nodes[1] still has both inbound HTLCs live when we start + // manipulating monitor persistence below. + let amt_msat = 20_000_000; + let (route, payment_hash, payment_preimage, payment_secret) = + get_route_and_payment_hash!(nodes[0], nodes[1], amt_msat); + assert_eq!(route.paths.len(), 2); + send_along_route_with_secret( + &nodes[0], route, &[&[&nodes[1]], &[&nodes[1]]], amt_msat, payment_hash, + payment_secret, + ); + + // Move both channels into `AWAITING_REMOTE_REVOKE` by having nodes[0] send fee updates and + // withholding nodes[1]'s responding `commitment_signed`s. When nodes[1] later claims the + // payment, the fulfill updates cannot be sent immediately and instead sit in each channel's + // holding cell. + { + let mut fee_est = chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap(); + *fee_est *= 2; + } + nodes[0].node.timer_tick_occurred(); + check_added_monitors(&nodes[0], 2); + + let node_0_id = nodes[0].node.get_our_node_id(); + let node_1_id = nodes[1].node.get_our_node_id(); + + let fee_msgs = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(fee_msgs.len(), 2); + for ev in &fee_msgs { + match ev { + MessageSendEvent::UpdateHTLCs { updates, .. } => { + nodes[1].node.handle_update_fee(node_0_id, updates.update_fee.as_ref().unwrap()); + nodes[1].node.handle_commitment_signed(node_0_id, &updates.commitment_signed); + check_added_monitors(&nodes[1], 1); + }, + _ => panic!("Unexpected message: {:?}", ev), + } + } + + // nodes[1] responds to each fee update with a `revoke_and_ack` and a new + // `commitment_signed`. Deliver only the `revoke_and_ack`s for now. The held + // `commitment_signed`s are delivered after nodes[1] claims the payment, creating the blocked + // post-claim monitor updates whose release is exercised after reload. + let node_1_msgs = nodes[1].node.get_and_clear_pending_msg_events(); + let mut commitment_signed_msgs = Vec::new(); + for ev in &node_1_msgs { + match ev { + MessageSendEvent::SendRevokeAndACK { msg, .. } => { + nodes[0].node.handle_revoke_and_ack(node_1_id, msg); + check_added_monitors(&nodes[0], 1); + }, + MessageSendEvent::UpdateHTLCs { updates, .. } => { + commitment_signed_msgs.push(updates.commitment_signed.clone()); + }, + _ => panic!("Unexpected message: {:?}", ev), + } + } + + let node_0_msgs = nodes[0].node.get_and_clear_pending_msg_events(); + for ev in &node_0_msgs { + match ev { + MessageSendEvent::SendRevokeAndACK { msg, .. } => { + nodes[1].node.handle_revoke_and_ack(node_0_id, msg); + check_added_monitors(&nodes[1], 1); + }, + _ => panic!("Unexpected message: {:?}", ev), + } + } + + // Snapshot channel B before the claim. The in-memory ChainMonitor applies updates even when + // the persister returns `InProgress`, so taking this snapshot after the claim would not model a + // crash between two separate monitor writes. + let mon_b_serialized = get_monitor!(nodes[1], chan_id_b).encode(); + + // Make both preimage monitor writes asynchronous. `claim_funds` attaches an in-memory MPP RAA + // blocker so neither channel can release later monitor updates until all channels have the + // preimage durably persisted. + chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); + chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); + nodes[1].node.claim_funds(payment_preimage); + check_added_monitors(&nodes[1], 2); + + // Complete only channel A's preimage update. Channel B will be reloaded from the stale snapshot + // above, simulating a crash where one monitor write completed and the other did not. + let (outpoint_a, update_id_a, _) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id_a).unwrap().clone(); + nodes[1].chain_monitor.chain_monitor.force_channel_monitor_updated(outpoint_a, update_id_a); + + // Now finish the fee-update commitment dance we held back. nodes[1] receives nodes[0]'s + // `revoke_and_ack`s while the MPP RAA blocker is still in place, so the resulting monitor + // updates are blocked behind state that is not serialized in the ChannelManager. + for commitment_signed in &commitment_signed_msgs { + nodes[0].node.handle_commitment_signed(node_1_id, commitment_signed); + check_added_monitors(&nodes[0], 1); + } + let node_0_msgs = nodes[0].node.get_and_clear_pending_msg_events(); + for ev in &node_0_msgs { + match ev { + MessageSendEvent::SendRevokeAndACK { msg, .. } => { + nodes[1].node.handle_revoke_and_ack(node_0_id, msg); + check_added_monitors(&nodes[1], 0); + }, + _ => panic!("Unexpected message: {:?}", ev), + } + } + + // Persist the ChannelManager after the blocked post-claim monitor updates have been recorded. + // Reload with channel A's up-to-date monitor and channel B's stale monitor. The preimage update + // for B is replayed during reload, putting both channels' preimages on disk. The remaining state + // under test is the blocked post-claim `revoke_and_ack` monitor updates after the in-memory MPP + // RAA blocker that created them is gone. + let node_1_serialized = nodes[1].node.encode(); + let mon_a_serialized = get_monitor!(nodes[1], chan_id_a).encode(); + + nodes[0].node.peer_disconnected(node_1_id); + reload_node!( + nodes[1], + node_1_serialized, + &[&mon_a_serialized, &mon_b_serialized], + persister, + new_chain_monitor, + nodes_1_deserialized + ); + + // Reconnect both peers by manually exchanging `channel_reestablish`s. This avoids relying on a + // more general reconnect helper while the channels intentionally have asymmetric monitor state. + let node_1_id = nodes[1].node.get_our_node_id(); + nodes[0].node.peer_connected(node_1_id, &msgs::Init { + features: nodes[1].node.init_features(), networks: None, remote_network_address: None, + }, true).unwrap(); + nodes[1].node.peer_connected(node_0_id, &msgs::Init { + features: nodes[0].node.init_features(), networks: None, remote_network_address: None, + }, false).unwrap(); + + let reestablish_0 = nodes[0].node.get_and_clear_pending_msg_events(); + let reestablish_1 = nodes[1].node.get_and_clear_pending_msg_events(); + let mut reestablish_0_chan_ids = Vec::new(); + let mut reestablish_1_chan_ids = Vec::new(); + for ev in &reestablish_1 { + match ev { + MessageSendEvent::SendChannelReestablish { node_id, msg } => { + assert_eq!(*node_id, node_0_id); + reestablish_1_chan_ids.push(msg.channel_id); + nodes[0].node.handle_channel_reestablish(node_1_id, msg); + }, + _ => panic!("Unexpected message: {:?}", ev), + } + } + for ev in &reestablish_0 { + match ev { + MessageSendEvent::SendChannelReestablish { node_id, msg } => { + assert_eq!(*node_id, node_1_id); + reestablish_0_chan_ids.push(msg.channel_id); + nodes[1].node.handle_channel_reestablish(node_0_id, msg); + }, + _ => panic!("Unexpected message: {:?}", ev), + } + } + assert_eq!(reestablish_0_chan_ids.len(), 2); + assert!(reestablish_0_chan_ids.contains(&chan_id_a)); + assert!(reestablish_0_chan_ids.contains(&chan_id_b)); + assert_eq!(reestablish_1_chan_ids.len(), 2); + assert!(reestablish_1_chan_ids.contains(&chan_id_a)); + assert!(reestablish_1_chan_ids.contains(&chan_id_b)); + // Only nodes[1] was reloaded with stale monitor state. nodes[0] responds to the + // `channel_reestablish`s without touching its monitors. nodes[1] applies the replayed channel B + // preimage update, releases channel A's held RAA update, and frees channel A's held fulfill + // during startup processing. + // Note that unlike the test in 0.3, we only generate the last monitor update for node B after + // get_and_clear_pending_msg_events as we only free the holding cell then. + check_added_monitors(&nodes[0], 0); + check_added_monitors(&nodes[1], 2); + + // The first message batch after reconnect contains channel updates from both nodes. nodes[1] + // also sends the channel A fulfill that startup processing released from the holding cell. + let restart_msgs_0 = nodes[0].node.get_and_clear_pending_msg_events(); + let restart_msgs_1 = nodes[1].node.get_and_clear_pending_msg_events(); + check_added_monitors(&nodes[1], 1); + let mut restart_scids_0 = Vec::new(); + let mut restart_scids_1 = Vec::new(); + let mut startup_fulfill_chan_ids = Vec::new(); + for ev in &restart_msgs_0 { + match ev { + MessageSendEvent::SendChannelUpdate { node_id, msg } => { + assert_eq!(*node_id, node_1_id); + restart_scids_0.push(msg.contents.short_channel_id); + }, + _ => panic!("Unexpected restart message from node 0: {:?}", ev), + } + } + for ev in &restart_msgs_1 { + match ev { + MessageSendEvent::SendChannelUpdate { node_id, msg } => { + assert_eq!(*node_id, node_0_id); + restart_scids_1.push(msg.contents.short_channel_id); + }, + MessageSendEvent::UpdateHTLCs { node_id, updates } => { + assert_eq!(*node_id, node_0_id); + startup_fulfill_chan_ids.push(updates.commitment_signed.channel_id); + assert_eq!(updates.update_fulfill_htlcs.len(), 1); + assert!(updates.update_add_htlcs.is_empty()); + assert!(updates.update_fail_htlcs.is_empty()); + assert!(updates.update_fail_malformed_htlcs.is_empty()); + assert!(updates.update_fee.is_none()); + for fulfill in &updates.update_fulfill_htlcs { + nodes[0].node.handle_update_fulfill_htlc(node_1_id, fulfill); + } + // Complete the standard commitment handshake for the released fulfill. The helper + // checks nodes[0]'s incoming commitment monitor update, nodes[1]'s response monitor + // updates, and nodes[0]'s held final monitor update. + do_commitment_signed_dance( + &nodes[0], &nodes[1], &updates.commitment_signed, false, false, + ); + }, + _ => panic!("Unexpected restart message from node 1: {:?}", ev), + } + } + assert_eq!(restart_scids_0.len(), 2); + assert!(restart_scids_0.contains(&scid_a)); + assert!(restart_scids_0.contains(&scid_b)); + assert_eq!(restart_scids_1.len(), 2); + assert!(restart_scids_1.contains(&scid_a)); + assert!(restart_scids_1.contains(&scid_b)); + assert_eq!(startup_fulfill_chan_ids, vec![chan_id_a]); + assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); + assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); + check_added_monitors(&nodes[0], 0); + check_added_monitors(&nodes[1], 0); + + // Receiving the startup-released fulfill gives nodes[0] the payment preimage. That is enough to + // emit `PaymentSent`, even though channel B's path-level success still needs its own fulfill. + let startup_payment_events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(startup_payment_events.len(), 2); + let mut saw_startup_payment_sent = false; + let mut startup_success_scids = Vec::new(); + for ev in &startup_payment_events { + match ev { + Event::PaymentSent { + payment_preimage: sent_preimage, + payment_hash: sent_hash, + fee_paid_msat, + .. + } => { + assert_eq!(*sent_preimage, payment_preimage); + assert_eq!(*sent_hash, payment_hash); + assert_eq!(*fee_paid_msat, Some(0)); + saw_startup_payment_sent = true; + }, + Event::PaymentPathSuccessful { payment_hash: Some(path_hash), path, .. } => { + assert_eq!(*path_hash, payment_hash); + assert_eq!(path.hops.len(), 1); + startup_success_scids.push(path.hops[0].short_channel_id); + }, + _ => panic!("Unexpected startup payment event: {:?}", ev), + } + } + assert!(saw_startup_payment_sent); + assert_eq!(startup_success_scids, vec![scid_a]); + + // Handling the claim event runs the event-completion action that releases the remaining + // RAA-blocked monitor update. The startup unblock path already released channel A, so channel B + // is the only fulfill that should be emitted here. + let claim_events = nodes[1].node.get_and_clear_pending_events(); + assert_eq!(claim_events.len(), 1); + match &claim_events[0] { + Event::PaymentClaimed { payment_hash: claimed_hash, amount_msat, htlcs, .. } => { + assert_eq!(*claimed_hash, payment_hash); + assert_eq!(*amount_msat, amt_msat); + assert_eq!(htlcs.len(), 2); + }, + _ => panic!("Unexpected event: {:?}", claim_events[0]), + } + // The `PaymentSent` event above releases the monitor update that nodes[0] held after the final + // channel A startup revocation. + check_added_monitors(&nodes[0], 1); + // Handling `PaymentClaimed` releases channel B's held revocation update and then the fulfill + // that was waiting behind it (unlike this test in 0.3, after we free the holding cell in + // get_and_clear_pending_msg_events below). + check_added_monitors(&nodes[1], 1); + + // Channel A's fulfill was already sent during startup. The `PaymentClaimed` completion action + // now frees channel B's held fulfill, and no other HTLC update should be bundled with it. + let fulfill_msgs = nodes[1].node.get_and_clear_pending_msg_events(); + check_added_monitors(&nodes[1], 1); + assert_eq!(fulfill_msgs.len(), 1); + match &fulfill_msgs[0] { + MessageSendEvent::UpdateHTLCs { node_id, updates } => { + assert_eq!(*node_id, node_0_id); + assert_eq!(updates.commitment_signed.channel_id, chan_id_b); + assert_eq!(updates.update_fulfill_htlcs.len(), 1); + assert!(updates.update_add_htlcs.is_empty()); + assert!(updates.update_fail_htlcs.is_empty()); + assert!(updates.update_fail_malformed_htlcs.is_empty()); + assert!(updates.update_fee.is_none()); + for fulfill in &updates.update_fulfill_htlcs { + nodes[0].node.handle_update_fulfill_htlc(node_1_id, fulfill); + } + // Complete the same commitment handshake for channel B. Here nodes[0]'s final monitor + // update is persisted immediately because `PaymentSent` already ran for channel A. + do_commitment_signed_dance( + &nodes[0], &nodes[1], &updates.commitment_signed, false, false, + ); + }, + _ => panic!("Unexpected fulfill message: {:?}", fulfill_msgs[0]), + } + check_added_monitors(&nodes[1], 0); + assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); + assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); + + let final_payment_events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(final_payment_events.len(), 1); + match &final_payment_events[0] { + Event::PaymentPathSuccessful { payment_hash: Some(path_hash), path, .. } => { + assert_eq!(*path_hash, payment_hash); + assert_eq!(path.hops.len(), 1); + assert_eq!(path.hops[0].short_channel_id, scid_b); + }, + _ => panic!("Unexpected final payment event: {:?}", final_payment_events[0]), + } + check_added_monitors(&nodes[0], 0); + assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); + check_added_monitors(&nodes[0], 0); + check_added_monitors(&nodes[1], 0); + + // Both MPP parts should have been fulfilled back to nodes[0]. If either channel still has a + // pending outbound HTLC, its fulfill remained stuck in nodes[1]'s holding cell after reload. + let pending: Vec<_> = nodes[0].node.list_channels().iter() + .filter(|channel| channel.channel_id == chan_id_a || channel.channel_id == chan_id_b) + .filter(|channel| !channel.pending_outbound_htlcs.is_empty()) + .map(|channel| channel.channel_id) + .collect(); + assert!(pending.is_empty(), "HTLC fulfills remained stuck on channels {:?}", pending); +} + fn do_forwarded_payment_no_manager_persistence(use_cs_commitment: bool, claim_htlc: bool, use_intercept: bool) { if !use_cs_commitment { assert!(!claim_htlc); } // If we go to forward a payment, and the ChannelMonitor persistence completes, but the From 60fe93de98c5c1bba98144a029d0f9de1f9b936c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 2 Apr 2026 16:38:15 +0000 Subject: [PATCH 06/17] Wipe empty entries from `actions_blocking_raa_monitor_updates` In a very specific case, forgetting to do so can lead to a debug assertion failure when we see a double-claim of an HTLC (see the included test). Found by @joostjager's work on growing the chanmon_consistency fuzzer. Backport of f14b4b2fd5889da3265be19db0891adcbc67068b Silent conflicts resolved in: * lightning/src/ln/functional_tests.rs --- lightning/src/ln/channelmanager.rs | 21 +++++---- lightning/src/ln/functional_tests.rs | 66 ++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 8 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 3b10363ce92..321e03e9974 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -7350,12 +7350,12 @@ where { if let Some(peer_state_mtx) = per_peer_state.get(&node_id) { let mut peer_state = peer_state_mtx.lock().unwrap(); - if let Some(blockers) = peer_state + let entry = peer_state .actions_blocking_raa_monitor_updates - .get_mut(&channel_id) - { + .entry(channel_id); + if let btree_map::Entry::Occupied(mut entry) = entry { let mut found_blocker = false; - blockers.retain(|iter| { + entry.get_mut().retain(|iter| { // Note that we could actually be blocked, in // which case we need to only remove the one // blocker which was added duplicatively. @@ -7363,6 +7363,9 @@ where if *iter == blocker { found_blocker = true; } *iter != blocker || !first_blocker }); + if entry.get().is_empty() { + entry.remove(); + } debug_assert!(found_blocker); } } else { @@ -10957,10 +10960,12 @@ where let peer_state = &mut *peer_state_lck; if let Some(blocker) = completed_blocker.take() { // Only do this on the first iteration of the loop. - if let Some(blockers) = peer_state.actions_blocking_raa_monitor_updates - .get_mut(&channel_id) - { - blockers.retain(|iter| iter != &blocker); + let entry = peer_state.actions_blocking_raa_monitor_updates.entry(channel_id); + if let btree_map::Entry::Occupied(mut entry) = entry { + entry.get_mut().retain(|iter| iter != &blocker); + if entry.get().is_empty() { + entry.remove(); + } } } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 41a5fb7cd4d..9fbcfecd41b 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -12017,3 +12017,69 @@ pub fn do_test_dust_limit_fee_accounting(can_afford: bool) { check_added_monitors(&nodes[1], 2); } } + +#[test] +fn test_dup_htlc_claim_onchain_and_offchain() { + // Tests what happens if we receive a claim first offchain, then see a counterparty broadcast + // their commitment transaction and re-claim the same HTLC on-chain. This was never broken, but + // the very specific ordering in this test did hit a debug assertion failure. + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + let legacy_cfg = test_default_channel_config(); + let node_chanmgrs = create_node_chanmgrs( + 3, + &node_cfgs, + &[Some(legacy_cfg.clone()), Some(legacy_cfg.clone()), Some(legacy_cfg)], + ); + let nodes = create_network(3, &node_cfgs, &node_chanmgrs); + + let node_b_id = nodes[1].node.get_our_node_id(); + let node_c_id = nodes[2].node.get_our_node_id(); + + create_announced_chan_between_nodes(&nodes, 0, 1); + let chan_bc = create_announced_chan_between_nodes(&nodes, 1, 2); + + // Route payment A -> B -> C. + let (payment_preimage, payment_hash, _, _) = + route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1_000_000); + + // C claims the payment. + nodes[2].node.claim_funds(payment_preimage); + expect_payment_claimed!(nodes[2], payment_hash, 1_000_000); + check_added_monitors(&nodes[2], 1); + + // Deliver only C's update_fulfill_htlc to B (NOT the commitment_signed). B learns + // the preimage and claims from A (adding an RAA blocker on B-C via + // internal_update_fulfill_htlc, then removing it when the A-B monitor update completes + // and the EmitEventOptionAndFreeOtherChannel action runs). + let cs_updates = get_htlc_update_msgs(&nodes[2], &node_b_id); + nodes[1].node.handle_update_fulfill_htlc(node_c_id, &cs_updates.update_fulfill_htlcs[0]); + check_added_monitors(&nodes[1], 1); + + // Ignore B's attempts to claim the HTLC from A. + nodes[1].node.get_and_clear_pending_msg_events(); + + // Get C's commitment transactions. C's commitment includes the HTLC and C has + // an HTLC-success transaction (claiming with preimage). Mine both on B. + let cs_txn = get_local_commitment_txn!(nodes[2], chan_bc.2); + assert!(cs_txn.len() >= 2, "Expected commitment + HTLC-success tx, got {}", cs_txn.len()); + + // Mine C's commitment on B. B sees the counterparty commitment on-chain. + mine_transaction(&nodes[1], &cs_txn[0]); + check_closed_broadcast(&nodes[1], 1, true); + check_added_monitors(&nodes[1], 1); + let events = nodes[1].node.get_and_clear_pending_events(); + assert!( + events.iter().any(|e| matches!(e, Event::ChannelClosed { .. })), + "Expected ChannelClosed event" + ); + + // Mine C's HTLC-success transaction. B's monitor sees the preimage being used on-chain + // and generates an HTLCEvent with the preimage. + mine_transaction(&nodes[1], &cs_txn[1]); + + // Advance past ANTI_REORG_DELAY so the on-chain HTLC resolution matures. This triggers + // the monitor to generate an HTLCEvent with the preimage via process_pending_monitor_events, + // which calls claim_funds_internal a second time. + connect_blocks(&nodes[1], ANTI_REORG_DELAY); +} From 8a39057b4e986cc2638333c3d8438bc37eac5bf6 Mon Sep 17 00:00:00 2001 From: Swagmuffin Date: Mon, 6 Apr 2026 19:14:45 -0700 Subject: [PATCH 07/17] Bypass channel monitor sync requests when no partition key given Backport of 6bf2352dc9eb7aaeafacd418a95e9d28d139f2d0 Conflicts resolved in: * lightning/src/chain/chainmonitor.rs --- lightning/src/chain/chainmonitor.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs index f1b9f729cb3..5136b07c0f3 100644 --- a/lightning/src/chain/chainmonitor.rs +++ b/lightning/src/chain/chainmonitor.rs @@ -327,7 +327,7 @@ where C::Target: chain::Filter, let funding_txid_hash = funding_outpoint.txid.to_raw_hash(); let funding_txid_hash_bytes = funding_txid_hash.as_byte_array(); let funding_txid_u32 = u32::from_be_bytes([funding_txid_hash_bytes[0], funding_txid_hash_bytes[1], funding_txid_hash_bytes[2], funding_txid_hash_bytes[3]]); - funding_txid_u32.wrapping_add(best_height.unwrap_or_default()) + best_height.map(|height| funding_txid_u32.wrapping_add(height)) }; let partition_factor = if channel_count < 15 { @@ -337,7 +337,7 @@ where C::Target: chain::Filter, }; let has_pending_claims = monitor_state.monitor.has_pending_claims(); - if has_pending_claims || get_partition_key(funding_outpoint) % partition_factor == 0 { + if has_pending_claims || get_partition_key(funding_outpoint).is_some_and(|key| key % partition_factor == 0) { log_trace!(logger, "Syncing Channel Monitor for channel {}", log_funding_info!(monitor)); // Even though we don't track monitor updates from chain-sync as pending, we still want // updates per-channel to be well-ordered so that users don't see a From 18389e3c6d93f7aba0e655a7e07559f0b4e87776 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Mon, 13 Apr 2026 10:49:06 +0200 Subject: [PATCH 08/17] Apply MPP receive timeout to keysend payments Incomplete keysend MPPs skipped the receive timeout path, allowing partial payments to hold HTLC slots until CLTV expiry instead of failing after `MPP_TIMEOUT_TICKS`. Apply the existing `total_mpp_amount_msat` completeness check to all MPP receives and add a regression test covering the keysend case. The timeout logic was originally added only for invoice-backed MPPs in 2022, and that invoice-only guard remained when receive-side MPP keysend support landed in 2023, leaving this gap latent until now. Co-Authored-By: HAL 9000 Backport of fd8846b5c8016f7b34166a69e8d3bd9617622611 Conflicts resolved in: * lightning/src/ln/channelmanager.rs * lightning/src/ln/payment_tests.rs --- lightning/src/ln/channelmanager.rs | 31 ++++++++--------- lightning/src/ln/payment_tests.rs | 54 +++++++++++++++++++++++++----- 2 files changed, 60 insertions(+), 25 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 321e03e9974..538068ab594 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -6736,23 +6736,20 @@ where debug_assert!(false); return false; } - if let OnionPayload::Invoice { .. } = payment.htlcs[0].onion_payload { - // Check if we've received all the parts we need for an MPP (the value of the parts adds to total_msat). - // In this case we're not going to handle any timeouts of the parts here. - // This condition determining whether the MPP is complete here must match - // exactly the condition used in `process_pending_htlc_forwards`. - if payment.htlcs[0].total_msat <= payment.htlcs.iter() - .fold(0, |total, htlc| total + htlc.sender_intended_value) - { - return true; - } else if payment.htlcs.iter_mut().any(|htlc| { - htlc.timer_ticks += 1; - return htlc.timer_ticks >= MPP_TIMEOUT_TICKS - }) { - timed_out_mpp_htlcs.extend(payment.htlcs.drain(..) - .map(|htlc: ClaimableHTLC| (htlc.prev_hop, *payment_hash))); - return false; - } + // Check if we've received all the parts we need for an MPP. + // This condition determining whether the MPP is complete here must match + // exactly the condition used in `process_pending_htlc_forwards`. + if payment.htlcs[0].total_msat <= payment.htlcs.iter() + .fold(0, |total, htlc| total + htlc.sender_intended_value) + { + return true; + } else if payment.htlcs.iter_mut().any(|htlc| { + htlc.timer_ticks += 1; + return htlc.timer_ticks >= MPP_TIMEOUT_TICKS + }) { + timed_out_mpp_htlcs.extend(payment.htlcs.drain(..) + .map(|htlc: ClaimableHTLC| (htlc.prev_hop, *payment_hash))); + return false; } true }); diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 47c12c358eb..ea3070e6bd2 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -290,7 +290,7 @@ fn mpp_retry_overpay() { expect_payment_sent!(&nodes[0], payment_preimage, Some(expected_total_fee_msat)); } -fn do_mpp_receive_timeout(send_partial_mpp: bool) { +fn do_mpp_receive_timeout(send_partial_mpp: bool, keysend: bool) { let chanmon_cfgs = create_chanmon_cfgs(4); let node_cfgs = create_node_cfgs(4, &chanmon_cfgs); let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]); @@ -301,7 +301,12 @@ fn do_mpp_receive_timeout(send_partial_mpp: bool) { let (chan_3_update, _, chan_3_id, _) = create_announced_chan_between_nodes(&nodes, 1, 3); let (chan_4_update, _, _, _) = create_announced_chan_between_nodes(&nodes, 2, 3); - let (mut route, payment_hash, payment_preimage, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[3], 100_000); + let (mut route, payment_hash, payment_preimage, payment_secret) = if keysend { + let payment_params = PaymentParameters::for_keysend(nodes[3].node.get_our_node_id(), TEST_FINAL_CLTV, true); + get_route_and_payment_hash!(nodes[0], nodes[3], payment_params, 100_000) + } else { + get_route_and_payment_hash!(nodes[0], nodes[3], 100_000) + }; let path = route.paths[0].clone(); route.paths.push(path); route.paths[0].hops[0].pubkey = nodes[1].node.get_our_node_id(); @@ -312,9 +317,24 @@ fn do_mpp_receive_timeout(send_partial_mpp: bool) { route.paths[1].hops[1].short_channel_id = chan_4_update.contents.short_channel_id; // Initiate the MPP payment. - nodes[0].node.send_payment_with_route(route, payment_hash, - RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0)).unwrap(); - check_added_monitors!(nodes[0], 2); // one monitor per path + let onion = RecipientOnionFields::secret_only(payment_secret); + if keysend { + let route_params = route.route_params.clone().unwrap(); + nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone())); + nodes[0] + .node + .send_spontaneous_payment( + Some(payment_preimage), + onion, + PaymentId(payment_hash.0), + route_params, + Retry::Attempts(0), + ) + .unwrap(); + } else { + nodes[0].node.send_payment_with_route(route, payment_hash, onion, PaymentId(payment_hash.0)).unwrap(); + } + check_added_monitors(&nodes[0], 2); // one monitor per path let mut events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 2); @@ -348,7 +368,19 @@ fn do_mpp_receive_timeout(send_partial_mpp: bool) { } else { // Pass half of the payment along the second path. let node_2_msgs = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut events); - pass_along_path(&nodes[0], &[&nodes[2], &nodes[3]], 200_000, payment_hash, Some(payment_secret), node_2_msgs, true, None); + let path = &[&nodes[2], &nodes[3]]; + let payment_secret = Some(payment_secret); + let expected_preimage = if keysend { Some(payment_preimage) } else { None }; + pass_along_path( + &nodes[0], + path, + 200_000, + payment_hash, + payment_secret, + node_2_msgs, + true, + expected_preimage, + ); // Even after MPP_TIMEOUT_TICKS we should not timeout the MPP if we have all the parts for _ in 0..MPP_TIMEOUT_TICKS { @@ -363,8 +395,14 @@ fn do_mpp_receive_timeout(send_partial_mpp: bool) { #[test] fn mpp_receive_timeout() { - do_mpp_receive_timeout(true); - do_mpp_receive_timeout(false); + do_mpp_receive_timeout(true, false); + do_mpp_receive_timeout(false, false); +} + +#[test] +fn keysend_mpp_receive_timeout() { + do_mpp_receive_timeout(true, true); + do_mpp_receive_timeout(false, true); } #[test] From f6f740de45fc1e520bb6ceabf2ed816fa5d897e9 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Tue, 5 May 2026 19:45:38 +0200 Subject: [PATCH 09/17] Fix signed comparison in `ElectrumClient` `GetHistoryRes::height` from electrum-client is a *signed* integer. Here we first check for `<= 0` *before* casting to `u32`. Signed-off-by: Elias Rohrer Backport of 8b383bb8d7586f151ba9cde8bfbe5f991473199c --- lightning-transaction-sync/src/electrum.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lightning-transaction-sync/src/electrum.rs b/lightning-transaction-sync/src/electrum.rs index a442ff9e119..f26f7624983 100644 --- a/lightning-transaction-sync/src/electrum.rs +++ b/lightning-transaction-sync/src/electrum.rs @@ -335,11 +335,11 @@ where let mut filtered_history = script_history.iter().filter(|h| h.tx_hash == **txid); if let Some(history) = filtered_history.next() { - let prob_conf_height = history.height as u32; - if prob_conf_height <= 0 { + if history.height <= 0 { // Skip if it's a an unconfirmed entry. continue; } + let prob_conf_height = history.height as u32; let confirmed_tx = self.get_confirmed_tx(tx, prob_conf_height)?; confirmed_txs.push(confirmed_tx); } From 151d16efaeb0c04a9db024b779261cf01876cbc2 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Tue, 5 May 2026 19:50:17 +0200 Subject: [PATCH 10/17] Free pending_query_count slot when DNS proof build fails `OMDomainResolver` rate-limits in-flight DNSSEC proof builds via a `pending_query_count` counter capped at `MAX_PENDING_RESPONSES` (1024). The counter was only released when the proof build succeeded, so any failure mode -- NXDOMAIN, insecure zones, unreachable resolvers, I/O timeouts, malformed names -- permanently consumed a slot. Because the queried name is attacker-controlled (it travels in over a `DNSSECQuery` onion message from any LN peer, given DNS resolution is an opt-in network-advertised feature), an adversary could exhaust the counter with ~1025 failing queries and persistently DoS the resolver for any subsequent legitimate BIP-353 lookups, until the process is restarted. Always release the slot once the proof build completes, regardless of outcome, and add a regression test which points the resolver at a TCP-refusing local port and asserts the counter returns to zero. Co-Authored-By: HAL 9000 Backport of fb4103d7788414b2b462911dfb988c70380b5f1d Conflicts resolved in: * lightning-dns-resolver/src/lib.rs --- lightning-dns-resolver/src/lib.rs | 90 ++++++++++++++++++++++++++++++- 1 file changed, 89 insertions(+), 1 deletion(-) diff --git a/lightning-dns-resolver/src/lib.rs b/lightning-dns-resolver/src/lib.rs index 8f855cb5fb7..03d27e64ec8 100644 --- a/lightning-dns-resolver/src/lib.rs +++ b/lightning-dns-resolver/src/lib.rs @@ -135,8 +135,8 @@ where let contents = DNSResolverMessage::DNSSECProof(DNSSECProof { name: q.0, proof }); let instructions = responder.respond().into_instructions(); us.pending_replies.lock().unwrap().push((contents, instructions)); - us.pending_query_count.fetch_sub(1, Ordering::Relaxed); } + us.pending_query_count.fetch_sub(1, Ordering::Relaxed); }); None } @@ -459,4 +459,92 @@ mod test { expect_payment_sent(&nodes[0], our_payment_preimage, None, true, true); } + + #[tokio::test] + async fn failed_query_does_not_leak_pending_counter() { + use std::sync::atomic::Ordering; + + let secp_ctx = Secp256k1::new(); + + // Resolver points at a port that should refuse TCP, so build_txt_proof_async + // returns Err quickly. + let resolver_keys = Arc::new(KeysManager::new(&[99; 32], 42, 43)); + let resolver_logger = TestLogger { node: "resolver" }; + let resolver = + Arc::new(OMDomainResolver::::ignoring_incoming_proofs( + "127.0.0.1:1".parse().unwrap(), + )); + let resolver_state = Arc::clone(&resolver.state); + let resolver_messenger = OnionMessenger::new( + Arc::clone(&resolver_keys), + Arc::clone(&resolver_keys), + resolver_logger, + DummyNodeLookup {}, + DirectlyConnectedRouter {}, + IgnoringMessageHandler {}, + IgnoringMessageHandler {}, + Arc::clone(&resolver), + IgnoringMessageHandler {}, + ); + let resolver_id = resolver_keys.get_node_id(Recipient::Node).unwrap(); + + let resolver_dest = Destination::Node(resolver_id); + let now = SystemTime::now().duration_since(SystemTime::UNIX_EPOCH).unwrap().as_secs(); + + let payment_id = PaymentId([42; 32]); + let name = HumanReadableName::from_encoded("matt@mattcorallo.com").unwrap(); + + let payer_keys = Arc::new(KeysManager::new(&[2; 32], 42, 43)); + let payer_logger = TestLogger { node: "payer" }; + let payer_id = payer_keys.get_node_id(Recipient::Node).unwrap(); + let payer = Arc::new(URIResolver { + resolved_uri: Mutex::new(None), + resolver: OMNameResolver::new(now as u32, 1), + pending_messages: Mutex::new(Vec::new()), + }); + let payer_messenger = Arc::new(OnionMessenger::new( + Arc::clone(&payer_keys), + Arc::clone(&payer_keys), + payer_logger, + DummyNodeLookup {}, + DirectlyConnectedRouter {}, + IgnoringMessageHandler {}, + IgnoringMessageHandler {}, + Arc::clone(&payer), + IgnoringMessageHandler {}, + )); + + let init_msg = get_om_init(); + payer_messenger.peer_connected(resolver_id, &init_msg, true).unwrap(); + resolver_messenger.peer_connected(payer_id, &init_msg, false).unwrap(); + + let (msg, context) = + payer.resolver.resolve_name(payment_id, name.clone(), &*payer_keys).unwrap(); + let query_context = MessageContext::DNSResolver(context); + let reply_path = BlindedMessagePath::one_hop( + payer_id, + query_context, + &*payer_keys, + &secp_ctx, + ).unwrap(); + payer.pending_messages.lock().unwrap().push(( + DNSResolverMessage::DNSSECQuery(msg), + MessageSendInstructions::WithSpecifiedReplyPath { + destination: resolver_dest, + reply_path, + }, + )); + + let query = payer_messenger.next_onion_message_for_peer(resolver_id).unwrap(); + resolver_messenger.handle_onion_message(payer_id, &query); + + let start = Instant::now(); + while resolver_state.pending_query_count.load(Ordering::Relaxed) != 0 { + tokio::time::sleep(Duration::from_millis(50)).await; + assert!( + start.elapsed() < Duration::from_secs(10), + "pending_query_count not decremented after failed proof: counter leaks" + ); + } + } } From b464ab744be7b70476ef90beae31b9c7b2787016 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Tue, 5 May 2026 20:10:01 +0200 Subject: [PATCH 11/17] Strip Unicode `Cf` characters in `PrintableString` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `PrintableString` is the sanitiser LDK uses to render untrusted strings (node aliases, BOLT-12 invoice / offer text, `UntrustedString`, LSPS messages, `lightning-invoice` descriptions) to logs and UI. It only replaced `char::is_control` matches (Unicode general category `Cc`) with U+FFFD, leaving the entire `Cf` (Format) category untouched. That is the exact category covering the bidirectional override / isolate codepoints (U+202A..U+202E, U+2066..U+2069) and zero-width characters (U+200B..U+200D, U+FEFF) behind the "Trojan Source" attack family (CVE-2021-42574): a peer can set its alias / invoice description / offer fields to e.g. `safe\u{202E}cipsxe.exe`, which previously passed through verbatim while a human reader sees `safeexe.cips` — defeating the threat model `PrintableString` exists to defend against. Replace `Cf` codepoints alongside `Cc` ones. The `Cf` ranges are inlined as a `matches!` table sourced from Unicode 16.0 to keep the change `no_std`-friendly with no new dependencies. Co-Authored-By: HAL 9000 Signed-off-by: Elias Rohrer Backport of 1a01b5ae4fb74bfff763b968719e362e546bd594 --- lightning-types/src/string.rs | 59 ++++++++++++++++++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) diff --git a/lightning-types/src/string.rs b/lightning-types/src/string.rs index ae5395a5289..e45c17d8586 100644 --- a/lightning-types/src/string.rs +++ b/lightning-types/src/string.rs @@ -31,7 +31,11 @@ impl<'a> fmt::Display for PrintableString<'a> { fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> { use core::fmt::Write; for c in self.0.chars() { - let c = if c.is_control() { core::char::REPLACEMENT_CHARACTER } else { c }; + let c = if c.is_control() || is_format_char(c) { + core::char::REPLACEMENT_CHARACTER + } else { + c + }; f.write_char(c)?; } @@ -39,6 +43,39 @@ impl<'a> fmt::Display for PrintableString<'a> { } } +// Codepoints in Unicode general category `Cf` (Format), per Unicode standard. These are not +// matched by `char::is_control` (which only covers `Cc`), but include the bidirectional override / +// isolate controls (e.g. U+202E RLO) and zero-width characters behind the "Trojan Source" attack +// family (CVE-2021-42574), where an attacker-supplied string renders to a human reader as +// something other than its byte content. Strip them alongside `Cc` characters when sanitising +// untrusted input. +fn is_format_char(c: char) -> bool { + matches!( + c as u32, + 0x00AD + | 0x0600..=0x0605 + | 0x061C + | 0x06DD + | 0x070F + | 0x0890..=0x0891 + | 0x08E2 + | 0x180E + | 0x200B..=0x200F + | 0x202A..=0x202E + | 0x2060..=0x2064 + | 0x2066..=0x206F + | 0xFEFF + | 0xFFF9..=0xFFFB + | 0x110BD + | 0x110CD + | 0x13430..=0x1343F + | 0x1BCA0..=0x1BCA3 + | 0x1D173..=0x1D17A + | 0xE0001 + | 0xE0020..=0xE007F + ) +} + #[cfg(test)] mod tests { use super::PrintableString; @@ -50,4 +87,24 @@ mod tests { "I \u{1F496} LDK!\u{FFFD}\u{26A1}", ); } + + #[test] + fn sanitizes_unicode_bidi_override_characters() { + // U+202E RIGHT-TO-LEFT OVERRIDE and friends are Unicode general category + // `Cf` (Format), not `Cc` (Control). They enable "Trojan Source" / + // bidi-spoofing attacks where an attacker-supplied string (e.g. a node + // alias gossiped from a peer) renders to a human reader as something + // other than its byte content. `PrintableString` is the sanitiser used + // for exactly these untrusted strings, so it must replace them. + let rendered = format!("{}", PrintableString("safe\u{202E}cipsxe.exe")); + assert!( + !rendered.contains('\u{202E}'), + "PrintableString left a U+202E RLO override in its output: {:?}", + rendered + ); + + // U+13440 is in the Egyptian Hieroglyph Format Controls block, but its + // general category is `Mn`, not `Cf`, so the `Cf` range ends at U+1343F. + assert_eq!(format!("{}", PrintableString("x\u{1343F}y\u{13440}z")), "x\u{FFFD}y\u{13440}z"); + } } From 9fcd60610efbdf67dadfab10582cf1f9c35e6087 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 7 May 2026 18:33:12 +0000 Subject: [PATCH 12/17] Add an auto-generated unicode character category file 1a01b5ae4fb74bfff763b968719e362e546bd594 added detection of unicode format characters in `PrintableString`, but used a hard-coded table which may eventually become out of date. Here we switch to an auto-generated table, include all `General_Category` `Other` characters, and also ban unallocated code points. Finally, CI validates that the file is kept up to date. Written by Claude Backport of 65e8cc8d5bb85af67efc29c006c613804ba1f44f --- .github/workflows/check_unicode.yml | 26 + contrib/gen_unicode_general_category.py | 308 +++++++++ lightning-types/src/lib.rs | 1 + lightning-types/src/string.rs | 39 +- lightning-types/src/unicode.rs | 799 ++++++++++++++++++++++++ 5 files changed, 1139 insertions(+), 34 deletions(-) create mode 100644 .github/workflows/check_unicode.yml create mode 100755 contrib/gen_unicode_general_category.py create mode 100644 lightning-types/src/unicode.rs diff --git a/.github/workflows/check_unicode.yml b/.github/workflows/check_unicode.yml new file mode 100644 index 00000000000..a01add3f814 --- /dev/null +++ b/.github/workflows/check_unicode.yml @@ -0,0 +1,26 @@ +name: Unicode listing up to date +on: + workflow_dispatch: + schedule: + - cron: '42 3 * * *' + +jobs: + check-unicode: + runs-on: ubuntu-latest + permissions: + issues: write + steps: + - name: Checkout source code + uses: actions/checkout@v4 + - name: Check unicode file state + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + curl --proto '=https' --tlsv1.2 -fsSL -o /tmp/UnicodeData.txt https://www.unicode.org/Public/UCD/latest/ucd/UnicodeData.txt + contrib/gen_unicode_general_category.py /tmp/UnicodeData.txt -o /tmp/unicode.rs + if ! diff -u lightning-types/src/unicode.rs /tmp/unicode.rs; then + TITLE="Unicode listing out of date: ${{ github.workflow }}" + RUN_URL="https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}" + BODY="The unicode character listing is out of date, see $RUN_URL" + gh issue create --title "$TITLE" --body "$BODY" + fi diff --git a/contrib/gen_unicode_general_category.py b/contrib/gen_unicode_general_category.py new file mode 100755 index 00000000000..4871e967b55 --- /dev/null +++ b/contrib/gen_unicode_general_category.py @@ -0,0 +1,308 @@ +#!/usr/bin/env python3 +# This file is Copyright its original authors, visible in version control +# history. +# +# This file is licensed under the Apache License, Version 2.0 or the MIT license +# , at your option. +# You may not use this file except in accordance with one or both of these +# licenses. + +"""Generate Unicode general-category predicates from `UnicodeData.txt`. + +Emits two `pub(crate)` functions taking a `char`, split into two disjoint +buckets across the Unicode top-level `C` ("Other") category so callers can +compose them: + + is_unicode_general_category_other — Cc / Cf / Cs / Co (assigned) + is_unicode_general_category_unassigned — Cn (plus codepoints above + U+10FFFF, which aren't + valid codepoints at all) + +`UnicodeData.txt` is the canonical machine-readable listing of every assigned +codepoint in the Unicode Character Database. Each line is `;`-separated; field +0 is the codepoint (hex), field 1 is the name, and field 2 is the two-letter +general category (e.g. `Lu`, `Cf`, `Mn`). Codepoints absent from the file have +category `Cn` (Unassigned) by convention. + +Two encoding details to preserve: + * Large blocks of contiguous same-category codepoints are written as two + consecutive entries whose names end in `, First>` and `, Last>`. Every + codepoint between First and Last (inclusive) shares the listed category. + * The codepoint range is U+0000..=U+10FFFF. + +Each `matches!` arm in the assigned-Other table carries an end-of-line comment +derived from the `UnicodeData.txt` name field — typically the longest common +word prefix or suffix across the names in the range, falling back to the set +of categories when the names share nothing meaningful. The unassigned table +omits per-arm comments since every range there has the same meaning by +construction. + +Usage: + contrib/gen_unicode_general_category.py UnicodeData.txt > out.rs +""" + +import argparse +import sys +from pathlib import Path + +MAX_CODEPOINT = 0x10FFFF + +LICENSE_HEADER = """\ +// This file is Copyright its original authors, visible in version control +// history. +// +// This file is licensed under the Apache License, Version 2.0 or the MIT license +// , at your option. +// You may not use this file except in accordance with one or both of these +// licenses. +""" + +GENERATED_NOTICE = """\ +// Auto-generated from the Unicode Character Database (UnicodeData.txt) by +// contrib/gen_unicode_general_category.py. Do not edit by hand; rerun the +// generator with an updated UnicodeData.txt to refresh the table. +""" + + +def _normalize_name(name): + """Strip the `<...>` wrapping and `, First` / `, Last` range markers so + that, e.g., `` becomes + `Non Private Use High Surrogate` and `` becomes `control`. + """ + if name.startswith("<") and name.endswith(">"): + inner = name[1:-1] + for suffix in (", First", ", Last"): + if inner.endswith(suffix): + inner = inner[: -len(suffix)] + return inner + return name + + +def parse_categories(path): + """Return `(cats, names)` mapping every codepoint listed in `path` to its + general category and to its (normalised) name. Codepoints absent from the + returned dicts have category `Cn` (Unassigned) and no name. + """ + cats = {} + names = {} + pending_first = None # (first_cp, first_cat, normalised_name) once a range opens. + with path.open() as f: + for lineno, raw in enumerate(f, 1): + line = raw.rstrip("\n") + if not line: + continue + fields = line.split(";") + if len(fields) < 3: + raise ValueError(f"{path}:{lineno}: expected at least 3 fields, got {len(fields)}") + cp = int(fields[0], 16) + name = fields[1] + cat = fields[2] + if pending_first is not None: + first_cp, first_cat, first_name = pending_first + if not name.endswith(", Last>"): + raise ValueError( + f"{path}:{lineno}: expected `, Last>` to close range " + f"opened at U+{first_cp:04X}, got name {name!r}" + ) + if cat != first_cat: + raise ValueError( + f"{path}:{lineno}: range U+{first_cp:04X}..=U+{cp:04X} " + f"has mismatched categories {first_cat!r} / {cat!r}" + ) + for x in range(first_cp, cp + 1): + cats[x] = cat + names[x] = first_name + pending_first = None + elif name.endswith(", First>"): + pending_first = (cp, cat, _normalize_name(name)) + else: + cats[cp] = cat + names[cp] = _normalize_name(name) + if pending_first is not None: + raise ValueError(f"{path}: dangling `, First>` entry at U+{pending_first[0]:04X}") + return cats, names + + +ASSIGNED_OTHER_CATS = frozenset({"Cc", "Cf", "Cs", "Co"}) + + +def coalesce_ranges(cats, names, target_cats, *, label): + """Walk U+0000..=U+10FFFF and return a list of `(start, end, label)` for + every contiguous run of codepoints whose general category is in + `target_cats`. Codepoints absent from `cats` are treated as `Cn`. + + If `label` is `True`, attach a comment summarising the codepoint names in + each range; otherwise every range gets an empty label. + """ + ranges = [] + start = None + for cp in range(MAX_CODEPOINT + 1): + in_target = cats.get(cp, "Cn") in target_cats + if in_target and start is None: + start = cp + elif not in_target and start is not None: + ranges.append((start, cp - 1)) + start = None + if start is not None: + ranges.append((start, MAX_CODEPOINT)) + + if not label: + return [(s, e, "") for s, e in ranges] + + labelled = [] + for s, e in ranges: + range_names = [] + range_cats = set() + for cp in range(s, e + 1): + range_cats.add(cats.get(cp, "Cn")) + n = names.get(cp) + if n is not None: + range_names.append(n) + labelled.append((s, e, _make_label(range_names, range_cats))) + return labelled + + +def _common_word_run(names, *, from_end): + """Return the longest sequence of words shared by every name, taken from + either the start (`from_end=False`) or the end (`from_end=True`) of each + name's whitespace-split tokens. + """ + if not names: + return "" + tokenised = [n.split() for n in names] + if from_end: + tokenised = [list(reversed(t)) for t in tokenised] + limit = min(len(t) for t in tokenised) + common = [] + for i in range(limit): + token = tokenised[0][i] + if all(t[i] == token for t in tokenised): + common.append(token) + else: + break + if from_end: + common.reverse() + return " ".join(common) + + +def _make_label(names, cats_in_range): + """Build a short human-readable label for a coalesced range. Applied to + the assigned-Other buckets only; each range there is `Cc`, `Cf`, `Cs`, + `Co`, or some contiguous union thereof. + + Rules, in order: + 1. All names identical → that name (e.g. `control`). + 2. Common leading or trailing words → the longer of the two. + 3. Otherwise, list the categories present (e.g. `Co / Cs`). + """ + unique = list(dict.fromkeys(names)) + if len(unique) == 1: + return unique[0] + + prefix = _common_word_run(names, from_end=False) + suffix = _common_word_run(names, from_end=True) + # Pick whichever is more informative; when both are non-empty, prefer the + # longer one. A multi-word prefix beats a single-word suffix. + label = prefix if len(prefix) >= len(suffix) else suffix + if label: + return label + return " / ".join(sorted(cats_in_range)) + + +def fmt_codepoint(cp): + # `UnicodeData.txt` uses 4-digit hex for the BMP and wider for higher + # planes; mirror that so the output stays readable next to the source data. + return f"0x{cp:04X}" if cp <= 0xFFFF else f"0x{cp:X}" + + +def _pattern(start, end): + if start == end: + return fmt_codepoint(start) + return f"{fmt_codepoint(start)}..={fmt_codepoint(end)}" + + +def _emit_matches_body(lines, arms): + """Append a `matches!(c as u32, ...)` body to `lines`, with one + `(pattern, label)` tuple per arm. The first arm sits at the `matches!` + argument indent and continuation `| ...` arms indent one level deeper, + matching the rustfmt convention used elsewhere in the tree. + """ + lines.append("\tmatches!(") + lines.append("\t\tc as u32,") + for i, (pattern, label) in enumerate(arms): + prefix = "\t\t" if i == 0 else "\t\t\t| " + comment = f" // {label}" if label else "" + lines.append(f"{prefix}{pattern}{comment}") + lines.append("\t)") + + +def render_rust(other_ranges, unassigned_ranges): + """Render the final Rust source defining both `char`-taking predicates. + + `other_ranges` and `unassigned_ranges` are lists of `(start, end, label)`. + The unassigned function additionally gets a synthetic final arm catching + `u32` values above U+10FFFF — these aren't valid Unicode codepoints, so + by definition they have no general category and the unassigned bucket is + the closest match. + """ + lines = [LICENSE_HEADER, GENERATED_NOTICE] + + lines.append("/// Returns `true` if `c` is in Unicode general category `Cc` (Control), `Cf`") + lines.append("/// (Format), `Cs` (Surrogate), or `Co` (Private Use) — the assigned codepoints") + lines.append("/// in the top-level `C` (\"Other\") category. The `Cs` portion of the table is") + lines.append("/// unreachable for `char` input (a `char` cannot hold a surrogate) but is kept") + lines.append("/// so the table mirrors the source UCD data verbatim. The disjoint `Cn`") + lines.append("/// (Unassigned) bucket is `is_unicode_general_category_unassigned`.") + lines.append("#[allow(dead_code)]") + lines.append("pub(crate) fn is_unicode_general_category_other(c: char) -> bool {") + other_arms = [(_pattern(s, e), label) for s, e, label in other_ranges] + _emit_matches_body(lines, other_arms) + lines.append("}") + lines.append("") + + lines.append("/// Returns `true` if `c` is in Unicode general category `Cn` (Unassigned), or") + lines.append("/// strictly above U+10FFFF. The trailing `0x110000..=u32::MAX` arm is") + lines.append("/// unreachable for `char` input (a `char` is bounded to U+10FFFF) but is kept") + lines.append("/// for defensive coverage of the underlying `u32`. The disjoint Cc / Cf / Cs /") + lines.append("/// Co bucket is `is_unicode_general_category_other`.") + lines.append("#[allow(dead_code)]") + lines.append("pub(crate) fn is_unicode_general_category_unassigned(c: char) -> bool {") + unassigned_arms = [(_pattern(s, e), label) for s, e, label in unassigned_ranges] + unassigned_arms.append(("0x110000..=u32::MAX", "above U+10FFFF — unreachable for `char`")) + _emit_matches_body(lines, unassigned_arms) + lines.append("}") + lines.append("") + + return "\n".join(lines) + + +def main(argv): + ap = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + ap.add_argument("unicode_data", type=Path, help="Path to UnicodeData.txt") + ap.add_argument( + "-o", "--output", type=Path, default=None, + help="Output Rust file (default: stdout)", + ) + args = ap.parse_args(argv) + + cats, names = parse_categories(args.unicode_data) + other = coalesce_ranges(cats, names, ASSIGNED_OTHER_CATS, label=True) + unassigned = coalesce_ranges(cats, names, frozenset({"Cn"}), label=False) + rust = render_rust(other, unassigned) + + if args.output is None: + sys.stdout.write(rust) + else: + args.output.write_text(rust) + print( + f"Wrote {args.output} " + f"({len(other)} assigned-Other ranges, " + f"{len(unassigned)} unassigned ranges).", + file=sys.stderr, + ) + + +if __name__ == "__main__": + main(sys.argv[1:]) diff --git a/lightning-types/src/lib.rs b/lightning-types/src/lib.rs index 49e7e59084e..ecd2fff2147 100644 --- a/lightning-types/src/lib.rs +++ b/lightning-types/src/lib.rs @@ -27,3 +27,4 @@ pub mod features; pub mod payment; pub mod routing; pub mod string; +mod unicode; diff --git a/lightning-types/src/string.rs b/lightning-types/src/string.rs index e45c17d8586..a21cad411be 100644 --- a/lightning-types/src/string.rs +++ b/lightning-types/src/string.rs @@ -12,6 +12,8 @@ use alloc::string::String; use core::fmt; +use crate::unicode::*; + /// Struct to `Display` fields in a safe way using `PrintableString` #[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, Default)] pub struct UntrustedString(pub String); @@ -31,7 +33,9 @@ impl<'a> fmt::Display for PrintableString<'a> { fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> { use core::fmt::Write; for c in self.0.chars() { - let c = if c.is_control() || is_format_char(c) { + let is_other = is_unicode_general_category_other(c); + let is_unassigned = is_unicode_general_category_unassigned(c); + let c = if c.is_control() || is_other || is_unassigned { core::char::REPLACEMENT_CHARACTER } else { c @@ -43,39 +47,6 @@ impl<'a> fmt::Display for PrintableString<'a> { } } -// Codepoints in Unicode general category `Cf` (Format), per Unicode standard. These are not -// matched by `char::is_control` (which only covers `Cc`), but include the bidirectional override / -// isolate controls (e.g. U+202E RLO) and zero-width characters behind the "Trojan Source" attack -// family (CVE-2021-42574), where an attacker-supplied string renders to a human reader as -// something other than its byte content. Strip them alongside `Cc` characters when sanitising -// untrusted input. -fn is_format_char(c: char) -> bool { - matches!( - c as u32, - 0x00AD - | 0x0600..=0x0605 - | 0x061C - | 0x06DD - | 0x070F - | 0x0890..=0x0891 - | 0x08E2 - | 0x180E - | 0x200B..=0x200F - | 0x202A..=0x202E - | 0x2060..=0x2064 - | 0x2066..=0x206F - | 0xFEFF - | 0xFFF9..=0xFFFB - | 0x110BD - | 0x110CD - | 0x13430..=0x1343F - | 0x1BCA0..=0x1BCA3 - | 0x1D173..=0x1D17A - | 0xE0001 - | 0xE0020..=0xE007F - ) -} - #[cfg(test)] mod tests { use super::PrintableString; diff --git a/lightning-types/src/unicode.rs b/lightning-types/src/unicode.rs new file mode 100644 index 00000000000..22b21969365 --- /dev/null +++ b/lightning-types/src/unicode.rs @@ -0,0 +1,799 @@ +// This file is Copyright its original authors, visible in version control +// history. +// +// This file is licensed under the Apache License, Version 2.0 or the MIT license +// , at your option. +// You may not use this file except in accordance with one or both of these +// licenses. + +// Auto-generated from the Unicode Character Database (UnicodeData.txt) by +// contrib/gen_unicode_general_category.py. Do not edit by hand; rerun the +// generator with an updated UnicodeData.txt to refresh the table. + +/// Returns `true` if `c` is in Unicode general category `Cc` (Control), `Cf` +/// (Format), `Cs` (Surrogate), or `Co` (Private Use) — the assigned codepoints +/// in the top-level `C` ("Other") category. The `Cs` portion of the table is +/// unreachable for `char` input (a `char` cannot hold a surrogate) but is kept +/// so the table mirrors the source UCD data verbatim. The disjoint `Cn` +/// (Unassigned) bucket is `is_unicode_general_category_unassigned`. +#[allow(dead_code)] +pub(crate) fn is_unicode_general_category_other(c: char) -> bool { + matches!( + c as u32, + 0x0000..=0x001F // control + | 0x007F..=0x009F // control + | 0x00AD // SOFT HYPHEN + | 0x0600..=0x0605 // ARABIC + | 0x061C // ARABIC LETTER MARK + | 0x06DD // ARABIC END OF AYAH + | 0x070F // SYRIAC ABBREVIATION MARK + | 0x0890..=0x0891 // MARK ABOVE + | 0x08E2 // ARABIC DISPUTED END OF AYAH + | 0x180E // MONGOLIAN VOWEL SEPARATOR + | 0x200B..=0x200F // Cf + | 0x202A..=0x202E // Cf + | 0x2060..=0x2064 // Cf + | 0x2066..=0x206F // Cf + | 0xD800..=0xF8FF // Co / Cs + | 0xFEFF // ZERO WIDTH NO-BREAK SPACE + | 0xFFF9..=0xFFFB // INTERLINEAR ANNOTATION + | 0x110BD // KAITHI NUMBER SIGN + | 0x110CD // KAITHI NUMBER SIGN ABOVE + | 0x13430..=0x1343F // EGYPTIAN HIEROGLYPH + | 0x1BCA0..=0x1BCA3 // SHORTHAND FORMAT + | 0x1D173..=0x1D17A // MUSICAL SYMBOL + | 0xE0001 // LANGUAGE TAG + | 0xE0020..=0xE007F // Cf + | 0xF0000..=0xFFFFD // Plane 15 Private Use + | 0x100000..=0x10FFFD // Plane 16 Private Use + ) +} + +/// Returns `true` if `c` is in Unicode general category `Cn` (Unassigned), or +/// strictly above U+10FFFF. The trailing `0x110000..=u32::MAX` arm is +/// unreachable for `char` input (a `char` is bounded to U+10FFFF) but is kept +/// for defensive coverage of the underlying `u32`. The disjoint Cc / Cf / Cs / +/// Co bucket is `is_unicode_general_category_other`. +#[allow(dead_code)] +pub(crate) fn is_unicode_general_category_unassigned(c: char) -> bool { + matches!( + c as u32, + 0x0378..=0x0379 + | 0x0380..=0x0383 + | 0x038B + | 0x038D + | 0x03A2 + | 0x0530 + | 0x0557..=0x0558 + | 0x058B..=0x058C + | 0x0590 + | 0x05C8..=0x05CF + | 0x05EB..=0x05EE + | 0x05F5..=0x05FF + | 0x070E + | 0x074B..=0x074C + | 0x07B2..=0x07BF + | 0x07FB..=0x07FC + | 0x082E..=0x082F + | 0x083F + | 0x085C..=0x085D + | 0x085F + | 0x086B..=0x086F + | 0x0892..=0x0896 + | 0x0984 + | 0x098D..=0x098E + | 0x0991..=0x0992 + | 0x09A9 + | 0x09B1 + | 0x09B3..=0x09B5 + | 0x09BA..=0x09BB + | 0x09C5..=0x09C6 + | 0x09C9..=0x09CA + | 0x09CF..=0x09D6 + | 0x09D8..=0x09DB + | 0x09DE + | 0x09E4..=0x09E5 + | 0x09FF..=0x0A00 + | 0x0A04 + | 0x0A0B..=0x0A0E + | 0x0A11..=0x0A12 + | 0x0A29 + | 0x0A31 + | 0x0A34 + | 0x0A37 + | 0x0A3A..=0x0A3B + | 0x0A3D + | 0x0A43..=0x0A46 + | 0x0A49..=0x0A4A + | 0x0A4E..=0x0A50 + | 0x0A52..=0x0A58 + | 0x0A5D + | 0x0A5F..=0x0A65 + | 0x0A77..=0x0A80 + | 0x0A84 + | 0x0A8E + | 0x0A92 + | 0x0AA9 + | 0x0AB1 + | 0x0AB4 + | 0x0ABA..=0x0ABB + | 0x0AC6 + | 0x0ACA + | 0x0ACE..=0x0ACF + | 0x0AD1..=0x0ADF + | 0x0AE4..=0x0AE5 + | 0x0AF2..=0x0AF8 + | 0x0B00 + | 0x0B04 + | 0x0B0D..=0x0B0E + | 0x0B11..=0x0B12 + | 0x0B29 + | 0x0B31 + | 0x0B34 + | 0x0B3A..=0x0B3B + | 0x0B45..=0x0B46 + | 0x0B49..=0x0B4A + | 0x0B4E..=0x0B54 + | 0x0B58..=0x0B5B + | 0x0B5E + | 0x0B64..=0x0B65 + | 0x0B78..=0x0B81 + | 0x0B84 + | 0x0B8B..=0x0B8D + | 0x0B91 + | 0x0B96..=0x0B98 + | 0x0B9B + | 0x0B9D + | 0x0BA0..=0x0BA2 + | 0x0BA5..=0x0BA7 + | 0x0BAB..=0x0BAD + | 0x0BBA..=0x0BBD + | 0x0BC3..=0x0BC5 + | 0x0BC9 + | 0x0BCE..=0x0BCF + | 0x0BD1..=0x0BD6 + | 0x0BD8..=0x0BE5 + | 0x0BFB..=0x0BFF + | 0x0C0D + | 0x0C11 + | 0x0C29 + | 0x0C3A..=0x0C3B + | 0x0C45 + | 0x0C49 + | 0x0C4E..=0x0C54 + | 0x0C57 + | 0x0C5B + | 0x0C5E..=0x0C5F + | 0x0C64..=0x0C65 + | 0x0C70..=0x0C76 + | 0x0C8D + | 0x0C91 + | 0x0CA9 + | 0x0CB4 + | 0x0CBA..=0x0CBB + | 0x0CC5 + | 0x0CC9 + | 0x0CCE..=0x0CD4 + | 0x0CD7..=0x0CDB + | 0x0CDF + | 0x0CE4..=0x0CE5 + | 0x0CF0 + | 0x0CF4..=0x0CFF + | 0x0D0D + | 0x0D11 + | 0x0D45 + | 0x0D49 + | 0x0D50..=0x0D53 + | 0x0D64..=0x0D65 + | 0x0D80 + | 0x0D84 + | 0x0D97..=0x0D99 + | 0x0DB2 + | 0x0DBC + | 0x0DBE..=0x0DBF + | 0x0DC7..=0x0DC9 + | 0x0DCB..=0x0DCE + | 0x0DD5 + | 0x0DD7 + | 0x0DE0..=0x0DE5 + | 0x0DF0..=0x0DF1 + | 0x0DF5..=0x0E00 + | 0x0E3B..=0x0E3E + | 0x0E5C..=0x0E80 + | 0x0E83 + | 0x0E85 + | 0x0E8B + | 0x0EA4 + | 0x0EA6 + | 0x0EBE..=0x0EBF + | 0x0EC5 + | 0x0EC7 + | 0x0ECF + | 0x0EDA..=0x0EDB + | 0x0EE0..=0x0EFF + | 0x0F48 + | 0x0F6D..=0x0F70 + | 0x0F98 + | 0x0FBD + | 0x0FCD + | 0x0FDB..=0x0FFF + | 0x10C6 + | 0x10C8..=0x10CC + | 0x10CE..=0x10CF + | 0x1249 + | 0x124E..=0x124F + | 0x1257 + | 0x1259 + | 0x125E..=0x125F + | 0x1289 + | 0x128E..=0x128F + | 0x12B1 + | 0x12B6..=0x12B7 + | 0x12BF + | 0x12C1 + | 0x12C6..=0x12C7 + | 0x12D7 + | 0x1311 + | 0x1316..=0x1317 + | 0x135B..=0x135C + | 0x137D..=0x137F + | 0x139A..=0x139F + | 0x13F6..=0x13F7 + | 0x13FE..=0x13FF + | 0x169D..=0x169F + | 0x16F9..=0x16FF + | 0x1716..=0x171E + | 0x1737..=0x173F + | 0x1754..=0x175F + | 0x176D + | 0x1771 + | 0x1774..=0x177F + | 0x17DE..=0x17DF + | 0x17EA..=0x17EF + | 0x17FA..=0x17FF + | 0x181A..=0x181F + | 0x1879..=0x187F + | 0x18AB..=0x18AF + | 0x18F6..=0x18FF + | 0x191F + | 0x192C..=0x192F + | 0x193C..=0x193F + | 0x1941..=0x1943 + | 0x196E..=0x196F + | 0x1975..=0x197F + | 0x19AC..=0x19AF + | 0x19CA..=0x19CF + | 0x19DB..=0x19DD + | 0x1A1C..=0x1A1D + | 0x1A5F + | 0x1A7D..=0x1A7E + | 0x1A8A..=0x1A8F + | 0x1A9A..=0x1A9F + | 0x1AAE..=0x1AAF + | 0x1ADE..=0x1ADF + | 0x1AEC..=0x1AFF + | 0x1B4D + | 0x1BF4..=0x1BFB + | 0x1C38..=0x1C3A + | 0x1C4A..=0x1C4C + | 0x1C8B..=0x1C8F + | 0x1CBB..=0x1CBC + | 0x1CC8..=0x1CCF + | 0x1CFB..=0x1CFF + | 0x1F16..=0x1F17 + | 0x1F1E..=0x1F1F + | 0x1F46..=0x1F47 + | 0x1F4E..=0x1F4F + | 0x1F58 + | 0x1F5A + | 0x1F5C + | 0x1F5E + | 0x1F7E..=0x1F7F + | 0x1FB5 + | 0x1FC5 + | 0x1FD4..=0x1FD5 + | 0x1FDC + | 0x1FF0..=0x1FF1 + | 0x1FF5 + | 0x1FFF + | 0x2065 + | 0x2072..=0x2073 + | 0x208F + | 0x209D..=0x209F + | 0x20C2..=0x20CF + | 0x20F1..=0x20FF + | 0x218C..=0x218F + | 0x242A..=0x243F + | 0x244B..=0x245F + | 0x2B74..=0x2B75 + | 0x2CF4..=0x2CF8 + | 0x2D26 + | 0x2D28..=0x2D2C + | 0x2D2E..=0x2D2F + | 0x2D68..=0x2D6E + | 0x2D71..=0x2D7E + | 0x2D97..=0x2D9F + | 0x2DA7 + | 0x2DAF + | 0x2DB7 + | 0x2DBF + | 0x2DC7 + | 0x2DCF + | 0x2DD7 + | 0x2DDF + | 0x2E5E..=0x2E7F + | 0x2E9A + | 0x2EF4..=0x2EFF + | 0x2FD6..=0x2FEF + | 0x3040 + | 0x3097..=0x3098 + | 0x3100..=0x3104 + | 0x3130 + | 0x318F + | 0x31E6..=0x31EE + | 0x321F + | 0xA48D..=0xA48F + | 0xA4C7..=0xA4CF + | 0xA62C..=0xA63F + | 0xA6F8..=0xA6FF + | 0xA7DD..=0xA7F0 + | 0xA82D..=0xA82F + | 0xA83A..=0xA83F + | 0xA878..=0xA87F + | 0xA8C6..=0xA8CD + | 0xA8DA..=0xA8DF + | 0xA954..=0xA95E + | 0xA97D..=0xA97F + | 0xA9CE + | 0xA9DA..=0xA9DD + | 0xA9FF + | 0xAA37..=0xAA3F + | 0xAA4E..=0xAA4F + | 0xAA5A..=0xAA5B + | 0xAAC3..=0xAADA + | 0xAAF7..=0xAB00 + | 0xAB07..=0xAB08 + | 0xAB0F..=0xAB10 + | 0xAB17..=0xAB1F + | 0xAB27 + | 0xAB2F + | 0xAB6C..=0xAB6F + | 0xABEE..=0xABEF + | 0xABFA..=0xABFF + | 0xD7A4..=0xD7AF + | 0xD7C7..=0xD7CA + | 0xD7FC..=0xD7FF + | 0xFA6E..=0xFA6F + | 0xFADA..=0xFAFF + | 0xFB07..=0xFB12 + | 0xFB18..=0xFB1C + | 0xFB37 + | 0xFB3D + | 0xFB3F + | 0xFB42 + | 0xFB45 + | 0xFDD0..=0xFDEF + | 0xFE1A..=0xFE1F + | 0xFE53 + | 0xFE67 + | 0xFE6C..=0xFE6F + | 0xFE75 + | 0xFEFD..=0xFEFE + | 0xFF00 + | 0xFFBF..=0xFFC1 + | 0xFFC8..=0xFFC9 + | 0xFFD0..=0xFFD1 + | 0xFFD8..=0xFFD9 + | 0xFFDD..=0xFFDF + | 0xFFE7 + | 0xFFEF..=0xFFF8 + | 0xFFFE..=0xFFFF + | 0x1000C + | 0x10027 + | 0x1003B + | 0x1003E + | 0x1004E..=0x1004F + | 0x1005E..=0x1007F + | 0x100FB..=0x100FF + | 0x10103..=0x10106 + | 0x10134..=0x10136 + | 0x1018F + | 0x1019D..=0x1019F + | 0x101A1..=0x101CF + | 0x101FE..=0x1027F + | 0x1029D..=0x1029F + | 0x102D1..=0x102DF + | 0x102FC..=0x102FF + | 0x10324..=0x1032C + | 0x1034B..=0x1034F + | 0x1037B..=0x1037F + | 0x1039E + | 0x103C4..=0x103C7 + | 0x103D6..=0x103FF + | 0x1049E..=0x1049F + | 0x104AA..=0x104AF + | 0x104D4..=0x104D7 + | 0x104FC..=0x104FF + | 0x10528..=0x1052F + | 0x10564..=0x1056E + | 0x1057B + | 0x1058B + | 0x10593 + | 0x10596 + | 0x105A2 + | 0x105B2 + | 0x105BA + | 0x105BD..=0x105BF + | 0x105F4..=0x105FF + | 0x10737..=0x1073F + | 0x10756..=0x1075F + | 0x10768..=0x1077F + | 0x10786 + | 0x107B1 + | 0x107BB..=0x107FF + | 0x10806..=0x10807 + | 0x10809 + | 0x10836 + | 0x10839..=0x1083B + | 0x1083D..=0x1083E + | 0x10856 + | 0x1089F..=0x108A6 + | 0x108B0..=0x108DF + | 0x108F3 + | 0x108F6..=0x108FA + | 0x1091C..=0x1091E + | 0x1093A..=0x1093E + | 0x1095A..=0x1097F + | 0x109B8..=0x109BB + | 0x109D0..=0x109D1 + | 0x10A04 + | 0x10A07..=0x10A0B + | 0x10A14 + | 0x10A18 + | 0x10A36..=0x10A37 + | 0x10A3B..=0x10A3E + | 0x10A49..=0x10A4F + | 0x10A59..=0x10A5F + | 0x10AA0..=0x10ABF + | 0x10AE7..=0x10AEA + | 0x10AF7..=0x10AFF + | 0x10B36..=0x10B38 + | 0x10B56..=0x10B57 + | 0x10B73..=0x10B77 + | 0x10B92..=0x10B98 + | 0x10B9D..=0x10BA8 + | 0x10BB0..=0x10BFF + | 0x10C49..=0x10C7F + | 0x10CB3..=0x10CBF + | 0x10CF3..=0x10CF9 + | 0x10D28..=0x10D2F + | 0x10D3A..=0x10D3F + | 0x10D66..=0x10D68 + | 0x10D86..=0x10D8D + | 0x10D90..=0x10E5F + | 0x10E7F + | 0x10EAA + | 0x10EAE..=0x10EAF + | 0x10EB2..=0x10EC1 + | 0x10EC8..=0x10ECF + | 0x10ED9..=0x10EF9 + | 0x10F28..=0x10F2F + | 0x10F5A..=0x10F6F + | 0x10F8A..=0x10FAF + | 0x10FCC..=0x10FDF + | 0x10FF7..=0x10FFF + | 0x1104E..=0x11051 + | 0x11076..=0x1107E + | 0x110C3..=0x110CC + | 0x110CE..=0x110CF + | 0x110E9..=0x110EF + | 0x110FA..=0x110FF + | 0x11135 + | 0x11148..=0x1114F + | 0x11177..=0x1117F + | 0x111E0 + | 0x111F5..=0x111FF + | 0x11212 + | 0x11242..=0x1127F + | 0x11287 + | 0x11289 + | 0x1128E + | 0x1129E + | 0x112AA..=0x112AF + | 0x112EB..=0x112EF + | 0x112FA..=0x112FF + | 0x11304 + | 0x1130D..=0x1130E + | 0x11311..=0x11312 + | 0x11329 + | 0x11331 + | 0x11334 + | 0x1133A + | 0x11345..=0x11346 + | 0x11349..=0x1134A + | 0x1134E..=0x1134F + | 0x11351..=0x11356 + | 0x11358..=0x1135C + | 0x11364..=0x11365 + | 0x1136D..=0x1136F + | 0x11375..=0x1137F + | 0x1138A + | 0x1138C..=0x1138D + | 0x1138F + | 0x113B6 + | 0x113C1 + | 0x113C3..=0x113C4 + | 0x113C6 + | 0x113CB + | 0x113D6 + | 0x113D9..=0x113E0 + | 0x113E3..=0x113FF + | 0x1145C + | 0x11462..=0x1147F + | 0x114C8..=0x114CF + | 0x114DA..=0x1157F + | 0x115B6..=0x115B7 + | 0x115DE..=0x115FF + | 0x11645..=0x1164F + | 0x1165A..=0x1165F + | 0x1166D..=0x1167F + | 0x116BA..=0x116BF + | 0x116CA..=0x116CF + | 0x116E4..=0x116FF + | 0x1171B..=0x1171C + | 0x1172C..=0x1172F + | 0x11747..=0x117FF + | 0x1183C..=0x1189F + | 0x118F3..=0x118FE + | 0x11907..=0x11908 + | 0x1190A..=0x1190B + | 0x11914 + | 0x11917 + | 0x11936 + | 0x11939..=0x1193A + | 0x11947..=0x1194F + | 0x1195A..=0x1199F + | 0x119A8..=0x119A9 + | 0x119D8..=0x119D9 + | 0x119E5..=0x119FF + | 0x11A48..=0x11A4F + | 0x11AA3..=0x11AAF + | 0x11AF9..=0x11AFF + | 0x11B0A..=0x11B5F + | 0x11B68..=0x11BBF + | 0x11BE2..=0x11BEF + | 0x11BFA..=0x11BFF + | 0x11C09 + | 0x11C37 + | 0x11C46..=0x11C4F + | 0x11C6D..=0x11C6F + | 0x11C90..=0x11C91 + | 0x11CA8 + | 0x11CB7..=0x11CFF + | 0x11D07 + | 0x11D0A + | 0x11D37..=0x11D39 + | 0x11D3B + | 0x11D3E + | 0x11D48..=0x11D4F + | 0x11D5A..=0x11D5F + | 0x11D66 + | 0x11D69 + | 0x11D8F + | 0x11D92 + | 0x11D99..=0x11D9F + | 0x11DAA..=0x11DAF + | 0x11DDC..=0x11DDF + | 0x11DEA..=0x11EDF + | 0x11EF9..=0x11EFF + | 0x11F11 + | 0x11F3B..=0x11F3D + | 0x11F5B..=0x11FAF + | 0x11FB1..=0x11FBF + | 0x11FF2..=0x11FFE + | 0x1239A..=0x123FF + | 0x1246F + | 0x12475..=0x1247F + | 0x12544..=0x12F8F + | 0x12FF3..=0x12FFF + | 0x13456..=0x1345F + | 0x143FB..=0x143FF + | 0x14647..=0x160FF + | 0x1613A..=0x167FF + | 0x16A39..=0x16A3F + | 0x16A5F + | 0x16A6A..=0x16A6D + | 0x16ABF + | 0x16ACA..=0x16ACF + | 0x16AEE..=0x16AEF + | 0x16AF6..=0x16AFF + | 0x16B46..=0x16B4F + | 0x16B5A + | 0x16B62 + | 0x16B78..=0x16B7C + | 0x16B90..=0x16D3F + | 0x16D7A..=0x16E3F + | 0x16E9B..=0x16E9F + | 0x16EB9..=0x16EBA + | 0x16ED4..=0x16EFF + | 0x16F4B..=0x16F4E + | 0x16F88..=0x16F8E + | 0x16FA0..=0x16FDF + | 0x16FE5..=0x16FEF + | 0x16FF7..=0x16FFF + | 0x18CD6..=0x18CFE + | 0x18D1F..=0x18D7F + | 0x18DF3..=0x1AFEF + | 0x1AFF4 + | 0x1AFFC + | 0x1AFFF + | 0x1B123..=0x1B131 + | 0x1B133..=0x1B14F + | 0x1B153..=0x1B154 + | 0x1B156..=0x1B163 + | 0x1B168..=0x1B16F + | 0x1B2FC..=0x1BBFF + | 0x1BC6B..=0x1BC6F + | 0x1BC7D..=0x1BC7F + | 0x1BC89..=0x1BC8F + | 0x1BC9A..=0x1BC9B + | 0x1BCA4..=0x1CBFF + | 0x1CCFD..=0x1CCFF + | 0x1CEB4..=0x1CEB9 + | 0x1CED1..=0x1CEDF + | 0x1CEF1..=0x1CEFF + | 0x1CF2E..=0x1CF2F + | 0x1CF47..=0x1CF4F + | 0x1CFC4..=0x1CFFF + | 0x1D0F6..=0x1D0FF + | 0x1D127..=0x1D128 + | 0x1D1EB..=0x1D1FF + | 0x1D246..=0x1D2BF + | 0x1D2D4..=0x1D2DF + | 0x1D2F4..=0x1D2FF + | 0x1D357..=0x1D35F + | 0x1D379..=0x1D3FF + | 0x1D455 + | 0x1D49D + | 0x1D4A0..=0x1D4A1 + | 0x1D4A3..=0x1D4A4 + | 0x1D4A7..=0x1D4A8 + | 0x1D4AD + | 0x1D4BA + | 0x1D4BC + | 0x1D4C4 + | 0x1D506 + | 0x1D50B..=0x1D50C + | 0x1D515 + | 0x1D51D + | 0x1D53A + | 0x1D53F + | 0x1D545 + | 0x1D547..=0x1D549 + | 0x1D551 + | 0x1D6A6..=0x1D6A7 + | 0x1D7CC..=0x1D7CD + | 0x1DA8C..=0x1DA9A + | 0x1DAA0 + | 0x1DAB0..=0x1DEFF + | 0x1DF1F..=0x1DF24 + | 0x1DF2B..=0x1DFFF + | 0x1E007 + | 0x1E019..=0x1E01A + | 0x1E022 + | 0x1E025 + | 0x1E02B..=0x1E02F + | 0x1E06E..=0x1E08E + | 0x1E090..=0x1E0FF + | 0x1E12D..=0x1E12F + | 0x1E13E..=0x1E13F + | 0x1E14A..=0x1E14D + | 0x1E150..=0x1E28F + | 0x1E2AF..=0x1E2BF + | 0x1E2FA..=0x1E2FE + | 0x1E300..=0x1E4CF + | 0x1E4FA..=0x1E5CF + | 0x1E5FB..=0x1E5FE + | 0x1E600..=0x1E6BF + | 0x1E6DF + | 0x1E6F6..=0x1E6FD + | 0x1E700..=0x1E7DF + | 0x1E7E7 + | 0x1E7EC + | 0x1E7EF + | 0x1E7FF + | 0x1E8C5..=0x1E8C6 + | 0x1E8D7..=0x1E8FF + | 0x1E94C..=0x1E94F + | 0x1E95A..=0x1E95D + | 0x1E960..=0x1EC70 + | 0x1ECB5..=0x1ED00 + | 0x1ED3E..=0x1EDFF + | 0x1EE04 + | 0x1EE20 + | 0x1EE23 + | 0x1EE25..=0x1EE26 + | 0x1EE28 + | 0x1EE33 + | 0x1EE38 + | 0x1EE3A + | 0x1EE3C..=0x1EE41 + | 0x1EE43..=0x1EE46 + | 0x1EE48 + | 0x1EE4A + | 0x1EE4C + | 0x1EE50 + | 0x1EE53 + | 0x1EE55..=0x1EE56 + | 0x1EE58 + | 0x1EE5A + | 0x1EE5C + | 0x1EE5E + | 0x1EE60 + | 0x1EE63 + | 0x1EE65..=0x1EE66 + | 0x1EE6B + | 0x1EE73 + | 0x1EE78 + | 0x1EE7D + | 0x1EE7F + | 0x1EE8A + | 0x1EE9C..=0x1EEA0 + | 0x1EEA4 + | 0x1EEAA + | 0x1EEBC..=0x1EEEF + | 0x1EEF2..=0x1EFFF + | 0x1F02C..=0x1F02F + | 0x1F094..=0x1F09F + | 0x1F0AF..=0x1F0B0 + | 0x1F0C0 + | 0x1F0D0 + | 0x1F0F6..=0x1F0FF + | 0x1F1AE..=0x1F1E5 + | 0x1F203..=0x1F20F + | 0x1F23C..=0x1F23F + | 0x1F249..=0x1F24F + | 0x1F252..=0x1F25F + | 0x1F266..=0x1F2FF + | 0x1F6D9..=0x1F6DB + | 0x1F6ED..=0x1F6EF + | 0x1F6FD..=0x1F6FF + | 0x1F7DA..=0x1F7DF + | 0x1F7EC..=0x1F7EF + | 0x1F7F1..=0x1F7FF + | 0x1F80C..=0x1F80F + | 0x1F848..=0x1F84F + | 0x1F85A..=0x1F85F + | 0x1F888..=0x1F88F + | 0x1F8AE..=0x1F8AF + | 0x1F8BC..=0x1F8BF + | 0x1F8C2..=0x1F8CF + | 0x1F8D9..=0x1F8FF + | 0x1FA58..=0x1FA5F + | 0x1FA6E..=0x1FA6F + | 0x1FA7D..=0x1FA7F + | 0x1FA8B..=0x1FA8D + | 0x1FAC7 + | 0x1FAC9..=0x1FACC + | 0x1FADD..=0x1FADE + | 0x1FAEB..=0x1FAEE + | 0x1FAF9..=0x1FAFF + | 0x1FB93 + | 0x1FBFB..=0x1FFFF + | 0x2A6E0..=0x2A6FF + | 0x2B81E..=0x2B81F + | 0x2CEAE..=0x2CEAF + | 0x2EBE1..=0x2EBEF + | 0x2EE5E..=0x2F7FF + | 0x2FA1E..=0x2FFFF + | 0x3134B..=0x3134F + | 0x3347A..=0xE0000 + | 0xE0002..=0xE001F + | 0xE0080..=0xE00FF + | 0xE01F0..=0xEFFFF + | 0xFFFFE..=0xFFFFF + | 0x10FFFE..=0x10FFFF + | 0x110000..=u32::MAX // above U+10FFFF — unreachable for `char` + ) +} From 188cca53968318a4fe0e3a0190fde17a0957acfe Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Tue, 5 May 2026 21:15:50 +0200 Subject: [PATCH 13/17] Roll back composite sub-handlers when one rejects `peer_connected` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `composite_custom_message_handler!` expanded `peer_connected` to call every sub-handler and remember the last error, but never undo the already-succeeded ones. The `CustomMessageHandler::peer_connected` contract is that `PeerManager` will *not* invoke `peer_disconnected` when `peer_connected` returns `Err` — so any per-peer state allocated by an earlier sub-handler that returned `Ok` was leaked permanently once a later sub-handler returned `Err`. A peer who can elicit `Err` from any sub-handler in the composite (feature-bit gate, banlist, etc.) could repeatedly reconnect to grow that leaked state without bound (slow resource DoS), and "currently connected" predicates in the leaking sub-handler would lie about peers that were actually rejected. Mirror the rollback pattern `PeerManager` already uses for the four built-in handlers (`peer_handler.rs:2149-2188`): record each sub-handler's `peer_connected` result, and if any returned `Err`, call `peer_disconnected` on the ones that succeeded before propagating the failure. Co-Authored-By: HAL 9000 Signed-off-by: Elias Rohrer Backport of 5455058ef2ec7994e4f19311477ecc662354dc52 Silent conflicts resolved in: * lightning-custom-message/src/lib.rs --- lightning-custom-message/src/lib.rs | 163 +++++++++++++++++++++++++++- 1 file changed, 158 insertions(+), 5 deletions(-) diff --git a/lightning-custom-message/src/lib.rs b/lightning-custom-message/src/lib.rs index fb0ba191cd3..73405109327 100644 --- a/lightning-custom-message/src/lib.rs +++ b/lightning-custom-message/src/lib.rs @@ -312,13 +312,25 @@ macro_rules! composite_custom_message_handler { } fn peer_connected(&self, their_node_id: $crate::bitcoin::secp256k1::PublicKey, msg: &$crate::lightning::ln::msgs::Init, inbound: bool) -> Result<(), ()> { - let mut result = Ok(()); + // Per the `CustomMessageHandler::peer_connected` contract, `peer_disconnected` + // will not be called by `PeerManager` if we return `Err`. To avoid leaking + // per-peer state in sub-handlers that already returned `Ok` when a later one + // errors, record each sub-handler's result and roll back the successful ones + // ourselves before propagating the failure. $( - if let Err(e) = self.$field.peer_connected(their_node_id, msg, inbound) { - result = Err(e); - } + let $field = self.$field.peer_connected(their_node_id, msg, inbound); )* - result + let any_err = false $( || $field.is_err() )*; + if any_err { + $( + if $field.is_ok() { + self.$field.peer_disconnected(their_node_id); + } + )* + Err(()) + } else { + Ok(()) + } } fn provided_node_features(&self) -> $crate::lightning::types::features::NodeFeatures { @@ -376,3 +388,144 @@ macro_rules! composite_custom_message_handler { } } } + +#[cfg(test)] +mod tests { + use bitcoin::io::Read; + use bitcoin::secp256k1::PublicKey; + use core::sync::atomic::{AtomicUsize, Ordering}; + use lightning::io; + use lightning::ln::msgs::{DecodeError, Init, LightningError}; + use lightning::ln::peer_handler::CustomMessageHandler; + use lightning::ln::wire::{CustomMessageReader, Type}; + use lightning::types::features::{InitFeatures, NodeFeatures}; + use lightning::util::ser::{Writeable, Writer}; + + #[derive(Debug)] + pub struct Foo; + impl Type for Foo { + fn type_id(&self) -> u16 { + 32768 + } + } + impl Writeable for Foo { + fn write(&self, _: &mut W) -> Result<(), io::Error> { + Ok(()) + } + } + + pub struct CountingHandler { + pub connect_count: AtomicUsize, + } + impl CustomMessageReader for CountingHandler { + type CustomMessage = Foo; + fn read( + &self, _t: u16, _b: &mut R, + ) -> Result, DecodeError> { + Ok(None) + } + } + impl CustomMessageHandler for CountingHandler { + fn handle_custom_message(&self, _msg: Foo, _: PublicKey) -> Result<(), LightningError> { + Ok(()) + } + fn get_and_clear_pending_msg(&self) -> Vec<(PublicKey, Foo)> { + vec![] + } + fn peer_disconnected(&self, _: PublicKey) { + self.connect_count.fetch_sub(1, Ordering::SeqCst); + } + fn peer_connected(&self, _: PublicKey, _: &Init, _: bool) -> Result<(), ()> { + self.connect_count.fetch_add(1, Ordering::SeqCst); + Ok(()) + } + fn provided_node_features(&self) -> NodeFeatures { + NodeFeatures::empty() + } + fn provided_init_features(&self, _: PublicKey) -> InitFeatures { + InitFeatures::empty() + } + } + + #[derive(Debug)] + pub struct Bar; + impl Type for Bar { + fn type_id(&self) -> u16 { + 32769 + } + } + impl Writeable for Bar { + fn write(&self, _: &mut W) -> Result<(), io::Error> { + Ok(()) + } + } + + pub struct ErroringHandler; + impl CustomMessageReader for ErroringHandler { + type CustomMessage = Bar; + fn read( + &self, _t: u16, _b: &mut R, + ) -> Result, DecodeError> { + Ok(None) + } + } + impl CustomMessageHandler for ErroringHandler { + fn handle_custom_message(&self, _msg: Bar, _: PublicKey) -> Result<(), LightningError> { + Ok(()) + } + fn get_and_clear_pending_msg(&self) -> Vec<(PublicKey, Bar)> { + vec![] + } + fn peer_disconnected(&self, _: PublicKey) { + debug_assert!(false); + } + fn peer_connected(&self, _: PublicKey, _: &Init, _: bool) -> Result<(), ()> { + Err(()) + } + fn provided_node_features(&self) -> NodeFeatures { + NodeFeatures::empty() + } + fn provided_init_features(&self, _: PublicKey) -> InitFeatures { + InitFeatures::empty() + } + } + + composite_custom_message_handler!( + pub struct CompositeHandler { + counting: CountingHandler, + erroring: ErroringHandler, + } + + pub enum CompositeMessage { + Foo(32768), + Bar(32769), + } + ); + + #[test] + fn peer_connected_failure_does_not_leak_subhandler_state() { + let composite = CompositeHandler { + counting: CountingHandler { connect_count: AtomicUsize::new(0) }, + erroring: ErroringHandler, + }; + let pk_bytes = [ + 0x02, 0x79, 0xBE, 0x66, 0x7E, 0xF9, 0xDC, 0xBB, 0xAC, 0x55, 0xA0, 0x62, 0x95, 0xCE, + 0x87, 0x0B, 0x07, 0x02, 0x9B, 0xFC, 0xDB, 0x2D, 0xCE, 0x28, 0xD9, 0x59, 0xF2, 0x81, + 0x5B, 0x16, 0xF8, 0x17, 0x98, + ]; + let pk = PublicKey::from_slice(&pk_bytes).unwrap(); + let init = + Init { features: InitFeatures::empty(), networks: None, remote_network_address: None }; + + let result = composite.peer_connected(pk, &init, true); + assert!(result.is_err(), "Composite must propagate the inner Err"); + + let leaked = composite.counting.connect_count.load(Ordering::SeqCst); + assert_eq!( + leaked, 0, + "CountingHandler tracked {leaked} connected peer(s) after the composite \ + returned Err; this state will never be cleaned up because per the trait \ + contract peer_disconnected won't be called when peer_connected returns Err.", + ); + } +} From f9f29217206f2c1990f950a7c1c30a8a6428a22a Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Tue, 5 May 2026 21:22:51 +0200 Subject: [PATCH 14/17] Validate Esplora merkle proof against the block header's merkle root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `EsploraSyncClient::get_confirmed_tx` parsed the SPV proof returned by the Esplora server but threw away the security check: the merkle root computed by `PartialMerkleTree::extract_matches` was discarded (`let _ = …`), and only the leaf-equality check (`matches[0] == txid`) remained. Anyone can construct a single-leaf partial tree advertising an arbitrary txid via `PartialMerkleTree::from_txids(&[txid], &[true])`, so this gate was vacuous. A malicious or compromised Esplora server could therefore convince `EsploraSyncClient` that any transaction was confirmed in any block by returning `MerkleBlock { header: real_header, txn: forged_partial_tree }`, causing LDK to feed a synthesized `ConfirmedTx` into `Confirm` implementations such as `ChannelManager` / `ChainMonitor`. From there, the channel-funding / closing / HTLC flows would treat the transaction as confirmed at an attacker-chosen height, with consequences ranging from premature state transitions to force-close races. Capture the merkle root returned by `extract_matches` and require it to equal `block_header.merkle_root`, matching the validation the Electrum sibling already performs via `validate_merkle_proof`. Co-Authored-By: HAL 9000 Backport of b64efcda8835c2b1aed3e8f20d186657b00b5ed9 --- lightning-transaction-sync/src/esplora.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lightning-transaction-sync/src/esplora.rs b/lightning-transaction-sync/src/esplora.rs index a191260bc01..e96110cc164 100644 --- a/lightning-transaction-sync/src/esplora.rs +++ b/lightning-transaction-sync/src/esplora.rs @@ -367,8 +367,13 @@ where let mut matches = Vec::new(); let mut indexes = Vec::new(); - let _ = merkle_block.txn.extract_matches(&mut matches, &mut indexes); - if indexes.len() != 1 || matches.len() != 1 || matches[0] != txid { + let computed_merkle_root = + merkle_block.txn.extract_matches(&mut matches, &mut indexes).ok(); + if computed_merkle_root != Some(block_header.merkle_root) + || indexes.len() != 1 + || matches.len() != 1 + || matches[0] != txid + { log_error!(self.logger, "Retrieved Merkle block for txid {} doesn't match expectations. This should not happen. Please verify server integrity.", txid); return Err(InternalError::Failed); } From 8b467cfce887e33379bbfdca03149008f591321b Mon Sep 17 00:00:00 2001 From: benthecarman Date: Wed, 10 Jun 2026 20:07:01 +0000 Subject: [PATCH 15/17] Skip stale fs store artifacts The exhaustive filesystem store listing treated leftover temp and trash files as namespace directories after identifying them as non-keys. Skip those artifacts before recursing so migrations can ignore crash leftovers. Backport of 9246d868cf64dd3ec960956597e64448a59537c6 Conflicts resolved due to moved/split file in: * lightning-persister/src/fs_store.rs --- lightning-persister/src/fs_store.rs | 60 +++++++++++++++++++++++------ 1 file changed, 48 insertions(+), 12 deletions(-) diff --git a/lightning-persister/src/fs_store.rs b/lightning-persister/src/fs_store.rs index 487e1a7ac14..de69c5744a6 100644 --- a/lightning-persister/src/fs_store.rs +++ b/lightning-persister/src/fs_store.rs @@ -331,19 +331,24 @@ impl KVStore for FilesystemStore { } } -fn dir_entry_is_key(p: &Path) -> Result { - if let Some(ext) = p.extension() { - #[cfg(target_os = "windows")] - { - // Clean up any trash files lying around. - if ext == "trash" { - fs::remove_file(p).ok(); - return Ok(false); +fn dir_entry_is_store_artifact(path: &Path) -> bool { + match path.extension().and_then(|ext| ext.to_str()) { + Some("tmp") => true, + Some("trash") => { + #[cfg(target_os = "windows")] + { + // Clean up any trash files lying around. + fs::remove_file(path).ok(); } - } - if ext == "tmp" { - return Ok(false); - } + true + }, + _ => false, + } +} + +fn dir_entry_is_key(p: &Path) -> Result { + if dir_entry_is_store_artifact(&p) { + return Ok(false); } let metadata = p.metadata().map_err(|e| { @@ -436,6 +441,9 @@ impl MigratableKVStore for FilesystemStore { 'primary_loop: for primary_entry in fs::read_dir(prefixed_dest)? { let primary_path = primary_entry?.path(); + if dir_entry_is_store_artifact(&primary_path) { + continue 'primary_loop; + } if dir_entry_is_key(&primary_path)? { let primary_namespace = String::new(); @@ -448,6 +456,9 @@ impl MigratableKVStore for FilesystemStore { // The primary_entry is actually also a directory. 'secondary_loop: for secondary_entry in fs::read_dir(&primary_path)? { let secondary_path = secondary_entry?.path(); + if dir_entry_is_store_artifact(&secondary_path) { + continue 'secondary_loop; + } if dir_entry_is_key(&secondary_path)? { let primary_namespace = get_key_from_dir_entry(&primary_path, prefixed_dest)?; @@ -461,6 +472,9 @@ impl MigratableKVStore for FilesystemStore { for tertiary_entry in fs::read_dir(&secondary_path)? { let tertiary_entry = tertiary_entry?; let tertiary_path = tertiary_entry.path(); + if dir_entry_is_store_artifact(&tertiary_path) { + continue; + } if dir_entry_is_key(&tertiary_path)? { let primary_namespace = @@ -529,6 +543,28 @@ mod tests { do_read_write_remove_list_persist(&fs_store); } + #[test] + fn list_all_keys_skips_leftover_store_artifacts() { + let mut temp_path = std::env::temp_dir(); + temp_path.push("test_list_all_keys_skips_leftover_store_artifacts"); + let fs_store = FilesystemStore::new(temp_path.clone()); + KVStore::write(&fs_store, "primary", "secondary", "key", &[1]).unwrap(); + + fs::write(temp_path.join("top_level.0.tmp"), b"stale").unwrap(); + fs::write(temp_path.join("top_level.0.trash"), b"stale").unwrap(); + + let primary_path = temp_path.join("primary"); + fs::write(primary_path.join("primary_level.0.tmp"), b"stale").unwrap(); + fs::write(primary_path.join("primary_level.0.trash"), b"stale").unwrap(); + + let secondary_path = primary_path.join("secondary"); + fs::write(secondary_path.join("secondary_level.0.tmp"), b"stale").unwrap(); + fs::write(secondary_path.join("secondary_level.0.trash"), b"stale").unwrap(); + + let keys = fs_store.list_all_keys().unwrap(); + assert_eq!(keys, vec![("primary".to_string(), "secondary".to_string(), "key".to_string())]); + } + #[test] fn test_data_migration() { let mut source_temp_path = std::env::temp_dir(); From ed0ba302fe52e18800e1fca7decc4b615abe7a13 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 29 May 2026 13:54:34 +0000 Subject: [PATCH 16/17] Stop using an introduction node in blinded message paths lnd is preparing to ship a release with opt-in onion messages without support for forwarding onion messages from non-channel peers. This breaks the common BOLT 12 OM flow today where we direct-connect to the blinded path introduction point and send the `invoice_request` without a channel. For CLN it turns out this is fine as they never select a peer for their introduction point at all. However, for LDK this would break existing nodes as nodes might now pick an lnd peer as an introduction node but it won't forward the onion message. For now, we just drop the separate introduction point selection and just always use ourselves as an introduction point (assuming we're an announced node). This should also have the side-effect of making offers marginally more robust, which may be worth it, even if it sucks to drop any pretense of privacy. Backport of 8c08a3065a1ff20f9b3b6f06e4c928e235e01f74 Conflicts resolved in: * lightning/src/ln/offers_tests.rs * lightning/src/onion_message/messenger.rs --- lightning/src/ln/offers_tests.rs | 152 ----------------------- lightning/src/onion_message/messenger.rs | 58 ++++----- 2 files changed, 26 insertions(+), 184 deletions(-) diff --git a/lightning/src/ln/offers_tests.rs b/lightning/src/ln/offers_tests.rs index 48171d4faeb..58ebd67316b 100644 --- a/lightning/src/ln/offers_tests.rs +++ b/lightning/src/ln/offers_tests.rs @@ -109,38 +109,6 @@ fn disconnect_peers<'a, 'b, 'c>(node_a: &Node<'a, 'b, 'c>, peers: &[&Node<'a, 'b } } -fn announce_node_address<'a, 'b, 'c>( - node: &Node<'a, 'b, 'c>, peers: &[&Node<'a, 'b, 'c>], address: SocketAddress, -) { - let features = node.onion_messenger.provided_node_features() - | node.gossip_sync.provided_node_features(); - let rgb = [0u8; 3]; - let announcement = UnsignedNodeAnnouncement { - features, - timestamp: 1000, - node_id: NodeId::from_pubkey(&node.keys_manager.get_node_id(Recipient::Node).unwrap()), - rgb, - alias: NodeAlias([0u8; 32]), - addresses: vec![address], - excess_address_data: Vec::new(), - excess_data: Vec::new(), - }; - let signature = node.keys_manager.sign_gossip_message( - UnsignedGossipMessage::NodeAnnouncement(&announcement) - ).unwrap(); - - let msg = NodeAnnouncement { - signature, - contents: announcement - }; - - let node_pubkey = node.node.get_our_node_id(); - node.gossip_sync.handle_node_announcement(None, &msg).unwrap(); - for peer in peers { - peer.gossip_sync.handle_node_announcement(Some(node_pubkey), &msg).unwrap(); - } -} - fn resolve_introduction_node<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, path: &BlindedMessagePath) -> PublicKey { path.public_introduction_node_id(&node.network_graph.read_only()) .and_then(|node_id| node_id.as_pubkey().ok()) @@ -254,126 +222,6 @@ fn extract_invoice_error<'a, 'b, 'c>( } } -/// Checks that blinded paths without Tor-only nodes are preferred when constructing an offer. -#[test] -fn prefers_non_tor_nodes_in_blinded_paths() { - let mut accept_forward_cfg = test_default_channel_config(); - accept_forward_cfg.accept_forwards_to_priv_channels = true; - - let mut features = channelmanager::provided_init_features(&accept_forward_cfg); - features.set_onion_messages_optional(); - features.set_route_blinding_optional(); - - let chanmon_cfgs = create_chanmon_cfgs(6); - let node_cfgs = create_node_cfgs(6, &chanmon_cfgs); - - *node_cfgs[1].override_init_features.borrow_mut() = Some(features); - - let node_chanmgrs = create_node_chanmgrs( - 6, &node_cfgs, &[None, Some(accept_forward_cfg), None, None, None, None] - ); - let nodes = create_network(6, &node_cfgs, &node_chanmgrs); - - create_unannounced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000_000, 1_000_000_000); - create_unannounced_chan_between_nodes_with_value(&nodes, 2, 3, 10_000_000, 1_000_000_000); - create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 10_000_000, 1_000_000_000); - create_announced_chan_between_nodes_with_value(&nodes, 1, 4, 10_000_000, 1_000_000_000); - create_announced_chan_between_nodes_with_value(&nodes, 1, 5, 10_000_000, 1_000_000_000); - create_announced_chan_between_nodes_with_value(&nodes, 2, 4, 10_000_000, 1_000_000_000); - create_announced_chan_between_nodes_with_value(&nodes, 2, 5, 10_000_000, 1_000_000_000); - - // Add an extra channel so that more than one of Bob's peers have MIN_PEER_CHANNELS. - create_announced_chan_between_nodes_with_value(&nodes, 4, 5, 10_000_000, 1_000_000_000); - - let (alice, bob, charlie, david) = (&nodes[0], &nodes[1], &nodes[2], &nodes[3]); - let bob_id = bob.node.get_our_node_id(); - let charlie_id = charlie.node.get_our_node_id(); - - disconnect_peers(alice, &[charlie, david, &nodes[4], &nodes[5]]); - disconnect_peers(david, &[bob, &nodes[4], &nodes[5]]); - - let tor = SocketAddress::OnionV2([255, 254, 253, 252, 251, 250, 249, 248, 247, 246, 38, 7]); - announce_node_address(charlie, &[alice, bob, david, &nodes[4], &nodes[5]], tor.clone()); - - let offer = bob.node - .create_offer_builder(None).unwrap() - .amount_msats(10_000_000) - .build().unwrap(); - assert_ne!(offer.issuer_signing_pubkey(), Some(bob_id)); - assert!(!offer.paths().is_empty()); - for path in offer.paths() { - let introduction_node_id = resolve_introduction_node(david, &path); - assert_ne!(introduction_node_id, bob_id); - assert_ne!(introduction_node_id, charlie_id); - } - - // Use a one-hop blinded path when Bob is announced and all his peers are Tor-only. - announce_node_address(&nodes[4], &[alice, bob, charlie, david, &nodes[5]], tor.clone()); - announce_node_address(&nodes[5], &[alice, bob, charlie, david, &nodes[4]], tor.clone()); - - let offer = bob.node - .create_offer_builder(None).unwrap() - .amount_msats(10_000_000) - .build().unwrap(); - assert_ne!(offer.issuer_signing_pubkey(), Some(bob_id)); - assert!(!offer.paths().is_empty()); - for path in offer.paths() { - let introduction_node_id = resolve_introduction_node(david, &path); - assert_eq!(introduction_node_id, bob_id); - } -} - -/// Checks that blinded paths prefer an introduction node that is the most connected. -#[test] -fn prefers_more_connected_nodes_in_blinded_paths() { - let mut accept_forward_cfg = test_default_channel_config(); - accept_forward_cfg.accept_forwards_to_priv_channels = true; - - let mut features = channelmanager::provided_init_features(&accept_forward_cfg); - features.set_onion_messages_optional(); - features.set_route_blinding_optional(); - - let chanmon_cfgs = create_chanmon_cfgs(6); - let node_cfgs = create_node_cfgs(6, &chanmon_cfgs); - - *node_cfgs[1].override_init_features.borrow_mut() = Some(features); - - let node_chanmgrs = create_node_chanmgrs( - 6, &node_cfgs, &[None, Some(accept_forward_cfg), None, None, None, None] - ); - let nodes = create_network(6, &node_cfgs, &node_chanmgrs); - - create_unannounced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000_000, 1_000_000_000); - create_unannounced_chan_between_nodes_with_value(&nodes, 2, 3, 10_000_000, 1_000_000_000); - create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 10_000_000, 1_000_000_000); - create_announced_chan_between_nodes_with_value(&nodes, 1, 4, 10_000_000, 1_000_000_000); - create_announced_chan_between_nodes_with_value(&nodes, 1, 5, 10_000_000, 1_000_000_000); - create_announced_chan_between_nodes_with_value(&nodes, 2, 4, 10_000_000, 1_000_000_000); - create_announced_chan_between_nodes_with_value(&nodes, 2, 5, 10_000_000, 1_000_000_000); - - // Add extra channels so that more than one of Bob's peers have MIN_PEER_CHANNELS and one has - // more than the others. - create_announced_chan_between_nodes_with_value(&nodes, 0, 4, 10_000_000, 1_000_000_000); - create_announced_chan_between_nodes_with_value(&nodes, 3, 4, 10_000_000, 1_000_000_000); - - let (alice, bob, charlie, david) = (&nodes[0], &nodes[1], &nodes[2], &nodes[3]); - let bob_id = bob.node.get_our_node_id(); - - disconnect_peers(alice, &[charlie, david, &nodes[4], &nodes[5]]); - disconnect_peers(david, &[bob, &nodes[4], &nodes[5]]); - - let offer = bob.node - .create_offer_builder(None).unwrap() - .amount_msats(10_000_000) - .build().unwrap(); - assert_ne!(offer.issuer_signing_pubkey(), Some(bob_id)); - assert!(!offer.paths().is_empty()); - for path in offer.paths() { - let introduction_node_id = resolve_introduction_node(david, &path); - assert_eq!(introduction_node_id, nodes[4].node.get_our_node_id()); - } -} - /// Checks that blinded paths are compact for short-lived offers. #[test] fn creates_short_lived_offer() { diff --git a/lightning/src/onion_message/messenger.rs b/lightning/src/onion_message/messenger.rs index c326cfca804..e1b7def8811 100644 --- a/lightning/src/onion_message/messenger.rs +++ b/lightning/src/onion_message/messenger.rs @@ -525,44 +525,38 @@ where // Limit the number of blinded paths that are computed. const MAX_PATHS: usize = 3; - // Ensure peers have at least three channels so that it is more difficult to infer the - // recipient's node_id. - const MIN_PEER_CHANNELS: usize = 3; - let network_graph = network_graph.deref().read_only(); let is_recipient_announced = network_graph.nodes().contains_key(&NodeId::from_pubkey(&recipient)); let has_one_peer = peers.len() == 1; - let mut peer_info = peers - // Limit to peers with announced channels unless the recipient is unannounced. - .filter_map(|peer| - network_graph - .node(&NodeId::from_pubkey(&peer.node_id)) - .filter(|info| - !is_recipient_announced || info.channels.len() >= MIN_PEER_CHANNELS - ) - .map(|info| (peer, info.is_tor_only(), info.channels.len())) - // Allow messages directly with the only peer when unannounced. - .or_else(|| (!is_recipient_announced && has_one_peer) - .then(|| (peer, false, 0)) - ) - ) - // Exclude Tor-only nodes when the recipient is announced. - .filter(|(_, is_tor_only, _)| !(*is_tor_only && is_recipient_announced)) - .collect::>(); - - // Prefer using non-Tor nodes with the most channels as the introduction node. - peer_info.sort_unstable_by(|(_, a_tor_only, a_channels), (_, b_tor_only, b_channels)| { - a_tor_only.cmp(b_tor_only).then(a_channels.cmp(b_channels).reverse()) - }); + let paths = if !is_recipient_announced { + let mut peer_info = peers + // Limit to peers with announced channels unless the recipient is unannounced. + .filter_map(|peer| + network_graph + .node(&NodeId::from_pubkey(&peer.node_id)) + .map(|info| (peer, info.is_tor_only(), info.channels.len())) + // Allow messages directly with the only peer + .or_else(|| has_one_peer.then(|| (peer, false, 0))) + ) + .collect::>(); + + // Prefer using non-Tor nodes with the most channels as the introduction node. + peer_info.sort_unstable_by(|(_, a_tor_only, a_channels), (_, b_tor_only, b_channels)| { + a_tor_only.cmp(b_tor_only).then(a_channels.cmp(b_channels).reverse()) + }); - let paths = peer_info.into_iter() - .map(|(peer, _, _)| { - BlindedMessagePath::new(&[peer], recipient, context.clone(), &**entropy_source, secp_ctx) - }) - .take(MAX_PATHS) - .collect::, _>>(); + peer_info + .into_iter() + .map(|(peer, _, _)| { + BlindedMessagePath::new(&[peer], recipient, context.clone(), &**entropy_source, secp_ctx) + }) + .take(MAX_PATHS) + .collect::, _>>() + } else { + Ok(vec![]) + }; let mut paths = match paths { Ok(paths) if !paths.is_empty() => Ok(paths), From e60acdefbb38560743c4a240b5de14ecb76d852c Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Mon, 1 Jun 2026 14:38:58 -0400 Subject: [PATCH 17/17] Set PaymentSent::fee_paid_msat in abandoned case If an outbound payment was abandoned with htlcs in-flight and later claimed, we would previously have the PaymentSent::fee_paid_msat be set to None. This contradicted some docs on the event that stated the field would always be Some after 0.0.103. Backport of 3e9e6e9324e8e8916d7f921485982789a860f61a Conflicts resolved in: * lightning/src/ln/functional_tests.rs * lightning/src/ln/outbound_payment.rs * lightning/src/ln/payment_tests.rs --- lightning/src/events/mod.rs | 3 ++- lightning/src/ln/functional_tests.rs | 2 +- lightning/src/ln/outbound_payment.rs | 7 +++++++ lightning/src/ln/payment_tests.rs | 30 ++++++++++++++++++++++++++++ 4 files changed, 40 insertions(+), 2 deletions(-) diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs index 4b492b0607a..4db61271c36 100644 --- a/lightning/src/events/mod.rs +++ b/lightning/src/events/mod.rs @@ -930,7 +930,8 @@ pub enum Event { /// If the recipient or an intermediate node misbehaves and gives us free money, this may /// overstate the amount paid, though this is unlikely. /// - /// This is only `None` for payments initiated on LDK versions prior to 0.0.103. + /// This is only `None` for payments abandoned but ultimately claimed when using LDK versions + /// prior to 0.3, 0.2.3, or 0.1.10. /// /// [`Route::get_total_fees`]: crate::routing::router::Route::get_total_fees fee_paid_msat: Option, diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 9fbcfecd41b..e5f7d918117 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -10281,7 +10281,7 @@ fn test_inconsistent_mpp_params() { do_claim_payment_along_route( ClaimAlongRouteArgs::new(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], our_payment_preimage) ); - expect_payment_sent(&nodes[0], our_payment_preimage, Some(None), true, true); + expect_payment_sent(&nodes[0], our_payment_preimage, Some(Some(2000)), true, true); } #[test] diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index b3a8bd9ffee..06ebee98575 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -133,6 +133,9 @@ pub(crate) enum PendingOutboundPayment { /// Will be `None` if the payment was serialized before 0.0.115 or if downgrading to 0.0.124 /// or later with a reason that was added after. reason: Option, + /// Preserved from `Retryable` so we can still report `fee_paid_msat` if an HTLC succeeds after + /// the payment was abandoned. Added in 0.3/0.1.10. + pending_fee_msat: Option, }, } @@ -209,6 +212,7 @@ impl PendingOutboundPayment { fn get_pending_fee_msat(&self) -> Option { match self { PendingOutboundPayment::Retryable { pending_fee_msat, .. } => pending_fee_msat.clone(), + PendingOutboundPayment::Abandoned { pending_fee_msat, .. } => pending_fee_msat.clone(), _ => None, } } @@ -251,6 +255,7 @@ impl PendingOutboundPayment { }, _ => new_hash_set(), }; + let pending_fee_msat = self.get_pending_fee_msat(); match self { Self::Retryable { payment_hash, .. } | Self::InvoiceReceived { payment_hash, .. } | @@ -260,6 +265,7 @@ impl PendingOutboundPayment { session_privs, payment_hash: *payment_hash, reason: Some(reason), + pending_fee_msat, }; }, _ => {} @@ -2409,6 +2415,7 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment, (0, session_privs, required), (1, reason, upgradable_option), (2, payment_hash, required), + (5, pending_fee_msat, option), }, (5, AwaitingInvoice) => { (0, expiration, required), diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index ea3070e6bd2..0873418c86d 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -1751,6 +1751,36 @@ fn abandoned_send_payment_idempotent() { claim_payment(&nodes[0], &[&nodes[1]], second_payment_preimage); } +#[test] +fn abandoned_payment_fulfilled_preserves_fee_paid_msat() { + // Previously, if we abandoned a payment with HTLCs in-flight and the payment eventually + // succeeded, we would set the `Event::PaymentSent::fee_paid_msat` to None, even though we had + // docs guaranteeing that it would always be Some after 0.0.103. + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]); + let nodes = create_network(3, &node_cfgs, &node_chanmgrs); + + create_announced_chan_between_nodes(&nodes, 0, 1); + create_announced_chan_between_nodes(&nodes, 1, 2); + + let amt_msat = 5_000_000; + let (route, payment_hash, payment_preimage, payment_secret) = + get_route_and_payment_hash!(&nodes[0], nodes[2], amt_msat); + let payment_id = PaymentId(payment_hash.0); + let onion = RecipientOnionFields::secret_only(payment_secret); + nodes[0].node.send_payment_with_route(route, payment_hash, onion, payment_id).unwrap(); + check_added_monitors(&nodes[0], 1); + + let path: &[&Node] = &[&nodes[1], &nodes[2]]; + pass_along_route(&nodes[0], &[path], amt_msat, payment_hash, payment_secret); + + nodes[0].node.abandon_payment(payment_id); + assert!(nodes[0].node.get_and_clear_pending_events().is_empty()); + + claim_payment_along_route(ClaimAlongRouteArgs::new(&nodes[0], &[path], payment_preimage)); +} + #[derive(PartialEq)] enum InterceptTest { Forward,