Skip to content

Remove explicit print in lightning-net-tokio, reduce redundant block connection logging #1048

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

Jump to bottom
Merged
merged 4 commits into from
Aug 18, 2021
Merged

Remove explicit print in lightning-net-tokio, reduce redundant block connection logging #1048

merged 4 commits into from
Aug 18, 2021

Conversation

TheBlueMatt
Copy link
Collaborator

Fixes #980.

@TheBlueMatt TheBlueMatt added this to the 0.0.101 milestone Aug 17, 2021
jkczyz
jkczyz reviewed Aug 17, 2021
View reviewed changes
@TheBlueMatt TheBlueMatt force-pushed the 2021-08-no-net-print branch from cf84655 to 0fa452e Compare August 17, 2021 02:21
@codecov
Copy link

codecov bot commented Aug 17, 2021 •
edited
Loading

Codecov Report

Merging #1048 (35d0b7a) into main (64159b3) will increase coverage by 1.30%.
The diff coverage is 73.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1048      +/-   ##
==========================================
+ Coverage   90.96%   92.26%   +1.30%     
==========================================
  Files          64       65       +1     
  Lines       32447    38813    +6366     
==========================================
+ Hits        29515    35812    +6297     
- Misses       2932     3001      +69
Impacted Files Coverage Δ
lightning/src/ln/script.rs 93.75% <ø> (ø)
lightning-net-tokio/src/lib.rs 76.98% <50.00%> (-0.38%) ⬇️
lightning/src/ln/peer_handler.rs 45.75% <75.00%> (-0.23%) ⬇️
lightning/src/chain/chainmonitor.rs 95.91% <100.00%> (+0.11%) ⬆️
lightning/src/chain/channelmonitor.rs 91.97% <100.00%> (-0.02%) ⬇️
lightning/src/util/ser_macros.rs 90.50% <0.00%> (-5.24%) ⬇️
lightning/src/ln/shutdown_tests.rs 95.58% <0.00%> (ø)
lightning/src/ln/chanmon_update_fail_tests.rs 97.90% <0.00%> (+0.03%) ⬆️
lightning/src/ln/functional_tests.rs 98.66% <0.00%> (+1.36%) ⬆️
... and 6 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 64159b3...35d0b7a. Read the comment docs.

jkczyz
jkczyz approved these changes Aug 17, 2021
View reviewed changes
@TheBlueMatt
Copy link
Collaborator Author

Addressed feedback and added one more commit to clean up docs.

valentinewallace
valentinewallace approved these changes Aug 17, 2021
View reviewed changes
lightning/src/chain/chainmonitor.rs Outdated
self.process_chain_data(header, txdata, |monitor, txdata| {
monitor.transactions_confirmed(
header, txdata, height, &*self.broadcaster, &*self.fee_estimator, &*self.logger)
});
}

fn transaction_unconfirmed(&self, txid: &Txid) {
log_debug!(self.logger, "{} reorganized out of chain", txid);
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: s/{}/txid {}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about Transaction {}?

@TheBlueMatt
Ensure we always at least log at TRACE when a peer disconnects
2e02aa7
@TheBlueMatt
Stop printing to stderr in lightning-net-tokio for disconnections
ecf1d65 
It isn't exactly a critical error situation when we disconnect a
socket, so we shouldn't be printing to stderr, entirely bypassing
user logging, when it happens. We do still print to stderr if we
fail to write the first message to the socket, but this should
never happen unless the user has a reasonably-configured system
with at least one packet in bytes available for the socket buffer.
@TheBlueMatt
Log chain calls in ChainMonitor, reducing logs in ChannelMonitor
1e7d479 
For users with many ChannelMonitors, we log a large volume per
block simply because each ChannelMonitor lots several times per
block. Instead, we move to log only once at the TRACE level per
block call in ChannelMonitors, relying instead on a DEBUG level
log in ChainMonitor before we call any ChannelMonitor functions.

For most users, this will reduce redundant logging and also log at
the DEBUG level for block events, which is appropriate.

Fixes lightningdevkit#980.
@TheBlueMatt TheBlueMatt force-pushed the 2021-08-no-net-print branch from 77178c6 to 5d84704 Compare August 17, 2021 21:03
@TheBlueMatt
Copy link
Collaborator Author

Pushed a trivial change as suggested by val and squashed, diff from Jeff's ACK:

$ git diff-tree -U1 77178c6 5d847048
diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs
index 8100153a..8969427a 100644
--- a/lightning/src/chain/chainmonitor.rs
+++ b/lightning/src/chain/chainmonitor.rs
@@ -199,3 +199,3 @@ where
 	fn transaction_unconfirmed(&self, txid: &Txid) {
-		log_debug!(self.logger, "{} reorganized out of chain", txid);
+		log_debug!(self.logger, "Transaction {} reorganized out of chain", txid);
 		let monitors = self.monitors.read().unwrap();

@TheBlueMatt
Clarify docs on ShutdownScript::new_witness_program somewhat
35d0b7a 
This is especially important for C or other language bindings
clients as the `version` field may be exported as a `u8`.
@TheBlueMatt TheBlueMatt force-pushed the 2021-08-no-net-print branch from 5d84704 to 35d0b7a Compare August 17, 2021 21:12
@TheBlueMatt
Copy link
Collaborator Author

Oops, had to tweak the last commit cause I misunderstood doclinks, diff is trivial, so will land after CI:

$ git diff-tree -U1 5d847048 35d0b7aa
diff --git a/lightning/src/ln/script.rs b/lightning/src/ln/script.rs
index bee544ea..4e81d76a 100644
--- a/lightning/src/ln/script.rs
+++ b/lightning/src/ln/script.rs
@@ -90,3 +90,4 @@ impl ShutdownScript {
 	///
-	/// Note for version-zero witness scripts you must use [`new_p2wpkh`] or [`new_p2wsh`] instead.
+	/// Note for version-zero witness scripts you must use [`ShutdownScript::new_p2wpkh`] or
+	/// [`ShutdownScript::new_p2wsh`] instead.
 	///
$

@TheBlueMatt TheBlueMatt merged commit 803d804 into lightningdevkit:main Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkczyz jkczyz jkczyz approved these changes

@valentinewallace valentinewallace valentinewallace approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.0.101
Development

Successfully merging this pull request may close these issues.

Clean up block connection logging in ChainMonitor/ChannelMonitor
3 participants
@TheBlueMatt @jkczyz @valentinewallace