Skip to content

Ignore channel updates in onion errors #3083

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 3 commits into from
Jun 7, 2024
+125 −183
Merged

Ignore channel updates in onion errors #3083

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
24c2468
Ignore channel updates in onion errors.
valentinewallace May 23, 2024
85db25b
Fix import order in gossip.rs.
valentinewallace May 28, 2024
abb53d1
Remove now-unused NetworkUpdate::ChannelUpdateMessage.
valentinewallace May 24, 2024
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
11 changes: 2 additions & 9 deletions lightning/src/ln/functional_test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2453,18 +2453,11 @@ pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>(
if let Some(chan_closed) = conditions.expected_blamed_chan_closed {
if let PathFailure::OnPath { network_update: Some(upd) } = failure {
match upd {
NetworkUpdate::ChannelUpdateMessage { ref msg } if !chan_closed => {
if let Some(scid) = conditions.expected_blamed_scid {
assert_eq!(msg.contents.short_channel_id, scid);
}
const CHAN_DISABLED_FLAG: u8 = 2;
assert_eq!(msg.contents.flags & CHAN_DISABLED_FLAG, 0);
},
NetworkUpdate::ChannelFailure { short_channel_id, is_permanent } if chan_closed => {
NetworkUpdate::ChannelFailure { short_channel_id, is_permanent } => {
if let Some(scid) = conditions.expected_blamed_scid {
assert_eq!(*short_channel_id, scid);
}
assert!(is_permanent);
assert_eq!(*is_permanent, chan_closed);
},
_ => panic!("Unexpected update type"),
}
Expand Down
64 changes: 37 additions & 27 deletions lightning/src/ln/onion_route_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,11 +180,6 @@ fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(
if expected_channel_update.is_some() {
match network_update {
Some(update) => match update {
&NetworkUpdate::ChannelUpdateMessage { .. } => {
if let NetworkUpdate::ChannelUpdateMessage { .. } = expected_channel_update.unwrap() {} else {
panic!("channel_update not found!");
}
},
&NetworkUpdate::ChannelFailure { ref short_channel_id, ref is_permanent } => {
if let NetworkUpdate::ChannelFailure { short_channel_id: ref expected_short_channel_id, is_permanent: ref expected_is_permanent } = expected_channel_update.unwrap() {
assert!(*short_channel_id == *expected_short_channel_id);
Expand Down Expand Up @@ -300,12 +295,13 @@ fn test_fee_failures() {
claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage_success);

// If the hop gives fee_insufficient but enough fees were provided, then the previous hop
// malleated the payment before forwarding, taking funds when they shouldn't have.
// malleated the payment before forwarding, taking funds when they shouldn't have. However,
// because we ignore channel update contents, we will still blame the 2nd channel.
let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]);
let short_channel_id = channels[0].0.contents.short_channel_id;
let short_channel_id = channels[1].0.contents.short_channel_id;
run_onion_failure_test("fee_insufficient", 0, &nodes, &route, &payment_hash, &payment_secret, |msg| {
msg.amount_msat -= 1;
}, || {}, true, Some(UPDATE|12), Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent: true}), Some(short_channel_id));
}, || {}, true, Some(UPDATE|12), Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent: false}), Some(short_channel_id));

// In an earlier version, we spuriously failed to forward payments if the expected feerate
// changed between the channel open and the payment.
Expand Down Expand Up @@ -478,7 +474,9 @@ fn test_onion_failure() {
let session_priv = SecretKey::from_slice(&[3; 32]).unwrap();
let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap();
msg.reason = onion_utils::build_first_hop_failure_packet(onion_keys[0].shared_secret.as_ref(), UPDATE|7, &err_data);
}, ||{}, true, Some(UPDATE|7), Some(NetworkUpdate::ChannelUpdateMessage{msg: chan_update.clone()}), Some(short_channel_id));
}, ||{}, true, Some(UPDATE|7),
Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent: false }),
Some(short_channel_id));

// Check we can still handle onion failures that include channel updates without a type prefix
let err_data_without_type = chan_update.encode_with_len();
Expand All @@ -488,7 +486,9 @@ fn test_onion_failure() {
let session_priv = SecretKey::from_slice(&[3; 32]).unwrap();
let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap();
msg.reason = onion_utils::build_first_hop_failure_packet(onion_keys[0].shared_secret.as_ref(), UPDATE|7, &err_data_without_type);
}, ||{}, true, Some(UPDATE|7), Some(NetworkUpdate::ChannelUpdateMessage{msg: chan_update}), Some(short_channel_id));
}, ||{}, true, Some(UPDATE|7),
Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent: false }),
Some(short_channel_id));

