Initial payment store implementation

[tnull]

Based on #8, #9, #10, #11.

This PR implements a PaymentInfoStorage that persists individual PaymentInfo objects after every update, which is done via a KVStorePersister::persist() under the key payments/{payment_hash}.

(So essentially same approach as taken for events and peer storage)

As the data is currently only read from disk upon restart, it still assumes a FilesystemPersister, but may be adapted in the future to support the interface more generically, if KVStorePersister would expose some more functionality.

[tnull]

Rebased on current version of #11, still needs some more love.

[jkczyz]

I wonder if a persister is the right abstraction. Seems we want some abstract data storage where persistence is more of an implementation detail?

[tnull]

I agree, however, this comes down to a question of naming/terminology, as get/put/del are probably the core operations of a KV datastore. So maybe I should just create one Datastore/KVStore trait that offers all three and extends KVStorePersister, instead of adding a KVStoreAccess trait and have the trait bounds be KVStorePersister + KVStoreUnpersister + KVStoreAccess...

[jkczyz]

@G8XSU How does VSS deal with removed data?

[G8XSU]

currently vss doesn't have a delete/remove data api.
from my understanding, ldk-node uses del/remove for events, i am not sure if it will be using vss for that.
Also in later commit i see that unpersist was removed.

[tnull]

currently vss doesn't have a delete/remove data api. from my understanding, ldk-node uses del/remove for events, i am not sure if it will be using vss for that. Also in later commit i see that unpersist was removed.

Yes, not only events, also payment metadata etc. I removed the KVStoreUnpersister trait as all now switched over to this much more powerful KVStore trait:

https://github.com/tnull/ldk-node/blob/c3e488be747e8ab4cc1d3cf5aea1d8abf223839d/src/io/mod.rs#L38-L63

[jkczyz]

@G8XSU How does VSS deal with removed data?

[tnull]

So I think from my side this PR should functionally be ready. An addition would be to add a "last_updated" fields to the payment info, but I'd be happy to push that to a smaller follow-up PR. Same goes for the SQLite implementation of KVStore.

[tnull]

Now added some refactoring to align the returned structs PaymentDetails/ChannelDetails/PeerDetails and exposed methods to remove and query for filtered payment details.

[jkczyz]

Does this hold for FilesystemStore ::remove though? At the end it checks if the file exists, which could happen if a write happens after fs::remove_file.

[jkczyz]

Feel free to squash the fixups.

[tnull]

Feel free to squash the fixups.

Alright, tried squashing them in a reasonable way. However, this PR is showing it's age in that it has gotten way to big over time, will keep follow-ups more succinct.

Also added two new commits addressing feedback.

[jkczyz]

LGTM. Feel free pass on the nit. Good to squash now.

[tnull]

Squashed fixups and included the following changes:

diff --git a/src/event.rs b/src/event.rs
index 8b04585..c655bc2 100644
--- a/src/event.rs
+++ b/src/event.rs
@@ -341,6 +341,8 @@ where
                                                self.channel_manager.fail_htlc_backwards(&payment_hash);

-                                               let mut update = PaymentDetailsUpdate::new(payment_hash);
-                                               update.status = Some(PaymentStatus::Failed);
+                                               let update = PaymentDetailsUpdate {
+                                                       status: Some(PaymentStatus::Failed),
+                                                       ..PaymentDetailsUpdate::new(payment_hash)
+                                               };
                                                self.payment_store.update(&update).expect("Failed to access payment store");
                                                return;
@@ -377,6 +379,8 @@ where
                                        self.channel_manager.fail_htlc_backwards(&payment_hash);

-                                       let mut update = PaymentDetailsUpdate::new(payment_hash);
-                                       update.status = Some(PaymentStatus::Failed);
+                                       let update = PaymentDetailsUpdate {
+                                               status: Some(PaymentStatus::Failed),
+                                               ..PaymentDetailsUpdate::new(payment_hash)
+                                       };
                                        self.payment_store.update(&update).expect("Failed to access payment store");
                                }
@@ -396,9 +400,11 @@ where
                                match purpose {
                                        PaymentPurpose::InvoicePayment { payment_preimage, payment_secret, .. } => {
-                                               let mut update = PaymentDetailsUpdate::new(payment_hash);
-                                               update.preimage = Some(payment_preimage);
-                                               update.secret = Some(Some(payment_secret));
-                                               update.amount_msat = Some(Some(amount_msat));
-                                               update.status = Some(PaymentStatus::Succeeded);
+                                               let update = PaymentDetailsUpdate {
+                                                       preimage: Some(payment_preimage),
+                                                       secret: Some(Some(payment_secret)),
+                                                       amount_msat: Some(Some(amount_msat)),
+                                                       status: Some(PaymentStatus::Succeeded),
+                                                       ..PaymentDetailsUpdate::new(payment_hash)
+                                               };
                                                match self.payment_store.update(&update) {
                                                        Ok(true) => (),
@@ -491,6 +497,8 @@ where
                                );

-                               let mut update = PaymentDetailsUpdate::new(payment_hash);
-                               update.status = Some(PaymentStatus::Failed);
+                               let update = PaymentDetailsUpdate {
+                                       status: Some(PaymentStatus::Failed),
+                                       ..PaymentDetailsUpdate::new(payment_hash)
+                               };
                                self.payment_store.update(&update).expect("Failed to access payment store");
                                self.event_queue

[G8XSU]

I am still not sure if KVStore should be a stateful object, keeping track of written buffer till now.
Can we define behavior in case of key overwrite ?

Also if client just uses a write couple of times and never commits, are we left with dangling in-memory footprint of that buffer? (because depending on kvstore impl, one might have to keep in-memory writer object for each key not commited yet)
Moreover, what will be the expectation in case of lost in-memory buffers when commit hasn't been called yet? (due to restart or crash).

Imo, it would make more sense to have Writer buffer out of KVStore and write once we are ready with full buffer for key. It makes for a much cleaner interface and we can avoid some of the missteps.

[tnull]

I am still not sure if KVStore should be a stateful object, keeping track of written buffer till now. Can we define behavior in case of key overwrite ?

You mean in case we have two writers racing the same key? The current policy would be "latest commit win", which should be perfectly fine I think. If we find any issue with that, the TransactionalWrite interface would allow us to implement this differently though.

Also if client just uses a write couple of times and never commits, are we left with dangling in-memory footprint of that buffer? (because depending on kvstore impl, one might have to keep in-memory writer object for each key not commited yet) Moreover, what will be the expectation in case of lost in-memory buffers when commit hasn't been called yet? (due to restart or crash).

Yes, we're keeping the buffers around until committed, which means they a) would be lost on crash and b) take up some memory until we're sure they have been persisted. I don't see any way around that though, be it in this or another interface design? In particular, I don't see how having the persister separate would magically allow us to get rid of the in-memory read/write buffers? Of course, the reader/writer objects are not meant to be kept around forever, but only to be acquired when you need them.

Imo, it would make more sense to have Writer buffer out of KVStore and write once we are ready with full buffer for key. It makes for a much cleaner interface and we can avoid some of the missteps.

As mentioned above, I still don't see a) how the current approach is particularly error-prone b) how having the KVStore::write taking a concrete byte slice argument would fundamentally improve on the current design. Plus, it would lead to a weird asymmetry as KVStore::read would return a generic Read but write would take a raw byte slice...

So I'm happy to be convinced otherwise, but I'm not seeing the issue or that the solution you propose would improve on it functionally or design-wise for that matter.