Avoid persisting a ChannelManager after each timer tick and send update_channel re-enable messages #916

TheBlueMatt · 2021-05-07T22:29:33Z

Only somewhat-related commits here, but they touch the same code.

codecov · 2021-05-08T15:16:29Z

Codecov Report

Merging #916 (ee36d64) into main (0ac3b44) will increase coverage by 0.08%.
The diff coverage is 92.17%.

@@            Coverage Diff             @@
##             main     #916      +/-   ##
==========================================
+ Coverage   90.46%   90.55%   +0.08%     
==========================================
  Files          59       59              
  Lines       29788    29911     +123     
==========================================
+ Hits        26948    27086     +138     
+ Misses       2840     2825      -15

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	`87.25% <71.42%> (-0.18%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.00% <87.50%> (+0.19%)`	⬆️
lightning/src/ln/channelmanager.rs	`83.49% <98.71%> (+0.16%)`	⬆️
lightning-block-sync/src/rest.rs	`65.45% <0.00%> (-1.79%)`	⬇️
lightning-block-sync/src/rpc.rs	`78.37% <0.00%> (-1.11%)`	⬇️
lightning-block-sync/src/poll.rs	`91.66% <0.00%> (-0.38%)`	⬇️
lightning-block-sync/src/lib.rs	`95.18% <0.00%> (-0.20%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.25% <0.00%> (-0.17%)`	⬇️
lightning-block-sync/src/init.rs	`93.56% <0.00%> (-0.15%)`	⬇️
... and 3 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 0ac3b44...ee36d64. Read the comment docs.

valentinewallace

Cool! I like how it sets a foundation for removing other unnecessary persists in the future too

lightning/src/ln/channelmanager.rs

lightning/src/ln/functional_tests.rs

lightning/src/ln/channel.rs

jkczyz · 2021-05-13T06:47:29Z

lightning/src/ln/functional_tests.rs

+				let short_id = msg.contents.short_channel_id;
+				// Check generated channel_update match list in PendingChannelUpdate
+				if short_id != short_id_1 && short_id != short_id_2 && short_id != short_id_3 {
+					panic!("Generated ChannelUpdate for wrong chan!");
+				}


Not sure I understand the value of this check. There aren't any other channels to update and it doesn't guarantee all channels are covered. Would the test suffice using one channel instead?

Hmm, possibly? From a black-box point-of-view, its somewhat nice to ensure that it works in any direction - inbound or outbound and across multiple channels, though given the implementation I agree it'd be hard to screw that up. I went ahead and simplified this check, though.

lightning/src/ln/channelmanager.rs

Currently, we only send an update_channel message after disconnecting a peer and waiting some time. We do not send a followup when the peer has been reconnected for some time. This changes that behavior to make the disconnect and reconnect channel updates symmetric, and also simplifies the state machine somewhat to make it more clear. Finally, it serializes the current announcement state so that we usually know when we need to send a new update_channel.

TheBlueMatt · 2021-05-14T01:43:51Z

Pushed two different approaches to cleaning up the notifierguard, see the two fixup commits.

jkczyz · 2021-05-14T17:26:05Z

Pushed two different approaches to cleaning up the notifierguard, see the two fixup commits.

I was hoping with the closure-based approach, the guarded code could go into the closure. But I guess that gets messy if you also need to return a value. Seems ok as is then.

For naming, I'd say the methods and enums are about notifying persisters rather than persisting itself, so should be named in such a manner (e.g., notify_on_drop).

TheBlueMatt · 2021-05-14T20:35:40Z

I was hoping with the closure-based approach, the guarded code could go into the closure.

Ah! Indeed we could.

lightning/src/ln/channelmanager.rs

jkczyz · 2021-05-14T21:05:02Z

lightning/src/ln/channelmanager.rs

+	fn notify_on_drop(lock: &'a RwLock<()>, notifier: &'a PersistenceNotifier) -> PersistenceNotifierGuard<'a, impl Fn() -> PersistenceGuardOption> {
+		PersistenceNotifierGuard::optionally_notify(lock, notifier, || -> PersistenceGuardOption { PersistenceGuardOption::DoPersist })
+	}
+
+	fn optionally_notify<F: Fn() -> PersistenceGuardOption>(lock: &'a RwLock<()>, notifier: &'a PersistenceNotifier, persist_check: F) -> PersistenceNotifierGuard<'a, F> {


Would Self for the return type work for these?

No, we could move it into its own impl block and use Self, but notify_on_drop always has to be in a concrete impl block and return something else. I figured it was easier to just put them both in a concrete impl block (with concrete types that we don't care about) and then make the return types explicit.

TheBlueMatt · 2021-05-14T22:38:36Z

Squashed without changes from 533eef8 to 39bd883.

lightning/src/ln/channelmanager.rs

valentinewallace · 2021-05-14T23:11:01Z

lightning/src/ln/channelmanager.rs

 	// We hold onto this result so the lock doesn't get released immediately.
 	_read_guard: RwLockReadGuard<'a, ()>,
 }

