diff --git a/CHANGELOG.md b/CHANGELOG.md index 07078cfa65..61133becae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,10 @@ # Pending ## Compatibility Notes +- Migrating between storage backends does not preserve the relative creation order of + pre-existing payments, as the generic KV store migration copies entries in an unspecified + order. Expect the order in which `Node::list_payments` returns pre-existing payments to + change once after such a migration. Payment contents and completeness are unaffected. - Pending JIT-channel payments created before upgrading may fail after upgrade because the prior LSPS2 fee-limit state stored in `PaymentKind::Bolt11Jit` is not migrated. - Upgrading from LDK Node v0.1 is no longer supported if the event queue still contains @@ -9,6 +13,13 @@ `v0.1.0-alpha.0` before upgrading LDK Node. ## Feature and API updates +- `Node::list_payments` is now paginated: it takes an optional `PageToken` and returns a + `PaymentDetailsPage` holding one page of payments, ordered from most recently created to + least recently created, plus the token for the next page. Ordering and page tokens come + from the configured storage backend, so a token stays valid across restarts. This replaces + the previous unpaginated `Node::list_payments`, and `Node::list_payments_with_filter` has + been removed; filter the returned pages instead. +- `Node::payment` now returns a `Result`, as retrieving a payment may fail. - The Bitcoin Core RPC and REST chain-source builder methods now accept an optional `wallet_rescan_from_height` argument. Passing a height lets fresh wallets rescan from a known birthday block instead of checkpointing at the current tip, which is useful when restoring a diff --git a/bindings/kotlin/ldk-node-jvm/lib/src/test/kotlin/org/lightningdevkit/ldknode/LibraryTest.kt b/bindings/kotlin/ldk-node-jvm/lib/src/test/kotlin/org/lightningdevkit/ldknode/LibraryTest.kt index 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 c1a926f2fd..9465ba07bc 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(); @@ -235,6 +237,7 @@ enum NodeError { "InvalidDateTime", "InvalidFeeRate", "InvalidScriptPubKey", + "InvalidPageToken", "DuplicatePayment", "UnsupportedCurrency", "InsufficientFunds", @@ -277,6 +280,10 @@ enum PaymentFailureReason { typedef dictionary PaymentDetails; +typedef dictionary PaymentDetailsPage; + +typedef interface PageToken; + [Remote] dictionary RouteParametersConfig { u64? max_total_routing_fee_msat; diff --git a/src/builder.rs b/src/builder.rs index f117800996..f538e99b83 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::{ @@ -1442,10 +1444,11 @@ fn build_with_store_internal( let (payment_store_res, node_metris_res, pending_payment_store_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)), @@ -1473,7 +1476,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), @@ -1728,8 +1735,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 772c4bd806..e5b96bcaae 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,21 @@ 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 storage backends' page size, so warming the cache costs a single page listing +// and one batch of reads, and the first page of `Node::list_payments` is served without going to +// the store at all. The remaining capacity fills as payments are used. +pub(crate) const PAYMENT_CACHE_WARMUP_COUNT: NonZeroUsize = NonZeroUsize::new(50).unwrap(); + // The default {Esplora,Electrum} client timeout we're using. const DEFAULT_PER_REQUEST_TIMEOUT_SECS: u8 = 10; diff --git a/src/data_store.rs b/src/data_store.rs index b1ed816df9..00ca690b21 100644 --- a/src/data_store.rs +++ b/src/data_store.rs @@ -5,14 +5,17 @@ // http://opensource.org/licenses/MIT>, at your option. You may not use this file except in // accordance with one or both of these licenses. -use std::collections::HashMap; +use std::collections::{BTreeMap, HashMap}; +use std::marker::PhantomData; +use std::num::NonZeroUsize; use std::ops::Deref; use std::sync::{Arc, Mutex}; -use lightning::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::logger::{log_debug, log_error, LdkLogger}; use crate::types::DynStore; use crate::Error; @@ -25,8 +28,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,143 +50,622 @@ 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`]. +/// +/// Modelled as an enum rather than a map plus a policy field so that a [`KeepAllEntries`] store +/// provably pays nothing for the bookkeeping a bounded one needs: its representation is just the +/// map it always was. +enum ObjectCache { + KeepAll(HashMap), + BoundedLru(LruCache), +} + +impl ObjectCache { + fn new(cache_limit: CacheLimit, objects: Vec) -> Self { + match cache_limit { + CacheLimit::Unbounded => Self::KeepAll(HashMap::from_iter( + objects.into_iter().map(|object| (object.id(), object)), + )), + CacheLimit::Bounded(capacity) => { + let mut lru = LruCache::new(capacity); + for object in objects { + lru.insert(object.id(), object); + } + Self::BoundedLru(lru) + }, + } + } + + /// Whether the cache holds *every* object of the namespace. + /// + /// If it does, a miss proves the object is absent from the store, and iterating the cache + /// yields a complete listing. If it doesn't, both require reading from the store. + fn is_authoritative(&self) -> bool { + matches!(self, Self::KeepAll(_)) + } + + /// Returns the cached object for `id`, marking it as most recently used. + fn get(&mut self, id: &SO::Id) -> Option { + match self { + Self::KeepAll(objects) => objects.get(id).cloned(), + Self::BoundedLru(lru) => lru.get(id), + } + } + + /// Returns 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_authoritative`]. + fn filter bool>(&self, f: F) -> Vec { + match self { + Self::KeepAll(objects) => objects.values().filter(f).cloned().collect(), + Self::BoundedLru(lru) => { + lru.entries.values().map(|(object, _)| object).filter(f).cloned().collect() + }, + } + } + + #[cfg(test)] + fn len(&self) -> usize { + match self { + Self::KeepAll(objects) => objects.len(), + Self::BoundedLru(lru) => lru.entries.len(), + } + } +} + +/// 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, } } + /// Stores `object`, overwriting any object previously stored under the same id. + /// + /// Returns whether an object was previously stored under that id. pub(crate) async fn insert(&self, object: SO) -> Result { - let _guard = self.mutation_lock.lock().await; + let _guard = self.mutation_lock.write().await; + let id = object.id(); + // Callers treat the return value as "this id was already known", so a cache miss is not + // enough to answer it under a bounded policy. + let replaced = self.contains(&id).await?; self.persist(&object).await?; - let mut locked_objects = self.objects.lock().expect("lock"); - let updated = locked_objects.insert(object.id(), object).is_some(); - Ok(updated) + self.cache.lock().expect("lock").insert(id, object); + Ok(replaced) } + /// Merges `object` into any object already stored under the same id, or stores it as-is if + /// there is none. + /// + /// Returns whether anything was written. pub(crate) async fn insert_or_update(&self, object: SO) -> Result { - let _guard = self.mutation_lock.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( + let _guard = self.mutation_lock.write().await; + + if !self.contains(id).await? { + return Ok(()); + } + + let store_key = id.encode_to_hex_str(); + KVStore::remove( + &*self.kv_store, + &self.primary_namespace, + &self.secondary_namespace, + &store_key, + false, + ) + .await + .map_err(|e| { + log_error!( + self.logger, + "Removing object data for key {}/{}/{} failed due to: {}", + &self.primary_namespace, + &self.secondary_namespace, + store_key, + e + ); + Error::PersistenceFailed + })?; + self.cache.lock().expect("lock").remove(id); + Ok(()) + } + + /// Returns the object stored under `id`, if any. + pub(crate) async fn get(&self, id: &SO::Id) -> Result, Error> { + let _guard = self.mutation_lock.read().await; + self.lookup(id).await + } + + /// Applies `update` to the object stored under its id. + pub(crate) async fn update(&self, update: SO::Update) -> Result { + let _guard = self.mutation_lock.write().await; + + let id = update.id(); + // As in `insert_or_update`, a cache miss is not evidence of absence: reporting `NotFound` + // for a merely evicted object would drop the update on the floor. + let Some(mut updated_object) = self.lookup(&id).await? else { + return Ok(DataStoreUpdateResult::NotFound); + }; + if !updated_object.update(update) { + return Ok(DataStoreUpdateResult::Unchanged); + } + + self.persist(&updated_object).await?; + self.cache.lock().expect("lock").insert(id, updated_object); + Ok(DataStoreUpdateResult::Updated) + } + + /// 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. That keeps tokens valid across restarts + /// and across changes to our caching, and it is why an object updated mid-pagination cannot + /// shift position and so be skipped or returned twice. + /// + /// Note this deliberately does not hold the mutation lock across its reads: a listing must not + /// block every writer for the duration of a round trip to a remote backend. Objects created or + /// removed while paginating may or may not be observed. + /// + /// Note also that a page may hold fewer objects than the backend's page size, because objects + /// removed between listing the keys and reading them are skipped. Iterate until + /// `next_page_token` is `None` rather than until a short page. + pub(crate) async fn list_page( + &self, page_token: Option, + ) -> Result, Error> { + let response = PaginatedKVStore::list_paginated( + &*self.kv_store, + &self.primary_namespace, + &self.secondary_namespace, + page_token, + ) + .await + .map_err(|e| { + log_error!( + self.logger, + "Listing objects under {}/{} failed due to: {}", + &self.primary_namespace, + &self.secondary_namespace, + e + ); + // The backend rejects a token it didn't issue, which is the caller's problem rather + // than a persistence failure. + if e.kind() == ErrorKind::InvalidInput { + Error::InvalidPageToken + } else { + Error::PersistenceFailed + } + })?; + + // Serve whatever we already hold, and note the rest to read below. We take the mutation + // lock only for this, so that we observe a consistent view of the cache without holding up + // writers while we read. + let mut objects: Vec> = vec![None; response.keys.len()]; + let mut missing = Vec::new(); + { + let _guard = self.mutation_lock.read().await; + let locked_cache = self.cache.lock().expect("lock"); + for (idx, key) in response.keys.iter().enumerate() { + // Note we deliberately peek rather than `get` here: a listing sweep walks the whole + // namespace, so letting it count as "use" would evict the working set it walks past. + match SO::Id::decode_from_hex_str(key).and_then(|id| locked_cache.peek(&id)) { + Some(object) => objects[idx] = Some(object), + None => missing.push((idx, key.clone())), + } + } + } + + self.read_missing(&mut objects, missing).await?; + + Ok(DataStorePage { + objects: objects.into_iter().flatten().collect(), + next_page_token: response.next_page_token, + }) + } + + /// Reads the objects we couldn't serve from the cache into their slots in `objects`. + /// + /// Reads run concurrently but are tracked by slot, as the order in which they finish says + /// nothing about the order of the page. Note the objects read here are deliberately *not* + /// cached, see [`Self::list_page`]. + async fn read_missing( + &self, objects: &mut [Option], missing: Vec<(usize, String)>, + ) -> Result<(), Error> { + // Matches every backend's page size, so in practice this is a single wave. We keep refilling + // anyway so that a backend paging more coarsely degrades gracefully rather than spawning an + // unbounded number of tasks. + const BATCH_SIZE: usize = 50; + + type ReadResult = (usize, String, Result, lightning::io::Error>); + let spawn_read = |set: &mut tokio::task::JoinSet, idx: usize, key: String| { + let read_fut = KVStore::read( &*self.kv_store, &self.primary_namespace, &self.secondary_namespace, - &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 - ); + &key, + ); + set.spawn(async move { (idx, key, read_fut.await) }); + }; + + let mut missing = missing.into_iter(); + let mut set = tokio::task::JoinSet::new(); + while set.len() < BATCH_SIZE { + let Some((idx, key)) = missing.next() else { break }; + spawn_read(&mut set, idx, key); + } + + while let Some(join_res) = set.join_next().await { + let (idx, key, read_res) = join_res.map_err(|e| { + log_error!(self.logger, "Failed to join object read task: {}", e); + set.abort_all(); Error::PersistenceFailed })?; - self.objects.lock().expect("lock").remove(id); + + if let Some((next_idx, next_key)) = missing.next() { + spawn_read(&mut set, next_idx, next_key); + } + + match read_res { + Ok(bytes) => match SO::read(&mut &bytes[..]) { + Ok(object) => objects[idx] = Some(object), + Err(e) => { + log_error!( + self.logger, + "Failed to deserialize object for key {}/{}/{}: {}", + &self.primary_namespace, + &self.secondary_namespace, + key, + e + ); + set.abort_all(); + return Err(Error::PersistenceFailed); + }, + }, + // The object was removed between us listing the keys and reading it, which is + // indistinguishable from it having been removed just before the listing. Skip it. + Err(e) if e.kind() == ErrorKind::NotFound => { + log_debug!( + self.logger, + "Skipping concurrently removed key {}/{}/{}", + &self.primary_namespace, + &self.secondary_namespace, + key + ); + }, + Err(e) => { + log_error!( + self.logger, + "Read for key {}/{}/{} failed due to: {}", + &self.primary_namespace, + &self.secondary_namespace, + key, + e + ); + set.abort_all(); + return Err(Error::PersistenceFailed); + }, + } } + Ok(()) } - /// Returns the current in-memory object for `id`. + /// Returns the object stored under `id`, reading through to the [`KVStore`] if the cache is + /// not authoritative and misses. /// - /// 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() + /// The caller must hold `mutation_lock`. + async fn lookup(&self, id: &SO::Id) -> Result, Error> { + let (cached_object, is_authoritative) = { + let mut locked_cache = self.cache.lock().expect("lock"); + (locked_cache.get(id), locked_cache.is_authoritative()) + }; + + if let Some(object) = cached_object { + return Ok(Some(object)); + } + if is_authoritative { + return Ok(None); + } + + let Some(bytes) = self.read_raw(id).await? else { + return Ok(None); + }; + let object = self.decode(id, &bytes)?; + self.cache.lock().expect("lock").insert(id.clone(), object.clone()); + Ok(Some(object)) } - pub(crate) async fn update(&self, update: SO::Update) -> Result { - let _guard = self.mutation_lock.lock().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 + /// Returns whether an object is stored under `id`, without deserializing it or caching it. + /// + /// The caller must hold `mutation_lock`. + async fn contains(&self, id: &SO::Id) -> Result { + let (is_cached, is_authoritative) = { + let locked_cache = self.cache.lock().expect("lock"); + (locked_cache.contains(id), locked_cache.is_authoritative()) }; - self.persist(&updated_object).await?; - let mut locked_objects = self.objects.lock().expect("lock"); - locked_objects.insert(id, updated_object); - Ok(DataStoreUpdateResult::Updated) + if is_cached { + return Ok(true); + } + if is_authoritative { + return Ok(false); + } + + Ok(self.read_raw(id).await?.is_some()) } - /// Returns in-memory objects matching `f`. + /// 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> { @@ -211,27 +700,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], @@ -241,12 +778,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 { @@ -258,6 +802,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 { @@ -269,22 +820,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; @@ -330,31 +888,131 @@ 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, ) } - #[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 data_store: DataStore> = DataStore::new( - Vec::new(), - primary_namespace.clone(), - secondary_namespace.clone(), - Arc::clone(&store), - logger, - ); - - let id = TestObjectId { id: [42u8; 4] }; - assert!(data_store.get(&id).is_none()); + /// 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, + } - let store_key = id.encode_to_hex_str(); + 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(true), writer.await.unwrap()); + assert_eq!(Some(new_object), data_store.get(&id).await.unwrap()); + } + + #[tokio::test] + async fn data_is_persisted() { + let store: Arc = Arc::new(DynStoreWrapper(InMemoryStore::new())); + let logger = Arc::new(TestLogger::new()); + 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), + logger, + ); + + let id = TestObjectId { id: [42u8; 4] }; + assert!(data_store.get(&id).await.unwrap().is_none()); + + let store_key = id.encode_to_hex_str(); // Check we start empty. assert!(KVStore::read(&*store, &primary_namespace, &secondary_namespace, &store_key) @@ -362,9 +1020,9 @@ mod tests { .is_err()); // Check we successfully store an object and return `false` - let object = TestObject { id, data: [23u8; 3] }; + let object = TestObject::new(id, [23u8; 3]); assert_eq!(Ok(false), data_store.insert(object.clone()).await); - assert_eq!(Some(object), data_store.get(&id)); + assert_eq!(Some(object), data_store.get(&id).await.unwrap()); assert!(KVStore::read(&*store, &primary_namespace, &secondary_namespace, &store_key) .await .is_ok()); @@ -373,25 +1031,25 @@ mod tests { let mut override_object = object.clone(); override_object.data = [24u8; 3]; assert_eq!(Ok(true), data_store.insert(override_object).await); - assert_eq!(Some(override_object), data_store.get(&id)); + assert_eq!(Some(override_object), data_store.get(&id).await.unwrap()); // Check update returns `Updated` - let update = TestObjectUpdate { id, data: [25u8; 3] }; + 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 @@ -406,50 +1064,743 @@ mod tests { #[tokio::test] async fn insert_or_update_does_not_mutate_memory_if_persist_fails() { let existing_id = TestObjectId { id: [42u8; 4] }; - let existing_object = TestObject { id: existing_id, data: [23u8; 3] }; + let existing_object = TestObject::new(existing_id, [23u8; 3]); let data_store = new_failing_data_store(vec![existing_object]); - let updated_object = TestObject { id: existing_id, data: [24u8; 3] }; + let updated_object = TestObject::new(existing_id, [24u8; 3]); assert_eq!( Err(Error::PersistenceFailed), data_store.insert_or_update(updated_object).await ); - 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 or list, so that tests can assert a + /// [`KeepAllEntries`] store never goes to the `KVStore` for a read. + struct CountingStore { + inner: InMemoryStore, + reads: Arc, + lists: Arc, + } + + impl KVStore for CountingStore { + fn read( + &self, primary_namespace: &str, secondary_namespace: &str, key: &str, + ) -> impl std::future::Future, io::Error>> + 'static + Send { + self.reads.fetch_add(1, Ordering::Relaxed); + self.inner.read(primary_namespace, secondary_namespace, key) + } + + fn write( + &self, primary_namespace: &str, secondary_namespace: &str, key: &str, buf: Vec, + ) -> impl std::future::Future> + 'static + Send { + self.inner.write(primary_namespace, secondary_namespace, key, buf) + } + + fn remove( + &self, primary_namespace: &str, secondary_namespace: &str, key: &str, lazy: bool, + ) -> impl std::future::Future> + 'static + Send { + self.inner.remove(primary_namespace, secondary_namespace, key, lazy) + } + + fn list( + &self, primary_namespace: &str, secondary_namespace: &str, + ) -> impl std::future::Future, io::Error>> + 'static + Send { + self.lists.fetch_add(1, Ordering::Relaxed); + self.inner.list(primary_namespace, secondary_namespace) + } + } + + impl PaginatedKVStore for CountingStore { + fn list_paginated( + &self, primary_namespace: &str, secondary_namespace: &str, + page_token: Option, + ) -> impl std::future::Future> + 'static + Send + { + self.lists.fetch_add(1, Ordering::Relaxed); + self.inner.list_paginated(primary_namespace, secondary_namespace, page_token) + } + } + + /// A store whose writes and removals can be made to fail on demand, while reads keep working. + /// + /// Note a store that fails *reads* would be useless for testing the write paths of a bounded + /// store, because it would already fail in the read-through that precedes the write. + struct WriteFailingStore { + inner: InMemoryStore, + fail_writes: Arc, + } + + impl KVStore for WriteFailingStore { + fn read( + &self, primary_namespace: &str, secondary_namespace: &str, key: &str, + ) -> impl std::future::Future, io::Error>> + 'static + Send { + self.inner.read(primary_namespace, secondary_namespace, key) + } + + fn write( + &self, primary_namespace: &str, secondary_namespace: &str, key: &str, buf: Vec, + ) -> impl std::future::Future> + 'static + Send { + let failing = self.fail_writes.load(Ordering::Relaxed); + let inner_fut = self.inner.write(primary_namespace, secondary_namespace, key, buf); + async move { + if failing { + return Err(io::Error::new(io::ErrorKind::Other, "write failed")); + } + inner_fut.await + } + } + + fn remove( + &self, primary_namespace: &str, secondary_namespace: &str, key: &str, lazy: bool, + ) -> impl std::future::Future> + 'static + Send { + let failing = self.fail_writes.load(Ordering::Relaxed); + let inner_fut = self.inner.remove(primary_namespace, secondary_namespace, key, lazy); + async move { + if failing { + return Err(io::Error::new(io::ErrorKind::Other, "remove failed")); + } + inner_fut.await + } + } + + fn list( + &self, primary_namespace: &str, secondary_namespace: &str, + ) -> impl std::future::Future, io::Error>> + 'static + Send { + self.inner.list(primary_namespace, secondary_namespace) + } + } + + impl PaginatedKVStore for WriteFailingStore { + fn list_paginated( + &self, primary_namespace: &str, secondary_namespace: &str, + page_token: Option, + ) -> impl std::future::Future> + 'static + Send + { + self.inner.list_paginated(primary_namespace, secondary_namespace, page_token) + } + } + + /// Returns a bounded store of the given capacity, together with a handle on the underlying + /// `KVStore`, and the ids of `num_objects` objects inserted through it. + /// + /// The objects are inserted in ascending id order, so with `capacity < num_objects` the + /// lowest ids have been evicted from memory by the time this returns, while remaining + /// available in the store. + async fn new_lru_store_with_objects( + capacity: usize, num_objects: u8, + ) -> ( + DataStore, KeepLeastRecentlyUsed>, + Arc, + Vec, + ) { + let kv_store = in_memory_store(); + let data_store = new_data_store(Arc::clone(&kv_store), keep_lru(capacity), Vec::new()); + let mut ids = Vec::new(); + for i in 0..num_objects { + let id = test_id(i); + data_store.insert(TestObject::new(id, [i; 3])).await.unwrap(); + ids.push(id); + } + (data_store, kv_store, ids) + } + + #[tokio::test] + async fn keep_all_never_reads_from_the_store() { + let reads = Arc::new(AtomicUsize::new(0)); + let lists = Arc::new(AtomicUsize::new(0)); + let kv_store: Arc = Arc::new(DynStoreWrapper(CountingStore { + inner: InMemoryStore::new(), + reads: Arc::clone(&reads), + lists: Arc::clone(&lists), + })); + let data_store = new_data_store(kv_store, KeepAllEntries, Vec::new()); + + let id = test_id(1); + let missing_id = test_id(2); + let object = TestObject::new(id, [23u8; 3]); + assert_eq!(Ok(false), data_store.insert(object).await); + + assert_eq!(Some(object), data_store.get(&id).await.unwrap()); + assert_eq!(None, data_store.get(&missing_id).await.unwrap()); + assert!(data_store.contains_key(&id).await.unwrap()); + assert!(!data_store.contains_key(&missing_id).await.unwrap()); + assert_eq!(1, data_store.list_filter(|_| true).await.len()); + let no_op = TestObjectUpdate { id, data: [23u8; 3], extra: None }; + assert_eq!(Ok(DataStoreUpdateResult::Unchanged), data_store.update(no_op).await); + + // The whole point of `KeepAllEntries` is that memory is the complete truth, so none of the + // above may go to the store. + assert_eq!(0, reads.load(Ordering::Relaxed)); + assert_eq!(0, lists.load(Ordering::Relaxed)); + } + + #[tokio::test] + async fn lru_evicts_least_recently_used() { + let (data_store, _kv_store, ids) = new_lru_store_with_objects(2, 3).await; + + assert_eq!(2, data_store.cached_len()); + assert!(!data_store.is_cached(&ids[0])); + assert!(data_store.is_cached(&ids[1])); + assert!(data_store.is_cached(&ids[2])); + } + + #[tokio::test] + async fn lru_get_marks_an_entry_as_recently_used() { + let (data_store, _kv_store, ids) = new_lru_store_with_objects(2, 2).await; + + assert!(data_store.get(&ids[0]).await.unwrap().is_some()); + + // With `ids[0]` freshly used, inserting a third object must evict `ids[1]` instead. + let new_id = test_id(9); + data_store.insert(TestObject::new(new_id, [9u8; 3])).await.unwrap(); + assert!(data_store.is_cached(&ids[0])); + assert!(!data_store.is_cached(&ids[1])); + assert!(data_store.is_cached(&new_id)); + } + + #[tokio::test] + async fn lru_get_reads_through_and_caches() { + let (data_store, _kv_store, ids) = new_lru_store_with_objects(1, 2).await; + let evicted_id = ids[0]; + assert!(!data_store.is_cached(&evicted_id)); + + let object = data_store.get(&evicted_id).await.unwrap(); + assert_eq!(Some(TestObject::new(evicted_id, [0u8; 3])), object); + assert!(data_store.is_cached(&evicted_id)); + assert_eq!(1, data_store.cached_len()); + } + + #[tokio::test] + async fn lru_contains_key_reads_through() { + let (data_store, _kv_store, ids) = new_lru_store_with_objects(1, 2).await; + let evicted_id = ids[0]; + assert!(!data_store.is_cached(&evicted_id)); + + assert!(data_store.contains_key(&evicted_id).await.unwrap()); + assert!(!data_store.contains_key(&test_id(99)).await.unwrap()); + // A mere existence probe must not displace the working set. + assert!(!data_store.is_cached(&evicted_id)); + } + + #[tokio::test] + async fn lru_update_reads_through_evicted_entry() { + let (data_store, _kv_store, ids) = new_lru_store_with_objects(1, 2).await; + let evicted_id = ids[0]; + assert!(!data_store.is_cached(&evicted_id)); + + // Without reading through, the evicted object would look absent and the update would be + // dropped on the floor. + let update = TestObjectUpdate { id: evicted_id, data: [25u8; 3], extra: None }; + assert_eq!(Ok(DataStoreUpdateResult::Updated), data_store.update(update).await); + assert_eq!([25u8; 3], data_store.get(&evicted_id).await.unwrap().unwrap().data); + } + + #[tokio::test] + async fn lru_insert_or_update_does_not_clobber_evicted_entry() { + let kv_store = in_memory_store(); + let data_store = new_data_store(kv_store, keep_lru(1), Vec::new()); + + let id = test_id(1); + let stored = TestObject { id, data: [23u8; 3], extra: Some(42) }; + data_store.insert(stored).await.unwrap(); + data_store.insert(TestObject::new(test_id(2), [24u8; 3])).await.unwrap(); + assert!(!data_store.is_cached(&id)); + + // The incoming object carries no `extra`, so merging must preserve the stored one. Without + // reading through, the evicted object would be overwritten wholesale and `extra` lost. + let incoming = TestObject { id, data: [25u8; 3], extra: None }; + assert_eq!(Ok(true), data_store.insert_or_update(incoming).await); + + let merged = data_store.get(&id).await.unwrap().unwrap(); + assert_eq!([25u8; 3], merged.data); + assert_eq!(Some(42), merged.extra); + } + + #[tokio::test] + async fn lru_insert_reports_replacement_of_evicted_entry() { + let (data_store, _kv_store, ids) = new_lru_store_with_objects(1, 2).await; + let evicted_id = ids[0]; + assert!(!data_store.is_cached(&evicted_id)); + + // Callers rely on this to detect ids they have already seen, so it must not be answered + // from the cache alone. + assert_eq!(Ok(true), data_store.insert(TestObject::new(evicted_id, [99u8; 3])).await); + assert_eq!(Ok(false), data_store.insert(TestObject::new(test_id(99), [99u8; 3])).await); + } + + #[tokio::test] + async fn lru_remove_removes_evicted_entry() { + let (data_store, kv_store, ids) = new_lru_store_with_objects(1, 2).await; + let evicted_id = ids[0]; + assert!(!data_store.is_cached(&evicted_id)); + + data_store.remove(&evicted_id).await.unwrap(); + + assert_eq!(None, data_store.get(&evicted_id).await.unwrap()); + let store_key = evicted_id.encode_to_hex_str(); + assert!(KVStore::read( + &*kv_store, + TEST_PRIMARY_NAMESPACE, + TEST_SECONDARY_NAMESPACE, + &store_key + ) + .await + .is_err()); + } + + #[tokio::test] + async fn lru_seeding_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 8546af0dd2..71ba9467e2 100644 --- a/src/error.rs +++ b/src/error.rs @@ -115,6 +115,8 @@ pub enum Error { InvalidFeeRate, /// The given script public key is invalid. InvalidScriptPubKey, + /// The given page token is invalid. + InvalidPageToken, /// A payment with the given hash has already been initiated. DuplicatePayment, /// The provided offer was denonminated in an unsupported currency. @@ -199,6 +201,7 @@ impl fmt::Display for Error { Self::InvalidDateTime => write!(f, "The given date time is invalid."), Self::InvalidFeeRate => write!(f, "The given fee rate is invalid."), Self::InvalidScriptPubKey => write!(f, "The given script pubkey is invalid."), + Self::InvalidPageToken => write!(f, "The given page token is invalid."), Self::DuplicatePayment => { write!(f, "A payment with the given hash has already been initiated.") }, diff --git a/src/event.rs b/src/event.rs index a5d04217b0..00ba598570 100644 --- a/src/event.rs +++ b/src/event.rs @@ -793,7 +793,13 @@ where .. } => { let payment_id = PaymentId(payment_hash.0); - let payment_info = self.payment_store.get(&payment_id); + let payment_info = match self.payment_store.get(&payment_id).await { + Ok(payment_info) => payment_info, + Err(e) => { + log_error!(self.logger, "Failed to access payment store: {}", e); + return Err(ReplayEvent()); + }, + }; if let Some(info) = payment_info.as_ref() { if info.direction == PaymentDirection::Outbound { log_info!( @@ -1211,24 +1217,31 @@ where }, }; - self.payment_store.get(&payment_id).map(|payment| { - let amount_msat = payment.amount_msat.expect( - "outbound payments should record their amount before they can succeed", - ); - log_info!( - self.logger, - "Successfully sent payment of {}msat{} from \ - payment hash {:?} with preimage {:?}", - amount_msat, - if let Some(fee) = fee_paid_msat { - format!(" (fee {} msat)", fee) - } else { - "".to_string() - }, - hex_utils::to_string(&payment_hash.0), - hex_utils::to_string(&payment_preimage.0) - ); - }); + match self.payment_store.get(&payment_id).await { + Ok(Some(payment)) => { + let amount_msat = payment.amount_msat.expect( + "outbound payments should record their amount before they can succeed", + ); + log_info!( + self.logger, + "Successfully sent payment of {}msat{} from \ + payment hash {:?} with preimage {:?}", + amount_msat, + if let Some(fee) = fee_paid_msat { + format!(" (fee {} msat)", fee) + } else { + "".to_string() + }, + hex_utils::to_string(&payment_hash.0), + hex_utils::to_string(&payment_preimage.0) + ); + }, + Ok(None) => {}, + Err(e) => { + log_error!(self.logger, "Failed to access payment store: {}", e); + return Err(ReplayEvent()); + }, + }; let event = Event::PaymentSuccessful { payment_id: Some(payment_id), payment_hash, diff --git a/src/ffi/types.rs b/src/ffi/types.rs index 0dc79758d6..a1f94a11cb 100644 --- a/src/ffi/types.rs +++ b/src/ffi/types.rs @@ -37,6 +37,7 @@ use lightning::offers::static_invoice::StaticInvoice as LdkStaticInvoice; use lightning::onion_message::dns_resolution::HumanReadableName as LdkHumanReadableName; pub use lightning::routing::gossip::{NodeAlias, NodeId, RoutingFees}; pub use lightning::routing::router::RouteParametersConfig; +use lightning::util::persist::PageToken as LdkPageToken; use lightning::util::ser::{Readable, Writeable, Writer}; use lightning_invoice::{Bolt11Invoice as LdkBolt11Invoice, Bolt11InvoiceDescriptionRef}; pub use lightning_invoice::{Description, SignedRawBolt11Invoice}; @@ -2745,3 +2746,50 @@ mod tests { assert_eq!(hrn1, hrn3); } } + +/// An opaque token used to continue a paginated listing. +/// +/// Obtain one from the page returned by a listing call and pass it back to retrieve the next page. +/// The value returned by `to_string` may be persisted and handed to the constructor later, so that +/// pagination can be resumed after a restart. +/// +/// The representation is defined by the storage backend and must be treated as opaque. A token is +/// only meaningful to the backend that issued it. +#[derive(Debug, Clone, PartialEq, Eq, uniffi::Object)] +#[uniffi::export(Debug, Display, Eq)] +pub struct PageToken { + pub(crate) inner: LdkPageToken, +} + +#[uniffi::export] +impl PageToken { + /// Constructs a token from the representation previously obtained via `to_string`. + #[uniffi::constructor] + pub fn new(token: String) -> Self { + Self { inner: LdkPageToken::new(token) } + } +} + +impl From for PageToken { + fn from(inner: LdkPageToken) -> Self { + Self { inner } + } +} + +impl From for LdkPageToken { + fn from(wrapper: PageToken) -> Self { + wrapper.inner + } +} + +impl AsRef for PageToken { + fn as_ref(&self) -> &LdkPageToken { + &self.inner + } +} + +impl std::fmt::Display for PageToken { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.inner) + } +} diff --git a/src/hex_utils.rs b/src/hex_utils.rs index 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..a60bbe6e30 100644 --- a/src/io/utils.rs +++ b/src/io/utils.rs @@ -7,6 +7,7 @@ use std::fs::{self, OpenOptions}; use std::io::Write; +use std::num::NonZeroUsize; use std::ops::Deref; #[cfg(unix)] use std::os::unix::fs::OpenOptionsExt; @@ -26,7 +27,7 @@ use lightning::routing::scoring::{ ChannelLiquidities, ProbabilisticScorer, ProbabilisticScoringDecayParameters, }; use lightning::util::persist::{ - migrate_kv_store_data_async, KVStore, KVSTORE_NAMESPACE_KEY_ALPHABET, + migrate_kv_store_data_async, KVStore, PaginatedKVStore, KVSTORE_NAMESPACE_KEY_ALPHABET, KVSTORE_NAMESPACE_KEY_MAX_LEN, NETWORK_GRAPH_PERSISTENCE_KEY, NETWORK_GRAPH_PERSISTENCE_PRIMARY_NAMESPACE, NETWORK_GRAPH_PERSISTENCE_SECONDARY_NAMESPACE, OUTPUT_SWEEPER_PERSISTENCE_KEY, OUTPUT_SWEEPER_PERSISTENCE_PRIMARY_NAMESPACE, @@ -222,69 +223,141 @@ where }) } -/// Read all objects of type `T` from the given namespace, spawning reads in parallel. +/// Reads all objects of type `T` from the given namespace, ordered from most recently created to +/// least recently created. pub(crate) async fn read_all_objects( kv_store: &DynStore, primary_namespace: &str, secondary_namespace: &str, logger: L, ) -> Result, std::io::Error> +where + T: Readable, + L: Deref, + L::Target: LdkLogger, +{ + read_objects_internal(kv_store, primary_namespace, secondary_namespace, None, logger).await +} + +/// Reads the `num_objects` most recently created objects of type `T` from the given namespace, +/// ordered from most recently created to least recently created. +/// +/// Returns fewer objects if the namespace holds fewer than `num_objects`. +pub(crate) async fn read_n_objects( + kv_store: &DynStore, primary_namespace: &str, secondary_namespace: &str, + num_objects: NonZeroUsize, logger: L, +) -> Result, std::io::Error> +where + T: Readable, + L: Deref, + L::Target: LdkLogger, +{ + read_objects_internal( + kv_store, + primary_namespace, + secondary_namespace, + Some(num_objects), + logger, + ) + .await +} + +/// Reads up to `num_objects` objects of type `T` from the given namespace, or all of them if +/// `None`, spawning reads in parallel. +/// +/// Objects are returned in the store's own creation order, most recently created first. Note we +/// take the keys from [`PaginatedKVStore::list_paginated`] rather than [`KVStore::list`], because +/// the latter is documented to return them in arbitrary order, which would make "the newest +/// `num_objects`" meaningless. +async fn read_objects_internal( + kv_store: &DynStore, primary_namespace: &str, secondary_namespace: &str, + num_objects: Option, logger: L, +) -> Result, std::io::Error> where T: Readable, L: Deref, L::Target: LdkLogger, { let type_name = std::any::type_name::(); - let mut res = Vec::new(); + let max_objects = num_objects.map_or(usize::MAX, |num_objects| num_objects.get()); + + // Collect the keys we're after, page by page, so that a bounded read doesn't pay for keys it + // would never look at. + let mut stored_keys: Vec = Vec::new(); + let mut page_token = None; + loop { + let response = PaginatedKVStore::list_paginated( + &*kv_store, + primary_namespace, + secondary_namespace, + page_token, + ) + .await?; + + let remaining = max_objects.saturating_sub(stored_keys.len()); + stored_keys.extend(response.keys.into_iter().take(remaining)); - let mut stored_keys = KVStore::list(&*kv_store, primary_namespace, secondary_namespace).await?; + if stored_keys.len() >= max_objects { + break; + } + + match response.next_page_token { + Some(next_page_token) => page_token = Some(next_page_token), + None => break, + } + } const BATCH_SIZE: usize = 50; + // Reads are tracked by slot, as the order in which they finish says nothing about the order we + // promised to return them in. + let mut objects: Vec> = Vec::new(); + objects.resize_with(stored_keys.len(), || None); + let mut keys = stored_keys.into_iter().enumerate(); let mut set = tokio::task::JoinSet::new(); // Fill JoinSet with tasks if possible - while set.len() < BATCH_SIZE && !stored_keys.is_empty() { - if let Some(next_key) = stored_keys.pop() { - let fut = KVStore::read(kv_store, primary_namespace, secondary_namespace, &next_key); - set.spawn(fut); - debug_assert!(set.len() <= BATCH_SIZE); - } + while set.len() < BATCH_SIZE { + let Some((idx, key)) = keys.next() else { break }; + let fut = KVStore::read(kv_store, primary_namespace, secondary_namespace, &key); + set.spawn(async move { (idx, fut.await) }); + debug_assert!(set.len() <= BATCH_SIZE); } - while let Some(read_res) = set.join_next().await { + while let Some(join_res) = set.join_next().await { // Exit early if we get an IO error. - let reader = read_res - .map_err(|e| { - log_error!(logger, "Failed to read {}: {}", type_name, e); - set.abort_all(); - e - })? - .map_err(|e| { - log_error!(logger, "Failed to read {}: {}", type_name, e); - set.abort_all(); - e - })?; + let (idx, read_res) = join_res.map_err(|e| { + log_error!(logger, "Failed to read {}: {}", type_name, e); + set.abort_all(); + e + })?; + let reader = read_res.map_err(|e| { + log_error!(logger, "Failed to read {}: {}", type_name, e); + set.abort_all(); + e + })?; // Refill set for every finished future, if we still have something to do. - if let Some(next_key) = stored_keys.pop() { + if let Some((next_idx, next_key)) = keys.next() { let fut = KVStore::read(kv_store, primary_namespace, secondary_namespace, &next_key); - set.spawn(fut); + set.spawn(async move { (next_idx, fut.await) }); debug_assert!(set.len() <= BATCH_SIZE); } // Handle result. let object = T::read(&mut &*reader).map_err(|e| { log_error!(logger, "Failed to deserialize {}: {}", type_name, e); + set.abort_all(); std::io::Error::new( std::io::ErrorKind::InvalidData, format!("Failed to deserialize {}", type_name), ) })?; - res.push(object); + objects[idx] = Some(object); } debug_assert!(set.is_empty()); - debug_assert!(stored_keys.is_empty()); + debug_assert!(keys.next().is_none()); + debug_assert!(objects.iter().all(|object| object.is_some())); - Ok(res) + Ok(objects.into_iter().flatten().collect()) } /// Read `OutputSweeper` state from the store. @@ -901,3 +974,144 @@ mod tests { v1_store } } + +#[cfg(test)] +mod read_objects_tests { + use std::num::NonZeroUsize; + use std::sync::Arc; + + use lightning::impl_writeable_tlv_based; + use lightning::util::persist::KVStore; + use lightning::util::ser::Writeable; + use lightning::util::test_utils::TestLogger; + + use super::test_utils::{InMemoryStore, IN_MEMORY_PAGE_SIZE}; + use super::{read_all_objects, read_n_objects}; + use crate::hex_utils; + use crate::types::{DynStore, DynStoreWrapper}; + + const TEST_PRIMARY_NAMESPACE: &str = "read_objects_test_primary"; + const TEST_SECONDARY_NAMESPACE: &str = "read_objects_test_secondary"; + + #[derive(Clone, Copy, Debug, Eq, PartialEq)] + struct TestObject { + id: u32, + } + impl_writeable_tlv_based!(TestObject, { (0, id, required) }); + + /// Writes `num_objects` objects with ascending ids, so that the highest id is the most + /// recently created one. + async fn store_with_objects(num_objects: u32) -> (Arc, Vec) { + let kv_store: Arc = Arc::new(DynStoreWrapper(InMemoryStore::new())); + let mut objects = Vec::new(); + for id in 0..num_objects { + let object = TestObject { id }; + KVStore::write( + &*kv_store, + TEST_PRIMARY_NAMESPACE, + TEST_SECONDARY_NAMESPACE, + &hex_utils::to_string(&id.to_be_bytes()), + object.encode(), + ) + .await + .unwrap(); + objects.push(object); + } + (kv_store, objects) + } + + fn newest_first(objects: &[TestObject], num_objects: usize) -> Vec { + objects.iter().rev().take(num_objects).cloned().collect() + } + + async fn read_n(kv_store: &DynStore, num_objects: usize) -> Vec { + read_n_objects( + kv_store, + TEST_PRIMARY_NAMESPACE, + TEST_SECONDARY_NAMESPACE, + NonZeroUsize::new(num_objects).unwrap(), + Arc::new(TestLogger::new()), + ) + .await + .unwrap() + } + + #[tokio::test] + async fn reads_the_newest_objects_within_a_single_page() { + let (kv_store, objects) = store_with_objects(IN_MEMORY_PAGE_SIZE as u32).await; + assert_eq!(newest_first(&objects, 10), read_n(&*kv_store, 10).await); + } + + #[tokio::test] + async fn reads_the_newest_objects_across_several_pages() { + let num_objects = 3 * IN_MEMORY_PAGE_SIZE + 7; + let (kv_store, objects) = store_with_objects(num_objects as u32).await; + + // Spanning more than one page is where paging the keys, rather than listing them all, + // actually has to work. + let wanted = 2 * IN_MEMORY_PAGE_SIZE + 3; + assert_eq!(newest_first(&objects, wanted), read_n(&*kv_store, wanted).await); + } + + #[tokio::test] + async fn reading_more_than_is_stored_returns_everything() { + let (kv_store, objects) = store_with_objects(5).await; + assert_eq!(newest_first(&objects, 5), read_n(&*kv_store, 500).await); + } + + #[tokio::test] + async fn reads_all_objects_newest_first() { + let num_objects = 2 * IN_MEMORY_PAGE_SIZE + 11; + let (kv_store, objects) = store_with_objects(num_objects as u32).await; + + let read: Vec = read_all_objects( + &*kv_store, + TEST_PRIMARY_NAMESPACE, + TEST_SECONDARY_NAMESPACE, + Arc::new(TestLogger::new()), + ) + .await + .unwrap(); + assert_eq!(newest_first(&objects, num_objects), read); + } + + #[tokio::test] + async fn reading_an_empty_namespace_yields_nothing() { + let kv_store: Arc = Arc::new(DynStoreWrapper(InMemoryStore::new())); + assert!(read_n(&*kv_store, 10).await.is_empty()); + + let all: Vec = read_all_objects( + &*kv_store, + TEST_PRIMARY_NAMESPACE, + TEST_SECONDARY_NAMESPACE, + Arc::new(TestLogger::new()), + ) + .await + .unwrap(); + assert!(all.is_empty()); + } + + #[tokio::test] + async fn an_undecodable_object_is_an_error() { + let (kv_store, _objects) = store_with_objects(3).await; + KVStore::write( + &*kv_store, + TEST_PRIMARY_NAMESPACE, + TEST_SECONDARY_NAMESPACE, + &hex_utils::to_string(&99u32.to_be_bytes()), + vec![0xff; 2], + ) + .await + .unwrap(); + + let res = read_n_objects::( + &*kv_store, + TEST_PRIMARY_NAMESPACE, + TEST_SECONDARY_NAMESPACE, + NonZeroUsize::new(10).unwrap(), + Arc::new(TestLogger::new()), + ) + .await; + assert_eq!(std::io::ErrorKind::InvalidData, res.unwrap_err().kind()); + } +} diff --git a/src/lib.rs b/src/lib.rs index 14ee734a3d..0ce45bfdd0 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 and pagination are backed by the configured storage backend, so + /// a token stays valid across restarts of the node. + /// + /// Payments created or removed while paginating may or may not be observed. Because the + /// ordering is by creation, a payment that exists throughout is never skipped nor returned + /// twice, but a page may hold fewer payments than the backend's page size. Iterate until + /// `next_page_token` is `None` rather than until a short page. + /// + /// Note that migrating between storage backends does not preserve the relative creation order + /// of pre-existing payments, so their order may change once after such a migration. /// /// For example, you could retrieve all stored outbound payments as follows: /// ``` /// # use ldk_node::Builder; /// # use ldk_node::config::Config; - /// # use ldk_node::payment::PaymentDirection; + /// # use ldk_node::payment::{PaymentDetails, PaymentDirection}; /// # use ldk_node::bitcoin::Network; /// # use ldk_node::entropy::{generate_entropy_mnemonic, NodeEntropy}; /// # use rand::distr::Alphanumeric; @@ -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 4503dfa061..07248080c3 100644 --- a/src/payment/bolt11.rs +++ b/src/payment/bolt11.rs @@ -276,7 +276,7 @@ impl Bolt11Payment { let payment_hash = invoice.payment_hash(); let payment_id = PaymentId(invoice.payment_hash().0); - if let Some(payment) = self.payment_store.get(&payment_id) { + if let Some(payment) = self.runtime.block_on(self.payment_store.get(&payment_id))? { if payment.status == PaymentStatus::Pending || payment.status == PaymentStatus::Succeeded { @@ -506,7 +506,7 @@ impl Bolt11Payment { return Err(Error::InvalidPaymentPreimage); } - if let Some(details) = self.payment_store.get(&payment_id) { + if let Some(details) = self.runtime.block_on(self.payment_store.get(&payment_id))? { // For payments requested via `receive*_via_jit_channel_for_hash()` // `skimmed_fee_msat` held by LSP must be taken into account. let skimmed_fee_msat = match details.kind { diff --git a/src/payment/mod.rs b/src/payment/mod.rs index fd75322ceb..cc55e1f943 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 d2b92747a2..f30304b9dc 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; @@ -1086,3 +1123,140 @@ mod tests { assert_eq!(decoded, PaymentKind::read(&mut &*reencoded).unwrap()); } } + +#[cfg(test)] +mod bounded_cache_tests { + use std::num::NonZeroUsize; + use std::sync::Arc; + + use lightning::util::test_utils::TestLogger; + + use super::*; + use crate::config::PAYMENT_CACHE_CAPACITY; + use crate::data_store::{DataStore, DataStoreUpdateResult, KeepLeastRecentlyUsed}; + use crate::io::test_utils::InMemoryStore; + use crate::io::{ + PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE, PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE, + }; + use crate::types::{DynStore, DynStoreWrapper}; + + type BoundedPaymentStore = DataStore, KeepLeastRecentlyUsed>; + + fn new_bounded_payment_store(capacity: usize) -> BoundedPaymentStore { + let kv_store: Arc = Arc::new(DynStoreWrapper(InMemoryStore::new())); + DataStore::new( + Vec::new(), + KeepLeastRecentlyUsed::new(NonZeroUsize::new(capacity).unwrap()), + PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE.to_string(), + PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE.to_string(), + kv_store, + Arc::new(TestLogger::new()), + ) + } + + fn bolt11_payment(seed: u8) -> PaymentDetails { + PaymentDetails::new( + PaymentId([seed; 32]), + PaymentKind::Bolt11 { + hash: PaymentHash([seed; 32]), + preimage: Some(PaymentPreimage([seed.wrapping_add(1); 32])), + secret: Some(PaymentSecret([seed.wrapping_add(2); 32])), + counterparty_skimmed_fee_msat: Some(seed as u64 * 7), + }, + Some(seed as u64 * 1_000), + Some(seed as u64 * 3), + PaymentDirection::Outbound, + PaymentStatus::Succeeded, + ) + } + + #[tokio::test] + async fn evicted_payments_survive_a_round_trip_through_the_store() { + // A bounded store hands back objects it deserialized rather than ones it kept, so every + // field a payment carries has to survive being written out and read back. + let data_store = new_bounded_payment_store(2); + + let payments: Vec = (1..=10u8).map(bolt11_payment).collect(); + for payment in &payments { + data_store.insert(payment.clone()).await.unwrap(); + } + assert_eq!(2, data_store.cached_len()); + + for payment in &payments { + assert_eq!(Some(payment.clone()), data_store.get(&payment.id).await.unwrap()); + } + } + + #[tokio::test] + async fn updating_an_evicted_payment_preserves_the_fields_it_omits() { + // This is the failure mode a bounded cache invites: the wallet builds a partial + // `PaymentDetails` from a transaction and merges it in, and a payment that happens to have + // been evicted must not lose the fields only the merge target knows about. + let data_store = new_bounded_payment_store(1); + + let mut stored = bolt11_payment(1); + stored.fee_paid_msat = Some(4_242); + data_store.insert(stored.clone()).await.unwrap(); + + // Push it out of the cache. + data_store.insert(bolt11_payment(2)).await.unwrap(); + + let mut update = PaymentDetailsUpdate::new(stored.id); + update.status = Some(PaymentStatus::Failed); + assert_eq!(Ok(DataStoreUpdateResult::Updated), data_store.update(update).await); + + let updated = data_store.get(&stored.id).await.unwrap().unwrap(); + assert_eq!(PaymentStatus::Failed, updated.status); + assert_eq!(Some(4_242), updated.fee_paid_msat); + assert_eq!(stored.kind, updated.kind); + assert_eq!(stored.amount_msat, updated.amount_msat); + } + + #[tokio::test] + async fn listing_covers_payments_the_cache_cannot_hold() { + let data_store = new_bounded_payment_store(3); + + let payments: Vec = (1..=60u8).map(bolt11_payment).collect(); + for payment in &payments { + data_store.insert(payment.clone()).await.unwrap(); + } + assert_eq!(3, data_store.cached_len()); + + let mut listed = Vec::new(); + let mut page_token = None; + loop { + let page = data_store.list_page(page_token).await.unwrap(); + listed.extend(page.objects); + match page.next_page_token { + Some(token) => page_token = Some(token), + None => break, + } + } + + let mut expected = payments; + expected.reverse(); + assert_eq!(expected, listed); + // Listing the whole history must not have displaced the cache. + assert_eq!(3, data_store.cached_len()); + } + + #[tokio::test] + async fn the_cache_stays_within_its_capacity() { + let capacity = 16; + let data_store = new_bounded_payment_store(capacity); + + for seed in 1..=200u8 { + data_store.insert(bolt11_payment(seed)).await.unwrap(); + assert!(data_store.cached_len() <= capacity); + } + assert_eq!(capacity, data_store.cached_len()); + } + + #[test] + fn payment_cache_capacity_is_sane() { + // Small enough to bound memory at well under a megabyte, large enough to cover the recent + // payments a node actually works with. + assert!(PAYMENT_CACHE_CAPACITY.get() >= 100); + assert!(PAYMENT_CACHE_CAPACITY.get() <= 10_000); + } +} diff --git a/src/types.rs b/src/types.rs index 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 f8d9d521eb..e490f8d009 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -257,6 +257,7 @@ impl Wallet { let payment_id = self .find_payment_by_txid(txid) + .await? .unwrap_or_else(|| PaymentId(txid.to_byte_array())); if self @@ -288,8 +289,9 @@ impl Wallet { } }, WalletEvent::ChainTipChanged { new_tip, .. } => { - let pending_payments: Vec = - self.pending_payment_store.list_filter(|p| { + let pending_payments: Vec = self + .pending_payment_store + .list_filter(|p| { debug_assert!( p.details.status == PaymentStatus::Pending, "Non-pending payment {:?} found in pending store", @@ -297,7 +299,8 @@ impl Wallet { ); p.details.status == PaymentStatus::Pending && matches!(p.details.kind, PaymentKind::Onchain { .. }) - }); + }) + .await; let mut unconfirmed_outbound_txids: Vec = Vec::new(); @@ -354,6 +357,7 @@ impl Wallet { WalletEvent::TxUnconfirmed { txid, tx, .. } => { let payment_id = self .find_payment_by_txid(txid) + .await? .unwrap_or_else(|| PaymentId(txid.to_byte_array())); if self @@ -384,7 +388,7 @@ impl Wallet { self.pending_payment_store.insert_or_update(pending_payment).await?; }, WalletEvent::TxReplaced { txid, conflicts, .. } => { - let Some(payment_id) = self.find_payment_by_txid(txid) else { + let Some(payment_id) = self.find_payment_by_txid(txid).await? else { log_error!( self.logger, "Could not find payment for replaced transaction {}. Skipping.", @@ -401,13 +405,13 @@ impl Wallet { // The payment already exists in the store at this point: `bump_fee_rbf` updates // the payment store with the replacement txid before the next sync cycle, so we // can safely fetch it here. + let stored_payment = self.payment_store.get(&payment_id).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()); @@ -416,6 +420,7 @@ impl Wallet { WalletEvent::TxDropped { txid, tx } => { let payment_id = self .find_payment_by_txid(txid) + .await? .unwrap_or_else(|| PaymentId(txid.to_byte_array())); if self @@ -1472,10 +1477,10 @@ impl Wallet { PendingPaymentDetails::new(payment, conflicting_txids, Vec::new()) } - fn find_payment_by_txid(&self, target_txid: Txid) -> Option { + async fn find_payment_by_txid(&self, target_txid: Txid) -> Result, Error> { let direct_payment_id = PaymentId(target_txid.to_byte_array()); - if self.pending_payment_store.contains_key(&direct_payment_id) { - return Some(direct_payment_id); + if self.pending_payment_store.contains_key(&direct_payment_id).await? { + return Ok(Some(direct_payment_id)); } if let Some(replaced_details) = self @@ -1484,12 +1489,13 @@ impl Wallet { matches!(p.details.kind, PaymentKind::Onchain { txid, .. } if txid == target_txid) || p.conflicting_txids.contains(&target_txid) }) + .await .first() { - return Some(replaced_details.details.id); + return Ok(Some(replaced_details.details.id)); } - None + Ok(None) } /// If `payment_id` refers to a classified funding payment, refreshes its confirmation status @@ -1501,7 +1507,7 @@ impl Wallet { async fn apply_funding_status_update( &self, payment_id: PaymentId, event_txid: Txid, confirmation_status: ConfirmationStatus, ) -> Result { - let Some(mut payment) = self.payment_store.get(&payment_id) else { + let Some(mut payment) = self.payment_store.get(&payment_id).await? else { return Ok(false); }; let tx_type = match &payment.kind { @@ -1519,7 +1525,7 @@ impl Wallet { // one broadcast (an earlier, lower-fee candidate may win) and may carry no figures at all // (`None`) for a round we didn't contribute to. (`direction` is invariant across a splice's // candidates and cannot be changed through the store anyway.) - if let Some(pending) = self.pending_payment_store.get(&payment_id) { + if let Some(pending) = self.pending_payment_store.get(&payment_id).await? { if let Some(candidate) = pending.candidate(event_txid) { payment.amount_msat = candidate.amount_msat; payment.fee_paid_msat = candidate.fee_paid_msat; @@ -1544,7 +1550,7 @@ impl Wallet { pub(crate) async fn bump_fee_rbf( &self, payment_id: PaymentId, fee_rate: Option, cur_anchor_reserve_sats: u64, ) -> Result { - let payment = self.payment_store.get(&payment_id).ok_or_else(|| { + let payment = self.payment_store.get(&payment_id).await?.ok_or_else(|| { log_error!(self.logger, "Payment {} not found in payment store", payment_id); Error::InvalidPaymentId })?; diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 1fbbaad7e2..701267f3e2 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()).unwrap(); + let payment = $node.payment(&payment_id.unwrap()).unwrap().unwrap(); if !matches!(payment.kind, ldk_node::payment::PaymentKind::Onchain { .. }) { assert_eq!(payment.fee_paid_msat, None); } @@ -292,7 +294,7 @@ macro_rules! expect_payment_successful_event { if let Some(fee_msat) = $fee_paid_msat { assert_eq!(fee_paid_msat, fee_msat); } - let payment = $node.payment(&$payment_id.unwrap()).unwrap(); + let payment = $node.payment(&$payment_id.unwrap()).unwrap().unwrap(); assert_eq!(payment.fee_paid_msat, fee_paid_msat); assert_eq!(payment_id, $payment_id); $node.event_handled().unwrap(); @@ -410,8 +412,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) @@ -429,7 +483,7 @@ fn assert_any_node_has_onchain_tx_type bool + Copy>( let observed: Vec = nodes .iter() .flat_map(|(name, node)| { - node.list_payments().into_iter().filter_map(move |payment| match payment.kind { + node.list_all_payments().into_iter().filter_map(move |payment| match payment.kind { PaymentKind::Onchain { tx_type, .. } => Some(format!("{}:{:?}", name, tx_type)), _ => None, }) @@ -448,7 +502,7 @@ fn assert_all_nodes_have_onchain_tx_type bool + Copy> let observed: Vec = nodes .iter() .flat_map(|(name, node)| { - node.list_payments().into_iter().filter_map(move |payment| match payment.kind { + node.list_all_payments().into_iter().filter_map(move |payment| match payment.kind { PaymentKind::Onchain { tx_type, .. } => Some(format!("{}:{:?}", name, tx_type)), _ => None, }) @@ -1090,28 +1144,28 @@ pub(crate) async fn do_channel_full_cycle( // Check we saw the node funding transactions. assert_eq!( node_a - .list_payments_with_filter(|p| p.direction == PaymentDirection::Inbound + .list_payments_matching(|p| p.direction == PaymentDirection::Inbound && matches!(p.kind, PaymentKind::Onchain { .. })) .len(), 1 ); assert_eq!( node_a - .list_payments_with_filter(|p| p.direction == PaymentDirection::Outbound + .list_payments_matching(|p| p.direction == PaymentDirection::Outbound && matches!(p.kind, PaymentKind::Onchain { .. })) .len(), 0 ); assert_eq!( node_b - .list_payments_with_filter(|p| p.direction == PaymentDirection::Inbound + .list_payments_matching(|p| p.direction == PaymentDirection::Inbound && matches!(p.kind, PaymentKind::Onchain { .. })) .len(), 1 ); assert_eq!( node_b - .list_payments_with_filter(|p| p.direction == PaymentDirection::Outbound + .list_payments_matching(|p| p.direction == PaymentDirection::Outbound && matches!(p.kind, PaymentKind::Onchain { .. })) .len(), 0 @@ -1164,7 +1218,7 @@ pub(crate) async fn do_channel_full_cycle( // Check we now see the channel funding transaction as outbound. assert_eq!( node_a - .list_payments_with_filter(|p| p.direction == PaymentDirection::Outbound + .list_payments_matching(|p| p.direction == PaymentDirection::Outbound && matches!(p.kind, PaymentKind::Onchain { .. })) .len(), 1 @@ -1242,24 +1296,24 @@ pub(crate) async fn do_channel_full_cycle( let payment_id = node_a.bolt11_payment().send(&invoice, None).unwrap(); assert_eq!(node_a.bolt11_payment().send(&invoice, None), Err(NodeError::DuplicatePayment)); - assert!(!node_a.list_payments_with_filter(|p| p.id == payment_id).is_empty()); + assert!(!node_a.list_payments_matching(|p| p.id == payment_id).is_empty()); - let outbound_payments_a = node_a.list_payments_with_filter(|p| { + let outbound_payments_a = node_a.list_payments_matching(|p| { p.direction == PaymentDirection::Outbound && matches!(p.kind, PaymentKind::Bolt11 { .. }) }); assert_eq!(outbound_payments_a.len(), 1); - let inbound_payments_a = node_a.list_payments_with_filter(|p| { + let inbound_payments_a = node_a.list_payments_matching(|p| { p.direction == PaymentDirection::Inbound && matches!(p.kind, PaymentKind::Bolt11 { .. }) }); assert_eq!(inbound_payments_a.len(), 0); - let outbound_payments_b = node_b.list_payments_with_filter(|p| { + let outbound_payments_b = node_b.list_payments_matching(|p| { p.direction == PaymentDirection::Outbound && matches!(p.kind, PaymentKind::Bolt11 { .. }) }); assert_eq!(outbound_payments_b.len(), 0); - let inbound_payments_b = node_b.list_payments_with_filter(|p| { + let inbound_payments_b = node_b.list_payments_matching(|p| { p.direction == PaymentDirection::Inbound && matches!(p.kind, PaymentKind::Bolt11 { .. }) }); assert_eq!(inbound_payments_b.len(), 1); @@ -1276,23 +1330,41 @@ pub(crate) async fn do_channel_full_cycle( }, } expect_event!(node_b, PaymentReceived); - assert_eq!(node_a.payment(&payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_a.payment(&payment_id).unwrap().direction, PaymentDirection::Outbound); - assert_eq!(node_a.payment(&payment_id).unwrap().amount_msat, Some(invoice_amount_1_msat)); - assert!(matches!(node_a.payment(&payment_id).unwrap().kind, PaymentKind::Bolt11 { .. })); - assert_eq!(node_b.payment(&payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_b.payment(&payment_id).unwrap().direction, PaymentDirection::Inbound); - assert_eq!(node_b.payment(&payment_id).unwrap().amount_msat, Some(invoice_amount_1_msat)); - assert!(matches!(node_b.payment(&payment_id).unwrap().kind, PaymentKind::Bolt11 { .. })); + assert_eq!(node_a.payment(&payment_id).unwrap().unwrap().status, PaymentStatus::Succeeded); + assert_eq!(node_a.payment(&payment_id).unwrap().unwrap().direction, PaymentDirection::Outbound); + assert_eq!( + node_a.payment(&payment_id).unwrap().unwrap().amount_msat, + Some(invoice_amount_1_msat) + ); + assert!(matches!( + node_a.payment(&payment_id).unwrap().unwrap().kind, + PaymentKind::Bolt11 { .. } + )); + assert_eq!(node_b.payment(&payment_id).unwrap().unwrap().status, PaymentStatus::Succeeded); + assert_eq!(node_b.payment(&payment_id).unwrap().unwrap().direction, PaymentDirection::Inbound); + assert_eq!( + node_b.payment(&payment_id).unwrap().unwrap().amount_msat, + Some(invoice_amount_1_msat) + ); + assert!(matches!( + node_b.payment(&payment_id).unwrap().unwrap().kind, + PaymentKind::Bolt11 { .. } + )); // Assert we fail duplicate outbound payments and check the status hasn't changed. assert_eq!(Err(NodeError::DuplicatePayment), node_a.bolt11_payment().send(&invoice, None)); - assert_eq!(node_a.payment(&payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_a.payment(&payment_id).unwrap().direction, PaymentDirection::Outbound); - assert_eq!(node_a.payment(&payment_id).unwrap().amount_msat, Some(invoice_amount_1_msat)); - assert_eq!(node_b.payment(&payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_b.payment(&payment_id).unwrap().direction, PaymentDirection::Inbound); - assert_eq!(node_b.payment(&payment_id).unwrap().amount_msat, Some(invoice_amount_1_msat)); + assert_eq!(node_a.payment(&payment_id).unwrap().unwrap().status, PaymentStatus::Succeeded); + assert_eq!(node_a.payment(&payment_id).unwrap().unwrap().direction, PaymentDirection::Outbound); + assert_eq!( + node_a.payment(&payment_id).unwrap().unwrap().amount_msat, + Some(invoice_amount_1_msat) + ); + assert_eq!(node_b.payment(&payment_id).unwrap().unwrap().status, PaymentStatus::Succeeded); + assert_eq!(node_b.payment(&payment_id).unwrap().unwrap().direction, PaymentDirection::Inbound); + assert_eq!( + node_b.payment(&payment_id).unwrap().unwrap().amount_msat, + Some(invoice_amount_1_msat) + ); // Test under-/overpayment let invoice_amount_2_msat = 2500_000; @@ -1329,14 +1401,26 @@ pub(crate) async fn do_channel_full_cycle( }, }; assert_eq!(received_amount, overpaid_amount_msat); - assert_eq!(node_a.payment(&payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_a.payment(&payment_id).unwrap().direction, PaymentDirection::Outbound); - assert_eq!(node_a.payment(&payment_id).unwrap().amount_msat, Some(overpaid_amount_msat)); - assert!(matches!(node_a.payment(&payment_id).unwrap().kind, PaymentKind::Bolt11 { .. })); - assert_eq!(node_b.payment(&payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_b.payment(&payment_id).unwrap().direction, PaymentDirection::Inbound); - assert_eq!(node_b.payment(&payment_id).unwrap().amount_msat, Some(overpaid_amount_msat)); - assert!(matches!(node_b.payment(&payment_id).unwrap().kind, PaymentKind::Bolt11 { .. })); + assert_eq!(node_a.payment(&payment_id).unwrap().unwrap().status, PaymentStatus::Succeeded); + assert_eq!(node_a.payment(&payment_id).unwrap().unwrap().direction, PaymentDirection::Outbound); + assert_eq!( + node_a.payment(&payment_id).unwrap().unwrap().amount_msat, + Some(overpaid_amount_msat) + ); + assert!(matches!( + node_a.payment(&payment_id).unwrap().unwrap().kind, + PaymentKind::Bolt11 { .. } + )); + assert_eq!(node_b.payment(&payment_id).unwrap().unwrap().status, PaymentStatus::Succeeded); + assert_eq!(node_b.payment(&payment_id).unwrap().unwrap().direction, PaymentDirection::Inbound); + assert_eq!( + node_b.payment(&payment_id).unwrap().unwrap().amount_msat, + Some(overpaid_amount_msat) + ); + assert!(matches!( + node_b.payment(&payment_id).unwrap().unwrap().kind, + PaymentKind::Bolt11 { .. } + )); // Test "zero-amount" invoice payment println!("\nB receive_variable_amount_payment"); @@ -1367,14 +1451,26 @@ pub(crate) async fn do_channel_full_cycle( }, }; assert_eq!(received_amount, determined_amount_msat); - assert_eq!(node_a.payment(&payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_a.payment(&payment_id).unwrap().direction, PaymentDirection::Outbound); - assert_eq!(node_a.payment(&payment_id).unwrap().amount_msat, Some(determined_amount_msat)); - assert!(matches!(node_a.payment(&payment_id).unwrap().kind, PaymentKind::Bolt11 { .. })); - assert_eq!(node_b.payment(&payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_b.payment(&payment_id).unwrap().direction, PaymentDirection::Inbound); - assert_eq!(node_b.payment(&payment_id).unwrap().amount_msat, Some(determined_amount_msat)); - assert!(matches!(node_b.payment(&payment_id).unwrap().kind, PaymentKind::Bolt11 { .. })); + assert_eq!(node_a.payment(&payment_id).unwrap().unwrap().status, PaymentStatus::Succeeded); + assert_eq!(node_a.payment(&payment_id).unwrap().unwrap().direction, PaymentDirection::Outbound); + assert_eq!( + node_a.payment(&payment_id).unwrap().unwrap().amount_msat, + Some(determined_amount_msat) + ); + assert!(matches!( + node_a.payment(&payment_id).unwrap().unwrap().kind, + PaymentKind::Bolt11 { .. } + )); + assert_eq!(node_b.payment(&payment_id).unwrap().unwrap().status, PaymentStatus::Succeeded); + assert_eq!(node_b.payment(&payment_id).unwrap().unwrap().direction, PaymentDirection::Inbound); + assert_eq!( + node_b.payment(&payment_id).unwrap().unwrap().amount_msat, + Some(determined_amount_msat) + ); + assert!(matches!( + node_b.payment(&payment_id).unwrap().unwrap().kind, + PaymentKind::Bolt11 { .. } + )); // Test claiming manually registered payments. let invoice_amount_3_msat = 5_532_000; @@ -1403,20 +1499,38 @@ pub(crate) async fn do_channel_full_cycle( .unwrap(); expect_payment_received_event!(node_b, claimable_amount_msat); expect_payment_successful_event!(node_a, Some(manual_payment_id), None); - assert_eq!(node_a.payment(&manual_payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_a.payment(&manual_payment_id).unwrap().direction, PaymentDirection::Outbound); assert_eq!( - node_a.payment(&manual_payment_id).unwrap().amount_msat, + node_a.payment(&manual_payment_id).unwrap().unwrap().status, + PaymentStatus::Succeeded + ); + assert_eq!( + node_a.payment(&manual_payment_id).unwrap().unwrap().direction, + PaymentDirection::Outbound + ); + assert_eq!( + node_a.payment(&manual_payment_id).unwrap().unwrap().amount_msat, Some(invoice_amount_3_msat) ); - assert!(matches!(node_a.payment(&manual_payment_id).unwrap().kind, PaymentKind::Bolt11 { .. })); - assert_eq!(node_b.payment(&manual_payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_b.payment(&manual_payment_id).unwrap().direction, PaymentDirection::Inbound); + assert!(matches!( + node_a.payment(&manual_payment_id).unwrap().unwrap().kind, + PaymentKind::Bolt11 { .. } + )); + assert_eq!( + node_b.payment(&manual_payment_id).unwrap().unwrap().status, + PaymentStatus::Succeeded + ); + assert_eq!( + node_b.payment(&manual_payment_id).unwrap().unwrap().direction, + PaymentDirection::Inbound + ); assert_eq!( - node_b.payment(&manual_payment_id).unwrap().amount_msat, + node_b.payment(&manual_payment_id).unwrap().unwrap().amount_msat, Some(invoice_amount_3_msat) ); - assert!(matches!(node_b.payment(&manual_payment_id).unwrap().kind, PaymentKind::Bolt11 { .. })); + assert!(matches!( + node_b.payment(&manual_payment_id).unwrap().unwrap().kind, + PaymentKind::Bolt11 { .. } + )); // Test failing manually registered payments. let invoice_amount_4_msat = 5_532_000; @@ -1442,30 +1556,36 @@ pub(crate) async fn do_channel_full_cycle( ); node_b.bolt11_payment().fail_for_hash(manual_fail_payment_hash).unwrap(); expect_event!(node_a, PaymentFailed); - assert_eq!(node_a.payment(&manual_fail_payment_id).unwrap().status, PaymentStatus::Failed); assert_eq!( - node_a.payment(&manual_fail_payment_id).unwrap().direction, + node_a.payment(&manual_fail_payment_id).unwrap().unwrap().status, + PaymentStatus::Failed + ); + assert_eq!( + node_a.payment(&manual_fail_payment_id).unwrap().unwrap().direction, PaymentDirection::Outbound ); assert_eq!( - node_a.payment(&manual_fail_payment_id).unwrap().amount_msat, + node_a.payment(&manual_fail_payment_id).unwrap().unwrap().amount_msat, Some(invoice_amount_4_msat) ); assert!(matches!( - node_a.payment(&manual_fail_payment_id).unwrap().kind, + node_a.payment(&manual_fail_payment_id).unwrap().unwrap().kind, PaymentKind::Bolt11 { .. } )); - assert_eq!(node_b.payment(&manual_fail_payment_id).unwrap().status, PaymentStatus::Failed); assert_eq!( - node_b.payment(&manual_fail_payment_id).unwrap().direction, + node_b.payment(&manual_fail_payment_id).unwrap().unwrap().status, + PaymentStatus::Failed + ); + assert_eq!( + node_b.payment(&manual_fail_payment_id).unwrap().unwrap().direction, PaymentDirection::Inbound ); assert_eq!( - node_b.payment(&manual_fail_payment_id).unwrap().amount_msat, + node_b.payment(&manual_fail_payment_id).unwrap().unwrap().amount_msat, Some(invoice_amount_4_msat) ); assert!(matches!( - node_b.payment(&manual_fail_payment_id).unwrap().kind, + node_b.payment(&manual_fail_payment_id).unwrap().unwrap().kind, PaymentKind::Bolt11 { .. } )); @@ -1490,39 +1610,53 @@ pub(crate) async fn do_channel_full_cycle( }, }; assert_eq!(received_keysend_amount, keysend_amount_msat); - assert_eq!(node_a.payment(&keysend_payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_a.payment(&keysend_payment_id).unwrap().direction, PaymentDirection::Outbound); - assert_eq!(node_a.payment(&keysend_payment_id).unwrap().amount_msat, Some(keysend_amount_msat)); + assert_eq!( + node_a.payment(&keysend_payment_id).unwrap().unwrap().status, + PaymentStatus::Succeeded + ); + assert_eq!( + node_a.payment(&keysend_payment_id).unwrap().unwrap().direction, + PaymentDirection::Outbound + ); + assert_eq!( + node_a.payment(&keysend_payment_id).unwrap().unwrap().amount_msat, + Some(keysend_amount_msat) + ); assert!(matches!( - node_a.payment(&keysend_payment_id).unwrap().kind, + node_a.payment(&keysend_payment_id).unwrap().unwrap().kind, PaymentKind::Spontaneous { .. } )); assert_eq!(received_custom_records, &custom_tlvs); - assert_eq!(node_b.payment(&keysend_payment_id).unwrap().status, PaymentStatus::Succeeded); - assert_eq!(node_b.payment(&keysend_payment_id).unwrap().direction, PaymentDirection::Inbound); - assert_eq!(node_b.payment(&keysend_payment_id).unwrap().amount_msat, Some(keysend_amount_msat)); + assert_eq!( + node_b.payment(&keysend_payment_id).unwrap().unwrap().status, + PaymentStatus::Succeeded + ); + assert_eq!( + node_b.payment(&keysend_payment_id).unwrap().unwrap().direction, + PaymentDirection::Inbound + ); + assert_eq!( + node_b.payment(&keysend_payment_id).unwrap().unwrap().amount_msat, + Some(keysend_amount_msat) + ); assert!(matches!( - node_b.payment(&keysend_payment_id).unwrap().kind, + node_b.payment(&keysend_payment_id).unwrap().unwrap().kind, PaymentKind::Spontaneous { .. } )); assert_eq!( - node_a.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Bolt11 { .. })).len(), + node_a.list_payments_matching(|p| matches!(p.kind, PaymentKind::Bolt11 { .. })).len(), 5 ); assert_eq!( - node_b.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Bolt11 { .. })).len(), + node_b.list_payments_matching(|p| matches!(p.kind, PaymentKind::Bolt11 { .. })).len(), 6 ); assert_eq!( - node_a - .list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Spontaneous { .. })) - .len(), + node_a.list_payments_matching(|p| matches!(p.kind, PaymentKind::Spontaneous { .. })).len(), 1 ); assert_eq!( - node_b - .list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Spontaneous { .. })) - .len(), + node_b.list_payments_matching(|p| matches!(p.kind, PaymentKind::Spontaneous { .. })).len(), 1 ); @@ -1547,7 +1681,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 @@ -1568,7 +1702,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 @@ -1745,13 +1879,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 7e5767dca6..4fcb29e476 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 df477588f1..4e6564be8a 100644 --- a/tests/integration_tests_rust.rs +++ b/tests/integration_tests_rust.rs @@ -28,7 +28,7 @@ use common::{ open_channel_with_all, premine_and_distribute_funds, premine_blocks, prepare_rbf, random_chain_source, random_config, setup_bitcoind_and_electrsd, setup_builder, setup_node, setup_two_nodes, splice_in_with_all, wait_for_block, wait_for_tx, InMemoryStore, - TestChainSource, TestConfig, TestStoreType, TestSyncStore, + NodePaymentExt, TestChainSource, TestConfig, TestStoreType, TestSyncStore, }; use electrsd::corepc_node::{self, Node as BitcoinD}; use electrsd::ElectrsD; @@ -58,7 +58,7 @@ use serde_json::json; async fn wait_for_classified_funding_payment(node: &Node, funding_txid: Txid) { let poll = async { loop { - let classified = node.list_payments().into_iter().any(|p| { + let classified = node.list_all_payments().into_iter().any(|p| { matches!( p.kind, PaymentKind::Onchain { txid, tx_type: Some(_), .. } if txid == funding_txid @@ -590,16 +590,15 @@ async fn split_underpaid_bolt11_payment() { expect_payment_successful_event!(node_b, Some(payment_id_b), None); // The receiver records the full invoice amount; each payer records only its own half. - let receiver_payments = - node_c.list_payments_with_filter(|p| p.id == receiver_payment_id.unwrap()); + let receiver_payments = node_c.list_payments_matching(|p| p.id == receiver_payment_id.unwrap()); assert_eq!(receiver_payments.len(), 1); assert_eq!(receiver_payments.first().unwrap().amount_msat, Some(amount_msat)); - let node_a_payments = node_a.list_payments_with_filter(|p| p.id == payment_id_a); + let node_a_payments = node_a.list_payments_matching(|p| p.id == payment_id_a); assert_eq!(node_a_payments.len(), 1); assert_eq!(node_a_payments.first().unwrap().amount_msat, Some(half_amount_msat)); - let node_b_payments = node_b.list_payments_with_filter(|p| p.id == payment_id_b); + let node_b_payments = node_b.list_payments_matching(|p| p.id == payment_id_b); assert_eq!(node_b_payments.len(), 1); assert_eq!(node_b_payments.first().unwrap().amount_msat, Some(half_amount_msat)); } @@ -723,8 +722,8 @@ async fn onchain_send_receive() { assert_eq!(node_a.list_balances().spendable_onchain_balance_sats, premine_amount_sat); assert_eq!(node_b.list_balances().spendable_onchain_balance_sats, premine_amount_sat); - let node_a_payments = node_a.list_payments(); - let node_b_payments = node_b.list_payments(); + let node_a_payments = node_a.list_all_payments(); + let node_b_payments = node_b.list_all_payments(); for payments in [&node_a_payments, &node_b_payments] { assert_eq!(payments.len(), 1) } @@ -752,10 +751,10 @@ async fn onchain_send_receive() { expect_channel_ready_event!(node_b, node_a.node_id()); let node_a_payments = - node_a.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Onchain { .. })); + node_a.list_payments_matching(|p| matches!(p.kind, PaymentKind::Onchain { .. })); assert_eq!(node_a_payments.len(), 1); let node_b_payments = - node_b.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Onchain { .. })); + node_b.list_payments_matching(|p| matches!(p.kind, PaymentKind::Onchain { .. })); assert_eq!(node_b_payments.len(), 2); let onchain_fee_buffer_sat = 1000; @@ -791,7 +790,7 @@ async fn onchain_send_receive() { node_b.sync_wallets().unwrap(); let payment_id = PaymentId(txid.to_byte_array()); - let payment_a = node_a.payment(&payment_id).unwrap(); + let payment_a = node_a.payment(&payment_id).unwrap().unwrap(); assert_eq!(payment_a.status, PaymentStatus::Pending); match payment_a.kind { PaymentKind::Onchain { status, tx_type, .. } => { @@ -801,7 +800,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, .. } => { @@ -827,13 +826,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); @@ -843,7 +842,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); @@ -870,10 +869,10 @@ async fn onchain_send_receive() { assert!(node_b.list_balances().spendable_onchain_balance_sats < expected_node_b_balance_upper); let node_a_payments = - node_a.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Onchain { .. })); + node_a.list_payments_matching(|p| matches!(p.kind, PaymentKind::Onchain { .. })); assert_eq!(node_a_payments.len(), 3); let node_b_payments = - node_b.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Onchain { .. })); + node_b.list_payments_matching(|p| matches!(p.kind, PaymentKind::Onchain { .. })); assert_eq!(node_b_payments.len(), 4); let addr_b = node_b.onchain_payment().new_address().unwrap(); @@ -894,10 +893,10 @@ async fn onchain_send_receive() { assert!(node_b.list_balances().spendable_onchain_balance_sats < expected_node_b_balance_upper); let node_a_payments = - node_a.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Onchain { .. })); + node_a.list_payments_matching(|p| matches!(p.kind, PaymentKind::Onchain { .. })); assert_eq!(node_a_payments.len(), 4); let node_b_payments = - node_b.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Onchain { .. })); + node_b.list_payments_matching(|p| matches!(p.kind, PaymentKind::Onchain { .. })); assert_eq!(node_b_payments.len(), 5); } @@ -932,7 +931,7 @@ async fn reorged_onchain_payment_returns_to_unconfirmed() { let payment_id = PaymentId(txid.to_byte_array()); for node in [&node_a, &node_b] { - let payment = node.payment(&payment_id).unwrap(); + let payment = node.payment(&payment_id).unwrap().unwrap(); assert_eq!(payment.status, PaymentStatus::Pending); match payment.kind { PaymentKind::Onchain { status, .. } => { @@ -958,7 +957,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, .. } => { @@ -1777,7 +1776,7 @@ async fn splice_channel() { // them to the channel balance since there may not be a change output. let expected_splice_in_lightning_balance_sat = 4_000_002; - let payments = node_b.list_payments(); + let payments = node_b.list_all_payments(); let payment = payments.into_iter().find(|p| p.id == PaymentId(txo.txid.to_byte_array())).unwrap(); assert_eq!(payment.fee_paid_msat, Some(expected_splice_in_fee_sat * 1_000)); @@ -1829,7 +1828,7 @@ async fn splice_channel() { let expected_splice_out_fee_sat = 183; - let payments = node_a.list_payments(); + let payments = node_a.list_all_payments(); let payment = payments.into_iter().find(|p| p.id == PaymentId(txo.txid.to_byte_array())).unwrap(); assert_eq!(payment.fee_paid_msat, Some(expected_splice_out_fee_sat * 1_000)); @@ -1943,7 +1942,8 @@ async fn run_rbf_splice_channel_test(confirm_original: bool) { // the RBF replaces it, so it can be force-confirmed (instead of the RBF) further below. let original_candidate: Option<(Option, String)> = if confirm_original { let payment_id = PaymentId(original_txo.txid.to_byte_array()); - let fee = node_b.payment(&payment_id).expect("splice payment exists").fee_paid_msat; + let fee = + node_b.payment(&payment_id).unwrap().expect("splice payment exists").fee_paid_msat; let raw_tx: String = bitcoind .client .call("getrawtransaction", &[json!(original_txo.txid.to_string())]) @@ -1981,7 +1981,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, @@ -1996,7 +1996,7 @@ async fn run_rbf_splice_channel_test(confirm_original: bool) { } assert_eq!(payment.status, PaymentStatus::Pending); // Only one Onchain Pending payment for this splice attempt (not one per candidate). - let splice_payments = node_b.list_payments_with_filter(|p| { + let splice_payments = node_b.list_payments_matching(|p| { p.direction == PaymentDirection::Outbound && matches!(p.kind, PaymentKind::Onchain { .. }) && p.status == PaymentStatus::Pending @@ -2056,7 +2056,7 @@ async fn run_rbf_splice_channel_test(confirm_original: bool) { // winning RBF candidate, and `fee_paid_msat` carries this node's `FundingContribution` fee. { let payment_id = PaymentId(original_txo.txid.to_byte_array()); - let payment = node_b.payment(&payment_id).expect("splice payment graduated"); + let payment = node_b.payment(&payment_id).unwrap().expect("splice payment graduated"); assert_eq!(payment.status, PaymentStatus::Succeeded); match payment.kind { PaymentKind::Onchain { txid, status: ConfirmationStatus::Confirmed { .. }, .. } => { @@ -2117,7 +2117,7 @@ async fn funding_payment_graduates_without_channel_ready() { // confirmations, asserted before draining any LDK event — so graduation is not driven by the // Lightning `ChannelReady` signal. let payment_id = PaymentId(funding_txo.txid.to_byte_array()); - let payment = node_a.payment(&payment_id).expect("funding payment exists"); + let payment = node_a.payment(&payment_id).unwrap().expect("funding payment exists"); assert_eq!(payment.status, PaymentStatus::Succeeded); match payment.kind { PaymentKind::Onchain { @@ -2180,7 +2180,7 @@ async fn splice_payment_reorged_to_unconfirmed() { node_b.sync_wallets().unwrap(); let payment_id = PaymentId(splice_txo.txid.to_byte_array()); - let payment = node_b.payment(&payment_id).expect("splice payment exists"); + let payment = node_b.payment(&payment_id).unwrap().expect("splice payment exists"); assert_eq!(payment.status, PaymentStatus::Pending); assert!(matches!( payment.kind, @@ -2203,7 +2203,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, @@ -2321,7 +2321,7 @@ async fn simple_bolt12_send_receive() { ref e => panic!("{} got unexpected event!: {:?}", "node_a", e), } let node_a_payments = - node_a.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Bolt12Offer { .. })); + node_a.list_payments_matching(|p| matches!(p.kind, PaymentKind::Bolt12Offer { .. })); assert_eq!(node_a_payments.len(), 1); match node_a_payments.first().unwrap().kind { PaymentKind::Bolt12Offer { @@ -2348,7 +2348,7 @@ async fn simple_bolt12_send_receive() { expect_payment_received_event!(node_b, expected_amount_msat); let node_b_payments = - node_b.list_payments_with_filter(|p| matches!(p.kind, PaymentKind::Bolt12Offer { .. })); + node_b.list_payments_matching(|p| matches!(p.kind, PaymentKind::Bolt12Offer { .. })); assert_eq!(node_b_payments.len(), 1); match node_b_payments.first().unwrap().kind { PaymentKind::Bolt12Offer { hash, preimage, secret, offer_id, .. } => { @@ -2386,7 +2386,7 @@ async fn simple_bolt12_send_receive() { .unwrap(); expect_payment_successful_event!(node_a, Some(payment_id), None); - let node_a_payments = node_a.list_payments_with_filter(|p| { + let node_a_payments = node_a.list_payments_matching(|p| { matches!(p.kind, PaymentKind::Bolt12Offer { .. }) && p.id == payment_id }); assert_eq!(node_a_payments.len(), 1); @@ -2416,7 +2416,7 @@ async fn simple_bolt12_send_receive() { expect_payment_received_event!(node_b, expected_amount_msat); let node_b_payment_id = PaymentId(payment_hash.0); - let node_b_payments = node_b.list_payments_with_filter(|p| { + let node_b_payments = node_b.list_payments_matching(|p| { matches!(p.kind, PaymentKind::Bolt12Offer { .. }) && p.id == node_b_payment_id }); assert_eq!(node_b_payments.len(), 1); @@ -2451,7 +2451,7 @@ async fn simple_bolt12_send_receive() { expect_payment_received_event!(node_a, overpaid_amount); let node_b_payment_id = node_b - .list_payments_with_filter(|p| { + .list_payments_matching(|p| { matches!(p.kind, PaymentKind::Bolt12Refund { .. }) && p.amount_msat == Some(overpaid_amount) }) @@ -2460,7 +2460,7 @@ async fn simple_bolt12_send_receive() { .id; expect_payment_successful_event!(node_b, Some(node_b_payment_id), None); - let node_b_payments = node_b.list_payments_with_filter(|p| { + let node_b_payments = node_b.list_payments_matching(|p| { matches!(p.kind, PaymentKind::Bolt12Refund { .. }) && p.id == node_b_payment_id }); assert_eq!(node_b_payments.len(), 1); @@ -2486,7 +2486,7 @@ async fn simple_bolt12_send_receive() { assert_eq!(node_b_payments.first().unwrap().amount_msat, Some(overpaid_amount)); let node_a_payment_id = PaymentId(invoice.payment_hash().0); - let node_a_payments = node_a.list_payments_with_filter(|p| { + let node_a_payments = node_a.list_payments_matching(|p| { matches!(p.kind, PaymentKind::Bolt12Refund { .. }) && p.id == node_a_payment_id }); assert_eq!(node_a_payments.len(), 1); @@ -3016,7 +3016,7 @@ async fn do_lsps2_client_service_integration(client_trusts_lsp: bool) { expect_payment_successful_event!(payer_node, Some(payment_id), None); let client_payment_id = expect_payment_received_event!(client_node, expected_received_amount_msat).unwrap(); - let client_payment = client_node.payment(&client_payment_id).unwrap(); + let client_payment = client_node.payment(&client_payment_id).unwrap().unwrap(); match client_payment.kind { PaymentKind::Bolt11 { counterparty_skimmed_fee_msat, .. } => { assert_eq!(counterparty_skimmed_fee_msat, Some(service_fee_msat)); @@ -3091,7 +3091,7 @@ async fn do_lsps2_client_service_integration(client_trusts_lsp: bool) { expect_payment_successful_event!(payer_node, Some(payment_id), None); let client_payment_id = expect_payment_received_event!(client_node, expected_received_amount_msat).unwrap(); - let client_payment = client_node.payment(&client_payment_id).unwrap(); + let client_payment = client_node.payment(&client_payment_id).unwrap().unwrap(); match client_payment.kind { PaymentKind::Bolt11 { counterparty_skimmed_fee_msat, .. } => { assert_eq!(counterparty_skimmed_fee_msat, Some(service_fee_msat)); @@ -3138,7 +3138,7 @@ async fn do_lsps2_client_service_integration(client_trusts_lsp: bool) { client_node.bolt11_payment().fail_for_hash(manual_payment_hash).unwrap(); expect_event!(payer_node, PaymentFailed); - assert_eq!(client_node.payment(&payment_id).unwrap().status, PaymentStatus::Failed); + assert_eq!(client_node.payment(&payment_id).unwrap().unwrap().status, PaymentStatus::Failed); } #[tokio::test(flavor = "multi_thread", worker_threads = 1)] @@ -3197,7 +3197,7 @@ async fn spontaneous_send_with_custom_preimage() { // check payment status and verify stored preimage expect_payment_successful_event!(node_a, Some(payment_id), None); let details: PaymentDetails = - node_a.list_payments_with_filter(|p| p.id == payment_id).first().unwrap().clone(); + node_a.list_payments_matching(|p| p.id == payment_id).first().unwrap().clone(); assert_eq!(details.status, PaymentStatus::Succeeded); if let PaymentKind::Spontaneous { preimage: Some(pi), .. } = details.kind { assert_eq!(pi.0, custom_bytes); @@ -3207,7 +3207,7 @@ async fn spontaneous_send_with_custom_preimage() { // Verify receiver side (node_b) expect_payment_received_event!(node_b, amount_msat); - let receiver_payments: Vec = node_b.list_payments_with_filter(|p| { + let receiver_payments: Vec = node_b.list_payments_matching(|p| { p.direction == PaymentDirection::Inbound && matches!(p.kind, PaymentKind::Spontaneous { .. }) }); @@ -3604,12 +3604,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 { .. }) }); @@ -3626,7 +3629,7 @@ async fn payment_persistence_after_restart() { let restarted_node_a = setup_node(&chain_source, config_a); // Assert all 200 payments are still in the store - let outbound_payments_after = restarted_node_a.list_payments_with_filter(|p| { + let outbound_payments_after = restarted_node_a.list_payments_matching(|p| { p.direction == PaymentDirection::Outbound && matches!(p.kind, PaymentKind::Bolt11 { .. }) }); assert_eq!( @@ -3869,7 +3872,7 @@ async fn onchain_fee_bump_rbf() { node_b.sync_wallets().unwrap(); let payment_id = PaymentId(txid.to_byte_array()); - let original_payment = node_b.payment(&payment_id).unwrap(); + let original_payment = node_b.payment(&payment_id).unwrap().unwrap(); let original_fee = original_payment.fee_paid_msat.unwrap(); // Non-existent payment id @@ -3898,7 +3901,7 @@ async fn onchain_fee_bump_rbf() { node_b.sync_wallets().unwrap(); // Verify fee increased and txid updated for node_b - let new_payment = node_b.payment(&payment_id).unwrap(); + let new_payment = node_b.payment(&payment_id).unwrap().unwrap(); assert!( new_payment.fee_paid_msat > Some(original_fee), "Fee should increase after RBF bump. Original: {}, New: {}", @@ -3922,7 +3925,7 @@ async fn onchain_fee_bump_rbf() { node_b.sync_wallets().unwrap(); // Verify second bump payment exists and txid updated for node_b - let second_payment = node_b.payment(&payment_id).unwrap(); + let second_payment = node_b.payment(&payment_id).unwrap().unwrap(); assert!( second_payment.fee_paid_msat > new_payment.fee_paid_msat, "Second bump should have higher fee than first bump" @@ -3948,7 +3951,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, .. } => { @@ -3958,7 +3961,7 @@ async fn onchain_fee_bump_rbf() { } // Verify node A received the funds correctly - let node_a_received_payment = node_a.list_payments_with_filter(|p| { + let node_a_received_payment = node_a.list_payments_matching(|p| { p.id == payment_id && matches!(p.kind, PaymentKind::Onchain { .. }) }); diff --git a/tests/reorg_test.rs b/tests/reorg_test.rs index 132d9de96b..23c19c160d 100644 --- a/tests/reorg_test.rs +++ b/tests/reorg_test.rs @@ -10,7 +10,7 @@ use proptest::proptest; use crate::common::{ expect_event, exponential_backoff_poll, generate_blocks_and_wait, invalidate_blocks, open_channel, premine_and_distribute_funds, random_chain_source, random_config, - setup_bitcoind_and_electrsd, setup_node, wait_for_outpoint_spend, wait_for_tx, + setup_bitcoind_and_electrsd, setup_node, wait_for_outpoint_spend, wait_for_tx, NodePaymentExt, }; async fn wait_for_pending_sweep_balance( @@ -119,7 +119,7 @@ proptest! { for (i, node) in nodes.iter().enumerate() { assert_eq!( node - .list_payments_with_filter(|p| p.direction == PaymentDirection::Outbound + .list_payments_matching(|p| p.direction == PaymentDirection::Outbound && matches!(p.kind, PaymentKind::Onchain { .. })) .len(), 1