multi: add BuildOnion, SendOnion, and TrackOnion RPCs #8907

calvinrzachman · 2024-07-11T04:38:43Z

Change Description

Initial exploration into possible approach for the addition of new SendOnion (and associated construction and tracking) RPCs to the daemon.

Opted for a very slim wrapper around direct delivery of UpdateAddHTLC to the HTLCSwitch for forwarding, eg: no extra tracking by way of ChannelRouter and the ControlTower structures. This may be suitable given intended use by central proxy with instantiated ChannelRouter component, but does put more burden on users of the RPC which do not re-use code for attempt tracking/persistence in this manner.
Error information is communicated via RPC protobuf message directly such that the error types can be recreated client side if desired - as would be the case if this RPC is used by a remotely instantiated ChannelRouter .

NOTE: This version of a SendOnion style RPC was influenced heavily by its intended use. How might the RPC change if it was to be utilized by users running a "normal" lnd deployment? Would the data persisted surrounding the forwarding of onions change (eg: perhaps the ControlTower would be involved in the flow)? What about with respect to error handling?

TODO

Decide whether its's preferable to allow users of SendOnion to specify the first hop by public key and map from that to a valid short_channel_id on the server. Messed around with some htlcswitch package interfaces for this to try and keep access to link functionality limited and it might be worthy of another pass to consider the options here.
Determine whether the persistence offered by the HTLCSwitch is sufficient for these RPCs or whether the onion must be persisted as suggested here.
Finalize contents of RPC protobuf messages.
moar tests and cleanup

coderabbitai · 2024-07-11T04:38:48Z

Important

Review skipped

Auto reviews are limited to specific labels.

Labels to auto review (1)

llm-review

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.
- @coderabbitai help me debug CodeRabbit configuration file.

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 an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

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

CodeRabbit Configuration 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.

These will be used by the SendOnion RPC to communicate information about HTLCSwitch errors to RPC callers. Remote instantiations of the ChannelRouter will rely on error information via RPC to reconstruct error types needed during error handling. NOTE: This may move to an existing package eventually. Also, maybe there is a better way to do this?

Add RPC for dispatching payments via onions. The payment route and onion are computed by the caller and the onion is delivered to the server for forwarding. NOTE: The server does NOT process or peel the onion so it assumed that the onion will be constructed such that the first hop is encrypted to one of the server's channel partners.

Add RPC which constructs a sphinx onion packet for the given payment route. NOTE: This is added primarily to aid with the itests added later.

Allow the switch to defer error handling when callers of GetAttemptResult do not provide an error decrypter.

Add RPC to lookup the status of a previously forwarded onion. Allow callers of the TrackOnion rpc to indicate whether they would like to handle errors themselves or delegate error decryption to the server.

Roasbeef · 2024-08-08T19:09:43Z

cc @starius

starius · 2024-08-26T17:43:20Z

lnrpc/routerrpc/router.proto

+
+    // A code representing the type of error that occurred. This can be used
+    // to programmatically distinguish between different kinds of errors.
+    string error_code = 3;


Does it make sense to add an enum for this?

starius · 2024-08-26T17:51:30Z

lnrpc/routerrpc/router.proto

@@ -442,6 +449,46 @@ message SendToRouteResponse {
    lnrpc.Failure failure = 2;
 }

