Add Validator component structure draft
#9464
Draft
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
Upcoming commits will introduce and enable a new alternative remote signer implementation. This new implementation establishes the connection between the remote signer and the watch-only node in the reverse direction compared to the existing implementation. In this setup, the remote signer initiates an outbound connection to the watch-only node, whereas the previous version allowed an inbound connection from the watch-only node. As a result, this type is referred to as an "outbound remote signer." This commit refactors the `RemoteSigner` configuration struct in preparation for the new implementation. The new implementation will require configuration changes on both the watch-only node side and the remote signer side. This refactor simplifies the process of integrating that functionality.
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.
This commit introduces a new macaroon entity that grants the caller access to connect a remote signer to LND, provided the LND instance is configured to allow an inbound connection from an outbound remote signer.
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.
RemoteSignerConnection is an interface that abstracts the communication with a remote signer. It extends the RemoteSignerRequests interface, and adds som additional methods to manage the connection and verify the health of the remote signer. As we'll add an outbound remote signer implementation in upcoming commits, we need this interface to abstract the commonalities of both the inbound (the current remote signer implementation) and outbound remote signer implementations, so that the RPCKeyRing doesn't need to know which type it's using.
This commit wraps the connection to the current remote signer implementation in the new `RemoteSignerConnection` interface within an `OutboundConnection` struct. The name `OutboundConnection` is chosen to reflect the fact that the watch-only node makes an outbound connection to the inbound remote signer in the current implementation, and this connection instance is used on the watch-only node's side.
`RemoteSignerConnectionBuilder` is a helper that creates instances of the RemoteSigner connection interface based on the `lncfg.RemoteSigner` config.
As the `RemoteSignerConnectionBuilder` can now create an `OutboundConnection` that matches the functionality of the previous remote signer communication implementation, we refactor the `rpcwallet` package to use a `RemoteSignerConnection` instance created by the `RemoteSignerConnectionBuilder`.
In preparation for using the `RemoteSignerConnection` interface for health checks, we refactor the `createLivenessMonitor` function to take a passed context param and to return an error.
With the `RPCKeyRing` now having the `RemoteSignerConnection` reference, we can use that reference to call the `Ping` implementation of the `RemoteSignerConnection` 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.
This commit introduces a new `watchonlynode` namespace to the `Config` struct. It is designed to be configured on nodes acting as remote signers in a remote signer setup and defines how the connection with the watch-only node is established. Note that enabling the watch-only node configuration is not yet supported in this commit and will be enabled in a future update.
CancelOrQuit is used to create a cancellable context that will be cancelled if the passed quit channel is signalled.
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 encapsulated in the `OutboundClient`. The `OutboundClient` establishes an outbound gRPC connection to the watch-only node to set up a stream between them. It then processes all requests sent by the watch-only node to the remote signer by forwarding them to the appropriate `walletrpc.WalletKitServer` and `signrpc.SignerServer`. An alternative implementation of `RemoteSignerClient`, called `NoOpClient`, is also provided. As the name implies, this client does not perform any operations. Note once again that this is the implementation for the signer node side, not the watch-only node.
Add a remote signer client builder that constructs either an outbound remote signer client or a No Op client, depending on the current configuration.
This commit adds a `RemoteSignerClient` instance to the main `lnd` server. The specific type of `RemoteSignerClient` used will depend on the configuration. If `watchonlynode.enable` is set to `true`, an `OutboundClient` instance will be used. Otherwise, a `NoOpClient` instance will be utilized.
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, the `lncfg` package can now permit the `watchonlynode.enable` setting to be configured. 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 configuration functionality on the watch-only node's side to support an inbound connection from an outbound remote signer. Note that enabling this option is not supported in this commit, but the ability to toggle it will be added in an upcoming commit.
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 `OutboundClient` implementation, defining the signer node's side of this functionality. The new implementation, called `SignCoordinator`, converts requests sent to the remote signer connection on the watch-only node side into the corresponding `SignCoordinatorStreams` request messages and transmits them over the stream. The requests we send to a remote signer are defined in the `RemoteSignerRequests` interface. 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 connection to the outbound remote signer is established, 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 the communication in the `RemoteSignerConnection` interface, making it usable through the `RPCKeyRing`. This commit introduces the `InboundConnection` 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 created the sub-servers. Prior to this commit, the RPC sub-servers were created and enabled only after all dependencies were in place. Such a limitation prevents accepting an incoming connection request from an outbound remote signer (e.g., a `walletrpc.SignCoordinatorStreams` RPC call) to the `WalletKitServer` until all dependencies for the RPC sub-servers are created. However, this limitation would not work, as we need the remote signer in order to create some of the dependencies for the other RPC sub-servers. Therefore, we need to enable calls to at least the `WalletKitServer` and the main RPC server before creating the remaining dependencies. This commit refactors the logic for the main RPC server and sub-servers, allowing them to be enabled 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 `RemoteSignerConnection` 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 `InboundRemoteSignerConnection` `AddConnection` function. This change ensures that the `InboundRemoteSignerConnection` `AddConnection` function is reachable from the `SignCoordinatorStreams` RPC endpoint implementation. Note that this field should only to be populated when the `RPCKeyRing` `RemoteSignerConnection` is an `InboundConnection` instance, as the only that connection type implements the `InboundRemoteSignerConnection` interface.
With the ability to reach the `InboundRemoteSignerConnection` `AddConnection` function in the `WalletKit` sub-server, we now implement the `SignCoordinatorStreams` RPC endpoint.
This commit populates the `RemoteSignerConnection` 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.
This commit populates the `RemoteSignerConnection` 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 `InboundConnection`, 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, the `lncfg` package can now permit the `remotesigner.allowinboundconnection` setting to be configured. This commit also adds support for the building `InboundConnection` instances in the `RemoteSignerConnectionBuilder`.
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 release notes to include information about the support for the new outbound 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.
testOutboundRSMacaroonEnforcement tests that a valid macaroon including the `remotesigner` entity is required to connect to a watch-only node that uses an outbound remote signer, while the watch-only node is in the state (WalletState_ALLOW_REMOTE_SIGNER) where it waits for the signer to connect.
This commit fixes that word wrapping for the deriveCustomScopeAccounts function docs, and ensures that it wraps at 80 characters or less.
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ViktorTigerstrom
changed the title
Validator component structure Validator component structure draft
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.
Based on #8754
This PR introduces the initial structure for remote signing validation. It defines a
Validatorcomponent that the remote signer client will use to processPSBTrequests before signing them. TheValidatorwill evaluate each request and determine whether it should be accepted or rejected, ultimately deciding if the request gets signed.The specific
Validatorimplementation will be configurable, allowing for different levels of validation and alternative remote signing validation implementations based on the configuration.The last commit includes pseudo code for one such validation implementation.
Note: This is a very early draft, and by no means a functional implementation at this stage.