Skip to content
This repository has been archived by the owner on Aug 7, 2024. It is now read-only.

Commit

Permalink
Merge pull request lightningdevkit#2924 from tnull/2024-03-add-user-c…
Browse files Browse the repository at this point in the history 
…hannel-id-to-payment-forwarded

Expose `{prev,next}_user_channel_id` fields in `PaymentForwarded`
  • Loading branch information
@valentinewallace
valentinewallace authored Mar 20, 2024
2 parents e4b6a50 + ab4b872 commit a36b529
Show file tree
Hide file tree
Showing 3 changed files with 69 additions and 25 deletions.
42 changes: 31 additions & 11 deletions lightning/src/events/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -797,12 +797,24 @@ pub enum Event {
/// This event is generated when a payment has been successfully forwarded through us and a
/// forwarding fee earned.
PaymentForwarded {
/// The incoming channel between the previous node and us. This is only `None` for events
/// generated or serialized by versions prior to 0.0.107.
/// The channel id of the incoming channel between the previous node and us.
///
/// This is only `None` for events generated or serialized by versions prior to 0.0.107.
prev_channel_id: Option<ChannelId>,
/// The outgoing channel between the next node and us. This is only `None` for events
/// generated or serialized by versions prior to 0.0.107.
/// The channel id of the outgoing channel between the next node and us.
///
/// This is only `None` for events generated or serialized by versions prior to 0.0.107.
next_channel_id: Option<ChannelId>,
/// The `user_channel_id` of the incoming channel between the previous node and us.
///
/// This is only `None` for events generated or serialized by versions prior to 0.0.122.
prev_user_channel_id: Option<u128>,
/// The `user_channel_id` of the outgoing channel between the next node and us.
///
/// This will be `None` if the payment was settled via an on-chain transaction. See the
/// caveat described for the `total_fee_earned_msat` field. Moreover it will be `None` for
/// events generated or serialized by versions prior to 0.0.122.
next_user_channel_id: Option<u128>,
/// The total fee, in milli-satoshis, which was earned as a result of the payment.
///
/// Note that if we force-closed the channel over which we forwarded an HTLC while the HTLC
Expand Down Expand Up @@ -1121,8 +1133,9 @@ impl Writeable for Event {
});
}
&Event::PaymentForwarded {
total_fee_earned_msat, prev_channel_id, claim_from_onchain_tx,
next_channel_id, outbound_amount_forwarded_msat, skimmed_fee_msat,
prev_channel_id, next_channel_id, prev_user_channel_id, next_user_channel_id,
total_fee_earned_msat, skimmed_fee_msat, claim_from_onchain_tx,
outbound_amount_forwarded_msat,
} => {
7u8.write(writer)?;
write_tlv_fields!(writer, {
Expand All @@ -1132,6 +1145,8 @@ impl Writeable for Event {
(3, next_channel_id, option),
(5, outbound_amount_forwarded_msat, option),
(7, skimmed_fee_msat, option),
(9, prev_user_channel_id, option),
(11, next_user_channel_id, option),
});
},
&Event::ChannelClosed { ref channel_id, ref user_channel_id, ref reason,
Expand Down Expand Up @@ -1427,23 +1442,28 @@ impl MaybeReadable for Event {
},
7u8 => {
let f = || {
let mut total_fee_earned_msat = None;
let mut prev_channel_id = None;
let mut claim_from_onchain_tx = false;
let mut next_channel_id = None;
let mut outbound_amount_forwarded_msat = None;
let mut prev_user_channel_id = None;
let mut next_user_channel_id = None;
let mut total_fee_earned_msat = None;
let mut skimmed_fee_msat = None;
let mut claim_from_onchain_tx = false;
let mut outbound_amount_forwarded_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),
(9, prev_user_channel_id, option),
(11, next_user_channel_id, option),
});
Ok(Some(Event::PaymentForwarded {
total_fee_earned_msat, prev_channel_id, claim_from_onchain_tx, next_channel_id,
outbound_amount_forwarded_msat, skimmed_fee_msat,
prev_channel_id, next_channel_id, prev_user_channel_id,
next_user_channel_id, total_fee_earned_msat, skimmed_fee_msat,
claim_from_onchain_tx, outbound_amount_forwarded_msat,
}))
};
f()
Expand Down
25 changes: 15 additions & 10 deletions lightning/src/ln/channelmanager.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5740,7 +5740,7 @@ where
fn claim_funds_internal(&self, source: HTLCSource, payment_preimage: PaymentPreimage,
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,
next_channel_outpoint: OutPoint, next_channel_id: ChannelId, next_user_channel_id: Option<u128>,
) {
match source {
HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
Expand All @@ -5759,11 +5759,10 @@ where
},
HTLCSource::PreviousHopData(hop_data) => {
let prev_channel_id = hop_data.channel_id;
let prev_user_channel_id = hop_data.user_channel_id;
let completed_blocker = RAAMonitorUpdateBlockingAction::from_prev_hop_data(&hop_data);
#[cfg(debug_assertions)]
let claiming_chan_funding_outpoint = hop_data.outpoint;
#[cfg(debug_assertions)]
let claiming_channel_id = hop_data.channel_id;
let res = self.claim_funds_from_hop(hop_data, payment_preimage,
|htlc_claim_value_msat, definitely_duplicate| {
let chan_to_release =
Expand Down Expand Up @@ -5821,7 +5820,7 @@ where
BackgroundEvent::MonitorUpdatesComplete {
channel_id, ..
} =>
*channel_id == claiming_channel_id,
*channel_id == prev_channel_id,
}
}), "{:?}", *background_events);
}
Expand All @@ -5845,12 +5844,14 @@ where
"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,
prev_user_channel_id,
next_user_channel_id,
total_fee_earned_msat,
skimmed_fee_msat,
claim_from_onchain_tx: from_onchain,
outbound_amount_forwarded_msat: forwarded_htlc_value_msat,
},
downstream_counterparty_and_funding_outpoint: chan_to_release,
})
Expand Down Expand Up @@ -6824,6 +6825,7 @@ where

