Add Outbound Remote Signer implementation
#8754
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Important Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Additionally, you can add CodeRabbit Configration File (
|
ViktorTigerstrom
changed the title
Outbound Remote Signer implementation
ViktorTigerstrom
force-pushed
the
2024-05-add-outbound-remote-signer
branch
9 times, most recently
from
0525eb6
to
6abefde
Compare
ViktorTigerstrom
force-pushed
the
2024-05-add-outbound-remote-signer
branch
from
6abefde
to
b34174d
Compare
This commit adds additional config options to the RemoteSigner config, enabling a new alternative remote signer implementation. This new implementation sets up the connection between the remote signer and the watch-only node in the opposite way compared to the previously available implementation. In this setup, the remote signer will make an outbound connection to the watch-only node, whereas the previous version allowed an inbound connection from the watch-only node. Therefore, we call this remote signer type an "outbound remote signer." The actual implementation for this new version will be added in the commits following this one. The new version is temporarily disabled in the config validation until the implementation commits have been added.
The documentation for the DefaultRemoteSignerRPCTimeout constant incorrectly specified that the value was also used as the timeout for requests to and from the remote signer. However, the value is only used as the timeout when setting up the connection to the remote signer. This commit corrects the documentation to reflect the actual usage of the constant.
To enable an outbound remote signer to connect to the watch-only lnd node, we add a SignCoordinatorStreams bi-directional streaming RPC endpoint. The stream created when the remote signer connects to this endpoint can be used to pass any requests to the remote signer and to receive the corresponding responses. We clearly define the types of requests and responses that can be sent over the stream, including all the requests that can be sent to the remote signer with the previous implementation. Those are the ones sent to the `signrpc.SignerClient` and `walletrpc.WalletKitClient` in the `lnwallet/rpcwallet.go` file. We also include messages for the required handshake between the remote signer and the watch-only node, and a message that the remote signer can send if it encounters an error while processing a request.
RemoteSigner is an interface that abstracts the communication with a remote signer. It extends the RemoteSignerRequests interface. Note that this is the interface for the remote signer on the watch-only node's side. As we'll add an outbound remote signer implementation in upcoming commits, we need this interface to abstract the commonalities of both the inbound and outbound remote signer implementations, so that the RPCKeyRing doesn't need to know which type it's using.
This commit wraps the current remote signer implementation in the new RemoteSigner interface within an InboundRemoteSigner struct.
`RemoteSignerBuilder` is a helper that creates instances of the RemoteSigner interface based on the lncfg.RemoteSigner config. It is intended to create different types of remote signers instances based on the SignerType specified in the config.
As the RemoteSignerBuilder can now create an InboundRemoteSigner that matches the functionality of the previous remote signer communication implementation, we refactor the rpcwallet package to use a RemoteSigner instance created by the RemoteSignerBuilder.
With the `RPCKeyRing` now having the `RemoteSigner` reference, we can use that reference to call the `Ping` implementation of the `RemoteSigner` interface for the health check of the remote signer. This allows different types of remote signers to specify their own implementation to verify if the remote signer is active.
The previous commits added the foundation for creating different types of remote signer connections and defined the RPC that an outbound remote signer would use to communicate with the watch-only node. We will now define the implementation that the outbound signer node will use to set up the stream to the watch-only node and process any sign request messages that the watch-only node sends to the signer node. This implementation is wrapped as the `RemoteSignerClient`. The `RemoteSignerClient` will make an outbound gRPC connection to the watch-only node to set up the stream between them and then process all requests that the watch-only node sends to the remote signer by passing them on to the respective `walletrpc.WalletKitServer` and `signrpc.SignerServer`. In the future, we may have more than one implementation of the remote signer client beyond just an outbound remote signer client, and we might then turn the `RemoteSignerClient` into a broader interface and rename this specific implementation to the `OutboundSignerClient`. Note once again that this is the implementation for the signer node side, not the watch-only node.
This commit adds a RemoteSignerClient instance to the main lnd server. The RemoteSignerClient will only fully start if it's enabled by the configuration, i.e. the `remotesigner.signertype` is set to `signer`. As we may have more than one implementation of the remote signer client beyond just an outbound remote signer client in the future, we create an remote signer client instance for any configuration.
As all the necessary pieces on the signer node side to let the remote signer make an outbound connection to the watch-only node are in place, we can now enable the signertype "signer" in the lncfg package. Note that we still haven't created the implementation on the watch-only node side to accept the connection and send the requests over the stream. This will be added in the upcoming commits.
This commit introduces an implementation for the watch-only node to send and receive messages over the `SignCoordinatorStreams` stream, which serves as the connection stream with an outbound remote signer. Previous commits added the `remoteSignerClient` implementation, defining the signer node's side of this functionality. The new implementation, called `SignCoordinator`, converts requests sent to the remote signer into the corresponding `SignCoordinatorStreams` request messages and transmits them over the stream. The requests we send to a remote signer are defined in the `RPCKeyRing` (`lnwallet/rpcwallet/rpcwallet.go`). When a response is received from the outbound remote signer, it is then converted back into the appropriate `walletrpc` or `signrpc` response. Additionally, the `SignCoordinator` includes functions to block and signal once the outbound remote signer has connected. Since requests cannot be processed before the outbound remote signer connects, any requests sent to the `SignCoordinator` will wait for the remote signer to connect before being processed.
As the previous commit implemented the foundation for the watch-only node to send and receive messages with an outbound remote signer (the `SignCoordinator` implementation), we can now wrap this implementation in the `RemoteSigner` interface, making it usable through the `RPCKeyRing`. This commit introduces the `OutboundRemoteSigner` implementation to achieve that.
To accept incoming connections from the remote signer and use the remote signer stream for any required signatures on the watch-only node, we must allow the connection from the remote signer before any signatures are needed. Currently, we only allow requests through the InterceptorChain into the rpc-servers after the WalletState has been set to RpcActive. This status is only set once the main RpcServer, along with all sub-servers, have been fully started and populated with their dependencies. The problem is that we need signatures from the remote signer to create some of the dependencies for the sub-servers. Because of this, we need to let the remote signer connect before all dependencies are created. To enable this, we add a new WalletState, AllowRemoteSigner, which allows connection requests from a remote signer to pass through the InterceptorChain when the AllowRemoteSigner state is set. This state is set before the RpcActive state.
Change the InterceptorChain behavior to allow a remote signer to call the walletrpc.SignCoordinatorStreams while the rpcState is set to allowRemoteSigner. This state precedes the rpcActive state, which allows all RPCs. This change is necessary because lnd needs the remote signer to be connected before some of the internal dependencies for RPC sub-servers can be created. These dependencies must be inserted into the sub-servers before moving the rpcState to rpcActive.
The SetServerActive moves the rpcState from rpcActive to serverActive. Update the docs to correctly reflect that.
To enable an outbound remote signer to connect to lnd before all dependencies for the RPC sub-servers are created, we need to separate the process of adding dependencies to the sub-servers from starting them. Prior to this commit, the RPC sub-servers could only be started once all dependencies were in place. This limitation prevented accepting an incoming connection request from an outbound remote signer (e.g., a walletrpc.SignCoordinatorStreams RPC call) to the WalletKitServer before it was started. Therefore, we need to start at least the WalletKitServer and the main RPC server before creating the rest of the dependencies. This commit refactors the logic for the main RPC server and sub-servers, allowing them to start before dependencies are inserted into the sub-servers. The WalletState for the InterceptorChain is only set to RpcActive after all dependencies have been created and inserted, ensuring that RPC requests won't be allowed into the sub-servers before the dependencies exist. An upcoming commit will set the state to AllowRemoteSigner before all dependencies are created, enabling an outbound remote signer to connect when needed.
This commit adds the `RemoteSigner` reference to the `WalletKit` `Config`, enabling it to be accessed from the `WalletKit` sub-server. When a remote signer connects by calling the `SignCoordinatorStreams` RPC endpoint, we need to pass the stream from the outbound remote signer to the `RemoteSigner` `Run` function. This change ensures that the `RemoteSigner` `Run` function is reachable from the `SignCoordinatorStreams` RPC endpoint implementation.
With the ability to reach the `RemoteSigner` `Run` function in the `WalletKit` sub-server, we now implement the `SignCoordinatorStreams` RPC endpoint.
This commit populates the `RemoteSigner` reference in the `WalletKit` config before other dependencies are added. To ensure that an outbound remote signer can connect before other dependencies are created, and since we use this reference in the walletrpc `SignCoordinatorStreams` RPC, we must populate this dependency prior to other dependencies during the lnd startup process.
Previous commits added functionality to handle the incoming connection from an outbound remote signer and ensured that the outbound remote signer could connect before any signatures from the remote signer are needed. However, one issue still remains: we need to ensure that we wait for the outbound remote signer to connect when starting lnd before executing any code that requires the remote signer to be connected. This commit adds a `ReadySignal` function to the `WalletController` that returns a channel, which will signal once the wallet is ready to be used. For an `OutboundRemoteSigner`, this channel will only signal once the outbound remote signer has connected. This can then be used to ensure that lnd waits for the outbound remote signer to connect during the startup process.
With the functionality in place to allow an outbound remote signer to connect before any signatures are needed and the ability to wait for this connection, this commit enables the functionality to wait for the remote signer to connect before proceeding with the startup process. This includes setting the `WalletState` in the `InterceptorChain` to `AllowRemoteSigner` before waiting for the outbound remote signer to connect.
With all the necessary components on the watch-only node side in place to support usage of an outbound remote signer, we can now enable the `outbound` signertype in the lncfg package. This commit also adds support for the `outbound` signertype in the `RemoteSignerBuilder`.
With support for the outbound remote signer now added, we update the documentation to detail how to enable the use of this new remote signer type.
Update the harness to allow creating a watch-only node without starting it. This is useful for tests that need to create a watch-only node prior to starting it, such as tests that use an outbound remote signer.
This commit fixes that word wrapping for the deriveCustomScopeAccounts function docs, and ensures that it wraps at 80 characters or less.
ViktorTigerstrom
force-pushed
the
2024-05-add-outbound-remote-signer
branch
from
b34174d
to
aabced7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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/ViktorTigerstrom/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.