From 229dddb4b0371869bb227dd3dd354d454113d9de Mon Sep 17 00:00:00 2001 From: Andrew Gouin Date: Thu, 6 Apr 2023 15:37:33 -0600 Subject: [PATCH] Fix single signer state load bug (#131) * Fix single signer state load bug * Don't create sign state in config init for single signer * Add unsupported warnings for single-signer mode * Use Fprintln to cmd stdout for single signer warning * Require accept-risk flag for single signer mode --- cmd/horcrux/cmd/config.go | 10 ++++------ cmd/horcrux/cmd/signer.go | 40 ++++++++++++++++++++++++++++++++++++++- docs/migrating.md | 18 ++++++++++-------- test/test_signer.go | 2 +- 4 files changed, 54 insertions(+), 16 deletions(-) diff --git a/cmd/horcrux/cmd/config.go b/cmd/horcrux/cmd/config.go index dfaa87b1..fe964334 100644 --- a/cmd/horcrux/cmd/config.go +++ b/cmd/horcrux/cmd/config.go @@ -139,13 +139,11 @@ func initCmd() *cobra.Command { return err } - // initialize state/{chainid}_priv_validator_state.json file - if _, err = signer.LoadOrCreateSignState(config.privValStateFile(cid)); err != nil { - return err - } - - // if node is a cosigner initialize state/{chainid}_priv_validator_state.json file + // if node is a cosigner initialize state files if cs { + if _, err = signer.LoadOrCreateSignState(config.privValStateFile(cid)); err != nil { + return err + } if _, err = signer.LoadOrCreateSignState(config.shareStateFile(cid)); err != nil { return err } diff --git a/cmd/horcrux/cmd/signer.go b/cmd/horcrux/cmd/signer.go index 7b1ebd07..62c5caf9 100644 --- a/cmd/horcrux/cmd/signer.go +++ b/cmd/horcrux/cmd/signer.go @@ -1,6 +1,7 @@ package cmd import ( + "fmt" "log" "os" @@ -12,6 +13,19 @@ import ( "github.com/tendermint/tendermint/types" ) +const ( + flagAcceptRisk = "accept-risk" + + singleSignerWarning = `@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@ +@ WARNING: SINGLE-SIGNER MODE SHOULD NOT BE USED FOR MAINNET! @ +@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@ +Horcrux single-signer mode does not give the level of improved +key security and fault tolerance that Horcrux MPC/cosigner mode +provides. While it is a simpler deployment configuration, +single-signer should only be used for experimentation +as it is not officially supported by Strangelove.` +) + func signerCmd() *cobra.Command { cmd := &cobra.Command{ Use: "signer", @@ -29,6 +43,13 @@ func startSignerCmd() *cobra.Command { Args: cobra.NoArgs, SilenceUsage: true, RunE: func(cmd *cobra.Command, args []string) (err error) { + fmt.Fprintln(cmd.OutOrStdout(), singleSignerWarning) + + acceptRisk, _ := cmd.Flags().GetBool(flagAcceptRisk) + if !acceptRisk { + panic(fmt.Errorf("risk not accepted. --accept-risk flag required to run single signer mode")) + } + if err = signer.RequireNotRunning(config.PidFile); err != nil { return err } @@ -62,8 +83,23 @@ func startSignerCmd() *cobra.Command { logger.Info("Tendermint Validator", "mode", cfg.Mode, "priv-key", cfg.PrivValKeyFile, "priv-state-dir", cfg.PrivValStateDir) + stateFile := config.privValStateFile(chainID) + + var val types.PrivValidator + + if _, err := os.Stat(stateFile); err != nil { + if !os.IsNotExist(err) { + panic(fmt.Errorf("failed to load state file: %s", stateFile)) + } + // The only scenario in which we want to initialize a new state file + // is when the state file does not exist. + val = privval.LoadFilePVEmptyState(cfg.PrivValKeyFile, stateFile) + } else { + val = privval.LoadFilePV(cfg.PrivValKeyFile, stateFile) + } + pv = &signer.PvGuard{ - PrivValidator: privval.LoadFilePVEmptyState(cfg.PrivValKeyFile, config.privValStateFile(chainID)), + PrivValidator: val, } pubkey, err := pv.GetPubKey() @@ -85,5 +121,7 @@ func startSignerCmd() *cobra.Command { }, } + cmd.Flags().Bool(flagAcceptRisk, false, "Single-signer-mode unsupported. Required to accept risk and proceed.") + return cmd } diff --git a/docs/migrating.md b/docs/migrating.md index c57b0018..7b432a7c 100644 --- a/docs/migrating.md +++ b/docs/migrating.md @@ -79,17 +79,19 @@ $ horcrux config init {my_chain_id} "tcp://10.168.0.2:1234" -c -p "tcp://10.168. $ horcrux config init {my_chain_id} "tcp://10.168.0.3:1234" -c -p "tcp://10.168.1.1:2222|1,tcp://10.168.1.2:2222|2" -l "tcp://10.168.1.3:2222" -t 2 --timeout 1500ms ``` -> **NOTE:** Note the node address (e.g. "tcp://10.168.0.1:1234") of each command. In this example, each horcrux node is communicating with a corresponding sentry. It is also possible to include a comma separated list of node addresses (e.g. "tcp://chain-node-1:1234,tcp://chain-node-2:1234", etc), allowing all horcrux nodes to communicate with all sentries. +> **Note** +> Note the node address (e.g. "tcp://10.168.0.1:1234") of each command. In this example, each horcrux node is communicating with a corresponding sentry. It is also possible to include a comma separated list of node addresses (e.g. "tcp://chain-node-1:1234,tcp://chain-node-2:1234", etc), allowing all horcrux nodes to communicate with all sentries. -> **NOTE:** The `-c` or `--cosigner` flag here says to configure the signer for cosigner operations. The signer can also be run in single signer configuration, if you want to do that don't pass `-c`, `-p` or `-t` or `--timeout`. +> **Warning** +> SINGLE-SIGNER MODE SHOULD NOT BE USED FOR MAINNET! Horcrux single-signer mode does not give the level of improved key security and fault tolerance that Horcrux MPC/cosigner mode provides. While it is a simpler deployment configuration, single-signer should only be used for experimentation as it is not officially supported by Strangelove. -> **NOTE:** The `-p` or `--peers` flag lets you set the addresses of the other signer nodes in the config. Two ports are required, the P2P port for RCP traffic, and the Raft port for key-value sharing. Note that each signer also has an index. This index corresponds to the shard of the private key it will sign with. Keeping the node names and the indexes the same helps avoid errors and allows you to work more quickly +#### Flags -> **NOTE:** The `-l` or `--listen` flag lets you set the listen address for the cosigner, which is used for communication between cosigners, Raft and GRPC. The DNS/IP used for this must be reachable by the other peers, i.e. do not use 0.0.0.0 for the hostname. - -> **NOTE:** The `-k` or `--keyfile` flag lets you set the file path for the private key share file if you would like to use a different path than `~/.horcrux/share.json`. - -> **NOTE:** The `--timeout` value defaults to `1000ms`. If you are running in disconnected data centers (i.e. across amazon AZs or gcp zones) increasing the timeout slightly helps to avoid missed blocks especially around proposals. +- `-c`/`--cosigner`: this flag instructs horcrux to configure the signer for MPC cosigner operations. This is the officially-supported configuration. The signer can also be run in single signer configuration for experimental, non-mainnet deployments. To enable single-signer mode, exclude the `-c`, `-p`, `-t`, and `--timeout` flags. +- `-p`/`--peers`: configures the addresses of the other signer nodes in the config. Two ports are required, the P2P port for RCP traffic, and the Raft port for key-value sharing. Note that each signer also has an index. This index corresponds to the shard of the private key it will sign with. Keeping the node names and the indexes the same helps avoid errors and allows you to work more quickly +- `-l`/`--listen`: configures the listen address for the cosigner, which is used for communication between cosigners, Raft and GRPC. The DNS/IP used for this must be reachable by the other peers, i.e. do not use 0.0.0.0 for the hostname. +- `-k`/`--keyfile`: configures the file path for the private key share file if you would like to use a different path than the default, `~/.horcrux/share.json`. +- `--timeout`: configures the timeout for cosigner-to-cosigner GRPC communication. This value defaults to `1000ms`. If you are running in disconnected data centers (i.e. across amazon AZs or gcp zones) increasing the timeout slightly helps to avoid missed blocks especially around proposals. ### 3. Split `priv_validator_key.json` and distribute key material diff --git a/test/test_signer.go b/test/test_signer.go index bd77b8ed..9961e9f6 100644 --- a/test/test_signer.go +++ b/test/test_signer.go @@ -436,7 +436,7 @@ func (ts *TestSigner) CreateSingleSignerContainer() error { Name: ts.Name(), Config: &docker.Config{ User: getDockerUserString(), - Cmd: []string{binary, "signer", "start", fmt.Sprintf("--home=%s", ts.Dir())}, + Cmd: []string{binary, "signer", "start", "--accept-risk", fmt.Sprintf("--home=%s", ts.Dir())}, Hostname: ts.Name(), ExposedPorts: map[docker.Port]struct{}{ docker.Port(signerPortDocker): {},