From 134d60a3e800b062f85270973c1e0702eeab077c Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 7 Mar 2024 10:37:08 +0100 Subject: [PATCH 1/2] Expose `{prev,next}_user_channel_id` fields in `PaymentForwarded` This is useful for users that track channels by `user_channel_id`. For example, in `lightning-liquidity` we currently keep a full `HashMap` around *just* to be able to associate `PaymentForwarded` events with the channels otherwise tracked by `user_channel_id`. --- lightning/src/events/mod.rs | 42 +++++++++++++++++------ lightning/src/ln/channelmanager.rs | 21 ++++++++---- lightning/src/ln/functional_test_utils.rs | 27 ++++++++++++--- 3 files changed, 68 insertions(+), 22 deletions(-) diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs index 485a23e0292..fcd37914e3d 100644 --- a/lightning/src/events/mod.rs +++ b/lightning/src/events/mod.rs @@ -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, - /// 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, + /// 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, + /// 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, /// 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 @@ -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, { @@ -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, @@ -1427,12 +1442,14 @@ 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), @@ -1440,10 +1457,13 @@ impl MaybeReadable 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), }); 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() diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index abfca35f972..78fa47e9272 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -5733,7 +5733,7 @@ where fn claim_funds_internal(&self, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option, skimmed_fee_msat: Option, from_onchain: bool, startup_replay: bool, next_channel_counterparty_node_id: Option, - next_channel_outpoint: OutPoint, next_channel_id: ChannelId, + next_channel_outpoint: OutPoint, next_channel_id: ChannelId, next_user_channel_id: Option, ) { match source { HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => { @@ -5752,6 +5752,7 @@ 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; @@ -5838,12 +5839,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, }) @@ -6817,6 +6820,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) @@ -6846,6 +6850,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( @@ -6857,7 +6862,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(()) @@ -7359,7 +7364,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 }; @@ -11319,7 +11324,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 diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index adf2768b415..0c8909b5685 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -2230,8 +2230,8 @@ pub fn expect_payment_forwarded>( ) { 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); @@ -2240,12 +2240,31 @@ pub fn expect_payment_forwarded>( 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); }, From ab4b87209886569ba94df10ee27dccf9c0562d05 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 7 Mar 2024 10:39:37 +0100 Subject: [PATCH 2/2] Remove redundant `claiming_channel_id` variable .. as it's the same as `prev_channel_id` defined a few lines above. --- lightning/src/ln/channelmanager.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 78fa47e9272..a95681da9f7 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -5756,8 +5756,6 @@ where 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 = @@ -5815,7 +5813,7 @@ where BackgroundEvent::MonitorUpdatesComplete { channel_id, .. } => - *channel_id == claiming_channel_id, + *channel_id == prev_channel_id, } }), "{:?}", *background_events); }