ChannelManager persistence #752

valentinewallace · 2020-11-17T02:24:49Z

Closes #743.

Current todos:

abstract out utilities for writing to disk/forming platform-agnostic filepaths, etc
rename FilesystemPersister to MonitorPersister? Or somehow align the names of the two persisters
make the manager persister test Windows-friendly
add more unit tests, currently there's just one integration test
make sure the ChannelManager signals for a new update whenever it needs to, and doesn't signal when it doesn't need to

codecov · 2020-11-17T02:37:10Z

Codecov Report

Merging #752 (a368093) into main (13e990e) will increase coverage by 0.00%.
The diff coverage is 95.69%.

@@           Coverage Diff           @@
##             main     #752   +/-   ##
=======================================
  Coverage   90.79%   90.80%           
=======================================
  Files          44       45    +1     
  Lines       24466    24547   +81     
=======================================
+ Hits        22215    22290   +75     
- Misses       2251     2257    +6

Impacted Files	Coverage Δ
lightning-persister/src/lib.rs	`92.23% <83.33%> (-4.07%)`	⬇️
lightning-persister/src/util.rs	`95.83% <95.83%> (ø)`
lightning/src/ln/channelmanager.rs	`85.24% <97.02%> (+0.33%)`	⬆️
lightning/src/util/macro_logger.rs	`88.33% <100.00%> (ø)`
lightning/src/ln/functional_tests.rs	`97.01% <0.00%> (+0.03%)`	⬆️

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 13e990e...a368093. Read the comment docs.

lightning/src/ln/channelmanager.rs

lightning-persister/src/manager.rs

jkczyz · 2020-11-20T23:08:40Z

abstract out utilities for writing to disk/forming platform-agnostic filepaths, etc

rename FilesystemPersister to MonitorPersister? Or somehow align the names of the two persisters

I thought a bit more about this after our offline discussion and reviewing the code. My idea would be to expand FilesystemPersister to cover this functionality but do so by implementing a separate channelmanager::Persist trait. This trait would consist of a provided method and a required method. The provided method would implement any logic common to any persister pertaining to interaction with ChannelManager (e.g., starting a thread and interacting with the condition variable). And then users would implement the required method for the actual persistence (e.g., FilesystemPersister would implement this to persist to disk). The provide method would call the required method as needed.

Thoughts?

TheBlueMatt · 2020-11-21T16:31:02Z

The provided method would implement any logic common to any persister pertaining to interaction with ChannelManager (e.g., starting a thread and interacting with the condition variable).

Hmm, what's the value of trait-ing it, then? I admit its definitely nice to have it be a similar API as the Channel/ChainMonitor stuff, but it just seems like additional re-entrancy to user code for what could be a utility function. Maybe if we were moving towards ChannelManager calling "write me to disk now" on some trait the way we do monitor stuff it'd be more symmetric, but I'm not sure we'll ever want to do that so explicitly given there's a lot of reason to do ChannelManager disk writing in the background.

jkczyz · 2020-11-22T05:58:43Z

The provided method would implement any logic common to any persister pertaining to interaction with ChannelManager (e.g., starting a thread and interacting with the condition variable).

Hmm, what's the value of trait-ing it, then? I admit its definitely nice to have it be a similar API as the Channel/ChainMonitor stuff, but it just seems like additional re-entrancy to user code for what could be a utility function.

The trait is a means of providing a template method. I'm presuming we (a) don't want users to write the provided logic on their own and (b) want to provide a means for users to customize how the data is persisted.

A utility function that is parameterized generically by something implementing this trait (i.e., without the provided method) would also be a reasonable approach that I'd be happy with. Is there a different way of defining a utility function that you were thinking of? Seems you would still need a trait unless you pass a closure.

Regarding reentrancy, I don't see either of these approaches as being reentrant. Though I suppose it depends on how you are defining reentrancy.

Maybe if we were moving towards ChannelManager calling "write me to disk now" on some trait the way we do monitor stuff it'd be more symmetric, but I'm not sure we'll ever want to do that so explicitly given there's a lot of reason to do ChannelManager disk writing in the background.

