diff --git a/CHANGELOG.md b/CHANGELOG.md index cfdc5701a2..7e88987ac4 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/builder.rs b/src/builder.rs index 7f28996695..87ba3b5d77 100644 --- a/src/builder.rs +++ b/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) ); diff --git a/src/event.rs b/src/event.rs index 657c938ed6..db0e5335fd 100644 --- a/src/event.rs +++ b/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, diff --git a/src/payment/bolt11.rs b/src/payment/bolt11.rs index e2a34828cf..9af5704e9b 100644 --- a/src/payment/bolt11.rs +++ b/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(()) } diff --git a/src/payment/store.rs b/src/payment/store.rs index 12d360134a..8266be7e82 100644 --- a/src/payment/store.rs +++ b/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, /// The fees that were paid for this payment. @@ -101,6 +107,18 @@ impl Writeable for PaymentDetails { fn write( &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::>, 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 = secret.0.ok_or(DecodeError::InvalidValue)?; let latest_update_timestamp: u64 = latest_update_timestamp.0.ok_or(DecodeError::InvalidValue)?; - let amount_msat: Option = amount_msat.0.ok_or(DecodeError::InvalidValue)?; + let mut amount_msat: Option = 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 = ::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)] diff --git a/tests/common/mod.rs b/tests/common/mod.rs index a2a3ae4cb1..0d923f8f36 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -1569,10 +1569,7 @@ pub(crate) async fn do_channel_full_cycle( 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( 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); diff --git a/tests/integration_tests_rust.rs b/tests/integration_tests_rust.rs index 404b846415..bb08215e72 100644 --- a/tests/integration_tests_rust.rs +++ b/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 { @@ -5370,7 +5370,7 @@ 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 @@ -5378,13 +5378,10 @@ async fn do_lsps2_client_service_integration(client_trusts_lsp: bool) { 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); diff --git a/tests/upgrade_downgrade_tests.rs b/tests/upgrade_downgrade_tests.rs index fe31989c7a..5c3fcc2b9f 100644 --- a/tests/upgrade_downgrade_tests.rs +++ b/tests/upgrade_downgrade_tests.rs @@ -49,6 +49,15 @@ const FAIL_AMOUNT_MSAT: u64 = 200_000; #[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn v0_7_for_hash_payments_can_be_manually_resolved_after_upgrade() { + do_v0_7_manual_payment_upgrade(false).await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +async fn v0_7_queued_claimable_payments_can_be_resolved_after_upgrade() { + do_v0_7_manual_payment_upgrade(true).await; +} + +async fn do_v0_7_manual_payment_upgrade(pay_before_upgrade: bool) { let (bitcoind, electrsd) = setup_bitcoind_and_electrsd(); let esplora_url = format!("http://{}", electrsd.esplora_url.as_ref().unwrap()); let chain_source = TestChainSource::Esplora(&electrsd); @@ -122,6 +131,25 @@ async fn v0_7_for_hash_payments_can_be_manually_resolved_after_upgrade() { .unwrap() .to_string(); + // Overpayment must remain claimable even though the migrated record holds the invoice amount. + let paid_amount_msat = CLAIM_AMOUNT_MSAT + 1_000; + let claim_invoice = CurrentBolt11Invoice::from_str(&claim_invoice).unwrap(); + let payer_claim_id = if pay_before_upgrade { + let id = payer + .bolt11_payment() + .send_using_amount(&claim_invoice, paid_amount_msat, None) + .unwrap(); + let event = tokio::time::timeout(Duration::from_secs(60), old_receiver.next_event_async()) + .await + .unwrap(); + assert!(matches!(event, ldk_node_070::Event::PaymentClaimable { + payment_hash, claimable_amount_msat, .. + } if payment_hash.0 == claim_hash.0 && claimable_amount_msat == paid_amount_msat)); + // Leave the event queued so it must survive the upgrade. + Some(id) + } else { + None + }; old_receiver.stop().unwrap(); let receiver = build_current_receiver(receiver_storage_path, receiver_addresses, &esplora_url); @@ -132,17 +160,27 @@ async fn v0_7_for_hash_payments_can_be_manually_resolved_after_upgrade() { payer.connect(receiver.node_id(), old_receiver_addr, true).unwrap(); wait_for_current_channel_usable(&payer, receiver.node_id(), CLAIM_AMOUNT_MSAT).await; - let claim_invoice = CurrentBolt11Invoice::from_str(&claim_invoice).unwrap(); - let payer_claim_id = payer.bolt11_payment().send(&claim_invoice, None).unwrap(); - let (claim_payment_id, claimable_amount_msat) = - expect_payment_claimable_event!(receiver, claim_hash, CLAIM_AMOUNT_MSAT); + let payer_claim_id = payer_claim_id.unwrap_or_else(|| { + payer.bolt11_payment().send_using_amount(&claim_invoice, paid_amount_msat, None).unwrap() + }); + let (claim_payment_id, _) = + expect_payment_claimable_event!(receiver, claim_hash, paid_amount_msat); assert_eq!(claim_payment_id, PaymentId(claim_hash.0)); + assert_eq!( + receiver.payment(&claim_payment_id).unwrap().unwrap().amount_msat, + Some(if pay_before_upgrade { CLAIM_AMOUNT_MSAT } else { paid_amount_msat }), + "only a new claimable event replaces the legacy invoice amount with the received amount" + ); receiver .bolt11_payment() - .claim_for_id(claim_payment_id, claimable_amount_msat, PaymentPreimage(claim_preimage)) + .claim_for_id(claim_payment_id, PaymentPreimage(claim_preimage)) .unwrap(); - let received_payment_id = expect_payment_received_event!(receiver, CLAIM_AMOUNT_MSAT); + let received_payment_id = expect_payment_received_event!(receiver, paid_amount_msat); assert_eq!(received_payment_id, claim_payment_id); + assert_eq!( + receiver.payment(&claim_payment_id).unwrap().unwrap().amount_msat, + Some(paid_amount_msat) + ); expect_payment_successful_event!(payer, payer_claim_id, None); assert_eq!( receiver.payment(&claim_payment_id).unwrap().unwrap().status,