Skip to content

Commit

Permalink
Threshold parameter validation (#112)
Browse files Browse the repository at this point in the history 
* Add assertions for t > n/2 for both key sharding and cosigner daemon start

* Show container logs for failed tests

* Update tests

* Fix set state test

* Prefer t.Setenv in tests. Fix error message
  • Loading branch information
@agouin
agouin authored Oct 21, 2022
1 parent c24ca49 commit 8b0803d
Show file tree
Hide file tree
Showing 5 changed files with 130 additions and 88 deletions.
21 changes: 14 additions & 7 deletions cmd/horcrux/cmd/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,9 +197,13 @@ func validateCosignerConfig(cfg DiskConfig) error {
if cfg.CosignerConfig == nil {
return fmt.Errorf("cosigner config can't be empty")
}
if len(cfg.CosignerConfig.Peers)+1 < cfg.CosignerConfig.Threshold {
return fmt.Errorf("number of peers + 1 (%d) must be greater than threshold (%d)",
len(cfg.CosignerConfig.Peers)+1, cfg.CosignerConfig.Threshold)
if cfg.CosignerConfig.Threshold <= cfg.CosignerConfig.Shares/2 {
return fmt.Errorf("threshold (%d) must be greater than number of shares (%d) / 2",
cfg.CosignerConfig.Threshold, cfg.CosignerConfig.Shares)
}
if cfg.CosignerConfig.Shares < cfg.CosignerConfig.Threshold {
return fmt.Errorf("number of shares (%d) must be greater or equal to threshold (%d)",
cfg.CosignerConfig.Shares, cfg.CosignerConfig.Threshold)
}

_, err := time.ParseDuration(cfg.CosignerConfig.Timeout)
Expand Down Expand Up @@ -335,6 +339,7 @@ func addPeersCmd() *cobra.Command {
return errors.New("no new peer nodes in args")
}
diff = append(config.Config.CosignerConfig.Peers, diff...)
config.Config.CosignerConfig.Shares = len(diff) + 1
if err := validateCosignerPeers(diff, config.Config.CosignerConfig.Shares); err != nil {
return err
}
Expand Down Expand Up @@ -378,6 +383,8 @@ func removePeersCmd() *cobra.Command {
if len(diff) == 0 {
return errors.New("cannot remove all peer nodes from config, please leave at least one")
}

config.Config.CosignerConfig.Shares = len(diff) + 1
// If none of the peer nodes in the args are listed in the config, just continue
// without throwing an error, as the peer nodes in the config remain untouched.
if err := validateCosignerPeers(diff, config.Config.CosignerConfig.Shares); err != nil {
Expand Down Expand Up @@ -584,11 +591,11 @@ func validateCosignerPeers(peers []CosignerPeer, shares int) error {
}
}

// Check that no more than {num-shares}-1 peers are in the peer list, assuming
// Check that exactly {num-shares}-1 peers are in the peer list, assuming
// the remaining peer ID is the ID the local node is configured with.
if len(peers) == shares {
return fmt.Errorf("too many peers (%v+local node = %v) for the specified number of key shares (%v)",
len(peers), len(peers)+1, shares)
if len(peers) != shares-1 {
return fmt.Errorf("incorrect number of peers. expected (%d shares - local node = %d peers)",
shares, shares-1)
}
return nil
}
Expand Down
80 changes: 17 additions & 63 deletions cmd/horcrux/cmd/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const (
)

func TestConfigInitCmd(t *testing.T) {
tmpHome := "/tmp/TestConfigInitCmd"
tmpHome := t.TempDir()
tcs := []struct {
name string
home string
Expand Down Expand Up @@ -71,9 +71,8 @@ func TestConfigInitCmd(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
tmpConfig := filepath.Join(tc.home, ".horcrux")

err := os.Setenv("HOME", tc.home)
require.NoError(t, err)
err = os.MkdirAll(tc.home, 0777)
t.Setenv("HOME", tc.home)
err := os.MkdirAll(tc.home, 0777)
require.NoError(t, err)

cmd := initCmd()
Expand Down Expand Up @@ -106,24 +105,10 @@ func TestConfigInitCmd(t *testing.T) {
}
})
}

t.Cleanup(func() {
files, err := filepath.Glob(tmpHome + "*")
require.NoError(t, err)

for _, file := range files {
os.RemoveAll(file)
}
})
}

func TestConfigChainIDSetCmd(t *testing.T) {
tmpHome := "/tmp/TestConfigChainIDSetCmd"

err := os.Setenv("HOME", tmpHome)
require.NoError(t, err)
err = os.MkdirAll(tmpHome, 0777)
require.NoError(t, err)
t.Setenv("HOME", t.TempDir())

cmd := initCmd()
cmd.SetOutput(io.Discard)
Expand All @@ -136,7 +121,7 @@ func TestConfigChainIDSetCmd(t *testing.T) {
"-l", "tcp://10.168.1.1:2222",
"--timeout", "1500ms",
})
err = cmd.Execute()
err := cmd.Execute()
require.NoError(t, err)

tcs := []struct {
Expand Down Expand Up @@ -171,19 +156,10 @@ func TestConfigChainIDSetCmd(t *testing.T) {
}
})
}

t.Cleanup(func() {
os.RemoveAll(tmpHome)
})
}

