Store info about claimed payments, incl HTLCs in ChannelMonitors

When we claim an MPP payment, then crash before persisting all the
relevant `ChannelMonitor`s, we rely on the payment data being
available in the `ChannelManager` on restart to re-claim any parts
that haven't yet been claimed. This is fine as long as the
`ChannelManager` was persisted before the `PaymentClaimable` event
was processed, which is generally the case in our
`lightning-background-processor`, but may not be in other cases or
in a somewhat rare race.

In order to fix this, we need to track where all the MPP parts of
a payment are in the `ChannelMonitor`, allowing us to re-claim any
missing pieces without reference to any `ChannelManager` data.

Further, in order to properly generate a `PaymentClaimed` event
against the re-started claim, we have to store various payment
metadata with the HTLC list as well.

Here we store the required MPP parts and metadata in
`ChannelMonitor`s and make them available to `ChannelManager` on
load.

lightning/src/chain/channelmonitor.rs

@@ -923,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.
+	/// revoked. Note that this happens one revocation after it theoretically could, leaving
-	payment_preimages: HashMap<PaymentHash, PaymentPreimage>,
+	/// 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
@@ -1150,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[..])?;
 		}
 
@@ -1228,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(())
@@ -2201,7 +2210,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
@@ -2422,7 +2431,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;
@@ -2577,7 +2586,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()
 	}
 }
@@ -2936,6 +2945,8 @@ 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,
 		payment_info: &Option<PaymentClaimDetails>, broadcaster: &B,
@@ -2944,8 +2955,17 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		    F::Target: FeeEstimator,
 		    L::Target: Logger,
 	{
-		// TODO: Store payment_info (but do not override any existing values)
+		self.payment_preimages.entry(payment_hash.clone())
-		self.payment_preimages.insert(payment_hash.clone(), payment_preimage.clone());
+			.and_modify(|(_, payment_infos)| {
+				if let Some(payment_info) = payment_info {
+					if !payment_infos.contains(&payment_info) {
+						payment_infos.push(payment_info.clone());
+					}
+				}
+			})
+			.or_insert_with(|| {
+				(payment_preimage.clone(), payment_info.clone().into_iter().collect())
+			});
 
 		let confirmed_spend_txid = self.funding_spend_confirmed.or_else(|| {
 			self.onchain_events_awaiting_threshold_conf.iter().find_map(|event| match event.event {
@@ -3602,7 +3622,7 @@ impl<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(
@@ -3690,7 +3710,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
@@ -3844,7 +3864,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;
 					}
@@ -4817,7 +4837,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
 		for _ in 0..payment_preimages_len {
 			let preimage: PaymentPreimage = Readable::read(reader)?;
 			let hash = PaymentHash(Sha256::hash(&preimage.0[..]).to_byte_array());
-			if let Some(_) = payment_preimages.insert(hash, preimage) {
+			if let Some(_) = payment_preimages.insert(hash, (preimage, Vec::new())) {
 				return Err(DecodeError::InvalidValue);
 			}
 		}
@@ -4900,6 +4920,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
 		let mut balances_empty_height = None;
 		let mut channel_id = None;
 		let mut holder_pays_commitment_tx_fee = None;
+		let mut payment_preimages_with_info: Option<HashMap<_, _>> = None;
 		read_tlv_fields!(reader, {
 			(1, funding_spend_confirmed, option),
 			(3, htlcs_resolved_on_chain, optional_vec),
@@ -4913,7 +4934,24 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
 			(19, channel_id, option),
 			(21, balances_empty_height, option),
 			(23, holder_pays_commitment_tx_fee, option),
+			(25, payment_preimages_with_info, option),
 		});
+		if let Some(payment_preimages_with_info) = payment_preimages_with_info {
+			if payment_preimages_with_info.len() != payment_preimages.len() {
+				return Err(DecodeError::InvalidValue);
+			}
+			for (payment_hash, (payment_preimage, _)) in payment_preimages.iter() {
+				// Note that because `payment_preimages` is built back from preimages directly,
+				// checking that the two maps have the same hash -> preimage pairs also checks that
+				// the payment hashes in `payment_preimages_with_info`'s preimages match its
+				// hashes.
+				let new_preimage = payment_preimages_with_info.get(payment_hash).map(|(p, _)| p);
+				if new_preimage != Some(payment_preimage) {
+					return Err(DecodeError::InvalidValue);
+				}
+			}
+			payment_preimages = payment_preimages_with_info;
+		}
 
 		// `HolderForceClosedWithInfo` replaced `HolderForceClosed` in v0.0.122. If we have both
 		// events, we can remove the `HolderForceClosed` event and just keep the `HolderForceClosedWithInfo`.

lightning/src/ln/channelmanager.rs

@@ -12987,7 +12987,7 @@ where
 		let bounded_fee_estimator = LowerBoundedFeeEstimator::new(args.fee_estimator);
 
 		for (_, monitor) in args.channel_monitors.iter() {
-			for (payment_hash, payment_preimage) in monitor.get_stored_preimages() {
+			for (payment_hash, (payment_preimage, _)) in monitor.get_stored_preimages() {
 				if let Some(payment) = claimable_payments.remove(&payment_hash) {
 					log_info!(args.logger, "Re-claiming HTLCs with payment hash {} as we've released the preimage to a ChannelMonitor!", &payment_hash);
 					let mut claimable_amt_msat = 0;

lightning/src/util/ser.rs

@@ -1001,6 +1001,7 @@ impl Readable for Vec<u8> {
 impl_for_vec!(ecdsa::Signature);
 impl_for_vec!(crate::chain::channelmonitor::ChannelMonitorUpdate);
 impl_for_vec!(crate::ln::channelmanager::MonitorUpdateCompletionAction);
+impl_for_vec!(crate::ln::channelmanager::PaymentClaimDetails);
 impl_for_vec!(crate::ln::msgs::SocketAddress);
 impl_for_vec!((A, B), A, B);
 impl_writeable_for_vec!(&crate::routing::router::BlindedTail);