let short_channel_id = channels[1].0.contents.short_channel_id;
run_onion_failure_test_with_fail_intercept("permanent_channel_failure", 100, &nodes, &route, &payment_hash, &payment_secret, |msg| {
Expand Down Expand Up @@ -523,7 +523,9 @@ fn test_onion_failure() {
let mut bogus_route = route.clone();
let route_len = bogus_route.paths[0].hops.len();
bogus_route.paths[0].hops[route_len-1].fee_msat = amt_to_forward;
run_onion_failure_test("amount_below_minimum", 0, &nodes, &bogus_route, &payment_hash, &payment_secret, |_| {}, ||{}, true, Some(UPDATE|11), Some(NetworkUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy(short_channel_id)}), Some(short_channel_id));
run_onion_failure_test("amount_below_minimum", 0, &nodes, &bogus_route, &payment_hash, &payment_secret, |_| {}, ||{}, true, Some(UPDATE|11),
Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent: false }),
Some(short_channel_id));

// Clear pending payments so that the following positive test has the correct payment hash.
for node in nodes.iter() {
Expand All @@ -535,24 +537,28 @@ fn test_onion_failure() {
let preimage = send_along_route(&nodes[0], bogus_route, &[&nodes[1], &nodes[2]], amt_to_forward+1).0;
claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], preimage);

let short_channel_id = channels[0].0.contents.short_channel_id;
// We ignore channel update contents in onion errors, so will blame the 2nd channel even though
// the first node is the one that messed up.
let short_channel_id = channels[1].0.contents.short_channel_id;
run_onion_failure_test("fee_insufficient", 0, &nodes, &route, &payment_hash, &payment_secret, |msg| {
msg.amount_msat -= 1;
}, || {}, true, Some(UPDATE|12), Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent: true}), Some(short_channel_id));
}, || {}, true, Some(UPDATE|12), Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent: false}), Some(short_channel_id));

let short_channel_id = channels[0].0.contents.short_channel_id;
let short_channel_id = channels[1].0.contents.short_channel_id;
run_onion_failure_test("incorrect_cltv_expiry", 0, &nodes, &route, &payment_hash, &payment_secret, |msg| {
// need to violate: cltv_expiry - cltv_expiry_delta >= outgoing_cltv_value
msg.cltv_expiry -= 1;
}, || {}, true, Some(UPDATE|13), Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent: true}), Some(short_channel_id));
}, || {}, true, Some(UPDATE|13), Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent: false}), Some(short_channel_id));

let short_channel_id = channels[1].0.contents.short_channel_id;
run_onion_failure_test("expiry_too_soon", 0, &nodes, &route, &payment_hash, &payment_secret, |msg| {
let height = msg.cltv_expiry - CLTV_CLAIM_BUFFER - LATENCY_GRACE_PERIOD_BLOCKS + 1;
connect_blocks(&nodes[0], height - nodes[0].best_block_info().1);
connect_blocks(&nodes[1], height - nodes[1].best_block_info().1);
connect_blocks(&nodes[2], height - nodes[2].best_block_info().1);
}, ||{}, true, Some(UPDATE|14), Some(NetworkUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy(short_channel_id)}), Some(short_channel_id));
}, ||{}, true, Some(UPDATE|14),
Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent: false }),
Some(short_channel_id));

run_onion_failure_test("unknown_payment_hash", 2, &nodes, &route, &payment_hash, &payment_secret, |_| {}, || {
nodes[2].node.fail_htlc_backwards(&payment_hash);
Expand Down Expand Up @@ -596,7 +602,9 @@ fn test_onion_failure() {
// disconnect event to the channel between nodes[1] ~ nodes[2]
nodes[1].node.peer_disconnected(&nodes[2].node.get_our_node_id());
nodes[2].node.peer_disconnected(&nodes[1].node.get_our_node_id());
}, true, Some(UPDATE|7), Some(NetworkUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy(short_channel_id)}), Some(short_channel_id));
}, true, Some(UPDATE|7),
Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent: false }),
Some(short_channel_id));
run_onion_failure_test("channel_disabled", 0, &nodes, &route, &payment_hash, &payment_secret, |_| {}, || {
// disconnect event to the channel between nodes[1] ~ nodes[2]
for _ in 0..DISABLE_GOSSIP_TICKS + 1 {
Expand All @@ -605,7 +613,9 @@ fn test_onion_failure() {
}
nodes[1].node.get_and_clear_pending_msg_events();
nodes[2].node.get_and_clear_pending_msg_events();
}, true, Some(UPDATE|20), Some(NetworkUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy(short_channel_id)}), Some(short_channel_id));
}, true, Some(UPDATE|20),
Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent: false }),
Some(short_channel_id));
reconnect_nodes(ReconnectArgs::new(&nodes[1], &nodes[2]));

