[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).

[carlaKC]

If I understand you correctly, you advocate for always overwriting the outgoing custom records with the

I don't have strong opinions/use case for it, just advocate for the map to either be a merge or an overwrite for the sake of consistency (right now it's an overwrite when it's populated and technically a merge when it's empty). Seems like this could be a source of confusion down the line if the API gets much use.

And I guess the most important change would be to clearly document the behavior in the API docs (which might currently be missing).

Opened #9215 to update docs, discussed this on contributor call and there wasn't much appetite for making a change so closing this issue w/ the docs update.

[ProofOfKeags]

I think the ideal API here allows for two cases:

1. don't touch it: leave it completely unmodified, I don't even want to have to think about it
2. full replacement: I am taking responsibility for the records, give the incoming ones as input, I will do a full replacement

My intuition here is that we should make an empty map be replacement with no records, and an absent map behave as the identity function (leave unmodified). However, this is where the details of protobuf encodings might get in our way. I believe that repeated in protobuf means that things can appear zero times. This would mean that there is no way to encode a present-but-empty map. Assuming we can't make this distinction, I think it's probably best that we opt for full-replacement even though that may require user action.

By inserting an interceptor into the workflow the user is claiming responsibility for interceding an otherwise automatic process. Losing the ability to delete records as outlined in #9215 is (in my view) not a good tradeoff here. It makes the API confusing and unpredictable (which accumulates and compounds) just to make things slightly easier for a process where the user has already claimed the responsibility.

If this suggestion is untenable, I think we should go with a full-merge based approach by:

1. having an output for deleted keys
2. having an output for added (replace on conflict) key-value pairs
   We always apply 1 before 2 here as I believe the sensible interpretation of having included them in both deleted and added means that they definitely wanted the stuff they added, not just to delete it.

That said, I do believe that this will be less intuitive and easier to get wrong as a user. Full-replacement is a more easily explained set of semantics.

[carlaKC]

This would mean that there is no way to encode a present-but-empty map. Assuming we can't make this distinction

For other APIs we've gone with a bool to distinguish between setting a zero value and no value at all, that's also an option here:

• Always merge the record map with existing fields (replace on conflict)
• Add a clear_custom_records option to clear custom_records (which must be set alongside an empty map)

This came up on the last contributor call and it seemed to me like there wasn't much enthusiasm to make any changes here, so the questions isn't really about API design - it's whether this is something that'll be written/reviewed/merged. I'm happy to review a change fwiw, but don't have a compelling enough use case/opinion to write the PR.

[ProofOfKeags]

I'm happy to review a change fwiw, but don't have a compelling enough use case/opinion to write the PR.

Seems reasonable.

Add a clear_custom_records option to clear custom_records (which must be set alongside an empty map)

If we go this route, then assuming we don't actually implement this yet, but we want the semantics to resemble the above AND we want the future change to be backwards compatible. Then that means that we would start with a full-merge API. Which iirc is not the way things are designed right?

[carlaKC]

Then that means that we would start with a full-merge API. Which iirc is not the way things are designed right?

Yes that's correct. Seems like a reasonable (and small) to update the API to merge (/overwrite) when records are populated on the request - @guggero would that work for your current use case?

[guggero]

Sure, I think that should work for our use case. I think it's just important to make those changes before we ship this new functionality with a tagged release. So the question is: do you have time to do the update? And would it be a new PR on top of the documentation change? Ideally we'd merge the doc change PR, then fix it with the new behavior in a new PR, then the diff to today's functionality will be even more clear.

[carlaKC]

Ideally we'd merge the doc change PR, then fix it with the new behavior in a new PR, then the diff to today's functionality will be even more clear.

SGTM! Let's go ahead with merging #9215 and I'll follow with the fix, looks pretty simple.

It is a bit difficult to test, so I think I'll rebase #8390 on it so that we can test the case where LND already has some records?