Avoid persisting a ChannelManager after each timer tick and send update_channel re-enable messages by TheBlueMatt · Pull Request #916 · lightningdevkit/rust-lightning

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

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

+ 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.

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.

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

+	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.

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


-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 · 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

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

+ 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.

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.

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

+	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.

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


-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 · 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

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

+ 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.

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.

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

+	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.

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


-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 · 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

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

+ 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.

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.

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

+	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.

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


-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 · 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

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

+ 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.

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.

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

+	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.

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


-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 · 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

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

+ 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.

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.

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

+	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.

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


-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 · 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

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

+ 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.

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.

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

+	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.

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


-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 · 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

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

+ 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.

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.

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

+	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.

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


-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 cad69f4Compare May 8, 2021 14:55

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

valentinewallace approved these changes May 10, 2021
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/functional_tests.rs Outdated
Comment threadlightning/src/ln/functional_tests.rs Outdated

TheBlueMatt force-pushed the 2021-05-fix-disabled-announcements branch from 9b69e1e to ba7a0c0Compare 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 4edd86dCompare May 14, 2021 01:42

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

TheBlueMatt force-pushed the 2021-05-fix-disabled-announcements branch from 0a448b3 to 0f24e79Compare 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 533eef8Compare 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 39bd883Compare 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 ee36d64Compare May 14, 2021 23:20

TheBlueMatt merged commit e0986de into lightningdevkit:mainMay 15, 2021

jkczyz mentioned this pull request May 15, 2021
Background processing of ChannelManager and ChannelMonitor events #920
Merged