Require WithContext log wrappers on OutboundPayments calls and pass payment hashes

[TheBlueMatt]

In much of LDK we pass around `Logger` objects both to avoid having
to `Clone` `Logger` `Deref`s (soon to only be `Logger`s) and to
allow us to set context with a wrapper such that any log calls on
that wrapper get additional useful metadata in them.
Sadly, when we added a `Logger` type to `OutboundPayments` we broke
the ability to do the second thing - payment information logged
directly or indirectly via logic in the `OutboundPayments` has no
context making log-searching rather challenging.
Here we move to instead using the automated `add_logging`
proc-macro to require that `OutboundPayment` functions receive a
`WithContext` logger, appropriately setting (especially) the
`payment_hash` as we do so.
Fixes #4307

I believe the first three commits can/should be backported to 0.2. Ideally we'd also backports a variant to 0.1 to fix#4307 there but I'm not really sure its quite worth it to write a whole new version just for 0.1.

[ldk-reviews-bot]

👋 Thanks for assigning @wpaulino 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 88.41202% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.10%. Comparing base (7fe3268) to head (878e632).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/outbound_payment.rs	85.40%	12 Missing and 8 partials ⚠️
lightning/src/ln/channelmanager.rs	86.53%	7 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4342 +/- ##
=========================================
+ Coverage 0 86.10% +86.10% 
=========================================
Files 0 156 +156 Lines 0 102526 +102526 Branches 0 102526 +102526 =========================================
+ Hits 0 88284 +88284 - Misses 0 11747 +11747 - Partials 0 2495 +2495 

Flag	Coverage Δ
tests	86.10% <88.41%> (?)

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.

[TheBlueMatt]

Planning to backport to 0.2 in #4344

[wpaulino]

Code LGTM though I'm not really a fan of the hidden logger argument.

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

[ldk-reviews-bot]

✅ Added second reviewer: @valentinewallace

[TheBlueMatt]

I'm not really a fan of the hidden logger argument.

I could go either way. It seems like an easy way to ensure we always have the logger which will make @joostjager happy, and in this case its fairly straightforward (because we're not trying to automate passing the logger from ChannelManager).

[valentinewallace]

I'm not really into this juice vs squeeze ratio. I think @joostjager should take a look since it's supposed to make him happy :) If it really does, I guess it's fine.

Mentioned to Matt offline but Claude pointed out that the visit-mutsyn feature allows removing ~all the boilerplate.

[joostjager]

Appreciate the attempt, but I don't think this is the right direction. The macro adds a layer of complexity, and the hidden logger argument that gets silently injected into every self.*() call isn't very intuitive. I agree with @wpaulino and @valentinewallace's hesitation.

My preference would still be to keep exploring something similar to what the tracing crate does with the context. For no-std the developer experience might not be optimal, but I still think that is a reasonable trade-off.

If that is unacceptable, I think we're better off sticking with explicit logger parameters and types. At least the code does what it says.

[TheBlueMatt]

Alrighty, explicit loggers it is.

[TheBlueMatt]

Had to rebase for a trivial conflict.

[valentinewallace]

LGTM. I think CI is still sad

[valentinewallace]

It might make sense to use the first-hop channel_id/peer here, similarly for claims below

[TheBlueMatt]

Oh it is done but in the last commit.

[valentinewallace]

Somewhat pre-existing, but could it be worth checking that we're not overriding an already set field with a different field here?

[TheBlueMatt]

The CI failure was a flake, but I added an avoid-overriding check in the context:

