Replace KVStorePersister with KVStore#2472
Conversation
tnull
force-pushed
the
2023-08-add-kvstore
branch
3 times, most recently
from
1697806 to
b4b6599
Compare
tnull
force-pushed
the
2023-08-add-kvstore
branch
3 times, most recently
from
fe9ad36 to
933c834
Compare
Codecov ReportPatch coverage:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2472 +/- ##
==========================================
+ Coverage 90.59% 91.72% +1.12%
==========================================
Files 110 112 +2
Lines 57509 66839 +9330
Branches 57509 66839 +9330
==========================================
+ Hits 52101 61307 +9206
- Misses 5408 5532 +124
☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here. |
|
Ran a few bench iterations with n = 1000 sample size each to check there is no regression in write performance. Unsurprisingly (as For example some results (Linux on Intel(R) Xeon(R) CPU E3-1226 v3 @ 3.30GHz): |
tnull
force-pushed
the
2023-08-add-kvstore
branch
9 times, most recently
from
5864ce0 to
00937c5
Compare
|
(Not exactly sure why the BP test is stuck on windows but no other platform, will need to setup a windows box to debug this further) |
tnull
force-pushed
the
2023-08-add-kvstore
branch
from
00937c5 to
4b3302f
Compare
|
Rebased on main. |
tnull
force-pushed
the
2023-08-add-kvstore
branch
4 times, most recently
from
a16ad2e to
157a613
Compare
Windows issues are fixed, CI failure is #2438. |
tnull
force-pushed
the
2023-08-add-kvstore
branch
2 times, most recently
from
1d36446 to
2d0aaee
Compare
|
Alright, I now squashed the previous round of fixups and made the following changes:
|
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Yay, LGTM 🎉. A few minor comments left on the filesystem storage, specifically.
tnull
force-pushed
the
2023-08-add-kvstore
branch
2 times, most recently
from
7e3396c to
801cff3
Compare
|
I think basically LGTM, feel free to squash and let's land this! |
tnull
force-pushed
the
2023-08-add-kvstore
branch
from
801cff3 to
60b1d42
Compare
Squashed fixups without further changes. |
| /// | ||
| /// Note that while setting the `lazy` flag reduces the I/O burden of multiple subsequent | ||
| /// `remove` calls, it also influences the atomicity guarantees as lazy `remove`s could | ||
| /// potentially get lost on crash after the method returns. Therefore, this flag should only be |
There was a problem hiding this comment.
I am not sure if we want to take responsibility for replaying removes.
Therefore, this flag should only be set for remove operations that are not necessary for functional correctness of application and will not have any side-effects if remove failed or is eventually executed.
There was a problem hiding this comment.
I don't see how the flag could be interpreted any other way? If its async and we crash, the thing that's storing stuff has to handle that.
There was a problem hiding this comment.
I am not sure if we want to take responsibility for replaying removes.
But that's not what the comment is saying? It says that lazy should only be used if we're fine risking the remove getting lost, we may see removed keys in list, and we're okay re-removing them once more if that's the case, i.e., our logic won't get tripped by the described behavior.
There was a problem hiding this comment.
we're fine risking the remove getting lost
I see that it is mentioned before, but this wasn't very clear in the "therefore" statement.
Also wanted to make sure that functional correctness of application doesn't depend on "eventually".
There was a problem hiding this comment.
Uh, I struggle to come up with a case where a stronger guarantee would make something fail. In particular as 'eventually' means 'at any point in time from now', it includes 'right now, before the method returns'?
But more concretely I thought I had that covered by stating that this is optional for the backend implementation ("If the lazy flag is set to true, the backend implementation might choose to lazily remove the given key at some point in time after the method returns"). Let me know if something in particular is missing.
There was a problem hiding this comment.
I think it is fine, I was more concerned about deletes getting lost altogether by backend implementation. (for lazy delete and not even executing eventually)
There was a problem hiding this comment.
Yeah, they might get lost under certain circumstances if we crash before they are actually executed. I don't think we can get around that because if we require the backend to remember and persist the eventual deletes atomically we might as well just do them...
There was a problem hiding this comment.
If its async and we crash, the thing that's storing stuff has to handle that.
Yes, ideally. Moreover, if it is async and backend forgets about it or lost them, it shouldn't matter to us(ldk) at all.
As long as we are on same page 👍
|
feel free to squash |
Squashed fixups and included the following changes (as I decided to move the key length to a > git diff-tree -U2 09b9fa8c 9e89a53b
diff --git a/lightning-persister/src/utils.rs b/lightning-persister/src/utils.rs
index bf042ff9..ae83aa47 100644
--- a/lightning-persister/src/utils.rs
+++ b/lightning-persister/src/utils.rs
@@ -1,6 +1,5 @@
-use lightning::util::persist::KVSTORE_NAMESPACE_KEY_ALPHABET;
+use lightning::util::persist::{KVSTORE_NAMESPACE_KEY_ALPHABET, KVSTORE_MAX_KEY_LEN};
use lightning::util::string::PrintableString;
-const KVSTORE_MAX_KEY_LEN: usize = 120;
pub(crate) fn is_valid_kvstore_str(key: &str) -> bool {
diff --git a/lightning/src/util/persist.rs b/lightning/src/util/persist.rs
index d1f70e24..4f00d3de 100644
--- a/lightning/src/util/persist.rs
+++ b/lightning/src/util/persist.rs
@@ -32,4 +32,7 @@ use crate::util::ser::{ReadableArgs, Writeable};
pub const KVSTORE_NAMESPACE_KEY_ALPHABET: &str = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-";
+/// The maximum number of characters keys may have.
+pub const KVSTORE_MAX_KEY_LEN: usize = 120;
+
/// The namespace under which the [`ChannelManager`] will be persisted.
pub const CHANNEL_MANAGER_PERSISTENCE_NAMESPACE: &str = "";
@@ -66,6 +69,6 @@ pub const SCORER_PERSISTENCE_KEY: &str = "scorer";
///
/// Keys and namespaces are required to be valid ASCII strings in the range of
-/// [`KVSTORE_NAMESPACE_KEY_ALPHABET`] and no longer than 120 characters. Empty namespaces and
-/// sub-namespaces (`""`) are assumed to be a valid, however, if `namespace` is empty,
+/// [`KVSTORE_NAMESPACE_KEY_ALPHABET`] and no longer than [`KVSTORE_MAX_KEY_LEN`]. Empty namespaces
+/// and sub-namespaces (`""`) are assumed to be a valid, however, if `namespace` is empty,
/// `sub_namespace` is required to be empty, too. This means that concerns should always be
/// separated by namespace first, before sub-namespaces are used. While the number of namespaces |
|
So, I grew paranoid about hitting the max path length limitations on windows and added a corresponding test case. Low and behold there of course was an issue, so I now included a fix (canonicalizating paths on Windows): g> git diff-tree -U2 9e89a53b af2bd741
diff --git a/lightning-persister/src/fs_store.rs b/lightning-persister/src/fs_store.rs
index 7e5c2568..77c03a64 100644
--- a/lightning-persister/src/fs_store.rs
+++ b/lightning-persister/src/fs_store.rs
@@ -67,4 +67,26 @@ impl FilesystemStore {
}
}
+
+ fn get_dest_dir_path(&self, namespace: &str, sub_namespace: &str) -> std::io::Result<PathBuf> {
+ let mut dest_dir_path = {
+ #[cfg(target_os = "windows")]
+ {
+ let data_dir = self.data_dir.clone();
+ fs::create_dir_all(data_dir.clone())?;
+ fs::canonicalize(data_dir)?
+ }
+ #[cfg(not(target_os = "windows"))]
+ {
+ self.data_dir.clone()
+ }
+ };
+
+ dest_dir_path.push(namespace);
+ if !sub_namespace.is_empty() {
+ dest_dir_path.push(sub_namespace);
+ }
+
+ Ok(dest_dir_path)
+ }
}
@@ -73,9 +95,5 @@ impl KVStore for FilesystemStore {
check_namespace_key_validity(namespace, sub_namespace, Some(key), "read")?;
- let mut dest_file_path = self.data_dir.clone();
- dest_file_path.push(namespace);
- if !sub_namespace.is_empty() {
- dest_file_path.push(sub_namespace);
- }
+ let mut dest_file_path = self.get_dest_dir_path(namespace, sub_namespace)?;
dest_file_path.push(key);
@@ -100,9 +118,5 @@ impl KVStore for FilesystemStore {
check_namespace_key_validity(namespace, sub_namespace, Some(key), "write")?;
- let mut dest_file_path = self.data_dir.clone();
- dest_file_path.push(namespace);
- if !sub_namespace.is_empty() {
- dest_file_path.push(sub_namespace);
- }
+ let mut dest_file_path = self.get_dest_dir_path(namespace, sub_namespace)?;
dest_file_path.push(key);
@@ -191,9 +205,5 @@ impl KVStore for FilesystemStore {
check_namespace_key_validity(namespace, sub_namespace, Some(key), "remove")?;
- let mut dest_file_path = self.data_dir.clone();
- dest_file_path.push(namespace);
- if !sub_namespace.is_empty() {
- dest_file_path.push(sub_namespace);
- }
+ let mut dest_file_path = self.get_dest_dir_path(namespace, sub_namespace)?;
dest_file_path.push(key);
@@ -284,10 +294,5 @@ impl KVStore for FilesystemStore {
check_namespace_key_validity(namespace, sub_namespace, None, "list")?;
- let mut prefixed_dest = self.data_dir.clone();
- prefixed_dest.push(namespace);
- if !sub_namespace.is_empty() {
- prefixed_dest.push(sub_namespace);
- }
-
+ let prefixed_dest = self.get_dest_dir_path(namespace, sub_namespace)?;
let mut keys = Vec::new();
diff --git a/lightning-persister/src/test_utils.rs b/lightning-persister/src/test_utils.rs
index 718b8ef1..91557500 100644
--- a/lightning-persister/src/test_utils.rs
+++ b/lightning-persister/src/test_utils.rs
@@ -1,3 +1,3 @@
-use lightning::util::persist::{KVStore, read_channel_monitors};
+use lightning::util::persist::{KVStore, KVSTORE_NAMESPACE_KEY_MAX_LEN, read_channel_monitors};
use lightning::ln::functional_test_utils::{connect_block, create_announced_chan_between_nodes,
create_chanmon_cfgs, create_dummy_block, create_network, create_node_cfgs, create_node_chanmgrs,
@@ -39,4 +39,20 @@ pub(crate) fn do_read_write_remove_list_persist<K: KVStore + RefUnwindSafe>(kv_s
let listed_keys = kv_store.list(namespace, sub_namespace).unwrap();
assert_eq!(listed_keys.len(), 0);
+
+ // Ensure we have no issue operating with namespace/sub_namespace/key being KVSTORE_NAMESPACE_KEY_MAX_LEN
+ let max_chars: String = std::iter::repeat('A').take(KVSTORE_NAMESPACE_KEY_MAX_LEN).collect();
+ kv_store.write(&max_chars, &max_chars, &max_chars, &data).unwrap();
+
+ let listed_keys = kv_store.list(&max_chars, &max_chars).unwrap();
+ assert_eq!(listed_keys.len(), 1);
+ assert_eq!(listed_keys[0], max_chars);
+
+ let read_data = kv_store.read(&max_chars, &max_chars, &max_chars).unwrap();
+ assert_eq!(data, &*read_data);
+
+ kv_store.remove(&max_chars, &max_chars, &max_chars, false).unwrap();
+
+ let listed_keys = kv_store.list(&max_chars, &max_chars).unwrap();
+ assert_eq!(listed_keys.len(), 0);
}
diff --git a/lightning-persister/src/utils.rs b/lightning-persister/src/utils.rs
index ae83aa47..54ec230d 100644
--- a/lightning-persister/src/utils.rs
+++ b/lightning-persister/src/utils.rs
@@ -1,8 +1,8 @@
-use lightning::util::persist::{KVSTORE_NAMESPACE_KEY_ALPHABET, KVSTORE_MAX_KEY_LEN};
+use lightning::util::persist::{KVSTORE_NAMESPACE_KEY_ALPHABET, KVSTORE_NAMESPACE_KEY_MAX_LEN};
use lightning::util::string::PrintableString;
pub(crate) fn is_valid_kvstore_str(key: &str) -> bool {
- key.len() <= KVSTORE_MAX_KEY_LEN && key.chars().all(|c| KVSTORE_NAMESPACE_KEY_ALPHABET.contains(c))
+ key.len() <= KVSTORE_NAMESPACE_KEY_MAX_LEN && key.chars().all(|c| KVSTORE_NAMESPACE_KEY_ALPHABET.contains(c))
}
diff --git a/lightning/src/util/persist.rs b/lightning/src/util/persist.rs
index 4f00d3de..c476edd7 100644
--- a/lightning/src/util/persist.rs
+++ b/lightning/src/util/persist.rs
@@ -32,6 +32,6 @@ use crate::util::ser::{ReadableArgs, Writeable};
pub const KVSTORE_NAMESPACE_KEY_ALPHABET: &str = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-";
-/// The maximum number of characters keys may have.
-pub const KVSTORE_MAX_KEY_LEN: usize = 120;
+/// The maximum number of characters namespaces and keys may have.
+pub const KVSTORE_NAMESPACE_KEY_MAX_LEN: usize = 120;
/// The namespace under which the [`ChannelManager`] will be persisted.
@@ -69,7 +69,7 @@ pub const SCORER_PERSISTENCE_KEY: &str = "scorer";
///
/// Keys and namespaces are required to be valid ASCII strings in the range of
-/// [`KVSTORE_NAMESPACE_KEY_ALPHABET`] and no longer than [`KVSTORE_MAX_KEY_LEN`]. Empty namespaces
-/// and sub-namespaces (`""`) are assumed to be a valid, however, if `namespace` is empty,
-/// `sub_namespace` is required to be empty, too. This means that concerns should always be
+/// [`KVSTORE_NAMESPACE_KEY_ALPHABET`] and no longer than [`KVSTORE_NAMESPACE_KEY_MAX_LEN`]. Empty
+/// namespaces and sub-namespaces (`""`) are assumed to be a valid, however, if `namespace` is
+/// empty, `sub_namespace` is required to be empty, too. This means that concerns should always be
/// separated by namespace first, before sub-namespaces are used. While the number of namespaces
/// will be relatively small and is determined at compile time, there may be many sub-namespaces |
We upstream the `KVStore` interface trait from LDK Node, which will replace `KVStorePersister` in the coming commits. Besides persistence, `KVStore` implementations will also offer to `list` keys present in a given `namespace` and `read` the stored values.
We add a utility function needed by upcoming `KVStore` implementation tests.
We upstream the `FilesystemStore` implementation, which is backwards compatible with `lightning-persister::FilesystemPersister`.
This replaces the `FilesystemPersister::read_channelmonitors` method, as we can now implement a single utility for all `KVStore`s.
Firstly, we switch our BP over to use `FilesystemStore`, which also gives us test coverage and ensures the compatibility. Then, we remove the superseded `KVStorePersister` trait and the `FilesystemPersister` code.
We re-add benchmarking for `FilesystemStore` now that we switched over to it.
|
Addressed the feedback and force-pushed with the following changes: > git diff-tree -U2 af2bd741 7a656719
diff --git a/lightning-persister/src/fs_store.rs b/lightning-persister/src/fs_store.rs
index 77c03a64..56d071da 100644
--- a/lightning-persister/src/fs_store.rs
+++ b/lightning-persister/src/fs_store.rs
@@ -240,5 +240,5 @@ impl KVStore for FilesystemStore {
//
// In order to assert we permanently removed the file in question we therefore
- // call `fsync` on the parent directory on platforms that support it,
+ // call `fsync` on the parent directory on platforms that support it.
dir_file.sync_all()?;
}
@@ -328,5 +328,5 @@ impl KVStore for FilesystemStore {
// If we otherwise don't find a file at the given path something went wrong.
if !metadata.is_file() {
- debug_assert!(false, "Failed to list keys of {}/{}: file coulnd't be accessed.",
+ debug_assert!(false, "Failed to list keys of {}/{}: file couldn't be accessed.",
PrintableString(namespace), PrintableString(sub_namespace));
let msg = format!("Failed to list keys of {}/{}: file couldn't be accessed.",
diff --git a/lightning/src/util/persist.rs b/lightning/src/util/persist.rs
index c476edd7..ca0605c9 100644
--- a/lightning/src/util/persist.rs
+++ b/lightning/src/util/persist.rs
@@ -61,5 +61,5 @@ pub const SCORER_PERSISTENCE_SUB_NAMESPACE: &str = "";
pub const SCORER_PERSISTENCE_KEY: &str = "scorer";
-/// Provides an interface that allows to store and retrieve persisted values that are associated
+/// Provides an interface that allows storage and retrieval of persisted values that are associated
/// with given keys.
///
@@ -147,5 +147,5 @@ impl<'a, A: KVStore, M: Deref, T: Deref, ES: Deref, NS: Deref, SP: Deref, F: Der
L::Target: 'static + Logger,
{
- /// Persist the given ['ChannelManager'] to disk, returning an error if persistence failed.
+ /// Persist the given [`ChannelManager`] to disk, returning an error if persistence failed.
fn persist_manager(&self, channel_manager: &ChannelManager<M, T, ES, NS, SP, F, R, L>) -> Result<(), io::Error> {
self.write(CHANNEL_MANAGER_PERSISTENCE_NAMESPACE, |
valentinewallace
left a comment
valentinewallace
left a comment
There was a problem hiding this comment.
This seems like it's had a lot of detailed review already so I'll ack in case we want to merge today and take another glance tomorrow. Can't see there being any blocking feedback.
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
One thing that needs addressing in a followup, but given the stack of PRs waiting, gonna go ahead and land this.
6 participants
So far, the main persistence interface implemented by the user was the
KVStorePersisterwhich provided a number of blanket implementations for convenience. However, as we'll need additional functionalities soon (namely, the capability of removing previously persisted data items), this interface was rather limited.Here, we upstream the
KVStoreinterface with slight modifications from LDK Node and add theFilesystemStoreimplementation as part ofa newlightning-storagecrate that supersedeslightning-persister.