Stop relying on ChannelMonitor persistence after manager read

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 \

fuzz/src/chanmon_consistency.rs

@@ -768,7 +768,7 @@ pub fn do_test<Out: Output>(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);
  }
 

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<PaymentClaimDetails>,
 	},
 	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),
@@ -919,8 +923,16 @@ pub(crate) struct ChannelMonitorImpl<Signer: EcdsaChannelSigner> {
 	/// 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<PaymentHash, PaymentPreimage>,
+	/// 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<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)>,
 
 	// 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
@@ -1146,7 +1158,7 @@ impl<Signer: EcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signer> {
  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[..])?;
  }
 
@@ -1224,6 +1236,7 @@ impl<Signer: EcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signer> {
  (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(())
@@ -1488,7 +1501,15 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 
 	/// 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<B: Deref, F: Deref, L: Deref>(
+	///
+	/// 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<B: Deref, F: Deref, L: Deref>(
  &self,
  payment_hash: &PaymentHash,
  payment_preimage: &PaymentPreimage,
@@ -1502,8 +1523,11 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 	{
  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
@@ -2194,7 +2218,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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
@@ -2415,7 +2439,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
  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;
@@ -2570,7 +2594,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
  res
 	}
 
-	pub(crate) fn get_stored_preimages(&self) -> HashMap<PaymentHash, PaymentPreimage> {
+	pub(crate) fn get_stored_preimages(&self) -> HashMap<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)> {
  self.inner.lock().unwrap().payment_preimages.clone()
 	}
 }
@@ -2929,14 +2953,27 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 
 	/// 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<B: Deref, F: Deref, L: Deref>(
- &mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage, broadcaster: &B,
+ &mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage,
+ payment_info: &Option<PaymentClaimDetails>, broadcaster: &B,
  fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &WithChannelMonitor<L>)
 	where B::Target: BroadcasterInterface,
  F::Target: FeeEstimator,
  L::Target: Logger,
 	{
- 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 {
@@ -3139,9 +3176,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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");
@@ -3593,7 +3630,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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(
@@ -3681,7 +3718,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  );
  (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
@@ -3835,7 +3872,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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;
  }
@@ -4808,7 +4845,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);
  }
  }
@@ -4891,6 +4928,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<HashMap<_, _>> = None;
  read_tlv_fields!(reader, {
  (1, funding_spend_confirmed, option),
  (3, htlcs_resolved_on_chain, optional_vec),
@@ -4904,7 +4942,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`.
@@ -5097,8 +5152,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!(
@@ -5228,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

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 \

fuzz/src/chanmon_consistency.rs

@@ -768,7 +768,7 @@ pub fn do_test<Out: Output>(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);
  }
 

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<PaymentClaimDetails>,
 	},
 	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),
@@ -919,8 +923,16 @@ pub(crate) struct ChannelMonitorImpl<Signer: EcdsaChannelSigner> {
 	/// 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<PaymentHash, PaymentPreimage>,
+	/// 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<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)>,
 
 	// 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
@@ -1146,7 +1158,7 @@ impl<Signer: EcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signer> {
  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[..])?;
  }
 
@@ -1224,6 +1236,7 @@ impl<Signer: EcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signer> {
  (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(())
@@ -1488,7 +1501,15 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 
 	/// 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<B: Deref, F: Deref, L: Deref>(
+	///
+	/// 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<B: Deref, F: Deref, L: Deref>(
  &self,
  payment_hash: &PaymentHash,
  payment_preimage: &PaymentPreimage,
@@ -1502,8 +1523,11 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 	{
  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
@@ -2194,7 +2218,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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
@@ -2415,7 +2439,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
  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;
@@ -2570,7 +2594,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
  res
 	}
 
-	pub(crate) fn get_stored_preimages(&self) -> HashMap<PaymentHash, PaymentPreimage> {
+	pub(crate) fn get_stored_preimages(&self) -> HashMap<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)> {
  self.inner.lock().unwrap().payment_preimages.clone()
 	}
 }
@@ -2929,14 +2953,27 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 
 	/// 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<B: Deref, F: Deref, L: Deref>(
- &mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage, broadcaster: &B,
+ &mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage,
+ payment_info: &Option<PaymentClaimDetails>, broadcaster: &B,
  fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &WithChannelMonitor<L>)
 	where B::Target: BroadcasterInterface,
  F::Target: FeeEstimator,
  L::Target: Logger,
 	{
- 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 {
@@ -3139,9 +3176,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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");
@@ -3593,7 +3630,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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(
@@ -3681,7 +3718,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  );
  (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
@@ -3835,7 +3872,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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;
  }
@@ -4808,7 +4845,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);
  }
  }
@@ -4891,6 +4928,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<HashMap<_, _>> = None;
  read_tlv_fields!(reader, {
  (1, funding_spend_confirmed, option),
  (3, htlcs_resolved_on_chain, optional_vec),
@@ -4904,7 +4942,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`.
@@ -5097,8 +5152,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!(
@@ -5228,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

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 \

fuzz/src/chanmon_consistency.rs

@@ -768,7 +768,7 @@ pub fn do_test<Out: Output>(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);
  }
 

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<PaymentClaimDetails>,
 	},
 	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),
