Make payment_id a required field in Events

The switch to tracking payments by ID happened with LDK Node v0.3.0,
which is >1.5 years old by now. We can be pretty certain that nobody is
upgrading from an older version to the upcoming v0.8.

Here we hence make the `payment_id` fields in `Event` required which is
a nice API simplification that will also be utilized in the next commit.

Co-Authored-By: HAL 9000

Author: tnull

CHANGELOG.md

@@ -5,6 +5,11 @@
   prior LSPS2 fee-limit state stored in `PaymentKind::Bolt11Jit` is not migrated.
 - Users of the VSS storage backend must upgrade their VSS server to at least version
   `v0.1.0-alpha.0` before upgrading LDK Node.
+- The `payment_id` field on the `PaymentSuccessful`, `PaymentFailed`, and
+  `PaymentReceived` events is now a required (non-optional) `PaymentId`. Events
+  persisted by LDK Node v0.2.1 or earlier (which stored `payment_id` as
+  optional) will fail to deserialize on read; users upgrading from those
+  versions need to drain pending events before the upgrade.
 
 ## Feature and API updates
 - The Bitcoin Core RPC and REST chain-source builder methods now accept an optional

benches/payments.rs

@@ -75,12 +75,8 @@ async fn send_payments(node_a: Arc<Node>, node_b: Arc<Node>) -> std::time::Durat
 	while success_count < total_payments {
 		match node_a.next_event_async().await {
 			Event::PaymentSuccessful { payment_id, payment_hash, .. } => {
-				if let Some(id) = payment_id {
-					success_count += 1;
-					println!("{}: Payment with id {:?} completed", payment_hash.0.as_hex(), id);
-				} else {
-					println!("Payment completed (no payment_id)");
-				}
+				success_count += 1;
+				println!("{}: Payment with id {:?} completed", payment_hash.0.as_hex(), payment_id);
 			},
 			Event::PaymentFailed { payment_id, payment_hash, .. } => {
 				println!("{}: Payment {:?} failed", payment_hash.unwrap().0.as_hex(), payment_id);

src/event.rs

@@ -103,9 +103,7 @@ pub enum Event {
 	/// A sent payment was successful.
 	PaymentSuccessful {
 		/// A local identifier used to track the payment.
-		///
-		/// Will only be `None` for events serialized with LDK Node v0.2.1 or prior.
-		payment_id: Option<PaymentId>,
+		payment_id: PaymentId,
 		/// The hash of the payment.
 		payment_hash: PaymentHash,
 		/// The preimage to the `payment_hash`.
@@ -131,9 +129,7 @@ pub enum Event {
 	/// A sent payment has failed.
 	PaymentFailed {
 		/// A local identifier used to track the payment.
-		///
-		/// Will only be `None` for events serialized with LDK Node v0.2.1 or prior.
-		payment_id: Option<PaymentId>,
+		payment_id: PaymentId,
 		/// The hash of the payment.
 		///
 		/// This will be `None` if the payment failed before receiving an invoice when paying a
@@ -149,9 +145,7 @@ pub enum Event {
 	/// A payment has been received.
 	PaymentReceived {
 		/// A local identifier used to track the payment.
-		///
-		/// Will only be `None` for events serialized with LDK Node v0.2.1 or prior.
-		payment_id: Option<PaymentId>,
+		payment_id: PaymentId,
 		/// The hash of the payment.
 		payment_hash: PaymentHash,
 		/// The value, in thousandths of a satoshi, that has been received.
@@ -298,18 +292,18 @@ impl_writeable_tlv_based_enum!(Event,
 	(0, PaymentSuccessful) => {
 		(0, payment_hash, required),
 		(1, fee_paid_msat, option),
-		(3, payment_id, option),
+		(3, payment_id, required),
 		(5, payment_preimage, option),
 		(7, bolt12_invoice, option),
 	},
 	(1, PaymentFailed) => {
 		(0, payment_hash, option),
 		(1, reason, upgradable_option),
-		(3, payment_id, option),
+		(3, payment_id, required),
 	},
 	(2, PaymentReceived) => {
 		(0, payment_hash, required),
-		(1, payment_id, option),
+		(1, payment_id, required),
 		(2, amount_msat, required),
 		(3, custom_records, optional_vec),
 	},
@@ -1095,7 +1089,7 @@ where
 				}
 
 				let event = Event::PaymentReceived {
-					payment_id: Some(payment_id),
+					payment_id,
 					payment_hash,
 					amount_msat,
 					custom_records: onion_fields
@@ -1160,7 +1154,7 @@ where
 					);
 				});
 				let event = Event::PaymentSuccessful {
-					payment_id: Some(payment_id),
+					payment_id,
 					payment_hash,
 					payment_preimage: Some(payment_preimage),
 					fee_paid_msat,
@@ -1196,8 +1190,7 @@ where
 					},
 				};
 
-				let event =
-					Event::PaymentFailed { payment_id: Some(payment_id), payment_hash, reason };
+				let event = Event::PaymentFailed { payment_id, payment_hash, reason };
 				match self.event_queue.add_event(event).await {
 					Ok(_) => return Ok(()),
 					Err(e) => {

tests/common/mod.rs

@@ -222,8 +222,8 @@ macro_rules! expect_payment_received_event {
 			ref e @ Event::PaymentReceived { payment_id, amount_msat, .. } => {
 				println!("{} got event {:?}", $node.node_id(), e);
 				assert_eq!(amount_msat, $amount_msat);
-				let payment = $node.payment(&payment_id.unwrap()).unwrap();
-				if !matches!(payment.kind, ldk_node::payment::PaymentKind::Onchain { .. }) {
+				let payment = $node.payment(&payment_id).unwrap();
+				if !matches!(payment.kind, PaymentKind::Onchain { .. }) {
 					assert_eq!(payment.fee_paid_msat, None);
 				}
 				$node.event_handled().unwrap();
@@ -290,7 +290,7 @@ macro_rules! expect_payment_successful_event {
 				if let Some(fee_msat) = $fee_paid_msat {
 					assert_eq!(fee_paid_msat, fee_msat);
 				}
-				let payment = $node.payment(&$payment_id.unwrap()).unwrap();
+				let payment = $node.payment(&$payment_id).unwrap();
 				assert_eq!(payment.fee_paid_msat, fee_paid_msat);
 				assert_eq!(payment_id, $payment_id);
 				$node.event_handled().unwrap();
@@ -1248,7 +1248,7 @@ pub(crate) async fn do_channel_full_cycle<E: ElectrumApi>(
 		.claim_for_hash(manual_payment_hash, claimable_amount_msat, manual_preimage)
 		.unwrap();
 	expect_payment_received_event!(node_b, claimable_amount_msat);
-	expect_payment_successful_event!(node_a, Some(manual_payment_id), None);
+	expect_payment_successful_event!(node_a, manual_payment_id, None);
 	assert_eq!(node_a.payment(&manual_payment_id).unwrap().status, PaymentStatus::Succeeded);
 	assert_eq!(node_a.payment(&manual_payment_id).unwrap().direction, PaymentDirection::Outbound);
 	assert_eq!(

tests/integration_tests_hrn.rs

@@ -79,5 +79,5 @@ async fn unified_send_to_hrn() {
 			},
 		};
 
-	expect_payment_successful_event!(node_a, Some(offer_payment_id), None);
+	expect_payment_successful_event!(node_a, offer_payment_id, None);
 }

tests/integration_tests_rust.rs

@@ -410,13 +410,12 @@ async fn split_underpaid_bolt11_payment() {
 		.unwrap();
 
 	let receiver_payment_id = expect_payment_received_event!(node_c, amount_msat);
-	assert_eq!(receiver_payment_id, Some(PaymentId(invoice.payment_hash().0)));
-	expect_payment_successful_event!(node_a, Some(payment_id_a), None);
-	expect_payment_successful_event!(node_b, Some(payment_id_b), None);
+	assert_eq!(receiver_payment_id, PaymentId(invoice.payment_hash().0));
+	expect_payment_successful_event!(node_a, payment_id_a, None);
+	expect_payment_successful_event!(node_b, payment_id_b, None);
 
 	// The receiver records the full invoice amount; each payer records only its own half.
-	let receiver_payments =
-		node_c.list_payments_with_filter(|p| p.id == receiver_payment_id.unwrap());
+	let receiver_payments = node_c.list_payments_with_filter(|p| p.id == receiver_payment_id);
 	assert_eq!(receiver_payments.len(), 1);
 	assert_eq!(receiver_payments.first().unwrap().amount_msat, Some(amount_msat));
 
@@ -1594,7 +1593,7 @@ async fn splice_channel() {
 	let payment_id =
 		node_b.spontaneous_payment().send(amount_msat, node_a.node_id(), None).unwrap();
 
-	expect_payment_successful_event!(node_b, Some(payment_id), None);
+	expect_payment_successful_event!(node_b, payment_id, None);
 	expect_payment_received_event!(node_a, amount_msat);
 
 	// Mine a block to give time for the HTLC to resolve
@@ -2116,7 +2115,7 @@ async fn simple_bolt12_send_receive() {
 	match event {
 		ref e @ Event::PaymentSuccessful { payment_id: ref evt_id, ref bolt12_invoice, .. } => {
 			println!("{} got event {:?}", node_a.node_id(), e);
-			assert_eq!(*evt_id, Some(payment_id));
+			assert_eq!(*evt_id, payment_id);
 			assert!(
 				bolt12_invoice.is_some(),
 				"bolt12_invoice should be present for BOLT12 payments"
@@ -2190,7 +2189,7 @@ async fn simple_bolt12_send_receive() {
 		)
 		.unwrap();
 
-	expect_payment_successful_event!(node_a, Some(payment_id), None);
+	expect_payment_successful_event!(node_a, payment_id, None);
 	let node_a_payments = node_a.list_payments_with_filter(|p| {
 		matches!(p.kind, PaymentKind::Bolt12Offer { .. }) && p.id == payment_id
 	});
@@ -2263,7 +2262,7 @@ async fn simple_bolt12_send_receive() {
 		.first()
 		.unwrap()
 		.id;
-	expect_payment_successful_event!(node_b, Some(node_b_payment_id), None);
+	expect_payment_successful_event!(node_b, node_b_payment_id, None);
 
 	let node_b_payments = node_b.list_payments_with_filter(|p| {
 		matches!(p.kind, PaymentKind::Bolt12Refund { .. }) && p.id == node_b_payment_id
@@ -2437,7 +2436,7 @@ async fn async_payment() {
 
 	node_receiver.start().unwrap();
 
-	expect_payment_successful_event!(node_sender, Some(payment_id), None);
+	expect_payment_successful_event!(node_sender, payment_id, None);
 }
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 1)]
@@ -2663,7 +2662,7 @@ async fn unified_send_receive_bip21_uri() {
 			},
 		};
 
-	expect_payment_successful_event!(node_a, Some(offer_payment_id), None);
+	expect_payment_successful_event!(node_a, offer_payment_id, None);
 
 	// Cut off the BOLT12 part to fallback to BOLT11.
 	let uri_str_without_offer = uri_str.split("&lno=").next().unwrap();
@@ -2683,7 +2682,7 @@ async fn unified_send_receive_bip21_uri() {
 				panic!("Expected Bolt11 payment but got error: {:?}", e);
 			},
 		};
-	expect_payment_successful_event!(node_a, Some(invoice_payment_id), None);
+	expect_payment_successful_event!(node_a, invoice_payment_id, None);
 
 	let expect_onchain_amount_sats = 800_000;
 	let onchain_uni_payment =
@@ -2818,9 +2817,9 @@ 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;
-	expect_payment_successful_event!(payer_node, Some(payment_id), None);
+	expect_payment_successful_event!(payer_node, payment_id, None);
 	let client_payment_id =
-		expect_payment_received_event!(client_node, expected_received_amount_msat).unwrap();
+		expect_payment_received_event!(client_node, expected_received_amount_msat);
 	let client_payment = client_node.payment(&client_payment_id).unwrap();
 	match client_payment.kind {
 		PaymentKind::Bolt11 { counterparty_skimmed_fee_msat, .. } => {
@@ -2847,7 +2846,7 @@ async fn do_lsps2_client_service_integration(client_trusts_lsp: bool) {
 	// are working as expected.
 	println!("Paying regular invoice!");
 	let payment_id = payer_node.bolt11_payment().send(&invoice, None).unwrap();
-	expect_payment_successful_event!(payer_node, Some(payment_id), None);
+	expect_payment_successful_event!(payer_node, payment_id, None);
 	expect_event!(service_node, PaymentForwarded);
 	expect_payment_received_event!(client_node, amount_msat);
 
@@ -2893,9 +2892,9 @@ async fn do_lsps2_client_service_integration(client_trusts_lsp: bool) {
 		.unwrap();
 
 	expect_event!(service_node, PaymentForwarded);
-	expect_payment_successful_event!(payer_node, Some(payment_id), None);
+	expect_payment_successful_event!(payer_node, payment_id, None);
 	let client_payment_id =
-		expect_payment_received_event!(client_node, expected_received_amount_msat).unwrap();
+		expect_payment_received_event!(client_node, expected_received_amount_msat);
 	let client_payment = client_node.payment(&client_payment_id).unwrap();
 	match client_payment.kind {
 		PaymentKind::Bolt11 { counterparty_skimmed_fee_msat, .. } => {
@@ -3000,7 +2999,7 @@ async fn spontaneous_send_with_custom_preimage() {
 		.unwrap();
 
 	// check payment status and verify stored preimage
-	expect_payment_successful_event!(node_a, Some(payment_id), None);
+	expect_payment_successful_event!(node_a, payment_id, None);
 	let details: PaymentDetails =
 		node_a.list_payments_with_filter(|p| p.id == payment_id).first().unwrap().clone();
 	assert_eq!(details.status, PaymentStatus::Succeeded);
@@ -3186,9 +3185,9 @@ async fn lsps2_client_trusts_lsp() {
 		.claim_for_hash(manual_payment_hash, jit_amount_msat, manual_preimage)
 		.unwrap();
 
-	expect_payment_successful_event!(payer_node, Some(payment_id), None);
+	expect_payment_successful_event!(payer_node, payment_id, None);
 
-	let _ = expect_payment_received_event!(client_node, expected_received_amount_msat).unwrap();
+	let _ = expect_payment_received_event!(client_node, expected_received_amount_msat);
 
 	// Check the nodes pick up on the confirmed funding tx now.
 	wait_for_tx(&electrsd.client, funding_txo.txid).await;

tests/upgrade_downgrade_tests.rs

@@ -304,7 +304,7 @@
 // ) {
 // 	match next_current_event(node).await {
 // 		ldk_node::Event::PaymentSuccessful { payment_id, .. } => {
-// 			assert_eq!(payment_id.as_ref(), Some(expected_payment_id));
+// 			assert_eq!(&payment_id, expected_payment_id);
 // 			node.event_handled().unwrap();
 // 		},
 // 		event => panic!("{} got unexpected event: {:?}", node.node_id(), event),
@@ -313,9 +313,8 @@
 //
 // async fn expect_current_payment_received(node: &CurrentNode, expected_amount_msat: u64) {
 // 	match next_current_event(node).await {
-// 		ldk_node::Event::PaymentReceived { amount_msat, payment_id, .. } => {
+// 		ldk_node::Event::PaymentReceived { amount_msat, .. } => {
 // 			assert_eq!(amount_msat, expected_amount_msat);
-// 			assert!(payment_id.is_some());
 // 			node.event_handled().unwrap();
 // 		},
 // 		event => panic!("{} got unexpected event: {:?}", node.node_id(), event),