From 7790e30880f955d0e378ffd4e4062a7b4747c361 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 16 Sep 2024 00:07:48 +0000 Subject: [PATCH] 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 428221d8d..6513e7b8e 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 699430749..9fdfc0eb7 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 5c1a82d4b..c37c32679 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); -- 2.39.5