Skip to content
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

Encode & test channel_update message type in failure messages. #1465

Merged
merged 1 commit into from
May 9, 2022
Merged

Encode & test channel_update message type in failure messages. #1465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 7 additions & 4 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ use ln::msgs;
use ln::msgs::NetAddress;
use ln::onion_utils;
use ln::msgs::{ChannelMessageHandler, DecodeError, LightningError, MAX_VALUE_MSAT, OptionalField};
use ln::wire::Encode;
use chain::keysinterface::{Sign, KeysInterface, KeysManager, InMemorySigner, Recipient};
use util::config::UserConfig;
use util::events::{EventHandler, EventsProvider, MessageSendEvent, MessageSendEventsProvider, ClosureReason};
Expand Down Expand Up @@ -2249,7 +2250,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
break None;
}
{
let mut res = VecWriter(Vec::with_capacity(chan_update.serialized_length() + 8 + 2));
let mut res = VecWriter(Vec::with_capacity(chan_update.serialized_length() + 2 + 8 + 2));
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to have a very tiny comment that documents what this sum will do!

This make it harder to read for people that switch implementation during the day

Copy link
Contributor Author

@tnull tnull May 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I would like to have a very tiny comment that documents what this sum will do!

I'm not sure that a comment would provide much more information that is not self-explanatory? Maybe chan_update.serialized_length() + mem::size_of<u16>() + mem::size_of<u64> + mem::size_of<u16>() would be more expressive, but would also render the line unnecessarily long? Also, this only reserves a certain amount of vector capacity, nothing would really break if the calculation would be off by a byte or two.

This make it harder to read for people that switch implementation during the day

Mh, how does this relate to other implementations?

Copy link
Contributor

Choose a reason for hiding this comment

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

Mh? I mean a comment to explain the meaning of the number // to the serialization length we sum up: 2 (flag) + 8 (whatever) + 2(whatever)

BTW, it is not relevent!

if let Some(chan_update) = chan_update {
if code == 0x1000 | 11 || code == 0x1000 | 12 {
msg.amount_msat.write(&mut res).expect("Writes cannot fail");
Expand All @@ -2261,7 +2262,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
// TODO: underspecified, follow https://github.com/lightningnetwork/lightning-rfc/issues/791
0u16.write(&mut res).expect("Writes cannot fail");
}
(chan_update.serialized_length() as u16).write(&mut res).expect("Writes cannot fail");
(chan_update.serialized_length() as u16 + 2).write(&mut res).expect("Writes cannot fail");
msgs::ChannelUpdate::TYPE.write(&mut res).expect("Writes cannot fail");
chan_update.write(&mut res).expect("Writes cannot fail");
}
return_err!(err, code, &res.0[..]);
Expand Down Expand Up @@ -3543,12 +3545,13 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
fn get_htlc_temp_fail_err_and_data(&self, desired_err_code: u16, scid: u64, chan: &Channel<Signer>) -> (u16, Vec<u8>) {
debug_assert_eq!(desired_err_code & 0x1000, 0x1000);
if let Ok(upd) = self.get_channel_update_for_onion(scid, chan) {
let mut enc = VecWriter(Vec::with_capacity(upd.serialized_length() + 4));
let mut enc = VecWriter(Vec::with_capacity(upd.serialized_length() + 6));
if desired_err_code == 0x1000 | 20 {
// TODO: underspecified, follow https://github.com/lightning/bolts/issues/791
0u16.write(&mut enc).expect("Writes cannot fail");
}
(upd.serialized_length() as u16).write(&mut enc).expect("Writes cannot fail");
(upd.serialized_length() as u16 + 2).write(&mut enc).expect("Writes cannot fail");
tnull marked this conversation as resolved.
Show resolved Hide resolved
msgs::ChannelUpdate::TYPE.write(&mut enc).expect("Writes cannot fail");
upd.write(&mut enc).expect("Writes cannot fail");
(desired_err_code, enc.0)
} else {
Expand Down
29 changes: 25 additions & 4 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 @@ -21,6 +21,7 @@ use routing::router::{get_route, PaymentParameters, Route, RouteHint, RouteHintH
use ln::features::{InitFeatures, InvoiceFeatures, NodeFeatures};
use ln::msgs;
use ln::msgs::{ChannelMessageHandler, ChannelUpdate, OptionalField};
use ln::wire::Encode;
use util::events::{Event, MessageSendEvent, MessageSendEventsProvider};
use util::ser::{Writeable, Writer};
use util::{byte_utils, test_utils};
Expand Down Expand Up @@ -438,13 +439,29 @@ fn test_onion_failure() {
Some(BADONION|PERM|6), None, Some(short_channel_id));

let short_channel_id = channels[1].0.contents.short_channel_id;
let chan_update = ChannelUpdate::dummy(short_channel_id);

let mut err_data = Vec::new();
err_data.extend_from_slice(&(chan_update.serialized_length() as u16 + 2).to_be_bytes());
err_data.extend_from_slice(&ChannelUpdate::TYPE.to_be_bytes());
err_data.extend_from_slice(&chan_update.encode());
run_onion_failure_test_with_fail_intercept("temporary_channel_failure", 100, &nodes, &route, &payment_hash, &payment_secret, |msg| {
msg.amount_msat -= 1;
}, |msg| {
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));

// 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();
run_onion_failure_test_with_fail_intercept("temporary_channel_failure", 100, &nodes, &route, &payment_hash, &payment_secret, |msg| {
msg.amount_msat -= 1;
}, |msg| {
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, &ChannelUpdate::dummy(short_channel_id).encode_with_len()[..]);
}, ||{}, true, Some(UPDATE|7), Some(NetworkUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy(short_channel_id)}), Some(short_channel_id));
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));

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 @@ -1097,11 +1114,15 @@ fn test_phantom_dust_exposure_failure() {
commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false);

