rpc+htlcswitch: add htlc transformation capabilities to the interceptor by ffranr · Pull Request #8633 · lightningnetwork/lnd

ffranr · 2024-04-09T16:43:51Z

Change Description

Closes #8619

Steps to Test

Steps for reviewers to follow to test the change.

Pull Request Checklist

Testing

Your PR passes all CI checks.
Tests covering the positive and negative (error paths) are included.
Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
Commits follow the Ideal Git Commit Structure.
Any new logging statements use an appropriate subsystem and logging level.
Any new lncli commands have appropriate tags in the comments for the rpc in the proto file.
There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

coderabbitai · 2024-04-09T16:43:56Z

Important

Auto Review Skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

guggero

Very nice how small this turned out. Just did a quick scan out of interest, not a deep review.

itest/list_on_test.go

lnrpc/routerrpc/router.proto

htlcswitch/interceptable_switch.go

lnrpc/routerrpc/router.proto

ffranr · 2024-04-13T12:01:30Z

There are two things left to do here:

Add support for modifying (via HTLC interception) p2p message lnwire.UpdateFulfillHTLC.
Write received UpdateAddHTLC to log so that the itest can ensure that Carol's node receives the correctly modified message.

I would be keen on feedback on what I have so far to check that I'm still on the right track.

Note that I couldn't include the record.CustomSet type as a field in UpdateAddHTLC because of an import cycle. Therefore, the code is currently serializing record.CustomSet into a blob before attaching it to UpdateAddHTLC.

It seems to me that much of package record should be merged into lnwire, is that a good idea?

ffranr · 2024-04-15T11:27:33Z

There are two things left to do here:
1. Add support for modifying (via HTLC interception) p2p message `lnwire.UpdateFulfillHTLC`.

2. Write received `UpdateAddHTLC` to log so that the itest can ensure that Carol's node receives the correctly modified message.
I would be keen on feedback on what I have so far to check that I'm still on the right track.

Note that I couldn't include the record.CustomSet type as a field in UpdateAddHTLC because of an import cycle. Therefore, the code is currently serializing record.CustomSet into a blob before attaching it to UpdateAddHTLC.

It seems to me that much of package record should be merged into lnwire, is that a good idea?

I'm not sure if we need to modify lnwire.UpdateFulfillHTLC also. We can leave that for a separate PR.
This is complete. We're checking Carol's log in the itest.

guggero

Did a first pass. Looks good, mostly nits 🎉

htlcswitch/interfaces.go

lnwire/update_add_htlc.go

htlcswitch/interceptable_switch.go

lnrpc/routerrpc/router.proto

itest/lnd_forward_interceptor_test.go

lnwire/update_add_htlc.go

record/custom_records.go

htlcswitch/interceptable_switch.go

lnrpc/routerrpc/forward_interceptor.go

guggero

Getting super close!

record/custom_records.go

lnwire/update_add_htlc.go

ffranr · 2024-04-29T10:42:22Z

New message encode/decode changes are in place.

Still need to modify lnwire.UpdateFulfillHTLC, but a review at this point would be useful to ensure correct encode/decode.

guggero

Looks very good!
Just a couple of suggestions to remove code duplication, increase clarity and re-usability.

guggero · 2024-04-29T11:12:29Z

lnwire/custom_records.go

Why do we need a completely new type? Can't we just create a ValidateCustomTypes(tlvMap tlv.TypeMap) function that does this? Especially since this is more or less an exact copy of the record.CustomSet type as well.

We don't need to introduce a new type; however, it aids in specifying permissible values for the type. The tlv.TypeMap is insufficiently restrictive. By incorporating the lnwire.CustomRecords type, we can directly annotate valid map keys within the type definition itself, rather than relegating this information to a separate ValidateCustomTypes function.

I think a new type might also add clarity as a function argument.

