[3/3]: Blinded Route Error Handling

[carlaKC]

This PR is the third and final in the route blinding forwarding series, including:

• Conversion of errors in a blinded route to INVALID_ONION_BLINDING
• Itest coverage for error handling.
• Advertizing of optional blinding feature bit.

Depends on #8160

[coderabbitai[bot]]

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

Walkthrough

This update introduces significant enhancements to the Lightning Network, focusing on improving privacy and functionality through the support of blinded payments. By simplifying configuration and adding new features for route blinding, the changes aim to enhance anonymity and usability. The modifications span across configuration, documentation, feature management, error handling, and testing, reflecting a comprehensive effort to support more secure and private transactions.

Changes

Files	Change Summary
config.go	Removed declaration for ProtocolOptions in DefaultConfig.
docs/release-notes/release-notes-0.18.0.md	Enhanced support for forwarding blinded payments.
feature/... (default_sets.go, deps.go, manager.go)	Added support for lnwire.RouteBlindingOptional and dependencies.
htlcswitch/... (circuit.go, hop/..., link.go, mock.go, switch.go)	Improved error handling and functionality for blinded routes.
itest/... (list_on_test.go, lnd_route_blinding_test.go)	Added tests for blinded routes and error handling scenarios.
lncfg/... (protocol.go, protocol_integration.go)	Removed redundant functions for route blinding configuration.
lnrpc/... (lightning.proto, .swagger.json, peersrpc/peers.swagger.json, routerrpc/router.swagger.json)	Added support for ROUTE_BLINDING_REQUIRED and OPTIONAL.
lnwire/features.go, server.go	Added feature bits for blinded payments and updated server configuration.

Possibly related issues

• lightningnetwork/lnd#8363: The enhancements made in this PR could potentially address the objectives of extending LND to support payment to multiple blinded paths, as it introduces foundational support for blinded payments and improved error handling in blinded routes. The focus on privacy and usability aligns with the issue's goals.

🐰 A Poem by CodeRabbit

In a network, lightning fast,
Privacy's shadow, overcast.
Blinded routes, now take the stage,
In the quest for privacy, a new age.
🌟 Through code, we weave a safer net,
Anonymity, a promise set.
🚀 To the future, we hop with glee,
In a world more private, we aim to be.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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.

[carlaKC]

Opening this as a draft because I still want to test this against other implementations, but basic functionality is in place.

[carlaKC]

~Hold off on review here - ran into an impl issue while testing :(~

Update: ready for review!

[carlaKC]

High level takeaway from first round of reivew is whether we want to simplify this and just add a isBlindedPacket to help us manage errors rather than looking at the presence of the blinding point. Happy to explore that direction, wanted to try to do this a "smart" way, but that can often turn out to be a dumb idea.

I do think that we (sadly) so have to keep handling in various place due to availability of the ErrorEncryptor for failures on outgoing link (done in the switch / not available on the incoming link) but I'll double check that!

[carlaKC]

Re-confirming interop w/ CLN v24.02.1 @ 42c2fac29 since we've had quite a few changes! With the small exception of the max cltv height check because I'm getting the wrong value, so I just commented that check out to test.

Tested LND1 as introduction point for the following topology, using the same offer-fetching trick as before to get a blinded path: Sender LND --> LND1 --(private)--> CLN.

Working on testing relaying (non-introduction) as well, but need to find a way to create a multi-hop path (looking into testing w/ ldk), seems like CLN just does one hop rn.

[carlaKC]

@orbitalturtle confirmed interop w/ Eclair (via LNDK) when LND is an intermediate forwarding node (ie, not introduction) because they have multi-hop blinded - setup writeup here!

[Roasbeef]

Needs a rebase!

[carlaKC]

Will do! Working on some of the suggestions from @ellemouton + @bitromortac's review, think they'll clean this up really nicely 🥇 Aiming to push later today, latest tomorrow.

[carlaKC]

Updated PR description with overview of approach, but tl;dr:

• Isolated blinded error handling in the incoming link as suggested, much cleaner!
• Using an ErrorEncrytor type to store whether we were part of a blinded route (over an isBlinding bool) because we don't always have this information on hand otherwise (see description for full details)

This could use some unit tests, but wanted to get an ACK on the approach with the ErrorEncryptor before going all in (perhaps I've missed something and we don't need this / other suggestions ?).

[carlaKC]

Ready for a look 🙏 @ellemouton @bitromortac

[carlaKC]

Pushed to address comments so far!

I'm not sure yet if the type wrapping method is the best approach

Thinking about alternatives, we could add a type enum to SphinxErrorEncryper to get rid of the wrappers? So when we decode and we get type = Sphinx or Intro or Relaying we always create a SphinxErrorEncrypter but then just set a Type field in that encrypter to the variant we're using. That would cut down on boilerplate a little, but we'd need to be a more careful with handling the various types - WDYT @bitromortac ?

[carlaKC]

Tried a few approaches for how we deal with RouteRole and nothing I could find is totally clean.

Problem:

• We need to know what hop type we were when HopPayload fails (so we can fail back accordingly)
• This API combines payload parsing and validation (rightfully so, as we have different types of hops), so when it returns with an error (and nil payload) we don't know whether we:
  • Could not parse our payload
  • Parsed our payload, got a blinding point inside of it, and then hit a validation error
  • Whether we had an update add blinding point (as it's in sphinxIterator's blindingKit)

Note that we don't face any of these issues once HopPayload has returned successfully, because we can now just look at the payload.

There are a few approaches we could go with, I've gone with returning RouteRole from HopPayload and breaking up parsing/validation of payload a bit more. This does break go patterns, because this value is used even when our error is non-nil, but it's as nice as I could get it.

The previous approach of setting this value in ErrInvalidBlinding has the downside of needing to set this values everywhere (propagating additional blinding context) and putting us at risk of mishandling errors if every error from HopPayload doesn't return ErrInvalidPayload (because we'll miss our role).

[carlaKC]

taking a look at that itest failure - restriction on mining > 100 blocks at a time added to itests?

Update: yep! Hit newly added restriction on blocks (forgot that itests run against master, which is why this didn't show up locally). Fix incoming!

[carlaKC]

Removed critical log + addressed itest issue.

Ran into an issue where the second level HTLC claim wasn't going through because the amount was too small to be feasible, so the test got stuck - @ziggie1984 very graciously helped me debug + is going to open up an issue.

[carlaKC]

It seems to be a too long test for the DefaultTimeout together with the call.

Bumped here, hopefully a minute is long enough for the GH potato - will see how this run goes and increase further if anything flakes!

[Roasbeef]

Fixed a race that popped up in the uint tests here (unrelated) here: https://github.com/lightningnetwork/lnd/pull/8694

[Roasbeef]

With that aside, there is a failure of a new itest added in this PR (neutrino backend):

--- FAIL: TestLightningNetworkDaemon/tranche03/145-of-154/neutrino/on_chain_to_blinded (40.61s)

[carlaKC]

With that aside, there is a failure of a new itest added in this PR (neutrino backend):

--- FAIL: TestLightningNetworkDaemon/tranche03/145-of-154/neutrino/on_chain_to_blinded (40.61s)

Thought our timeout might be too slow, bumping up timeout to larger window so that there's time for CI to resolve on-chain.

[Roasbeef]

Made #8696 to address that sqlite race.