-
Notifications
You must be signed in to change notification settings - Fork 427
Add monitor_updating_paused logging #4298
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
Add monitor_updating_paused logging #4298
Conversation
|
👋 Thanks for assigning @valentinewallace as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4298 +/- ##
==========================================
- Coverage 89.38% 86.59% -2.79%
==========================================
Files 180 158 -22
Lines 139834 102093 -37741
Branches 139834 102093 -37741
==========================================
- Hits 124985 88404 -36581
+ Misses 12262 11280 -982
+ Partials 2587 2409 -178
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lightning/src/ln/channel.rs
Outdated
| &mut self, resend_raa: bool, resend_commitment: bool, resend_channel_ready: bool, | ||
| mut pending_forwards: Vec<(PendingHTLCInfo, u64)>, | ||
| fn monitor_updating_paused<L: Deref>( | ||
| &mut self, logger: &L, resend_raa: bool, resend_commitment: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think we usually have the logger as (one of) the last arguments, which makes sense to me because it's kinda the least important
lightning/src/ln/channel.rs
Outdated
| None => return Err((self, ChannelError::close("Received funding_signed before our first commitment point was available".to_owned()))), | ||
| }; | ||
| self.context.assert_no_commitment_advancement(holder_commitment_point.next_transaction_number(), "funding_signed"); | ||
| let mut holder_commitment_point = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and elsewhere, this seems more readable:
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 598a9166f..81c5c4c07 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -13818,17 +13818,13 @@ where
ChannelError::close("Received funding_signed in strange state!".to_owned()),
));
}
- let mut holder_commitment_point =
- match self.unfunded_context.holder_commitment_point {
- Some(point) => point,
- None => return Err((
- self,
- ChannelError::close(
- "Received funding_signed before our first commitment point was available"
- .to_owned(),
- ),
- )),
- };
+ let mut holder_commitment_point = match self.unfunded_context.holder_commitment_point {
+ Some(point) => point,
+ None => {
+ let err = "Received funding_signed before our first commitment point was available";
+ return Err((self, ChannelError::close(err.to_owned())));
+ },
+ };
self.context.assert_no_commitment_advancement(
holder_commitment_point.next_transaction_number(),
"funding_signed",There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion applied
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
7c7dc32 to
ed38fe3
Compare
valentinewallace
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be landable with 1 review, feel free to squash!
13a910b to
4c043e2
Compare
|
Fmt squashed |
| &self.signer_provider, | ||
| their_features, | ||
| target_feerate_sats_per_1000_weight, | ||
| override_shutdown_script, | ||
| &self.logger, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change and the above are in the wrong commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. Fixed.
Using extend is slightly cleaner because it doesn't require mut on the parameters.
Extract error message strings into local variables before constructing ChannelError return tuples, reducing nesting and improving readability.
4c043e2 to
51b5ef7
Compare
| pub fn shutdown( | ||
| &mut self, signer_provider: &SP, their_features: &InitFeatures, msg: &msgs::Shutdown, | ||
| pub fn shutdown<L: Deref>( | ||
| &mut self, logger: &L, signer_provider: &SP, their_features: &InitFeatures, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logger is still the first argument here but not gonna block
Make the logger better available