Repository navigation
Expose the node's KV store to applications - #1174
benthecarman wants to merge 1 commit into
Conversation
Applications built on LDK Node may want to persist their own data alongside the node's state, e.g., so it lives in the same database and is backed up together. Currently the node's store isn't reachable once the node is built, and opening a second handle to it isn't an option with the Postgres backend, as the store takes an exclusive lease on its table. Add `Node::kv_store`, returning a handle to the store the node persists its state to, and make `DynStoreRef` public so applications can hold on to it. Applications must only use namespaces of their own. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
I've assigned @tnull as a reviewer! |
Camillarhi
left a comment
Camillarhi
left a comment
There was a problem hiding this comment.
Thank for looking into this. Left a few comments
| pub fn kv_store(&self) -> DynStoreRef { | ||
| DynStoreRef(Arc::clone(&self.kv_store)) | ||
| } |
There was a problem hiding this comment.
Since this hands out a clone of the Arc, the store now lives as long as the last handle does. For Postgres, that means the lease is also held until then, so if an app keeps this handle around after dropping the node, building a new node on the same table fails with KVStoreAlreadyInUse.
Could we document that all handles need to be dropped before building a new node on the same store? Probably also worth mentioning that lease-loss panics can now surface in app tasks when they write through this.
| /// **Note:** Applications must only read and write namespaces of their own. Modifying or | ||
| /// removing any data written by the node itself may lead to loss of funds. |
There was a problem hiding this comment.
How would an app know which namespaces are its own? The node and LDK use "", monitors, payments, bdk_wallet, etc., but those constants are all pub(crate). There's also nothing stopping a future release from adding a namespace an app already uses.
Given the loss-of-funds risk, could we enforce this instead of only documenting it? E.g. return a small wrapper that rejects the empty primary namespace and the ones we use internally, or require some prefix like app_. At minimum it'd be good to list the reserved ones here. What do you think?
| let (bitcoind, electrsd) = setup_bitcoind_and_electrsd(); | ||
| let chain_source = random_chain_source(&bitcoind, &electrsd); | ||
| let mut config = random_config(); | ||
| config.store_type = TestStoreType::Sqlite; |
There was a problem hiding this comment.
Postgres is a motivating case here, but the test only covers SQLite, where the lease behavior doesn't apply. Would be nice to have a test that checks what happens when a handle is still held across a rebuild.
3 participants
Applications built on LDK Node may want to persist their own data alongside the node's state, so it lives in the same database and is backed up together. Right now nothing can reach the node's store once the node is built. Opening a second handle doesn't work with the Postgres backend either, because the store takes an exclusive lease on its table.
This adds
Node::kv_store, which returns a handle to the store the node persists its state to. It also makesDynStoreRefpublic, re-exported asldk_node::io::DynStoreRef, so applications can keep the handle. The docs say applications must only use namespaces of their own.The motivating use case is LDK Server's
SubscribeEvents(lightningdevkit/ldk-server#245). We want to write each event to a durable, indexed log in the node's store before callingevent_handled(), so subscribers can resume from an event index and get every event at least once.Open to renaming
DynStoreRefnow that it's part of the public API.