Skip to content

expose from/to_channel_id in PaymentForwarded Event #1231

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

Closed
wants to merge 1 commit into from
Closed

expose from/to_channel_id in PaymentForwarded Event #1231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,12 +167,14 @@ pub struct HTLCUpdate {
pub(crate) payment_preimage: Option<PaymentPreimage>,
pub(crate) source: HTLCSource,
pub(crate) onchain_value_satoshis: Option<u64>,
pub(crate) forward_channel_id: [u8; 32],
}
impl_writeable_tlv_based!(HTLCUpdate, {
(0, payment_hash, required),
(1, onchain_value_satoshis, option),
(2, source, required),
(4, payment_preimage, option),
(6, forward_channel_id, required)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

little confused by the tlv encoding for internal messages? if a message isn't going over the wire then what's the deal with the identifier? i guess it functions the same? wasn't sure to mark this 5 or 6 and would love some generic thoughts on how these are used throughout the code base

Copy link
Contributor

Choose a reason for hiding this comment

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

This is needed when serializing the data for persistence. Similar to TLVs over the wire, even fields are required and odd are optional. So a new field should typically be optional and use the next odd number (3 in this case). That way, if you serialize an object it can still be read by a newer version of the code that added new fields. In the code, the field would be an Option unless it could be generated somehow (e.g., a payment hash from a preimage).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, we use it internally for most structs "because why not" - its a pretty decent way to do forward-compat and we already have the code for it, so we might as well use it. That said, we can't set this to even or required it - required means it must be there or the serialized object is invalid (ie it would imply all LDK objects from the current version(s) will be invalid to a new version with this patch), and even would imply that any old version would see this object as invalid (we try to ensure at least a version-or-two back can read objects written by the latest version of LDK, where possible).

});

/// If an HTLC expires within this many blocks, don't try to claim it in a shared transaction,
Expand Down Expand Up @@ -2512,6 +2514,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
payment_preimage: None,
source: source.clone(),
onchain_value_satoshis,
forward_channel_id: self.funding_info.0.to_channel_id(),
}));
if let Some(idx) = input_idx {
self.htlcs_resolved_on_chain.push(IrrevocablyResolvedHTLC { input_idx: idx, payment_preimage: None });
Expand Down Expand Up @@ -2851,6 +2854,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
payment_preimage: Some(payment_preimage),
payment_hash,
onchain_value_satoshis: Some(amount_msat / 1000),
forward_channel_id: self.funding_info.0.to_channel_id(),
}));
}
} else if offered_preimage_claim {
Expand All @@ -2872,6 +2876,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
payment_preimage: Some(payment_preimage),
payment_hash,
onchain_value_satoshis: Some(amount_msat / 1000),
forward_channel_id: self.funding_info.0.to_channel_id(),
}));
}
} else {
Expand Down
12 changes: 9 additions & 3 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3952,7 +3952,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
}
}

fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder<Signer>>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool) {
fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder<Signer>>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool, to_channel_id: [u8; 32]) {
match source {
HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
mem::drop(channel_state_lock);
Expand Down Expand Up @@ -4030,6 +4030,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
// update to. Instead, we simply document in `PaymentForwarded` that this
// can happen.
}

// Seems like if the prev hop force closed then we wont be able to get the channel_id from short_channel_id
let from_channel_id = channel_state_lock.short_to_id.get(&hop_data.short_channel_id).map(|chan_id| chan_id.clone());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

would be nice to have a way to always get the channel_id even if it has been closed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, we don't keep that (or, really, any) information after a channel closure. In general ChannelManager never stores any historical data at all - it provides users with events and users can figure it out if they want to. I think its fine to have a None if the channel has been closed, but you could also provide the SCID and let users do the mapping themselves if they want it as a channel id?


mem::drop(channel_state_lock);
if let ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) = res {
let result: Result<(), _> = Err(err);
Expand All @@ -4044,6 +4048,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana

let mut pending_events = self.pending_events.lock().unwrap();
pending_events.push(events::Event::PaymentForwarded {
from_channel_id,
to_channel_id,
fee_earned_msat,
claim_from_onchain_tx: from_onchain,
});
Expand Down Expand Up @@ -4458,7 +4464,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
}
};
self.claim_funds_internal(channel_lock, htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false);
self.claim_funds_internal(channel_lock, htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, msg.channel_id);
Ok(())
}

