From 36c725fe1c86fba9a43b70a0fdb2d36767fe29bd Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 24 Dec 2019 15:52:47 -0500 Subject: [PATCH] Flatten OnionHopData struct with the Realm0 struct. Previously OnionHopData contained a OnionRealm0HopData field however instead of bumping the realm number, it has been replaced with a length, used to indicte the length of a TLV-formatted object. Because a TLV-formatted hop data can contain the same information as a realm-0 hop data, we flatten the field and simply keep track of what format it was in. --- lightning/src/ln/channelmanager.rs | 14 +++--- lightning/src/ln/functional_tests.rs | 4 +- lightning/src/ln/msgs.rs | 68 ++++++++++++---------------- lightning/src/ln/onion_utils.rs | 60 ++++++++++-------------- 4 files changed, 63 insertions(+), 83 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index daf66ffe..ca6562b9 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -943,11 +943,11 @@ impl ChannelManager where M::T return_err!("The final CLTV expiry is too soon to handle", 17, &[0;0]); } // final_incorrect_htlc_amount - if next_hop_data.data.amt_to_forward > msg.amount_msat { + if next_hop_data.amt_to_forward > msg.amount_msat { return_err!("Upstream node sent less than we were supposed to receive in payment", 19, &byte_utils::be64_to_array(msg.amount_msat)); } // final_incorrect_cltv_expiry - if next_hop_data.data.outgoing_cltv_value != msg.cltv_expiry { + if next_hop_data.outgoing_cltv_value != msg.cltv_expiry { return_err!("Upstream node set CLTV to the wrong value", 18, &byte_utils::be32_to_array(msg.cltv_expiry)); } @@ -961,8 +961,8 @@ impl ChannelManager where M::T payment_hash: msg.payment_hash.clone(), short_channel_id: 0, incoming_shared_secret: shared_secret, - amt_to_forward: next_hop_data.data.amt_to_forward, - outgoing_cltv_value: next_hop_data.data.outgoing_cltv_value, + amt_to_forward: next_hop_data.amt_to_forward, + outgoing_cltv_value: next_hop_data.outgoing_cltv_value, }) } else { let mut new_packet_data = [0; 20*65]; @@ -992,10 +992,10 @@ impl ChannelManager where M::T PendingHTLCStatus::Forward(PendingForwardHTLCInfo { onion_packet: Some(outgoing_packet), payment_hash: msg.payment_hash.clone(), - short_channel_id: next_hop_data.data.short_channel_id, + short_channel_id: next_hop_data.short_channel_id, incoming_shared_secret: shared_secret, - amt_to_forward: next_hop_data.data.amt_to_forward, - outgoing_cltv_value: next_hop_data.data.outgoing_cltv_value, + amt_to_forward: next_hop_data.amt_to_forward, + outgoing_cltv_value: next_hop_data.outgoing_cltv_value, }) }; diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 7c379d21..b9a2dde8 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -5036,7 +5036,7 @@ fn test_onion_failure() { let cur_height = nodes[0].node.latest_block_height.load(Ordering::Acquire) as u32 + 1; let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route, &session_priv).unwrap(); let (mut onion_payloads, _htlc_msat, _htlc_cltv) = onion_utils::build_onion_payloads(&route, cur_height).unwrap(); - onion_payloads[0].realm = 3; + onion_payloads[0].format = msgs::OnionHopDataFormat::BogusRealm(3); msg.onion_routing_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash); }, ||{}, true, Some(PERM|1), Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: channels[1].0.contents.short_channel_id, is_permanent: true}));//XXX incremented channels idx here @@ -5046,7 +5046,7 @@ fn test_onion_failure() { let cur_height = nodes[0].node.latest_block_height.load(Ordering::Acquire) as u32 + 1; let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route, &session_priv).unwrap(); let (mut onion_payloads, _htlc_msat, _htlc_cltv) = onion_utils::build_onion_payloads(&route, cur_height).unwrap(); - onion_payloads[1].realm = 3; + onion_payloads[1].format = msgs::OnionHopDataFormat::BogusRealm(3); msg.onion_routing_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash); }, ||{}, false, Some(PERM|1), Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: channels[1].0.contents.short_channel_id, is_permanent: true})); diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 519c5654..9d1563d6 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -608,21 +608,25 @@ pub trait RoutingMessageHandler : Send + Sync { fn should_request_full_sync(&self, node_id: &PublicKey) -> bool; } -pub(crate) struct OnionRealm0HopData { - pub(crate) short_channel_id: u64, - pub(crate) amt_to_forward: u64, - pub(crate) outgoing_cltv_value: u32, - // 12 bytes of 0-padding -} - mod fuzzy_internal_msgs { // These types aren't intended to be pub, but are exposed for direct fuzzing (as we deserialize // them from untrusted input): - use super::OnionRealm0HopData; + pub(crate) enum OnionHopDataFormat { + Legacy, // aka Realm-0 + // Some tests expect to be able to generate bogus non-deserializable OnionHopDatas. In the + // future we can use bogus TLV attributes, but for now we have to expose a "bogus realm" + // option. + #[cfg(test)] + BogusRealm(u8), + } + pub struct OnionHopData { - pub(crate) realm: u8, - pub(crate) data: OnionRealm0HopData, + pub(crate) format: OnionHopDataFormat, + pub(crate) short_channel_id: u64, + pub(crate) amt_to_forward: u64, + pub(crate) outgoing_cltv_value: u32, + // 12 bytes of 0-padding pub(crate) hmac: [u8; 32], } @@ -960,36 +964,18 @@ impl_writeable!(UpdateAddHTLC, 32+8+8+32+4+1366, { onion_routing_packet }); -impl Writeable for OnionRealm0HopData { +impl Writeable for OnionHopData { fn write(&self, w: &mut W) -> Result<(), ::std::io::Error> { - w.size_hint(32); + w.size_hint(65); + match self.format { + OnionHopDataFormat::Legacy => 0u8.write(w)?, + #[cfg(test)] + OnionHopDataFormat::BogusRealm(v) => v.write(w)?, + } self.short_channel_id.write(w)?; self.amt_to_forward.write(w)?; self.outgoing_cltv_value.write(w)?; w.write_all(&[0;12])?; - Ok(()) - } -} - -impl Readable for OnionRealm0HopData { - fn read(r: &mut R) -> Result { - Ok(OnionRealm0HopData { - short_channel_id: Readable::read(r)?, - amt_to_forward: Readable::read(r)?, - outgoing_cltv_value: { - let v: u32 = Readable::read(r)?; - r.read_exact(&mut [0; 12])?; - v - } - }) - } -} - -impl Writeable for OnionHopData { - fn write(&self, w: &mut W) -> Result<(), ::std::io::Error> { - w.size_hint(65); - self.realm.write(w)?; - self.data.write(w)?; self.hmac.write(w)?; Ok(()) } @@ -998,14 +984,20 @@ impl Writeable for OnionHopData { impl Readable for OnionHopData { fn read(r: &mut R) -> Result { Ok(OnionHopData { - realm: { + format: { let r: u8 = Readable::read(r)?; if r != 0 { return Err(DecodeError::UnknownVersion); } - r + OnionHopDataFormat::Legacy + }, + short_channel_id: Readable::read(r)?, + amt_to_forward: Readable::read(r)?, + outgoing_cltv_value: { + let v: u32 = Readable::read(r)?; + r.read_exact(&mut [0; 12])?; + v }, - data: Readable::read(r)?, hmac: Readable::read(r)?, }) } diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 98f4b644..1c1105cf 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -121,12 +121,10 @@ pub(super) fn build_onion_payloads(route: &Route, starting_htlc_offset: u32) -> let value_msat = if cur_value_msat == 0 { hop.fee_msat } else { cur_value_msat }; let cltv = if cur_cltv == starting_htlc_offset { hop.cltv_expiry_delta + starting_htlc_offset } else { cur_cltv }; res.insert(0, msgs::OnionHopData { - realm: 0, - data: msgs::OnionRealm0HopData { - short_channel_id: last_short_channel_id, - amt_to_forward: value_msat, - outgoing_cltv_value: cltv, - }, + format: msgs::OnionHopDataFormat::Legacy, + short_channel_id: last_short_channel_id, + amt_to_forward: value_msat, + outgoing_cltv_value: cltv, hmac: [0; 32], }); cur_value_msat += hop.fee_msat; @@ -516,48 +514,38 @@ mod tests { // Test vectors below are flat-out wrong: they claim to set outgoing_cltv_value to non-0 :/ let payloads = vec!( msgs::OnionHopData { - realm: 0, - data: msgs::OnionRealm0HopData { - short_channel_id: 0, - amt_to_forward: 0, - outgoing_cltv_value: 0, - }, + format: msgs::OnionHopDataFormat::Legacy, + short_channel_id: 0, + amt_to_forward: 0, + outgoing_cltv_value: 0, hmac: [0; 32], }, msgs::OnionHopData { - realm: 0, - data: msgs::OnionRealm0HopData { - short_channel_id: 0x0101010101010101, - amt_to_forward: 0x0100000001, - outgoing_cltv_value: 0, - }, + format: msgs::OnionHopDataFormat::Legacy, + short_channel_id: 0x0101010101010101, + amt_to_forward: 0x0100000001, + outgoing_cltv_value: 0, hmac: [0; 32], }, msgs::OnionHopData { - realm: 0, - data: msgs::OnionRealm0HopData { - short_channel_id: 0x0202020202020202, - amt_to_forward: 0x0200000002, - outgoing_cltv_value: 0, - }, + format: msgs::OnionHopDataFormat::Legacy, + short_channel_id: 0x0202020202020202, + amt_to_forward: 0x0200000002, + outgoing_cltv_value: 0, hmac: [0; 32], }, msgs::OnionHopData { - realm: 0, - data: msgs::OnionRealm0HopData { - short_channel_id: 0x0303030303030303, - amt_to_forward: 0x0300000003, - outgoing_cltv_value: 0, - }, + format: msgs::OnionHopDataFormat::Legacy, + short_channel_id: 0x0303030303030303, + amt_to_forward: 0x0300000003, + outgoing_cltv_value: 0, hmac: [0; 32], }, msgs::OnionHopData { - realm: 0, - data: msgs::OnionRealm0HopData { - short_channel_id: 0x0404040404040404, - amt_to_forward: 0x0400000004, - outgoing_cltv_value: 0, - }, + format: msgs::OnionHopDataFormat::Legacy, + short_channel_id: 0x0404040404040404, + amt_to_forward: 0x0400000004, + outgoing_cltv_value: 0, hmac: [0; 32], }, ); -- 2.30.2