@@ -919,8 +923,16 @@ pub(crate) struct ChannelMonitorImpl<Signer: EcdsaChannelSigner> {
 	/// 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<PaymentHash, PaymentPreimage>,
+	/// 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<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)>,
 
 	// 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
@@ -1146,7 +1158,7 @@ impl<Signer: EcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signer> {
  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[..])?;
  }
 
@@ -1224,6 +1236,7 @@ impl<Signer: EcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signer> {
  (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(())
@@ -1488,7 +1501,15 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 
 	/// 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<B: Deref, F: Deref, L: Deref>(
+	///
+	/// 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<B: Deref, F: Deref, L: Deref>(
  &self,
  payment_hash: &PaymentHash,
  payment_preimage: &PaymentPreimage,
@@ -1502,8 +1523,11 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 	{
  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
@@ -2194,7 +2218,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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
@@ -2415,7 +2439,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
  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;
@@ -2570,7 +2594,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
  res
 	}
 
-	pub(crate) fn get_stored_preimages(&self) -> HashMap<PaymentHash, PaymentPreimage> {
+	pub(crate) fn get_stored_preimages(&self) -> HashMap<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)> {
  self.inner.lock().unwrap().payment_preimages.clone()
 	}
 }
@@ -2929,14 +2953,27 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 
 	/// 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<B: Deref, F: Deref, L: Deref>(
- &mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage, broadcaster: &B,
+ &mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage,
+ payment_info: &Option<PaymentClaimDetails>, broadcaster: &B,
  fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &WithChannelMonitor<L>)
 	where B::Target: BroadcasterInterface,
  F::Target: FeeEstimator,
  L::Target: Logger,
 	{
- 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 {
@@ -3139,9 +3176,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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");
@@ -3593,7 +3630,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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(
@@ -3681,7 +3718,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  );
  (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
@@ -3835,7 +3872,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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;
  }
@@ -4808,7 +4845,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);
  }
  }
