Skip to content

Cleanup logging #965

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.

Sign up for GitHub

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

Merged
merged 14 commits into from
Jun 29, 2021
Merged

Cleanup logging #965

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
d61d698
Don't print file paths in fuzz logger as they can be very long
TheBlueMatt Jun 18, 2021
c5a6135
log_debug information about network graph updates from payments
TheBlueMatt Jun 18, 2021
95f9523
Drop rust-bitcoin crate patches in fuzz now that they're merged
TheBlueMatt Jun 21, 2021
a146ef2
Do not generate error messages when we receive our own gossip
TheBlueMatt Jun 21, 2021
133e28f
Add error logs when a ChannelManager as inconsistent monitor state
TheBlueMatt Jun 21, 2021
3ea4279
Unify message sending to use PeerManager::enqueue_message
TheBlueMatt Jun 22, 2021
d36a875
More consistently log in msg handling, incl full msg logging at trace
TheBlueMatt Jun 22, 2021
7fa6a7d
Allow logging to specify an explicit log level instead of a macro
TheBlueMatt Jun 28, 2021
1f592b0
Do not log_debug when we receive duplicate gossip messages
TheBlueMatt Jun 22, 2021
7eff56b
Update logging in channel and channelmanager to better levels
TheBlueMatt Jun 22, 2021
76ea834
Add additional TRACE-level logging during pathfinding in router
TheBlueMatt Jun 23, 2021
74717d3
Increase the log level of several channelmonitor/onchain logs.
TheBlueMatt Jun 23, 2021
6d446a6
Correct inbound HTLC upgrade logs on revoke_and_ack receipt
TheBlueMatt Jun 27, 2021
6d98aed
Add debug log when we stop tracking confirmed on-chain packages
TheBlueMatt Jun 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3354,8 +3354,13 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
match channel_state.by_id.entry(chan_id) {
hash_map::Entry::Occupied(mut chan) => {
if chan.get().get_counterparty_node_id() != *counterparty_node_id {
// TODO: see issue #153, need a consistent behavior on obnoxious behavior from random node
return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!".to_owned(), chan_id));
if chan.get().should_announce() {
// If the announcement is about a channel of ours which is public, some
// other peer may simply be forwarding all its gossip to us. Don't provide
// a scary-looking error message and return Ok instead.
return Ok(());
}
return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a channel_update for a channel from the wrong node - it shouldn't know about our private channels!".to_owned(), chan_id));
Copy link

Choose a reason for hiding this comment

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

Though #949 blurs this assertion, a direct peer might learn a channel_update for outbound-from-us payments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#949 should only be sending the message directly to the channel counterparty, so I think its still correct. Its true we do tell other nodes about our private channels in invoices, but they still shouldn't remember that, nor should they ever have channel_updates for those messages. Do you have a suggestion for a different concrete wording?

Copy link

@ariard ariard Jun 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think your comment is right if this peer is applying the same gossip policy than us ? Some weird LN client could not sort discovered gossips according to their sources and treat them uniformly, I don't think that's something the specification is covering (and IMHO it should be strongly opinionated on this).

So as a suggestion maybe prefix "Gossip policy: Got a channel_update for a channel from the wrong node - we consider it shouldn't know about this private channels!" ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The specification is clear that you should not accept or forward a channel_update unless you've received the corresponding channel_announcement, which we will never generate signatures for, so it cannot exist.

}
try_chan_entry!(self, chan.get_mut().channel_update(&msg), channel_state, chan);
},
Expand Down