Two follow-ups to #948

CHANGELOG.md

@@ -1,6 +1,13 @@
 # Pending
 
 ## Compatibility Notes
+- Inbound BOLT11 payments now report amounts net of any withheld LSP fee as soon as they
+  become claimable, consistently with completed payments and BOLT12. Pending invoices from
+  v0.7 and earlier retain invoice-based amounts, minus any recorded fee, until another payment
+  event supplies the actual amount. Their historical serialization format is preserved.
+- `Bolt11Payment::claim_for_id` no longer takes a `claimable_amount_msat` argument. Applications
+  must check the amount reported in `PaymentClaimable` against their expectations before every
+  manual claim, including payments for invoices created before upgrading.
 - The language bindings now expose `Mnemonic` as an object instead of a string alias. Existing
   mnemonic phrases must be passed through its fallible constructor, which returns
   `NodeError::InvalidMnemonic` for invalid input; generated mnemonics can be converted back to a

src/builder.rs

@@ -2826,7 +2826,7 @@ mod tests {
 		);
 		assert_eq!(node.bolt11_payment().fail_for_id(payment_id), Err(Error::NotRunning));
 		assert_eq!(
-			node.bolt11_payment().claim_for_id(payment_id, 0, PaymentPreimage([0; 32])),
+			node.bolt11_payment().claim_for_id(payment_id, PaymentPreimage([0; 32])),
 			Err(Error::NotRunning)
 		);
 

src/event.rs

@@ -224,6 +224,9 @@ pub enum Event {
 	/// If the provided parameters don't match the expectations or the preimage can't be
 	/// retrieved in time, should be failed-back via [`fail_for_id`].
 	///
+	/// For every payment, the caller must verify that `claimable_amount_msat` meets their
+	/// expectations before claiming, including payments for invoices created before upgrading.
+	///
 	/// Note claiming will necessarily fail after the `claim_deadline` has been reached.
 	///
 	/// This event is only emitted for unknown BOLT 11 payments if
@@ -1085,24 +1088,25 @@ where
 						self.fail_claimable_payment(payment_id, &payment_hash).await?;
 						return Ok(());
 					}
+				}
 
-					if let Some(info) = payment_info.as_ref() {
-						match &info.kind {
-							PaymentKind::Bolt11 { .. } => {
-								let update = PaymentDetailsUpdate {
-									counterparty_skimmed_fee_msat: Some(Some(counterparty_skimmed_fee_msat)),
-									..PaymentDetailsUpdate::new(payment_id)
-								};
-								match self.payment_store.update(update).await {
-									Ok(_) => (),
-									Err(e) => {
-										log_error!(self.logger, "Failed to access payment store: {}", e);
-										return Err(ReplayEvent());
-									},
-								};
-							},
-							_ => debug_assert!(false, "We only expect the counterparty to get away with withholding fees for BOLT11 payments."),
-						}
+				if let Some(info) = payment_info.as_ref() {
+					match &info.kind {
+						PaymentKind::Bolt11 { .. } => {
+							let update = PaymentDetailsUpdate {
+								amount_msat: Some(Some(amount_msat)),
+								counterparty_skimmed_fee_msat: Some(
+									(counterparty_skimmed_fee_msat > 0)
+										.then_some(counterparty_skimmed_fee_msat),
+								),
+								..PaymentDetailsUpdate::new(payment_id)
+							};
+							if let Err(e) = self.payment_store.update(update).await {
+								log_error!(self.logger, "Failed to access payment store: {}", e);
+								return Err(ReplayEvent());
+							}
+						},
+						_ => debug_assert_eq!(counterparty_skimmed_fee_msat, 0, "We only expect the counterparty to get away with withholding fees for BOLT11 payments."),
 					}
 				}
 
@@ -1124,8 +1128,6 @@ where
 							return Ok(());
 						}
 