That is reentrancy in how I would have thought: ChannelManager calling back to user code. A user directly calling either a utility function or a provided trait method, which in turn call a required trait method is not, IMO. The same as calling a provided method of an Iterator which in turn calls a user-defined next method would not.

Am I missing something about how reentrancy is involved the current use case? Perhaps because a thread is involved?

valentinewallace · 2020-11-23T20:17:10Z

A utility function that is parameterized generically by something implementing this trait (i.e., without the provided method) would also be a reasonable approach that I'd be happy with.

I like this approach. IIUC: so there'd be a channelmanager::Persist trait, with only one method (write_manager or so). Then we'd keep the current ManagerPersister struct (probably renamed) with its start() method, only the start() method would take in an implementer of channelmanager::Persist, and call into that in its background thread when it gets a signal to persist. Is this accurate to what you're suggesting @jkczyz?

Since the current ManagerPersister struct is gonna have other responsibilities besides persisting the manager (i.e. calling timer_chan_freshness_every_min() and possibly persisting router data, it not have the best name right now. Maybe BackgroundProcesser or something like that?

TheBlueMatt · 2020-11-23T20:38:45Z

Am I missing something about how reentrancy is involved the current use case? Perhaps because a thread is involved?

I think I didn't really understand your proposal, then. I understood it as "move the thread-spawn logic and all of the "start thinking about keeping the chanman on disk" logic into a trait with a template method but which users could override. That, afaiu, would basically imply just ChannelManager calling that method immediately on startup and...that's it? That would seem needlessly reentrant.

jkczyz · 2020-11-23T22:24:06Z

A utility function that is parameterized generically by something implementing this trait (i.e., without the provided method) would also be a reasonable approach that I'd be happy with.

I like this approach. IIUC: so there'd be a channelmanager::Persist trait, with only one method (write_manager or so). Then we'd keep the current ManagerPersister struct (probably renamed) with its start() method, only the start() method would take in an implementer of channelmanager::Persist, and call into that in its background thread when it gets a signal to persist. Is this accurate to what you're suggesting @jkczyz?

Almost.

Correct as far as the trait definition. However, the aforementioned utility function would be essentially what start is except it would be parameterized with P: channelmanager::Persist (or something that dereferences to it). It would accept a persister: P instead of filename and filepath and use it here in place of calling utils::write_to_file.

Then FilesystemPersister would implement channelmanager::Persist to write to a file. Users could also implement the trait to persist elsewhere (e.g., a database), if they prefer.

Since the current ManagerPersister struct is gonna have other responsibilities besides persisting the manager (i.e. calling timer_chan_freshness_every_min() and possibly persisting router data, it not have the best name right now. Maybe BackgroundProcesser or something like that?

Yeah, I'd imagine this would be named differently. Note that the utility function mentioned above could instead be a BackgroundProcesser struct or such (presumably because it needs to hold the thread_terminator state). Regardless of how it is implemented, the takeaway is start would be parameterized by channelmanager::Persist.

jkczyz · 2020-11-23T22:25:49Z

Am I missing something about how reentrancy is involved the current use case? Perhaps because a thread is involved?

I think I didn't really understand your proposal, then. I understood it as "move the thread-spawn logic and all of the "start thinking about keeping the chanman on disk" logic into a trait with a template method but which users could override. That, afaiu, would basically imply just ChannelManager calling that method immediately on startup and...that's it? That would seem needlessly reentrant.

Let me know if my explanation in #752 (comment) makes it any clearer.

jkczyz · 2020-11-23T22:30:31Z

A utility function that is parameterized generically by something implementing this trait (i.e., without the provided method) would also be a reasonable approach that I'd be happy with.

I like this approach. IIUC: so there'd be a channelmanager::Persist trait, with only one method (write_manager or so). Then we'd keep the current ManagerPersister struct (probably renamed) with its start() method, only the start() method would take in an implementer of channelmanager::Persist, and call into that in its background thread when it gets a signal to persist. Is this accurate to what you're suggesting @jkczyz?

Almost.

Correct as far as the trait definition. However, the aforementioned utility function would be essentially what start is except it would be parameterized with P: channelmanager::Persist (or something that dereferences to it). It would accept a persister: P instead of filename and filepath and use it here in place of calling utils::write_to_file.

Actually, I think I just repeated what you said. :) So what you said was accurate. I misread the part about start taking an implementer of channelmanager::Persist as implementing channelmanager::Persist.

TheBlueMatt · 2020-11-23T22:49:13Z

Let me know if my explanation in #752 (comment) makes it any clearer.

Yep! Thanks, that's much clearer. Such a design sounds good to me.

valentinewallace · 2020-11-25T16:39:45Z

Making some changes, I'll update this comment when this is good for review again.

Edit: should be good for another review

lightning/src/ln/channelmanager.rs

lightning-persister/src/background_processor.rs

TheBlueMatt

Looks pretty good. Mostly nitpicking.

lightning-persister/src/utils.rs

lightning/src/ln/channelmanager.rs

lightning-persister/src/background_processor.rs

lightning/src/ln/channelmanager.rs

background-processor/src/lib.rs

TheBlueMatt · 2021-02-11T19:48:48Z

background-processor/src/lib.rs

+	      K: 'static + KeysInterface<ChanKeySigner=ChanSigner>,
+	      F: 'static + FeeEstimator,
+	      L: 'static + Logger,
+	      PM: 'static + Send + Fn(Arc<ChannelManager<ChanSigner, Arc<M>, Arc<T>, Arc<K>, Arc<F>, Arc<L>>>) -> Result<(), std::io::Error>,


Is there any reason for this function to take an Arc by ownership instead of a &ChannelManager?
nit: ideally we wouldn't require a ChannelManager parameterized by Arcs for every type, though I'm not sure how you'd meed the required 'static liftime without it so its not really a big deal.

It's giving me this super helpful error atm: https://i.imgur.com/sD3T8A0.png

Heh, yea, I'd seen that too. i believe its because the callback referenced is expecting the Arc still (cause what its calling is expecting the Arc).

Hum even when I make FilesystemPersister::persist_manager expect a reference, still this error. I think what I need to do is specify the type of node in callback, so now trying to figure out how to specify type parameters in the callback closure...

Edit: figured it out

lightning-persister/src/lib.rs

TheBlueMatt

One more comment, then looks good.

TheBlueMatt · 2021-02-11T20:03:54Z

lightning/src/ln/channelmanager.rs

+	}
+
+	/// Blocks until ChannelManager needs to be persisted.
+	pub fn wait(&self) {


May be worth noting here and above that only one waiter is woken at a time for each event (because of the re-setting the guard bool to false in wait in the PersistenceNotifier).

Hm I tested with multiple listeners just now and all were notified, but maybe it's unreliable and it's just not showing up in testing? I'll add a comment...

Right, on a multi-core host you may get lucky and the threads both step basically at the same rate, but it's not guaranteed.

Wait, no, huh? It swaps the wakeup bool back to false before returning. Can you share your test?

I added this to test_background_processor:

diff --git a/background-processor/src/lib.rs b/background-processor/src/lib.rs index 5d975eb9..7bc76343 100644 --- a/background-processor/src/lib.rs +++ b/background-processor/src/lib.rs @@ -203,8 +203,11 @@ mod tests { // Initiate the background processors to watch each node. let data_dir = nodes[0].persister.get_data_dir(); + let data_dir1 = nodes[1].persister.get_data_dir(); let callback = move |node| FilesystemPersister::persist_manager(data_dir.clone(), node); + let callback1 = move |node| FilesystemPersister::persist_manager(data_dir1.clone(), node); BackgroundProcessor::start(callback, nodes[0].node.clone(), nodes[0].logger.clone()); + BackgroundProcessor::start(callback1, nodes[0].node.clone(), nodes[0].logger.clone()); // Go through the channel creation process until each node should have something persisted. let tx = open_channel!(nodes[0], nodes[1], 100000); @@ -239,6 +242,10 @@ mod tests { if !nodes[0].node.get_persistence_condvar_value() { break } } + let filepath1 = get_full_filepath("test_background_processor_persister_1".to_string(), "manager".to_string()); + let mut expected_bytes = Vec::new(); + check_persisted_data!(nodes[0].node, filepath1.clone(), expected_bytes); + // Force-close the channel. nodes[0].node.force_close_channel(&OutPoint { txid: tx.txid(), index: 0 }.to_channel_id()).unwrap();

I believe an open_channel!() call will result in a number of calls to notify, and there's a good chance some of them don't necessarily imply a change in the serialized version of the channelmanager (or, maybe more likely, the delay between a notify wakeup and the write getting the write lock may be enough that the next update runs first).

TheBlueMatt · 2021-02-11T20:06:07Z

lightning/src/util/macro_logger.rs

 macro_rules! log_internal {
 	($logger: expr, $lvl:expr, $($arg:tt)+) => (
-		$logger.log(&::util::logger::Record::new($lvl, format_args!($($arg)+), module_path!(), file!(), line!()));
+		$logger.log(&$crate::util::logger::Record::new($lvl, format_args!($($arg)+), module_path!(), file!(), line!()));


Wait, how does this work? is crate resolved with reference to the place the macro is defined? I guess if it works, it works, but that surprises me.

I don't understand it 100% but I think it's the special magic explained here: http://web.mit.edu/rust-lang_v1.25/arch/amd64_ubuntu1404/share/doc/rust/html/book/first-edition/macros.html#the-variable-crate

jkczyz

No other major concerns. Just a couple nits.

background-processor/src/lib.rs

TheBlueMatt · 2021-02-12T21:44:20Z

Hmm, looks like a test hung on windows:

test tests::test_background_processor ... test tests::test_background_processor has been running for over 60 seconds

valentinewallace · 2021-02-12T22:44:29Z

Hmm, looks like a test hung on windows:

test tests::test_background_processor ... test tests::test_background_processor has been running for over 60 seconds

I'm somewhat baffled, never seen this problem before. Best I can guess from reading up about Rust threads a bit is that we were leaving a few threads in detached states in tests, and this can cause weirdness, especially on low-resourced machines such as those that GH is presumably using for CI? Let me know if this seems like a completely implausible explanation.

jkczyz · 2021-02-13T16:21:58Z

Hmm, looks like a test hung on windows:
test tests::test_background_processor ... test tests::test_background_processor has been running for over 60 seconds

I'm somewhat baffled, never seen this problem before. Best I can guess from reading up about Rust threads a bit is that we were leaving a few threads in detached states in tests, and this can cause weirdness, especially on low-resourced machines such as those that GH is presumably using for CI? Let me know if this seems like a completely implausible explanation.

Not sure if this related to threads. I don't think spawned threads need to terminate for a test to complete. More likely related to any busy looping on something that never occurs. Perhaps the FilesystemPersister failed?

Rather than having this test actually persist data and busy wait, could we instead use a channel to coordinate whether the callback method was called? The sender end would be in the callback and the test would use the receiver end to be notified of the callback.

See as an example: https://github.com/rust-lang/rust/blob/32cbc65e6bf793d99dc609d11f4a4c93176cdbe2/library/std/src/sync/barrier/tests.rs

That way we remove all dependencies on FilesystemPersister from this test, which is tested independently.

valentinewallace · 2021-02-18T21:03:38Z

Not sure if this related to threads. I don't think spawned threads need to terminate for a test to complete. More likely related to any busy looping on something that never occurs. Perhaps the FilesystemPersister failed?

Rather than having this test actually persist data and busy wait, could we instead use a channel to coordinate whether the callback method was called? The sender end would be in the callback and the test would use the receiver end to be notified of the callback.

See as an example: https://github.com/rust-lang/rust/blob/32cbc65e6bf793d99dc609d11f4a4c93176cdbe2/library/std/src/sync/barrier/tests.rs

That way we remove all dependencies on FilesystemPersister from this test, which is tested independently.

It did end up being an issue with the FilesystemPersister. I still think it's worth having an integration test, since we wouldn't have found the problem (which is fixed in commit Fix Windows ... -- thanks @TheBlueMatt for the debugging assist!) without it.

TheBlueMatt · 2021-02-18T22:18:06Z

lightning-persister/src/util.rs

+		let src = PathBuf::from(tmp_filename.clone());
+		let dst = PathBuf::from(filename_with_path.clone());
+		if Path::new(&filename_with_path.clone()).exists() {
+			unsafe {winapi::um::winbase::ReplaceFileW(


Does ReplaceFileW work even if the destination file doesn't exist? If so, maybe its worth asking if rust upstream should be using that instead of MoveFileExW?

Does ReplaceFileW work even if the destination file doesn't exist?

I don't think so, or at least I get background_processor tests that never terminate: https://github.com/valentinewallace/rust-lightning/runs/1931376827

TheBlueMatt

LGTM. Stupid Windowz.

lightning-persister/src/util.rs

These will be used in upcoming commits for the BackgroundProcessor to log.

Windows started giving 'Access is denied' errors after a few rounds of persistence. This seems to fix it.

This will allow the ChannelManager to signal when it has new updates to persist, and adds a way for ChannelManager persisters to be notified when they should re-persist the ChannelManager to disk/backups. Feature-gate the wait_timeout function because the core lightning crate shouldn't depend on wallclock time unless users opt into it.

Other includes calling timer_chan_freshness_every_minute() and in the future, possibly persisting channel graph data. This struct is suitable for things that need to happen periodically and can happen in the background.

valentinewallace · 2021-02-19T20:01:24Z

Rebased!

jkczyz · 2021-02-19T20:25:00Z

It did end up being an issue with the FilesystemPersister. I still think it's worth having an integration test, since we wouldn't have found the problem (which is fixed in commit Fix Windows ... -- thanks @TheBlueMatt for the debugging assist!) without it.

Ah, I should have asked if the failure was deterministic. :)

My general feeling is if it can be reproduced by a unit test, then that would be preferable to using an integration test for testing two behaviors that could be unit tested separately in different modules. Not sure if that is the case here, but I'm fine with leaving it for a follow-up if so.

jkczyz · 2021-02-19T20:39:10Z

ACK a368093

valentinewallace force-pushed the chanman-persistence branch from 77a29b3 to 1d49404 Compare November 17, 2020 02:25

TheBlueMatt reviewed Nov 17, 2020

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

lightning-persister/src/manager.rs Outdated Show resolved Hide resolved

lightning-persister/src/manager.rs Outdated Show resolved Hide resolved

lightning-persister/src/manager.rs Outdated Show resolved Hide resolved

jkczyz self-requested a review November 17, 2020 23:27

valentinewallace force-pushed the chanman-persistence branch 3 times, most recently from 3352f40 to cb20f3b Compare November 19, 2020 18:23

valentinewallace marked this pull request as ready for review November 19, 2020 21:20

TheBlueMatt added this to the 0.0.13 milestone Nov 21, 2020

valentinewallace force-pushed the chanman-persistence branch 3 times, most recently from 20fd381 to 28237a9 Compare November 24, 2020 23:44

valentinewallace force-pushed the chanman-persistence branch 2 times, most recently from d10cabf to 4ad0f94 Compare November 25, 2020 19:49

devrandom reviewed Dec 1, 2020

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

devrandom reviewed Dec 1, 2020

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

devrandom reviewed Dec 1, 2020

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

devrandom reviewed Dec 1, 2020

View reviewed changes

lightning-persister/src/background_processor.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Dec 1, 2020

View reviewed changes

jkczyz reviewed Dec 2, 2020

View reviewed changes

lightning-persister/src/background_processor.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Feb 9, 2021

View reviewed changes

background-processor/src/lib.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Feb 11, 2021

View reviewed changes

valentinewallace force-pushed the chanman-persistence branch from d9459e0 to 3c95a48 Compare February 11, 2021 21:28

jkczyz reviewed Feb 11, 2021

View reviewed changes

background-processor/src/lib.rs Outdated Show resolved Hide resolved

background-processor/src/lib.rs Outdated Show resolved Hide resolved

valentinewallace force-pushed the chanman-persistence branch from 3c95a48 to 4ddc992 Compare February 12, 2021 21:15

TheBlueMatt approved these changes Feb 12, 2021

View reviewed changes

jkczyz approved these changes Feb 12, 2021

View reviewed changes

valentinewallace force-pushed the chanman-persistence branch from 4ddc992 to 25e40d5 Compare February 12, 2021 22:34

TheBlueMatt mentioned this pull request Feb 15, 2021

SPV client utility for syncing a lightning node #791

Merged

valentinewallace force-pushed the chanman-persistence branch from 25e40d5 to f9832b2 Compare February 18, 2021 21:01

TheBlueMatt reviewed Feb 18, 2021

View reviewed changes

TheBlueMatt approved these changes Feb 19, 2021

View reviewed changes

lightning-persister/src/util.rs Outdated Show resolved Hide resolved

valentinewallace force-pushed the chanman-persistence branch 3 times, most recently from 94ee55b to 5c6b319 Compare February 19, 2021 18:17

valentinewallace added 5 commits February 19, 2021 15:00

Make logger macros public

f41cfb4

These will be used in upcoming commits for the BackgroundProcessor to log.

Abstract out disk-writing utilities from FilesystemPersister

36eaccc

Fix Windows persistence

a88dfbf

Windows started giving 'Access is denied' errors after a few rounds of persistence. This seems to fix it.

Add BackgroundProcessor for ChannelManager persistence and other

a368093

Other includes calling timer_chan_freshness_every_minute() and in the future, possibly persisting channel graph data. This struct is suitable for things that need to happen periodically and can happen in the background.

valentinewallace force-pushed the chanman-persistence branch from 5c6b319 to a368093 Compare February 19, 2021 20:00

TheBlueMatt merged commit 041d7aa into lightningdevkit:main Feb 19, 2021

ChannelManager persistence #752

ChannelManager persistence #752

Conversation

valentinewallace commented Nov 17, 2020 • edited Loading

codecov bot commented Nov 17, 2020 • edited Loading

Codecov Report

jkczyz commented Nov 20, 2020

TheBlueMatt commented Nov 21, 2020

jkczyz commented Nov 22, 2020

valentinewallace commented Nov 23, 2020

TheBlueMatt commented Nov 23, 2020

jkczyz commented Nov 23, 2020

jkczyz commented Nov 23, 2020

jkczyz commented Nov 23, 2020 • edited Loading

TheBlueMatt commented Nov 23, 2020

valentinewallace commented Nov 25, 2020 • 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

valentinewallace Feb 12, 2021 • 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

valentinewallace Feb 11, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Feb 12, 2021

valentinewallace commented Feb 12, 2021

jkczyz commented Feb 13, 2021

valentinewallace commented Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

valentinewallace Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

valentinewallace commented Feb 19, 2021

jkczyz commented Feb 19, 2021

jkczyz commented Feb 19, 2021

valentinewallace commented Nov 17, 2020 •

edited

Loading

codecov bot commented Nov 17, 2020 •

edited

Loading

jkczyz commented Nov 23, 2020 •

edited

Loading

valentinewallace commented Nov 25, 2020 •

edited

Loading

valentinewallace Feb 12, 2021 •

edited

Loading

valentinewallace Feb 11, 2021 •

edited

Loading

valentinewallace commented Feb 18, 2021 •

edited

Loading

valentinewallace Feb 18, 2021 •

edited

Loading