Add `Outbound` Remote Signer implementation

[ViktorT-11]

This PR introduces the functionality to utilize an alternative remote signer implementation, in which the remote signer node establishes an outbound connection to the watch-only node.

In the existing remote signer implementation, the watch-only node establishes an outbound connection to the remote signer, which accepts an inbound connection. The implementation introduced by this PR eliminates the need for the remote signer to allow any inbound connections.

To enable an outbound remote signer using the functionality introduced in this PR, please run make release-install & follow these steps: https://github.com/ViktorT-11/lnd/blob/2024-05-add-outbound-remote-signer/docs/remote-signing.md#outbound-remote-signer-example

Note: This PR does not address the requirement for the remote signer to remain online while the watch-only node is operational. Currently, all RPC requests sent to the Remote signer will fail if it goes offline, and the health monitor will then proceed to shutdown the watch-only node. Additionally, this PR does not implement any validation on the remote signer side, i.e. the Remote Signer will blindly sign whatever is sent to it. These issues will be addressed in future PRs.

Final note: I plan resolve any CI issues ASAP, so reviewers can await those fixes before starting the review if preferred. I also intend to add some review comments on a few points where feedback would be particularly helpful.

[coderabbitai[bot]]

[!IMPORTANT]

Review skipped

Auto reviews are limited to specific labels.

:label: 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.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ 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>, please review it.
  • 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 gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @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 using 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 generate docstrings to generate docstrings for this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai plan to trigger planning for file edits and PR creation.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

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.

[ziggie1984]

Interesting PR 👀, does this somehow relate to the VLS project, a bit of time ago somebody told me that there is a proxy in the making to connect LND to the VLS signer project, wondering if there is some progress made in that direction ?

[ViktorT-11]

Interesting PR 👀, does this somehow relate to the VLS project?

Thanks a lot! I would say that the current PR is somewhat related to VLS in terms of future functionality it could enable. However, this PR only focuses on reversing the connection setup between the watch-only node and the signer node, so that the signer node makes the outbound connection to the watch-only node instead of the other way around. In a setup where mobile signer wallets use VLS for validation which are connected to watch-only nodes, this functionality is a necessary pre-requisite unless the mobile wallets are online constantly.

However, this PR as it currently stand does not add any validation or enable an integration with VLS yet. The goal is to address such needs in future PRs. My hope is that the RemoteSignerClient introduced in this PR can be updated in the future to support such functionality, but this is still to be determined.

a bit of time ago somebody told me that there is a proxy in the making to connect LND to the VLS signer project.

I think the project you're referring to is: https://github.com/NYDIG/lndsigner

[ViktorT-11]

Thank you very much for the in-depth review @guggero :pray::fire::rocket:!!

I think I've addressed most of your feedback, and I think a lot of the changes made things quite a bit more cleaner :fire:!

I'd just like to raise one more point once again from my previous comment, just to understand if it's something we'd need to address:

It could be useful to let the sign requests that's passed over the stream from the watch-only node to the signer, also pass through the RPC interceptor on the signer node side. If that's something you think we should look into supporting, let me know!

@Roasbeef, thank you so much for the review comments as well! I've responded to your comments, and I hope that answers your questions. Let me know if there's a smarter way to achieve the same functionality :)!

Thanks again :fire:!

[ViktorT-11]

Thank you so much for the reviews @Roasbeef & @guggero :tada:!! Really appreciate the great feedback :pray:! Working on addressing your feedback now.

I'm running into a new itest error for the unannounced_channels itest, that seems to occur due to changes that have been merged in master since September, since the latest changes I've made to the PR aren't causing the failure. I'm investigating this and will try to find a solution asap!

[ViktorT-11]

Thanks for the really great feedback @Roasbeef & @guggero! I've addressed t with the latest push :rocket:! I'll re-request reviews once I've responded to your comments.

[ViktorT-11]

Thanks a lot for the reviews @Roasbeef & @guggero! It's immensely appreciated :pray:! I've addressed your feedback and responded to your comments above :tada:!

Additionally, here's my response to your main comments:

From @Roasbeef's main comment:

Can we use some of the recent concurrency abstraction PRs to simplify all the wait group and mutex usage?

See: https://github.com/lightningnetwork/lnd/pull/8754#discussion_r1830828851

Can we update the flows (potentially sending additional information in the handshake?) to not require a restart of the watch only node during provisioning?

See: https://github.com/lightningnetwork/lnd/pull/8754#discussion_r1830801103

From @guggero's main comment:

Pass along a signature from the outbound remote signer to the watch-only node, to authenticate (authentication in the other direction is done through macaroons).

See: https://github.com/lightningnetwork/lnd/pull/8754#discussion_r1830779166

Handle disconnections of the outbound remote signer: should lnd just wait? Or go into a "safe" mode? Can p2p connections remain alive (how long does a noise session key remain valid without needing ECDH operations)?

So I think we've decided offline that we're not going to prio enabling that the watch-only node can remain online when the signer is offline for now? Instead, we'll focus on making it super fast on starting up the watch-only node once the signer comes back online. If I've misunderstood our prios, let me know! Other than that, I totally agree that that'd be the approach for making it possible for the watch-only node to remain online when the signer is offline.

Given that without those two items it probably doesn't make sense to ship this feature to users yet, does it make sense to not yet expose (or just hide in the CLI help output and add a "not yet finished" comment) some of the flags to enable this mode?

Given my response, I do think it might be worth shipping the feature prior to those items being addressed? Potentially, we'd want real Auth functionality present prior to shipping it though.

[ViktorT-11]

Thank you so much for the testing and review @guggero :pray:!!

I've addressed your feedback and left comments above :).

I did test shutting down the outbound remote signer and restarting it before the watch-only node noticed the connection was gone. That didn't work, but that is kind of expected since we're only going to look at that in the follow-up PR that can deal with connection interruptions.

Yes, that's correct. That's unfortunately "per design" currently, and like you say, should be addressed once in a follow-up that makes it possible for the signer to disconnect. It and would be addressed by changing sign_coordinator so it's not limited to only allowing a single connection to be set up.

Finally: I want to flag this once more: The sign requests that pass over the stream to the remote signer, never goes through the remote signer node's RPC interceptor. Are we ok with that in this PR?

[ViktorT-11]

Rebased on master.

[ViktorT-11]

Fixed some minor docs issues.

[ViktorT-11]

Thanks a lot for the in depth review @ellemouton :tada::pray:!

I addressed your feedback in the latest push! Haven't had time to update the commit messages and respond to your comments yet. I'll do so tomorrow and then re-request a review :)

[ViktorT-11]

Thanks a lot for the review @ellemouton :pray:!!! Really appreciate. I've addressed your feedback with the latest pushes. I've also responded to some of your comments which I couldn't fully address. Please let me know if you agree with my comments, or know ways to address them :)

[ViktorT-11]

Rebased on master, and some minor lint fixes.

[lightninglabs-deploy]

@roasbeef: review reminder @guggero: review reminder @yyforyongyu: review reminder @viktortigerstrom, remember to re-request review from reviewers when ready

[guggero]

!lightninglabs-deploy mute