-impl<'a> PersistenceNotifierGuard<'a> {
-	fn new(lock: &'a RwLock<()>, notifier: &'a PersistenceNotifier) -> Self {
+impl<'a> PersistenceNotifierGuard<'a, fn() -> NotifyOption> { // We don't care what the concrete F is here, its unused


nit: it's
Also a bit confused by this comment because it is used here, isn't it?

No, the concrete type here is a function pointer (ie fn() -> NotifyOption), but we don't actually ever use a PersistenceNotifierGuard with a function pointer, only closures.

Currently, when a user calls `ChannelManager::timer_tick_occurred` we always set the persister's update flag to true. This results in a ChannelManager persistence after each timer tick, even when nothing happened. Instead, we add a new flag to `PersistenceNotifierGuard` to indicate if we should skip setting the update flag.

When we had a event which caused us to set the persist flag in a PersistenceNotifier in between wait calls, we will still wait, potentially not persisting a ChannelManager when we should. Worse, for wait_timeout, this caused us to always wait up to the timeout, but then always return true that a persistence is needed. Instead, we simply check the persist flag before waiting, returning immediately if it is set.

TheBlueMatt · 2021-05-14T23:21:06Z

Fixed two minor typos, will merge after CI:

$ git diff-tree -U1 39bd883e ee36d647
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 1e99cb72..92291293 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -537,3 +537,3 @@ enum NotifyOption {
 ///
-/// We allow callers to either always notify by constructing with `notify_on_drop` or chose to
+/// We allow callers to either always notify by constructing with `notify_on_drop` or choose to
 /// notify or not based on whether relevant changes have been made, providing a closure to
@@ -547,3 +547,3 @@ struct PersistenceNotifierGuard<'a, F: Fn() -> NotifyOption> {
 
-impl<'a> PersistenceNotifierGuard<'a, fn() -> NotifyOption> { // We don't care what the concrete F is here, its unused
+impl<'a> PersistenceNotifierGuard<'a, fn() -> NotifyOption> { // We don't care what the concrete F is here, it's unused
 	fn notify_on_drop(lock: &'a RwLock<()>, notifier: &'a PersistenceNotifier) -> PersistenceNotifierGuard<'a, impl Fn() -> NotifyOption> {
$

TheBlueMatt force-pushed the 2021-05-fix-disabled-announcements branch from 1dabdc0 to cad69f4 Compare May 8, 2021 14:55

TheBlueMatt force-pushed the 2021-05-fix-disabled-announcements branch 2 times, most recently from 8d7a09f to 9b69e1e Compare May 8, 2021 21:20

valentinewallace approved these changes May 10, 2021

View reviewed changes

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

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

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

TheBlueMatt force-pushed the 2021-05-fix-disabled-announcements branch from 9b69e1e to ba7a0c0 Compare May 11, 2021 15:43

jkczyz reviewed May 13, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-05-fix-disabled-announcements branch 2 times, most recently from e981c57 to 4edd86d Compare May 14, 2021 01:42

TheBlueMatt force-pushed the 2021-05-fix-disabled-announcements branch from 4edd86d to 0a448b3 Compare May 14, 2021 20:35

TheBlueMatt force-pushed the 2021-05-fix-disabled-announcements branch from 0a448b3 to 0f24e79 Compare May 14, 2021 20:54

jkczyz approved these changes May 14, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-05-fix-disabled-announcements branch from 0f24e79 to 533eef8 Compare May 14, 2021 21:25

jkczyz approved these changes May 14, 2021

View reviewed changes

Test new channel_update re-enabled announcements

1dd9a4d

TheBlueMatt force-pushed the 2021-05-fix-disabled-announcements branch from 533eef8 to 39bd883 Compare May 14, 2021 22:37

valentinewallace approved these changes May 14, 2021

View reviewed changes

TheBlueMatt added 4 commits May 14, 2021 23:20

Rename Channel::UpdateStatus to be more descriptive and update docs

33c06c0

Simplify+expand per-channel check in test_announce_disable_channels

ee36d64

TheBlueMatt force-pushed the 2021-05-fix-disabled-announcements branch from 39bd883 to ee36d64 Compare May 14, 2021 23:20

TheBlueMatt merged commit e0986de into lightningdevkit:main May 15, 2021

jkczyz mentioned this pull request May 15, 2021

Background processing of ChannelManager and ChannelMonitor events #920

Merged

Avoid persisting a ChannelManager after each timer tick and send update_channel re-enable messages #916

Avoid persisting a ChannelManager after each timer tick and send update_channel re-enable messages #916

Uh oh!

Conversation

TheBlueMatt commented May 7, 2021

Uh oh!

codecov bot commented May 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkczyz May 13, 2021

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt May 13, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt commented May 14, 2021

Uh oh!

jkczyz commented May 14, 2021

Uh oh!

TheBlueMatt commented May 14, 2021

Uh oh!

Uh oh!

Uh oh!

jkczyz May 14, 2021

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt May 14, 2021

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented May 14, 2021

Uh oh!

Uh oh!

valentinewallace May 14, 2021

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt May 14, 2021

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented May 14, 2021

Uh oh!

Uh oh!

codecov bot commented May 8, 2021 •

edited

Loading