[API]: ForwardInterceptor overwrites existing CustomRecords

[carlaKC]

This is an API design question, not a bug.

We're interested in the following scenario: 1) The forward interceptor is used with the MODIFIED action 2) We're intercepting a HTLC with a custom range endorsed TLV 3) The interceptor request's out_wire_custom_records are either: 3.1) Populated by the interceptor request 3.2) Empty

I'm using the endorsement field in (2) because I hit this in #8390, but the same would apply to any custom record that LND may want to attach by default in the future. If we don't forsee LND ever appending custom records to HTLCs then this isn't as interesting of a question.

We have different API behaviors depending on whether or not out_wire_custom_records are set by the interceptor: 3.1) If the interceptor sets custom records, we only propagate the records provided in the interceptor request 3.2) If the interceptor request does not provide custom records, we propagate whatever was already there

This happens because we overwrite htlc.CustomRecords if the interceptor sets a value, and otherwise leave as-is.

I think it would be more consistent for the interceptor to always require that the MODIFIED request provides all of the TLVs they want to propagate - ie, do not passively propagate the records in the 3.2 case. This has the downside of requiring the caller to copy all custom records that they're not concerned with from response to request, but I think is cleaner than the alternative of trying to specify mutation of the existing records via protobuf.

[guggero]

Good question. I didn't deeply think about this when implementing/polishing the latest version of the interceptor.

My main concern was to bes fully backward compatible: If your interceptor client code doesn't change, any custom records in the HTLC will just be forwarded, not breaking any custom protocols in the process. But I guess since you'd need to change your code to use the new MODIFY action anyway, the backward compatibility would still hold if we changed the behavior.

One thing the current version can't do is allow the user to clear all custom records (e.g. don't forward any of them), since an empty out_wire_custom_records map is interpreted as "don't modify".

If I understand you correctly, you advocate for always overwriting the outgoing custom records with the out_wire_custom_records map (on MODIFY action only), even if the specified map is empty, therefore allowing the user to clear any custom records?

If you can see a use case for that, I would support that change. And I guess the most important change would be to clearly document the behavior in the API docs (which might currently be missing).