$ git diff-tree -U2 91e4354e36 878e632cdb
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 35e4b9fcc0..1ad1f22f15 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -5803,5 +5803,4 @@ where
fn abandon_payment_with_reason(&self, payment_id: PaymentId, reason: PaymentFailureReason) {
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
-
self.pending_outbound_payments.abandon_payment(payment_id, reason, &self.pending_events);
}
diff --git a/lightning/src/util/logger.rs b/lightning/src/util/logger.rs
index 2921688f93..0d2eb47fa6 100644
--- a/lightning/src/util/logger.rs+++ b/lightning/src/util/logger.rs@@ -315,14 +315,14 @@ where
{
fn log(&self, mut record: Record) {
- if self.peer_id.is_some() {+ if self.peer_id.is_some() && record.peer_id.is_none() {
record.peer_id = self.peer_id
};
- if self.channel_id.is_some() {+ if self.channel_id.is_some() && record.channel_id.is_none() {
record.channel_id = self.channel_id;
}
- if self.payment_hash.is_some() {+ if self.payment_hash.is_some() && record.payment_hash.is_none() {
record.payment_hash = self.payment_hash;
}
- if self.payment_id.is_some() {+ if self.payment_id.is_some() && record.payment_id.is_none() {
record.payment_id = self.payment_id;
}

[valentinewallace]

I added an avoid-overriding check in the context:

$ git diff-tree -U2 91e4354e36 878e632cdb
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 35e4b9fcc0..1ad1f22f15 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -5803,5 +5803,4 @@ where
fn abandon_payment_with_reason(&self, payment_id: PaymentId, reason: PaymentFailureReason) {
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
-
self.pending_outbound_payments.abandon_payment(payment_id, reason, &self.pending_events);
}
diff --git a/lightning/src/util/logger.rs b/lightning/src/util/logger.rs
index 2921688f93..0d2eb47fa6 100644
--- a/lightning/src/util/logger.rs+++ b/lightning/src/util/logger.rs@@ -315,14 +315,14 @@ where
{
fn log(&self, mut record: Record) {
- if self.peer_id.is_some() {+ if self.peer_id.is_some() && record.peer_id.is_none() {
record.peer_id = self.peer_id
};
- if self.channel_id.is_some() {+ if self.channel_id.is_some() && record.channel_id.is_none() {
record.channel_id = self.channel_id;
}
- if self.payment_hash.is_some() {+ if self.payment_hash.is_some() && record.payment_hash.is_none() {
record.payment_hash = self.payment_hash;
}
- if self.payment_id.is_some() {+ if self.payment_id.is_some() && record.payment_id.is_none() {
record.payment_id = self.payment_id;
}

Is it a bug if this happens? I was thinking more like adding debug_asserts

[TheBlueMatt]

I tried that but we do already hit it in a few places. I doesn't really seem like a bug per se - we might be forwarding from a channel, wrap the logger, then go to wrap it again when for the to channel.

[valentinewallace]

Left a comment regarding missing debug_asserts, but I assume there's a good reason. Would like to land this soon to unblock #4311

[valentinewallace]

Pretty trivial now since the proc-macros left, so landing with 1 review

[TheBlueMatt]

Backported in #4344.

[TheBlueMatt]

In much of LDK we pass around `Logger` objects both to avoid having
to `Clone` `Logger` `Deref`s (soon to only be `Logger`s) and to
allow us to set context with a wrapper such that any log calls on
that wrapper get additional useful metadata in them.
Sadly, when we added a `Logger` type to `OutboundPayments` we broke
the ability to do the second thing - payment information logged
directly or indirectly via logic in the `OutboundPayments` has no
context making log-searching rather challenging.
Here we move to instead using the automated `add_logging`
proc-macro to require that `OutboundPayment` functions receive a
`WithContext` logger, appropriately setting (especially) the
`payment_hash` as we do so.
Fixes #4307

I believe the first three commits can/should be backported to 0.2. Ideally we'd also backports a variant to 0.1 to fix#4307 there but I'm not really sure its quite worth it to write a whole new version just for 0.1.

[ldk-reviews-bot]

👋 Thanks for assigning @wpaulino 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 88.41202% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.10%. Comparing base (7fe3268) to head (878e632).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/outbound_payment.rs	85.40%	12 Missing and 8 partials ⚠️
lightning/src/ln/channelmanager.rs	86.53%	7 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4342 +/- ##
=========================================
+ Coverage 0 86.10% +86.10% 
=========================================
Files 0 156 +156 Lines 0 102526 +102526 Branches 0 102526 +102526 =========================================
+ Hits 0 88284 +88284 - Misses 0 11747 +11747 - Partials 0 2495 +2495 

Flag	Coverage Δ
tests	86.10% <88.41%> (?)

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.

[TheBlueMatt]

Planning to backport to 0.2 in #4344

[wpaulino]

Code LGTM though I'm not really a fan of the hidden logger argument.

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

[ldk-reviews-bot]

✅ Added second reviewer: @valentinewallace

[TheBlueMatt]

I'm not really a fan of the hidden logger argument.

I could go either way. It seems like an easy way to ensure we always have the logger which will make @joostjager happy, and in this case its fairly straightforward (because we're not trying to automate passing the logger from ChannelManager).

[valentinewallace]

I'm not really into this juice vs squeeze ratio. I think @joostjager should take a look since it's supposed to make him happy :) If it really does, I guess it's fine.

Mentioned to Matt offline but Claude pointed out that the visit-mutsyn feature allows removing ~all the boilerplate.

[joostjager]

Appreciate the attempt, but I don't think this is the right direction. The macro adds a layer of complexity, and the hidden logger argument that gets silently injected into every self.*() call isn't very intuitive. I agree with @wpaulino and @valentinewallace's hesitation.

My preference would still be to keep exploring something similar to what the tracing crate does with the context. For no-std the developer experience might not be optimal, but I still think that is a reasonable trade-off.

If that is unacceptable, I think we're better off sticking with explicit logger parameters and types. At least the code does what it says.

[TheBlueMatt]

Alrighty, explicit loggers it is.

[TheBlueMatt]

Had to rebase for a trivial conflict.

[valentinewallace]

LGTM. I think CI is still sad

[valentinewallace]

It might make sense to use the first-hop channel_id/peer here, similarly for claims below

[TheBlueMatt]

Oh it is done but in the last commit.

[valentinewallace]

Somewhat pre-existing, but could it be worth checking that we're not overriding an already set field with a different field here?

[TheBlueMatt]

The CI failure was a flake, but I added an avoid-overriding check in the context:

$ git diff-tree -U2 91e4354e36 878e632cdb
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 35e4b9fcc0..1ad1f22f15 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -5803,5 +5803,4 @@ where
fn abandon_payment_with_reason(&self, payment_id: PaymentId, reason: PaymentFailureReason) {
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
-
self.pending_outbound_payments.abandon_payment(payment_id, reason, &self.pending_events);
}
diff --git a/lightning/src/util/logger.rs b/lightning/src/util/logger.rs
index 2921688f93..0d2eb47fa6 100644
--- a/lightning/src/util/logger.rs+++ b/lightning/src/util/logger.rs@@ -315,14 +315,14 @@ where
{
fn log(&self, mut record: Record) {
- if self.peer_id.is_some() {+ if self.peer_id.is_some() && record.peer_id.is_none() {
record.peer_id = self.peer_id
};
- if self.channel_id.is_some() {+ if self.channel_id.is_some() && record.channel_id.is_none() {
record.channel_id = self.channel_id;
}
- if self.payment_hash.is_some() {+ if self.payment_hash.is_some() && record.payment_hash.is_none() {
record.payment_hash = self.payment_hash;
}
- if self.payment_id.is_some() {+ if self.payment_id.is_some() && record.payment_id.is_none() {
record.payment_id = self.payment_id;
}

[valentinewallace]

I added an avoid-overriding check in the context:

$ git diff-tree -U2 91e4354e36 878e632cdb
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 35e4b9fcc0..1ad1f22f15 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -5803,5 +5803,4 @@ where
fn abandon_payment_with_reason(&self, payment_id: PaymentId, reason: PaymentFailureReason) {
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
-
self.pending_outbound_payments.abandon_payment(payment_id, reason, &self.pending_events);
}
diff --git a/lightning/src/util/logger.rs b/lightning/src/util/logger.rs
index 2921688f93..0d2eb47fa6 100644
--- a/lightning/src/util/logger.rs+++ b/lightning/src/util/logger.rs@@ -315,14 +315,14 @@ where
{
fn log(&self, mut record: Record) {
- if self.peer_id.is_some() {+ if self.peer_id.is_some() && record.peer_id.is_none() {
record.peer_id = self.peer_id
};
- if self.channel_id.is_some() {+ if self.channel_id.is_some() && record.channel_id.is_none() {
record.channel_id = self.channel_id;
}
- if self.payment_hash.is_some() {+ if self.payment_hash.is_some() && record.payment_hash.is_none() {
record.payment_hash = self.payment_hash;
}
- if self.payment_id.is_some() {+ if self.payment_id.is_some() && record.payment_id.is_none() {
record.payment_id = self.payment_id;
}

Is it a bug if this happens? I was thinking more like adding debug_asserts

[TheBlueMatt]

I tried that but we do already hit it in a few places. I doesn't really seem like a bug per se - we might be forwarding from a channel, wrap the logger, then go to wrap it again when for the to channel.

[valentinewallace]

Left a comment regarding missing debug_asserts, but I assume there's a good reason. Would like to land this soon to unblock #4311

[valentinewallace]

Pretty trivial now since the proc-macros left, so landing with 1 review

[TheBlueMatt]

Backported in #4344.

[TheBlueMatt]

In much of LDK we pass around `Logger` objects both to avoid having
to `Clone` `Logger` `Deref`s (soon to only be `Logger`s) and to
allow us to set context with a wrapper such that any log calls on
that wrapper get additional useful metadata in them.
Sadly, when we added a `Logger` type to `OutboundPayments` we broke
the ability to do the second thing - payment information logged
directly or indirectly via logic in the `OutboundPayments` has no
context making log-searching rather challenging.
Here we move to instead using the automated `add_logging`
proc-macro to require that `OutboundPayment` functions receive a
`WithContext` logger, appropriately setting (especially) the
`payment_hash` as we do so.
Fixes #4307

I believe the first three commits can/should be backported to 0.2. Ideally we'd also backports a variant to 0.1 to fix#4307 there but I'm not really sure its quite worth it to write a whole new version just for 0.1.

[ldk-reviews-bot]

👋 Thanks for assigning @wpaulino 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 88.41202% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.10%. Comparing base (7fe3268) to head (878e632).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/outbound_payment.rs	85.40%	12 Missing and 8 partials ⚠️
lightning/src/ln/channelmanager.rs	86.53%	7 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4342 +/- ##
=========================================
+ Coverage 0 86.10% +86.10% 
=========================================
Files 0 156 +156 Lines 0 102526 +102526 Branches 0 102526 +102526 =========================================
+ Hits 0 88284 +88284 - Misses 0 11747 +11747 - Partials 0 2495 +2495 

Flag	Coverage Δ
tests	86.10% <88.41%> (?)

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.

[TheBlueMatt]

Planning to backport to 0.2 in #4344

[wpaulino]

Code LGTM though I'm not really a fan of the hidden logger argument.

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

[ldk-reviews-bot]

✅ Added second reviewer: @valentinewallace

[TheBlueMatt]

I'm not really a fan of the hidden logger argument.

I could go either way. It seems like an easy way to ensure we always have the logger which will make @joostjager happy, and in this case its fairly straightforward (because we're not trying to automate passing the logger from ChannelManager).

[valentinewallace]

I'm not really into this juice vs squeeze ratio. I think @joostjager should take a look since it's supposed to make him happy :) If it really does, I guess it's fine.

Mentioned to Matt offline but Claude pointed out that the visit-mutsyn feature allows removing ~all the boilerplate.

[joostjager]

Appreciate the attempt, but I don't think this is the right direction. The macro adds a layer of complexity, and the hidden logger argument that gets silently injected into every self.*() call isn't very intuitive. I agree with @wpaulino and @valentinewallace's hesitation.

My preference would still be to keep exploring something similar to what the tracing crate does with the context. For no-std the developer experience might not be optimal, but I still think that is a reasonable trade-off.

If that is unacceptable, I think we're better off sticking with explicit logger parameters and types. At least the code does what it says.

[TheBlueMatt]

Alrighty, explicit loggers it is.

[TheBlueMatt]

Had to rebase for a trivial conflict.

[valentinewallace]

LGTM. I think CI is still sad

[valentinewallace]

It might make sense to use the first-hop channel_id/peer here, similarly for claims below

[TheBlueMatt]

Oh it is done but in the last commit.

[valentinewallace]

Somewhat pre-existing, but could it be worth checking that we're not overriding an already set field with a different field here?

[TheBlueMatt]

The CI failure was a flake, but I added an avoid-overriding check in the context:

$ git diff-tree -U2 91e4354e36 878e632cdb
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 35e4b9fcc0..1ad1f22f15 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -5803,5 +5803,4 @@ where
fn abandon_payment_with_reason(&self, payment_id: PaymentId, reason: PaymentFailureReason) {
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
-
self.pending_outbound_payments.abandon_payment(payment_id, reason, &self.pending_events);
}
diff --git a/lightning/src/util/logger.rs b/lightning/src/util/logger.rs
index 2921688f93..0d2eb47fa6 100644
--- a/lightning/src/util/logger.rs+++ b/lightning/src/util/logger.rs@@ -315,14 +315,14 @@ where
{
fn log(&self, mut record: Record) {
- if self.peer_id.is_some() {+ if self.peer_id.is_some() && record.peer_id.is_none() {
record.peer_id = self.peer_id
};
- if self.channel_id.is_some() {+ if self.channel_id.is_some() && record.channel_id.is_none() {
record.channel_id = self.channel_id;
}
- if self.payment_hash.is_some() {+ if self.payment_hash.is_some() && record.payment_hash.is_none() {
record.payment_hash = self.payment_hash;
}
- if self.payment_id.is_some() {+ if self.payment_id.is_some() && record.payment_id.is_none() {
record.payment_id = self.payment_id;
}

[valentinewallace]

I added an avoid-overriding check in the context:

$ git diff-tree -U2 91e4354e36 878e632cdb
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 35e4b9fcc0..1ad1f22f15 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -5803,5 +5803,4 @@ where
fn abandon_payment_with_reason(&self, payment_id: PaymentId, reason: PaymentFailureReason) {
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
-
self.pending_outbound_payments.abandon_payment(payment_id, reason, &self.pending_events);
}
diff --git a/lightning/src/util/logger.rs b/lightning/src/util/logger.rs
index 2921688f93..0d2eb47fa6 100644
--- a/lightning/src/util/logger.rs+++ b/lightning/src/util/logger.rs@@ -315,14 +315,14 @@ where
{
fn log(&self, mut record: Record) {
- if self.peer_id.is_some() {+ if self.peer_id.is_some() && record.peer_id.is_none() {
record.peer_id = self.peer_id
};
- if self.channel_id.is_some() {+ if self.channel_id.is_some() && record.channel_id.is_none() {
record.channel_id = self.channel_id;
}
- if self.payment_hash.is_some() {+ if self.payment_hash.is_some() && record.payment_hash.is_none() {
record.payment_hash = self.payment_hash;
}
- if self.payment_id.is_some() {+ if self.payment_id.is_some() && record.payment_id.is_none() {
record.payment_id = self.payment_id;
}

Is it a bug if this happens? I was thinking more like adding debug_asserts

[TheBlueMatt]

I tried that but we do already hit it in a few places. I doesn't really seem like a bug per se - we might be forwarding from a channel, wrap the logger, then go to wrap it again when for the to channel.

[valentinewallace]

Left a comment regarding missing debug_asserts, but I assume there's a good reason. Would like to land this soon to unblock #4311

[valentinewallace]

Pretty trivial now since the proc-macros left, so landing with 1 review

[TheBlueMatt]

Backported in #4344.

[TheBlueMatt]

In much of LDK we pass around `Logger` objects both to avoid having
to `Clone` `Logger` `Deref`s (soon to only be `Logger`s) and to
allow us to set context with a wrapper such that any log calls on
that wrapper get additional useful metadata in them.
Sadly, when we added a `Logger` type to `OutboundPayments` we broke
the ability to do the second thing - payment information logged
directly or indirectly via logic in the `OutboundPayments` has no
context making log-searching rather challenging.
Here we move to instead using the automated `add_logging`
proc-macro to require that `OutboundPayment` functions receive a
`WithContext` logger, appropriately setting (especially) the
`payment_hash` as we do so.
Fixes #4307

I believe the first three commits can/should be backported to 0.2. Ideally we'd also backports a variant to 0.1 to fix#4307 there but I'm not really sure its quite worth it to write a whole new version just for 0.1.

[ldk-reviews-bot]

👋 Thanks for assigning @wpaulino 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 88.41202% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.10%. Comparing base (7fe3268) to head (878e632).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/outbound_payment.rs	85.40%	12 Missing and 8 partials ⚠️
lightning/src/ln/channelmanager.rs	86.53%	7 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4342 +/- ##
=========================================
+ Coverage 0 86.10% +86.10% 
=========================================
Files 0 156 +156 Lines 0 102526 +102526 Branches 0 102526 +102526 =========================================
+ Hits 0 88284 +88284 - Misses 0 11747 +11747 - Partials 0 2495 +2495 

Flag	Coverage Δ
tests	86.10% <88.41%> (?)

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.

[TheBlueMatt]

Planning to backport to 0.2 in #4344

[wpaulino]

Code LGTM though I'm not really a fan of the hidden logger argument.

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

[ldk-reviews-bot]

✅ Added second reviewer: @valentinewallace

[TheBlueMatt]

I'm not really a fan of the hidden logger argument.

I could go either way. It seems like an easy way to ensure we always have the logger which will make @joostjager happy, and in this case its fairly straightforward (because we're not trying to automate passing the logger from ChannelManager).

[valentinewallace]

I'm not really into this juice vs squeeze ratio. I think @joostjager should take a look since it's supposed to make him happy :) If it really does, I guess it's fine.

Mentioned to Matt offline but Claude pointed out that the visit-mutsyn feature allows removing ~all the boilerplate.

[joostjager]

Appreciate the attempt, but I don't think this is the right direction. The macro adds a layer of complexity, and the hidden logger argument that gets silently injected into every self.*() call isn't very intuitive. I agree with @wpaulino and @valentinewallace's hesitation.

My preference would still be to keep exploring something similar to what the tracing crate does with the context. For no-std the developer experience might not be optimal, but I still think that is a reasonable trade-off.

If that is unacceptable, I think we're better off sticking with explicit logger parameters and types. At least the code does what it says.

[TheBlueMatt]

Alrighty, explicit loggers it is.

[TheBlueMatt]

Had to rebase for a trivial conflict.

[valentinewallace]

LGTM. I think CI is still sad

[valentinewallace]

It might make sense to use the first-hop channel_id/peer here, similarly for claims below

[TheBlueMatt]

Oh it is done but in the last commit.

[valentinewallace]

Somewhat pre-existing, but could it be worth checking that we're not overriding an already set field with a different field here?

[TheBlueMatt]

The CI failure was a flake, but I added an avoid-overriding check in the context:

$ git diff-tree -U2 91e4354e36 878e632cdb
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 35e4b9fcc0..1ad1f22f15 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -5803,5 +5803,4 @@ where
fn abandon_payment_with_reason(&self, payment_id: PaymentId, reason: PaymentFailureReason) {
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
-
self.pending_outbound_payments.abandon_payment(payment_id, reason, &self.pending_events);
}
diff --git a/lightning/src/util/logger.rs b/lightning/src/util/logger.rs
index 2921688f93..0d2eb47fa6 100644
--- a/lightning/src/util/logger.rs+++ b/lightning/src/util/logger.rs@@ -315,14 +315,14 @@ where
{
fn log(&self, mut record: Record) {
- if self.peer_id.is_some() {+ if self.peer_id.is_some() && record.peer_id.is_none() {
record.peer_id = self.peer_id
};
- if self.channel_id.is_some() {+ if self.channel_id.is_some() && record.channel_id.is_none() {
record.channel_id = self.channel_id;
}
- if self.payment_hash.is_some() {+ if self.payment_hash.is_some() && record.payment_hash.is_none() {
record.payment_hash = self.payment_hash;
}
- if self.payment_id.is_some() {+ if self.payment_id.is_some() && record.payment_id.is_none() {
record.payment_id = self.payment_id;
}

[valentinewallace]

I added an avoid-overriding check in the context:

$ git diff-tree -U2 91e4354e36 878e632cdb
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 35e4b9fcc0..1ad1f22f15 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -5803,5 +5803,4 @@ where
fn abandon_payment_with_reason(&self, payment_id: PaymentId, reason: PaymentFailureReason) {
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
-
self.pending_outbound_payments.abandon_payment(payment_id, reason, &self.pending_events);
}
diff --git a/lightning/src/util/logger.rs b/lightning/src/util/logger.rs
index 2921688f93..0d2eb47fa6 100644
--- a/lightning/src/util/logger.rs+++ b/lightning/src/util/logger.rs@@ -315,14 +315,14 @@ where
{
fn log(&self, mut record: Record) {
- if self.peer_id.is_some() {+ if self.peer_id.is_some() && record.peer_id.is_none() {
record.peer_id = self.peer_id
};
- if self.channel_id.is_some() {+ if self.channel_id.is_some() && record.channel_id.is_none() {
record.channel_id = self.channel_id;
}
- if self.payment_hash.is_some() {+ if self.payment_hash.is_some() && record.payment_hash.is_none() {
record.payment_hash = self.payment_hash;
}
- if self.payment_id.is_some() {+ if self.payment_id.is_some() && record.payment_id.is_none() {
record.payment_id = self.payment_id;
}

Is it a bug if this happens? I was thinking more like adding debug_asserts

[TheBlueMatt]

I tried that but we do already hit it in a few places. I doesn't really seem like a bug per se - we might be forwarding from a channel, wrap the logger, then go to wrap it again when for the to channel.

[valentinewallace]

Left a comment regarding missing debug_asserts, but I assume there's a good reason. Would like to land this soon to unblock #4311

[valentinewallace]

Pretty trivial now since the proc-macros left, so landing with 1 review

[TheBlueMatt]

Backported in #4344.

[TheBlueMatt]

In much of LDK we pass around `Logger` objects both to avoid having
to `Clone` `Logger` `Deref`s (soon to only be `Logger`s) and to
allow us to set context with a wrapper such that any log calls on
that wrapper get additional useful metadata in them.
Sadly, when we added a `Logger` type to `OutboundPayments` we broke
the ability to do the second thing - payment information logged
directly or indirectly via logic in the `OutboundPayments` has no
context making log-searching rather challenging.
Here we move to instead using the automated `add_logging`
proc-macro to require that `OutboundPayment` functions receive a
`WithContext` logger, appropriately setting (especially) the
`payment_hash` as we do so.
Fixes #4307

I believe the first three commits can/should be backported to 0.2. Ideally we'd also backports a variant to 0.1 to fix#4307 there but I'm not really sure its quite worth it to write a whole new version just for 0.1.

[ldk-reviews-bot]

👋 Thanks for assigning @wpaulino 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 88.41202% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.10%. Comparing base (7fe3268) to head (878e632).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/outbound_payment.rs	85.40%	12 Missing and 8 partials ⚠️
lightning/src/ln/channelmanager.rs	86.53%	7 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4342 +/- ##
=========================================
+ Coverage 0 86.10% +86.10% 
=========================================
Files 0 156 +156 Lines 0 102526 +102526 Branches 0 102526 +102526 =========================================
+ Hits 0 88284 +88284 - Misses 0 11747 +11747 - Partials 0 2495 +2495 

Flag	Coverage Δ
tests	86.10% <88.41%> (?)

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.

[TheBlueMatt]

Planning to backport to 0.2 in #4344

[wpaulino]

Code LGTM though I'm not really a fan of the hidden logger argument.

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

[ldk-reviews-bot]

✅ Added second reviewer: @valentinewallace

[TheBlueMatt]

I'm not really a fan of the hidden logger argument.

I could go either way. It seems like an easy way to ensure we always have the logger which will make @joostjager happy, and in this case its fairly straightforward (because we're not trying to automate passing the logger from ChannelManager).

[valentinewallace]

I'm not really into this juice vs squeeze ratio. I think @joostjager should take a look since it's supposed to make him happy :) If it really does, I guess it's fine.

Mentioned to Matt offline but Claude pointed out that the visit-mutsyn feature allows removing ~all the boilerplate.

[joostjager]

Appreciate the attempt, but I don't think this is the right direction. The macro adds a layer of complexity, and the hidden logger argument that gets silently injected into every self.*() call isn't very intuitive. I agree with @wpaulino and @valentinewallace's hesitation.

My preference would still be to keep exploring something similar to what the tracing crate does with the context. For no-std the developer experience might not be optimal, but I still think that is a reasonable trade-off.

If that is unacceptable, I think we're better off sticking with explicit logger parameters and types. At least the code does what it says.

[TheBlueMatt]

Alrighty, explicit loggers it is.

[TheBlueMatt]

Had to rebase for a trivial conflict.

[valentinewallace]

LGTM. I think CI is still sad

[valentinewallace]

It might make sense to use the first-hop channel_id/peer here, similarly for claims below

[TheBlueMatt]

Oh it is done but in the last commit.

[valentinewallace]

Somewhat pre-existing, but could it be worth checking that we're not overriding an already set field with a different field here?

[TheBlueMatt]

The CI failure was a flake, but I added an avoid-overriding check in the context:

$ git diff-tree -U2 91e4354e36 878e632cdb
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 35e4b9fcc0..1ad1f22f15 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -5803,5 +5803,4 @@ where
fn abandon_payment_with_reason(&self, payment_id: PaymentId, reason: PaymentFailureReason) {
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
-
self.pending_outbound_payments.abandon_payment(payment_id, reason, &self.pending_events);
}
diff --git a/lightning/src/util/logger.rs b/lightning/src/util/logger.rs
index 2921688f93..0d2eb47fa6 100644
--- a/lightning/src/util/logger.rs+++ b/lightning/src/util/logger.rs@@ -315,14 +315,14 @@ where
{
fn log(&self, mut record: Record) {
- if self.peer_id.is_some() {+ if self.peer_id.is_some() && record.peer_id.is_none() {
record.peer_id = self.peer_id
};
- if self.channel_id.is_some() {+ if self.channel_id.is_some() && record.channel_id.is_none() {
record.channel_id = self.channel_id;
}
- if self.payment_hash.is_some() {+ if self.payment_hash.is_some() && record.payment_hash.is_none() {
record.payment_hash = self.payment_hash;
}
- if self.payment_id.is_some() {+ if self.payment_id.is_some() && record.payment_id.is_none() {
record.payment_id = self.payment_id;
}

[valentinewallace]

I added an avoid-overriding check in the context:

$ git diff-tree -U2 91e4354e36 878e632cdb
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 35e4b9fcc0..1ad1f22f15 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -5803,5 +5803,4 @@ where
fn abandon_payment_with_reason(&self, payment_id: PaymentId, reason: PaymentFailureReason) {
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
-
self.pending_outbound_payments.abandon_payment(payment_id, reason, &self.pending_events);
}
diff --git a/lightning/src/util/logger.rs b/lightning/src/util/logger.rs
index 2921688f93..0d2eb47fa6 100644
--- a/lightning/src/util/logger.rs+++ b/lightning/src/util/logger.rs@@ -315,14 +315,14 @@ where
{
fn log(&self, mut record: Record) {
- if self.peer_id.is_some() {+ if self.peer_id.is_some() && record.peer_id.is_none() {
record.peer_id = self.peer_id
};
- if self.channel_id.is_some() {+ if self.channel_id.is_some() && record.channel_id.is_none() {
record.channel_id = self.channel_id;
}
- if self.payment_hash.is_some() {+ if self.payment_hash.is_some() && record.payment_hash.is_none() {
record.payment_hash = self.payment_hash;
}
- if self.payment_id.is_some() {+ if self.payment_id.is_some() && record.payment_id.is_none() {
record.payment_id = self.payment_id;
}

Is it a bug if this happens? I was thinking more like adding debug_asserts

[TheBlueMatt]

I tried that but we do already hit it in a few places. I doesn't really seem like a bug per se - we might be forwarding from a channel, wrap the logger, then go to wrap it again when for the to channel.

[valentinewallace]

Left a comment regarding missing debug_asserts, but I assume there's a good reason. Would like to land this soon to unblock #4311

[valentinewallace]

Pretty trivial now since the proc-macros left, so landing with 1 review

[TheBlueMatt]

Backported in #4344.

[TheBlueMatt]

In much of LDK we pass around `Logger` objects both to avoid having
to `Clone` `Logger` `Deref`s (soon to only be `Logger`s) and to
allow us to set context with a wrapper such that any log calls on
that wrapper get additional useful metadata in them.
Sadly, when we added a `Logger` type to `OutboundPayments` we broke
the ability to do the second thing - payment information logged
directly or indirectly via logic in the `OutboundPayments` has no
context making log-searching rather challenging.
Here we move to instead using the automated `add_logging`
proc-macro to require that `OutboundPayment` functions receive a
`WithContext` logger, appropriately setting (especially) the
`payment_hash` as we do so.
Fixes #4307

I believe the first three commits can/should be backported to 0.2. Ideally we'd also backports a variant to 0.1 to fix#4307 there but I'm not really sure its quite worth it to write a whole new version just for 0.1.

[ldk-reviews-bot]

👋 Thanks for assigning @wpaulino 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 88.41202% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.10%. Comparing base (7fe3268) to head (878e632).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/outbound_payment.rs	85.40%	12 Missing and 8 partials ⚠️
lightning/src/ln/channelmanager.rs	86.53%	7 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4342 +/- ##
=========================================
+ Coverage 0 86.10% +86.10% 
=========================================
Files 0 156 +156 Lines 0 102526 +102526 Branches 0 102526 +102526 =========================================
+ Hits 0 88284 +88284 - Misses 0 11747 +11747 - Partials 0 2495 +2495 

Flag	Coverage Δ
tests	86.10% <88.41%> (?)

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.

[TheBlueMatt]

Planning to backport to 0.2 in #4344

[wpaulino]

Code LGTM though I'm not really a fan of the hidden logger argument.

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

[ldk-reviews-bot]

✅ Added second reviewer: @valentinewallace

[TheBlueMatt]

I'm not really a fan of the hidden logger argument.

I could go either way. It seems like an easy way to ensure we always have the logger which will make @joostjager happy, and in this case its fairly straightforward (because we're not trying to automate passing the logger from ChannelManager).

[valentinewallace]

I'm not really into this juice vs squeeze ratio. I think @joostjager should take a look since it's supposed to make him happy :) If it really does, I guess it's fine.

Mentioned to Matt offline but Claude pointed out that the visit-mutsyn feature allows removing ~all the boilerplate.

[joostjager]

Appreciate the attempt, but I don't think this is the right direction. The macro adds a layer of complexity, and the hidden logger argument that gets silently injected into every self.*() call isn't very intuitive. I agree with @wpaulino and @valentinewallace's hesitation.

My preference would still be to keep exploring something similar to what the tracing crate does with the context. For no-std the developer experience might not be optimal, but I still think that is a reasonable trade-off.

If that is unacceptable, I think we're better off sticking with explicit logger parameters and types. At least the code does what it says.

[TheBlueMatt]

Alrighty, explicit loggers it is.

[TheBlueMatt]

Had to rebase for a trivial conflict.

[valentinewallace]

LGTM. I think CI is still sad

[valentinewallace]

It might make sense to use the first-hop channel_id/peer here, similarly for claims below

[TheBlueMatt]

Oh it is done but in the last commit.

[valentinewallace]

Somewhat pre-existing, but could it be worth checking that we're not overriding an already set field with a different field here?

[TheBlueMatt]

The CI failure was a flake, but I added an avoid-overriding check in the context:

$ git diff-tree -U2 91e4354e36 878e632cdb
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 35e4b9fcc0..1ad1f22f15 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -5803,5 +5803,4 @@ where
fn abandon_payment_with_reason(&self, payment_id: PaymentId, reason: PaymentFailureReason) {
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
-
self.pending_outbound_payments.abandon_payment(payment_id, reason, &self.pending_events);
}
diff --git a/lightning/src/util/logger.rs b/lightning/src/util/logger.rs
index 2921688f93..0d2eb47fa6 100644
--- a/lightning/src/util/logger.rs+++ b/lightning/src/util/logger.rs@@ -315,14 +315,14 @@ where
{
fn log(&self, mut record: Record) {
- if self.peer_id.is_some() {+ if self.peer_id.is_some() && record.peer_id.is_none() {
record.peer_id = self.peer_id
};
- if self.channel_id.is_some() {+ if self.channel_id.is_some() && record.channel_id.is_none() {
record.channel_id = self.channel_id;
}
- if self.payment_hash.is_some() {+ if self.payment_hash.is_some() && record.payment_hash.is_none() {
record.payment_hash = self.payment_hash;
}
- if self.payment_id.is_some() {+ if self.payment_id.is_some() && record.payment_id.is_none() {
record.payment_id = self.payment_id;
}

[valentinewallace]

I added an avoid-overriding check in the context:

$ git diff-tree -U2 91e4354e36 878e632cdb
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 35e4b9fcc0..1ad1f22f15 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -5803,5 +5803,4 @@ where
fn abandon_payment_with_reason(&self, payment_id: PaymentId, reason: PaymentFailureReason) {
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
-
self.pending_outbound_payments.abandon_payment(payment_id, reason, &self.pending_events);
}
diff --git a/lightning/src/util/logger.rs b/lightning/src/util/logger.rs
index 2921688f93..0d2eb47fa6 100644
--- a/lightning/src/util/logger.rs+++ b/lightning/src/util/logger.rs@@ -315,14 +315,14 @@ where
{
fn log(&self, mut record: Record) {
- if self.peer_id.is_some() {+ if self.peer_id.is_some() && record.peer_id.is_none() {
record.peer_id = self.peer_id
};
- if self.channel_id.is_some() {+ if self.channel_id.is_some() && record.channel_id.is_none() {
record.channel_id = self.channel_id;
}
- if self.payment_hash.is_some() {+ if self.payment_hash.is_some() && record.payment_hash.is_none() {
record.payment_hash = self.payment_hash;
}
- if self.payment_id.is_some() {+ if self.payment_id.is_some() && record.payment_id.is_none() {
record.payment_id = self.payment_id;
}

Is it a bug if this happens? I was thinking more like adding debug_asserts

[TheBlueMatt]

I tried that but we do already hit it in a few places. I doesn't really seem like a bug per se - we might be forwarding from a channel, wrap the logger, then go to wrap it again when for the to channel.

[valentinewallace]

Left a comment regarding missing debug_asserts, but I assume there's a good reason. Would like to land this soon to unblock #4311

[valentinewallace]

Pretty trivial now since the proc-macros left, so landing with 1 review

[TheBlueMatt]

Backported in #4344.

[TheBlueMatt]

In much of LDK we pass around `Logger` objects both to avoid having
to `Clone` `Logger` `Deref`s (soon to only be `Logger`s) and to
allow us to set context with a wrapper such that any log calls on
that wrapper get additional useful metadata in them.
Sadly, when we added a `Logger` type to `OutboundPayments` we broke
the ability to do the second thing - payment information logged
directly or indirectly via logic in the `OutboundPayments` has no
context making log-searching rather challenging.
Here we move to instead using the automated `add_logging`
proc-macro to require that `OutboundPayment` functions receive a
`WithContext` logger, appropriately setting (especially) the
`payment_hash` as we do so.
Fixes #4307

I believe the first three commits can/should be backported to 0.2. Ideally we'd also backports a variant to 0.1 to fix#4307 there but I'm not really sure its quite worth it to write a whole new version just for 0.1.

[ldk-reviews-bot]

👋 Thanks for assigning @wpaulino 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 88.41202% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.10%. Comparing base (7fe3268) to head (878e632).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/outbound_payment.rs	85.40%	12 Missing and 8 partials ⚠️
lightning/src/ln/channelmanager.rs	86.53%	7 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4342 +/- ##
=========================================
+ Coverage 0 86.10% +86.10% 
=========================================
Files 0 156 +156 Lines 0 102526 +102526 Branches 0 102526 +102526 =========================================
+ Hits 0 88284 +88284 - Misses 0 11747 +11747 - Partials 0 2495 +2495 

Flag	Coverage Δ
tests	86.10% <88.41%> (?)

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.

[TheBlueMatt]

Planning to backport to 0.2 in #4344

[wpaulino]

Code LGTM though I'm not really a fan of the hidden logger argument.

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

[ldk-reviews-bot]

✅ Added second reviewer: @valentinewallace

[TheBlueMatt]

I'm not really a fan of the hidden logger argument.

I could go either way. It seems like an easy way to ensure we always have the logger which will make @joostjager happy, and in this case its fairly straightforward (because we're not trying to automate passing the logger from ChannelManager).

[valentinewallace]

I'm not really into this juice vs squeeze ratio. I think @joostjager should take a look since it's supposed to make him happy :) If it really does, I guess it's fine.

Mentioned to Matt offline but Claude pointed out that the visit-mutsyn feature allows removing ~all the boilerplate.

[joostjager]

Appreciate the attempt, but I don't think this is the right direction. The macro adds a layer of complexity, and the hidden logger argument that gets silently injected into every self.*() call isn't very intuitive. I agree with @wpaulino and @valentinewallace's hesitation.

My preference would still be to keep exploring something similar to what the tracing crate does with the context. For no-std the developer experience might not be optimal, but I still think that is a reasonable trade-off.

If that is unacceptable, I think we're better off sticking with explicit logger parameters and types. At least the code does what it says.

[TheBlueMatt]

Alrighty, explicit loggers it is.

[TheBlueMatt]

Had to rebase for a trivial conflict.

[valentinewallace]

LGTM. I think CI is still sad

[valentinewallace]

It might make sense to use the first-hop channel_id/peer here, similarly for claims below

[TheBlueMatt]

Oh it is done but in the last commit.

[valentinewallace]

Somewhat pre-existing, but could it be worth checking that we're not overriding an already set field with a different field here?

[TheBlueMatt]

The CI failure was a flake, but I added an avoid-overriding check in the context:

$ git diff-tree -U2 91e4354e36 878e632cdb
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 35e4b9fcc0..1ad1f22f15 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -5803,5 +5803,4 @@ where
fn abandon_payment_with_reason(&self, payment_id: PaymentId, reason: PaymentFailureReason) {
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
-
self.pending_outbound_payments.abandon_payment(payment_id, reason, &self.pending_events);
}
diff --git a/lightning/src/util/logger.rs b/lightning/src/util/logger.rs
index 2921688f93..0d2eb47fa6 100644
--- a/lightning/src/util/logger.rs+++ b/lightning/src/util/logger.rs@@ -315,14 +315,14 @@ where
{
fn log(&self, mut record: Record) {
- if self.peer_id.is_some() {+ if self.peer_id.is_some() && record.peer_id.is_none() {
record.peer_id = self.peer_id
};
- if self.channel_id.is_some() {+ if self.channel_id.is_some() && record.channel_id.is_none() {
record.channel_id = self.channel_id;
}
- if self.payment_hash.is_some() {+ if self.payment_hash.is_some() && record.payment_hash.is_none() {
record.payment_hash = self.payment_hash;
}
- if self.payment_id.is_some() {+ if self.payment_id.is_some() && record.payment_id.is_none() {
record.payment_id = self.payment_id;
}

[valentinewallace]

I added an avoid-overriding check in the context:

$ git diff-tree -U2 91e4354e36 878e632cdb
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 35e4b9fcc0..1ad1f22f15 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -5803,5 +5803,4 @@ where
fn abandon_payment_with_reason(&self, payment_id: PaymentId, reason: PaymentFailureReason) {
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
-
self.pending_outbound_payments.abandon_payment(payment_id, reason, &self.pending_events);
}
diff --git a/lightning/src/util/logger.rs b/lightning/src/util/logger.rs
index 2921688f93..0d2eb47fa6 100644
--- a/lightning/src/util/logger.rs+++ b/lightning/src/util/logger.rs@@ -315,14 +315,14 @@ where
{
fn log(&self, mut record: Record) {
- if self.peer_id.is_some() {+ if self.peer_id.is_some() && record.peer_id.is_none() {
record.peer_id = self.peer_id
};
- if self.channel_id.is_some() {+ if self.channel_id.is_some() && record.channel_id.is_none() {
record.channel_id = self.channel_id;
}
- if self.payment_hash.is_some() {+ if self.payment_hash.is_some() && record.payment_hash.is_none() {
record.payment_hash = self.payment_hash;
}
- if self.payment_id.is_some() {+ if self.payment_id.is_some() && record.payment_id.is_none() {
record.payment_id = self.payment_id;
}

Is it a bug if this happens? I was thinking more like adding debug_asserts

[TheBlueMatt]

I tried that but we do already hit it in a few places. I doesn't really seem like a bug per se - we might be forwarding from a channel, wrap the logger, then go to wrap it again when for the to channel.

[valentinewallace]

Left a comment regarding missing debug_asserts, but I assume there's a good reason. Would like to land this soon to unblock #4311

[valentinewallace]

Pretty trivial now since the proc-macros left, so landing with 1 review

[TheBlueMatt]

Backported in #4344.

[TheBlueMatt]

In much of LDK we pass around `Logger` objects both to avoid having
to `Clone` `Logger` `Deref`s (soon to only be `Logger`s) and to
allow us to set context with a wrapper such that any log calls on
that wrapper get additional useful metadata in them.
Sadly, when we added a `Logger` type to `OutboundPayments` we broke
the ability to do the second thing - payment information logged
directly or indirectly via logic in the `OutboundPayments` has no
context making log-searching rather challenging.
Here we move to instead using the automated `add_logging`
proc-macro to require that `OutboundPayment` functions receive a
`WithContext` logger, appropriately setting (especially) the
`payment_hash` as we do so.
Fixes #4307

I believe the first three commits can/should be backported to 0.2. Ideally we'd also backports a variant to 0.1 to fix#4307 there but I'm not really sure its quite worth it to write a whole new version just for 0.1.

[ldk-reviews-bot]

👋 Thanks for assigning @wpaulino 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 88.41202% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.10%. Comparing base (7fe3268) to head (878e632).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/outbound_payment.rs	85.40%	12 Missing and 8 partials ⚠️
lightning/src/ln/channelmanager.rs	86.53%	7 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4342 +/- ##
=========================================
+ Coverage 0 86.10% +86.10% 
=========================================
Files 0 156 +156 Lines 0 102526 +102526 Branches 0 102526 +102526 =========================================
+ Hits 0 88284 +88284 - Misses 0 11747 +11747 - Partials 0 2495 +2495 

Flag	Coverage Δ
tests	86.10% <88.41%> (?)

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.

[TheBlueMatt]

Planning to backport to 0.2 in #4344

[wpaulino]

Code LGTM though I'm not really a fan of the hidden logger argument.

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

[ldk-reviews-bot]

✅ Added second reviewer: @valentinewallace

[TheBlueMatt]

I'm not really a fan of the hidden logger argument.

I could go either way. It seems like an easy way to ensure we always have the logger which will make @joostjager happy, and in this case its fairly straightforward (because we're not trying to automate passing the logger from ChannelManager).

[valentinewallace]

I'm not really into this juice vs squeeze ratio. I think @joostjager should take a look since it's supposed to make him happy :) If it really does, I guess it's fine.

Mentioned to Matt offline but Claude pointed out that the visit-mutsyn feature allows removing ~all the boilerplate.

[joostjager]

Appreciate the attempt, but I don't think this is the right direction. The macro adds a layer of complexity, and the hidden logger argument that gets silently injected into every self.*() call isn't very intuitive. I agree with @wpaulino and @valentinewallace's hesitation.

My preference would still be to keep exploring something similar to what the tracing crate does with the context. For no-std the developer experience might not be optimal, but I still think that is a reasonable trade-off.

If that is unacceptable, I think we're better off sticking with explicit logger parameters and types. At least the code does what it says.

[TheBlueMatt]

Alrighty, explicit loggers it is.

[TheBlueMatt]

Had to rebase for a trivial conflict.

[valentinewallace]

LGTM. I think CI is still sad

[valentinewallace]

It might make sense to use the first-hop channel_id/peer here, similarly for claims below

[TheBlueMatt]

Oh it is done but in the last commit.

[valentinewallace]

Somewhat pre-existing, but could it be worth checking that we're not overriding an already set field with a different field here?

[TheBlueMatt]

The CI failure was a flake, but I added an avoid-overriding check in the context:

$ git diff-tree -U2 91e4354e36 878e632cdb
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 35e4b9fcc0..1ad1f22f15 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -5803,5 +5803,4 @@ where
fn abandon_payment_with_reason(&self, payment_id: PaymentId, reason: PaymentFailureReason) {
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
-
self.pending_outbound_payments.abandon_payment(payment_id, reason, &self.pending_events);
}
diff --git a/lightning/src/util/logger.rs b/lightning/src/util/logger.rs
index 2921688f93..0d2eb47fa6 100644
--- a/lightning/src/util/logger.rs+++ b/lightning/src/util/logger.rs@@ -315,14 +315,14 @@ where
{
fn log(&self, mut record: Record) {
- if self.peer_id.is_some() {+ if self.peer_id.is_some() && record.peer_id.is_none() {
record.peer_id = self.peer_id
};
- if self.channel_id.is_some() {+ if self.channel_id.is_some() && record.channel_id.is_none() {
record.channel_id = self.channel_id;
}
- if self.payment_hash.is_some() {+ if self.payment_hash.is_some() && record.payment_hash.is_none() {
record.payment_hash = self.payment_hash;
}
- if self.payment_id.is_some() {+ if self.payment_id.is_some() && record.payment_id.is_none() {
record.payment_id = self.payment_id;
}

[valentinewallace]

I added an avoid-overriding check in the context:

$ git diff-tree -U2 91e4354e36 878e632cdb
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 35e4b9fcc0..1ad1f22f15 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -5803,5 +5803,4 @@ where
fn abandon_payment_with_reason(&self, payment_id: PaymentId, reason: PaymentFailureReason) {
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
-
self.pending_outbound_payments.abandon_payment(payment_id, reason, &self.pending_events);
}
diff --git a/lightning/src/util/logger.rs b/lightning/src/util/logger.rs
index 2921688f93..0d2eb47fa6 100644
--- a/lightning/src/util/logger.rs+++ b/lightning/src/util/logger.rs@@ -315,14 +315,14 @@ where
{
fn log(&self, mut record: Record) {
- if self.peer_id.is_some() {+ if self.peer_id.is_some() && record.peer_id.is_none() {
record.peer_id = self.peer_id
};
- if self.channel_id.is_some() {+ if self.channel_id.is_some() && record.channel_id.is_none() {
record.channel_id = self.channel_id;
}
- if self.payment_hash.is_some() {+ if self.payment_hash.is_some() && record.payment_hash.is_none() {
record.payment_hash = self.payment_hash;
}
- if self.payment_id.is_some() {+ if self.payment_id.is_some() && record.payment_id.is_none() {
record.payment_id = self.payment_id;
}

Is it a bug if this happens? I was thinking more like adding debug_asserts

[TheBlueMatt]

I tried that but we do already hit it in a few places. I doesn't really seem like a bug per se - we might be forwarding from a channel, wrap the logger, then go to wrap it again when for the to channel.

[valentinewallace]

Left a comment regarding missing debug_asserts, but I assume there's a good reason. Would like to land this soon to unblock #4311

[valentinewallace]

Pretty trivial now since the proc-macros left, so landing with 1 review

[TheBlueMatt]

Backported in #4344.