func TestConfigNodesAddAndRemove(t *testing.T) {
tmpHome := "/tmp/TestConfigNodesAddAndRemove"

err := os.Setenv("HOME", tmpHome)
require.NoError(t, err)
err = os.MkdirAll(tmpHome, 0777)
require.NoError(t, err)
t.Setenv("HOME", t.TempDir())

cmd := initCmd()
cmd.SetOutput(io.Discard)
Expand All @@ -196,7 +172,7 @@ func TestConfigNodesAddAndRemove(t *testing.T) {
"-l", "tcp://10.168.1.1:2222",
"--timeout", "1500ms",
})
err = cmd.Execute()
err := cmd.Execute()
require.NoError(t, err)

tcs := []struct {
Expand Down Expand Up @@ -317,19 +293,10 @@ func TestConfigNodesAddAndRemove(t *testing.T) {
require.Equal(t, tc.expectNodes, config.Config.ChainNodes)
})
}

t.Cleanup(func() {
os.RemoveAll(tmpHome)
})
}

func TestConfigPeersAddAndRemove(t *testing.T) {
tmpHome := "/tmp/TestConfigPeersAddAndRemove"

err := os.Setenv("HOME", tmpHome)
require.NoError(t, err)
err = os.MkdirAll(tmpHome, 0777)
require.NoError(t, err)
t.Setenv("HOME", t.TempDir())

cmd := initCmd()
cmd.SetOutput(io.Discard)
Expand All @@ -338,11 +305,11 @@ func TestConfigPeersAddAndRemove(t *testing.T) {
"tcp://10.168.0.1:1234",
"-c",
"-p", "tcp://10.168.1.2:2222|2,tcp://10.168.1.3:2222|3,tcp://10.168.1.4:2222|4",
"-t", "2",
"-t", "3",
"-l", "tcp://10.168.1.1:2222",
"--timeout", "1500ms",
})
err = cmd.Execute()
err := cmd.Execute()
require.NoError(t, err)

tcs := []struct {
Expand Down Expand Up @@ -439,7 +406,7 @@ func TestConfigPeersAddAndRemove(t *testing.T) {
{
name: "add peer with ID out of range",
cmd: addPeersCmd(),
args: []string{"tcp://10.168.1.5:2222|5"},
args: []string{"tcp://10.168.1.5:2222|6"},
expectPeers: []CosignerPeer{
{ShareID: 2, P2PAddr: "tcp://10.168.1.2:2222"},
{ShareID: 3, P2PAddr: "tcp://10.168.1.3:2222"},
Expand All @@ -464,10 +431,6 @@ func TestConfigPeersAddAndRemove(t *testing.T) {
require.Equal(t, tc.expectPeers, config.Config.CosignerConfig.Peers)
})
}

t.Cleanup(func() {
os.RemoveAll(tmpHome)
})
}

func TestDiffSetChainNode(t *testing.T) {
Expand Down Expand Up @@ -603,12 +566,7 @@ func TestDiffSetCosignerPeer(t *testing.T) {
}

func TestSetShares(t *testing.T) {
tmpHome := "/tmp/TestSetShares"

err := os.Setenv("HOME", tmpHome)
require.NoError(t, err)
err = os.MkdirAll(tmpHome, 0777)
require.NoError(t, err)
t.Setenv("HOME", t.TempDir())

cmd := initCmd()
cmd.SetOutput(io.Discard)
Expand All @@ -621,7 +579,7 @@ func TestSetShares(t *testing.T) {
"-l", "tcp://10.168.1.1:2222",
"--timeout", "1500ms",
})
err = cmd.Execute()
err := cmd.Execute()
require.NoError(t, err)

tcs := []struct {
Expand All @@ -632,20 +590,20 @@ func TestSetShares(t *testing.T) {
}{ // Do NOT change the order of the test cases!
{
name: "valid number of shares",
args: []string{"4"},
expectShares: 4,
args: []string{"3"},
expectShares: 3,
expectErr: false,
},
{
name: "too few shares for number of peers",
args: []string{"1"},
expectShares: 4,
expectShares: 3,
expectErr: true,
},
{
name: "invalid number of shares",
args: []string{"-1"},
expectShares: 4,
expectShares: 3,
expectErr: true,
},
}
Expand All @@ -666,8 +624,4 @@ func TestSetShares(t *testing.T) {
}
})
}

