diff --git a/lightning-persister/src/test_utils.rs b/lightning-persister/src/test_utils.rs index 5dd28f98b9a..735870e49f7 100644 --- a/lightning-persister/src/test_utils.rs +++ b/lightning-persister/src/test_utils.rs @@ -1,4 +1,3 @@ -use lightning::chain::channelmonitor::CLOSED_CHANNEL_UPDATE_ID; use lightning::events::ClosureReason; use lightning::ln::functional_test_utils::{ connect_block, create_announced_chan_between_nodes, create_chanmon_cfgs, create_dummy_block, @@ -168,5 +167,5 @@ pub(crate) fn do_test_store(store_0: &K, store_1: &K) { check_added_monitors!(nodes[1], 1); // Make sure everything is persisted as expected after close. - check_persisted_data!(CLOSED_CHANNEL_UPDATE_ID); + check_persisted_data!(11); } diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 2d76c09f1bb..01698aab6f8 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -89,11 +89,9 @@ pub struct ChannelMonitorUpdate { /// [`ChannelMonitorUpdateStatus::InProgress`] have been applied to all copies of a given /// ChannelMonitor when ChannelManager::channel_monitor_updated is called. /// - /// The only instances we allow where update_id values are not strictly increasing have a - /// special update ID of [`CLOSED_CHANNEL_UPDATE_ID`]. This update ID is used for updates that - /// will force close the channel by broadcasting the latest commitment transaction or - /// special post-force-close updates, like providing preimages necessary to claim outputs on the - /// broadcast commitment transaction. See its docs for more details. + /// Note that for [`ChannelMonitorUpdate`]s generated on LDK versions prior to 0.1 after the + /// channel was closed, this value may be [`u64::MAX`]. In that case, multiple updates may + /// appear with the same ID, and all should be replayed. /// /// [`ChannelMonitorUpdateStatus::InProgress`]: super::ChannelMonitorUpdateStatus::InProgress pub update_id: u64, @@ -104,15 +102,9 @@ pub struct ChannelMonitorUpdate { pub channel_id: Option, } -/// The update ID used for a [`ChannelMonitorUpdate`] that is either: -/// -/// (1) attempting to force close the channel by broadcasting our latest commitment transaction or -/// (2) providing a preimage (after the channel has been force closed) from a forward link that -/// allows us to spend an HTLC output on this channel's (the backward link's) broadcasted -/// commitment transaction. -/// -/// No other [`ChannelMonitorUpdate`]s are allowed after force-close. -pub const CLOSED_CHANNEL_UPDATE_ID: u64 = core::u64::MAX; +/// LDK prior to 0.1 used this constant as the [`ChannelMonitorUpdate::update_id`] for any +/// [`ChannelMonitorUpdate`]s which were generated after the channel was closed. +const LEGACY_CLOSED_CHANNEL_UPDATE_ID: u64 = core::u64::MAX; impl Writeable for ChannelMonitorUpdate { fn write(&self, w: &mut W) -> Result<(), io::Error> { @@ -1553,6 +1545,8 @@ impl ChannelMonitor { /// Gets the update_id from the latest ChannelMonitorUpdate which was applied to this /// ChannelMonitor. + /// + /// Note that for channels closed prior to LDK 0.1, this may return [`u64::MAX`]. pub fn get_latest_update_id(&self) -> u64 { self.inner.lock().unwrap().get_latest_update_id() } @@ -1717,6 +1711,12 @@ impl ChannelMonitor { self.inner.lock().unwrap().get_cur_holder_commitment_number() } + /// Gets whether we've been notified that this channel is closed by the `ChannelManager` (i.e. + /// via a [`ChannelMonitorUpdateStep::ChannelForceClosed`]). + pub(crate) fn offchain_closed(&self) -> bool { + self.inner.lock().unwrap().lockdown_from_offchain + } + /// Gets the `node_id` of the counterparty for this channel. /// /// Will be `None` for channels constructed on LDK versions prior to 0.0.110 and always `Some` @@ -3116,11 +3116,11 @@ impl ChannelMonitorImpl { F::Target: FeeEstimator, L::Target: Logger, { - if self.latest_update_id == CLOSED_CHANNEL_UPDATE_ID && updates.update_id == CLOSED_CHANNEL_UPDATE_ID { - log_info!(logger, "Applying post-force-closed update to monitor {} with {} change(s).", + if self.latest_update_id == LEGACY_CLOSED_CHANNEL_UPDATE_ID && updates.update_id == LEGACY_CLOSED_CHANNEL_UPDATE_ID { + log_info!(logger, "Applying pre-0.1 post-force-closed update to monitor {} with {} change(s).", log_funding_info!(self), updates.updates.len()); - } else if updates.update_id == CLOSED_CHANNEL_UPDATE_ID { - log_info!(logger, "Applying force close update to monitor {} with {} change(s).", + } else if updates.update_id == LEGACY_CLOSED_CHANNEL_UPDATE_ID { + log_info!(logger, "Applying pre-0.1 force close update to monitor {} with {} change(s).", log_funding_info!(self), updates.updates.len()); } else { log_info!(logger, "Applying update to monitor {}, bringing update_id from {} to {} with {} change(s).", @@ -3143,14 +3143,14 @@ impl ChannelMonitorImpl { // The `ChannelManager` may also queue redundant `ChannelForceClosed` updates if it still // thinks the channel needs to have its commitment transaction broadcast, so we'll allow // them as well. - if updates.update_id == CLOSED_CHANNEL_UPDATE_ID { + if updates.update_id == LEGACY_CLOSED_CHANNEL_UPDATE_ID || self.lockdown_from_offchain { assert_eq!(updates.updates.len(), 1); match updates.updates[0] { ChannelMonitorUpdateStep::ChannelForceClosed { .. } => {}, // We should have already seen a `ChannelForceClosed` update if we're trying to // provide a preimage at this point. ChannelMonitorUpdateStep::PaymentPreimage { .. } => - debug_assert_eq!(self.latest_update_id, CLOSED_CHANNEL_UPDATE_ID), + debug_assert!(self.lockdown_from_offchain), _ => { log_error!(logger, "Attempted to apply post-force-close ChannelMonitorUpdate of type {}", updates.updates[0].variant_name()); panic!("Attempted to apply post-force-close ChannelMonitorUpdate that wasn't providing a payment preimage"); @@ -3230,17 +3230,29 @@ impl ChannelMonitorImpl { self.counterparty_commitment_txs_from_update(updates); } - // If the updates succeeded and we were in an already closed channel state, then there's no - // need to refuse any updates we expect to receive afer seeing a confirmed commitment. - if ret.is_ok() && updates.update_id == CLOSED_CHANNEL_UPDATE_ID && self.latest_update_id == updates.update_id { - return Ok(()); - } - self.latest_update_id = updates.update_id; - // Refuse updates after we've detected a spend onchain, but only if we haven't processed a - // force closed monitor update yet. - if ret.is_ok() && self.funding_spend_seen && self.latest_update_id != CLOSED_CHANNEL_UPDATE_ID { + // Refuse updates after we've detected a spend onchain (or if the channel was otherwise + // closed), but only if the update isn't the kind of update we expect to see after channel + // closure. + let mut is_pre_close_update = false; + for update in updates.updates.iter() { + match update { + ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo { .. } + |ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { .. } + |ChannelMonitorUpdateStep::ShutdownScript { .. } + |ChannelMonitorUpdateStep::CommitmentSecret { .. } => + is_pre_close_update = true, + // After a channel is closed, we don't communicate with our peer about it, so the + // only things we will update is getting a new preimage (from a different channel) + // or being told that the channel is closed. All other updates are generated while + // talking to our peer. + ChannelMonitorUpdateStep::PaymentPreimage { .. } => {}, + ChannelMonitorUpdateStep::ChannelForceClosed { .. } => {}, + } + } + + if ret.is_ok() && (self.funding_spend_seen || self.lockdown_from_offchain) && is_pre_close_update { log_error!(logger, "Refusing Channel Monitor Update as counterparty attempted to update commitment after funding was spent"); Err(()) } else { ret } diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index cd9d5bf8b2c..80aa0b8479c 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -45,7 +45,7 @@ use crate::ln::chan_utils; use crate::ln::onion_utils::HTLCFailReason; use crate::chain::BestBlock; use crate::chain::chaininterface::{FeeEstimator, ConfirmationTarget, LowerBoundedFeeEstimator}; -use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, LATENCY_GRACE_PERIOD_BLOCKS, CLOSED_CHANNEL_UPDATE_ID}; +use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, LATENCY_GRACE_PERIOD_BLOCKS}; use crate::chain::transaction::{OutPoint, TransactionData}; use crate::sign::ecdsa::EcdsaChannelSigner; use crate::sign::{EntropySource, ChannelSigner, SignerProvider, NodeSigner, Recipient}; @@ -3656,7 +3656,7 @@ impl ChannelContext where SP::Target: SignerProvider { // monitor update to the user, even if we return one). // See test_duplicate_chan_id and test_pre_lockin_no_chan_closed_update for more. if !self.channel_state.is_pre_funded_state() { - self.latest_monitor_update_id = CLOSED_CHANNEL_UPDATE_ID; + self.latest_monitor_update_id += 1; Some((self.get_counterparty_node_id(), funding_txo, self.channel_id(), ChannelMonitorUpdate { update_id: self.latest_monitor_update_id, counterparty_node_id: Some(self.counterparty_node_id), diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index f31c23b5a67..7cb484d4fac 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -40,7 +40,7 @@ use crate::blinded_path::payment::{BlindedPaymentPath, Bolt12OfferContext, Bolt1 use crate::chain; use crate::chain::{Confirm, ChannelMonitorUpdateStatus, Watch, BestBlock}; use crate::chain::chaininterface::{BroadcasterInterface, ConfirmationTarget, FeeEstimator, LowerBoundedFeeEstimator}; -use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, WithChannelMonitor, ChannelMonitorUpdateStep, HTLC_FAIL_BACK_BUFFER, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY, MonitorEvent, CLOSED_CHANNEL_UPDATE_ID}; +use crate::chain::channelmonitor::{Balance, ChannelMonitor, ChannelMonitorUpdate, WithChannelMonitor, ChannelMonitorUpdateStep, HTLC_FAIL_BACK_BUFFER, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY, MonitorEvent}; use crate::chain::transaction::{OutPoint, TransactionData}; use crate::events; use crate::events::{Event, EventHandler, EventsProvider, MessageSendEvent, MessageSendEventsProvider, ClosureReason, HTLCDestination, PaymentFailureReason, ReplayEvent}; @@ -49,7 +49,7 @@ use crate::events::{Event, EventHandler, EventsProvider, MessageSendEvent, Messa use crate::ln::inbound_payment; use crate::ln::types::ChannelId; use crate::types::payment::{PaymentHash, PaymentPreimage, PaymentSecret}; -use crate::ln::channel::{self, Channel, ChannelPhase, ChannelContext, ChannelError, ChannelUpdateStatus, ShutdownResult, UnfundedChannelContext, UpdateFulfillCommitFetch, OutboundV1Channel, InboundV1Channel, WithChannelContext}; +use crate::ln::channel::{self, Channel, ChannelPhase, ChannelError, ChannelUpdateStatus, ShutdownResult, UpdateFulfillCommitFetch, OutboundV1Channel, InboundV1Channel, WithChannelContext}; use crate::ln::channel_state::ChannelDetails; use crate::types::features::{Bolt12InvoiceFeatures, ChannelFeatures, ChannelTypeFeatures, InitFeatures, NodeFeatures}; #[cfg(any(feature = "_test_utils", test))] @@ -973,9 +973,9 @@ impl ClaimablePayments { #[derive(Debug)] enum BackgroundEvent { /// Handle a ChannelMonitorUpdate which closes the channel or for an already-closed channel. - /// This is only separated from [`Self::MonitorUpdateRegeneratedOnStartup`] as the - /// maybe-non-closing variant needs a public key to handle channel resumption, whereas if the - /// channel has been force-closed we do not need the counterparty node_id. + /// This is only separated from [`Self::MonitorUpdateRegeneratedOnStartup`] as for truly + /// ancient [`ChannelMonitor`]s that haven't seen an update since LDK 0.0.118 we may not have + /// the counterparty node ID available. /// /// Note that any such events are lost on shutdown, so in general they must be updates which /// are regenerated on startup. @@ -1299,6 +1299,13 @@ pub(super) struct PeerState where SP::Target: SignerProvider { /// will remove a preimage that needs to be durably in an upstream channel first), we put an /// entry here to note that the channel with the key's ID is blocked on a set of actions. actions_blocking_raa_monitor_updates: BTreeMap>, + /// The latest [`ChannelMonitor::get_latest_update_id`] value for all closed channels as they + /// exist on-disk/in our [`chain::Watch`]. This *ignores* all pending updates not yet applied + /// in [`ChannelManager::pending_background_events`]. + /// + /// If there are any updates pending in [`Self::in_flight_monitor_updates`] this will contain + /// the highest `update_id` of all the pending in-flight updates. + closed_channel_monitor_update_ids: BTreeMap, /// The peer is currently connected (i.e. we've seen a /// [`ChannelMessageHandler::peer_connected`] and no corresponding /// [`ChannelMessageHandler::peer_disconnected`]. @@ -1325,6 +1332,7 @@ impl PeerState where SP::Target: SignerProvider { ) && self.monitor_update_blocked_actions.is_empty() && self.in_flight_monitor_updates.is_empty() + && self.closed_channel_monitor_update_ids.is_empty() } // Returns a count of all channels we have with this peer, including unfunded channels. @@ -2888,8 +2896,41 @@ macro_rules! handle_error { } }; } +/// When a channel is removed, two things need to happen: +/// (a) This must be called in the same `per_peer_state` lock as the channel-closing action, +/// (b) [`ChannelManager::finish_close_channel`] needs to be called without holding any locks +/// (except [`ChannelManager::total_consistency_lock`]. +/// +/// Note that this step can be skipped if the channel was never opened (through the creation of a +/// [`ChannelMonitor`]/channel funding transaction) to begin with. macro_rules! update_maps_on_chan_removal { - ($self: expr, $channel_context: expr) => {{ + ($self: expr, $peer_state: expr, $channel_context: expr) => {{ + // If there's a possibility that we need to generate further monitor updates for this + // channel, we need to store the last update_id of it. However, we don't want to insert + // into the map (which prevents the `PeerState` from being cleaned up) for channels that + // never even got confirmations (which would open us up to DoS attacks). + let mut update_id = $channel_context.get_latest_monitor_update_id(); + if $channel_context.get_funding_tx_confirmation_height().is_some() || $channel_context.minimum_depth() == Some(0) || update_id > 1 { + // There may be some pending background events which we have to ignore when setting the + // latest update ID. + for event in $self.pending_background_events.lock().unwrap().iter() { + match event { + BackgroundEvent::MonitorUpdateRegeneratedOnStartup { counterparty_node_id, channel_id, update, .. } => { + if *channel_id == $channel_context.channel_id() && *counterparty_node_id == $channel_context.get_counterparty_node_id() { + update_id = cmp::min(update_id, update.update_id - 1); + } + }, + BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup(..) => { + // This is only generated for very old channels which were already closed + // on startup, so it should never be present for a channel that is closing + // here. + }, + BackgroundEvent::MonitorUpdatesComplete { .. } => {}, + } + } + let chan_id = $channel_context.channel_id(); + $peer_state.closed_channel_monitor_update_ids.insert(chan_id, update_id); + } if let Some(outpoint) = $channel_context.get_funding_txo() { $self.outpoint_to_peer.lock().unwrap().remove(&outpoint); } @@ -2912,7 +2953,7 @@ macro_rules! update_maps_on_chan_removal { /// Returns (boolean indicating if we should remove the Channel object from memory, a mapped error) macro_rules! convert_chan_phase_err { - ($self: ident, $err: expr, $channel: expr, $channel_id: expr, MANUAL_CHANNEL_UPDATE, $channel_update: expr) => { + ($self: ident, $peer_state: expr, $err: expr, $channel: expr, $channel_id: expr, MANUAL_CHANNEL_UPDATE, $channel_update: expr) => { match $err { ChannelError::Warn(msg) => { (false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Warn(msg), *$channel_id)) @@ -2923,7 +2964,7 @@ macro_rules! convert_chan_phase_err { ChannelError::Close((msg, reason)) => { let logger = WithChannelContext::from(&$self.logger, &$channel.context, None); log_error!(logger, "Closing channel {} due to close-required error: {}", $channel_id, msg); - update_maps_on_chan_removal!($self, $channel.context); + update_maps_on_chan_removal!($self, $peer_state, $channel.context); let shutdown_res = $channel.context.force_shutdown(true, reason); let err = MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, shutdown_res, $channel_update); @@ -2931,42 +2972,42 @@ macro_rules! convert_chan_phase_err { }, } }; - ($self: ident, $err: expr, $channel: expr, $channel_id: expr, FUNDED_CHANNEL) => { - convert_chan_phase_err!($self, $err, $channel, $channel_id, MANUAL_CHANNEL_UPDATE, { $self.get_channel_update_for_broadcast($channel).ok() }) + ($self: ident, $peer_state: expr, $err: expr, $channel: expr, $channel_id: expr, FUNDED_CHANNEL) => { + convert_chan_phase_err!($self, $peer_state, $err, $channel, $channel_id, MANUAL_CHANNEL_UPDATE, { $self.get_channel_update_for_broadcast($channel).ok() }) }; - ($self: ident, $err: expr, $channel: expr, $channel_id: expr, UNFUNDED_CHANNEL) => { - convert_chan_phase_err!($self, $err, $channel, $channel_id, MANUAL_CHANNEL_UPDATE, None) + ($self: ident, $peer_state: expr, $err: expr, $channel: expr, $channel_id: expr, UNFUNDED_CHANNEL) => { + convert_chan_phase_err!($self, $peer_state, $err, $channel, $channel_id, MANUAL_CHANNEL_UPDATE, None) }; - ($self: ident, $err: expr, $channel_phase: expr, $channel_id: expr) => { + ($self: ident, $peer_state: expr, $err: expr, $channel_phase: expr, $channel_id: expr) => { match $channel_phase { ChannelPhase::Funded(channel) => { - convert_chan_phase_err!($self, $err, channel, $channel_id, FUNDED_CHANNEL) + convert_chan_phase_err!($self, $peer_state, $err, channel, $channel_id, FUNDED_CHANNEL) }, ChannelPhase::UnfundedOutboundV1(channel) => { - convert_chan_phase_err!($self, $err, channel, $channel_id, UNFUNDED_CHANNEL) + convert_chan_phase_err!($self, $peer_state, $err, channel, $channel_id, UNFUNDED_CHANNEL) }, ChannelPhase::UnfundedInboundV1(channel) => { - convert_chan_phase_err!($self, $err, channel, $channel_id, UNFUNDED_CHANNEL) + convert_chan_phase_err!($self, $peer_state, $err, channel, $channel_id, UNFUNDED_CHANNEL) }, #[cfg(any(dual_funding, splicing))] ChannelPhase::UnfundedOutboundV2(channel) => { - convert_chan_phase_err!($self, $err, channel, $channel_id, UNFUNDED_CHANNEL) + convert_chan_phase_err!($self, $peer_state, $err, channel, $channel_id, UNFUNDED_CHANNEL) }, #[cfg(any(dual_funding, splicing))] ChannelPhase::UnfundedInboundV2(channel) => { - convert_chan_phase_err!($self, $err, channel, $channel_id, UNFUNDED_CHANNEL) + convert_chan_phase_err!($self, $peer_state, $err, channel, $channel_id, UNFUNDED_CHANNEL) }, } }; } macro_rules! break_chan_phase_entry { - ($self: ident, $res: expr, $entry: expr) => { + ($self: ident, $peer_state: expr, $res: expr, $entry: expr) => { match $res { Ok(res) => res, Err(e) => { let key = *$entry.key(); - let (drop, res) = convert_chan_phase_err!($self, e, $entry.get_mut(), &key); + let (drop, res) = convert_chan_phase_err!($self, $peer_state, e, $entry.get_mut(), &key); if drop { $entry.remove_entry(); } @@ -2977,12 +3018,12 @@ macro_rules! break_chan_phase_entry { } macro_rules! try_chan_phase_entry { - ($self: ident, $res: expr, $entry: expr) => { + ($self: ident, $peer_state: expr, $res: expr, $entry: expr) => { match $res { Ok(res) => res, Err(e) => { let key = *$entry.key(); - let (drop, res) = convert_chan_phase_err!($self, e, $entry.get_mut(), &key); + let (drop, res) = convert_chan_phase_err!($self, $peer_state, e, $entry.get_mut(), &key); if drop { $entry.remove_entry(); } @@ -2993,10 +3034,10 @@ macro_rules! try_chan_phase_entry { } macro_rules! remove_channel_phase { - ($self: expr, $entry: expr) => { + ($self: expr, $peer_state: expr, $entry: expr) => { { let channel = $entry.remove_entry().1; - update_maps_on_chan_removal!($self, &channel.context()); + update_maps_on_chan_removal!($self, $peer_state, &channel.context()); channel } } @@ -3674,7 +3715,7 @@ where peer_state_lock, peer_state, per_peer_state, chan); } } else { - let mut chan_phase = remove_channel_phase!(self, chan_phase_entry); + let mut chan_phase = remove_channel_phase!(self, peer_state, chan_phase_entry); shutdown_result = Some(chan_phase.context_mut().force_shutdown(false, ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) })); } }, @@ -3762,6 +3803,69 @@ where self.close_channel_internal(channel_id, counterparty_node_id, target_feerate_sats_per_1000_weight, shutdown_script) } + /// Ensures any saved latest ID in [`PeerState::closed_channel_monitor_update_ids`] is updated, + /// then applies the provided [`ChannelMonitorUpdate`]. + #[must_use] + fn apply_post_close_monitor_update( + &self, counterparty_node_id: PublicKey, channel_id: ChannelId, funding_txo: OutPoint, + mut monitor_update: ChannelMonitorUpdate, + ) -> ChannelMonitorUpdateStatus { + // Note that there may be some post-close updates which need to be well-ordered with + // respect to the `update_id`, so we hold the `closed_channel_monitor_update_ids` lock + // here (and also make sure the `monitor_update` we're applying has the right id. + let per_peer_state = self.per_peer_state.read().unwrap(); + let mut peer_state_lock = per_peer_state.get(&counterparty_node_id) + .expect("We must always have a peer entry for a peer with which we have channels that have ChannelMonitors") + .lock().unwrap(); + let peer_state = &mut *peer_state_lock; + match peer_state.channel_by_id.entry(channel_id) { + hash_map::Entry::Occupied(mut chan_phase) => { + if let ChannelPhase::Funded(chan) = chan_phase.get_mut() { + let in_flight = handle_new_monitor_update!(self, funding_txo, + monitor_update, peer_state_lock, peer_state, per_peer_state, chan); + return if in_flight { ChannelMonitorUpdateStatus::InProgress } else { ChannelMonitorUpdateStatus::Completed }; + } else { + debug_assert!(false, "We shouldn't have an update for a non-funded channel"); + } + }, + hash_map::Entry::Vacant(_) => {}, + } + match peer_state.closed_channel_monitor_update_ids.entry(channel_id) { + btree_map::Entry::Vacant(entry) => { + let is_closing_unupdated_monitor = monitor_update.update_id == 1 + && monitor_update.updates.len() == 1 + && matches!(&monitor_update.updates[0], ChannelMonitorUpdateStep::ChannelForceClosed { .. }); + // If the ChannelMonitorUpdate is closing a channel that never got past initial + // funding (to have any commitment updates), we'll skip inserting in + // `update_maps_on_chan_removal`, allowing us to avoid keeping around the PeerState + // for that peer. In that specific case we expect no entry in the map here. In any + // other cases, this is a bug, but in production we go ahead and recover by + // inserting the update_id and hoping its right. + debug_assert!(is_closing_unupdated_monitor, "Expected closing monitor against an unused channel, got {:?}", monitor_update); + if !is_closing_unupdated_monitor { + entry.insert(monitor_update.update_id); + } + }, + btree_map::Entry::Occupied(entry) => { + // If we're running in a threaded environment its possible we generate updates for + // a channel that is closing, then apply some preimage update, then go back and + // apply the close monitor update here. In order to ensure the updates are still + // well-ordered, we have to use the `closed_channel_monitor_update_ids` map to + // override the `update_id`, taking care to handle old monitors where the + // `latest_update_id` is already `u64::MAX`. + let latest_update_id = entry.into_mut(); + *latest_update_id = latest_update_id.saturating_add(1); + monitor_update.update_id = *latest_update_id; + } + } + self.chain_monitor.update_channel(funding_txo, &monitor_update) + } + + /// When a channel is removed, two things need to happen: + /// (a) [`update_maps_on_chan_removal`] must be called in the same `per_peer_state` lock as + /// the channel-closing action, + /// (b) this needs to be called without holding any locks (except + /// [`ChannelManager::total_consistency_lock`]. fn finish_close_channel(&self, mut shutdown_res: ShutdownResult) { debug_assert_ne!(self.per_peer_state.held_by_thread(), LockHeldState::HeldByThread); #[cfg(debug_assertions)] @@ -3786,7 +3890,7 @@ where // force-closing. The monitor update on the required in-memory copy should broadcast // the latest local state, which is the best we can do anyway. Thus, it is safe to // ignore the result here. - let _ = self.chain_monitor.update_channel(funding_txo, &monitor_update); + let _ = self.apply_post_close_monitor_update(shutdown_res.counterparty_node_id, shutdown_res.channel_id, funding_txo, monitor_update); } let mut shutdown_results = Vec::new(); if let Some(txid) = shutdown_res.unbroadcasted_batch_funding_txid { @@ -3798,7 +3902,7 @@ where if let Some(peer_state_mutex) = per_peer_state.get(&counterparty_node_id) { let mut peer_state = peer_state_mutex.lock().unwrap(); if let Some(mut chan) = peer_state.channel_by_id.remove(&channel_id) { - update_maps_on_chan_removal!(self, &chan.context()); + update_maps_on_chan_removal!(self, peer_state, &chan.context()); shutdown_results.push(chan.context_mut().force_shutdown(false, ClosureReason::FundingBatchClosure)); } } @@ -3858,7 +3962,7 @@ where let logger = WithContext::from(&self.logger, Some(*peer_node_id), Some(*channel_id), None); if let hash_map::Entry::Occupied(chan_phase_entry) = peer_state.channel_by_id.entry(channel_id.clone()) { log_error!(logger, "Force-closing channel {}", channel_id); - let mut chan_phase = remove_channel_phase!(self, chan_phase_entry); + let mut chan_phase = remove_channel_phase!(self, peer_state, chan_phase_entry); mem::drop(peer_state); mem::drop(per_peer_state); match chan_phase { @@ -4364,7 +4468,7 @@ where first_hop_htlc_msat: htlc_msat, payment_id, }, onion_packet, None, &self.fee_estimator, &&logger); - match break_chan_phase_entry!(self, send_res, chan_phase_entry) { + match break_chan_phase_entry!(self, peer_state, send_res, chan_phase_entry) { Some(monitor_update) => { match handle_new_monitor_update!(self, funding_txo, monitor_update, peer_state_lock, peer_state, per_peer_state, chan) { false => { @@ -5142,7 +5246,7 @@ where .map(|peer_state_mutex| peer_state_mutex.lock().unwrap()) .and_then(|mut peer_state| peer_state.channel_by_id.remove(&channel_id).map(|chan| (chan, peer_state))) .map(|(mut chan, mut peer_state)| { - update_maps_on_chan_removal!(self, &chan.context()); + update_maps_on_chan_removal!(self, peer_state, &chan.context()); let closure_reason = ClosureReason::ProcessingError { err: e.clone() }; shutdown_results.push(chan.context_mut().force_shutdown(false, closure_reason)); peer_state.pending_msg_events.push(events::MessageSendEvent::HandleError { @@ -6136,30 +6240,9 @@ where let _ = self.chain_monitor.update_channel(funding_txo, &update); }, BackgroundEvent::MonitorUpdateRegeneratedOnStartup { counterparty_node_id, funding_txo, channel_id, update } => { - let mut updated_chan = false; - { - 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; - match peer_state.channel_by_id.entry(channel_id) { - hash_map::Entry::Occupied(mut chan_phase) => { - if let ChannelPhase::Funded(chan) = chan_phase.get_mut() { - updated_chan = true; - handle_new_monitor_update!(self, funding_txo, update.clone(), - peer_state_lock, peer_state, per_peer_state, chan); - } else { - debug_assert!(false, "We shouldn't have an update for a non-funded channel"); - } - }, - hash_map::Entry::Vacant(_) => {}, - } - } - } - if !updated_chan { - // TODO: Track this as in-flight even though the channel is closed. - let _ = self.chain_monitor.update_channel(funding_txo, &update); - } + // The monitor update will be replayed on startup if it doesnt complete, so no + // use bothering to care about the monitor update completing. + let _ = 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(); @@ -6276,34 +6359,32 @@ where let mut pending_peers_awaiting_removal = Vec::new(); let mut shutdown_channels = Vec::new(); - let mut process_unfunded_channel_tick = | - chan_id: &ChannelId, - context: &mut ChannelContext, - unfunded_context: &mut UnfundedChannelContext, - pending_msg_events: &mut Vec, - counterparty_node_id: PublicKey, - | { - context.maybe_expire_prev_config(); - if unfunded_context.should_expire_unfunded_channel() { - let logger = WithChannelContext::from(&self.logger, context, None); - log_error!(logger, - "Force-closing pending channel with ID {} for not establishing in a timely manner", chan_id); - update_maps_on_chan_removal!(self, &context); - shutdown_channels.push(context.force_shutdown(false, ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) })); - pending_msg_events.push(MessageSendEvent::HandleError { - node_id: counterparty_node_id, - action: msgs::ErrorAction::SendErrorMessage { - msg: msgs::ErrorMessage { - channel_id: *chan_id, - data: "Force-closing pending channel due to timeout awaiting establishment handshake".to_owned(), + macro_rules! process_unfunded_channel_tick { + ($peer_state: expr, $chan: expr, $pending_msg_events: expr) => { { + let context = &mut $chan.context; + context.maybe_expire_prev_config(); + if $chan.unfunded_context.should_expire_unfunded_channel() { + let logger = WithChannelContext::from(&self.logger, context, None); + log_error!(logger, + "Force-closing pending channel with ID {} for not establishing in a timely manner", + context.channel_id()); + update_maps_on_chan_removal!(self, $peer_state, context); + shutdown_channels.push(context.force_shutdown(false, ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) })); + $pending_msg_events.push(MessageSendEvent::HandleError { + node_id: context.get_counterparty_node_id(), + action: msgs::ErrorAction::SendErrorMessage { + msg: msgs::ErrorMessage { + channel_id: context.channel_id(), + data: "Force-closing pending channel due to timeout awaiting establishment handshake".to_owned(), + }, }, - }, - }); - false - } else { - true - } - }; + }); + false + } else { + true + } + } } + } { let per_peer_state = self.per_peer_state.read().unwrap(); @@ -6324,7 +6405,7 @@ where if chan_needs_persist == NotifyOption::DoPersist { should_persist = NotifyOption::DoPersist; } if let Err(e) = chan.timer_check_closing_negotiation_progress() { - let (needs_close, err) = convert_chan_phase_err!(self, e, chan, chan_id, FUNDED_CHANNEL); + let (needs_close, err) = convert_chan_phase_err!(self, peer_state, e, chan, chan_id, FUNDED_CHANNEL); handle_errors.push((Err(err), counterparty_node_id)); if needs_close { return false; } } @@ -6389,22 +6470,18 @@ where true }, ChannelPhase::UnfundedInboundV1(chan) => { - process_unfunded_channel_tick(chan_id, &mut chan.context, &mut chan.unfunded_context, - pending_msg_events, counterparty_node_id) + process_unfunded_channel_tick!(peer_state, chan, pending_msg_events) }, ChannelPhase::UnfundedOutboundV1(chan) => { - process_unfunded_channel_tick(chan_id, &mut chan.context, &mut chan.unfunded_context, - pending_msg_events, counterparty_node_id) + process_unfunded_channel_tick!(peer_state, chan, pending_msg_events) }, #[cfg(any(dual_funding, splicing))] ChannelPhase::UnfundedInboundV2(chan) => { - process_unfunded_channel_tick(chan_id, &mut chan.context, &mut chan.unfunded_context, - pending_msg_events, counterparty_node_id) + process_unfunded_channel_tick!(peer_state, chan, pending_msg_events) }, #[cfg(any(dual_funding, splicing))] ChannelPhase::UnfundedOutboundV2(chan) => { - process_unfunded_channel_tick(chan_id, &mut chan.context, &mut chan.unfunded_context, - pending_msg_events, counterparty_node_id) + process_unfunded_channel_tick!(peer_state, chan, pending_msg_events) }, } }); @@ -6922,11 +6999,10 @@ where &self, prev_hop: HTLCPreviousHopData, payment_preimage: PaymentPreimage, payment_info: Option, completion_action: ComplFunc, ) { - let counterparty_node_id = - match self.short_to_chan_info.read().unwrap().get(&prev_hop.short_channel_id) { - Some((cp_id, _dup_chan_id)) => Some(cp_id.clone()), - None => None - }; + let counterparty_node_id = prev_hop.counterparty_node_id.or_else(|| { + let short_to_chan_info = self.short_to_chan_info.read().unwrap(); + short_to_chan_info.get(&prev_hop.short_channel_id).map(|(cp_id, _)| *cp_id) + }); let htlc_source = HTLCClaimSource { counterparty_node_id, @@ -7067,8 +7143,9 @@ where } } } + let preimage_update = ChannelMonitorUpdate { - update_id: CLOSED_CHANNEL_UPDATE_ID, + update_id: 0, // apply_post_close_monitor_update will set the right value counterparty_node_id: prev_hop.counterparty_node_id, updates: vec![ChannelMonitorUpdateStep::PaymentPreimage { payment_preimage, @@ -7077,10 +7154,20 @@ where channel_id: Some(prev_hop.channel_id), }; + if prev_hop.counterparty_node_id.is_none() { + let payment_hash: PaymentHash = payment_preimage.into(); + panic!( + "Prior to upgrading to LDK 0.1, all pending HTLCs forwarded by LDK 0.0.123 or before must be resolved. It appears at least the HTLC with payment_hash {} (preimage {}) was not resolved. Please downgrade to LDK 0.0.125 and resolve the HTLC prior to upgrading.", + payment_hash, + payment_preimage, + ); + } + let counterparty_node_id = prev_hop.counterparty_node_id.expect("Checked immediately above"); + if !during_init { // We update the ChannelMonitor on the backward link, after // receiving an `update_fulfill_htlc` from the forward link. - let update_res = self.chain_monitor.update_channel(prev_hop.funding_txo, &preimage_update); + let update_res = self.apply_post_close_monitor_update(counterparty_node_id, prev_hop.channel_id, prev_hop.funding_txo, preimage_update); if update_res != ChannelMonitorUpdateStatus::Completed { // TODO: This needs to be handled somehow - if we receive a monitor update // with a preimage we *must* somehow manage to propagate it to the upstream @@ -7094,18 +7181,17 @@ where // If we're running during init we cannot update a monitor directly - they probably // haven't actually been loaded yet. Instead, push the monitor update as a background // event. - // Note that while it's safe to use `ClosedMonitorUpdateRegeneratedOnStartup` here (the - // channel is already closed) we need to ultimately handle the monitor update - // completion action only after we've completed the monitor update. This is the only - // way to guarantee this update *will* be regenerated on startup (otherwise if this was - // from a forwarded HTLC the downstream preimage may be deleted before we claim - // upstream). Thus, we need to transition to some new `BackgroundEvent` type which will - // complete the monitor update completion action from `completion_action`. - self.pending_background_events.lock().unwrap().push( - BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup(( - prev_hop.funding_txo, prev_hop.channel_id, preimage_update, - ))); + // TODO: Track this update as pending and only complete the completion action when it + // finishes. + let event = BackgroundEvent::MonitorUpdateRegeneratedOnStartup { + counterparty_node_id, + funding_txo: prev_hop.funding_txo, + channel_id: prev_hop.channel_id, + update: preimage_update, + }; + self.pending_background_events.lock().unwrap().push(event); } + // Note that we do process the completion action here. This totally could be a // duplicate claim, but we have no way of knowing without interrogating the // `ChannelMonitor` we've provided the above update to. Instead, note that `Event`s are @@ -7114,40 +7200,26 @@ where let (action_opt, raa_blocker_opt) = completion_action(None, false); if let Some(raa_blocker) = raa_blocker_opt { - let counterparty_node_id = prev_hop.counterparty_node_id.or_else(|| - // prev_hop.counterparty_node_id is always available for payments received after - // LDK 0.0.123, but for those received on 0.0.123 and claimed later, we need to - // look up the counterparty in the `action_opt`, if possible. - action_opt.as_ref().and_then(|action| - if let MonitorUpdateCompletionAction::PaymentClaimed { pending_mpp_claim, .. } = action { - pending_mpp_claim.as_ref().map(|(node_id, _, _, _)| *node_id) - } else { None } - ) - ); - if let Some(counterparty_node_id) = counterparty_node_id { - // TODO: Avoid always blocking the world for the write lock here. - let mut per_peer_state = self.per_peer_state.write().unwrap(); - let peer_state_mutex = per_peer_state.entry(counterparty_node_id).or_insert_with(|| - Mutex::new(PeerState { - channel_by_id: new_hash_map(), - inbound_channel_request_by_id: new_hash_map(), - latest_features: InitFeatures::empty(), - pending_msg_events: Vec::new(), - in_flight_monitor_updates: BTreeMap::new(), - monitor_update_blocked_actions: BTreeMap::new(), - actions_blocking_raa_monitor_updates: BTreeMap::new(), - is_connected: false, - })); - let mut peer_state = peer_state_mutex.lock().unwrap(); + // TODO: Avoid always blocking the world for the write lock here. + let mut per_peer_state = self.per_peer_state.write().unwrap(); + let peer_state_mutex = per_peer_state.entry(counterparty_node_id).or_insert_with(|| + Mutex::new(PeerState { + channel_by_id: new_hash_map(), + inbound_channel_request_by_id: new_hash_map(), + latest_features: InitFeatures::empty(), + pending_msg_events: Vec::new(), + in_flight_monitor_updates: BTreeMap::new(), + monitor_update_blocked_actions: BTreeMap::new(), + actions_blocking_raa_monitor_updates: BTreeMap::new(), + closed_channel_monitor_update_ids: BTreeMap::new(), + is_connected: false, + })); + let mut peer_state = peer_state_mutex.lock().unwrap(); - peer_state.actions_blocking_raa_monitor_updates - .entry(prev_hop.channel_id) - .or_default() - .push(raa_blocker); - } else { - debug_assert!(false, - "RAA ChannelMonitorUpdate blockers are only set with PaymentClaimed completion actions, so we should always have a counterparty node id"); - } + peer_state.actions_blocking_raa_monitor_updates + .entry(prev_hop.channel_id) + .or_default() + .push(raa_blocker); } self.handle_monitor_update_completion_actions(action_opt); @@ -7181,8 +7253,6 @@ where let prev_channel_id = hop_data.channel_id; let prev_user_channel_id = hop_data.user_channel_id; let completed_blocker = RAAMonitorUpdateBlockingAction::from_prev_hop_data(&hop_data); - #[cfg(debug_assertions)] - let claiming_chan_funding_outpoint = hop_data.outpoint; self.claim_funds_from_hop(hop_data, payment_preimage, None, |htlc_claim_value_msat, definitely_duplicate| { let chan_to_release = @@ -7207,48 +7277,6 @@ where // monitor updates still in flight. In that case, we shouldn't // immediately free, but instead let that monitor update complete // in the background. - #[cfg(debug_assertions)] { - let background_events = self.pending_background_events.lock().unwrap(); - // There should be a `BackgroundEvent` pending... - assert!(background_events.iter().any(|ev| { - match ev { - // to apply a monitor update that blocked the claiming channel, - BackgroundEvent::MonitorUpdateRegeneratedOnStartup { - funding_txo, update, .. - } => { - if *funding_txo == claiming_chan_funding_outpoint { - assert!(update.updates.iter().any(|upd| - if let ChannelMonitorUpdateStep::PaymentPreimage { - payment_preimage: update_preimage, .. - } = upd { - payment_preimage == *update_preimage - } else { false } - ), "{:?}", update); - true - } else { false } - }, - // or the channel we'd unblock is already closed, - BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup( - (funding_txo, _channel_id, monitor_update) - ) => { - if *funding_txo == next_channel_outpoint { - assert_eq!(monitor_update.updates.len(), 1); - assert!(matches!( - monitor_update.updates[0], - ChannelMonitorUpdateStep::ChannelForceClosed { .. } - )); - true - } else { false } - }, - // or the monitor update has completed and will unblock - // immediately once we get going. - BackgroundEvent::MonitorUpdatesComplete { - channel_id, .. - } => - *channel_id == prev_channel_id, - } - }), "{:?}", *background_events); - } (None, None) } else if definitely_duplicate { if let Some(other_chan) = chan_to_release { @@ -7897,7 +7925,7 @@ where hash_map::Entry::Occupied(mut phase) => { match phase.get_mut() { ChannelPhase::UnfundedOutboundV1(chan) => { - try_chan_phase_entry!(self, chan.accept_channel(&msg, &self.default_configuration.channel_handshake_limits, &peer_state.latest_features), phase); + try_chan_phase_entry!(self, peer_state, chan.accept_channel(&msg, &self.default_configuration.channel_handshake_limits, &peer_state.latest_features), phase); (chan.context.get_value_satoshis(), chan.context.get_funding_redeemscript().to_p2wsh(), chan.context.get_user_id()) }, _ => { @@ -7945,14 +7973,14 @@ where // Really we should be returning the channel_id the peer expects based // on their funding info here, but they're horribly confused anyway, so // there's not a lot we can do to save them. - return Err(convert_chan_phase_err!(self, err, &mut ChannelPhase::UnfundedInboundV1(inbound_chan), &msg.temporary_channel_id).1); + return Err(convert_chan_phase_err!(self, peer_state, err, &mut ChannelPhase::UnfundedInboundV1(inbound_chan), &msg.temporary_channel_id).1); }, } }, Some(mut phase) => { let err_msg = format!("Got an unexpected funding_created message from peer with counterparty_node_id {}", counterparty_node_id); let err = ChannelError::close(err_msg); - return Err(convert_chan_phase_err!(self, err, &mut phase, &msg.temporary_channel_id).1); + return Err(convert_chan_phase_err!(self, peer_state, err, &mut phase, &msg.temporary_channel_id).1); }, None => return Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", counterparty_node_id), msg.temporary_channel_id)) }; @@ -7968,7 +7996,7 @@ where // on the channel. let err = ChannelError::close($err.to_owned()); chan.unset_funding_info(msg.temporary_channel_id); - return Err(convert_chan_phase_err!(self, err, chan, &funded_channel_id, UNFUNDED_CHANNEL).1); + return Err(convert_chan_phase_err!(self, peer_state, err, chan, &funded_channel_id, UNFUNDED_CHANNEL).1); } } } match peer_state.channel_by_id.entry(funded_channel_id) { @@ -8057,7 +8085,7 @@ where // found an (unreachable) panic when the monitor update contained // within `shutdown_finish` was applied. chan.unset_funding_info(msg.channel_id); - return Err(convert_chan_phase_err!(self, e, &mut ChannelPhase::Funded(chan), &msg.channel_id).1); + return Err(convert_chan_phase_err!(self, peer_state, e, &mut ChannelPhase::Funded(chan), &msg.channel_id).1); } }, Err((chan, e)) => { @@ -8066,7 +8094,7 @@ where // We've already removed this outbound channel from the map in // `PeerState` above so at this point we just need to clean up any // lingering entries concerning this channel as it is safe to do so. - return Err(convert_chan_phase_err!(self, e, &mut ChannelPhase::UnfundedOutboundV1(chan), &msg.channel_id).1); + return Err(convert_chan_phase_err!(self, peer_state, e, &mut ChannelPhase::UnfundedOutboundV1(chan), &msg.channel_id).1); } } } else { @@ -8092,7 +8120,7 @@ where hash_map::Entry::Occupied(mut chan_phase_entry) => { if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() { let logger = WithChannelContext::from(&self.logger, &chan.context, None); - let announcement_sigs_opt = try_chan_phase_entry!(self, chan.channel_ready(&msg, &self.node_signer, + let announcement_sigs_opt = try_chan_phase_entry!(self, peer_state, chan.channel_ready(&msg, &self.node_signer, self.chain_hash, &self.default_configuration, &self.best_block.read().unwrap(), &&logger), chan_phase_entry); if let Some(announcement_sigs) = announcement_sigs_opt { log_trace!(logger, "Sending announcement_signatures for channel {}", chan.context.channel_id()); @@ -8122,7 +8150,7 @@ where Ok(()) } else { - try_chan_phase_entry!(self, Err(ChannelError::close( + try_chan_phase_entry!(self, peer_state, Err(ChannelError::close( "Got a channel_ready message for an unfunded channel!".into())), chan_phase_entry) } }, @@ -8156,7 +8184,7 @@ where } let funding_txo_opt = chan.context.get_funding_txo(); - let (shutdown, monitor_update_opt, htlcs) = try_chan_phase_entry!(self, + let (shutdown, monitor_update_opt, htlcs) = try_chan_phase_entry!(self, peer_state, chan.shutdown(&self.signer_provider, &peer_state.latest_features, &msg), chan_phase_entry); dropped_htlcs = htlcs; @@ -8179,7 +8207,7 @@ where let context = phase.context_mut(); let logger = WithChannelContext::from(&self.logger, context, None); log_error!(logger, "Immediately closing unfunded channel {} as peer asked to cooperatively shut it down (which is unnecessary)", &msg.channel_id); - let mut chan = remove_channel_phase!(self, chan_phase_entry); + let mut chan = remove_channel_phase!(self, peer_state, chan_phase_entry); finish_shutdown = Some(chan.context_mut().force_shutdown(false, ClosureReason::CounterpartyCoopClosedUnfundedChannel)); }, // TODO(dual_funding): Combine this match arm with above. @@ -8187,7 +8215,7 @@ where ChannelPhase::UnfundedInboundV2(_) | ChannelPhase::UnfundedOutboundV2(_) => { let context = phase.context_mut(); log_error!(self.logger, "Immediately closing unfunded channel {} as peer asked to cooperatively shut it down (which is unnecessary)", &msg.channel_id); - let mut chan = remove_channel_phase!(self, chan_phase_entry); + let mut chan = remove_channel_phase!(self, peer_state, chan_phase_entry); finish_shutdown = Some(chan.context_mut().force_shutdown(false, ClosureReason::CounterpartyCoopClosedUnfundedChannel)); }, } @@ -8221,7 +8249,7 @@ where hash_map::Entry::Occupied(mut chan_phase_entry) => { if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() { let logger = WithChannelContext::from(&self.logger, &chan.context, None); - let (closing_signed, tx, shutdown_result) = try_chan_phase_entry!(self, chan.closing_signed(&self.fee_estimator, &msg, &&logger), chan_phase_entry); + let (closing_signed, tx, shutdown_result) = try_chan_phase_entry!(self, peer_state, chan.closing_signed(&self.fee_estimator, &msg, &&logger), chan_phase_entry); debug_assert_eq!(shutdown_result.is_some(), chan.is_shutdown()); if let Some(msg) = closing_signed { peer_state.pending_msg_events.push(events::MessageSendEvent::SendClosingSigned { @@ -8235,10 +8263,10 @@ where // also implies there are no pending HTLCs left on the channel, so we can // fully delete it from tracking (the channel monitor is still around to // watch for old state broadcasts)! - (tx, Some(remove_channel_phase!(self, chan_phase_entry)), shutdown_result) + (tx, Some(remove_channel_phase!(self, peer_state, chan_phase_entry)), shutdown_result) } else { (tx, None, shutdown_result) } } else { - return try_chan_phase_entry!(self, Err(ChannelError::close( + return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close( "Got a closing_signed message for an unfunded channel!".into())), chan_phase_entry); } }, @@ -8336,9 +8364,9 @@ where } } } - try_chan_phase_entry!(self, chan.update_add_htlc(&msg, pending_forward_info, &self.fee_estimator), chan_phase_entry); + try_chan_phase_entry!(self, peer_state, chan.update_add_htlc(&msg, pending_forward_info, &self.fee_estimator), chan_phase_entry); } else { - return try_chan_phase_entry!(self, Err(ChannelError::close( + return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close( "Got an update_add_htlc message for an unfunded channel!".into())), chan_phase_entry); } }, @@ -8362,7 +8390,7 @@ where match peer_state.channel_by_id.entry(msg.channel_id) { hash_map::Entry::Occupied(mut chan_phase_entry) => { if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() { - let res = try_chan_phase_entry!(self, chan.update_fulfill_htlc(&msg), chan_phase_entry); + let res = try_chan_phase_entry!(self, peer_state, chan.update_fulfill_htlc(&msg), chan_phase_entry); if let HTLCSource::PreviousHopData(prev_hop) = &res.0 { let logger = WithChannelContext::from(&self.logger, &chan.context, None); log_trace!(logger, @@ -8382,7 +8410,7 @@ where next_user_channel_id = chan.context.get_user_id(); res } else { - return try_chan_phase_entry!(self, Err(ChannelError::close( + return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close( "Got an update_fulfill_htlc message for an unfunded channel!".into())), chan_phase_entry); } }, @@ -8411,9 +8439,9 @@ where match peer_state.channel_by_id.entry(msg.channel_id) { hash_map::Entry::Occupied(mut chan_phase_entry) => { if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() { - try_chan_phase_entry!(self, chan.update_fail_htlc(&msg, HTLCFailReason::from_msg(msg)), chan_phase_entry); + try_chan_phase_entry!(self, peer_state, chan.update_fail_htlc(&msg, HTLCFailReason::from_msg(msg)), chan_phase_entry); } else { - return try_chan_phase_entry!(self, Err(ChannelError::close( + return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close( "Got an update_fail_htlc message for an unfunded channel!".into())), chan_phase_entry); } }, @@ -8437,12 +8465,12 @@ where hash_map::Entry::Occupied(mut chan_phase_entry) => { if (msg.failure_code & 0x8000) == 0 { let chan_err = ChannelError::close("Got update_fail_malformed_htlc with BADONION not set".to_owned()); - try_chan_phase_entry!(self, Err(chan_err), chan_phase_entry); + try_chan_phase_entry!(self, peer_state, Err(chan_err), chan_phase_entry); } if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() { - try_chan_phase_entry!(self, chan.update_fail_malformed_htlc(&msg, HTLCFailReason::reason(msg.failure_code, msg.sha256_of_onion.to_vec())), chan_phase_entry); + try_chan_phase_entry!(self, peer_state, chan.update_fail_malformed_htlc(&msg, HTLCFailReason::reason(msg.failure_code, msg.sha256_of_onion.to_vec())), chan_phase_entry); } else { - return try_chan_phase_entry!(self, Err(ChannelError::close( + return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close( "Got an update_fail_malformed_htlc message for an unfunded channel!".into())), chan_phase_entry); } Ok(()) @@ -8465,14 +8493,14 @@ where if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() { let logger = WithChannelContext::from(&self.logger, &chan.context, None); let funding_txo = chan.context.get_funding_txo(); - let monitor_update_opt = try_chan_phase_entry!(self, chan.commitment_signed(&msg, &&logger), chan_phase_entry); + let monitor_update_opt = try_chan_phase_entry!(self, peer_state, chan.commitment_signed(&msg, &&logger), chan_phase_entry); if let Some(monitor_update) = monitor_update_opt { handle_new_monitor_update!(self, funding_txo.unwrap(), monitor_update, peer_state_lock, peer_state, per_peer_state, chan); } Ok(()) } else { - return try_chan_phase_entry!(self, Err(ChannelError::close( + return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close( "Got a commitment_signed message for an unfunded channel!".into())), chan_phase_entry); } }, @@ -8665,7 +8693,7 @@ where &peer_state.actions_blocking_raa_monitor_updates, funding_txo, msg.channel_id, *counterparty_node_id) } else { false }; - let (htlcs_to_fail, monitor_update_opt) = try_chan_phase_entry!(self, + let (htlcs_to_fail, monitor_update_opt) = try_chan_phase_entry!(self, peer_state, chan.revoke_and_ack(&msg, &self.fee_estimator, &&logger, mon_update_blocked), chan_phase_entry); if let Some(monitor_update) = monitor_update_opt { let funding_txo = funding_txo_opt @@ -8675,7 +8703,7 @@ where } htlcs_to_fail } else { - return try_chan_phase_entry!(self, Err(ChannelError::close( + return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close( "Got a revoke_and_ack message for an unfunded channel!".into())), chan_phase_entry); } }, @@ -8699,9 +8727,9 @@ where hash_map::Entry::Occupied(mut chan_phase_entry) => { if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() { let logger = WithChannelContext::from(&self.logger, &chan.context, None); - try_chan_phase_entry!(self, chan.update_fee(&self.fee_estimator, &msg, &&logger), chan_phase_entry); + try_chan_phase_entry!(self, peer_state, chan.update_fee(&self.fee_estimator, &msg, &&logger), chan_phase_entry); } else { - return try_chan_phase_entry!(self, Err(ChannelError::close( + return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close( "Got an update_fee message for an unfunded channel!".into())), chan_phase_entry); } }, @@ -8727,7 +8755,7 @@ where } peer_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelAnnouncement { - msg: try_chan_phase_entry!(self, chan.announcement_signatures( + msg: try_chan_phase_entry!(self, peer_state, chan.announcement_signatures( &self.node_signer, self.chain_hash, self.best_block.read().unwrap().height, msg, &self.default_configuration ), chan_phase_entry), @@ -8736,7 +8764,7 @@ where update_msg: Some(self.get_channel_update_for_broadcast(chan).unwrap()), }); } else { - return try_chan_phase_entry!(self, Err(ChannelError::close( + return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close( "Got an announcement_signatures message for an unfunded channel!".into())), chan_phase_entry); } }, @@ -8780,7 +8808,7 @@ where } else { let logger = WithChannelContext::from(&self.logger, &chan.context, None); log_debug!(logger, "Received channel_update {:?} for channel {}.", msg, chan_id); - let did_change = try_chan_phase_entry!(self, chan.channel_update(&msg), chan_phase_entry); + let did_change = try_chan_phase_entry!(self, peer_state, chan.channel_update(&msg), chan_phase_entry); // If nothing changed after applying their update, we don't need to bother // persisting. if !did_change { @@ -8788,7 +8816,7 @@ where } } } else { - return try_chan_phase_entry!(self, Err(ChannelError::close( + return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close( "Got a channel_update for an unfunded channel!".into())), chan_phase_entry); } }, @@ -8819,7 +8847,7 @@ where // disconnect, so Channel's reestablish will never hand us any holding cell // freed HTLCs to fail backwards. If in the future we no longer drop pending // add-HTLCs on disconnect, we may be handed HTLCs to fail backwards here. - let responses = try_chan_phase_entry!(self, chan.channel_reestablish( + let responses = try_chan_phase_entry!(self, peer_state, chan.channel_reestablish( msg, &&logger, &self.node_signer, self.chain_hash, &self.default_configuration, &*self.best_block.read().unwrap()), chan_phase_entry); let mut channel_update = None; @@ -8850,7 +8878,7 @@ where } need_lnd_workaround } else { - return try_chan_phase_entry!(self, Err(ChannelError::close( + return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close( "Got a channel_reestablish message for an unfunded channel!".into())), chan_phase_entry); } }, @@ -8937,7 +8965,7 @@ where let peer_state = &mut *peer_state_lock; let pending_msg_events = &mut peer_state.pending_msg_events; if let hash_map::Entry::Occupied(chan_phase_entry) = peer_state.channel_by_id.entry(channel_id) { - if let ChannelPhase::Funded(mut chan) = remove_channel_phase!(self, chan_phase_entry) { + if let ChannelPhase::Funded(mut chan) = remove_channel_phase!(self, peer_state, chan_phase_entry) { let reason = if let MonitorEvent::HolderForceClosedWithInfo { reason, .. } = monitor_event { reason } else { @@ -9130,7 +9158,10 @@ where _ => unblock_chan(chan, &mut peer_state.pending_msg_events), }; if let Some(shutdown_result) = shutdown_result { - log_trace!(self.logger, "Removing channel after unblocking signer"); + let context = &chan.context(); + let logger = WithChannelContext::from(&self.logger, context, None); + log_trace!(logger, "Removing channel {} now that the signer is unblocked", context.channel_id()); + update_maps_on_chan_removal!(self, peer_state, context); shutdown_results.push(shutdown_result); false } else { @@ -9186,13 +9217,13 @@ where log_info!(logger, "Broadcasting {}", log_tx!(tx)); self.tx_broadcaster.broadcast_transactions(&[&tx]); - update_maps_on_chan_removal!(self, &chan.context); + update_maps_on_chan_removal!(self, peer_state, &chan.context); false } else { true } }, Err(e) => { has_update = true; - let (close_channel, res) = convert_chan_phase_err!(self, e, chan, channel_id, FUNDED_CHANNEL); + let (close_channel, res) = convert_chan_phase_err!(self, peer_state, e, chan, channel_id, FUNDED_CHANNEL); handle_errors.push((chan.context.get_counterparty_node_id(), Err(res))); !close_channel } @@ -10404,7 +10435,7 @@ where } } } else if let Err(reason) = res { - update_maps_on_chan_removal!(self, &channel.context); + update_maps_on_chan_removal!(self, peer_state, &channel.context); // It looks like our counterparty went on-chain or funding transaction was // reorged out of the main chain. Close the channel. let reason_message = format!("{}", reason); @@ -10833,7 +10864,7 @@ where }, }; // Clean up for removal. - update_maps_on_chan_removal!(self, &context); + update_maps_on_chan_removal!(self, peer_state, &context); failed_channels.push(context.force_shutdown(false, ClosureReason::DisconnectedPeer)); false }); @@ -10941,6 +10972,7 @@ where in_flight_monitor_updates: BTreeMap::new(), monitor_update_blocked_actions: BTreeMap::new(), actions_blocking_raa_monitor_updates: BTreeMap::new(), + closed_channel_monitor_update_ids: BTreeMap::new(), is_connected: true, })); }, @@ -12395,11 +12427,24 @@ where let best_block_height: u32 = Readable::read(reader)?; let best_block_hash: BlockHash = Readable::read(reader)?; - let mut failed_htlcs = Vec::new(); + let empty_peer_state = || { + PeerState { + channel_by_id: new_hash_map(), + inbound_channel_request_by_id: new_hash_map(), + latest_features: InitFeatures::empty(), + pending_msg_events: Vec::new(), + in_flight_monitor_updates: BTreeMap::new(), + monitor_update_blocked_actions: BTreeMap::new(), + actions_blocking_raa_monitor_updates: BTreeMap::new(), + closed_channel_monitor_update_ids: BTreeMap::new(), + is_connected: false, + } + }; + let mut failed_htlcs = Vec::new(); let channel_count: u64 = Readable::read(reader)?; let mut funding_txo_set = hash_set_with_capacity(cmp::min(channel_count as usize, 128)); - let mut funded_peer_channels: HashMap>> = hash_map_with_capacity(cmp::min(channel_count as usize, 128)); + let mut per_peer_state = hash_map_with_capacity(cmp::min(channel_count as usize, MAX_ALLOC_SIZE/mem::size_of::<(PublicKey, Mutex>)>())); let mut outpoint_to_peer = hash_map_with_capacity(cmp::min(channel_count as usize, 128)); let mut short_to_chan_info = hash_map_with_capacity(cmp::min(channel_count as usize, 128)); let mut channel_closures = VecDeque::new(); @@ -12441,7 +12486,19 @@ where if shutdown_result.unbroadcasted_batch_funding_txid.is_some() { return Err(DecodeError::InvalidValue); } - if let Some((counterparty_node_id, funding_txo, channel_id, update)) = shutdown_result.monitor_update { + if let Some((counterparty_node_id, funding_txo, channel_id, mut update)) = shutdown_result.monitor_update { + // Our channel information is out of sync with the `ChannelMonitor`, so + // force the update to use the `ChannelMonitor`'s update_id for the close + // update. + let latest_update_id = monitor.get_latest_update_id(); + update.update_id = latest_update_id.saturating_add(1); + per_peer_state.entry(counterparty_node_id) + .or_insert_with(|| Mutex::new(empty_peer_state())) + .lock().unwrap() + .closed_channel_monitor_update_ids.entry(channel_id) + .and_modify(|v| *v = cmp::max(latest_update_id, *v)) + .or_insert(latest_update_id); + close_background_events.push(BackgroundEvent::MonitorUpdateRegeneratedOnStartup { counterparty_node_id, funding_txo, channel_id, update }); @@ -12487,17 +12544,10 @@ where if let Some(funding_txo) = channel.context.get_funding_txo() { outpoint_to_peer.insert(funding_txo, channel.context.get_counterparty_node_id()); } - match funded_peer_channels.entry(channel.context.get_counterparty_node_id()) { - hash_map::Entry::Occupied(mut entry) => { - let by_id_map = entry.get_mut(); - by_id_map.insert(channel.context.channel_id(), ChannelPhase::Funded(channel)); - }, - hash_map::Entry::Vacant(entry) => { - let mut by_id_map = new_hash_map(); - by_id_map.insert(channel.context.channel_id(), ChannelPhase::Funded(channel)); - entry.insert(by_id_map); - } - } + per_peer_state.entry(channel.context.get_counterparty_node_id()) + .or_insert_with(|| Mutex::new(empty_peer_state())) + .get_mut().unwrap() + .channel_by_id.insert(channel.context.channel_id(), ChannelPhase::Funded(channel)); } } else if channel.is_awaiting_initial_mon_persist() { // If we were persisted and shut down while the initial ChannelMonitor persistence @@ -12525,17 +12575,56 @@ where for (funding_txo, monitor) in args.channel_monitors.iter() { if !funding_txo_set.contains(funding_txo) { + if let Some(counterparty_node_id) = monitor.get_counterparty_node_id() { + // If the ChannelMonitor had any updates, we may need to update it further and + // thus track it in `closed_channel_monitor_update_ids`. If the channel never + // had any updates at all, there can't be any HTLCs pending which we need to + // claim. + // Note that a `ChannelMonitor` is created with `update_id` 0 and after we + // provide it with a closure update its `update_id` will be at 1. + if !monitor.offchain_closed() || monitor.get_latest_update_id() > 1 { + per_peer_state.entry(counterparty_node_id) + .or_insert_with(|| Mutex::new(empty_peer_state())) + .lock().unwrap() + .closed_channel_monitor_update_ids.entry(monitor.channel_id()) + .and_modify(|v| *v = cmp::max(monitor.get_latest_update_id(), *v)) + .or_insert(monitor.get_latest_update_id()); + } + } + + if monitor.offchain_closed() { + // We already appled a ChannelForceClosed update. + continue; + } + let logger = WithChannelMonitor::from(&args.logger, monitor, None); let channel_id = monitor.channel_id(); log_info!(logger, "Queueing monitor update to ensure missing channel {} is force closed", &channel_id); - let monitor_update = ChannelMonitorUpdate { - update_id: CLOSED_CHANNEL_UPDATE_ID, + let mut monitor_update = ChannelMonitorUpdate { + update_id: monitor.get_latest_update_id().saturating_add(1), counterparty_node_id: None, updates: vec![ChannelMonitorUpdateStep::ChannelForceClosed { should_broadcast: true }], channel_id: Some(monitor.channel_id()), }; - close_background_events.push(BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup((*funding_txo, channel_id, monitor_update))); + if let Some(counterparty_node_id) = monitor.get_counterparty_node_id() { + let update = BackgroundEvent::MonitorUpdateRegeneratedOnStartup { + counterparty_node_id, + funding_txo: *funding_txo, + channel_id, + update: monitor_update, + }; + close_background_events.push(update); + } else { + // This is a fairly old `ChannelMonitor` that hasn't seen an update to its + // off-chain state since LDK 0.0.118 (as in LDK 0.0.119 any off-chain + // `ChannelMonitorUpdate` will set the counterparty ID). + // Thus, we assume that it has no pending HTLCs and we will not need to + // generate a `ChannelMonitorUpdate` for it aside from this + // `ChannelForceClosed` one. + monitor_update.update_id = u64::MAX; + close_background_events.push(BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup((*funding_txo, channel_id, monitor_update))); + } } } @@ -12564,27 +12653,13 @@ where claimable_htlcs_list.push((payment_hash, previous_hops)); } - let peer_state_from_chans = |channel_by_id| { - PeerState { - channel_by_id, - inbound_channel_request_by_id: new_hash_map(), - latest_features: InitFeatures::empty(), - pending_msg_events: Vec::new(), - in_flight_monitor_updates: BTreeMap::new(), - monitor_update_blocked_actions: BTreeMap::new(), - actions_blocking_raa_monitor_updates: BTreeMap::new(), - is_connected: false, - } - }; - let peer_count: u64 = Readable::read(reader)?; - let mut per_peer_state = hash_map_with_capacity(cmp::min(peer_count as usize, MAX_ALLOC_SIZE/mem::size_of::<(PublicKey, Mutex>)>())); for _ in 0..peer_count { - let peer_pubkey = Readable::read(reader)?; - let peer_chans = funded_peer_channels.remove(&peer_pubkey).unwrap_or(new_hash_map()); - let mut peer_state = peer_state_from_chans(peer_chans); - peer_state.latest_features = Readable::read(reader)?; - per_peer_state.insert(peer_pubkey, Mutex::new(peer_state)); + let peer_pubkey: PublicKey = Readable::read(reader)?; + let latest_features = Readable::read(reader)?; + if let Some(peer_state) = per_peer_state.get_mut(&peer_pubkey) { + peer_state.get_mut().unwrap().latest_features = latest_features; + } } let event_count: u64 = Readable::read(reader)?; @@ -12796,7 +12871,7 @@ where // still open, we need to replay any monitor updates that are for closed channels, // creating the neccessary peer_state entries as we go. let peer_state_mutex = per_peer_state.entry(counterparty_id).or_insert_with(|| { - Mutex::new(peer_state_from_chans(new_hash_map())) + Mutex::new(empty_peer_state()) }); let mut peer_state = peer_state_mutex.lock().unwrap(); handle_in_flight_updates!(counterparty_id, chan_in_flight_updates, @@ -12929,11 +13004,97 @@ where // Whether the downstream channel was closed or not, try to re-apply any payment // preimages from it which may be needed in upstream channels for forwarded // payments. + let mut fail_read = false; let outbound_claimed_htlcs_iter = monitor.get_all_current_outbound_htlcs() .into_iter() .filter_map(|(htlc_source, (htlc, preimage_opt))| { - if let HTLCSource::PreviousHopData(_) = htlc_source { + if let HTLCSource::PreviousHopData(prev_hop) = &htlc_source { if let Some(payment_preimage) = preimage_opt { + let inbound_edge_monitor = args.channel_monitors.get(&prev_hop.outpoint); + // Note that for channels which have gone to chain, + // `get_all_current_outbound_htlcs` is never pruned and always returns + // a constant set until the monitor is removed/archived. Thus, we + // want to skip replaying claims that have definitely been resolved + // on-chain. + + // If the inbound monitor is not present, we assume it was fully + // resolved and properly archived, implying this payment had plenty + // of time to get claimed and we can safely skip any further + // attempts to claim it (they wouldn't succeed anyway as we don't + // have a monitor against which to do so). + let inbound_edge_monitor = if let Some(monitor) = inbound_edge_monitor { + monitor + } else { + return None; + }; + // Second, if the inbound edge of the payment's monitor has been + // fully claimed we've had at least `ANTI_REORG_DELAY` blocks to + // get any PaymentForwarded event(s) to the user and assume that + // there's no need to try to replay the claim just for that. + let inbound_edge_balances = inbound_edge_monitor.get_claimable_balances(); + if inbound_edge_balances.is_empty() { + return None; + } + + if prev_hop.counterparty_node_id.is_none() { + // We no longer support claiming an HTLC where we don't have + // the counterparty_node_id available if the claim has to go to + // a closed channel. Its possible we can get away with it if + // the channel is not yet closed, but its by no means a + // guarantee. + + // Thus, in this case we are a bit more aggressive with our + // pruning - if we have no use for the claim (because the + // inbound edge of the payment's monitor has already claimed + // the HTLC) we skip trying to replay the claim. + let htlc_payment_hash: PaymentHash = payment_preimage.into(); + let balance_could_incl_htlc = |bal| match bal { + &Balance::ClaimableOnChannelClose { .. } => { + // The channel is still open, assume we can still + // claim against it + true + }, + &Balance::MaybePreimageClaimableHTLC { payment_hash, .. } => { + payment_hash == htlc_payment_hash + }, + _ => false, + }; + let htlc_may_be_in_balances = + inbound_edge_balances.iter().any(balance_could_incl_htlc); + if !htlc_may_be_in_balances { + return None; + } + + // First check if we're absolutely going to fail - if we need + // to replay this claim to get the preimage into the inbound + // edge monitor but the channel is closed (and thus we'll + // immediately panic if we call claim_funds_from_hop). + if short_to_chan_info.get(&prev_hop.short_channel_id).is_none() { + log_error!(args.logger, + "We need to replay the HTLC claim for payment_hash {} (preimage {}) but cannot do so as the HTLC was forwarded prior to LDK 0.0.124.\ + All HTLCs that were forwarded by LDK 0.0.123 and prior must be resolved prior to upgrading to LDK 0.1", + htlc_payment_hash, + payment_preimage, + ); + fail_read = true; + } + + // At this point we're confident we need the claim, but the + // inbound edge channel is still live. As long as this remains + // the case, we can conceivably proceed, but we run some risk + // of panicking at runtime. The user ideally should have read + // the release notes and we wouldn't be here, but we go ahead + // and let things run in the hope that it'll all just work out. + log_error!(args.logger, + "We need to replay the HTLC claim for payment_hash {} (preimage {}) but don't have all the required information to do so reliably.\ + As long as the channel for the inbound edge of the forward remains open, this may work okay, but we may panic at runtime!\ + All HTLCs that were forwarded by LDK 0.0.123 and prior must be resolved prior to upgrading to LDK 0.1\ + Continuing anyway, though panics may occur!", + htlc_payment_hash, + payment_preimage, + ); + } + Some((htlc_source, payment_preimage, htlc.amount_msat, // Check if `counterparty_opt.is_none()` to see if the // downstream chan is closed (because we don't have a @@ -12953,6 +13114,9 @@ where for tuple in outbound_claimed_htlcs_iter { pending_claims_to_replay.push(tuple); } + if fail_read { + return Err(DecodeError::InvalidValue); + } } } @@ -13029,6 +13193,33 @@ where } } + // Similar to the above cases for forwarded payments, if we have any pending inbound HTLCs + // which haven't yet been claimed, we may be missing counterparty_node_id info and would + // panic if we attempted to claim them at this point. + for (payment_hash, payment) in claimable_payments.iter() { + for htlc in payment.htlcs.iter() { + if htlc.prev_hop.counterparty_node_id.is_some() { + continue; + } + if short_to_chan_info.get(&htlc.prev_hop.short_channel_id).is_some() { + log_error!(args.logger, + "We do not have the required information to claim a pending payment with payment hash {} reliably.\ + As long as the channel for the inbound edge of the forward remains open, this may work okay, but we may panic at runtime!\ + All HTLCs that were received by LDK 0.0.123 and prior must be resolved prior to upgrading to LDK 0.1\ + Continuing anyway, though panics may occur!", + payment_hash, + ); + } else { + log_error!(args.logger, + "We do not have the required information to claim a pending payment with payment hash {}.\ + All HTLCs that were received by LDK 0.0.123 and prior must be resolved prior to upgrading to LDK 0.1", + payment_hash, + ); + return Err(DecodeError::InvalidValue); + } + } + } + let mut secp_ctx = Secp256k1::new(); secp_ctx.seeded_randomize(&args.entropy_source.get_secure_random_bytes()); @@ -13910,15 +14101,15 @@ mod tests { let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); - let chan = create_announced_chan_between_nodes(&nodes, 0, 1); + create_chan_between_nodes_with_value_init(&nodes[0], &nodes[1], 1_000_000, 0); nodes[0].node.peer_disconnected(nodes[1].node.get_our_node_id()); nodes[1].node.peer_disconnected(nodes[0].node.get_our_node_id()); + let chan_id = nodes[0].node.list_channels()[0].channel_id; let error_message = "Channel force-closed"; - nodes[0].node.force_close_broadcasting_latest_txn(&chan.2, &nodes[1].node.get_our_node_id(), error_message.to_string()).unwrap(); - check_closed_broadcast!(nodes[0], true); + nodes[0].node.force_close_broadcasting_latest_txn(&chan_id, &nodes[1].node.get_our_node_id(), error_message.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); + check_closed_event!(nodes[0], 1, ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) }, [nodes[1].node.get_our_node_id()], 1_000_000); { // Assert that nodes[1] is awaiting removal for nodes[0] once nodes[1] has been @@ -13937,6 +14128,31 @@ mod tests { } } + #[test] + fn test_drop_peers_when_removing_unfunded_channels() { + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + exchange_open_accept_chan(&nodes[0], &nodes[1], 1_000_000, 0); + let events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 1, "Unexpected events {:?}", events); + match events[0] { + Event::FundingGenerationReady { .. } => {} + _ => panic!("Unexpected event {:?}", events), + } + + nodes[0].node.peer_disconnected(nodes[1].node.get_our_node_id()); + nodes[1].node.peer_disconnected(nodes[0].node.get_our_node_id()); + check_closed_event!(nodes[0], 1, ClosureReason::DisconnectedPeer, [nodes[1].node.get_our_node_id()], 1_000_000); + check_closed_event!(nodes[1], 1, ClosureReason::DisconnectedPeer, [nodes[0].node.get_our_node_id()], 1_000_000); + + // At this point the state for the peers should have been removed. + assert_eq!(nodes[0].node.per_peer_state.read().unwrap().len(), 0); + assert_eq!(nodes[1].node.per_peer_state.read().unwrap().len(), 0); + } + #[test] fn bad_inbound_payment_hash() { // Add coverage for checking that a user-provided payment hash matches the payment secret. diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index ee73b30baef..dfef2e09cd2 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -15,7 +15,7 @@ use crate::chain; use crate::chain::{ChannelMonitorUpdateStatus, Confirm, Listen, Watch}; use crate::chain::chaininterface::LowerBoundedFeeEstimator; use crate::chain::channelmonitor; -use crate::chain::channelmonitor::{Balance, CLOSED_CHANNEL_UPDATE_ID, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY}; +use crate::chain::channelmonitor::{Balance, ChannelMonitorUpdateStep, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY}; use crate::chain::transaction::OutPoint; use crate::sign::{ecdsa::EcdsaChannelSigner, EntropySource, OutputSpender, SignerProvider}; use crate::events::{Event, FundingInfo, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentPurpose, ClosureReason, HTLCDestination, PaymentFailureReason}; @@ -11005,7 +11005,8 @@ fn test_close_in_funding_batch() { let mut monitor_updates = nodes[0].chain_monitor.monitor_updates.lock().unwrap(); let monitor_updates_1 = monitor_updates.get(&channel_id_1).unwrap(); assert_eq!(monitor_updates_1.len(), 1); - assert_eq!(monitor_updates_1[0].update_id, CLOSED_CHANNEL_UPDATE_ID); + assert_eq!(monitor_updates_1[0].updates.len(), 1); + assert!(matches!(monitor_updates_1[0].updates[0], ChannelMonitorUpdateStep::ChannelForceClosed { .. })); } let msg_events = nodes[0].node.get_and_clear_pending_msg_events(); @@ -11092,10 +11093,12 @@ fn test_batch_funding_close_after_funding_signed() { let mut monitor_updates = nodes[0].chain_monitor.monitor_updates.lock().unwrap(); let monitor_updates_1 = monitor_updates.get(&channel_id_1).unwrap(); assert_eq!(monitor_updates_1.len(), 1); - assert_eq!(monitor_updates_1[0].update_id, CLOSED_CHANNEL_UPDATE_ID); + assert_eq!(monitor_updates_1[0].updates.len(), 1); + assert!(matches!(monitor_updates_1[0].updates[0], ChannelMonitorUpdateStep::ChannelForceClosed { .. })); let monitor_updates_2 = monitor_updates.get(&channel_id_2).unwrap(); assert_eq!(monitor_updates_2.len(), 1); - assert_eq!(monitor_updates_2[0].update_id, CLOSED_CHANNEL_UPDATE_ID); + assert_eq!(monitor_updates_2[0].updates.len(), 1); + assert!(matches!(monitor_updates_2[0].updates[0], ChannelMonitorUpdateStep::ChannelForceClosed { .. })); } let msg_events = nodes[0].node.get_and_clear_pending_msg_events(); match msg_events[0] { diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index 4aad8f569fc..99ad31d3583 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -2302,9 +2302,6 @@ fn do_test_restored_packages_retry(check_old_monitor_retries_after_upgrade: bool // Connecting more blocks should result in the HTLC transactions being rebroadcast. connect_blocks(&nodes[0], crate::chain::package::LOW_FREQUENCY_BUMP_INTERVAL); - if check_old_monitor_retries_after_upgrade { - check_added_monitors(&nodes[0], 1); - } { let txn = nodes[0].tx_broadcaster.txn_broadcast(); assert_eq!(txn.len(), 1); @@ -3014,7 +3011,6 @@ fn do_test_anchors_monitor_fixes_counterparty_payment_script_on_reload(confirm_c // If we saw the commitment before our `counterparty_payment_script` was fixed, we'll never // get the spendable output event for the `to_remote` output, so we'll need to get it // manually via `get_spendable_outputs`. - check_added_monitors(&nodes[1], 1); let outputs = get_monitor!(nodes[1], chan_id).get_spendable_outputs(&commitment_tx, commitment_tx_conf_height); assert_eq!(outputs.len(), 1); let spend_tx = nodes[1].keys_manager.backing.spend_spendable_outputs( diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 7839b49be77..46a6e302655 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -993,7 +993,6 @@ fn do_test_completed_payment_not_retryable_on_reload(use_dust: bool) { nodes[1].node.peer_disconnected(nodes[0].node.get_our_node_id()); nodes[0].node.test_process_background_events(); - check_added_monitors(&nodes[0], 1); let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]); reconnect_args.send_channel_ready = (true, true); @@ -1023,7 +1022,6 @@ fn do_test_completed_payment_not_retryable_on_reload(use_dust: bool) { nodes[1].node.peer_disconnected(nodes[0].node.get_our_node_id()); nodes[0].node.test_process_background_events(); - check_added_monitors(&nodes[0], 1); reconnect_nodes(ReconnectArgs::new(&nodes[0], &nodes[1])); @@ -1162,7 +1160,6 @@ fn do_test_dup_htlc_onchain_doesnt_fail_on_reload(persist_manager_post_event: bo let height = nodes[0].blocks.lock().unwrap().len() as u32 - 1; nodes[0].chain_monitor.chain_monitor.block_connected(&claim_block, height); assert!(nodes[0].node.get_and_clear_pending_events().is_empty()); - check_added_monitors(&nodes[0], 1); } #[test] @@ -3522,7 +3519,6 @@ fn do_no_missing_sent_on_reload(persist_manager_with_payment: bool, at_midpoint: reload_node!(nodes[0], test_default_channel_config(), &nodes[0].node.encode(), &[&chan_0_monitor_serialized], persister_c, chain_monitor_c, nodes_0_deserialized_c); let events = nodes[0].node.get_and_clear_pending_events(); assert!(events.is_empty()); - check_added_monitors(&nodes[0], 1); } #[test] diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs index c32fca6bd75..d614737a05e 100644 --- a/lightning/src/ln/reload_tests.rs +++ b/lightning/src/ln/reload_tests.rs @@ -11,7 +11,7 @@ use crate::chain::{ChannelMonitorUpdateStatus, Watch}; use crate::chain::chaininterface::LowerBoundedFeeEstimator; -use crate::chain::channelmonitor::{CLOSED_CHANNEL_UPDATE_ID, ChannelMonitor}; +use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdateStep}; use crate::sign::EntropySource; use crate::chain::transaction::OutPoint; use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider}; @@ -1264,7 +1264,8 @@ fn test_reload_partial_funding_batch() { let mut monitor_updates = nodes[0].chain_monitor.monitor_updates.lock().unwrap(); let monitor_updates_1 = monitor_updates.get(&channel_id_1).unwrap(); assert_eq!(monitor_updates_1.len(), 1); - assert_eq!(monitor_updates_1[0].update_id, CLOSED_CHANNEL_UPDATE_ID); + assert_eq!(monitor_updates_1[0].updates.len(), 1); + assert!(matches!(monitor_updates_1[0].updates[0], ChannelMonitorUpdateStep::ChannelForceClosed { .. })); } // The funding transaction should not have been broadcast, but we broadcast the force-close diff --git a/lightning/src/sync/debug_sync.rs b/lightning/src/sync/debug_sync.rs index 776e35e8ce0..183c074701e 100644 --- a/lightning/src/sync/debug_sync.rs +++ b/lightning/src/sync/debug_sync.rs @@ -311,6 +311,10 @@ impl Mutex { } res } + + pub fn get_mut<'a>(&'a mut self) -> LockResult<&'a mut T> { + self.inner.get_mut().map_err(|_| ()) + } } impl<'a, T: 'a> LockTestExt<'a> for Mutex { diff --git a/lightning/src/sync/nostd_sync.rs b/lightning/src/sync/nostd_sync.rs index 03fa65b69c6..b3963da762e 100644 --- a/lightning/src/sync/nostd_sync.rs +++ b/lightning/src/sync/nostd_sync.rs @@ -40,6 +40,10 @@ impl Mutex { pub fn into_inner(self) -> LockResult { Ok(self.inner.into_inner()) } + + pub fn get_mut<'a>(&'a mut self) -> LockResult<&'a mut T> { + Ok(self.inner.get_mut()) + } } impl<'a, T: 'a> LockTestExt<'a> for Mutex { diff --git a/lightning/src/util/persist.rs b/lightning/src/util/persist.rs index 8a385eda9a6..732b549555e 100644 --- a/lightning/src/util/persist.rs +++ b/lightning/src/util/persist.rs @@ -21,9 +21,7 @@ use crate::{io, log_error}; use crate::chain; use crate::chain::chaininterface::{BroadcasterInterface, FeeEstimator}; use crate::chain::chainmonitor::Persist; -use crate::chain::channelmonitor::{ - ChannelMonitor, ChannelMonitorUpdate, CLOSED_CHANNEL_UPDATE_ID, -}; +use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate}; use crate::chain::transaction::OutPoint; use crate::ln::channelmanager::AChannelManager; use crate::routing::gossip::NetworkGraph; @@ -728,16 +726,17 @@ where /// - No full monitor is found in [`KVStore`] /// - The number of pending updates exceeds `maximum_pending_updates` as given to [`Self::new`] /// - LDK commands re-persisting the entire monitor through this function, specifically when - /// `update` is `None`. - /// - The update is at [`CLOSED_CHANNEL_UPDATE_ID`] + /// `update` is `None`. + /// - The update is at [`u64::MAX`], indicating an update generated by pre-0.1 LDK. fn update_persisted_channel( &self, funding_txo: OutPoint, update: Option<&ChannelMonitorUpdate>, monitor: &ChannelMonitor, ) -> chain::ChannelMonitorUpdateStatus { + const LEGACY_CLOSED_CHANNEL_UPDATE_ID: u64 = u64::MAX; if let Some(update) = update { - if update.update_id != CLOSED_CHANNEL_UPDATE_ID - && update.update_id % self.maximum_pending_updates != 0 - { + let persist_update = update.update_id != LEGACY_CLOSED_CHANNEL_UPDATE_ID + && update.update_id % self.maximum_pending_updates != 0; + if persist_update { let monitor_name = MonitorName::from(funding_txo); let update_name = UpdateName::from(update.update_id); match self.kv_store.write( @@ -764,7 +763,7 @@ where // In case of channel-close monitor update, we need to read old monitor before persisting // the new one in order to determine the cleanup range. let maybe_old_monitor = match monitor.get_latest_update_id() { - CLOSED_CHANNEL_UPDATE_ID => self.read_monitor(&monitor_name).ok(), + LEGACY_CLOSED_CHANNEL_UPDATE_ID => self.read_monitor(&monitor_name).ok(), _ => None, }; @@ -772,23 +771,24 @@ where let monitor_update_status = self.persist_new_channel(funding_txo, monitor); if let chain::ChannelMonitorUpdateStatus::Completed = monitor_update_status { - let cleanup_range = - if monitor.get_latest_update_id() == CLOSED_CHANNEL_UPDATE_ID { - // If there is an error while reading old monitor, we skip clean up. - maybe_old_monitor.map(|(_, ref old_monitor)| { - let start = old_monitor.get_latest_update_id(); - // We never persist an update with update_id = CLOSED_CHANNEL_UPDATE_ID - let end = cmp::min( - start.saturating_add(self.maximum_pending_updates), - CLOSED_CHANNEL_UPDATE_ID - 1, - ); - (start, end) - }) - } else { - let end = monitor.get_latest_update_id(); - let start = end.saturating_sub(self.maximum_pending_updates); - Some((start, end)) - }; + let channel_closed_legacy = + monitor.get_latest_update_id() == LEGACY_CLOSED_CHANNEL_UPDATE_ID; + let cleanup_range = if channel_closed_legacy { + // If there is an error while reading old monitor, we skip clean up. + maybe_old_monitor.map(|(_, ref old_monitor)| { + let start = old_monitor.get_latest_update_id(); + // We never persist an update with the legacy closed update_id + let end = cmp::min( + start.saturating_add(self.maximum_pending_updates), + LEGACY_CLOSED_CHANNEL_UPDATE_ID - 1, + ); + (start, end) + }) + } else { + let end = monitor.get_latest_update_id(); + let start = end.saturating_sub(self.maximum_pending_updates); + Some((start, end)) + }; if let Some((start, end)) = cleanup_range { self.cleanup_in_range(monitor_name, start, end); @@ -1185,24 +1185,19 @@ mod tests { // check that when we read it, we got the right update id assert_eq!(mon.get_latest_update_id(), $expected_update_id); - // if the CM is at consolidation threshold, ensure no updates are stored. let monitor_name = MonitorName::from(mon.get_funding_txo().0); - if mon.get_latest_update_id() % persister_0_max_pending_updates == 0 - || mon.get_latest_update_id() == CLOSED_CHANNEL_UPDATE_ID - { - assert_eq!( - persister_0 - .kv_store - .list( - CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE, - monitor_name.as_str() - ) - .unwrap() - .len(), - 0, - "updates stored when they shouldn't be in persister 0" - ); - } + assert_eq!( + persister_0 + .kv_store + .list( + CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE, + monitor_name.as_str() + ) + .unwrap() + .len() as u64, + mon.get_latest_update_id() % persister_0_max_pending_updates, + "Wrong number of updates stored in persister 0", + ); } persisted_chan_data_1 = persister_1.read_all_channel_monitors_with_updates().unwrap(); @@ -1210,23 +1205,18 @@ mod tests { for (_, mon) in persisted_chan_data_1.iter() { assert_eq!(mon.get_latest_update_id(), $expected_update_id); let monitor_name = MonitorName::from(mon.get_funding_txo().0); - // if the CM is at consolidation threshold, ensure no updates are stored. - if mon.get_latest_update_id() % persister_1_max_pending_updates == 0 - || mon.get_latest_update_id() == CLOSED_CHANNEL_UPDATE_ID - { - assert_eq!( - persister_1 - .kv_store - .list( - CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE, - monitor_name.as_str() - ) - .unwrap() - .len(), - 0, - "updates stored when they shouldn't be in persister 1" - ); - } + assert_eq!( + persister_1 + .kv_store + .list( + CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE, + monitor_name.as_str() + ) + .unwrap() + .len() as u64, + mon.get_latest_update_id() % persister_1_max_pending_updates, + "Wrong number of updates stored in persister 1", + ); } }; } @@ -1283,28 +1273,8 @@ mod tests { check_added_monitors!(nodes[1], 1); // Make sure everything is persisted as expected after close. - check_persisted_data!(CLOSED_CHANNEL_UPDATE_ID); - - // Make sure the expected number of stale updates is present. - let persisted_chan_data = persister_0.read_all_channel_monitors_with_updates().unwrap(); - let (_, monitor) = &persisted_chan_data[0]; - let monitor_name = MonitorName::from(monitor.get_funding_txo().0); - // The channel should have 0 updates, as it wrote a full monitor and consolidated. - assert_eq!( - persister_0 - .kv_store - .list(CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE, monitor_name.as_str()) - .unwrap() - .len(), - 0 - ); - assert_eq!( - persister_1 - .kv_store - .list(CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE, monitor_name.as_str()) - .unwrap() - .len(), - 0 + check_persisted_data!( + persister_0_max_pending_updates * 2 * EXPECTED_UPDATES_PER_PAYMENT + 1 ); } @@ -1452,40 +1422,6 @@ mod tests { UpdateName::from(1).as_str() ) .is_err()); - - // Force close. - let chan_id = nodes[0].node.list_channels()[0].channel_id; - let node_id_1 = nodes[1].node.get_our_node_id(); - let err_msg = "Channel force-closed".to_string(); - nodes[0].node.force_close_broadcasting_latest_txn(&chan_id, &node_id_1, err_msg).unwrap(); - let reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) }; - check_closed_event(&nodes[0], 1, reason, false, &[node_id_1], 100000); - check_closed_broadcast!(nodes[0], true); - check_added_monitors!(nodes[0], 1); - - // Write an update near u64::MAX - persister_0 - .kv_store - .write( - CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE, - monitor_name.as_str(), - UpdateName::from(u64::MAX - 1).as_str(), - &[0u8; 1], - ) - .unwrap(); - - // Do the stale update cleanup - persister_0.cleanup_stale_updates(false).unwrap(); - - // Confirm the stale update is unreadable/gone - assert!(persister_0 - .kv_store - .read( - CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE, - monitor_name.as_str(), - UpdateName::from(u64::MAX - 1).as_str() - ) - .is_err()); } fn persist_fn(_persist: P) -> bool diff --git a/pending_changelog/matt-no-upgrade-skip.txt b/pending_changelog/matt-no-upgrade-skip.txt new file mode 100644 index 00000000000..f5fcb8c5f25 --- /dev/null +++ b/pending_changelog/matt-no-upgrade-skip.txt @@ -0,0 +1,6 @@ +## Backwards Compatibility + * Nodes with pending forwarded HTLCs or unclaimed payments cannot be + upgraded directly from 0.0.123 or earlier to 0.1. Instead, they must + first either resolve all pending HTLCs (including those pending + resolution on-chain), or run 0.0.124 and resolve any HTLCs that were + originally forwarded or received running 0.0.123 or earlier.