+message SendOnionRequest {


I found a similar RPC in core-lightning: https://github.com/ElementsProject/lightning/blob/v24.08rc2/cln-grpc/proto/node.proto#L859-L872

Does it make sense to unify the APIs? Some fields have different names and some of core-lightning's fields do not exist in LND.

starius · 2024-08-26T18:04:55Z

lnrpc/routerrpc/config.go

+
+	// HTLCSwitch contains shared logic between this sub server and the
+	// main rpc server.
+	// HtlcSwitch     *htlcswitch.Switch


This line seems to be a commented code.

starius · 2024-08-26T18:06:33Z

htlcswitch/switch.go

@@ -622,6 +626,76 @@ func (s *Switch) SendHTLC(firstHop lnwire.ShortChannelID, attemptID uint64,
 	return link.handleSwitchPacket(packet)
 }

+// TranslateErrorForRPC converts an error from the underlying HTLC switch to
+// a form that we can package for delivery to SendOnion rpc clients.
+func TranslateErrorForRPC(err error) (string, string) {


Please add a unit test for the function.

starius · 2024-08-26T18:06:43Z

htlcswitch/switch.go

+
+// ParseForwardingError converts an error from the format in SendOnion rpc
+// protos to a forwarding error type.
+func ParseForwardingError(errStr string) (*ForwardingError, error) {


Please add a unit test for the function.

starius · 2024-08-26T19:02:25Z

lnrpc/routerrpc/router_server.go

+	// key? This is untrusted input received via RPC.
+	sessionKey, _ := btcec.PrivKeyFromBytes(sessionKeyBytes)
+
+	var pubKeys []*btcec.PublicKey


I propose to preallocate the slice.

starius · 2024-08-26T19:08:08Z

itest/lnd_sendonion_test.go

+	routesReq := &lnrpc.QueryRoutesRequest{
+		PubKey: dave.PubKeyStr,
+		Amt:    paymentAmt,
+		// AmtMsat: paymentAmt,


Commented code.

Can we test with millisatoshis resolution?

starius · 2024-08-26T19:09:10Z

itest/lnd_sendonion_test.go

+		Timelock:       route.TotalTimeLock,
+		PaymentHash:    paymentHash,
+		OnionBlob:      onionResp.OnionBlob,
+		AttemptId:      0,


I propose to use some other value just to check that it is passed correctly (0 is the default for integer types).

starius · 2024-08-26T19:10:07Z

itest/lnd_sendonion_test.go

+	// resp, err := alice.RPC.SendOnion(onionReq)
+	// require.NoError(ht, err, "unable to send payment via onion")


Commented code.

+1 for the suggestion in the note.

starius · 2024-08-26T19:12:43Z

itest/lnd_sendonion_test.go

+		PaymentHash: paymentHash,
+		SessionKey:  onionResp.SessionKey,
+		HopPubkeys:  onionResp.HopPubkeys,
+		// SharedSecrets: [][]byte,


IIUC SharedSecrets is not covered by any itest (this test and test testTrackOnion).

I propose to add a test scenario testing this field.

starius · 2024-11-12T17:15:15Z

lnrpc/routerrpc/router_server.go

+		Amount:      lnwire.MilliSatoshi(req.Amount),
+		Expiry:      req.Timelock,
+		PaymentHash: hash,
+		OnionBlob:   [1366]byte(req.OnionBlob),


I propose to replace 1366 with lnwire.OnionPacketSize.

starius · 2024-11-12T17:17:40Z

lnrpc/routerrpc/router_server.go

+		return nil, status.Error(codes.InvalidArgument,
+			"onion blob is required")
+	}
+	if len(req.OnionBlob) > lnwire.OnionPacketSize {


Is it ok for req.OnionBlob to be shorter than lnwire.OnionPacketSize? IIUC in our use case it is always exactly lnwire.OnionPacketSize. Can we check for exact length match (== lnwire.OnionPacketSize)?

lightninglabs-deploy · 2024-11-19T17:54:59Z

@sputn1ck: review reminder
@calvinrzachman, remember to re-request review from reviewers when ready

calvinrzachman added 10 commits August 7, 2024 11:27

routerrpc: add new SendOnion rpc proto

66907a6

routerrpc: add new BuildOnion rpc proto

40b3165

routerrpc: add BuildOnion rpc

5a13943

Add RPC which constructs a sphinx onion packet for the given payment route. NOTE: This is added primarily to aid with the itests added later.

htlcswitch: return encrypted error blob when missing deobfuscator

929396d

Allow the switch to defer error handling when callers of GetAttemptResult do not provide an error decrypter.

routerrpc: add new TrackOnion rpc proto

4790933

routerrpc: add TrackOnion rpc

d11a8d8

Add RPC to lookup the status of a previously forwarded onion. Allow callers of the TrackOnion rpc to indicate whether they would like to handle errors themselves or delegate error decryption to the server.

itest: add send_onion test

fece048

itest: add track_onion test

5b183a3

calvinrzachman force-pushed the send-onion-rpc-7.10.24 branch from e6ef062 to 5b183a3 Compare August 7, 2024 15:41

calvinrzachman marked this pull request as ready for review August 8, 2024 19:08

Roasbeef requested a review from sputn1ck August 8, 2024 19:08

levmi assigned calvinrzachman Aug 20, 2024

starius reviewed Aug 26, 2024

View reviewed changes

calvinrzachman mentioned this pull request Sep 17, 2024

multi: persist results for local payments until marked for deletion calvinrzachman/lnd#12

Open

saubyk added this to the v0.19.0 milestone Oct 23, 2024

calvinrzachman mentioned this pull request Nov 7, 2024

multi: allow remote deletion of local payment attempt results calvinrzachman/lnd#13

Open

starius reviewed Nov 12, 2024

View reviewed changes

calvinrzachman mentioned this pull request Nov 20, 2024

multi: allow remote deletion of local payment attempt results #9289

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi: add BuildOnion, SendOnion, and TrackOnion RPCs #8907

multi: add BuildOnion, SendOnion, and TrackOnion RPCs #8907

calvinrzachman commented Jul 11, 2024 •

edited

Loading

coderabbitai bot commented Jul 11, 2024 •

edited

Loading

Review skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

Roasbeef commented Aug 8, 2024

starius Aug 26, 2024

starius Aug 26, 2024

starius Aug 26, 2024

starius Aug 26, 2024

starius Aug 26, 2024

starius Aug 26, 2024

starius Aug 26, 2024

starius Aug 26, 2024

starius Aug 26, 2024

starius Aug 26, 2024

starius Nov 12, 2024

starius Nov 12, 2024

lightninglabs-deploy commented Nov 19, 2024

		// resp, err := alice.RPC.SendOnion(onionReq)
		// require.NoError(ht, err, "unable to send payment via onion")

multi: add BuildOnion, SendOnion, and TrackOnion RPCs #8907

Are you sure you want to change the base?

multi: add BuildOnion, SendOnion, and TrackOnion RPCs #8907

Conversation

calvinrzachman commented Jul 11, 2024 • edited Loading

Change Description

TODO

coderabbitai bot commented Jul 11, 2024 • edited Loading

Review skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Roasbeef commented Aug 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightninglabs-deploy commented Nov 19, 2024

calvinrzachman commented Jul 11, 2024 •

edited

Loading

coderabbitai bot commented Jul 11, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)