Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/io/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ pub(crate) mod utils;
#[cfg(feature = "storage-vss")]
pub mod vss_store;

pub use crate::types::DynStoreRef;

/// The event queue will be persisted under this key.
pub(crate) const EVENT_QUEUE_PERSISTENCE_PRIMARY_NAMESPACE: &str = "";
pub(crate) const EVENT_QUEUE_PERSISTENCE_SECONDARY_NAMESPACE: &str = "";
Expand Down
17 changes: 15 additions & 2 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,8 @@ use probing::{run_prober, Prober};
use runtime::Runtime;
pub use tokio;
use types::{
Broadcaster, BumpTransactionEventHandler, ChainMonitor, ChannelManager, DynStore, Graph,
KeysManager, OnionMessenger, PaymentStore, PeerManager, Router, Scorer, Sweeper, Wallet,
Broadcaster, BumpTransactionEventHandler, ChainMonitor, ChannelManager, DynStore, DynStoreRef,
Graph, KeysManager, OnionMessenger, PaymentStore, PeerManager, Router, Scorer, Sweeper, Wallet,
};
pub use types::{
ChannelCounterparty, ChannelDetails, CustomTlvRecord, PeerDetails, ReserveType, UserChannelId,
Expand Down Expand Up @@ -982,6 +982,19 @@ impl Node {
self.config.as_ref().clone()
}

/// Returns a handle to the [`KVStore`] the [`Node`] persists its state to.
///
/// This allows applications to persist their own data in the same store as the node, e.g., to
/// keep it in the same database and back it up together.
///
/// **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.
Comment on lines +990 to +991

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

///
/// [`KVStore`]: lightning::util::persist::KVStore
pub fn kv_store(&self) -> DynStoreRef {
DynStoreRef(Arc::clone(&self.kv_store))
}
Comment on lines +994 to +996

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/// Returns the next event in the event queue, if currently available.
///
/// Will return `Some(..)` if an event is available and `None` otherwise.
Expand Down
6 changes: 5 additions & 1 deletion src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,12 +118,16 @@ impl<'a> PaginatedKVStore for dyn DynStoreTrait + 'a {

pub(crate) type DynStore = dyn DynStoreTrait;

/// A handle to the store a [`Node`] persists its state to, as returned by [`Node::kv_store`].
///
/// [`Node`]: crate::Node
/// [`Node::kv_store`]: crate::Node::kv_store
// Newtype wrapper that implements `KVStore` for `Arc<DynStore>`. This is needed because `KVStore`
// methods return `impl Future`, which is not object-safe. `DynStoreTrait` works around this by
// returning `Pin<Box<dyn Future>>` instead, and this wrapper bridges the two by delegating
// `KVStore` methods to the corresponding `DynStoreTrait::*_async` methods.
#[derive(Clone)]
pub(crate) struct DynStoreRef(pub(crate) Arc<DynStore>);
pub struct DynStoreRef(pub(crate) Arc<DynStore>);

impl KVStore for DynStoreRef {
fn read(
Expand Down
28 changes: 28 additions & 0 deletions tests/integration_tests_rust.rs
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,34 @@ async fn address_pool_is_reloaded_on_restart() {
expect_channel_ready_event!(node_b, node_a.node_id());
}

#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
async fn application_data_persists_in_node_kv_store() {
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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


let (primary_namespace, secondary_namespace) = ("app", "data");
let node = setup_node(&chain_source, config.clone());
let kv_store = node.kv_store();
KVStore::write(&kv_store, primary_namespace, secondary_namespace, "key", vec![42; 32])
.await
.unwrap();
node.stop().unwrap();
drop(node);
drop(kv_store);

// Data written through the handle is persisted alongside the node's state and survives a
// restart.
let node = setup_node(&chain_source, config);
let kv_store = node.kv_store();
let data = KVStore::read(&kv_store, primary_namespace, secondary_namespace, "key").await;
assert_eq!(data.unwrap(), vec![42; 32]);
let keys = KVStore::list(&kv_store, primary_namespace, secondary_namespace).await.unwrap();
assert_eq!(keys, vec!["key".to_string()]);
node.stop().unwrap();
}

#[cfg(feature = "chain-bitcoind")]
#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
async fn channel_full_cycle() {
Expand Down
Loading