add `KVStorePersister` trait and blanket implementations of `Persist` and `Persister` for any type that implements `KVStorePersister` #1417

johncantrell97 · 2022-04-11T17:56:44Z

Introduces a KVStorePersister trait for persisting Writeable objects with a certain String key.
Adds blanket Persist and Persister implementations for any type that implements KVStorePersister
implement KVStorePersister for FilesystemPersister using util::write_to_file to persist to filesystem.
Moved Persister trait from lightning-background-processor to new lightning::util::persister module
Use String to build paths in FilesystemPersister to avoid needing to go from PathBuf to String (to use as key in KVStorePersister).

TheBlueMatt

Concept ACK.

TheBlueMatt · 2022-04-11T18:23:04Z

lightning-persister/src/lib.rs

@@ -32,6 +32,22 @@ use std::io::{Cursor, Error};
 use std::ops::Deref;
 use std::path::{Path, PathBuf};

+impl DiskWriteable for Vec<u8> {


Would strongly prefer to not encode into a vec and then dump, instead we can just define persist_bytes as generic over a DiskWriteable.

Hm, but then it's only intended to be used by a FilesystemPersister. Part of the motivation for this is to make it easier to implement a generic persistence layer.

I mean we can also make the DiskWriteable trait public and define it in terms of a Writer?

Or, really, we could drop DiskWriteable entirely and use the lightning crate's existing Writeable trait.

nice. removed DiskWriteable and was able to use Writeable everywhere instead and avoid the extra encode()

TheBlueMatt · 2022-04-11T18:24:27Z

lightning-persister/src/util.rs

+pub(crate) fn write_to_file<D: DiskWriteable>(full_filepath: String, data: &D) -> std::io::Result<()> {
+	let full_filepath = PathBuf::from(full_filepath);
+	let path = full_filepath.parent().unwrap();
+	let filename = full_filepath.file_name().unwrap().to_str().unwrap().to_string();


bleh. Can we not at least leave this an str or maybe split on a / manually instead of going through PathBuf? Are there any platforms that use '' for paths anymore?

removed need for PathBuf here -- the original implementation took in a PathBuf which is why I tried to 'put it back'

TheBlueMatt · 2022-04-11T18:27:16Z

lightning-persister/src/lib.rs

-		let path = PathBuf::from(data_dir);
-		util::write_to_file(path, "network_graph".to_string(), network_graph)
+	pub fn path_to_monitor_data_str(&self) -> &str {
+		self.path_to_monitor_data().to_str().unwrap()


Oof. Let's not keep converting PathBuf->str->String->PathBuf->.... quite a waste of allocations.

yeah, well if we are okay with building path using "/" then we don't need to do this I guess? In the current code the method to get monitors path returned a PathBuf but we need a String key for the new trait. Only way to avoid this is to return a String for the monitors path using "/"

Hmm, I guess Winblowz still uses "".

TheBlueMatt · 2022-04-11T19:25:02Z

lightning-persister/src/lib.rs

+}
+
+/// Trait for a key-value store for persisting some writeable object at some key
+pub trait PersistObject<W: Writeable> {


Maybe lets call this KVStorePersister?

sure, sounds good to me

TheBlueMatt · 2022-04-11T19:27:02Z

lightning-persister/src/lib.rs

-		let path = PathBuf::from(data_dir);
-		util::write_to_file(path, "network_graph".to_string(), network_graph)
+	pub fn path_to_monitor_data_str(&self) -> &str {
+		self.path_to_monitor_data().to_str().unwrap()


Hmm, I guess Winblowz still uses "".

TheBlueMatt · 2022-04-11T19:27:34Z

lightning-persister/src/util.rs

-pub(crate) fn write_to_file<D: DiskWriteable>(path: PathBuf, filename: String, data: &D) -> std::io::Result<()> {
-	fs::create_dir_all(path.clone())?;
+pub(crate) fn write_to_file<W: Writeable>(filename_with_path: String, data: &W) -> std::io::Result<()> {
+	let mut path_parts: Vec<&str> = filename_with_path.split("/").collect();


Can we right-split and not collect into a Vec?

hm well if we split with char '/' it returns DoubleEndedIterator so don't need rsplit.
but if we're not okay with assuming '/' as separator then we can't do this anyway?

tried doing this by folding the iterator into a String but I'm not really sure if it's better than just collecting and joining

oh looks like you can just collect straight into a PathBuf (which is what we want anyway) and avoids using the '/'. Solves both problems at once? at least for here

er I guess we're still splitting on "/" so doesn't entirely avoid it.

I'm not really sure what to do with this PathBuf <--> String issue

it's annoying because on both sides of the trait we want PathBuf but the trait is defined over a String key.

ended up keeping everything String's and building paths manually with format!() and std::path::MAIN_SEPARATOR. Not sure if this is the best way though.

Finally convert to PathBuf inside of util::write_to_file when needed.

TheBlueMatt

Shaping up nicely, I think.

TheBlueMatt · 2022-04-12T20:37:08Z

lightning-background-processor/src/lib.rs

-	L::Target: 'static + Logger,
-{
-	/// Persist the given [`ChannelManager`] to disk, returning an error if persistence failed
-	/// (which will cause the [`BackgroundProcessor`] which called this method to exit).


We should move this line somewhere else in the BP docs, I'd think, not move it to the lightning-persister crate.

hm the BP docs already also have this:

/// [Persister::persist_manager] is responsible for writing out the [ChannelManager] to disk, and/or
/// uploading to one or more backup services. See [ChannelManager::write] for writing out a
/// [ChannelManager]. See [FilesystemPersister::persist_manager] for Rust-Lightning's
/// provided implementation.

could just add the bit about failure causing BP to exit. also should update the bit here about the provided implementation.

TheBlueMatt · 2022-04-12T20:37:36Z

lightning-persister/src/lib.rs

+}
+
+/// Trait for a key-value store for persisting some writeable object at some key
+pub trait KVStorePersister<W: Writeable> {


Shouldn't the W be on the persist method? A single KVStorePersister should probably care about being able to persist more than one type of W.

agreed, moved it

TheBlueMatt · 2022-04-12T20:38:14Z

lightning-persister/src/lib.rs

+	/// Persist the given [`ChannelManager`] to disk, returning an error if persistence failed
+	/// (which will cause the BackgroundProcessor which called this method to exit).
+	fn persist_manager<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>(&self, channel_manager: &ChannelManager<Signer, M, T, K, F, L>) -> Result<(), std::io::Error> where
+	M::Target: 'static + chain::Watch<Signer>,


Hmm, what is the error if you keep the types on the Persister trait? It does seem like a single instance of a Persister should really only care about persisting one type of ChannelManager, not multiple.

The error is on the test struct Persister that is used in the BackgroundProcessor tests:

error[E0284]: type annotations needed --> lightning-background-processor/src/lib.rs:440:39 | 440 | None => self.filesystem_persister.persist_graph(network_graph), | ^^^^^^^^^^^^^ cannot infer type for type parameter `M` declared on the trait `Persister` | = note: cannot satisfy `<_ as Deref>::Target == _`

Being explicit solves the issue for me -
LPPersister::<Signer, M, T, K, F, L>::persist_graph(&self.filesystem_persister, network_graph)

ended up just removing this whole section of code since it's unnecessary to explicitly impl Persister anymore. Just implemented KVStorePersister here and handled throwing errors based on the key

TheBlueMatt · 2022-04-12T20:44:22Z

lightning-persister/src/util.rs

-pub(crate) fn write_to_file<D: DiskWriteable>(path: PathBuf, filename: String, data: &D) -> std::io::Result<()> {
-	fs::create_dir_all(path.clone())?;
+pub(crate) fn write_to_file<W: Writeable>(filename_with_path: String, data: &W) -> std::io::Result<()> {
+	let full_path = PathBuf::from(&filename_with_path);


If you move the tmp_filename creation to above here we can cut an allocation by dropping the reference here.

Drop & as per above?

TheBlueMatt · 2022-04-12T20:45:58Z

lightning-persister/src/util.rs

 	// Do a crazy dance with lots of fsync()s to be overly cautious here...
 	// We never want to end up in a state where we've lost the old data, or end up using the
 	// old data on power loss after we've returned.
 	// The way to atomically write a file on Unix platforms is:
 	// open(tmpname), write(tmpfile), fsync(tmpfile), close(tmpfile), rename(), fsync(dir)
-	let filename_with_path = get_full_filepath(path, filename);
-	let tmp_filename = format!("{}.tmp", filename_with_path.clone());
+	let tmp_filename = format!("{}.tmp", filename_with_path);


Not your bug, but lets drop format and instead clone the filename_with_path and push_str the ".tmp" string.

TheBlueMatt · 2022-04-12T20:52:37Z

lightning-persister/src/lib.rs

+
+impl<W: Writeable> KVStorePersister<W> for FilesystemPersister {
+	fn persist(&self, key: String, object: &W) -> std::io::Result<()> {
+		util::write_to_file(format!("{}{}{}", self.path_to_channel_data, MAIN_SEPARATOR, key), object)


Oh, right, I forgot we build the key first and then want to push a prefix onto it...means we can't avoid a second allocation here.

TheBlueMatt · 2022-04-12T20:56:10Z

lightning-persister/src/lib.rs

@@ -187,18 +164,39 @@ impl<ChannelSigner: Sign> chainmonitor::Persist<ChannelSigner> for FilesystemPer
 	// even broadcasting!

 	fn persist_new_channel(&self, funding_txo: OutPoint, monitor: &ChannelMonitor<ChannelSigner>, _update_id: chainmonitor::MonitorUpdateId) -> Result<(), chain::ChannelMonitorUpdateErr> {
-		let filename = format!("{}_{}", funding_txo.txid.to_hex(), funding_txo.index);
-		util::write_to_file(self.path_to_monitor_data(), filename, monitor)
+		let key = format!("monitors{}{}_{}", MAIN_SEPARATOR, funding_txo.txid.to_hex(), funding_txo.index);


I was thinking lets not boterh directly implementing Persist or Persister at all for the FilesystemPersister, instead opting to implement them automatically for any KVStorePersister.

ah that's a good idea. only downside I can think of is that it removes control of the keys used from an implementor. I guess they could always override the implementation? I'm not sure how/if that would work though

codecov-commenter · 2022-04-14T00:44:30Z

Codecov Report

Merging #1417 (a857771) into main (83595db) will increase coverage by 0.58%.
The diff coverage is 98.33%.

❗ Current head a857771 differs from pull request most recent head 08a84e9. Consider uploading reports for the commit 08a84e9 to get more accurate results

@@            Coverage Diff             @@
##             main    #1417      +/-   ##
==========================================
+ Coverage   90.84%   91.42%   +0.58%     
==========================================
  Files          74       75       +1     
  Lines       41288    44329    +3041     
  Branches    41288    44329    +3041     
==========================================
+ Hits        37507    40528    +3021     
- Misses       3781     3801      +20

Impacted Files	Coverage Δ
lightning/src/util/persist.rs	`93.75% <93.75%> (ø)`
lightning-background-processor/src/lib.rs	`95.22% <100.00%> (+0.01%)`	⬆️
lightning-persister/src/lib.rs	`93.33% <100.00%> (-0.61%)`	⬇️
lightning-persister/src/util.rs	`95.77% <100.00%> (ø)`
lightning/src/ln/functional_test_utils.rs	`95.58% <0.00%> (+0.04%)`	⬆️
lightning-invoice/src/utils.rs	`97.02% <0.00%> (+0.34%)`	⬆️
lightning/src/ln/onion_route_tests.rs	`98.10% <0.00%> (+0.47%)`	⬆️
lightning/src/routing/router.rs	`93.19% <0.00%> (+0.69%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.93% <0.00%> (+0.79%)`	⬆️
lightning/src/ln/priv_short_conf_tests.rs	`98.93% <0.00%> (+1.09%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83595db...08a84e9. Read the comment docs.

TheBlueMatt

OK! This is looking good now. I have a few further comments on the KVStore trait but I think we're basically there.

TheBlueMatt · 2022-04-14T15:51:25Z

lightning-background-processor/src/lib.rs

@@ -133,6 +119,7 @@ impl BackgroundProcessor {
 	/// The thread runs indefinitely unless the object is dropped, [`stop`] is called, or
 	/// [`Persister::persist_manager`] returns an error. In case of an error, the error is retrieved by calling
 	/// either [`join`] or [`stop`].
+	/// 


nit: spurious line addition?

TheBlueMatt · 2022-04-14T16:06:21Z

lightning/src/util/persist.rs

+
+impl<A: KVStorePersister, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> Persister<Signer, M, T, K, F, L> for A 
+where 
+M::Target: 'static + chain::Watch<Signer>,


nit: generally we indent the conditions here.

TheBlueMatt · 2022-04-14T16:06:49Z

lightning/src/util/persist.rs

+}
+
+impl<A: KVStorePersister, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> Persister<Signer, M, T, K, F, L> for A 
+where 


nit: spurious whitespace at EOL here and a few other places. I'd generally suggest glancing with a local git show which should highlight such whitespace depending on your terminal settings.

why not just use rustfmt for the project and run on pre-commit or whatever?

Sadly rustfmt tends to generate garbage code formatting IMO, doubly so on our codebase. #410 (comment)

TheBlueMatt · 2022-04-14T16:07:57Z

lightning/src/util/persist.rs

+}
+
+impl<ChannelSigner: Sign, K: KVStorePersister> Persist<ChannelSigner> for K {
+	// TODO: We really need a way for the persister to inform the user that its time to crash/shut


Lets open an issue for this? Using the Event::ChannelClosed event may make sense here, I think.

TheBlueMatt · 2022-04-14T16:08:49Z

lightning/src/util/persist.rs

+// You may not use this file except in accordance with one or both of these
+// licenses.
+
+//! Traits for handling persistence


Can we expand on this somewhat? Lets say something about how we expose a simple KVStore trait which can be used to implement both a graph + manager persistence and chain::Persist all in one simple implementation.

TheBlueMatt · 2022-04-14T16:13:42Z

lightning/src/util/persist.rs

+	/// Persist the given writeable using the provided key
+	fn persist<W: Writeable>(&self, key: String, object: &W) -> io::Result<()>;
+	/// Get the key for persisting a channel manager
+	fn get_channel_manager_key(&self) -> String { String::from("manager") }


Hmmmm, do users have a good reason to override this? ISTM its less work to just implement Persister if they want to bother with overriding the key. We might as well just remove the key-fetch functions and make them a part of the trait definition.

It's not entirely clear if there's a good reason to override yet but it might be useful to know what the keys are when implementing KVStorePersister persist. For example you might want to ignore errors when persisting the network_graph and just print a warning to console. To do that you'd need to be able to know the key being used by persist_graph inside of persist.

Without this you'd have to just hardcode the string in your persist implementation.

When you say "make them part of the train definition" do you mean just hardcode the strings into the implementation of Persister/Persist?

removed these for now

TheBlueMatt · 2022-04-14T16:13:58Z

lightning/src/util/persist.rs

+/// Trait for a key-value store for persisting some writeable object at some key
+pub trait KVStorePersister {
+	/// Persist the given writeable using the provided key
+	fn persist<W: Writeable>(&self, key: String, object: &W) -> io::Result<()>;


nit: given the keys for several things are static, lets just make it a &str instead of a String.

TheBlueMatt · 2022-04-15T19:40:54Z

Looks like this needs rebase now, sorry about that.

johncantrell97 · 2022-04-16T00:57:54Z

think I addressed all your comments, rebased, and have all checks passing. would love another pass when you get a chance, think this is pretty much ready at this point

TheBlueMatt

A few trivial comments but I'm happy with this.

TheBlueMatt · 2022-04-16T16:51:14Z

lightning-persister/src/util.rs

 	// Do a crazy dance with lots of fsync()s to be overly cautious here...
 	// We never want to end up in a state where we've lost the old data, or end up using the
 	// old data on power loss after we've returned.
 	// The way to atomically write a file on Unix platforms is:
 	// open(tmpname), write(tmpfile), fsync(tmpfile), close(tmpfile), rename(), fsync(dir)
-	let filename_with_path = get_full_filepath(path, filename);
-	let tmp_filename = format!("{}.tmp", filename_with_path.clone());
+


nit: spurious newline (also EOL whitespace :) )

TheBlueMatt · 2022-04-16T17:22:01Z

lightning/src/util/persist.rs

+use crate::{chain::{keysinterface::{Sign, KeysInterface}, self, transaction::{OutPoint}, chaininterface::{BroadcasterInterface, FeeEstimator}, chainmonitor::{Persist, MonitorUpdateId}, channelmonitor::{ChannelMonitor, ChannelMonitorUpdate}}, ln::channelmanager::ChannelManager, routing::network_graph::NetworkGraph};
+use super::{logger::Logger, ser::Writeable};
+
+/// Trait for a key-value store for persisting some writeable object at some key


Given we rely on the specific paths downstream, both in FilesystemPersister::read_channelmanager and even downstream in the sample node (:sob:), we should document the paths here. Ideally we'd add a read method on KVStorePersister and then implement the loads here against that, but I don't think we need to do that in this specific PR given its not a new issue.

yeah, that's why I had at first added the key getters.

I actually already implemented it in Sensei because it seemed like you didn't want in ldk. I added a read and a list method since read_channelmonitors needs to be able to list all keys/filenames.

Here's what I did, let me know if you think it's roughly what you'd want to add to KVStorePersister for #1427
https://github.com/L2-Technology/sensei/pull/37/files#diff-af422d8a9ab5e8778884033d55578f060f9d14dde012232f4faeb2b8a49cec92R12

Right, I'd prefer we keep the keys as part of the util::persist module and abstract it away from the user instead of exposing them. That would imply, like you say, adding a read and list method in a followup and having utils to read the data. That is also important so that we can change the persistence behavior in the future, eg for #1426.

TheBlueMatt · 2022-04-16T17:30:44Z

lightning-background-processor/src/lib.rs

@@ -161,8 +146,8 @@ impl BackgroundProcessor {
 	/// [`stop`]: Self::stop
 	/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
 	/// [`ChannelManager::write`]: lightning::ln::channelmanager::ChannelManager#impl-Writeable
-	/// [`FilesystemPersister::persist_manager`]: lightning_persister::FilesystemPersister::persist_manager
-	/// [`FilesystemPersister::persist_network_graph`]: lightning_persister::FilesystemPersister::persist_network_graph
+	/// [`FilesystemPersister::persist_manager`]: lightning_persister::FilesystemPersister#impl-Persister


Hmm, this link doesn't work now, it seems. I think maybe just link directly to Persister::persist_*

TheBlueMatt · 2022-04-16T17:43:04Z

lightning-persister/src/util.rs

-	use super::{DiskWriteable, get_full_filepath, write_to_file};
+	use lightning::util::ser::{Writer, Writeable};
+
+use super::{write_to_file};


nit: indentation.

TheBlueMatt

Basically LGTM. Needs a second review.

TheBlueMatt · 2022-04-17T15:26:31Z

lightning-background-processor/src/lib.rs

@@ -138,11 +123,11 @@ impl BackgroundProcessor {
 	///
 	/// [`Persister::persist_manager`] is responsible for writing out the [`ChannelManager`] to disk, and/or
 	/// uploading to one or more backup services. See [`ChannelManager::write`] for writing out a
-	/// [`ChannelManager`]. See [`FilesystemPersister::persist_manager`] for Rust-Lightning's
+	/// [`ChannelManager`]. See [`Persister::persist_manager`] for Rust-Lightning's


Oops, I think I was sleepwalking, sorry. Seems strange to link to the trait for "Rust-Lightning's provided implementation", maybe lets just link directly to FilesystemPersister with no specific method link? Also, let's call it "LDK's provided implementation" now :)

yeah, linking to FilesystemPersister is also kind of strange because they won't find a persist_manager or persist_graph implementation there.

also i don't think FilesystemPersister is used in BP anymore and so it complains about no item in scope -- should I just import it only to be used in the comment?

Ah, right, we should remove it as a dependency (I think it can be a dev-dependency now, no?), then. We can also just say "see the lightning-persister crate", no? I don't think its an issue that we don't link to a specific function, given there's an implementation for the trait mentioned in the previous sentence.

TheBlueMatt · 2022-04-17T15:30:54Z

lightning/src/util/persist.rs

+// You may not use this file except in accordance with one or both of these
+// licenses.
+
+//! Expose a simple key-value store trait KVStorePersister that allows one to 


s/Expose/This module contains/

TheBlueMatt · 2022-04-17T15:31:21Z

lightning/src/util/persist.rs

+// licenses.
+
+//! Expose a simple key-value store trait KVStorePersister that allows one to 
+//! implement the persistence for ChannelManager, NetworkGraph, and 


nit: here and a few other places in this file you have whitespace at EOL. %s/ $//g should fix it :)

TheBlueMatt · 2022-04-17T15:31:57Z

lightning/src/util/persist.rs

+
+/// Trait for a key-value store for persisting some writeable object at some key
+/// Implementing `KVStorePersister` provides auto-implementations for `Persister`
+/// and `Persist` traits.  It uses "manager", "network_graph", 


Oh, lets link to the traits with []s.

johncantrell97 · 2022-04-19T18:36:52Z

hopefully this is good-to-go at this point pending another review?

jkczyz · 2022-04-20T05:11:05Z

lightning-background-processor/src/lib.rs

 	use lightning_invoice::payment::{InvoicePayer, RetryAttempts};
 	use lightning_invoice::utils::DefaultRouter;
-	use lightning_persister::FilesystemPersister;
-	use std::fs;
+	use lightning_persister::{FilesystemPersister};


nit: remove braces

jkczyz · 2022-04-20T05:11:39Z

lightning-background-processor/src/lib.rs

@@ -414,12 +401,14 @@ mod tests {
 	struct Persister {
 		data_dir: String,
 		graph_error: Option<(std::io::ErrorKind, &'static str)>,
-		manager_error: Option<(std::io::ErrorKind, &'static str)>
+		manager_error: Option<(std::io::ErrorKind, &'static str)>,
+		filesystem_persister: FilesystemPersister


nit: I realize this wasn't there before, but cold you add a trailing comma?

jkczyz · 2022-04-20T05:16:33Z

lightning-persister/src/lib.rs

-		let path = PathBuf::from(data_dir);
-		util::write_to_file(path, "network_graph".to_string(), network_graph)
+	pub(crate) fn path_to_monitor_data(&self) -> String {
+			format!("{}{}monitors", self.path_to_channel_data, MAIN_SEPARATOR)


remove extra tab

jkczyz · 2022-04-20T05:22:09Z

lightning-persister/src/lib.rs

-		let path = PathBuf::from(data_dir);
-		util::write_to_file(path, "network_graph".to_string(), network_graph)
+	pub(crate) fn path_to_monitor_data(&self) -> String {
+			format!("{}{}monitors", self.path_to_channel_data, MAIN_SEPARATOR)


Given this is used only once, could we instead inline it at the call site where it already uses PathBuf. That would prevent double separators, IIUC. Not sure how we care about that, but no need for the method at very least.

yeah that makes sense to me. though in #1427 we will be using string keys to list monitors dir and read monitors so we might run into the string => PathBuf => string conversion fun again.

jkczyz · 2022-04-20T05:24:31Z

lightning-persister/src/util.rs

-pub(crate) fn write_to_file<D: DiskWriteable>(path: PathBuf, filename: String, data: &D) -> std::io::Result<()> {
-	fs::create_dir_all(path.clone())?;
+pub(crate) fn write_to_file<W: Writeable>(filename_with_path: String, data: &W) -> std::io::Result<()> {
+	let full_path = PathBuf::from(&filename_with_path);


Drop & as per above?

jkczyz · 2022-04-20T05:28:19Z

lightning/src/util/persist.rs

+//! This module contains a simple key-value store trait KVStorePersister that
+//! allows one to implement the persistence for ChannelManager, NetworkGraph,
+//! and ChannelMonitor all in one place.


Could you tick and bracket the types so they are linked in the docs?

jkczyz · 2022-04-20T05:32:12Z

lightning-persister/src/lib.rs

@@ -124,7 +66,7 @@ impl FilesystemPersister {
 		where K::Target: KeysInterface<Signer=Signer> + Sized,
 	{
 		let path = self.path_to_monitor_data();
-		if !Path::new(&path).exists() {
+		if !Path::new(&PathBuf::from(&path)).exists() {


Likewise with dropping & on path.

jkczyz · 2022-04-20T05:46:18Z

lightning-persister/src/util.rs

@@ -40,21 +33,23 @@ fn path_to_windows_str<T: AsRef<OsStr>>(path: T) -> Vec<winapi::shared::ntdef::W
 }

 #[allow(bare_trait_objects)]
-pub(crate) fn write_to_file<D: DiskWriteable>(path: PathBuf, filename: String, data: &D) -> std::io::Result<()> {
-	fs::create_dir_all(path.clone())?;
+pub(crate) fn write_to_file<W: Writeable>(filename_with_path: String, data: &W) -> std::io::Result<()> {


Not sure I understand why PathBuf is avoided so much. Can't we simply change the the type of filename_with_path to PathBuf and never convert it to String? Seems we are allocating by using format!, which has the double separator drawback, too.

The problem is that the KVStorePersister trait uses a String key instead of a PathBuf because it's intended to be used for more than filesystem persistence. KVStorePersister is the caller of write_to_file so it only has a String key.

I think the idea was that if we are going to need to convert the PathBuf to a String in order to persist via KVStorePersister then we might as well avoid building the PathBuf in the first place.

As for this method, I also just realized we were recreating the PathBuf and cloning filename_from_path down where we do the rename/copy from tmp to final. I removed it and just re-used the PathBuf we create at the top.

The problem is that the KVStorePersister trait uses a String key instead of a PathBuf because it's intended to be used for more than filesystem persistence. KVStorePersister is the caller of write_to_file so it only has a String key.

Doesn't PathBuf::push just store a reference? Its parameter is P: AsRef<Path>, where str and String's implementations call Path::new, which is described as "a cost-free conversion", and where there is a blanket implementation for reference types.

I think the idea was that if we are going to need to convert the PathBuf to a String in order to persist via KVStorePersister then we might as well avoid building the PathBuf in the first place.

Not sure I follow why FilesystemPersister::persist's implementation of KVStorePersister would need to convert to a string. It's given a string and from there it can cheaply wrap it in a PathBuf and never need to convert back to a string if write_to_file is changed to take a PathBuf.

Yeah it's not the implementation of KVStorePersister that has the conversion issue (going from String to PathBuf is free I think) it's the usage of persist. On main it uses PathBuf's for building up the paths for manager,graph,monitors but persist needs the paths as &str so originally this branch was taking the PathBuf's and converting to str

in the next iteration I removed the usage of PathBuf for building up the paths in the first place.

but yeah now the blanket implementation is using "/" which is a problem. I think either we use the MAIN_SEPARATOR thing there or maybe your suggestion of keys being &[&str] would work?

Yeah it's not the implementation of KVStorePersister that has the conversion issue (going from String to PathBuf is free I think) it's the usage of persist. On main it uses PathBuf's for building up the paths for manager,graph,monitors but persist needs the paths as &str so originally this branch was taking the PathBuf's and converting to str

Do you mean for FilesystemPersister::path_to_channel_data? Isn't that a one-time occurrence? persist takes the key, yes, but it's the FilesystemPersister implementation that tacks on path_to_channel_data as the path prefix.

Hm I was just trying to explain how we got here, since yeah the original provided implementation was only part of FilesystemPersister and it used PathBuf for everything (manager, graph, monitor).

At this point this PR is such a mess it's getting hard to follow for me. I was mostly following matts comments that led us here. Can we maybe just sync on this on slack real quick to make a final call so we can just get this over the finish line?

jkczyz · 2022-04-20T15:26:07Z

lightning/src/util/persist.rs

+		let key = format!("monitors/{}_{}", funding_txo.txid.to_hex(), funding_txo.index);
+		self.persist(&key, monitor)
+			.map_err(|_| chain::ChannelMonitorUpdateErr::PermanentFailure)
+	}
+
+	fn update_persisted_channel(&self, funding_txo: OutPoint, _update: &Option<ChannelMonitorUpdate>, monitor: &ChannelMonitor<ChannelSigner>, _update_id: MonitorUpdateId) -> Result<(), chain::ChannelMonitorUpdateErr> {
+		let key = format!("monitors/{}_{}", funding_txo.txid.to_hex(), funding_txo.index);


Should we use the separator constant here? Or Maybe key should be &[&str] to avoid having filesystem concepts in a blanket implementation?

jkczyz · 2022-04-20T15:35:17Z

lightning-persister/src/util.rs

@@ -40,21 +33,23 @@ fn path_to_windows_str<T: AsRef<OsStr>>(path: T) -> Vec<winapi::shared::ntdef::W
 }

 #[allow(bare_trait_objects)]
-pub(crate) fn write_to_file<D: DiskWriteable>(path: PathBuf, filename: String, data: &D) -> std::io::Result<()> {
-	fs::create_dir_all(path.clone())?;
+pub(crate) fn write_to_file<W: Writeable>(filename_with_path: String, data: &W) -> std::io::Result<()> {


The problem is that the KVStorePersister trait uses a String key instead of a PathBuf because it's intended to be used for more than filesystem persistence. KVStorePersister is the caller of write_to_file so it only has a String key.

Doesn't PathBuf::push just store a reference? Its parameter is P: AsRef<Path>, where str and String's implementations call Path::new, which is described as "a cost-free conversion", and where there is a blanket implementation for reference types.

I think the idea was that if we are going to need to convert the PathBuf to a String in order to persist via KVStorePersister then we might as well avoid building the PathBuf in the first place.

Not sure I follow why FilesystemPersister::persist's implementation of KVStorePersister would need to convert to a string. It's given a string and from there it can cheaply wrap it in a PathBuf and never need to convert back to a string if write_to_file is changed to take a PathBuf.

jkczyz · 2022-04-20T15:36:08Z

lightning-persister/src/util.rs

+	let mut tmp_filename = filename_with_path.clone();
+	tmp_filename.push_str(".tmp");


FWIW, PathBuf has a set_extension method, which avoid needing the above clone IIUC. Essentially, you could make one PathBuf for the final path, clone it, and set the temp extension on the clone.

jkczyz

Largely looks good. Just some minor comments.

jkczyz · 2022-04-20T18:50:53Z

lightning-persister/src/lib.rs

 use std::ops::Deref;
-use std::path::{Path, PathBuf};
+use std::path::{Path, PathBuf, MAIN_SEPARATOR};


no longer needed

jkczyz · 2022-04-20T18:53:53Z

lightning-persister/src/lib.rs

+		let mut dest_file = PathBuf::from(self.path_to_channel_data.clone());
+		dest_file.push(key);


Should we allocate using PathBuf::with_capacity using the size of the two strings and then push a reference to self.path_to_channel_data and key?

Like this?

let mut dest_file = PathBuf::with_capacity(self.path_to_channel_data.len() + key.len()); dest_file.push(&self.path_to_channel_data); dest_file.push(key);

Fix several "unused" warnings introduced in #1417

TheBlueMatt reviewed Apr 11, 2022

View reviewed changes

johncantrell97 force-pushed the generic-persist branch 2 times, most recently from 75900b7 to 324fbd5 Compare April 11, 2022 19:12

johncantrell97 changed the title ~~implement Persist and Persister on FilesystemPersister with generic PersistBytes trait~~ implement Persist and Persister on FilesystemPersister with generic PersistObject trait Apr 11, 2022

TheBlueMatt reviewed Apr 11, 2022

View reviewed changes

johncantrell97 force-pushed the generic-persist branch 5 times, most recently from 6fa6a98 to 95fc538 Compare April 12, 2022 14:12

johncantrell97 changed the title ~~implement Persist and Persister on FilesystemPersister with generic PersistObject trait~~ implement Persist and Persister on FilesystemPersister with KVStorePersister trait Apr 12, 2022

jkczyz self-requested a review April 12, 2022 19:44

TheBlueMatt reviewed Apr 12, 2022

View reviewed changes

TheBlueMatt mentioned this pull request Apr 13, 2022

Add utils to persist scorer in BackgroundProcessor #1416

Merged

johncantrell97 force-pushed the generic-persist branch 3 times, most recently from 4314915 to cb6c7c6 Compare April 14, 2022 00:44

johncantrell97 force-pushed the generic-persist branch 2 times, most recently from a66abe9 to 6600143 Compare April 14, 2022 01:05

johncantrell97 changed the title ~~implement Persist and Persister on FilesystemPersister with KVStorePersister trait~~ add KVStorePersister trait and blanket implementations of Persist and Persister for any type that implements KVStorePersister Apr 14, 2022

TheBlueMatt reviewed Apr 14, 2022

View reviewed changes

johncantrell97 force-pushed the generic-persist branch from 6600143 to 11fedb6 Compare April 15, 2022 19:36

johncantrell97 force-pushed the generic-persist branch 3 times, most recently from d975106 to 6299afc Compare April 15, 2022 21:06

TheBlueMatt mentioned this pull request Apr 16, 2022

Use ChannelMonitorUpdates in FilesystemPersister #1426

Open

TheBlueMatt reviewed Apr 16, 2022

View reviewed changes

TheBlueMatt mentioned this pull request Apr 16, 2022

Move object reads to KVStorePersister post-1417 #1427

Closed

johncantrell97 force-pushed the generic-persist branch from 6299afc to 54914cd Compare April 16, 2022 21:33

TheBlueMatt reviewed Apr 17, 2022

View reviewed changes

TheBlueMatt assigned jkczyz Apr 17, 2022

johncantrell97 force-pushed the generic-persist branch 2 times, most recently from 70095c5 to 960191e Compare April 19, 2022 02:07

jkczyz reviewed Apr 20, 2022

View reviewed changes

johncantrell97 force-pushed the generic-persist branch 3 times, most recently from 9a1a978 to a857771 Compare April 20, 2022 12:22

jkczyz reviewed Apr 20, 2022

View reviewed changes

johncantrell97 force-pushed the generic-persist branch from a857771 to 7e9d008 Compare April 20, 2022 18:43

jkczyz reviewed Apr 20, 2022

View reviewed changes

TheBlueMatt previously approved these changes Apr 20, 2022

View reviewed changes

johncantrell97 dismissed TheBlueMatt’s stale review via 08a84e9 April 20, 2022 20:17

johncantrell97 force-pushed the generic-persist branch from 7e9d008 to 08a84e9 Compare April 20, 2022 20:17

implement Persist and Persister with generic KVStorePersister trait

4964944

johncantrell97 force-pushed the generic-persist branch from 08a84e9 to 4964944 Compare April 20, 2022 20:47

TheBlueMatt approved these changes Apr 20, 2022

View reviewed changes

jkczyz approved these changes Apr 20, 2022

View reviewed changes

TheBlueMatt merged commit d0f69f7 into lightningdevkit:main Apr 21, 2022

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Apr 24, 2022

Fix several "unused" warnings introduced in lightningdevkit#1417

fa9fc0d

valentinewallace added a commit that referenced this pull request Apr 25, 2022

Merge pull request #1448 from TheBlueMatt/2022-04-fix-warnings

d1b984d

Fix several "unused" warnings introduced in #1417

tnull mentioned this pull request Sep 8, 2022

Allow keys to be unpersisted #1705

Closed

		let mut tmp_filename = filename_with_path.clone();
		tmp_filename.push_str(".tmp");

		let mut dest_file = PathBuf::from(self.path_to_channel_data.clone());
		dest_file.push(key);

add KVStorePersister trait and blanket implementations of Persist and Persister for any type that implements KVStorePersister #1417

add KVStorePersister trait and blanket implementations of Persist and Persister for any type that implements KVStorePersister #1417

Conversation

johncantrell97 commented Apr 11, 2022 • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johncantrell97 Apr 12, 2022 • edited Loading

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 14, 2022 • 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

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

Choose a reason for hiding this comment

TheBlueMatt commented Apr 15, 2022

johncantrell97 commented Apr 16, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johncantrell97 commented Apr 19, 2022

add `KVStorePersister` trait and blanket implementations of `Persist` and `Persister` for any type that implements `KVStorePersister` #1417

add `KVStorePersister` trait and blanket implementations of `Persist` and `Persister` for any type that implements `KVStorePersister` #1417

johncantrell97 commented Apr 11, 2022 •

edited

Loading

johncantrell97 Apr 12, 2022 •

edited

Loading

codecov-commenter commented Apr 14, 2022 •

edited

Loading