Attributable failures prefactor by joostjager · Pull Request #3650 · lightningdevkit/rust-lightning

joostjager · 2025-03-06T14:59:16Z

Preparatory non-functional commits extracted from #3611

ldk-reviews-bot · 2025-03-06T14:59:19Z

👋 Thanks for assigning @arik-so 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.

joostjager · 2025-03-06T22:08:49Z

Rebased

ldk-reviews-bot · 2025-03-06T23:43:33Z

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

codecov · 2025-03-07T07:42:27Z

Codecov Report

Attention: Patch coverage is 86.33540% with 22 lines in your changes missing coverage. Please review.

Project coverage is 89.22%. Comparing base (4c43a5b) to head (1426197).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	31.81%	2 Missing and 13 partials ⚠️
lightning/src/ln/onion_utils.rs	92.30%	4 Missing and 2 partials ⚠️
lightning/src/ln/onion_payment.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3650 +/- ##
==========================================
- Coverage 89.24% 89.22% -0.02% 
==========================================
Files 155 155 Lines 119280 119314 +34 Branches 119280 119314 +34 ==========================================
+ Hits 106446 106463 +17 - Misses 10240 10248 +8 - Partials 2594 2603 +9

☔ 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 · 2025-03-08T15:00:02Z

🔔 1st Reminder

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

arik-so

re-acking

TheBlueMatt · 2025-03-10T12:52:09Z

 	packet.hmac = Hmac::from_engine(hmac).to_byte_array();

-	packet
+OnionErrorPacket { data: packet.encode() }


Bleh, it seems pretty weird that we now have OnionErrorPackets flying around (at least outside of onion_utils.rs) that are both decrypted and encrypted. I think the reason it was written the way it was previously was specifically to avoid that - holding the encoded bytes until we get around to encrypting, then making it an OnionErrorPacket. It seems much too easy now to double-en/decrypt or forget to en/decrypt.

I think a similar situation already existed for intermediate nodes? In
rust-lightning/lightning/src/ln/onion_utils.rs
Line 1389 in 4c43a5b
encrypt_failure_packet(incoming_packet_shared_secret,&err.data)
, an OnionErrorPacket received from downstream is encrypted into again an OnionErrorPacket.
The way I see it is that there's "data" in the packet that needs to be encrypted, and it doesn't matter whether that data is 0, 1, 2, etc times encrypted already.

Open to alternatives of course. Were you thinking of pulling the encrypt step into build_failure_packet too?

