Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log when a channel is closed on startup due to stale ChannelManager #1029

Merged

Conversation

TheBlueMatt
Copy link
Collaborator

This is one of the riskiest parts of our API from the perspective
of accidental force-closes - if users delay persisting the
ChannelManager much at all after a ChannelMonitor we may hit a
force-close after restart.

The fact that we don't log at all when this happens is criminal.

@codecov
Copy link

codecov bot commented Aug 1, 2021

Codecov Report

Merging #1029 (5307b5e) into main (bee9a1e) will increase coverage by 1.56%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1029      +/-   ##
==========================================
+ Coverage   90.80%   92.37%   +1.56%     
==========================================
  Files          60       61       +1     
  Lines       31460    40551    +9091     
==========================================
+ Hits        28568    37459    +8891     
- Misses       2892     3092     +200     
Impacted Files Coverage Δ
lightning-background-processor/src/lib.rs 95.58% <ø> (ø)
lightning/src/ln/channelmanager.rs 90.28% <100.00%> (+4.66%) ⬆️
lightning/src/ln/chanmon_update_fail_tests.rs 97.73% <0.00%> (-0.01%) ⬇️
lightning/src/lib.rs 100.00% <0.00%> (ø)
lightning/src/ln/features.rs 99.56% <0.00%> (+0.12%) ⬆️
lightning/src/ln/reorg_tests.rs 99.76% <0.00%> (+0.13%) ⬆️
lightning/src/ln/onion_utils.rs 95.21% <0.00%> (+0.29%) ⬆️
lightning-block-sync/src/http.rs 94.04% <0.00%> (+0.33%) ⬆️
lightning/src/chain/onchaintx.rs 94.66% <0.00%> (+0.47%) ⬆️
lightning/src/ln/msgs.rs 89.24% <0.00%> (+0.55%) ⬆️
... and 16 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 bee9a1e...5307b5e. Read the comment docs.

Copy link

@ariard ariard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 05de81b

This is one of the riskiest parts of our API from the perspective
of accidental force-closes - if users delay persisting the
ChannelManager much at all after a ChannelMonitor we may hit a
force-close after restart.

The fact that we don't log at all when this happens is criminal.
It is easy for users to have a bug where they drop a
`BackgroundProcessor` immediately, causing it to start and then
immediately stop. Instead, add a `#[must_use]` tag to provide a
compiler warning for such instances.
@TheBlueMatt TheBlueMatt force-pushed the 2021-07-log-channel-close branch from 05de81b to 5307b5e Compare August 5, 2021 20:24
@TheBlueMatt
Copy link
Collaborator Author

Pushed a trivial wording clarification as suggested by @ariard. Compared to ACKs full diff is below, will merge after CI:

$ git diff-tree -U1 05de81bf 5307b5e8
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index bf19dc10..c5db4075 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -4962,3 +4962,4 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 					// But if the channel is behind of the monitor, close the channel:
-					log_error!(args.logger, "A ChannelManager is stale compared to the current ChannelMonitor! The channel will be closed.");
+					log_error!(args.logger, "A ChannelManager is stale compared to the current ChannelMonitor!");
+					log_error!(args.logger, " The channel will be force-closed and the latest commitment transaction from the ChannelMonitor broadcast.");
 					log_error!(args.logger, " The ChannelMonitor for channel {} is at update_id {} but the ChannelManager is at update_id {}.",
$

@TheBlueMatt TheBlueMatt merged commit 8530078 into lightningdevkit:main Aug 5, 2021
@valentinewallace valentinewallace mentioned this pull request Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants