From 5a90f014f283f20e783c5db32ffb63c019a8dbe6 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 28 Feb 2023 10:45:44 -0800 Subject: [PATCH 1/6] Use CLOSED_CHANNEL_UPDATE_ID in force closing ChannelMonitorUpdates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, all that is required to force close a channel is to broadcast either of the available commitment transactions, but this changes with anchor outputs – commitment transactions may need to have additional fees attached in order to confirm in a timely manner. While we may be able to just queue a new update using the channel's next available update ID, this may result in a violation of the `ChannelMonitor` API (each update ID must strictly increase by 1) if the channel had updates that were persisted by its `ChannelMonitor`, but not the `ChannelManager`. Therefore, we choose to re-purpose the existing `CLOSED_CHANNEL_UPDATE_ID` update ID to also apply to `ChannelMonitorUpdate`s that will force close their respective channel by broadcasting the holder's latest commitment transaction. --- lightning-persister/src/lib.rs | 3 ++- lightning/src/chain/channelmonitor.rs | 31 ++++++++++++++++----------- lightning/src/ln/channel.rs | 4 ++-- 3 files changed, 23 insertions(+), 15 deletions(-) diff --git a/lightning-persister/src/lib.rs b/lightning-persister/src/lib.rs index 488378576e5..01cb4081ccc 100644 --- a/lightning-persister/src/lib.rs +++ b/lightning-persister/src/lib.rs @@ -141,6 +141,7 @@ mod tests { use bitcoin::{Txid, TxMerkleNode}; use lightning::chain::ChannelMonitorUpdateStatus; use lightning::chain::chainmonitor::Persist; + use lightning::chain::channelmonitor::CLOSED_CHANNEL_UPDATE_ID; use lightning::chain::transaction::OutPoint; use lightning::{check_closed_broadcast, check_closed_event, check_added_monitors}; use lightning::ln::functional_test_utils::*; @@ -253,7 +254,7 @@ mod tests { check_added_monitors!(nodes[1], 1); // Make sure everything is persisted as expected after close. - check_persisted_data!(11); + check_persisted_data!(CLOSED_CHANNEL_UPDATE_ID); } // Test that if the persister's path to channel data is read-only, writing a diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 1ebb65534b3..cd6b4c941a9 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -76,27 +76,30 @@ pub struct ChannelMonitorUpdate { pub(crate) updates: Vec, /// The sequence number of this update. Updates *must* be replayed in-order according to this /// sequence number (and updates may panic if they are not). The update_id values are strictly - /// increasing and increase by one for each new update, with one exception specified below. + /// increasing and increase by one for each new update, with two exceptions specified below. /// /// This sequence number is also used to track up to which points updates which returned /// [`ChannelMonitorUpdateStatus::InProgress`] have been applied to all copies of a given /// ChannelMonitor when ChannelManager::channel_monitor_updated is called. /// - /// The only instance where update_id values are not strictly increasing is the case where we - /// allow post-force-close updates with a special update ID of [`CLOSED_CHANNEL_UPDATE_ID`]. See - /// its docs for more details. + /// 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. /// /// [`ChannelMonitorUpdateStatus::InProgress`]: super::ChannelMonitorUpdateStatus::InProgress pub update_id: u64, } -/// If: -/// (1) a channel has been force closed and -/// (2) we receive a preimage from a forward link that allows us to spend an HTLC output on -/// this channel's (the backward link's) broadcasted commitment transaction -/// then we allow the `ChannelManager` to send a `ChannelMonitorUpdate` with this update ID, -/// with the update providing said payment preimage. No other update types are allowed after -/// force-close. +/// 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; impl Writeable for ChannelMonitorUpdate { @@ -2272,7 +2275,11 @@ impl ChannelMonitorImpl { if updates.update_id == CLOSED_CHANNEL_UPDATE_ID { assert_eq!(updates.updates.len(), 1); match updates.updates[0] { - ChannelMonitorUpdateStep::PaymentPreimage { .. } => {}, + 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), _ => { 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"); diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 7a389b573e6..ae45f5e8fb6 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -33,7 +33,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}; +use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, LATENCY_GRACE_PERIOD_BLOCKS, CLOSED_CHANNEL_UPDATE_ID}; use crate::chain::transaction::{OutPoint, TransactionData}; use crate::chain::keysinterface::{WriteableEcdsaChannelSigner, EntropySource, ChannelSigner, SignerProvider, NodeSigner, Recipient}; use crate::routing::gossip::NodeId; @@ -6081,7 +6081,7 @@ impl Channel { // 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 & (ChannelState::FundingSent as u32 | ChannelState::ChannelReady as u32 | ChannelState::ShutdownComplete as u32) != 0 { - self.latest_monitor_update_id += 1; + self.latest_monitor_update_id = CLOSED_CHANNEL_UPDATE_ID; Some((funding_txo, ChannelMonitorUpdate { update_id: self.latest_monitor_update_id, updates: vec![ChannelMonitorUpdateStep::ChannelForceClosed { should_broadcast }], From bd4eb0da76207a68396de8b36fb640467255cc31 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 28 Feb 2023 10:45:48 -0800 Subject: [PATCH 2/6] Queue BackgroundEvent to force close channels upon ChannelManager::read This results in a new, potentially redundant, `ChannelMonitorUpdate` that must be applied to `ChannelMonitor`s to broadcast the holder's latest commitment transaction. This is a behavior change for anchor channels since their commitments may require additional fees to be attached through a child anchor transaction. Recall that anchor transactions are only generated by the event consumer after processing a `BumpTransactionEvent::ChannelClose` event, which is yielded after applying a `ChannelMonitorUpdateStep::ChannelForceClosed` monitor update. Assuming the node operator is not watching the mempool to generate these anchor transactions without LDK, an anchor channel which we had to fail when deserializing our `ChannelManager` would have its commitment transaction broadcast by itself, potentially exposing the node operator to loss of funds if the commitment transaction's fee is not enough to be accepted into the network's mempools. --- lightning/src/chain/channelmonitor.rs | 18 +++++---- lightning/src/ln/channelmanager.rs | 29 +++++++++++---- lightning/src/ln/monitor_tests.rs | 53 +++++++++++++-------------- lightning/src/ln/payment_tests.rs | 15 ++++++-- lightning/src/ln/reload_tests.rs | 18 +++++---- lightning/src/ln/reorg_tests.rs | 7 +--- 6 files changed, 81 insertions(+), 59 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index cd6b4c941a9..df5160df015 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -69,8 +69,7 @@ use crate::sync::{Mutex, LockTestExt}; /// much smaller than a full [`ChannelMonitor`]. However, for large single commitment transaction /// updates (e.g. ones during which there are hundreds of HTLCs pending on the commitment /// transaction), a single update may reach upwards of 1 MiB in serialized size. -#[cfg_attr(any(test, fuzzing, feature = "_test_utils"), derive(PartialEq, Eq))] -#[derive(Clone)] +#[derive(Clone, PartialEq, Eq)] #[must_use] pub struct ChannelMonitorUpdate { pub(crate) updates: Vec, @@ -491,8 +490,7 @@ impl_writeable_tlv_based_enum_upgradable!(OnchainEvent, ); -#[cfg_attr(any(test, fuzzing, feature = "_test_utils"), derive(PartialEq, Eq))] -#[derive(Clone)] +#[derive(Clone, PartialEq, Eq)] pub(crate) enum ChannelMonitorUpdateStep { LatestHolderCommitmentTXInfo { commitment_tx: HolderCommitmentTransaction, @@ -2268,10 +2266,14 @@ impl ChannelMonitorImpl { { log_info!(logger, "Applying update to monitor {}, bringing update_id from {} to {} with {} changes.", log_funding_info!(self), self.latest_update_id, updates.update_id, updates.updates.len()); - // ChannelMonitor updates may be applied after force close if we receive a - // preimage for a broadcasted commitment transaction HTLC output that we'd - // like to claim on-chain. If this is the case, we no longer have guaranteed - // access to the monitor's update ID, so we use a sentinel value instead. + // ChannelMonitor updates may be applied after force close if we receive a preimage for a + // broadcasted commitment transaction HTLC output that we'd like to claim on-chain. If this + // is the case, we no longer have guaranteed access to the monitor's update ID, so we use a + // sentinel value instead. + // + // 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 { assert_eq!(updates.updates.len(), 1); match updates.updates[0] { diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 90afc0169f6..2c62cc74d69 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -7310,6 +7310,7 @@ where let mut id_to_peer = HashMap::with_capacity(cmp::min(channel_count as usize, 128)); let mut short_to_chan_info = HashMap::with_capacity(cmp::min(channel_count as usize, 128)); let mut channel_closures = Vec::new(); + let mut pending_background_events = Vec::new(); for _ in 0..channel_count { let mut channel: Channel<::Signer> = Channel::read(reader, ( &args.entropy_source, &args.signer_provider, best_block_height, &provided_channel_type_features(&args.default_config) @@ -7339,9 +7340,11 @@ where log_error!(args.logger, " The channel will be force-closed and the latest commitment transaction from the ChannelMonitor broadcast."); log_error!(args.logger, " The ChannelMonitor for channel {} is at update_id {} but the ChannelManager is at update_id {}.", log_bytes!(channel.channel_id()), monitor.get_latest_update_id(), channel.get_latest_monitor_update_id()); - let (_, mut new_failed_htlcs) = channel.force_shutdown(true); + let (monitor_update, mut new_failed_htlcs) = channel.force_shutdown(true); + if let Some(monitor_update) = monitor_update { + pending_background_events.push(BackgroundEvent::ClosingMonitorUpdate(monitor_update)); + } failed_htlcs.append(&mut new_failed_htlcs); - monitor.broadcast_latest_holder_commitment_txn(&args.tx_broadcaster, &args.logger); channel_closures.push(events::Event::ChannelClosed { channel_id: channel.channel_id(), user_channel_id: channel.get_user_id(), @@ -7406,10 +7409,13 @@ where } } - for (funding_txo, monitor) in args.channel_monitors.iter_mut() { + for (funding_txo, _) in args.channel_monitors.iter() { if !funding_txo_set.contains(funding_txo) { - log_info!(args.logger, "Broadcasting latest holder commitment transaction for closed channel {}", log_bytes!(funding_txo.to_channel_id())); - monitor.broadcast_latest_holder_commitment_txn(&args.tx_broadcaster, &args.logger); + let monitor_update = ChannelMonitorUpdate { + update_id: CLOSED_CHANNEL_UPDATE_ID, + updates: vec![ChannelMonitorUpdateStep::ChannelForceClosed { should_broadcast: true }], + }; + pending_background_events.push(BackgroundEvent::ClosingMonitorUpdate((*funding_txo, monitor_update))); } } @@ -7462,10 +7468,17 @@ where } let background_event_count: u64 = Readable::read(reader)?; - let mut pending_background_events_read: Vec = Vec::with_capacity(cmp::min(background_event_count as usize, MAX_ALLOC_SIZE/mem::size_of::())); for _ in 0..background_event_count { match ::read(reader)? { - 0 => pending_background_events_read.push(BackgroundEvent::ClosingMonitorUpdate((Readable::read(reader)?, Readable::read(reader)?))), + 0 => { + let (funding_txo, monitor_update): (OutPoint, ChannelMonitorUpdate) = (Readable::read(reader)?, Readable::read(reader)?); + if pending_background_events.iter().find(|e| { + let BackgroundEvent::ClosingMonitorUpdate((pending_funding_txo, pending_monitor_update)) = e; + *pending_funding_txo == funding_txo && *pending_monitor_update == monitor_update + }).is_none() { + pending_background_events.push(BackgroundEvent::ClosingMonitorUpdate((funding_txo, monitor_update))); + } + } _ => return Err(DecodeError::InvalidValue), } } @@ -7840,7 +7853,7 @@ where per_peer_state: FairRwLock::new(per_peer_state), pending_events: Mutex::new(pending_events_read), - pending_background_events: Mutex::new(pending_background_events_read), + pending_background_events: Mutex::new(pending_background_events), total_consistency_lock: RwLock::new(()), persistence_notifier: Notifier::new(), diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index 3bd50293ff2..7ed99682005 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -1859,15 +1859,18 @@ fn test_anchors_aggregated_revoked_htlc_tx() { let chan_a = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 20_000_000); let chan_b = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 20_000_000); + // Serialize Bob with the initial state of both channels, which we'll use later. + let bob_serialized = nodes[1].node.encode(); + // Route two payments for each channel from Alice to Bob to lock in the HTLCs. let payment_a = route_payment(&nodes[0], &[&nodes[1]], 50_000_000); let payment_b = route_payment(&nodes[0], &[&nodes[1]], 50_000_000); let payment_c = route_payment(&nodes[0], &[&nodes[1]], 50_000_000); let payment_d = route_payment(&nodes[0], &[&nodes[1]], 50_000_000); - // Serialize Bob with the HTLCs locked in. We'll restart Bob later on with the state at this - // point such that he broadcasts a revoked commitment transaction. - let bob_serialized = nodes[1].node.encode(); + // Serialize Bob's monitors with the HTLCs locked in. We'll restart Bob later on with the state + // at this point such that he broadcasts a revoked commitment transaction with the HTLCs + // present. let bob_serialized_monitor_a = get_monitor!(nodes[1], chan_a.2).encode(); let bob_serialized_monitor_b = get_monitor!(nodes[1], chan_b.2).encode(); @@ -1897,30 +1900,26 @@ fn test_anchors_aggregated_revoked_htlc_tx() { } } - // Bob force closes by broadcasting his revoked state for each channel. - nodes[1].node.force_close_broadcasting_latest_txn(&chan_a.2, &nodes[0].node.get_our_node_id()).unwrap(); - check_added_monitors(&nodes[1], 1); - check_closed_broadcast(&nodes[1], 1, true); - check_closed_event!(&nodes[1], 1, ClosureReason::HolderForceClosed); - let revoked_commitment_a = { - let mut txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); - assert_eq!(txn.len(), 1); - let revoked_commitment = txn.pop().unwrap(); - assert_eq!(revoked_commitment.output.len(), 6); // 2 HTLC outputs + 1 to_self output + 1 to_remote output + 2 anchor outputs - check_spends!(revoked_commitment, chan_a.3); - revoked_commitment - }; - nodes[1].node.force_close_broadcasting_latest_txn(&chan_b.2, &nodes[0].node.get_our_node_id()).unwrap(); - check_added_monitors(&nodes[1], 1); - check_closed_broadcast(&nodes[1], 1, true); - check_closed_event!(&nodes[1], 1, ClosureReason::HolderForceClosed); - let revoked_commitment_b = { - let mut txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); - assert_eq!(txn.len(), 1); - let revoked_commitment = txn.pop().unwrap(); - assert_eq!(revoked_commitment.output.len(), 6); // 2 HTLC outputs + 1 to_self output + 1 to_remote output + 2 anchor outputs - check_spends!(revoked_commitment, chan_b.3); - revoked_commitment + // Bob force closes by restarting with the outdated state, prompting the ChannelMonitors to + // broadcast the latest commitment transaction known to them, which in our case is the one with + // the HTLCs still pending. + nodes[1].node.timer_tick_occurred(); + check_added_monitors(&nodes[1], 2); + check_closed_event!(&nodes[1], 2, ClosureReason::OutdatedChannelManager); + let (revoked_commitment_a, revoked_commitment_b) = { + let txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + assert_eq!(txn.len(), 2); + assert_eq!(txn[0].output.len(), 6); // 2 HTLC outputs + 1 to_self output + 1 to_remote output + 2 anchor outputs + assert_eq!(txn[1].output.len(), 6); // 2 HTLC outputs + 1 to_self output + 1 to_remote output + 2 anchor outputs + if txn[0].input[0].previous_output.txid == chan_a.3.txid() { + check_spends!(&txn[0], &chan_a.3); + check_spends!(&txn[1], &chan_b.3); + (txn[0].clone(), txn[1].clone()) + } else { + check_spends!(&txn[1], &chan_a.3); + check_spends!(&txn[0], &chan_b.3); + (txn[1].clone(), txn[0].clone()) + } }; // Bob should now receive two events to bump his revoked commitment transaction fees. diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 15361b98ad7..8d5c4b54b7b 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -334,9 +334,15 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) { check_closed_event!(nodes[0], 1, ClosureReason::OutdatedChannelManager); assert!(nodes[0].node.list_channels().is_empty()); assert!(nodes[0].node.has_pending_payments()); - let as_broadcasted_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); - assert_eq!(as_broadcasted_txn.len(), 1); - assert_eq!(as_broadcasted_txn[0], as_commitment_tx); + nodes[0].node.timer_tick_occurred(); + if !confirm_before_reload { + let as_broadcasted_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + assert_eq!(as_broadcasted_txn.len(), 1); + assert_eq!(as_broadcasted_txn[0], as_commitment_tx); + } else { + assert!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty()); + } + check_added_monitors!(nodes[0], 1); nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id()); nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: nodes[1].node.init_features(), remote_network_address: None }, true).unwrap(); @@ -499,9 +505,11 @@ fn do_test_completed_payment_not_retryable_on_reload(use_dust: bool) { // On reload, the ChannelManager should realize it is stale compared to the ChannelMonitor and // force-close the channel. check_closed_event!(nodes[0], 1, ClosureReason::OutdatedChannelManager); + nodes[0].node.timer_tick_occurred(); assert!(nodes[0].node.list_channels().is_empty()); assert!(nodes[0].node.has_pending_payments()); assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0).len(), 1); + check_added_monitors!(nodes[0], 1); nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: nodes[1].node.init_features(), remote_network_address: None }, true).unwrap(); assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); @@ -2794,6 +2802,7 @@ fn do_no_missing_sent_on_midpoint_reload(persist_manager_with_payment: bool) { if let Event::PaymentSent { payment_preimage, .. } = events[1] { assert_eq!(payment_preimage, our_payment_preimage); } else { panic!(); } // Note that we don't get a PaymentPathSuccessful here as we leave the HTLC pending to avoid // the double-claim that would otherwise appear at the end of this test. + nodes[0].node.timer_tick_occurred(); let as_broadcasted_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); assert_eq!(as_broadcasted_txn.len(), 1); diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs index 14deefd3f93..70570c68b46 100644 --- a/lightning/src/ln/reload_tests.rs +++ b/lightning/src/ln/reload_tests.rs @@ -422,20 +422,22 @@ fn test_manager_serialize_deserialize_inconsistent_monitor() { nodes_0_deserialized = nodes_0_deserialized_tmp; assert!(nodes_0_read.is_empty()); - { // Channel close should result in a commitment tx - let txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap(); - assert_eq!(txn.len(), 1); - check_spends!(txn[0], funding_tx); - assert_eq!(txn[0].input[0].previous_output.txid, funding_tx.txid()); - } - for monitor in node_0_monitors.drain(..) { assert_eq!(nodes[0].chain_monitor.watch_channel(monitor.get_funding_txo().0, monitor), ChannelMonitorUpdateStatus::Completed); check_added_monitors!(nodes[0], 1); } nodes[0].node = &nodes_0_deserialized; + check_closed_event!(nodes[0], 1, ClosureReason::OutdatedChannelManager); + { // Channel close should result in a commitment tx + nodes[0].node.timer_tick_occurred(); + let txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap(); + assert_eq!(txn.len(), 1); + check_spends!(txn[0], funding_tx); + assert_eq!(txn[0].input[0].previous_output.txid, funding_tx.txid()); + } + check_added_monitors!(nodes[0], 1); // nodes[1] and nodes[2] have no lost state with nodes[0]... reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (0, 0), (false, false)); @@ -920,8 +922,10 @@ fn do_forwarded_payment_no_manager_persistence(use_cs_commitment: bool, claim_ht }); } + nodes[1].node.timer_tick_occurred(); let bs_commitment_tx = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); assert_eq!(bs_commitment_tx.len(), 1); + check_added_monitors!(nodes[1], 1); nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id()); reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (0, 0), (false, false)); diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs index 69e0c9afa9e..c22458830e3 100644 --- a/lightning/src/ln/reorg_tests.rs +++ b/lightning/src/ln/reorg_tests.rs @@ -320,12 +320,7 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_ let chan_0_monitor_serialized = get_monitor!(nodes[0], chan.2).encode(); reload_node!(nodes[0], *nodes[0].node.get_current_default_configuration(), &nodes_0_serialized, &[&chan_0_monitor_serialized], persister, new_chain_monitor, nodes_0_deserialized); - if !reorg_after_reload { - // If the channel is already closed when we reload the node, we'll broadcast a closing - // transaction via the ChannelMonitor which is missing a corresponding channel. - assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().len(), 1); - nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().clear(); - } + assert!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty()); } if reorg_after_reload { From 00cfc6b82300541ace7eee4625267799183513e8 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 28 Feb 2023 10:45:48 -0800 Subject: [PATCH 3/6] Avoid refusing ChannelMonitorUpdates we expect to receive after closing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is no need to fill the user's logs with errors that are expected to be hit based on specific edge cases, like providing preimages after a monitor has seen a confirmed commitment on-chain. This doesn't really change our behavior – we still apply and persist the state changes resulting from processing these updates regardless of whether they succeed or not. --- lightning/src/chain/channelmonitor.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index df5160df015..97e1e8ee612 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -2373,6 +2373,13 @@ impl ChannelMonitorImpl { }, } } + + // 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; if ret.is_ok() && self.funding_spend_seen { From 04ee9486434f23ab73d60f5be06bfea426db0060 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 28 Feb 2023 10:45:49 -0800 Subject: [PATCH 4/6] Remove unused broadcast_latest_holder_commitment_txn method --- lightning/src/chain/channelmonitor.rs | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 97e1e8ee612..4e6c67982f7 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1202,17 +1202,6 @@ impl ChannelMonitor { payment_hash, payment_preimage, broadcaster, fee_estimator, logger) } - pub(crate) fn broadcast_latest_holder_commitment_txn( - &self, - broadcaster: &B, - logger: &L, - ) where - B::Target: BroadcasterInterface, - L::Target: Logger, - { - self.inner.lock().unwrap().broadcast_latest_holder_commitment_txn(broadcaster, logger); - } - /// Updates a ChannelMonitor on the basis of some new information provided by the Channel /// itself. /// From 2166c8a2c46f9a8c50c3dd909fc47a9558a8355b Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Wed, 22 Mar 2023 11:46:05 -0700 Subject: [PATCH 5/6] Ignore lockorder violation on same callsite with different construction As long as the lock order on such locks is still valid, we should allow them regardless of whether they were constructed at the same location or not. Note that we can only really enforce this if we require one lock call per line, or if we have access to symbol columns (as we do on Linux and macOS). We opt for a smaller patch by relying on the latter. This was previously triggered by some recent test changes to `test_manager_serialize_deserialize_inconsistent_monitor`. When the test ends and a node is dropped causing us to persist each, we'd detect a possible lockorder violation deadlock across three different `Mutex` instances that are held at the same location when serializing our `per_peer_states` in `ChannelManager::write`. The presumed lockorder violation happens because the first `Mutex` held shares the same construction location with the third one, while the second `Mutex` has a different construction location. When we hold the second one, we consider the first as a dependency, and then consider the second as a dependency when holding the third, causing a circular dependency (since the third shares the same construction location as the first). This isn't considered a lockorder violation that could result in a deadlock as the comment suggests inline though, since we are under a dependent write lock which no one else can have access to. --- lightning/src/sync/debug_sync.rs | 75 ++++++++++++++++++++------------ 1 file changed, 46 insertions(+), 29 deletions(-) diff --git a/lightning/src/sync/debug_sync.rs b/lightning/src/sync/debug_sync.rs index 11824d5bc73..11557be82af 100644 --- a/lightning/src/sync/debug_sync.rs +++ b/lightning/src/sync/debug_sync.rs @@ -74,24 +74,31 @@ struct LockDep { _lockdep_trace: Backtrace, } +// Locates the frame preceding the earliest `debug_sync` frame in the call stack. This ensures we +// can properly detect a lock's construction and acquiral callsites, since the latter may contain +// multiple `debug_sync` frames. #[cfg(feature = "backtrace")] -fn get_construction_location(backtrace: &Backtrace) -> (String, Option) { - // Find the first frame that is after `debug_sync` (or that is in our tests) and use - // that as the mutex construction site. Note that the first few frames may be in - // the `backtrace` crate, so we have to ignore those. +fn locate_call_symbol(backtrace: &Backtrace) -> (String, Option) { + // Find the earliest `debug_sync` frame (or that is in our tests) and use the frame preceding it + // as the callsite. Note that the first few frames may be in the `backtrace` crate, so we have + // to ignore those. let sync_mutex_constr_regex = regex::Regex::new(r"lightning.*debug_sync").unwrap(); let mut found_debug_sync = false; - for frame in backtrace.frames() { - for symbol in frame.symbols() { - let symbol_name = symbol.name().unwrap().as_str().unwrap(); - if !sync_mutex_constr_regex.is_match(symbol_name) { - if found_debug_sync { - return (format!("{}:{}", symbol.filename().unwrap().display(), symbol.lineno().unwrap()), symbol.colno()); - } - } else { found_debug_sync = true; } + let mut symbol_after_latest_debug_sync = None; + for frame in backtrace.frames().iter() { + for symbol in frame.symbols().iter() { + if let Some(symbol_name) = symbol.name().map(|name| name.as_str()).flatten() { + if !sync_mutex_constr_regex.is_match(symbol_name) { + if found_debug_sync { + symbol_after_latest_debug_sync = Some(symbol); + found_debug_sync = false; + } + } else { found_debug_sync = true; } + } } } - panic!("Couldn't find mutex construction callsite"); + let symbol = symbol_after_latest_debug_sync.expect("Couldn't find lock call symbol"); + (format!("{}:{}", symbol.filename().unwrap().display(), symbol.lineno().unwrap()), symbol.colno()) } impl LockMetadata { @@ -108,13 +115,13 @@ impl LockMetadata { #[cfg(feature = "backtrace")] { let (lock_constr_location, lock_constr_colno) = - get_construction_location(&res._lock_construction_bt); + locate_call_symbol(&res._lock_construction_bt); LOCKS_INIT.call_once(|| { unsafe { LOCKS = Some(StdMutex::new(HashMap::new())); } }); let mut locks = unsafe { LOCKS.as_ref() }.unwrap().lock().unwrap(); match locks.entry(lock_constr_location) { hash_map::Entry::Occupied(e) => { assert_eq!(lock_constr_colno, - get_construction_location(&e.get()._lock_construction_bt).1, + locate_call_symbol(&e.get()._lock_construction_bt).1, "Because Windows doesn't support column number results in backtraces, we cannot construct two mutexes on the same line or we risk lockorder detection false positives."); return Arc::clone(e.get()) }, @@ -126,9 +133,8 @@ impl LockMetadata { fn pre_lock(this: &Arc, _double_lock_self_allowed: bool) { LOCKS_HELD.with(|held| { - // For each lock which is currently locked, check that no lock's locked-before - // set includes the lock we're about to lock, which would imply a lockorder - // inversion. + // For each lock that is currently held, check that no lock's `locked_before` set + // includes the lock we're about to hold, which would imply a lockorder inversion. for (locked_idx, _locked) in held.borrow().iter() { if *locked_idx == this.lock_idx { // Note that with `feature = "backtrace"` set, we may be looking at different @@ -138,23 +144,34 @@ impl LockMetadata { #[cfg(feature = "backtrace")] debug_assert!(_double_lock_self_allowed, "Tried to acquire a lock while it was held!\nLock constructed at {}", - get_construction_location(&this._lock_construction_bt).0); + locate_call_symbol(&this._lock_construction_bt).0); #[cfg(not(feature = "backtrace"))] panic!("Tried to acquire a lock while it was held!"); } } for (_locked_idx, locked) in held.borrow().iter() { for (locked_dep_idx, _locked_dep) in locked.locked_before.lock().unwrap().iter() { - if *locked_dep_idx == this.lock_idx && *locked_dep_idx != locked.lock_idx { - #[cfg(feature = "backtrace")] - panic!("Tried to violate existing lockorder.\nMutex that should be locked after the current lock was created at the following backtrace.\nNote that to get a backtrace for the lockorder violation, you should set RUST_BACKTRACE=1\nLock being taken constructed at: {} ({}):\n{:?}\nLock constructed at: {} ({})\n{:?}\n\nLock dep created at:\n{:?}\n\n", - get_construction_location(&this._lock_construction_bt).0, - this.lock_idx, this._lock_construction_bt, - get_construction_location(&locked._lock_construction_bt).0, - locked.lock_idx, locked._lock_construction_bt, - _locked_dep._lockdep_trace); - #[cfg(not(feature = "backtrace"))] - panic!("Tried to violate existing lockorder. Build with the backtrace feature for more info."); + let is_dep_this_lock = *locked_dep_idx == this.lock_idx; + let has_same_construction = *locked_dep_idx == locked.lock_idx; + if is_dep_this_lock && !has_same_construction { + #[allow(unused_mut, unused_assignments)] + let mut has_same_callsite = false; + #[cfg(feature = "backtrace")] { + has_same_callsite = _double_lock_self_allowed && + locate_call_symbol(&_locked_dep._lockdep_trace) == + locate_call_symbol(&Backtrace::new()); + } + if !has_same_callsite { + #[cfg(feature = "backtrace")] + panic!("Tried to violate existing lockorder.\nMutex that should be locked after the current lock was created at the following backtrace.\nNote that to get a backtrace for the lockorder violation, you should set RUST_BACKTRACE=1\nLock being taken constructed at: {} ({}):\n{:?}\nLock constructed at: {} ({})\n{:?}\n\nLock dep created at:\n{:?}\n\n", + locate_call_symbol(&this._lock_construction_bt).0, + this.lock_idx, this._lock_construction_bt, + locate_call_symbol(&locked._lock_construction_bt).0, + locked.lock_idx, locked._lock_construction_bt, + _locked_dep._lockdep_trace); + #[cfg(not(feature = "backtrace"))] + panic!("Tried to violate existing lockorder. Build with the backtrace feature for more info."); + } } } // Insert any already-held locks in our locked-before set. From 9fe475057acf92ae78dbfdb499221a5af4cfae2c Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 28 Mar 2023 13:22:47 -0700 Subject: [PATCH 6/6] Add pending changelog noting possible backwards compat panic --- pending_changelog/2059.txt | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 pending_changelog/2059.txt diff --git a/pending_changelog/2059.txt b/pending_changelog/2059.txt new file mode 100644 index 00000000000..d2ee0bcb7ea --- /dev/null +++ b/pending_changelog/2059.txt @@ -0,0 +1,4 @@ +## Backwards Compatibility + +- Providing `ChannelMonitorUpdate`s generated by LDK 0.0.115 to a +`ChannelMonitor` on 0.0.114 or before may panic.