diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs index 76a7f884ad2..3fe17475fff 100644 --- a/lightning/src/events/mod.rs +++ b/lightning/src/events/mod.rs @@ -387,8 +387,25 @@ pub enum Event { /// /// Payments received on LDK versions prior to 0.0.115 will have this field unset. onion_fields: Option, - /// The value, in thousandths of a satoshi, that this payment is for. + /// The value, in thousandths of a satoshi, that this payment is claimable for. May be greater + /// than the invoice amount. + /// + /// May be less than the invoice amount if [`ChannelConfig::accept_underpaying_htlcs`] is set + /// and the previous hop took an extra fee. + /// + /// # Note + /// If [`ChannelConfig::accept_underpaying_htlcs`] is set and you claim without verifying this + /// field, you may lose money! + /// + /// [`ChannelConfig::accept_underpaying_htlcs`]: crate::util::config::ChannelConfig::accept_underpaying_htlcs amount_msat: u64, + /// The value, in thousands of a satoshi, that was skimmed off of this payment as an extra fee + /// taken by our channel counterparty. + /// + /// Will always be 0 unless [`ChannelConfig::accept_underpaying_htlcs`] is set. + /// + /// [`ChannelConfig::accept_underpaying_htlcs`]: crate::util::config::ChannelConfig::accept_underpaying_htlcs + counterparty_skimmed_fee_msat: u64, /// Information for claiming this received payment, based on whether the purpose of the /// payment is to pay an invoice or to send a spontaneous payment. purpose: PaymentPurpose, @@ -430,7 +447,8 @@ pub enum Event { /// The payment hash of the claimed payment. Note that LDK will not stop you from /// registering duplicate payment hashes for inbound payments. payment_hash: PaymentHash, - /// The value, in thousandths of a satoshi, that this payment is for. + /// The value, in thousandths of a satoshi, that this payment is for. May be greater than the + /// invoice amount. amount_msat: u64, /// The purpose of the claimed payment, i.e. whether the payment was for an invoice or a /// spontaneous payment. @@ -623,6 +641,7 @@ pub enum Event { inbound_amount_msat: u64, /// How many msats the payer intended to route to the next node. Depending on the reason you are /// intercepting this payment, you might take a fee by forwarding less than this amount. + /// Forwarding less than this amount may break compatibility with LDK versions prior to 0.0.116. /// /// Note that LDK will NOT check that expected fees were factored into this value. You MUST /// check that whatever fee you want has been included here or subtract it as required. Further, @@ -832,8 +851,8 @@ impl Writeable for Event { // We never write out FundingGenerationReady events as, upon disconnection, peers // drop any channels which have not yet exchanged funding_signed. }, - &Event::PaymentClaimable { ref payment_hash, ref amount_msat, ref purpose, - ref receiver_node_id, ref via_channel_id, ref via_user_channel_id, + &Event::PaymentClaimable { ref payment_hash, ref amount_msat, counterparty_skimmed_fee_msat, + ref purpose, ref receiver_node_id, ref via_channel_id, ref via_user_channel_id, ref claim_deadline, ref onion_fields } => { 1u8.write(writer)?; @@ -848,6 +867,8 @@ impl Writeable for Event { payment_preimage = Some(*preimage); } } + let skimmed_fee_opt = if counterparty_skimmed_fee_msat == 0 { None } + else { Some(counterparty_skimmed_fee_msat) }; write_tlv_fields!(writer, { (0, payment_hash, required), (1, receiver_node_id, option), @@ -859,6 +880,7 @@ impl Writeable for Event { (7, claim_deadline, option), (8, payment_preimage, option), (9, onion_fields, option), + (10, skimmed_fee_opt, option), }); }, &Event::PaymentSent { ref payment_id, ref payment_preimage, ref payment_hash, ref fee_paid_msat } => { @@ -1058,6 +1080,7 @@ impl MaybeReadable for Event { let mut payment_preimage = None; let mut payment_secret = None; let mut amount_msat = 0; + let mut counterparty_skimmed_fee_msat_opt = None; let mut receiver_node_id = None; let mut _user_payment_id = None::; // For compatibility with 0.0.103 and earlier let mut via_channel_id = None; @@ -1075,6 +1098,7 @@ impl MaybeReadable for Event { (7, claim_deadline, option), (8, payment_preimage, option), (9, onion_fields, option), + (10, counterparty_skimmed_fee_msat_opt, option), }); let purpose = match payment_secret { Some(secret) => PaymentPurpose::InvoicePayment { @@ -1088,6 +1112,7 @@ impl MaybeReadable for Event { receiver_node_id, payment_hash, amount_msat, + counterparty_skimmed_fee_msat: counterparty_skimmed_fee_msat_opt.unwrap_or(0), purpose, via_channel_id, via_user_channel_id, diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index e74dbe42a77..388ef6a8b30 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -224,6 +224,7 @@ struct OutboundHTLCOutput { payment_hash: PaymentHash, state: OutboundHTLCState, source: HTLCSource, + skimmed_fee_msat: Option, } /// See AwaitingRemoteRevoke ChannelState for more info @@ -235,6 +236,8 @@ enum HTLCUpdateAwaitingACK { payment_hash: PaymentHash, source: HTLCSource, onion_routing_packet: msgs::OnionPacket, + // The extra fee we're skimming off the top of this HTLC. + skimmed_fee_msat: Option, }, ClaimHTLC { payment_preimage: PaymentPreimage, @@ -3052,8 +3055,13 @@ impl Channel { // handling this case better and maybe fulfilling some of the HTLCs while attempting // to rebalance channels. match &htlc_update { - &HTLCUpdateAwaitingACK::AddHTLC {amount_msat, cltv_expiry, ref payment_hash, ref source, ref onion_routing_packet, ..} => { - match self.send_htlc(amount_msat, *payment_hash, cltv_expiry, source.clone(), onion_routing_packet.clone(), false, logger) { + &HTLCUpdateAwaitingACK::AddHTLC { + amount_msat, cltv_expiry, ref payment_hash, ref source, ref onion_routing_packet, + skimmed_fee_msat, .. + } => { + match self.send_htlc(amount_msat, *payment_hash, cltv_expiry, source.clone(), + onion_routing_packet.clone(), false, skimmed_fee_msat, logger) + { Ok(update_add_msg_option) => update_add_htlcs.push(update_add_msg_option.unwrap()), Err(e) => { match e { @@ -3695,6 +3703,7 @@ impl Channel { payment_hash: htlc.payment_hash, cltv_expiry: htlc.cltv_expiry, onion_routing_packet: (**onion_packet).clone(), + skimmed_fee_msat: htlc.skimmed_fee_msat, }); } } @@ -5042,11 +5051,13 @@ impl Channel { /// commitment update. /// /// `Err`s will only be [`ChannelError::Ignore`]. - pub fn queue_add_htlc(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, - onion_routing_packet: msgs::OnionPacket, logger: &L) - -> Result<(), ChannelError> where L::Target: Logger { + pub fn queue_add_htlc( + &mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, + onion_routing_packet: msgs::OnionPacket, skimmed_fee_msat: Option, logger: &L + ) -> Result<(), ChannelError> where L::Target: Logger { self - .send_htlc(amount_msat, payment_hash, cltv_expiry, source, onion_routing_packet, true, logger) + .send_htlc(amount_msat, payment_hash, cltv_expiry, source, onion_routing_packet, true, + skimmed_fee_msat, logger) .map(|msg_opt| assert!(msg_opt.is_none(), "We forced holding cell?")) .map_err(|err| { if let ChannelError::Ignore(_) = err { /* fine */ } @@ -5071,9 +5082,11 @@ impl Channel { /// on this [`Channel`] if `force_holding_cell` is false. /// /// `Err`s will only be [`ChannelError::Ignore`]. - fn send_htlc(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, - onion_routing_packet: msgs::OnionPacket, mut force_holding_cell: bool, logger: &L) - -> Result, ChannelError> where L::Target: Logger { + fn send_htlc( + &mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, + onion_routing_packet: msgs::OnionPacket, mut force_holding_cell: bool, + skimmed_fee_msat: Option, logger: &L + ) -> Result, ChannelError> where L::Target: Logger { if (self.context.channel_state & (ChannelState::ChannelReady as u32 | BOTH_SIDES_SHUTDOWN_MASK)) != (ChannelState::ChannelReady as u32) { return Err(ChannelError::Ignore("Cannot send HTLC until channel is fully established and we haven't started shutting down".to_owned())); } @@ -5125,6 +5138,7 @@ impl Channel { cltv_expiry, source, onion_routing_packet, + skimmed_fee_msat, }); return Ok(None); } @@ -5136,6 +5150,7 @@ impl Channel { cltv_expiry, state: OutboundHTLCState::LocalAnnounced(Box::new(onion_routing_packet.clone())), source, + skimmed_fee_msat, }); let res = msgs::UpdateAddHTLC { @@ -5145,6 +5160,7 @@ impl Channel { payment_hash, cltv_expiry, onion_routing_packet, + skimmed_fee_msat, }; self.context.next_holder_htlc_id += 1; @@ -5283,8 +5299,12 @@ impl Channel { /// /// Shorthand for calling [`Self::send_htlc`] followed by a commitment update, see docs on /// [`Self::send_htlc`] and [`Self::build_commitment_no_state_update`] for more info. - pub fn send_htlc_and_commit(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, onion_routing_packet: msgs::OnionPacket, logger: &L) -> Result, ChannelError> where L::Target: Logger { - let send_res = self.send_htlc(amount_msat, payment_hash, cltv_expiry, source, onion_routing_packet, false, logger); + pub fn send_htlc_and_commit( + &mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, + onion_routing_packet: msgs::OnionPacket, skimmed_fee_msat: Option, logger: &L + ) -> Result, ChannelError> where L::Target: Logger { + let send_res = self.send_htlc(amount_msat, payment_hash, cltv_expiry, source, + onion_routing_packet, false, skimmed_fee_msat, logger); if let Err(e) = &send_res { if let ChannelError::Ignore(_) = e {} else { debug_assert!(false, "Sending cannot trigger channel failure"); } } match send_res? { Some(_) => { @@ -6609,9 +6629,10 @@ impl Writeable for Channel { } let mut preimages: Vec<&Option> = vec![]; + let mut pending_outbound_skimmed_fees: Vec> = Vec::new(); (self.context.pending_outbound_htlcs.len() as u64).write(writer)?; - for htlc in self.context.pending_outbound_htlcs.iter() { + for (idx, htlc) in self.context.pending_outbound_htlcs.iter().enumerate() { htlc.htlc_id.write(writer)?; htlc.amount_msat.write(writer)?; htlc.cltv_expiry.write(writer)?; @@ -6647,18 +6668,37 @@ impl Writeable for Channel { reason.write(writer)?; } } + if let Some(skimmed_fee) = htlc.skimmed_fee_msat { + if pending_outbound_skimmed_fees.is_empty() { + for _ in 0..idx { pending_outbound_skimmed_fees.push(None); } + } + pending_outbound_skimmed_fees.push(Some(skimmed_fee)); + } else if !pending_outbound_skimmed_fees.is_empty() { + pending_outbound_skimmed_fees.push(None); + } } + let mut holding_cell_skimmed_fees: Vec> = Vec::new(); (self.context.holding_cell_htlc_updates.len() as u64).write(writer)?; - for update in self.context.holding_cell_htlc_updates.iter() { + for (idx, update) in self.context.holding_cell_htlc_updates.iter().enumerate() { match update { - &HTLCUpdateAwaitingACK::AddHTLC { ref amount_msat, ref cltv_expiry, ref payment_hash, ref source, ref onion_routing_packet } => { + &HTLCUpdateAwaitingACK::AddHTLC { + ref amount_msat, ref cltv_expiry, ref payment_hash, ref source, ref onion_routing_packet, + skimmed_fee_msat, + } => { 0u8.write(writer)?; amount_msat.write(writer)?; cltv_expiry.write(writer)?; payment_hash.write(writer)?; source.write(writer)?; onion_routing_packet.write(writer)?; + + if let Some(skimmed_fee) = skimmed_fee_msat { + if holding_cell_skimmed_fees.is_empty() { + for _ in 0..idx { holding_cell_skimmed_fees.push(None); } + } + holding_cell_skimmed_fees.push(Some(skimmed_fee)); + } else if !holding_cell_skimmed_fees.is_empty() { holding_cell_skimmed_fees.push(None); } }, &HTLCUpdateAwaitingACK::ClaimHTLC { ref payment_preimage, ref htlc_id } => { 1u8.write(writer)?; @@ -6825,6 +6865,8 @@ impl Writeable for Channel { (29, self.context.temporary_channel_id, option), (31, channel_pending_event_emitted, option), (33, self.context.pending_monitor_updates, vec_type), + (35, pending_outbound_skimmed_fees, optional_vec), + (37, holding_cell_skimmed_fees, optional_vec), }); Ok(()) @@ -6935,6 +6977,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch }, _ => return Err(DecodeError::InvalidValue), }, + skimmed_fee_msat: None, }); } @@ -6948,6 +6991,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch payment_hash: Readable::read(reader)?, source: Readable::read(reader)?, onion_routing_packet: Readable::read(reader)?, + skimmed_fee_msat: None, }, 1 => HTLCUpdateAwaitingACK::ClaimHTLC { payment_preimage: Readable::read(reader)?, @@ -7103,6 +7147,9 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch let mut pending_monitor_updates = Some(Vec::new()); + let mut pending_outbound_skimmed_fees_opt: Option>> = None; + let mut holding_cell_skimmed_fees_opt: Option>> = None; + read_tlv_fields!(reader, { (0, announcement_sigs, option), (1, minimum_depth, option), @@ -7126,6 +7173,8 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch (29, temporary_channel_id, option), (31, channel_pending_event_emitted, option), (33, pending_monitor_updates, vec_type), + (35, pending_outbound_skimmed_fees_opt, optional_vec), + (37, holding_cell_skimmed_fees_opt, optional_vec), }); let (channel_keys_id, holder_signer) = if let Some(channel_keys_id) = channel_keys_id { @@ -7180,6 +7229,25 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch let holder_max_accepted_htlcs = holder_max_accepted_htlcs.unwrap_or(DEFAULT_MAX_HTLCS); + if let Some(skimmed_fees) = pending_outbound_skimmed_fees_opt { + let mut iter = skimmed_fees.into_iter(); + for htlc in pending_outbound_htlcs.iter_mut() { + htlc.skimmed_fee_msat = iter.next().ok_or(DecodeError::InvalidValue)?; + } + // We expect all skimmed fees to be consumed above + if iter.next().is_some() { return Err(DecodeError::InvalidValue) } + } + if let Some(skimmed_fees) = holding_cell_skimmed_fees_opt { + let mut iter = skimmed_fees.into_iter(); + for htlc in holding_cell_htlc_updates.iter_mut() { + if let HTLCUpdateAwaitingACK::AddHTLC { ref mut skimmed_fee_msat, .. } = htlc { + *skimmed_fee_msat = iter.next().ok_or(DecodeError::InvalidValue)?; + } + } + // We expect all skimmed fees to be consumed above + if iter.next().is_some() { return Err(DecodeError::InvalidValue) } + } + Ok(Channel { context: ChannelContext { user_id, @@ -7522,7 +7590,8 @@ mod tests { session_priv: SecretKey::from_slice(&hex::decode("0fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap()[..]).unwrap(), first_hop_htlc_msat: 548, payment_id: PaymentId([42; 32]), - } + }, + skimmed_fee_msat: None, }); // Make sure when Node A calculates their local commitment transaction, none of the HTLCs pass @@ -8079,6 +8148,7 @@ mod tests { payment_hash: PaymentHash([0; 32]), state: OutboundHTLCState::Committed, source: HTLCSource::dummy(), + skimmed_fee_msat: None, }; out.payment_hash.0 = Sha256::hash(&hex::decode("0202020202020202020202020202020202020202020202020202020202020202").unwrap()).into_inner(); out @@ -8091,6 +8161,7 @@ mod tests { payment_hash: PaymentHash([0; 32]), state: OutboundHTLCState::Committed, source: HTLCSource::dummy(), + skimmed_fee_msat: None, }; out.payment_hash.0 = Sha256::hash(&hex::decode("0303030303030303030303030303030303030303030303030303030303030303").unwrap()).into_inner(); out @@ -8492,6 +8563,7 @@ mod tests { payment_hash: PaymentHash([0; 32]), state: OutboundHTLCState::Committed, source: HTLCSource::dummy(), + skimmed_fee_msat: None, }; out.payment_hash.0 = Sha256::hash(&hex::decode("0505050505050505050505050505050505050505050505050505050505050505").unwrap()).into_inner(); out @@ -8504,6 +8576,7 @@ mod tests { payment_hash: PaymentHash([0; 32]), state: OutboundHTLCState::Committed, source: HTLCSource::dummy(), + skimmed_fee_msat: None, }; out.payment_hash.0 = Sha256::hash(&hex::decode("0505050505050505050505050505050505050505050505050505050505050505").unwrap()).into_inner(); out diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 164e4c2242d..aef1abe307f 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -131,6 +131,9 @@ pub(super) struct PendingHTLCInfo { /// may overshoot this in either case) pub(super) outgoing_amt_msat: u64, pub(super) outgoing_cltv_value: u32, + /// The fee being skimmed off the top of this HTLC. If this is a forward, it'll be the fee we are + /// skimming. If we're receiving this HTLC, it's the fee that our counterparty skimmed. + pub(super) skimmed_fee_msat: Option, } #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug @@ -210,6 +213,8 @@ struct ClaimableHTLC { total_value_received: Option, /// The sender intended sum total of all MPP parts specified in the onion total_msat: u64, + /// The extra fee our counterparty skimmed off the top of this HTLC. + counterparty_skimmed_fee_msat: Option, } /// A payment identifier used to uniquely identify a payment to LDK. @@ -2521,9 +2526,11 @@ where } } - 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]>) -> Result - { + 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 { // final_incorrect_cltv_expiry if hop_data.outgoing_cltv_value > cltv_expiry { return Err(ReceiveError { @@ -2549,7 +2556,10 @@ where msg: "The final CLTV expiry is too soon to handle", }); } - if hop_data.amt_to_forward > amt_msat { + if (!allow_underpay && hop_data.amt_to_forward > amt_msat) || + (allow_underpay && hop_data.amt_to_forward > + amt_msat.saturating_add(counterparty_skimmed_fee_msat.unwrap_or(0))) + { return Err(ReceiveError { err_code: 19, err_data: amt_msat.to_be_bytes().to_vec(), @@ -2616,15 +2626,18 @@ where incoming_amt_msat: Some(amt_msat), outgoing_amt_msat: hop_data.amt_to_forward, outgoing_cltv_value: hop_data.outgoing_cltv_value, + skimmed_fee_msat: counterparty_skimmed_fee_msat, }) } - fn decode_update_add_htlc_onion(&self, msg: &msgs::UpdateAddHTLC) -> PendingHTLCStatus { + fn decode_update_add_htlc_onion( + &self, msg: &msgs::UpdateAddHTLC + ) -> Result<(onion_utils::Hop, [u8; 32], Option>), HTLCFailureMsg> { macro_rules! return_malformed_err { ($msg: expr, $err_code: expr) => { { log_info!(self.logger, "Failed to accept/forward incoming HTLC: {}", $msg); - return PendingHTLCStatus::Fail(HTLCFailureMsg::Malformed(msgs::UpdateFailMalformedHTLC { + return Err(HTLCFailureMsg::Malformed(msgs::UpdateFailMalformedHTLC { channel_id: msg.channel_id, htlc_id: msg.htlc_id, sha256_of_onion: Sha256::hash(&msg.onion_routing_packet.hop_data).into_inner(), @@ -2655,7 +2668,7 @@ where ($msg: expr, $err_code: expr, $data: expr) => { { log_info!(self.logger, "Failed to accept/forward incoming HTLC: {}", $msg); - return PendingHTLCStatus::Fail(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC { + return Err(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC { channel_id: msg.channel_id, htlc_id: msg.htlc_id, reason: HTLCFailReason::reason($err_code, $data.to_vec()) @@ -2674,11 +2687,186 @@ where return_err!(err_msg, err_code, &[0; 0]); }, }; + 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, + }, .. + } => { + let next_pk = onion_utils::next_hop_packet_pubkey(&self.secp_ctx, + msg.onion_routing_packet.public_key.unwrap(), &shared_secret); + (short_channel_id, amt_to_forward, outgoing_cltv_value, Some(next_pk)) + }, + // 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 { .. }, .. }, .. + } => { + return_err!("Final Node OnionHopData provided for us as an intermediary node", 0x4000 | 22, &[0; 0]); + } + }; + + // Perform outbound checks here instead of in [`Self::construct_pending_htlc_info`] because we + // can't hold the outbound peer state lock at the same time as the inbound peer state lock. + if let Some((err, mut code, chan_update)) = loop { + let id_option = self.short_to_chan_info.read().unwrap().get(&outgoing_scid).cloned(); + let forwarding_chan_info_opt = match id_option { + None => { // unknown_next_peer + // Note that this is likely a timing oracle for detecting whether an scid is a + // phantom or an intercept. + if (self.default_configuration.accept_intercept_htlcs && + fake_scid::is_valid_intercept(&self.fake_scid_rand_bytes, outgoing_scid, &self.genesis_hash)) || + fake_scid::is_valid_phantom(&self.fake_scid_rand_bytes, outgoing_scid, &self.genesis_hash) + { + None + } else { + break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None)); + } + }, + Some((cp_id, id)) => Some((cp_id.clone(), id.clone())), + }; + let chan_update_opt = if let Some((counterparty_node_id, forwarding_id)) = forwarding_chan_info_opt { + let per_peer_state = self.per_peer_state.read().unwrap(); + let peer_state_mutex_opt = per_peer_state.get(&counterparty_node_id); + if peer_state_mutex_opt.is_none() { + break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None)); + } + let mut peer_state_lock = peer_state_mutex_opt.unwrap().lock().unwrap(); + let peer_state = &mut *peer_state_lock; + let chan = match peer_state.channel_by_id.get_mut(&forwarding_id) { + None => { + // Channel was removed. The short_to_chan_info and channel_by_id maps + // have no consistency guarantees. + break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None)); + }, + Some(chan) => chan + }; + if !chan.context.should_announce() && !self.default_configuration.accept_forwards_to_priv_channels { + // Note that the behavior here should be identical to the above block - we + // should NOT reveal the existence or non-existence of a private channel if + // we don't allow forwards outbound over them. + break Some(("Refusing to forward to a private channel based on our config.", 0x4000 | 10, None)); + } + if chan.context.get_channel_type().supports_scid_privacy() && outgoing_scid != chan.context.outbound_scid_alias() { + // `option_scid_alias` (referred to in LDK as `scid_privacy`) means + // "refuse to forward unless the SCID alias was used", so we pretend + // we don't have the channel here. + break Some(("Refusing to forward over real channel SCID as our counterparty requested.", 0x4000 | 10, None)); + } + let chan_update_opt = self.get_channel_update_for_onion(outgoing_scid, chan).ok(); + + // Note that we could technically not return an error yet here and just hope + // that the connection is reestablished or monitor updated by the time we get + // around to doing the actual forward, but better to fail early if we can and + // hopefully an attacker trying to path-trace payments cannot make this occur + // on a small/per-node/per-channel scale. + if !chan.context.is_live() { // channel_disabled + // If the channel_update we're going to return is disabled (i.e. the + // peer has been disabled for some time), return `channel_disabled`, + // otherwise return `temporary_channel_failure`. + if chan_update_opt.as_ref().map(|u| u.contents.flags & 2 == 2).unwrap_or(false) { + break Some(("Forwarding channel has been disconnected for some time.", 0x1000 | 20, chan_update_opt)); + } else { + break Some(("Forwarding channel is not in a ready state.", 0x1000 | 7, chan_update_opt)); + } + } + if outgoing_amt_msat < chan.context.get_counterparty_htlc_minimum_msat() { // amount_below_minimum + break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, chan_update_opt)); + } + if let Err((err, code)) = chan.htlc_satisfies_config(&msg, outgoing_amt_msat, outgoing_cltv_value) { + break Some((err, code, chan_update_opt)); + } + chan_update_opt + } else { + if (msg.cltv_expiry as u64) < (outgoing_cltv_value) as u64 + MIN_CLTV_EXPIRY_DELTA as u64 { + // We really should set `incorrect_cltv_expiry` here but as we're not + // forwarding over a real channel we can't generate a channel_update + // for it. Instead we just return a generic temporary_node_failure. + break Some(( + "Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta", + 0x2000 | 2, None, + )); + } + None + }; + + let cur_height = self.best_block.read().unwrap().height() + 1; + // Theoretically, channel counterparty shouldn't send us a HTLC expiring now, + // but we want to be robust wrt to counterparty packet sanitization (see + // HTLC_FAIL_BACK_BUFFER rationale). + if msg.cltv_expiry <= cur_height + HTLC_FAIL_BACK_BUFFER as u32 { // expiry_too_soon + break Some(("CLTV expiry is too close", 0x1000 | 14, chan_update_opt)); + } + if msg.cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far + break Some(("CLTV expiry is too far in the future", 21, None)); + } + // If the HTLC expires ~now, don't bother trying to forward it to our + // counterparty. They should fail it anyway, but we don't want to bother with + // the round-trips or risk them deciding they definitely want the HTLC and + // force-closing to ensure they get it if we're offline. + // We previously had a much more aggressive check here which tried to ensure + // our counterparty receives an HTLC which has *our* risk threshold met on it, + // but there is no need to do that, and since we're a bit conservative with our + // risk threshold it just results in failing to forward payments. + if (outgoing_cltv_value) as u64 <= (cur_height + LATENCY_GRACE_PERIOD_BLOCKS) as u64 { + break Some(("Outgoing CLTV value is too soon", 0x1000 | 14, chan_update_opt)); + } + + break None; + } + { + let mut res = VecWriter(Vec::with_capacity(chan_update.serialized_length() + 2 + 8 + 2)); + if let Some(chan_update) = chan_update { + if code == 0x1000 | 11 || code == 0x1000 | 12 { + msg.amount_msat.write(&mut res).expect("Writes cannot fail"); + } + else if code == 0x1000 | 13 { + msg.cltv_expiry.write(&mut res).expect("Writes cannot fail"); + } + else if code == 0x1000 | 20 { + // TODO: underspecified, follow https://github.com/lightning/bolts/issues/791 + 0u16.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"); + } else if code & 0x1000 == 0x1000 { + // If we're trying to return an error that requires a `channel_update` but + // we're forwarding to a phantom or intercept "channel" (i.e. cannot + // generate an update), just use the generic "temporary_node_failure" + // instead. + code = 0x2000 | 2; + } + return_err!(err, code, &res.0[..]); + } + Ok((next_hop, shared_secret, next_packet_pk_opt)) + } - let pending_forward_info = match next_hop { + fn construct_pending_htlc_status<'a>( + &self, msg: &msgs::UpdateAddHTLC, shared_secret: [u8; 32], decoded_hop: onion_utils::Hop, + allow_underpay: bool, next_packet_pubkey_opt: Option> + ) -> PendingHTLCStatus { + macro_rules! return_err { + ($msg: expr, $err_code: expr, $data: expr) => { + { + log_info!(self.logger, "Failed to accept/forward incoming HTLC: {}", $msg); + return PendingHTLCStatus::Fail(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC { + channel_id: msg.channel_id, + htlc_id: msg.htlc_id, + reason: HTLCFailReason::reason($err_code, $data.to_vec()) + .get_encrypted_failure_packet(&shared_secret, &None), + })); + } + } + } + match decoded_hop { onion_utils::Hop::Receive(next_hop_data) => { // OUR PAYMENT! - match self.construct_recv_pending_htlc_info(next_hop_data, shared_secret, msg.payment_hash, msg.amount_msat, msg.cltv_expiry, None) { + match self.construct_recv_pending_htlc_info(next_hop_data, shared_secret, msg.payment_hash, + msg.amount_msat, msg.cltv_expiry, None, allow_underpay, msg.skimmed_fee_msat) + { Ok(info) => { // Note that we could obviously respond immediately with an update_fulfill_htlc // message, however that would leak that we are the recipient of this payment, so @@ -2690,10 +2878,10 @@ where } }, onion_utils::Hop::Forward { next_hop_data, next_hop_hmac, new_packet_bytes } => { - let new_pubkey = msg.onion_routing_packet.public_key.unwrap(); + debug_assert!(next_packet_pubkey_opt.is_some()); let outgoing_packet = msgs::OnionPacket { version: 0, - public_key: onion_utils::next_hop_packet_pubkey(&self.secp_ctx, new_pubkey, &shared_secret), + public_key: next_packet_pubkey_opt.unwrap_or(Err(secp256k1::Error::InvalidPublicKey)), hop_data: new_packet_bytes, hmac: next_hop_hmac.clone(), }; @@ -2715,150 +2903,10 @@ where 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, }) } - }; - - if let &PendingHTLCStatus::Forward(PendingHTLCInfo { ref routing, ref outgoing_amt_msat, ref outgoing_cltv_value, .. }) = &pending_forward_info { - // If short_channel_id is 0 here, we'll reject the HTLC as there cannot be a channel - // with a short_channel_id of 0. This is important as various things later assume - // short_channel_id is non-0 in any ::Forward. - if let &PendingHTLCRouting::Forward { ref short_channel_id, .. } = routing { - if let Some((err, mut code, chan_update)) = loop { - let id_option = self.short_to_chan_info.read().unwrap().get(short_channel_id).cloned(); - let forwarding_chan_info_opt = match id_option { - None => { // unknown_next_peer - // Note that this is likely a timing oracle for detecting whether an scid is a - // phantom or an intercept. - if (self.default_configuration.accept_intercept_htlcs && - fake_scid::is_valid_intercept(&self.fake_scid_rand_bytes, *short_channel_id, &self.genesis_hash)) || - fake_scid::is_valid_phantom(&self.fake_scid_rand_bytes, *short_channel_id, &self.genesis_hash) - { - None - } else { - break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None)); - } - }, - Some((cp_id, id)) => Some((cp_id.clone(), id.clone())), - }; - let chan_update_opt = if let Some((counterparty_node_id, forwarding_id)) = forwarding_chan_info_opt { - let per_peer_state = self.per_peer_state.read().unwrap(); - let peer_state_mutex_opt = per_peer_state.get(&counterparty_node_id); - if peer_state_mutex_opt.is_none() { - break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None)); - } - let mut peer_state_lock = peer_state_mutex_opt.unwrap().lock().unwrap(); - let peer_state = &mut *peer_state_lock; - let chan = match peer_state.channel_by_id.get_mut(&forwarding_id) { - None => { - // Channel was removed. The short_to_chan_info and channel_by_id maps - // have no consistency guarantees. - break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None)); - }, - Some(chan) => chan - }; - if !chan.context.should_announce() && !self.default_configuration.accept_forwards_to_priv_channels { - // Note that the behavior here should be identical to the above block - we - // should NOT reveal the existence or non-existence of a private channel if - // we don't allow forwards outbound over them. - break Some(("Refusing to forward to a private channel based on our config.", 0x4000 | 10, None)); - } - if chan.context.get_channel_type().supports_scid_privacy() && *short_channel_id != chan.context.outbound_scid_alias() { - // `option_scid_alias` (referred to in LDK as `scid_privacy`) means - // "refuse to forward unless the SCID alias was used", so we pretend - // we don't have the channel here. - break Some(("Refusing to forward over real channel SCID as our counterparty requested.", 0x4000 | 10, None)); - } - let chan_update_opt = self.get_channel_update_for_onion(*short_channel_id, chan).ok(); - - // Note that we could technically not return an error yet here and just hope - // that the connection is reestablished or monitor updated by the time we get - // around to doing the actual forward, but better to fail early if we can and - // hopefully an attacker trying to path-trace payments cannot make this occur - // on a small/per-node/per-channel scale. - if !chan.context.is_live() { // channel_disabled - // If the channel_update we're going to return is disabled (i.e. the - // peer has been disabled for some time), return `channel_disabled`, - // otherwise return `temporary_channel_failure`. - if chan_update_opt.as_ref().map(|u| u.contents.flags & 2 == 2).unwrap_or(false) { - break Some(("Forwarding channel has been disconnected for some time.", 0x1000 | 20, chan_update_opt)); - } else { - break Some(("Forwarding channel is not in a ready state.", 0x1000 | 7, chan_update_opt)); - } - } - if *outgoing_amt_msat < chan.context.get_counterparty_htlc_minimum_msat() { // amount_below_minimum - break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, chan_update_opt)); - } - if let Err((err, code)) = chan.htlc_satisfies_config(&msg, *outgoing_amt_msat, *outgoing_cltv_value) { - break Some((err, code, chan_update_opt)); - } - chan_update_opt - } else { - if (msg.cltv_expiry as u64) < (*outgoing_cltv_value) as u64 + MIN_CLTV_EXPIRY_DELTA as u64 { - // We really should set `incorrect_cltv_expiry` here but as we're not - // forwarding over a real channel we can't generate a channel_update - // for it. Instead we just return a generic temporary_node_failure. - break Some(( - "Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta", - 0x2000 | 2, None, - )); - } - None - }; - - let cur_height = self.best_block.read().unwrap().height() + 1; - // Theoretically, channel counterparty shouldn't send us a HTLC expiring now, - // but we want to be robust wrt to counterparty packet sanitization (see - // HTLC_FAIL_BACK_BUFFER rationale). - if msg.cltv_expiry <= cur_height + HTLC_FAIL_BACK_BUFFER as u32 { // expiry_too_soon - break Some(("CLTV expiry is too close", 0x1000 | 14, chan_update_opt)); - } - if msg.cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far - break Some(("CLTV expiry is too far in the future", 21, None)); - } - // If the HTLC expires ~now, don't bother trying to forward it to our - // counterparty. They should fail it anyway, but we don't want to bother with - // the round-trips or risk them deciding they definitely want the HTLC and - // force-closing to ensure they get it if we're offline. - // We previously had a much more aggressive check here which tried to ensure - // our counterparty receives an HTLC which has *our* risk threshold met on it, - // but there is no need to do that, and since we're a bit conservative with our - // risk threshold it just results in failing to forward payments. - if (*outgoing_cltv_value) as u64 <= (cur_height + LATENCY_GRACE_PERIOD_BLOCKS) as u64 { - break Some(("Outgoing CLTV value is too soon", 0x1000 | 14, chan_update_opt)); - } - - break None; - } - { - let mut res = VecWriter(Vec::with_capacity(chan_update.serialized_length() + 2 + 8 + 2)); - if let Some(chan_update) = chan_update { - if code == 0x1000 | 11 || code == 0x1000 | 12 { - msg.amount_msat.write(&mut res).expect("Writes cannot fail"); - } - else if code == 0x1000 | 13 { - msg.cltv_expiry.write(&mut res).expect("Writes cannot fail"); - } - else if code == 0x1000 | 20 { - // TODO: underspecified, follow https://github.com/lightning/bolts/issues/791 - 0u16.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"); - } else if code & 0x1000 == 0x1000 { - // If we're trying to return an error that requires a `channel_update` but - // we're forwarding to a phantom or intercept "channel" (i.e. cannot - // generate an update), just use the generic "temporary_node_failure" - // instead. - code = 0x2000 | 2; - } - return_err!(err, code, &res.0[..]); - } - } } - - pending_forward_info } /// Gets the current [`channel_update`] for the given channel. This first checks if the channel is @@ -2984,7 +3032,7 @@ where session_priv: session_priv.clone(), first_hop_htlc_msat: htlc_msat, payment_id, - }, onion_packet, &self.logger); + }, onion_packet, None, &self.logger); match break_chan_entry!(self, send_res, chan) { Some(monitor_update) => { let update_id = monitor_update.update_id; @@ -3451,13 +3499,16 @@ where /// [`ChannelManager::fail_intercepted_htlc`] MUST be called in response to the event. /// /// Note that LDK does not enforce fee requirements in `amt_to_forward_msat`, and will not stop - /// you from forwarding more than you received. + /// you from forwarding more than you received. See + /// [`HTLCIntercepted::expected_outbound_amount_msat`] for more on forwarding a different amount + /// than expected. /// /// Errors if the event was not handled in time, in which case the HTLC was automatically failed /// backwards. /// /// [`UserConfig::accept_intercept_htlcs`]: crate::util::config::UserConfig::accept_intercept_htlcs /// [`HTLCIntercepted`]: events::Event::HTLCIntercepted + /// [`HTLCIntercepted::expected_outbound_amount_msat`]: events::Event::HTLCIntercepted::expected_outbound_amount_msat // TODO: when we move to deciding the best outbound channel at forward time, only take // `next_node_id` and not `next_hop_channel_id` pub fn forward_intercepted_htlc(&self, intercept_id: InterceptId, next_hop_channel_id: &[u8; 32], next_node_id: PublicKey, amt_to_forward_msat: u64) -> Result<(), APIError> { @@ -3496,7 +3547,10 @@ where }, _ => unreachable!() // Only `PendingHTLCRouting::Forward`s are intercepted }; + let skimmed_fee_msat = + payment.forward_info.outgoing_amt_msat.saturating_sub(amt_to_forward_msat); let pending_htlc_info = PendingHTLCInfo { + skimmed_fee_msat: if skimmed_fee_msat == 0 { None } else { Some(skimmed_fee_msat) }, outgoing_amt_msat: amt_to_forward_msat, routing, ..payment.forward_info }; @@ -3566,7 +3620,7 @@ where prev_short_channel_id, prev_htlc_id, prev_funding_outpoint, prev_user_channel_id, forward_info: PendingHTLCInfo { routing, incoming_shared_secret, payment_hash, outgoing_amt_msat, - outgoing_cltv_value, incoming_amt_msat: _ + outgoing_cltv_value, .. } }) => { macro_rules! failure_handler { @@ -3628,7 +3682,10 @@ where }; match next_hop { onion_utils::Hop::Receive(hop_data) => { - match self.construct_recv_pending_htlc_info(hop_data, incoming_shared_secret, payment_hash, outgoing_amt_msat, outgoing_cltv_value, Some(phantom_shared_secret)) { + match self.construct_recv_pending_htlc_info(hop_data, + incoming_shared_secret, payment_hash, outgoing_amt_msat, + 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)) } @@ -3679,7 +3736,7 @@ where prev_short_channel_id, prev_htlc_id, prev_funding_outpoint, prev_user_channel_id: _, forward_info: PendingHTLCInfo { incoming_shared_secret, payment_hash, outgoing_amt_msat, outgoing_cltv_value, - routing: PendingHTLCRouting::Forward { onion_packet, .. }, incoming_amt_msat: _, + routing: PendingHTLCRouting::Forward { onion_packet, .. }, skimmed_fee_msat, .. }, }) => { log_trace!(self.logger, "Adding HTLC from short id {} with payment_hash {} to channel with short id {} after delay", prev_short_channel_id, log_bytes!(payment_hash.0), short_chan_id); @@ -3693,7 +3750,7 @@ where }); if let Err(e) = chan.get_mut().queue_add_htlc(outgoing_amt_msat, payment_hash, outgoing_cltv_value, htlc_source.clone(), - onion_packet, &self.logger) + onion_packet, skimmed_fee_msat, &self.logger) { if let ChannelError::Ignore(msg) = e { log_trace!(self.logger, "Failed to forward HTLC with payment_hash {}: {}", log_bytes!(payment_hash.0), msg); @@ -3737,7 +3794,8 @@ where HTLCForwardInfo::AddHTLC(PendingAddHTLCInfo { prev_short_channel_id, prev_htlc_id, prev_funding_outpoint, prev_user_channel_id, forward_info: PendingHTLCInfo { - routing, incoming_shared_secret, payment_hash, incoming_amt_msat, outgoing_amt_msat, .. + routing, incoming_shared_secret, payment_hash, incoming_amt_msat, outgoing_amt_msat, + skimmed_fee_msat, .. } }) => { let (cltv_expiry, onion_payload, payment_data, phantom_shared_secret, mut onion_fields) = match routing { @@ -3778,6 +3836,7 @@ where total_msat: if let Some(data) = &payment_data { data.total_msat } else { outgoing_amt_msat }, cltv_expiry, onion_payload, + counterparty_skimmed_fee_msat: skimmed_fee_msat, }; let mut committed_to_claimable = false; @@ -3874,11 +3933,16 @@ where htlcs.push(claimable_htlc); let amount_msat = htlcs.iter().map(|htlc| htlc.value).sum(); htlcs.iter_mut().for_each(|htlc| htlc.total_value_received = Some(amount_msat)); + let counterparty_skimmed_fee_msat = htlcs.iter() + .map(|htlc| htlc.counterparty_skimmed_fee_msat.unwrap_or(0)).sum(); + debug_assert!(total_value.saturating_sub(amount_msat) <= + counterparty_skimmed_fee_msat); new_events.push_back((events::Event::PaymentClaimable { receiver_node_id: Some(receiver_node_id), payment_hash, purpose: $purpose, amount_msat, + counterparty_skimmed_fee_msat, via_channel_id: Some(prev_channel_id), via_user_channel_id: Some(prev_user_channel_id), claim_deadline: Some(earliest_expiry - HTLC_FAIL_BACK_BUFFER), @@ -5358,7 +5422,7 @@ where //encrypted with the same key. It's not immediately obvious how to usefully exploit that, //but we should prevent it anyway. - let pending_forward_info = self.decode_update_add_htlc_onion(msg); + let decoded_hop_res = self.decode_update_add_htlc_onion(msg); let per_peer_state = self.per_peer_state.read().unwrap(); let peer_state_mutex = per_peer_state.get(counterparty_node_id) .ok_or_else(|| { @@ -5370,6 +5434,12 @@ where match peer_state.channel_by_id.entry(msg.channel_id) { hash_map::Entry::Occupied(mut chan) => { + let pending_forward_info = match decoded_hop_res { + Ok((next_hop, shared_secret, next_packet_pk_opt)) => + self.construct_pending_htlc_status(msg, shared_secret, next_hop, + chan.get().context.config().accept_underpaying_htlcs, next_packet_pk_opt), + Err(e) => PendingHTLCStatus::Fail(e) + }; let create_pending_htlc_status = |chan: &Channel<::Signer>, pending_forward_info: PendingHTLCStatus, error_code: u16| { // If the update_add is completely bogus, the call will Err and we will close, // but if we've sent a shutdown and they haven't acknowledged it yet, we just @@ -7409,6 +7479,7 @@ impl_writeable_tlv_based!(PendingHTLCInfo, { (6, outgoing_amt_msat, required), (8, outgoing_cltv_value, required), (9, incoming_amt_msat, option), + (10, skimmed_fee_msat, option), }); @@ -7507,6 +7578,7 @@ impl Writeable for ClaimableHTLC { (5, self.total_value_received, option), (6, self.cltv_expiry, required), (8, keysend_preimage, option), + (10, self.counterparty_skimmed_fee_msat, option), }); Ok(()) } @@ -7514,24 +7586,19 @@ impl Writeable for ClaimableHTLC { impl Readable for ClaimableHTLC { fn read(reader: &mut R) -> Result { - let mut prev_hop = crate::util::ser::RequiredWrapper(None); - let mut value = 0; - let mut sender_intended_value = None; - let mut payment_data: Option = None; - let mut cltv_expiry = 0; - let mut total_value_received = None; - let mut total_msat = None; - let mut keysend_preimage: Option = None; - read_tlv_fields!(reader, { + _init_and_read_tlv_fields!(reader, { (0, prev_hop, required), (1, total_msat, option), - (2, value, required), + (2, value_ser, required), (3, sender_intended_value, option), - (4, payment_data, option), + (4, payment_data_opt, option), (5, total_value_received, option), (6, cltv_expiry, required), - (8, keysend_preimage, option) + (8, keysend_preimage, option), + (10, counterparty_skimmed_fee_msat, option), }); + let payment_data: Option = payment_data_opt; + let value = value_ser.0.unwrap(); let onion_payload = match keysend_preimage { Some(p) => { if payment_data.is_some() { @@ -7560,7 +7627,8 @@ impl Readable for ClaimableHTLC { total_value_received, total_msat: total_msat.unwrap(), onion_payload, - cltv_expiry, + cltv_expiry: cltv_expiry.0.unwrap(), + counterparty_skimmed_fee_msat, }) } } @@ -9681,6 +9749,50 @@ mod tests { get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, last_random_pk); } + #[test] + fn reject_excessively_underpaying_htlcs() { + let chanmon_cfg = create_chanmon_cfgs(1); + let node_cfg = create_node_cfgs(1, &chanmon_cfg); + let node_chanmgr = create_node_chanmgrs(1, &node_cfg, &[None]); + 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, + 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, + }), + } + }; + // 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, .. }) = + 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)) + { + assert_eq!(err_code, 19); + } 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, + 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, + }), + } + }; + 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()); + } + #[cfg(anchors)] #[test] fn test_anchors_zero_fee_htlc_tx_fallback() { diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 6107642cbf2..dc5f2b41fe5 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -2124,7 +2124,7 @@ pub fn do_pass_along_path<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_p match &events_2[0] { Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref via_channel_id, ref via_user_channel_id, - claim_deadline, onion_fields, + claim_deadline, onion_fields, .. } => { assert_eq!(our_payment_hash, *payment_hash); assert_eq!(node.node.get_our_node_id(), receiver_node_id.unwrap()); @@ -2186,7 +2186,20 @@ pub fn send_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, route: Route (our_payment_preimage, our_payment_hash, our_payment_secret, payment_id) } -pub fn do_claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_paths: &[&[&Node<'a, 'b, 'c>]], skip_last: bool, our_payment_preimage: PaymentPreimage) -> u64 { +pub fn do_claim_payment_along_route<'a, 'b, 'c>( + origin_node: &Node<'a, 'b, 'c>, expected_paths: &[&[&Node<'a, 'b, 'c>]], skip_last: bool, + our_payment_preimage: PaymentPreimage +) -> u64 { + let extra_fees = vec![0; expected_paths.len()]; + do_claim_payment_along_route_with_extra_penultimate_hop_fees(origin_node, expected_paths, + &extra_fees[..], skip_last, our_payment_preimage) +} + +pub fn do_claim_payment_along_route_with_extra_penultimate_hop_fees<'a, 'b, 'c>( + origin_node: &Node<'a, 'b, 'c>, expected_paths: &[&[&Node<'a, 'b, 'c>]], expected_extra_fees: + &[u32], skip_last: bool, our_payment_preimage: PaymentPreimage +) -> u64 { + assert_eq!(expected_paths.len(), expected_extra_fees.len()); for path in expected_paths.iter() { assert_eq!(path.last().unwrap().node.get_our_node_id(), expected_paths[0].last().unwrap().node.get_our_node_id()); } @@ -2236,7 +2249,7 @@ pub fn do_claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, } } - for (expected_route, (path_msgs, next_hop)) in expected_paths.iter().zip(per_path_msgs.drain(..)) { + for (i, (expected_route, (path_msgs, next_hop))) in expected_paths.iter().zip(per_path_msgs.drain(..)).enumerate() { let mut next_msgs = Some(path_msgs); let mut expected_next_node = next_hop; @@ -2251,10 +2264,10 @@ pub fn do_claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, } } macro_rules! mid_update_fulfill_dance { - ($node: expr, $prev_node: expr, $next_node: expr, $new_msgs: expr) => { + ($idx: expr, $node: expr, $prev_node: expr, $next_node: expr, $new_msgs: expr) => { { $node.node.handle_update_fulfill_htlc(&$prev_node.node.get_our_node_id(), &next_msgs.as_ref().unwrap().0); - let fee = { + let mut fee = { let per_peer_state = $node.node.per_peer_state.read().unwrap(); let peer_state = per_peer_state.get(&$prev_node.node.get_our_node_id()) .unwrap().lock().unwrap(); @@ -2265,6 +2278,7 @@ pub fn do_claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, channel.context.config().forwarding_fee_base_msat } }; + if $idx == 1 { fee += expected_extra_fees[i]; } expect_payment_forwarded!($node, $next_node, $prev_node, Some(fee as u64), false, false); expected_total_fee_msat += fee as u64; check_added_monitors!($node, 1); @@ -2296,7 +2310,7 @@ pub fn do_claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, } else { next_node = expected_route[expected_route.len() - 1 - idx - 1]; } - mid_update_fulfill_dance!(node, prev_node, next_node, update_next_msgs); + mid_update_fulfill_dance!(idx, node, prev_node, next_node, update_next_msgs); } else { assert!(!update_next_msgs); assert!(node.node.get_and_clear_pending_msg_events().is_empty()); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index f03ecd14f66..63b3b2bb0a4 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -1393,6 +1393,7 @@ fn test_fee_spike_violation_fails_htlc() { payment_hash: payment_hash, cltv_expiry: htlc_cltv, onion_routing_packet: onion_packet, + skimmed_fee_msat: None, }; nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &msg); @@ -1582,6 +1583,7 @@ fn test_chan_reserve_violation_inbound_htlc_outbound_channel() { payment_hash: payment_hash, cltv_expiry: htlc_cltv, onion_routing_packet: onion_packet, + skimmed_fee_msat: None, }; nodes[0].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &msg); @@ -1758,6 +1760,7 @@ fn test_chan_reserve_violation_inbound_htlc_inbound_chan() { payment_hash: our_payment_hash_1, cltv_expiry: htlc_cltv, onion_routing_packet: onion_packet, + skimmed_fee_msat: None, }; nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &msg); @@ -3410,6 +3413,7 @@ fn fail_backward_pending_htlc_upon_channel_failure() { payment_hash, cltv_expiry, onion_routing_packet, + skimmed_fee_msat: None, }; nodes[0].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &update_add_htlc); } @@ -6259,6 +6263,7 @@ fn test_update_add_htlc_bolt2_receiver_check_max_htlc_limit() { payment_hash: our_payment_hash, cltv_expiry: htlc_cltv, onion_routing_packet: onion_packet.clone(), + skimmed_fee_msat: None, }; for i in 0..50 { diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 3dd4a6da70c..672c6ae6e96 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -610,6 +610,11 @@ pub struct UpdateAddHTLC { pub payment_hash: PaymentHash, /// The expiry height of the HTLC pub cltv_expiry: u32, + /// The extra fee skimmed by the sender of this message. See + /// [`ChannelConfig::accept_underpaying_htlcs`]. + /// + /// [`ChannelConfig::accept_underpaying_htlcs`]: crate::util::config::ChannelConfig::accept_underpaying_htlcs + pub skimmed_fee_msat: Option, pub(crate) onion_routing_packet: OnionPacket, } @@ -1903,8 +1908,10 @@ impl_writeable_msg!(UpdateAddHTLC, { amount_msat, payment_hash, cltv_expiry, - onion_routing_packet -}, {}); + onion_routing_packet, +}, { + (65537, skimmed_fee_msat, option) +}); impl Readable for OnionMessage { fn read(r: &mut R) -> Result { @@ -3330,7 +3337,8 @@ mod tests { amount_msat: 3608586615801332854, payment_hash: PaymentHash([1; 32]), cltv_expiry: 821716, - onion_routing_packet + onion_routing_packet, + skimmed_fee_msat: None, }; let encoded_value = update_add_htlc.encode(); let target_value = hex::decode("020202020202020202020202020202020202020202020202020202020202020200083a840000034d32144668701144760101010101010101010101010101010101010101010101010101010101010101000c89d4ff031b84c5567b126440995d3ed5aaba0565d71e1834604819ff9c17f5e9d5dd078f010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010202020202020202020202020202020202020202020202020202020202020202").unwrap(); diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 90c7ad7625c..fa607f68098 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -1736,6 +1736,133 @@ fn do_test_intercepted_payment(test: InterceptTest) { } } +#[test] +fn accept_underpaying_htlcs_config() { + do_accept_underpaying_htlcs_config(1); + do_accept_underpaying_htlcs_config(2); + do_accept_underpaying_htlcs_config(3); +} + +fn do_accept_underpaying_htlcs_config(num_mpp_parts: usize) { + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + let mut intercept_forwards_config = test_default_channel_config(); + intercept_forwards_config.accept_intercept_htlcs = true; + let mut underpay_config = test_default_channel_config(); + underpay_config.channel_config.accept_underpaying_htlcs = true; + let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, Some(intercept_forwards_config), Some(underpay_config)]); + let nodes = create_network(3, &node_cfgs, &node_chanmgrs); + + let mut chan_ids = Vec::new(); + for _ in 0..num_mpp_parts { + let _ = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000, 0); + let channel_id = create_unannounced_chan_between_nodes_with_value(&nodes, 1, 2, 2_000_000, 0).0.channel_id; + chan_ids.push(channel_id); + } + + // Send the initial payment. + let amt_msat = 900_000; + let skimmed_fee_msat = 20; + let mut route_hints = Vec::new(); + for _ in 0..num_mpp_parts { + route_hints.push(RouteHint(vec![RouteHintHop { + src_node_id: nodes[1].node.get_our_node_id(), + short_channel_id: nodes[1].node.get_intercept_scid(), + fees: RoutingFees { + base_msat: 1000, + proportional_millionths: 0, + }, + cltv_expiry_delta: MIN_CLTV_EXPIRY_DELTA, + htlc_minimum_msat: None, + htlc_maximum_msat: Some(amt_msat / num_mpp_parts as u64 + 5), + }])); + } + let payment_params = PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), TEST_FINAL_CLTV) + .with_route_hints(route_hints).unwrap() + .with_bolt11_features(nodes[2].node.invoice_features()).unwrap(); + let route_params = RouteParameters { + payment_params, + final_value_msat: amt_msat, + }; + let (payment_hash, payment_secret) = nodes[2].node.create_inbound_payment(Some(amt_msat), 60 * 60, None).unwrap(); + nodes[0].node.send_payment(payment_hash, RecipientOnionFields::secret_only(payment_secret), + PaymentId(payment_hash.0), route_params, Retry::Attempts(0)).unwrap(); + check_added_monitors!(nodes[0], num_mpp_parts); // one monitor per path + let mut events: Vec = nodes[0].node.get_and_clear_pending_msg_events().into_iter().map(|e| SendEvent::from_event(e)).collect(); + assert_eq!(events.len(), num_mpp_parts); + + // Forward the intercepted payments. + for (idx, ev) in events.into_iter().enumerate() { + nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &ev.msgs[0]); + do_commitment_signed_dance(&nodes[1], &nodes[0], &ev.commitment_msg, false, true); + + let events = nodes[1].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 1); + let (intercept_id, expected_outbound_amt_msat) = match events[0] { + crate::events::Event::HTLCIntercepted { + intercept_id, expected_outbound_amount_msat, payment_hash: pmt_hash, .. + } => { + assert_eq!(pmt_hash, payment_hash); + (intercept_id, expected_outbound_amount_msat) + }, + _ => panic!() + }; + nodes[1].node.forward_intercepted_htlc(intercept_id, &chan_ids[idx], + nodes[2].node.get_our_node_id(), expected_outbound_amt_msat - skimmed_fee_msat).unwrap(); + expect_pending_htlcs_forwardable!(nodes[1]); + let payment_event = { + { + let mut added_monitors = nodes[1].chain_monitor.added_monitors.lock().unwrap(); + assert_eq!(added_monitors.len(), 1); + added_monitors.clear(); + } + let mut events = nodes[1].node.get_and_clear_pending_msg_events(); + assert_eq!(events.len(), 1); + SendEvent::from_event(events.remove(0)) + }; + nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &payment_event.msgs[0]); + do_commitment_signed_dance(&nodes[2], &nodes[1], &payment_event.commitment_msg, false, true); + if idx == num_mpp_parts - 1 { + expect_pending_htlcs_forwardable!(nodes[2]); + } + } + + // Claim the payment and check that the skimmed fee is as expected. + let payment_preimage = nodes[2].node.get_payment_preimage(payment_hash, payment_secret).unwrap(); + let events = nodes[2].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 1); + match events[0] { + crate::events::Event::PaymentClaimable { + ref payment_hash, ref purpose, amount_msat, counterparty_skimmed_fee_msat, receiver_node_id, .. + } => { + assert_eq!(payment_hash, payment_hash); + assert_eq!(amt_msat - skimmed_fee_msat * num_mpp_parts as u64, amount_msat); + assert_eq!(skimmed_fee_msat * num_mpp_parts as u64, counterparty_skimmed_fee_msat); + assert_eq!(nodes[2].node.get_our_node_id(), receiver_node_id.unwrap()); + match purpose { + crate::events::PaymentPurpose::InvoicePayment { payment_preimage: ev_payment_preimage, + payment_secret: ev_payment_secret, .. } => + { + assert_eq!(payment_preimage, ev_payment_preimage.unwrap()); + assert_eq!(payment_secret, *ev_payment_secret); + }, + _ => panic!(), + } + }, + _ => panic!("Unexpected event"), + } + let mut expected_paths_vecs = Vec::new(); + let mut expected_paths = Vec::new(); + for _ in 0..num_mpp_parts { expected_paths_vecs.push(vec!(&nodes[1], &nodes[2])); } + for i in 0..num_mpp_parts { expected_paths.push(&expected_paths_vecs[i][..]); } + let total_fee_msat = do_claim_payment_along_route_with_extra_penultimate_hop_fees( + &nodes[0], &expected_paths[..], &vec![skimmed_fee_msat as u32; num_mpp_parts][..], false, + payment_preimage); + // The sender doesn't know that the penultimate hop took an extra fee. + expect_payment_sent(&nodes[0], payment_preimage, + Some(Some(total_fee_msat - skimmed_fee_msat * num_mpp_parts as u64)), true); +} + #[derive(PartialEq)] enum AutoRetry { Success, diff --git a/lightning/src/util/config.rs b/lightning/src/util/config.rs index 8f1f77b32aa..1faf595ac8f 100644 --- a/lightning/src/util/config.rs +++ b/lightning/src/util/config.rs @@ -397,6 +397,38 @@ pub struct ChannelConfig { /// [`Normal`]: crate::chain::chaininterface::ConfirmationTarget::Normal /// [`Background`]: crate::chain::chaininterface::ConfirmationTarget::Background pub force_close_avoidance_max_fee_satoshis: u64, + /// If set, allows this channel's counterparty to skim an additional fee off this node's inbound + /// HTLCs. Useful for liquidity providers to offload on-chain channel costs to end users. + /// + /// Usage: + /// - The payee will set this option and set its invoice route hints to use [intercept scids] + /// generated by this channel's counterparty. + /// - The counterparty will get an [`HTLCIntercepted`] event upon payment forward, and call + /// [`forward_intercepted_htlc`] with less than the amount provided in + /// [`HTLCIntercepted::expected_outbound_amount_msat`]. The difference between the expected and + /// actual forward amounts is their fee. + // TODO: link to LSP JIT channel invoice generation spec when it's merged + /// + /// # Note + /// It's important for payee wallet software to verify that [`PaymentClaimable::amount_msat`] is + /// as-expected if this feature is activated, otherwise they may lose money! + /// [`PaymentClaimable::counterparty_skimmed_fee_msat`] provides the fee taken by the + /// counterparty. + /// + /// # Note + /// Switching this config flag on may break compatibility with versions of LDK prior to 0.0.116. + /// Unsetting this flag between restarts may lead to payment receive failures. + /// + /// Default value: false. + /// + /// [intercept scids]: crate::ln::channelmanager::ChannelManager::get_intercept_scid + /// [`forward_intercepted_htlc`]: crate::ln::channelmanager::ChannelManager::forward_intercepted_htlc + /// [`HTLCIntercepted`]: crate::events::Event::HTLCIntercepted + /// [`HTLCIntercepted::expected_outbound_amount_msat`]: crate::events::Event::HTLCIntercepted::expected_outbound_amount_msat + /// [`PaymentClaimable::amount_msat`]: crate::events::Event::PaymentClaimable::amount_msat + /// [`PaymentClaimable::counterparty_skimmed_fee_msat`]: crate::events::Event::PaymentClaimable::counterparty_skimmed_fee_msat + // TODO: link to bLIP when it's merged + pub accept_underpaying_htlcs: bool, } impl ChannelConfig { @@ -429,12 +461,14 @@ impl Default for ChannelConfig { cltv_expiry_delta: 6 * 12, // 6 blocks/hour * 12 hours max_dust_htlc_exposure_msat: 5_000_000, force_close_avoidance_max_fee_satoshis: 1000, + accept_underpaying_htlcs: false, } } } impl_writeable_tlv_based!(ChannelConfig, { (0, forwarding_fee_proportional_millionths, required), + (1, accept_underpaying_htlcs, (default_value, false)), (2, forwarding_fee_base_msat, required), (4, cltv_expiry_delta, required), (6, max_dust_htlc_exposure_msat, required), @@ -543,6 +577,7 @@ impl crate::util::ser::Readable for LegacyChannelConfig { cltv_expiry_delta, force_close_avoidance_max_fee_satoshis, forwarding_fee_base_msat, + accept_underpaying_htlcs: false, }, announced_channel, commit_upfront_shutdown_pubkey, diff --git a/pending_changelog/forward-underpaying-htlc.txt b/pending_changelog/forward-underpaying-htlc.txt new file mode 100644 index 00000000000..5b6c2237753 --- /dev/null +++ b/pending_changelog/forward-underpaying-htlc.txt @@ -0,0 +1,6 @@ +## Backwards Compat + +* Forwarding less than the expected amount in `ChannelManager::forward_intercepted_htlc` may break + compatibility with versions of LDK prior to 0.0.116 +* Setting `ChannelConfig::accept_underpaying_htlcs` may break compatibility with versions of LDK + prior to 0.0.116, and unsetting the feature between restarts may lead to payment failures.