fn internal_update_fulfill_htlc(&self, counterparty_node_id: &PublicKey, msg: &msgs::UpdateFulfillHTLC) -> Result<(), MsgHandleErrInternal> {
let funding_txo;
let next_user_channel_id;
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)
Expand Down Expand Up @@ -6853,6 +6855,7 @@ where
// outbound HTLC is claimed. This is guaranteed to all complete before we
// process the RAA as messages are processed from single peers serially.
funding_txo = chan.context.get_funding_txo().expect("We won't accept a fulfill until funded");
next_user_channel_id = chan.context.get_user_id();
res
} else {
return try_chan_phase_entry!(self, Err(ChannelError::Close(
Expand All @@ -6864,7 +6867,7 @@ where
};
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
funding_txo, msg.channel_id, Some(next_user_channel_id),
);

Ok(())
Expand Down Expand Up @@ -7366,7 +7369,7 @@ where
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), None, true,
false, counterparty_node_id, funding_outpoint, channel_id);
false, counterparty_node_id, funding_outpoint, channel_id, None);
} 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 @@ -11386,7 +11389,9 @@ where
// 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), None,
downstream_closed, true, downstream_node_id, downstream_funding, downstream_channel_id);
downstream_closed, true, downstream_node_id, downstream_funding,
downstream_channel_id, None
);
}

//TODO: Broadcast channel update for closed channels, but only after we've made a
Expand Down
27 changes: 23 additions & 4 deletions lightning/src/ln/functional_test_utils.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2231,8 +2231,8 @@ pub fn expect_payment_forwarded<CM: AChannelManager, H: NodeHolder<CM=CM>>(
) -> Option<u64> {
match event {
Event::PaymentForwarded {
total_fee_earned_msat, prev_channel_id, claim_from_onchain_tx, next_channel_id,
outbound_amount_forwarded_msat: _, skimmed_fee_msat
prev_channel_id, next_channel_id, prev_user_channel_id, next_user_channel_id,
total_fee_earned_msat, skimmed_fee_msat, claim_from_onchain_tx, ..
} => {
if allow_1_msat_fee_overpay {
// Aggregating fees for blinded paths may result in a rounding error, causing slight
Expand All @@ -2249,12 +2249,31 @@ pub fn expect_payment_forwarded<CM: AChannelManager, H: NodeHolder<CM=CM>>(
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()));
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() &&
x.user_channel_id == prev_user_channel_id.unwrap()
));
}
// We check for force closures since a force closed channel is removed from the
// node's channel list
if !downstream_force_closed {
assert!(node.node().list_channels().iter().any(|x| x.counterparty.node_id == next_node.node().get_our_node_id() && x.channel_id == next_channel_id.unwrap()));
// As documented, `next_user_channel_id` will only be `Some` if we didn't settle via an
// onchain transaction, just as the `total_fee_earned_msat` field. Rather than
// introducing yet another variable, we use the latter's state as a flag to detect
// this and only check if it's `Some`.
if total_fee_earned_msat.is_none() {
assert!(node.node().list_channels().iter().any(|x|
x.counterparty.node_id == next_node.node().get_our_node_id() &&
x.channel_id == next_channel_id.unwrap()
));
} else {
assert!(node.node().list_channels().iter().any(|x|
x.counterparty.node_id == next_node.node().get_our_node_id() &&
x.channel_id == next_channel_id.unwrap() &&
x.user_channel_id == next_user_channel_id.unwrap()
));
}
}
assert_eq!(claim_from_onchain_tx, downstream_force_closed);
total_fee_earned_msat
Expand Down

0 comments on commit a36b529

Please sign in to comment.