Skip to content

Revert attribution of failures #3817

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 10, 2025
Merged

Revert attribution of failures #3817

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
74e6b57
Refactor attribution data mutation test
joostjager Jun 3, 2025
567ac64
Revert attribution of failures
joostjager Jun 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
187 changes: 124 additions & 63 deletions lightning/src/ln/onion_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -979,6 +979,8 @@ mod fuzzy_onion_utils {
pub(crate) onion_error_code: Option<LocalHTLCFailureReason>,
#[cfg(any(test, feature = "_test_utils"))]
pub(crate) onion_error_data: Option<Vec<u8>>,
#[cfg(test)]
pub(crate) attribution_failed_channel: Option<u64>,
}
}
#[cfg(fuzzing)]
Expand Down Expand Up @@ -1053,6 +1055,8 @@ where
onion_error_code: None,
#[cfg(any(test, feature = "_test_utils"))]
onion_error_data: None,
#[cfg(test)]
attribution_failed_channel: None,
};
}

Expand Down Expand Up @@ -1120,6 +1124,9 @@ where
// the first 20 hops. Determine the number of hops to be used for attribution data.
let attributable_hop_count = usize::min(path.hops.len(), MAX_HOPS);

// Keep track of the first hop for which the attribution data failed to check out.
let mut attribution_failed_channel = None;

