Support tiered data storage#692
Conversation
|
👋 I see @TheBlueMatt was un-assigned. |
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
4 times, most recently
from
29f47f3 to
264aa7f
Compare
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 10th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 11th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 12th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 13th Reminder Hey @tnull! This PR has been waiting for your review. |
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
from
264aa7f to
493dd9a
Compare
|
🔔 14th Reminder Hey @tnull! This PR has been waiting for your review. |
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
2 times, most recently
from
a30cbfb to
1e7bdbc
Compare
|
🔔 15th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 16th Reminder Hey @tnull! This PR has been waiting for your review. |
There was a problem hiding this comment.
Thanks for looking into this and excuse the delay here!
I did a first pass, generally this looks already pretty good, but it is a huge PR and in some areas could be simplified. For instance, we should drop the generic Retry logic as the concrete implementations would already implement that if they need it. Likewise, we shouldn't fallback to the backup for now as it's really only meant as disaster recovery (KVStore caching is out-of-scope for this PR now, even though we might want to explore that soon, too). There is also no need to replicate the write-ordering locks in TierStore, but we'll need them in ForeignKVStoreAdapter (which might be mergeable with DynStore?).
Generally, if you find opportunities to reduce the size of the changeset here it would be appreciated. It would also be cool if you could try to break up the PR into more feature commits, but feel free to leave as is if not.
|
|
||
| /// Configuration for exponential backoff retry behavior. | ||
| #[derive(Debug, Copy, Clone)] | ||
| pub struct RetryConfig { |
There was a problem hiding this comment.
Retrying is pretty specific to the particular KVStore implementation. I don't think we should expose retrying params on top of what we already do for implementations internally.
There was a problem hiding this comment.
Sure thing. This has been dropped.
I considered it because users can choose their own KVStore implementation without considering retrying. I thought to add some level of control but the added complexity and size of the changeset might not be worth it.
| } | ||
|
|
||
| pub struct TierStoreInner { | ||
| /// For remote data. |
| [Throws=BuildError] | ||
| Node build_with_vss_store_and_header_provider(NodeEntropy node_entropy, string vss_url, string store_id, VssHeaderProvider header_provider); | ||
| [Throws=BuildError] | ||
| Node build_with_tier_store(NodeEntropy node_entropy, DynStore primary_store); |
There was a problem hiding this comment.
Can we now also expose Builder::build_with_store?
There was a problem hiding this comment.
Yes we can.
This is exposed here 451a392
| Self { inner: Arc::new(adapter) } | ||
| } | ||
|
|
||
| pub fn from_ldk_store(store: Arc<dyn LdkSyncAndAsyncKVStore + Send + Sync>) -> Arc<Self> { |
There was a problem hiding this comment.
If we rather make this an impl From<Arc<dyn LdkSyncAndAsyncKVStore + Send + Sync>> for Arc .., we can drop the wrap_storemacro and just use.into()` instead.
| } | ||
|
|
||
| let store = wrap_store!(Arc::new(tier_store)); | ||
| self.build_with_store(node_entropy, store) |
There was a problem hiding this comment.
I think we need to use build_with_store_internal here to make sure we're using the same Runtime etc.
| Arc::clone(&outer_lock.entry(locking_key).or_default()) | ||
| } | ||
|
|
||
| async fn execute_locked_write< |
There was a problem hiding this comment.
I'm confused, why are we replicating all the write-ordering logic here? Given the implementations are required to fullfill LDK's requirements already, can't we just call the inner write and be done with it?
There was a problem hiding this comment.
Again, the reasoning here is that users might not implement the KVStore trait for their own stores as required and thus, the onus of correctness will fall to us. If we can't get it from their inner stores, the wrapper TierStore should provide some guarantees. I do agree that if the inner store already satisfies LDK's requirements, the wrapper shouldn't need to duplicate that logic. The write-ordering has been removed.
| ) -> Pin<Box<dyn Future<Output = Result<(), lightning::io::Error>> + Send>> { | ||
| let inner = self.inner.clone(); | ||
|
|
||
| let primary_namespace = primary_namespace.to_string(); |
There was a problem hiding this comment.
I think here (and in remove) we need to add the write-ordering logic to ensure we follow LDK's KVStore::write requirements.
| /// A type alias for [`SyncAndAsyncKVStore`] with `Sync`/`Send` markers; | ||
| pub type DynStore = dyn SyncAndAsyncKVStore + Sync + Send; | ||
| #[cfg(feature = "uniffi")] | ||
| pub(crate) use crate::DynStore; |
There was a problem hiding this comment.
This is odd. Why do we need this?
There was a problem hiding this comment.
I had initially used the same name and the feature gating became necessary to differentiate the types at call sites when uniffi was enabled.
| } | ||
| } | ||
|
|
||
| pub struct DelayedStore { |
There was a problem hiding this comment.
Not quite sure what coverage we gain with DelayedStore? IMO, we might be better off dropping all this boilerplate.
There was a problem hiding this comment.
This has been removed here cb66b59.
I couldn't think of any way better to test backup queue overflow not impacting primary writes. I agree that the boilerplate seems too much for just that single case.
| ) -> Result<Vec<String>, IOError>; | ||
| } | ||
|
|
||
| pub struct ForeignKVStoreAdapter { |
There was a problem hiding this comment.
Hmm, any reason this needs to be separate from DynStore? Couldn't we merge them?
There was a problem hiding this comment.
At the time I believe I introduced the extra wrapper due to the limitation of foreign trait implementation on foreign types, i.e. I couldn't implement KVStore for Arc<dyn LdkSyncAndAsyncKVStore> but with the new changes, this is no longer relevant and has been removed.
| } | ||
| } | ||
|
|
||
| #[async_trait] |
There was a problem hiding this comment.
Probably not worth taking a dependency just to desugar impl Future<Output = Result<(), IOError>> + Send + Sync + 'a.
There was a problem hiding this comment.
Probably not worth taking a dependency just to desugar
impl Future<Output = Result<(), IOError>> + Send + Sync + 'a.
Well, it's 'the official'/supported way to do async traits with Uniffi: https://mozilla.github.io/uniffi-rs/latest/futures.html#exporting-async-trait-methods
There was a problem hiding this comment.
Does uniffi require that in some way? Its just a comically-overkill way to sugar impl Future which is kinda nuts...
There was a problem hiding this comment.
Oh, worse, it looks like async_trait desugars to the Pin<Box<dyn ...>> version...which is dumb but at least for uniffi it shouldn't matter cause we'd have to do that anyway.
There was a problem hiding this comment.
Unfortunately this will need a substantial rebase now that #696 landed, sorry for that!
Besides some tedious rebase work, we now have DynStoreWrapper which can be used on the ffi and the non-ffi side. I do wonder if we can actually just merge that wrapper with the TierStore, as well as the ForeignKVStoreAdapter/DynStore. Seems all these wrapper structs may not be needed and we could just get away with one trait and one wrapper struct, mostly?
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
3 times, most recently
from
c1563e3 to
a2458e4
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tnull! This PR has been waiting for your review. |
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
from
8dbb312 to
0ae61b7
Compare
|
🔔 4th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @tnull! This PR has been waiting for your review. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Excuse the delay here! Mostly looks good, just a few comments.
| let $builder = Builder::from_config($config.clone()); | ||
| #[cfg(not(feature = "uniffi"))] | ||
| #[allow(unused_mut)] | ||
| let mut $builder = Builder::from_config($config.clone()); |
There was a problem hiding this comment.
I'm confused: why do we suddenly need mut under uniffi, especially given we don't add any bindings in this PR now?
There was a problem hiding this comment.
I had previously introduced a test helper setup_node_with_builder that took a closure that required the builder to be mutable. This affected all tests (including uniffi gated ones) that utilized the helper as wrapped in setup_node. In hindsight, it was an unnecessary refactor. I've addressed the removal in 1ef68f2
| // http://www.apache.org/licenses/LICENSE-2.0> or the MIT license <LICENSE-MIT or | ||
| // http://opensource.org/licenses/MIT>, at your option. You may not use this file except in | ||
| // accordance with one or both of these licenses. | ||
| #![allow(dead_code)] // TODO: Temporal warning silencer. Will be removed in later commit. |
There was a problem hiding this comment.
The fixup for this seems to be in the wrong commit.
| /// can be rebuilt if lost. | ||
| /// | ||
| /// If not set, non-critical data will be stored in the primary store. | ||
| pub fn set_ephemeral_store(&mut self, ephemeral_store: Arc<DynStore>) -> &mut Self { |
There was a problem hiding this comment.
I think the ephemeral store should always be a simple SQLite. Note that DynStore might currently not even be constructed outside of the crate right now.
There was a problem hiding this comment.
Noted. I thought we'd leave the store type decision to the users and made DynStore public for this reason, but as stated here, the decision has been reverted for SQLite.
| &self, primary_namespace: &str, secondary_namespace: &str, key: &str, buf: Vec<u8>, | ||
| ) -> io::Result<()> { | ||
| if let Some(backup_store) = self.backup_store.as_ref() { | ||
| let primary_res = KVStoreSync::write( |
There was a problem hiding this comment.
Codex:
- [P1] Serialize primary and backup writes per key — /home/tnull/workspace/ldk-node/src/io/tier_store.rs:319-327
When two operations for the same key run at the same time, this method writes to the primary and backup without a shared per-key lock. One call can update the primary, a second can update both stores, and then the first can update the backup, so both calls return Ok(()) while the backup
contains an older value than the primary. Please serialize each primary+backup write/remove pair per key in both the sync and async paths.
Hmm, maybe we need some kind of key-level locks afterall? WDYT?
There was a problem hiding this comment.
The intuition is correct here. A possible DAG without per-key lock is:
[w1_prim] -> [w2_prim] -> [w2_bck] -> [w1_bck] with the backup ending up with stale data from an out-of-order write.
I've re-added the per-key locks and also added clean_locks (following the same pattern as VssStore) to prune entries when no in-flight operations remain for a key.
| } | ||
| } | ||
|
|
||
| fn ephemeral_store( |
There was a problem hiding this comment.
This method is super confusing, as it makes the callsite look as if we always delegate to the ephemeral store if it's set. Can we rather inline this, ie., have the callsites use is_ephemeral_cached_key to decide and then delegate to self.ephemeral_store when needed?
There was a problem hiding this comment.
This has been updated.
| /// If not set, durable data will be stored only in the primary store. | ||
| /// | ||
| /// [`SQLITE_BACKUP_DB_FILE_NAME`]: crate::io::sqlite_store::SQLITE_BACKUP_DB_FILE_NAME | ||
| pub fn set_backup_storage_dir_path(&self, backup_storage_dir_path: String) { |
There was a problem hiding this comment.
I guess adjusting ArcedNodeBuilder should be part of the uniffi follow-up?
| } | ||
|
|
||
| pub(crate) trait DynStoreTrait: Send + Sync { | ||
| pub trait DynStoreTrait: Send + Sync { |
There was a problem hiding this comment.
This was made public so users could create their own ephemeral DynStore but since you've suggested that it be a SQLite store, I'll revert the change here.
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
3 times, most recently
from
deb81d0 to
e53deb6
Compare
|
Hi @tnull |
This commit: Adds `TierStore`, a tiered `KVStore`/`KVStoreSync` implementation that routes node persistence across three storage roles: - a primary store for durable, authoritative data - an optional backup store for a second durable copy of primary-backed data - an optional ephemeral store for rebuildable cached data such as the network graph and scorer TierStore routes ephemeral cache data to the ephemeral store when configured, while durable data remains primary+backup. Reads and lists do not consult the backup store during normal operation. For primary+backup writes and removals, this implementation treats the backup store as part of the persistence success path rather than as a best-effort background mirror. Earlier designs used asynchronous backup queueing to avoid blocking the primary path, but that weakens the durability contract by allowing primary success to be reported before backup persistence has completed. TierStore now issues primary and backup operations together and only returns success once both complete. This gives callers a clearer persistence guarantee when a backup store is configured: acknowledged primary+backup mutations have been attempted against both durable stores. The tradeoff is that dual-store operations are not atomic across stores, so an error may still be returned after one store has already been updated. TierStore also implements `KVStoreSync` in terms of dedicated synchronous helpers that call the wrapped stores' sync interfaces directly. This preserves the inner stores' synchronous semantics instead of routing sync operations through a previously held async runtime. Additionally, adds unit coverage for the current contract, including: - basic read/write/remove/list persistence - routing of ephemeral data away from the primary store - backup participation in the foreground success path for writes and removals
Add native builder support for tiered storage by introducing `TierStoreConfig` and builder methods for configuring ephemeral storage and a local SQLite backup mirror. During node construction, wrap the configured primary store in `TierStore` and attach secondary tiers for cache-like ephemeral data and mirrored durable backup writes. The builder constructs the backup store internally using a dedicated SQLite database file and rejects configurations where the backup path conflicts with the primary storage path. Add test coverage for full-cycle backup mirroring, same-path rejection, and UniFFI-backed builder configuration. Update `setup_builder!` so FFI-backed builder tests can use mutable configuration helpers.
- Remove set_backup_storage_dir_path and set_ephemeral_store from ArcedNodeBuilder — these tiered-storage configuration methods belong to the FFI bindings PR (lightningdevkit#871) and should not be introduced here. - Gate builder_configures_sqlite_backup_store and sqlite_backup_rejects_primary_storage_path tests behind not exposed through the arced wrapper. - Revert the setup_node_with_builder helper added to test utils: the backup-related test now configures its builder inline, which also removes the &mut Builder requirement that forced setup_builder! to emit mut unconditionally. It's now correctly immutable when uniffi is enabled.
Replace the ephemeral_store() method with direct is_ephemeral_cached_key() checks followed by self.ephemeral_store.as_ref() at each callsite, making the routing decision explicit.
Here we serialize per-key writes in TierStore to prevent out-of-order backup updates. Without serialization, concurrent writes to the same key can interleave across the primary and backup stores (e.g. [w1.primary] → [w2.primary] → [w2.backup] → [w1.backup]), leaving the backup with stale data. By adding a per-key TokioMutex that serializes write and remove operations for a given (namespace, key) tuple, we ensure both the primary and backup stores reflect the same final value. The lock map is cleaned up after each operation when no other in-flight operations hold a reference, following the same pattern used in VssStore.
Replace Builder::set_ephemeral_store(Arc<DynStore>) with set_ephemeral_storage_dir_path(String), constructing the SQLite store internally to mirror the backup store pattern. Remove BackupStorePathConflict since all three stores use distinct DB file names and cannot collide even in the same directory.
remove blank lines
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
from
e53deb6 to
4e221ed
Compare
4 participants
What this PR does
In this PR we introduce
TierStore, a three-tiered (KVStore+KVStoreSync) implementation that manages data across three distinct storage layers based on criticality.Background
As we have moved towards supporting remote storage with
VssStore, we need to recognize that not all data has the same storage requirements. Currently, all data goes to a single store which creates some problems:This PR proposes tiered storage that provides granular control over where different data types are stored. The tiers include:
Additionally, we also permit the configuration of
Nodewith tiered storage allowing callers to:Nodewith a primary store.These configuration options also extend to our foreign interface, allowing bindings target to build the
Nodewith their own (KVStore+KVStoreSync) implementations. A sample Python implementation is provided and tested.Concerns
VssStorehas built-in retry logic. Wrapping it inTierStorecreates nested retries.KVStoreto the FFIRelated Issues