Skip to content

Expose {prev,next}_user_channel_id fields in PaymentForwarded #2924

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 20, 2024
Merged

Expose {prev,next}_user_channel_id fields in PaymentForwarded #2924

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
134d60a
Expose `{prev,next}_user_channel_id` fields in `PaymentForwarded`
tnull Mar 7, 2024
ab4b872
Remove redundant `claiming_channel_id` variable
tnull Mar 7, 2024
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
42 changes: 31 additions & 11 deletions lightning/src/events/mod.rs
View file
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5733,7 +5733,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 @@ -5752,11 +5752,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 @@ -5814,7 +5813,7 @@ where
BackgroundEvent::MonitorUpdatesComplete {
channel_id, ..
} =>
*channel_id == claiming_channel_id,
*channel_id == prev_channel_id,
}
}), "{:?}", *background_events);
}
Expand All @@ -5838,12 +5837,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 @@ -6817,6 +6818,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 @@ -6846,6 +6848,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 @@ -6857,7 +6860,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),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, tests pass when I revert this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, good catch. That's due to the map_or in the test which makes it too forgiving. I'll see to revisit that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now pushed a fixup to the test: #2924 (comment)

It's not perfect, but I wanted to avoid cluttering the macro by introducing yet another expected_X variable just for this slim use case. Let me know if you'd prefer it over the current approach though.

);

Ok(())
Expand Down Expand Up @@ -7359,7 +7362,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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we consider including this field in MonitorEvents in the future so it can be available here?

Copy link
Contributor Author

@tnull tnull Mar 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought about that too, but that might require starting to track it in the ChannelMonitors first, which might be a bit out-of-scope?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also currently don't have any concept of how to handle user channel ids after a channel closes - is recycling allowed at that point?

} 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 @@ -11319,7 +11322,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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2230,8 +2230,8 @@ pub fn expect_payment_forwarded<CM: AChannelManager, H: NodeHolder<CM=CM>>(
) {
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, ..
} => {
assert_eq!(total_fee_earned_msat, expected_fee);

Expand All @@ -2240,12 +2240,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);
},
Expand Down