Expand Down Expand Up @@ -4777,7 +4783,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
MonitorEvent::HTLCEvent(htlc_update) => {
if let Some(preimage) = htlc_update.payment_preimage {
log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", log_bytes!(preimage.0));
self.claim_funds_internal(self.channel_state.lock().unwrap(), htlc_update.source, preimage, htlc_update.onchain_value_satoshis.map(|v| v * 1000), true);
self.claim_funds_internal(self.channel_state.lock().unwrap(), htlc_update.source, preimage, htlc_update.onchain_value_satoshis.map(|v| v * 1000), true, htlc_update.forward_channel_id);
} else {
log_trace!(self.logger, "Failing HTLC with hash {} from our monitor", log_bytes!(htlc_update.payment_hash.0));
self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), htlc_update.source, &htlc_update.payment_hash, HTLCFailReason::Reason { failure_code: 0x4000 | 8, data: Vec::new() });
Expand Down
27 changes: 24 additions & 3 deletions lightning/src/util/events.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,11 @@ pub enum Event {
/// If this is `true`, the forwarded HTLC was claimed by our counterparty via an on-chain
/// transaction.
claim_from_onchain_tx: bool,
/// The channel_id of the channel which sent us the payment. If the channel has been
/// force-closed this will be None
from_channel_id: Option<[u8; 32]>,
/// The channel_id of the channel which we forwarded the payment along
to_channel_id: [u8; 32],
Comment on lines +363 to +367
Copy link
Contributor

Choose a reason for hiding this comment

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

Note, given my earlier comment, these would both need to be an Option. You may need a different way to distinguish a force close in the case of from_channel_id, though.

},
/// Used to indicate that a channel with the given `channel_id` is in the process of closure.
ChannelClosed {
Expand Down Expand Up @@ -478,13 +483,20 @@ impl Writeable for Event {
(0, VecWriteWrapper(outputs), required),
});
},
&Event::PaymentForwarded { fee_earned_msat, claim_from_onchain_tx } => {
&Event::PaymentForwarded {
fee_earned_msat,
claim_from_onchain_tx,
from_channel_id,
to_channel_id,
} => {
7u8.write(writer)?;
write_tlv_fields!(writer, {
(0, fee_earned_msat, option),
(2, claim_from_onchain_tx, required),
(4, from_channel_id, option),
(6, to_channel_id, required)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

similar to comment above about tlv fields for HTLCUpdate -- plz help

Copy link
Contributor

Choose a reason for hiding this comment

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

These would be 1 and 3. IIRC, later you may have an odd field become required once we decide to no longer support an older serialization format. This would allow us to drop Option from the interface. For example, PaymentPathFailed has an odd Option<PaymentId> because the event preexisted PaymentId, but at some point we could make the field simply PaymentId and stop supporting old serialization formats.

});
},
}
&Event::ChannelClosed { ref channel_id, ref user_channel_id, ref reason } => {
9u8.write(writer)?;
write_tlv_fields!(writer, {
Expand Down Expand Up @@ -638,11 +650,20 @@ impl MaybeReadable for Event {
let f = || {
let mut fee_earned_msat = None;
let mut claim_from_onchain_tx = false;
let mut from_channel_id = None;
let mut to_channel_id = [0; 32];
read_tlv_fields!(reader, {
(0, fee_earned_msat, option),
(2, claim_from_onchain_tx, required),
(4, from_channel_id, option),
(6, to_channel_id, required)
});
Ok(Some(Event::PaymentForwarded { fee_earned_msat, claim_from_onchain_tx }))
Ok(Some(Event::PaymentForwarded {
fee_earned_msat,
claim_from_onchain_tx,
from_channel_id,
to_channel_id,
}))
};
f()
},
Expand Down