Yea, certainly not saying the old code was perfect and indeed its already the case that things are encrypted multiple times. I guess I just saw OnionErrorPacket as "encrypted thing ready to go over the wire/received on the wire". Certainly stuff internal to onion_utils doesn't matter quite as much, but once it leaves onion_utils.rs it looks like it'd be really easy to have code that just forgets to encrypt/decrypt (ie the callsite would just read build_failure_packet and the fact that its unencrypted isn't obvious).
Pulling the encrypt step into build_failure_packet seems like one perfectly good way to address it.

Done. Just kept a non-public unencrypted version of build_failure_packet for the bolt test vector test.

TheBlueMatt · 2025-03-10T18:19:14Z

-	shared_secret: &[u8], raw_packet: &[u8],
-) -> msgs::OnionErrorPacket {
+/// Encrypts/decrypts a failure packet.
+pub(super) fn crypt_failure_packet(shared_secret: &[u8], packet: &mut OnionErrorPacket) {


Does this need to remain public now?

It's used in onion_route_tests.rs - I think there is no other way to make it available there, or is there?

Often we do a test wrapper like the below. For four LoC we could even just copy them into the tests, though.
#[cfg(test)] pub fn test_crypt_failure_packet(shared_secret, packet) { crypt_failure_packet(shared_secret, packet) }

Well, it's about testing that function also, so wouldn't want to copy it.
Created the test wrapper.

TheBlueMatt

This LGTM once fixups are squashed.

Prepares for extending OnionErrorPacket with attribution data.

This commits prepares the persistence layer for the addition of attribution data. Changes: - Expand InboundHTLCRemovalReason serialization instead of using the macro. When attribution data is added, it can't just be serialized along with the existing fields because it would break backwards compatibility. Instead the new field needs to go into the tlv block. - Stop using OnionErrorPacket in the UpdateFailHTLC message. When attribution data is added to OnionErrorPacket, it would not be serialized for the wire properly because also here the new field needs to go in the tlv extension of the message. - Prepare HTLCFailReasonRepr serialization for that addition of attribution data. Co-authored-by: Matt Corallo <git@bluematt.me>

joostjager · 2025-03-06T14:59:16Z

Preparatory non-functional commits extracted from #3611

ldk-reviews-bot · 2025-03-06T14:59:19Z

👋 Thanks for assigning @arik-so 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.

joostjager · 2025-03-06T22:08:49Z

Rebased

ldk-reviews-bot · 2025-03-06T23:43:33Z

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

codecov · 2025-03-07T07:42:27Z

Codecov Report

Attention: Patch coverage is 86.33540% with 22 lines in your changes missing coverage. Please review.

Project coverage is 89.22%. Comparing base (4c43a5b) to head (1426197).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	31.81%	2 Missing and 13 partials ⚠️
lightning/src/ln/onion_utils.rs	92.30%	4 Missing and 2 partials ⚠️
lightning/src/ln/onion_payment.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3650 +/- ##
==========================================
- Coverage 89.24% 89.22% -0.02% 
==========================================
Files 155 155 Lines 119280 119314 +34 Branches 119280 119314 +34 ==========================================
+ Hits 106446 106463 +17 - Misses 10240 10248 +8 - Partials 2594 2603 +9

☔ 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 · 2025-03-08T15:00:02Z

🔔 1st Reminder

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

arik-so

re-acking

TheBlueMatt · 2025-03-10T12:52:09Z

 	packet.hmac = Hmac::from_engine(hmac).to_byte_array();

-	packet
+OnionErrorPacket { data: packet.encode() }


Bleh, it seems pretty weird that we now have OnionErrorPackets flying around (at least outside of onion_utils.rs) that are both decrypted and encrypted. I think the reason it was written the way it was previously was specifically to avoid that - holding the encoded bytes until we get around to encrypting, then making it an OnionErrorPacket. It seems much too easy now to double-en/decrypt or forget to en/decrypt.

I think a similar situation already existed for intermediate nodes? In
rust-lightning/lightning/src/ln/onion_utils.rs
Line 1389 in 4c43a5b
encrypt_failure_packet(incoming_packet_shared_secret,&err.data)
, an OnionErrorPacket received from downstream is encrypted into again an OnionErrorPacket.
The way I see it is that there's "data" in the packet that needs to be encrypted, and it doesn't matter whether that data is 0, 1, 2, etc times encrypted already.

Open to alternatives of course. Were you thinking of pulling the encrypt step into build_failure_packet too?

Yea, certainly not saying the old code was perfect and indeed its already the case that things are encrypted multiple times. I guess I just saw OnionErrorPacket as "encrypted thing ready to go over the wire/received on the wire". Certainly stuff internal to onion_utils doesn't matter quite as much, but once it leaves onion_utils.rs it looks like it'd be really easy to have code that just forgets to encrypt/decrypt (ie the callsite would just read build_failure_packet and the fact that its unencrypted isn't obvious).
Pulling the encrypt step into build_failure_packet seems like one perfectly good way to address it.

Done. Just kept a non-public unencrypted version of build_failure_packet for the bolt test vector test.

TheBlueMatt · 2025-03-10T18:19:14Z

-	shared_secret: &[u8], raw_packet: &[u8],
-) -> msgs::OnionErrorPacket {
+/// Encrypts/decrypts a failure packet.
+pub(super) fn crypt_failure_packet(shared_secret: &[u8], packet: &mut OnionErrorPacket) {


Does this need to remain public now?

It's used in onion_route_tests.rs - I think there is no other way to make it available there, or is there?

Often we do a test wrapper like the below. For four LoC we could even just copy them into the tests, though.
#[cfg(test)] pub fn test_crypt_failure_packet(shared_secret, packet) { crypt_failure_packet(shared_secret, packet) }

Well, it's about testing that function also, so wouldn't want to copy it.
Created the test wrapper.

TheBlueMatt

This LGTM once fixups are squashed.

Prepares for extending OnionErrorPacket with attribution data.

This commits prepares the persistence layer for the addition of attribution data. Changes: - Expand InboundHTLCRemovalReason serialization instead of using the macro. When attribution data is added, it can't just be serialized along with the existing fields because it would break backwards compatibility. Instead the new field needs to go into the tlv block. - Stop using OnionErrorPacket in the UpdateFailHTLC message. When attribution data is added to OnionErrorPacket, it would not be serialized for the wire properly because also here the new field needs to go in the tlv extension of the message. - Prepare HTLCFailReasonRepr serialization for that addition of attribution data. Co-authored-by: Matt Corallo <git@bluematt.me>

joostjager · 2025-03-06T14:59:16Z

Preparatory non-functional commits extracted from #3611

ldk-reviews-bot · 2025-03-06T14:59:19Z

👋 Thanks for assigning @arik-so 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.

joostjager · 2025-03-06T22:08:49Z

Rebased

ldk-reviews-bot · 2025-03-06T23:43:33Z

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

codecov · 2025-03-07T07:42:27Z

Codecov Report

Attention: Patch coverage is 86.33540% with 22 lines in your changes missing coverage. Please review.

Project coverage is 89.22%. Comparing base (4c43a5b) to head (1426197).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	31.81%	2 Missing and 13 partials ⚠️
lightning/src/ln/onion_utils.rs	92.30%	4 Missing and 2 partials ⚠️
lightning/src/ln/onion_payment.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3650 +/- ##
==========================================
- Coverage 89.24% 89.22% -0.02% 
==========================================
Files 155 155 Lines 119280 119314 +34 Branches 119280 119314 +34 ==========================================
+ Hits 106446 106463 +17 - Misses 10240 10248 +8 - Partials 2594 2603 +9

☔ 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 · 2025-03-08T15:00:02Z

🔔 1st Reminder

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

arik-so

re-acking

TheBlueMatt · 2025-03-10T12:52:09Z

 	packet.hmac = Hmac::from_engine(hmac).to_byte_array();

-	packet
+OnionErrorPacket { data: packet.encode() }


Bleh, it seems pretty weird that we now have OnionErrorPackets flying around (at least outside of onion_utils.rs) that are both decrypted and encrypted. I think the reason it was written the way it was previously was specifically to avoid that - holding the encoded bytes until we get around to encrypting, then making it an OnionErrorPacket. It seems much too easy now to double-en/decrypt or forget to en/decrypt.

I think a similar situation already existed for intermediate nodes? In
rust-lightning/lightning/src/ln/onion_utils.rs
Line 1389 in 4c43a5b
encrypt_failure_packet(incoming_packet_shared_secret,&err.data)
, an OnionErrorPacket received from downstream is encrypted into again an OnionErrorPacket.
The way I see it is that there's "data" in the packet that needs to be encrypted, and it doesn't matter whether that data is 0, 1, 2, etc times encrypted already.

Open to alternatives of course. Were you thinking of pulling the encrypt step into build_failure_packet too?

Yea, certainly not saying the old code was perfect and indeed its already the case that things are encrypted multiple times. I guess I just saw OnionErrorPacket as "encrypted thing ready to go over the wire/received on the wire". Certainly stuff internal to onion_utils doesn't matter quite as much, but once it leaves onion_utils.rs it looks like it'd be really easy to have code that just forgets to encrypt/decrypt (ie the callsite would just read build_failure_packet and the fact that its unencrypted isn't obvious).
Pulling the encrypt step into build_failure_packet seems like one perfectly good way to address it.

Done. Just kept a non-public unencrypted version of build_failure_packet for the bolt test vector test.

TheBlueMatt · 2025-03-10T18:19:14Z

-	shared_secret: &[u8], raw_packet: &[u8],
-) -> msgs::OnionErrorPacket {
+/// Encrypts/decrypts a failure packet.
+pub(super) fn crypt_failure_packet(shared_secret: &[u8], packet: &mut OnionErrorPacket) {


Does this need to remain public now?

It's used in onion_route_tests.rs - I think there is no other way to make it available there, or is there?

Often we do a test wrapper like the below. For four LoC we could even just copy them into the tests, though.
#[cfg(test)] pub fn test_crypt_failure_packet(shared_secret, packet) { crypt_failure_packet(shared_secret, packet) }

Well, it's about testing that function also, so wouldn't want to copy it.
Created the test wrapper.

TheBlueMatt

This LGTM once fixups are squashed.

Prepares for extending OnionErrorPacket with attribution data.

This commits prepares the persistence layer for the addition of attribution data. Changes: - Expand InboundHTLCRemovalReason serialization instead of using the macro. When attribution data is added, it can't just be serialized along with the existing fields because it would break backwards compatibility. Instead the new field needs to go into the tlv block. - Stop using OnionErrorPacket in the UpdateFailHTLC message. When attribution data is added to OnionErrorPacket, it would not be serialized for the wire properly because also here the new field needs to go in the tlv extension of the message. - Prepare HTLCFailReasonRepr serialization for that addition of attribution data. Co-authored-by: Matt Corallo <git@bluematt.me>

joostjager · 2025-03-06T14:59:16Z

Preparatory non-functional commits extracted from #3611

ldk-reviews-bot · 2025-03-06T14:59:19Z

👋 Thanks for assigning @arik-so 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.

joostjager · 2025-03-06T22:08:49Z

Rebased

ldk-reviews-bot · 2025-03-06T23:43:33Z

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

codecov · 2025-03-07T07:42:27Z

Codecov Report

Attention: Patch coverage is 86.33540% with 22 lines in your changes missing coverage. Please review.

Project coverage is 89.22%. Comparing base (4c43a5b) to head (1426197).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	31.81%	2 Missing and 13 partials ⚠️
lightning/src/ln/onion_utils.rs	92.30%	4 Missing and 2 partials ⚠️
lightning/src/ln/onion_payment.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3650 +/- ##
==========================================
- Coverage 89.24% 89.22% -0.02% 
==========================================
Files 155 155 Lines 119280 119314 +34 Branches 119280 119314 +34 ==========================================
+ Hits 106446 106463 +17 - Misses 10240 10248 +8 - Partials 2594 2603 +9

☔ 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 · 2025-03-08T15:00:02Z

🔔 1st Reminder

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

arik-so

re-acking

TheBlueMatt · 2025-03-10T12:52:09Z

 	packet.hmac = Hmac::from_engine(hmac).to_byte_array();

-	packet
+OnionErrorPacket { data: packet.encode() }


Bleh, it seems pretty weird that we now have OnionErrorPackets flying around (at least outside of onion_utils.rs) that are both decrypted and encrypted. I think the reason it was written the way it was previously was specifically to avoid that - holding the encoded bytes until we get around to encrypting, then making it an OnionErrorPacket. It seems much too easy now to double-en/decrypt or forget to en/decrypt.

I think a similar situation already existed for intermediate nodes? In
rust-lightning/lightning/src/ln/onion_utils.rs
Line 1389 in 4c43a5b
encrypt_failure_packet(incoming_packet_shared_secret,&err.data)
, an OnionErrorPacket received from downstream is encrypted into again an OnionErrorPacket.
The way I see it is that there's "data" in the packet that needs to be encrypted, and it doesn't matter whether that data is 0, 1, 2, etc times encrypted already.

Open to alternatives of course. Were you thinking of pulling the encrypt step into build_failure_packet too?

Yea, certainly not saying the old code was perfect and indeed its already the case that things are encrypted multiple times. I guess I just saw OnionErrorPacket as "encrypted thing ready to go over the wire/received on the wire". Certainly stuff internal to onion_utils doesn't matter quite as much, but once it leaves onion_utils.rs it looks like it'd be really easy to have code that just forgets to encrypt/decrypt (ie the callsite would just read build_failure_packet and the fact that its unencrypted isn't obvious).
Pulling the encrypt step into build_failure_packet seems like one perfectly good way to address it.

Done. Just kept a non-public unencrypted version of build_failure_packet for the bolt test vector test.

TheBlueMatt · 2025-03-10T18:19:14Z

-	shared_secret: &[u8], raw_packet: &[u8],
-) -> msgs::OnionErrorPacket {
+/// Encrypts/decrypts a failure packet.
+pub(super) fn crypt_failure_packet(shared_secret: &[u8], packet: &mut OnionErrorPacket) {


Does this need to remain public now?

It's used in onion_route_tests.rs - I think there is no other way to make it available there, or is there?

Often we do a test wrapper like the below. For four LoC we could even just copy them into the tests, though.
#[cfg(test)] pub fn test_crypt_failure_packet(shared_secret, packet) { crypt_failure_packet(shared_secret, packet) }

Well, it's about testing that function also, so wouldn't want to copy it.
Created the test wrapper.

TheBlueMatt

This LGTM once fixups are squashed.

Prepares for extending OnionErrorPacket with attribution data.

This commits prepares the persistence layer for the addition of attribution data. Changes: - Expand InboundHTLCRemovalReason serialization instead of using the macro. When attribution data is added, it can't just be serialized along with the existing fields because it would break backwards compatibility. Instead the new field needs to go into the tlv block. - Stop using OnionErrorPacket in the UpdateFailHTLC message. When attribution data is added to OnionErrorPacket, it would not be serialized for the wire properly because also here the new field needs to go in the tlv extension of the message. - Prepare HTLCFailReasonRepr serialization for that addition of attribution data. Co-authored-by: Matt Corallo <git@bluematt.me>

joostjager · 2025-03-06T14:59:16Z

Preparatory non-functional commits extracted from #3611

ldk-reviews-bot · 2025-03-06T14:59:19Z

👋 Thanks for assigning @arik-so 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.

joostjager · 2025-03-06T22:08:49Z

Rebased

ldk-reviews-bot · 2025-03-06T23:43:33Z

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

codecov · 2025-03-07T07:42:27Z

Codecov Report

Attention: Patch coverage is 86.33540% with 22 lines in your changes missing coverage. Please review.

Project coverage is 89.22%. Comparing base (4c43a5b) to head (1426197).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	31.81%	2 Missing and 13 partials ⚠️
lightning/src/ln/onion_utils.rs	92.30%	4 Missing and 2 partials ⚠️
lightning/src/ln/onion_payment.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3650 +/- ##
==========================================
- Coverage 89.24% 89.22% -0.02% 
==========================================
Files 155 155 Lines 119280 119314 +34 Branches 119280 119314 +34 ==========================================
+ Hits 106446 106463 +17 - Misses 10240 10248 +8 - Partials 2594 2603 +9

☔ 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 · 2025-03-08T15:00:02Z

🔔 1st Reminder

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

arik-so

re-acking

TheBlueMatt · 2025-03-10T12:52:09Z

 	packet.hmac = Hmac::from_engine(hmac).to_byte_array();

-	packet
+OnionErrorPacket { data: packet.encode() }


Bleh, it seems pretty weird that we now have OnionErrorPackets flying around (at least outside of onion_utils.rs) that are both decrypted and encrypted. I think the reason it was written the way it was previously was specifically to avoid that - holding the encoded bytes until we get around to encrypting, then making it an OnionErrorPacket. It seems much too easy now to double-en/decrypt or forget to en/decrypt.

I think a similar situation already existed for intermediate nodes? In
rust-lightning/lightning/src/ln/onion_utils.rs
Line 1389 in 4c43a5b
encrypt_failure_packet(incoming_packet_shared_secret,&err.data)
, an OnionErrorPacket received from downstream is encrypted into again an OnionErrorPacket.
The way I see it is that there's "data" in the packet that needs to be encrypted, and it doesn't matter whether that data is 0, 1, 2, etc times encrypted already.

Open to alternatives of course. Were you thinking of pulling the encrypt step into build_failure_packet too?

Yea, certainly not saying the old code was perfect and indeed its already the case that things are encrypted multiple times. I guess I just saw OnionErrorPacket as "encrypted thing ready to go over the wire/received on the wire". Certainly stuff internal to onion_utils doesn't matter quite as much, but once it leaves onion_utils.rs it looks like it'd be really easy to have code that just forgets to encrypt/decrypt (ie the callsite would just read build_failure_packet and the fact that its unencrypted isn't obvious).
Pulling the encrypt step into build_failure_packet seems like one perfectly good way to address it.

Done. Just kept a non-public unencrypted version of build_failure_packet for the bolt test vector test.

TheBlueMatt · 2025-03-10T18:19:14Z

-	shared_secret: &[u8], raw_packet: &[u8],
-) -> msgs::OnionErrorPacket {
+/// Encrypts/decrypts a failure packet.
+pub(super) fn crypt_failure_packet(shared_secret: &[u8], packet: &mut OnionErrorPacket) {


Does this need to remain public now?

It's used in onion_route_tests.rs - I think there is no other way to make it available there, or is there?

Often we do a test wrapper like the below. For four LoC we could even just copy them into the tests, though.
#[cfg(test)] pub fn test_crypt_failure_packet(shared_secret, packet) { crypt_failure_packet(shared_secret, packet) }

Well, it's about testing that function also, so wouldn't want to copy it.
Created the test wrapper.

TheBlueMatt

This LGTM once fixups are squashed.

Prepares for extending OnionErrorPacket with attribution data.

This commits prepares the persistence layer for the addition of attribution data. Changes: - Expand InboundHTLCRemovalReason serialization instead of using the macro. When attribution data is added, it can't just be serialized along with the existing fields because it would break backwards compatibility. Instead the new field needs to go into the tlv block. - Stop using OnionErrorPacket in the UpdateFailHTLC message. When attribution data is added to OnionErrorPacket, it would not be serialized for the wire properly because also here the new field needs to go in the tlv extension of the message. - Prepare HTLCFailReasonRepr serialization for that addition of attribution data. Co-authored-by: Matt Corallo <git@bluematt.me>

joostjager · 2025-03-06T14:59:16Z

Preparatory non-functional commits extracted from #3611

ldk-reviews-bot · 2025-03-06T14:59:19Z

👋 Thanks for assigning @arik-so 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.

joostjager · 2025-03-06T22:08:49Z

Rebased

ldk-reviews-bot · 2025-03-06T23:43:33Z

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

codecov · 2025-03-07T07:42:27Z

Codecov Report

Attention: Patch coverage is 86.33540% with 22 lines in your changes missing coverage. Please review.

Project coverage is 89.22%. Comparing base (4c43a5b) to head (1426197).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	31.81%	2 Missing and 13 partials ⚠️
lightning/src/ln/onion_utils.rs	92.30%	4 Missing and 2 partials ⚠️
lightning/src/ln/onion_payment.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3650 +/- ##
==========================================
- Coverage 89.24% 89.22% -0.02% 
==========================================
Files 155 155 Lines 119280 119314 +34 Branches 119280 119314 +34 ==========================================
+ Hits 106446 106463 +17 - Misses 10240 10248 +8 - Partials 2594 2603 +9

☔ 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 · 2025-03-08T15:00:02Z

🔔 1st Reminder

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

arik-so

re-acking

TheBlueMatt · 2025-03-10T12:52:09Z

 	packet.hmac = Hmac::from_engine(hmac).to_byte_array();

-	packet
+OnionErrorPacket { data: packet.encode() }


Bleh, it seems pretty weird that we now have OnionErrorPackets flying around (at least outside of onion_utils.rs) that are both decrypted and encrypted. I think the reason it was written the way it was previously was specifically to avoid that - holding the encoded bytes until we get around to encrypting, then making it an OnionErrorPacket. It seems much too easy now to double-en/decrypt or forget to en/decrypt.

I think a similar situation already existed for intermediate nodes? In
rust-lightning/lightning/src/ln/onion_utils.rs
Line 1389 in 4c43a5b
encrypt_failure_packet(incoming_packet_shared_secret,&err.data)
, an OnionErrorPacket received from downstream is encrypted into again an OnionErrorPacket.
The way I see it is that there's "data" in the packet that needs to be encrypted, and it doesn't matter whether that data is 0, 1, 2, etc times encrypted already.

Open to alternatives of course. Were you thinking of pulling the encrypt step into build_failure_packet too?

Yea, certainly not saying the old code was perfect and indeed its already the case that things are encrypted multiple times. I guess I just saw OnionErrorPacket as "encrypted thing ready to go over the wire/received on the wire". Certainly stuff internal to onion_utils doesn't matter quite as much, but once it leaves onion_utils.rs it looks like it'd be really easy to have code that just forgets to encrypt/decrypt (ie the callsite would just read build_failure_packet and the fact that its unencrypted isn't obvious).
Pulling the encrypt step into build_failure_packet seems like one perfectly good way to address it.

Done. Just kept a non-public unencrypted version of build_failure_packet for the bolt test vector test.

TheBlueMatt · 2025-03-10T18:19:14Z

-	shared_secret: &[u8], raw_packet: &[u8],
-) -> msgs::OnionErrorPacket {
+/// Encrypts/decrypts a failure packet.
+pub(super) fn crypt_failure_packet(shared_secret: &[u8], packet: &mut OnionErrorPacket) {


Does this need to remain public now?

It's used in onion_route_tests.rs - I think there is no other way to make it available there, or is there?

Often we do a test wrapper like the below. For four LoC we could even just copy them into the tests, though.
#[cfg(test)] pub fn test_crypt_failure_packet(shared_secret, packet) { crypt_failure_packet(shared_secret, packet) }

Well, it's about testing that function also, so wouldn't want to copy it.
Created the test wrapper.

TheBlueMatt

This LGTM once fixups are squashed.

Prepares for extending OnionErrorPacket with attribution data.

This commits prepares the persistence layer for the addition of attribution data. Changes: - Expand InboundHTLCRemovalReason serialization instead of using the macro. When attribution data is added, it can't just be serialized along with the existing fields because it would break backwards compatibility. Instead the new field needs to go into the tlv block. - Stop using OnionErrorPacket in the UpdateFailHTLC message. When attribution data is added to OnionErrorPacket, it would not be serialized for the wire properly because also here the new field needs to go in the tlv extension of the message. - Prepare HTLCFailReasonRepr serialization for that addition of attribution data. Co-authored-by: Matt Corallo <git@bluematt.me>

joostjager · 2025-03-06T14:59:16Z

Preparatory non-functional commits extracted from #3611

ldk-reviews-bot · 2025-03-06T14:59:19Z

👋 Thanks for assigning @arik-so 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.

joostjager · 2025-03-06T22:08:49Z

Rebased

ldk-reviews-bot · 2025-03-06T23:43:33Z

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

codecov · 2025-03-07T07:42:27Z

Codecov Report

Attention: Patch coverage is 86.33540% with 22 lines in your changes missing coverage. Please review.

Project coverage is 89.22%. Comparing base (4c43a5b) to head (1426197).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	31.81%	2 Missing and 13 partials ⚠️
lightning/src/ln/onion_utils.rs	92.30%	4 Missing and 2 partials ⚠️
lightning/src/ln/onion_payment.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3650 +/- ##
==========================================
- Coverage 89.24% 89.22% -0.02% 
==========================================
Files 155 155 Lines 119280 119314 +34 Branches 119280 119314 +34 ==========================================
+ Hits 106446 106463 +17 - Misses 10240 10248 +8 - Partials 2594 2603 +9

☔ 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 · 2025-03-08T15:00:02Z

🔔 1st Reminder

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

arik-so

re-acking

TheBlueMatt · 2025-03-10T12:52:09Z

 	packet.hmac = Hmac::from_engine(hmac).to_byte_array();

-	packet
+OnionErrorPacket { data: packet.encode() }


Bleh, it seems pretty weird that we now have OnionErrorPackets flying around (at least outside of onion_utils.rs) that are both decrypted and encrypted. I think the reason it was written the way it was previously was specifically to avoid that - holding the encoded bytes until we get around to encrypting, then making it an OnionErrorPacket. It seems much too easy now to double-en/decrypt or forget to en/decrypt.

I think a similar situation already existed for intermediate nodes? In
rust-lightning/lightning/src/ln/onion_utils.rs
Line 1389 in 4c43a5b
encrypt_failure_packet(incoming_packet_shared_secret,&err.data)
, an OnionErrorPacket received from downstream is encrypted into again an OnionErrorPacket.
The way I see it is that there's "data" in the packet that needs to be encrypted, and it doesn't matter whether that data is 0, 1, 2, etc times encrypted already.

Open to alternatives of course. Were you thinking of pulling the encrypt step into build_failure_packet too?

Yea, certainly not saying the old code was perfect and indeed its already the case that things are encrypted multiple times. I guess I just saw OnionErrorPacket as "encrypted thing ready to go over the wire/received on the wire". Certainly stuff internal to onion_utils doesn't matter quite as much, but once it leaves onion_utils.rs it looks like it'd be really easy to have code that just forgets to encrypt/decrypt (ie the callsite would just read build_failure_packet and the fact that its unencrypted isn't obvious).
Pulling the encrypt step into build_failure_packet seems like one perfectly good way to address it.

Done. Just kept a non-public unencrypted version of build_failure_packet for the bolt test vector test.

TheBlueMatt · 2025-03-10T18:19:14Z

-	shared_secret: &[u8], raw_packet: &[u8],
-) -> msgs::OnionErrorPacket {
+/// Encrypts/decrypts a failure packet.
+pub(super) fn crypt_failure_packet(shared_secret: &[u8], packet: &mut OnionErrorPacket) {


Does this need to remain public now?

It's used in onion_route_tests.rs - I think there is no other way to make it available there, or is there?

Often we do a test wrapper like the below. For four LoC we could even just copy them into the tests, though.
#[cfg(test)] pub fn test_crypt_failure_packet(shared_secret, packet) { crypt_failure_packet(shared_secret, packet) }

Well, it's about testing that function also, so wouldn't want to copy it.
Created the test wrapper.

TheBlueMatt

This LGTM once fixups are squashed.

Prepares for extending OnionErrorPacket with attribution data.

This commits prepares the persistence layer for the addition of attribution data. Changes: - Expand InboundHTLCRemovalReason serialization instead of using the macro. When attribution data is added, it can't just be serialized along with the existing fields because it would break backwards compatibility. Instead the new field needs to go into the tlv block. - Stop using OnionErrorPacket in the UpdateFailHTLC message. When attribution data is added to OnionErrorPacket, it would not be serialized for the wire properly because also here the new field needs to go in the tlv extension of the message. - Prepare HTLCFailReasonRepr serialization for that addition of attribution data. Co-authored-by: Matt Corallo <git@bluematt.me>

joostjager · 2025-03-06T14:59:16Z

Preparatory non-functional commits extracted from #3611

ldk-reviews-bot · 2025-03-06T14:59:19Z

👋 Thanks for assigning @arik-so 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.

joostjager · 2025-03-06T22:08:49Z

Rebased

ldk-reviews-bot · 2025-03-06T23:43:33Z

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

codecov · 2025-03-07T07:42:27Z

Codecov Report

Attention: Patch coverage is 86.33540% with 22 lines in your changes missing coverage. Please review.

Project coverage is 89.22%. Comparing base (4c43a5b) to head (1426197).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	31.81%	2 Missing and 13 partials ⚠️
lightning/src/ln/onion_utils.rs	92.30%	4 Missing and 2 partials ⚠️
lightning/src/ln/onion_payment.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3650 +/- ##
==========================================
- Coverage 89.24% 89.22% -0.02% 
==========================================
Files 155 155 Lines 119280 119314 +34 Branches 119280 119314 +34 ==========================================
+ Hits 106446 106463 +17 - Misses 10240 10248 +8 - Partials 2594 2603 +9

☔ 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 · 2025-03-08T15:00:02Z

🔔 1st Reminder

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

arik-so

re-acking

TheBlueMatt · 2025-03-10T12:52:09Z

 	packet.hmac = Hmac::from_engine(hmac).to_byte_array();

-	packet
+OnionErrorPacket { data: packet.encode() }


Bleh, it seems pretty weird that we now have OnionErrorPackets flying around (at least outside of onion_utils.rs) that are both decrypted and encrypted. I think the reason it was written the way it was previously was specifically to avoid that - holding the encoded bytes until we get around to encrypting, then making it an OnionErrorPacket. It seems much too easy now to double-en/decrypt or forget to en/decrypt.

I think a similar situation already existed for intermediate nodes? In
rust-lightning/lightning/src/ln/onion_utils.rs
Line 1389 in 4c43a5b
encrypt_failure_packet(incoming_packet_shared_secret,&err.data)
, an OnionErrorPacket received from downstream is encrypted into again an OnionErrorPacket.
The way I see it is that there's "data" in the packet that needs to be encrypted, and it doesn't matter whether that data is 0, 1, 2, etc times encrypted already.

Open to alternatives of course. Were you thinking of pulling the encrypt step into build_failure_packet too?

Yea, certainly not saying the old code was perfect and indeed its already the case that things are encrypted multiple times. I guess I just saw OnionErrorPacket as "encrypted thing ready to go over the wire/received on the wire". Certainly stuff internal to onion_utils doesn't matter quite as much, but once it leaves onion_utils.rs it looks like it'd be really easy to have code that just forgets to encrypt/decrypt (ie the callsite would just read build_failure_packet and the fact that its unencrypted isn't obvious).
Pulling the encrypt step into build_failure_packet seems like one perfectly good way to address it.

Done. Just kept a non-public unencrypted version of build_failure_packet for the bolt test vector test.

TheBlueMatt · 2025-03-10T18:19:14Z

-	shared_secret: &[u8], raw_packet: &[u8],
-) -> msgs::OnionErrorPacket {
+/// Encrypts/decrypts a failure packet.
+pub(super) fn crypt_failure_packet(shared_secret: &[u8], packet: &mut OnionErrorPacket) {


Does this need to remain public now?

It's used in onion_route_tests.rs - I think there is no other way to make it available there, or is there?

Often we do a test wrapper like the below. For four LoC we could even just copy them into the tests, though.
#[cfg(test)] pub fn test_crypt_failure_packet(shared_secret, packet) { crypt_failure_packet(shared_secret, packet) }

Well, it's about testing that function also, so wouldn't want to copy it.
Created the test wrapper.

TheBlueMatt

This LGTM once fixups are squashed.

Prepares for extending OnionErrorPacket with attribution data.

This commits prepares the persistence layer for the addition of attribution data. Changes: - Expand InboundHTLCRemovalReason serialization instead of using the macro. When attribution data is added, it can't just be serialized along with the existing fields because it would break backwards compatibility. Instead the new field needs to go into the tlv block. - Stop using OnionErrorPacket in the UpdateFailHTLC message. When attribution data is added to OnionErrorPacket, it would not be serialized for the wire properly because also here the new field needs to go in the tlv extension of the message. - Prepare HTLCFailReasonRepr serialization for that addition of attribution data. Co-authored-by: Matt Corallo <git@bluematt.me>

joostjager added the weekly goal Someone wants to land this this week label Mar 6, 2025

ldk-reviews-bot requested a review from arik-so March 6, 2025 15:09

joostjager force-pushed the attributable-failures-prefactor branch from 894c91e to f82cb5aCompare March 6, 2025 22:08

arik-so reviewed Mar 6, 2025
View reviewed changes

Comment threadlightning/src/ln/onion_route_tests.rs
Comment threadlightning/src/ln/onion_utils.rs Outdated

joostjager force-pushed the attributable-failures-prefactor branch from f82cb5a to 8d01eb9Compare March 7, 2025 07:33

joostjager requested a review from arik-so March 7, 2025 07:34

arik-so previously approved these changes Mar 10, 2025
View reviewed changes

Comment threadlightning/src/ln/onion_route_tests.rs
Comment threadlightning/src/ln/onion_route_tests.rs

joostjager dismissed arik-so’s stale review via 47a06c5March 10, 2025 07:27

joostjager force-pushed the attributable-failures-prefactor branch from 8d01eb9 to 47a06c5Compare March 10, 2025 07:27

joostjager requested a review from arik-so March 10, 2025 07:28

arik-so previously approved these changes Mar 10, 2025
View reviewed changes

joostjager requested a review from TheBlueMatt March 10, 2025 08:14

TheBlueMatt reviewed Mar 10, 2025
View reviewed changes

joostjager dismissed arik-so’s stale review via 8450e58March 10, 2025 13:33

joostjager requested a review from TheBlueMatt March 10, 2025 13:34

joostjager force-pushed the attributable-failures-prefactor branch 3 times, most recently from fe6cd21 to baffc82Compare March 10, 2025 15:36

TheBlueMatt reviewed Mar 10, 2025
View reviewed changes

joostjager force-pushed the attributable-failures-prefactor branch from baffc82 to 9fbbcdcCompare March 10, 2025 19:29

joostjager requested a review from TheBlueMatt March 10, 2025 19:30

TheBlueMatt reviewed Mar 10, 2025
View reviewed changes

joostjagerand others added 2 commits March 10, 2025 23:22

Refactor onion_utils to encrypt/decrypt OnionErrorPacket types
ea0f099
Prepares for extending OnionErrorPacket with attribution data.

joostjager force-pushed the attributable-failures-prefactor branch from db221e2 to 1426197Compare March 10, 2025 22:26

TheBlueMatt requested a review from arik-so March 10, 2025 23:19

arik-so approved these changes Mar 11, 2025
View reviewed changes

joostjager merged commit 0e1bbc4 into lightningdevkit:mainMar 11, 2025