Further decouple ChannelManager from Channel state somewhat #3539
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jkczyz
reviewed
Jan 15, 2025
| /// Should be called when the peer is disconnected. Returns true if the channel can be resumed | ||
| /// when the peer reconnects (via [`Self::peer_connected_get_handshake`]). If not, the channel | ||
| /// must be immediately closed. | ||
| pub fn peer_disconnected_is_resumable<L: Deref>(&mut self, logger: &L) -> bool where L::Target: Logger { |
There was a problem hiding this comment.
How about naming this is_disconnected_peer_resumable?
There was a problem hiding this comment.
Its not stateless, though, I wanted to communicate that its a "this peer has disconnected" notification, plus "should i discard the channel". Not sure how best to communicate it, this name is a bit awkward.
|
LGTM once the logging context is restored |
TheBlueMatt
force-pushed
the
2025-01-3513-followups
branch
from
e25da14 to
2abe846
Compare
|
Sorry for the delay, fixed the logger issue: $ git diff-tree -U2 e25da1455 2abe8469b
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index fcc11ca05..16599f126 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -9455,6 +9455,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
// Returns whether we should remove this channel as it's just been closed.
let unblock_chan = |phase: &mut Channel<SP>, pending_msg_events: &mut Vec<MessageSendEvent>| -> Option<ShutdownResult> {
+ let logger = WithChannelContext::from(&self.logger, &phase.context(), None);
let node_id = phase.context().get_counterparty_node_id();
- if let Some(msgs) = phase.signer_maybe_unblocked(self.chain_hash, &self.logger) {
+ if let Some(msgs) = phase.signer_maybe_unblocked(self.chain_hash, &&logger) {
if let Some(msg) = msgs.open_channel {
pending_msg_events.push(events::MessageSendEvent::SendOpenChannel {
@@ -9513,7 +9514,4 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
}
if let Some(broadcast_tx) = msgs.signed_closing_tx {
- let channel_id = chan.context.channel_id();
- let counterparty_node_id = chan.context.get_counterparty_node_id();
- let logger = WithContext::from(&self.logger, Some(counterparty_node_id), Some(channel_id), None);
log_info!(logger, "Broadcasting closing tx {}", log_tx!(broadcast_tx));
self.tx_broadcaster.broadcast_transactions(&[&broadcast_tx]); |
|
FYI, this will conflict with #3550. May be easier to get that in first, but either way is fine by me. |
After lightningdevkit#3513 we have a bit more encapsulation of channel logic in channel.rs with channelmanager.rs needing a bit less knowledge of which specific state a channel is in. This continues that trend slightly when unblocking the signer.
|
Rebased. |
TheBlueMatt
force-pushed
the
2025-01-3513-followups
branch
from
2abe846 to
6852e8a
Compare
wpaulino
approved these changes
Jan 27, 2025
jkczyz
reviewed
Jan 27, 2025
After lightningdevkit#3513 we have a bit more encapsulation of channel logic in channel.rs with channelmanager.rs needing a bit less knowledge of which specific state a channel is in. This continues that trend slightly when a peer disconnects.
After lightningdevkit#3513 we have a bit more encapsulation of channel logic in channel.rs with channelmanager.rs needing a bit less knowledge of which specific state a channel is in. This continues that trend slightly when a peer reconnects.
TheBlueMatt
force-pushed
the
2025-01-3513-followups
branch
from
6852e8a to
bece44c
Compare
|
Ugh, fixed the dangling link after the third commit. |
jkczyz
approved these changes
Jan 28, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Just a few tiny followups to #3513 to keep moving towards ChannelManager caring less about the specific state the channel is in.