Log cases where an onion failure cannot be attributed or interpreted by joostjager · Pull Request #3629 · lightningdevkit/rust-lightning

joostjager · 2025-02-28T09:52:09Z

Create more visibility into these edge cases. The non-attributable failure in particular can be used to disrupt sender operation and it is therefore good to at least log these cases clearly.

Additionally a bug is fixed where an unreadable failure with valid hmac wasn't properly attributed to a node.

joostjager · 2025-02-28T09:53:25Z

- Ok(p) => p,
- Err(_) => return,
- };
+ let decrypt_result = decrypt_onion_error_packet(&mut encrypted_packet, shared_secret);


Check hmac first to distinguish between unreadable failures and hmac mismatches.

codecov · 2025-02-28T10:01:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.17%. Comparing base (eaeed77) to head (1d40811).

Additional details and impacted files

@@ Coverage Diff @@## main #3629 +/- ##
========================================
Coverage 89.16% 89.17% ========================================
Files 152 152 Lines 118791 118947 +156 Branches 118791 118947 +156 ========================================
+ Hits 105921 106068 +147 - Misses 10312 10316 +4 - Partials 2558 2563 +5

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

joostjager · 2025-02-28T10:54:06Z

Coverage reports suggests more test coverage needed...

joostjager · 2025-02-28T13:20:02Z

