[RFC] move the bolt12 invoice inside HTLCSource::OutboundRoute

[vincenzopalazzo]

Matt noted during the last round of review the following:

Oof. Sorry I missed this until now. This is not, in fact, "only used for retries", we use it on claims only, in fact. If a user is relying on the event field for PoP, what this can mean is that we can initiate a send, restart with a stale ChannelManager, notice the payment is pending, then when it claims fail to provide the invoice (only the preimage) to the payer.
In practice, to fix this, we'll need to include the PaidBolt12Invoice in the HTLCSource::OutboundRoute, I believe.

This commit is trying to store the PaidBolt12Invoice inside the HTLCSource::OutboundRoute, but this is not enough because we have to store the invoice also inside the PendingOutboundPayment.

Fixes: #3714

[ldk-reviews-bot]

👋 I see @wpaulino was un-assigned.
If you'd like another reviewer assignemnt, please click here.

[TheBlueMatt]

Yea, this all makes sense I think. Sorry to make you undo some of the previous patch.

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

[vincenzopalazzo]

Thanks Matt! I rebased on the main to fix the commit checks and addressed some of the review comments. We should be good for another round

[ldk-reviews-bot]

✅ Added second reviewer: @wpaulino

[TheBlueMatt]

Conceptually looks good, but there's still quite a few FIXMEs left, do you want to address those?

[ldk-reviews-bot]

🔔 1st Reminder

Hey @wpaulino! 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.

[vincenzopalazzo]

Jumping in finishing this tomorrow, sorry for the delay!

[codecov]

Codecov Report

Attention: Patch coverage is 72.22222% with 10 lines in your changes missing coverage. Please review.

Project coverage is 89.12%. Comparing base (c6921fa) to head (d6d1c2f).
Report is 44 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/outbound_payment.rs	66.66%	3 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs	80.00%	2 Missing and 1 partial ⚠️
lightning/src/offers/static_invoice.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3719 +/- ##
==========================================
- Coverage 89.15% 89.12% -0.03% 
==========================================
Files 156 157 +1 Lines 123837 124125 +288 Branches 123837 124125 +288 ==========================================
+ Hits 110408 110629 +221 - Misses 10754 10807 +53 - Partials 2675 2689 +14 

☔ 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]

🔔 2nd Reminder

Hey @TheBlueMatt@wpaulino! 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]

🔔 1st Reminder

Hey @TheBlueMatt@wpaulino! 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.

[wpaulino]

Good to squash. Also would prefer a more concise commit message as opposed to just quoting a message from Matt.

[TheBlueMatt]

A few nits.

[vincenzopalazzo]

Mh! ok I am trying to go back to this @TheBlueMatt#3719 (comment)

Do you think that having the invoice will bring any benefit for the has function inside the OutboundRoute ? There is any case where the hash will be equal, where the invoice will change the hash instead?

In addition, we would like to keep the odd value in here or changing this to even?

[TheBlueMatt]

I think its best to just not because it violates the Rust API guidelines etc. We shouldn't need to hash the Invoice fields tho, because we can just impl hash to hash the bytes which are the full encoded invoice.

[vincenzopalazzo]

OK, I looked at the code implementation this morning with a fresh mind, and I think I addressed your concern. Thanks for pointing me in the right direction.

[wpaulino]

It doesn't avoid any clones, but we should have pay_route_internal take a reference as that's what it requires.

diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index ce0538931..4affbc2d6 100644
--- a/lightning/src/ln/outbound_payment.rs+++ b/lightning/src/ln/outbound_payment.rs@@ -1068,7 +1068,7 @@ impl OutboundPayments {
core::mem::drop(outbounds);
let result = self.pay_route_internal(
- &route, payment_hash, &recipient_onion, keysend_preimage, invoice_request, Some(bolt12_invoice), payment_id,+ &route, payment_hash, &recipient_onion, keysend_preimage, invoice_request, Some(&bolt12_invoice), payment_id,
Some(route_params.final_value_msat), &onion_session_privs, node_signer, best_block_height,
&send_payment_along_path
);
@@ -1523,7 +1523,7 @@ impl OutboundPayments {
}
};
let res = self.pay_route_internal(&route, payment_hash, &recipient_onion, keysend_preimage,
- invoice_request.as_ref(), bolt12_invoice, payment_id, Some(total_msat), &onion_session_privs, node_signer,+ invoice_request.as_ref(), bolt12_invoice.as_ref(), payment_id, Some(total_msat), &onion_session_privs, node_signer,
best_block_height, &send_payment_along_path);
log_info!(logger, "Result retrying payment id {}: {:?}", &payment_id, res);
if let Err(e) = res {
@@ -1868,7 +1868,7 @@ impl OutboundPayments {
fn pay_route_internal<NS: Deref, F>(
&self, route: &Route, payment_hash: PaymentHash, recipient_onion: &RecipientOnionFields,
- keysend_preimage: Option<PaymentPreimage>, invoice_request: Option<&InvoiceRequest>, bolt12_invoice: Option<PaidBolt12Invoice>,+ keysend_preimage: Option<PaymentPreimage>, invoice_request: Option<&InvoiceRequest>, bolt12_invoice: Option<&PaidBolt12Invoice>,
payment_id: PaymentId, recv_value_msat: Option<u64>, onion_session_privs: &Vec<[u8; 32]>,
node_signer: &NS, best_block_height: u32, send_payment_along_path: &F
) -> Result<(), PaymentSendFailure>
@@ -1924,7 +1924,7 @@ impl OutboundPayments {
let path_res = send_payment_along_path(SendAlongPathArgs {
path: &path, payment_hash: &payment_hash, recipient_onion, total_value,
cur_height, payment_id, keysend_preimage: &keysend_preimage, invoice_request,
- bolt12_invoice: bolt12_invoice.as_ref(),+ bolt12_invoice,
session_priv_bytes: *session_priv_bytes
});
results.push(path_res);

[TheBlueMatt]

Agree with @wpaulino that that nit might be very marginally nicer, but either way this LGTM.

[vincenzopalazzo]

Done, thanks for the review!

[vincenzopalazzo]

Matt noted during the last round of review the following:

Oof. Sorry I missed this until now. This is not, in fact, "only used for retries", we use it on claims only, in fact. If a user is relying on the event field for PoP, what this can mean is that we can initiate a send, restart with a stale ChannelManager, notice the payment is pending, then when it claims fail to provide the invoice (only the preimage) to the payer.
In practice, to fix this, we'll need to include the PaidBolt12Invoice in the HTLCSource::OutboundRoute, I believe.

This commit is trying to store the PaidBolt12Invoice inside the HTLCSource::OutboundRoute, but this is not enough because we have to store the invoice also inside the PendingOutboundPayment.

Fixes: #3714

[ldk-reviews-bot]

👋 I see @wpaulino was un-assigned.
If you'd like another reviewer assignemnt, please click here.

[TheBlueMatt]

Yea, this all makes sense I think. Sorry to make you undo some of the previous patch.

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

[vincenzopalazzo]

Thanks Matt! I rebased on the main to fix the commit checks and addressed some of the review comments. We should be good for another round

[ldk-reviews-bot]

✅ Added second reviewer: @wpaulino

[TheBlueMatt]

Conceptually looks good, but there's still quite a few FIXMEs left, do you want to address those?

[ldk-reviews-bot]

🔔 1st Reminder

Hey @wpaulino! 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.

[vincenzopalazzo]

Jumping in finishing this tomorrow, sorry for the delay!

[codecov]

Codecov Report

Attention: Patch coverage is 72.22222% with 10 lines in your changes missing coverage. Please review.

Project coverage is 89.12%. Comparing base (c6921fa) to head (d6d1c2f).
Report is 44 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/outbound_payment.rs	66.66%	3 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs	80.00%	2 Missing and 1 partial ⚠️
lightning/src/offers/static_invoice.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3719 +/- ##
==========================================
- Coverage 89.15% 89.12% -0.03% 
==========================================
Files 156 157 +1 Lines 123837 124125 +288 Branches 123837 124125 +288 ==========================================
+ Hits 110408 110629 +221 - Misses 10754 10807 +53 - Partials 2675 2689 +14 

☔ 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]

🔔 2nd Reminder

Hey @TheBlueMatt@wpaulino! 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]

🔔 1st Reminder

Hey @TheBlueMatt@wpaulino! 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.

[wpaulino]

Good to squash. Also would prefer a more concise commit message as opposed to just quoting a message from Matt.

[TheBlueMatt]

A few nits.

[vincenzopalazzo]

Mh! ok I am trying to go back to this @TheBlueMatt#3719 (comment)

Do you think that having the invoice will bring any benefit for the has function inside the OutboundRoute ? There is any case where the hash will be equal, where the invoice will change the hash instead?

In addition, we would like to keep the odd value in here or changing this to even?

[TheBlueMatt]

I think its best to just not because it violates the Rust API guidelines etc. We shouldn't need to hash the Invoice fields tho, because we can just impl hash to hash the bytes which are the full encoded invoice.

[vincenzopalazzo]

OK, I looked at the code implementation this morning with a fresh mind, and I think I addressed your concern. Thanks for pointing me in the right direction.

[wpaulino]

It doesn't avoid any clones, but we should have pay_route_internal take a reference as that's what it requires.

diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index ce0538931..4affbc2d6 100644
--- a/lightning/src/ln/outbound_payment.rs+++ b/lightning/src/ln/outbound_payment.rs@@ -1068,7 +1068,7 @@ impl OutboundPayments {
core::mem::drop(outbounds);
let result = self.pay_route_internal(
- &route, payment_hash, &recipient_onion, keysend_preimage, invoice_request, Some(bolt12_invoice), payment_id,+ &route, payment_hash, &recipient_onion, keysend_preimage, invoice_request, Some(&bolt12_invoice), payment_id,
Some(route_params.final_value_msat), &onion_session_privs, node_signer, best_block_height,
&send_payment_along_path
);
@@ -1523,7 +1523,7 @@ impl OutboundPayments {
}
};
let res = self.pay_route_internal(&route, payment_hash, &recipient_onion, keysend_preimage,
- invoice_request.as_ref(), bolt12_invoice, payment_id, Some(total_msat), &onion_session_privs, node_signer,+ invoice_request.as_ref(), bolt12_invoice.as_ref(), payment_id, Some(total_msat), &onion_session_privs, node_signer,
best_block_height, &send_payment_along_path);
log_info!(logger, "Result retrying payment id {}: {:?}", &payment_id, res);
if let Err(e) = res {
@@ -1868,7 +1868,7 @@ impl OutboundPayments {
fn pay_route_internal<NS: Deref, F>(
&self, route: &Route, payment_hash: PaymentHash, recipient_onion: &RecipientOnionFields,
- keysend_preimage: Option<PaymentPreimage>, invoice_request: Option<&InvoiceRequest>, bolt12_invoice: Option<PaidBolt12Invoice>,+ keysend_preimage: Option<PaymentPreimage>, invoice_request: Option<&InvoiceRequest>, bolt12_invoice: Option<&PaidBolt12Invoice>,
payment_id: PaymentId, recv_value_msat: Option<u64>, onion_session_privs: &Vec<[u8; 32]>,
node_signer: &NS, best_block_height: u32, send_payment_along_path: &F
) -> Result<(), PaymentSendFailure>
@@ -1924,7 +1924,7 @@ impl OutboundPayments {
let path_res = send_payment_along_path(SendAlongPathArgs {
path: &path, payment_hash: &payment_hash, recipient_onion, total_value,
cur_height, payment_id, keysend_preimage: &keysend_preimage, invoice_request,
- bolt12_invoice: bolt12_invoice.as_ref(),+ bolt12_invoice,
session_priv_bytes: *session_priv_bytes
});
results.push(path_res);

[TheBlueMatt]

Agree with @wpaulino that that nit might be very marginally nicer, but either way this LGTM.

[vincenzopalazzo]

Done, thanks for the review!

[vincenzopalazzo]

Matt noted during the last round of review the following:

Oof. Sorry I missed this until now. This is not, in fact, "only used for retries", we use it on claims only, in fact. If a user is relying on the event field for PoP, what this can mean is that we can initiate a send, restart with a stale ChannelManager, notice the payment is pending, then when it claims fail to provide the invoice (only the preimage) to the payer.
In practice, to fix this, we'll need to include the PaidBolt12Invoice in the HTLCSource::OutboundRoute, I believe.

This commit is trying to store the PaidBolt12Invoice inside the HTLCSource::OutboundRoute, but this is not enough because we have to store the invoice also inside the PendingOutboundPayment.

Fixes: #3714

[ldk-reviews-bot]

👋 I see @wpaulino was un-assigned.
If you'd like another reviewer assignemnt, please click here.

[TheBlueMatt]

Yea, this all makes sense I think. Sorry to make you undo some of the previous patch.

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

[vincenzopalazzo]

Thanks Matt! I rebased on the main to fix the commit checks and addressed some of the review comments. We should be good for another round

[ldk-reviews-bot]

✅ Added second reviewer: @wpaulino

[TheBlueMatt]

Conceptually looks good, but there's still quite a few FIXMEs left, do you want to address those?

[ldk-reviews-bot]

🔔 1st Reminder

Hey @wpaulino! 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.

[vincenzopalazzo]

Jumping in finishing this tomorrow, sorry for the delay!

[codecov]

Codecov Report

Attention: Patch coverage is 72.22222% with 10 lines in your changes missing coverage. Please review.

Project coverage is 89.12%. Comparing base (c6921fa) to head (d6d1c2f).
Report is 44 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/outbound_payment.rs	66.66%	3 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs	80.00%	2 Missing and 1 partial ⚠️
lightning/src/offers/static_invoice.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3719 +/- ##
==========================================
- Coverage 89.15% 89.12% -0.03% 
==========================================
Files 156 157 +1 Lines 123837 124125 +288 Branches 123837 124125 +288 ==========================================
+ Hits 110408 110629 +221 - Misses 10754 10807 +53 - Partials 2675 2689 +14 

☔ 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]

🔔 2nd Reminder

Hey @TheBlueMatt@wpaulino! 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]

🔔 1st Reminder

Hey @TheBlueMatt@wpaulino! 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.

[wpaulino]

Good to squash. Also would prefer a more concise commit message as opposed to just quoting a message from Matt.

[TheBlueMatt]

A few nits.

[vincenzopalazzo]

Mh! ok I am trying to go back to this @TheBlueMatt#3719 (comment)

Do you think that having the invoice will bring any benefit for the has function inside the OutboundRoute ? There is any case where the hash will be equal, where the invoice will change the hash instead?

In addition, we would like to keep the odd value in here or changing this to even?

[TheBlueMatt]

I think its best to just not because it violates the Rust API guidelines etc. We shouldn't need to hash the Invoice fields tho, because we can just impl hash to hash the bytes which are the full encoded invoice.

[vincenzopalazzo]

OK, I looked at the code implementation this morning with a fresh mind, and I think I addressed your concern. Thanks for pointing me in the right direction.

[wpaulino]

It doesn't avoid any clones, but we should have pay_route_internal take a reference as that's what it requires.

diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index ce0538931..4affbc2d6 100644
--- a/lightning/src/ln/outbound_payment.rs+++ b/lightning/src/ln/outbound_payment.rs@@ -1068,7 +1068,7 @@ impl OutboundPayments {
core::mem::drop(outbounds);
let result = self.pay_route_internal(
- &route, payment_hash, &recipient_onion, keysend_preimage, invoice_request, Some(bolt12_invoice), payment_id,+ &route, payment_hash, &recipient_onion, keysend_preimage, invoice_request, Some(&bolt12_invoice), payment_id,
Some(route_params.final_value_msat), &onion_session_privs, node_signer, best_block_height,
&send_payment_along_path
);
@@ -1523,7 +1523,7 @@ impl OutboundPayments {
}
};
let res = self.pay_route_internal(&route, payment_hash, &recipient_onion, keysend_preimage,
- invoice_request.as_ref(), bolt12_invoice, payment_id, Some(total_msat), &onion_session_privs, node_signer,+ invoice_request.as_ref(), bolt12_invoice.as_ref(), payment_id, Some(total_msat), &onion_session_privs, node_signer,
best_block_height, &send_payment_along_path);
log_info!(logger, "Result retrying payment id {}: {:?}", &payment_id, res);
if let Err(e) = res {
@@ -1868,7 +1868,7 @@ impl OutboundPayments {
fn pay_route_internal<NS: Deref, F>(
&self, route: &Route, payment_hash: PaymentHash, recipient_onion: &RecipientOnionFields,
- keysend_preimage: Option<PaymentPreimage>, invoice_request: Option<&InvoiceRequest>, bolt12_invoice: Option<PaidBolt12Invoice>,+ keysend_preimage: Option<PaymentPreimage>, invoice_request: Option<&InvoiceRequest>, bolt12_invoice: Option<&PaidBolt12Invoice>,
payment_id: PaymentId, recv_value_msat: Option<u64>, onion_session_privs: &Vec<[u8; 32]>,
node_signer: &NS, best_block_height: u32, send_payment_along_path: &F
) -> Result<(), PaymentSendFailure>
@@ -1924,7 +1924,7 @@ impl OutboundPayments {
let path_res = send_payment_along_path(SendAlongPathArgs {
path: &path, payment_hash: &payment_hash, recipient_onion, total_value,
cur_height, payment_id, keysend_preimage: &keysend_preimage, invoice_request,
- bolt12_invoice: bolt12_invoice.as_ref(),+ bolt12_invoice,
session_priv_bytes: *session_priv_bytes
});
results.push(path_res);

[TheBlueMatt]

Agree with @wpaulino that that nit might be very marginally nicer, but either way this LGTM.

[vincenzopalazzo]

Done, thanks for the review!

[vincenzopalazzo]

Matt noted during the last round of review the following:

Oof. Sorry I missed this until now. This is not, in fact, "only used for retries", we use it on claims only, in fact. If a user is relying on the event field for PoP, what this can mean is that we can initiate a send, restart with a stale ChannelManager, notice the payment is pending, then when it claims fail to provide the invoice (only the preimage) to the payer.
In practice, to fix this, we'll need to include the PaidBolt12Invoice in the HTLCSource::OutboundRoute, I believe.

This commit is trying to store the PaidBolt12Invoice inside the HTLCSource::OutboundRoute, but this is not enough because we have to store the invoice also inside the PendingOutboundPayment.

Fixes: #3714

[ldk-reviews-bot]

👋 I see @wpaulino was un-assigned.
If you'd like another reviewer assignemnt, please click here.

[TheBlueMatt]

Yea, this all makes sense I think. Sorry to make you undo some of the previous patch.

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

[vincenzopalazzo]

Thanks Matt! I rebased on the main to fix the commit checks and addressed some of the review comments. We should be good for another round

[ldk-reviews-bot]

✅ Added second reviewer: @wpaulino

[TheBlueMatt]

Conceptually looks good, but there's still quite a few FIXMEs left, do you want to address those?

[ldk-reviews-bot]

🔔 1st Reminder

Hey @wpaulino! 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.

[vincenzopalazzo]

Jumping in finishing this tomorrow, sorry for the delay!

[codecov]

Codecov Report

Attention: Patch coverage is 72.22222% with 10 lines in your changes missing coverage. Please review.

Project coverage is 89.12%. Comparing base (c6921fa) to head (d6d1c2f).
Report is 44 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/outbound_payment.rs	66.66%	3 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs	80.00%	2 Missing and 1 partial ⚠️
lightning/src/offers/static_invoice.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3719 +/- ##
==========================================
- Coverage 89.15% 89.12% -0.03% 
==========================================
Files 156 157 +1 Lines 123837 124125 +288 Branches 123837 124125 +288 ==========================================
+ Hits 110408 110629 +221 - Misses 10754 10807 +53 - Partials 2675 2689 +14 

☔ 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]

🔔 2nd Reminder

Hey @TheBlueMatt@wpaulino! 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]

🔔 1st Reminder

Hey @TheBlueMatt@wpaulino! 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.

[wpaulino]

Good to squash. Also would prefer a more concise commit message as opposed to just quoting a message from Matt.

[TheBlueMatt]

A few nits.

[vincenzopalazzo]

Mh! ok I am trying to go back to this @TheBlueMatt#3719 (comment)

Do you think that having the invoice will bring any benefit for the has function inside the OutboundRoute ? There is any case where the hash will be equal, where the invoice will change the hash instead?

In addition, we would like to keep the odd value in here or changing this to even?

[TheBlueMatt]

I think its best to just not because it violates the Rust API guidelines etc. We shouldn't need to hash the Invoice fields tho, because we can just impl hash to hash the bytes which are the full encoded invoice.

[vincenzopalazzo]

OK, I looked at the code implementation this morning with a fresh mind, and I think I addressed your concern. Thanks for pointing me in the right direction.

[wpaulino]

It doesn't avoid any clones, but we should have pay_route_internal take a reference as that's what it requires.

diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index ce0538931..4affbc2d6 100644
--- a/lightning/src/ln/outbound_payment.rs+++ b/lightning/src/ln/outbound_payment.rs@@ -1068,7 +1068,7 @@ impl OutboundPayments {
core::mem::drop(outbounds);
let result = self.pay_route_internal(
- &route, payment_hash, &recipient_onion, keysend_preimage, invoice_request, Some(bolt12_invoice), payment_id,+ &route, payment_hash, &recipient_onion, keysend_preimage, invoice_request, Some(&bolt12_invoice), payment_id,
Some(route_params.final_value_msat), &onion_session_privs, node_signer, best_block_height,
&send_payment_along_path
);
@@ -1523,7 +1523,7 @@ impl OutboundPayments {
}
};
let res = self.pay_route_internal(&route, payment_hash, &recipient_onion, keysend_preimage,
- invoice_request.as_ref(), bolt12_invoice, payment_id, Some(total_msat), &onion_session_privs, node_signer,+ invoice_request.as_ref(), bolt12_invoice.as_ref(), payment_id, Some(total_msat), &onion_session_privs, node_signer,
best_block_height, &send_payment_along_path);
log_info!(logger, "Result retrying payment id {}: {:?}", &payment_id, res);
if let Err(e) = res {
@@ -1868,7 +1868,7 @@ impl OutboundPayments {
fn pay_route_internal<NS: Deref, F>(
&self, route: &Route, payment_hash: PaymentHash, recipient_onion: &RecipientOnionFields,
- keysend_preimage: Option<PaymentPreimage>, invoice_request: Option<&InvoiceRequest>, bolt12_invoice: Option<PaidBolt12Invoice>,+ keysend_preimage: Option<PaymentPreimage>, invoice_request: Option<&InvoiceRequest>, bolt12_invoice: Option<&PaidBolt12Invoice>,
payment_id: PaymentId, recv_value_msat: Option<u64>, onion_session_privs: &Vec<[u8; 32]>,
node_signer: &NS, best_block_height: u32, send_payment_along_path: &F
) -> Result<(), PaymentSendFailure>
@@ -1924,7 +1924,7 @@ impl OutboundPayments {
let path_res = send_payment_along_path(SendAlongPathArgs {
path: &path, payment_hash: &payment_hash, recipient_onion, total_value,
cur_height, payment_id, keysend_preimage: &keysend_preimage, invoice_request,
- bolt12_invoice: bolt12_invoice.as_ref(),+ bolt12_invoice,
session_priv_bytes: *session_priv_bytes
});
results.push(path_res);

[TheBlueMatt]

Agree with @wpaulino that that nit might be very marginally nicer, but either way this LGTM.

[vincenzopalazzo]

Done, thanks for the review!

[vincenzopalazzo]

Matt noted during the last round of review the following:

Oof. Sorry I missed this until now. This is not, in fact, "only used for retries", we use it on claims only, in fact. If a user is relying on the event field for PoP, what this can mean is that we can initiate a send, restart with a stale ChannelManager, notice the payment is pending, then when it claims fail to provide the invoice (only the preimage) to the payer.
In practice, to fix this, we'll need to include the PaidBolt12Invoice in the HTLCSource::OutboundRoute, I believe.

This commit is trying to store the PaidBolt12Invoice inside the HTLCSource::OutboundRoute, but this is not enough because we have to store the invoice also inside the PendingOutboundPayment.

Fixes: #3714

[ldk-reviews-bot]

👋 I see @wpaulino was un-assigned.
If you'd like another reviewer assignemnt, please click here.

[TheBlueMatt]

Yea, this all makes sense I think. Sorry to make you undo some of the previous patch.

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

[vincenzopalazzo]

Thanks Matt! I rebased on the main to fix the commit checks and addressed some of the review comments. We should be good for another round

[ldk-reviews-bot]

✅ Added second reviewer: @wpaulino

[TheBlueMatt]

Conceptually looks good, but there's still quite a few FIXMEs left, do you want to address those?

[ldk-reviews-bot]

🔔 1st Reminder

Hey @wpaulino! 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.

[vincenzopalazzo]

Jumping in finishing this tomorrow, sorry for the delay!

[codecov]

Codecov Report

Attention: Patch coverage is 72.22222% with 10 lines in your changes missing coverage. Please review.

Project coverage is 89.12%. Comparing base (c6921fa) to head (d6d1c2f).
Report is 44 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/outbound_payment.rs	66.66%	3 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs	80.00%	2 Missing and 1 partial ⚠️
lightning/src/offers/static_invoice.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3719 +/- ##
==========================================
- Coverage 89.15% 89.12% -0.03% 
==========================================
Files 156 157 +1 Lines 123837 124125 +288 Branches 123837 124125 +288 ==========================================
+ Hits 110408 110629 +221 - Misses 10754 10807 +53 - Partials 2675 2689 +14 

☔ 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]

🔔 2nd Reminder

Hey @TheBlueMatt@wpaulino! 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]

🔔 1st Reminder

Hey @TheBlueMatt@wpaulino! 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.

[wpaulino]

Good to squash. Also would prefer a more concise commit message as opposed to just quoting a message from Matt.

[TheBlueMatt]

A few nits.

[vincenzopalazzo]

Mh! ok I am trying to go back to this @TheBlueMatt#3719 (comment)

Do you think that having the invoice will bring any benefit for the has function inside the OutboundRoute ? There is any case where the hash will be equal, where the invoice will change the hash instead?

In addition, we would like to keep the odd value in here or changing this to even?

[TheBlueMatt]

I think its best to just not because it violates the Rust API guidelines etc. We shouldn't need to hash the Invoice fields tho, because we can just impl hash to hash the bytes which are the full encoded invoice.

[vincenzopalazzo]

OK, I looked at the code implementation this morning with a fresh mind, and I think I addressed your concern. Thanks for pointing me in the right direction.

[wpaulino]

It doesn't avoid any clones, but we should have pay_route_internal take a reference as that's what it requires.

diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index ce0538931..4affbc2d6 100644
--- a/lightning/src/ln/outbound_payment.rs+++ b/lightning/src/ln/outbound_payment.rs@@ -1068,7 +1068,7 @@ impl OutboundPayments {
core::mem::drop(outbounds);
let result = self.pay_route_internal(
- &route, payment_hash, &recipient_onion, keysend_preimage, invoice_request, Some(bolt12_invoice), payment_id,+ &route, payment_hash, &recipient_onion, keysend_preimage, invoice_request, Some(&bolt12_invoice), payment_id,
Some(route_params.final_value_msat), &onion_session_privs, node_signer, best_block_height,
&send_payment_along_path
);
@@ -1523,7 +1523,7 @@ impl OutboundPayments {
}
};
let res = self.pay_route_internal(&route, payment_hash, &recipient_onion, keysend_preimage,
- invoice_request.as_ref(), bolt12_invoice, payment_id, Some(total_msat), &onion_session_privs, node_signer,+ invoice_request.as_ref(), bolt12_invoice.as_ref(), payment_id, Some(total_msat), &onion_session_privs, node_signer,
best_block_height, &send_payment_along_path);
log_info!(logger, "Result retrying payment id {}: {:?}", &payment_id, res);
if let Err(e) = res {
@@ -1868,7 +1868,7 @@ impl OutboundPayments {
fn pay_route_internal<NS: Deref, F>(
&self, route: &Route, payment_hash: PaymentHash, recipient_onion: &RecipientOnionFields,
- keysend_preimage: Option<PaymentPreimage>, invoice_request: Option<&InvoiceRequest>, bolt12_invoice: Option<PaidBolt12Invoice>,+ keysend_preimage: Option<PaymentPreimage>, invoice_request: Option<&InvoiceRequest>, bolt12_invoice: Option<&PaidBolt12Invoice>,
payment_id: PaymentId, recv_value_msat: Option<u64>, onion_session_privs: &Vec<[u8; 32]>,
node_signer: &NS, best_block_height: u32, send_payment_along_path: &F
) -> Result<(), PaymentSendFailure>
@@ -1924,7 +1924,7 @@ impl OutboundPayments {
let path_res = send_payment_along_path(SendAlongPathArgs {
path: &path, payment_hash: &payment_hash, recipient_onion, total_value,
cur_height, payment_id, keysend_preimage: &keysend_preimage, invoice_request,
- bolt12_invoice: bolt12_invoice.as_ref(),+ bolt12_invoice,
session_priv_bytes: *session_priv_bytes
});
results.push(path_res);

[TheBlueMatt]

Agree with @wpaulino that that nit might be very marginally nicer, but either way this LGTM.

[vincenzopalazzo]

Done, thanks for the review!

[vincenzopalazzo]

Matt noted during the last round of review the following:

Oof. Sorry I missed this until now. This is not, in fact, "only used for retries", we use it on claims only, in fact. If a user is relying on the event field for PoP, what this can mean is that we can initiate a send, restart with a stale ChannelManager, notice the payment is pending, then when it claims fail to provide the invoice (only the preimage) to the payer.
In practice, to fix this, we'll need to include the PaidBolt12Invoice in the HTLCSource::OutboundRoute, I believe.

This commit is trying to store the PaidBolt12Invoice inside the HTLCSource::OutboundRoute, but this is not enough because we have to store the invoice also inside the PendingOutboundPayment.

Fixes: #3714

[ldk-reviews-bot]

👋 I see @wpaulino was un-assigned.
If you'd like another reviewer assignemnt, please click here.

[TheBlueMatt]

Yea, this all makes sense I think. Sorry to make you undo some of the previous patch.

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

[vincenzopalazzo]

Thanks Matt! I rebased on the main to fix the commit checks and addressed some of the review comments. We should be good for another round

[ldk-reviews-bot]

✅ Added second reviewer: @wpaulino

[TheBlueMatt]

Conceptually looks good, but there's still quite a few FIXMEs left, do you want to address those?

[ldk-reviews-bot]

🔔 1st Reminder

Hey @wpaulino! 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.

[vincenzopalazzo]

Jumping in finishing this tomorrow, sorry for the delay!

[codecov]

Codecov Report

Attention: Patch coverage is 72.22222% with 10 lines in your changes missing coverage. Please review.

Project coverage is 89.12%. Comparing base (c6921fa) to head (d6d1c2f).
Report is 44 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/outbound_payment.rs	66.66%	3 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs	80.00%	2 Missing and 1 partial ⚠️
lightning/src/offers/static_invoice.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3719 +/- ##
==========================================
- Coverage 89.15% 89.12% -0.03% 
==========================================
Files 156 157 +1 Lines 123837 124125 +288 Branches 123837 124125 +288 ==========================================
+ Hits 110408 110629 +221 - Misses 10754 10807 +53 - Partials 2675 2689 +14 

☔ 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]

🔔 2nd Reminder

Hey @TheBlueMatt@wpaulino! 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]

🔔 1st Reminder

Hey @TheBlueMatt@wpaulino! 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.

[wpaulino]

Good to squash. Also would prefer a more concise commit message as opposed to just quoting a message from Matt.

[TheBlueMatt]

A few nits.

[vincenzopalazzo]

Mh! ok I am trying to go back to this @TheBlueMatt#3719 (comment)

Do you think that having the invoice will bring any benefit for the has function inside the OutboundRoute ? There is any case where the hash will be equal, where the invoice will change the hash instead?

In addition, we would like to keep the odd value in here or changing this to even?

[TheBlueMatt]

I think its best to just not because it violates the Rust API guidelines etc. We shouldn't need to hash the Invoice fields tho, because we can just impl hash to hash the bytes which are the full encoded invoice.

[vincenzopalazzo]

OK, I looked at the code implementation this morning with a fresh mind, and I think I addressed your concern. Thanks for pointing me in the right direction.

[wpaulino]

It doesn't avoid any clones, but we should have pay_route_internal take a reference as that's what it requires.

diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index ce0538931..4affbc2d6 100644
--- a/lightning/src/ln/outbound_payment.rs+++ b/lightning/src/ln/outbound_payment.rs@@ -1068,7 +1068,7 @@ impl OutboundPayments {
core::mem::drop(outbounds);
let result = self.pay_route_internal(
- &route, payment_hash, &recipient_onion, keysend_preimage, invoice_request, Some(bolt12_invoice), payment_id,+ &route, payment_hash, &recipient_onion, keysend_preimage, invoice_request, Some(&bolt12_invoice), payment_id,
Some(route_params.final_value_msat), &onion_session_privs, node_signer, best_block_height,
&send_payment_along_path
);
@@ -1523,7 +1523,7 @@ impl OutboundPayments {
}
};
let res = self.pay_route_internal(&route, payment_hash, &recipient_onion, keysend_preimage,
- invoice_request.as_ref(), bolt12_invoice, payment_id, Some(total_msat), &onion_session_privs, node_signer,+ invoice_request.as_ref(), bolt12_invoice.as_ref(), payment_id, Some(total_msat), &onion_session_privs, node_signer,
best_block_height, &send_payment_along_path);
log_info!(logger, "Result retrying payment id {}: {:?}", &payment_id, res);
if let Err(e) = res {
@@ -1868,7 +1868,7 @@ impl OutboundPayments {
fn pay_route_internal<NS: Deref, F>(
&self, route: &Route, payment_hash: PaymentHash, recipient_onion: &RecipientOnionFields,
- keysend_preimage: Option<PaymentPreimage>, invoice_request: Option<&InvoiceRequest>, bolt12_invoice: Option<PaidBolt12Invoice>,+ keysend_preimage: Option<PaymentPreimage>, invoice_request: Option<&InvoiceRequest>, bolt12_invoice: Option<&PaidBolt12Invoice>,
payment_id: PaymentId, recv_value_msat: Option<u64>, onion_session_privs: &Vec<[u8; 32]>,
node_signer: &NS, best_block_height: u32, send_payment_along_path: &F
) -> Result<(), PaymentSendFailure>
@@ -1924,7 +1924,7 @@ impl OutboundPayments {
let path_res = send_payment_along_path(SendAlongPathArgs {
path: &path, payment_hash: &payment_hash, recipient_onion, total_value,
cur_height, payment_id, keysend_preimage: &keysend_preimage, invoice_request,
- bolt12_invoice: bolt12_invoice.as_ref(),+ bolt12_invoice,
session_priv_bytes: *session_priv_bytes
});
results.push(path_res);

[TheBlueMatt]

Agree with @wpaulino that that nit might be very marginally nicer, but either way this LGTM.

[vincenzopalazzo]

Done, thanks for the review!

[vincenzopalazzo]

Matt noted during the last round of review the following:

Oof. Sorry I missed this until now. This is not, in fact, "only used for retries", we use it on claims only, in fact. If a user is relying on the event field for PoP, what this can mean is that we can initiate a send, restart with a stale ChannelManager, notice the payment is pending, then when it claims fail to provide the invoice (only the preimage) to the payer.
In practice, to fix this, we'll need to include the PaidBolt12Invoice in the HTLCSource::OutboundRoute, I believe.

This commit is trying to store the PaidBolt12Invoice inside the HTLCSource::OutboundRoute, but this is not enough because we have to store the invoice also inside the PendingOutboundPayment.

Fixes: #3714

[ldk-reviews-bot]

👋 I see @wpaulino was un-assigned.
If you'd like another reviewer assignemnt, please click here.

[TheBlueMatt]

Yea, this all makes sense I think. Sorry to make you undo some of the previous patch.

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

[vincenzopalazzo]

Thanks Matt! I rebased on the main to fix the commit checks and addressed some of the review comments. We should be good for another round

[ldk-reviews-bot]

✅ Added second reviewer: @wpaulino

[TheBlueMatt]

Conceptually looks good, but there's still quite a few FIXMEs left, do you want to address those?

[ldk-reviews-bot]

🔔 1st Reminder

Hey @wpaulino! 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.

[vincenzopalazzo]

Jumping in finishing this tomorrow, sorry for the delay!

[codecov]

Codecov Report

Attention: Patch coverage is 72.22222% with 10 lines in your changes missing coverage. Please review.

Project coverage is 89.12%. Comparing base (c6921fa) to head (d6d1c2f).
Report is 44 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/outbound_payment.rs	66.66%	3 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs	80.00%	2 Missing and 1 partial ⚠️
lightning/src/offers/static_invoice.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3719 +/- ##
==========================================
- Coverage 89.15% 89.12% -0.03% 
==========================================
Files 156 157 +1 Lines 123837 124125 +288 Branches 123837 124125 +288 ==========================================
+ Hits 110408 110629 +221 - Misses 10754 10807 +53 - Partials 2675 2689 +14 

☔ 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]

🔔 2nd Reminder

Hey @TheBlueMatt@wpaulino! 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]

🔔 1st Reminder

Hey @TheBlueMatt@wpaulino! 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.

[wpaulino]

Good to squash. Also would prefer a more concise commit message as opposed to just quoting a message from Matt.

[TheBlueMatt]

A few nits.

[vincenzopalazzo]

Mh! ok I am trying to go back to this @TheBlueMatt#3719 (comment)

Do you think that having the invoice will bring any benefit for the has function inside the OutboundRoute ? There is any case where the hash will be equal, where the invoice will change the hash instead?

In addition, we would like to keep the odd value in here or changing this to even?

[TheBlueMatt]

I think its best to just not because it violates the Rust API guidelines etc. We shouldn't need to hash the Invoice fields tho, because we can just impl hash to hash the bytes which are the full encoded invoice.

[vincenzopalazzo]

OK, I looked at the code implementation this morning with a fresh mind, and I think I addressed your concern. Thanks for pointing me in the right direction.

[wpaulino]

It doesn't avoid any clones, but we should have pay_route_internal take a reference as that's what it requires.

diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index ce0538931..4affbc2d6 100644
--- a/lightning/src/ln/outbound_payment.rs+++ b/lightning/src/ln/outbound_payment.rs@@ -1068,7 +1068,7 @@ impl OutboundPayments {
core::mem::drop(outbounds);
let result = self.pay_route_internal(
- &route, payment_hash, &recipient_onion, keysend_preimage, invoice_request, Some(bolt12_invoice), payment_id,+ &route, payment_hash, &recipient_onion, keysend_preimage, invoice_request, Some(&bolt12_invoice), payment_id,
Some(route_params.final_value_msat), &onion_session_privs, node_signer, best_block_height,
&send_payment_along_path
);
@@ -1523,7 +1523,7 @@ impl OutboundPayments {
}
};
let res = self.pay_route_internal(&route, payment_hash, &recipient_onion, keysend_preimage,
- invoice_request.as_ref(), bolt12_invoice, payment_id, Some(total_msat), &onion_session_privs, node_signer,+ invoice_request.as_ref(), bolt12_invoice.as_ref(), payment_id, Some(total_msat), &onion_session_privs, node_signer,
best_block_height, &send_payment_along_path);
log_info!(logger, "Result retrying payment id {}: {:?}", &payment_id, res);
if let Err(e) = res {
@@ -1868,7 +1868,7 @@ impl OutboundPayments {
fn pay_route_internal<NS: Deref, F>(
&self, route: &Route, payment_hash: PaymentHash, recipient_onion: &RecipientOnionFields,
- keysend_preimage: Option<PaymentPreimage>, invoice_request: Option<&InvoiceRequest>, bolt12_invoice: Option<PaidBolt12Invoice>,+ keysend_preimage: Option<PaymentPreimage>, invoice_request: Option<&InvoiceRequest>, bolt12_invoice: Option<&PaidBolt12Invoice>,
payment_id: PaymentId, recv_value_msat: Option<u64>, onion_session_privs: &Vec<[u8; 32]>,
node_signer: &NS, best_block_height: u32, send_payment_along_path: &F
) -> Result<(), PaymentSendFailure>
@@ -1924,7 +1924,7 @@ impl OutboundPayments {
let path_res = send_payment_along_path(SendAlongPathArgs {
path: &path, payment_hash: &payment_hash, recipient_onion, total_value,
cur_height, payment_id, keysend_preimage: &keysend_preimage, invoice_request,
- bolt12_invoice: bolt12_invoice.as_ref(),+ bolt12_invoice,
session_priv_bytes: *session_priv_bytes
});
results.push(path_res);

[TheBlueMatt]

Agree with @wpaulino that that nit might be very marginally nicer, but either way this LGTM.

[vincenzopalazzo]

Done, thanks for the review!

[vincenzopalazzo]

Matt noted during the last round of review the following:

Oof. Sorry I missed this until now. This is not, in fact, "only used for retries", we use it on claims only, in fact. If a user is relying on the event field for PoP, what this can mean is that we can initiate a send, restart with a stale ChannelManager, notice the payment is pending, then when it claims fail to provide the invoice (only the preimage) to the payer.
In practice, to fix this, we'll need to include the PaidBolt12Invoice in the HTLCSource::OutboundRoute, I believe.

This commit is trying to store the PaidBolt12Invoice inside the HTLCSource::OutboundRoute, but this is not enough because we have to store the invoice also inside the PendingOutboundPayment.

Fixes: #3714

[ldk-reviews-bot]

👋 I see @wpaulino was un-assigned.
If you'd like another reviewer assignemnt, please click here.

[TheBlueMatt]

Yea, this all makes sense I think. Sorry to make you undo some of the previous patch.

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

[vincenzopalazzo]

Thanks Matt! I rebased on the main to fix the commit checks and addressed some of the review comments. We should be good for another round

[ldk-reviews-bot]

✅ Added second reviewer: @wpaulino

[TheBlueMatt]

Conceptually looks good, but there's still quite a few FIXMEs left, do you want to address those?

[ldk-reviews-bot]

🔔 1st Reminder

Hey @wpaulino! 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.

[vincenzopalazzo]

Jumping in finishing this tomorrow, sorry for the delay!

[codecov]

Codecov Report

Attention: Patch coverage is 72.22222% with 10 lines in your changes missing coverage. Please review.

Project coverage is 89.12%. Comparing base (c6921fa) to head (d6d1c2f).
Report is 44 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/outbound_payment.rs	66.66%	3 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs	80.00%	2 Missing and 1 partial ⚠️
lightning/src/offers/static_invoice.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3719 +/- ##
==========================================
- Coverage 89.15% 89.12% -0.03% 
==========================================
Files 156 157 +1 Lines 123837 124125 +288 Branches 123837 124125 +288 ==========================================
+ Hits 110408 110629 +221 - Misses 10754 10807 +53 - Partials 2675 2689 +14 

☔ 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]

🔔 2nd Reminder

Hey @TheBlueMatt@wpaulino! 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]

🔔 1st Reminder

Hey @TheBlueMatt@wpaulino! 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.

[wpaulino]

Good to squash. Also would prefer a more concise commit message as opposed to just quoting a message from Matt.

[TheBlueMatt]

A few nits.

[vincenzopalazzo]

Mh! ok I am trying to go back to this @TheBlueMatt#3719 (comment)

Do you think that having the invoice will bring any benefit for the has function inside the OutboundRoute ? There is any case where the hash will be equal, where the invoice will change the hash instead?

In addition, we would like to keep the odd value in here or changing this to even?

[TheBlueMatt]

I think its best to just not because it violates the Rust API guidelines etc. We shouldn't need to hash the Invoice fields tho, because we can just impl hash to hash the bytes which are the full encoded invoice.

[vincenzopalazzo]

OK, I looked at the code implementation this morning with a fresh mind, and I think I addressed your concern. Thanks for pointing me in the right direction.

[wpaulino]

It doesn't avoid any clones, but we should have pay_route_internal take a reference as that's what it requires.

diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index ce0538931..4affbc2d6 100644
--- a/lightning/src/ln/outbound_payment.rs+++ b/lightning/src/ln/outbound_payment.rs@@ -1068,7 +1068,7 @@ impl OutboundPayments {
core::mem::drop(outbounds);
let result = self.pay_route_internal(
- &route, payment_hash, &recipient_onion, keysend_preimage, invoice_request, Some(bolt12_invoice), payment_id,+ &route, payment_hash, &recipient_onion, keysend_preimage, invoice_request, Some(&bolt12_invoice), payment_id,
Some(route_params.final_value_msat), &onion_session_privs, node_signer, best_block_height,
&send_payment_along_path
);
@@ -1523,7 +1523,7 @@ impl OutboundPayments {
}
};
let res = self.pay_route_internal(&route, payment_hash, &recipient_onion, keysend_preimage,
- invoice_request.as_ref(), bolt12_invoice, payment_id, Some(total_msat), &onion_session_privs, node_signer,+ invoice_request.as_ref(), bolt12_invoice.as_ref(), payment_id, Some(total_msat), &onion_session_privs, node_signer,
best_block_height, &send_payment_along_path);
log_info!(logger, "Result retrying payment id {}: {:?}", &payment_id, res);
if let Err(e) = res {
@@ -1868,7 +1868,7 @@ impl OutboundPayments {
fn pay_route_internal<NS: Deref, F>(
&self, route: &Route, payment_hash: PaymentHash, recipient_onion: &RecipientOnionFields,
- keysend_preimage: Option<PaymentPreimage>, invoice_request: Option<&InvoiceRequest>, bolt12_invoice: Option<PaidBolt12Invoice>,+ keysend_preimage: Option<PaymentPreimage>, invoice_request: Option<&InvoiceRequest>, bolt12_invoice: Option<&PaidBolt12Invoice>,
payment_id: PaymentId, recv_value_msat: Option<u64>, onion_session_privs: &Vec<[u8; 32]>,
node_signer: &NS, best_block_height: u32, send_payment_along_path: &F
) -> Result<(), PaymentSendFailure>
@@ -1924,7 +1924,7 @@ impl OutboundPayments {
let path_res = send_payment_along_path(SendAlongPathArgs {
path: &path, payment_hash: &payment_hash, recipient_onion, total_value,
cur_height, payment_id, keysend_preimage: &keysend_preimage, invoice_request,
- bolt12_invoice: bolt12_invoice.as_ref(),+ bolt12_invoice,
session_priv_bytes: *session_priv_bytes
});
results.push(path_res);

[TheBlueMatt]

Agree with @wpaulino that that nit might be very marginally nicer, but either way this LGTM.

[vincenzopalazzo]

Done, thanks for the review!