From 2a292efb56e252a00e67b55c1e7faf574cdeeea1 Mon Sep 17 00:00:00 2001 From: Alexander Simmerl Date: Tue, 13 Feb 2018 19:34:50 +0100 Subject: [PATCH] Return error for all PrivValidator methoods As calls to the private validator can involve side-effects like network communication it is desirable for all methods returning an error to not break the control flow of the caller. * adjust PrivValidator interface --- types/priv_validator.go | 4 +- types/priv_validator/json.go | 19 ++++++- types/priv_validator/priv_validator_test.go | 57 +++++++++++++++------ types/priv_validator/socket.go | 22 +++++--- types/priv_validator/socket_test.go | 35 +++++++++++-- types/priv_validator/unencrypted.go | 15 ++++-- 6 files changed, 114 insertions(+), 38 deletions(-) diff --git a/types/priv_validator.go b/types/priv_validator.go index 4038b1adf..237e3f796 100644 --- a/types/priv_validator.go +++ b/types/priv_validator.go @@ -46,8 +46,8 @@ type ValidatorID struct { // PrivValidator defines the functionality of a local Tendermint validator // that signs votes, proposals, and heartbeats, and never double signs. type PrivValidator2 interface { - Address() Address // redundant since .PubKey().Address() - PubKey() crypto.PubKey + Address() (Address, error) // redundant since .PubKey().Address() + PubKey() (crypto.PubKey, error) SignVote(chainID string, vote *Vote) error SignProposal(chainID string, proposal *Proposal) error diff --git a/types/priv_validator/json.go b/types/priv_validator/json.go index 8fe7a3749..6e5261386 100644 --- a/types/priv_validator/json.go +++ b/types/priv_validator/json.go @@ -76,7 +76,12 @@ func (pvj *PrivValidatorJSON) SignProposal(chainID string, proposal *types.Propo // String returns a string representation of the PrivValidatorJSON. func (pvj *PrivValidatorJSON) String() string { - return fmt.Sprintf("PrivValidator{%v %v}", pvj.Address(), pvj.PrivValidatorUnencrypted.String()) + addr, err := pvj.Address() + if err != nil { + panic(err) + } + + return fmt.Sprintf("PrivValidator{%v %v}", addr, pvj.PrivValidatorUnencrypted.String()) } func (pvj *PrivValidatorJSON) Save() { @@ -170,7 +175,17 @@ func (pvs PrivValidatorsByAddress) Len() int { } func (pvs PrivValidatorsByAddress) Less(i, j int) bool { - return bytes.Compare(pvs[i].Address(), pvs[j].Address()) == -1 + iaddr, err := pvs[j].Address() + if err != nil { + panic(err) + } + + jaddr, err := pvs[i].Address() + if err != nil { + panic(err) + } + + return bytes.Compare(iaddr, jaddr) == -1 } func (pvs PrivValidatorsByAddress) Swap(i, j int) { diff --git a/types/priv_validator/priv_validator_test.go b/types/priv_validator/priv_validator_test.go index 6f68256da..664d59cf2 100644 --- a/types/priv_validator/priv_validator_test.go +++ b/types/priv_validator/priv_validator_test.go @@ -17,7 +17,7 @@ import ( ) func TestGenLoadValidator(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) _, tempFilePath := cmn.Tempfile("priv_validator_") privVal := GenPrivValidatorJSON(tempFilePath) @@ -25,24 +25,33 @@ func TestGenLoadValidator(t *testing.T) { height := int64(100) privVal.LastSignedInfo.Height = height privVal.Save() - addr := privVal.Address() + addr, err := privVal.Address() + require.Nil(err) privVal = LoadPrivValidatorJSON(tempFilePath) - assert.Equal(addr, privVal.Address(), "expected privval addr to be the same") + pAddr, err := privVal.Address() + require.Nil(err) + + assert.Equal(addr, pAddr, "expected privval addr to be the same") assert.Equal(height, privVal.LastSignedInfo.Height, "expected privval.LastHeight to have been saved") } func TestLoadOrGenValidator(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) _, tempFilePath := cmn.Tempfile("priv_validator_") if err := os.Remove(tempFilePath); err != nil { t.Error(err) } privVal := LoadOrGenPrivValidatorJSON(tempFilePath) - addr := privVal.Address() + addr, err := privVal.Address() + require.Nil(err) + privVal = LoadOrGenPrivValidatorJSON(tempFilePath) - assert.Equal(addr, privVal.Address(), "expected privval addr to be the same") + pAddr, err := privVal.Address() + require.Nil(err) + + assert.Equal(addr, pAddr, "expected privval addr to be the same") } func TestUnmarshalValidator(t *testing.T) { @@ -87,8 +96,14 @@ func TestUnmarshalValidator(t *testing.T) { require.Nil(err, "%+v", err) // make sure the values match - assert.EqualValues(addrBytes, val.Address()) - assert.EqualValues(pubKey, val.PubKey()) + vAddr, err := val.Address() + require.Nil(err) + + pKey, err := val.PubKey() + require.Nil(err) + + assert.EqualValues(addrBytes, vAddr) + assert.EqualValues(pubKey, pKey) assert.EqualValues(privKey, val.PrivKey) // export it and make sure it is the same @@ -98,7 +113,7 @@ func TestUnmarshalValidator(t *testing.T) { } func TestSignVote(t *testing.T) { - assert := assert.New(t) + assert, require := assert.New(t), require.New(t) _, tempFilePath := cmn.Tempfile("priv_validator_") privVal := GenPrivValidatorJSON(tempFilePath) @@ -109,8 +124,11 @@ func TestSignVote(t *testing.T) { voteType := types.VoteTypePrevote // sign a vote for first time - vote := newVote(privVal.Address(), 0, height, round, voteType, block1) - err := privVal.SignVote("mychainid", vote) + addr, err := privVal.Address() + require.Nil(err) + + vote := newVote(addr, 0, height, round, voteType, block1) + err = privVal.SignVote("mychainid", vote) assert.NoError(err, "expected no error signing vote") // try to sign the same vote again; should be fine @@ -119,10 +137,10 @@ func TestSignVote(t *testing.T) { // now try some bad votes cases := []*types.Vote{ - newVote(privVal.Address(), 0, height, round-1, voteType, block1), // round regression - newVote(privVal.Address(), 0, height-1, round, voteType, block1), // height regression - newVote(privVal.Address(), 0, height-2, round+4, voteType, block1), // height regression and different round - newVote(privVal.Address(), 0, height, round, voteType, block2), // different block + newVote(addr, 0, height, round-1, voteType, block1), // round regression + newVote(addr, 0, height-1, round, voteType, block1), // height regression + newVote(addr, 0, height-2, round+4, voteType, block1), // height regression and different round + newVote(addr, 0, height, round, voteType, block2), // different block } for _, c := range cases { @@ -179,6 +197,8 @@ func TestSignProposal(t *testing.T) { } func TestDifferByTimestamp(t *testing.T) { + require := require.New(t) + _, tempFilePath := cmn.Tempfile("priv_validator_") privVal := GenPrivValidatorJSON(tempFilePath) @@ -208,10 +228,13 @@ func TestDifferByTimestamp(t *testing.T) { // test vote { + addr, err := privVal.Address() + require.Nil(err) + voteType := types.VoteTypePrevote blockID := types.BlockID{[]byte{1, 2, 3}, types.PartSetHeader{}} - vote := newVote(privVal.Address(), 0, height, round, voteType, blockID) - err := privVal.SignVote("mychainid", vote) + vote := newVote(addr, 0, height, round, voteType, blockID) + err = privVal.SignVote("mychainid", vote) assert.NoError(t, err, "expected no error signing vote") signBytes := types.SignBytes(chainID, vote) diff --git a/types/priv_validator/socket.go b/types/priv_validator/socket.go index 1ca3b7d1b..ae94371a9 100644 --- a/types/priv_validator/socket.go +++ b/types/priv_validator/socket.go @@ -86,23 +86,28 @@ func (pvsc *PrivValidatorSocketClient) OnStop() { } // Address is an alias for PubKey().Address(). -func (pvsc *PrivValidatorSocketClient) Address() cmn.HexBytes { - return pvsc.PubKey().Address() +func (pvsc *PrivValidatorSocketClient) Address() (cmn.HexBytes, error) { + p, err := pvsc.PubKey() + if err != nil { + return nil, err + } + + return p.Address(), nil } // PubKey implements PrivValidator. -func (pvsc *PrivValidatorSocketClient) PubKey() crypto.PubKey { +func (pvsc *PrivValidatorSocketClient) PubKey() (crypto.PubKey, error) { err := writeMsg(pvsc.conn, &PubKeyMsg{}) if err != nil { - panic(err) + return crypto.PubKey{}, err } res, err := readMsg(pvsc.conn) if err != nil { - panic(err) + return crypto.PubKey{}, err } - return res.(*PubKeyMsg).PubKey + return res.(*PubKeyMsg).PubKey, nil } // SignVote implements PrivValidator. @@ -316,7 +321,10 @@ func (pvss *PrivValidatorSocketServer) handleConnection(conn net.Conn) { switch r := req.(type) { case *PubKeyMsg: - res = &PubKeyMsg{pvss.privVal.PubKey()} + var p crypto.PubKey + + p, err = pvss.privVal.PubKey() + res = &PubKeyMsg{p} case *SignVoteMsg: err = pvss.privVal.SignVote(pvss.chainID, r.Vote) res = &SignVoteMsg{r.Vote} diff --git a/types/priv_validator/socket_test.go b/types/priv_validator/socket_test.go index 627d4fb18..9a290abb4 100644 --- a/types/priv_validator/socket_test.go +++ b/types/priv_validator/socket_test.go @@ -8,7 +8,6 @@ import ( "github.com/stretchr/testify/require" crypto "github.com/tendermint/go-crypto" - cmn "github.com/tendermint/tmlibs/common" "github.com/tendermint/tmlibs/log" "github.com/tendermint/tendermint/types" @@ -51,8 +50,21 @@ func TestPrivValidatorSocketServer(t *testing.T) { assert.True(pvsc.IsRunning()) - assert.Equal(pvsc.Address(), cmn.HexBytes(pvss.privVal.PubKey().Address())) - assert.Equal(pvsc.PubKey(), pvss.privVal.PubKey()) + cAddr, err := pvsc.Address() + require.Nil(err) + + sAddr, err := pvss.privVal.Address() + require.Nil(err) + + assert.Equal(cAddr, sAddr) + + cKey, err := pvsc.PubKey() + require.Nil(err) + + sKey, err := pvss.privVal.PubKey() + require.Nil(err) + + assert.Equal(cKey, sKey) err = pvsc.SignProposal(chainID, &types.Proposal{ Timestamp: time.Now(), @@ -104,8 +116,21 @@ func TestPrivValidatorSocketServerWithoutSecret(t *testing.T) { assert.True(pvsc.IsRunning()) - assert.Equal(pvsc.Address(), cmn.HexBytes(pvss.privVal.PubKey().Address())) - assert.Equal(pvsc.PubKey(), pvss.privVal.PubKey()) + cAddr, err := pvsc.Address() + require.Nil(err) + + sAddr, err := pvss.privVal.Address() + require.Nil(err) + + assert.Equal(cAddr, sAddr) + + cKey, err := pvsc.PubKey() + require.Nil(err) + + sKey, err := pvss.privVal.PubKey() + require.Nil(err) + + assert.Equal(cKey, sKey) err = pvsc.SignProposal(chainID, &types.Proposal{ Timestamp: time.Now(), diff --git a/types/priv_validator/unencrypted.go b/types/priv_validator/unencrypted.go index 483f57c11..3ef38eeca 100644 --- a/types/priv_validator/unencrypted.go +++ b/types/priv_validator/unencrypted.go @@ -35,15 +35,20 @@ func NewPrivValidatorUnencrypted(priv crypto.PrivKey) *PrivValidatorUnencrypted // String returns a string representation of the PrivValidatorUnencrypted func (upv *PrivValidatorUnencrypted) String() string { - return fmt.Sprintf("PrivValidator{%v %v}", upv.Address(), upv.LastSignedInfo.String()) + addr, err := upv.Address() + if err != nil { + panic(err) + } + + return fmt.Sprintf("PrivValidator{%v %v}", addr, upv.LastSignedInfo.String()) } -func (upv *PrivValidatorUnencrypted) Address() cmn.HexBytes { - return upv.PrivKey.PubKey().Address() +func (upv *PrivValidatorUnencrypted) Address() (cmn.HexBytes, error) { + return upv.PrivKey.PubKey().Address(), nil } -func (upv *PrivValidatorUnencrypted) PubKey() crypto.PubKey { - return upv.PrivKey.PubKey() +func (upv *PrivValidatorUnencrypted) PubKey() (crypto.PubKey, error) { + return upv.PrivKey.PubKey(), nil } func (upv *PrivValidatorUnencrypted) SignVote(chainID string, vote *types.Vote) error {