@@ -4891,6 +4928,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<HashMap<_, _>> = None;
  read_tlv_fields!(reader, {
  (1, funding_spend_confirmed, option),
  (3, htlcs_resolved_on_chain, optional_vec),
@@ -4904,7 +4942,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`.
@@ -5097,8 +5152,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!(
@@ -5228,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

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 \

fuzz/src/chanmon_consistency.rs

@@ -768,7 +768,7 @@ pub fn do_test<Out: Output>(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);
  }
 

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<PaymentClaimDetails>,
 	},
 	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),
@@ -919,8 +923,16 @@ pub(crate) struct ChannelMonitorImpl<Signer: EcdsaChannelSigner> {
 	/// 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<PaymentHash, PaymentPreimage>,
+	/// 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<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)>,
 
 	// 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
@@ -1146,7 +1158,7 @@ impl<Signer: EcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signer> {
  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[..])?;
  }
 
@@ -1224,6 +1236,7 @@ impl<Signer: EcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signer> {
  (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(())
@@ -1488,7 +1501,15 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 
 	/// 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<B: Deref, F: Deref, L: Deref>(
+	///
+	/// 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<B: Deref, F: Deref, L: Deref>(
  &self,
  payment_hash: &PaymentHash,
  payment_preimage: &PaymentPreimage,
@@ -1502,8 +1523,11 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 	{
  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
@@ -2194,7 +2218,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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
@@ -2415,7 +2439,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
  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;
@@ -2570,7 +2594,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
  res
 	}
 
-	pub(crate) fn get_stored_preimages(&self) -> HashMap<PaymentHash, PaymentPreimage> {
+	pub(crate) fn get_stored_preimages(&self) -> HashMap<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)> {
  self.inner.lock().unwrap().payment_preimages.clone()
 	}
 }
@@ -2929,14 +2953,27 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 
 	/// 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<B: Deref, F: Deref, L: Deref>(
- &mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage, broadcaster: &B,
+ &mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage,
+ payment_info: &Option<PaymentClaimDetails>, broadcaster: &B,
  fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &WithChannelMonitor<L>)
 	where B::Target: BroadcasterInterface,
  F::Target: FeeEstimator,
  L::Target: Logger,
 	{
- 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 {
@@ -3139,9 +3176,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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");
@@ -3593,7 +3630,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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(
@@ -3681,7 +3718,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  );
  (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
@@ -3835,7 +3872,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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;
  }
@@ -4808,7 +4845,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);
  }
  }
@@ -4891,6 +4928,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<HashMap<_, _>> = None;
  read_tlv_fields!(reader, {
  (1, funding_spend_confirmed, option),
  (3, htlcs_resolved_on_chain, optional_vec),
@@ -4904,7 +4942,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`.
@@ -5097,8 +5152,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!(
@@ -5228,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

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 \

fuzz/src/chanmon_consistency.rs

@@ -768,7 +768,7 @@ pub fn do_test<Out: Output>(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);
  }
 

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<PaymentClaimDetails>,
 	},
 	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),
@@ -919,8 +923,16 @@ pub(crate) struct ChannelMonitorImpl<Signer: EcdsaChannelSigner> {
 	/// 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<PaymentHash, PaymentPreimage>,
+	/// 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<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)>,
 
 	// 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
@@ -1146,7 +1158,7 @@ impl<Signer: EcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signer> {
  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[..])?;
  }
 
@@ -1224,6 +1236,7 @@ impl<Signer: EcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signer> {
  (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(())
@@ -1488,7 +1501,15 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 
 	/// 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<B: Deref, F: Deref, L: Deref>(
+	///
+	/// 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<B: Deref, F: Deref, L: Deref>(
  &self,
  payment_hash: &PaymentHash,
  payment_preimage: &PaymentPreimage,
@@ -1502,8 +1523,11 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 	{
  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
@@ -2194,7 +2218,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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
@@ -2415,7 +2439,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
  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;
@@ -2570,7 +2594,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
  res
 	}
 
-	pub(crate) fn get_stored_preimages(&self) -> HashMap<PaymentHash, PaymentPreimage> {
+	pub(crate) fn get_stored_preimages(&self) -> HashMap<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)> {
  self.inner.lock().unwrap().payment_preimages.clone()
 	}
 }
@@ -2929,14 +2953,27 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 
 	/// 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<B: Deref, F: Deref, L: Deref>(
- &mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage, broadcaster: &B,
+ &mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage,
+ payment_info: &Option<PaymentClaimDetails>, broadcaster: &B,
  fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &WithChannelMonitor<L>)
 	where B::Target: BroadcasterInterface,
  F::Target: FeeEstimator,
  L::Target: Logger,
 	{
- 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 {
@@ -3139,9 +3176,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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");
@@ -3593,7 +3630,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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(
@@ -3681,7 +3718,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  );
  (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
@@ -3835,7 +3872,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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;
  }
@@ -4808,7 +4845,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);
  }
  }
