Add SqliteStore backend

[tnull]

Based on #101.

Finally, we add an SQLite-based KVStore implementation which becomes the new default
default for Node::build. The FilesystemStore is still configurable via Node::build_with_fs_store

[G8XSU]

Can we move the last commit of SQLite impl in separate PR, would like to review it separately. :)

[tnull]

Can we move the last commit of SQLite impl in separate PR, would like to review it separately. :)

Mh, I'd actually rather not have yet another PR to maintain and rebase. But whatever. Now moved the first 3 commits to #101 upon which this PR is now based.

[tnull]

Rebased on current #101 and added a corresponding fixup that avoids allocating msg strings unnecessarily.

[G8XSU]

since this is not a filesystem store, we don't need or shouldn't use parent dir as namespace,
instead we could use a static string as namespace like "ldk_core_store"?

[tnull]

since this is not a filesystem store, we don't need or shouldn't use parent dir as namespace, instead we could use a static string as namespace like "ldk_core_store"?

Yes we should, as unfortunately LDK persistence makes strong assumptions about the structure of the keys and for example gives us channel monitors as keys monitors/[txid][index]. Plus, having the different backends decide to rename namespaces is a bad idea when it comes to migration between backends. I now DRYed up the key splitting however.

[G8XSU]

"having the different backends decide to rename namespaces is a bad idea". yes exactly
that is why we should not be using anything to do with parent directories and key parsing from ldk-key as namespace keys. (for all kvstores including filesystem)

afaiu, ldk doesn't enforce anything by having keys like "monitors/[txid][index]", ldk is just providing a prefix.
we could have something like "ldk_core" as namespace and "monitors/[txid][index]" as full key.

otherwise we are letting ldk-core create namespaces dynamically instead of ldk-node having fixed set of namespaces.(even though we own ldk-core, this is not ideal from ldk-node user perspective)

otherwise, in future, if ldk decides to store something new as "xyzMeta/" we would end up creating a namespace out of it and migration code will need to change.

[tnull]

"having the different backends decide to rename namespaces is a bad idea". yes exactly that is why we should not be using anything to do with parent directories and key parsing from ldk-key as namespace keys. (for all kvstores including filesystem)

afaiu, ldk doesn't enforce anything by having keys like "monitors/[txid][index]", ldk is just providing a prefix. we could have something like "ldk_core" as namespace and "monitors/[txid][index]" as full key.

No, it's clearly not just a prefix but an implied namespace, actually even an implied path as all assumptions are made around FilesystemPersister. We should def. not buy into the hardcoded prefixes but rather extract them and use them in a sane way. This is exactly why I added the namespaces to the KVStore interface in the first place, and I'll be looking into upstreaming them, as the hardcoded strings cluttered over different places in the code is just a mess. I'll def. won't throw "everything LDK" into a single namespace.

otherwise we are letting ldk-core create namespaces dynamically instead of ldk-node having fixed set of namespaces.(even though we own ldk-core, this is not ideal from ldk-node user perspective)

We have a pretty good grasp on what's going on in the upstream codebase and we don't make any promises about what namespaces exactly the user needs to handle: the interface takes a namespace: &str instead of an enum for a reason. So yes, any implementation of the KVStore should be able to handle new namespaces without issue.

otherwise, in future, if ldk decides to store something new as "xyzMeta/" we would end up creating a namespace out of it and migration code will need to change.

Yes, this was why I thought about adding a list_namespaces method to the interface to make all data explorable by way of the interface. However, I still think we made the right decision to keep migration out-of-bound.

[G8XSU]

No, it's clearly not just a prefix but an implied namespace, actually even an implied path as all assumptions are made around FilesystemPersister

there is no enforced implied namespace, cash-lit doesn't care about it, ldk doesn't care about it. We are making that assumption right now in this PR, as we split the key into a file-path for a non-filesystem-directory datastore. We shouldn't be thinking of this having any relation to file-path or filesystem-directory/subdirectory.

[tnull]

there is no enforced implied namespace, cash-lit doesn't care about it, ldk doesn't care about it. We are making that assumption right now in this PR, as we split the key into a file-path for a non-filesystem-directory datastore. We shouldn't be thinking of this having any relation to file-path or filesystem-directory/subdirectory.

Alright, I assume you'd then argue to do the same in FilesystemStore, i.e., save all LDK objects in a single directory? The prefix is exactly why I added namespaces in the first place, and I'll be using them in this PR for this KVStore unless there is a convincing technical argument not to. Even though I don't think it matters all too much, please note it would also be less performant to always retrieve all rows from the entire LDK namespace and filter them down to the keys we want, e.g., in read_channelmonitors.

I'm sorry but this simply doesn't make sense. If VSS doesn't support namespaces that's fine, then you should find a workaround in the respectiveKVStore impl and that may be to keep the keys prefixed, but maybe there is a better way to do it also.

[tnull]

Rebased on main after merging #101 and addressed the feedback so far.

[tnull]

Rebased on main after #88 landed.

[tnull]

@G8XSU@jkczyz Can I squash the fixups here?

[jkczyz]

@G8XSU@jkczyz Can I squash the fixups here?

SGTM

[tnull]

SGTM

Squashed without further changes.

[G8XSU]

Overall LGTM

[tnull]

Amended last commit to include the following change:

diff --git a/src/io/sqlite_store.rs b/src/io/sqlite_store.rs
index 6d7e166..ee38f5d 100644
--- a/src/io/sqlite_store.rs+++ b/src/io/sqlite_store.rs@@ -118,9 +118,8 @@ impl KVStore for SqliteStore {
fn remove(&self, namespace: &str, key: &str) -> std::io::Result<bool> {
+ let locked_conn = self.connection.lock().unwrap();+
let sql = format!("DELETE FROM {} WHERE namespace=:namespace AND key=:key;", KV_TABLE_NAME);
- let changes = self- .connection- .lock()- .unwrap()+ let changes = locked_conn
.execute(
&sql,

[tnull]

Rebased on main after #108 was merged.

[imaginator]

What's the reasoning behind having KV storage instead of a full SQL schema?

[tnull]

What's the reasoning behind having KV storage instead of a full SQL schema?

Well, for one LDK's 'native' serialization format is TLV-encoded blobs that are persisted under particular keys. BDK will follow a similar approach with the upcoming changes in 1.0. So it didn't really make sense to follow an entirely different model for LDK Node's own objects. Additionally, I'm not sure a 'real' SQL schema would have much benefits over the key value approach and the latter also allows a much simpler KVStore abstraction that can be easier implemented against a variety of backends.