Do not always persist ChannelManager on channel_update messages #972
Conversation
Codecov Report
@@ Coverage Diff @@
## main #972 +/- ##
==========================================
- Coverage 90.63% 90.52% -0.11%
==========================================
Files 60 60
Lines 30538 30654 +116
==========================================
+ Hits 27678 27750 +72
- Misses 2860 2904 +44
Continue to review full report at Codecov.
|
TheBlueMatt
force-pushed
the
2021-06-skip-notify-chansync
branch
from
f7acf9e to
e5dd416
Compare
| let node_b_chan_info = nodes[1].node.list_channels()[0].clone(); | ||
|
|
||
| // The first two nodes (which opened a channel) should now require fresh persistence | ||
| assert!(nodes[0].node.await_persistable_update_timeout(Duration::from_millis(1))); |
There was a problem hiding this comment.
Not sure if by design, but don't think this test covers the DoPersist case: https://i.imgur.com/7fAhVOh.png
There was a problem hiding this comment.
I'm not sure what I'm looking at there, but if not that's a bug - await_persistable_update_timeout is supposed to return true only if we have a reason to persist.
There was a problem hiding this comment.
Just meant that the test doesn't cover the case where we do want to persist after handling a channel update
There was a problem hiding this comment.
Right, yes, sorry. As I was writing the test I found another bug, so the test is finished in the last commit at #949.
If we receive a `channel_update` message for a channel unrelated to our own, we shouldn't trigger a persistence of our `ChannelManager`. This avoids significant persistence traffic during initial node startup.
TheBlueMatt
force-pushed
the
2021-06-skip-notify-chansync
branch
from
b7d249a to
803da87
Compare
|
Squashed without diff: |
| @@ -288,7 +288,7 @@ impl HTLCCandidate { | |||
| } | |||
|
|
|||
| /// Information needed for constructing an invoice route hint for this channel. | |||
| #[derive(Clone)] | |||
| #[derive(Clone, Debug, PartialEq)] | |||
There was a problem hiding this comment.
Instead of implementing PartialEq, test that fee_base_msat hasn't changed.
There was a problem hiding this comment.
I think its generally a good thing to have either way - its a public struct and its reasonable for users to want to compare it? If you disagree we can change it in a followup.
Based on #965 as it fixes a bug which would make this PR useless.
If we receive a
channel_updatemessage for a channel unrelated toour own, we shouldn't trigger a persistence of our
ChannelManager. This avoids significant persistence traffic duringinitial node startup.