diff --git a/CHANGELOG.md b/CHANGELOG.md index 8853e63610..46112d79be 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, 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 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 006878a4c8..2770e02879 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 be5fdc8084..fddf5940ca 100644 --- a/bindings/ldk_node.udl +++ b/bindings/ldk_node.udl @@ -147,11 +147,13 @@ 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); BalanceDetails list_balances(); - sequence list_payments(); + [Throws=NodeError] + PaymentDetailsPage list_payments(PageToken? page_token); sequence list_peers(); sequence list_channels(); NetworkGraph network_graph(); @@ -236,6 +238,7 @@ enum NodeError { "InvalidDateTime", "InvalidFeeRate", "InvalidScriptPubKey", + "InvalidPageToken", "DuplicatePayment", "UnsupportedCurrency", "InsufficientFunds", @@ -279,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/builder.rs b/src/builder.rs index dc41aef1a2..f0f38783fb 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -50,9 +50,11 @@ 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, + PAYMENT_CACHE_WARMUP_COUNT, }; use crate::connection::ConnectionManager; +use crate::data_store::{KeepAllEntries, KeepLeastRecentlyUsed}; use crate::entropy::NodeEntropy; use crate::event::EventQueue; use crate::fee_estimator::OnchainFeeEstimator; @@ -60,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::{ @@ -1458,10 +1460,11 @@ fn build_with_store_internal( let (payment_store_res, node_metris_res, pending_payment_store_res, address_pool_res) = runtime .block_on(async move { tokio::join!( - read_all_objects( + 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)), @@ -1490,7 +1493,11 @@ fn build_with_store_internal( let payment_store = match payment_store_res { Ok(payments) => Arc::new(PaymentStore::new( - payments, + // 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), @@ -1745,8 +1752,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/config.rs b/src/config.rs index aa8cc7e615..a409b9e48f 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,22 @@ 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 number of payments we read into the cache when starting up. +// +// 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. const DEFAULT_PER_REQUEST_TIMEOUT_SECS: u8 = 10; diff --git a/src/data_store.rs b/src/data_store.rs index a440a2e1e8..a9fe0d0f59 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -5,14 +5,19 @@ // 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::util::persist::KVStore; +use lightning::io::ErrorKind; +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; @@ -25,8 +30,15 @@ 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 + 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 { @@ -40,135 +52,358 @@ 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. +pub(crate) struct KeepLeastRecentlyUsed { + capacity: NonZeroUsize, +} + +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`]. +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) + }, + } + } + + fn is_keep_all(&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 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 { + 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_keep_all`]. + 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(), + } + } +} + +/// 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, { - objects: Mutex>, - mutation_lock: tokio::sync::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 `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. + /// + /// 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, 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, - mutation_lock: tokio::sync::Mutex::new(()), + cache, + mutation_lock: tokio::sync::RwLock::new(()), primary_namespace, secondary_namespace, kv_store, logger, + cache_policy: PhantomData, } } - pub(crate) async fn insert(&self, object: SO) -> Result { - let _guard = self.mutation_lock.lock().await; + /// Stores `object`, overwriting any object previously stored under the same id. + pub(crate) async fn insert(&self, object: SO) -> Result<(), Error> { + let _guard = self.mutation_lock.write().await; + let id = object.id(); 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(()) } /// 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.lock().await; + 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.lock().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, + let _guard = self.mutation_lock.write().await; + + let known_absent = { + let cache = self.cache.lock().expect("lock"); + cache.is_keep_all() && !cache.contains(id) + }; + if known_absent { + 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 current in-memory object for `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 get(&self, id: &SO::Id) -> Option { - self.objects.lock().expect("lock").get(id).cloned() + /// 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; + 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.lock().await; + 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 + 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) } @@ -179,36 +414,277 @@ 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>( &self, id: &SO::Id, f: F, ) -> Result, Error> { - let _guard = self.mutation_lock.lock().await; + self.mutate_with(id, |current| async move { Ok(f(current.as_ref())) }).await + } - let current = self.objects.lock().expect("lock").get(id).cloned(); - let new_object = match f(current.as_ref()) { + 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.write().await; + + let current = self.lookup(id).await?; + let new_object = match f(current).await? { Some(new_object) => new_object, None => return Ok(None), }; 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)) } - /// Returns in-memory objects matching `f`. + /// 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 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.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. 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 + /// 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 + /// `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::with_capacity(response.keys.len()); + { + 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. + /// + /// The caller must hold `mutation_lock`. + async fn lookup(&self, id: &SO::Id) -> Result, Error> { + let (cached_object, is_keep_all) = { + let mut locked_cache = self.cache.lock().expect("lock"); + (locked_cache.get(id), locked_cache.is_keep_all()) + }; + + if let Some(object) = cached_object { + return Ok(Some(object)); + } + if is_keep_all { + 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_keep_all) = { + let locked_cache = self.cache.lock().expect("lock"); + (locked_cache.contains(id), locked_cache.is_keep_all()) + }; + + if is_cached { + return Ok(true); + } + if is_keep_all { + 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 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 { - self.objects.lock().expect("lock").values().filter(f).cloned().collect::>() + /// 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> { @@ -243,27 +719,75 @@ where Ok(()) } - /// Returns whether the in-memory store contains `id`. + #[cfg(test)] + pub(crate) fn cached_len(&self) -> usize { + self.cache.lock().expect("lock").len() + } + + #[cfg(test)] + pub(crate) 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`. /// - /// 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) + /// 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; + 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}; use lightning::util::test_utils::TestLogger; use lightning::{impl_writeable_tlv_based, io}; + use tokio::sync::Notify; 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"; + 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], @@ -273,12 +797,19 @@ 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) }); 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 { @@ -290,6 +821,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 { @@ -301,22 +839,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; @@ -362,21 +907,121 @@ 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, ) } + /// 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::new(id, [23u8; 3]); + let new_object = TestObject::new(id, [24u8; 3]); + + let data_store: Arc>> = Arc::new(DataStore::new( + vec![old_object], + KeepAllEntries, + TEST_PRIMARY_NAMESPACE.to_string(), + TEST_SECONDARY_NAMESPACE.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(()), 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())); 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), @@ -384,7 +1029,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(); @@ -393,37 +1038,37 @@ mod tests { .await .is_err()); - // 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)); + // Check we successfully store an object. + let object = TestObject::new(id, [23u8; 3]); + 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!(Some(override_object), data_store.get(&id)); + assert_eq!(Ok(()), data_store.insert(override_object).await); + 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).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] }; + 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 @@ -439,10 +1084,11 @@ 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, primary_namespace.clone(), secondary_namespace.clone(), Arc::clone(&store), @@ -450,7 +1096,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()); @@ -459,7 +1105,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 @@ -471,11 +1117,12 @@ 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], - "datastore_test_primary".to_string(), - "datastore_test_secondary".to_string(), + KeepAllEntries, + TEST_PRIMARY_NAMESPACE.to_string(), + TEST_SECONDARY_NAMESPACE.to_string(), store, logger, ); @@ -488,33 +1135,71 @@ 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()); + } + + #[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::new(id, [23u8; 3]); + let other_object = TestObject::new(other_id, [24u8; 3]); + let data_store: DataStore> = DataStore::new( + vec![existing_object], + KeepAllEntries, + TEST_PRIMARY_NAMESPACE.to_string(), + TEST_SECONDARY_NAMESPACE.to_string(), + Arc::clone(&store), + Arc::clone(&logger), + ); + let other_store: DataStore> = DataStore::new( + vec![other_object], + KeepAllEntries, + "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::new(id, [24u8; 3]); 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_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], - "datastore_test_primary".to_string(), - "datastore_test_secondary".to_string(), + KeepAllEntries, + TEST_PRIMARY_NAMESPACE.to_string(), + TEST_SECONDARY_NAMESPACE.to_string(), store, logger, ); // 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; @@ -524,7 +1209,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. @@ -535,78 +1220,817 @@ 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] 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 ); - 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] }; + let new_object = TestObject::new(new_id, [34u8; 3]); assert_eq!( 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] 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 ); - 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] }; + 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).is_none()); + assert!(data_store.get(&new_id).await.unwrap().is_none()); } #[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); - assert!(data_store.get(&id).is_none()); + assert!(data_store.get(&id).await.unwrap().is_none()); } #[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)); + assert_eq!(Some(object), data_store.get(&id).await.unwrap()); } #[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)); + assert_eq!(Some(object), data_store.get(&id).await.unwrap()); + } + + /// 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, + } + + 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.writes.fetch_add(1, Ordering::Relaxed); + 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.removes.fetch_add(1, Ordering::Relaxed); + 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 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), + removes: Arc::new(AtomicUsize::new(0)), + 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(()), 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_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), + removes: Arc::new(AtomicUsize::new(0)), + 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(); + + 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] + 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_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_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(); + 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()); + } + + /// 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/error.rs b/src/error.rs index 1fbc5066a3..9a03c446fa 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.") }, diff --git a/src/event.rs b/src/event.rs index be54969c7a..0a35697552 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!( @@ -987,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); } @@ -1088,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()); } } @@ -1140,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 @@ -1189,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 @@ -1228,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()); } } @@ -1288,7 +1241,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 +1413,33 @@ 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 read payment {} for success logging: {}", + payment_id, + e + ); + }, + }; let event = Event::PaymentSuccessful { payment_id, payment_hash, diff --git a/src/ffi/types.rs b/src/ffi/types.rs index 9c5d338631..34c3ae6715 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 054e8b1f28..d60c4e188a 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 156fef3a38..82418e7d5d 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 aadb4b79a8..fa9b3e8cae 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/io/utils.rs b/src/io/utils.rs index 4657688f51..b9255120f3 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,8 +27,8 @@ use lightning::routing::scoring::{ ChannelLiquidities, ProbabilisticScorer, ProbabilisticScoringDecayParameters, }; use lightning::util::persist::{ - migrate_kv_store_data_async, KVStore, 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, @@ -222,69 +223,202 @@ 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(()) +} - Ok(res) +/// 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 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: Option = None; + loop { + let response = PaginatedKVStore::list_paginated( + &*kv_store, + primary_namespace, + secondary_namespace, + page_token.clone(), + ) + .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; + } + + 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, + } + } + + // 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, + |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) + })?; + 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), + ) + })?; + objects[idx] = Some(object); + Ok(()) + }, + |e| { + log_error!(logger, "Failed to read {}: {}", type_name, e); + e.into() + }, + ) + .await?; + + debug_assert!(objects.iter().all(|object| object.is_some())); + + Ok(objects.into_iter().flatten().collect()) } /// Read `OutputSweeper` state from the store. @@ -901,3 +1035,243 @@ 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, PageToken, PaginatedKVStore, PaginatedListResponse}; + 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()); + } + + /// 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()); + } +} diff --git a/src/lib.rs b/src/lib.rs index 14ee734a3d..2bee539f73 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; @@ -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. @@ -2194,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, 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 + /// 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; @@ -2215,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.payment_store.list_filter(f) - } - - /// Retrieves all payments. - pub fn list_payments(&self) -> Vec { - self.payment_store.list_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/bolt11.rs b/src/payment/bolt11.rs index e33dd3950f..5f0a70c2c6 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, @@ -488,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/mod.rs b/src/payment/mod.rs index 1ac6103bea..b0f4901a7c 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/spontaneous.rs b/src/payment/spontaneous.rs index 45dab644d4..f4e1cd93d6 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/payment/store.rs b/src/payment/store.rs index cb76e78cc3..292eab6aa9 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; @@ -1320,7 +1357,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 +1405,7 @@ mod tests { let logger = Arc::new(TestLogger::new()); DataStore::>::new( seed, + KeepAllEntries, "payment_test_primary".to_string(), "payment_test_secondary".to_string(), store, @@ -1379,7 +1417,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 +1440,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 +1466,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, @@ -1500,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 22429d980b..65156982eb 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, 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>; +pub(crate) type PaymentStore = DataStore, KeepLeastRecentlyUsed>; /// 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>; diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index df95c11ecc..892f409143 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -55,6 +55,8 @@ use persist::KVStoreWalletPersister; use crate::config::{Config, ADDRESS_POOL_SIZE}; 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; @@ -343,6 +345,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 +382,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 +392,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 +486,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 +524,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,13 +543,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).await?; 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()); @@ -556,6 +562,7 @@ impl Wallet { let payment_id = self .find_payment_by_txid(txid) + .await? .unwrap_or_else(|| PaymentId(txid.to_byte_array())); if self @@ -1583,7 +1590,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 { @@ -1797,12 +1804,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. @@ -1816,17 +1824,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(()) @@ -1900,10 +1907,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 +1923,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 +1947,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 +1973,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 +2012,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 })?; @@ -2685,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, @@ -2811,6 +2821,7 @@ mod tests { .unwrap(); 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(&store), @@ -2818,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), @@ -3823,7 +3835,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, @@ -3832,7 +3844,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 @@ -3873,7 +3885,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, @@ -3884,7 +3896,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" ); } @@ -3924,9 +3936,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. @@ -3948,8 +3960,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_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 // input whenever the previous funding transaction touched it (e.g. it funded the original @@ -3972,7 +3984,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_page(None).await.unwrap().objects.is_empty()); // Control: a funding transaction the wallet participates in is still recorded. let script_pubkey = wallet @@ -3989,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); + 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())); } @@ -4038,8 +4050,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_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); @@ -4053,10 +4065,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. @@ -4068,7 +4080,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 @@ -4081,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]); @@ -4126,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).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(); @@ -4135,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); - 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)); @@ -4160,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()); @@ -4199,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); - 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, diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 9c1bb75c03..85f618c958 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, @@ -224,7 +226,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 +324,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(); @@ -440,8 +442,60 @@ 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 { + 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, mut f: F, + ) -> Vec { + self.list_all_payments().into_iter().filter(|p| f(&p)).collect() + } +} + 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 +513,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 +532,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 +1175,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 +1249,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 +1327,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); @@ -1307,12 +1361,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 +1407,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 +1447,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 +1483,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 +1516,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,35 +1552,31 @@ 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)); 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 ); @@ -1551,7 +1602,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 @@ -1573,7 +1624,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 @@ -1750,13 +1801,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 53f60d19f7..88cc62102f 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 c1e973091d..fd247f74cf 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; @@ -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, .. } => { @@ -1103,13 +1103,13 @@ 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(); + 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); @@ -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); } @@ -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, .. } => { @@ -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)); @@ -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), ) }; @@ -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, @@ -2474,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 @@ -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, @@ -2828,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 { @@ -2855,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, .. } => { @@ -2893,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); @@ -2921,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); @@ -2956,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) }) @@ -2965,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); @@ -2990,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); @@ -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)] @@ -3704,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); @@ -3714,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 { .. }) }); @@ -4114,12 +4121,15 @@ 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); // 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 { .. }) }); @@ -4136,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!( @@ -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, .. } => { @@ -4468,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 6e6d2d2783..efab1480ea 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 diff --git a/tests/upgrade_downgrade_tests.rs b/tests/upgrade_downgrade_tests.rs index 84d528944f..7af7c60309 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);