// Ensure the payment fails with the expected error.
let mut error_data = channel.1.encode_with_len();
let mut err_data = Vec::new();
err_data.extend_from_slice(&(channel.1.serialized_length() as u16 + 2).to_be_bytes());
err_data.extend_from_slice(&ChannelUpdate::TYPE.to_be_bytes());
err_data.extend_from_slice(&channel.1.encode());

let mut fail_conditions = PaymentFailedConditions::new()
.blamed_scid(channel.0.contents.short_channel_id)
.blamed_chan_closed(false)
.expected_htlc_error_data(0x1000 | 7, &error_data);
.expected_htlc_error_data(0x1000 | 7, &err_data);
expect_payment_failed_conditions!(nodes[0], payment_hash, false, fail_conditions);
}

Expand Down
23 changes: 20 additions & 3 deletions lightning/src/ln/onion_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use ln::{PaymentHash, PaymentPreimage, PaymentSecret};
use ln::channelmanager::HTLCSource;
use ln::msgs;
use ln::wire::Encode;
use routing::network_graph::NetworkUpdate;
use routing::router::RouteHop;
use util::chacha20::{ChaCha20, ChaChaReader};
Expand Down Expand Up @@ -404,7 +405,21 @@ pub(super) fn process_onion_failure<T: secp256k1::Signing, L: Deref>(secp_ctx: &
else if error_code & UPDATE == UPDATE {
if let Some(update_len_slice) = err_packet.failuremsg.get(debug_field_size+2..debug_field_size+4) {
let update_len = u16::from_be_bytes(update_len_slice.try_into().expect("len is 2")) as usize;
if let Some(update_slice) = err_packet.failuremsg.get(debug_field_size + 4..debug_field_size + 4 + update_len) {
if let Some(mut update_slice) = err_packet.failuremsg.get(debug_field_size + 4..debug_field_size + 4 + update_len) {
// 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.
tnull marked this conversation as resolved.
Show resolved Hide resolved
// 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.");
}
if let Ok(chan_update) = msgs::ChannelUpdate::read(&mut Cursor::new(&update_slice)) {
// if channel_update should NOT have caused the failure:
// MAY treat the channel_update as invalid.
Expand Down Expand Up @@ -434,6 +449,8 @@ pub(super) fn process_onion_failure<T: secp256k1::Signing, L: Deref>(secp_ctx: &
// 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,
Expand Down Expand Up @@ -478,10 +495,10 @@ pub(super) fn process_onion_failure<T: secp256k1::Signing, L: Deref>(secp_ctx: &

let (description, title) = errors::get_onion_error_description(error_code);
if debug_field_size > 0 && err_packet.failuremsg.len() >= 4 + debug_field_size {
log_warn!(logger, "Onion Error[from {}: {}({:#x}) {}({})] {}", route_hop.pubkey, title, error_code, debug_field, log_bytes!(&err_packet.failuremsg[4..4+debug_field_size]), description);
log_info!(logger, "Onion Error[from {}: {}({:#x}) {}({})] {}", route_hop.pubkey, title, error_code, debug_field, log_bytes!(&err_packet.failuremsg[4..4+debug_field_size]), description);
}
else {
log_warn!(logger, "Onion Error[from {}: {}({:#x})] {}", route_hop.pubkey, title, error_code, description);
log_info!(logger, "Onion Error[from {}: {}({:#x})] {}", route_hop.pubkey, title, error_code, description);
}
} else {
// Useless packet that we can't use but it passed HMAC, so it
Expand Down
14 changes: 11 additions & 3 deletions lightning/src/ln/priv_short_conf_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ use routing::network_graph::RoutingFees;
use routing::router::{PaymentParameters, RouteHint, RouteHintHop};
use ln::features::{InitFeatures, InvoiceFeatures};
use ln::msgs;
use ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, OptionalField};
use ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, OptionalField, ChannelUpdate};
use ln::wire::Encode;
use util::enforcing_trait_impls::EnforcingSigner;
use util::events::{Event, MessageSendEvent, MessageSendEventsProvider};
use util::config::UserConfig;
Expand Down Expand Up @@ -531,9 +532,14 @@ fn test_scid_alias_returned() {
let signature = Secp256k1::new().sign_ecdsa(&hash_to_message!(&msg_hash[..]), &nodes[1].keys_manager.get_node_secret(Recipient::Node).unwrap());
let msg = msgs::ChannelUpdate { signature, contents };

let mut err_data = Vec::new();
err_data.extend_from_slice(&(msg.serialized_length() as u16 + 2).to_be_bytes());
err_data.extend_from_slice(&ChannelUpdate::TYPE.to_be_bytes());
err_data.extend_from_slice(&msg.encode());

expect_payment_failed_conditions!(nodes[0], payment_hash, false,
PaymentFailedConditions::new().blamed_scid(last_hop[0].inbound_scid_alias.unwrap())
.blamed_chan_closed(false).expected_htlc_error_data(0x1000|7, &msg.encode_with_len()));
.blamed_chan_closed(false).expected_htlc_error_data(0x1000|7, &err_data));

route.paths[0][1].fee_msat = 10_000; // Reset to the correct payment amount
route.paths[0][0].fee_msat = 0; // But set fee paid to the middle hop to 0
Expand All @@ -551,7 +557,9 @@ fn test_scid_alias_returned() {

let mut err_data = Vec::new();
err_data.extend_from_slice(&10_000u64.to_be_bytes());
err_data.extend_from_slice(&msg.encode_with_len());
err_data.extend_from_slice(&(msg.serialized_length() as u16 + 2).to_be_bytes());
err_data.extend_from_slice(&ChannelUpdate::TYPE.to_be_bytes());
err_data.extend_from_slice(&msg.encode());
expect_payment_failed_conditions!(nodes[0], payment_hash, false,
PaymentFailedConditions::new().blamed_scid(last_hop[0].inbound_scid_alias.unwrap())
.blamed_chan_closed(false).expected_htlc_error_data(0x1000|12, &err_data));
Expand Down