Skip to content

Commit

Permalink
Expose skimmed_fee_msat in PaymentForwarded
Browse files Browse the repository at this point in the history
We generally allow routing nodes to forward less than the expected HTLC
amount, if the receiver knowingly accepts this and claims the
underpaying HTLC (see `ChannelConfig::accept_underpaying_htlcs`). This
use case is in particular useful for the LSPS2/JIT channel setting where
the intial underpaying HTLC pays for the channel open.

While we previously exposed the withheld amount as
`PaymentClaimable::counterparty_skimmed_fee_msat` on the receiver side,
we did not individually provide it on the forwarding node's side.
Here, we therefore expose this additionally withheld amount via
`PaymentForwarded::skimmed_fee_msat`.
  • Loading branch information
tnull committed Jan 31, 2024
1 parent 8e47266 commit c5bb00a
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 24 deletions.
19 changes: 17 additions & 2 deletions lightning/src/events/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -797,6 +797,18 @@ pub enum Event {
/// `PaymentForwarded` events are generated for the same payment iff `total_fee_earned_msat` is
/// `None`.
total_fee_earned_msat: Option<u64>,
/// The share of the total fee, in milli-satoshis, which was withheld in addition to the
/// forwarding fee.
///
/// This will only be `Some` if we forwarded an intercepted HTLC with less than the
/// expected amount. This means our counterparty accepted to receive less than the invoice
/// amount, e.g., by claiming the payment featuring a corresponding
/// [`PaymentClaimable::counterparty_skimmed_fee_msat`]
///
/// The caveat described above the `total_fee_earned_msat` field applies here as well.
///
/// [`PaymentClaimable::counterparty_skimmed_fee_msat`]: Self::PaymentClaimable::counterparty_skimmed_fee_msat
skimmed_fee_msat: Option<u64>,
/// If this is `true`, the forwarded HTLC was claimed by our counterparty via an on-chain
/// transaction.
claim_from_onchain_tx: bool,
Expand Down Expand Up @@ -1084,7 +1096,7 @@ impl Writeable for Event {
}
&Event::PaymentForwarded {
total_fee_earned_msat, prev_channel_id, claim_from_onchain_tx,
next_channel_id, outbound_amount_forwarded_msat
next_channel_id, outbound_amount_forwarded_msat, skimmed_fee_msat,
} => {
7u8.write(writer)?;
write_tlv_fields!(writer, {
Expand All @@ -1093,6 +1105,7 @@ impl Writeable for Event {
(2, claim_from_onchain_tx, required),
(3, next_channel_id, option),
(5, outbound_amount_forwarded_msat, option),
(7, skimmed_fee_msat, option),
});
},
&Event::ChannelClosed { ref channel_id, ref user_channel_id, ref reason,
Expand Down Expand Up @@ -1389,16 +1402,18 @@ impl MaybeReadable for Event {
let mut claim_from_onchain_tx = false;
let mut next_channel_id = None;
let mut outbound_amount_forwarded_msat = None;
let mut skimmed_fee_msat = None;
read_tlv_fields!(reader, {
(0, total_fee_earned_msat, option),
(1, prev_channel_id, option),
(2, claim_from_onchain_tx, required),
(3, next_channel_id, option),
(5, outbound_amount_forwarded_msat, option),
(7, skimmed_fee_msat, option),
});
Ok(Some(Event::PaymentForwarded {
total_fee_earned_msat, prev_channel_id, claim_from_onchain_tx, next_channel_id,
outbound_amount_forwarded_msat
outbound_amount_forwarded_msat, skimmed_fee_msat,
}))
};
f()
Expand Down
6 changes: 3 additions & 3 deletions lightning/src/ln/chanmon_update_fail_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@
//! here. See also the chanmon_fail_consistency fuzz test.

use bitcoin::blockdata::constants::genesis_block;
use bitcoin::hash_types::BlockHash;
use bitcoin::network::constants::Network;
use bitcoin::hash_types::BlockHash; use bitcoin::network::constants::Network;
use crate::chain::channelmonitor::{ANTI_REORG_DELAY, ChannelMonitor};
use crate::chain::transaction::OutPoint;
use crate::chain::{ChannelMonitorUpdateStatus, Listen, Watch};
Expand Down Expand Up @@ -3404,7 +3403,8 @@ fn do_test_reload_mon_update_completion_actions(close_during_reload: bool) {
let bc_update_id = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id_bc).unwrap().2;
let mut events = nodes[1].node.get_and_clear_pending_events();
assert_eq!(events.len(), if close_during_reload { 2 } else { 1 });
expect_payment_forwarded(events.pop().unwrap(), &nodes[1], &nodes[0], &nodes[2], Some(1000), close_during_reload, false);
expect_payment_forwarded(events.pop().unwrap(), &nodes[1], &nodes[0], &nodes[2], Some(1000),
None, close_during_reload, false);
if close_during_reload {
match events[0] {
Event::ChannelClosed { .. } => {},
Expand Down
4 changes: 2 additions & 2 deletions lightning/src/ln/channel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3327,15 +3327,15 @@ impl<SP: Deref> Channel<SP> where
Err(ChannelError::Close("Remote tried to fulfill/fail an HTLC we couldn't find".to_owned()))
}

pub fn update_fulfill_htlc(&mut self, msg: &msgs::UpdateFulfillHTLC) -> Result<(HTLCSource, u64), ChannelError> {
pub fn update_fulfill_htlc(&mut self, msg: &msgs::UpdateFulfillHTLC) -> Result<(HTLCSource, u64, Option<u64>), ChannelError> {
if !matches!(self.context.channel_state, ChannelState::ChannelReady(_)) {
return Err(ChannelError::Close("Got fulfill HTLC message when channel was not in an operational state".to_owned()));
}
if self.context.channel_state.is_peer_disconnected() {
return Err(ChannelError::Close("Peer sent update_fulfill_htlc when we needed a channel_reestablish".to_owned()));
}

self.mark_outbound_htlc_removed(msg.htlc_id, Some(msg.payment_preimage), None).map(|htlc| (htlc.source.clone(), htlc.amount_msat))
self.mark_outbound_htlc_removed(msg.htlc_id, Some(msg.payment_preimage), None).map(|htlc| (htlc.source.clone(), htlc.amount_msat, htlc.skimmed_fee_msat))
}

pub fn update_fail_htlc(&mut self, msg: &msgs::UpdateFailHTLC, fail_reason: HTLCFailReason) -> Result<(), ChannelError> {
Expand Down
24 changes: 16 additions & 8 deletions lightning/src/ln/channelmanager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5690,9 +5690,9 @@ where
}

fn claim_funds_internal(&self, source: HTLCSource, payment_preimage: PaymentPreimage,
forwarded_htlc_value_msat: Option<u64>, from_onchain: bool, startup_replay: bool,
next_channel_counterparty_node_id: Option<PublicKey>, next_channel_outpoint: OutPoint,
next_channel_id: ChannelId,
forwarded_htlc_value_msat: Option<u64>, skimmed_fee_msat: Option<u64>, from_onchain: bool,
startup_replay: bool, next_channel_counterparty_node_id: Option<PublicKey>,
next_channel_outpoint: OutPoint, next_channel_id: ChannelId,
) {
match source {
HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
Expand Down Expand Up @@ -5793,13 +5793,16 @@ where
Some(claimed_htlc_value - forwarded_htlc_value)
} else { None }
} else { None };
debug_assert!(skimmed_fee_msat <= total_fee_earned_msat,
"skimmed_fee_msat must always be included in total_fee_earned_msat");
Some(MonitorUpdateCompletionAction::EmitEventAndFreeOtherChannel {
event: events::Event::PaymentForwarded {
total_fee_earned_msat,
claim_from_onchain_tx: from_onchain,
prev_channel_id: Some(prev_channel_id),
next_channel_id: Some(next_channel_id),
outbound_amount_forwarded_msat: forwarded_htlc_value_msat,
skimmed_fee_msat,
},
downstream_counterparty_and_funding_outpoint: chan_to_release,
})
Expand Down Expand Up @@ -6739,7 +6742,7 @@ where

fn internal_update_fulfill_htlc(&self, counterparty_node_id: &PublicKey, msg: &msgs::UpdateFulfillHTLC) -> Result<(), MsgHandleErrInternal> {
let funding_txo;
let (htlc_source, forwarded_htlc_value) = {
let (htlc_source, forwarded_htlc_value, skimmed_fee_msat) = {
let per_peer_state = self.per_peer_state.read().unwrap();
let peer_state_mutex = per_peer_state.get(counterparty_node_id)
.ok_or_else(|| {
Expand Down Expand Up @@ -6777,8 +6780,11 @@ where
hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", counterparty_node_id), msg.channel_id))
}
};
self.claim_funds_internal(htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value),
false, false, Some(*counterparty_node_id), funding_txo, msg.channel_id);
self.claim_funds_internal(htlc_source, msg.payment_preimage.clone(),
Some(forwarded_htlc_value), skimmed_fee_msat, false, false, Some(*counterparty_node_id),
funding_txo, msg.channel_id
);

Ok(())
}

Expand Down Expand Up @@ -7276,7 +7282,9 @@ where
let logger = WithContext::from(&self.logger, counterparty_node_id, Some(channel_id));
if let Some(preimage) = htlc_update.payment_preimage {
log_trace!(logger, "Claiming HTLC with preimage {} from our monitor", preimage);
self.claim_funds_internal(htlc_update.source, preimage, htlc_update.htlc_value_satoshis.map(|v| v * 1000), true, false, counterparty_node_id, funding_outpoint, channel_id);
self.claim_funds_internal(htlc_update.source, preimage,
htlc_update.htlc_value_satoshis.map(|v| v * 1000), None, true,
false, counterparty_node_id, funding_outpoint, channel_id);
} else {
log_trace!(logger, "Failing HTLC with hash {} from our monitor", &htlc_update.payment_hash);
let receiver = HTLCDestination::NextHopChannel { node_id: counterparty_node_id, channel_id };
Expand Down Expand Up @@ -11168,7 +11176,7 @@ where
// We use `downstream_closed` in place of `from_onchain` here just as a guess - we
// don't remember in the `ChannelMonitor` where we got a preimage from, but if the
// channel is closed we just assume that it probably came from an on-chain claim.
channel_manager.claim_funds_internal(source, preimage, Some(downstream_value),
channel_manager.claim_funds_internal(source, preimage, Some(downstream_value), None,
downstream_closed, true, downstream_node_id, downstream_funding, downstream_channel_id);
}

Expand Down
24 changes: 18 additions & 6 deletions lightning/src/ln/functional_test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2203,14 +2203,19 @@ macro_rules! expect_payment_path_successful {

pub fn expect_payment_forwarded<CM: AChannelManager, H: NodeHolder<CM=CM>>(
event: Event, node: &H, prev_node: &H, next_node: &H, expected_fee: Option<u64>,
upstream_force_closed: bool, downstream_force_closed: bool
expected_extra_fees_msat: Option<u64>, upstream_force_closed: bool,
downstream_force_closed: bool
) {
match event {
Event::PaymentForwarded {
total_fee_earned_msat, prev_channel_id, claim_from_onchain_tx, next_channel_id,
outbound_amount_forwarded_msat: _
outbound_amount_forwarded_msat: _, skimmed_fee_msat
} => {
assert_eq!(total_fee_earned_msat, expected_fee);

// Check that the (knowingly) withheld amount is always less or equal to the expected
// overpaid amount.
assert!(skimmed_fee_msat == expected_extra_fees_msat);
if !upstream_force_closed {
// Is the event prev_channel_id in one of the channels between the two nodes?
assert!(node.node().list_channels().iter().any(|x| x.counterparty.node_id == prev_node.node().get_our_node_id() && x.channel_id == prev_channel_id.unwrap()));
Expand All @@ -2226,13 +2231,14 @@ pub fn expect_payment_forwarded<CM: AChannelManager, H: NodeHolder<CM=CM>>(
}
}

#[macro_export]
macro_rules! expect_payment_forwarded {
($node: expr, $prev_node: expr, $next_node: expr, $expected_fee: expr, $upstream_force_closed: expr, $downstream_force_closed: expr) => {
let mut events = $node.node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
$crate::ln::functional_test_utils::expect_payment_forwarded(
events.pop().unwrap(), &$node, &$prev_node, &$next_node, $expected_fee,
$upstream_force_closed, $downstream_force_closed);
$crate::ln::functional_test_utils::expect_payment_forwarded( events.pop().unwrap(), &$node,
&$prev_node, &$next_node, $expected_fee, None, $upstream_force_closed,
$downstream_force_closed);
}
}

Expand Down Expand Up @@ -2696,11 +2702,17 @@ pub fn pass_claimed_payment_along_route<'a, 'b, 'c, 'd>(args: ClaimAlongRouteArg
channel.context().config().forwarding_fee_base_msat
}
};

let mut expected_extra_fee = None;
if $idx == 1 {
fee += expected_extra_fees[i];
fee += expected_min_htlc_overpay[i];
expected_extra_fee = if expected_extra_fees[i] > 0 { Some(expected_extra_fees[i] as u64) } else { None };
}
expect_payment_forwarded!(*$node, $next_node, $prev_node, Some(fee as u64), false, false);
let mut events = $node.node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
expect_payment_forwarded(events.pop().unwrap(), *$node, $next_node, $prev_node,
Some(fee as u64), expected_extra_fee, false, false);
expected_total_fee_msat += fee as u64;
check_added_monitors!($node, 1);
let new_next_msgs = if $new_msgs {
Expand Down
6 changes: 3 additions & 3 deletions lightning/src/ln/functional_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2890,7 +2890,7 @@ fn test_htlc_on_chain_success() {
let chan_id = Some(chan_1.2);
match forwarded_events[1] {
Event::PaymentForwarded { total_fee_earned_msat, prev_channel_id, claim_from_onchain_tx,
next_channel_id, outbound_amount_forwarded_msat
next_channel_id, outbound_amount_forwarded_msat, ..
} => {
assert_eq!(total_fee_earned_msat, Some(1000));
assert_eq!(prev_channel_id, chan_id);
Expand All @@ -2902,7 +2902,7 @@ fn test_htlc_on_chain_success() {
}
match forwarded_events[2] {
Event::PaymentForwarded { total_fee_earned_msat, prev_channel_id, claim_from_onchain_tx,
next_channel_id, outbound_amount_forwarded_msat
next_channel_id, outbound_amount_forwarded_msat, ..
} => {
assert_eq!(total_fee_earned_msat, Some(1000));
assert_eq!(prev_channel_id, chan_id);
Expand Down Expand Up @@ -4917,7 +4917,7 @@ fn test_onchain_to_onchain_claim() {
}
match events[1] {
Event::PaymentForwarded { total_fee_earned_msat, prev_channel_id, claim_from_onchain_tx,
next_channel_id, outbound_amount_forwarded_msat
next_channel_id, outbound_amount_forwarded_msat, ..
} => {
assert_eq!(total_fee_earned_msat, Some(1000));
assert_eq!(prev_channel_id, Some(chan_1.2));
Expand Down

0 comments on commit c5bb00a

Please sign in to comment.