From 9c5acf19f39120f79f9fa2a03e9310a3af55d8c9 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Thu, 23 Mar 2023 18:34:47 -0400 Subject: [PATCH 1/6] Move pending forward htlc construction into method In upcoming blinded paths work, this method will grow to handle blinded forwards. --- lightning/src/ln/channelmanager.rs | 94 +++++++++++++++++------------- 1 file changed, 55 insertions(+), 39 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 07bb18ff3..847f0dcfc 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -341,7 +341,7 @@ impl HTLCSource { } } -struct ReceiveError { +struct InboundOnionErr { err_code: u16, err_data: Vec, msg: &'static str, @@ -2616,14 +2616,52 @@ where } } + fn construct_fwd_pending_htlc_info( + &self, msg: &msgs::UpdateAddHTLC, hop_data: msgs::OnionHopData, hop_hmac: [u8; 32], + new_packet_bytes: [u8; onion_utils::ONION_DATA_LEN], shared_secret: [u8; 32], + next_packet_pubkey_opt: Option> + ) -> Result { + debug_assert!(next_packet_pubkey_opt.is_some()); + let outgoing_packet = msgs::OnionPacket { + version: 0, + public_key: next_packet_pubkey_opt.unwrap_or(Err(secp256k1::Error::InvalidPublicKey)), + hop_data: new_packet_bytes, + hmac: hop_hmac.clone(), + }; + + let short_channel_id = match hop_data.format { + msgs::OnionHopDataFormat::NonFinalNode { short_channel_id } => short_channel_id, + msgs::OnionHopDataFormat::FinalNode { .. } => { + return Err(InboundOnionErr { + msg: "Final Node OnionHopData provided for us as an intermediary node", + err_code: 0x4000 | 22, + err_data: Vec::new(), + }) + }, + }; + + Ok(PendingHTLCInfo { + routing: PendingHTLCRouting::Forward { + onion_packet: outgoing_packet, + short_channel_id, + }, + payment_hash: msg.payment_hash.clone(), + incoming_shared_secret: shared_secret, + incoming_amt_msat: Some(msg.amount_msat), + outgoing_amt_msat: hop_data.amt_to_forward, + outgoing_cltv_value: hop_data.outgoing_cltv_value, + skimmed_fee_msat: None, + }) + } + fn construct_recv_pending_htlc_info( &self, hop_data: msgs::OnionHopData, shared_secret: [u8; 32], payment_hash: PaymentHash, amt_msat: u64, cltv_expiry: u32, phantom_shared_secret: Option<[u8; 32]>, allow_underpay: bool, counterparty_skimmed_fee_msat: Option, - ) -> Result { + ) -> Result { // final_incorrect_cltv_expiry if hop_data.outgoing_cltv_value > cltv_expiry { - return Err(ReceiveError { + return Err(InboundOnionErr { msg: "Upstream node set CLTV to less than the CLTV set by the sender", err_code: 18, err_data: cltv_expiry.to_be_bytes().to_vec() @@ -2641,7 +2679,7 @@ where let mut err_data = Vec::with_capacity(12); err_data.extend_from_slice(&amt_msat.to_be_bytes()); err_data.extend_from_slice(¤t_height.to_be_bytes()); - return Err(ReceiveError { + return Err(InboundOnionErr { err_code: 0x4000 | 15, err_data, msg: "The final CLTV expiry is too soon to handle", }); @@ -2650,7 +2688,7 @@ where (allow_underpay && hop_data.amt_to_forward > amt_msat.saturating_add(counterparty_skimmed_fee_msat.unwrap_or(0))) { - return Err(ReceiveError { + return Err(InboundOnionErr { err_code: 19, err_data: amt_msat.to_be_bytes().to_vec(), msg: "Upstream node sent less than we were supposed to receive in payment", @@ -2659,7 +2697,7 @@ where let routing = match hop_data.format { msgs::OnionHopDataFormat::NonFinalNode { .. } => { - return Err(ReceiveError { + return Err(InboundOnionErr { err_code: 0x4000|22, err_data: Vec::new(), msg: "Got non final data with an HMAC of 0", @@ -2674,14 +2712,14 @@ where // time discrepancies due to a hash collision with X. let hashed_preimage = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner()); if hashed_preimage != payment_hash { - return Err(ReceiveError { + return Err(InboundOnionErr { err_code: 0x4000|22, err_data: Vec::new(), msg: "Payment preimage didn't match payment hash", }); } if !self.default_configuration.accept_mpp_keysend && payment_data.is_some() { - return Err(ReceiveError { + return Err(InboundOnionErr { err_code: 0x4000|22, err_data: Vec::new(), msg: "We don't support MPP keysend payments", @@ -2701,7 +2739,7 @@ where phantom_shared_secret, } } else { - return Err(ReceiveError { + return Err(InboundOnionErr { err_code: 0x4000|0x2000|3, err_data: Vec::new(), msg: "We require payment_secrets", @@ -2964,37 +3002,15 @@ where // delay) once they've send us a commitment_signed! PendingHTLCStatus::Forward(info) }, - Err(ReceiveError { err_code, err_data, msg }) => return_err!(msg, err_code, &err_data) + Err(InboundOnionErr { err_code, err_data, msg }) => return_err!(msg, err_code, &err_data) } }, onion_utils::Hop::Forward { next_hop_data, next_hop_hmac, new_packet_bytes } => { - debug_assert!(next_packet_pubkey_opt.is_some()); - let outgoing_packet = msgs::OnionPacket { - version: 0, - public_key: next_packet_pubkey_opt.unwrap_or(Err(secp256k1::Error::InvalidPublicKey)), - hop_data: new_packet_bytes, - hmac: next_hop_hmac.clone(), - }; - - let short_channel_id = match next_hop_data.format { - msgs::OnionHopDataFormat::NonFinalNode { short_channel_id } => short_channel_id, - msgs::OnionHopDataFormat::FinalNode { .. } => { - return_err!("Final Node OnionHopData provided for us as an intermediary node", 0x4000 | 22, &[0;0]); - }, - }; - - PendingHTLCStatus::Forward(PendingHTLCInfo { - routing: PendingHTLCRouting::Forward { - onion_packet: outgoing_packet, - short_channel_id, - }, - payment_hash: msg.payment_hash.clone(), - incoming_shared_secret: shared_secret, - incoming_amt_msat: Some(msg.amount_msat), - outgoing_amt_msat: next_hop_data.amt_to_forward, - outgoing_cltv_value: next_hop_data.outgoing_cltv_value, - skimmed_fee_msat: None, - }) + match self.construct_fwd_pending_htlc_info(msg, next_hop_data, next_hop_hmac, + new_packet_bytes, shared_secret, next_packet_pubkey_opt) { + Ok(info) => PendingHTLCStatus::Forward(info), + Err(InboundOnionErr { err_code, err_data, msg }) => return_err!(msg, err_code, &err_data) + } } } } @@ -3777,7 +3793,7 @@ where outgoing_cltv_value, Some(phantom_shared_secret), false, None) { Ok(info) => phantom_receives.push((prev_short_channel_id, prev_funding_outpoint, prev_user_channel_id, vec![(info, prev_htlc_id)])), - Err(ReceiveError { err_code, err_data, msg }) => failed_payment!(msg, err_code, err_data, Some(phantom_shared_secret)) + Err(InboundOnionErr { err_code, err_data, msg }) => failed_payment!(msg, err_code, err_data, Some(phantom_shared_secret)) } }, _ => panic!(), @@ -10018,7 +10034,7 @@ mod tests { }; // Check that if the amount we received + the penultimate hop extra fee is less than the sender // intended amount, we fail the payment. - if let Err(crate::ln::channelmanager::ReceiveError { err_code, .. }) = + if let Err(crate::ln::channelmanager::InboundOnionErr { err_code, .. }) = node[0].node.construct_recv_pending_htlc_info(hop_data, [0; 32], PaymentHash([0; 32]), sender_intended_amt_msat - extra_fee_msat - 1, 42, None, true, Some(extra_fee_msat)) { From 9473f1cb5fd9d84069d28c8bac340cd0ea3dda60 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Wed, 3 May 2023 19:15:30 -0400 Subject: [PATCH 2/6] Remove indentation in payment receive util This also set us up for supporting receiving blinded onion payloads. --- lightning/src/ln/channelmanager.rs | 93 +++++++++++++++--------------- 1 file changed, 46 insertions(+), 47 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 847f0dcfc..2abd3c3b9 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2659,6 +2659,16 @@ where amt_msat: u64, cltv_expiry: u32, phantom_shared_secret: Option<[u8; 32]>, allow_underpay: bool, counterparty_skimmed_fee_msat: Option, ) -> Result { + let (payment_data, keysend_preimage, payment_metadata) = match hop_data.format { + msgs::OnionHopDataFormat::FinalNode { payment_data, keysend_preimage, payment_metadata } => + (payment_data, keysend_preimage, payment_metadata), + _ => + return Err(InboundOnionErr { + err_code: 0x4000|22, + err_data: Vec::new(), + msg: "Got non final data with an HMAC of 0", + }), + }; // final_incorrect_cltv_expiry if hop_data.outgoing_cltv_value > cltv_expiry { return Err(InboundOnionErr { @@ -2695,57 +2705,46 @@ where }); } - let routing = match hop_data.format { - msgs::OnionHopDataFormat::NonFinalNode { .. } => { + let routing = if let Some(payment_preimage) = keysend_preimage { + // We need to check that the sender knows the keysend preimage before processing this + // payment further. Otherwise, an intermediary routing hop forwarding non-keysend-HTLC X + // could discover the final destination of X, by probing the adjacent nodes on the route + // with a keysend payment of identical payment hash to X and observing the processing + // time discrepancies due to a hash collision with X. + let hashed_preimage = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner()); + if hashed_preimage != payment_hash { return Err(InboundOnionErr { err_code: 0x4000|22, err_data: Vec::new(), - msg: "Got non final data with an HMAC of 0", + msg: "Payment preimage didn't match payment hash", }); - }, - msgs::OnionHopDataFormat::FinalNode { payment_data, keysend_preimage, payment_metadata } => { - if let Some(payment_preimage) = keysend_preimage { - // We need to check that the sender knows the keysend preimage before processing this - // payment further. Otherwise, an intermediary routing hop forwarding non-keysend-HTLC X - // could discover the final destination of X, by probing the adjacent nodes on the route - // with a keysend payment of identical payment hash to X and observing the processing - // time discrepancies due to a hash collision with X. - let hashed_preimage = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner()); - if hashed_preimage != payment_hash { - return Err(InboundOnionErr { - err_code: 0x4000|22, - err_data: Vec::new(), - msg: "Payment preimage didn't match payment hash", - }); - } - if !self.default_configuration.accept_mpp_keysend && payment_data.is_some() { - return Err(InboundOnionErr { - err_code: 0x4000|22, - err_data: Vec::new(), - msg: "We don't support MPP keysend payments", - }); - } - PendingHTLCRouting::ReceiveKeysend { - payment_data, - payment_preimage, - payment_metadata, - incoming_cltv_expiry: hop_data.outgoing_cltv_value, - } - } else if let Some(data) = payment_data { - PendingHTLCRouting::Receive { - payment_data: data, - payment_metadata, - incoming_cltv_expiry: hop_data.outgoing_cltv_value, - phantom_shared_secret, - } - } else { - return Err(InboundOnionErr { - err_code: 0x4000|0x2000|3, - err_data: Vec::new(), - msg: "We require payment_secrets", - }); - } - }, + } + if !self.default_configuration.accept_mpp_keysend && payment_data.is_some() { + return Err(InboundOnionErr { + err_code: 0x4000|22, + err_data: Vec::new(), + msg: "We don't support MPP keysend payments", + }); + } + PendingHTLCRouting::ReceiveKeysend { + payment_data, + payment_preimage, + payment_metadata, + incoming_cltv_expiry: hop_data.outgoing_cltv_value, + } + } else if let Some(data) = payment_data { + PendingHTLCRouting::Receive { + payment_data: data, + payment_metadata, + incoming_cltv_expiry: hop_data.outgoing_cltv_value, + phantom_shared_secret, + } + } else { + return Err(InboundOnionErr { + err_code: 0x4000|0x2000|3, + err_data: Vec::new(), + msg: "We require payment_secrets", + }); }; Ok(PendingHTLCInfo { routing, From 0c37488ff4c620edcc479e3dcad17a53d52ca114 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Tue, 1 Aug 2023 14:07:51 -0700 Subject: [PATCH 3/6] Remove outdated documentation of a panic --- lightning/src/ln/msgs.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index a514f4923..cfee16033 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -1440,7 +1440,6 @@ mod fuzzy_internal_msgs { pub struct OnionHopData { pub(crate) format: OnionHopDataFormat, /// The value, in msat, of the payment after this hop's fee is deducted. - /// Message serialization may panic if this value is more than 21 million Bitcoin. pub(crate) amt_to_forward: u64, pub(crate) outgoing_cltv_value: u32, } From 02a6d895a576d24c612d83c834f30ea2fd7ab67d Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Fri, 24 Mar 2023 12:46:16 -0400 Subject: [PATCH 4/6] Receive payment onions as new InboundPayload instead of OnionHopData To support route blinding, we want to split OnionHopData into two separate structs, one for inbound onions and one for outbound onions. This is because blinded payloads change the fields present in the onion hop data struct based on whether we're sending vs receiving (outbound onions include encrypted blobs, inbound onions can decrypt those blobs and contain the decrypted fields themselves). In upcoming commits, we'll add variants for blinded payloads to the new InboundPayload enum. --- fuzz/src/bin/gen_target.sh | 2 +- ...ata_target.rs => onion_hop_data_target.rs} | 16 ++-- fuzz/src/lib.rs | 1 + fuzz/src/msg_targets/gen_target.sh | 1 - fuzz/src/msg_targets/mod.rs | 1 - ...sg_onion_hop_data.rs => onion_hop_data.rs} | 13 ++- fuzz/targets.h | 2 +- lightning/src/ln/channelmanager.rs | 87 +++++++++--------- lightning/src/ln/msgs.rs | 91 +++++++++++-------- lightning/src/ln/onion_utils.rs | 4 +- 10 files changed, 116 insertions(+), 102 deletions(-) rename fuzz/src/bin/{msg_onion_hop_data_target.rs => onion_hop_data_target.rs} (86%) rename fuzz/src/{msg_targets/msg_onion_hop_data.rs => onion_hop_data.rs} (59%) diff --git a/fuzz/src/bin/gen_target.sh b/fuzz/src/bin/gen_target.sh index 34cae5107..fe17e4bab 100755 --- a/fuzz/src/bin/gen_target.sh +++ b/fuzz/src/bin/gen_target.sh @@ -20,6 +20,7 @@ GEN_TEST refund_deser GEN_TEST router GEN_TEST zbase32 GEN_TEST indexedmap +GEN_TEST onion_hop_data GEN_TEST msg_accept_channel msg_targets:: GEN_TEST msg_announcement_signatures msg_targets:: @@ -51,7 +52,6 @@ GEN_TEST msg_update_add_htlc msg_targets:: GEN_TEST msg_error_message msg_targets:: GEN_TEST msg_channel_update msg_targets:: -GEN_TEST msg_onion_hop_data msg_targets:: GEN_TEST msg_ping msg_targets:: GEN_TEST msg_pong msg_targets:: diff --git a/fuzz/src/bin/msg_onion_hop_data_target.rs b/fuzz/src/bin/onion_hop_data_target.rs similarity index 86% rename from fuzz/src/bin/msg_onion_hop_data_target.rs rename to fuzz/src/bin/onion_hop_data_target.rs index ae21e9bd9..b8a357229 100644 --- a/fuzz/src/bin/msg_onion_hop_data_target.rs +++ b/fuzz/src/bin/onion_hop_data_target.rs @@ -16,14 +16,14 @@ compile_error!("Fuzz targets need cfg=fuzzing"); extern crate lightning_fuzz; -use lightning_fuzz::msg_targets::msg_onion_hop_data::*; +use lightning_fuzz::onion_hop_data::*; #[cfg(feature = "afl")] #[macro_use] extern crate afl; #[cfg(feature = "afl")] fn main() { fuzz!(|data| { - msg_onion_hop_data_run(data.as_ptr(), data.len()); + onion_hop_data_run(data.as_ptr(), data.len()); }); } @@ -33,7 +33,7 @@ fn main() { fn main() { loop { fuzz!(|data| { - msg_onion_hop_data_run(data.as_ptr(), data.len()); + onion_hop_data_run(data.as_ptr(), data.len()); }); } } @@ -42,7 +42,7 @@ fn main() { #[macro_use] extern crate libfuzzer_sys; #[cfg(feature = "libfuzzer_fuzz")] fuzz_target!(|data: &[u8]| { - msg_onion_hop_data_run(data.as_ptr(), data.len()); + onion_hop_data_run(data.as_ptr(), data.len()); }); #[cfg(feature = "stdin_fuzz")] @@ -51,7 +51,7 @@ fn main() { let mut data = Vec::with_capacity(8192); std::io::stdin().read_to_end(&mut data).unwrap(); - msg_onion_hop_data_run(data.as_ptr(), data.len()); + onion_hop_data_run(data.as_ptr(), data.len()); } #[test] @@ -63,11 +63,11 @@ fn run_test_cases() { use std::sync::{atomic, Arc}; { let data: Vec = vec![0]; - msg_onion_hop_data_run(data.as_ptr(), data.len()); + onion_hop_data_run(data.as_ptr(), data.len()); } let mut threads = Vec::new(); let threads_running = Arc::new(atomic::AtomicUsize::new(0)); - if let Ok(tests) = fs::read_dir("test_cases/msg_onion_hop_data") { + if let Ok(tests) = fs::read_dir("test_cases/onion_hop_data") { for test in tests { let mut data: Vec = Vec::new(); let path = test.unwrap().path(); @@ -82,7 +82,7 @@ fn run_test_cases() { let panic_logger = string_logger.clone(); let res = if ::std::panic::catch_unwind(move || { - msg_onion_hop_data_test(&data, panic_logger); + onion_hop_data_test(&data, panic_logger); }).is_err() { Some(string_logger.into_string()) } else { None }; diff --git a/fuzz/src/lib.rs b/fuzz/src/lib.rs index 92142e564..6cdeb8ab5 100644 --- a/fuzz/src/lib.rs +++ b/fuzz/src/lib.rs @@ -28,5 +28,6 @@ pub mod process_network_graph; pub mod refund_deser; pub mod router; pub mod zbase32; +pub mod onion_hop_data; pub mod msg_targets; diff --git a/fuzz/src/msg_targets/gen_target.sh b/fuzz/src/msg_targets/gen_target.sh index 3937c5001..d89df19d9 100755 --- a/fuzz/src/msg_targets/gen_target.sh +++ b/fuzz/src/msg_targets/gen_target.sh @@ -20,7 +20,6 @@ GEN_TEST lightning::ln::msgs::ChannelReady test_msg_simple "" GEN_TEST lightning::ln::msgs::FundingSigned test_msg_simple "" GEN_TEST lightning::ln::msgs::GossipTimestampFilter test_msg_simple "" GEN_TEST lightning::ln::msgs::Init test_msg_simple "" -GEN_TEST lightning::ln::msgs::OnionHopData test_msg_simple "" GEN_TEST lightning::ln::msgs::OpenChannel test_msg_simple "" GEN_TEST lightning::ln::msgs::Ping test_msg_simple "" GEN_TEST lightning::ln::msgs::Pong test_msg_simple "" diff --git a/fuzz/src/msg_targets/mod.rs b/fuzz/src/msg_targets/mod.rs index fe3bd14a7..302dda440 100644 --- a/fuzz/src/msg_targets/mod.rs +++ b/fuzz/src/msg_targets/mod.rs @@ -8,7 +8,6 @@ pub mod msg_channel_ready; pub mod msg_funding_signed; pub mod msg_gossip_timestamp_filter; pub mod msg_init; -pub mod msg_onion_hop_data; pub mod msg_open_channel; pub mod msg_ping; pub mod msg_pong; diff --git a/fuzz/src/msg_targets/msg_onion_hop_data.rs b/fuzz/src/onion_hop_data.rs similarity index 59% rename from fuzz/src/msg_targets/msg_onion_hop_data.rs rename to fuzz/src/onion_hop_data.rs index 59b3674f9..54b283ab0 100644 --- a/fuzz/src/msg_targets/msg_onion_hop_data.rs +++ b/fuzz/src/onion_hop_data.rs @@ -10,16 +10,19 @@ // This file is auto-generated by gen_target.sh based on msg_target_template.txt // To modify it, modify msg_target_template.txt and run gen_target.sh instead. -use crate::msg_targets::utils::VecWriter; use crate::utils::test_logger; #[inline] -pub fn msg_onion_hop_data_test(data: &[u8], _out: Out) { - test_msg_simple!(lightning::ln::msgs::OnionHopData, data); +pub fn onion_hop_data_test(data: &[u8], _out: Out) { + use lightning::util::ser::Readable; + let mut r = ::std::io::Cursor::new(data); + let _ = ::read(&mut r); } #[no_mangle] -pub extern "C" fn msg_onion_hop_data_run(data: *const u8, datalen: usize) { +pub extern "C" fn onion_hop_data_run(data: *const u8, datalen: usize) { + use lightning::util::ser::Readable; let data = unsafe { std::slice::from_raw_parts(data, datalen) }; - test_msg_simple!(lightning::ln::msgs::OnionHopData, data); + let mut r = ::std::io::Cursor::new(data); + let _ = ::read(&mut r); } diff --git a/fuzz/targets.h b/fuzz/targets.h index eb8d66f41..9b5a6d455 100644 --- a/fuzz/targets.h +++ b/fuzz/targets.h @@ -13,6 +13,7 @@ void refund_deser_run(const unsigned char* data, size_t data_len); void router_run(const unsigned char* data, size_t data_len); void zbase32_run(const unsigned char* data, size_t data_len); void indexedmap_run(const unsigned char* data, size_t data_len); +void onion_hop_data_run(const unsigned char* data, size_t data_len); void msg_accept_channel_run(const unsigned char* data, size_t data_len); void msg_announcement_signatures_run(const unsigned char* data, size_t data_len); void msg_channel_reestablish_run(const unsigned char* data, size_t data_len); @@ -40,7 +41,6 @@ void msg_gossip_timestamp_filter_run(const unsigned char* data, size_t data_len) void msg_update_add_htlc_run(const unsigned char* data, size_t data_len); void msg_error_message_run(const unsigned char* data, size_t data_len); void msg_channel_update_run(const unsigned char* data, size_t data_len); -void msg_onion_hop_data_run(const unsigned char* data, size_t data_len); void msg_ping_run(const unsigned char* data, size_t data_len); void msg_pong_run(const unsigned char* data, size_t data_len); void msg_channel_details_run(const unsigned char* data, size_t data_len); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 2abd3c3b9..6bfa4f0b0 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2617,7 +2617,7 @@ where } fn construct_fwd_pending_htlc_info( - &self, msg: &msgs::UpdateAddHTLC, hop_data: msgs::OnionHopData, hop_hmac: [u8; 32], + &self, msg: &msgs::UpdateAddHTLC, hop_data: msgs::InboundOnionPayload, hop_hmac: [u8; 32], new_packet_bytes: [u8; onion_utils::ONION_DATA_LEN], shared_secret: [u8; 32], next_packet_pubkey_opt: Option> ) -> Result { @@ -2626,18 +2626,18 @@ where version: 0, public_key: next_packet_pubkey_opt.unwrap_or(Err(secp256k1::Error::InvalidPublicKey)), hop_data: new_packet_bytes, - hmac: hop_hmac.clone(), + hmac: hop_hmac, }; - let short_channel_id = match hop_data.format { - msgs::OnionHopDataFormat::NonFinalNode { short_channel_id } => short_channel_id, - msgs::OnionHopDataFormat::FinalNode { .. } => { + let (short_channel_id, amt_to_forward, outgoing_cltv_value) = match hop_data { + msgs::InboundOnionPayload::Forward { short_channel_id, amt_to_forward, outgoing_cltv_value } => + (short_channel_id, amt_to_forward, outgoing_cltv_value), + msgs::InboundOnionPayload::Receive { .. } => return Err(InboundOnionErr { msg: "Final Node OnionHopData provided for us as an intermediary node", err_code: 0x4000 | 22, err_data: Vec::new(), - }) - }, + }), }; Ok(PendingHTLCInfo { @@ -2645,23 +2645,25 @@ where onion_packet: outgoing_packet, short_channel_id, }, - payment_hash: msg.payment_hash.clone(), + payment_hash: msg.payment_hash, incoming_shared_secret: shared_secret, incoming_amt_msat: Some(msg.amount_msat), - outgoing_amt_msat: hop_data.amt_to_forward, - outgoing_cltv_value: hop_data.outgoing_cltv_value, + outgoing_amt_msat: amt_to_forward, + outgoing_cltv_value, skimmed_fee_msat: None, }) } fn construct_recv_pending_htlc_info( - &self, hop_data: msgs::OnionHopData, shared_secret: [u8; 32], payment_hash: PaymentHash, + &self, hop_data: msgs::InboundOnionPayload, shared_secret: [u8; 32], payment_hash: PaymentHash, amt_msat: u64, cltv_expiry: u32, phantom_shared_secret: Option<[u8; 32]>, allow_underpay: bool, counterparty_skimmed_fee_msat: Option, ) -> Result { - let (payment_data, keysend_preimage, payment_metadata) = match hop_data.format { - msgs::OnionHopDataFormat::FinalNode { payment_data, keysend_preimage, payment_metadata } => - (payment_data, keysend_preimage, payment_metadata), + let (payment_data, keysend_preimage, onion_amt_msat, outgoing_cltv_value, payment_metadata) = match hop_data { + msgs::InboundOnionPayload::Receive { + payment_data, keysend_preimage, amt_msat, outgoing_cltv_value, payment_metadata, .. + } => + (payment_data, keysend_preimage, amt_msat, outgoing_cltv_value, payment_metadata), _ => return Err(InboundOnionErr { err_code: 0x4000|22, @@ -2670,7 +2672,7 @@ where }), }; // final_incorrect_cltv_expiry - if hop_data.outgoing_cltv_value > cltv_expiry { + if outgoing_cltv_value > cltv_expiry { return Err(InboundOnionErr { msg: "Upstream node set CLTV to less than the CLTV set by the sender", err_code: 18, @@ -2685,7 +2687,7 @@ where // payment logic has enough time to fail the HTLC backward before our onchain logic triggers a // channel closure (see HTLC_FAIL_BACK_BUFFER rationale). let current_height: u32 = self.best_block.read().unwrap().height(); - if (hop_data.outgoing_cltv_value as u64) <= current_height as u64 + HTLC_FAIL_BACK_BUFFER as u64 + 1 { + if (outgoing_cltv_value as u64) <= current_height as u64 + HTLC_FAIL_BACK_BUFFER as u64 + 1 { let mut err_data = Vec::with_capacity(12); err_data.extend_from_slice(&amt_msat.to_be_bytes()); err_data.extend_from_slice(¤t_height.to_be_bytes()); @@ -2694,8 +2696,8 @@ where msg: "The final CLTV expiry is too soon to handle", }); } - if (!allow_underpay && hop_data.amt_to_forward > amt_msat) || - (allow_underpay && hop_data.amt_to_forward > + if (!allow_underpay && onion_amt_msat > amt_msat) || + (allow_underpay && onion_amt_msat > amt_msat.saturating_add(counterparty_skimmed_fee_msat.unwrap_or(0))) { return Err(InboundOnionErr { @@ -2730,13 +2732,13 @@ where payment_data, payment_preimage, payment_metadata, - incoming_cltv_expiry: hop_data.outgoing_cltv_value, + incoming_cltv_expiry: outgoing_cltv_value, } } else if let Some(data) = payment_data { PendingHTLCRouting::Receive { payment_data: data, payment_metadata, - incoming_cltv_expiry: hop_data.outgoing_cltv_value, + incoming_cltv_expiry: outgoing_cltv_value, phantom_shared_secret, } } else { @@ -2751,8 +2753,8 @@ where payment_hash, incoming_shared_secret: shared_secret, incoming_amt_msat: Some(amt_msat), - outgoing_amt_msat: hop_data.amt_to_forward, - outgoing_cltv_value: hop_data.outgoing_cltv_value, + outgoing_amt_msat: onion_amt_msat, + outgoing_cltv_value, skimmed_fee_msat: counterparty_skimmed_fee_msat, }) } @@ -2816,9 +2818,8 @@ where }; let (outgoing_scid, outgoing_amt_msat, outgoing_cltv_value, next_packet_pk_opt) = match next_hop { onion_utils::Hop::Forward { - next_hop_data: msgs::OnionHopData { - format: msgs::OnionHopDataFormat::NonFinalNode { short_channel_id }, amt_to_forward, - outgoing_cltv_value, + next_hop_data: msgs::InboundOnionPayload::Forward { + short_channel_id, amt_to_forward, outgoing_cltv_value }, .. } => { let next_pk = onion_utils::next_hop_packet_pubkey(&self.secp_ctx, @@ -2828,9 +2829,7 @@ where // We'll do receive checks in [`Self::construct_pending_htlc_info`] so we have access to the // inbound channel's state. onion_utils::Hop::Receive { .. } => return Ok((next_hop, shared_secret, None)), - onion_utils::Hop::Forward { - next_hop_data: msgs::OnionHopData { format: msgs::OnionHopDataFormat::FinalNode { .. }, .. }, .. - } => { + onion_utils::Hop::Forward { next_hop_data: msgs::InboundOnionPayload::Receive { .. }, .. } => { return_err!("Final Node OnionHopData provided for us as an intermediary node", 0x4000 | 22, &[0; 0]); } }; @@ -10020,16 +10019,14 @@ mod tests { let node = create_network(1, &node_cfg, &node_chanmgr); let sender_intended_amt_msat = 100; let extra_fee_msat = 10; - let hop_data = msgs::OnionHopData { - amt_to_forward: 100, + let hop_data = msgs::InboundOnionPayload::Receive { + amt_msat: 100, outgoing_cltv_value: 42, - format: msgs::OnionHopDataFormat::FinalNode { - keysend_preimage: None, - payment_metadata: None, - payment_data: Some(msgs::FinalOnionHopData { - payment_secret: PaymentSecret([0; 32]), total_msat: sender_intended_amt_msat, - }), - } + payment_metadata: None, + keysend_preimage: None, + payment_data: Some(msgs::FinalOnionHopData { + payment_secret: PaymentSecret([0; 32]), total_msat: sender_intended_amt_msat, + }), }; // Check that if the amount we received + the penultimate hop extra fee is less than the sender // intended amount, we fail the payment. @@ -10041,16 +10038,14 @@ mod tests { } else { panic!(); } // If amt_received + extra_fee is equal to the sender intended amount, we're fine. - let hop_data = msgs::OnionHopData { // This is the same hop_data as above, OnionHopData doesn't implement Clone - amt_to_forward: 100, + let hop_data = msgs::InboundOnionPayload::Receive { // This is the same payload as above, InboundOnionPayload doesn't implement Clone + amt_msat: 100, outgoing_cltv_value: 42, - format: msgs::OnionHopDataFormat::FinalNode { - keysend_preimage: None, - payment_metadata: None, - payment_data: Some(msgs::FinalOnionHopData { - payment_secret: PaymentSecret([0; 32]), total_msat: sender_intended_amt_msat, - }), - } + payment_metadata: None, + keysend_preimage: None, + payment_data: Some(msgs::FinalOnionHopData { + payment_secret: PaymentSecret([0; 32]), total_msat: sender_intended_amt_msat, + }), }; assert!(node[0].node.construct_recv_pending_htlc_info(hop_data, [0; 32], PaymentHash([0; 32]), sender_intended_amt_msat - extra_fee_msat, 42, None, true, Some(extra_fee_msat)).is_ok()); diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index cfee16033..590b26632 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -1419,11 +1419,27 @@ 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): #[derive(Clone)] - pub(crate) struct FinalOnionHopData { - pub(crate) payment_secret: PaymentSecret, + pub struct FinalOnionHopData { + pub payment_secret: PaymentSecret, /// The total value, in msat, of the payment as received by the ultimate recipient. /// Message serialization may panic if this value is more than 21 million Bitcoin. - pub(crate) total_msat: u64, + pub total_msat: u64, + } + + pub enum InboundOnionPayload { + Forward { + short_channel_id: u64, + /// The value, in msat, of the payment after this hop's fee is deducted. + amt_to_forward: u64, + outgoing_cltv_value: u32, + }, + Receive { + payment_data: Option, + payment_metadata: Option>, + keysend_preimage: Option, + amt_msat: u64, + outgoing_cltv_value: u32, + }, } pub(crate) enum OnionHopDataFormat { @@ -1974,7 +1990,7 @@ impl Writeable for OnionHopData { } } -impl Readable for OnionHopData { +impl Readable for InboundOnionPayload { fn read(r: &mut R) -> Result { let mut amt = HighZeroBytesDroppedBigSize(0u64); let mut cltv_value = HighZeroBytesDroppedBigSize(0u32); @@ -1992,39 +2008,35 @@ impl Readable for OnionHopData { (5482373484, keysend_preimage, option) }); - let format = if let Some(short_channel_id) = short_id { - if payment_data.is_some() { return Err(DecodeError::InvalidValue); } + if amt.0 > MAX_VALUE_MSAT { return Err(DecodeError::InvalidValue) } + if let Some(short_channel_id) = short_id { + if payment_data.is_some() { return Err(DecodeError::InvalidValue) } if payment_metadata.is_some() { return Err(DecodeError::InvalidValue); } - OnionHopDataFormat::NonFinalNode { + Ok(Self::Forward { short_channel_id, - } + amt_to_forward: amt.0, + outgoing_cltv_value: cltv_value.0, + }) } else { if let Some(data) = &payment_data { if data.total_msat > MAX_VALUE_MSAT { return Err(DecodeError::InvalidValue); } } - OnionHopDataFormat::FinalNode { + Ok(Self::Receive { payment_data, payment_metadata: payment_metadata.map(|w| w.0), keysend_preimage, - } - }; - - if amt.0 > MAX_VALUE_MSAT { - return Err(DecodeError::InvalidValue); + amt_msat: amt.0, + outgoing_cltv_value: cltv_value.0, + }) } - Ok(OnionHopData { - format, - amt_to_forward: amt.0, - outgoing_cltv_value: cltv_value.0, - }) } } // ReadableArgs because we need onion_utils::decode_next_hop to accommodate payment packets and // onion message packets. -impl ReadableArgs<()> for OnionHopData { +impl ReadableArgs<()> for InboundOnionPayload { fn read(r: &mut R, _arg: ()) -> Result { ::read(r) } @@ -3525,7 +3537,7 @@ mod tests { #[test] fn encoding_nonfinal_onion_hop_data() { - let mut msg = msgs::OnionHopData { + let msg = msgs::OnionHopData { format: OnionHopDataFormat::NonFinalNode { short_channel_id: 0xdeadbeef1bad1dea, }, @@ -3535,17 +3547,18 @@ mod tests { let encoded_value = msg.encode(); let target_value = hex::decode("1a02080badf00d010203040404ffffffff0608deadbeef1bad1dea").unwrap(); assert_eq!(encoded_value, target_value); - msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap(); - if let OnionHopDataFormat::NonFinalNode { short_channel_id } = msg.format { + + let inbound_msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap(); + if let msgs::InboundOnionPayload::Forward { short_channel_id, amt_to_forward, outgoing_cltv_value } = inbound_msg { assert_eq!(short_channel_id, 0xdeadbeef1bad1dea); + assert_eq!(amt_to_forward, 0x0badf00d01020304); + assert_eq!(outgoing_cltv_value, 0xffffffff); } else { panic!(); } - assert_eq!(msg.amt_to_forward, 0x0badf00d01020304); - assert_eq!(msg.outgoing_cltv_value, 0xffffffff); } #[test] fn encoding_final_onion_hop_data() { - let mut msg = msgs::OnionHopData { + let msg = msgs::OnionHopData { format: OnionHopDataFormat::FinalNode { payment_data: None, payment_metadata: None, @@ -3557,16 +3570,18 @@ mod tests { let encoded_value = msg.encode(); let target_value = hex::decode("1002080badf00d010203040404ffffffff").unwrap(); assert_eq!(encoded_value, target_value); - msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap(); - if let OnionHopDataFormat::FinalNode { payment_data: None, .. } = msg.format { } else { panic!(); } - assert_eq!(msg.amt_to_forward, 0x0badf00d01020304); - assert_eq!(msg.outgoing_cltv_value, 0xffffffff); + + let inbound_msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap(); + if let msgs::InboundOnionPayload::Receive { payment_data: None, amt_msat, outgoing_cltv_value, .. } = inbound_msg { + assert_eq!(amt_msat, 0x0badf00d01020304); + assert_eq!(outgoing_cltv_value, 0xffffffff); + } else { panic!(); } } #[test] fn encoding_final_onion_hop_data_with_secret() { let expected_payment_secret = PaymentSecret([0x42u8; 32]); - let mut msg = msgs::OnionHopData { + let msg = msgs::OnionHopData { format: OnionHopDataFormat::FinalNode { payment_data: Some(FinalOnionHopData { payment_secret: expected_payment_secret, @@ -3581,19 +3596,21 @@ mod tests { let encoded_value = msg.encode(); let target_value = hex::decode("3602080badf00d010203040404ffffffff082442424242424242424242424242424242424242424242424242424242424242421badca1f").unwrap(); assert_eq!(encoded_value, target_value); - msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap(); - if let OnionHopDataFormat::FinalNode { + + let inbound_msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap(); + if let msgs::InboundOnionPayload::Receive { payment_data: Some(FinalOnionHopData { payment_secret, total_msat: 0x1badca1f }), + amt_msat, outgoing_cltv_value, payment_metadata: None, keysend_preimage: None, - } = msg.format { + } = inbound_msg { assert_eq!(payment_secret, expected_payment_secret); + assert_eq!(amt_msat, 0x0badf00d01020304); + assert_eq!(outgoing_cltv_value, 0xffffffff); } else { panic!(); } - assert_eq!(msg.amt_to_forward, 0x0badf00d01020304); - assert_eq!(msg.outgoing_cltv_value, 0xffffffff); } #[test] @@ -3743,7 +3760,7 @@ mod tests { // payload length to be encoded over multiple bytes rather than a single u8. let big_payload = encode_big_payload().unwrap(); let mut rd = Cursor::new(&big_payload[..]); - ::read(&mut rd).unwrap(); + ::read(&mut rd).unwrap(); } // see above test, needs to be a separate method for use of the serialization macros. fn encode_big_payload() -> Result, io::Error> { diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 52eb7bcb5..a915cf29d 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -763,11 +763,11 @@ impl NextPacketBytes for Vec { pub(crate) enum Hop { /// This onion payload was for us, not for forwarding to a next-hop. Contains information for /// verifying the incoming payment. - Receive(msgs::OnionHopData), + Receive(msgs::InboundOnionPayload), /// This onion payload needs to be forwarded to a next-hop. Forward { /// Onion payload data used in forwarding the payment. - next_hop_data: msgs::OnionHopData, + next_hop_data: msgs::InboundOnionPayload, /// HMAC of the next hop's onion packet. next_hop_hmac: [u8; 32], /// Bytes of the onion packet we're forwarding. From 67868aec721b38c993ce0e48ecc46549092963f6 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Fri, 24 Mar 2023 14:35:52 -0400 Subject: [PATCH 5/6] Replace OnionHopData with OutboundPayload for outbound onions Follows on from the previous commit, see its message --- lightning/src/ln/functional_tests.rs | 4 +- lightning/src/ln/msgs.rs | 92 ++++++++++++--------------- lightning/src/ln/onion_route_tests.rs | 10 +-- lightning/src/ln/onion_utils.rs | 67 ++++++++++--------- 4 files changed, 82 insertions(+), 91 deletions(-) diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 271ff541a..474735f25 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -8039,7 +8039,9 @@ fn test_onion_value_mpp_set_calculation() { RecipientOnionFields::secret_only(our_payment_secret), height + 1, &None).unwrap(); // Edit amt_to_forward to simulate the sender having set // the final amount and the routing node taking less fee - onion_payloads[1].amt_to_forward = 99_000; + if let msgs::OutboundOnionPayload::Receive { ref mut amt_msat, .. } = onion_payloads[1] { + *amt_msat = 99_000; + } else { panic!() } let new_onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &our_payment_hash).unwrap(); payment_event.msgs[0].onion_routing_packet = new_onion_packet; } diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 590b26632..0635dafd7 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -1442,24 +1442,22 @@ mod fuzzy_internal_msgs { }, } - pub(crate) enum OnionHopDataFormat { - NonFinalNode { + pub(crate) enum OutboundOnionPayload { + Forward { short_channel_id: u64, + /// The value, in msat, of the payment after this hop's fee is deducted. + amt_to_forward: u64, + outgoing_cltv_value: u32, }, - FinalNode { + Receive { payment_data: Option, payment_metadata: Option>, keysend_preimage: Option, + amt_msat: u64, + outgoing_cltv_value: u32, }, } - pub struct OnionHopData { - pub(crate) format: OnionHopDataFormat, - /// The value, in msat, of the payment after this hop's fee is deducted. - pub(crate) amt_to_forward: u64, - pub(crate) outgoing_cltv_value: u32, - } - pub struct DecodedOnionErrorPacket { pub(crate) hmac: [u8; 32], pub(crate) failuremsg: Vec, @@ -1966,20 +1964,22 @@ impl Readable for FinalOnionHopData { } } -impl Writeable for OnionHopData { +impl Writeable for OutboundOnionPayload { fn write(&self, w: &mut W) -> Result<(), io::Error> { - match self.format { - OnionHopDataFormat::NonFinalNode { short_channel_id } => { + match self { + Self::Forward { short_channel_id, amt_to_forward, outgoing_cltv_value } => { _encode_varint_length_prefixed_tlv!(w, { - (2, HighZeroBytesDroppedBigSize(self.amt_to_forward), required), - (4, HighZeroBytesDroppedBigSize(self.outgoing_cltv_value), required), + (2, HighZeroBytesDroppedBigSize(*amt_to_forward), required), + (4, HighZeroBytesDroppedBigSize(*outgoing_cltv_value), required), (6, short_channel_id, required) }); }, - OnionHopDataFormat::FinalNode { ref payment_data, ref payment_metadata, ref keysend_preimage } => { + Self::Receive { + ref payment_data, ref payment_metadata, ref keysend_preimage, amt_msat, outgoing_cltv_value + } => { _encode_varint_length_prefixed_tlv!(w, { - (2, HighZeroBytesDroppedBigSize(self.amt_to_forward), required), - (4, HighZeroBytesDroppedBigSize(self.outgoing_cltv_value), required), + (2, HighZeroBytesDroppedBigSize(*amt_msat), required), + (4, HighZeroBytesDroppedBigSize(*outgoing_cltv_value), required), (8, payment_data, option), (16, payment_metadata.as_ref().map(|m| WithoutLength(m)), option), (5482373484, keysend_preimage, option) @@ -2454,7 +2454,7 @@ mod tests { use hex; use crate::ln::{PaymentPreimage, PaymentHash, PaymentSecret}; use crate::ln::features::{ChannelFeatures, ChannelTypeFeatures, InitFeatures, NodeFeatures}; - use crate::ln::msgs::{self, FinalOnionHopData, OnionErrorPacket, OnionHopDataFormat}; + use crate::ln::msgs::{self, FinalOnionHopData, OnionErrorPacket}; use crate::routing::gossip::{NodeAlias, NodeId}; use crate::util::ser::{Writeable, Readable, Hostname, TransactionU16LenLimited}; @@ -3537,14 +3537,12 @@ mod tests { #[test] fn encoding_nonfinal_onion_hop_data() { - let msg = msgs::OnionHopData { - format: OnionHopDataFormat::NonFinalNode { - short_channel_id: 0xdeadbeef1bad1dea, - }, + let outbound_msg = msgs::OutboundOnionPayload::Forward { + short_channel_id: 0xdeadbeef1bad1dea, amt_to_forward: 0x0badf00d01020304, outgoing_cltv_value: 0xffffffff, }; - let encoded_value = msg.encode(); + let encoded_value = outbound_msg.encode(); let target_value = hex::decode("1a02080badf00d010203040404ffffffff0608deadbeef1bad1dea").unwrap(); assert_eq!(encoded_value, target_value); @@ -3558,16 +3556,14 @@ mod tests { #[test] fn encoding_final_onion_hop_data() { - let msg = msgs::OnionHopData { - format: OnionHopDataFormat::FinalNode { - payment_data: None, - payment_metadata: None, - keysend_preimage: None, - }, - amt_to_forward: 0x0badf00d01020304, + let outbound_msg = msgs::OutboundOnionPayload::Receive { + payment_data: None, + payment_metadata: None, + keysend_preimage: None, + amt_msat: 0x0badf00d01020304, outgoing_cltv_value: 0xffffffff, }; - let encoded_value = msg.encode(); + let encoded_value = outbound_msg.encode(); let target_value = hex::decode("1002080badf00d010203040404ffffffff").unwrap(); assert_eq!(encoded_value, target_value); @@ -3581,19 +3577,17 @@ mod tests { #[test] fn encoding_final_onion_hop_data_with_secret() { let expected_payment_secret = PaymentSecret([0x42u8; 32]); - let msg = msgs::OnionHopData { - format: OnionHopDataFormat::FinalNode { - payment_data: Some(FinalOnionHopData { - payment_secret: expected_payment_secret, - total_msat: 0x1badca1f - }), - payment_metadata: None, - keysend_preimage: None, - }, - amt_to_forward: 0x0badf00d01020304, + let outbound_msg = msgs::OutboundOnionPayload::Receive { + payment_data: Some(FinalOnionHopData { + payment_secret: expected_payment_secret, + total_msat: 0x1badca1f + }), + payment_metadata: None, + keysend_preimage: None, + amt_msat: 0x0badf00d01020304, outgoing_cltv_value: 0xffffffff, }; - let encoded_value = msg.encode(); + let encoded_value = outbound_msg.encode(); let target_value = hex::decode("3602080badf00d010203040404ffffffff082442424242424242424242424242424242424242424242424242424242424242421badca1f").unwrap(); assert_eq!(encoded_value, target_value); @@ -3765,20 +3759,18 @@ mod tests { // see above test, needs to be a separate method for use of the serialization macros. fn encode_big_payload() -> Result, io::Error> { use crate::util::ser::HighZeroBytesDroppedBigSize; - let payload = msgs::OnionHopData { - format: OnionHopDataFormat::NonFinalNode { - short_channel_id: 0xdeadbeef1bad1dea, - }, + let payload = msgs::OutboundOnionPayload::Forward { + short_channel_id: 0xdeadbeef1bad1dea, amt_to_forward: 1000, outgoing_cltv_value: 0xffffffff, }; let mut encoded_payload = Vec::new(); let test_bytes = vec![42u8; 1000]; - if let OnionHopDataFormat::NonFinalNode { short_channel_id } = payload.format { + if let msgs::OutboundOnionPayload::Forward { short_channel_id, amt_to_forward, outgoing_cltv_value } = payload { _encode_varint_length_prefixed_tlv!(&mut encoded_payload, { (1, test_bytes, required_vec), - (2, HighZeroBytesDroppedBigSize(payload.amt_to_forward), required), - (4, HighZeroBytesDroppedBigSize(payload.outgoing_cltv_value), required), + (2, HighZeroBytesDroppedBigSize(amt_to_forward), required), + (4, HighZeroBytesDroppedBigSize(outgoing_cltv_value), required), (6, short_channel_id, required) }); } diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index e5faedfe7..9945b8c49 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -252,7 +252,7 @@ struct BogusOnionHopData { data: Vec } impl BogusOnionHopData { - fn new(orig: msgs::OnionHopData) -> Self { + fn new(orig: msgs::OutboundOnionPayload) -> Self { Self { data: orig.encode() } } } @@ -875,15 +875,15 @@ fn test_always_create_tlv_format_onion_payloads() { let (onion_payloads, _htlc_msat, _htlc_cltv) = onion_utils::build_onion_payloads( &route.paths[0], 40000, RecipientOnionFields::spontaneous_empty(), cur_height, &None).unwrap(); - match onion_payloads[0].format { - msgs::OnionHopDataFormat::NonFinalNode {..} => {}, + match onion_payloads[0] { + msgs::OutboundOnionPayload::Forward {..} => {}, _ => { panic!( "Should have generated a `msgs::OnionHopDataFormat::NonFinalNode` payload for `hops[0]`, despite that the features signals no support for variable length onions" )} } - match onion_payloads[1].format { - msgs::OnionHopDataFormat::FinalNode {..} => {}, + match onion_payloads[1] { + msgs::OutboundOnionPayload::Receive {..} => {}, _ => {panic!( "Should have generated a `msgs::OnionHopDataFormat::FinalNode` payload for `hops[1]`, despite that the features signals no support for variable length onions" diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index a915cf29d..f3579a56b 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -149,11 +149,11 @@ pub(super) fn construct_onion_keys(secp_ctx: &Secp256k1) -> Result<(Vec, u64, u32), APIError> { +pub(super) fn build_onion_payloads(path: &Path, total_msat: u64, mut recipient_onion: RecipientOnionFields, starting_htlc_offset: u32, keysend_preimage: &Option) -> Result<(Vec, u64, u32), APIError> { let mut cur_value_msat = 0u64; let mut cur_cltv = starting_htlc_offset; let mut last_short_channel_id = 0; - let mut res: Vec = Vec::with_capacity(path.hops.len()); + let mut res: Vec = Vec::with_capacity(path.hops.len()); for (idx, hop) in path.hops.iter().rev().enumerate() { // First hop gets special values so that it can check, on receipt, that everything is @@ -161,25 +161,25 @@ pub(super) fn build_onion_payloads(path: &Path, total_msat: u64, mut recipient_o // the intended recipient). 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 { - format: if idx == 0 { - msgs::OnionHopDataFormat::FinalNode { - payment_data: if let Some(secret) = recipient_onion.payment_secret.take() { - Some(msgs::FinalOnionHopData { - payment_secret: secret, - total_msat, - }) - } else { None }, - payment_metadata: recipient_onion.payment_metadata.take(), - keysend_preimage: *keysend_preimage, - } - } else { - msgs::OnionHopDataFormat::NonFinalNode { - short_channel_id: last_short_channel_id, - } - }, - amt_to_forward: value_msat, - outgoing_cltv_value: cltv, + res.insert(0, if idx == 0 { + msgs::OutboundOnionPayload::Receive { + payment_data: if let Some(secret) = recipient_onion.payment_secret.take() { + Some(msgs::FinalOnionHopData { + payment_secret: secret, + total_msat, + }) + } else { None }, + payment_metadata: recipient_onion.payment_metadata.take(), + keysend_preimage: *keysend_preimage, + amt_msat: value_msat, + outgoing_cltv_value: cltv, + } + } else { + msgs::OutboundOnionPayload::Forward { + short_channel_id: last_short_channel_id, + amt_to_forward: value_msat, + outgoing_cltv_value: cltv, + } }); cur_value_msat += hop.fee_msat; if cur_value_msat >= 21000000 * 100000000 * 1000 { @@ -208,7 +208,10 @@ fn shift_slice_right(arr: &mut [u8], amt: usize) { } } -pub(super) fn construct_onion_packet(payloads: Vec, onion_keys: Vec, prng_seed: [u8; 32], associated_data: &PaymentHash) -> Result { +pub(super) fn construct_onion_packet( + payloads: Vec, onion_keys: Vec, prng_seed: [u8; 32], + associated_data: &PaymentHash +) -> Result { let mut packet_data = [0; ONION_DATA_LEN]; let mut chacha = ChaCha20::new(&prng_seed, &[0; 8]); @@ -988,10 +991,8 @@ mod tests { // with raw hex instead of our in-memory enums, as the payloads contains custom types, and // we have no way of representing that with our enums. let payloads = vec!( - RawOnionHopData::new(msgs::OnionHopData { - format: msgs::OnionHopDataFormat::NonFinalNode { - short_channel_id: 1, - }, + RawOnionHopData::new(msgs::OutboundOnionPayload::Forward { + short_channel_id: 1, amt_to_forward: 15000, outgoing_cltv_value: 1500, }), @@ -1013,17 +1014,13 @@ mod tests { RawOnionHopData { data: hex::decode("52020236b00402057806080000000000000002fd02013c0102030405060708090a0b0c0d0e0f0102030405060708090a0b0c0d0e0f0102030405060708090a0b0c0d0e0f0102030405060708090a0b0c0d0e0f").unwrap(), }, - RawOnionHopData::new(msgs::OnionHopData { - format: msgs::OnionHopDataFormat::NonFinalNode { - short_channel_id: 3, - }, + RawOnionHopData::new(msgs::OutboundOnionPayload::Forward { + short_channel_id: 3, amt_to_forward: 12500, outgoing_cltv_value: 1250, }), - RawOnionHopData::new(msgs::OnionHopData { - format: msgs::OnionHopDataFormat::NonFinalNode { - short_channel_id: 4, - }, + RawOnionHopData::new(msgs::OutboundOnionPayload::Forward { + short_channel_id: 4, amt_to_forward: 10000, outgoing_cltv_value: 1000, }), @@ -1101,7 +1098,7 @@ mod tests { data: Vec } impl RawOnionHopData { - fn new(orig: msgs::OnionHopData) -> Self { + fn new(orig: msgs::OutboundOnionPayload) -> Self { Self { data: orig.encode() } } } From c9d3544314f5823b8ac597996a80f695693d8702 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Wed, 12 Jul 2023 22:13:06 -0400 Subject: [PATCH 6/6] Remove unnecessary vecs in channel.rs --- lightning/src/ln/channel.rs | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 8232c5e1b..8e2ffbdde 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -3077,9 +3077,9 @@ impl Channel { let mut htlc_updates = Vec::new(); mem::swap(&mut htlc_updates, &mut self.context.holding_cell_htlc_updates); - let mut update_add_htlcs = Vec::with_capacity(htlc_updates.len()); - let mut update_fulfill_htlcs = Vec::with_capacity(htlc_updates.len()); - let mut update_fail_htlcs = Vec::with_capacity(htlc_updates.len()); + let mut update_add_count = 0; + let mut update_fulfill_count = 0; + let mut update_fail_count = 0; let mut htlcs_to_fail = Vec::new(); for htlc_update in htlc_updates.drain(..) { // Note that this *can* fail, though it should be due to rather-rare conditions on @@ -3095,7 +3095,7 @@ impl Channel { match self.send_htlc(amount_msat, *payment_hash, cltv_expiry, source.clone(), onion_routing_packet.clone(), false, skimmed_fee_msat, fee_estimator, logger) { - Ok(update_add_msg_option) => update_add_htlcs.push(update_add_msg_option.unwrap()), + Ok(_) => update_add_count += 1, Err(e) => { match e { ChannelError::Ignore(ref msg) => { @@ -3122,11 +3122,11 @@ impl Channel { // not fail - any in between attempts to claim the HTLC will have resulted // in it hitting the holding cell again and we cannot change the state of a // holding cell HTLC from fulfill to anything else. - let (update_fulfill_msg_option, mut additional_monitor_update) = - if let UpdateFulfillFetch::NewClaim { msg, monitor_update, .. } = self.get_update_fulfill_htlc(htlc_id, *payment_preimage, logger) { - (msg, monitor_update) - } else { unreachable!() }; - update_fulfill_htlcs.push(update_fulfill_msg_option.unwrap()); + let mut additional_monitor_update = + if let UpdateFulfillFetch::NewClaim { monitor_update, .. } = + self.get_update_fulfill_htlc(htlc_id, *payment_preimage, logger) + { monitor_update } else { unreachable!() }; + update_fulfill_count += 1; monitor_update.updates.append(&mut additional_monitor_update.updates); }, &HTLCUpdateAwaitingACK::FailHTLC { htlc_id, ref err_packet } => { @@ -3137,7 +3137,8 @@ impl Channel { // not fail - we should never end up in a state where we double-fail // an HTLC or fail-then-claim an HTLC as it indicates we didn't wait // for a full revocation before failing. - update_fail_htlcs.push(update_fail_msg_option.unwrap()) + debug_assert!(update_fail_msg_option.is_some()); + update_fail_count += 1; }, Err(e) => { if let ChannelError::Ignore(_) = e {} @@ -3149,7 +3150,7 @@ impl Channel { }, } } - if update_add_htlcs.is_empty() && update_fulfill_htlcs.is_empty() && update_fail_htlcs.is_empty() && self.context.holding_cell_update_fee.is_none() { + if update_add_count == 0 && update_fulfill_count == 0 && update_fail_count == 0 && self.context.holding_cell_update_fee.is_none() { return (None, htlcs_to_fail); } let update_fee = if let Some(feerate) = self.context.holding_cell_update_fee.take() { @@ -3166,7 +3167,7 @@ impl Channel { log_debug!(logger, "Freeing holding cell in channel {} resulted in {}{} HTLCs added, {} HTLCs fulfilled, and {} HTLCs failed.", log_bytes!(self.context.channel_id()), if update_fee.is_some() { "a fee update, " } else { "" }, - update_add_htlcs.len(), update_fulfill_htlcs.len(), update_fail_htlcs.len()); + update_add_count, update_fulfill_count, update_fail_count); self.monitor_updating_paused(false, true, false, Vec::new(), Vec::new(), Vec::new()); (self.push_ret_blockable_mon_update(monitor_update), htlcs_to_fail)