Stop keeping all DataStore entries in-memory, add pagination

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, and token lifetime follows that backend's guarantees.
+  This replaces the previous unpaginated `Node::list_payments`, and
+  `Node::list_payments_with_filter` has been removed; filter the returned pages instead.
+- `Node::payment` now returns a `Result`, as retrieving a payment may fail.
 - The Bitcoin Core RPC and REST chain-source builder methods now accept an optional
   `wallet_rescan_from_height` argument. Passing a height lets fresh wallets rescan from a known
   birthday block instead of checkpointing at the current tip, which is useful when restoring a

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)
 

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<PaymentDetails> list_payments();
+	[Throws=NodeError]
+	PaymentDetailsPage list_payments(PageToken? page_token);
 	sequence<PeerDetails> list_peers();
 	sequence<ChannelDetails> 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;

src/builder.rs

@@ -50,18 +50,20 @@ 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;
 use crate::gossip::GossipSource;
 use crate::io::sqlite_store::SqliteStore;
 use crate::io::utils::{
 	open_or_migrate_fs_store, read_all_objects, read_event_queue,
-	read_external_pathfinding_scores_from_cache, read_network_graph, read_node_metrics,
-	read_output_sweeper, read_peer_info, read_scorer,
+	read_external_pathfinding_scores_from_cache, read_n_objects, read_network_graph,
+	read_node_metrics, read_output_sweeper, read_peer_info, read_scorer,
 };
 use crate::io::vss_store::VssStoreBuilder;
 use crate::io::{
@@ -1458,10 +1460,11 @@ fn build_with_store_internal(
 	let (payment_store_res, node_metris_res, pending_payment_store_res) =
 		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)),
@@ -1489,7 +1492,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),
@@ -1744,8 +1751,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),

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;