Add SQLite `KVStore` implementation #2473

tnull · 2023-08-04T09:20:03Z

Based on #2472.

We add an SQLite based implementation for the newly upstreamed KVStore interface.

tnull · 2023-08-04T09:20:42Z

Unfortunately rusqlite currently doesn't meet our MSRV, will have to figure out how to deal with that.

codecov-commenter · 2023-08-04T11:22:05Z

Codecov Report

Attention: 246 lines in your changes are missing coverage. Please review.

Comparison is base (6016101) 89.03% compared to head (b67b16d) 88.80%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2473      +/-   ##
==========================================
- Coverage   89.03%   88.80%   -0.23%     
==========================================
  Files         112      114       +2     
  Lines       86351    86830     +479     
  Branches    86351    86830     +479     
==========================================
+ Hits        76879    77112     +233     
- Misses       7235     7444     +209     
- Partials     2237     2274      +37

Files	Coverage Δ
lightning-persister/src/sqlite_store/migrations.rs	`71.85% <71.85%> (ø)`
lightning-persister/src/sqlite_store/mod.rs	`56.57% <56.57%> (ø)`
lightning-persister/src/test_utils.rs	`43.22% <6.03%> (-56.78%)`	⬇️

... and 11 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt

MSRV isn't a huge deal, just need to feature-gate it and test a relevant MSRV for the feature (does rustsqlite provide some MSRV guarantees or does it just break compat at will?)

lightning-storage/src/sqlite_store.rs

TheBlueMatt · 2023-08-13T21:24:07Z

lightning-storage/Cargo.toml

 libc = "0.2"
+rusqlite = { version = "0.28.0", features = ["bundled"] }


This should be optional, creating a feature for sqlite. Also, probably want no-default-features, at least.

Actually, I think we should really discuss whether we want to make this implementation the new recommended default, in which case I'd argue we shouldn't make it optional but rather bump MSRV to have it covered.

I'm not entirely sure what "recommended default" is - I think users can pretty easily decide if they want sqlite or filesystem storage, and we should describe in the docs when to use each.

For one I think we should support it in bindings and, at least on the Swift side, should offer the necessary glue code to make offer default implementations for the corresponding interfaces there.

Hmmmm, I'm not a huge fan of the dep tree here. eg fallible-streaming-iterator is one guy's personal repo with no external contributors - https://github.com/sfackler/fallible-streaming-iterator/commits/master?before=9217bc5e381b54b4ef4c38959488a5dc993b7b81+35&branch=master&qualified_name=refs%2Fheads%2Fmaster

Exposing our users' money to sfackler is...probably not great (but I'm sure they're a great person!)

We probably can contribute upstream making those deps optional, though - they seem to see relatively little use in the library itself?

So generally rusqlite seems to be the most mature SQLite library out there and so far the project and code base seem pretty solid to me.

That said, I agree that reducing the dependency tree would be favorable going forward. In fact making the fallible-iterator dependency optional had been considered by rusqlite before. I now asked over at rusqlite/rusqlite#833 whether they'd be willing to accept a corresponding contribution, let's see what they say.

I'd say just open a pr to make both optional. No real reason to ask to ask, just do it :)

Yeah, already started and made some progress. Just wanted to ask before I spend too much time going down that road :)

TheBlueMatt · 2023-08-13T21:41:38Z

Ah, looks like rusqlite doesn't have any MSRV guidelines, and is allergic to it. That's really obnoxious. I wonder if we should just link native sqlite? Its not like we need more than about two functions anyway, and we'd avoid all the dependencies of rusqlite.

tnull · 2023-08-25T12:27:17Z

Rebased on current state of #2472.

tnull · 2023-09-07T11:08:05Z

Rebased on current state of #2472 and added the corresponding schema migrations. Currently exploring whether there is a good way to implement batch delete, or if we should just ignore the lazy tag here.

tnull · 2023-09-07T11:11:45Z

Ah, looks like rusqlite doesn't have any MSRV guidelines, and is allergic to it. That's really obnoxious. I wonder if we should just link native sqlite? Its not like we need more than about two functions anyway, and we'd avoid all the dependencies of rusqlite.

I'd rather not reimplement the world and check that it actually works on all platforms. While they don't seem to be willing to guarantee a particular MSRV, they also promised to be very conservative in that respect, and so far I can confirm that.

G8XSU · 2023-09-11T18:51:22Z

Is this ready for review? should we rebase?

tnull · 2023-09-11T18:58:40Z

Is this ready for review? should we rebase?

Yes, although it probably won't make it into 0.0.117, which is why I now backported the changes here to lightningdevkit/ldk-node#151.

That said, rebased on main.

lightning-persister/Cargo.toml