-						let invoice_amount_msat =
-							amount_msat.saturating_add(counterparty_skimmed_fee_msat);
 						let kind = PaymentKind::Bolt11 {
 							hash: payment_hash,
 							preimage: None,
@@ -1139,7 +1141,7 @@ where
 						let payment = PaymentDetails::new(
 							payment_id,
 							kind,
-							Some(invoice_amount_msat),
+							Some(amount_msat),
 							None,
 							PaymentDirection::Inbound,
 							PaymentStatus::Pending,
@@ -1212,8 +1214,6 @@ where
 						..
 					} => {
 						if should_insert_payment {
-							let invoice_amount_msat =
-								amount_msat.saturating_add(counterparty_skimmed_fee_msat);
 							let kind = PaymentKind::Bolt11 {
 								hash: payment_hash,
 								preimage: payment_preimage,
@@ -1229,7 +1229,7 @@ where
 							let payment = PaymentDetails::new(
 								payment_id,
 								kind,
-								Some(invoice_amount_msat),
+								Some(amount_msat),
 								None,
 								PaymentDirection::Inbound,
 								PaymentStatus::Pending,

src/payment/bolt11.rs

@@ -414,16 +414,19 @@ impl Bolt11Payment {
 	/// This should be called in response to a [`PaymentClaimable`] event as soon as the preimage is
 	/// available.
 	///
-	/// Will check that the payment is known, and that the given preimage and claimable amount
-	/// match our expectations before attempting to claim the payment, and will return an error
-	/// otherwise.
+	/// For every payment, the caller must verify that the event's `claimable_amount_msat` meets
+	/// their expectations before calling, including payments for invoices created before upgrading.
+	/// Calling this method approves claiming the payment reported by the event.
+	///
+	/// Will check that the payment is known and that the given preimage matches its payment hash
+	/// before attempting to claim the payment, and will return an error otherwise.
 	///
 	/// When claiming the payment has succeeded, a [`PaymentReceived`] event will be emitted.
 	///
 	/// [`PaymentClaimable`]: crate::Event::PaymentClaimable
 	/// [`PaymentReceived`]: crate::Event::PaymentReceived
 	pub fn claim_for_id(
-		&self, payment_id: PaymentId, claimable_amount_msat: u64, preimage: PaymentPreimage,
+		&self, payment_id: PaymentId, preimage: PaymentPreimage,
 	) -> Result<(), Error> {
 		if !*self.is_running.read().expect("lock") {
 			return Err(Error::NotRunning);
@@ -461,25 +464,6 @@ impl Bolt11Payment {
 			return Err(Error::InvalidPaymentPreimage);
 		}
 
-		// For payments requested via `receive*_via_jit_channel_for_hash()`
-		// `skimmed_fee_msat` held by LSP must be taken into account.
-		let skimmed_fee_msat = match details.kind {
-			PaymentKind::Bolt11 {
-				counterparty_skimmed_fee_msat: Some(skimmed_fee_msat), ..
-			} => skimmed_fee_msat,
-			_ => 0,
-		};
-		if let Some(invoice_amount_msat) = details.amount_msat {
-			if claimable_amount_msat < invoice_amount_msat.saturating_sub(skimmed_fee_msat) {
-				log_error!(
-					self.logger,
-					"Failed to manually claim payment {} as the claimable amount is less than expected",
-					payment_id
-				);
-				return Err(Error::InvalidAmount);
-			}
-		}
-
 		self.channel_manager.claim_funds(preimage);
 		Ok(())
 	}

src/payment/store.rs

@@ -67,6 +67,12 @@ pub struct PaymentDetails {
 	pub kind: PaymentKind,
 	/// The amount transferred.
 	///
+	/// For inbound Lightning payments, this excludes any fee withheld by our channel
+	/// counterparty once the payment is observed.
+	///
+	/// Pending invoices created with v0.7 or earlier can retain the requested amount, minus
+	/// any recorded counterparty fee, until another payment event updates them.
+	///
 	/// Will be `None` for variable-amount payments until we receive them.
 	pub amount_msat: Option<u64>,
 	/// The fees that were paid for this payment.
@@ -101,6 +107,18 @@ impl Writeable for PaymentDetails {
 	fn write<W: lightning::util::ser::Writer>(
 		&self, writer: &mut W,
 	) -> Result<(), lightning::io::Error> {
+		// Pending BOLT11 amounts have historically been gross on disk. Keep that encoding
+		// so reads can always subtract the fee exactly once, including after a restart.
+		let mut amount_msat = self.amount_msat;
+		if self.direction == PaymentDirection::Inbound && self.status == PaymentStatus::Pending {
+			if let (
+				Some(amount),
+				PaymentKind::Bolt11 { counterparty_skimmed_fee_msat: Some(fee), .. },
+			) = (amount_msat, &self.kind)
+			{
+				amount_msat = Some(amount.saturating_add(*fee));
+			}
+		}
 		write_tlv_fields!(writer, {
 			(0, self.id, required), // Used to be `hash` for v0.2.1 and prior
 			// 2 used to be `preimage` before it was moved to `kind` in v0.3.0
@@ -109,7 +127,7 @@ impl Writeable for PaymentDetails {
 			// 4 used to be `secret` before it was moved to `kind` in v0.3.0
 			(4, None::<Option<PaymentSecret>>, required),
 			(5, self.latest_update_timestamp, required),
-			(6, self.amount_msat, required),
+			(6, amount_msat, required),
 			(7, self.fee_paid_msat, option),
 			(8, self.direction, required),
 			(10, self.status, required)
@@ -141,7 +159,7 @@ impl Readable for PaymentDetails {
 		let secret: Option<PaymentSecret> = secret.0.ok_or(DecodeError::InvalidValue)?;
 		let latest_update_timestamp: u64 =
 			latest_update_timestamp.0.ok_or(DecodeError::InvalidValue)?;
-		let amount_msat: Option<u64> = amount_msat.0.ok_or(DecodeError::InvalidValue)?;
+		let mut amount_msat: Option<u64> = amount_msat.0.ok_or(DecodeError::InvalidValue)?;
 		let direction: PaymentDirection = direction.0.ok_or(DecodeError::InvalidValue)?;
 		let status: PaymentStatus = status.0.ok_or(DecodeError::InvalidValue)?;
 
@@ -168,6 +186,18 @@ impl Readable for PaymentDetails {
 			}
 		};
 
+		// Only pending inbound BOLT11 records use the historical gross encoding.
+		// Completed amounts already exclude the fee and must not be adjusted again.
+		if direction == PaymentDirection::Inbound && status == PaymentStatus::Pending {
+			if let (
+				Some(amount),
+				PaymentKind::Bolt11 { counterparty_skimmed_fee_msat: Some(fee), .. },
+			) = (amount_msat, &kind)
+			{
+				amount_msat = Some(amount.saturating_sub(*fee));
+			}
+		}
+
 		Ok(PaymentDetails {
 			id,
 			kind,
@@ -784,6 +814,87 @@ mod tests {
 
 	use super::*;
 
+	#[test]
+	fn v0_7_pending_bolt11_amounts_roundtrip_as_net() {
+		use ldk_node_070::lightning::util::ser::Writeable as OldWriteable;
+		use ldk_node_070::payment::{
+			LSPFeeLimits as OldFeeLimits, PaymentDetails as OldDetails,
+			PaymentDirection as OldDirection, PaymentKind as OldKind, PaymentStatus as OldStatus,
+		};
+
+		for (status, amount, fee, expected, persisted) in [
+			(OldStatus::Pending, Some(100_000), Some(1_000), Some(99_000), Some(100_000)),
+			(OldStatus::Pending, Some(100_000), Some(100_000), Some(0), Some(100_000)),
+			(OldStatus::Pending, Some(100_000), Some(150_000), Some(0), Some(150_000)),
+			(OldStatus::Pending, Some(100_000), None, Some(100_000), Some(100_000)),
+			(OldStatus::Pending, None, Some(1_000), None, None),
+			(OldStatus::Succeeded, Some(99_000), Some(1_000), Some(99_000), Some(99_000)),
+			(OldStatus::Failed, Some(100_000), Some(1_000), Some(100_000), Some(100_000)),
+		] {
+			let old = OldDetails {
+				id: ldk_node_070::lightning::ln::channelmanager::PaymentId([42; 32]),
+				kind: OldKind::Bolt11Jit {
+					hash: ldk_node_070::lightning_types::payment::PaymentHash([42; 32]),
+					preimage: None,
+					secret: None,
+					counterparty_skimmed_fee_msat: fee,
+					lsp_fee_limits: OldFeeLimits {
+						max_total_opening_fee_msat: Some(fee.unwrap_or(1_000)),
+						max_proportional_opening_fee_ppm_msat: None,
+					},
+				},
+				amount_msat: amount,
+				fee_paid_msat: None,
+				direction: OldDirection::Inbound,
+				status,
+				latest_update_timestamp: 1,
+			};
+			let mut payment = PaymentDetails::read(&mut OldWriteable::encode(&old).as_slice())
+				.expect("legacy payments with excessive fees must remain readable");
+			assert_eq!(
+				payment.amount_msat, expected,
+				"v0.7 pending JIT amounts must exclude the fee"
+			);
+			for _ in 0..3 {
+				let encoded = payment.encode();
+				let raw = <OldDetails as ldk_node_070::lightning::util::ser::Readable>::read(
+					&mut encoded.as_slice(),
+				)
+				.unwrap();
+				assert_eq!(
+					raw.amount_msat, persisted,
+					"the persisted amount must retain its historical meaning"
+				);
+				let decoded = PaymentDetails::read(&mut encoded.as_slice()).unwrap();
+				assert_eq!(decoded, payment, "restarting must not deduct the fee again");
+				payment = decoded;
+			}
+		}
+	}
+
+	#[test]
+	fn pending_bolt11_amount_overflow_remains_writable() {
+		let mut payment = PaymentDetails::new(
+			PaymentId([42; 32]),
+			PaymentKind::Bolt11 {
+				hash: PaymentHash([42; 32]),
+				preimage: None,
+				secret: None,
+				counterparty_skimmed_fee_msat: Some(u64::MAX - 50),
+			},
+			Some(100),
+			None,
+			PaymentDirection::Inbound,
+			PaymentStatus::Pending,
+		);
+		let mut encoded = Vec::new();
+		assert!(payment.write(&mut encoded).is_ok(), "excessive fees must not prevent persistence");
+		let decoded = PaymentDetails::read(&mut encoded.as_slice()).unwrap();
+		payment.amount_msat = Some(50);
+		assert_eq!(decoded, payment);
+		assert_eq!(PaymentDetails::read(&mut decoded.encode().as_slice()).unwrap(), decoded);
+	}
+
 	/// We refactored `PaymentDetails` to hold a payment id and moved some required fields into
 	/// `PaymentKind`. Here, we keep the old layout available in order test de/ser compatibility.
 	#[derive(Clone, Debug, PartialEq, Eq)]

tests/common/mod.rs

@@ -1569,10 +1569,7 @@ pub(crate) async fn do_channel_full_cycle<E: ElectrumApi>(
 	let (manual_payment_id, claimable_amount_msat) =
 		expect_payment_claimable_event!(node_b, manual_payment_hash, invoice_amount_3_msat);
 	assert_ne!(manual_payment_id.0, manual_payment_hash.0);
-	node_b
-		.bolt11_payment()
-		.claim_for_id(manual_payment_id, claimable_amount_msat, manual_preimage)
-		.unwrap();
+	node_b.bolt11_payment().claim_for_id(manual_payment_id, manual_preimage).unwrap();
 	let received_payment_id = expect_payment_received_event!(node_b, claimable_amount_msat);
 	assert_eq!(received_payment_id, manual_payment_id);
 	expect_payment_successful_event!(node_a, outbound_manual_payment_id, None);
@@ -1612,10 +1609,7 @@ pub(crate) async fn do_channel_full_cycle<E: ElectrumApi>(
 
 	let (manual_cltv_payment_id, claimable_amount_msat) =
 		expect_payment_claimable_event!(node_b, manual_cltv_payment_hash, manual_cltv_amount_msat);
-	node_b
-		.bolt11_payment()
-		.claim_for_id(manual_cltv_payment_id, claimable_amount_msat, manual_cltv_preimage)
-		.unwrap();
+	node_b.bolt11_payment().claim_for_id(manual_cltv_payment_id, manual_cltv_preimage).unwrap();
 	let received_payment_id = expect_payment_received_event!(node_b, claimable_amount_msat);
 	assert_eq!(received_payment_id, manual_cltv_payment_id);
 	expect_payment_successful_event!(node_a, outbound_manual_cltv_payment_id, None);

tests/integration_tests_rust.rs

@@ -1009,7 +1009,7 @@ async fn a_claim_is_classified_after_its_record_graduated() {
 	expect_event!(node_a, ChannelClosed);
 	expect_event!(node_b, ChannelClosed);
 
-	node_b.bolt11_payment().claim_for_id(payment_id, claimable_amount_msat, preimage).unwrap();
+	node_b.bolt11_payment().claim_for_id(payment_id, preimage).unwrap();
 	expect_payment_received_event!(node_b, claimable_amount_msat);
 
 	let onchain_payments = |node: &TestNode| -> Vec<PaymentDetails> {
@@ -5370,21 +5370,18 @@ async fn do_lsps2_client_service_integration(client_trusts_lsp: bool) {
 
 	let service_fee_msat = (jit_amount_msat * channel_opening_fee_ppm as u64) / 1_000_000;
 	let expected_received_amount_msat = jit_amount_msat - service_fee_msat;
-	let (client_payment_id, claimable_amount_msat) = expect_payment_claimable_event!(
+	let (client_payment_id, _) = expect_payment_claimable_event!(
 		client_node,
 		manual_payment_hash,
 		expected_received_amount_msat
 	);
 	assert_ne!(client_payment_id.0, manual_payment_hash.0);
 	assert_eq!(
 		client_node.payment(&client_payment_id).unwrap().unwrap().amount_msat,
-		Some(jit_amount_msat)
+		Some(expected_received_amount_msat)
 	);
 	println!("Claiming payment!");
-	client_node
-		.bolt11_payment()
-		.claim_for_id(client_payment_id, claimable_amount_msat, manual_preimage)
-		.unwrap();
+	client_node.bolt11_payment().claim_for_id(client_payment_id, manual_preimage).unwrap();
 
 	expect_event!(service_node, PaymentForwarded);
 	expect_payment_successful_event!(payer_node, payer_payment_id, None);
@@ -5599,7 +5596,7 @@ async fn live_node_apis_return_not_running_after_stop() {
 	// Payments.
 	assert_eq!(node_a.bolt11_payment().send(&invoice, None), Err(NodeError::NotRunning));
 	assert_eq!(
-		node_a.bolt11_payment().claim_for_id(payment_id, 0, preimage),
+		node_a.bolt11_payment().claim_for_id(payment_id, preimage),
 		Err(NodeError::NotRunning)
 	);
 	assert_eq!(node_a.bolt11_payment().fail_for_id(payment_id), Err(NodeError::NotRunning));
@@ -5781,10 +5778,7 @@ async fn lsps2_client_trusts_lsp() {
 		expected_received_amount_msat
 	);
 
-	client_node
-		.bolt11_payment()
-		.claim_for_id(client_payment_id, jit_amount_msat, manual_preimage)
-		.unwrap();
+	client_node.bolt11_payment().claim_for_id(client_payment_id, manual_preimage).unwrap();
 
 	expect_payment_successful_event!(payer_node, payer_payment_id, None);