+ is_permanent: true,
+ });
+ let short_channel_id = Some(route_hop.short_channel_id);
+ res = Some(FailureLearnings {


This is a bug fix. Previously unreadable failures weren't attributable even though the hmac checked out.

joostjager · 2025-02-28T13:30:38Z

+	fn build_test_path() -> Path {
+ Path {
+ hops: vec![
+ RouteHop {


rustfmt wanted this

joostjager · 2025-02-28T13:31:10Z

+ .unwrap(),
+ channel_features: ChannelFeatures::empty(),
+ node_features: NodeFeatures::empty(),
+ short_channel_id: 1,


it is a move, but just these short_channel_id's made unique to make it easier to test things for attr errors

joostjager · 2025-02-28T13:31:57Z

Tests added

TheBlueMatt · 2025-02-28T19:27:50Z

+
+ let network_update = Some(NetworkUpdate::NodeFailure {
+ node_id: route_hop.pubkey,
+ is_permanent: true,


IIRC is_permanentNodeFailures result in removing the node from the graph entirely, which seems a bit harsh, I think?

The same was already done for a missing failure code, so it seemed reasonable to apply the same penalty for the more severe case where the message cannot even be decoded?

I think the node is removed for a week? That seems fine to me because in this case there must be a bug in the remote node software.

Hmmmmmmmm, I guess okay if only because we don't have a way to punish all of a node's channels. It does seem like we should find a way to do that, though, without outright graph removal. I know some other stuff does that, but I'd call that legacy needs-updating too :)
Indeed, after a while we'd accept fresh announcements of that node's channels, but generally we don't resync so we'd most likely only get them on restart (or when the node opens new channels).

This function still decrypted the message in-place even when it returned an error. This is confusing for the caller.

Create more visibility into these edge cases. The non-attributable failure in particular can be used to disrupt sender operation and it is therefore good to at least log these cases clearly.

This commit fixes a bug where a node penalty was not applied where it should.

arik-so · 2025-03-04T06:05:05Z

-	packet: &mut Vec<u8>, shared_secret: SharedSecret,
-) -> Result<msgs::DecodedOnionErrorPacket, msgs::DecodeError> {
+/// Decrypt the error packet in-place.
+fn decrypt_onion_error_packet(packet: &mut Vec<u8>, shared_secret: SharedSecret) {


I almost wonder whether it's one of those instances where in-place processing is what largely contributes to confusion. Getting a result back with the decrypted packet or with the decryption error, without worrying about the input argument getting changed, seems preferable.

The confusion was mainly that this used to be a function that returned a value and still had a side effect too. A side effect that was also needed, because the decrypted packet needs to be processed for the next hop.
Could have returned a decrypted copy in addition to the decoded failure to make it a pure function, probably with some performance implications. But I think now that this function has no return value anymore and only decrypts in-place, it's already a lot better.

joostjager · 2025-02-28T09:52:09Z

Create more visibility into these edge cases. The non-attributable failure in particular can be used to disrupt sender operation and it is therefore good to at least log these cases clearly.

Additionally a bug is fixed where an unreadable failure with valid hmac wasn't properly attributed to a node.

joostjager · 2025-02-28T09:53:25Z

- Ok(p) => p,
- Err(_) => return,
- };
+ let decrypt_result = decrypt_onion_error_packet(&mut encrypted_packet, shared_secret);


Check hmac first to distinguish between unreadable failures and hmac mismatches.

codecov · 2025-02-28T10:01:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.17%. Comparing base (eaeed77) to head (1d40811).

Additional details and impacted files

@@ Coverage Diff @@## main #3629 +/- ##
========================================
Coverage 89.16% 89.17% ========================================
Files 152 152 Lines 118791 118947 +156 Branches 118791 118947 +156 ========================================
+ Hits 105921 106068 +147 - Misses 10312 10316 +4 - Partials 2558 2563 +5

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

joostjager · 2025-02-28T10:54:06Z

Coverage reports suggests more test coverage needed...

joostjager · 2025-02-28T13:20:02Z

+ is_permanent: true,
+ });
+ let short_channel_id = Some(route_hop.short_channel_id);
+ res = Some(FailureLearnings {


This is a bug fix. Previously unreadable failures weren't attributable even though the hmac checked out.

joostjager · 2025-02-28T13:30:38Z

+	fn build_test_path() -> Path {
+ Path {
+ hops: vec![
+ RouteHop {


rustfmt wanted this

joostjager · 2025-02-28T13:31:10Z

+ .unwrap(),
+ channel_features: ChannelFeatures::empty(),
+ node_features: NodeFeatures::empty(),
+ short_channel_id: 1,


it is a move, but just these short_channel_id's made unique to make it easier to test things for attr errors

joostjager · 2025-02-28T13:31:57Z

Tests added

TheBlueMatt · 2025-02-28T19:27:50Z

+
+ let network_update = Some(NetworkUpdate::NodeFailure {
+ node_id: route_hop.pubkey,
+ is_permanent: true,


IIRC is_permanentNodeFailures result in removing the node from the graph entirely, which seems a bit harsh, I think?

The same was already done for a missing failure code, so it seemed reasonable to apply the same penalty for the more severe case where the message cannot even be decoded?

I think the node is removed for a week? That seems fine to me because in this case there must be a bug in the remote node software.

Hmmmmmmmm, I guess okay if only because we don't have a way to punish all of a node's channels. It does seem like we should find a way to do that, though, without outright graph removal. I know some other stuff does that, but I'd call that legacy needs-updating too :)
Indeed, after a while we'd accept fresh announcements of that node's channels, but generally we don't resync so we'd most likely only get them on restart (or when the node opens new channels).

This function still decrypted the message in-place even when it returned an error. This is confusing for the caller.

Create more visibility into these edge cases. The non-attributable failure in particular can be used to disrupt sender operation and it is therefore good to at least log these cases clearly.

This commit fixes a bug where a node penalty was not applied where it should.

arik-so · 2025-03-04T06:05:05Z

-	packet: &mut Vec<u8>, shared_secret: SharedSecret,
-) -> Result<msgs::DecodedOnionErrorPacket, msgs::DecodeError> {
+/// Decrypt the error packet in-place.
+fn decrypt_onion_error_packet(packet: &mut Vec<u8>, shared_secret: SharedSecret) {


I almost wonder whether it's one of those instances where in-place processing is what largely contributes to confusion. Getting a result back with the decrypted packet or with the decryption error, without worrying about the input argument getting changed, seems preferable.

The confusion was mainly that this used to be a function that returned a value and still had a side effect too. A side effect that was also needed, because the decrypted packet needs to be processed for the next hop.
Could have returned a decrypted copy in addition to the decoded failure to make it a pure function, probably with some performance implications. But I think now that this function has no return value anymore and only decrypts in-place, it's already a lot better.

joostjager · 2025-02-28T09:52:09Z

Create more visibility into these edge cases. The non-attributable failure in particular can be used to disrupt sender operation and it is therefore good to at least log these cases clearly.

Additionally a bug is fixed where an unreadable failure with valid hmac wasn't properly attributed to a node.

joostjager · 2025-02-28T09:53:25Z

- Ok(p) => p,
- Err(_) => return,
- };
+ let decrypt_result = decrypt_onion_error_packet(&mut encrypted_packet, shared_secret);


Check hmac first to distinguish between unreadable failures and hmac mismatches.

codecov · 2025-02-28T10:01:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.17%. Comparing base (eaeed77) to head (1d40811).

Additional details and impacted files

@@ Coverage Diff @@## main #3629 +/- ##
========================================
Coverage 89.16% 89.17% ========================================
Files 152 152 Lines 118791 118947 +156 Branches 118791 118947 +156 ========================================
+ Hits 105921 106068 +147 - Misses 10312 10316 +4 - Partials 2558 2563 +5

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

joostjager · 2025-02-28T10:54:06Z

Coverage reports suggests more test coverage needed...

joostjager · 2025-02-28T13:20:02Z

+ is_permanent: true,
+ });
+ let short_channel_id = Some(route_hop.short_channel_id);
+ res = Some(FailureLearnings {


This is a bug fix. Previously unreadable failures weren't attributable even though the hmac checked out.

joostjager · 2025-02-28T13:30:38Z

+	fn build_test_path() -> Path {
+ Path {
+ hops: vec![
+ RouteHop {


rustfmt wanted this

joostjager · 2025-02-28T13:31:10Z

+ .unwrap(),
+ channel_features: ChannelFeatures::empty(),
+ node_features: NodeFeatures::empty(),
+ short_channel_id: 1,


it is a move, but just these short_channel_id's made unique to make it easier to test things for attr errors

joostjager · 2025-02-28T13:31:57Z

Tests added

TheBlueMatt · 2025-02-28T19:27:50Z

+
+ let network_update = Some(NetworkUpdate::NodeFailure {
+ node_id: route_hop.pubkey,
+ is_permanent: true,


IIRC is_permanentNodeFailures result in removing the node from the graph entirely, which seems a bit harsh, I think?

The same was already done for a missing failure code, so it seemed reasonable to apply the same penalty for the more severe case where the message cannot even be decoded?

I think the node is removed for a week? That seems fine to me because in this case there must be a bug in the remote node software.

Hmmmmmmmm, I guess okay if only because we don't have a way to punish all of a node's channels. It does seem like we should find a way to do that, though, without outright graph removal. I know some other stuff does that, but I'd call that legacy needs-updating too :)
Indeed, after a while we'd accept fresh announcements of that node's channels, but generally we don't resync so we'd most likely only get them on restart (or when the node opens new channels).

This function still decrypted the message in-place even when it returned an error. This is confusing for the caller.

Create more visibility into these edge cases. The non-attributable failure in particular can be used to disrupt sender operation and it is therefore good to at least log these cases clearly.

This commit fixes a bug where a node penalty was not applied where it should.

arik-so · 2025-03-04T06:05:05Z

-	packet: &mut Vec<u8>, shared_secret: SharedSecret,
-) -> Result<msgs::DecodedOnionErrorPacket, msgs::DecodeError> {
+/// Decrypt the error packet in-place.
+fn decrypt_onion_error_packet(packet: &mut Vec<u8>, shared_secret: SharedSecret) {


I almost wonder whether it's one of those instances where in-place processing is what largely contributes to confusion. Getting a result back with the decrypted packet or with the decryption error, without worrying about the input argument getting changed, seems preferable.

The confusion was mainly that this used to be a function that returned a value and still had a side effect too. A side effect that was also needed, because the decrypted packet needs to be processed for the next hop.
Could have returned a decrypted copy in addition to the decoded failure to make it a pure function, probably with some performance implications. But I think now that this function has no return value anymore and only decrypts in-place, it's already a lot better.

joostjager · 2025-02-28T09:52:09Z

Create more visibility into these edge cases. The non-attributable failure in particular can be used to disrupt sender operation and it is therefore good to at least log these cases clearly.

Additionally a bug is fixed where an unreadable failure with valid hmac wasn't properly attributed to a node.

joostjager · 2025-02-28T09:53:25Z

- Ok(p) => p,
- Err(_) => return,
- };
+ let decrypt_result = decrypt_onion_error_packet(&mut encrypted_packet, shared_secret);


Check hmac first to distinguish between unreadable failures and hmac mismatches.

codecov · 2025-02-28T10:01:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.17%. Comparing base (eaeed77) to head (1d40811).

Additional details and impacted files

@@ Coverage Diff @@## main #3629 +/- ##
========================================
Coverage 89.16% 89.17% ========================================
Files 152 152 Lines 118791 118947 +156 Branches 118791 118947 +156 ========================================
+ Hits 105921 106068 +147 - Misses 10312 10316 +4 - Partials 2558 2563 +5

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

joostjager · 2025-02-28T10:54:06Z

Coverage reports suggests more test coverage needed...

joostjager · 2025-02-28T13:20:02Z

+ is_permanent: true,
+ });
+ let short_channel_id = Some(route_hop.short_channel_id);
+ res = Some(FailureLearnings {


This is a bug fix. Previously unreadable failures weren't attributable even though the hmac checked out.

joostjager · 2025-02-28T13:30:38Z

+	fn build_test_path() -> Path {
+ Path {
+ hops: vec![
+ RouteHop {


rustfmt wanted this

joostjager · 2025-02-28T13:31:10Z

+ .unwrap(),
+ channel_features: ChannelFeatures::empty(),
+ node_features: NodeFeatures::empty(),
+ short_channel_id: 1,


it is a move, but just these short_channel_id's made unique to make it easier to test things for attr errors

joostjager · 2025-02-28T13:31:57Z

Tests added

TheBlueMatt · 2025-02-28T19:27:50Z

+
+ let network_update = Some(NetworkUpdate::NodeFailure {
+ node_id: route_hop.pubkey,
+ is_permanent: true,


IIRC is_permanentNodeFailures result in removing the node from the graph entirely, which seems a bit harsh, I think?

The same was already done for a missing failure code, so it seemed reasonable to apply the same penalty for the more severe case where the message cannot even be decoded?

I think the node is removed for a week? That seems fine to me because in this case there must be a bug in the remote node software.

Hmmmmmmmm, I guess okay if only because we don't have a way to punish all of a node's channels. It does seem like we should find a way to do that, though, without outright graph removal. I know some other stuff does that, but I'd call that legacy needs-updating too :)
Indeed, after a while we'd accept fresh announcements of that node's channels, but generally we don't resync so we'd most likely only get them on restart (or when the node opens new channels).

This function still decrypted the message in-place even when it returned an error. This is confusing for the caller.

Create more visibility into these edge cases. The non-attributable failure in particular can be used to disrupt sender operation and it is therefore good to at least log these cases clearly.

This commit fixes a bug where a node penalty was not applied where it should.

arik-so · 2025-03-04T06:05:05Z

-	packet: &mut Vec<u8>, shared_secret: SharedSecret,
-) -> Result<msgs::DecodedOnionErrorPacket, msgs::DecodeError> {
+/// Decrypt the error packet in-place.
+fn decrypt_onion_error_packet(packet: &mut Vec<u8>, shared_secret: SharedSecret) {


I almost wonder whether it's one of those instances where in-place processing is what largely contributes to confusion. Getting a result back with the decrypted packet or with the decryption error, without worrying about the input argument getting changed, seems preferable.

The confusion was mainly that this used to be a function that returned a value and still had a side effect too. A side effect that was also needed, because the decrypted packet needs to be processed for the next hop.
Could have returned a decrypted copy in addition to the decoded failure to make it a pure function, probably with some performance implications. But I think now that this function has no return value anymore and only decrypts in-place, it's already a lot better.

joostjager · 2025-02-28T09:52:09Z

Create more visibility into these edge cases. The non-attributable failure in particular can be used to disrupt sender operation and it is therefore good to at least log these cases clearly.

Additionally a bug is fixed where an unreadable failure with valid hmac wasn't properly attributed to a node.

joostjager · 2025-02-28T09:53:25Z

- Ok(p) => p,
- Err(_) => return,
- };
+ let decrypt_result = decrypt_onion_error_packet(&mut encrypted_packet, shared_secret);


Check hmac first to distinguish between unreadable failures and hmac mismatches.

codecov · 2025-02-28T10:01:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.17%. Comparing base (eaeed77) to head (1d40811).

Additional details and impacted files

@@ Coverage Diff @@## main #3629 +/- ##
========================================
Coverage 89.16% 89.17% ========================================
Files 152 152 Lines 118791 118947 +156 Branches 118791 118947 +156 ========================================
+ Hits 105921 106068 +147 - Misses 10312 10316 +4 - Partials 2558 2563 +5

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

joostjager · 2025-02-28T10:54:06Z

Coverage reports suggests more test coverage needed...

joostjager · 2025-02-28T13:20:02Z

+ is_permanent: true,
+ });
+ let short_channel_id = Some(route_hop.short_channel_id);
+ res = Some(FailureLearnings {


This is a bug fix. Previously unreadable failures weren't attributable even though the hmac checked out.

joostjager · 2025-02-28T13:30:38Z

+	fn build_test_path() -> Path {
+ Path {
+ hops: vec![
+ RouteHop {


rustfmt wanted this

joostjager · 2025-02-28T13:31:10Z

+ .unwrap(),
+ channel_features: ChannelFeatures::empty(),
+ node_features: NodeFeatures::empty(),
+ short_channel_id: 1,


it is a move, but just these short_channel_id's made unique to make it easier to test things for attr errors

joostjager · 2025-02-28T13:31:57Z

Tests added

TheBlueMatt · 2025-02-28T19:27:50Z

+
+ let network_update = Some(NetworkUpdate::NodeFailure {
+ node_id: route_hop.pubkey,
+ is_permanent: true,


IIRC is_permanentNodeFailures result in removing the node from the graph entirely, which seems a bit harsh, I think?

The same was already done for a missing failure code, so it seemed reasonable to apply the same penalty for the more severe case where the message cannot even be decoded?

I think the node is removed for a week? That seems fine to me because in this case there must be a bug in the remote node software.

Hmmmmmmmm, I guess okay if only because we don't have a way to punish all of a node's channels. It does seem like we should find a way to do that, though, without outright graph removal. I know some other stuff does that, but I'd call that legacy needs-updating too :)
Indeed, after a while we'd accept fresh announcements of that node's channels, but generally we don't resync so we'd most likely only get them on restart (or when the node opens new channels).

This function still decrypted the message in-place even when it returned an error. This is confusing for the caller.

Create more visibility into these edge cases. The non-attributable failure in particular can be used to disrupt sender operation and it is therefore good to at least log these cases clearly.

This commit fixes a bug where a node penalty was not applied where it should.

arik-so · 2025-03-04T06:05:05Z

-	packet: &mut Vec<u8>, shared_secret: SharedSecret,
-) -> Result<msgs::DecodedOnionErrorPacket, msgs::DecodeError> {
+/// Decrypt the error packet in-place.
+fn decrypt_onion_error_packet(packet: &mut Vec<u8>, shared_secret: SharedSecret) {


I almost wonder whether it's one of those instances where in-place processing is what largely contributes to confusion. Getting a result back with the decrypted packet or with the decryption error, without worrying about the input argument getting changed, seems preferable.

The confusion was mainly that this used to be a function that returned a value and still had a side effect too. A side effect that was also needed, because the decrypted packet needs to be processed for the next hop.
Could have returned a decrypted copy in addition to the decoded failure to make it a pure function, probably with some performance implications. But I think now that this function has no return value anymore and only decrypts in-place, it's already a lot better.

joostjager · 2025-02-28T09:52:09Z

Create more visibility into these edge cases. The non-attributable failure in particular can be used to disrupt sender operation and it is therefore good to at least log these cases clearly.

Additionally a bug is fixed where an unreadable failure with valid hmac wasn't properly attributed to a node.

joostjager · 2025-02-28T09:53:25Z

- Ok(p) => p,
- Err(_) => return,
- };
+ let decrypt_result = decrypt_onion_error_packet(&mut encrypted_packet, shared_secret);


Check hmac first to distinguish between unreadable failures and hmac mismatches.

codecov · 2025-02-28T10:01:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.17%. Comparing base (eaeed77) to head (1d40811).

Additional details and impacted files

@@ Coverage Diff @@## main #3629 +/- ##
========================================
Coverage 89.16% 89.17% ========================================
Files 152 152 Lines 118791 118947 +156 Branches 118791 118947 +156 ========================================
+ Hits 105921 106068 +147 - Misses 10312 10316 +4 - Partials 2558 2563 +5

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

joostjager · 2025-02-28T10:54:06Z

Coverage reports suggests more test coverage needed...

joostjager · 2025-02-28T13:20:02Z

+ is_permanent: true,
+ });
+ let short_channel_id = Some(route_hop.short_channel_id);
+ res = Some(FailureLearnings {


This is a bug fix. Previously unreadable failures weren't attributable even though the hmac checked out.

joostjager · 2025-02-28T13:30:38Z

+	fn build_test_path() -> Path {
+ Path {
+ hops: vec![
+ RouteHop {


rustfmt wanted this

joostjager · 2025-02-28T13:31:10Z

+ .unwrap(),
+ channel_features: ChannelFeatures::empty(),
+ node_features: NodeFeatures::empty(),
+ short_channel_id: 1,


it is a move, but just these short_channel_id's made unique to make it easier to test things for attr errors

joostjager · 2025-02-28T13:31:57Z

Tests added

TheBlueMatt · 2025-02-28T19:27:50Z

+
+ let network_update = Some(NetworkUpdate::NodeFailure {
+ node_id: route_hop.pubkey,
+ is_permanent: true,


IIRC is_permanentNodeFailures result in removing the node from the graph entirely, which seems a bit harsh, I think?

The same was already done for a missing failure code, so it seemed reasonable to apply the same penalty for the more severe case where the message cannot even be decoded?

I think the node is removed for a week? That seems fine to me because in this case there must be a bug in the remote node software.

Hmmmmmmmm, I guess okay if only because we don't have a way to punish all of a node's channels. It does seem like we should find a way to do that, though, without outright graph removal. I know some other stuff does that, but I'd call that legacy needs-updating too :)
Indeed, after a while we'd accept fresh announcements of that node's channels, but generally we don't resync so we'd most likely only get them on restart (or when the node opens new channels).

This function still decrypted the message in-place even when it returned an error. This is confusing for the caller.

Create more visibility into these edge cases. The non-attributable failure in particular can be used to disrupt sender operation and it is therefore good to at least log these cases clearly.

This commit fixes a bug where a node penalty was not applied where it should.

arik-so · 2025-03-04T06:05:05Z

-	packet: &mut Vec<u8>, shared_secret: SharedSecret,
-) -> Result<msgs::DecodedOnionErrorPacket, msgs::DecodeError> {
+/// Decrypt the error packet in-place.
+fn decrypt_onion_error_packet(packet: &mut Vec<u8>, shared_secret: SharedSecret) {


I almost wonder whether it's one of those instances where in-place processing is what largely contributes to confusion. Getting a result back with the decrypted packet or with the decryption error, without worrying about the input argument getting changed, seems preferable.

The confusion was mainly that this used to be a function that returned a value and still had a side effect too. A side effect that was also needed, because the decrypted packet needs to be processed for the next hop.
Could have returned a decrypted copy in addition to the decoded failure to make it a pure function, probably with some performance implications. But I think now that this function has no return value anymore and only decrypts in-place, it's already a lot better.

joostjager · 2025-02-28T09:52:09Z

Create more visibility into these edge cases. The non-attributable failure in particular can be used to disrupt sender operation and it is therefore good to at least log these cases clearly.

Additionally a bug is fixed where an unreadable failure with valid hmac wasn't properly attributed to a node.

joostjager · 2025-02-28T09:53:25Z

- Ok(p) => p,
- Err(_) => return,
- };
+ let decrypt_result = decrypt_onion_error_packet(&mut encrypted_packet, shared_secret);


Check hmac first to distinguish between unreadable failures and hmac mismatches.

codecov · 2025-02-28T10:01:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.17%. Comparing base (eaeed77) to head (1d40811).

Additional details and impacted files

@@ Coverage Diff @@## main #3629 +/- ##
========================================
Coverage 89.16% 89.17% ========================================
Files 152 152 Lines 118791 118947 +156 Branches 118791 118947 +156 ========================================
+ Hits 105921 106068 +147 - Misses 10312 10316 +4 - Partials 2558 2563 +5

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

joostjager · 2025-02-28T10:54:06Z

Coverage reports suggests more test coverage needed...

joostjager · 2025-02-28T13:20:02Z

+ is_permanent: true,
+ });
+ let short_channel_id = Some(route_hop.short_channel_id);
+ res = Some(FailureLearnings {


This is a bug fix. Previously unreadable failures weren't attributable even though the hmac checked out.

joostjager · 2025-02-28T13:30:38Z

+	fn build_test_path() -> Path {
+ Path {
+ hops: vec![
+ RouteHop {


rustfmt wanted this

joostjager · 2025-02-28T13:31:10Z

+ .unwrap(),
+ channel_features: ChannelFeatures::empty(),
+ node_features: NodeFeatures::empty(),
+ short_channel_id: 1,


it is a move, but just these short_channel_id's made unique to make it easier to test things for attr errors

joostjager · 2025-02-28T13:31:57Z

Tests added

TheBlueMatt · 2025-02-28T19:27:50Z

+
+ let network_update = Some(NetworkUpdate::NodeFailure {
+ node_id: route_hop.pubkey,
+ is_permanent: true,


IIRC is_permanentNodeFailures result in removing the node from the graph entirely, which seems a bit harsh, I think?

The same was already done for a missing failure code, so it seemed reasonable to apply the same penalty for the more severe case where the message cannot even be decoded?

I think the node is removed for a week? That seems fine to me because in this case there must be a bug in the remote node software.

Hmmmmmmmm, I guess okay if only because we don't have a way to punish all of a node's channels. It does seem like we should find a way to do that, though, without outright graph removal. I know some other stuff does that, but I'd call that legacy needs-updating too :)
Indeed, after a while we'd accept fresh announcements of that node's channels, but generally we don't resync so we'd most likely only get them on restart (or when the node opens new channels).

This function still decrypted the message in-place even when it returned an error. This is confusing for the caller.

Create more visibility into these edge cases. The non-attributable failure in particular can be used to disrupt sender operation and it is therefore good to at least log these cases clearly.

This commit fixes a bug where a node penalty was not applied where it should.

arik-so · 2025-03-04T06:05:05Z

-	packet: &mut Vec<u8>, shared_secret: SharedSecret,
-) -> Result<msgs::DecodedOnionErrorPacket, msgs::DecodeError> {
+/// Decrypt the error packet in-place.
+fn decrypt_onion_error_packet(packet: &mut Vec<u8>, shared_secret: SharedSecret) {


I almost wonder whether it's one of those instances where in-place processing is what largely contributes to confusion. Getting a result back with the decrypted packet or with the decryption error, without worrying about the input argument getting changed, seems preferable.

The confusion was mainly that this used to be a function that returned a value and still had a side effect too. A side effect that was also needed, because the decrypted packet needs to be processed for the next hop.
Could have returned a decrypted copy in addition to the decoded failure to make it a pure function, probably with some performance implications. But I think now that this function has no return value anymore and only decrypts in-place, it's already a lot better.

joostjager · 2025-02-28T09:52:09Z

Create more visibility into these edge cases. The non-attributable failure in particular can be used to disrupt sender operation and it is therefore good to at least log these cases clearly.

Additionally a bug is fixed where an unreadable failure with valid hmac wasn't properly attributed to a node.

joostjager · 2025-02-28T09:53:25Z

- Ok(p) => p,
- Err(_) => return,
- };
+ let decrypt_result = decrypt_onion_error_packet(&mut encrypted_packet, shared_secret);


Check hmac first to distinguish between unreadable failures and hmac mismatches.

codecov · 2025-02-28T10:01:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.17%. Comparing base (eaeed77) to head (1d40811).

Additional details and impacted files

@@ Coverage Diff @@## main #3629 +/- ##
========================================
Coverage 89.16% 89.17% ========================================
Files 152 152 Lines 118791 118947 +156 Branches 118791 118947 +156 ========================================
+ Hits 105921 106068 +147 - Misses 10312 10316 +4 - Partials 2558 2563 +5

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

joostjager · 2025-02-28T10:54:06Z

Coverage reports suggests more test coverage needed...

joostjager · 2025-02-28T13:20:02Z

+ is_permanent: true,
+ });
+ let short_channel_id = Some(route_hop.short_channel_id);
+ res = Some(FailureLearnings {


This is a bug fix. Previously unreadable failures weren't attributable even though the hmac checked out.

joostjager · 2025-02-28T13:30:38Z

+	fn build_test_path() -> Path {
+ Path {
+ hops: vec![
+ RouteHop {


rustfmt wanted this

joostjager · 2025-02-28T13:31:10Z

+ .unwrap(),
+ channel_features: ChannelFeatures::empty(),
+ node_features: NodeFeatures::empty(),
+ short_channel_id: 1,


it is a move, but just these short_channel_id's made unique to make it easier to test things for attr errors

joostjager · 2025-02-28T13:31:57Z

Tests added

TheBlueMatt · 2025-02-28T19:27:50Z

+
+ let network_update = Some(NetworkUpdate::NodeFailure {
+ node_id: route_hop.pubkey,
+ is_permanent: true,


IIRC is_permanentNodeFailures result in removing the node from the graph entirely, which seems a bit harsh, I think?

The same was already done for a missing failure code, so it seemed reasonable to apply the same penalty for the more severe case where the message cannot even be decoded?

I think the node is removed for a week? That seems fine to me because in this case there must be a bug in the remote node software.

Hmmmmmmmm, I guess okay if only because we don't have a way to punish all of a node's channels. It does seem like we should find a way to do that, though, without outright graph removal. I know some other stuff does that, but I'd call that legacy needs-updating too :)
Indeed, after a while we'd accept fresh announcements of that node's channels, but generally we don't resync so we'd most likely only get them on restart (or when the node opens new channels).

This function still decrypted the message in-place even when it returned an error. This is confusing for the caller.

Create more visibility into these edge cases. The non-attributable failure in particular can be used to disrupt sender operation and it is therefore good to at least log these cases clearly.

This commit fixes a bug where a node penalty was not applied where it should.

arik-so · 2025-03-04T06:05:05Z

-	packet: &mut Vec<u8>, shared_secret: SharedSecret,
-) -> Result<msgs::DecodedOnionErrorPacket, msgs::DecodeError> {
+/// Decrypt the error packet in-place.
+fn decrypt_onion_error_packet(packet: &mut Vec<u8>, shared_secret: SharedSecret) {


I almost wonder whether it's one of those instances where in-place processing is what largely contributes to confusion. Getting a result back with the decrypted packet or with the decryption error, without worrying about the input argument getting changed, seems preferable.

The confusion was mainly that this used to be a function that returned a value and still had a side effect too. A side effect that was also needed, because the decrypted packet needs to be processed for the next hop.
Could have returned a decrypted copy in addition to the decoded failure to make it a pure function, probably with some performance implications. But I think now that this function has no return value anymore and only decrypts in-place, it's already a lot better.

joostjager commented Feb 28, 2025
View reviewed changes

joostjager force-pushed the log-attribution-failures branch 5 times, most recently from 56b0aa7 to 70c8e56Compare February 28, 2025 13:16

joostjager commented Feb 28, 2025
View reviewed changes

Add test coverage for process_onion_failure
447148d

joostjager force-pushed the log-attribution-failures branch from 70c8e56 to 47caa1eCompare February 28, 2025 13:24

joostjager commented Feb 28, 2025
View reviewed changes

joostjager force-pushed the log-attribution-failures branch 3 times, most recently from cdda315 to fc11fe4Compare February 28, 2025 13:49

TheBlueMatt reviewed Feb 28, 2025
View reviewed changes

joostjager force-pushed the log-attribution-failures branch from fc11fe4 to 9f16e69Compare March 3, 2025 09:33

joostjager added 3 commits March 3, 2025 10:44

Remove error side effect from decrypt_onion_error_packet
f307ab7
This function still decrypted the message in-place even when it returned an error. This is confusing for the caller.

Log cases where an onion failure cannot be attributed or interpreted
00f42de
Create more visibility into these edge cases. The non-attributable failure in particular can be used to disrupt sender operation and it is therefore good to at least log these cases clearly.

Properly attribute unreadable failures with a valid hmac
f38244c
This commit fixes a bug where a node penalty was not applied where it should.

joostjager force-pushed the log-attribution-failures branch from 9f16e69 to f38244cCompare March 3, 2025 09:47

joostjager requested a review from TheBlueMatt March 3, 2025 09:50

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

TheBlueMatt approved these changes Mar 4, 2025
View reviewed changes

ldk-reviews-bot requested a review from arik-so March 4, 2025 02:30

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

arik-so merged commit 0216d7d into lightningdevkit:mainMar 4, 2025

optout21 mentioned this pull request Mar 5, 2025
[Early Draft][Splicing] Partial, perform interactive transaction negotiation for new funding #3444
Closed

joostjager mentioned this pull request Jul 3, 2025
Attribution Data and HTLC Hold Times #3908
Open
8 tasks