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