lightning-persister/src/sqlite_store/mod.rs

G8XSU · 2023-09-11T21:14:38Z

lightning-persister/src/test_utils.rs

@@ -119,3 +124,112 @@ pub(crate) fn do_test_store<K: KVStore>(store_0: &K, store_1: &K) {
 	// Make sure everything is persisted as expected after close.
 	check_persisted_data!(CLOSED_CHANNEL_UPDATE_ID);
 }
+
+// A `KVStore` impl for testing purposes that wraps all our `KVStore`s and asserts their synchronicity.
+pub(crate) struct TestSyncStore {


Can we separate this into a new PR?
I don't completely understand the need for this.
If all stores are tested independently and pass their respective unit tests, why should we have sync store ?
We should test each one of them against a set api contract/template instead of testing them against each other. (Otherwise we are implying that we are not confident with our existing api contract or tests)

Is there a specific kind of scenario we expect this test to be "catching" that won't/can't be caught using independent store unit tests?

Can we separate this into a new PR? I don't completely understand the need for this. If all stores are tested independently and pass their respective unit tests, why should we have sync store ? We should test each one of them against a set api contract/template instead of testing them against each other. (Otherwise we are implying that we are not confident with our existing api contract or tests)

Is there a specific kind of scenario we expect this test to be "catching" that won't/can't be caught using independent store unit tests?

So, while we do and should test stores independently, we should also actually use all implementations at least for some integration tests that run actual code with actual data and assert all stores behave as expected. If we just run all tests against TestStore and test other implementations vs. synthetic test cases, we don't get a lot of test coverage of the actual usage patterns.

So, having this TestSyncStore is an easy way to use all store implementations in integration tests going forward. While we currently basically have no test cases making use of this in rust-lightning yet, there will some added with #2359, and we should definitely add more coverage going forward. We already use this in LDK Node to run most of our tests and it works great to find minor deviations and smaller stuff to keep in mind. As eventually all KVStore stuff will be upstreamed, we might as well add it as part of this PR.

We upstream our `SqliteStore` implementation that allows persistence towards an SQLite database backend.

tnull · 2023-09-29T10:48:54Z

Rebased on current main and included latest changes from LDK Node's upgrade-to-117 PR, i.e., now accounting for the renamed namespaces.

We add a `KVStore` implementation that wraps all three of our `KVStore` implementation and uses them in lock step while asserting they behave identical. This will be useful for (upcoming) integration tests.

TheBlueMatt · 2023-10-15T16:23:06Z

I guess this is blocked on #2473 (comment)? Are we gonna contribute upstream to making the various dependencies optional?

tnull · 2023-10-15T16:42:34Z

I guess this is blocked on #2473 (comment)?

While I agree that reducing the dependency tree is nice, it would be great if we could move forward with this independently to not be blocked on the change landing and a new version being released, etc.

Are we gonna contribute upstream to making the various dependencies optional?

Yes, hoping to get to it soon, already started as mentioned above. While making the fallible-iterator dependency optional should be trivial, I'm not so sure if it will be as simple (or even feasible) for fallible-streaming-iterator, as upstream a while back restructured their code in a few places around that. We'll see.

TheBlueMatt · 2023-10-15T16:53:34Z

Hmm, looking at fallible-streaming-iterator I'm not sure we should be taking anything that depends on it - its a solo single-dev project with relatively limited outside contribution (ie few people who are paying attention to what happens in it) - https://github.com/sfackler/fallible-streaming-iterator/commits/master. At least fallible-iterator has two folks merging it and some outside contribution - https://github.com/sfackler/rust-fallible-iterator/commits/master Sadly, hashlink is also a solo project, though it does have some outside contribution - https://github.com/kyren/hashlink/commits/master

The alternative, as much as it sucks, would be to use libsqlite3-sys directly.

tnull · 2023-10-16T07:41:03Z

Hmm, looking at fallible-streaming-iterator I'm not sure we should be taking anything that depends on it - its a solo single-dev project with relatively limited outside contribution (ie few people who are paying attention to what happens in it) - https://github.com/sfackler/fallible-streaming-iterator/commits/master. At least fallible-iterator has two folks merging it and some outside contribution - https://github.com/sfackler/rust-fallible-iterator/commits/master Sadly, hashlink is also a solo project, though it does have some outside contribution - https://github.com/kyren/hashlink/commits/master

Alright, will look into it further and find a solution. Just double-checked the rusqlite maintainers would be up for making it optional, now only have to figure out a reasonable way to do so.

tnull force-pushed the 2023-08-add-sqlite-store branch 2 times, most recently from f623b6b to 58f92d9 Compare August 4, 2023 09:52

tnull force-pushed the 2023-08-add-sqlite-store branch 4 times, most recently from 3def7e5 to 3e5c1f3 Compare August 4, 2023 18:25

TheBlueMatt reviewed Aug 13, 2023

View reviewed changes

tnull force-pushed the 2023-08-add-sqlite-store branch from 3e5c1f3 to 6123531 Compare August 25, 2023 12:26

tnull force-pushed the 2023-08-add-sqlite-store branch from f020ff1 to 352ebf4 Compare August 25, 2023 13:52

tnull mentioned this pull request Aug 29, 2023

Replace KVStorePersister with KVStore #2472

Merged

tnull force-pushed the 2023-08-add-sqlite-store branch from 352ebf4 to 2417956 Compare September 7, 2023 11:05

tnull force-pushed the 2023-08-add-sqlite-store branch 6 times, most recently from 9ea7009 to aa90767 Compare September 8, 2023 09:16

tnull force-pushed the 2023-08-add-sqlite-store branch from aa90767 to afc7f68 Compare September 11, 2023 18:57

TheBlueMatt reviewed Sep 11, 2023

View reviewed changes

lightning-persister/Cargo.toml Outdated Show resolved Hide resolved

lightning-persister/Cargo.toml Outdated Show resolved Hide resolved

lightning-persister/src/sqlite_store/mod.rs Outdated Show resolved Hide resolved

tnull force-pushed the 2023-08-add-sqlite-store branch 2 times, most recently from 69ed8b6 to a4ac95c Compare September 11, 2023 20:32

G8XSU reviewed Sep 11, 2023

View reviewed changes

tnull added 2 commits September 29, 2023 12:30

Add SqliteStore implementation

8640d12

We upstream our `SqliteStore` implementation that allows persistence towards an SQLite database backend.

f Add schema migrations and test them

56d7566

tnull force-pushed the 2023-08-add-sqlite-store branch from a4ac95c to 1a5dd06 Compare September 29, 2023 10:48

tnull added 3 commits September 29, 2023 12:51

f Account for renamed namespaces

260e8ee

Add TestSyncStore

617d002

We add a `KVStore` implementation that wraps all three of our `KVStore` implementation and uses them in lock step while asserting they behave identical. This will be useful for (upcoming) integration tests.

f Make more robust and account for renamed namespaces

b67b16d

tnull force-pushed the 2023-08-add-sqlite-store branch from 1a5dd06 to b67b16d Compare September 29, 2023 10:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SQLite `KVStore` implementation #2473

Add SQLite `KVStore` implementation #2473

tnull commented Aug 4, 2023

tnull commented Aug 4, 2023

codecov-commenter commented Aug 4, 2023 •

edited

Loading

TheBlueMatt left a comment

TheBlueMatt Aug 13, 2023

tnull Sep 7, 2023

TheBlueMatt Sep 11, 2023

tnull Sep 11, 2023

TheBlueMatt Sep 11, 2023

TheBlueMatt Sep 11, 2023 •

edited

Loading

TheBlueMatt Sep 11, 2023

tnull Sep 12, 2023

TheBlueMatt Sep 12, 2023

tnull Sep 12, 2023

TheBlueMatt commented Aug 13, 2023

tnull commented Aug 25, 2023

tnull commented Sep 7, 2023

tnull commented Sep 7, 2023

G8XSU commented Sep 11, 2023

tnull commented Sep 11, 2023 •

edited

Loading

G8XSU Sep 11, 2023

tnull Sep 12, 2023

tnull commented Sep 29, 2023

TheBlueMatt commented Oct 15, 2023

tnull commented Oct 15, 2023

TheBlueMatt commented Oct 15, 2023

tnull commented Oct 16, 2023

		libc = "0.2"
		rusqlite = { version = "0.28.0", features = ["bundled"] }

Add SQLite KVStore implementation #2473

Are you sure you want to change the base?

Add SQLite KVStore implementation #2473

Conversation

tnull commented Aug 4, 2023

tnull commented Aug 4, 2023

codecov-commenter commented Aug 4, 2023 • edited Loading

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Aug 13, 2023

tnull commented Aug 25, 2023

tnull commented Sep 7, 2023

tnull commented Sep 7, 2023

G8XSU commented Sep 11, 2023

tnull commented Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented Sep 29, 2023

TheBlueMatt commented Oct 15, 2023

tnull commented Oct 15, 2023

TheBlueMatt commented Oct 15, 2023

tnull commented Oct 16, 2023

Add SQLite `KVStore` implementation #2473

Add SQLite `KVStore` implementation #2473

codecov-commenter commented Aug 4, 2023 •

edited

Loading

TheBlueMatt Sep 11, 2023 •

edited

Loading

tnull commented Sep 11, 2023 •

edited

Loading