Hi @Roasbeef I would like to take on this.

Hi @Roasbeef I would like to take on this.

Hi @Abdulkbk ! thanks so much for volunteering to deliver lnd issues. @ffranr has been assigned to handle the issue for now but if you'd like to work on another, @saubyk can be contacted on lnd slack for alternative issues

Hi @Roasbeef I would like to take on this.

Hi @Abdulkbk ! thanks so much for volunteering to deliver lnd issues. @ffranr has been assigned to handle the issue for now but if you'd like to work on another, @saubyk can be contacted on lnd slack for alternative issues

Hi @dstadulis thanks for the reply. I'll be reaching out to @saubyk on slack for another one then.

On a high level, from what I understand (from standup), the path forward for this issue is to recognise that we should not attempt to modify HTLC fields which are part of the onion. Instead, we should modify fields which are included in the update_add_htlc p2p message.

And then, the final node in the route should disregard the onion message and use the p2p message fields instead. Is that correct?

We might also need to modify this code so that we can extra the p2p custom records field in the final node. However, I think that this code is only called when retrieving an invoice. Does that mean we intend on adding a p2p message custom records field to the returned invoice?

As things currently stand, the CustomRecords field found in the invoice is the CustomRecords field that was included in the onion blob and not in the p2p message.

What we seem to be doing here is using the HTLC interceptor to generate a update_add_htlc p2p message which will be sent to the next hop in the chain. The onion blob will remain unchanged.

The "settled" action interceptor response effectively does this already by updating the preimage.

It's correct that we cannot change anything that's in the onion.
But looking at the spec, there are a couple of fields in the update_add_htlc message that we could influence.
The two most important for the use case in mind definitely are the amount_msat and update_add_htlc_tlvs.

AFAIU the amount_msat in the p2p message is allowed to differ from the amount_msat in the onion, as long as the final recipient is okay with it being different (they can reject the HTLC if they disagree, or they can accept).

And I think there's a confusion around the custom TLV fields: The goal is not to modify the custom records that are displayed on the invoice. The goal is to transmit additional information to the final recipient in order for them to accept an HTLC who's p2p wire message amount_msat might be 0 or dust but has assets attached with the corresponding value. Those custom values will only be used during the validation/decision process and not necessarily persisted anywhere.
So maybe we can for now just log them?

My Current stratergy is to pass the custom records added during HTLC interception to the peer by adding them to the ExtraData field of UpdateAddHTLC. And in doing so, I'm serialising and flattening the custom records map so that it's a single ExtraData TLV type. That way we won't mistakenly over write any existing fields.

Does that sound reasonable to you guys @Roasbeef @guggero ?

@ffranr so we do need to also modify the invoice logic here to include the TLV fields in the wire message. That'll be part of #8616. I think doing both of them concurrently will be useful as it'll enable us to do a proper itest.

My Current stratergy is to pass the custom records added during HTLC interception to the peer by adding them to the ExtraData field of UpdateAddHTLC. And in doing so, I'm serialising and flattening the custom records map so that it's a single ExtraData TLV type. That way we won't mistakenly over write any existing fields.

This sounds good. So then now we'll add a new hold response type, this type can then say add this ExtraData TLV blob to the outgoing HTLC (swap the information, so mutate now) and also modify the amt outgoing to this value.

Note that there're two custom records here:

1. The custom records that are in the HTLC onion. IIUC, this is already included in the request (lnd asking interceptor waht to do)
2. The custom records that we want to place in the outgoing wire message. This isn't included yet, and will be needed to do this transformation properly.

We should consider this issue complete now that #8633 has been merged into staging. We have a separate issue for the InvoiceAcceptor/invoice settlement interceptor work.