// Handle packed channel/node updates for passing back for the route handler
let mut iter = nontrampolines.chain(trampolines.into_iter().flatten()).enumerate().peekable();
while let Some((route_hop_idx, (route_hop_option, shared_secret))) = iter.next() {
Expand Down Expand Up @@ -1191,44 +1198,53 @@ where

let um = gen_um_from_shared_secret(shared_secret.as_ref());

// Check attr error HMACs if present.
if let Some(ref mut attribution_data) = encrypted_packet.attribution_data {
// Only consider hops in the regular path for attribution data. Failures in a blinded path are not attributable.
if route_hop_idx < attributable_hop_count {
// Calculate position relative to the last attributable hop. The last attributable hop is at position 0.
// The failure node does not need to come from the last attributable hop, but we need to look at the
// chain of HMACs that does include all data up to the last attributable hop. For a more nearby failure,
// the verified HMACs will include some zero padding data. Failures beyond the last attributable hop
// will not be attributable.
let position = attributable_hop_count - route_hop_idx - 1;
let hold_time = attribution_data.verify(
&encrypted_packet.data,
shared_secret.as_ref(),
position,
);
if let Some(hold_time) = hold_time {
hop_hold_times.push(hold_time);

log_debug!(logger, "Htlc hold time at pos {}: {} ms", route_hop_idx, hold_time);

// Shift attribution data to prepare for processing the next hop.
attribution_data.shift_left();
} else {
res = Some(FailureLearnings {
network_update: None,
short_channel_id: route_hop.short_channel_id(),
payment_failed_permanently: false,
failed_within_blinded_path: false,
});

log_debug!(
logger,
"Invalid HMAC in attribution data for node at pos {}",
route_hop_idx
// Only check attribution when an attribution data failure has not yet occurred.
if attribution_failed_channel.is_none() {
// Check attr error HMACs if present.
if let Some(ref mut attribution_data) = encrypted_packet.attribution_data {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-existing, but don't we need to check whether the node supports attributable failures here?

As-is we would:

  • Set attribution_failed_channel for a node that doesn't support attributable failures
  • Not set attribution_failed_channel for a node that does support attributable failures but didn't include them

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is currently written as if there is universal support for it, but then - and this is what this PR changes - we don't act on it (yet).

Not set attribution_failed_channel for a node that does support attributable failures but didn't include them

I think we do now, because when we don't receive attribution_data from the first hop, we blame that hop?

I am not completely sure how to weave the feature bit into this when we have this deployed. If by that time the large majority supports it, maybe the logic as is is fine? If the original failure field is readable, we don't even need to look at attribution_data because the failure is already attributable. So we'd still only make a decision for that random data vulnerability that currently doesn't seem to be exploited.

But interested to hear how you see this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we do now, because when we don't receive attribution_data from the first hop, we blame that hop?

👍

I am not completely sure how to weave the feature bit into this when we have this deployed

Wouldn't we just check RouteHop.node_features and then attribute if there's no attribution_data?

But interested to hear how you see this.

It just didn't seem right to blame a node that doesn't support the feature, but maybe it's a good incentive to upgrade (if you don't you're still not avoiding blame).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't we just check RouteHop.node_features and then attribute if there's no attribution_data?

That indeed seems the straight forward way to do it. Punish node pairs for not supporting what they advertize. I just don't know if it is still necessary when we get there. Perhaps so many nodes have upgraded already, that we can punish regardless of the feature bit. Also because this code path only becomes active when the original failure message can't be deciphered, so it remains the exception either way.

// Only consider hops in the regular path for attribution data. Failures in a blinded path are not
// attributable.
if route_hop_idx < attributable_hop_count {
// Calculate position relative to the last attributable hop. The last attributable hop is at
// position 0. The failure node does not need to come from the last attributable hop, but we need to
// look at the chain of HMACs that does include all data up to the last attributable hop. For a more
// nearby failure, the verified HMACs will include some zero padding data. Failures beyond the last
// attributable hop will not be attributable.
let position = attributable_hop_count - route_hop_idx - 1;
let hold_time = attribution_data.verify(
&encrypted_packet.data,
shared_secret.as_ref(),
position,
);
if let Some(hold_time) = hold_time {
hop_hold_times.push(hold_time);

log_debug!(
logger,
"Htlc hold time at pos {}: {} ms",
route_hop_idx,
hold_time
);

break;
// Shift attribution data to prepare for processing the next hop.
attribution_data.shift_left();
} else {
// Store the failing hop, but continue processing the failure for the remaining hops. During the
// upgrade period, it may happen that nodes along the way drop attribution data. If the legacy
// failure is still valid, it should be processed normally.
attribution_failed_channel = route_hop.short_channel_id();

log_debug!(
logger,
"Invalid HMAC in attribution data for node at pos {}",
route_hop_idx
);
}
}
} else {
// When no attribution data is provided at all, blame the first hop when the failing node turns out to
// be unindentifiable.
attribution_failed_channel = route_hop.short_channel_id();
}
}

Expand Down Expand Up @@ -1421,14 +1437,17 @@ where
onion_error_code: _error_code_ret,
#[cfg(any(test, feature = "_test_utils"))]
onion_error_data: _error_packet_ret,
#[cfg(test)]
attribution_failed_channel,
}
} else {
// only not set either packet unparseable or hmac does not match with any
// payment not retryable only when garbage is from the final node
log_warn!(
logger,
"Non-attributable failure encountered on route {}",
path.hops.iter().map(|h| h.pubkey.to_string()).collect::<Vec<_>>().join("->")
"Non-attributable failure encountered on route {}. Attributation data failed for channel {}",
path.hops.iter().map(|h| h.pubkey.to_string()).collect::<Vec<_>>().join("->"),
attribution_failed_channel.unwrap_or_default(),
);

DecodedOnionFailure {
Expand All @@ -1441,6 +1460,8 @@ where
onion_error_code: None,
#[cfg(any(test, feature = "_test_utils"))]
onion_error_data: None,
#[cfg(test)]
attribution_failed_channel,
}
}
}
Expand Down Expand Up @@ -2046,6 +2067,8 @@ impl HTLCFailReason {
onion_error_code: Some(*failure_reason),
#[cfg(any(test, feature = "_test_utils"))]
onion_error_data: Some(data.clone()),
#[cfg(test)]
attribution_failed_channel: None,
}
} else {
unreachable!();
Expand Down Expand Up @@ -2787,6 +2810,7 @@ fn process_failure_packet(

#[cfg(test)]
mod tests {
use core::iter;
use std::sync::Arc;

use crate::io;
Expand Down Expand Up @@ -3122,20 +3146,40 @@ mod tests {
const FAILURE_MESSAGE_LEN: usize = 1060;

for mutating_node in 0..5 {
for mutated_index in
0..FAILURE_MESSAGE_LEN + HOLD_TIME_LEN * MAX_HOPS + HMAC_LEN * HMAC_COUNT
let attribution_data_mutations = (0..HOLD_TIME_LEN * MAX_HOPS)
.map(AttributionDataMutationType::HoldTimes)
.chain((0..HMAC_LEN * HMAC_COUNT).map(AttributionDataMutationType::Hmacs));

let failure_mutations = (0..FAILURE_MESSAGE_LEN).map(MutationType::FailureMessage);

for mutation_type in failure_mutations
.chain(attribution_data_mutations.map(MutationType::AttributionData))
.chain(iter::once(MutationType::DropAttributionData))
{
let mutation = Mutation { node: mutating_node, index: mutated_index };
// If the mutation is in the attribution data and not in the failure message itself, the invalid
// attribution data should be ignored and the failure should still surface.
let failure_ok = matches!(mutation_type, MutationType::DropAttributionData)
|| matches!(mutation_type, MutationType::AttributionData(_));

let mutation = Mutation { node: mutating_node, mutation_type };
let decrypted_failure =
test_attributable_failure_packet_onion_with_mutation(Some(mutation));

if decrypted_failure.onion_error_code
== Some(LocalHTLCFailureReason::IncorrectPaymentDetails)
{
if failure_ok {
assert_eq!(
decrypted_failure.onion_error_code,
Some(LocalHTLCFailureReason::IncorrectPaymentDetails)
);
continue;
}

assert!(decrypted_failure.short_channel_id == Some(mutating_node as u64));
// Currently attribution data isn't used yet to identify the failing node, because this would hinder the
// upgrade path.
assert!(decrypted_failure.short_channel_id.is_none());

// Assert that attribution data is interpreted correctly via a test-only field.
assert!(decrypted_failure.attribution_failed_channel == Some(mutating_node as u64));

assert_eq!(decrypted_failure.hold_times, [5, 4, 3, 2, 1][..mutating_node]);
}
}
Expand All @@ -3151,9 +3195,20 @@ mod tests {
assert_eq!(decrypted_failure.hold_times, [5, 4, 3, 2, 1]);
}

enum AttributionDataMutationType {
HoldTimes(usize),
Hmacs(usize),
}

enum MutationType {
FailureMessage(usize),
AttributionData(AttributionDataMutationType),
DropAttributionData,
}

struct Mutation {
node: usize,
index: usize,
mutation_type: MutationType,
}

fn test_attributable_failure_packet_onion_with_mutation(
Expand Down Expand Up @@ -3222,24 +3277,30 @@ mod tests {
EXPECTED_MESSAGES[0].assert_eq(&onion_error);

let mut mutated = false;
let mutate_packet = |packet: &mut OnionErrorPacket, mutated_index| {
let data_len = packet.data.len();
if mutated_index < data_len {
// Mutate legacy failure message.
packet.data[mutated_index] ^= 1;
} else if mutated_index < data_len + HOLD_TIME_LEN * MAX_HOPS {
// Mutate hold times.
packet.attribution_data.as_mut().unwrap().hold_times[mutated_index - data_len] ^= 1;
} else {
// Mutate HMACs.
packet.attribution_data.as_mut().unwrap().hmacs
[mutated_index - data_len - HOLD_TIME_LEN * MAX_HOPS] ^= 1;
let mutate_packet = |packet: &mut OnionErrorPacket, mutation_type: &MutationType| {
match mutation_type {
MutationType::FailureMessage(i) => {
// Mutate legacy failure message.
packet.data[*i] ^= 1;
},
MutationType::AttributionData(AttributionDataMutationType::HoldTimes(i)) => {
// Mutate hold times.
packet.attribution_data.as_mut().unwrap().hold_times[*i] ^= 1;
},
MutationType::AttributionData(AttributionDataMutationType::Hmacs(i)) => {
// Mutate hold times.
packet.attribution_data.as_mut().unwrap().hmacs[*i] ^= 1;
},
MutationType::DropAttributionData => {
// Drop attribution data completely. This simulates a node that does not support the feature.
packet.attribution_data = None;
},
}
};

if let Some(Mutation { node, index }) = mutation {
if let Some(Mutation { node, ref mutation_type }) = mutation {
if node == 4 {
mutate_packet(&mut onion_error, index);
mutate_packet(&mut onion_error, mutation_type);
mutated = true;
}
}
Expand All @@ -3250,9 +3311,9 @@ mod tests {
process_failure_packet(&mut onion_error, shared_secret, hold_time);
super::crypt_failure_packet(shared_secret, &mut onion_error);

if let Some(Mutation { node, index }) = mutation {
if let Some(Mutation { node, ref mutation_type }) = mutation {
if node == idx {
mutate_packet(&mut onion_error, index);
mutate_packet(&mut onion_error, mutation_type);
mutated = true;
}
}
Expand Down Expand Up @@ -3542,7 +3603,7 @@ mod tests {
// With attributable failures, it should still be possible to identify the failing node.
let logger: TestLogger = TestLogger::new();
let decrypted_failure = test_failure_attribution(&logger, onion_error_packet);
assert_eq!(decrypted_failure.short_channel_id, Some(0));
assert_eq!(decrypted_failure.attribution_failed_channel, Some(0));
}

#[test]
Expand Down Expand Up @@ -3610,7 +3671,7 @@ mod tests {
// Expect attribution up to hop 20.
let expected_failed_chan =
if failure_pos < MAX_HOPS { Some(failure_pos as u64) } else { None };
assert_eq!(decrypted_failure.short_channel_id, expected_failed_chan);
assert_eq!(decrypted_failure.attribution_failed_channel, expected_failed_chan);
}
}

Expand Down