expose from/to_channel_id in PaymentForwarded Event #1231
Conversation
| } | ||
| 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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).
| @@ -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()); | |||
There was a problem hiding this comment.
would be nice to have a way to always get the channel_id even if it has been closed
There was a problem hiding this comment.
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?
| 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) |
There was a problem hiding this comment.
similar to comment above about tlv fields for HTLCUpdate -- plz help
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Seeing @TheBlueMatt began to answer, but might as well add my comments.
| } | ||
| 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) |
There was a problem hiding this comment.
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).
| /// 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], |
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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.
|
We ended up exposing the source via #1419. Given this hasn't been touched since Jan 10 gonna go ahead and close, if you want to revisit this probably best to open a new PR at this point. |
This is a rough first attempt of what I want to do. I'm really new to the code base so I'm not sure this is really the right way forward and am opening this just to get a quick review. Is this the right idea or should I be doing something else entirely?
Thanks