rpc+htlcswitch: add HTLC transformation capabilities to the interceptor

[Roasbeef]

Today we have the HTLCInterceptor, it's useful for doing things like implementing a custom forwarding policy. Today you can accept/reject, and even provide a custom preimage to short circuit normal forwarding an "inject" the preimage directly into the route.

Protocol improvement proposals such as the endorsement bit also need the ability to transform an incoming HTLC into a new outgoing HTLC. On example is if you get the bit set on the incoming link, but decide to not set it on the outgoing link. Today, the htlc interceptor won't allow such a workflow. We should extend the interceptor to support this feature.

Steps To Completion

• Extend the ForwardHtlcInterceptResponse struct with the ability to swap out the outgoing amt and/or custom TLV blobs.

• Update the intercept-able switch to be able to process the new ResolveHoldForwardAction type.

[Abdulkbk]

Hi @Roasbeef I would like to take on this.

[dstadulis]

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

[Abdulkbk]

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.

[ffranr]

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.

[ffranr]

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.

[guggero]

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?

[ffranr]

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 ?