@@ -4891,6 +4928,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<HashMap<_, _>> = None;
  read_tlv_fields!(reader, {
  (1, funding_spend_confirmed, option),
  (3, htlcs_resolved_on_chain, optional_vec),
@@ -4904,7 +4942,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`.
@@ -5097,8 +5152,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!(
@@ -5228,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

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 \

fuzz/src/chanmon_consistency.rs

@@ -768,7 +768,7 @@ pub fn do_test<Out: Output>(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);
  }
 

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<PaymentClaimDetails>,
 	},
 	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),
@@ -919,8 +923,16 @@ pub(crate) struct ChannelMonitorImpl<Signer: EcdsaChannelSigner> {
 	/// 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<PaymentHash, PaymentPreimage>,
+	/// 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<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)>,
 
 	// 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
@@ -1146,7 +1158,7 @@ impl<Signer: EcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signer> {
  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[..])?;
  }
 
@@ -1224,6 +1236,7 @@ impl<Signer: EcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signer> {
  (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(())
@@ -1488,7 +1501,15 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 
 	/// 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<B: Deref, F: Deref, L: Deref>(
+	///
+	/// 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<B: Deref, F: Deref, L: Deref>(
  &self,
  payment_hash: &PaymentHash,
  payment_preimage: &PaymentPreimage,
@@ -1502,8 +1523,11 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 	{
  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
@@ -2194,7 +2218,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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
@@ -2415,7 +2439,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
  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;
@@ -2570,7 +2594,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
  res
 	}
 
-	pub(crate) fn get_stored_preimages(&self) -> HashMap<PaymentHash, PaymentPreimage> {
+	pub(crate) fn get_stored_preimages(&self) -> HashMap<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)> {
  self.inner.lock().unwrap().payment_preimages.clone()
 	}
 }
@@ -2929,14 +2953,27 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 
 	/// 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<B: Deref, F: Deref, L: Deref>(
- &mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage, broadcaster: &B,
+ &mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage,
+ payment_info: &Option<PaymentClaimDetails>, broadcaster: &B,
  fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &WithChannelMonitor<L>)
 	where B::Target: BroadcasterInterface,
  F::Target: FeeEstimator,
  L::Target: Logger,
 	{
- 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 {
@@ -3139,9 +3176,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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");
@@ -3593,7 +3630,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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(
@@ -3681,7 +3718,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  );
  (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
@@ -3835,7 +3872,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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;
  }
@@ -4808,7 +4845,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);
  }
  }
@@ -4891,6 +4928,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<HashMap<_, _>> = None;
  read_tlv_fields!(reader, {
  (1, funding_spend_confirmed, option),
  (3, htlcs_resolved_on_chain, optional_vec),
@@ -4904,7 +4942,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`.
@@ -5097,8 +5152,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!(
@@ -5228,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

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 \

fuzz/src/chanmon_consistency.rs

@@ -768,7 +768,7 @@ pub fn do_test<Out: Output>(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);
  }
 

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<PaymentClaimDetails>,
 	},
 	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),
@@ -919,8 +923,16 @@ pub(crate) struct ChannelMonitorImpl<Signer: EcdsaChannelSigner> {
 	/// 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<PaymentHash, PaymentPreimage>,
+	/// 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<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)>,
 
 	// 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
@@ -1146,7 +1158,7 @@ impl<Signer: EcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signer> {
  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[..])?;
  }
 
