Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow forwarding less than the amount in the onion #2319

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 29 additions & 4 deletions lightning/src/events/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<RecipientOnionFields>,
/// 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.
valentinewallace marked this conversation as resolved.
Show resolved Hide resolved
///
/// 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,
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)?;
Expand All @@ -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),
Expand All @@ -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 } => {
Expand Down Expand Up @@ -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::<u64>; // For compatibility with 0.0.103 and earlier
let mut via_channel_id = None;
Expand All @@ -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 {
Expand All @@ -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,
Expand Down
103 changes: 88 additions & 15 deletions lightning/src/ln/channel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ struct OutboundHTLCOutput {
payment_hash: PaymentHash,
state: OutboundHTLCState,
source: HTLCSource,
skimmed_fee_msat: Option<u64>,
}

/// See AwaitingRemoteRevoke ChannelState for more info
Expand All @@ -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<u64>,
},
ClaimHTLC {
payment_preimage: PaymentPreimage,
Expand Down Expand Up @@ -3052,8 +3055,13 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
// 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 {
Expand Down Expand Up @@ -3695,6 +3703,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
payment_hash: htlc.payment_hash,
cltv_expiry: htlc.cltv_expiry,
onion_routing_packet: (**onion_packet).clone(),
skimmed_fee_msat: htlc.skimmed_fee_msat,
});
}
}
Expand Down Expand Up @@ -5042,11 +5051,13 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
/// commitment update.
///
/// `Err`s will only be [`ChannelError::Ignore`].
pub fn queue_add_htlc<L: Deref>(&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<L: Deref>(
&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource,
onion_routing_packet: msgs::OnionPacket, skimmed_fee_msat: Option<u64>, 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 */ }
Expand All @@ -5071,9 +5082,11 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
/// on this [`Channel`] if `force_holding_cell` is false.
///
/// `Err`s will only be [`ChannelError::Ignore`].
fn send_htlc<L: Deref>(&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<Option<msgs::UpdateAddHTLC>, ChannelError> where L::Target: Logger {
fn send_htlc<L: Deref>(
&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<u64>, logger: &L
) -> Result<Option<msgs::UpdateAddHTLC>, 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()));
}
Expand Down Expand Up @@ -5125,6 +5138,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
cltv_expiry,
source,
onion_routing_packet,
skimmed_fee_msat,
});
return Ok(None);
}
Expand All @@ -5136,6 +5150,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
cltv_expiry,
state: OutboundHTLCState::LocalAnnounced(Box::new(onion_routing_packet.clone())),
source,
skimmed_fee_msat,
});

let res = msgs::UpdateAddHTLC {
Expand All @@ -5145,6 +5160,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
payment_hash,
cltv_expiry,
onion_routing_packet,
skimmed_fee_msat,
};
self.context.next_holder_htlc_id += 1;

Expand Down Expand Up @@ -5283,8 +5299,12 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
///
/// 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<L: Deref>(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, onion_routing_packet: msgs::OnionPacket, logger: &L) -> Result<Option<&ChannelMonitorUpdate>, 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<L: Deref>(
&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource,
onion_routing_packet: msgs::OnionPacket, skimmed_fee_msat: Option<u64>, logger: &L
) -> Result<Option<&ChannelMonitorUpdate>, 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(_) => {
Expand Down Expand Up @@ -6609,9 +6629,10 @@ impl<Signer: WriteableEcdsaChannelSigner> Writeable for Channel<Signer> {
}

let mut preimages: Vec<&Option<PaymentPreimage>> = vec![];
let mut pending_outbound_skimmed_fees: Vec<Option<u64>> = 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)?;
Expand Down Expand Up @@ -6647,18 +6668,37 @@ impl<Signer: WriteableEcdsaChannelSigner> Writeable for Channel<Signer> {
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<Option<u64>> = 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)?;
Expand Down Expand Up @@ -6825,6 +6865,8 @@ impl<Signer: WriteableEcdsaChannelSigner> Writeable for Channel<Signer> {
(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(())
Expand Down Expand Up @@ -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,
});
}

Expand All @@ -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)?,
Expand Down Expand Up @@ -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<Vec<Option<u64>>> = None;
let mut holding_cell_skimmed_fees_opt: Option<Vec<Option<u64>>> = None;

read_tlv_fields!(reader, {
(0, announcement_sigs, option),
(1, minimum_depth, option),
Expand All @@ -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),
TheBlueMatt marked this conversation as resolved.
Show resolved Hide resolved
(37, holding_cell_skimmed_fees_opt, optional_vec),
});

let (channel_keys_id, holder_signer) = if let Some(channel_keys_id) = channel_keys_id {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
Loading