Skip to content

Log cases where an onion failure cannot be attributed or interpreted #3629

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 4 commits into from
Mar 4, 2025
Merged

Log cases where an onion failure cannot be attributed or interpreted #3629

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
447148d
Add test coverage for process_onion_failure
joostjager Feb 28, 2025
f307ab7
Remove error side effect from decrypt_onion_error_packet
joostjager Mar 3, 2025
00f42de
Log cases where an onion failure cannot be attributed or interpreted
joostjager Feb 28, 2025
f38244c
Properly attribute unreadable failures with a valid hmac
joostjager Feb 28, 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
26 changes: 13 additions & 13 deletions lightning/src/ln/onion_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -931,15 +931,11 @@ pub(crate) struct DecodedOnionFailure {
pub(crate) onion_error_data: Option<Vec<u8>>,
}

/// Note that we always decrypt `packet` in-place here even if the deserialization into
/// [`msgs::DecodedOnionErrorPacket`] ultimately fails.
fn decrypt_onion_error_packet(
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) {
Copy link
Contributor

@arik-so arik-so Mar 4, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@joostjager joostjager Mar 4, 2025

Choose a reason for hiding this comment

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

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.

let ammag = gen_ammag_from_shared_secret(shared_secret.as_ref());
let mut chacha = ChaCha20::new(&ammag, &[0u8; 8]);
chacha.process_in_place(packet);
msgs::DecodedOnionErrorPacket::read(&mut Cursor::new(packet))
}

/// Process failure we got back from upstream on a payment we sent (implying htlc_source is an
Expand Down Expand Up @@ -1021,9 +1017,11 @@ where
{
// Actually parse the onion error data in tests so we can check that blinded hops fail
// back correctly.
let err_packet =
decrypt_onion_error_packet(&mut encrypted_packet, shared_secret)
.unwrap();
decrypt_onion_error_packet(&mut encrypted_packet, shared_secret);
let err_packet = msgs::DecodedOnionErrorPacket::read(&mut Cursor::new(
&encrypted_packet,
))
.unwrap();
error_code_ret = Some(u16::from_be_bytes(
err_packet.failuremsg.get(0..2).unwrap().try_into().unwrap(),
));
Expand All @@ -1044,10 +1042,12 @@ where
let amt_to_forward = htlc_msat - route_hop.fee_msat;
htlc_msat = amt_to_forward;

let err_packet = match decrypt_onion_error_packet(&mut encrypted_packet, shared_secret) {
Ok(p) => p,
Err(_) => return,
};
decrypt_onion_error_packet(&mut encrypted_packet, shared_secret);
let err_packet =
match msgs::DecodedOnionErrorPacket::read(&mut Cursor::new(&encrypted_packet)) {
Ok(p) => p,
Err(_) => return,
};
let um = gen_um_from_shared_secret(shared_secret.as_ref());
let mut hmac = HmacEngine::<Sha256>::new(&um);
hmac.input(&err_packet.encode()[32..]);
Expand Down