Drop debug_assert that would have us panic for replayed PaymentClaimeds

In cd8958f we changed the internal API behavior of
`PaymentStore::update`. While previously it would return `Ok(true)` the
to-be-updated entry was found in the store, it now returns `Ok(true)` if
not only the entry was found but it was actually updated and
re-persisted. This was an improvement as it allows us to avoid
unnecessary re-persists if nothing changed.

However, there were 1-2 places that implicitly relied on that behavior
for logging purposes which we didn't correctly update to the new
behavior. Unfortunately, one instance in handling `PaymentClaimed`
events actually even `debug_assert`ed on the return value, which lead to
some unnecessary panics in `debug` in case `PaymentClaimed` got
replayed. Here we rectify this by dropping the `debug_assert`.

Author: tnull

src/event.rs

@@ -20,7 +20,7 @@ use crate::logger::Logger;
 
 use crate::payment::store::{
 	PaymentDetails, PaymentDetailsUpdate, PaymentDirection, PaymentKind, PaymentStatus,
-	PaymentStore,
+	PaymentStore, PaymentStoreUpdateResult,
 };
 
 use crate::io::{
@@ -906,14 +906,17 @@ where
 				};
 
 				match self.payment_store.update(&update) {
-					Ok(true) => (),
-					Ok(false) => {
+					Ok(PaymentStoreUpdateResult::Updated)
+					| Ok(PaymentStoreUpdateResult::Unchanged) => (
+						// No need to do anything if the idempotent update was applied, which might
+						// be the result of a replayed event.
+					),
+					Ok(PaymentStoreUpdateResult::NotFound) => {
 						log_error!(
 							self.logger,
-							"Payment with ID {} couldn't be found in store",
+							"Claimed payment with ID {} couldn't be found in store",
 							payment_id,
 						);
-						debug_assert!(false);
 					},
 					Err(e) => {
 						log_error!(

src/payment/bolt11.rs

@@ -16,7 +16,7 @@ use crate::liquidity::LiquiditySource;
 use crate::logger::{log_error, log_info, LdkLogger, Logger};
 use crate::payment::store::{
 	LSPFeeLimits, PaymentDetails, PaymentDetailsUpdate, PaymentDirection, PaymentKind,
-	PaymentStatus, PaymentStore,
+	PaymentStatus, PaymentStore, PaymentStoreUpdateResult,
 };
 use crate::payment::SendingParameters;
 use crate::peer_store::{PeerInfo, PeerStore};
@@ -408,13 +408,25 @@ impl Bolt11Payment {
 			..PaymentDetailsUpdate::new(payment_id)
 		};
 
-		if !self.payment_store.update(&update)? {
-			log_error!(
-				self.logger,
-				"Failed to manually fail unknown payment with hash: {}",
-				payment_hash
-			);
-			return Err(Error::InvalidPaymentHash);
+		match self.payment_store.update(&update) {
+			Ok(PaymentStoreUpdateResult::Updated) | Ok(PaymentStoreUpdateResult::Unchanged) => (),
+			Ok(PaymentStoreUpdateResult::NotFound) => {
+				log_error!(
+					self.logger,
+					"Failed to manually fail unknown payment with hash {}",
+					payment_hash,
+				);
+				return Err(Error::InvalidPaymentHash);
+			},
+			Err(e) => {
+				log_error!(
+					self.logger,
+					"Failed to manually fail payment with hash {}: {}",
+					payment_hash,
+					e
+				);
+				return Err(e);
+			},
 		}
 
 		self.channel_manager.fail_htlc_backwards(&payment_hash);

src/payment/store.rs

@@ -590,6 +590,13 @@ impl From<&PaymentDetails> for PaymentDetailsUpdate {
 	}
 }
 
+#[derive(PartialEq, Eq, Debug, Clone, Copy)]
+pub(crate) enum PaymentStoreUpdateResult {
+	Updated,
+	Unchanged,
+	NotFound,
+}
+
 pub(crate) struct PaymentStore<L: Deref>
 where
 	L::Target: LdkLogger,
@@ -670,17 +677,22 @@ where
 		self.payments.lock().unwrap().get(id).cloned()
 	}
 
-	pub(crate) fn update(&self, update: &PaymentDetailsUpdate) -> Result<bool, Error> {
-		let mut updated = false;
+	pub(crate) fn update(
+		&self, update: &PaymentDetailsUpdate,
+	) -> Result<PaymentStoreUpdateResult, Error> {
 		let mut locked_payments = self.payments.lock().unwrap();
 
 		if let Some(payment) = locked_payments.get_mut(&update.id) {
-			updated = payment.update(update);
+			let updated = payment.update(update);
 			if updated {
 				self.persist_info(&update.id, payment)?;
+				Ok(PaymentStoreUpdateResult::Updated)
+			} else {
+				Ok(PaymentStoreUpdateResult::Unchanged)
 			}
+		} else {
+			Ok(PaymentStoreUpdateResult::NotFound)
 		}
-		Ok(updated)
 	}
 
 	pub(crate) fn list_filter<F: FnMut(&&PaymentDetails) -> bool>(
@@ -789,9 +801,22 @@ mod tests {
 		assert_eq!(Ok(true), payment_store.insert(payment));
 		assert!(payment_store.get(&id).is_some());
 
+		// Check update returns `Updated`
 		let mut update = PaymentDetailsUpdate::new(id);
 		update.status = Some(PaymentStatus::Succeeded);
-		assert_eq!(Ok(true), payment_store.update(&update));
+		assert_eq!(Ok(PaymentStoreUpdateResult::Updated), payment_store.update(&update));
+
+		// Check no-op update yields `Unchanged`
+		let mut update = PaymentDetailsUpdate::new(id);
+		update.status = Some(PaymentStatus::Succeeded);
+		assert_eq!(Ok(PaymentStoreUpdateResult::Unchanged), payment_store.update(&update));
+
+		// Check bogus update yields `NotFound`
+		let bogus_id = PaymentId([84u8; 32]);
+		let mut update = PaymentDetailsUpdate::new(bogus_id);
+		update.status = Some(PaymentStatus::Succeeded);
+		assert_eq!(Ok(PaymentStoreUpdateResult::NotFound), payment_store.update(&update));
+
 		assert!(payment_store.get(&id).is_some());
 
 		assert_eq!(PaymentStatus::Succeeded, payment_store.get(&id).unwrap().status);