run_onion_failure_test("expiry_too_far", 0, &nodes, &route, &payment_hash, &payment_secret, |msg| {
Expand Down Expand Up @@ -844,9 +854,9 @@ fn do_test_onion_failure_stale_channel_update(announced_channel: bool) {
// We'll be attempting to route payments using the default ChannelUpdate for channels. This will
// lead to onion failures at the first hop once we update the ChannelConfig for the
// second hop.
let expect_onion_failure = |name: &str, error_code: u16, channel_update: &msgs::ChannelUpdate| {
let expect_onion_failure = |name: &str, error_code: u16| {
let short_channel_id = channel_to_update.1;
let network_update = NetworkUpdate::ChannelUpdateMessage { msg: channel_update.clone() };
let network_update = NetworkUpdate::ChannelFailure { short_channel_id, is_permanent: false };
run_onion_failure_test(
name, 0, &nodes, &route, &payment_hash, &payment_secret, |_| {}, || {}, true,
Some(error_code), Some(network_update), Some(short_channel_id),
Expand Down Expand Up @@ -878,7 +888,7 @@ fn do_test_onion_failure_stale_channel_update(announced_channel: bool) {
// Connect a block, which should expire the previous config, leading to a failure when
// forwarding the HTLC.
expire_prev_config();
expect_onion_failure("fee_insufficient", UPDATE|12, &msg);
expect_onion_failure("fee_insufficient", UPDATE|12);

// Redundant updates should not trigger a new ChannelUpdate.
assert!(update_and_get_channel_update(&config, false, None, false).is_none());
Expand All @@ -891,15 +901,15 @@ fn do_test_onion_failure_stale_channel_update(announced_channel: bool) {
// new ChannelUpdate.
config.forwarding_fee_base_msat = default_config.forwarding_fee_base_msat;
config.cltv_expiry_delta = u16::max_value();
let msg = update_and_get_channel_update(&config, true, Some(&msg), true).unwrap();
expect_onion_failure("incorrect_cltv_expiry", UPDATE|13, &msg);
assert!(update_and_get_channel_update(&config, true, Some(&msg), true).is_some());
expect_onion_failure("incorrect_cltv_expiry", UPDATE|13);

// Reset the proportional fee and increase the CLTV expiry delta which should trigger a new
// ChannelUpdate.
config.cltv_expiry_delta = default_config.cltv_expiry_delta;
config.forwarding_fee_proportional_millionths = u32::max_value();
let msg = update_and_get_channel_update(&config, true, Some(&msg), true).unwrap();
expect_onion_failure("fee_insufficient", UPDATE|12, &msg);
assert!(update_and_get_channel_update(&config, true, Some(&msg), true).is_some());
expect_onion_failure("fee_insufficient", UPDATE|12);

// To test persistence of the updated config, we'll re-initialize the ChannelManager.
let config_after_restart = {
Expand Down Expand Up @@ -1530,10 +1540,10 @@ fn do_test_phantom_dust_exposure_failure(multiplier_dust_limit: bool) {
err_data.extend_from_slice(&channel.1.encode());

let mut fail_conditions = PaymentFailedConditions::new()
.blamed_scid(channel.0.contents.short_channel_id)
.blamed_scid(route.paths[0].hops.last().as_ref().unwrap().short_channel_id)
.blamed_chan_closed(false)
.expected_htlc_error_data(0x1000 | 7, &err_data);
expect_payment_failed_conditions(&nodes[0], payment_hash, false, fail_conditions);
expect_payment_failed_conditions(&nodes[0], payment_hash, false, fail_conditions);
}

#[test]
Expand Down
105 changes: 7 additions & 98 deletions lightning/src/ln/onion_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ use crate::ln::channelmanager::{HTLCSource, RecipientOnionFields};
use crate::ln::features::{ChannelFeatures, NodeFeatures};
use crate::ln::msgs;
use crate::ln::types::{PaymentHash, PaymentPreimage};
use crate::ln::wire::Encode;
use crate::routing::gossip::NetworkUpdate;
use crate::routing::router::{Path, RouteHop, RouteParameters};
use crate::sign::NodeSigner;
Expand Down Expand Up @@ -806,106 +805,16 @@ where
{
let update_len =
u16::from_be_bytes(update_len_slice.try_into().expect("len is 2")) as usize;
if let Some(mut update_slice) = err_packet
if err_packet
.failuremsg
.get(debug_field_size + 4..debug_field_size + 4 + update_len)
.is_some()
{
// Historically, the BOLTs were unclear if the message type
// bytes should be included here or not. The BOLTs have now
// been updated to indicate that they *are* included, but many
// nodes still send messages without the type bytes, so we
// support both here.
// TODO: Switch to hard require the type prefix, as the current
// permissiveness introduces the (although small) possibility
// that we fail to decode legitimate channel updates that
// happen to start with ChannelUpdate::TYPE, i.e., [0x01, 0x02].
if update_slice.len() > 2
&& update_slice[0..2] == msgs::ChannelUpdate::TYPE.to_be_bytes()
{
update_slice = &update_slice[2..];
} else {
log_trace!(logger, "Failure provided features a channel update without type prefix. Deprecated, but allowing for now.");
}
let update_opt = msgs::ChannelUpdate::read(&mut Cursor::new(&update_slice));
if update_opt.is_ok() || update_slice.is_empty() {
// if channel_update should NOT have caused the failure:
// MAY treat the channel_update as invalid.
let is_chan_update_invalid = match error_code & 0xff {
7 => false,
11 => {
update_opt.is_ok()
&& amt_to_forward
> update_opt.as_ref().unwrap().contents.htlc_minimum_msat
},
12 => {
update_opt.is_ok()
&& amt_to_forward
.checked_mul(
update_opt
.as_ref()
.unwrap()
.contents
.fee_proportional_millionths as u64,
)
.map(|prop_fee| prop_fee / 1_000_000)
.and_then(|prop_fee| {
prop_fee.checked_add(
update_opt.as_ref().unwrap().contents.fee_base_msat
as u64,
)
})
.map(|fee_msats| route_hop.fee_msat >= fee_msats)
.unwrap_or(false)
},
13 => {
update_opt.is_ok()
&& route_hop.cltv_expiry_delta as u16
>= update_opt.as_ref().unwrap().contents.cltv_expiry_delta
},
14 => false, // expiry_too_soon; always valid?
20 => update_opt.as_ref().unwrap().contents.flags & 2 == 0,
_ => false, // unknown error code; take channel_update as valid
};
if is_chan_update_invalid {
// This probably indicates the node which forwarded
// to the node in question corrupted something.
network_update = Some(NetworkUpdate::ChannelFailure {
short_channel_id: route_hop.short_channel_id,
is_permanent: true,
});
} else {
if let Ok(chan_update) = update_opt {
// Make sure the ChannelUpdate contains the expected
// short channel id.
if failing_route_hop.short_channel_id
== chan_update.contents.short_channel_id
{
short_channel_id = Some(failing_route_hop.short_channel_id);
} else {
log_info!(logger, "Node provided a channel_update for which it was not authoritative, ignoring.");
}
network_update =
Some(NetworkUpdate::ChannelUpdateMessage { msg: chan_update })
} else {
// The node in question intentionally encoded a 0-length channel update. This is
// likely due to https://github.com/ElementsProject/lightning/issues/6200.
short_channel_id = Some(failing_route_hop.short_channel_id);
network_update = Some(NetworkUpdate::ChannelFailure {
short_channel_id: failing_route_hop.short_channel_id,
is_permanent: false,
});
}
};
} else {
// If the channel_update had a non-zero length (i.e. was
// present) but we couldn't read it, treat it as a total
// node failure.
log_info!(
logger,
"Failed to read a channel_update of len {} in an onion",
update_slice.len()
);
}
network_update = Some(NetworkUpdate::ChannelFailure {
short_channel_id: failing_route_hop.short_channel_id,
is_permanent: false,
});
short_channel_id = Some(failing_route_hop.short_channel_id);
}
}
if network_update.is_none() {
Expand Down
Loading
Loading