t.Cleanup(func() {
os.RemoveAll(tmpHome)
})
}
31 changes: 24 additions & 7 deletions cmd/horcrux/cmd/key2shares.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,17 @@ func CreateCosignerSharesCmd() *cobra.Command {
Args: validateCreateCosignerShares,
Short: "Create cosigner shares",
RunE: func(cmd *cobra.Command, args []string) (err error) {
threshold, _ := strconv.ParseInt(args[1], 10, 64)
numShares, _ := strconv.ParseInt(args[2], 10, 64)
threshold, shares := args[1], args[2]
t, err := strconv.ParseInt(threshold, 10, 64)
if err != nil {
return fmt.Errorf("error parsing threshold (%s): %w", threshold, err)
}
n, err := strconv.ParseInt(shares, 10, 64)
if err != nil {
return fmt.Errorf("error parsing shares (%s): %w", shares, err)
}

csKeys, err := signer.CreateCosignerSharesFromFile(args[0], threshold, numShares)
csKeys, err := signer.CreateCosignerSharesFromFile(args[0], t, n)
if err != nil {
return err
}
Expand All @@ -66,11 +73,21 @@ func validateCreateCosignerShares(cmd *cobra.Command, args []string) error {
if !os.FileExists(args[0]) {
return fmt.Errorf("priv_validator.json file(%s) doesn't exist", args[0])
}
if _, err := strconv.ParseInt(args[1], 10, 64); err != nil {
return fmt.Errorf("shards must be an integer got(%s)", args[1])
threshold, shares := args[1], args[2]
t, err := strconv.ParseInt(threshold, 10, 64)
if err != nil {
return fmt.Errorf("error parsing threshold (%s): %w", threshold, err)
}
n, err := strconv.ParseInt(shares, 10, 64)
if err != nil {
return fmt.Errorf("error parsing shares (%s): %w", shares, err)
}
if t > n {
return fmt.Errorf("threshold cannot be greater than total shares, got [threshold](%d) > [shares](%d)", t, n)
}
if _, err := strconv.ParseInt(args[2], 10, 64); err != nil {
return fmt.Errorf("threshold must be an integer got(%s)", args[2])
if t <= n/2 {
return fmt.Errorf("threshold must be greater than total shares "+
"divided by 2, got [threshold](%d) <= [shares](%d) / 2", t, n)
}
return nil
}
71 changes: 71 additions & 0 deletions cmd/horcrux/cmd/key2shares_test.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
package cmd

import (
"io"
"path/filepath"
"testing"

"github.com/stretchr/testify/require"
"github.com/tendermint/tendermint/crypto/ed25519"
"github.com/tendermint/tendermint/privval"
)

func TestKey2Shares(t *testing.T) {
tmp := t.TempDir()

privValidatorKeyFile := filepath.Join(tmp, "priv_validator_key.json")
privValidatorStateFile := filepath.Join(tmp, "priv_validator_state.json")
pv := privval.NewFilePV(ed25519.GenPrivKey(), privValidatorKeyFile, privValidatorStateFile)
pv.Save()

tcs := []struct {
name string
args []string
expectErr bool
}{
{
name: "valid threshold and shares",
args: []string{privValidatorKeyFile, "2", "3"},
expectErr: false,
},
{
name: "valid threshold and shares 2",
args: []string{privValidatorKeyFile, "3", "5"},
expectErr: false,
},
{
name: "threshold exactly half of shares",
args: []string{privValidatorKeyFile, "2", "4"},
expectErr: true,
},
{
name: "threshold less than half of shares",
args: []string{privValidatorKeyFile, "1", "3"},
expectErr: true,
},
{
name: "threshold exceeds shares",
args: []string{privValidatorKeyFile, "4", "3"},
expectErr: true,
},
{
name: "non-numeric threshold and shares",
args: []string{privValidatorKeyFile, "two", "three"},
expectErr: true,
},
}

for _, tc := range tcs {
t.Run(tc.name, func(t *testing.T) {
cmd := CreateCosignerSharesCmd()
cmd.SetOutput(io.Discard)
cmd.SetArgs(tc.args)
err := cmd.Execute()
if tc.expectErr {
require.Error(t, err)
} else {
require.NoError(t, err)
}
})
}
}
Loading

0 comments on commit 8b0803d

Please sign in to comment.