From 4741653f76c650f31065f6f614ec37d8625cf0f5 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 23 Oct 2024 15:59:23 +0000 Subject: [PATCH 01/11] Allow `clippy::unwrap-or-default` because its usually wrong `or_default` is generally less readable than writing out the thing we're writing, as `Default` is opaque but explicit constructors generally are not. Thus, we ignore the clippy lint (ideally we could invert it and ban the use of `Default` in the crate entirely but alas). --- ci/check-lint.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ci/check-lint.sh b/ci/check-lint.sh index a0e467192ef..b20690707a1 100755 --- a/ci/check-lint.sh +++ b/ci/check-lint.sh @@ -2,6 +2,8 @@ set -e set -x RUSTFLAGS='-D warnings' cargo clippy -- \ + `# Things where clippy is just wrong` \ + -A clippy::unwrap-or-default \ `# Errors` \ -A clippy::erasing_op \ -A clippy::never_loop \ From a65d37b48b0452103e1714e11ebca8d875648feb Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 30 Sep 2024 21:02:53 +0000 Subject: [PATCH 02/11] Add missing `inbound_payment_id_secret` write in `ChannelManager` In aa09c33a1719944769ba98624bfe18ea33083f44 we added a new secret in `ChannelManager` with which to derive inbound `PaymentId`s. We added read support for the new field, but forgot to add writing support for it. Here we fix this oversight. --- lightning/src/ln/channelmanager.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 3914384ca82..3b3a0e37e73 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -12024,6 +12024,7 @@ where (11, self.probing_cookie_secret, required), (13, htlc_onion_fields, optional_vec), (14, decode_update_add_htlcs_opt, option), + (15, self.inbound_payment_id_secret, required), }); Ok(()) From d9175f454bfce665af32c9a3ec54482fbb6a484d Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 14 Jun 2024 14:10:38 +0000 Subject: [PATCH 03/11] Use a struct to track MPP parts pending claiming When we started tracking which channels had MPP parts claimed durably on-disk in their `ChannelMonitor`, we did so with a tuple. This was fine in that it was only ever accessed in two places, but as we will start tracking it through to the `ChannelMonitor`s themselves in the coming commit(s), it is useful to have it in a struct instead. --- lightning/src/ln/channelmanager.rs | 42 +++++++++++++++++++++++------- 1 file changed, 33 insertions(+), 9 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 3b3a0e37e73..c1750c5af76 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1062,10 +1062,20 @@ impl_writeable_tlv_based_enum!(EventCompletionAction, } ); +#[derive(Clone, Debug, PartialEq, Eq)] +/// The source of an HTLC which is being claimed as a part of an incoming payment. Each part is +/// tracked in [`PendingMPPClaim`]. +struct MPPClaimHTLCSource { + counterparty_node_id: PublicKey, + funding_txo: OutPoint, + channel_id: ChannelId, + htlc_id: u64, +} + #[derive(Debug)] pub(crate) struct PendingMPPClaim { - channels_without_preimage: Vec<(PublicKey, OutPoint, ChannelId, u64)>, - channels_with_preimage: Vec<(PublicKey, OutPoint, ChannelId)>, + channels_without_preimage: Vec, + channels_with_preimage: Vec, } #[derive(Clone)] @@ -6765,8 +6775,12 @@ where let pending_mpp_claim_ptr_opt = if sources.len() > 1 { let channels_without_preimage = sources.iter().filter_map(|htlc| { if let Some(cp_id) = htlc.prev_hop.counterparty_node_id { - let prev_hop = &htlc.prev_hop; - Some((cp_id, prev_hop.outpoint, prev_hop.channel_id, prev_hop.htlc_id)) + Some(MPPClaimHTLCSource { + counterparty_node_id: cp_id, + funding_txo: htlc.prev_hop.outpoint, + channel_id: htlc.prev_hop.channel_id, + htlc_id: htlc.prev_hop.htlc_id, + }) } else { None } @@ -7184,15 +7198,25 @@ where if *pending_claim == claim_ptr { let mut pending_claim_state_lock = pending_claim.0.lock().unwrap(); let pending_claim_state = &mut *pending_claim_state_lock; - pending_claim_state.channels_without_preimage.retain(|(cp, outp, cid, hid)| { - if *cp == counterparty_node_id && *cid == chan_id && *hid == htlc_id { - pending_claim_state.channels_with_preimage.push((*cp, *outp, *cid)); + pending_claim_state.channels_without_preimage.retain(|htlc_info| { + let this_claim = + htlc_info.counterparty_node_id == counterparty_node_id + && htlc_info.channel_id == chan_id + && htlc_info.htlc_id == htlc_id; + if this_claim { + pending_claim_state.channels_with_preimage.push(htlc_info.clone()); false } else { true } }); if pending_claim_state.channels_without_preimage.is_empty() { - for (cp, outp, cid) in pending_claim_state.channels_with_preimage.iter() { - freed_channels.push((*cp, *outp, *cid, blocker.clone())); + for htlc_info in pending_claim_state.channels_with_preimage.iter() { + let freed_chan = ( + htlc_info.counterparty_node_id, + htlc_info.funding_txo, + htlc_info.channel_id, + blocker.clone() + ); + freed_channels.push(freed_chan); } } !pending_claim_state.channels_without_preimage.is_empty() From b8661ef6cf7f13c9f904fbd25c59fb59415f0d73 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 15 Sep 2024 23:50:31 +0000 Subject: [PATCH 04/11] Pass info about claimed payments, incl HTLCs to `ChannelMonitor`s When we claim an MPP payment, then crash before persisting all the relevant `ChannelMonitor`s, we rely on the payment data being available in the `ChannelManager` on restart to re-claim any parts that haven't yet been claimed. This is fine as long as the `ChannelManager` was persisted before the `PaymentClaimable` event was processed, which is generally the case in our `lightning-background-processor`, but may not be in other cases or in a somewhat rare race. In order to fix this, we need to track where all the MPP parts of a payment are in the `ChannelMonitor`, allowing us to re-claim any missing pieces without reference to any `ChannelManager` data. Further, in order to properly generate a `PaymentClaimed` event against the re-started claim, we have to store various payment metadata with the HTLC list as well. Here we take the first step, building a list of MPP parts and metadata in `ChannelManager` and passing it through to `ChannelMonitor` in the `ChannelMonitorUpdate`s. --- lightning/src/chain/channelmonitor.rs | 27 ++++++++--- lightning/src/ln/channel.rs | 24 +++++++--- lightning/src/ln/channelmanager.rs | 69 +++++++++++++++++++-------- pending_changelog/3322-a.txt | 6 +++ 4 files changed, 93 insertions(+), 33 deletions(-) create mode 100644 pending_changelog/3322-a.txt diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index d29c806ee88..428221d8d5c 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -38,7 +38,7 @@ use crate::types::payment::{PaymentHash, PaymentPreimage}; use crate::ln::msgs::DecodeError; use crate::ln::channel_keys::{DelayedPaymentKey, DelayedPaymentBasepoint, HtlcBasepoint, HtlcKey, RevocationKey, RevocationBasepoint}; use crate::ln::chan_utils::{self,CommitmentTransaction, CounterpartyCommitmentSecrets, HTLCOutputInCommitment, HTLCClaim, ChannelTransactionParameters, HolderCommitmentTransaction, TxCreationKeys}; -use crate::ln::channelmanager::{HTLCSource, SentHTLCId}; +use crate::ln::channelmanager::{HTLCSource, SentHTLCId, PaymentClaimDetails}; use crate::chain; use crate::chain::{BestBlock, WatchedOutput}; use crate::chain::chaininterface::{BroadcasterInterface, ConfirmationTarget, FeeEstimator, LowerBoundedFeeEstimator}; @@ -546,6 +546,9 @@ pub(crate) enum ChannelMonitorUpdateStep { }, PaymentPreimage { payment_preimage: PaymentPreimage, + /// If this preimage was from an inbound payment claim, information about the claim should + /// be included here to enable claim replay on startup. + payment_info: Option, }, CommitmentSecret { idx: u64, @@ -594,6 +597,7 @@ impl_writeable_tlv_based_enum_upgradable!(ChannelMonitorUpdateStep, }, (2, PaymentPreimage) => { (0, payment_preimage, required), + (1, payment_info, option), }, (3, CommitmentSecret) => { (0, idx, required), @@ -1502,8 +1506,11 @@ impl ChannelMonitor { { let mut inner = self.inner.lock().unwrap(); let logger = WithChannelMonitor::from_impl(logger, &*inner, Some(*payment_hash)); + // Note that we don't pass any MPP claim parts here. This is generally not okay but in this + // case is acceptable as we only call this method from `ChannelManager` deserialization in + // cases where we are replaying a claim started on a previous version of LDK. inner.provide_payment_preimage( - payment_hash, payment_preimage, broadcaster, fee_estimator, &logger) + payment_hash, payment_preimage, &None, broadcaster, fee_estimator, &logger) } /// Updates a ChannelMonitor on the basis of some new information provided by the Channel @@ -2930,12 +2937,14 @@ impl ChannelMonitorImpl { /// Provides a payment_hash->payment_preimage mapping. Will be automatically pruned when all /// commitment_tx_infos which contain the payment hash have been revoked. fn provide_payment_preimage( - &mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage, broadcaster: &B, + &mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage, + payment_info: &Option, broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator, logger: &WithChannelMonitor) where B::Target: BroadcasterInterface, F::Target: FeeEstimator, L::Target: Logger, { + // TODO: Store payment_info (but do not override any existing values) self.payment_preimages.insert(payment_hash.clone(), payment_preimage.clone()); let confirmed_spend_txid = self.funding_spend_confirmed.or_else(|| { @@ -3139,9 +3148,9 @@ impl ChannelMonitorImpl { log_trace!(logger, "Updating ChannelMonitor with latest counterparty commitment transaction info"); self.provide_latest_counterparty_commitment_tx(*commitment_txid, htlc_outputs.clone(), *commitment_number, *their_per_commitment_point, logger) }, - ChannelMonitorUpdateStep::PaymentPreimage { payment_preimage } => { + ChannelMonitorUpdateStep::PaymentPreimage { payment_preimage, payment_info } => { log_trace!(logger, "Updating ChannelMonitor with payment preimage"); - self.provide_payment_preimage(&PaymentHash(Sha256::hash(&payment_preimage.0[..]).to_byte_array()), &payment_preimage, broadcaster, &bounded_fee_estimator, logger) + self.provide_payment_preimage(&PaymentHash(Sha256::hash(&payment_preimage.0[..]).to_byte_array()), &payment_preimage, payment_info, broadcaster, &bounded_fee_estimator, logger) }, ChannelMonitorUpdateStep::CommitmentSecret { idx, secret } => { log_trace!(logger, "Updating ChannelMonitor with commitment secret"); @@ -5097,8 +5106,12 @@ mod tests { assert_eq!(replay_update.updates.len(), 1); if let ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { .. } = replay_update.updates[0] { } else { panic!(); } - replay_update.updates.push(ChannelMonitorUpdateStep::PaymentPreimage { payment_preimage: payment_preimage_1 }); - replay_update.updates.push(ChannelMonitorUpdateStep::PaymentPreimage { payment_preimage: payment_preimage_2 }); + replay_update.updates.push(ChannelMonitorUpdateStep::PaymentPreimage { + payment_preimage: payment_preimage_1, payment_info: None, + }); + replay_update.updates.push(ChannelMonitorUpdateStep::PaymentPreimage { + payment_preimage: payment_preimage_2, payment_info: None, + }); let broadcaster = TestBroadcaster::with_blocks(Arc::clone(&nodes[1].blocks)); assert!( diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 42458e4769f..42e4b2f3e72 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -32,7 +32,7 @@ use crate::ln::msgs; use crate::ln::msgs::{ClosingSigned, ClosingSignedFeeRange, DecodeError}; use crate::ln::script::{self, ShutdownScript}; use crate::ln::channel_state::{ChannelShutdownState, CounterpartyForwardingInfo, InboundHTLCDetails, InboundHTLCStateDetails, OutboundHTLCDetails, OutboundHTLCStateDetails}; -use crate::ln::channelmanager::{self, PendingHTLCStatus, HTLCSource, SentHTLCId, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, MAX_LOCAL_BREAKDOWN_TIMEOUT}; +use crate::ln::channelmanager::{self, PendingHTLCStatus, HTLCSource, SentHTLCId, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, PaymentClaimDetails, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, MAX_LOCAL_BREAKDOWN_TIMEOUT}; use crate::ln::chan_utils::{ CounterpartyCommitmentSecrets, TxCreationKeys, HTLCOutputInCommitment, htlc_success_tx_weight, htlc_timeout_tx_weight, make_funding_redeemscript, ChannelPublicKeys, CommitmentTransaction, @@ -4039,14 +4039,17 @@ impl Channel where // (see equivalent if condition there). assert!(!self.context.channel_state.can_generate_new_commitment()); let mon_update_id = self.context.latest_monitor_update_id; // Forget the ChannelMonitor update - let fulfill_resp = self.get_update_fulfill_htlc(htlc_id_arg, payment_preimage_arg, logger); + let fulfill_resp = self.get_update_fulfill_htlc(htlc_id_arg, payment_preimage_arg, None, logger); self.context.latest_monitor_update_id = mon_update_id; if let UpdateFulfillFetch::NewClaim { msg, .. } = fulfill_resp { assert!(msg.is_none()); // The HTLC must have ended up in the holding cell. } } - fn get_update_fulfill_htlc(&mut self, htlc_id_arg: u64, payment_preimage_arg: PaymentPreimage, logger: &L) -> UpdateFulfillFetch where L::Target: Logger { + fn get_update_fulfill_htlc( + &mut self, htlc_id_arg: u64, payment_preimage_arg: PaymentPreimage, + payment_info: Option, logger: &L, + ) -> UpdateFulfillFetch where L::Target: Logger { // Either ChannelReady got set (which means it won't be unset) or there is no way any // caller thought we could have something claimed (cause we wouldn't have accepted in an // incoming HTLC anyway). If we got to ShutdownComplete, callers aren't allowed to call us, @@ -4104,6 +4107,7 @@ impl Channel where counterparty_node_id: Some(self.context.counterparty_node_id), updates: vec![ChannelMonitorUpdateStep::PaymentPreimage { payment_preimage: payment_preimage_arg.clone(), + payment_info, }], channel_id: Some(self.context.channel_id()), }; @@ -4171,9 +4175,12 @@ impl Channel where } } - pub fn get_update_fulfill_htlc_and_commit(&mut self, htlc_id: u64, payment_preimage: PaymentPreimage, logger: &L) -> UpdateFulfillCommitFetch where L::Target: Logger { + pub fn get_update_fulfill_htlc_and_commit( + &mut self, htlc_id: u64, payment_preimage: PaymentPreimage, + payment_info: Option, logger: &L, + ) -> UpdateFulfillCommitFetch where L::Target: Logger { let release_cs_monitor = self.context.blocked_monitor_updates.is_empty(); - match self.get_update_fulfill_htlc(htlc_id, payment_preimage, logger) { + match self.get_update_fulfill_htlc(htlc_id, payment_preimage, payment_info, logger) { UpdateFulfillFetch::NewClaim { mut monitor_update, htlc_value_msat, msg } => { // Even if we aren't supposed to let new monitor updates with commitment state // updates run, we still need to push the preimage ChannelMonitorUpdateStep no @@ -4934,9 +4941,14 @@ impl Channel where // not fail - any in between attempts to claim the HTLC will have resulted // in it hitting the holding cell again and we cannot change the state of a // holding cell HTLC from fulfill to anything else. + // + // Note that we should have already provided a preimage-containing + // `ChannelMonitorUpdate` to the user, making this one redundant, however + // there's no harm in including the extra `ChannelMonitorUpdateStep` here. + // We do not bother to track and include `payment_info` here, however. let mut additional_monitor_update = if let UpdateFulfillFetch::NewClaim { monitor_update, .. } = - self.get_update_fulfill_htlc(htlc_id, *payment_preimage, logger) + self.get_update_fulfill_htlc(htlc_id, *payment_preimage, None, logger) { monitor_update } else { unreachable!() }; update_fulfill_count += 1; monitor_update.updates.append(&mut additional_monitor_update.updates); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index c1750c5af76..699430749e9 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -801,6 +801,7 @@ pub(super) enum RAACommitmentOrder { } /// Information about a payment which is currently being claimed. +#[derive(Clone, Debug, PartialEq, Eq)] struct ClaimingPayment { amount_msat: u64, payment_purpose: events::PaymentPurpose, @@ -1072,12 +1073,36 @@ struct MPPClaimHTLCSource { htlc_id: u64, } +impl_writeable_tlv_based!(MPPClaimHTLCSource, { + (0, counterparty_node_id, required), + (2, funding_txo, required), + (4, channel_id, required), + (6, htlc_id, required), +}); + #[derive(Debug)] pub(crate) struct PendingMPPClaim { channels_without_preimage: Vec, channels_with_preimage: Vec, } +#[derive(Clone, Debug, PartialEq, Eq)] +/// When we're claiming a(n MPP) payment, we want to store information about thay payment in the +/// [`ChannelMonitor`] so that we can replay the claim without any information from the +/// [`ChannelManager`] at all. This struct stores that information with enough to replay claims +/// against all MPP parts as well as generate an [`Event::PaymentClaimed`]. +pub(crate) struct PaymentClaimDetails { + mpp_parts: Vec, + /// Use [`ClaimingPayment`] as a stable source of all the fields we need to generate the + /// [`Event::PaymentClaimed`]. + claiming_payment: ClaimingPayment, +} + +impl_writeable_tlv_based!(PaymentClaimDetails, { + (0, mpp_parts, required_vec), + (2, claiming_payment, required), +}); + #[derive(Clone)] pub(crate) struct PendingMPPClaimPointer(Arc>); @@ -6675,6 +6700,7 @@ where let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); + let claiming_payment; let sources = { let mut claimable_payments = self.claimable_payments.lock().unwrap(); if let Some(payment) = claimable_payments.claimable_payments.remove(&payment_hash) { @@ -6689,7 +6715,7 @@ where } let payment_id = payment.inbound_payment_id(&self.inbound_payment_id_secret); - let claiming_payment = claimable_payments.pending_claiming_payments + claiming_payment = claimable_payments.pending_claiming_payments .entry(payment_hash) .and_modify(|_| { debug_assert!(false, "Shouldn't get a duplicate pending claim event ever"); @@ -6708,7 +6734,7 @@ where onion_fields: payment.onion_fields, payment_id: Some(payment_id), } - }); + }).clone(); if let Some(RecipientOnionFields { ref custom_tlvs, .. }) = claiming_payment.onion_fields { if !custom_tlvs_known && custom_tlvs.iter().any(|(typ, _)| typ % 2 == 0) { @@ -6772,26 +6798,27 @@ where return; } if valid_mpp { + let mpp_parts: Vec<_> = sources.iter().filter_map(|htlc| { + if let Some(cp_id) = htlc.prev_hop.counterparty_node_id { + Some(MPPClaimHTLCSource { + counterparty_node_id: cp_id, + funding_txo: htlc.prev_hop.outpoint, + channel_id: htlc.prev_hop.channel_id, + htlc_id: htlc.prev_hop.htlc_id, + }) + } else { + None + } + }).collect(); let pending_mpp_claim_ptr_opt = if sources.len() > 1 { - let channels_without_preimage = sources.iter().filter_map(|htlc| { - if let Some(cp_id) = htlc.prev_hop.counterparty_node_id { - Some(MPPClaimHTLCSource { - counterparty_node_id: cp_id, - funding_txo: htlc.prev_hop.outpoint, - channel_id: htlc.prev_hop.channel_id, - htlc_id: htlc.prev_hop.htlc_id, - }) - } else { - None - } - }).collect(); Some(Arc::new(Mutex::new(PendingMPPClaim { - channels_without_preimage, + channels_without_preimage: mpp_parts.clone(), channels_with_preimage: Vec::new(), }))) } else { None }; + let payment_info = Some(PaymentClaimDetails { mpp_parts, claiming_payment }); for htlc in sources { let this_mpp_claim = pending_mpp_claim_ptr_opt.as_ref().and_then(|pending_mpp_claim| if let Some(cp_id) = htlc.prev_hop.counterparty_node_id { @@ -6807,7 +6834,7 @@ where } }); self.claim_funds_from_hop( - htlc.prev_hop, payment_preimage, + htlc.prev_hop, payment_preimage, payment_info.clone(), |_, definitely_duplicate| { debug_assert!(!definitely_duplicate, "We shouldn't claim duplicatively from a payment"); (Some(MonitorUpdateCompletionAction::PaymentClaimed { payment_hash, pending_mpp_claim: this_mpp_claim }), raa_blocker) @@ -6837,7 +6864,7 @@ where ComplFunc: FnOnce(Option, bool) -> (Option, Option) >( &self, prev_hop: HTLCPreviousHopData, payment_preimage: PaymentPreimage, - completion_action: ComplFunc, + payment_info: Option, completion_action: ComplFunc, ) { //TODO: Delay the claimed_funds relaying just like we do outbound relay! @@ -6871,7 +6898,8 @@ where if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() { let counterparty_node_id = chan.context.get_counterparty_node_id(); let logger = WithChannelContext::from(&self.logger, &chan.context, None); - let fulfill_res = chan.get_update_fulfill_htlc_and_commit(prev_hop.htlc_id, payment_preimage, &&logger); + let fulfill_res = + chan.get_update_fulfill_htlc_and_commit(prev_hop.htlc_id, payment_preimage, payment_info, &&logger); match fulfill_res { UpdateFulfillCommitFetch::NewClaim { htlc_value_msat, monitor_update } => { @@ -6959,6 +6987,7 @@ where counterparty_node_id: None, updates: vec![ChannelMonitorUpdateStep::PaymentPreimage { payment_preimage, + payment_info, }], channel_id: Some(prev_hop.channel_id), }; @@ -7069,7 +7098,7 @@ where 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, + self.claim_funds_from_hop(hop_data, payment_preimage, None, |htlc_claim_value_msat, definitely_duplicate| { let chan_to_release = if let Some(node_id) = next_channel_counterparty_node_id { @@ -7105,7 +7134,7 @@ where if *funding_txo == claiming_chan_funding_outpoint { assert!(update.updates.iter().any(|upd| if let ChannelMonitorUpdateStep::PaymentPreimage { - payment_preimage: update_preimage + payment_preimage: update_preimage, .. } = upd { payment_preimage == *update_preimage } else { false } diff --git a/pending_changelog/3322-a.txt b/pending_changelog/3322-a.txt new file mode 100644 index 00000000000..83849926a4e --- /dev/null +++ b/pending_changelog/3322-a.txt @@ -0,0 +1,6 @@ +API Changes +=========== + +Additional information is now stored in `ChannelMonitorUpdate`s which may increase the size of +`ChannelMonitorUpdate`s claiming inbound payments substantially. The expected maximum size of +`ChannelMonitorUpdate`s shouldn't change materially. From 7790e30880f955d0e378ffd4e4062a7b4747c361 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 16 Sep 2024 00:07:48 +0000 Subject: [PATCH 05/11] Store info about claimed payments, incl HTLCs in `ChannelMonitor`s When we claim an MPP payment, then crash before persisting all the relevant `ChannelMonitor`s, we rely on the payment data being available in the `ChannelManager` on restart to re-claim any parts that haven't yet been claimed. This is fine as long as the `ChannelManager` was persisted before the `PaymentClaimable` event was processed, which is generally the case in our `lightning-background-processor`, but may not be in other cases or in a somewhat rare race. In order to fix this, we need to track where all the MPP parts of a payment are in the `ChannelMonitor`, allowing us to re-claim any missing pieces without reference to any `ChannelManager` data. Further, in order to properly generate a `PaymentClaimed` event against the re-started claim, we have to store various payment metadata with the HTLC list as well. Here we store the required MPP parts and metadata in `ChannelMonitor`s and make them available to `ChannelManager` on load. --- lightning/src/chain/channelmonitor.rs | 62 +++++++++++++++++++++------ lightning/src/ln/channelmanager.rs | 2 +- lightning/src/util/ser.rs | 1 + 3 files changed, 52 insertions(+), 13 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 428221d8d5c..6513e7b8e68 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -923,8 +923,16 @@ pub(crate) struct ChannelMonitorImpl { /// The set of payment hashes from inbound payments for which we know the preimage. Payment /// preimages that are not included in any unrevoked local commitment transaction or unrevoked /// remote commitment transactions are automatically removed when commitment transactions are - /// revoked. - payment_preimages: HashMap, + /// revoked. Note that this happens one revocation after it theoretically could, leaving + /// preimages present here for the previous state even when the channel is "at rest". This is a + /// good safety buffer, but also is important as it ensures we retain payment preimages for the + /// previous local commitment transaction, which may have been broadcast already when we see + /// the revocation (in setups with redundant monitors). + /// + /// We also store [`PaymentClaimDetails`] here, tracking the payment information(s) for this + /// preimage for inbound payments. This allows us to rebuild the inbound payment information on + /// startup even if we lost our `ChannelManager`. + payment_preimages: HashMap)>, // Note that `MonitorEvent`s MUST NOT be generated during update processing, only generated // during chain data processing. This prevents a race in `ChainMonitor::update_channel` (and @@ -1150,7 +1158,7 @@ impl Writeable for ChannelMonitorImpl { writer.write_all(&byte_utils::be48_to_array(self.current_holder_commitment_number))?; writer.write_all(&(self.payment_preimages.len() as u64).to_be_bytes())?; - for payment_preimage in self.payment_preimages.values() { + for (payment_preimage, _) in self.payment_preimages.values() { writer.write_all(&payment_preimage.0[..])?; } @@ -1228,6 +1236,7 @@ impl Writeable for ChannelMonitorImpl { (19, self.channel_id, required), (21, self.balances_empty_height, option), (23, self.holder_pays_commitment_tx_fee, option), + (25, self.payment_preimages, required), }); Ok(()) @@ -2201,7 +2210,7 @@ impl ChannelMonitorImpl { outbound_payment, }); } - } else if let Some(payment_preimage) = self.payment_preimages.get(&htlc.payment_hash) { + } else if let Some((payment_preimage, _)) = self.payment_preimages.get(&htlc.payment_hash) { // Otherwise (the payment was inbound), only expose it as claimable if // we know the preimage. // Note that if there is a pending claim, but it did not use the @@ -2422,7 +2431,7 @@ impl ChannelMonitor { outbound_payment, }); } - } else if us.payment_preimages.get(&htlc.payment_hash).is_some() { + } else if us.payment_preimages.contains_key(&htlc.payment_hash) { inbound_claiming_htlc_rounded_msat += rounded_value_msat; if htlc.transaction_output_index.is_some() { claimable_inbound_htlc_value_sat += htlc.amount_msat / 1000; @@ -2577,7 +2586,7 @@ impl ChannelMonitor { res } - pub(crate) fn get_stored_preimages(&self) -> HashMap { + pub(crate) fn get_stored_preimages(&self) -> HashMap)> { self.inner.lock().unwrap().payment_preimages.clone() } } @@ -2936,6 +2945,8 @@ impl ChannelMonitorImpl { /// Provides a payment_hash->payment_preimage mapping. Will be automatically pruned when all /// commitment_tx_infos which contain the payment hash have been revoked. + /// + /// Note that this is often called multiple times for the same payment and must be idempotent. fn provide_payment_preimage( &mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage, payment_info: &Option, broadcaster: &B, @@ -2944,8 +2955,17 @@ impl ChannelMonitorImpl { F::Target: FeeEstimator, L::Target: Logger, { - // TODO: Store payment_info (but do not override any existing values) - self.payment_preimages.insert(payment_hash.clone(), payment_preimage.clone()); + self.payment_preimages.entry(payment_hash.clone()) + .and_modify(|(_, payment_infos)| { + if let Some(payment_info) = payment_info { + if !payment_infos.contains(&payment_info) { + payment_infos.push(payment_info.clone()); + } + } + }) + .or_insert_with(|| { + (payment_preimage.clone(), payment_info.clone().into_iter().collect()) + }); let confirmed_spend_txid = self.funding_spend_confirmed.or_else(|| { self.onchain_events_awaiting_threshold_conf.iter().find_map(|event| match event.event { @@ -3602,7 +3622,7 @@ impl ChannelMonitorImpl { return (claimable_outpoints, to_counterparty_output_info); } } - let preimage = if htlc.offered { if let Some(p) = self.payment_preimages.get(&htlc.payment_hash) { Some(*p) } else { None } } else { None }; + let preimage = if htlc.offered { if let Some((p, _)) = self.payment_preimages.get(&htlc.payment_hash) { Some(*p) } else { None } } else { None }; if preimage.is_some() || !htlc.offered { let counterparty_htlc_outp = if htlc.offered { PackageSolvingData::CounterpartyOfferedHTLCOutput( @@ -3690,7 +3710,7 @@ impl ChannelMonitorImpl { ); (htlc_output, conf_height) } else { - let payment_preimage = if let Some(preimage) = self.payment_preimages.get(&htlc.payment_hash) { + let payment_preimage = if let Some((preimage, _)) = self.payment_preimages.get(&htlc.payment_hash) { preimage.clone() } else { // We can't build an HTLC-Success transaction without the preimage @@ -3844,7 +3864,7 @@ impl ChannelMonitorImpl { for htlc in self.current_holder_commitment_tx.htlc_outputs.iter() { if let Some(vout) = htlc.0.transaction_output_index { let preimage = if !htlc.0.offered { - if let Some(preimage) = self.payment_preimages.get(&htlc.0.payment_hash) { Some(preimage.clone()) } else { + if let Some((preimage, _)) = self.payment_preimages.get(&htlc.0.payment_hash) { Some(preimage.clone()) } else { // We can't build an HTLC-Success transaction without the preimage continue; } @@ -4817,7 +4837,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP for _ in 0..payment_preimages_len { let preimage: PaymentPreimage = Readable::read(reader)?; let hash = PaymentHash(Sha256::hash(&preimage.0[..]).to_byte_array()); - if let Some(_) = payment_preimages.insert(hash, preimage) { + if let Some(_) = payment_preimages.insert(hash, (preimage, Vec::new())) { return Err(DecodeError::InvalidValue); } } @@ -4900,6 +4920,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP let mut balances_empty_height = None; let mut channel_id = None; let mut holder_pays_commitment_tx_fee = None; + let mut payment_preimages_with_info: Option> = None; read_tlv_fields!(reader, { (1, funding_spend_confirmed, option), (3, htlcs_resolved_on_chain, optional_vec), @@ -4913,7 +4934,24 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP (19, channel_id, option), (21, balances_empty_height, option), (23, holder_pays_commitment_tx_fee, option), + (25, payment_preimages_with_info, option), }); + if let Some(payment_preimages_with_info) = payment_preimages_with_info { + if payment_preimages_with_info.len() != payment_preimages.len() { + return Err(DecodeError::InvalidValue); + } + for (payment_hash, (payment_preimage, _)) in payment_preimages.iter() { + // Note that because `payment_preimages` is built back from preimages directly, + // checking that the two maps have the same hash -> preimage pairs also checks that + // the payment hashes in `payment_preimages_with_info`'s preimages match its + // hashes. + let new_preimage = payment_preimages_with_info.get(payment_hash).map(|(p, _)| p); + if new_preimage != Some(payment_preimage) { + return Err(DecodeError::InvalidValue); + } + } + payment_preimages = payment_preimages_with_info; + } // `HolderForceClosedWithInfo` replaced `HolderForceClosed` in v0.0.122. If we have both // events, we can remove the `HolderForceClosed` event and just keep the `HolderForceClosedWithInfo`. diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 699430749e9..9fdfc0eb7ab 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -12987,7 +12987,7 @@ where let bounded_fee_estimator = LowerBoundedFeeEstimator::new(args.fee_estimator); for (_, monitor) in args.channel_monitors.iter() { - for (payment_hash, payment_preimage) in monitor.get_stored_preimages() { + for (payment_hash, (payment_preimage, _)) in monitor.get_stored_preimages() { if let Some(payment) = claimable_payments.remove(&payment_hash) { log_info!(args.logger, "Re-claiming HTLCs with payment hash {} as we've released the preimage to a ChannelMonitor!", &payment_hash); let mut claimable_amt_msat = 0; diff --git a/lightning/src/util/ser.rs b/lightning/src/util/ser.rs index 5c1a82d4b6f..c37c326790b 100644 --- a/lightning/src/util/ser.rs +++ b/lightning/src/util/ser.rs @@ -1001,6 +1001,7 @@ impl Readable for Vec { impl_for_vec!(ecdsa::Signature); impl_for_vec!(crate::chain::channelmonitor::ChannelMonitorUpdate); impl_for_vec!(crate::ln::channelmanager::MonitorUpdateCompletionAction); +impl_for_vec!(crate::ln::channelmanager::PaymentClaimDetails); impl_for_vec!(crate::ln::msgs::SocketAddress); impl_for_vec!((A, B), A, B); impl_writeable_for_vec!(&crate::routing::router::BlindedTail); From a26033f0cb879c7514953c11dfb9547e982e633e Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 30 Sep 2024 19:42:51 +0000 Subject: [PATCH 06/11] Move `ChannelManager`-read preimage relay to after struct build In a coming commit we'll use the existing `ChannelManager` claim flow to claim HTLCs which we found partially claimed on startup, necessitating having a full `ChannelManager` when we go to do so. Here we move the re-claim logic down in the `ChannelManager`-read logic so that we have that. --- lightning/src/ln/channelmanager.rs | 127 +++++++++++++++-------------- 1 file changed, 67 insertions(+), 60 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 9fdfc0eb7ab..462c2cc26bb 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1087,7 +1087,7 @@ pub(crate) struct PendingMPPClaim { } #[derive(Clone, Debug, PartialEq, Eq)] -/// When we're claiming a(n MPP) payment, we want to store information about thay payment in the +/// When we're claiming a(n MPP) payment, we want to store information about that payment in the /// [`ChannelMonitor`] so that we can replay the claim without any information from the /// [`ChannelManager`] at all. This struct stores that information with enough to replay claims /// against all MPP parts as well as generate an [`Event::PaymentClaimed`]. @@ -12986,65 +12986,6 @@ where let bounded_fee_estimator = LowerBoundedFeeEstimator::new(args.fee_estimator); - for (_, monitor) in args.channel_monitors.iter() { - for (payment_hash, (payment_preimage, _)) in monitor.get_stored_preimages() { - if let Some(payment) = claimable_payments.remove(&payment_hash) { - log_info!(args.logger, "Re-claiming HTLCs with payment hash {} as we've released the preimage to a ChannelMonitor!", &payment_hash); - let mut claimable_amt_msat = 0; - let mut receiver_node_id = Some(our_network_pubkey); - let phantom_shared_secret = payment.htlcs[0].prev_hop.phantom_shared_secret; - if phantom_shared_secret.is_some() { - let phantom_pubkey = args.node_signer.get_node_id(Recipient::PhantomNode) - .expect("Failed to get node_id for phantom node recipient"); - receiver_node_id = Some(phantom_pubkey) - } - for claimable_htlc in &payment.htlcs { - claimable_amt_msat += claimable_htlc.value; - - // Add a holding-cell claim of the payment to the Channel, which should be - // applied ~immediately on peer reconnection. Because it won't generate a - // new commitment transaction we can just provide the payment preimage to - // the corresponding ChannelMonitor and nothing else. - // - // We do so directly instead of via the normal ChannelMonitor update - // procedure as the ChainMonitor hasn't yet been initialized, implying - // we're not allowed to call it directly yet. Further, we do the update - // without incrementing the ChannelMonitor update ID as there isn't any - // reason to. - // If we were to generate a new ChannelMonitor update ID here and then - // crash before the user finishes block connect we'd end up force-closing - // this channel as well. On the flip side, there's no harm in restarting - // without the new monitor persisted - we'll end up right back here on - // restart. - let previous_channel_id = claimable_htlc.prev_hop.channel_id; - if let Some(peer_node_id) = outpoint_to_peer.get(&claimable_htlc.prev_hop.outpoint) { - let peer_state_mutex = per_peer_state.get(peer_node_id).unwrap(); - let mut peer_state_lock = peer_state_mutex.lock().unwrap(); - let peer_state = &mut *peer_state_lock; - if let Some(ChannelPhase::Funded(channel)) = peer_state.channel_by_id.get_mut(&previous_channel_id) { - let logger = WithChannelContext::from(&args.logger, &channel.context, Some(payment_hash)); - channel.claim_htlc_while_disconnected_dropping_mon_update(claimable_htlc.prev_hop.htlc_id, payment_preimage, &&logger); - } - } - if let Some(previous_hop_monitor) = args.channel_monitors.get(&claimable_htlc.prev_hop.outpoint) { - previous_hop_monitor.provide_payment_preimage(&payment_hash, &payment_preimage, &args.tx_broadcaster, &bounded_fee_estimator, &args.logger); - } - } - let payment_id = payment.inbound_payment_id(&inbound_payment_id_secret.unwrap()); - pending_events_read.push_back((events::Event::PaymentClaimed { - receiver_node_id, - payment_hash, - purpose: payment.purpose, - amount_msat: claimable_amt_msat, - htlcs: payment.htlcs.iter().map(events::ClaimedHTLC::from).collect(), - sender_intended_total_msat: payment.htlcs.first().map(|htlc| htlc.total_msat), - onion_fields: payment.onion_fields, - payment_id: Some(payment_id), - }, None)); - } - } - } - for (node_id, monitor_update_blocked_actions) in monitor_update_blocked_actions_per_peer.unwrap() { if let Some(peer_state) = per_peer_state.get(&node_id) { for (channel_id, actions) in monitor_update_blocked_actions.iter() { @@ -13145,6 +13086,72 @@ where default_configuration: args.default_config, }; + for (_, monitor) in args.channel_monitors.iter() { + for (payment_hash, (payment_preimage, _)) in monitor.get_stored_preimages() { + let per_peer_state = channel_manager.per_peer_state.read().unwrap(); + let mut claimable_payments = channel_manager.claimable_payments.lock().unwrap(); + let payment = claimable_payments.claimable_payments.remove(&payment_hash); + mem::drop(claimable_payments); + if let Some(payment) = payment { + log_info!(channel_manager.logger, "Re-claiming HTLCs with payment hash {} as we've released the preimage to a ChannelMonitor!", &payment_hash); + let mut claimable_amt_msat = 0; + let mut receiver_node_id = Some(our_network_pubkey); + let phantom_shared_secret = payment.htlcs[0].prev_hop.phantom_shared_secret; + if phantom_shared_secret.is_some() { + let phantom_pubkey = channel_manager.node_signer.get_node_id(Recipient::PhantomNode) + .expect("Failed to get node_id for phantom node recipient"); + receiver_node_id = Some(phantom_pubkey) + } + for claimable_htlc in &payment.htlcs { + claimable_amt_msat += claimable_htlc.value; + + // Add a holding-cell claim of the payment to the Channel, which should be + // applied ~immediately on peer reconnection. Because it won't generate a + // new commitment transaction we can just provide the payment preimage to + // the corresponding ChannelMonitor and nothing else. + // + // We do so directly instead of via the normal ChannelMonitor update + // procedure as the ChainMonitor hasn't yet been initialized, implying + // we're not allowed to call it directly yet. Further, we do the update + // without incrementing the ChannelMonitor update ID as there isn't any + // reason to. + // If we were to generate a new ChannelMonitor update ID here and then + // crash before the user finishes block connect we'd end up force-closing + // this channel as well. On the flip side, there's no harm in restarting + // without the new monitor persisted - we'll end up right back here on + // restart. + let previous_channel_id = claimable_htlc.prev_hop.channel_id; + let peer_node_id_opt = channel_manager.outpoint_to_peer.lock().unwrap() + .get(&claimable_htlc.prev_hop.outpoint).cloned(); + if let Some(peer_node_id) = peer_node_id_opt { + let peer_state_mutex = per_peer_state.get(&peer_node_id).unwrap(); + let mut peer_state_lock = peer_state_mutex.lock().unwrap(); + let peer_state = &mut *peer_state_lock; + if let Some(ChannelPhase::Funded(channel)) = peer_state.channel_by_id.get_mut(&previous_channel_id) { + let logger = WithChannelContext::from(&channel_manager.logger, &channel.context, Some(payment_hash)); + channel.claim_htlc_while_disconnected_dropping_mon_update(claimable_htlc.prev_hop.htlc_id, payment_preimage, &&logger); + } + } + if let Some(previous_hop_monitor) = args.channel_monitors.get(&claimable_htlc.prev_hop.outpoint) { + previous_hop_monitor.provide_payment_preimage(&payment_hash, &payment_preimage, &channel_manager.tx_broadcaster, &channel_manager.fee_estimator, &channel_manager.logger); + } + } + let mut pending_events = channel_manager.pending_events.lock().unwrap(); + let payment_id = payment.inbound_payment_id(&inbound_payment_id_secret.unwrap()); + pending_events.push_back((events::Event::PaymentClaimed { + receiver_node_id, + payment_hash, + purpose: payment.purpose, + amount_msat: claimable_amt_msat, + htlcs: payment.htlcs.iter().map(events::ClaimedHTLC::from).collect(), + sender_intended_total_msat: payment.htlcs.first().map(|htlc| htlc.total_msat), + onion_fields: payment.onion_fields, + payment_id: Some(payment_id), + }, None)); + } + } + } + for htlc_source in failed_htlcs.drain(..) { let (source, payment_hash, counterparty_node_id, channel_id) = htlc_source; let receiver = HTLCDestination::NextHopChannel { node_id: Some(counterparty_node_id), channel_id }; From 17cf179e75e00aca6f89e81f6c88c922084864e2 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 16 Sep 2024 00:16:51 +0000 Subject: [PATCH 07/11] Move payment claim initialization to an fn on `ClaimablePayments` Here we wrap the logic which moves claimable payments from `claimable_payments` to `pending_claiming_payments` to a new utility function on `ClaimablePayments`. This will allow us to call this new logic during `ChannelManager` deserialization in a few commits. --- lightning/src/ln/channelmanager.rs | 133 ++++++++++++++++++----------- 1 file changed, 82 insertions(+), 51 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 462c2cc26bb..cda6b67e8f9 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -899,6 +899,73 @@ struct ClaimablePayments { pending_claiming_payments: HashMap, } +impl ClaimablePayments { + /// Moves a payment from [`Self::claimable_payments`] to [`Self::pending_claiming_payments`]. + /// + /// If `custom_tlvs_known` is false and custom even TLVs are set by the sender, the set of + /// pending HTLCs will be returned in the `Err` variant of this method. They MUST then be + /// failed by the caller as they will not be in either [`Self::claimable_payments`] or + /// [`Self::pending_claiming_payments`]. + /// + /// If `custom_tlvs_known` is true, and a matching payment is found, it will always be moved. + /// + /// If no payment is found, `Err(Vec::new())` is returned. + fn begin_claiming_payment( + &mut self, payment_hash: PaymentHash, node_signer: &S, logger: &L, + inbound_payment_id_secret: &[u8; 32], custom_tlvs_known: bool, + ) -> Result<(Vec, ClaimingPayment), Vec> + where L::Target: Logger, S::Target: NodeSigner, + { + match self.claimable_payments.remove(&payment_hash) { + Some(payment) => { + let mut receiver_node_id = node_signer.get_node_id(Recipient::Node) + .expect("Failed to get node_id for node recipient"); + for htlc in payment.htlcs.iter() { + if htlc.prev_hop.phantom_shared_secret.is_some() { + let phantom_pubkey = node_signer.get_node_id(Recipient::PhantomNode) + .expect("Failed to get node_id for phantom node recipient"); + receiver_node_id = phantom_pubkey; + break; + } + } + + if let Some(RecipientOnionFields { custom_tlvs, .. }) = &payment.onion_fields { + if !custom_tlvs_known && custom_tlvs.iter().any(|(typ, _)| typ % 2 == 0) { + log_info!(logger, "Rejecting payment with payment hash {} as we cannot accept payment with unknown even TLVs: {}", + &payment_hash, log_iter!(custom_tlvs.iter().map(|(typ, _)| typ).filter(|typ| *typ % 2 == 0))); + return Err(payment.htlcs); + } + } + + let payment_id = payment.inbound_payment_id(inbound_payment_id_secret); + let claiming_payment = self.pending_claiming_payments + .entry(payment_hash) + .and_modify(|_| { + debug_assert!(false, "Shouldn't get a duplicate pending claim event ever"); + log_error!(logger, "Got a duplicate pending claimable event on payment hash {}! Please report this bug", + &payment_hash); + }) + .or_insert_with(|| { + let htlcs = payment.htlcs.iter().map(events::ClaimedHTLC::from).collect(); + let sender_intended_value = payment.htlcs.first().map(|htlc| htlc.total_msat); + ClaimingPayment { + amount_msat: payment.htlcs.iter().map(|source| source.value).sum(), + payment_purpose: payment.purpose, + receiver_node_id, + htlcs, + sender_intended_value, + onion_fields: payment.onion_fields, + payment_id: Some(payment_id), + } + }).clone(); + + Ok((payment.htlcs, claiming_payment)) + }, + None => Err(Vec::new()) + } + } +} + /// Events which we process internally but cannot be processed immediately at the generation site /// usually because we're running pre-full-init. They are handled immediately once we detect we are /// running normally, and specifically must be processed before any other non-background @@ -6700,60 +6767,24 @@ where let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); - let claiming_payment; - let sources = { - let mut claimable_payments = self.claimable_payments.lock().unwrap(); - if let Some(payment) = claimable_payments.claimable_payments.remove(&payment_hash) { - let mut receiver_node_id = self.our_network_pubkey; - for htlc in payment.htlcs.iter() { - if htlc.prev_hop.phantom_shared_secret.is_some() { - let phantom_pubkey = self.node_signer.get_node_id(Recipient::PhantomNode) - .expect("Failed to get node_id for phantom node recipient"); - receiver_node_id = phantom_pubkey; - break; - } - } - - let payment_id = payment.inbound_payment_id(&self.inbound_payment_id_secret); - claiming_payment = claimable_payments.pending_claiming_payments - .entry(payment_hash) - .and_modify(|_| { - debug_assert!(false, "Shouldn't get a duplicate pending claim event ever"); - log_error!(self.logger, "Got a duplicate pending claimable event on payment hash {}! Please report this bug", - &payment_hash); - }) - .or_insert_with(|| { - let htlcs = payment.htlcs.iter().map(events::ClaimedHTLC::from).collect(); - let sender_intended_value = payment.htlcs.first().map(|htlc| htlc.total_msat); - ClaimingPayment { - amount_msat: payment.htlcs.iter().map(|source| source.value).sum(), - payment_purpose: payment.purpose, - receiver_node_id, - htlcs, - sender_intended_value, - onion_fields: payment.onion_fields, - payment_id: Some(payment_id), - } - }).clone(); + let (sources, claiming_payment) = { + let res = self.claimable_payments.lock().unwrap().begin_claiming_payment( + payment_hash, &self.node_signer, &self.logger, &self.inbound_payment_id_secret, + custom_tlvs_known, + ); - if let Some(RecipientOnionFields { ref custom_tlvs, .. }) = claiming_payment.onion_fields { - if !custom_tlvs_known && custom_tlvs.iter().any(|(typ, _)| typ % 2 == 0) { - log_info!(self.logger, "Rejecting payment with payment hash {} as we cannot accept payment with unknown even TLVs: {}", - &payment_hash, log_iter!(custom_tlvs.iter().map(|(typ, _)| typ).filter(|typ| *typ % 2 == 0))); - claimable_payments.pending_claiming_payments.remove(&payment_hash); - mem::drop(claimable_payments); - for htlc in payment.htlcs { - let reason = self.get_htlc_fail_reason_from_failure_code(FailureCode::InvalidOnionPayload(None), &htlc); - let source = HTLCSource::PreviousHopData(htlc.prev_hop); - let receiver = HTLCDestination::FailedPayment { payment_hash }; - self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver); - } - return; + match res { + Ok((htlcs, payment_info)) => (htlcs, payment_info), + Err(htlcs) => { + for htlc in htlcs { + let reason = self.get_htlc_fail_reason_from_failure_code(FailureCode::InvalidOnionPayload(None), &htlc); + let source = HTLCSource::PreviousHopData(htlc.prev_hop); + let receiver = HTLCDestination::FailedPayment { payment_hash }; + self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver); } + return; } - - payment.htlcs - } else { return; } + } }; debug_assert!(!sources.is_empty()); From 254b78fd3553c977a00f9b81cf829e81427b0a04 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 15 Sep 2024 23:27:35 +0000 Subject: [PATCH 08/11] Handle duplicate payment claims during initialization In the next commit we'll start using (much of) the normal HTLC claim pipeline to replay payment claims on startup. In order to do so, however, we have to properly handle cases where we get a `DuplicateClaim` back from the channel for an inbound-payment HTLC. Here we do so, handling the `MonitorUpdateCompletionAction` and allowing an already-completed RAA blocker. --- lightning/src/ln/channelmanager.rs | 62 ++++++++++++++++++------------ 1 file changed, 37 insertions(+), 25 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index cda6b67e8f9..779ac25f860 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -6962,7 +6962,16 @@ where UpdateFulfillCommitFetch::DuplicateClaim {} => { let (action_opt, raa_blocker_opt) = completion_action(None, true); if let Some(raa_blocker) = raa_blocker_opt { - debug_assert!(peer_state.actions_blocking_raa_monitor_updates.get(&chan_id).unwrap().contains(&raa_blocker)); + // If we're making a claim during startup, its a replay of a + // payment claim from a `ChannelMonitor`. In some cases (MPP or + // if the HTLC was only recently removed) we make such claims + // after an HTLC has been removed from a channel entirely, and + // thus the RAA blocker has long since completed. + // + // In any other case, the RAA blocker must still be present and + // blocking RAAs. + debug_assert!(during_init || + peer_state.actions_blocking_raa_monitor_updates.get(&chan_id).unwrap().contains(&raa_blocker)); } let action = if let Some(action) = action_opt { action @@ -6974,38 +6983,41 @@ where log_trace!(logger, "Completing monitor update completion action for channel {} as claim was redundant: {:?}", chan_id, action); - let (node_id, _funding_outpoint, channel_id, blocker) = if let MonitorUpdateCompletionAction::FreeOtherChannelImmediately { downstream_counterparty_node_id: node_id, - downstream_funding_outpoint: funding_outpoint, + downstream_funding_outpoint: _, blocking_action: blocker, downstream_channel_id: channel_id, } = action { - (node_id, funding_outpoint, channel_id, blocker) + if let Some(peer_state_mtx) = per_peer_state.get(&node_id) { + let mut peer_state = peer_state_mtx.lock().unwrap(); + if let Some(blockers) = peer_state + .actions_blocking_raa_monitor_updates + .get_mut(&channel_id) + { + let mut found_blocker = false; + blockers.retain(|iter| { + // Note that we could actually be blocked, in + // which case we need to only remove the one + // blocker which was added duplicatively. + let first_blocker = !found_blocker; + if *iter == blocker { found_blocker = true; } + *iter != blocker || !first_blocker + }); + debug_assert!(found_blocker); + } + } else { + debug_assert!(false); + } + } else if matches!(action, MonitorUpdateCompletionAction::PaymentClaimed { .. }) { + debug_assert!(during_init, + "Duplicate claims should always either be for forwarded payments(freeing another channel immediately) or during init (for claim replay)"); + mem::drop(per_peer_state); + self.handle_monitor_update_completion_actions([action]); } else { debug_assert!(false, - "Duplicate claims should always free another channel immediately"); + "Duplicate claims should always either be for forwarded payments(freeing another channel immediately) or during init (for claim replay)"); return; }; - if let Some(peer_state_mtx) = per_peer_state.get(&node_id) { - let mut peer_state = peer_state_mtx.lock().unwrap(); - if let Some(blockers) = peer_state - .actions_blocking_raa_monitor_updates - .get_mut(&channel_id) - { - let mut found_blocker = false; - blockers.retain(|iter| { - // Note that we could actually be blocked, in - // which case we need to only remove the one - // blocker which was added duplicatively. - let first_blocker = !found_blocker; - if *iter == blocker { found_blocker = true; } - *iter != blocker || !first_blocker - }); - debug_assert!(found_blocker); - } - } else { - debug_assert!(false); - } } } } From 4896e20086ef555e70ec171278e66bd14788a265 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 30 Sep 2024 20:09:01 +0000 Subject: [PATCH 09/11] Replay MPP claims via background events using new CM metadata When we claim an MPP payment, then crash before persisting all the relevant `ChannelMonitor`s, we rely on the payment data being available in the `ChannelManager` on restart to re-claim any parts that haven't yet been claimed. This is fine as long as the `ChannelManager` was persisted before the `PaymentClaimable` event was processed, which is generally the case in our `lightning-background-processor`, but may not be in other cases or in a somewhat rare race. In order to fix this, we need to track where all the MPP parts of a payment are in the `ChannelMonitor`, allowing us to re-claim any missing pieces without reference to any `ChannelManager` data. Further, in order to properly generate a `PaymentClaimed` event against the re-started claim, we have to store various payment metadata with the HTLC list as well. Here we finally implement claiming using the new MPP part list and metadata stored in `ChannelMonitor`s. In doing so, we use much more of the existing HTLC-claiming pipeline in `ChannelManager`, utilizing the on-startup background events flow as well as properly re-applying the RAA-blockers to ensure preimages cannot be lost. --- lightning/src/ln/channel.rs | 2 +- lightning/src/ln/channelmanager.rs | 250 +++++++++++++++++++++-------- lightning/src/ln/reload_tests.rs | 27 +++- 3 files changed, 203 insertions(+), 76 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 42e4b2f3e72..7018896267b 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1290,7 +1290,7 @@ pub(super) struct ChannelContext where SP::Target: SignerProvider { // further `send_update_fee` calls, dropping the previous holding cell update entirely. holding_cell_update_fee: Option, next_holder_htlc_id: u64, - next_counterparty_htlc_id: u64, + pub(super) next_counterparty_htlc_id: u64, feerate_per_kw: u32, /// The timestamp set on our latest `channel_update` message for this channel. It is updated diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 779ac25f860..6f88985d9da 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1130,9 +1130,34 @@ impl_writeable_tlv_based_enum!(EventCompletionAction, } ); +/// The source argument which is passed to [`ChannelManager::claim_mpp_part`]. +/// +/// This is identical to [`MPPClaimHTLCSource`] except that [`Self::counterparty_node_id`] is an +/// `Option`, whereas it is required in [`MPPClaimHTLCSource`]. In the future, we should ideally +/// drop this and merge the two, however doing so may break upgrades for nodes which have pending +/// forwarded payments. +struct HTLCClaimSource { + counterparty_node_id: Option, + funding_txo: OutPoint, + channel_id: ChannelId, + htlc_id: u64, +} + +impl From<&MPPClaimHTLCSource> for HTLCClaimSource { + fn from(o: &MPPClaimHTLCSource) -> HTLCClaimSource { + HTLCClaimSource { + counterparty_node_id: Some(o.counterparty_node_id), + funding_txo: o.funding_txo, + channel_id: o.channel_id, + htlc_id: o.htlc_id, + } + } +} + #[derive(Clone, Debug, PartialEq, Eq)] /// The source of an HTLC which is being claimed as a part of an incoming payment. Each part is -/// tracked in [`PendingMPPClaim`]. +/// tracked in [`PendingMPPClaim`] as well as in [`ChannelMonitor`]s, so that it can be converted +/// to an [`HTLCClaimSource`] for claim replays on startup. struct MPPClaimHTLCSource { counterparty_node_id: PublicKey, funding_txo: OutPoint, @@ -6896,6 +6921,27 @@ 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 htlc_source = HTLCClaimSource { + counterparty_node_id, + funding_txo: prev_hop.outpoint, + channel_id: prev_hop.channel_id, + htlc_id: prev_hop.htlc_id, + }; + self.claim_mpp_part(htlc_source, payment_preimage, payment_info, completion_action) + } + + fn claim_mpp_part< + ComplFunc: FnOnce(Option, bool) -> (Option, Option) + >( + &self, prev_hop: HTLCClaimSource, payment_preimage: PaymentPreimage, + payment_info: Option, completion_action: ComplFunc, ) { //TODO: Delay the claimed_funds relaying just like we do outbound relay! @@ -6912,12 +6958,8 @@ where { let per_peer_state = self.per_peer_state.read().unwrap(); let chan_id = prev_hop.channel_id; - let counterparty_node_id_opt = 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 peer_state_opt = counterparty_node_id_opt.as_ref().map( + let peer_state_opt = prev_hop.counterparty_node_id.as_ref().map( |counterparty_node_id| per_peer_state.get(counterparty_node_id) .map(|peer_mutex| peer_mutex.lock().unwrap()) ).unwrap_or(None); @@ -6944,7 +6986,7 @@ where peer_state.actions_blocking_raa_monitor_updates.entry(chan_id).or_insert_with(Vec::new).push(raa_blocker); } if !during_init { - handle_new_monitor_update!(self, prev_hop.outpoint, monitor_update, peer_state_lock, + handle_new_monitor_update!(self, prev_hop.funding_txo, monitor_update, peer_state_lock, peer_state, per_peer_state, chan); } else { // If we're running during init we cannot update a monitor directly - @@ -6953,7 +6995,7 @@ where self.pending_background_events.lock().unwrap().push( BackgroundEvent::MonitorUpdateRegeneratedOnStartup { counterparty_node_id, - funding_txo: prev_hop.outpoint, + funding_txo: prev_hop.funding_txo, channel_id: prev_hop.channel_id, update: monitor_update.clone(), }); @@ -7027,7 +7069,7 @@ where } let preimage_update = ChannelMonitorUpdate { update_id: CLOSED_CHANNEL_UPDATE_ID, - counterparty_node_id: None, + counterparty_node_id: prev_hop.counterparty_node_id, updates: vec![ChannelMonitorUpdateStep::PaymentPreimage { payment_preimage, payment_info, @@ -7038,7 +7080,7 @@ where 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.outpoint, &preimage_update); + let update_res = self.chain_monitor.update_channel(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 @@ -7061,7 +7103,7 @@ where // complete the monitor update completion action from `completion_action`. self.pending_background_events.lock().unwrap().push( BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup(( - prev_hop.outpoint, prev_hop.channel_id, preimage_update, + prev_hop.funding_txo, prev_hop.channel_id, preimage_update, ))); } // Note that we do process the completion action here. This totally could be a @@ -7312,7 +7354,7 @@ where onion_fields, payment_id, }) = payment { - self.pending_events.lock().unwrap().push_back((events::Event::PaymentClaimed { + let event = events::Event::PaymentClaimed { payment_hash, purpose, amount_msat, @@ -7321,7 +7363,16 @@ where sender_intended_total_msat, onion_fields, payment_id, - }, None)); + }; + let event_action = (event, None); + let mut pending_events = self.pending_events.lock().unwrap(); + // If we're replaying a claim on startup we may end up duplicating an event + // that's already in our queue, so check before we push another one. The + // `payment_id` should suffice to ensure we never spuriously drop a second + // event for a duplicate payment. + if !pending_events.contains(&event_action) { + pending_events.push_back(event_action); + } } }, MonitorUpdateCompletionAction::EmitEventAndFreeOtherChannel { @@ -13130,67 +13181,126 @@ where }; for (_, monitor) in args.channel_monitors.iter() { - for (payment_hash, (payment_preimage, _)) in monitor.get_stored_preimages() { - let per_peer_state = channel_manager.per_peer_state.read().unwrap(); - let mut claimable_payments = channel_manager.claimable_payments.lock().unwrap(); - let payment = claimable_payments.claimable_payments.remove(&payment_hash); - mem::drop(claimable_payments); - if let Some(payment) = payment { - log_info!(channel_manager.logger, "Re-claiming HTLCs with payment hash {} as we've released the preimage to a ChannelMonitor!", &payment_hash); - let mut claimable_amt_msat = 0; - let mut receiver_node_id = Some(our_network_pubkey); - let phantom_shared_secret = payment.htlcs[0].prev_hop.phantom_shared_secret; - if phantom_shared_secret.is_some() { - let phantom_pubkey = channel_manager.node_signer.get_node_id(Recipient::PhantomNode) - .expect("Failed to get node_id for phantom node recipient"); - receiver_node_id = Some(phantom_pubkey) - } - for claimable_htlc in &payment.htlcs { - claimable_amt_msat += claimable_htlc.value; - - // Add a holding-cell claim of the payment to the Channel, which should be - // applied ~immediately on peer reconnection. Because it won't generate a - // new commitment transaction we can just provide the payment preimage to - // the corresponding ChannelMonitor and nothing else. - // - // We do so directly instead of via the normal ChannelMonitor update - // procedure as the ChainMonitor hasn't yet been initialized, implying - // we're not allowed to call it directly yet. Further, we do the update - // without incrementing the ChannelMonitor update ID as there isn't any - // reason to. - // If we were to generate a new ChannelMonitor update ID here and then - // crash before the user finishes block connect we'd end up force-closing - // this channel as well. On the flip side, there's no harm in restarting - // without the new monitor persisted - we'll end up right back here on - // restart. - let previous_channel_id = claimable_htlc.prev_hop.channel_id; - let peer_node_id_opt = channel_manager.outpoint_to_peer.lock().unwrap() - .get(&claimable_htlc.prev_hop.outpoint).cloned(); - if let Some(peer_node_id) = peer_node_id_opt { - let peer_state_mutex = per_peer_state.get(&peer_node_id).unwrap(); - let mut peer_state_lock = peer_state_mutex.lock().unwrap(); - let peer_state = &mut *peer_state_lock; - if let Some(ChannelPhase::Funded(channel)) = peer_state.channel_by_id.get_mut(&previous_channel_id) { - let logger = WithChannelContext::from(&channel_manager.logger, &channel.context, Some(payment_hash)); - channel.claim_htlc_while_disconnected_dropping_mon_update(claimable_htlc.prev_hop.htlc_id, payment_preimage, &&logger); + for (payment_hash, (payment_preimage, payment_claims)) in monitor.get_stored_preimages() { + if !payment_claims.is_empty() { + for payment_claim in payment_claims { + if payment_claim.mpp_parts.is_empty() { + return Err(DecodeError::InvalidValue); + } + let pending_claims = PendingMPPClaim { + channels_without_preimage: payment_claim.mpp_parts.clone(), + channels_with_preimage: Vec::new(), + }; + let pending_claim_ptr_opt = Some(Arc::new(Mutex::new(pending_claims))); + + // While it may be duplicative to generate a PaymentClaimed here, trying to + // figure out if the user definitely saw it before shutdown would require some + // nontrivial logic and may break as we move away from regularly persisting + // ChannelManager. Instead, we rely on the users' event handler being + // idempotent and just blindly generate one no matter what, letting the + // preimages eventually timing out from ChannelMonitors to prevent us from + // doing so forever. + + let claim_found = + channel_manager.claimable_payments.lock().unwrap().begin_claiming_payment( + payment_hash, &channel_manager.node_signer, &channel_manager.logger, + &channel_manager.inbound_payment_id_secret, true, + ); + if claim_found.is_err() { + let mut claimable_payments = channel_manager.claimable_payments.lock().unwrap(); + match claimable_payments.pending_claiming_payments.entry(payment_hash) { + hash_map::Entry::Occupied(_) => { + debug_assert!(false, "Entry was added in begin_claiming_payment"); + return Err(DecodeError::InvalidValue); + }, + hash_map::Entry::Vacant(entry) => { + entry.insert(payment_claim.claiming_payment); + }, } } - if let Some(previous_hop_monitor) = args.channel_monitors.get(&claimable_htlc.prev_hop.outpoint) { - previous_hop_monitor.provide_payment_preimage(&payment_hash, &payment_preimage, &channel_manager.tx_broadcaster, &channel_manager.fee_estimator, &channel_manager.logger); + + for part in payment_claim.mpp_parts.iter() { + let pending_mpp_claim = pending_claim_ptr_opt.as_ref().map(|ptr| ( + part.counterparty_node_id, part.channel_id, part.htlc_id, + PendingMPPClaimPointer(Arc::clone(&ptr)) + )); + let pending_claim_ptr = pending_claim_ptr_opt.as_ref().map(|ptr| + RAAMonitorUpdateBlockingAction::ClaimedMPPPayment { + pending_claim: PendingMPPClaimPointer(Arc::clone(&ptr)), + } + ); + // Note that we don't need to pass the `payment_info` here - its + // already (clearly) durably on disk in the `ChannelMonitor` so there's + // no need to worry about getting it into others. + channel_manager.claim_mpp_part( + part.into(), payment_preimage, None, + |_, _| + (Some(MonitorUpdateCompletionAction::PaymentClaimed { payment_hash, pending_mpp_claim }), pending_claim_ptr) + ); } } - let mut pending_events = channel_manager.pending_events.lock().unwrap(); - let payment_id = payment.inbound_payment_id(&inbound_payment_id_secret.unwrap()); - pending_events.push_back((events::Event::PaymentClaimed { - receiver_node_id, - payment_hash, - purpose: payment.purpose, - amount_msat: claimable_amt_msat, - htlcs: payment.htlcs.iter().map(events::ClaimedHTLC::from).collect(), - sender_intended_total_msat: payment.htlcs.first().map(|htlc| htlc.total_msat), - onion_fields: payment.onion_fields, - payment_id: Some(payment_id), - }, None)); + } else { + let per_peer_state = channel_manager.per_peer_state.read().unwrap(); + let mut claimable_payments = channel_manager.claimable_payments.lock().unwrap(); + let payment = claimable_payments.claimable_payments.remove(&payment_hash); + mem::drop(claimable_payments); + if let Some(payment) = payment { + log_info!(channel_manager.logger, "Re-claiming HTLCs with payment hash {} as we've released the preimage to a ChannelMonitor!", &payment_hash); + let mut claimable_amt_msat = 0; + let mut receiver_node_id = Some(our_network_pubkey); + let phantom_shared_secret = payment.htlcs[0].prev_hop.phantom_shared_secret; + if phantom_shared_secret.is_some() { + let phantom_pubkey = channel_manager.node_signer.get_node_id(Recipient::PhantomNode) + .expect("Failed to get node_id for phantom node recipient"); + receiver_node_id = Some(phantom_pubkey) + } + for claimable_htlc in &payment.htlcs { + claimable_amt_msat += claimable_htlc.value; + + // Add a holding-cell claim of the payment to the Channel, which should be + // applied ~immediately on peer reconnection. Because it won't generate a + // new commitment transaction we can just provide the payment preimage to + // the corresponding ChannelMonitor and nothing else. + // + // We do so directly instead of via the normal ChannelMonitor update + // procedure as the ChainMonitor hasn't yet been initialized, implying + // we're not allowed to call it directly yet. Further, we do the update + // without incrementing the ChannelMonitor update ID as there isn't any + // reason to. + // If we were to generate a new ChannelMonitor update ID here and then + // crash before the user finishes block connect we'd end up force-closing + // this channel as well. On the flip side, there's no harm in restarting + // without the new monitor persisted - we'll end up right back here on + // restart. + let previous_channel_id = claimable_htlc.prev_hop.channel_id; + let peer_node_id_opt = channel_manager.outpoint_to_peer.lock().unwrap() + .get(&claimable_htlc.prev_hop.outpoint).cloned(); + if let Some(peer_node_id) = peer_node_id_opt { + let peer_state_mutex = per_peer_state.get(&peer_node_id).unwrap(); + let mut peer_state_lock = peer_state_mutex.lock().unwrap(); + let peer_state = &mut *peer_state_lock; + if let Some(ChannelPhase::Funded(channel)) = peer_state.channel_by_id.get_mut(&previous_channel_id) { + let logger = WithChannelContext::from(&channel_manager.logger, &channel.context, Some(payment_hash)); + channel.claim_htlc_while_disconnected_dropping_mon_update(claimable_htlc.prev_hop.htlc_id, payment_preimage, &&logger); + } + } + if let Some(previous_hop_monitor) = args.channel_monitors.get(&claimable_htlc.prev_hop.outpoint) { + previous_hop_monitor.provide_payment_preimage(&payment_hash, &payment_preimage, &channel_manager.tx_broadcaster, &channel_manager.fee_estimator, &channel_manager.logger); + } + } + let mut pending_events = channel_manager.pending_events.lock().unwrap(); + let payment_id = payment.inbound_payment_id(&inbound_payment_id_secret.unwrap()); + pending_events.push_back((events::Event::PaymentClaimed { + receiver_node_id, + payment_hash, + purpose: payment.purpose, + amount_msat: claimable_amt_msat, + htlcs: payment.htlcs.iter().map(events::ClaimedHTLC::from).collect(), + sender_intended_total_msat: payment.htlcs.first().map(|htlc| htlc.total_msat), + onion_fields: payment.onion_fields, + payment_id: Some(payment_id), + }, None)); + } } } } diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs index 6ae465b324e..a043d5c0c86 100644 --- a/lightning/src/ln/reload_tests.rs +++ b/lightning/src/ln/reload_tests.rs @@ -878,27 +878,39 @@ fn do_test_partial_claim_before_restart(persist_both_monitors: bool) { // Now restart nodes[3]. reload_node!(nodes[3], original_manager, &[&updated_monitor.0, &original_monitor.0], persister, new_chain_monitor, nodes_3_deserialized); - // On startup the preimage should have been copied into the non-persisted monitor: + // Until the startup background events are processed (in `get_and_clear_pending_events`, + // below), the preimage is not copied to the non-persisted monitor... assert!(get_monitor!(nodes[3], chan_id_persisted).get_stored_preimages().contains_key(&payment_hash)); - assert!(get_monitor!(nodes[3], chan_id_not_persisted).get_stored_preimages().contains_key(&payment_hash)); + assert_eq!( + get_monitor!(nodes[3], chan_id_not_persisted).get_stored_preimages().contains_key(&payment_hash), + persist_both_monitors, + ); nodes[1].node.peer_disconnected(nodes[3].node.get_our_node_id()); nodes[2].node.peer_disconnected(nodes[3].node.get_our_node_id()); // During deserialization, we should have closed one channel and broadcast its latest // commitment transaction. We should also still have the original PaymentClaimable event we - // never finished processing. + // never finished processing as well as a PaymentClaimed event regenerated when we replayed the + // preimage onto the non-persisted monitor. let events = nodes[3].node.get_and_clear_pending_events(); assert_eq!(events.len(), if persist_both_monitors { 4 } else { 3 }); if let Event::PaymentClaimable { amount_msat: 15_000_000, .. } = events[0] { } else { panic!(); } if let Event::ChannelClosed { reason: ClosureReason::OutdatedChannelManager, .. } = events[1] { } else { panic!(); } if persist_both_monitors { if let Event::ChannelClosed { reason: ClosureReason::OutdatedChannelManager, .. } = events[2] { } else { panic!(); } - check_added_monitors(&nodes[3], 2); + if let Event::PaymentClaimed { amount_msat: 15_000_000, .. } = events[3] { } else { panic!(); } + check_added_monitors(&nodes[3], 6); } else { - check_added_monitors(&nodes[3], 1); + if let Event::PaymentClaimed { amount_msat: 15_000_000, .. } = events[2] { } else { panic!(); } + check_added_monitors(&nodes[3], 3); } + // Now that we've processed background events, the preimage should have been copied into the + // non-persisted monitor: + assert!(get_monitor!(nodes[3], chan_id_persisted).get_stored_preimages().contains_key(&payment_hash)); + assert!(get_monitor!(nodes[3], chan_id_not_persisted).get_stored_preimages().contains_key(&payment_hash)); + // On restart, we should also get a duplicate PaymentClaimed event as we persisted the // ChannelManager prior to handling the original one. if let Event::PaymentClaimed { payment_hash: our_payment_hash, amount_msat: 15_000_000, .. } = @@ -948,6 +960,11 @@ fn do_test_partial_claim_before_restart(persist_both_monitors: bool) { nodes[0].node.handle_update_fulfill_htlc(nodes[2].node.get_our_node_id(), &cs_updates.update_fulfill_htlcs[0]); commitment_signed_dance!(nodes[0], nodes[2], cs_updates.commitment_signed, false, true); expect_payment_sent!(nodes[0], payment_preimage); + + // Ensure that the remaining channel is fully operation and not blocked (and that after a + // cycle of commitment updates the payment preimage is ultimately pruned). + send_payment(&nodes[0], &[&nodes[2], &nodes[3]], 100_000); + assert!(!get_monitor!(nodes[3], chan_id_not_persisted).get_stored_preimages().contains_key(&payment_hash)); } } From 6c203d803e51ce5c5d1014646dc2d9372337c4fb Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 20 Jun 2024 15:17:10 +0000 Subject: [PATCH 10/11] Stop relying on `ChannelMonitor` persistence after manager read When we discover we've only partially claimed an MPP HTLC during `ChannelManager` reading, we need to add the payment preimage to all other `ChannelMonitor`s that were a part of the payment. We previously did this with a direct call on the `ChannelMonitor`, requiring users write the full `ChannelMonitor` to disk to ensure that updated information made it. This adds quite a bit of delay during initial startup - fully resilvering each `ChannelMonitor` just to handle this one case is incredibly excessive. Over the past few commits we dropped the need to pass HTLCs directly to the `ChannelMonitor`s using the background events to provide `ChannelMonitorUpdate`s insetad. Thus, here we finally drop the requirement to resilver `ChannelMonitor`s on startup. --- fuzz/src/chanmon_consistency.rs | 2 +- lightning/src/ln/channelmanager.rs | 15 +++++++++------ lightning/src/ln/functional_test_utils.rs | 4 ++-- lightning/src/ln/reload_tests.rs | 4 ++-- pending_changelog/3322-b.txt | 7 +++++++ 5 files changed, 21 insertions(+), 11 deletions(-) create mode 100644 pending_changelog/3322-b.txt diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index f897ba6e092..821acfc5301 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -768,7 +768,7 @@ pub fn do_test(data: &[u8], underlying_out: Out, anchors: bool) { chain_monitor.latest_monitors.lock().unwrap().insert(outpoint, prev_state); } let mut monitor_refs = new_hash_map(); - for (outpoint, monitor) in monitors.iter_mut() { + for (outpoint, monitor) in monitors.iter() { monitor_refs.insert(*outpoint, monitor); } diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 6f88985d9da..dc07165113e 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1629,7 +1629,7 @@ where /// let mut channel_monitors = read_channel_monitors(); /// let args = ChannelManagerReadArgs::new( /// entropy_source, node_signer, signer_provider, fee_estimator, chain_monitor, tx_broadcaster, -/// router, message_router, logger, default_config, channel_monitors.iter_mut().collect(), +/// router, message_router, logger, default_config, channel_monitors.iter().collect(), /// ); /// let (block_hash, channel_manager) = /// <(BlockHash, ChannelManager<_, _, _, _, _, _, _, _, _>)>::read(&mut reader, args)?; @@ -12231,9 +12231,12 @@ impl Readable for VecDeque<(Event, Option)> { /// 3) If you are not fetching full blocks, register all relevant [`ChannelMonitor`] outpoints the /// same way you would handle a [`chain::Filter`] call using /// [`ChannelMonitor::get_outputs_to_watch`] and [`ChannelMonitor::get_funding_txo`]. -/// 4) Reconnect blocks on your [`ChannelMonitor`]s. -/// 5) Disconnect/connect blocks on the [`ChannelManager`]. -/// 6) Re-persist the [`ChannelMonitor`]s to ensure the latest state is on disk. +/// 4) Disconnect/connect blocks on your [`ChannelMonitor`]s to get them in sync with the chain. +/// 5) Disconnect/connect blocks on the [`ChannelManager`] to get it in sync with the chain. +/// 6) Optionally re-persist the [`ChannelMonitor`]s to ensure the latest state is on disk. +/// This is important if you have replayed a nontrivial number of blocks in step (4), allowing +/// you to avoid having to replay the same blocks if you shut down quickly after startup. It is +/// otherwise not required. /// Note that if you're using a [`ChainMonitor`] for your [`chain::Watch`] implementation, you /// will likely accomplish this as a side-effect of calling [`chain::Watch::watch_channel`] in /// the next step. @@ -12316,7 +12319,7 @@ where /// this struct. /// /// This is not exported to bindings users because we have no HashMap bindings - pub channel_monitors: HashMap::EcdsaSigner>>, + pub channel_monitors: HashMap::EcdsaSigner>>, } impl<'a, M: Deref, T: Deref, ES: Deref, NS: Deref, SP: Deref, F: Deref, R: Deref, MR: Deref, L: Deref> @@ -12339,7 +12342,7 @@ where entropy_source: ES, node_signer: NS, signer_provider: SP, fee_estimator: F, chain_monitor: M, tx_broadcaster: T, router: R, message_router: MR, logger: L, default_config: UserConfig, - mut channel_monitors: Vec<&'a mut ChannelMonitor<::EcdsaSigner>>, + mut channel_monitors: Vec<&'a ChannelMonitor<::EcdsaSigner>>, ) -> Self { Self { entropy_source, node_signer, signer_provider, fee_estimator, chain_monitor, diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 70c64fd2192..de0b4c7d4bb 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -686,7 +686,7 @@ impl<'a, 'b, 'c> Drop for Node<'a, 'b, 'c> { // them to ensure we can write and reload our ChannelManager. { let mut channel_monitors = new_hash_map(); - for monitor in deserialized_monitors.iter_mut() { + for monitor in deserialized_monitors.iter() { channel_monitors.insert(monitor.get_funding_txo().0, monitor); } @@ -1128,7 +1128,7 @@ pub fn _reload_node<'a, 'b, 'c>(node: &'a Node<'a, 'b, 'c>, default_config: User let mut node_read = &chanman_encoded[..]; let (_, node_deserialized) = { let mut channel_monitors = new_hash_map(); - for monitor in monitors_read.iter_mut() { + for monitor in monitors_read.iter() { assert!(channel_monitors.insert(monitor.get_funding_txo().0, monitor).is_none()); } <(BlockHash, TestChannelManager<'b, 'c>)>::read(&mut node_read, ChannelManagerReadArgs { diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs index a043d5c0c86..40f7c785bc5 100644 --- a/lightning/src/ln/reload_tests.rs +++ b/lightning/src/ln/reload_tests.rs @@ -426,7 +426,7 @@ fn test_manager_serialize_deserialize_inconsistent_monitor() { chain_monitor: nodes[0].chain_monitor, tx_broadcaster: nodes[0].tx_broadcaster, logger: &logger, - channel_monitors: node_0_stale_monitors.iter_mut().map(|monitor| { (monitor.get_funding_txo().0, monitor) }).collect(), + channel_monitors: node_0_stale_monitors.iter().map(|monitor| { (monitor.get_funding_txo().0, monitor) }).collect(), }) { } else { panic!("If the monitor(s) are stale, this indicates a bug and we should get an Err return"); }; @@ -444,7 +444,7 @@ fn test_manager_serialize_deserialize_inconsistent_monitor() { chain_monitor: nodes[0].chain_monitor, tx_broadcaster: nodes[0].tx_broadcaster, logger: &logger, - channel_monitors: node_0_monitors.iter_mut().map(|monitor| { (monitor.get_funding_txo().0, monitor) }).collect(), + channel_monitors: node_0_monitors.iter().map(|monitor| { (monitor.get_funding_txo().0, monitor) }).collect(), }).unwrap(); nodes_0_deserialized = nodes_0_deserialized_tmp; assert!(nodes_0_read.is_empty()); diff --git a/pending_changelog/3322-b.txt b/pending_changelog/3322-b.txt new file mode 100644 index 00000000000..c8bb0c64bd9 --- /dev/null +++ b/pending_changelog/3322-b.txt @@ -0,0 +1,7 @@ +API Updates +=========== + +As a part of adding robustness against several unlikely scenarios, redundant `PaymentClaimed` +`Event`s will be generated more frequently on startup for payments received on LDK 0.1 and +newer. A new `Event::PaymentClaimed::payment_id` field may be used to better differentiate +between redundant payments. From ba264323f83495ae3cc862cb06c27311fe20f1bd Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 15 Sep 2024 17:24:19 +0000 Subject: [PATCH 11/11] Doc the on-upgrade `ChannelMonitor` startup persistence semantics Because the new startup `ChannelMonitor` persistence semantics rely on new information stored in `ChannelMonitor` only for claims made in the upgraded code, users upgrading from previous version of LDK must apply the old `ChannelMonitor` persistence semantics at least once (as the old code will be used to handle partial claims). --- lightning/src/chain/channelmonitor.rs | 14 +++++++++++-- lightning/src/ln/channel.rs | 4 +++- lightning/src/ln/channelmanager.rs | 21 +++++++++++++++++-- lightning/src/ln/functional_tests.rs | 5 ++++- lightning/src/ln/monitor_tests.rs | 12 ++++++----- lightning/src/ln/reload_tests.rs | 6 ++++-- lightning/src/ln/reorg_tests.rs | 4 ++-- .../matt-persist-preimage-on-upgrade.txt | 8 +++++++ 8 files changed, 59 insertions(+), 15 deletions(-) create mode 100644 pending_changelog/matt-persist-preimage-on-upgrade.txt diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 6513e7b8e68..2d76c09f1bb 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1501,7 +1501,15 @@ impl ChannelMonitor { /// This is used to provide payment preimage(s) out-of-band during startup without updating the /// off-chain state with a new commitment transaction. - pub(crate) fn provide_payment_preimage( + /// + /// It is used only for legacy (created prior to LDK 0.1) pending payments on upgrade, and the + /// flow that uses it assumes that this [`ChannelMonitor`] is persisted prior to the + /// [`ChannelManager`] being persisted (as the state necessary to call this method again is + /// removed from the [`ChannelManager`] and thus a persistence inversion would imply we do not + /// get the preimage back into this [`ChannelMonitor`] on startup). + /// + /// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager + pub(crate) fn provide_payment_preimage_unsafe_legacy( &self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage, @@ -5279,7 +5287,9 @@ mod tests { preimages_slice_to_htlc_outputs!(preimages[15..20]), 281474976710654, dummy_key, &logger); for &(ref preimage, ref hash) in preimages.iter() { let bounded_fee_estimator = LowerBoundedFeeEstimator::new(&fee_estimator); - monitor.provide_payment_preimage(hash, preimage, &broadcaster, &bounded_fee_estimator, &logger); + monitor.provide_payment_preimage_unsafe_legacy( + hash, preimage, &broadcaster, &bounded_fee_estimator, &logger + ); } // Now provide a secret, pruning preimages 10-15 diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 7018896267b..cd9d5bf8b2c 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4027,12 +4027,14 @@ impl Channel where /// Claims an HTLC while we're disconnected from a peer, dropping the [`ChannelMonitorUpdate`] /// entirely. /// + /// This is only used for payments received prior to LDK 0.1. + /// /// The [`ChannelMonitor`] for this channel MUST be updated out-of-band with the preimage /// provided (i.e. without calling [`crate::chain::Watch::update_channel`]). /// /// The HTLC claim will end up in the holding cell (because the caller must ensure the peer is /// disconnected). - pub fn claim_htlc_while_disconnected_dropping_mon_update + pub fn claim_htlc_while_disconnected_dropping_mon_update_legacy (&mut self, htlc_id_arg: u64, payment_preimage_arg: PaymentPreimage, logger: &L) where L::Target: Logger { // Assert that we'll add the HTLC claim to the holding cell in `get_update_fulfill_htlc` diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index dc07165113e..f31c23b5a67 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -13284,11 +13284,28 @@ where let peer_state = &mut *peer_state_lock; if let Some(ChannelPhase::Funded(channel)) = peer_state.channel_by_id.get_mut(&previous_channel_id) { let logger = WithChannelContext::from(&channel_manager.logger, &channel.context, Some(payment_hash)); - channel.claim_htlc_while_disconnected_dropping_mon_update(claimable_htlc.prev_hop.htlc_id, payment_preimage, &&logger); + channel.claim_htlc_while_disconnected_dropping_mon_update_legacy( + claimable_htlc.prev_hop.htlc_id, payment_preimage, &&logger + ); } } if let Some(previous_hop_monitor) = args.channel_monitors.get(&claimable_htlc.prev_hop.outpoint) { - previous_hop_monitor.provide_payment_preimage(&payment_hash, &payment_preimage, &channel_manager.tx_broadcaster, &channel_manager.fee_estimator, &channel_manager.logger); + // Note that this is unsafe as we no longer require the + // `ChannelMonitor`s to be re-persisted prior to this + // `ChannelManager` being persisted after we get started running. + // If this `ChannelManager` gets persisted first then we crash, we + // won't have the `claimable_payments` entry we need to re-enter + // this code block, causing us to not re-apply the preimage to this + // `ChannelMonitor`. + // + // We should never be here with modern payment claims, however, as + // they should always include the HTLC list. Instead, this is only + // for nodes during upgrade, and we explicitly require the old + // persistence semantics on upgrade in the release notes. + previous_hop_monitor.provide_payment_preimage_unsafe_legacy( + &payment_hash, &payment_preimage, &channel_manager.tx_broadcaster, + &channel_manager.fee_estimator, &channel_manager.logger + ); } } let mut pending_events = channel_manager.pending_events.lock().unwrap(); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 42a57b114cb..ee73b30baef 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -3702,7 +3702,10 @@ fn test_force_close_fail_back() { // Now check that if we add the preimage to ChannelMonitor it broadcasts our HTLC-Success.. { get_monitor!(nodes[2], payment_event.commitment_msg.channel_id) - .provide_payment_preimage(&our_payment_hash, &our_payment_preimage, &node_cfgs[2].tx_broadcaster, &LowerBoundedFeeEstimator::new(node_cfgs[2].fee_estimator), &node_cfgs[2].logger); + .provide_payment_preimage_unsafe_legacy( + &our_payment_hash, &our_payment_preimage, &node_cfgs[2].tx_broadcaster, + &LowerBoundedFeeEstimator::new(node_cfgs[2].fee_estimator), &node_cfgs[2].logger + ); } mine_transaction(&nodes[2], &commitment_tx); let mut node_txn = nodes[2].tx_broadcaster.txn_broadcast(); diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index dfc6c02b61a..4aad8f569fc 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -1883,8 +1883,10 @@ fn do_test_revoked_counterparty_aggregated_claims(anchors: bool) { // Cheat by giving A's ChannelMonitor the preimage to the to-be-claimed HTLC so that we have an // HTLC-claim transaction on the to-be-revoked state. - get_monitor!(nodes[0], chan_id).provide_payment_preimage(&claimed_payment_hash, &claimed_payment_preimage, - &node_cfgs[0].tx_broadcaster, &LowerBoundedFeeEstimator::new(node_cfgs[0].fee_estimator), &nodes[0].logger); + get_monitor!(nodes[0], chan_id).provide_payment_preimage_unsafe_legacy( + &claimed_payment_hash, &claimed_payment_preimage, &node_cfgs[0].tx_broadcaster, + &LowerBoundedFeeEstimator::new(node_cfgs[0].fee_estimator), &nodes[0].logger + ); // Now get the latest commitment transaction from A and then update the fee to revoke it let as_revoked_txn = get_local_commitment_txn!(nodes[0], chan_id); @@ -2507,11 +2509,11 @@ fn do_test_yield_anchors_events(have_htlcs: bool) { } if have_htlcs { - get_monitor!(nodes[0], chan_id).provide_payment_preimage( + get_monitor!(nodes[0], chan_id).provide_payment_preimage_unsafe_legacy( &payment_hash_2.unwrap(), &payment_preimage_2.unwrap(), &node_cfgs[0].tx_broadcaster, &LowerBoundedFeeEstimator::new(node_cfgs[0].fee_estimator), &nodes[0].logger ); - get_monitor!(nodes[1], chan_id).provide_payment_preimage( + get_monitor!(nodes[1], chan_id).provide_payment_preimage_unsafe_legacy( &payment_hash_1.unwrap(), &payment_preimage_1.unwrap(), &node_cfgs[1].tx_broadcaster, &LowerBoundedFeeEstimator::new(node_cfgs[1].fee_estimator), &nodes[1].logger ); @@ -2706,7 +2708,7 @@ fn test_anchors_aggregated_revoked_htlc_tx() { for chan_id in [chan_a.2, chan_b.2].iter() { let monitor = get_monitor!(nodes[1], chan_id); for payment in [payment_a, payment_b, payment_c, payment_d].iter() { - monitor.provide_payment_preimage( + monitor.provide_payment_preimage_unsafe_legacy( &payment.1, &payment.0, &node_cfgs[1].tx_broadcaster, &LowerBoundedFeeEstimator::new(node_cfgs[1].fee_estimator), &nodes[1].logger ); diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs index 40f7c785bc5..c32fca6bd75 100644 --- a/lightning/src/ln/reload_tests.rs +++ b/lightning/src/ln/reload_tests.rs @@ -1041,8 +1041,10 @@ fn do_forwarded_payment_no_manager_persistence(use_cs_commitment: bool, claim_ht check_added_monitors!(nodes[2], 1); if claim_htlc { - get_monitor!(nodes[2], chan_id_2).provide_payment_preimage(&payment_hash, &payment_preimage, - &nodes[2].tx_broadcaster, &LowerBoundedFeeEstimator(nodes[2].fee_estimator), &nodes[2].logger); + get_monitor!(nodes[2], chan_id_2).provide_payment_preimage_unsafe_legacy( + &payment_hash, &payment_preimage, &nodes[2].tx_broadcaster, + &LowerBoundedFeeEstimator(nodes[2].fee_estimator), &nodes[2].logger + ); } assert!(nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty()); diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs index a05e40300fc..c629c5bbce6 100644 --- a/lightning/src/ln/reorg_tests.rs +++ b/lightning/src/ln/reorg_tests.rs @@ -674,7 +674,7 @@ fn test_htlc_preimage_claim_holder_commitment_after_counterparty_commitment_reor // Provide the preimage now, such that we only claim from the holder commitment (since it's // currently confirmed) and not the counterparty's. - get_monitor!(nodes[1], chan_id).provide_payment_preimage( + get_monitor!(nodes[1], chan_id).provide_payment_preimage_unsafe_legacy( &payment_hash, &payment_preimage, &nodes[1].tx_broadcaster, &LowerBoundedFeeEstimator(nodes[1].fee_estimator), &nodes[1].logger ); @@ -749,7 +749,7 @@ fn test_htlc_preimage_claim_prev_counterparty_commitment_after_current_counterpa // Provide the preimage now, such that we only claim from the previous commitment (since it's // currently confirmed) and not the latest. - get_monitor!(nodes[1], chan_id).provide_payment_preimage( + get_monitor!(nodes[1], chan_id).provide_payment_preimage_unsafe_legacy( &payment_hash, &payment_preimage, &nodes[1].tx_broadcaster, &LowerBoundedFeeEstimator(nodes[1].fee_estimator), &nodes[1].logger ); diff --git a/pending_changelog/matt-persist-preimage-on-upgrade.txt b/pending_changelog/matt-persist-preimage-on-upgrade.txt new file mode 100644 index 00000000000..fc53469c6f6 --- /dev/null +++ b/pending_changelog/matt-persist-preimage-on-upgrade.txt @@ -0,0 +1,8 @@ +# Backwards Compatibility + * The `ChannelManager` deserialization semantics no longer require that + `ChannelMonitor`s be re-persisted after `(BlockHash, ChannelManager)::read` + is called prior to normal node operation. This applies to upgraded nodes + only *after* a startup with the old semantics completes at least once. IOW, + you must deserialize the `ChannelManager` with upgraded LDK, persist the + `ChannelMonitor`s then continue to normal startup once, and thereafter you + may skip the `ChannelMonitor` persistence step.