Channel logging improvements

[joostjager]

Additional trace logs that helped with debugging #4218 and might be generally useful.

[ldk-reviews-bot]

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[codecov]

Codecov Report

❌ Patch coverage is 91.04478% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.32%. Comparing base (bb5504e) to head (9b2d57c).
⚠️ Report is 54 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	93.84%	1 Missing and 3 partials ⚠️
lightning/src/ln/channel.rs	0.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4242 +/- ##
==========================================
- Coverage 89.33% 89.32% -0.01% 
==========================================
Files 180 180 Lines 138680 139331 +651 Branches 138680 139331 +651 ==========================================
+ Hits 123888 124460 +572 - Misses 12172 12238 +66 - Partials 2620 2633 +13 

Flag	Coverage Δ
fuzzing	35.04% <32.83%> (-0.95%)	⬇️
tests	88.66% <91.04%> (-0.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 2nd Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 3rd Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[valentinewallace]

I like having more logs in these pipelines 👌

[valentinewallace]

You could include some extra info here:

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 63ecdd1fb..e799a0bda 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -9451,7 +9451,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
for action in actions.into_iter() {
match action {
MonitorUpdateCompletionAction::PaymentClaimed { payment_hash, pending_mpp_claim } => {
- let logger = WithContext::from(&self.logger, None, None, Some(payment_hash));+ let logger = WithContext::from(&self.logger, pending_mpp_claim.as_ref().map(|c| c.0), pending_mpp_claim.as_ref().map(|c| c.1), Some(payment_hash));
log_trace!(logger, "Handling PaymentClaimed monitor update completion action");
if let Some((counterparty_node_id, chan_id, claim_ptr)) = pending_mpp_claim {
valentine@Valentines-MacBook-Pro rust-lightning %

[joostjager]

Good addition. Rewrote it a bit.

[joostjager]

Tacked on rustfmt for updated method.

[valentinewallace]

IMO rustfmt mangles this code a bit at the moment.

Suggestion:

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index ea6409d0e..cd0adaaef 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -9465,58 +9465,41 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
WithContext::from(&self.logger, peer_id, chan_id, Some(payment_hash));
log_trace!(logger, "Handling PaymentClaimed monitor update completion action");
- if let Some((counterparty_node_id, chan_id, claim_ptr)) = pending_mpp_claim {+ if let Some((cp_node_id, chan_id, claim_ptr)) = pending_mpp_claim {
let per_peer_state = self.per_peer_state.read().unwrap();
- per_peer_state.get(&counterparty_node_id).map(|peer_state_mutex| {+ per_peer_state.get(&cp_node_id).map(|peer_state_mutex| {
let mut peer_state = peer_state_mutex.lock().unwrap();
let blockers_entry =
peer_state.actions_blocking_raa_monitor_updates.entry(chan_id);
if let btree_map::Entry::Occupied(mut blockers) = blockers_entry {
blockers.get_mut().retain(|blocker| {
- if let &RAAMonitorUpdateBlockingAction::ClaimedMPPPayment {- pending_claim,- } = &blocker- {- if *pending_claim == claim_ptr {- let mut pending_claim_state_lock =- pending_claim.0.lock().unwrap();- let pending_claim_state =- &mut *pending_claim_state_lock;- pending_claim_state.channels_without_preimage.retain(- |(cp, cid)| {- let this_claim = *cp == counterparty_node_id- && *cid == chan_id;- if this_claim {- pending_claim_state- .channels_with_preimage- .push((*cp, *cid));- false- } else {- true- }- },- );- if pending_claim_state- .channels_without_preimage- .is_empty()- {- for (cp, cid) in pending_claim_state- .channels_with_preimage- .iter()- {- let freed_chan = (*cp, *cid, blocker.clone());- freed_channels.push(freed_chan);- }- }- !pending_claim_state- .channels_without_preimage- .is_empty()+ let pending_claim = match &blocker {+ RAAMonitorUpdateBlockingAction::ClaimedMPPPayment {+ pending_claim,+ } => pending_claim,+ _ => return true,+ };+ if *pending_claim != claim_ptr {+ return true;+ }+ let mut claim_state_lock = pending_claim.0.lock().unwrap();+ let claim_state = &mut *claim_state_lock;+ claim_state.channels_without_preimage.retain(|(cp, cid)| {+ let this_claim = *cp == cp_node_id && *cid == chan_id;+ if this_claim {+ claim_state.channels_with_preimage.push((*cp, *cid));+ false
} else {
true
}
- } else {- true+ });+ if claim_state.channels_without_preimage.is_empty() {+ for (cp, cid) in claim_state.channels_with_preimage.iter() {+ let freed_chan = (*cp, *cid, blocker.clone());+ freed_channels.push(freed_chan);+ }
}
+ !claim_state.channels_without_preimage.is_empty()
});
if blockers.get().is_empty() {
blockers.remove();

[joostjager]

Applied patch, even though I think we should not do such hardly related refactorings. We've had a bug introduced before because of making rustfmt look nice.

[ldk-reviews-bot]

👋 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.

[valentinewallace]

LGTM