(We can't use record.CustomSet in lnwire because of an import cycle.)

Why do we frequently choose a more generic type (tlv.TypeMap) that lacks the necessary restrictiveness, over a more specific type (lnwire.CustomRecords)? IMO, this practice often results in missed opportunities for documentation. Using overly generic types in function signatures makes them harder to understand and decreases code clarity. Opting for more specific types can enhance both documentation and code readability.

Why do we frequently choose a more generic type (tlv.TypeMap) that lacks the necessary restrictiveness

I think it's generally not the best idea to have too much business/protocol logic in the wire/encoding level, as that reduces the re-usability. I know that shifts the responsibility of validating the content to the caller. But should a wire message really be dictating what its content should be, rather than focusing on how to encode the content?

To me the encoding function as it looks now mostly distracts from what we actually do: merging two sets of TLV records into a single encoded blob.

We have to ensure that the TLV types we're merging from custom records don't mess with any future protocol extensions which might add new TLV types. In other words, we need to make sure that the custom records TLV types are >= 65536. Are you saying we shouldn't enforce that at the point of encoding in lnwire?

I've added a method called CustomRecords.ExtendRecordProducers which moves the protocol logic to the CustomRecords type away from the UpdateAddHtlc.Encode method.

lnwire/update_add_htlc.go

guggero

A bit more bike shedding around custom types and validation, but the rest looks good to me!

record/custom_records.go

lnwire/update_add_htlc.go

guggero · 2024-04-30T07:22:29Z

lnwire/update_add_htlc.go

If we're going with a custom type, then this could be a method on it.

I've moved this to a new method called CustomRecords.ExtendRecordProducers. Let me know what you think.

guggero · 2024-04-30T07:24:45Z

lnwire/update_add_htlc.go

What if we ever want to encode a "known" custom type here? Then this validation would disallow it. I think what we want to check instead is just that we don't get any collisions between the known records and the custom ones added by the interceptor.

I've added a new method called CustomRecords.ExtendRecordProducers which uses the validation that you suggest. It simply checks for no collisions. The records slice can contain high TLV types even before extending with records from CustomRecords.

lnwire/extra_bytes.go

guggero

Looks good once the new unit test failure has been addressed.

lnwire/custom_records.go

lnrpc/routerrpc/forward_interceptor.go

This commit introduces the `CustomRecords` type in the `lnwire` package, designed to hold arbitrary byte slices. Each entry in this map can associate with TLV type values that are greater than or equal to 65536.

- Add instance constructor `NewExtraOpaqueDataFromTlvTypeMap`. - Add method `Copy`. - Add method `RecordProducers`.

- Introduce the field `CustomRecords` to the type `UpdateAddHtlc`. - Encode and decode the new field into the `ExtraData` field of the `update_add_htlc` wire message.

This commit adds a random `CustomRecords` field to the `UpdateAddHTLC` message encode/decode unit test.

- Introduce the field `CustomRecords` to the type `UpdateFulfillHtlc`. - Encode and decode the new field into the `ExtraData` field of the `update_fulfill_htlc` wire message. - Empty `ExtraData` field is set to `nil`.

This commit defines random fields for the `UpdateFulfillHTLC` message as part of the encode/decode unit tests.

Introduce `ResumeModified` action to resume standard behavior of a p2p message with optional modifications as specified by the client during interception.

This commit extends the forward HTLC intercept response with fields that can be used in conjunction with a `ResumeModified` action to modify the intercepted HTLC p2p message.

Implement an integration test where an HTLC is intercepted and the interception response modifies fields in the resultant p2p message.

GeorgeTsagk

LGTM 🧱
+tACK 🟢

got a few Qs, non-blocking

GeorgeTsagk · 2024-05-16T11:18:01Z

intercepted_forward.go

+func (f *interceptedForward) ResumeModified(_, _ fn.Option[lnwire.MilliSatoshi],
+	_ fn.Option[record.CustomSet]) error {
+
+	return ErrCannotResume


maybe I'm missing something, why are we defaulting to err here? is this some old/outdated impementation?

This code relates to the on-chain resolution flow. In the on-chain resolution flow, an intercepted forward can not be resumed. Therefore, it can't be resume modified either.

GeorgeTsagk · 2024-05-16T12:16:31Z

itest/lnd_forward_interceptor_test.go

+	require.NoError(ht, err, "failed to send request")
+
+	// Check that the modified UpdateAddHTLC message fields were reported in
+	// Carol's log.


we used this trick to get the itests running

should we now consider reading these values the "healthy" way? i.e Carol also intercepts and acquires values from htlc message?

I think ideally Carol should intercept as you suggest. But the log lookup should do for this PR, IMO. I'll see if I can put another PR so that Carol can intercept also. That way this test wont be bound to a log message.

lnwire/custom_records.go

lnwire/extra_bytes.go

guggero reviewed Apr 9, 2024

View reviewed changes

itest/list_on_test.go Outdated Show resolved Hide resolved

lnrpc/routerrpc/router.proto Outdated Show resolved Hide resolved

Roasbeef reviewed Apr 11, 2024

View reviewed changes

htlcswitch/interceptable_switch.go Outdated Show resolved Hide resolved

htlcswitch/interceptable_switch.go Outdated Show resolved Hide resolved

htlcswitch/interceptable_switch.go Outdated Show resolved Hide resolved

lnrpc/routerrpc/router.proto Outdated Show resolved Hide resolved

ffranr force-pushed the 8619-rpc+htlcswitch-add-htlc-transformation-capabilities-to-the-interceptor branch 2 times, most recently from 60596eb to c5d25a8 Compare April 13, 2024 11:55

ffranr requested review from Roasbeef and guggero April 13, 2024 12:01

ffranr force-pushed the 8619-rpc+htlcswitch-add-htlc-transformation-capabilities-to-the-interceptor branch from c5d25a8 to 071d8f7 Compare April 15, 2024 11:26

ffranr force-pushed the 8619-rpc+htlcswitch-add-htlc-transformation-capabilities-to-the-interceptor branch from 071d8f7 to 2d70c13 Compare April 15, 2024 11:34

ffranr marked this pull request as ready for review April 15, 2024 11:40

This was referenced Apr 16, 2024

LightningPayment first hop custom records #8656

Closed

Enhance update_add_htlc with remote peer's custom records #8660

Merged

guggero reviewed Apr 22, 2024

View reviewed changes

ffranr force-pushed the 8619-rpc+htlcswitch-add-htlc-transformation-capabilities-to-the-interceptor branch from 2d70c13 to c91e771 Compare April 23, 2024 14:51

ffranr requested a review from guggero April 23, 2024 15:24

Roasbeef reviewed Apr 23, 2024

View reviewed changes

lnwire/update_add_htlc.go Outdated Show resolved Hide resolved

Roasbeef requested changes Apr 24, 2024

View reviewed changes

record/custom_records.go Outdated Show resolved Hide resolved

htlcswitch/interceptable_switch.go Outdated Show resolved Hide resolved

lnrpc/routerrpc/forward_interceptor.go Outdated Show resolved Hide resolved

guggero reviewed Apr 25, 2024

View reviewed changes

record/custom_records.go Outdated Show resolved Hide resolved

record/custom_records.go Outdated Show resolved Hide resolved

lnwire/update_add_htlc.go Outdated Show resolved Hide resolved

ffranr force-pushed the 8619-rpc+htlcswitch-add-htlc-transformation-capabilities-to-the-interceptor branch 2 times, most recently from 5ccbc21 to f73c885 Compare April 29, 2024 10:40

ffranr requested review from Roasbeef and guggero April 29, 2024 10:41

guggero reviewed Apr 29, 2024

View reviewed changes

ffranr force-pushed the 8619-rpc+htlcswitch-add-htlc-transformation-capabilities-to-the-interceptor branch 3 times, most recently from 264e2cb to 0f5ec6b Compare April 29, 2024 21:42

guggero reviewed Apr 30, 2024

View reviewed changes

ffranr added 2 commits May 14, 2024 11:57

multi: improve comment grammar

043f696

htlcswitch: add missing method doc

8b1d9c9

ffranr force-pushed the 8619-rpc+htlcswitch-add-htlc-transformation-capabilities-to-the-interceptor branch from 9e9e4d4 to 9091805 Compare May 14, 2024 10:57

ffranr changed the base branch from custom-channels-integration to 0-19-staging May 14, 2024 10:59

ffranr force-pushed the 8619-rpc+htlcswitch-add-htlc-transformation-capabilities-to-the-interceptor branch from 9091805 to d89df52 Compare May 14, 2024 11:03

ffranr requested a review from GeorgeTsagk May 14, 2024 11:06

ffranr force-pushed the 8619-rpc+htlcswitch-add-htlc-transformation-capabilities-to-the-interceptor branch 2 times, most recently from 62f93d9 to b5a7696 Compare May 14, 2024 12:08

ellemouton reviewed May 15, 2024

View reviewed changes

lnwire/extra_bytes.go Outdated Show resolved Hide resolved

ffranr force-pushed the 8619-rpc+htlcswitch-add-htlc-transformation-capabilities-to-the-interceptor branch from b5a7696 to 1e0ea65 Compare May 15, 2024 14:49

guggero reviewed May 15, 2024

View reviewed changes

lnwire/custom_records.go Outdated Show resolved Hide resolved

lnrpc/routerrpc/forward_interceptor.go Outdated Show resolved Hide resolved

ffranr force-pushed the 8619-rpc+htlcswitch-add-htlc-transformation-capabilities-to-the-interceptor branch 2 times, most recently from 01587b2 to 5d3458f Compare May 15, 2024 15:42

ffranr and others added 13 commits May 15, 2024 16:47

lnwire: add type CustomRecords

71c3251

This commit introduces the `CustomRecords` type in the `lnwire` package, designed to hold arbitrary byte slices. Each entry in this map can associate with TLV type values that are greater than or equal to 65536.

lnwire: add unit tests for ExtraOpaqueData.PackRecords

795aff5

lnwire: add ExtraOpaqueData helper functions and methods

41a5b9a

- Add instance constructor `NewExtraOpaqueDataFromTlvTypeMap`. - Add method `Copy`. - Add method `RecordProducers`.

lnwire: add custom records field to type UpdateAddHtlc

2b3618c

- Introduce the field `CustomRecords` to the type `UpdateAddHtlc`. - Encode and decode the new field into the `ExtraData` field of the `update_add_htlc` wire message.

lnwire: add encoding/decoding unit tests for UpdateAddHtlc message

b93d288

lnwire: rand CustomRecords for UpdateAddHTLC encode/decode fuzz test

6fa2db0

This commit adds a random `CustomRecords` field to the `UpdateAddHTLC` message encode/decode unit test.

lnwire: add custom records field to type UpdateFulfillHtlc

d247cc9

- Introduce the field `CustomRecords` to the type `UpdateFulfillHtlc`. - Encode and decode the new field into the `ExtraData` field of the `update_fulfill_htlc` wire message. - Empty `ExtraData` field is set to `nil`.

lnwire: add encoding/decoding unit tests for UpdateFulfillHTLC message

5c3f7be

lnwire: specify random fields for UpdateFulfillHTLC message in fuzz test

406fbe2

This commit defines random fields for the `UpdateFulfillHTLC` message as part of the encode/decode unit tests.

htlcswitch: add resume modified HTLC action to switch

31ee274

Introduce `ResumeModified` action to resume standard behavior of a p2p message with optional modifications as specified by the client during interception.

routerrpc: extend HTLC forward interceptor resp with modification fields

bd5de43

This commit extends the forward HTLC intercept response with fields that can be used in conjunction with a `ResumeModified` action to modify the intercepted HTLC p2p message.

itest: add itest for field modification HTLC interception response

8025296

Implement an integration test where an HTLC is intercepted and the interception response modifies fields in the resultant p2p message.

multi: add incoming htlc amount to interceptor

03dceca

ffranr force-pushed the 8619-rpc+htlcswitch-add-htlc-transformation-capabilities-to-the-interceptor branch from 5d3458f to 03dceca Compare May 15, 2024 15:48

GeorgeTsagk approved these changes May 16, 2024

View reviewed changes

guggero merged commit a908c57 into lightningnetwork:0-19-staging May 16, 2024

ffranr mentioned this pull request May 17, 2024

rpc+htlcswitch: add HTLC transformation capabilities to the interceptor #8619

Closed

2 tasks

Conversation

ffranr commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Description

Steps to Test

Pull Request Checklist

Testing

Code Style and Documentation

Uh oh!

coderabbitai bot commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ffranr commented Apr 13, 2024

Uh oh!

ffranr commented Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ffranr commented Apr 29, 2024

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ffranr commented Apr 9, 2024 •

edited

Loading

coderabbitai bot commented Apr 9, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)

ffranr commented Apr 15, 2024 •

edited

Loading