From a6ffb98cfb7765473b77405ea86020e8fa07a5e8 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 13 Aug 2026 09:22:12 +0200 Subject: [PATCH 01/26] data_store: Share mutation implementation Keep mutation locking, persistence, and in-memory publication in one implementation so synchronous and asynchronous transformations can share the same atomic write path. Co-Authored-By: HAL 9000 --- src/data_store.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/data_store.rs b/src/data_store.rs index a440a2e1e..1de3729c0 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -6,6 +6,7 @@ // accordance with one or both of these licenses. use std::collections::HashMap; +use std::future::Future; use std::ops::Deref; use std::sync::{Arc, Mutex}; @@ -187,10 +188,18 @@ where pub(crate) async fn mutate) -> Option>( &self, id: &SO::Id, f: F, ) -> Result, Error> { + self.mutate_with(id, |current| async move { Ok(f(current.as_ref())) }).await + } + + async fn mutate_with(&self, id: &SO::Id, f: F) -> Result, Error> + where + F: FnOnce(Option) -> Fut, + Fut: Future, Error>>, + { let _guard = self.mutation_lock.lock().await; let current = self.objects.lock().expect("lock").get(id).cloned(); - let new_object = match f(current.as_ref()) { + let new_object = match f(current).await? { Some(new_object) => new_object, None => return Ok(None), }; From d34be06083ba25f102c3f6b83d84038533141d4b Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 6 Aug 2026 12:49:44 +0200 Subject: [PATCH 02/26] 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 df95c11ec..141ceac29 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -537,13 +537,13 @@ impl Wallet { // cycle, and an id resolved through the candidate history comes from a // classification whose payment-store write strictly precedes the candidate // history it was resolved from. 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 d695378e4de4ec16f392104473ab068842c6bb87 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 6 Aug 2026 12:54:48 +0200 Subject: [PATCH 03/26] 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 | 66 ++++++++++++++++++++------------- src/lib.rs | 11 +++--- src/payment/bolt11.rs | 19 +++++----- src/payment/spontaneous.rs | 2 +- src/wallet/mod.rs | 36 +++++++++++------- tests/common/mod.rs | 29 ++++++++------- tests/integration_tests_rust.rs | 52 +++++++++++++++----------- 9 files changed, 144 insertions(+), 106 deletions(-) diff --git a/bindings/ldk_node.udl b/bindings/ldk_node.udl index be5fdc808..e4e0e4c9e 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 1de3729c0..8e67248eb 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -143,13 +143,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 { @@ -211,12 +211,12 @@ where Ok(Some(new_object)) } - /// 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::>() } @@ -252,13 +252,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)) } } @@ -393,7 +393,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(); @@ -405,7 +405,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()); @@ -414,12 +414,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] }; @@ -580,12 +580,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] @@ -595,7 +595,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] @@ -606,7 +606,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] @@ -616,6 +616,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 be54969c7..8808641c6 100644 --- a/src/event.rs +++ b/src/event.rs @@ -691,23 +691,31 @@ where }) } - fn resolve_inbound_payment_id( + async fn resolve_inbound_payment_id( &self, event_payment_id: Option, payment_hash: &PaymentHash, - ) -> (PaymentId, Option) { + ) -> Result<(PaymentId, Option), ReplayEvent> { let legacy_id = PaymentId(payment_hash.0); let payment_id = event_payment_id.unwrap_or(legacy_id); - if let Some(info) = self.payment_store.get(&payment_id) { - return (payment_id, Some(info)); + let payment_info = self.payment_store.get(&payment_id).await.map_err(|e| { + log_error!(self.logger, "Failed to access payment store: {}", e); + ReplayEvent() + })?; + if let Some(info) = payment_info { + return Ok((payment_id, Some(info))); } if legacy_id != payment_id { - if let Some(info) = self.payment_store.get(&legacy_id) { - return (legacy_id, Some(info)); + let legacy_payment_info = self.payment_store.get(&legacy_id).await.map_err(|e| { + log_error!(self.logger, "Failed to access payment store: {}", e); + ReplayEvent() + })?; + if let Some(info) = legacy_payment_info { + return Ok((legacy_id, Some(info))); } } - (payment_id, None) + Ok((payment_id, None)) } pub async fn handle_event(&self, event: LdkEvent) -> Result<(), ReplayEvent> { @@ -827,7 +835,7 @@ where .. } => { let (payment_id, mut payment_info) = - self.resolve_inbound_payment_id(payment_id, &payment_hash); + self.resolve_inbound_payment_id(payment_id, &payment_hash).await?; if let Some(info) = payment_info.as_ref() { if info.direction == PaymentDirection::Outbound { log_info!( @@ -1288,7 +1296,8 @@ where sender_intended_total_msat: _, onion_fields, } => { - let (payment_id, _) = self.resolve_inbound_payment_id(payment_id, &payment_hash); + let (payment_id, _) = + self.resolve_inbound_payment_id(payment_id, &payment_hash).await?; log_info!( self.logger, "Claimed payment with ID {} from payment hash {} of {}msat.", @@ -1459,22 +1468,29 @@ 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{} with payment hash {}", - amount_msat, - if let Some(fee) = fee_paid_msat { - format!(" (fee {} msat)", fee) - } else { - "".to_string() - }, - hex_utils::to_string(&payment_hash.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{} with payment hash {}", + amount_msat, + if let Some(fee) = fee_paid_msat { + format!(" (fee {} msat)", fee) + } else { + "".to_string() + }, + hex_utils::to_string(&payment_hash.0), + ); + }, + Ok(None) => {}, + Err(e) => { + log_error!(self.logger, "Failed to access payment store: {}", e); + return Err(ReplayEvent()); + }, + }; let event = Event::PaymentSuccessful { 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 e33dd3950..208c677c3 100644 --- a/src/payment/bolt11.rs +++ b/src/payment/bolt11.rs @@ -208,7 +208,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 { @@ -423,14 +423,15 @@ impl Bolt11Payment { pub fn claim_for_id( &self, payment_id: PaymentId, claimable_amount_msat: u64, preimage: PaymentPreimage, ) -> Result<(), Error> { - let details = self.payment_store.get(&payment_id).ok_or_else(|| { - log_error!( - self.logger, - "Failed to manually claim unknown payment with ID: {}", - payment_id - ); - Error::InvalidPaymentId - })?; + let details = + self.runtime.block_on(self.payment_store.get(&payment_id))?.ok_or_else(|| { + log_error!( + self.logger, + "Failed to manually claim unknown payment with ID: {}", + payment_id + ); + Error::InvalidPaymentId + })?; let payment_hash = match details.kind { PaymentKind::Bolt11 { hash, .. } => hash, 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 141ceac29..0c6f6b413 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -343,6 +343,7 @@ impl Wallet { let payment_id = self .find_payment_by_txid(txid) + .await? .unwrap_or_else(|| PaymentId(txid.to_byte_array())); if self @@ -379,8 +380,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", @@ -388,7 +390,8 @@ impl Wallet { ); p.details.status == PaymentStatus::Pending && matches!(p.details.kind, PaymentKind::Onchain { .. }) - }); + }) + .await; let mut unconfirmed_outbound_txids: Vec = Vec::new(); @@ -481,6 +484,7 @@ impl Wallet { let payment_id = self .find_payment_by_txid(txid) + .await? .unwrap_or_else(|| PaymentId(txid.to_byte_array())); if self @@ -518,7 +522,7 @@ impl Wallet { // classification either. let _guard = self.funding_payment_update_lock.lock().await; - 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.", @@ -537,7 +541,7 @@ impl Wallet { // cycle, and an id resolved through the candidate history comes from a // classification whose payment-store write strictly precedes the candidate // history it was resolved from. 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", @@ -556,6 +560,7 @@ impl Wallet { let payment_id = self .find_payment_by_txid(txid) + .await? .unwrap_or_else(|| PaymentId(txid.to_byte_array())); if self @@ -1900,10 +1905,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 @@ -1916,12 +1921,13 @@ impl Wallet { // txids (an earlier RBF round may confirm) back to the record. || p.candidate(target_txid).is_some() }) + .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 @@ -1939,9 +1945,11 @@ impl Wallet { &self, _guard: &tokio::sync::MutexGuard<'_, ()>, payment_id: PaymentId, event_txid: Txid, confirmation_status: ConfirmationStatus, ) -> Result { - // The funding-type gate, the candidate lookup, and the write share the store's mutation - // lock: against a separate `get`, a classification merging in between would have its + // The caller's wallet-level lock keeps the candidate history stable while we await its + // read. The funding-type gate and write then share the payment store's mutation lock: + // against a separate payment `get`, a classification merging in between would have its // `tx_type` and contribution figures clobbered by this stale snapshot. + let pending_payment = self.pending_payment_store.get(&payment_id).await?; let mut handled = None; self.payment_store .mutate(&payment_id, |existing| { @@ -1963,7 +1971,7 @@ impl Wallet { // invariant across a splice's candidates and cannot be changed through the store // anyway.) let mut target = payment.clone(); - if let Some(pending) = self.pending_payment_store.get(&payment_id) { + if let Some(pending) = pending_payment.as_ref() { if let Some(candidate) = pending.candidate(event_txid) { target.amount_msat = candidate.amount_msat; target.fee_paid_msat = candidate.fee_paid_msat; @@ -2002,7 +2010,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 9c1bb75c0..8b447fa73 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(); + let payment = $node.payment(&payment_id).unwrap().unwrap(); if !matches!(payment.kind, ldk_node::payment::PaymentKind::Onchain { .. }) { assert_eq!(payment.fee_paid_msat, None); } @@ -322,7 +322,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(); + let payment = $node.payment(&$payment_id).unwrap().unwrap(); assert_eq!(payment.fee_paid_msat, fee_paid_msat); assert_eq!(payment_id, $payment_id); $node.event_handled().unwrap(); @@ -1307,12 +1307,12 @@ pub(crate) async fn do_channel_full_cycle( }, } let inbound_payment_id = expect_payment_received_event!(node_b, invoice_amount_1_msat); - let outbound_payment = node_a.payment(&outbound_payment_id).unwrap(); + let outbound_payment = node_a.payment(&outbound_payment_id).unwrap().unwrap(); assert_eq!(outbound_payment.status, PaymentStatus::Succeeded); assert_eq!(outbound_payment.direction, PaymentDirection::Outbound); assert_eq!(outbound_payment.amount_msat, Some(invoice_amount_1_msat)); assert!(matches!(&outbound_payment.kind, PaymentKind::Bolt11 { .. })); - let inbound_payment = node_b.payment(&inbound_payment_id).unwrap(); + let inbound_payment = node_b.payment(&inbound_payment_id).unwrap().unwrap(); assert_eq!(inbound_payment.status, PaymentStatus::Succeeded); assert_eq!(inbound_payment.direction, PaymentDirection::Inbound); assert_eq!(inbound_payment.amount_msat, Some(invoice_amount_1_msat)); @@ -1353,12 +1353,12 @@ pub(crate) async fn do_channel_full_cycle( }, }; assert_eq!(received_amount, overpaid_amount_msat); - let outbound_payment = node_a.payment(&outbound_payment_id).unwrap(); + let outbound_payment = node_a.payment(&outbound_payment_id).unwrap().unwrap(); assert_eq!(outbound_payment.status, PaymentStatus::Succeeded); assert_eq!(outbound_payment.direction, PaymentDirection::Outbound); assert_eq!(outbound_payment.amount_msat, Some(overpaid_amount_msat)); assert!(matches!(&outbound_payment.kind, PaymentKind::Bolt11 { .. })); - let inbound_payment = node_b.payment(&inbound_payment_id).unwrap(); + let inbound_payment = node_b.payment(&inbound_payment_id).unwrap().unwrap(); assert_eq!(inbound_payment.status, PaymentStatus::Succeeded); assert_eq!(inbound_payment.direction, PaymentDirection::Inbound); assert_eq!(inbound_payment.amount_msat, Some(overpaid_amount_msat)); @@ -1393,12 +1393,12 @@ pub(crate) async fn do_channel_full_cycle( }, }; assert_eq!(received_amount, determined_amount_msat); - let outbound_payment = node_a.payment(&outbound_payment_id).unwrap(); + let outbound_payment = node_a.payment(&outbound_payment_id).unwrap().unwrap(); assert_eq!(outbound_payment.status, PaymentStatus::Succeeded); assert_eq!(outbound_payment.direction, PaymentDirection::Outbound); assert_eq!(outbound_payment.amount_msat, Some(determined_amount_msat)); assert!(matches!(&outbound_payment.kind, PaymentKind::Bolt11 { .. })); - let inbound_payment = node_b.payment(&inbound_payment_id).unwrap(); + let inbound_payment = node_b.payment(&inbound_payment_id).unwrap().unwrap(); assert_eq!(inbound_payment.status, PaymentStatus::Succeeded); assert_eq!(inbound_payment.direction, PaymentDirection::Inbound); assert_eq!(inbound_payment.amount_msat, Some(determined_amount_msat)); @@ -1429,12 +1429,12 @@ pub(crate) async fn do_channel_full_cycle( let received_payment_id = expect_payment_received_event!(node_b, claimable_amount_msat); assert_eq!(received_payment_id, manual_payment_id); expect_payment_successful_event!(node_a, outbound_manual_payment_id, None); - let outbound_manual_payment = node_a.payment(&outbound_manual_payment_id).unwrap(); + let outbound_manual_payment = node_a.payment(&outbound_manual_payment_id).unwrap().unwrap(); assert_eq!(outbound_manual_payment.status, PaymentStatus::Succeeded); assert_eq!(outbound_manual_payment.direction, PaymentDirection::Outbound); assert_eq!(outbound_manual_payment.amount_msat, Some(invoice_amount_3_msat)); assert!(matches!(&outbound_manual_payment.kind, PaymentKind::Bolt11 { .. })); - let manual_payment = node_b.payment(&manual_payment_id).unwrap(); + let manual_payment = node_b.payment(&manual_payment_id).unwrap().unwrap(); assert_eq!(manual_payment.status, PaymentStatus::Succeeded); assert_eq!(manual_payment.direction, PaymentDirection::Inbound); assert_eq!(manual_payment.amount_msat, Some(invoice_amount_3_msat)); @@ -1462,12 +1462,13 @@ pub(crate) async fn do_channel_full_cycle( assert_ne!(manual_fail_payment_id.0, manual_fail_payment_hash.0); node_b.bolt11_payment().fail_for_id(manual_fail_payment_id).unwrap(); expect_event!(node_a, PaymentFailed); - let outbound_manual_fail_payment = node_a.payment(&outbound_manual_fail_payment_id).unwrap(); + let outbound_manual_fail_payment = + node_a.payment(&outbound_manual_fail_payment_id).unwrap().unwrap(); assert_eq!(outbound_manual_fail_payment.status, PaymentStatus::Failed); assert_eq!(outbound_manual_fail_payment.direction, PaymentDirection::Outbound); assert_eq!(outbound_manual_fail_payment.amount_msat, Some(invoice_amount_4_msat)); assert!(matches!(&outbound_manual_fail_payment.kind, PaymentKind::Bolt11 { .. })); - let manual_fail_payment = node_b.payment(&manual_fail_payment_id).unwrap(); + let manual_fail_payment = node_b.payment(&manual_fail_payment_id).unwrap().unwrap(); assert_eq!(manual_fail_payment.status, PaymentStatus::Failed); assert_eq!(manual_fail_payment.direction, PaymentDirection::Inbound); assert_eq!(manual_fail_payment.amount_msat, Some(invoice_amount_4_msat)); @@ -1497,13 +1498,13 @@ pub(crate) async fn do_channel_full_cycle( }, }; assert_eq!(received_keysend_amount, keysend_amount_msat); - let keysend_payment = node_a.payment(&keysend_payment_id).unwrap(); + let keysend_payment = node_a.payment(&keysend_payment_id).unwrap().unwrap(); assert_eq!(keysend_payment.status, PaymentStatus::Succeeded); assert_eq!(keysend_payment.direction, PaymentDirection::Outbound); assert_eq!(keysend_payment.amount_msat, Some(keysend_amount_msat)); assert!(matches!(&keysend_payment.kind, PaymentKind::Spontaneous { .. })); assert_eq!(received_custom_records, &custom_tlvs); - let received_keysend_payment = node_b.payment(&received_keysend_payment_id).unwrap(); + let received_keysend_payment = node_b.payment(&received_keysend_payment_id).unwrap().unwrap(); assert_eq!(received_keysend_payment.status, PaymentStatus::Succeeded); assert_eq!(received_keysend_payment.direction, PaymentDirection::Inbound); assert_eq!(received_keysend_payment.amount_msat, Some(keysend_amount_msat)); diff --git a/tests/integration_tests_rust.rs b/tests/integration_tests_rust.rs index c1e973091..c9fcdcbcb 100644 --- a/tests/integration_tests_rust.rs +++ b/tests/integration_tests_rust.rs @@ -1067,7 +1067,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, .. } => { @@ -1077,7 +1077,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, .. } => { @@ -1109,7 +1109,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); @@ -1119,7 +1119,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); @@ -1208,7 +1208,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, .. } => { @@ -1234,7 +1234,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, .. } => { @@ -2421,7 +2421,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())]) @@ -2459,7 +2460,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, @@ -2534,7 +2535,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 { .. }, .. } => { @@ -2595,7 +2596,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 { @@ -2658,7 +2659,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, @@ -2681,7 +2682,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, @@ -3521,7 +3522,7 @@ async fn do_lsps2_client_service_integration(client_trusts_lsp: bool) { expect_payment_successful_event!(payer_node, payer_payment_id, None); let client_payment_id = expect_payment_received_event!(client_node, expected_received_amount_msat); - 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)); @@ -3586,7 +3587,10 @@ async fn do_lsps2_client_service_integration(client_trusts_lsp: bool) { expected_received_amount_msat ); assert_ne!(client_payment_id.0, manual_payment_hash.0); - assert_eq!(client_node.payment(&client_payment_id).unwrap().amount_msat, Some(jit_amount_msat)); + assert_eq!( + client_node.payment(&client_payment_id).unwrap().unwrap().amount_msat, + Some(jit_amount_msat) + ); println!("Claiming payment!"); client_node .bolt11_payment() @@ -3598,7 +3602,7 @@ async fn do_lsps2_client_service_integration(client_trusts_lsp: bool) { let received_payment_id = expect_payment_received_event!(client_node, expected_received_amount_msat); assert_eq!(received_payment_id, client_payment_id); - 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)); @@ -3645,7 +3649,10 @@ async fn do_lsps2_client_service_integration(client_trusts_lsp: bool) { client_node.bolt11_payment().fail_for_id(client_payment_id).unwrap(); expect_event!(payer_node, PaymentFailed); - assert_eq!(client_node.payment(&client_payment_id).unwrap().status, PaymentStatus::Failed); + assert_eq!( + client_node.payment(&client_payment_id).unwrap().unwrap().status, + PaymentStatus::Failed + ); } #[tokio::test(flavor = "multi_thread", worker_threads = 1)] @@ -4114,7 +4121,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); @@ -4379,7 +4389,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 @@ -4408,7 +4418,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: {}", @@ -4432,7 +4442,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" @@ -4458,7 +4468,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 a5f1bcba6bad79c605069dd8ae75e7722fdc6cf0 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Wed, 12 Aug 2026 15:11:59 +0200 Subject: [PATCH 04/26] f Adapt upstream asynchronous reads Upstream funding-payment lifecycle changes introduced more synchronous DataStore call sites after the reader conversion was written. Update them and allow an atomic mutation to await fallible reads so cross-store status decisions remain ordered. Co-Authored-By: HAL 9000 --- src/data_store.rs | 58 +++++++++++++++++++++++++++++--- src/payment/bolt11.rs | 17 +++++----- src/payment/store.rs | 6 ++-- src/wallet/mod.rs | 34 +++++++++---------- tests/upgrade_downgrade_tests.rs | 11 +++--- 5 files changed, 89 insertions(+), 37 deletions(-) diff --git a/src/data_store.rs b/src/data_store.rs index 8e67248eb..79d4f85c1 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -211,6 +211,19 @@ where Ok(Some(new_object)) } + /// Like [`Self::mutate`], but allows the transformation to await fallible reads. + /// + /// The mutation lock remains held while `f` runs. This is useful when the new state must be + /// decided from an async read of another store without letting a concurrent writer invalidate + /// that decision. Callers must keep cross-store lock ordering consistent to avoid deadlocks. + pub(crate) async fn mutate_async(&self, id: &SO::Id, f: F) -> Result, Error> + where + F: FnOnce(Option) -> Fut, + Fut: Future, Error>>, + { + self.mutate_with(id, f).await + } + /// Returns all stored objects matching `f`. /// /// The async mutation lock serializes writers, but this synchronous reader cannot wait on it. @@ -468,7 +481,7 @@ mod tests { .await; assert_eq!(Ok(Some(object)), result); - assert_eq!(Some(object), data_store.get(&id)); + assert_eq!(Some(object), data_store.get(&id).await.unwrap()); let store_key = id.encode_to_hex_str(); assert!(KVStore::read(&*store, &primary_namespace, &secondary_namespace, &store_key) .await @@ -499,7 +512,42 @@ mod tests { .await; let expected = TestObject { id, data: [24u8, 23u8, 23u8] }; assert_eq!(Ok(Some(expected)), result); - assert_eq!(Some(expected), data_store.get(&id)); + assert_eq!(Some(expected), data_store.get(&id).await.unwrap()); + } + + #[tokio::test] + async fn mutate_async_awaits_fallible_reads() { + let store: Arc = Arc::new(DynStoreWrapper(InMemoryStore::new())); + let logger = Arc::new(TestLogger::new()); + let id = TestObjectId { id: [42u8; 4] }; + let other_id = TestObjectId { id: [43u8; 4] }; + let existing_object = TestObject { id, data: [23u8; 3] }; + let other_object = TestObject { id: other_id, data: [24u8; 3] }; + let data_store: DataStore> = DataStore::new( + vec![existing_object], + "datastore_test_primary".to_string(), + "datastore_test_secondary".to_string(), + Arc::clone(&store), + Arc::clone(&logger), + ); + let other_store: DataStore> = DataStore::new( + vec![other_object], + "other_datastore_test_primary".to_string(), + "other_datastore_test_secondary".to_string(), + store, + logger, + ); + + let result = data_store + .mutate_async(&id, |existing| async move { + let mut updated = existing.unwrap(); + updated.data = other_store.get(&other_id).await?.unwrap().data; + Ok(Some(updated)) + }) + .await; + let expected = TestObject { id, data: [24u8; 3] }; + assert_eq!(Ok(Some(expected)), result); + assert_eq!(Some(expected), data_store.get(&id).await.unwrap()); } #[tokio::test] @@ -544,7 +592,7 @@ mod tests { }) .await; assert_eq!(Ok(None), result); - assert_eq!(Some(existing_object), data_store.get(&id)); + assert_eq!(Some(existing_object), data_store.get(&id).await.unwrap()); } #[tokio::test] @@ -558,7 +606,7 @@ mod tests { Err(Error::PersistenceFailed), data_store.mutate(&existing_id, |_| Some(changed)).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] }; @@ -566,7 +614,7 @@ mod tests { Err(Error::PersistenceFailed), data_store.mutate(&new_id, |_| Some(new_object)).await ); - assert!(data_store.get(&new_id).is_none()); + assert!(data_store.get(&new_id).await.unwrap().is_none()); } #[tokio::test] diff --git a/src/payment/bolt11.rs b/src/payment/bolt11.rs index 208c677c3..5f0a70c2c 100644 --- a/src/payment/bolt11.rs +++ b/src/payment/bolt11.rs @@ -489,14 +489,15 @@ impl Bolt11Payment { /// /// [`PaymentClaimable`]: crate::Event::PaymentClaimable pub fn fail_for_id(&self, payment_id: PaymentId) -> Result<(), Error> { - let details = self.payment_store.get(&payment_id).ok_or_else(|| { - log_error!( - self.logger, - "Failed to manually fail unknown payment with ID {}", - payment_id, - ); - Error::InvalidPaymentId - })?; + let details = + self.runtime.block_on(self.payment_store.get(&payment_id))?.ok_or_else(|| { + log_error!( + self.logger, + "Failed to manually fail unknown payment with ID {}", + payment_id, + ); + Error::InvalidPaymentId + })?; let payment_hash = match details.kind { PaymentKind::Bolt11 { hash, .. } => hash, diff --git a/src/payment/store.rs b/src/payment/store.rs index cb76e78cc..15eb3b815 100644 --- a/src/payment/store.rs +++ b/src/payment/store.rs @@ -1379,7 +1379,7 @@ mod tests { // wallet sync already advanced the record — downgrades it. let store = new_store(vec![advanced.clone()]); store.insert_or_update(fresh.clone()).await.unwrap(); - let downgraded = store.get(&id).unwrap(); + let downgraded = store.get(&id).await.unwrap().unwrap(); assert_eq!( downgraded.status, PaymentStatus::Pending, @@ -1402,7 +1402,7 @@ mod tests { }) .await; assert!(matches!(written, Ok(Some(_))), "the reclassification must merge"); - let merged = store.get(&id).unwrap(); + let merged = store.get(&id).await.unwrap().unwrap(); assert_eq!(merged.status, PaymentStatus::Succeeded); assert!(matches!( merged.kind, @@ -1428,7 +1428,7 @@ mod tests { }) .await; assert!(matches!(written, Ok(Some(_))), "the fresh details must insert"); - let inserted = store.get(&id).unwrap(); + let inserted = store.get(&id).await.unwrap().unwrap(); assert_eq!(inserted.status, PaymentStatus::Pending); assert!(matches!( inserted.kind, diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index 0c6f6b413..6132fe23b 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -1802,12 +1802,13 @@ impl Wallet { // is ordered before the removal, which then also deletes anything inserted here. A // status read taken before this write goes stale when graduation lands in between, and // would re-index the graduated payment. + let payment_store = Arc::clone(&self.payment_store); self.pending_payment_store - .mutate(&id, |existing| { + .mutate_async(&id, move |existing| async move { // The record was written above and payment records are never removed, so absence // means the write failed out; fall back to the fresh details. - let recorded = self.payment_store.get(&id).unwrap_or(details); - match existing { + let recorded = payment_store.get(&id).await?.unwrap_or(details); + Ok(match existing { // The inserted entry embeds the post-write record rather than the fresh // details, so a confirmation wallet sync already recorded keeps driving // graduation. @@ -1821,17 +1822,16 @@ impl Wallet { // transaction before it was classified (its arms and this write pair // serialize on the cross-store lock, so nothing lands in between): merge // only the classification into the existing entry. - Some(entry) => { + Some(mut entry) => { let pending_update = PendingPaymentDetailsUpdate { id, payment_update: Some(update), conflicting_txids: None, candidates, }; - let mut updated = entry.clone(); - updated.update(pending_update).then_some(updated) + entry.update(pending_update).then_some(entry) }, - } + }) }) .await?; Ok(()) @@ -3831,7 +3831,7 @@ mod tests { let event = WalletEvent::ChainTipChanged { old_tip: block_id(9), new_tip: block_id(10) }; wallet.update_payment_store(vec![event]).await.unwrap(); - let payment = wallet.payment_store.get(&payment_id).unwrap(); + let payment = wallet.payment_store.get(&payment_id).await.unwrap().unwrap(); assert_eq!(payment.status, PaymentStatus::Succeeded); assert_eq!( payment.amount_msat, @@ -3840,7 +3840,7 @@ mod tests { ); assert_eq!(payment.fee_paid_msat, Some(999)); assert!(payment.latest_update_timestamp > 0, "the graduation write must timestamp"); - assert!(wallet.pending_payment_store.get(&payment_id).is_none()); + assert!(wallet.pending_payment_store.get(&payment_id).await.unwrap().is_none()); } /// When the live record has diverged from the pending-store snapshot — here the snapshot @@ -3881,7 +3881,7 @@ mod tests { let event = WalletEvent::ChainTipChanged { old_tip: block_id(9), new_tip: block_id(10) }; wallet.update_payment_store(vec![event]).await.unwrap(); - let payment = wallet.payment_store.get(&payment_id).unwrap(); + let payment = wallet.payment_store.get(&payment_id).await.unwrap().unwrap(); assert_eq!( payment.status, PaymentStatus::Pending, @@ -3892,7 +3892,7 @@ mod tests { PaymentKind::Onchain { status: ConfirmationStatus::Unconfirmed, .. } )); assert!( - wallet.pending_payment_store.get(&payment_id).is_some(), + wallet.pending_payment_store.get(&payment_id).await.unwrap().is_some(), "the entry must survive for future events to drive" ); } @@ -3932,9 +3932,9 @@ mod tests { // The first candidate resolves via the txid-derived id and the active candidate via the // record's current txid; the middle one must resolve through the candidate history. - assert_eq!(wallet.find_payment_by_txid(txid1), Some(payment_id)); - assert_eq!(wallet.find_payment_by_txid(txid3), Some(payment_id)); - assert_eq!(wallet.find_payment_by_txid(txid2), Some(payment_id)); + assert_eq!(wallet.find_payment_by_txid(txid1).await.unwrap(), Some(payment_id)); + assert_eq!(wallet.find_payment_by_txid(txid3).await.unwrap(), Some(payment_id)); + assert_eq!(wallet.find_payment_by_txid(txid2).await.unwrap(), Some(payment_id)); } /// A funding-typed broadcast that doesn't touch the on-chain wallet must not be recorded. @@ -4134,7 +4134,7 @@ mod tests { // Liveness sanity only (both pre- and post-fix stall here): while classification is // parked, no second record may have been committed. tokio::time::sleep(Duration::from_millis(250)).await; - assert!(wallet.payment_store.list_filter(|_| true).len() <= 1); + assert!(wallet.payment_store.list_filter(|_| true).await.len() <= 1); drop(gate_guard); classification.await.unwrap().unwrap(); @@ -4143,7 +4143,7 @@ mod tests { // Both writers converge on the classified record: the confirmation refreshes it in // place with the confirmed candidate's figures rather than minting a second record // keyed by the event txid. - let payments = wallet.payment_store.list_filter(|_| true); + let payments = wallet.payment_store.list_filter(|_| true).await; assert_eq!(payments.len(), 1, "the confirmation must not mint a duplicate record"); let payment = &payments[0]; assert_eq!(payment.id, payment_id); @@ -4207,7 +4207,7 @@ mod tests { // Both writers converge on one record carrying the classification: the generic // fallback must not clobber the contribution-derived figures with its wallet-derived // view of the transaction. - let payments = wallet.payment_store.list_filter(|_| true); + let payments = wallet.payment_store.list_filter(|_| true).await; assert_eq!(payments.len(), 1); let payment = &payments[0]; assert_eq!(payment.id, payment_id); diff --git a/tests/upgrade_downgrade_tests.rs b/tests/upgrade_downgrade_tests.rs index 84d528944..7af7c6030 100644 --- a/tests/upgrade_downgrade_tests.rs +++ b/tests/upgrade_downgrade_tests.rs @@ -142,7 +142,10 @@ async fn v0_7_for_hash_payments_can_be_manually_resolved_after_upgrade() { let received_payment_id = expect_payment_received_event!(receiver, CLAIM_AMOUNT_MSAT); assert_eq!(received_payment_id, claim_payment_id); expect_payment_successful_event!(payer, payer_claim_id, None); - assert_eq!(receiver.payment(&claim_payment_id).unwrap().status, PaymentStatus::Succeeded); + assert_eq!( + receiver.payment(&claim_payment_id).unwrap().unwrap().status, + PaymentStatus::Succeeded + ); let fail_invoice = CurrentBolt11Invoice::from_str(&fail_invoice).unwrap(); let payer_fail_id = payer.bolt11_payment().send(&fail_invoice, None).unwrap(); @@ -151,8 +154,8 @@ async fn v0_7_for_hash_payments_can_be_manually_resolved_after_upgrade() { assert_eq!(fail_payment_id, PaymentId(fail_hash.0)); receiver.bolt11_payment().fail_for_id(fail_payment_id).unwrap(); expect_event!(payer, PaymentFailed); - assert_eq!(payer.payment(&payer_fail_id).unwrap().status, PaymentStatus::Failed); - assert_eq!(receiver.payment(&fail_payment_id).unwrap().status, PaymentStatus::Failed); + assert_eq!(payer.payment(&payer_fail_id).unwrap().unwrap().status, PaymentStatus::Failed); + assert_eq!(receiver.payment(&fail_payment_id).unwrap().unwrap().status, PaymentStatus::Failed); payer.stop().unwrap(); receiver.stop().unwrap(); @@ -200,7 +203,7 @@ fn payment_hash_from_preimage(preimage: [u8; 32]) -> PaymentHash { fn assert_legacy_manual_payment(node: &Node, payment_hash: PaymentHash, amount_msat: u64) { let payment_id = PaymentId(payment_hash.0); - let payment = node.payment(&payment_id).unwrap(); + let payment = node.payment(&payment_id).unwrap().unwrap(); assert_eq!(payment.id, payment_id); assert_eq!(payment.amount_msat, Some(amount_msat)); assert_eq!(payment.direction, PaymentDirection::Inbound); From bc1c4c7a66ce62601e189406edadd258b8d4b3e9 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Wed, 12 Aug 2026 15:34:56 +0200 Subject: [PATCH 05/26] f Do not replay sent payments for logging A read used only to include the amount in the success log can fail after the authoritative payment update succeeds. Log that failure and continue emitting PaymentSuccessful instead of replaying the completed event. Co-Authored-By: HAL 9000 --- src/event.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/event.rs b/src/event.rs index 8808641c6..10f538f32 100644 --- a/src/event.rs +++ b/src/event.rs @@ -1487,8 +1487,12 @@ where }, Ok(None) => {}, Err(e) => { - log_error!(self.logger, "Failed to access payment store: {}", e); - return Err(ReplayEvent()); + log_error!( + self.logger, + "Failed to read payment {} for success logging: {}", + payment_id, + e + ); }, }; let event = Event::PaymentSuccessful { From 32823936b8966a73608d440b3cbaa1052c08b9a3 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 13 Aug 2026 19:05:22 +0200 Subject: [PATCH 06/26] f Adapt later asynchronous reads Upstream funding-rebroadcast work added more DataStore reads after the async conversion was written. Await those reads and propagate failures so the branch continues to compile when rebased. Co-Authored-By: HAL 9000 --- src/wallet/mod.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index 6132fe23b..eb8036b82 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -1588,7 +1588,7 @@ impl Wallet { // downgrade, leaving no trace that a re-broadcast arrived. Log the arrival so tests can // observe the traffic. The read cannot go stale: only the broadcast loop writes // interactive-funding classifications, and it runs this classification too. - if let Some(current) = self.payment_store.get(&payment_id) { + if let Some(current) = self.payment_store.get(&payment_id).await? { if matches!( current.kind, PaymentKind::Onchain { @@ -3956,8 +3956,8 @@ mod tests { // No inputs or outputs involve the wallet: nothing to record. wallet.classify_funding(&dummy_tx(), &channels, tx_type.clone()).await.unwrap(); - assert!(wallet.payment_store.list_filter(|_| true).is_empty()); - assert!(wallet.pending_payment_store.list_filter(|_| true).is_empty()); + assert!(wallet.payment_store.list_filter(|_| true).await.is_empty()); + assert!(wallet.pending_payment_store.list_filter(|_| true).await.is_empty()); // A computable fee is not wallet participation. The wallet can resolve a splice's shared // input whenever the previous funding transaction touched it (e.g. it funded the original @@ -3980,7 +3980,7 @@ mod tests { }], }; wallet.classify_funding(&splice_tx, &channels, tx_type.clone()).await.unwrap(); - assert!(wallet.payment_store.list_filter(|_| true).is_empty()); + assert!(wallet.payment_store.list_filter(|_| true).await.is_empty()); // Control: a funding transaction the wallet participates in is still recorded. let script_pubkey = wallet @@ -3997,7 +3997,7 @@ mod tests { output: vec![TxOut { value: Amount::from_sat(10_000), script_pubkey }], }; wallet.classify_funding(&funded_tx, &channels, tx_type).await.unwrap(); - let payments = wallet.payment_store.list_filter(|_| true); + let payments = wallet.payment_store.list_filter(|_| true).await; assert_eq!(payments.len(), 1); assert_eq!(payments[0].id, PaymentId(funded_tx.compute_txid().to_byte_array())); } @@ -4046,8 +4046,8 @@ mod tests { let channels = vec![(counterparty_node_id, ChannelId([7u8; 32]))]; let tx_type = TransactionType::Funding { channels: vec![] }; - let assert_unchanged = |confirmed: bool| { - let payments = wallet.payment_store.list_filter(|_| true); + async fn assert_unchanged(wallet: &Wallet, payment_id: PaymentId, confirmed: bool) { + let payments = wallet.payment_store.list_filter(|_| true).await; assert_eq!(payments.len(), 1, "the rebroadcast must not mint a second record"); let payment = &payments[0]; assert_eq!(payment.id, payment_id); @@ -4061,10 +4061,10 @@ mod tests { } => assert_eq!(matches!(status, ConfirmationStatus::Confirmed { .. }), confirmed), kind => panic!("unexpected kind {:?}", kind), } - }; + } wallet.classify_funding(&tx, &channels, tx_type.clone()).await.unwrap(); - assert_unchanged(false); + assert_unchanged(&wallet, payment_id, false).await; // Confirm the record, then replay the rebroadcast: a monitor-update completion can race // wallet sync around confirmation. @@ -4076,7 +4076,7 @@ mod tests { }; wallet.update_payment_store(vec![event]).await.unwrap(); wallet.classify_funding(&tx, &channels, tx_type).await.unwrap(); - assert_unchanged(true); + assert_unchanged(&wallet, payment_id, true).await; } /// Barrier test, classification-first ordering: wallet sync's confirmation handling must From c679b8208430d1ed606dc9bea71cb5de19454130 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 6 Aug 2026 12:57:30 +0200 Subject: [PATCH 07/26] 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 79d4f85c1..037e8154f 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -46,7 +46,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, @@ -65,7 +70,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, @@ -74,7 +79,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"); @@ -85,7 +90,7 @@ where /// Like [`Self::insert`], but when an entry with the object's id already exists, merges the /// object's full update ([`StorableObject::to_update`]) into it instead of replacing it. 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 = { @@ -115,7 +120,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(); @@ -144,16 +149,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"); @@ -225,11 +227,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::>() } @@ -266,20 +265,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; @@ -391,6 +390,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 ba74ad70d2fc1638e9436578793a89f8975d3372 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 6 Aug 2026 13:05:49 +0200 Subject: [PATCH 08/26] 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 | 959 +++++++++++++++++++++++++++++++++++++++++----- src/types.rs | 6 +- 3 files changed, 868 insertions(+), 103 deletions(-) diff --git a/src/builder.rs b/src/builder.rs index dc41aef1a..6e1da8bf8 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; @@ -1491,6 +1492,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), @@ -1745,8 +1747,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 037e8154f..7ab1a87f7 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -5,11 +5,14 @@ // 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::future::Future; +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}; @@ -26,7 +29,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; } @@ -41,137 +44,350 @@ 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) } /// Like [`Self::insert`], but when an entry with the object's id already exists, merges the /// object's full update ([`StorableObject::to_update`]) into it instead of replacing it. + /// + /// 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) } @@ -182,9 +398,9 @@ where /// one critical section of the mutation lock, so no concurrent writer can land in between — /// unlike a separate [`Self::get`] followed by an insert or update. /// - /// The closure runs on a clone of the entry with the in-memory map lock released, so it may - /// freely read this store or others (reads see the pre-mutation state) without ordering map - /// locks against each other. Keep it cheap and non-blocking. + /// The closure runs on a clone of the entry with the in-memory cache lock released, so it may + /// freely inspect other in-memory state without ordering cache locks against each other. Keep + /// it cheap and non-blocking. /// /// Returns the written object, or `None` when the closure declined to write. pub(crate) async fn mutate) -> Option>( @@ -198,9 +414,9 @@ where F: FnOnce(Option) -> Fut, Fut: Future, Error>>, { - let _guard = self.mutation_lock.lock().await; + let _guard = self.mutation_lock.write().await; - let current = self.objects.lock().expect("lock").get(id).cloned(); + let current = self.lookup(id).await?; let new_object = match f(current).await? { Some(new_object) => new_object, None => return Ok(None), @@ -208,8 +424,7 @@ where debug_assert!(new_object.id() == *id, "mutate closure must not change the object's id"); self.persist(&new_object).await?; - let mut locked_objects = self.objects.lock().expect("lock"); - locked_objects.insert(new_object.id(), new_object.clone()); + self.cache.lock().expect("lock").insert(new_object.id(), new_object.clone()); Ok(Some(new_object)) } @@ -226,10 +441,99 @@ where self.mutate_with(id, f).await } - /// 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> { @@ -264,15 +568,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}; @@ -285,6 +609,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], @@ -300,6 +652,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 { @@ -311,6 +666,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 { @@ -322,22 +684,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; @@ -383,8 +752,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, ) @@ -455,13 +825,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, )); @@ -491,10 +862,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), @@ -512,7 +884,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) @@ -526,22 +898,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 @@ -716,10 +1088,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 @@ -727,7 +1099,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()); } @@ -735,7 +1107,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); @@ -745,10 +1117,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()); } @@ -756,10 +1128,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 add7c26f0fc5f7c32adcac2b733a06bcf12db918 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Wed, 12 Aug 2026 15:14:46 +0200 Subject: [PATCH 09/26] f Adapt upstream store construction Upstream funding-payment tests still constructed DataStore without an explicit cache policy and inspected the former map field. Keep those tests on the unbounded policy and update them for the cache-backed representation. Co-Authored-By: HAL 9000 --- src/data_store.rs | 35 ++++++++++++++++++++--------------- src/payment/store.rs | 3 ++- src/wallet/mod.rs | 4 ++++ 3 files changed, 26 insertions(+), 16 deletions(-) diff --git a/src/data_store.rs b/src/data_store.rs index 7ab1a87f7..b7de5f666 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -933,6 +933,7 @@ mod tests { let secondary_namespace = "datastore_test_secondary".to_string(); let data_store: DataStore> = DataStore::new( Vec::new(), + KeepAllEntries, primary_namespace.clone(), secondary_namespace.clone(), Arc::clone(&store), @@ -940,7 +941,7 @@ mod tests { ); let id = TestObjectId { id: [42u8; 4] }; - let object = TestObject { id, data: [23u8; 3] }; + let object = TestObject::new(id, [23u8; 3]); let result = data_store .mutate(&id, |existing| { assert!(existing.is_none()); @@ -961,9 +962,10 @@ mod tests { let store: Arc = Arc::new(DynStoreWrapper(InMemoryStore::new())); let logger = Arc::new(TestLogger::new()); let id = TestObjectId { id: [42u8; 4] }; - let existing_object = TestObject { id, data: [23u8; 3] }; + let existing_object = TestObject::new(id, [23u8; 3]); let data_store: DataStore> = DataStore::new( vec![existing_object], + KeepAllEntries, "datastore_test_primary".to_string(), "datastore_test_secondary".to_string(), store, @@ -978,7 +980,7 @@ mod tests { Some(new_object) }) .await; - let expected = TestObject { id, data: [24u8, 23u8, 23u8] }; + let expected = TestObject::new(id, [24u8, 23u8, 23u8]); assert_eq!(Ok(Some(expected)), result); assert_eq!(Some(expected), data_store.get(&id).await.unwrap()); } @@ -989,10 +991,11 @@ mod tests { let logger = Arc::new(TestLogger::new()); let id = TestObjectId { id: [42u8; 4] }; let other_id = TestObjectId { id: [43u8; 4] }; - let existing_object = TestObject { id, data: [23u8; 3] }; - let other_object = TestObject { id: other_id, data: [24u8; 3] }; + let existing_object = TestObject::new(id, [23u8; 3]); + let other_object = TestObject::new(other_id, [24u8; 3]); let data_store: DataStore> = DataStore::new( vec![existing_object], + KeepAllEntries, "datastore_test_primary".to_string(), "datastore_test_secondary".to_string(), Arc::clone(&store), @@ -1000,6 +1003,7 @@ mod tests { ); let other_store: DataStore> = DataStore::new( vec![other_object], + KeepAllEntries, "other_datastore_test_primary".to_string(), "other_datastore_test_secondary".to_string(), store, @@ -1013,19 +1017,20 @@ mod tests { Ok(Some(updated)) }) .await; - let expected = TestObject { id, data: [24u8; 3] }; + let expected = TestObject::new(id, [24u8; 3]); assert_eq!(Ok(Some(expected)), result); assert_eq!(Some(expected), data_store.get(&id).await.unwrap()); } #[tokio::test] - async fn mutate_runs_the_closure_without_the_map_lock() { + async fn mutate_runs_the_closure_without_the_cache_lock() { let store: Arc = Arc::new(DynStoreWrapper(InMemoryStore::new())); let logger = Arc::new(TestLogger::new()); let id = TestObjectId { id: [42u8; 4] }; - let existing_object = TestObject { id, data: [23u8; 3] }; + let existing_object = TestObject::new(id, [23u8; 3]); let data_store: DataStore> = DataStore::new( vec![existing_object], + KeepAllEntries, "datastore_test_primary".to_string(), "datastore_test_secondary".to_string(), store, @@ -1033,13 +1038,13 @@ mod tests { ); // Closures gate cross-store decisions on reads of other stores, which lock their own - // in-memory maps. Holding this store's map lock across the closure would order it - // before theirs and invite lock-order inversions, so the closure must run with the map + // in-memory caches. Holding this store's cache lock across the closure would order it + // before theirs and invite lock-order inversions, so the closure must run with the cache // lock released. let result = data_store .mutate(&id, |existing| { assert_eq!(Some(&existing_object), existing); - assert!(data_store.objects.try_lock().is_ok()); + assert!(data_store.cache.try_lock().is_ok()); None }) .await; @@ -1049,7 +1054,7 @@ mod tests { #[tokio::test] async fn mutate_persists_nothing_when_closure_declines() { let id = TestObjectId { id: [42u8; 4] }; - let existing_object = TestObject { id, data: [23u8; 3] }; + let existing_object = TestObject::new(id, [23u8; 3]); let data_store = new_failing_data_store(vec![existing_object]); // Returning `None` must not attempt a write (the store fails all writes) nor touch memory. @@ -1066,10 +1071,10 @@ mod tests { #[tokio::test] async fn mutate_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 changed = TestObject { id: existing_id, data: [24u8; 3] }; + let changed = TestObject::new(existing_id, [24u8; 3]); assert_eq!( Err(Error::PersistenceFailed), data_store.mutate(&existing_id, |_| Some(changed)).await @@ -1077,7 +1082,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.mutate(&new_id, |_| Some(new_object)).await diff --git a/src/payment/store.rs b/src/payment/store.rs index 15eb3b815..c706706c8 100644 --- a/src/payment/store.rs +++ b/src/payment/store.rs @@ -1320,7 +1320,7 @@ mod tests { use std::str::FromStr; use std::sync::Arc; - use crate::data_store::DataStore; + use crate::data_store::{DataStore, KeepAllEntries}; use crate::io::test_utils::InMemoryStore; use crate::types::{DynStore, DynStoreWrapper}; @@ -1368,6 +1368,7 @@ mod tests { let logger = Arc::new(TestLogger::new()); DataStore::>::new( seed, + KeepAllEntries, "payment_test_primary".to_string(), "payment_test_secondary".to_string(), store, diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index eb8036b82..80a8fc5b7 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -54,6 +54,8 @@ use lightning_invoice::RawBolt11Invoice; use persist::KVStoreWalletPersister; use crate::config::{Config, ADDRESS_POOL_SIZE}; +#[cfg(test)] +use crate::data_store::KeepAllEntries; use crate::data_store::StorableObject; use crate::fee_estimator::{ConfirmationTarget, FeeEstimator, OnchainFeeEstimator}; use crate::logger::{log_debug, log_error, log_info, log_trace, LdkLogger, Logger}; @@ -2819,6 +2821,7 @@ mod tests { .unwrap(); let payment_store = Arc::new(PaymentStore::new( Vec::new(), + KeepAllEntries, PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE.to_string(), PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE.to_string(), Arc::clone(&store), @@ -2826,6 +2829,7 @@ mod tests { )); let pending_payment_store = Arc::new(PendingPaymentStore::new( Vec::new(), + KeepAllEntries, PENDING_PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE.to_string(), PENDING_PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE.to_string(), Arc::clone(&store), From dd515bbd8c139e4a9595e52344d4710a17a29ad7 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Wed, 12 Aug 2026 15:37:51 +0200 Subject: [PATCH 10/26] f Avoid reads before inserts The replacement result only fed diagnostics after the overwrite. Drop it so bounded stores persist objects without first reading the backend. Co-Authored-By: HAL 9000 --- src/data_store.rs | 51 ++++++++++-------- src/event.rs | 135 ++++++++++++++-------------------------------- 2 files changed, 68 insertions(+), 118 deletions(-) diff --git a/src/data_store.rs b/src/data_store.rs index b7de5f666..bd0954297 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -293,18 +293,13 @@ where } /// 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 { + pub(crate) async fn insert(&self, object: SO) -> Result<(), Error> { 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?; self.cache.lock().expect("lock").insert(id, object); - Ok(replaced) + Ok(()) } /// Like [`Self::insert`], but when an entry with the object's id already exists, merges the @@ -854,7 +849,7 @@ mod tests { ); release_write.notify_one(); - assert_eq!(Ok(true), writer.await.unwrap()); + assert_eq!(Ok(()), writer.await.unwrap()); assert_eq!(Some(new_object), data_store.get(&id).await.unwrap()); } @@ -883,18 +878,18 @@ mod tests { .await .is_err()); - // Check we successfully store an object and return `false` + // Check we successfully store an object. let object = TestObject::new(id, [23u8; 3]); - assert_eq!(Ok(false), data_store.insert(object.clone()).await); + assert_eq!(Ok(()), 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) .await .is_ok()); - // Test re-insertion returns `true` + // Test re-insertion overwrites the object. let mut override_object = object.clone(); override_object.data = [24u8; 3]; - assert_eq!(Ok(true), data_store.insert(override_object).await); + assert_eq!(Ok(()), data_store.insert(override_object).await); assert_eq!(Some(override_object), data_store.get(&id).await.unwrap()); // Check update returns `Updated` @@ -1140,11 +1135,11 @@ mod tests { 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. + /// A store that counts how often it is asked to read, write, or list. struct CountingStore { inner: InMemoryStore, reads: Arc, + writes: Arc, lists: Arc, } @@ -1159,6 +1154,7 @@ mod tests { fn write( &self, primary_namespace: &str, secondary_namespace: &str, key: &str, buf: Vec, ) -> impl std::future::Future> + 'static + Send { + self.writes.fetch_add(1, Ordering::Relaxed); self.inner.write(primary_namespace, secondary_namespace, key, buf) } @@ -1273,10 +1269,12 @@ mod tests { #[tokio::test] async fn keep_all_never_reads_from_the_store() { let reads = Arc::new(AtomicUsize::new(0)); + let writes = 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), + writes: Arc::clone(&writes), lists: Arc::clone(&lists), })); let data_store = new_data_store(kv_store, KeepAllEntries, Vec::new()); @@ -1284,7 +1282,7 @@ mod tests { 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!(Ok(()), 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()); @@ -1383,15 +1381,22 @@ mod tests { } #[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)); + async fn lru_insert_writes_without_reading() { + let reads = Arc::new(AtomicUsize::new(0)); + let writes = 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), + writes: Arc::clone(&writes), + lists, + })); + let data_store = new_data_store(kv_store, keep_lru(1), Vec::new()); + + data_store.insert(TestObject::new(test_id(1), [1u8; 3])).await.unwrap(); - // 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); + assert_eq!(0, reads.load(Ordering::Relaxed), "insert must not read from the store"); + assert_eq!(1, writes.load(Ordering::Relaxed), "insert must write exactly once"); } #[tokio::test] diff --git a/src/event.rs b/src/event.rs index 10f538f32..0a3569755 100644 --- a/src/event.rs +++ b/src/event.rs @@ -995,25 +995,14 @@ where PaymentStatus::Pending, ); - match self.payment_store.insert(payment.clone()).await { - Ok(false) => (), - Ok(true) => { - log_error!( - self.logger, - "Bolt11InvoicePayment with ID {} was previously known", - payment_id, - ); - debug_assert!(false); - }, - Err(e) => { - log_error!( - self.logger, - "Failed to insert payment with ID {}: {}", - payment_id, - e - ); - return Err(ReplayEvent()); - }, + if let Err(e) = self.payment_store.insert(payment.clone()).await { + log_error!( + self.logger, + "Failed to insert payment with ID {}: {}", + payment_id, + e + ); + return Err(ReplayEvent()); } payment_info = Some(payment); } @@ -1096,25 +1085,14 @@ where PaymentStatus::Pending, ); - match self.payment_store.insert(payment).await { - Ok(false) => (), - Ok(true) => { - log_error!( - self.logger, - "Bolt11InvoicePayment with ID {} was previously known", - payment_id, - ); - debug_assert!(false); - }, - Err(e) => { - log_error!( - self.logger, - "Failed to insert payment with ID {}: {}", - payment_id, - e - ); - return Err(ReplayEvent()); - }, + if let Err(e) = self.payment_store.insert(payment).await { + log_error!( + self.logger, + "Failed to insert payment with ID {}: {}", + payment_id, + e + ); + return Err(ReplayEvent()); } } @@ -1148,25 +1126,14 @@ where PaymentStatus::Pending, ); - match self.payment_store.insert(payment).await { - Ok(false) => (), - Ok(true) => { - log_error!( - self.logger, - "Bolt12OfferPayment with ID {} was previously known", - payment_id, - ); - debug_assert!(false); - }, - Err(e) => { - log_error!( - self.logger, - "Failed to insert payment with ID {}: {}", - payment_id, - e - ); - return Err(ReplayEvent()); - }, + if let Err(e) = self.payment_store.insert(payment).await { + log_error!( + self.logger, + "Failed to insert payment with ID {}: {}", + payment_id, + e + ); + return Err(ReplayEvent()); } } payment_preimage @@ -1197,25 +1164,14 @@ where PaymentStatus::Pending, ); - match self.payment_store.insert(payment).await { - Ok(false) => (), - Ok(true) => { - log_error!( - self.logger, - "Bolt12RefundPayment with ID {} was previously known", - payment_id, - ); - debug_assert!(false); - }, - Err(e) => { - log_error!( - self.logger, - "Failed to insert payment with ID {}: {}", - payment_id, - e - ); - return Err(ReplayEvent()); - }, + if let Err(e) = self.payment_store.insert(payment).await { + log_error!( + self.logger, + "Failed to insert payment with ID {}: {}", + payment_id, + e + ); + return Err(ReplayEvent()); } } payment_preimage @@ -1236,25 +1192,14 @@ where PaymentStatus::Pending, ); - match self.payment_store.insert(payment).await { - Ok(false) => (), - Ok(true) => { - log_error!( - self.logger, - "Spontaneous payment with ID {} was previously known", - payment_id, - ); - debug_assert!(false); - }, - Err(e) => { - log_error!( - self.logger, - "Failed to insert payment with ID {}: {}", - payment_id, - e - ); - return Err(ReplayEvent()); - }, + if let Err(e) = self.payment_store.insert(payment).await { + log_error!( + self.logger, + "Failed to insert payment with ID {}: {}", + payment_id, + e + ); + return Err(ReplayEvent()); } } From b4e982f13cad655db6a9504a484738362f73575b Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Wed, 12 Aug 2026 15:39:24 +0200 Subject: [PATCH 11/26] f Avoid reads before removals Backend removal is idempotent, even when a bounded cache cannot prove an entry is absent. Call it directly so deleting an evicted entry takes one remote operation while authoritative cache misses stay local. Co-Authored-By: HAL 9000 --- src/data_store.rs | 44 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/src/data_store.rs b/src/data_store.rs index bd0954297..617d551cc 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -333,7 +333,11 @@ where pub(crate) async fn remove(&self, id: &SO::Id) -> Result<(), Error> { let _guard = self.mutation_lock.write().await; - if !self.contains(id).await? { + let known_absent = { + let cache = self.cache.lock().expect("lock"); + cache.is_authoritative() && !cache.contains(id) + }; + if known_absent { return Ok(()); } @@ -1135,11 +1139,12 @@ mod tests { assert_eq!(Some(object), data_store.get(&id).await.unwrap()); } - /// A store that counts how often it is asked to read, write, or list. + /// A store that counts how often it is asked to read, write, remove, or list. struct CountingStore { inner: InMemoryStore, reads: Arc, writes: Arc, + removes: Arc, lists: Arc, } @@ -1161,6 +1166,7 @@ mod tests { fn remove( &self, primary_namespace: &str, secondary_namespace: &str, key: &str, lazy: bool, ) -> impl std::future::Future> + 'static + Send { + self.removes.fetch_add(1, Ordering::Relaxed); self.inner.remove(primary_namespace, secondary_namespace, key, lazy) } @@ -1275,6 +1281,7 @@ mod tests { inner: InMemoryStore::new(), reads: Arc::clone(&reads), writes: Arc::clone(&writes), + removes: Arc::new(AtomicUsize::new(0)), lists: Arc::clone(&lists), })); let data_store = new_data_store(kv_store, KeepAllEntries, Vec::new()); @@ -1389,6 +1396,7 @@ mod tests { inner: InMemoryStore::new(), reads: Arc::clone(&reads), writes: Arc::clone(&writes), + removes: Arc::new(AtomicUsize::new(0)), lists, })); let data_store = new_data_store(kv_store, keep_lru(1), Vec::new()); @@ -1419,6 +1427,38 @@ mod tests { .is_err()); } + #[tokio::test] + async fn lru_remove_removes_without_reading() { + let reads = Arc::new(AtomicUsize::new(0)); + let removes = Arc::new(AtomicUsize::new(0)); + let kv_store: Arc = Arc::new(DynStoreWrapper(CountingStore { + inner: InMemoryStore::new(), + reads: Arc::clone(&reads), + writes: Arc::new(AtomicUsize::new(0)), + removes: Arc::clone(&removes), + lists: Arc::new(AtomicUsize::new(0)), + })); + let data_store = new_data_store(Arc::clone(&kv_store), keep_lru(1), Vec::new()); + let evicted_id = test_id(1); + data_store.insert(TestObject::new(evicted_id, [1u8; 3])).await.unwrap(); + data_store.insert(TestObject::new(test_id(2), [2u8; 3])).await.unwrap(); + assert!(!data_store.is_cached(&evicted_id)); + + data_store.remove(&evicted_id).await.unwrap(); + + assert_eq!(0, reads.load(Ordering::Relaxed), "remove must not read from the store"); + assert_eq!(1, removes.load(Ordering::Relaxed), "remove must execute exactly once"); + 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(); From 19e9a985504245fc7ece8139eedca1869719c3d9 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 13 Aug 2026 09:26:39 +0200 Subject: [PATCH 12/26] f Simplify cache policy internals Name the cache-mode check after the concrete policy it detects so callers do not imply a separate consistency guarantee. Remove redundant commentary and reuse the shared test namespaces. Co-Authored-By: HAL 9000 --- src/data_store.rs | 44 +++++++++++++++++--------------------------- 1 file changed, 17 insertions(+), 27 deletions(-) diff --git a/src/data_store.rs b/src/data_store.rs index 617d551cc..6b5bae003 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -162,10 +162,6 @@ impl LruCache { } /// 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), @@ -187,11 +183,7 @@ impl ObjectCache { } } - /// 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 { + fn is_keep_all(&self) -> bool { matches!(self, Self::KeepAll(_)) } @@ -230,7 +222,7 @@ impl ObjectCache { } /// Returns the *cached* objects matching `f`, which is only a complete listing of the - /// namespace if [`Self::is_authoritative`]. + /// namespace if [`Self::is_keep_all`]. fn filter bool>(&self, f: F) -> Vec { match self { Self::KeepAll(objects) => objects.values().filter(f).cloned().collect(), @@ -335,7 +327,7 @@ where let known_absent = { let cache = self.cache.lock().expect("lock"); - cache.is_authoritative() && !cache.contains(id) + cache.is_keep_all() && !cache.contains(id) }; if known_absent { return Ok(()); @@ -376,8 +368,6 @@ where let _guard = self.mutation_lock.write().await; let id = update.id(); - // 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); }; @@ -451,15 +441,15 @@ where /// /// The caller must hold `mutation_lock`. async fn lookup(&self, id: &SO::Id) -> Result, Error> { - let (cached_object, is_authoritative) = { + let (cached_object, is_keep_all) = { let mut locked_cache = self.cache.lock().expect("lock"); - (locked_cache.get(id), locked_cache.is_authoritative()) + (locked_cache.get(id), locked_cache.is_keep_all()) }; if let Some(object) = cached_object { return Ok(Some(object)); } - if is_authoritative { + if is_keep_all { return Ok(None); } @@ -475,15 +465,15 @@ where /// /// The caller must hold `mutation_lock`. async fn contains(&self, id: &SO::Id) -> Result { - let (is_cached, is_authoritative) = { + let (is_cached, is_keep_all) = { let locked_cache = self.cache.lock().expect("lock"); - (locked_cache.contains(id), locked_cache.is_authoritative()) + (locked_cache.contains(id), locked_cache.is_keep_all()) }; if is_cached { return Ok(true); } - if is_authoritative { + if is_keep_all { return Ok(false); } @@ -928,8 +918,8 @@ mod tests { async fn mutate_inserts_when_absent() { 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, @@ -965,8 +955,8 @@ mod tests { let data_store: DataStore> = DataStore::new( vec![existing_object], KeepAllEntries, - "datastore_test_primary".to_string(), - "datastore_test_secondary".to_string(), + TEST_PRIMARY_NAMESPACE.to_string(), + TEST_SECONDARY_NAMESPACE.to_string(), store, logger, ); @@ -995,8 +985,8 @@ mod tests { let data_store: DataStore> = DataStore::new( vec![existing_object], KeepAllEntries, - "datastore_test_primary".to_string(), - "datastore_test_secondary".to_string(), + TEST_PRIMARY_NAMESPACE.to_string(), + TEST_SECONDARY_NAMESPACE.to_string(), Arc::clone(&store), Arc::clone(&logger), ); @@ -1030,8 +1020,8 @@ mod tests { let data_store: DataStore> = DataStore::new( vec![existing_object], KeepAllEntries, - "datastore_test_primary".to_string(), - "datastore_test_secondary".to_string(), + TEST_PRIMARY_NAMESPACE.to_string(), + TEST_SECONDARY_NAMESPACE.to_string(), store, logger, ); From a04645aad8721ae4869f2741dedded0fc763f5f6 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 6 Aug 2026 13:07:55 +0200 Subject: [PATCH 13/26] 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 e4e0e4c9e..735b42b4e 100644 --- a/bindings/ldk_node.udl +++ b/bindings/ldk_node.udl @@ -237,6 +237,7 @@ enum NodeError { "InvalidDateTime", "InvalidFeeRate", "InvalidScriptPubKey", + "InvalidPageToken", "DuplicatePayment", "UnsupportedCurrency", "InsufficientFunds", diff --git a/src/error.rs b/src/error.rs index 1fbc5066a..9a03c446f 100644 --- a/src/error.rs +++ b/src/error.rs @@ -117,6 +117,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. @@ -204,6 +206,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 fbd520e00ab51674389f8445a4b97645cba33597 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 6 Aug 2026 13:12:53 +0200 Subject: [PATCH 14/26] 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 | 60 ++++++++++----------- tests/reorg_test.rs | 5 +- 4 files changed, 89 insertions(+), 61 deletions(-) diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 8b447fa73..288e0a189 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, @@ -440,8 +442,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) @@ -459,7 +490,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, }) @@ -478,7 +509,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, }) @@ -1121,28 +1152,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 @@ -1195,7 +1226,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 @@ -1273,24 +1304,24 @@ pub(crate) async fn do_channel_full_cycle( let outbound_payment_id = node_a.bolt11_payment().send(&invoice, None).unwrap(); assert_eq!(Err(NodeError::DuplicatePayment), node_a.bolt11_payment().send(&invoice, None)); - assert!(!node_a.list_payments_with_filter(|p| p.id == outbound_payment_id).is_empty()); + assert!(!node_a.list_payments_matching(|p| p.id == outbound_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(), 0); @@ -1510,23 +1541,19 @@ pub(crate) async fn do_channel_full_cycle( assert_eq!(received_keysend_payment.amount_msat, Some(keysend_amount_msat)); assert!(matches!(&received_keysend_payment.kind, 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(), 5 ); 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 ); @@ -1552,7 +1579,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 @@ -1574,7 +1601,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 @@ -1751,13 +1778,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 53f60d19f..88cc62102 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 c9fcdcbcb..67124eb06 100644 --- a/tests/integration_tests_rust.rs +++ b/tests/integration_tests_rust.rs @@ -30,7 +30,7 @@ use common::{ open_channel_push_amt, 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; @@ -62,7 +62,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 @@ -812,15 +812,15 @@ async fn split_underpaid_bolt11_payment() { expect_payment_successful_event!(node_b, payment_id_b, None); // The receiver records the full invoice amount; each payer records only its own half. - let receiver_payments = node_c.list_payments_with_filter(|p| p.id == receiver_payment_id); + let receiver_payments = node_c.list_payments_matching(|p| p.id == receiver_payment_id); 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)); } @@ -999,8 +999,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) } @@ -1028,10 +1028,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; @@ -1103,10 +1103,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(); @@ -1146,10 +1146,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(); @@ -1170,10 +1170,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); } @@ -2064,7 +2064,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)); @@ -2117,7 +2117,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)); @@ -2475,7 +2475,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 @@ -2829,7 +2829,7 @@ async fn simple_bolt12_send_receive() { assert!(payer_proof.offer_issuer().is_none()); assert!(payer_proof.invoice_created_at().is_none()); 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 { @@ -2856,7 +2856,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, .. } => { @@ -2894,7 +2894,7 @@ async fn simple_bolt12_send_receive() { .unwrap(); expect_payment_successful_event!(node_a, 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); @@ -2922,7 +2922,7 @@ async fn simple_bolt12_send_receive() { assert_eq!(node_a_payments.first().unwrap().amount_msat, Some(expected_amount_msat)); let node_b_payment_id = expect_payment_received_event!(node_b, expected_amount_msat); - 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); @@ -2957,7 +2957,7 @@ async fn simple_bolt12_send_receive() { let node_a_payment_id = 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) }) @@ -2966,7 +2966,7 @@ async fn simple_bolt12_send_receive() { .id; expect_payment_successful_event!(node_b, 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); @@ -2991,7 +2991,7 @@ async fn simple_bolt12_send_receive() { } assert_eq!(node_b_payments.first().unwrap().amount_msat, Some(overpaid_amount)); - 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); @@ -3711,7 +3711,7 @@ async fn spontaneous_send_with_custom_preimage() { // check payment status and verify stored preimage expect_payment_successful_event!(node_a, payment_id, None); let details: PaymentDetails = - node_a.list_payments_with_filter(|p| p.id == payment_id).first().unwrap().clone(); + 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); @@ -3721,7 +3721,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 { .. }) }); @@ -4129,7 +4129,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 { .. }) }); @@ -4146,7 +4146,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!( @@ -4478,7 +4478,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 6e6d2d278..efab1480e 100644 --- a/tests/reorg_test.rs +++ b/tests/reorg_test.rs @@ -12,7 +12,8 @@ use serde_json::json; 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, TestChainSource, + setup_bitcoind_and_electrsd, setup_node, wait_for_outpoint_spend, wait_for_tx, NodePaymentExt, + TestChainSource, }; #[test] @@ -176,7 +177,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 4d9bbb204e637a40264a0beb9531ef6453fd7d73 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 13 Aug 2026 19:06:51 +0200 Subject: [PATCH 15/26] f Adapt later payment listing tests Upstream funding-rebroadcast tests added more direct payment-listing calls after the shared test helper was introduced. Route them through the helper so pagination remains centralized. Co-Authored-By: HAL 9000 --- tests/integration_tests_rust.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration_tests_rust.rs b/tests/integration_tests_rust.rs index 67124eb06..fd247f74c 100644 --- a/tests/integration_tests_rust.rs +++ b/tests/integration_tests_rust.rs @@ -2224,7 +2224,7 @@ async fn zero_conf_splice_out_funding_rebroadcast_canary() { ); // The re-offers must not have minted a record for a transaction the wallet has no stake in. - let splice_records = node_a.list_payments_with_filter( + let splice_records = node_a.list_payments_matching( |p| matches!(p.kind, PaymentKind::Onchain { txid, .. } if txid == txo.txid), ); assert!( @@ -2294,7 +2294,7 @@ async fn zero_conf_splice_in_funding_rebroadcast_canary() { expect_channel_ready_event!(node_b, node_a.node_id()); let splice_payments = |node: &Node| { - node.list_payments_with_filter( + node.list_payments_matching( |p| matches!(p.kind, PaymentKind::Onchain { txid, .. } if txid == txo.txid), ) }; From 03a5322e6ed4919cb1c2940a932e215960938c81 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Wed, 12 Aug 2026 15:53:15 +0200 Subject: [PATCH 16/26] io: Share bounded store reads Multiple callers need bounded concurrent KVStore reads while retaining their own ordering and error policy. Extract the scheduler from read_all_objects so later consumers reuse one fill, refill, and abort implementation. Co-Authored-By: HAL 9000 --- src/io/utils.rs | 126 +++++++++++++++++++++++++++++++----------------- 1 file changed, 83 insertions(+), 43 deletions(-) diff --git a/src/io/utils.rs b/src/io/utils.rs index 4657688f5..855a85c06 100644 --- a/src/io/utils.rs +++ b/src/io/utils.rs @@ -222,67 +222,107 @@ where }) } -/// Read all objects of type `T` from the given namespace, spawning reads in parallel. -pub(crate) async fn read_all_objects( - kv_store: &DynStore, primary_namespace: &str, secondary_namespace: &str, logger: L, -) -> Result, std::io::Error> +/// Runs reads from a single namespace with bounded concurrency, passing each result to +/// `handle_read` as it completes. +/// +/// `reads` pairs caller-owned context with each key so callers can preserve ordering or other +/// metadata without coupling it to the scheduling machinery. Any join or handler error aborts +/// the remaining reads. +pub(crate) async fn process_kv_store_reads( + kv_store: &DynStore, primary_namespace: &str, secondary_namespace: &str, reads: I, + mut handle_read: H, mut handle_join_error: J, +) -> Result<(), E> where - T: Readable, - L: Deref, - L::Target: LdkLogger, + C: Send + 'static, + I: IntoIterator, + H: FnMut(C, String, Result, lightning::io::Error>) -> Result<(), E>, + J: FnMut(tokio::task::JoinError) -> E, { - let type_name = std::any::type_name::(); - let mut res = Vec::new(); - - let mut stored_keys = KVStore::list(&*kv_store, primary_namespace, secondary_namespace).await?; - const BATCH_SIZE: usize = 50; + type ReadResult = (C, String, Result, lightning::io::Error>); + let spawn_read = |set: &mut tokio::task::JoinSet>, context: C, key: String| { + let read_fut = KVStore::read(kv_store, primary_namespace, secondary_namespace, &key); + set.spawn(async move { (context, key, read_fut.await) }); + }; + + let mut reads = reads.into_iter(); 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((context, key)) = reads.next() else { break }; + spawn_read(&mut set, context, key); + debug_assert!(set.len() <= BATCH_SIZE); } - while let Some(read_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); + while let Some(join_res) = set.join_next().await { + let (context, key, read_res) = match join_res { + Ok(read_res) => read_res, + Err(e) => { set.abort_all(); - e - })?; + return Err(handle_join_error(e)); + }, + }; // Refill set for every finished future, if we still have something to do. - if let Some(next_key) = stored_keys.pop() { - let fut = KVStore::read(kv_store, primary_namespace, secondary_namespace, &next_key); - set.spawn(fut); + if let Some((next_context, next_key)) = reads.next() { + spawn_read(&mut set, next_context, next_key); 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); - std::io::Error::new( - std::io::ErrorKind::InvalidData, - format!("Failed to deserialize {}", type_name), - ) - })?; - res.push(object); + if let Err(e) = handle_read(context, key, read_res) { + set.abort_all(); + return Err(e); + } } debug_assert!(set.is_empty()); - debug_assert!(stored_keys.is_empty()); + debug_assert!(reads.next().is_none()); + Ok(()) +} + +/// Read all objects of type `T` from the given namespace, spawning reads in parallel. +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, +{ + let type_name = std::any::type_name::(); + let mut res = Vec::new(); + + let stored_keys = KVStore::list(&*kv_store, primary_namespace, secondary_namespace).await?; + // Preserve the prior `Vec::pop` order. + let reads = stored_keys.into_iter().rev().map(|key| ((), key)); + process_kv_store_reads( + kv_store, + primary_namespace, + secondary_namespace, + reads, + |(), _key, read_res| -> Result<(), std::io::Error> { + let reader = read_res.map_err(|e| { + log_error!(logger, "Failed to read {}: {}", type_name, e); + std::io::Error::from(e) + })?; + let object = T::read(&mut &*reader).map_err(|e| { + log_error!(logger, "Failed to deserialize {}: {}", type_name, e); + std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!("Failed to deserialize {}", type_name), + ) + })?; + res.push(object); + Ok(()) + }, + |e| { + log_error!(logger, "Failed to read {}: {}", type_name, e); + e.into() + }, + ) + .await?; Ok(res) } From 3591b7706d84f7f498d41a88c57e0bcd4a241b51 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 6 Aug 2026 13:21:31 +0200 Subject: [PATCH 17/26] 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 | 451 +++++++++++++++++- 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, 625 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8853e6361..400f9afa1 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 @@ -19,6 +23,13 @@ `Event::PaymentClaimable`. ## 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 735b42b4e..fddf5940c 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(); @@ -281,6 +282,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 6b5bae003..dc958308a 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -13,10 +13,11 @@ 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::io::utils::process_kv_store_reads; +use crate::logger::{log_debug, log_error, LdkLogger}; use crate::types::DynStore; use crate::Error; @@ -29,8 +30,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 { @@ -195,6 +203,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 { @@ -241,6 +257,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, @@ -436,6 +461,138 @@ 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> { + process_kv_store_reads( + &*self.kv_store, + &self.primary_namespace, + &self.secondary_namespace, + missing, + |idx, key, read_res| { + 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 + ); + 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 + ); + return Err(Error::PersistenceFailed); + }, + } + Ok(()) + }, + |e| { + log_error!(self.logger, "Failed to join object read task: {}", e); + Error::PersistenceFailed + }, + ) + .await + } + /// Returns the object stored under `id`, reading through to the [`KVStore`] if the cache is /// not authoritative and misses. /// @@ -595,7 +752,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"; @@ -635,6 +792,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) }); @@ -1561,4 +1722,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 9c5d33863..34c3ae671 100644 --- a/src/ffi/types.rs +++ b/src/ffi/types.rs @@ -39,6 +39,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}; @@ -2887,3 +2888,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 1ac6103be..b0f4901a7 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 c706706c8..21de2b680 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 288e0a189..85f618c95 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -461,13 +461,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 ce6b8b02751e80f777c786992634b401144d363a Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Tue, 11 Aug 2026 13:15:39 +0200 Subject: [PATCH 18/26] f Document pagination consistency Clarify that concurrent updates can make one payment page reflect multiple points in time. This is the expected tradeoff for avoiding writer stalls during remote reads. Assisted by AI tooling. Co-Authored-By: HAL 9000 --- src/data_store.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/data_store.rs b/src/data_store.rs index dc958308a..4ab9191ad 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -473,7 +473,9 @@ where /// /// 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. + /// removed while paginating may or may not be observed. Likewise, a page is not a point-in-time + /// snapshot: concurrently updated objects may reflect different moments depending on whether + /// they came from the cache or the storage backend. /// /// 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 From 01090c88865e81b5fa6df7ebb90eada7570c16e5 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Tue, 11 Aug 2026 13:33:54 +0200 Subject: [PATCH 19/26] f Qualify payment page token lifetime Do not promise that page tokens survive node restarts when custom stores only need to keep them valid for a reasonable timeframe. Describe token lifetime as a storage-backend guarantee instead. Assisted by AI tooling. Co-Authored-By: HAL 9000 --- CHANGELOG.md | 6 +++--- src/data_store.rs | 7 ++++--- src/lib.rs | 4 ++-- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 400f9afa1..46112d79b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,9 +26,9 @@ - `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. + from the configured storage backend, and token lifetime follows that backend's guarantees. + 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 diff --git a/src/data_store.rs b/src/data_store.rs index 4ab9191ad..75fe1589d 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -467,9 +467,10 @@ where /// [`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. + /// it unchanged and never derive an order of our own. This keeps pagination independent of our + /// caching, while the token lifetime remains the storage backend's own. The backend's creation + /// ordering is also 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 diff --git a/src/lib.rs b/src/lib.rs index 0ce45bfdd..2bee539f7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -2199,8 +2199,8 @@ impl Node { /// /// 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. + /// where it left off. Ordering, pagination, and token lifetime are determined by the configured + /// storage backend, including whether a token remains 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 From 997ccf1599ca823e2c381879c27e69f2d4531d70 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 13 Aug 2026 09:27:28 +0200 Subject: [PATCH 20/26] f Preallocate missing payment reads Size the missing-read work list from the storage response so a full page does not grow the vector incrementally. Co-Authored-By: HAL 9000 --- src/data_store.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/data_store.rs b/src/data_store.rs index 75fe1589d..8e9980827 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -512,7 +512,7 @@ where // 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 mut missing = Vec::with_capacity(response.keys.len()); { let _guard = self.mutation_lock.read().await; let locked_cache = self.cache.lock().expect("lock"); From 345c1d24b483c9f1f7bdc930b6a50e41f7d3ccbe Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 6 Aug 2026 13:53:04 +0200 Subject: [PATCH 21/26] 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 | 39 +++++------- src/config.rs | 9 +++ src/data_store.rs | 6 +- src/payment/store.rs | 137 +++++++++++++++++++++++++++++++++++++++++++ src/types.rs | 4 +- 5 files changed, 165 insertions(+), 30 deletions(-) diff --git a/src/builder.rs b/src/builder.rs index 6e1da8bf8..84a501ebb 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; @@ -1456,15 +1456,9 @@ 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, address_pool_res) = runtime - .block_on(async move { + let (node_metris_res, pending_payment_store_res, address_pool_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, @@ -1489,20 +1483,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 aa8cc7e61..a651542fa 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 8e9980827..d32c92c12 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -88,12 +88,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 } @@ -718,12 +716,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 21de2b680..292eab6aa 100644 --- a/src/payment/store.rs +++ b/src/payment/store.rs @@ -1538,3 +1538,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 9abea2bdc7b4f70a635d8d15fdcf9bfa46951148 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Wed, 12 Aug 2026 15:16:57 +0200 Subject: [PATCH 22/26] f Adapt upstream payment store tests Use the bounded policy when constructing the payment store in wallet tests. Preserve race tests that count persisted records by inspecting the payment namespace instead of relying on a complete in-memory cache. Co-Authored-By: HAL 9000 --- src/wallet/mod.rs | 45 ++++++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index 80a8fc5b7..a13b87407 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -54,9 +54,9 @@ use lightning_invoice::RawBolt11Invoice; use persist::KVStoreWalletPersister; use crate::config::{Config, ADDRESS_POOL_SIZE}; -#[cfg(test)] -use crate::data_store::KeepAllEntries; use crate::data_store::StorableObject; +#[cfg(test)] +use crate::data_store::{KeepAllEntries, KeepLeastRecentlyUsed}; use crate::fee_estimator::{ConfirmationTarget, FeeEstimator, OnchainFeeEstimator}; use crate::logger::{log_debug, log_error, log_info, log_trace, LdkLogger, Logger}; use crate::payment::pending_payment_store::PendingPaymentDetailsUpdate; @@ -2695,7 +2695,7 @@ mod tests { use lightning::util::persist::{KVStore, PageToken, PaginatedKVStore, PaginatedListResponse}; use super::*; - use crate::config::EsploraSyncConfig; + use crate::config::{EsploraSyncConfig, PAYMENT_CACHE_CAPACITY}; use crate::io::test_utils::InMemoryStore; use crate::io::{ BDK_WALLET_ADDRESS_POOL_KEY, BDK_WALLET_ADDRESS_POOL_PRIMARY_NAMESPACE, @@ -2821,7 +2821,7 @@ mod tests { .unwrap(); let payment_store = Arc::new(PaymentStore::new( Vec::new(), - KeepAllEntries, + KeepLeastRecentlyUsed::new(PAYMENT_CACHE_CAPACITY), PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE.to_string(), PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE.to_string(), Arc::clone(&store), @@ -4093,7 +4093,7 @@ mod tests { async fn funding_confirmation_waits_for_classification() { let gated = NamespaceGatedStore::new(PENDING_PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE); let store: Arc = Arc::new(DynStoreWrapper(gated.clone())); - let wallet = new_test_wallet(store, false).await; + let wallet = new_test_wallet(Arc::clone(&store), false).await; let txid1 = Txid::from_byte_array([1u8; 32]); let txid2 = Txid::from_byte_array([2u8; 32]); @@ -4138,7 +4138,14 @@ mod tests { // Liveness sanity only (both pre- and post-fix stall here): while classification is // parked, no second record may have been committed. tokio::time::sleep(Duration::from_millis(250)).await; - assert!(wallet.payment_store.list_filter(|_| true).await.len() <= 1); + let payment_keys = KVStore::list( + &*store, + PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE, + PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE, + ) + .await + .unwrap(); + assert!(payment_keys.len() <= 1); drop(gate_guard); classification.await.unwrap().unwrap(); @@ -4147,9 +4154,15 @@ mod tests { // Both writers converge on the classified record: the confirmation refreshes it in // place with the confirmed candidate's figures rather than minting a second record // keyed by the event txid. - let payments = wallet.payment_store.list_filter(|_| true).await; - assert_eq!(payments.len(), 1, "the confirmation must not mint a duplicate record"); - let payment = &payments[0]; + let payment_keys = KVStore::list( + &*store, + PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE, + PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE, + ) + .await + .unwrap(); + assert_eq!(payment_keys.len(), 1, "the confirmation must not mint a duplicate record"); + let payment = wallet.payment_store.get(&payment_id).await.unwrap().unwrap(); assert_eq!(payment.id, payment_id); assert_eq!(payment.amount_msat, Some(2_000_000)); assert_eq!(payment.fee_paid_msat, Some(999)); @@ -4172,7 +4185,7 @@ mod tests { #[tokio::test(flavor = "multi_thread")] async fn funding_classification_waits_for_wallet_sync() { let store: Arc = Arc::new(DynStoreWrapper(InMemoryStore::new())); - let wallet = new_test_wallet(store, false).await; + let wallet = new_test_wallet(Arc::clone(&store), false).await; let txid = Txid::from_byte_array([3u8; 32]); let payment_id = PaymentId(txid.to_byte_array()); @@ -4211,9 +4224,15 @@ mod tests { // Both writers converge on one record carrying the classification: the generic // fallback must not clobber the contribution-derived figures with its wallet-derived // view of the transaction. - let payments = wallet.payment_store.list_filter(|_| true).await; - assert_eq!(payments.len(), 1); - let payment = &payments[0]; + let payment_keys = KVStore::list( + &*store, + PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE, + PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE, + ) + .await + .unwrap(); + assert_eq!(payment_keys.len(), 1); + let payment = wallet.payment_store.get(&payment_id).await.unwrap().unwrap(); assert_eq!(payment.id, payment_id); assert_eq!( payment.amount_msat, From 23f92a018649f23e18d1a62d285ce2bb0a6c6cb3 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 13 Aug 2026 19:07:42 +0200 Subject: [PATCH 23/26] f Adapt later bounded-store tests Upstream funding-rebroadcast tests added full payment-store scans after the bounded cache was written. Read the first storage page instead so the tests retain their record-count assertions without requiring a complete in-memory cache. Co-Authored-By: HAL 9000 --- src/wallet/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index a13b87407..892f40914 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -3960,7 +3960,7 @@ mod tests { // No inputs or outputs involve the wallet: nothing to record. wallet.classify_funding(&dummy_tx(), &channels, tx_type.clone()).await.unwrap(); - assert!(wallet.payment_store.list_filter(|_| true).await.is_empty()); + assert!(wallet.payment_store.list_page(None).await.unwrap().objects.is_empty()); assert!(wallet.pending_payment_store.list_filter(|_| true).await.is_empty()); // A computable fee is not wallet participation. The wallet can resolve a splice's shared @@ -3984,7 +3984,7 @@ mod tests { }], }; wallet.classify_funding(&splice_tx, &channels, tx_type.clone()).await.unwrap(); - assert!(wallet.payment_store.list_filter(|_| true).await.is_empty()); + assert!(wallet.payment_store.list_page(None).await.unwrap().objects.is_empty()); // Control: a funding transaction the wallet participates in is still recorded. let script_pubkey = wallet @@ -4001,7 +4001,7 @@ mod tests { output: vec![TxOut { value: Amount::from_sat(10_000), script_pubkey }], }; wallet.classify_funding(&funded_tx, &channels, tx_type).await.unwrap(); - let payments = wallet.payment_store.list_filter(|_| true).await; + let payments = wallet.payment_store.list_page(None).await.unwrap().objects; assert_eq!(payments.len(), 1); assert_eq!(payments[0].id, PaymentId(funded_tx.compute_txid().to_byte_array())); } @@ -4051,7 +4051,7 @@ mod tests { let tx_type = TransactionType::Funding { channels: vec![] }; async fn assert_unchanged(wallet: &Wallet, payment_id: PaymentId, confirmed: bool) { - let payments = wallet.payment_store.list_filter(|_| true).await; + let payments = wallet.payment_store.list_page(None).await.unwrap().objects; assert_eq!(payments.len(), 1, "the rebroadcast must not mint a second record"); let payment = &payments[0]; assert_eq!(payment.id, payment_id); From 329f6169264c1fbdac4bfa7eb36ace1c42413096 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 6 Aug 2026 14:58:25 +0200 Subject: [PATCH 24/26] 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 | 44 ++++++--- src/config.rs | 7 ++ src/data_store.rs | 28 ++++++ src/io/utils.rs | 233 ++++++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 288 insertions(+), 24 deletions(-) diff --git a/src/builder.rs b/src/builder.rs index 84a501ebb..f0f38783f 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::{ @@ -1456,9 +1457,16 @@ 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, address_pool_res) = - runtime.block_on(async move { + let (payment_store_res, node_metris_res, pending_payment_store_res, address_pool_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, @@ -1483,17 +1491,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 a651542fa..c396a3fab 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 d32c92c12..a9fe0d0f5 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -291,6 +291,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, @@ -1611,6 +1615,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 855a85c06..de5e24701 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, @@ -282,27 +283,98 @@ where Ok(()) } -/// 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)); + + if stored_keys.len() >= max_objects { + break; + } - let stored_keys = KVStore::list(&*kv_store, primary_namespace, secondary_namespace).await?; - // Preserve the prior `Vec::pop` order. - let reads = stored_keys.into_iter().rev().map(|key| ((), key)); + match response.next_page_token { + Some(next_page_token) => page_token = Some(next_page_token), + None => break, + } + } + + // 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 reads = stored_keys.into_iter().enumerate(); process_kv_store_reads( kv_store, primary_namespace, secondary_namespace, reads, - |(), _key, read_res| -> Result<(), std::io::Error> { + |idx, _key, read_res| -> Result<(), std::io::Error> { let reader = read_res.map_err(|e| { log_error!(logger, "Failed to read {}: {}", type_name, e); std::io::Error::from(e) @@ -314,7 +386,7 @@ where format!("Failed to deserialize {}", type_name), ) })?; - res.push(object); + objects[idx] = Some(object); Ok(()) }, |e| { @@ -324,7 +396,9 @@ where ) .await?; - Ok(res) + debug_assert!(objects.iter().all(|object| object.is_some())); + + Ok(objects.into_iter().flatten().collect()) } /// Read `OutputSweeper` state from the store. @@ -941,3 +1015,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()); + } +} From f9edefff2d3abd02fc6644a63a9095676fbf7136 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 13 Aug 2026 09:28:28 +0200 Subject: [PATCH 25/26] f Qualify payment cache warmup Describe the built-in backend assumption and limit the cache-hit claim to startup, before normal activity can evict warmed entries. Co-Authored-By: HAL 9000 --- src/config.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/config.rs b/src/config.rs index c396a3fab..a409b9e48 100644 --- a/src/config.rs +++ b/src/config.rs @@ -59,9 +59,10 @@ pub(crate) const PAYMENT_CACHE_CAPACITY: NonZeroUsize = NonZeroUsize::new(1000). // 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. +// This matches the built-in storage backends' page size, so warming the cache costs a single page +// listing and one batch of reads. Immediately after startup, a first-page `Node::list_payments` +// call reads only its keys from storage; the payment bodies come from the cache. Later activity +// may displace those entries. pub(crate) const PAYMENT_CACHE_WARMUP_COUNT: NonZeroUsize = NonZeroUsize::new(50).unwrap(); // The default {Esplora,Electrum} client timeout we're using. From 280f7c156916519663dd45d83d18c52ad1becbf6 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 13 Aug 2026 11:04:02 +0200 Subject: [PATCH 26/26] f Bail out on a page token that does not advance Seeding a store walks the namespace page by page, trusting the backend to move the token along. A custom store that hands back the token it was given would have us fetch the same page forever, and this runs during `Builder::build`, so the node would never finish starting. Treat a token that does not advance as the broken store it indicates and fail the read instead. Co-Authored-By: HAL 9000 --- src/io/utils.rs | 129 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 124 insertions(+), 5 deletions(-) diff --git a/src/io/utils.rs b/src/io/utils.rs index de5e24701..b9255120f 100644 --- a/src/io/utils.rs +++ b/src/io/utils.rs @@ -27,8 +27,8 @@ use lightning::routing::scoring::{ ChannelLiquidities, ProbabilisticScorer, ProbabilisticScoringDecayParameters, }; use lightning::util::persist::{ - migrate_kv_store_data_async, KVStore, PaginatedKVStore, KVSTORE_NAMESPACE_KEY_ALPHABET, - KVSTORE_NAMESPACE_KEY_MAX_LEN, NETWORK_GRAPH_PERSISTENCE_KEY, + migrate_kv_store_data_async, KVStore, PageToken, 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, OUTPUT_SWEEPER_PERSISTENCE_SECONDARY_NAMESPACE, SCORER_PERSISTENCE_KEY, @@ -341,13 +341,13 @@ where // 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; + let mut page_token: Option = None; loop { let response = PaginatedKVStore::list_paginated( &*kv_store, primary_namespace, secondary_namespace, - page_token, + page_token.clone(), ) .await?; @@ -359,6 +359,26 @@ where } match response.next_page_token { + // A token that doesn't advance would have us ask for the same page for as long as the + // store cares to hand it back, and this runs during `Builder::build`. Give up on the + // store rather than never returning. + Some(next_page_token) if page_token.as_ref() == Some(&next_page_token) => { + log_error!( + logger, + "Failed to read {}: listing {}/{} handed back a page token that does not advance", + type_name, + primary_namespace, + secondary_namespace + ); + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!( + "Non-advancing page token while listing {}/{}", + PrintableString(primary_namespace), + PrintableString(secondary_namespace) + ), + )); + }, Some(next_page_token) => page_token = Some(next_page_token), None => break, } @@ -1022,7 +1042,7 @@ mod read_objects_tests { use std::sync::Arc; use lightning::impl_writeable_tlv_based; - use lightning::util::persist::KVStore; + use lightning::util::persist::{KVStore, PageToken, PaginatedKVStore, PaginatedListResponse}; use lightning::util::ser::Writeable; use lightning::util::test_utils::TestLogger; @@ -1155,4 +1175,103 @@ mod read_objects_tests { .await; assert_eq!(std::io::ErrorKind::InvalidData, res.unwrap_err().kind()); } + + /// A store whose `list_paginated` ignores the token it is given and keeps handing back the + /// same page along with the same token, standing in for a custom backend that got pagination + /// wrong. + /// + /// It stops after `stuck_pages` calls so that a reader which does not notice terminates + /// instead of hanging this test. + struct StuckTokenStore { + inner: InMemoryStore, + stuck_pages: usize, + calls: std::sync::Mutex, + } + + impl KVStore for StuckTokenStore { + fn read( + &self, primary_namespace: &str, secondary_namespace: &str, key: &str, + ) -> impl std::future::Future, lightning::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, lightning::io::Error>> + 'static + Send + { + self.inner.list(primary_namespace, secondary_namespace) + } + } + + impl PaginatedKVStore for StuckTokenStore { + fn list_paginated( + &self, primary_namespace: &str, secondary_namespace: &str, + _page_token: Option, + ) -> impl std::future::Future> + + 'static + + Send { + let call = { + let mut calls = self.calls.lock().unwrap(); + *calls += 1; + *calls + }; + let give_up = call > self.stuck_pages; + // Always list the very first page, whatever we were asked to continue from. + let inner_fut = self.inner.list_paginated(primary_namespace, secondary_namespace, None); + async move { + let mut response = inner_fut.await?; + response.next_page_token = + if give_up { None } else { Some(PageToken::new("stuck".to_string())) }; + Ok(response) + } + } + } + + #[tokio::test] + async fn a_page_token_that_does_not_advance_is_an_error() { + let stuck = StuckTokenStore { + inner: InMemoryStore::new(), + stuck_pages: 5, + calls: std::sync::Mutex::new(0), + }; + let kv_store: Arc = Arc::new(DynStoreWrapper(stuck)); + for id in 0..3u32 { + KVStore::write( + &*kv_store, + TEST_PRIMARY_NAMESPACE, + TEST_SECONDARY_NAMESPACE, + &hex_utils::to_string(&id.to_be_bytes()), + TestObject { id }.encode(), + ) + .await + .unwrap(); + } + + // Without a guard this walks the same page over and over, and only terminates here + // because the store eventually relents. A real one would not, and the read would never + // return. + let res: Result, _> = read_all_objects( + &*kv_store, + TEST_PRIMARY_NAMESPACE, + TEST_SECONDARY_NAMESPACE, + Arc::new(TestLogger::new()), + ) + .await; + assert_eq!(std::io::ErrorKind::InvalidData, res.unwrap_err().kind()); + } }