@@ -1224,6 +1236,7 @@ impl<Signer: EcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signer> {
  (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(())
@@ -1488,7 +1501,15 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 
 	/// 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<B: Deref, F: Deref, L: Deref>(
+	///
+	/// 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<B: Deref, F: Deref, L: Deref>(
  &self,
  payment_hash: &PaymentHash,
  payment_preimage: &PaymentPreimage,
@@ -1502,8 +1523,11 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 	{
  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
@@ -2194,7 +2218,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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
@@ -2415,7 +2439,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
  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;
@@ -2570,7 +2594,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
  res
 	}
 
-	pub(crate) fn get_stored_preimages(&self) -> HashMap<PaymentHash, PaymentPreimage> {
+	pub(crate) fn get_stored_preimages(&self) -> HashMap<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)> {
  self.inner.lock().unwrap().payment_preimages.clone()
 	}
 }
@@ -2929,14 +2953,27 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 
 	/// 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<B: Deref, F: Deref, L: Deref>(
- &mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage, broadcaster: &B,
+ &mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage,
+ payment_info: &Option<PaymentClaimDetails>, broadcaster: &B,
  fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &WithChannelMonitor<L>)
 	where B::Target: BroadcasterInterface,
  F::Target: FeeEstimator,
  L::Target: Logger,
 	{
- 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 {
@@ -3139,9 +3176,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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");
@@ -3593,7 +3630,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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(
@@ -3681,7 +3718,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  );
  (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
@@ -3835,7 +3872,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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;
  }
@@ -4808,7 +4845,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);
  }
  }
@@ -4891,6 +4928,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<HashMap<_, _>> = None;
  read_tlv_fields!(reader, {
  (1, funding_spend_confirmed, option),
  (3, htlcs_resolved_on_chain, optional_vec),
@@ -4904,7 +4942,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`.
@@ -5097,8 +5152,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!(
@@ -5228,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

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 \

fuzz/src/chanmon_consistency.rs

@@ -768,7 +768,7 @@ pub fn do_test<Out: Output>(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);
  }
 

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<PaymentClaimDetails>,
 	},
 	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),
@@ -919,8 +923,16 @@ pub(crate) struct ChannelMonitorImpl<Signer: EcdsaChannelSigner> {
 	/// 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<PaymentHash, PaymentPreimage>,
+	/// 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<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)>,
 
 	// 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
@@ -1146,7 +1158,7 @@ impl<Signer: EcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signer> {
  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[..])?;
  }
 
@@ -1224,6 +1236,7 @@ impl<Signer: EcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signer> {
  (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(())
@@ -1488,7 +1501,15 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 
 	/// 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<B: Deref, F: Deref, L: Deref>(
+	///
+	/// 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<B: Deref, F: Deref, L: Deref>(
  &self,
  payment_hash: &PaymentHash,
  payment_preimage: &PaymentPreimage,
@@ -1502,8 +1523,11 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 	{
  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
@@ -2194,7 +2218,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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
@@ -2415,7 +2439,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
  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;
@@ -2570,7 +2594,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
  res
 	}
 
-	pub(crate) fn get_stored_preimages(&self) -> HashMap<PaymentHash, PaymentPreimage> {
+	pub(crate) fn get_stored_preimages(&self) -> HashMap<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)> {
  self.inner.lock().unwrap().payment_preimages.clone()
 	}
 }
@@ -2929,14 +2953,27 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 
 	/// 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<B: Deref, F: Deref, L: Deref>(
- &mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage, broadcaster: &B,
+ &mut self, payment_hash: &PaymentHash, payment_preimage: &PaymentPreimage,
+ payment_info: &Option<PaymentClaimDetails>, broadcaster: &B,
  fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &WithChannelMonitor<L>)
 	where B::Target: BroadcasterInterface,
  F::Target: FeeEstimator,
  L::Target: Logger,
 	{
- 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 {
@@ -3139,9 +3176,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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");
@@ -3593,7 +3630,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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(
@@ -3681,7 +3718,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  );
  (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
@@ -3835,7 +3872,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
  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;
  }
@@ -4808,7 +4845,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);
  }
  }
@@ -4891,6 +4928,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<HashMap<_, _>> = None;
  read_tlv_fields!(reader, {
  (1, funding_spend_confirmed, option),
  (3, htlcs_resolved_on_chain, optional_vec),
@@ -4904,7 +4942,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`.
@@ -5097,8 +5152,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!(
@@ -5228,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