From e246fc54cea3c1ed43408dbf142eba8e187c8c7e Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 6 Aug 2026 12:49:44 +0200 Subject: [PATCH 1/9] wallet: Deduplicate payment lookup on TxReplaced The `WalletEvent::TxReplaced` handler read the payment from the store twice, once inside a `debug_assert!` and once for real, so the assertion and the value actually used could in principle disagree. Reuse a single lookup instead. Co-Authored-By: HAL 9000 --- src/wallet/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index f8d9d521e..899eb34ec 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -401,13 +401,13 @@ impl Wallet { // The payment already exists in the store at this point: `bump_fee_rbf` updates // the payment store with the replacement txid before the next sync cycle, so we // can safely fetch it here. + let stored_payment = self.payment_store.get(&payment_id); debug_assert!( - self.payment_store.get(&payment_id).is_some(), + stored_payment.is_some(), "Payment {:?} expected in store during WalletEvent::TxReplaced but not found", payment_id, ); - let payment = - self.payment_store.get(&payment_id).ok_or(Error::InvalidPaymentId)?; + let payment = stored_payment.ok_or(Error::InvalidPaymentId)?; let pending_payment_details = self.create_pending_payment_from_tx(payment, conflict_txids.clone()); From 48503b96a35c3ec2401b16d93ff672c096d14701 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 6 Aug 2026 12:54:48 +0200 Subject: [PATCH 2/9] data_store: Make readers async and fallible `DataStore::get`, `contains_key` and `list_filter` were synchronous and infallible because every entry of a namespace is held in memory, so a lookup could never fail or block. That assumption goes away once a store may keep only a subset of its entries in memory and has to read through to the `KVStore` on a miss. Turn the readers into async methods and let `get` and `contains_key` report an error, so that a failed store read is never mistaken for "no such object". Behavior is unchanged: every reader still answers from memory and always returns `Ok`. `Node::payment` consequently returns a `Result`. Async and fallible are introduced together on purpose, so that adding the read-through paths later does not have to churn the same call sites twice. Co-Authored-By: HAL 9000 --- bindings/ldk_node.udl | 1 + src/data_store.rs | 34 +++--- src/event.rs | 51 +++++---- src/lib.rs | 11 +- src/payment/bolt11.rs | 4 +- src/payment/spontaneous.rs | 2 +- src/wallet/mod.rs | 32 +++--- tests/common/mod.rs | 196 +++++++++++++++++++++++--------- tests/integration_tests_rust.rs | 44 +++---- 9 files changed, 242 insertions(+), 133 deletions(-) diff --git a/bindings/ldk_node.udl b/bindings/ldk_node.udl index c1a926f2f..6b09cb9c5 100644 --- a/bindings/ldk_node.udl +++ b/bindings/ldk_node.udl @@ -147,6 +147,7 @@ interface Node { void update_channel_config([ByRef]UserChannelId user_channel_id, PublicKey counterparty_node_id, ChannelConfig channel_config); [Throws=NodeError] void sync_wallets(); + [Throws=NodeError] PaymentDetails? payment([ByRef]PaymentId payment_id); [Throws=NodeError] void remove_payment([ByRef]PaymentId payment_id); diff --git a/src/data_store.rs b/src/data_store.rs index b1ed816df..b25f2a91c 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -140,13 +140,13 @@ where Ok(()) } - /// Returns the current in-memory object for `id`. + /// Returns the object stored under `id`, if any. /// /// The async mutation lock serializes writers, but this synchronous reader cannot wait on it. /// Until store reads are async, callers may temporarily see in-memory state that has not yet /// caught up to a write in progress. - pub(crate) fn get(&self, id: &SO::Id) -> Option { - self.objects.lock().expect("lock").get(id).cloned() + pub(crate) async fn get(&self, id: &SO::Id) -> Result, Error> { + Ok(self.objects.lock().expect("lock").get(id).cloned()) } pub(crate) async fn update(&self, update: SO::Update) -> Result { @@ -170,12 +170,12 @@ where Ok(DataStoreUpdateResult::Updated) } - /// Returns in-memory objects matching `f`. + /// Returns all stored objects matching `f`. /// /// The async mutation lock serializes writers, but this synchronous reader cannot wait on it. /// Until store reads are async, callers may temporarily see in-memory state that has not yet /// caught up to a write in progress. - pub(crate) fn list_filter bool>(&self, f: F) -> Vec { + pub(crate) async fn list_filter bool>(&self, f: F) -> Vec { self.objects.lock().expect("lock").values().filter(f).cloned().collect::>() } @@ -211,13 +211,13 @@ where Ok(()) } - /// Returns whether the in-memory store contains `id`. + /// Returns whether an object is stored under `id`. /// /// The async mutation lock serializes writers, but this synchronous reader cannot wait on it. /// Until store reads are async, callers may temporarily see in-memory state that has not yet /// caught up to a write in progress. - pub(crate) fn contains_key(&self, id: &SO::Id) -> bool { - self.objects.lock().expect("lock").contains_key(id) + pub(crate) async fn contains_key(&self, id: &SO::Id) -> Result { + Ok(self.objects.lock().expect("lock").contains_key(id)) } } @@ -352,7 +352,7 @@ mod tests { ); let id = TestObjectId { id: [42u8; 4] }; - assert!(data_store.get(&id).is_none()); + assert!(data_store.get(&id).await.unwrap().is_none()); let store_key = id.encode_to_hex_str(); @@ -364,7 +364,7 @@ mod tests { // Check we successfully store an object and return `false` let object = TestObject { id, data: [23u8; 3] }; assert_eq!(Ok(false), data_store.insert(object.clone()).await); - assert_eq!(Some(object), data_store.get(&id)); + assert_eq!(Some(object), data_store.get(&id).await.unwrap()); assert!(KVStore::read(&*store, &primary_namespace, &secondary_namespace, &store_key) .await .is_ok()); @@ -373,12 +373,12 @@ mod tests { let mut override_object = object.clone(); override_object.data = [24u8; 3]; assert_eq!(Ok(true), data_store.insert(override_object).await); - assert_eq!(Some(override_object), data_store.get(&id)); + assert_eq!(Some(override_object), data_store.get(&id).await.unwrap()); // Check update returns `Updated` let update = TestObjectUpdate { id, data: [25u8; 3] }; assert_eq!(Ok(DataStoreUpdateResult::Updated), data_store.update(update).await); - assert_eq!(data_store.get(&id).unwrap().data, [25u8; 3]); + assert_eq!(data_store.get(&id).await.unwrap().unwrap().data, [25u8; 3]); // Check no-op update yields `Unchanged` let update = TestObjectUpdate { id, data: [25u8; 3] }; @@ -414,12 +414,12 @@ mod tests { Err(Error::PersistenceFailed), data_store.insert_or_update(updated_object).await ); - assert_eq!(Some(existing_object), data_store.get(&existing_id)); + assert_eq!(Some(existing_object), data_store.get(&existing_id).await.unwrap()); let new_id = TestObjectId { id: [55u8; 4] }; let new_object = TestObject { id: new_id, data: [34u8; 3] }; assert_eq!(Err(Error::PersistenceFailed), data_store.insert_or_update(new_object).await); - assert!(data_store.get(&new_id).is_none()); + assert!(data_store.get(&new_id).await.unwrap().is_none()); } #[tokio::test] @@ -429,7 +429,7 @@ mod tests { let data_store = new_failing_data_store(vec![]); assert_eq!(Err(Error::PersistenceFailed), data_store.insert(object).await); - assert!(data_store.get(&id).is_none()); + assert!(data_store.get(&id).await.unwrap().is_none()); } #[tokio::test] @@ -440,7 +440,7 @@ mod tests { let update = TestObjectUpdate { id, data: [24u8; 3] }; assert_eq!(Err(Error::PersistenceFailed), data_store.update(update).await); - assert_eq!(Some(object), data_store.get(&id)); + assert_eq!(Some(object), data_store.get(&id).await.unwrap()); } #[tokio::test] @@ -450,6 +450,6 @@ mod tests { let data_store = new_failing_data_store(vec![object]); assert_eq!(Err(Error::PersistenceFailed), data_store.remove(&id).await); - assert_eq!(Some(object), data_store.get(&id)); + assert_eq!(Some(object), data_store.get(&id).await.unwrap()); } } diff --git a/src/event.rs b/src/event.rs index a5d04217b..00ba59857 100644 --- a/src/event.rs +++ b/src/event.rs @@ -793,7 +793,13 @@ where .. } => { let payment_id = PaymentId(payment_hash.0); - let payment_info = self.payment_store.get(&payment_id); + let payment_info = match self.payment_store.get(&payment_id).await { + Ok(payment_info) => payment_info, + Err(e) => { + log_error!(self.logger, "Failed to access payment store: {}", e); + return Err(ReplayEvent()); + }, + }; if let Some(info) = payment_info.as_ref() { if info.direction == PaymentDirection::Outbound { log_info!( @@ -1211,24 +1217,31 @@ where }, }; - self.payment_store.get(&payment_id).map(|payment| { - let amount_msat = payment.amount_msat.expect( - "outbound payments should record their amount before they can succeed", - ); - log_info!( - self.logger, - "Successfully sent payment of {}msat{} from \ - payment hash {:?} with preimage {:?}", - amount_msat, - if let Some(fee) = fee_paid_msat { - format!(" (fee {} msat)", fee) - } else { - "".to_string() - }, - hex_utils::to_string(&payment_hash.0), - hex_utils::to_string(&payment_preimage.0) - ); - }); + match self.payment_store.get(&payment_id).await { + Ok(Some(payment)) => { + let amount_msat = payment.amount_msat.expect( + "outbound payments should record their amount before they can succeed", + ); + log_info!( + self.logger, + "Successfully sent payment of {}msat{} from \ + payment hash {:?} with preimage {:?}", + amount_msat, + if let Some(fee) = fee_paid_msat { + format!(" (fee {} msat)", fee) + } else { + "".to_string() + }, + hex_utils::to_string(&payment_hash.0), + hex_utils::to_string(&payment_preimage.0) + ); + }, + Ok(None) => {}, + Err(e) => { + log_error!(self.logger, "Failed to access payment store: {}", e); + return Err(ReplayEvent()); + }, + }; let event = Event::PaymentSuccessful { payment_id: Some(payment_id), payment_hash, diff --git a/src/lib.rs b/src/lib.rs index 14ee734a3..242988d47 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -2136,9 +2136,10 @@ impl Node { /// Retrieve the details of a specific payment with the given id. /// - /// Returns `Some` if the payment was known and `None` otherwise. - pub fn payment(&self, payment_id: &PaymentId) -> Option { - self.payment_store.get(payment_id) + /// Returns `Ok(Some(..))` if the payment was known and `Ok(None)` otherwise. Returns an error + /// if the payment could not be retrieved from the store. + pub fn payment(&self, payment_id: &PaymentId) -> Result, Error> { + self.runtime.block_on(self.payment_store.get(payment_id)) } /// Remove the payment with the given id from the store. @@ -2220,12 +2221,12 @@ impl Node { pub fn list_payments_with_filter bool>( &self, f: F, ) -> Vec { - self.payment_store.list_filter(f) + self.runtime.block_on(self.payment_store.list_filter(f)) } /// Retrieves all payments. pub fn list_payments(&self) -> Vec { - self.payment_store.list_filter(|_| true) + self.list_payments_with_filter(|_| true) } /// Retrieves a list of known peers. diff --git a/src/payment/bolt11.rs b/src/payment/bolt11.rs index 4503dfa06..07248080c 100644 --- a/src/payment/bolt11.rs +++ b/src/payment/bolt11.rs @@ -276,7 +276,7 @@ impl Bolt11Payment { let payment_hash = invoice.payment_hash(); let payment_id = PaymentId(invoice.payment_hash().0); - if let Some(payment) = self.payment_store.get(&payment_id) { + if let Some(payment) = self.runtime.block_on(self.payment_store.get(&payment_id))? { if payment.status == PaymentStatus::Pending || payment.status == PaymentStatus::Succeeded { @@ -506,7 +506,7 @@ impl Bolt11Payment { return Err(Error::InvalidPaymentPreimage); } - if let Some(details) = self.payment_store.get(&payment_id) { + if let Some(details) = self.runtime.block_on(self.payment_store.get(&payment_id))? { // 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 { diff --git a/src/payment/spontaneous.rs b/src/payment/spontaneous.rs index 45dab644d..f4e1cd93d 100644 --- a/src/payment/spontaneous.rs +++ b/src/payment/spontaneous.rs @@ -68,7 +68,7 @@ impl SpontaneousPayment { let payment_hash = PaymentHash::from(payment_preimage); let payment_id = PaymentId(payment_hash.0); - if let Some(payment) = self.payment_store.get(&payment_id) { + if let Some(payment) = self.runtime.block_on(self.payment_store.get(&payment_id))? { if payment.status == PaymentStatus::Pending || payment.status == PaymentStatus::Succeeded { diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index 899eb34ec..e490f8d00 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -257,6 +257,7 @@ impl Wallet { let payment_id = self .find_payment_by_txid(txid) + .await? .unwrap_or_else(|| PaymentId(txid.to_byte_array())); if self @@ -288,8 +289,9 @@ impl Wallet { } }, WalletEvent::ChainTipChanged { new_tip, .. } => { - let pending_payments: Vec = - self.pending_payment_store.list_filter(|p| { + let pending_payments: Vec = self + .pending_payment_store + .list_filter(|p| { debug_assert!( p.details.status == PaymentStatus::Pending, "Non-pending payment {:?} found in pending store", @@ -297,7 +299,8 @@ impl Wallet { ); p.details.status == PaymentStatus::Pending && matches!(p.details.kind, PaymentKind::Onchain { .. }) - }); + }) + .await; let mut unconfirmed_outbound_txids: Vec = Vec::new(); @@ -354,6 +357,7 @@ impl Wallet { WalletEvent::TxUnconfirmed { txid, tx, .. } => { let payment_id = self .find_payment_by_txid(txid) + .await? .unwrap_or_else(|| PaymentId(txid.to_byte_array())); if self @@ -384,7 +388,7 @@ impl Wallet { self.pending_payment_store.insert_or_update(pending_payment).await?; }, WalletEvent::TxReplaced { txid, conflicts, .. } => { - let Some(payment_id) = self.find_payment_by_txid(txid) else { + let Some(payment_id) = self.find_payment_by_txid(txid).await? else { log_error!( self.logger, "Could not find payment for replaced transaction {}. Skipping.", @@ -401,7 +405,7 @@ impl Wallet { // The payment already exists in the store at this point: `bump_fee_rbf` updates // the payment store with the replacement txid before the next sync cycle, so we // can safely fetch it here. - let stored_payment = self.payment_store.get(&payment_id); + let stored_payment = self.payment_store.get(&payment_id).await?; debug_assert!( stored_payment.is_some(), "Payment {:?} expected in store during WalletEvent::TxReplaced but not found", @@ -416,6 +420,7 @@ impl Wallet { WalletEvent::TxDropped { txid, tx } => { let payment_id = self .find_payment_by_txid(txid) + .await? .unwrap_or_else(|| PaymentId(txid.to_byte_array())); if self @@ -1472,10 +1477,10 @@ impl Wallet { PendingPaymentDetails::new(payment, conflicting_txids, Vec::new()) } - fn find_payment_by_txid(&self, target_txid: Txid) -> Option { + async fn find_payment_by_txid(&self, target_txid: Txid) -> Result, Error> { let direct_payment_id = PaymentId(target_txid.to_byte_array()); - if self.pending_payment_store.contains_key(&direct_payment_id) { - return Some(direct_payment_id); + if self.pending_payment_store.contains_key(&direct_payment_id).await? { + return Ok(Some(direct_payment_id)); } if let Some(replaced_details) = self @@ -1484,12 +1489,13 @@ impl Wallet { matches!(p.details.kind, PaymentKind::Onchain { txid, .. } if txid == target_txid) || p.conflicting_txids.contains(&target_txid) }) + .await .first() { - return Some(replaced_details.details.id); + return Ok(Some(replaced_details.details.id)); } - None + Ok(None) } /// If `payment_id` refers to a classified funding payment, refreshes its confirmation status @@ -1501,7 +1507,7 @@ impl Wallet { async fn apply_funding_status_update( &self, payment_id: PaymentId, event_txid: Txid, confirmation_status: ConfirmationStatus, ) -> Result { - let Some(mut payment) = self.payment_store.get(&payment_id) else { + let Some(mut payment) = self.payment_store.get(&payment_id).await? else { return Ok(false); }; let tx_type = match &payment.kind { @@ -1519,7 +1525,7 @@ impl Wallet { // one broadcast (an earlier, lower-fee candidate may win) and may carry no figures at all // (`None`) for a round we didn't contribute to. (`direction` is invariant across a splice's // candidates and cannot be changed through the store anyway.) - if let Some(pending) = self.pending_payment_store.get(&payment_id) { + if let Some(pending) = self.pending_payment_store.get(&payment_id).await? { if let Some(candidate) = pending.candidate(event_txid) { payment.amount_msat = candidate.amount_msat; payment.fee_paid_msat = candidate.fee_paid_msat; @@ -1544,7 +1550,7 @@ impl Wallet { pub(crate) async fn bump_fee_rbf( &self, payment_id: PaymentId, fee_rate: Option, cur_anchor_reserve_sats: u64, ) -> Result { - let payment = self.payment_store.get(&payment_id).ok_or_else(|| { + let payment = self.payment_store.get(&payment_id).await?.ok_or_else(|| { log_error!(self.logger, "Payment {} not found in payment store", payment_id); Error::InvalidPaymentId })?; diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 1fbbaad7e..9689b3296 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -224,7 +224,7 @@ 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(); + let payment = $node.payment(&payment_id.unwrap()).unwrap().unwrap(); if !matches!(payment.kind, ldk_node::payment::PaymentKind::Onchain { .. }) { assert_eq!(payment.fee_paid_msat, None); } @@ -292,7 +292,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()).unwrap().unwrap(); assert_eq!(payment.fee_paid_msat, fee_paid_msat); assert_eq!(payment_id, $payment_id); $node.event_handled().unwrap(); @@ -1276,23 +1276,41 @@ pub(crate) async fn do_channel_full_cycle( }, } expect_event!(node_b, PaymentReceived); - assert_eq!(node_a.payment(&payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_a.payment(&payment_id).unwrap().direction, PaymentDirection::Outbound); - assert_eq!(node_a.payment(&payment_id).unwrap().amount_msat, Some(invoice_amount_1_msat)); - assert!(matches!(node_a.payment(&payment_id).unwrap().kind, PaymentKind::Bolt11 { .. })); - assert_eq!(node_b.payment(&payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_b.payment(&payment_id).unwrap().direction, PaymentDirection::Inbound); - assert_eq!(node_b.payment(&payment_id).unwrap().amount_msat, Some(invoice_amount_1_msat)); - assert!(matches!(node_b.payment(&payment_id).unwrap().kind, PaymentKind::Bolt11 { .. })); + assert_eq!(node_a.payment(&payment_id).unwrap().unwrap().status, PaymentStatus::Succeeded); + assert_eq!(node_a.payment(&payment_id).unwrap().unwrap().direction, PaymentDirection::Outbound); + assert_eq!( + node_a.payment(&payment_id).unwrap().unwrap().amount_msat, + Some(invoice_amount_1_msat) + ); + assert!(matches!( + node_a.payment(&payment_id).unwrap().unwrap().kind, + PaymentKind::Bolt11 { .. } + )); + assert_eq!(node_b.payment(&payment_id).unwrap().unwrap().status, PaymentStatus::Succeeded); + assert_eq!(node_b.payment(&payment_id).unwrap().unwrap().direction, PaymentDirection::Inbound); + assert_eq!( + node_b.payment(&payment_id).unwrap().unwrap().amount_msat, + Some(invoice_amount_1_msat) + ); + assert!(matches!( + node_b.payment(&payment_id).unwrap().unwrap().kind, + PaymentKind::Bolt11 { .. } + )); // Assert we fail duplicate outbound payments and check the status hasn't changed. assert_eq!(Err(NodeError::DuplicatePayment), node_a.bolt11_payment().send(&invoice, None)); - assert_eq!(node_a.payment(&payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_a.payment(&payment_id).unwrap().direction, PaymentDirection::Outbound); - assert_eq!(node_a.payment(&payment_id).unwrap().amount_msat, Some(invoice_amount_1_msat)); - assert_eq!(node_b.payment(&payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_b.payment(&payment_id).unwrap().direction, PaymentDirection::Inbound); - assert_eq!(node_b.payment(&payment_id).unwrap().amount_msat, Some(invoice_amount_1_msat)); + assert_eq!(node_a.payment(&payment_id).unwrap().unwrap().status, PaymentStatus::Succeeded); + assert_eq!(node_a.payment(&payment_id).unwrap().unwrap().direction, PaymentDirection::Outbound); + assert_eq!( + node_a.payment(&payment_id).unwrap().unwrap().amount_msat, + Some(invoice_amount_1_msat) + ); + assert_eq!(node_b.payment(&payment_id).unwrap().unwrap().status, PaymentStatus::Succeeded); + assert_eq!(node_b.payment(&payment_id).unwrap().unwrap().direction, PaymentDirection::Inbound); + assert_eq!( + node_b.payment(&payment_id).unwrap().unwrap().amount_msat, + Some(invoice_amount_1_msat) + ); // Test under-/overpayment let invoice_amount_2_msat = 2500_000; @@ -1329,14 +1347,26 @@ pub(crate) async fn do_channel_full_cycle( }, }; assert_eq!(received_amount, overpaid_amount_msat); - assert_eq!(node_a.payment(&payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_a.payment(&payment_id).unwrap().direction, PaymentDirection::Outbound); - assert_eq!(node_a.payment(&payment_id).unwrap().amount_msat, Some(overpaid_amount_msat)); - assert!(matches!(node_a.payment(&payment_id).unwrap().kind, PaymentKind::Bolt11 { .. })); - assert_eq!(node_b.payment(&payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_b.payment(&payment_id).unwrap().direction, PaymentDirection::Inbound); - assert_eq!(node_b.payment(&payment_id).unwrap().amount_msat, Some(overpaid_amount_msat)); - assert!(matches!(node_b.payment(&payment_id).unwrap().kind, PaymentKind::Bolt11 { .. })); + assert_eq!(node_a.payment(&payment_id).unwrap().unwrap().status, PaymentStatus::Succeeded); + assert_eq!(node_a.payment(&payment_id).unwrap().unwrap().direction, PaymentDirection::Outbound); + assert_eq!( + node_a.payment(&payment_id).unwrap().unwrap().amount_msat, + Some(overpaid_amount_msat) + ); + assert!(matches!( + node_a.payment(&payment_id).unwrap().unwrap().kind, + PaymentKind::Bolt11 { .. } + )); + assert_eq!(node_b.payment(&payment_id).unwrap().unwrap().status, PaymentStatus::Succeeded); + assert_eq!(node_b.payment(&payment_id).unwrap().unwrap().direction, PaymentDirection::Inbound); + assert_eq!( + node_b.payment(&payment_id).unwrap().unwrap().amount_msat, + Some(overpaid_amount_msat) + ); + assert!(matches!( + node_b.payment(&payment_id).unwrap().unwrap().kind, + PaymentKind::Bolt11 { .. } + )); // Test "zero-amount" invoice payment println!("\nB receive_variable_amount_payment"); @@ -1367,14 +1397,26 @@ pub(crate) async fn do_channel_full_cycle( }, }; assert_eq!(received_amount, determined_amount_msat); - assert_eq!(node_a.payment(&payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_a.payment(&payment_id).unwrap().direction, PaymentDirection::Outbound); - assert_eq!(node_a.payment(&payment_id).unwrap().amount_msat, Some(determined_amount_msat)); - assert!(matches!(node_a.payment(&payment_id).unwrap().kind, PaymentKind::Bolt11 { .. })); - assert_eq!(node_b.payment(&payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_b.payment(&payment_id).unwrap().direction, PaymentDirection::Inbound); - assert_eq!(node_b.payment(&payment_id).unwrap().amount_msat, Some(determined_amount_msat)); - assert!(matches!(node_b.payment(&payment_id).unwrap().kind, PaymentKind::Bolt11 { .. })); + assert_eq!(node_a.payment(&payment_id).unwrap().unwrap().status, PaymentStatus::Succeeded); + assert_eq!(node_a.payment(&payment_id).unwrap().unwrap().direction, PaymentDirection::Outbound); + assert_eq!( + node_a.payment(&payment_id).unwrap().unwrap().amount_msat, + Some(determined_amount_msat) + ); + assert!(matches!( + node_a.payment(&payment_id).unwrap().unwrap().kind, + PaymentKind::Bolt11 { .. } + )); + assert_eq!(node_b.payment(&payment_id).unwrap().unwrap().status, PaymentStatus::Succeeded); + assert_eq!(node_b.payment(&payment_id).unwrap().unwrap().direction, PaymentDirection::Inbound); + assert_eq!( + node_b.payment(&payment_id).unwrap().unwrap().amount_msat, + Some(determined_amount_msat) + ); + assert!(matches!( + node_b.payment(&payment_id).unwrap().unwrap().kind, + PaymentKind::Bolt11 { .. } + )); // Test claiming manually registered payments. let invoice_amount_3_msat = 5_532_000; @@ -1403,20 +1445,38 @@ pub(crate) async fn do_channel_full_cycle( .unwrap(); expect_payment_received_event!(node_b, claimable_amount_msat); expect_payment_successful_event!(node_a, Some(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!( - node_a.payment(&manual_payment_id).unwrap().amount_msat, + node_a.payment(&manual_payment_id).unwrap().unwrap().status, + PaymentStatus::Succeeded + ); + assert_eq!( + node_a.payment(&manual_payment_id).unwrap().unwrap().direction, + PaymentDirection::Outbound + ); + assert_eq!( + node_a.payment(&manual_payment_id).unwrap().unwrap().amount_msat, Some(invoice_amount_3_msat) ); - assert!(matches!(node_a.payment(&manual_payment_id).unwrap().kind, PaymentKind::Bolt11 { .. })); - assert_eq!(node_b.payment(&manual_payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_b.payment(&manual_payment_id).unwrap().direction, PaymentDirection::Inbound); + assert!(matches!( + node_a.payment(&manual_payment_id).unwrap().unwrap().kind, + PaymentKind::Bolt11 { .. } + )); assert_eq!( - node_b.payment(&manual_payment_id).unwrap().amount_msat, + node_b.payment(&manual_payment_id).unwrap().unwrap().status, + PaymentStatus::Succeeded + ); + assert_eq!( + node_b.payment(&manual_payment_id).unwrap().unwrap().direction, + PaymentDirection::Inbound + ); + assert_eq!( + node_b.payment(&manual_payment_id).unwrap().unwrap().amount_msat, Some(invoice_amount_3_msat) ); - assert!(matches!(node_b.payment(&manual_payment_id).unwrap().kind, PaymentKind::Bolt11 { .. })); + assert!(matches!( + node_b.payment(&manual_payment_id).unwrap().unwrap().kind, + PaymentKind::Bolt11 { .. } + )); // Test failing manually registered payments. let invoice_amount_4_msat = 5_532_000; @@ -1442,30 +1502,36 @@ pub(crate) async fn do_channel_full_cycle( ); node_b.bolt11_payment().fail_for_hash(manual_fail_payment_hash).unwrap(); expect_event!(node_a, PaymentFailed); - assert_eq!(node_a.payment(&manual_fail_payment_id).unwrap().status, PaymentStatus::Failed); assert_eq!( - node_a.payment(&manual_fail_payment_id).unwrap().direction, + node_a.payment(&manual_fail_payment_id).unwrap().unwrap().status, + PaymentStatus::Failed + ); + assert_eq!( + node_a.payment(&manual_fail_payment_id).unwrap().unwrap().direction, PaymentDirection::Outbound ); assert_eq!( - node_a.payment(&manual_fail_payment_id).unwrap().amount_msat, + node_a.payment(&manual_fail_payment_id).unwrap().unwrap().amount_msat, Some(invoice_amount_4_msat) ); assert!(matches!( - node_a.payment(&manual_fail_payment_id).unwrap().kind, + node_a.payment(&manual_fail_payment_id).unwrap().unwrap().kind, PaymentKind::Bolt11 { .. } )); - assert_eq!(node_b.payment(&manual_fail_payment_id).unwrap().status, PaymentStatus::Failed); assert_eq!( - node_b.payment(&manual_fail_payment_id).unwrap().direction, + node_b.payment(&manual_fail_payment_id).unwrap().unwrap().status, + PaymentStatus::Failed + ); + assert_eq!( + node_b.payment(&manual_fail_payment_id).unwrap().unwrap().direction, PaymentDirection::Inbound ); assert_eq!( - node_b.payment(&manual_fail_payment_id).unwrap().amount_msat, + node_b.payment(&manual_fail_payment_id).unwrap().unwrap().amount_msat, Some(invoice_amount_4_msat) ); assert!(matches!( - node_b.payment(&manual_fail_payment_id).unwrap().kind, + node_b.payment(&manual_fail_payment_id).unwrap().unwrap().kind, PaymentKind::Bolt11 { .. } )); @@ -1490,19 +1556,37 @@ pub(crate) async fn do_channel_full_cycle( }, }; assert_eq!(received_keysend_amount, keysend_amount_msat); - assert_eq!(node_a.payment(&keysend_payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_a.payment(&keysend_payment_id).unwrap().direction, PaymentDirection::Outbound); - assert_eq!(node_a.payment(&keysend_payment_id).unwrap().amount_msat, Some(keysend_amount_msat)); + assert_eq!( + node_a.payment(&keysend_payment_id).unwrap().unwrap().status, + PaymentStatus::Succeeded + ); + assert_eq!( + node_a.payment(&keysend_payment_id).unwrap().unwrap().direction, + PaymentDirection::Outbound + ); + assert_eq!( + node_a.payment(&keysend_payment_id).unwrap().unwrap().amount_msat, + Some(keysend_amount_msat) + ); assert!(matches!( - node_a.payment(&keysend_payment_id).unwrap().kind, + node_a.payment(&keysend_payment_id).unwrap().unwrap().kind, PaymentKind::Spontaneous { .. } )); assert_eq!(received_custom_records, &custom_tlvs); - assert_eq!(node_b.payment(&keysend_payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_b.payment(&keysend_payment_id).unwrap().direction, PaymentDirection::Inbound); - assert_eq!(node_b.payment(&keysend_payment_id).unwrap().amount_msat, Some(keysend_amount_msat)); + assert_eq!( + node_b.payment(&keysend_payment_id).unwrap().unwrap().status, + PaymentStatus::Succeeded + ); + assert_eq!( + node_b.payment(&keysend_payment_id).unwrap().unwrap().direction, + PaymentDirection::Inbound + ); + assert_eq!( + node_b.payment(&keysend_payment_id).unwrap().unwrap().amount_msat, + Some(keysend_amount_msat) + ); assert!(matches!( - node_b.payment(&keysend_payment_id).unwrap().kind, + node_b.payment(&keysend_payment_id).unwrap().unwrap().kind, PaymentKind::Spontaneous { .. } )); assert_eq!( diff --git a/tests/integration_tests_rust.rs b/tests/integration_tests_rust.rs index df477588f..e332b4af7 100644 --- a/tests/integration_tests_rust.rs +++ b/tests/integration_tests_rust.rs @@ -791,7 +791,7 @@ async fn onchain_send_receive() { node_b.sync_wallets().unwrap(); let payment_id = PaymentId(txid.to_byte_array()); - let payment_a = node_a.payment(&payment_id).unwrap(); + let payment_a = node_a.payment(&payment_id).unwrap().unwrap(); assert_eq!(payment_a.status, PaymentStatus::Pending); match payment_a.kind { PaymentKind::Onchain { status, tx_type, .. } => { @@ -801,7 +801,7 @@ async fn onchain_send_receive() { _ => panic!("Unexpected payment kind"), } assert!(payment_a.fee_paid_msat > Some(0)); - let payment_b = node_b.payment(&payment_id).unwrap(); + let payment_b = node_b.payment(&payment_id).unwrap().unwrap(); assert_eq!(payment_b.status, PaymentStatus::Pending); match payment_b.kind { PaymentKind::Onchain { status, tx_type, .. } => { @@ -833,7 +833,7 @@ async fn onchain_send_receive() { node_b.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Onchain { .. })); assert_eq!(node_b_payments.len(), 3); - let payment_a = node_a.payment(&payment_id).unwrap(); + let payment_a = node_a.payment(&payment_id).unwrap().unwrap(); match payment_a.kind { PaymentKind::Onchain { txid: _txid, status, tx_type } => { assert_eq!(_txid, txid); @@ -843,7 +843,7 @@ async fn onchain_send_receive() { _ => panic!("Unexpected payment kind"), } - let payment_b = node_b.payment(&payment_id).unwrap(); + let payment_b = node_b.payment(&payment_id).unwrap().unwrap(); match payment_b.kind { PaymentKind::Onchain { txid: _txid, status, tx_type } => { assert_eq!(_txid, txid); @@ -932,7 +932,7 @@ async fn reorged_onchain_payment_returns_to_unconfirmed() { let payment_id = PaymentId(txid.to_byte_array()); for node in [&node_a, &node_b] { - let payment = node.payment(&payment_id).unwrap(); + let payment = node.payment(&payment_id).unwrap().unwrap(); assert_eq!(payment.status, PaymentStatus::Pending); match payment.kind { PaymentKind::Onchain { status, .. } => { @@ -958,7 +958,7 @@ async fn reorged_onchain_payment_returns_to_unconfirmed() { node_b.sync_wallets().unwrap(); for node in [&node_a, &node_b] { - let payment = node.payment(&payment_id).unwrap(); + let payment = node.payment(&payment_id).unwrap().unwrap(); assert_eq!(payment.status, PaymentStatus::Pending); match payment.kind { PaymentKind::Onchain { status, .. } => { @@ -1943,7 +1943,8 @@ async fn run_rbf_splice_channel_test(confirm_original: bool) { // the RBF replaces it, so it can be force-confirmed (instead of the RBF) further below. let original_candidate: Option<(Option, String)> = if confirm_original { let payment_id = PaymentId(original_txo.txid.to_byte_array()); - let fee = node_b.payment(&payment_id).expect("splice payment exists").fee_paid_msat; + let fee = + node_b.payment(&payment_id).unwrap().expect("splice payment exists").fee_paid_msat; let raw_tx: String = bitcoind .client .call("getrawtransaction", &[json!(original_txo.txid.to_string())]) @@ -1981,7 +1982,7 @@ async fn run_rbf_splice_channel_test(confirm_original: bool) { // the replacement. let rbf_candidate_fee = { let payment_id = PaymentId(original_txo.txid.to_byte_array()); - let payment = node_b.payment(&payment_id).expect("splice payment exists"); + let payment = node_b.payment(&payment_id).unwrap().expect("splice payment exists"); match payment.kind { PaymentKind::Onchain { txid, @@ -2056,7 +2057,7 @@ async fn run_rbf_splice_channel_test(confirm_original: bool) { // winning RBF candidate, and `fee_paid_msat` carries this node's `FundingContribution` fee. { let payment_id = PaymentId(original_txo.txid.to_byte_array()); - let payment = node_b.payment(&payment_id).expect("splice payment graduated"); + let payment = node_b.payment(&payment_id).unwrap().expect("splice payment graduated"); assert_eq!(payment.status, PaymentStatus::Succeeded); match payment.kind { PaymentKind::Onchain { txid, status: ConfirmationStatus::Confirmed { .. }, .. } => { @@ -2117,7 +2118,7 @@ async fn funding_payment_graduates_without_channel_ready() { // confirmations, asserted before draining any LDK event — so graduation is not driven by the // Lightning `ChannelReady` signal. let payment_id = PaymentId(funding_txo.txid.to_byte_array()); - let payment = node_a.payment(&payment_id).expect("funding payment exists"); + let payment = node_a.payment(&payment_id).unwrap().expect("funding payment exists"); assert_eq!(payment.status, PaymentStatus::Succeeded); match payment.kind { PaymentKind::Onchain { @@ -2180,7 +2181,7 @@ async fn splice_payment_reorged_to_unconfirmed() { node_b.sync_wallets().unwrap(); let payment_id = PaymentId(splice_txo.txid.to_byte_array()); - let payment = node_b.payment(&payment_id).expect("splice payment exists"); + let payment = node_b.payment(&payment_id).unwrap().expect("splice payment exists"); assert_eq!(payment.status, PaymentStatus::Pending); assert!(matches!( payment.kind, @@ -2203,7 +2204,7 @@ async fn splice_payment_reorged_to_unconfirmed() { // The funding payment returns to `Unconfirmed` and stays `Pending`, exercising the // `TxUnconfirmed` arm for a funding payment. - let payment = node_b.payment(&payment_id).expect("splice payment still exists"); + let payment = node_b.payment(&payment_id).unwrap().expect("splice payment still exists"); assert_eq!(payment.status, PaymentStatus::Pending); assert!(matches!( payment.kind, @@ -3016,7 +3017,7 @@ async fn do_lsps2_client_service_integration(client_trusts_lsp: bool) { expect_payment_successful_event!(payer_node, Some(payment_id), None); let client_payment_id = expect_payment_received_event!(client_node, expected_received_amount_msat).unwrap(); - let client_payment = client_node.payment(&client_payment_id).unwrap(); + let client_payment = client_node.payment(&client_payment_id).unwrap().unwrap(); match client_payment.kind { PaymentKind::Bolt11 { counterparty_skimmed_fee_msat, .. } => { assert_eq!(counterparty_skimmed_fee_msat, Some(service_fee_msat)); @@ -3091,7 +3092,7 @@ async fn do_lsps2_client_service_integration(client_trusts_lsp: bool) { expect_payment_successful_event!(payer_node, Some(payment_id), None); let client_payment_id = expect_payment_received_event!(client_node, expected_received_amount_msat).unwrap(); - let client_payment = client_node.payment(&client_payment_id).unwrap(); + let client_payment = client_node.payment(&client_payment_id).unwrap().unwrap(); match client_payment.kind { PaymentKind::Bolt11 { counterparty_skimmed_fee_msat, .. } => { assert_eq!(counterparty_skimmed_fee_msat, Some(service_fee_msat)); @@ -3138,7 +3139,7 @@ async fn do_lsps2_client_service_integration(client_trusts_lsp: bool) { client_node.bolt11_payment().fail_for_hash(manual_payment_hash).unwrap(); expect_event!(payer_node, PaymentFailed); - assert_eq!(client_node.payment(&payment_id).unwrap().status, PaymentStatus::Failed); + assert_eq!(client_node.payment(&payment_id).unwrap().unwrap().status, PaymentStatus::Failed); } #[tokio::test(flavor = "multi_thread", worker_threads = 1)] @@ -3604,7 +3605,10 @@ async fn payment_persistence_after_restart() { } // Verify payment succeeded - assert_eq!(node_a.payment(&payment_id).unwrap().status, PaymentStatus::Succeeded); + assert_eq!( + node_a.payment(&payment_id).unwrap().unwrap().status, + PaymentStatus::Succeeded + ); } println!("All {} payments completed successfully", num_payments); @@ -3869,7 +3873,7 @@ async fn onchain_fee_bump_rbf() { node_b.sync_wallets().unwrap(); let payment_id = PaymentId(txid.to_byte_array()); - let original_payment = node_b.payment(&payment_id).unwrap(); + let original_payment = node_b.payment(&payment_id).unwrap().unwrap(); let original_fee = original_payment.fee_paid_msat.unwrap(); // Non-existent payment id @@ -3898,7 +3902,7 @@ async fn onchain_fee_bump_rbf() { node_b.sync_wallets().unwrap(); // Verify fee increased and txid updated for node_b - let new_payment = node_b.payment(&payment_id).unwrap(); + let new_payment = node_b.payment(&payment_id).unwrap().unwrap(); assert!( new_payment.fee_paid_msat > Some(original_fee), "Fee should increase after RBF bump. Original: {}, New: {}", @@ -3922,7 +3926,7 @@ async fn onchain_fee_bump_rbf() { node_b.sync_wallets().unwrap(); // Verify second bump payment exists and txid updated for node_b - let second_payment = node_b.payment(&payment_id).unwrap(); + let second_payment = node_b.payment(&payment_id).unwrap().unwrap(); assert!( second_payment.fee_paid_msat > new_payment.fee_paid_msat, "Second bump should have higher fee than first bump" @@ -3948,7 +3952,7 @@ async fn onchain_fee_bump_rbf() { ); // Verify final payment is confirmed - let final_payment = node_b.payment(&payment_id).unwrap(); + let final_payment = node_b.payment(&payment_id).unwrap().unwrap(); assert_eq!(final_payment.status, PaymentStatus::Succeeded); match final_payment.kind { PaymentKind::Onchain { status, .. } => { From 121f3a1712c56c870767eebc8847a3317e256d79 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 6 Aug 2026 12:57:30 +0200 Subject: [PATCH 3/9] data_store: Let readers wait for in-flight writes Mutations persist to the `KVStore` first and only then update the in-memory state, so that a failed write leaves memory untouched. The readers, however, did not wait on the mutation lock, so during that window they could hand out an object the store had already moved past. The in-code comments documented this as a known caveat. Now that the readers are async they can wait, so turn the mutation lock into a read-write lock: mutations take the write guard across both steps, readers take the read guard and therefore never observe the intermediate state. This also becomes load-bearing once entries may be read back from the store on a cache miss, because a reader that repopulates memory from a value it read before a concurrent write would otherwise leave memory durably disagreeing with the store. Readers now block for the duration of an in-flight write, which for a remote backend is one network round trip. Co-Authored-By: HAL 9000 --- src/data_store.rs | 132 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 114 insertions(+), 18 deletions(-) diff --git a/src/data_store.rs b/src/data_store.rs index b25f2a91c..5e7bc1ed3 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -45,7 +45,12 @@ where L::Target: LdkLogger, { objects: Mutex>, - mutation_lock: tokio::sync::Mutex<()>, + // Serializes mutations against each other and against readers. Writers hold the write guard + // across both the store write and the subsequent in-memory update, so readers taking the read + // guard never observe the window in between, in which the store is already ahead of memory. + // + // Note the `objects` lock is always taken *inside* this one, and never held across an `.await`. + mutation_lock: tokio::sync::RwLock<()>, primary_namespace: String, secondary_namespace: String, kv_store: Arc, @@ -64,7 +69,7 @@ where Mutex::new(HashMap::from_iter(objects.into_iter().map(|obj| (obj.id(), obj)))); Self { objects, - mutation_lock: tokio::sync::Mutex::new(()), + mutation_lock: tokio::sync::RwLock::new(()), primary_namespace, secondary_namespace, kv_store, @@ -73,7 +78,7 @@ where } pub(crate) async fn insert(&self, object: SO) -> Result { - let _guard = self.mutation_lock.lock().await; + let _guard = self.mutation_lock.write().await; self.persist(&object).await?; let mut locked_objects = self.objects.lock().expect("lock"); @@ -82,7 +87,7 @@ where } pub(crate) async fn insert_or_update(&self, object: SO) -> Result { - let _guard = self.mutation_lock.lock().await; + let _guard = self.mutation_lock.write().await; let id = object.id(); let data_to_persist = { @@ -112,7 +117,7 @@ where } pub(crate) async fn remove(&self, id: &SO::Id) -> Result<(), Error> { - let _guard = self.mutation_lock.lock().await; + let _guard = self.mutation_lock.write().await; let should_remove = { self.objects.lock().expect("lock").contains_key(id) }; if should_remove { let store_key = id.encode_to_hex_str(); @@ -141,16 +146,13 @@ where } /// Returns the object stored under `id`, if any. - /// - /// The async mutation lock serializes writers, but this synchronous reader cannot wait on it. - /// Until store reads are async, callers may temporarily see in-memory state that has not yet - /// caught up to a write in progress. pub(crate) async fn get(&self, id: &SO::Id) -> Result, Error> { + let _guard = self.mutation_lock.read().await; Ok(self.objects.lock().expect("lock").get(id).cloned()) } pub(crate) async fn update(&self, update: SO::Update) -> Result { - let _guard = self.mutation_lock.lock().await; + let _guard = self.mutation_lock.write().await; let id = update.id(); let updated_object = { let locked_objects = self.objects.lock().expect("lock"); @@ -171,11 +173,8 @@ where } /// Returns all stored objects matching `f`. - /// - /// The async mutation lock serializes writers, but this synchronous reader cannot wait on it. - /// Until store reads are async, callers may temporarily see in-memory state that has not yet - /// caught up to a write in progress. pub(crate) async fn list_filter bool>(&self, f: F) -> Vec { + let _guard = self.mutation_lock.read().await; self.objects.lock().expect("lock").values().filter(f).cloned().collect::>() } @@ -212,20 +211,20 @@ where } /// Returns whether an object is stored under `id`. - /// - /// The async mutation lock serializes writers, but this synchronous reader cannot wait on it. - /// Until store reads are async, callers may temporarily see in-memory state that has not yet - /// caught up to a write in progress. pub(crate) async fn contains_key(&self, id: &SO::Id) -> Result { + let _guard = self.mutation_lock.read().await; Ok(self.objects.lock().expect("lock").contains_key(id)) } } #[cfg(test)] mod tests { + use std::time::Duration; + use lightning::util::persist::{PageToken, PaginatedKVStore, PaginatedListResponse}; use lightning::util::test_utils::TestLogger; use lightning::{impl_writeable_tlv_based, io}; + use tokio::sync::Notify; use super::*; use crate::hex_utils; @@ -337,6 +336,103 @@ mod tests { ) } + /// A store that parks every `write` until it is released, so that tests can hold a write in + /// flight and observe what concurrent readers see in the meantime. + struct GatedStore { + inner: InMemoryStore, + /// Notified by the store once a `write` has parked. + write_parked: Arc, + /// Awaited by the store; notify to let the parked `write` proceed. + release_write: Arc, + } + + impl KVStore for GatedStore { + fn read( + &self, primary_namespace: &str, secondary_namespace: &str, key: &str, + ) -> impl std::future::Future, io::Error>> + 'static + Send { + self.inner.read(primary_namespace, secondary_namespace, key) + } + + fn write( + &self, primary_namespace: &str, secondary_namespace: &str, key: &str, buf: Vec, + ) -> impl std::future::Future> + 'static + Send { + let write_parked = Arc::clone(&self.write_parked); + let release_write = Arc::clone(&self.release_write); + let inner_fut = self.inner.write(primary_namespace, secondary_namespace, key, buf); + async move { + write_parked.notify_one(); + release_write.notified().await; + inner_fut.await + } + } + + fn remove( + &self, primary_namespace: &str, secondary_namespace: &str, key: &str, lazy: bool, + ) -> impl std::future::Future> + 'static + Send { + self.inner.remove(primary_namespace, secondary_namespace, key, lazy) + } + + fn list( + &self, primary_namespace: &str, secondary_namespace: &str, + ) -> impl std::future::Future, io::Error>> + 'static + Send { + self.inner.list(primary_namespace, secondary_namespace) + } + } + + impl PaginatedKVStore for GatedStore { + fn list_paginated( + &self, primary_namespace: &str, secondary_namespace: &str, + page_token: Option, + ) -> impl std::future::Future> + 'static + Send + { + self.inner.list_paginated(primary_namespace, secondary_namespace, page_token) + } + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn readers_wait_for_in_flight_writes() { + let write_parked = Arc::new(Notify::new()); + let release_write = Arc::new(Notify::new()); + let store: Arc = Arc::new(DynStoreWrapper(GatedStore { + inner: InMemoryStore::new(), + write_parked: Arc::clone(&write_parked), + release_write: Arc::clone(&release_write), + })); + let logger = Arc::new(TestLogger::new()); + + let id = TestObjectId { id: [42u8; 4] }; + let old_object = TestObject { id, data: [23u8; 3] }; + let new_object = TestObject { id, data: [24u8; 3] }; + + let data_store: Arc>> = Arc::new(DataStore::new( + vec![old_object], + "datastore_test_primary".to_string(), + "datastore_test_secondary".to_string(), + store, + logger, + )); + + let writer_store = Arc::clone(&data_store); + let writer = tokio::spawn(async move { writer_store.insert(new_object).await }); + + // Wait until the write has been handed to the store and parked there, i.e., until the + // object has been persisted but the in-memory state has not caught up yet. + write_parked.notified().await; + + // A reader must not be able to observe that window: it has to wait for the writer rather + // than hand out the pre-write object. + let read_res = tokio::time::timeout(Duration::from_millis(200), data_store.get(&id)).await; + assert!( + read_res.is_err(), + "Reader observed {:?} while a write was still in flight", + read_res.unwrap() + ); + + release_write.notify_one(); + assert_eq!(Ok(true), writer.await.unwrap()); + assert_eq!(Some(new_object), data_store.get(&id).await.unwrap()); + } + #[tokio::test] async fn data_is_persisted() { let store: Arc = Arc::new(DynStoreWrapper(InMemoryStore::new())); From 55432cb1bb71a1241a11efc146e7a47850d1daa5 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 6 Aug 2026 13:05:49 +0200 Subject: [PATCH 4/9] data_store: Add a per-store cache policy `DataStore` held every object of its namespace in memory for the lifetime of the node. That is fine for the pending-payment store, which drops entries as payments resolve, but the payment store grows without bound, so memory use and startup time grow with a node's history. Give each store a caching policy, either keeping all entries as before, or keeping only a bounded number of least recently used ones and reading the rest back from the store on demand. Both existing stores keep all their entries, so nothing changes yet. The policy is a type parameter rather than a plain value so that `list_filter`, which can only answer correctly while everything is in memory, is unavailable on a bounded store. Reaching for a full scan where it would silently return a subset is a compile error. A bounded store also has to read through on its write paths, not just on reads: merging, updating or removing against a cache miss would otherwise overwrite an evicted object with a partial one, drop an update as if the object were unknown, or leave a removed object in the store forever. A miss is only evidence of absence when the cache holds everything. Co-Authored-By: HAL 9000 --- src/builder.rs | 6 + src/data_store.rs | 948 +++++++++++++++++++++++++++++++++++++++++----- src/types.rs | 6 +- 3 files changed, 864 insertions(+), 96 deletions(-) diff --git a/src/builder.rs b/src/builder.rs index f11780099..f0b8d17d7 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -53,6 +53,7 @@ use crate::config::{ DEFAULT_MAX_PROBE_AMOUNT_MSAT, DEFAULT_MIN_PROBE_AMOUNT_MSAT, }; use crate::connection::ConnectionManager; +use crate::data_store::KeepAllEntries; use crate::entropy::NodeEntropy; use crate::event::EventQueue; use crate::fee_estimator::OnchainFeeEstimator; @@ -1474,6 +1475,7 @@ fn build_with_store_internal( let payment_store = match payment_store_res { Ok(payments) => Arc::new(PaymentStore::new( payments, + KeepAllEntries, PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE.to_string(), PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE.to_string(), Arc::clone(&kv_store), @@ -1728,8 +1730,12 @@ fn build_with_store_internal( }; let pending_payment_store = match pending_payment_store_res { + // NOTE: This store must keep all its entries in memory: the wallet scans it in full on + // every chain tip change and to resolve replaced transactions. It stays bounded anyway, + // as entries are removed once a payment is no longer pending. Ok(pending_payments) => Arc::new(PendingPaymentStore::new( pending_payments, + KeepAllEntries, PENDING_PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE.to_string(), PENDING_PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE.to_string(), Arc::clone(&kv_store), diff --git a/src/data_store.rs b/src/data_store.rs index 5e7bc1ed3..a9dd3337d 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -5,10 +5,13 @@ // http://opensource.org/licenses/MIT>, at your option. You may not use this file except in // accordance with one or both of these licenses. -use std::collections::HashMap; +use std::collections::{BTreeMap, HashMap}; +use std::marker::PhantomData; +use std::num::NonZeroUsize; use std::ops::Deref; use std::sync::{Arc, Mutex}; +use lightning::io::ErrorKind; use lightning::util::persist::KVStore; use lightning::util::ser::{Readable, Writeable}; @@ -25,7 +28,7 @@ pub(crate) trait StorableObject: Clone + Readable + Writeable { fn to_update(&self) -> Self::Update; } -pub(crate) trait StorableObjectId: std::hash::Hash + PartialEq + Eq { +pub(crate) trait StorableObjectId: Clone + std::hash::Hash + PartialEq + Eq { fn encode_to_hex_str(&self) -> String; } @@ -40,142 +43,446 @@ pub(crate) enum DataStoreUpdateResult { NotFound, } -pub(crate) struct DataStore +/// How many of a namespace's objects a [`DataStore`] keeps in memory. +#[derive(PartialEq, Eq, Debug, Clone, Copy)] +pub(crate) enum CacheLimit { + /// Keep every object in memory. + Unbounded, + /// Keep at most this many objects in memory. + Bounded(NonZeroUsize), +} + +/// The caching policy of a [`DataStore`]. +/// +/// This is a type parameter rather than a plain value so that operations which are only +/// meaningful while every object is held in memory — currently [`DataStore::list_filter`] — can be +/// restricted to the stores that satisfy that, and using them elsewhere is a compile error rather +/// than a silently incomplete result. +pub(crate) trait CachePolicy: Send + Sync + 'static { + fn cache_limit(&self) -> CacheLimit; +} + +/// Keeps every object of the namespace in memory. +/// +/// Reads are served entirely from memory and never hit the [`KVStore`]. Required for stores whose +/// consumers rely on full scans via [`DataStore::list_filter`], and for stores small enough that +/// bounding them would buy nothing. +pub(crate) struct KeepAllEntries; + +impl CachePolicy for KeepAllEntries { + fn cache_limit(&self) -> CacheLimit { + CacheLimit::Unbounded + } +} + +/// Keeps at most `capacity` least-recently-used objects in memory, reading through to the +/// [`KVStore`] whenever a lookup misses. +/// +/// Suitable for namespaces that grow without bound over a node's lifetime. +#[allow(dead_code)] // Constructed once a store opts into a bounded cache. +pub(crate) struct KeepLeastRecentlyUsed { + capacity: NonZeroUsize, +} + +#[allow(dead_code)] // See above. +impl KeepLeastRecentlyUsed { + pub(crate) fn new(capacity: NonZeroUsize) -> Self { + Self { capacity } + } +} + +impl CachePolicy for KeepLeastRecentlyUsed { + fn cache_limit(&self) -> CacheLimit { + CacheLimit::Bounded(self.capacity) + } +} + +/// A least-recently-used cache of at most `capacity` objects. +/// +/// Recency is tracked with a monotonically increasing sequence number per entry, mirrored in +/// `recency` so that the least recently used entry is the first one in it. This trades `O(1)` for +/// `O(log n)` against a far smaller amount of code than an intrusive list would need, which is a +/// good deal at the cache sizes we expect. +struct LruCache { + capacity: NonZeroUsize, + entries: HashMap, + recency: BTreeMap, + // Wrapping this would take 2^64 mutations of a single store, so we don't guard against it. + next_seq: u64, +} + +impl LruCache { + fn new(capacity: NonZeroUsize) -> Self { + Self { capacity, entries: HashMap::new(), recency: BTreeMap::new(), next_seq: 0 } + } + + fn take_seq(&mut self) -> u64 { + let seq = self.next_seq; + self.next_seq += 1; + seq + } + + fn insert(&mut self, id: SO::Id, object: SO) { + let seq = self.take_seq(); + if let Some((_, prev_seq)) = self.entries.insert(id.clone(), (object, seq)) { + self.recency.remove(&prev_seq); + } + self.recency.insert(seq, id); + debug_assert_eq!(self.entries.len(), self.recency.len()); + + while self.entries.len() > self.capacity.get() { + let Some((_, evicted_id)) = self.recency.pop_first() else { break }; + self.entries.remove(&evicted_id); + } + debug_assert_eq!(self.entries.len(), self.recency.len()); + debug_assert!(self.entries.len() <= self.capacity.get()); + } + + fn get(&mut self, id: &SO::Id) -> Option { + let seq = self.take_seq(); + let (object, prev_seq) = { + let entry = self.entries.get_mut(id)?; + let prev_seq = entry.1; + entry.1 = seq; + (entry.0.clone(), prev_seq) + }; + self.recency.remove(&prev_seq); + self.recency.insert(seq, id.clone()); + debug_assert_eq!(self.entries.len(), self.recency.len()); + Some(object) + } + + fn remove(&mut self, id: &SO::Id) { + if let Some((_, seq)) = self.entries.remove(id) { + self.recency.remove(&seq); + } + debug_assert_eq!(self.entries.len(), self.recency.len()); + } +} + +/// The in-memory part of a [`DataStore`]. +/// +/// Modelled as an enum rather than a map plus a policy field so that a [`KeepAllEntries`] store +/// provably pays nothing for the bookkeeping a bounded one needs: its representation is just the +/// map it always was. +enum ObjectCache { + KeepAll(HashMap), + BoundedLru(LruCache), +} + +impl ObjectCache { + fn new(cache_limit: CacheLimit, objects: Vec) -> Self { + match cache_limit { + CacheLimit::Unbounded => Self::KeepAll(HashMap::from_iter( + objects.into_iter().map(|object| (object.id(), object)), + )), + CacheLimit::Bounded(capacity) => { + let mut lru = LruCache::new(capacity); + for object in objects { + lru.insert(object.id(), object); + } + Self::BoundedLru(lru) + }, + } + } + + /// Whether the cache holds *every* object of the namespace. + /// + /// If it does, a miss proves the object is absent from the store, and iterating the cache + /// yields a complete listing. If it doesn't, both require reading from the store. + fn is_authoritative(&self) -> bool { + matches!(self, Self::KeepAll(_)) + } + + /// Returns the cached object for `id`, marking it as most recently used. + fn get(&mut self, id: &SO::Id) -> Option { + match self { + Self::KeepAll(objects) => objects.get(id).cloned(), + Self::BoundedLru(lru) => lru.get(id), + } + } + + /// Returns whether `id` is cached, without marking it as most recently used. + fn contains(&self, id: &SO::Id) -> bool { + match self { + Self::KeepAll(objects) => objects.contains_key(id), + Self::BoundedLru(lru) => lru.entries.contains_key(id), + } + } + + fn insert(&mut self, id: SO::Id, object: SO) { + match self { + Self::KeepAll(objects) => { + objects.insert(id, object); + }, + Self::BoundedLru(lru) => lru.insert(id, object), + } + } + + fn remove(&mut self, id: &SO::Id) { + match self { + Self::KeepAll(objects) => { + objects.remove(id); + }, + Self::BoundedLru(lru) => lru.remove(id), + } + } + + /// Returns the *cached* objects matching `f`, which is only a complete listing of the + /// namespace if [`Self::is_authoritative`]. + fn filter bool>(&self, f: F) -> Vec { + match self { + Self::KeepAll(objects) => objects.values().filter(f).cloned().collect(), + Self::BoundedLru(lru) => { + lru.entries.values().map(|(object, _)| object).filter(f).cloned().collect() + }, + } + } + + #[cfg(test)] + fn len(&self) -> usize { + match self { + Self::KeepAll(objects) => objects.len(), + Self::BoundedLru(lru) => lru.entries.len(), + } + } +} + +pub(crate) struct DataStore where L::Target: LdkLogger, { - objects: Mutex>, + cache: Mutex>, // Serializes mutations against each other and against readers. Writers hold the write guard // across both the store write and the subsequent in-memory update, so readers taking the read // guard never observe the window in between, in which the store is already ahead of memory. // - // Note the `objects` lock is always taken *inside* this one, and never held across an `.await`. + // Note the `cache` lock is always taken *inside* this one, and never held across an `.await`. mutation_lock: tokio::sync::RwLock<()>, primary_namespace: String, secondary_namespace: String, kv_store: Arc, logger: L, + cache_policy: PhantomData

, } -impl DataStore +impl DataStore where L::Target: LdkLogger, { + /// Creates a new store over the given namespace. + /// + /// `objects` seeds the cache and must already be persisted under that namespace: under a + /// bounded policy any object beyond `cache_policy`'s capacity is dropped from memory + /// immediately, and is only recoverable by reading it back from the store. pub(crate) fn new( - objects: Vec, primary_namespace: String, secondary_namespace: String, + objects: Vec, cache_policy: P, primary_namespace: String, secondary_namespace: String, kv_store: Arc, logger: L, ) -> Self { - let objects = - Mutex::new(HashMap::from_iter(objects.into_iter().map(|obj| (obj.id(), obj)))); + let cache = Mutex::new(ObjectCache::new(cache_policy.cache_limit(), objects)); Self { - objects, + cache, mutation_lock: tokio::sync::RwLock::new(()), primary_namespace, secondary_namespace, kv_store, logger, + cache_policy: PhantomData, } } + /// Stores `object`, overwriting any object previously stored under the same id. + /// + /// Returns whether an object was previously stored under that id. pub(crate) async fn insert(&self, object: SO) -> Result { let _guard = self.mutation_lock.write().await; + let id = object.id(); + // Callers treat the return value as "this id was already known", so a cache miss is not + // enough to answer it under a bounded policy. + let replaced = self.contains(&id).await?; self.persist(&object).await?; - let mut locked_objects = self.objects.lock().expect("lock"); - let updated = locked_objects.insert(object.id(), object).is_some(); - Ok(updated) + self.cache.lock().expect("lock").insert(id, object); + Ok(replaced) } + /// Merges `object` into any object already stored under the same id, or stores it as-is if + /// there is none. + /// + /// Returns whether anything was written. pub(crate) async fn insert_or_update(&self, object: SO) -> Result { let _guard = self.mutation_lock.write().await; let id = object.id(); - let data_to_persist = { - let locked_objects = self.objects.lock().expect("lock"); - if let Some(existing_object) = locked_objects.get(&id) { - let mut updated_object = existing_object.clone(); - let updated = updated_object.update(object.to_update()); - if updated { - Some(updated_object) - } else { - None - } - } else { - Some(object) - } + // Note we have to look through to the store here: merging against a cache miss would + // overwrite an evicted object with whatever the caller happens to know about it. + let data_to_persist = match self.lookup(&id).await? { + Some(mut existing_object) => { + existing_object.update(object.to_update()).then_some(existing_object) + }, + None => Some(object), }; match data_to_persist { Some(updated_object) => { self.persist(&updated_object).await?; - let mut locked_objects = self.objects.lock().expect("lock"); - locked_objects.insert(id, updated_object); + self.cache.lock().expect("lock").insert(id, updated_object); Ok(true) }, None => Ok(false), } } + /// Removes the object stored under `id`, if any. pub(crate) async fn remove(&self, id: &SO::Id) -> Result<(), Error> { let _guard = self.mutation_lock.write().await; - let should_remove = { self.objects.lock().expect("lock").contains_key(id) }; - if should_remove { - let store_key = id.encode_to_hex_str(); - KVStore::remove( - &*self.kv_store, + + if !self.contains(id).await? { + return Ok(()); + } + + let store_key = id.encode_to_hex_str(); + KVStore::remove( + &*self.kv_store, + &self.primary_namespace, + &self.secondary_namespace, + &store_key, + false, + ) + .await + .map_err(|e| { + log_error!( + self.logger, + "Removing object data for key {}/{}/{} failed due to: {}", &self.primary_namespace, &self.secondary_namespace, - &store_key, - false, - ) - .await - .map_err(|e| { - log_error!( - self.logger, - "Removing object data for key {}/{}/{} failed due to: {}", - &self.primary_namespace, - &self.secondary_namespace, - store_key, - e - ); - Error::PersistenceFailed - })?; - self.objects.lock().expect("lock").remove(id); - } + store_key, + e + ); + Error::PersistenceFailed + })?; + self.cache.lock().expect("lock").remove(id); Ok(()) } /// Returns the object stored under `id`, if any. pub(crate) async fn get(&self, id: &SO::Id) -> Result, Error> { let _guard = self.mutation_lock.read().await; - Ok(self.objects.lock().expect("lock").get(id).cloned()) + self.lookup(id).await } + /// Applies `update` to the object stored under its id. pub(crate) async fn update(&self, update: SO::Update) -> Result { let _guard = self.mutation_lock.write().await; + let id = update.id(); - let updated_object = { - let locked_objects = self.objects.lock().expect("lock"); - let Some(object) = locked_objects.get(&id) else { - return Ok(DataStoreUpdateResult::NotFound); - }; - let mut updated_object = object.clone(); - if !updated_object.update(update) { - return Ok(DataStoreUpdateResult::Unchanged); - } - updated_object + // As in `insert_or_update`, a cache miss is not evidence of absence: reporting `NotFound` + // for a merely evicted object would drop the update on the floor. + let Some(mut updated_object) = self.lookup(&id).await? else { + return Ok(DataStoreUpdateResult::NotFound); }; + if !updated_object.update(update) { + return Ok(DataStoreUpdateResult::Unchanged); + } self.persist(&updated_object).await?; - let mut locked_objects = self.objects.lock().expect("lock"); - locked_objects.insert(id, updated_object); + self.cache.lock().expect("lock").insert(id, updated_object); Ok(DataStoreUpdateResult::Updated) } - /// Returns all stored objects matching `f`. - pub(crate) async fn list_filter bool>(&self, f: F) -> Vec { + /// Returns whether an object is stored under `id`. + pub(crate) async fn contains_key(&self, id: &SO::Id) -> Result { let _guard = self.mutation_lock.read().await; - self.objects.lock().expect("lock").values().filter(f).cloned().collect::>() + self.contains(id).await + } + + /// Returns the object stored under `id`, reading through to the [`KVStore`] if the cache is + /// not authoritative and misses. + /// + /// The caller must hold `mutation_lock`. + async fn lookup(&self, id: &SO::Id) -> Result, Error> { + let (cached_object, is_authoritative) = { + let mut locked_cache = self.cache.lock().expect("lock"); + (locked_cache.get(id), locked_cache.is_authoritative()) + }; + + if let Some(object) = cached_object { + return Ok(Some(object)); + } + if is_authoritative { + return Ok(None); + } + + let Some(bytes) = self.read_raw(id).await? else { + return Ok(None); + }; + let object = self.decode(id, &bytes)?; + self.cache.lock().expect("lock").insert(id.clone(), object.clone()); + Ok(Some(object)) + } + + /// Returns whether an object is stored under `id`, without deserializing it or caching it. + /// + /// The caller must hold `mutation_lock`. + async fn contains(&self, id: &SO::Id) -> Result { + let (is_cached, is_authoritative) = { + let locked_cache = self.cache.lock().expect("lock"); + (locked_cache.contains(id), locked_cache.is_authoritative()) + }; + + if is_cached { + return Ok(true); + } + if is_authoritative { + return Ok(false); + } + + Ok(self.read_raw(id).await?.is_some()) + } + + /// Reads the bytes stored under `id`, returning `Ok(None)` if and only if the key is absent. + /// + /// The caller must hold `mutation_lock`. + async fn read_raw(&self, id: &SO::Id) -> Result>, Error> { + let store_key = id.encode_to_hex_str(); + match KVStore::read( + &*self.kv_store, + &self.primary_namespace, + &self.secondary_namespace, + &store_key, + ) + .await + { + Ok(bytes) => Ok(Some(bytes)), + // An absent key is a legitimate answer, everything else is a failure we must not + // report as "no such object". + Err(e) if e.kind() == ErrorKind::NotFound => Ok(None), + Err(e) => { + log_error!( + self.logger, + "Read for key {}/{}/{} failed due to: {}", + &self.primary_namespace, + &self.secondary_namespace, + store_key, + e + ); + Err(Error::PersistenceFailed) + }, + } + } + + fn decode(&self, id: &SO::Id, bytes: &[u8]) -> Result { + SO::read(&mut &bytes[..]).map_err(|e| { + log_error!( + self.logger, + "Failed to deserialize object for key {}/{}/{}: {}", + &self.primary_namespace, + &self.secondary_namespace, + id.encode_to_hex_str(), + e + ); + Error::PersistenceFailed + }) } async fn persist(&self, object: &SO) -> Result<(), Error> { @@ -210,15 +517,35 @@ where Ok(()) } - /// Returns whether an object is stored under `id`. - pub(crate) async fn contains_key(&self, id: &SO::Id) -> Result { + #[cfg(test)] + fn cached_len(&self) -> usize { + self.cache.lock().expect("lock").len() + } + + #[cfg(test)] + fn is_cached(&self, id: &SO::Id) -> bool { + self.cache.lock().expect("lock").contains(id) + } +} + +impl DataStore +where + L::Target: LdkLogger, +{ + /// Returns all stored objects matching `f`. + /// + /// Only available on stores that keep every object in memory: answering this on a bounded + /// store would mean reading its entire namespace back, which is exactly what such a store + /// exists to avoid. + pub(crate) async fn list_filter bool>(&self, f: F) -> Vec { let _guard = self.mutation_lock.read().await; - Ok(self.objects.lock().expect("lock").contains_key(id)) + self.cache.lock().expect("lock").filter(f) } } #[cfg(test)] mod tests { + use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; use std::time::Duration; use lightning::util::persist::{PageToken, PaginatedKVStore, PaginatedListResponse}; @@ -231,6 +558,34 @@ mod tests { use crate::io::test_utils::InMemoryStore; use crate::types::DynStoreWrapper; + const TEST_PRIMARY_NAMESPACE: &str = "datastore_test_primary"; + const TEST_SECONDARY_NAMESPACE: &str = "datastore_test_secondary"; + + fn new_data_store( + kv_store: Arc, cache_policy: P, objects: Vec, + ) -> DataStore, P> { + DataStore::new( + objects, + cache_policy, + TEST_PRIMARY_NAMESPACE.to_string(), + TEST_SECONDARY_NAMESPACE.to_string(), + kv_store, + Arc::new(TestLogger::new()), + ) + } + + fn keep_lru(capacity: usize) -> KeepLeastRecentlyUsed { + KeepLeastRecentlyUsed::new(NonZeroUsize::new(capacity).unwrap()) + } + + fn in_memory_store() -> Arc { + Arc::new(DynStoreWrapper(InMemoryStore::new())) + } + + fn test_id(id: u8) -> TestObjectId { + TestObjectId { id: [id; 4] } + } + #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] struct TestObjectId { id: [u8; 4], @@ -246,6 +601,9 @@ mod tests { struct TestObjectUpdate { id: TestObjectId, data: [u8; 3], + /// Only applied when `Some`, mirroring how a real update treats an absent field as "leave + /// whatever is stored alone". + extra: Option, } impl StorableObjectUpdate for TestObjectUpdate { fn id(&self) -> TestObjectId { @@ -257,6 +615,13 @@ mod tests { struct TestObject { id: TestObjectId, data: [u8; 3], + extra: Option, + } + + impl TestObject { + fn new(id: TestObjectId, data: [u8; 3]) -> Self { + Self { id, data, extra: None } + } } impl StorableObject for TestObject { @@ -268,22 +633,29 @@ mod tests { } fn update(&mut self, update: Self::Update) -> bool { + let mut updated = false; if self.data != update.data { self.data = update.data; - true - } else { - false + updated = true; } + if let Some(extra) = update.extra { + if self.extra != Some(extra) { + self.extra = Some(extra); + updated = true; + } + } + updated } fn to_update(&self) -> Self::Update { - Self::Update { id: self.id, data: self.data } + Self::Update { id: self.id, data: self.data, extra: self.extra } } } impl_writeable_tlv_based!(TestObject, { (0, id, required), (2, data, required), + (4, extra, option), }); struct FailingStore; @@ -329,8 +701,9 @@ mod tests { let logger = Arc::new(TestLogger::new()); DataStore::new( objects, - "datastore_test_primary".to_string(), - "datastore_test_secondary".to_string(), + KeepAllEntries, + TEST_PRIMARY_NAMESPACE.to_string(), + TEST_SECONDARY_NAMESPACE.to_string(), store, logger, ) @@ -401,13 +774,14 @@ mod tests { let logger = Arc::new(TestLogger::new()); let id = TestObjectId { id: [42u8; 4] }; - let old_object = TestObject { id, data: [23u8; 3] }; - let new_object = TestObject { id, data: [24u8; 3] }; + let old_object = TestObject::new(id, [23u8; 3]); + let new_object = TestObject::new(id, [24u8; 3]); let data_store: Arc>> = Arc::new(DataStore::new( vec![old_object], - "datastore_test_primary".to_string(), - "datastore_test_secondary".to_string(), + KeepAllEntries, + TEST_PRIMARY_NAMESPACE.to_string(), + TEST_SECONDARY_NAMESPACE.to_string(), store, logger, )); @@ -437,10 +811,11 @@ mod tests { async fn data_is_persisted() { let store: Arc = Arc::new(DynStoreWrapper(InMemoryStore::new())); let logger = Arc::new(TestLogger::new()); - let primary_namespace = "datastore_test_primary".to_string(); - let secondary_namespace = "datastore_test_secondary".to_string(); + let primary_namespace = TEST_PRIMARY_NAMESPACE.to_string(); + let secondary_namespace = TEST_SECONDARY_NAMESPACE.to_string(); let data_store: DataStore> = DataStore::new( Vec::new(), + KeepAllEntries, primary_namespace.clone(), secondary_namespace.clone(), Arc::clone(&store), @@ -458,7 +833,7 @@ mod tests { .is_err()); // Check we successfully store an object and return `false` - let object = TestObject { id, data: [23u8; 3] }; + let object = TestObject::new(id, [23u8; 3]); assert_eq!(Ok(false), data_store.insert(object.clone()).await); assert_eq!(Some(object), data_store.get(&id).await.unwrap()); assert!(KVStore::read(&*store, &primary_namespace, &secondary_namespace, &store_key) @@ -472,22 +847,22 @@ mod tests { assert_eq!(Some(override_object), data_store.get(&id).await.unwrap()); // Check update returns `Updated` - let update = TestObjectUpdate { id, data: [25u8; 3] }; + let update = TestObjectUpdate { id, data: [25u8; 3], extra: None }; assert_eq!(Ok(DataStoreUpdateResult::Updated), data_store.update(update).await); assert_eq!(data_store.get(&id).await.unwrap().unwrap().data, [25u8; 3]); // Check no-op update yields `Unchanged` - let update = TestObjectUpdate { id, data: [25u8; 3] }; + let update = TestObjectUpdate { id, data: [25u8; 3], extra: None }; assert_eq!(Ok(DataStoreUpdateResult::Unchanged), data_store.update(update).await); // Check bogus update yields `NotFound` let bogus_id = TestObjectId { id: [84u8; 4] }; - let update = TestObjectUpdate { id: bogus_id, data: [12u8; 3] }; + let update = TestObjectUpdate { id: bogus_id, data: [12u8; 3], extra: None }; assert_eq!(Ok(DataStoreUpdateResult::NotFound), data_store.update(update).await); // Check `insert_or_update` inserts unknown objects let iou_id = TestObjectId { id: [55u8; 4] }; - let iou_object = TestObject { id: iou_id, data: [34u8; 3] }; + let iou_object = TestObject::new(iou_id, [34u8; 3]); assert_eq!(Ok(true), data_store.insert_or_update(iou_object.clone()).await); // Check `insert_or_update` doesn't update the same object @@ -502,10 +877,10 @@ mod tests { #[tokio::test] async fn insert_or_update_does_not_mutate_memory_if_persist_fails() { let existing_id = TestObjectId { id: [42u8; 4] }; - let existing_object = TestObject { id: existing_id, data: [23u8; 3] }; + let existing_object = TestObject::new(existing_id, [23u8; 3]); let data_store = new_failing_data_store(vec![existing_object]); - let updated_object = TestObject { id: existing_id, data: [24u8; 3] }; + let updated_object = TestObject::new(existing_id, [24u8; 3]); assert_eq!( Err(Error::PersistenceFailed), data_store.insert_or_update(updated_object).await @@ -513,7 +888,7 @@ mod tests { assert_eq!(Some(existing_object), data_store.get(&existing_id).await.unwrap()); let new_id = TestObjectId { id: [55u8; 4] }; - let new_object = TestObject { id: new_id, data: [34u8; 3] }; + let new_object = TestObject::new(new_id, [34u8; 3]); assert_eq!(Err(Error::PersistenceFailed), data_store.insert_or_update(new_object).await); assert!(data_store.get(&new_id).await.unwrap().is_none()); } @@ -521,7 +896,7 @@ mod tests { #[tokio::test] async fn insert_does_not_mutate_memory_if_persist_fails() { let id = TestObjectId { id: [42u8; 4] }; - let object = TestObject { id, data: [23u8; 3] }; + let object = TestObject::new(id, [23u8; 3]); let data_store = new_failing_data_store(vec![]); assert_eq!(Err(Error::PersistenceFailed), data_store.insert(object).await); @@ -531,10 +906,10 @@ mod tests { #[tokio::test] async fn update_does_not_mutate_memory_if_persist_fails() { let id = TestObjectId { id: [42u8; 4] }; - let object = TestObject { id, data: [23u8; 3] }; + let object = TestObject::new(id, [23u8; 3]); let data_store = new_failing_data_store(vec![object]); - let update = TestObjectUpdate { id, data: [24u8; 3] }; + let update = TestObjectUpdate { id, data: [24u8; 3], extra: None }; assert_eq!(Err(Error::PersistenceFailed), data_store.update(update).await); assert_eq!(Some(object), data_store.get(&id).await.unwrap()); } @@ -542,10 +917,397 @@ mod tests { #[tokio::test] async fn remove_does_not_mutate_memory_if_persist_fails() { let id = TestObjectId { id: [42u8; 4] }; - let object = TestObject { id, data: [23u8; 3] }; + let object = TestObject::new(id, [23u8; 3]); let data_store = new_failing_data_store(vec![object]); assert_eq!(Err(Error::PersistenceFailed), data_store.remove(&id).await); assert_eq!(Some(object), data_store.get(&id).await.unwrap()); } + + /// A store that counts how often it is asked to read or list, so that tests can assert a + /// [`KeepAllEntries`] store never goes to the `KVStore` for a read. + struct CountingStore { + inner: InMemoryStore, + reads: Arc, + lists: Arc, + } + + impl KVStore for CountingStore { + fn read( + &self, primary_namespace: &str, secondary_namespace: &str, key: &str, + ) -> impl std::future::Future, io::Error>> + 'static + Send { + self.reads.fetch_add(1, Ordering::Relaxed); + self.inner.read(primary_namespace, secondary_namespace, key) + } + + fn write( + &self, primary_namespace: &str, secondary_namespace: &str, key: &str, buf: Vec, + ) -> impl std::future::Future> + 'static + Send { + self.inner.write(primary_namespace, secondary_namespace, key, buf) + } + + fn remove( + &self, primary_namespace: &str, secondary_namespace: &str, key: &str, lazy: bool, + ) -> impl std::future::Future> + 'static + Send { + self.inner.remove(primary_namespace, secondary_namespace, key, lazy) + } + + fn list( + &self, primary_namespace: &str, secondary_namespace: &str, + ) -> impl std::future::Future, io::Error>> + 'static + Send { + self.lists.fetch_add(1, Ordering::Relaxed); + self.inner.list(primary_namespace, secondary_namespace) + } + } + + impl PaginatedKVStore for CountingStore { + fn list_paginated( + &self, primary_namespace: &str, secondary_namespace: &str, + page_token: Option, + ) -> impl std::future::Future> + 'static + Send + { + self.lists.fetch_add(1, Ordering::Relaxed); + self.inner.list_paginated(primary_namespace, secondary_namespace, page_token) + } + } + + /// A store whose writes and removals can be made to fail on demand, while reads keep working. + /// + /// Note a store that fails *reads* would be useless for testing the write paths of a bounded + /// store, because it would already fail in the read-through that precedes the write. + struct WriteFailingStore { + inner: InMemoryStore, + fail_writes: Arc, + } + + impl KVStore for WriteFailingStore { + fn read( + &self, primary_namespace: &str, secondary_namespace: &str, key: &str, + ) -> impl std::future::Future, io::Error>> + 'static + Send { + self.inner.read(primary_namespace, secondary_namespace, key) + } + + fn write( + &self, primary_namespace: &str, secondary_namespace: &str, key: &str, buf: Vec, + ) -> impl std::future::Future> + 'static + Send { + let failing = self.fail_writes.load(Ordering::Relaxed); + let inner_fut = self.inner.write(primary_namespace, secondary_namespace, key, buf); + async move { + if failing { + return Err(io::Error::new(io::ErrorKind::Other, "write failed")); + } + inner_fut.await + } + } + + fn remove( + &self, primary_namespace: &str, secondary_namespace: &str, key: &str, lazy: bool, + ) -> impl std::future::Future> + 'static + Send { + let failing = self.fail_writes.load(Ordering::Relaxed); + let inner_fut = self.inner.remove(primary_namespace, secondary_namespace, key, lazy); + async move { + if failing { + return Err(io::Error::new(io::ErrorKind::Other, "remove failed")); + } + inner_fut.await + } + } + + fn list( + &self, primary_namespace: &str, secondary_namespace: &str, + ) -> impl std::future::Future, io::Error>> + 'static + Send { + self.inner.list(primary_namespace, secondary_namespace) + } + } + + impl PaginatedKVStore for WriteFailingStore { + fn list_paginated( + &self, primary_namespace: &str, secondary_namespace: &str, + page_token: Option, + ) -> impl std::future::Future> + 'static + Send + { + self.inner.list_paginated(primary_namespace, secondary_namespace, page_token) + } + } + + /// Returns a bounded store of the given capacity, together with a handle on the underlying + /// `KVStore`, and the ids of `num_objects` objects inserted through it. + /// + /// The objects are inserted in ascending id order, so with `capacity < num_objects` the + /// lowest ids have been evicted from memory by the time this returns, while remaining + /// available in the store. + async fn new_lru_store_with_objects( + capacity: usize, num_objects: u8, + ) -> ( + DataStore, KeepLeastRecentlyUsed>, + Arc, + Vec, + ) { + let kv_store = in_memory_store(); + let data_store = new_data_store(Arc::clone(&kv_store), keep_lru(capacity), Vec::new()); + let mut ids = Vec::new(); + for i in 0..num_objects { + let id = test_id(i); + data_store.insert(TestObject::new(id, [i; 3])).await.unwrap(); + ids.push(id); + } + (data_store, kv_store, ids) + } + + #[tokio::test] + async fn keep_all_never_reads_from_the_store() { + let reads = Arc::new(AtomicUsize::new(0)); + let lists = Arc::new(AtomicUsize::new(0)); + let kv_store: Arc = Arc::new(DynStoreWrapper(CountingStore { + inner: InMemoryStore::new(), + reads: Arc::clone(&reads), + lists: Arc::clone(&lists), + })); + let data_store = new_data_store(kv_store, KeepAllEntries, Vec::new()); + + let id = test_id(1); + let missing_id = test_id(2); + let object = TestObject::new(id, [23u8; 3]); + assert_eq!(Ok(false), data_store.insert(object).await); + + assert_eq!(Some(object), data_store.get(&id).await.unwrap()); + assert_eq!(None, data_store.get(&missing_id).await.unwrap()); + assert!(data_store.contains_key(&id).await.unwrap()); + assert!(!data_store.contains_key(&missing_id).await.unwrap()); + assert_eq!(1, data_store.list_filter(|_| true).await.len()); + let no_op = TestObjectUpdate { id, data: [23u8; 3], extra: None }; + assert_eq!(Ok(DataStoreUpdateResult::Unchanged), data_store.update(no_op).await); + + // The whole point of `KeepAllEntries` is that memory is the complete truth, so none of the + // above may go to the store. + assert_eq!(0, reads.load(Ordering::Relaxed)); + assert_eq!(0, lists.load(Ordering::Relaxed)); + } + + #[tokio::test] + async fn lru_evicts_least_recently_used() { + let (data_store, _kv_store, ids) = new_lru_store_with_objects(2, 3).await; + + assert_eq!(2, data_store.cached_len()); + assert!(!data_store.is_cached(&ids[0])); + assert!(data_store.is_cached(&ids[1])); + assert!(data_store.is_cached(&ids[2])); + } + + #[tokio::test] + async fn lru_get_marks_an_entry_as_recently_used() { + let (data_store, _kv_store, ids) = new_lru_store_with_objects(2, 2).await; + + assert!(data_store.get(&ids[0]).await.unwrap().is_some()); + + // With `ids[0]` freshly used, inserting a third object must evict `ids[1]` instead. + let new_id = test_id(9); + data_store.insert(TestObject::new(new_id, [9u8; 3])).await.unwrap(); + assert!(data_store.is_cached(&ids[0])); + assert!(!data_store.is_cached(&ids[1])); + assert!(data_store.is_cached(&new_id)); + } + + #[tokio::test] + async fn lru_get_reads_through_and_caches() { + let (data_store, _kv_store, ids) = new_lru_store_with_objects(1, 2).await; + let evicted_id = ids[0]; + assert!(!data_store.is_cached(&evicted_id)); + + let object = data_store.get(&evicted_id).await.unwrap(); + assert_eq!(Some(TestObject::new(evicted_id, [0u8; 3])), object); + assert!(data_store.is_cached(&evicted_id)); + assert_eq!(1, data_store.cached_len()); + } + + #[tokio::test] + async fn lru_contains_key_reads_through() { + let (data_store, _kv_store, ids) = new_lru_store_with_objects(1, 2).await; + let evicted_id = ids[0]; + assert!(!data_store.is_cached(&evicted_id)); + + assert!(data_store.contains_key(&evicted_id).await.unwrap()); + assert!(!data_store.contains_key(&test_id(99)).await.unwrap()); + // A mere existence probe must not displace the working set. + assert!(!data_store.is_cached(&evicted_id)); + } + + #[tokio::test] + async fn lru_update_reads_through_evicted_entry() { + let (data_store, _kv_store, ids) = new_lru_store_with_objects(1, 2).await; + let evicted_id = ids[0]; + assert!(!data_store.is_cached(&evicted_id)); + + // Without reading through, the evicted object would look absent and the update would be + // dropped on the floor. + let update = TestObjectUpdate { id: evicted_id, data: [25u8; 3], extra: None }; + assert_eq!(Ok(DataStoreUpdateResult::Updated), data_store.update(update).await); + assert_eq!([25u8; 3], data_store.get(&evicted_id).await.unwrap().unwrap().data); + } + + #[tokio::test] + async fn lru_insert_or_update_does_not_clobber_evicted_entry() { + let kv_store = in_memory_store(); + let data_store = new_data_store(kv_store, keep_lru(1), Vec::new()); + + let id = test_id(1); + let stored = TestObject { id, data: [23u8; 3], extra: Some(42) }; + data_store.insert(stored).await.unwrap(); + data_store.insert(TestObject::new(test_id(2), [24u8; 3])).await.unwrap(); + assert!(!data_store.is_cached(&id)); + + // The incoming object carries no `extra`, so merging must preserve the stored one. Without + // reading through, the evicted object would be overwritten wholesale and `extra` lost. + let incoming = TestObject { id, data: [25u8; 3], extra: None }; + assert_eq!(Ok(true), data_store.insert_or_update(incoming).await); + + let merged = data_store.get(&id).await.unwrap().unwrap(); + assert_eq!([25u8; 3], merged.data); + assert_eq!(Some(42), merged.extra); + } + + #[tokio::test] + async fn lru_insert_reports_replacement_of_evicted_entry() { + let (data_store, _kv_store, ids) = new_lru_store_with_objects(1, 2).await; + let evicted_id = ids[0]; + assert!(!data_store.is_cached(&evicted_id)); + + // Callers rely on this to detect ids they have already seen, so it must not be answered + // from the cache alone. + assert_eq!(Ok(true), data_store.insert(TestObject::new(evicted_id, [99u8; 3])).await); + assert_eq!(Ok(false), data_store.insert(TestObject::new(test_id(99), [99u8; 3])).await); + } + + #[tokio::test] + async fn lru_remove_removes_evicted_entry() { + let (data_store, kv_store, ids) = new_lru_store_with_objects(1, 2).await; + let evicted_id = ids[0]; + assert!(!data_store.is_cached(&evicted_id)); + + data_store.remove(&evicted_id).await.unwrap(); + + assert_eq!(None, data_store.get(&evicted_id).await.unwrap()); + let store_key = evicted_id.encode_to_hex_str(); + assert!(KVStore::read( + &*kv_store, + TEST_PRIMARY_NAMESPACE, + TEST_SECONDARY_NAMESPACE, + &store_key + ) + .await + .is_err()); + } + + #[tokio::test] + async fn lru_seeding_trims_to_capacity() { + let kv_store = in_memory_store(); + let seed_store = new_data_store(Arc::clone(&kv_store), KeepAllEntries, Vec::new()); + let mut objects = Vec::new(); + for i in 0..5u8 { + let object = TestObject::new(test_id(i), [i; 3]); + seed_store.insert(object).await.unwrap(); + objects.push(object); + } + + let data_store = new_data_store(kv_store, keep_lru(2), objects.clone()); + assert_eq!(2, data_store.cached_len()); + + // Everything the cache dropped is still reachable through the store. + for object in objects { + assert_eq!(Some(object), data_store.get(&object.id()).await.unwrap()); + } + } + + #[tokio::test] + async fn lru_reports_read_failures_rather_than_absence() { + let data_store = + new_data_store(Arc::new(DynStoreWrapper(FailingStore)), keep_lru(1), Vec::new()); + + let id = test_id(1); + assert_eq!(Err(Error::PersistenceFailed), data_store.get(&id).await); + assert_eq!(Err(Error::PersistenceFailed), data_store.contains_key(&id).await); + assert_eq!(Err(Error::PersistenceFailed), data_store.remove(&id).await); + assert_eq!( + Err(Error::PersistenceFailed), + data_store.insert_or_update(TestObject::new(id, [1u8; 3])).await + ); + let update = TestObjectUpdate { id, data: [1u8; 3], extra: None }; + assert_eq!(Err(Error::PersistenceFailed), data_store.update(update).await); + + // By contrast, a store that simply doesn't hold the key must report exactly that. + let working_store = new_data_store(in_memory_store(), keep_lru(1), Vec::new()); + assert_eq!(Ok(None), working_store.get(&id).await); + assert_eq!(Ok(false), working_store.contains_key(&id).await); + assert_eq!(Ok(()), working_store.remove(&id).await); + let update = TestObjectUpdate { id, data: [1u8; 3], extra: None }; + assert_eq!(Ok(DataStoreUpdateResult::NotFound), working_store.update(update).await); + } + + #[tokio::test] + async fn lru_does_not_mutate_memory_if_persist_fails() { + let fail_writes = Arc::new(AtomicBool::new(false)); + let kv_store: Arc = Arc::new(DynStoreWrapper(WriteFailingStore { + inner: InMemoryStore::new(), + fail_writes: Arc::clone(&fail_writes), + })); + let data_store = new_data_store(kv_store, keep_lru(1), Vec::new()); + + let id = test_id(1); + let stored = TestObject::new(id, [23u8; 3]); + data_store.insert(stored).await.unwrap(); + // Evict it, so every operation below has to read through first. + data_store.insert(TestObject::new(test_id(2), [24u8; 3])).await.unwrap(); + assert!(!data_store.is_cached(&id)); + + fail_writes.store(true, Ordering::Relaxed); + + assert_eq!( + Err(Error::PersistenceFailed), + data_store.insert_or_update(TestObject::new(id, [25u8; 3])).await + ); + let update = TestObjectUpdate { id, data: [26u8; 3], extra: None }; + assert_eq!(Err(Error::PersistenceFailed), data_store.update(update).await); + assert_eq!( + Err(Error::PersistenceFailed), + data_store.insert(TestObject::new(id, [27u8; 3])).await + ); + assert_eq!(Err(Error::PersistenceFailed), data_store.remove(&id).await); + + fail_writes.store(false, Ordering::Relaxed); + assert_eq!(Some(stored), data_store.get(&id).await.unwrap()); + } + + #[test] + fn lru_cache_keeps_its_indices_in_sync() { + let mut lru: LruCache = LruCache::new(NonZeroUsize::new(2).unwrap()); + let first = test_id(1); + let second = test_id(2); + let third = test_id(3); + + lru.insert(first, TestObject::new(first, [1u8; 3])); + lru.insert(second, TestObject::new(second, [2u8; 3])); + assert_eq!(2, lru.entries.len()); + assert_eq!(2, lru.recency.len()); + + // Re-inserting a known id must replace rather than grow. + lru.insert(first, TestObject::new(first, [11u8; 3])); + assert_eq!(2, lru.entries.len()); + assert_eq!(2, lru.recency.len()); + + // `first` was just written, so `second` is the one to go. + lru.insert(third, TestObject::new(third, [3u8; 3])); + assert_eq!(2, lru.entries.len()); + assert_eq!(2, lru.recency.len()); + assert!(lru.entries.contains_key(&first)); + assert!(!lru.entries.contains_key(&second)); + assert!(lru.entries.contains_key(&third)); + + lru.remove(&first); + assert_eq!(1, lru.entries.len()); + assert_eq!(1, lru.recency.len()); + // Removing an unknown id is a no-op. + lru.remove(&second); + assert_eq!(1, lru.entries.len()); + assert_eq!(1, lru.recency.len()); + } } diff --git a/src/types.rs b/src/types.rs index 22429d980..742d69be5 100644 --- a/src/types.rs +++ b/src/types.rs @@ -45,7 +45,7 @@ use lightning_types::features::ChannelTypeFeatures; use crate::chain::bitcoind::UtxoSourceClient; use crate::chain::ChainSource; use crate::config::{AnchorChannelsConfig, ChannelConfig}; -use crate::data_store::DataStore; +use crate::data_store::{DataStore, KeepAllEntries}; use crate::fee_estimator::OnchainFeeEstimator; use crate::ffi::maybe_wrap; use crate::logger::Logger; @@ -375,7 +375,7 @@ pub(crate) type BumpTransactionEventHandler = Arc, >; -pub(crate) type PaymentStore = DataStore>; +pub(crate) type PaymentStore = DataStore, KeepAllEntries>; /// A local, potentially user-provided, identifier of a channel. /// @@ -757,4 +757,4 @@ impl From<&(u64, Vec)> for CustomTlvRecord { } } -pub(crate) type PendingPaymentStore = DataStore>; +pub(crate) type PendingPaymentStore = DataStore, KeepAllEntries>; From c59e38690bb833a387035b3498d60c63a9bcc90e Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 6 Aug 2026 13:07:55 +0200 Subject: [PATCH 5/9] Add an `InvalidPageToken` error variant Paginated listing hands the storage backend a token supplied by the caller, which the backend rejects if it is malformed. Reporting that as `PersistenceFailed` would be misleading, as nothing failed to persist, and would give a bindings user who round-trips a token through their own storage no way to tell a bad token from a broken store. Co-Authored-By: HAL 9000 --- bindings/ldk_node.udl | 1 + src/error.rs | 3 +++ 2 files changed, 4 insertions(+) diff --git a/bindings/ldk_node.udl b/bindings/ldk_node.udl index 6b09cb9c5..c8230199a 100644 --- a/bindings/ldk_node.udl +++ b/bindings/ldk_node.udl @@ -236,6 +236,7 @@ enum NodeError { "InvalidDateTime", "InvalidFeeRate", "InvalidScriptPubKey", + "InvalidPageToken", "DuplicatePayment", "UnsupportedCurrency", "InsufficientFunds", diff --git a/src/error.rs b/src/error.rs index 8546af0dd..71ba9467e 100644 --- a/src/error.rs +++ b/src/error.rs @@ -115,6 +115,8 @@ pub enum Error { InvalidFeeRate, /// The given script public key is invalid. InvalidScriptPubKey, + /// The given page token is invalid. + InvalidPageToken, /// A payment with the given hash has already been initiated. DuplicatePayment, /// The provided offer was denonminated in an unsupported currency. @@ -199,6 +201,7 @@ impl fmt::Display for Error { Self::InvalidDateTime => write!(f, "The given date time is invalid."), Self::InvalidFeeRate => write!(f, "The given fee rate is invalid."), Self::InvalidScriptPubKey => write!(f, "The given script pubkey is invalid."), + Self::InvalidPageToken => write!(f, "The given page token is invalid."), Self::DuplicatePayment => { write!(f, "A payment with the given hash has already been initiated.") }, From d1a51a9d4d26489bfe4a2b148d46db510c94344b Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 6 Aug 2026 13:12:53 +0200 Subject: [PATCH 6/9] tests: Introduce a payment listing helper Tests reach for the payment history in a great many places, all of them spelling out how it is retrieved. Route them through a helper trait instead, so that they state what they want and the retrieval lives in one place. Pure refactor: the helper currently just forwards to the existing listing API. Co-Authored-By: HAL 9000 --- tests/common/mod.rs | 79 +++++++++++++++++++--------- tests/integration_tests_migration.rs | 6 +-- tests/integration_tests_rust.rs | 61 +++++++++++---------- tests/reorg_test.rs | 4 +- 4 files changed, 88 insertions(+), 62 deletions(-) diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 9689b3296..efa6db988 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -43,7 +43,9 @@ use ldk_node::config::{ }; use ldk_node::entropy::{generate_entropy_mnemonic, NodeEntropy}; use ldk_node::io::sqlite_store::SqliteStore; -use ldk_node::payment::{PaymentDirection, PaymentKind, PaymentStatus, TransactionType}; +use ldk_node::payment::{ + PaymentDetails, PaymentDirection, PaymentKind, PaymentStatus, TransactionType, +}; use ldk_node::probing::ProbingConfig; use ldk_node::{ Builder, ChannelShutdownState, CustomTlvRecord, Event, LightningBalance, Node, NodeError, @@ -410,8 +412,37 @@ pub(crate) type TestNode = Arc; #[cfg(not(feature = "uniffi"))] pub(crate) type TestNode = Node; +/// Payment listing helpers for tests. +/// +/// These exist so that tests state *what* they want from the payment history rather than how it is +/// retrieved, and so that the retrieval can change in one place. +pub(crate) trait NodePaymentExt { + /// Returns all known payments, from most recently created to least recently created. + fn list_all_payments(&self) -> Vec; + + /// Returns all known payments matching `f`, from most recently created to least recently + /// created. + fn list_payments_matching bool>( + &self, f: F, + ) -> Vec; +} + +// Implemented on `Node` rather than on `TestNode` so that it applies both when `TestNode` is a +// `Node` and when it is an `Arc`. +impl NodePaymentExt for Node { + fn list_all_payments(&self) -> Vec { + self.list_payments() + } + + fn list_payments_matching bool>( + &self, f: F, + ) -> Vec { + self.list_payments_with_filter(f) + } +} + fn has_onchain_tx_type bool>(node: &TestNode, predicate: F) -> bool { - node.list_payments().into_iter().any(|payment| { + node.list_all_payments().into_iter().any(|payment| { matches!( payment.kind, PaymentKind::Onchain { tx_type: Some(ref tx_type), .. } if predicate(tx_type) @@ -429,7 +460,7 @@ fn assert_any_node_has_onchain_tx_type bool + Copy>( let observed: Vec = nodes .iter() .flat_map(|(name, node)| { - node.list_payments().into_iter().filter_map(move |payment| match payment.kind { + node.list_all_payments().into_iter().filter_map(move |payment| match payment.kind { PaymentKind::Onchain { tx_type, .. } => Some(format!("{}:{:?}", name, tx_type)), _ => None, }) @@ -448,7 +479,7 @@ fn assert_all_nodes_have_onchain_tx_type bool + Copy> let observed: Vec = nodes .iter() .flat_map(|(name, node)| { - node.list_payments().into_iter().filter_map(move |payment| match payment.kind { + node.list_all_payments().into_iter().filter_map(move |payment| match payment.kind { PaymentKind::Onchain { tx_type, .. } => Some(format!("{}:{:?}", name, tx_type)), _ => None, }) @@ -1090,28 +1121,28 @@ pub(crate) async fn do_channel_full_cycle( // Check we saw the node funding transactions. assert_eq!( node_a - .list_payments_with_filter(|p| p.direction == PaymentDirection::Inbound + .list_payments_matching(|p| p.direction == PaymentDirection::Inbound && matches!(p.kind, PaymentKind::Onchain { .. })) .len(), 1 ); assert_eq!( node_a - .list_payments_with_filter(|p| p.direction == PaymentDirection::Outbound + .list_payments_matching(|p| p.direction == PaymentDirection::Outbound && matches!(p.kind, PaymentKind::Onchain { .. })) .len(), 0 ); assert_eq!( node_b - .list_payments_with_filter(|p| p.direction == PaymentDirection::Inbound + .list_payments_matching(|p| p.direction == PaymentDirection::Inbound && matches!(p.kind, PaymentKind::Onchain { .. })) .len(), 1 ); assert_eq!( node_b - .list_payments_with_filter(|p| p.direction == PaymentDirection::Outbound + .list_payments_matching(|p| p.direction == PaymentDirection::Outbound && matches!(p.kind, PaymentKind::Onchain { .. })) .len(), 0 @@ -1164,7 +1195,7 @@ pub(crate) async fn do_channel_full_cycle( // Check we now see the channel funding transaction as outbound. assert_eq!( node_a - .list_payments_with_filter(|p| p.direction == PaymentDirection::Outbound + .list_payments_matching(|p| p.direction == PaymentDirection::Outbound && matches!(p.kind, PaymentKind::Onchain { .. })) .len(), 1 @@ -1242,24 +1273,24 @@ pub(crate) async fn do_channel_full_cycle( let payment_id = node_a.bolt11_payment().send(&invoice, None).unwrap(); assert_eq!(node_a.bolt11_payment().send(&invoice, None), Err(NodeError::DuplicatePayment)); - assert!(!node_a.list_payments_with_filter(|p| p.id == payment_id).is_empty()); + assert!(!node_a.list_payments_matching(|p| p.id == payment_id).is_empty()); - let outbound_payments_a = node_a.list_payments_with_filter(|p| { + let outbound_payments_a = node_a.list_payments_matching(|p| { p.direction == PaymentDirection::Outbound && matches!(p.kind, PaymentKind::Bolt11 { .. }) }); assert_eq!(outbound_payments_a.len(), 1); - let inbound_payments_a = node_a.list_payments_with_filter(|p| { + let inbound_payments_a = node_a.list_payments_matching(|p| { p.direction == PaymentDirection::Inbound && matches!(p.kind, PaymentKind::Bolt11 { .. }) }); assert_eq!(inbound_payments_a.len(), 0); - let outbound_payments_b = node_b.list_payments_with_filter(|p| { + let outbound_payments_b = node_b.list_payments_matching(|p| { p.direction == PaymentDirection::Outbound && matches!(p.kind, PaymentKind::Bolt11 { .. }) }); assert_eq!(outbound_payments_b.len(), 0); - let inbound_payments_b = node_b.list_payments_with_filter(|p| { + let inbound_payments_b = node_b.list_payments_matching(|p| { p.direction == PaymentDirection::Inbound && matches!(p.kind, PaymentKind::Bolt11 { .. }) }); assert_eq!(inbound_payments_b.len(), 1); @@ -1590,23 +1621,19 @@ pub(crate) async fn do_channel_full_cycle( PaymentKind::Spontaneous { .. } )); assert_eq!( - node_a.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Bolt11 { .. })).len(), + node_a.list_payments_matching(|p| matches!(p.kind, PaymentKind::Bolt11 { .. })).len(), 5 ); assert_eq!( - node_b.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Bolt11 { .. })).len(), + node_b.list_payments_matching(|p| matches!(p.kind, PaymentKind::Bolt11 { .. })).len(), 6 ); assert_eq!( - node_a - .list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Spontaneous { .. })) - .len(), + node_a.list_payments_matching(|p| matches!(p.kind, PaymentKind::Spontaneous { .. })).len(), 1 ); assert_eq!( - node_b - .list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Spontaneous { .. })) - .len(), + node_b.list_payments_matching(|p| matches!(p.kind, PaymentKind::Spontaneous { .. })).len(), 1 ); @@ -1631,7 +1658,7 @@ pub(crate) async fn do_channel_full_cycle( assert_eq!( node_a - .list_payments_with_filter(|p| p.direction == PaymentDirection::Inbound + .list_payments_matching(|p| p.direction == PaymentDirection::Inbound && matches!(p.kind, PaymentKind::Onchain { .. })) .len(), 2 @@ -1652,7 +1679,7 @@ pub(crate) async fn do_channel_full_cycle( assert_eq!( node_a - .list_payments_with_filter(|p| p.direction == PaymentDirection::Outbound + .list_payments_matching(|p| p.direction == PaymentDirection::Outbound && matches!(p.kind, PaymentKind::Onchain { .. })) .len(), 2 @@ -1829,13 +1856,13 @@ pub(crate) async fn do_channel_full_cycle( // Now we should have seen the channel closing transaction on-chain. let node_a_inbound_onchain_count = node_a - .list_payments_with_filter(|p| { + .list_payments_matching(|p| { p.direction == PaymentDirection::Inbound && matches!(p.kind, PaymentKind::Onchain { .. }) }) .len(); let node_b_inbound_onchain_count = node_b - .list_payments_with_filter(|p| { + .list_payments_matching(|p| { p.direction == PaymentDirection::Inbound && matches!(p.kind, PaymentKind::Onchain { .. }) }) diff --git a/tests/integration_tests_migration.rs b/tests/integration_tests_migration.rs index 7e5767dca..4fcb29e47 100644 --- a/tests/integration_tests_migration.rs +++ b/tests/integration_tests_migration.rs @@ -15,7 +15,7 @@ use std::path::PathBuf; use common::{ drop_table, expect_channel_ready_event, expect_payment_received_event, - expect_payment_successful_event, test_connection_string, + expect_payment_successful_event, test_connection_string, NodePaymentExt, }; use ldk_node::entropy::NodeEntropy; use ldk_node::io::postgres_store::PostgresStore; @@ -224,7 +224,7 @@ async fn migrate_node_across_all_backends() { // Capture the state we expect to survive every migration. let expected_balance_sats = node.list_balances().total_onchain_balance_sats; let expected_ln_balance_sats = node.list_balances().total_lightning_balance_sats; - let mut expected_payments = node.list_payments(); + let mut expected_payments = node.list_all_payments(); expected_payments.sort_by_key(|p| p.id.0); assert!(expected_payments.len() >= 4); @@ -249,7 +249,7 @@ async fn migrate_node_across_all_backends() { assert_eq!(node.list_balances().total_onchain_balance_sats, expected_balance_sats); assert_eq!(node.list_balances().total_lightning_balance_sats, expected_ln_balance_sats); assert_eq!(node.list_channels().len(), 1); - let mut migrated_payments = node.list_payments(); + let mut migrated_payments = node.list_all_payments(); migrated_payments.sort_by_key(|p| p.id.0); assert_eq!(migrated_payments, expected_payments); diff --git a/tests/integration_tests_rust.rs b/tests/integration_tests_rust.rs index e332b4af7..4e6564be8 100644 --- a/tests/integration_tests_rust.rs +++ b/tests/integration_tests_rust.rs @@ -28,7 +28,7 @@ use common::{ open_channel_with_all, premine_and_distribute_funds, premine_blocks, prepare_rbf, random_chain_source, random_config, setup_bitcoind_and_electrsd, setup_builder, setup_node, setup_two_nodes, splice_in_with_all, wait_for_block, wait_for_tx, InMemoryStore, - TestChainSource, TestConfig, TestStoreType, TestSyncStore, + NodePaymentExt, TestChainSource, TestConfig, TestStoreType, TestSyncStore, }; use electrsd::corepc_node::{self, Node as BitcoinD}; use electrsd::ElectrsD; @@ -58,7 +58,7 @@ use serde_json::json; async fn wait_for_classified_funding_payment(node: &Node, funding_txid: Txid) { let poll = async { loop { - let classified = node.list_payments().into_iter().any(|p| { + let classified = node.list_all_payments().into_iter().any(|p| { matches!( p.kind, PaymentKind::Onchain { txid, tx_type: Some(_), .. } if txid == funding_txid @@ -590,16 +590,15 @@ async fn split_underpaid_bolt11_payment() { expect_payment_successful_event!(node_b, Some(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_matching(|p| p.id == receiver_payment_id.unwrap()); assert_eq!(receiver_payments.len(), 1); assert_eq!(receiver_payments.first().unwrap().amount_msat, Some(amount_msat)); - let node_a_payments = node_a.list_payments_with_filter(|p| p.id == payment_id_a); + let node_a_payments = node_a.list_payments_matching(|p| p.id == payment_id_a); assert_eq!(node_a_payments.len(), 1); assert_eq!(node_a_payments.first().unwrap().amount_msat, Some(half_amount_msat)); - let node_b_payments = node_b.list_payments_with_filter(|p| p.id == payment_id_b); + let node_b_payments = node_b.list_payments_matching(|p| p.id == payment_id_b); assert_eq!(node_b_payments.len(), 1); assert_eq!(node_b_payments.first().unwrap().amount_msat, Some(half_amount_msat)); } @@ -723,8 +722,8 @@ async fn onchain_send_receive() { assert_eq!(node_a.list_balances().spendable_onchain_balance_sats, premine_amount_sat); assert_eq!(node_b.list_balances().spendable_onchain_balance_sats, premine_amount_sat); - let node_a_payments = node_a.list_payments(); - let node_b_payments = node_b.list_payments(); + let node_a_payments = node_a.list_all_payments(); + let node_b_payments = node_b.list_all_payments(); for payments in [&node_a_payments, &node_b_payments] { assert_eq!(payments.len(), 1) } @@ -752,10 +751,10 @@ async fn onchain_send_receive() { expect_channel_ready_event!(node_b, node_a.node_id()); let node_a_payments = - node_a.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Onchain { .. })); + node_a.list_payments_matching(|p| matches!(p.kind, PaymentKind::Onchain { .. })); assert_eq!(node_a_payments.len(), 1); let node_b_payments = - node_b.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Onchain { .. })); + node_b.list_payments_matching(|p| matches!(p.kind, PaymentKind::Onchain { .. })); assert_eq!(node_b_payments.len(), 2); let onchain_fee_buffer_sat = 1000; @@ -827,10 +826,10 @@ async fn onchain_send_receive() { assert!(node_b.list_balances().spendable_onchain_balance_sats < expected_node_b_balance_upper); let node_a_payments = - node_a.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Onchain { .. })); + node_a.list_payments_matching(|p| matches!(p.kind, PaymentKind::Onchain { .. })); assert_eq!(node_a_payments.len(), 2); let node_b_payments = - node_b.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Onchain { .. })); + node_b.list_payments_matching(|p| matches!(p.kind, PaymentKind::Onchain { .. })); assert_eq!(node_b_payments.len(), 3); let payment_a = node_a.payment(&payment_id).unwrap().unwrap(); @@ -870,10 +869,10 @@ async fn onchain_send_receive() { assert!(node_b.list_balances().spendable_onchain_balance_sats < expected_node_b_balance_upper); let node_a_payments = - node_a.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Onchain { .. })); + node_a.list_payments_matching(|p| matches!(p.kind, PaymentKind::Onchain { .. })); assert_eq!(node_a_payments.len(), 3); let node_b_payments = - node_b.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Onchain { .. })); + node_b.list_payments_matching(|p| matches!(p.kind, PaymentKind::Onchain { .. })); assert_eq!(node_b_payments.len(), 4); let addr_b = node_b.onchain_payment().new_address().unwrap(); @@ -894,10 +893,10 @@ async fn onchain_send_receive() { assert!(node_b.list_balances().spendable_onchain_balance_sats < expected_node_b_balance_upper); let node_a_payments = - node_a.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Onchain { .. })); + node_a.list_payments_matching(|p| matches!(p.kind, PaymentKind::Onchain { .. })); assert_eq!(node_a_payments.len(), 4); let node_b_payments = - node_b.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Onchain { .. })); + node_b.list_payments_matching(|p| matches!(p.kind, PaymentKind::Onchain { .. })); assert_eq!(node_b_payments.len(), 5); } @@ -1777,7 +1776,7 @@ async fn splice_channel() { // them to the channel balance since there may not be a change output. let expected_splice_in_lightning_balance_sat = 4_000_002; - let payments = node_b.list_payments(); + let payments = node_b.list_all_payments(); let payment = payments.into_iter().find(|p| p.id == PaymentId(txo.txid.to_byte_array())).unwrap(); assert_eq!(payment.fee_paid_msat, Some(expected_splice_in_fee_sat * 1_000)); @@ -1829,7 +1828,7 @@ async fn splice_channel() { let expected_splice_out_fee_sat = 183; - let payments = node_a.list_payments(); + let payments = node_a.list_all_payments(); let payment = payments.into_iter().find(|p| p.id == PaymentId(txo.txid.to_byte_array())).unwrap(); assert_eq!(payment.fee_paid_msat, Some(expected_splice_out_fee_sat * 1_000)); @@ -1997,7 +1996,7 @@ async fn run_rbf_splice_channel_test(confirm_original: bool) { } assert_eq!(payment.status, PaymentStatus::Pending); // Only one Onchain Pending payment for this splice attempt (not one per candidate). - let splice_payments = node_b.list_payments_with_filter(|p| { + let splice_payments = node_b.list_payments_matching(|p| { p.direction == PaymentDirection::Outbound && matches!(p.kind, PaymentKind::Onchain { .. }) && p.status == PaymentStatus::Pending @@ -2322,7 +2321,7 @@ async fn simple_bolt12_send_receive() { ref e => panic!("{} got unexpected event!: {:?}", "node_a", e), } let node_a_payments = - node_a.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Bolt12Offer { .. })); + node_a.list_payments_matching(|p| matches!(p.kind, PaymentKind::Bolt12Offer { .. })); assert_eq!(node_a_payments.len(), 1); match node_a_payments.first().unwrap().kind { PaymentKind::Bolt12Offer { @@ -2349,7 +2348,7 @@ async fn simple_bolt12_send_receive() { expect_payment_received_event!(node_b, expected_amount_msat); let node_b_payments = - node_b.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Bolt12Offer { .. })); + node_b.list_payments_matching(|p| matches!(p.kind, PaymentKind::Bolt12Offer { .. })); assert_eq!(node_b_payments.len(), 1); match node_b_payments.first().unwrap().kind { PaymentKind::Bolt12Offer { hash, preimage, secret, offer_id, .. } => { @@ -2387,7 +2386,7 @@ async fn simple_bolt12_send_receive() { .unwrap(); expect_payment_successful_event!(node_a, Some(payment_id), None); - let node_a_payments = node_a.list_payments_with_filter(|p| { + let node_a_payments = node_a.list_payments_matching(|p| { matches!(p.kind, PaymentKind::Bolt12Offer { .. }) && p.id == payment_id }); assert_eq!(node_a_payments.len(), 1); @@ -2417,7 +2416,7 @@ async fn simple_bolt12_send_receive() { expect_payment_received_event!(node_b, expected_amount_msat); let node_b_payment_id = PaymentId(payment_hash.0); - let node_b_payments = node_b.list_payments_with_filter(|p| { + let node_b_payments = node_b.list_payments_matching(|p| { matches!(p.kind, PaymentKind::Bolt12Offer { .. }) && p.id == node_b_payment_id }); assert_eq!(node_b_payments.len(), 1); @@ -2452,7 +2451,7 @@ async fn simple_bolt12_send_receive() { expect_payment_received_event!(node_a, overpaid_amount); let node_b_payment_id = node_b - .list_payments_with_filter(|p| { + .list_payments_matching(|p| { matches!(p.kind, PaymentKind::Bolt12Refund { .. }) && p.amount_msat == Some(overpaid_amount) }) @@ -2461,7 +2460,7 @@ async fn simple_bolt12_send_receive() { .id; expect_payment_successful_event!(node_b, Some(node_b_payment_id), None); - let node_b_payments = node_b.list_payments_with_filter(|p| { + let node_b_payments = node_b.list_payments_matching(|p| { matches!(p.kind, PaymentKind::Bolt12Refund { .. }) && p.id == node_b_payment_id }); assert_eq!(node_b_payments.len(), 1); @@ -2487,7 +2486,7 @@ async fn simple_bolt12_send_receive() { assert_eq!(node_b_payments.first().unwrap().amount_msat, Some(overpaid_amount)); let node_a_payment_id = PaymentId(invoice.payment_hash().0); - let node_a_payments = node_a.list_payments_with_filter(|p| { + let node_a_payments = node_a.list_payments_matching(|p| { matches!(p.kind, PaymentKind::Bolt12Refund { .. }) && p.id == node_a_payment_id }); assert_eq!(node_a_payments.len(), 1); @@ -3198,7 +3197,7 @@ async fn spontaneous_send_with_custom_preimage() { // check payment status and verify stored preimage expect_payment_successful_event!(node_a, Some(payment_id), None); let details: PaymentDetails = - node_a.list_payments_with_filter(|p| p.id == payment_id).first().unwrap().clone(); + node_a.list_payments_matching(|p| p.id == payment_id).first().unwrap().clone(); assert_eq!(details.status, PaymentStatus::Succeeded); if let PaymentKind::Spontaneous { preimage: Some(pi), .. } = details.kind { assert_eq!(pi.0, custom_bytes); @@ -3208,7 +3207,7 @@ async fn spontaneous_send_with_custom_preimage() { // Verify receiver side (node_b) expect_payment_received_event!(node_b, amount_msat); - let receiver_payments: Vec = node_b.list_payments_with_filter(|p| { + let receiver_payments: Vec = node_b.list_payments_matching(|p| { p.direction == PaymentDirection::Inbound && matches!(p.kind, PaymentKind::Spontaneous { .. }) }); @@ -3613,7 +3612,7 @@ async fn payment_persistence_after_restart() { println!("All {} payments completed successfully", num_payments); // Verify node_a has 200 outbound Bolt11 payments before shutdown - let outbound_payments_before = node_a.list_payments_with_filter(|p| { + let outbound_payments_before = node_a.list_payments_matching(|p| { p.direction == PaymentDirection::Outbound && matches!(p.kind, PaymentKind::Bolt11 { .. }) }); @@ -3630,7 +3629,7 @@ async fn payment_persistence_after_restart() { let restarted_node_a = setup_node(&chain_source, config_a); // Assert all 200 payments are still in the store - let outbound_payments_after = restarted_node_a.list_payments_with_filter(|p| { + let outbound_payments_after = restarted_node_a.list_payments_matching(|p| { p.direction == PaymentDirection::Outbound && matches!(p.kind, PaymentKind::Bolt11 { .. }) }); assert_eq!( @@ -3962,7 +3961,7 @@ async fn onchain_fee_bump_rbf() { } // Verify node A received the funds correctly - let node_a_received_payment = node_a.list_payments_with_filter(|p| { + let node_a_received_payment = node_a.list_payments_matching(|p| { p.id == payment_id && matches!(p.kind, PaymentKind::Onchain { .. }) }); diff --git a/tests/reorg_test.rs b/tests/reorg_test.rs index 132d9de96..23c19c160 100644 --- a/tests/reorg_test.rs +++ b/tests/reorg_test.rs @@ -10,7 +10,7 @@ use proptest::proptest; use crate::common::{ expect_event, exponential_backoff_poll, generate_blocks_and_wait, invalidate_blocks, open_channel, premine_and_distribute_funds, random_chain_source, random_config, - setup_bitcoind_and_electrsd, setup_node, wait_for_outpoint_spend, wait_for_tx, + setup_bitcoind_and_electrsd, setup_node, wait_for_outpoint_spend, wait_for_tx, NodePaymentExt, }; async fn wait_for_pending_sweep_balance( @@ -119,7 +119,7 @@ proptest! { for (i, node) in nodes.iter().enumerate() { assert_eq!( node - .list_payments_with_filter(|p| p.direction == PaymentDirection::Outbound + .list_payments_matching(|p| p.direction == PaymentDirection::Outbound && matches!(p.kind, PaymentKind::Onchain { .. })) .len(), 1 From 98dd9df8e5eced521ab5d65e3d3c763761b64cce Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 6 Aug 2026 13:21:31 +0200 Subject: [PATCH 7/9] Paginate `Node::list_payments` Returning the entire payment history in one call requires holding it all in memory, which is exactly what a node with a long history cannot afford, and it gives an app no way to show recent payments without loading every old one. Return one page at a time instead, ordered from most recently created to least recently created, and drop the unpaginated variants. The ordering and the page tokens are the storage backend's own: we hand its opaque token straight back to it and never derive an order of ours. That keeps tokens valid across restarts and independent of what we happen to hold in memory, and it means a store that caches only a subset of its namespace can still list all of it, reading back whatever it does not hold. Listing deliberately neither waits for in-flight writes across its reads nor disturbs the cache. Blocking every writer for the duration of a round trip to a remote backend because something asked for a page would be a poor trade, and letting a sweep of the whole namespace count as use would evict the very entries a node works with most. Co-Authored-By: HAL 9000 --- CHANGELOG.md | 11 + .../lightningdevkit/ldknode/LibraryTest.kt | 8 +- bindings/ldk_node.udl | 7 +- src/data_store.rs | 475 +++++++++++++++++- src/ffi/types.rs | 48 ++ src/hex_utils.rs | 1 - src/io/in_memory_store.rs | 2 +- src/io/test_utils.rs | 2 +- src/lib.rs | 55 +- src/payment/mod.rs | 4 +- src/payment/store.rs | 37 ++ tests/common/mod.rs | 29 +- 12 files changed, 649 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 07078cfa6..61133beca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,10 @@ # Pending ## Compatibility Notes +- Migrating between storage backends does not preserve the relative creation order of + pre-existing payments, as the generic KV store migration copies entries in an unspecified + order. Expect the order in which `Node::list_payments` returns pre-existing payments to + change once after such a migration. Payment contents and completeness are unaffected. - Pending JIT-channel payments created before upgrading may fail after upgrade because the prior LSPS2 fee-limit state stored in `PaymentKind::Bolt11Jit` is not migrated. - Upgrading from LDK Node v0.1 is no longer supported if the event queue still contains @@ -9,6 +13,13 @@ `v0.1.0-alpha.0` before upgrading LDK Node. ## Feature and API updates +- `Node::list_payments` is now paginated: it takes an optional `PageToken` and returns a + `PaymentDetailsPage` holding one page of payments, ordered from most recently created to + least recently created, plus the token for the next page. Ordering and page tokens come + from the configured storage backend, so a token stays valid across restarts. This replaces + the previous unpaginated `Node::list_payments`, and `Node::list_payments_with_filter` has + been removed; filter the returned pages instead. +- `Node::payment` now returns a `Result`, as retrieving a payment may fail. - The Bitcoin Core RPC and REST chain-source builder methods now accept an optional `wallet_rescan_from_height` argument. Passing a height lets fresh wallets rescan from a known birthday block instead of checkpointing at the current tip, which is useful when restoring a diff --git a/bindings/kotlin/ldk-node-jvm/lib/src/test/kotlin/org/lightningdevkit/ldknode/LibraryTest.kt b/bindings/kotlin/ldk-node-jvm/lib/src/test/kotlin/org/lightningdevkit/ldknode/LibraryTest.kt index 006878a4c..2770e0287 100644 --- a/bindings/kotlin/ldk-node-jvm/lib/src/test/kotlin/org/lightningdevkit/ldknode/LibraryTest.kt +++ b/bindings/kotlin/ldk-node-jvm/lib/src/test/kotlin/org/lightningdevkit/ldknode/LibraryTest.kt @@ -301,8 +301,12 @@ class LibraryTest { assert(paymentReceivedEvent is Event.PaymentReceived) node2.eventHandled() - assert(node1.listPayments().size == 3) - assert(node2.listPayments().size == 2) + assert(node1.listPayments(null).payments.size == 3) + assert(node2.listPayments(null).payments.size == 2) + + // A page token has to survive a round trip through a string, so that an app can persist + // one and resume paginating after a restart. + assert(PageToken("some-page-token").toString() == "some-page-token") node2.closeChannel(userChannelId, nodeId1) diff --git a/bindings/ldk_node.udl b/bindings/ldk_node.udl index c8230199a..9465ba07b 100644 --- a/bindings/ldk_node.udl +++ b/bindings/ldk_node.udl @@ -152,7 +152,8 @@ interface Node { [Throws=NodeError] void remove_payment([ByRef]PaymentId payment_id); BalanceDetails list_balances(); - sequence list_payments(); + [Throws=NodeError] + PaymentDetailsPage list_payments(PageToken? page_token); sequence list_peers(); sequence list_channels(); NetworkGraph network_graph(); @@ -279,6 +280,10 @@ enum PaymentFailureReason { typedef dictionary PaymentDetails; +typedef dictionary PaymentDetailsPage; + +typedef interface PageToken; + [Remote] dictionary RouteParametersConfig { u64? max_total_routing_fee_msat; diff --git a/src/data_store.rs b/src/data_store.rs index a9dd3337d..e08d0ec1f 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -12,10 +12,10 @@ use std::ops::Deref; use std::sync::{Arc, Mutex}; use lightning::io::ErrorKind; -use lightning::util::persist::KVStore; +use lightning::util::persist::{KVStore, PageToken, PaginatedKVStore}; use lightning::util::ser::{Readable, Writeable}; -use crate::logger::{log_error, LdkLogger}; +use crate::logger::{log_debug, log_error, LdkLogger}; use crate::types::DynStore; use crate::Error; @@ -28,8 +28,15 @@ pub(crate) trait StorableObject: Clone + Readable + Writeable { fn to_update(&self) -> Self::Update; } -pub(crate) trait StorableObjectId: Clone + std::hash::Hash + PartialEq + Eq { +pub(crate) trait StorableObjectId: Clone + std::hash::Hash + PartialEq + Eq + Sized { fn encode_to_hex_str(&self) -> String; + + /// Recovers an id from the representation produced by [`Self::encode_to_hex_str`]. + /// + /// Returns `None` if `s` is not one. Callers listing a namespace must treat that as a cache + /// miss and read the object instead, whose own id is authoritative, rather than assume the + /// store only ever hands back keys we wrote. + fn decode_from_hex_str(s: &str) -> Option; } pub(crate) trait StorableObjectUpdate { @@ -202,6 +209,14 @@ impl ObjectCache { } } + /// Returns the cached object for `id`, without marking it as most recently used. + fn peek(&self, id: &SO::Id) -> Option { + match self { + Self::KeepAll(objects) => objects.get(id).cloned(), + Self::BoundedLru(lru) => lru.entries.get(id).map(|(object, _)| object.clone()), + } + } + /// Returns whether `id` is cached, without marking it as most recently used. fn contains(&self, id: &SO::Id) -> bool { match self { @@ -248,6 +263,15 @@ impl ObjectCache { } } +/// A page of objects, as returned by [`DataStore::list_page`]. +pub(crate) struct DataStorePage { + /// The objects in this page, ordered from most recently created to least recently created. + pub objects: Vec, + /// The token to pass to the next [`DataStore::list_page`] call, or `None` if this was the + /// last page. + pub next_page_token: Option, +} + pub(crate) struct DataStore where L::Target: LdkLogger, @@ -396,6 +420,163 @@ where self.contains(id).await } + /// Returns a page of objects, ordered from most recently created to least recently created. + /// + /// Pass `None` to start at the most recently created object, and the returned + /// [`DataStorePage::next_page_token`] to continue from where the previous call left off. + /// + /// The ordering and the tokens are the storage backend's own: we hand its opaque token back to + /// it unchanged and never derive an order of our own. That keeps tokens valid across restarts + /// and across changes to our caching, and it is why an object updated mid-pagination cannot + /// shift position and so be skipped or returned twice. + /// + /// Note this deliberately does not hold the mutation lock across its reads: a listing must not + /// block every writer for the duration of a round trip to a remote backend. Objects created or + /// removed while paginating may or may not be observed. + /// + /// Note also that a page may hold fewer objects than the backend's page size, because objects + /// removed between listing the keys and reading them are skipped. Iterate until + /// `next_page_token` is `None` rather than until a short page. + pub(crate) async fn list_page( + &self, page_token: Option, + ) -> Result, Error> { + let response = PaginatedKVStore::list_paginated( + &*self.kv_store, + &self.primary_namespace, + &self.secondary_namespace, + page_token, + ) + .await + .map_err(|e| { + log_error!( + self.logger, + "Listing objects under {}/{} failed due to: {}", + &self.primary_namespace, + &self.secondary_namespace, + e + ); + // The backend rejects a token it didn't issue, which is the caller's problem rather + // than a persistence failure. + if e.kind() == ErrorKind::InvalidInput { + Error::InvalidPageToken + } else { + Error::PersistenceFailed + } + })?; + + // Serve whatever we already hold, and note the rest to read below. We take the mutation + // lock only for this, so that we observe a consistent view of the cache without holding up + // writers while we read. + let mut objects: Vec> = vec![None; response.keys.len()]; + let mut missing = Vec::new(); + { + let _guard = self.mutation_lock.read().await; + let locked_cache = self.cache.lock().expect("lock"); + for (idx, key) in response.keys.iter().enumerate() { + // Note we deliberately peek rather than `get` here: a listing sweep walks the whole + // namespace, so letting it count as "use" would evict the working set it walks past. + match SO::Id::decode_from_hex_str(key).and_then(|id| locked_cache.peek(&id)) { + Some(object) => objects[idx] = Some(object), + None => missing.push((idx, key.clone())), + } + } + } + + self.read_missing(&mut objects, missing).await?; + + Ok(DataStorePage { + objects: objects.into_iter().flatten().collect(), + next_page_token: response.next_page_token, + }) + } + + /// Reads the objects we couldn't serve from the cache into their slots in `objects`. + /// + /// Reads run concurrently but are tracked by slot, as the order in which they finish says + /// nothing about the order of the page. Note the objects read here are deliberately *not* + /// cached, see [`Self::list_page`]. + async fn read_missing( + &self, objects: &mut [Option], missing: Vec<(usize, String)>, + ) -> Result<(), Error> { + // Matches every backend's page size, so in practice this is a single wave. We keep refilling + // anyway so that a backend paging more coarsely degrades gracefully rather than spawning an + // unbounded number of tasks. + const BATCH_SIZE: usize = 50; + + type ReadResult = (usize, String, Result, lightning::io::Error>); + let spawn_read = |set: &mut tokio::task::JoinSet, idx: usize, key: String| { + let read_fut = KVStore::read( + &*self.kv_store, + &self.primary_namespace, + &self.secondary_namespace, + &key, + ); + set.spawn(async move { (idx, key, read_fut.await) }); + }; + + let mut missing = missing.into_iter(); + let mut set = tokio::task::JoinSet::new(); + while set.len() < BATCH_SIZE { + let Some((idx, key)) = missing.next() else { break }; + spawn_read(&mut set, idx, key); + } + + while let Some(join_res) = set.join_next().await { + let (idx, key, read_res) = join_res.map_err(|e| { + log_error!(self.logger, "Failed to join object read task: {}", e); + set.abort_all(); + Error::PersistenceFailed + })?; + + if let Some((next_idx, next_key)) = missing.next() { + spawn_read(&mut set, next_idx, next_key); + } + + match read_res { + Ok(bytes) => match SO::read(&mut &bytes[..]) { + Ok(object) => objects[idx] = Some(object), + Err(e) => { + log_error!( + self.logger, + "Failed to deserialize object for key {}/{}/{}: {}", + &self.primary_namespace, + &self.secondary_namespace, + key, + e + ); + set.abort_all(); + return Err(Error::PersistenceFailed); + }, + }, + // The object was removed between us listing the keys and reading it, which is + // indistinguishable from it having been removed just before the listing. Skip it. + Err(e) if e.kind() == ErrorKind::NotFound => { + log_debug!( + self.logger, + "Skipping concurrently removed key {}/{}/{}", + &self.primary_namespace, + &self.secondary_namespace, + key + ); + }, + Err(e) => { + log_error!( + self.logger, + "Read for key {}/{}/{} failed due to: {}", + &self.primary_namespace, + &self.secondary_namespace, + key, + e + ); + set.abort_all(); + return Err(Error::PersistenceFailed); + }, + } + } + + Ok(()) + } + /// Returns the object stored under `id`, reading through to the [`KVStore`] if the cache is /// not authoritative and misses. /// @@ -555,7 +736,7 @@ mod tests { use super::*; use crate::hex_utils; - use crate::io::test_utils::InMemoryStore; + use crate::io::test_utils::{InMemoryStore, IN_MEMORY_PAGE_SIZE}; use crate::types::DynStoreWrapper; const TEST_PRIMARY_NAMESPACE: &str = "datastore_test_primary"; @@ -595,6 +776,10 @@ mod tests { fn encode_to_hex_str(&self) -> String { hex_utils::to_string(&self.id) } + + fn decode_from_hex_str(s: &str) -> Option { + hex_utils::to_vec(s)?.try_into().ok().map(|id| Self { id }) + } } impl_writeable_tlv_based!(TestObjectId, { (0, id, required) }); @@ -1310,4 +1495,286 @@ mod tests { assert_eq!(1, lru.entries.len()); assert_eq!(1, lru.recency.len()); } + + /// A store that reports one key from `list_paginated` that it will then fail to read, standing + /// in for an entry removed between the two calls. + struct PhantomKeyStore { + inner: InMemoryStore, + phantom_key: String, + } + + impl KVStore for PhantomKeyStore { + fn read( + &self, primary_namespace: &str, secondary_namespace: &str, key: &str, + ) -> impl std::future::Future, io::Error>> + 'static + Send { + self.inner.read(primary_namespace, secondary_namespace, key) + } + + fn write( + &self, primary_namespace: &str, secondary_namespace: &str, key: &str, buf: Vec, + ) -> impl std::future::Future> + 'static + Send { + self.inner.write(primary_namespace, secondary_namespace, key, buf) + } + + fn remove( + &self, primary_namespace: &str, secondary_namespace: &str, key: &str, lazy: bool, + ) -> impl std::future::Future> + 'static + Send { + self.inner.remove(primary_namespace, secondary_namespace, key, lazy) + } + + fn list( + &self, primary_namespace: &str, secondary_namespace: &str, + ) -> impl std::future::Future, io::Error>> + 'static + Send { + self.inner.list(primary_namespace, secondary_namespace) + } + } + + impl PaginatedKVStore for PhantomKeyStore { + fn list_paginated( + &self, primary_namespace: &str, secondary_namespace: &str, + page_token: Option, + ) -> impl std::future::Future> + 'static + Send + { + let phantom_key = self.phantom_key.clone(); + let inner_fut = + self.inner.list_paginated(primary_namespace, secondary_namespace, page_token); + async move { + let mut response = inner_fut.await?; + response.keys.insert(0, phantom_key); + Ok(response) + } + } + } + + /// Sweeps every page and returns the objects in the order they were listed. + async fn list_all_pages( + data_store: &DataStore, P>, + ) -> Vec { + let mut all = Vec::new(); + let mut page_token = None; + loop { + let page = data_store.list_page(page_token).await.unwrap(); + all.extend(page.objects); + match page.next_page_token { + Some(token) => page_token = Some(token), + None => break, + } + } + all + } + + /// Inserts `num_objects` objects with ascending ids through `data_store`. + async fn insert_ascending( + data_store: &DataStore, P>, num_objects: usize, + ) -> Vec { + let mut objects = Vec::new(); + for i in 0..num_objects { + let id = TestObjectId { id: (i as u32).to_be_bytes() }; + let object = TestObject::new(id, [7u8; 3]); + data_store.insert(object).await.unwrap(); + objects.push(object); + } + objects + } + + #[tokio::test] + async fn list_page_walks_pages_in_reverse_creation_order() { + let data_store = new_data_store(in_memory_store(), KeepAllEntries, Vec::new()); + let num_objects = 2 * IN_MEMORY_PAGE_SIZE + 25; + let inserted = insert_ascending(&data_store, num_objects).await; + + // Check the pages themselves are sized and terminated as expected. + let first = data_store.list_page(None).await.unwrap(); + assert_eq!(IN_MEMORY_PAGE_SIZE, first.objects.len()); + let second = data_store.list_page(first.next_page_token).await.unwrap(); + assert_eq!(IN_MEMORY_PAGE_SIZE, second.objects.len()); + let third = data_store.list_page(second.next_page_token).await.unwrap(); + assert_eq!(25, third.objects.len()); + assert!(third.next_page_token.is_none()); + + let mut expected = inserted; + expected.reverse(); + assert_eq!(expected, list_all_pages(&data_store).await); + } + + #[tokio::test] + async fn list_page_orders_by_creation_not_by_update() { + let data_store = new_data_store(in_memory_store(), KeepAllEntries, Vec::new()); + let inserted = insert_ascending(&data_store, IN_MEMORY_PAGE_SIZE + 10).await; + + // Touch the oldest object. Ordering by update time would move it to the front and so drop + // or duplicate entries across pages; ordering by creation must leave it where it is. + let oldest = inserted.first().unwrap(); + let update = TestObjectUpdate { id: oldest.id, data: [99u8; 3], extra: None }; + assert_eq!(Ok(DataStoreUpdateResult::Updated), data_store.update(update).await); + + let listed = list_all_pages(&data_store).await; + assert_eq!(inserted.len(), listed.len()); + assert_eq!(oldest.id, listed.last().unwrap().id); + assert_eq!([99u8; 3], listed.last().unwrap().data); + } + + #[tokio::test] + async fn list_page_token_survives_a_restart() { + let kv_store = in_memory_store(); + let num_objects = IN_MEMORY_PAGE_SIZE + 10; + let inserted = { + let data_store = new_data_store(Arc::clone(&kv_store), KeepAllEntries, Vec::new()); + insert_ascending(&data_store, num_objects).await + }; + + let first_page = { + let data_store = new_data_store(Arc::clone(&kv_store), KeepAllEntries, Vec::new()); + data_store.list_page(None).await.unwrap() + }; + let token = first_page.next_page_token.clone().unwrap(); + + // Resume from a *fresh* store, i.e., with nothing in memory, as an app would after being + // restarted between two pages. Because the ordering and the token are the backend's own, + // and not something we number ourselves, the continuation must still line up exactly. + let data_store = new_data_store(Arc::clone(&kv_store), KeepAllEntries, Vec::new()); + let second_page = data_store.list_page(Some(token)).await.unwrap(); + + let mut expected = inserted; + expected.reverse(); + assert_eq!(expected[..IN_MEMORY_PAGE_SIZE], first_page.objects[..]); + assert_eq!(expected[IN_MEMORY_PAGE_SIZE..], second_page.objects[..]); + assert!(second_page.next_page_token.is_none()); + } + + #[tokio::test] + async fn list_page_does_not_repeat_entries_after_removals_and_a_restart() { + let kv_store = in_memory_store(); + let num_objects = IN_MEMORY_PAGE_SIZE + 10; + let inserted = { + let data_store = new_data_store(Arc::clone(&kv_store), KeepAllEntries, Vec::new()); + insert_ascending(&data_store, num_objects).await + }; + + let data_store = new_data_store(Arc::clone(&kv_store), KeepAllEntries, inserted.clone()); + let first_page = data_store.list_page(None).await.unwrap(); + let token = first_page.next_page_token.clone().unwrap(); + let seen: Vec = first_page.objects.iter().map(|o| o.id).collect(); + + // Remove the oldest objects, including the one the token points at, then resume from a + // fresh store. An implementation that renumbered its own ordering on load would hand back + // entries from the first page again here. + let cursor_id = seen.last().copied().unwrap(); + data_store.remove(&cursor_id).await.unwrap(); + for object in inserted.iter().take(5) { + data_store.remove(&object.id).await.unwrap(); + } + + let resumed = new_data_store(Arc::clone(&kv_store), KeepAllEntries, Vec::new()); + let second_page = resumed.list_page(Some(token)).await.unwrap(); + for object in &second_page.objects { + assert!( + !seen.contains(&object.id), + "Object {:?} was returned on more than one page", + object.id + ); + } + } + + #[tokio::test] + async fn list_page_serves_entries_that_are_not_in_memory() { + // The point of doing this against the store rather than an ordering of our own: a bounded + // store can only hold a fraction of the namespace, yet must still list all of it. + let data_store = new_data_store(in_memory_store(), keep_lru(10), Vec::new()); + let inserted = insert_ascending(&data_store, 2 * IN_MEMORY_PAGE_SIZE + 25).await; + assert_eq!(10, data_store.cached_len()); + + let mut expected = inserted; + expected.reverse(); + assert_eq!(expected, list_all_pages(&data_store).await); + } + + #[tokio::test] + async fn list_page_does_not_disturb_the_cache() { + let data_store = new_data_store(in_memory_store(), keep_lru(2), Vec::new()); + let first = test_id(1); + let second = test_id(2); + data_store.insert(TestObject::new(first, [1u8; 3])).await.unwrap(); + data_store.insert(TestObject::new(second, [2u8; 3])).await.unwrap(); + + // Make `first` the most recently used, then sweep the whole namespace. + assert!(data_store.get(&first).await.unwrap().is_some()); + assert_eq!(2, list_all_pages(&data_store).await.len()); + assert_eq!(2, data_store.cached_len()); + + // The sweep must not have counted as use, so `second` is still the one to evict. + let third = test_id(3); + data_store.insert(TestObject::new(third, [3u8; 3])).await.unwrap(); + assert!(data_store.is_cached(&first)); + assert!(!data_store.is_cached(&second)); + assert!(data_store.is_cached(&third)); + } + + #[tokio::test] + async fn list_page_skips_concurrently_removed_keys() { + let phantom_id = test_id(200); + let kv_store: Arc = Arc::new(DynStoreWrapper(PhantomKeyStore { + inner: InMemoryStore::new(), + phantom_key: phantom_id.encode_to_hex_str(), + })); + let data_store = new_data_store(kv_store, keep_lru(1), Vec::new()); + let inserted = insert_ascending(&data_store, 3).await; + + // A key that vanished between being listed and being read is skipped rather than failing + // the whole listing. + let page = data_store.list_page(None).await.unwrap(); + assert_eq!(3, page.objects.len()); + for object in inserted { + assert!(page.objects.contains(&object)); + } + } + + #[tokio::test] + async fn list_page_reports_failures() { + let failing = + new_data_store(Arc::new(DynStoreWrapper(FailingStore)), KeepAllEntries, Vec::new()); + assert_eq!(Err(Error::PersistenceFailed), failing.list_page(None).await.map(|_| ())); + } + + #[tokio::test] + async fn list_page_rejects_a_malformed_token() { + let data_store = new_data_store(in_memory_store(), KeepAllEntries, Vec::new()); + insert_ascending(&data_store, 1).await; + + let token = PageToken::new("not-a-token".to_string()); + assert_eq!( + Err(Error::InvalidPageToken), + data_store.list_page(Some(token)).await.map(|_| ()) + ); + } + + #[tokio::test] + async fn list_page_reports_undecodable_objects() { + let kv_store = in_memory_store(); + let data_store = new_data_store(Arc::clone(&kv_store), keep_lru(1), Vec::new()); + insert_ascending(&data_store, 2).await; + + // Corrupt an object that is no longer cached, so that listing has to read it back. + let corrupted_id = TestObjectId { id: 0u32.to_be_bytes() }; + assert!(!data_store.is_cached(&corrupted_id)); + KVStore::write( + &*kv_store, + TEST_PRIMARY_NAMESPACE, + TEST_SECONDARY_NAMESPACE, + &corrupted_id.encode_to_hex_str(), + vec![0xff; 3], + ) + .await + .unwrap(); + + assert_eq!(Err(Error::PersistenceFailed), data_store.list_page(None).await.map(|_| ())); + } + + #[tokio::test] + async fn list_page_on_an_empty_store() { + let data_store = new_data_store(in_memory_store(), KeepAllEntries, Vec::new()); + let page = data_store.list_page(None).await.unwrap(); + assert!(page.objects.is_empty()); + assert!(page.next_page_token.is_none()); + } } diff --git a/src/ffi/types.rs b/src/ffi/types.rs index 0dc79758d..a1f94a11c 100644 --- a/src/ffi/types.rs +++ b/src/ffi/types.rs @@ -37,6 +37,7 @@ use lightning::offers::static_invoice::StaticInvoice as LdkStaticInvoice; use lightning::onion_message::dns_resolution::HumanReadableName as LdkHumanReadableName; pub use lightning::routing::gossip::{NodeAlias, NodeId, RoutingFees}; pub use lightning::routing::router::RouteParametersConfig; +use lightning::util::persist::PageToken as LdkPageToken; use lightning::util::ser::{Readable, Writeable, Writer}; use lightning_invoice::{Bolt11Invoice as LdkBolt11Invoice, Bolt11InvoiceDescriptionRef}; pub use lightning_invoice::{Description, SignedRawBolt11Invoice}; @@ -2745,3 +2746,50 @@ mod tests { assert_eq!(hrn1, hrn3); } } + +/// An opaque token used to continue a paginated listing. +/// +/// Obtain one from the page returned by a listing call and pass it back to retrieve the next page. +/// The value returned by `to_string` may be persisted and handed to the constructor later, so that +/// pagination can be resumed after a restart. +/// +/// The representation is defined by the storage backend and must be treated as opaque. A token is +/// only meaningful to the backend that issued it. +#[derive(Debug, Clone, PartialEq, Eq, uniffi::Object)] +#[uniffi::export(Debug, Display, Eq)] +pub struct PageToken { + pub(crate) inner: LdkPageToken, +} + +#[uniffi::export] +impl PageToken { + /// Constructs a token from the representation previously obtained via `to_string`. + #[uniffi::constructor] + pub fn new(token: String) -> Self { + Self { inner: LdkPageToken::new(token) } + } +} + +impl From for PageToken { + fn from(inner: LdkPageToken) -> Self { + Self { inner } + } +} + +impl From for LdkPageToken { + fn from(wrapper: PageToken) -> Self { + wrapper.inner + } +} + +impl AsRef for PageToken { + fn as_ref(&self) -> &LdkPageToken { + &self.inner + } +} + +impl std::fmt::Display for PageToken { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.inner) + } +} diff --git a/src/hex_utils.rs b/src/hex_utils.rs index 054e8b1f2..d60c4e188 100644 --- a/src/hex_utils.rs +++ b/src/hex_utils.rs @@ -7,7 +7,6 @@ use std::fmt::Write; -#[cfg(feature = "uniffi")] pub fn to_vec(hex: &str) -> Option> { // Reject malformed hex strings. if hex.len() % 2 != 0 { diff --git a/src/io/in_memory_store.rs b/src/io/in_memory_store.rs index 156fef3a3..82418e7d5 100644 --- a/src/io/in_memory_store.rs +++ b/src/io/in_memory_store.rs @@ -15,7 +15,7 @@ use lightning::util::persist::{ KVStore, MigratableKVStore, PageToken, PaginatedKVStore, PaginatedListResponse, }; -const IN_MEMORY_PAGE_SIZE: usize = 50; +pub(crate) const IN_MEMORY_PAGE_SIZE: usize = 50; pub struct InMemoryStore { persisted_bytes: Mutex>>>, diff --git a/src/io/test_utils.rs b/src/io/test_utils.rs index aadb4b79a..fa9b3e8ca 100644 --- a/src/io/test_utils.rs +++ b/src/io/test_utils.rs @@ -159,7 +159,7 @@ impl chainmonitor::Persist const EXPECTED_UPDATES_PER_PAYMENT: u64 = 5; -pub(crate) use in_memory_store::InMemoryStore; +pub(crate) use in_memory_store::{InMemoryStore, IN_MEMORY_PAGE_SIZE}; pub(crate) fn random_storage_path() -> PathBuf { let mut temp_path = std::env::temp_dir(); diff --git a/src/lib.rs b/src/lib.rs index 242988d47..0ce45bfdd 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -172,8 +172,8 @@ use logger::{log_debug, log_error, log_info, log_trace, LdkLogger, Logger}; use payment::asynchronous::om_mailbox::OnionMessageMailbox; use payment::asynchronous::static_invoice_store::StaticInvoiceStore; use payment::{ - Bolt11Payment, Bolt12Payment, OnchainPayment, PaymentDetails, SpontaneousPayment, - UnifiedPayment, + Bolt11Payment, Bolt12Payment, OnchainPayment, PaymentDetails, PaymentDetailsPage, + SpontaneousPayment, UnifiedPayment, }; use peer_store::{PeerInfo, PeerStore}; #[cfg(feature = "uniffi")] @@ -192,7 +192,7 @@ pub use types::{ pub use vss_client; use crate::config::{LIQUIDITY_DISCOVERY_RETRY_INITIAL_DELAY, LIQUIDITY_DISCOVERY_RETRY_MAX_DELAY}; -use crate::ffi::maybe_wrap; +use crate::ffi::{maybe_deref, maybe_wrap}; use crate::liquidity::Liquidity; use crate::scoring::setup_background_pathfinding_scores_sync; use crate::wallet::FundingAmount; @@ -2195,13 +2195,26 @@ impl Node { } } - /// Retrieves all payments that match the given predicate. + /// Retrieves a page of payments, ordered from most recently created to least recently created. + /// + /// Pass `None` to start at the most recently created payment, and the + /// [`PaymentDetailsPage::next_page_token`] returned by the previous call to continue from + /// where it left off. Ordering and pagination are backed by the configured storage backend, so + /// a token stays valid across restarts of the node. + /// + /// Payments created or removed while paginating may or may not be observed. Because the + /// ordering is by creation, a payment that exists throughout is never skipped nor returned + /// twice, but a page may hold fewer payments than the backend's page size. Iterate until + /// `next_page_token` is `None` rather than until a short page. + /// + /// Note that migrating between storage backends does not preserve the relative creation order + /// of pre-existing payments, so their order may change once after such a migration. /// /// For example, you could retrieve all stored outbound payments as follows: /// ``` /// # use ldk_node::Builder; /// # use ldk_node::config::Config; - /// # use ldk_node::payment::PaymentDirection; + /// # use ldk_node::payment::{PaymentDetails, PaymentDirection}; /// # use ldk_node::bitcoin::Network; /// # use ldk_node::entropy::{generate_entropy_mnemonic, NodeEntropy}; /// # use rand::distr::Alphanumeric; @@ -2216,17 +2229,29 @@ impl Node { /// # let mnemonic = generate_entropy_mnemonic(None); /// # let node_entropy = NodeEntropy::from_bip39_mnemonic(mnemonic, None); /// # let node = builder.build(node_entropy.into()).unwrap(); - /// node.list_payments_with_filter(|p| p.direction == PaymentDirection::Outbound); + /// let mut outbound = Vec::new(); + /// let mut page_token = None; + /// loop { + /// let page = node.list_payments(page_token)?; + /// outbound.extend( + /// page.payments.into_iter().filter(|p| p.direction == PaymentDirection::Outbound), + /// ); + /// match page.next_page_token { + /// Some(token) => page_token = Some(token), + /// None => break, + /// } + /// } + /// # Ok::<(), ldk_node::NodeError>(()) /// ``` - pub fn list_payments_with_filter bool>( - &self, f: F, - ) -> Vec { - self.runtime.block_on(self.payment_store.list_filter(f)) - } - - /// Retrieves all payments. - pub fn list_payments(&self) -> Vec { - self.list_payments_with_filter(|_| true) + pub fn list_payments( + &self, page_token: Option, + ) -> Result { + let ldk_page_token = page_token.as_ref().map(|token| maybe_deref(token).clone()); + let page = self.runtime.block_on(self.payment_store.list_page(ldk_page_token))?; + Ok(PaymentDetailsPage { + payments: page.objects, + next_page_token: page.next_page_token.map(maybe_wrap), + }) } /// Retrieves a list of known peers. diff --git a/src/payment/mod.rs b/src/payment/mod.rs index fd75322ce..cc55e1f94 100644 --- a/src/payment/mod.rs +++ b/src/payment/mod.rs @@ -23,7 +23,7 @@ pub use onchain::OnchainPayment; pub(crate) use pending_payment_store::{FundingTxCandidate, PendingPaymentDetails}; pub use spontaneous::SpontaneousPayment; pub use store::{ - Channel, ConfirmationStatus, LSPS2Parameters, PaymentDetails, PaymentDirection, PaymentKind, - PaymentStatus, TransactionType, + Channel, ConfirmationStatus, LSPS2Parameters, PageToken, PaymentDetails, PaymentDetailsPage, + PaymentDirection, PaymentKind, PaymentStatus, TransactionType, }; pub use unified::{UnifiedPayment, UnifiedPaymentResult}; diff --git a/src/payment/store.rs b/src/payment/store.rs index d2b92747a..c16a34ed9 100644 --- a/src/payment/store.rs +++ b/src/payment/store.rs @@ -25,6 +25,39 @@ use lightning_types::string::UntrustedString; use crate::data_store::{StorableObject, StorableObjectId, StorableObjectUpdate}; use crate::hex_utils; +/// An opaque token used to continue a paginated listing. +/// +/// See [`Node::list_payments`] for how to use it. +/// +/// [`Node::list_payments`]: crate::Node::list_payments +#[cfg(not(feature = "uniffi"))] +pub type PageToken = lightning::util::persist::PageToken; +/// An opaque token used to continue a paginated listing. +/// +/// See [`Node::list_payments`] for how to use it. +/// +/// [`Node::list_payments`]: crate::Node::list_payments +#[cfg(feature = "uniffi")] +pub type PageToken = std::sync::Arc; + +/// A page of payments, as returned by [`Node::list_payments`]. +/// +/// [`Node::list_payments`]: crate::Node::list_payments +#[derive(Clone, Debug, PartialEq, Eq)] +#[cfg_attr(feature = "uniffi", derive(uniffi::Record))] +pub struct PaymentDetailsPage { + /// The payments in this page, ordered from most recently created to least recently created. + /// + /// Note this may hold fewer payments than the storage backend's page size even when further + /// pages remain, so iterate until `next_page_token` is `None` rather than until a short page. + pub payments: Vec, + /// The token to pass to the next [`Node::list_payments`] call, or `None` if this was the last + /// page. + /// + /// [`Node::list_payments`]: crate::Node::list_payments + pub next_page_token: Option, +} + /// Represents a payment. #[derive(Clone, Debug, PartialEq, Eq)] #[cfg_attr(feature = "uniffi", derive(uniffi::Record))] @@ -152,6 +185,10 @@ impl StorableObjectId for PaymentId { fn encode_to_hex_str(&self) -> String { hex_utils::to_string(&self.0) } + + fn decode_from_hex_str(s: &str) -> Option { + hex_utils::to_vec(s)?.try_into().ok().map(PaymentId) + } } impl StorableObject for PaymentDetails { type Id = PaymentId; diff --git a/tests/common/mod.rs b/tests/common/mod.rs index efa6db988..701267f3e 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -431,13 +431,36 @@ pub(crate) trait NodePaymentExt { // `Node` and when it is an `Arc`. impl NodePaymentExt for Node { fn list_all_payments(&self) -> Vec { - self.list_payments() + let mut all = Vec::new(); + let mut seen = HashSet::new(); + let mut page_token = None; + let mut num_pages = 0; + loop { + let page = self.list_payments(page_token).unwrap(); + for payment in page.payments { + // Every test that looks at payments now exercises pagination, so assert the + // properties it is supposed to have while we are here. + assert!( + seen.insert(payment.id), + "Payment {:?} was returned on more than one page", + payment.id + ); + all.push(payment); + } + num_pages += 1; + assert!(num_pages < 1_000, "Pagination did not terminate after {} pages", num_pages); + match page.next_page_token { + Some(token) => page_token = Some(token), + None => break, + } + } + all } fn list_payments_matching bool>( - &self, f: F, + &self, mut f: F, ) -> Vec { - self.list_payments_with_filter(f) + self.list_all_payments().into_iter().filter(|p| f(&p)).collect() } } From 822f2c02331484ec07492bd7a7ec5f722c699d3b Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 6 Aug 2026 13:53:04 +0200 Subject: [PATCH 8/9] Bound the payment store's in-memory cache The payment history grows for the lifetime of a node, and holding all of it in memory was the reason `Node::list_payments` had to hand back everything at once. Now that the store can read entries back on demand and listing goes through the storage backend, the payment store no longer has to. Keep the most recently used payments in memory and read the rest back as they are needed. At roughly 400 to 500 bytes per cached payment, 1000 of them bound this at well under a megabyte, regardless of how long a node has been running. Also stop reading the payment history at startup, which would otherwise mean fetching a node's entire history from the storage backend only to immediately drop all but the newest entries. The cache now starts empty and fills as payments are used. One consequence worth noting: a payment that fails to deserialize no longer fails the build, as we no longer read them all up front. It surfaces when that payment is accessed instead. Co-Authored-By: HAL 9000 --- src/builder.rs | 56 ++++++++---------- src/config.rs | 9 +++ src/data_store.rs | 6 +- src/payment/store.rs | 137 +++++++++++++++++++++++++++++++++++++++++++ src/types.rs | 4 +- 5 files changed, 173 insertions(+), 39 deletions(-) diff --git a/src/builder.rs b/src/builder.rs index f0b8d17d7..18ee80bd1 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -50,10 +50,10 @@ use crate::config::{ default_user_config, may_announce_channel, AnnounceError, AsyncPaymentsRole, BitcoindRestClientConfig, Config, ElectrumSyncConfig, EsploraSyncConfig, HRNResolverConfig, TorConfig, DEFAULT_ESPLORA_SERVER_URL, DEFAULT_LOG_FILENAME, DEFAULT_LOG_LEVEL, - DEFAULT_MAX_PROBE_AMOUNT_MSAT, DEFAULT_MIN_PROBE_AMOUNT_MSAT, + DEFAULT_MAX_PROBE_AMOUNT_MSAT, DEFAULT_MIN_PROBE_AMOUNT_MSAT, PAYMENT_CACHE_CAPACITY, }; use crate::connection::ConnectionManager; -use crate::data_store::KeepAllEntries; +use crate::data_store::{KeepAllEntries, KeepLeastRecentlyUsed}; use crate::entropy::NodeEntropy; use crate::event::EventQueue; use crate::fee_estimator::OnchainFeeEstimator; @@ -1440,24 +1440,17 @@ fn build_with_store_internal( let kv_store_ref = Arc::clone(&kv_store); let logger_ref = Arc::clone(&logger); - let (payment_store_res, node_metris_res, pending_payment_store_res) = - runtime.block_on(async move { - tokio::join!( - read_all_objects( - &*kv_store_ref, - PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE, - PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE, - Arc::clone(&logger_ref), - ), - read_node_metrics(&*kv_store_ref, Arc::clone(&logger_ref)), - read_all_objects( - &*kv_store_ref, - PENDING_PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE, - PENDING_PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE, - Arc::clone(&logger_ref), - ) + let (node_metris_res, pending_payment_store_res) = runtime.block_on(async move { + tokio::join!( + read_node_metrics(&*kv_store_ref, Arc::clone(&logger_ref)), + read_all_objects( + &*kv_store_ref, + PENDING_PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE, + PENDING_PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE, + Arc::clone(&logger_ref), ) - }); + ) + }); // Initialize the status fields. let node_metrics = match node_metris_res { @@ -1472,20 +1465,17 @@ fn build_with_store_internal( }, }; - let payment_store = match payment_store_res { - Ok(payments) => Arc::new(PaymentStore::new( - payments, - KeepAllEntries, - PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE.to_string(), - PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE.to_string(), - Arc::clone(&kv_store), - Arc::clone(&logger), - )), - Err(e) => { - log_error!(logger, "Failed to read payment data from store: {}", e); - return Err(BuildError::ReadFailed); - }, - }; + // The payment store caches a bounded number of payments and reads the rest back on demand, so + // we start it empty rather than paying to read a node's entire payment history at startup only + // to immediately drop all but the most recent entries. + let payment_store = Arc::new(PaymentStore::new( + Vec::new(), + KeepLeastRecentlyUsed::new(PAYMENT_CACHE_CAPACITY), + PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE.to_string(), + PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE.to_string(), + Arc::clone(&kv_store), + Arc::clone(&logger), + )); let (chain_source, chain_tip_opt) = match chain_data_source_config { Some(ChainDataSourceConfig::Esplora { server_url, headers, sync_config }) => { diff --git a/src/config.rs b/src/config.rs index 772c4bd80..8ac899d11 100644 --- a/src/config.rs +++ b/src/config.rs @@ -8,6 +8,7 @@ //! Objects for configuring the node. use std::fmt; +use std::num::NonZeroUsize; use std::str::FromStr; use std::time::Duration; @@ -48,6 +49,14 @@ pub(crate) const DEFAULT_FEE_RATE_CACHE_UPDATE_TIMEOUT_SECS: u64 = 10; // The default timeout after which we abort a transaction broadcast operation. pub(crate) const DEFAULT_TX_BROADCAST_TIMEOUT_SECS: u64 = 10; +// The number of payments we keep in memory. +// +// The payment history grows for the lifetime of a node, so we cache only the most recently used +// payments and read the rest back from the store as they are needed. At roughly 400 to 500 bytes +// per cached payment, this bounds the payment store's share of memory at well under a megabyte, +// while still covering the recent payments a node actually works with. +pub(crate) const PAYMENT_CACHE_CAPACITY: NonZeroUsize = NonZeroUsize::new(1000).unwrap(); + // The default {Esplora,Electrum} client timeout we're using. const DEFAULT_PER_REQUEST_TIMEOUT_SECS: u8 = 10; diff --git a/src/data_store.rs b/src/data_store.rs index e08d0ec1f..8eaa0e385 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -86,12 +86,10 @@ impl CachePolicy for KeepAllEntries { /// [`KVStore`] whenever a lookup misses. /// /// Suitable for namespaces that grow without bound over a node's lifetime. -#[allow(dead_code)] // Constructed once a store opts into a bounded cache. pub(crate) struct KeepLeastRecentlyUsed { capacity: NonZeroUsize, } -#[allow(dead_code)] // See above. impl KeepLeastRecentlyUsed { pub(crate) fn new(capacity: NonZeroUsize) -> Self { Self { capacity } @@ -699,12 +697,12 @@ where } #[cfg(test)] - fn cached_len(&self) -> usize { + pub(crate) fn cached_len(&self) -> usize { self.cache.lock().expect("lock").len() } #[cfg(test)] - fn is_cached(&self, id: &SO::Id) -> bool { + pub(crate) fn is_cached(&self, id: &SO::Id) -> bool { self.cache.lock().expect("lock").contains(id) } } diff --git a/src/payment/store.rs b/src/payment/store.rs index c16a34ed9..f30304b9d 100644 --- a/src/payment/store.rs +++ b/src/payment/store.rs @@ -1123,3 +1123,140 @@ mod tests { assert_eq!(decoded, PaymentKind::read(&mut &*reencoded).unwrap()); } } + +#[cfg(test)] +mod bounded_cache_tests { + use std::num::NonZeroUsize; + use std::sync::Arc; + + use lightning::util::test_utils::TestLogger; + + use super::*; + use crate::config::PAYMENT_CACHE_CAPACITY; + use crate::data_store::{DataStore, DataStoreUpdateResult, KeepLeastRecentlyUsed}; + use crate::io::test_utils::InMemoryStore; + use crate::io::{ + PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE, PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE, + }; + use crate::types::{DynStore, DynStoreWrapper}; + + type BoundedPaymentStore = DataStore, KeepLeastRecentlyUsed>; + + fn new_bounded_payment_store(capacity: usize) -> BoundedPaymentStore { + let kv_store: Arc = Arc::new(DynStoreWrapper(InMemoryStore::new())); + DataStore::new( + Vec::new(), + KeepLeastRecentlyUsed::new(NonZeroUsize::new(capacity).unwrap()), + PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE.to_string(), + PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE.to_string(), + kv_store, + Arc::new(TestLogger::new()), + ) + } + + fn bolt11_payment(seed: u8) -> PaymentDetails { + PaymentDetails::new( + PaymentId([seed; 32]), + PaymentKind::Bolt11 { + hash: PaymentHash([seed; 32]), + preimage: Some(PaymentPreimage([seed.wrapping_add(1); 32])), + secret: Some(PaymentSecret([seed.wrapping_add(2); 32])), + counterparty_skimmed_fee_msat: Some(seed as u64 * 7), + }, + Some(seed as u64 * 1_000), + Some(seed as u64 * 3), + PaymentDirection::Outbound, + PaymentStatus::Succeeded, + ) + } + + #[tokio::test] + async fn evicted_payments_survive_a_round_trip_through_the_store() { + // A bounded store hands back objects it deserialized rather than ones it kept, so every + // field a payment carries has to survive being written out and read back. + let data_store = new_bounded_payment_store(2); + + let payments: Vec = (1..=10u8).map(bolt11_payment).collect(); + for payment in &payments { + data_store.insert(payment.clone()).await.unwrap(); + } + assert_eq!(2, data_store.cached_len()); + + for payment in &payments { + assert_eq!(Some(payment.clone()), data_store.get(&payment.id).await.unwrap()); + } + } + + #[tokio::test] + async fn updating_an_evicted_payment_preserves_the_fields_it_omits() { + // This is the failure mode a bounded cache invites: the wallet builds a partial + // `PaymentDetails` from a transaction and merges it in, and a payment that happens to have + // been evicted must not lose the fields only the merge target knows about. + let data_store = new_bounded_payment_store(1); + + let mut stored = bolt11_payment(1); + stored.fee_paid_msat = Some(4_242); + data_store.insert(stored.clone()).await.unwrap(); + + // Push it out of the cache. + data_store.insert(bolt11_payment(2)).await.unwrap(); + + let mut update = PaymentDetailsUpdate::new(stored.id); + update.status = Some(PaymentStatus::Failed); + assert_eq!(Ok(DataStoreUpdateResult::Updated), data_store.update(update).await); + + let updated = data_store.get(&stored.id).await.unwrap().unwrap(); + assert_eq!(PaymentStatus::Failed, updated.status); + assert_eq!(Some(4_242), updated.fee_paid_msat); + assert_eq!(stored.kind, updated.kind); + assert_eq!(stored.amount_msat, updated.amount_msat); + } + + #[tokio::test] + async fn listing_covers_payments_the_cache_cannot_hold() { + let data_store = new_bounded_payment_store(3); + + let payments: Vec = (1..=60u8).map(bolt11_payment).collect(); + for payment in &payments { + data_store.insert(payment.clone()).await.unwrap(); + } + assert_eq!(3, data_store.cached_len()); + + let mut listed = Vec::new(); + let mut page_token = None; + loop { + let page = data_store.list_page(page_token).await.unwrap(); + listed.extend(page.objects); + match page.next_page_token { + Some(token) => page_token = Some(token), + None => break, + } + } + + let mut expected = payments; + expected.reverse(); + assert_eq!(expected, listed); + // Listing the whole history must not have displaced the cache. + assert_eq!(3, data_store.cached_len()); + } + + #[tokio::test] + async fn the_cache_stays_within_its_capacity() { + let capacity = 16; + let data_store = new_bounded_payment_store(capacity); + + for seed in 1..=200u8 { + data_store.insert(bolt11_payment(seed)).await.unwrap(); + assert!(data_store.cached_len() <= capacity); + } + assert_eq!(capacity, data_store.cached_len()); + } + + #[test] + fn payment_cache_capacity_is_sane() { + // Small enough to bound memory at well under a megabyte, large enough to cover the recent + // payments a node actually works with. + assert!(PAYMENT_CACHE_CAPACITY.get() >= 100); + assert!(PAYMENT_CACHE_CAPACITY.get() <= 10_000); + } +} diff --git a/src/types.rs b/src/types.rs index 742d69be5..65156982e 100644 --- a/src/types.rs +++ b/src/types.rs @@ -45,7 +45,7 @@ use lightning_types::features::ChannelTypeFeatures; use crate::chain::bitcoind::UtxoSourceClient; use crate::chain::ChainSource; use crate::config::{AnchorChannelsConfig, ChannelConfig}; -use crate::data_store::{DataStore, KeepAllEntries}; +use crate::data_store::{DataStore, KeepAllEntries, KeepLeastRecentlyUsed}; use crate::fee_estimator::OnchainFeeEstimator; use crate::ffi::maybe_wrap; use crate::logger::Logger; @@ -375,7 +375,7 @@ pub(crate) type BumpTransactionEventHandler = Arc, >; -pub(crate) type PaymentStore = DataStore, KeepAllEntries>; +pub(crate) type PaymentStore = DataStore, KeepLeastRecentlyUsed>; /// A local, potentially user-provided, identifier of a channel. /// From 24c4fefedf5f4bbbc4595169689c4175191e1845 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 6 Aug 2026 14:58:25 +0200 Subject: [PATCH 9/9] io: Read only as many objects as a store needs Seeding a store meant reading its entire namespace, which for a bounded cache means fetching a node's whole payment history at startup only to drop all but the newest entries. The previous commit sidestepped that by not seeding the payment store at all, leaving it cold and no longer catching unreadable payment data at build time. Give the reader a bound instead, and seed the payment store with the newest 50 payments. That matches the storage backends' page size, so warming the cache costs a single page listing and one batch of reads, and the first page of `Node::list_payments` is answered without going to the store. Take the keys from the paginated listing rather than `KVStore::list`, which is documented to return them in arbitrary order and would therefore make "the newest 50" meaningless. Objects now come back in the store's own creation order, newest first, where before they came back in whatever order the reads happened to finish. Note the cache treats the objects it is seeded with as increasingly recently used, so a newest-first read has to be reversed before seeding, or the newest entries would be the first ones evicted. Co-Authored-By: HAL 9000 --- src/builder.rs | 61 +++++++---- src/config.rs | 7 ++ src/data_store.rs | 28 +++++ src/io/utils.rs | 268 +++++++++++++++++++++++++++++++++++++++++----- 4 files changed, 314 insertions(+), 50 deletions(-) diff --git a/src/builder.rs b/src/builder.rs index 18ee80bd1..f538e99b8 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -51,6 +51,7 @@ use crate::config::{ BitcoindRestClientConfig, Config, ElectrumSyncConfig, EsploraSyncConfig, HRNResolverConfig, TorConfig, DEFAULT_ESPLORA_SERVER_URL, DEFAULT_LOG_FILENAME, DEFAULT_LOG_LEVEL, DEFAULT_MAX_PROBE_AMOUNT_MSAT, DEFAULT_MIN_PROBE_AMOUNT_MSAT, PAYMENT_CACHE_CAPACITY, + PAYMENT_CACHE_WARMUP_COUNT, }; use crate::connection::ConnectionManager; use crate::data_store::{KeepAllEntries, KeepLeastRecentlyUsed}; @@ -61,8 +62,8 @@ use crate::gossip::GossipSource; use crate::io::sqlite_store::SqliteStore; use crate::io::utils::{ open_or_migrate_fs_store, read_all_objects, read_event_queue, - read_external_pathfinding_scores_from_cache, read_network_graph, read_node_metrics, - read_output_sweeper, read_peer_info, read_scorer, + read_external_pathfinding_scores_from_cache, read_n_objects, read_network_graph, + read_node_metrics, read_output_sweeper, read_peer_info, read_scorer, }; use crate::io::vss_store::VssStoreBuilder; use crate::io::{ @@ -1440,17 +1441,25 @@ fn build_with_store_internal( let kv_store_ref = Arc::clone(&kv_store); let logger_ref = Arc::clone(&logger); - let (node_metris_res, pending_payment_store_res) = runtime.block_on(async move { - tokio::join!( - read_node_metrics(&*kv_store_ref, Arc::clone(&logger_ref)), - read_all_objects( - &*kv_store_ref, - PENDING_PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE, - PENDING_PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE, - Arc::clone(&logger_ref), + let (payment_store_res, node_metris_res, pending_payment_store_res) = + runtime.block_on(async move { + tokio::join!( + read_n_objects( + &*kv_store_ref, + PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE, + PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE, + PAYMENT_CACHE_WARMUP_COUNT, + Arc::clone(&logger_ref), + ), + read_node_metrics(&*kv_store_ref, Arc::clone(&logger_ref)), + read_all_objects( + &*kv_store_ref, + PENDING_PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE, + PENDING_PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE, + Arc::clone(&logger_ref), + ) ) - ) - }); + }); // Initialize the status fields. let node_metrics = match node_metris_res { @@ -1465,17 +1474,23 @@ fn build_with_store_internal( }, }; - // The payment store caches a bounded number of payments and reads the rest back on demand, so - // we start it empty rather than paying to read a node's entire payment history at startup only - // to immediately drop all but the most recent entries. - let payment_store = Arc::new(PaymentStore::new( - Vec::new(), - KeepLeastRecentlyUsed::new(PAYMENT_CACHE_CAPACITY), - PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE.to_string(), - PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE.to_string(), - Arc::clone(&kv_store), - Arc::clone(&logger), - )); + let payment_store = match payment_store_res { + Ok(payments) => Arc::new(PaymentStore::new( + // The read hands us the newest payments first, while the cache treats the objects it + // is seeded with as increasingly recently used. Reverse them, so that the newest + // payment is the last one to be evicted rather than the first. + payments.into_iter().rev().collect(), + KeepLeastRecentlyUsed::new(PAYMENT_CACHE_CAPACITY), + PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE.to_string(), + PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE.to_string(), + Arc::clone(&kv_store), + Arc::clone(&logger), + )), + Err(e) => { + log_error!(logger, "Failed to read payment data from store: {}", e); + return Err(BuildError::ReadFailed); + }, + }; let (chain_source, chain_tip_opt) = match chain_data_source_config { Some(ChainDataSourceConfig::Esplora { server_url, headers, sync_config }) => { diff --git a/src/config.rs b/src/config.rs index 8ac899d11..e5b96bcaa 100644 --- a/src/config.rs +++ b/src/config.rs @@ -57,6 +57,13 @@ pub(crate) const DEFAULT_TX_BROADCAST_TIMEOUT_SECS: u64 = 10; // while still covering the recent payments a node actually works with. pub(crate) const PAYMENT_CACHE_CAPACITY: NonZeroUsize = NonZeroUsize::new(1000).unwrap(); +// The number of payments we read into the cache when starting up. +// +// This matches the storage backends' page size, so warming the cache costs a single page listing +// and one batch of reads, and the first page of `Node::list_payments` is served without going to +// the store at all. The remaining capacity fills as payments are used. +pub(crate) const PAYMENT_CACHE_WARMUP_COUNT: NonZeroUsize = NonZeroUsize::new(50).unwrap(); + // The default {Esplora,Electrum} client timeout we're using. const DEFAULT_PER_REQUEST_TIMEOUT_SECS: u8 = 10; diff --git a/src/data_store.rs b/src/data_store.rs index 8eaa0e385..00ca690b2 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -297,6 +297,10 @@ where /// `objects` seeds the cache and must already be persisted under that namespace: under a /// bounded policy any object beyond `cache_policy`'s capacity is dropped from memory /// immediately, and is only recoverable by reading it back from the store. + /// + /// They are taken in ascending order of recency, i.e., the last one given is treated as the + /// most recently used and is therefore the last to be evicted. Callers seeding from a + /// newest-first source have to reverse it. pub(crate) fn new( objects: Vec, cache_policy: P, primary_namespace: String, secondary_namespace: String, kv_store: Arc, logger: L, @@ -1381,6 +1385,30 @@ mod tests { .is_err()); } + #[tokio::test] + async fn lru_seeding_treats_the_last_object_as_most_recently_used() { + // The builder relies on this to hand a newest-first read over in reverse: whichever + // objects are given last must be the ones that survive, or seeding the cache would + // preferentially throw away the newest entries. + let kv_store = in_memory_store(); + let seed_store = new_data_store(Arc::clone(&kv_store), KeepAllEntries, Vec::new()); + let mut objects = Vec::new(); + for i in 0..5u8 { + let object = TestObject::new(test_id(i), [i; 3]); + seed_store.insert(object).await.unwrap(); + objects.push(object); + } + + let data_store = new_data_store(kv_store, keep_lru(2), objects.clone()); + + assert_eq!(2, data_store.cached_len()); + assert!(data_store.is_cached(&objects[3].id)); + assert!(data_store.is_cached(&objects[4].id)); + for object in objects.iter().take(3) { + assert!(!data_store.is_cached(&object.id)); + } + } + #[tokio::test] async fn lru_seeding_trims_to_capacity() { let kv_store = in_memory_store(); diff --git a/src/io/utils.rs b/src/io/utils.rs index 4657688f5..a60bbe6e3 100644 --- a/src/io/utils.rs +++ b/src/io/utils.rs @@ -7,6 +7,7 @@ use std::fs::{self, OpenOptions}; use std::io::Write; +use std::num::NonZeroUsize; use std::ops::Deref; #[cfg(unix)] use std::os::unix::fs::OpenOptionsExt; @@ -26,7 +27,7 @@ use lightning::routing::scoring::{ ChannelLiquidities, ProbabilisticScorer, ProbabilisticScoringDecayParameters, }; use lightning::util::persist::{ - migrate_kv_store_data_async, KVStore, KVSTORE_NAMESPACE_KEY_ALPHABET, + migrate_kv_store_data_async, KVStore, PaginatedKVStore, KVSTORE_NAMESPACE_KEY_ALPHABET, KVSTORE_NAMESPACE_KEY_MAX_LEN, NETWORK_GRAPH_PERSISTENCE_KEY, NETWORK_GRAPH_PERSISTENCE_PRIMARY_NAMESPACE, NETWORK_GRAPH_PERSISTENCE_SECONDARY_NAMESPACE, OUTPUT_SWEEPER_PERSISTENCE_KEY, OUTPUT_SWEEPER_PERSISTENCE_PRIMARY_NAMESPACE, @@ -222,69 +223,141 @@ where }) } -/// Read all objects of type `T` from the given namespace, spawning reads in parallel. +/// Reads all objects of type `T` from the given namespace, ordered from most recently created to +/// least recently created. pub(crate) async fn read_all_objects( kv_store: &DynStore, primary_namespace: &str, secondary_namespace: &str, logger: L, ) -> Result, std::io::Error> +where + T: Readable, + L: Deref, + L::Target: LdkLogger, +{ + read_objects_internal(kv_store, primary_namespace, secondary_namespace, None, logger).await +} + +/// Reads the `num_objects` most recently created objects of type `T` from the given namespace, +/// ordered from most recently created to least recently created. +/// +/// Returns fewer objects if the namespace holds fewer than `num_objects`. +pub(crate) async fn read_n_objects( + kv_store: &DynStore, primary_namespace: &str, secondary_namespace: &str, + num_objects: NonZeroUsize, logger: L, +) -> Result, std::io::Error> +where + T: Readable, + L: Deref, + L::Target: LdkLogger, +{ + read_objects_internal( + kv_store, + primary_namespace, + secondary_namespace, + Some(num_objects), + logger, + ) + .await +} + +/// Reads up to `num_objects` objects of type `T` from the given namespace, or all of them if +/// `None`, spawning reads in parallel. +/// +/// Objects are returned in the store's own creation order, most recently created first. Note we +/// take the keys from [`PaginatedKVStore::list_paginated`] rather than [`KVStore::list`], because +/// the latter is documented to return them in arbitrary order, which would make "the newest +/// `num_objects`" meaningless. +async fn read_objects_internal( + kv_store: &DynStore, primary_namespace: &str, secondary_namespace: &str, + num_objects: Option, logger: L, +) -> Result, std::io::Error> where T: Readable, L: Deref, L::Target: LdkLogger, { let type_name = std::any::type_name::(); - let mut res = Vec::new(); + let max_objects = num_objects.map_or(usize::MAX, |num_objects| num_objects.get()); + + // Collect the keys we're after, page by page, so that a bounded read doesn't pay for keys it + // would never look at. + let mut stored_keys: Vec = Vec::new(); + let mut page_token = None; + loop { + let response = PaginatedKVStore::list_paginated( + &*kv_store, + primary_namespace, + secondary_namespace, + page_token, + ) + .await?; + + let remaining = max_objects.saturating_sub(stored_keys.len()); + stored_keys.extend(response.keys.into_iter().take(remaining)); - let mut stored_keys = KVStore::list(&*kv_store, primary_namespace, secondary_namespace).await?; + if stored_keys.len() >= max_objects { + break; + } + + match response.next_page_token { + Some(next_page_token) => page_token = Some(next_page_token), + None => break, + } + } const BATCH_SIZE: usize = 50; + // Reads are tracked by slot, as the order in which they finish says nothing about the order we + // promised to return them in. + let mut objects: Vec> = Vec::new(); + objects.resize_with(stored_keys.len(), || None); + let mut keys = stored_keys.into_iter().enumerate(); let mut set = tokio::task::JoinSet::new(); // Fill JoinSet with tasks if possible - while set.len() < BATCH_SIZE && !stored_keys.is_empty() { - if let Some(next_key) = stored_keys.pop() { - let fut = KVStore::read(kv_store, primary_namespace, secondary_namespace, &next_key); - set.spawn(fut); - debug_assert!(set.len() <= BATCH_SIZE); - } + while set.len() < BATCH_SIZE { + let Some((idx, key)) = keys.next() else { break }; + let fut = KVStore::read(kv_store, primary_namespace, secondary_namespace, &key); + set.spawn(async move { (idx, fut.await) }); + debug_assert!(set.len() <= BATCH_SIZE); } - while let Some(read_res) = set.join_next().await { + while let Some(join_res) = set.join_next().await { // Exit early if we get an IO error. - let reader = read_res - .map_err(|e| { - log_error!(logger, "Failed to read {}: {}", type_name, e); - set.abort_all(); - e - })? - .map_err(|e| { - log_error!(logger, "Failed to read {}: {}", type_name, e); - set.abort_all(); - e - })?; + let (idx, read_res) = join_res.map_err(|e| { + log_error!(logger, "Failed to read {}: {}", type_name, e); + set.abort_all(); + e + })?; + let reader = read_res.map_err(|e| { + log_error!(logger, "Failed to read {}: {}", type_name, e); + set.abort_all(); + e + })?; // Refill set for every finished future, if we still have something to do. - if let Some(next_key) = stored_keys.pop() { + if let Some((next_idx, next_key)) = keys.next() { let fut = KVStore::read(kv_store, primary_namespace, secondary_namespace, &next_key); - set.spawn(fut); + set.spawn(async move { (next_idx, fut.await) }); debug_assert!(set.len() <= BATCH_SIZE); } // Handle result. let object = T::read(&mut &*reader).map_err(|e| { log_error!(logger, "Failed to deserialize {}: {}", type_name, e); + set.abort_all(); std::io::Error::new( std::io::ErrorKind::InvalidData, format!("Failed to deserialize {}", type_name), ) })?; - res.push(object); + objects[idx] = Some(object); } debug_assert!(set.is_empty()); - debug_assert!(stored_keys.is_empty()); + debug_assert!(keys.next().is_none()); + debug_assert!(objects.iter().all(|object| object.is_some())); - Ok(res) + Ok(objects.into_iter().flatten().collect()) } /// Read `OutputSweeper` state from the store. @@ -901,3 +974,144 @@ mod tests { v1_store } } + +#[cfg(test)] +mod read_objects_tests { + use std::num::NonZeroUsize; + use std::sync::Arc; + + use lightning::impl_writeable_tlv_based; + use lightning::util::persist::KVStore; + use lightning::util::ser::Writeable; + use lightning::util::test_utils::TestLogger; + + use super::test_utils::{InMemoryStore, IN_MEMORY_PAGE_SIZE}; + use super::{read_all_objects, read_n_objects}; + use crate::hex_utils; + use crate::types::{DynStore, DynStoreWrapper}; + + const TEST_PRIMARY_NAMESPACE: &str = "read_objects_test_primary"; + const TEST_SECONDARY_NAMESPACE: &str = "read_objects_test_secondary"; + + #[derive(Clone, Copy, Debug, Eq, PartialEq)] + struct TestObject { + id: u32, + } + impl_writeable_tlv_based!(TestObject, { (0, id, required) }); + + /// Writes `num_objects` objects with ascending ids, so that the highest id is the most + /// recently created one. + async fn store_with_objects(num_objects: u32) -> (Arc, Vec) { + let kv_store: Arc = Arc::new(DynStoreWrapper(InMemoryStore::new())); + let mut objects = Vec::new(); + for id in 0..num_objects { + let object = TestObject { id }; + KVStore::write( + &*kv_store, + TEST_PRIMARY_NAMESPACE, + TEST_SECONDARY_NAMESPACE, + &hex_utils::to_string(&id.to_be_bytes()), + object.encode(), + ) + .await + .unwrap(); + objects.push(object); + } + (kv_store, objects) + } + + fn newest_first(objects: &[TestObject], num_objects: usize) -> Vec { + objects.iter().rev().take(num_objects).cloned().collect() + } + + async fn read_n(kv_store: &DynStore, num_objects: usize) -> Vec { + read_n_objects( + kv_store, + TEST_PRIMARY_NAMESPACE, + TEST_SECONDARY_NAMESPACE, + NonZeroUsize::new(num_objects).unwrap(), + Arc::new(TestLogger::new()), + ) + .await + .unwrap() + } + + #[tokio::test] + async fn reads_the_newest_objects_within_a_single_page() { + let (kv_store, objects) = store_with_objects(IN_MEMORY_PAGE_SIZE as u32).await; + assert_eq!(newest_first(&objects, 10), read_n(&*kv_store, 10).await); + } + + #[tokio::test] + async fn reads_the_newest_objects_across_several_pages() { + let num_objects = 3 * IN_MEMORY_PAGE_SIZE + 7; + let (kv_store, objects) = store_with_objects(num_objects as u32).await; + + // Spanning more than one page is where paging the keys, rather than listing them all, + // actually has to work. + let wanted = 2 * IN_MEMORY_PAGE_SIZE + 3; + assert_eq!(newest_first(&objects, wanted), read_n(&*kv_store, wanted).await); + } + + #[tokio::test] + async fn reading_more_than_is_stored_returns_everything() { + let (kv_store, objects) = store_with_objects(5).await; + assert_eq!(newest_first(&objects, 5), read_n(&*kv_store, 500).await); + } + + #[tokio::test] + async fn reads_all_objects_newest_first() { + let num_objects = 2 * IN_MEMORY_PAGE_SIZE + 11; + let (kv_store, objects) = store_with_objects(num_objects as u32).await; + + let read: Vec = read_all_objects( + &*kv_store, + TEST_PRIMARY_NAMESPACE, + TEST_SECONDARY_NAMESPACE, + Arc::new(TestLogger::new()), + ) + .await + .unwrap(); + assert_eq!(newest_first(&objects, num_objects), read); + } + + #[tokio::test] + async fn reading_an_empty_namespace_yields_nothing() { + let kv_store: Arc = Arc::new(DynStoreWrapper(InMemoryStore::new())); + assert!(read_n(&*kv_store, 10).await.is_empty()); + + let all: Vec = read_all_objects( + &*kv_store, + TEST_PRIMARY_NAMESPACE, + TEST_SECONDARY_NAMESPACE, + Arc::new(TestLogger::new()), + ) + .await + .unwrap(); + assert!(all.is_empty()); + } + + #[tokio::test] + async fn an_undecodable_object_is_an_error() { + let (kv_store, _objects) = store_with_objects(3).await; + KVStore::write( + &*kv_store, + TEST_PRIMARY_NAMESPACE, + TEST_SECONDARY_NAMESPACE, + &hex_utils::to_string(&99u32.to_be_bytes()), + vec![0xff; 2], + ) + .await + .unwrap(); + + let res = read_n_objects::( + &*kv_store, + TEST_PRIMARY_NAMESPACE, + TEST_SECONDARY_NAMESPACE, + NonZeroUsize::new(10).unwrap(), + Arc::new(TestLogger::new()), + ) + .await; + assert_eq!(std::io::ErrorKind::InvalidData, res.unwrap_err().kind()); + } +}