From a1cc9ac642ffe25e66565faac30065327b7ea4b9 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Wed, 20 Dec 2017 14:18:15 -0600 Subject: [PATCH 1/3] priv validator returns last sign bytes if h/r/s matches since now we have time in the msgs and we might crash between writing the priv val and writing to wal. Refs #984 --- types/priv_validator.go | 38 ++++++++++++++++-------------------- types/priv_validator_test.go | 4 ---- 2 files changed, 17 insertions(+), 25 deletions(-) diff --git a/types/priv_validator.go b/types/priv_validator.go index 5dfd521f4..5f4507e43 100644 --- a/types/priv_validator.go +++ b/types/priv_validator.go @@ -193,8 +193,8 @@ func (privVal *PrivValidatorFS) Reset() { privVal.Save() } -// SignVote signs a canonical representation of the vote, along with the chainID. -// Implements PrivValidator. +// SignVote signs a canonical representation of the vote, along with the +// chainID. Implements PrivValidator. func (privVal *PrivValidatorFS) SignVote(chainID string, vote *Vote) error { privVal.mtx.Lock() defer privVal.mtx.Unlock() @@ -206,8 +206,8 @@ func (privVal *PrivValidatorFS) SignVote(chainID string, vote *Vote) error { return nil } -// SignProposal signs a canonical representation of the proposal, along with the chainID. -// Implements PrivValidator. +// SignProposal signs a canonical representation of the proposal, along with +// the chainID. Implements PrivValidator. func (privVal *PrivValidatorFS) SignProposal(chainID string, proposal *Proposal) error { privVal.mtx.Lock() defer privVal.mtx.Unlock() @@ -219,40 +219,36 @@ func (privVal *PrivValidatorFS) SignProposal(chainID string, proposal *Proposal) return nil } -// signBytesHRS signs the given signBytes if the height/round/step (HRS) -// are greater than the latest state. If the HRS are equal, -// it returns the privValidator.LastSignature. +// signBytesHRS signs the given signBytes if the height/round/step (HRS) are +// greater than the latest state. If the HRS are equal, it returns the +// privValidator.LastSignature. func (privVal *PrivValidatorFS) signBytesHRS(height int64, round int, step int8, signBytes []byte) (crypto.Signature, error) { - sig := crypto.Signature{} - // If height regression, err + if privVal.LastHeight > height { return sig, errors.New("Height regression") } - // More cases for when the height matches + if privVal.LastHeight == height { - // If round regression, err if privVal.LastRound > round { return sig, errors.New("Round regression") } - // If step regression, err + if privVal.LastRound == round { if privVal.LastStep > step { return sig, errors.New("Step regression") } else if privVal.LastStep == step { if privVal.LastSignBytes != nil { if privVal.LastSignature.Empty() { - cmn.PanicSanity("privVal: LastSignature is nil but LastSignBytes is not!") - } - // so we dont sign a conflicting vote or proposal - // NOTE: proposals are non-deterministic (include time), - // so we can actually lose them, but will still never sign conflicting ones - if bytes.Equal(privVal.LastSignBytes, signBytes) { - // log.Notice("Using privVal.LastSignature", "sig", privVal.LastSignature) - return privVal.LastSignature, nil + panic("privVal: LastSignature is nil but LastSignBytes is not!") } + // NOTE: we might crash between writing the priv val and writing to + // wal. since we have time in votes and proposals, signBytes can be + // different from LastSignBytes. we might be signing conflicting + // bytes here. + return privVal.LastSignature, nil } - return sig, errors.New("Step regression") + return sig, errors.New("No LastSignature found") } } } diff --git a/types/priv_validator_test.go b/types/priv_validator_test.go index 4e55fd49b..c14773c51 100644 --- a/types/priv_validator_test.go +++ b/types/priv_validator_test.go @@ -99,7 +99,6 @@ func TestSignVote(t *testing.T) { privVal := GenPrivValidatorFS(tempFilePath) block1 := BlockID{[]byte{1, 2, 3}, PartSetHeader{}} - block2 := BlockID{[]byte{3, 2, 1}, PartSetHeader{}} height, round := int64(10), 1 voteType := VoteTypePrevote @@ -117,7 +116,6 @@ func TestSignVote(t *testing.T) { 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 } for _, c := range cases { @@ -133,7 +131,6 @@ func TestSignProposal(t *testing.T) { privVal := GenPrivValidatorFS(tempFilePath) block1 := PartSetHeader{5, []byte{1, 2, 3}} - block2 := PartSetHeader{10, []byte{3, 2, 1}} height, round := int64(10), 1 // sign a proposal for first time @@ -150,7 +147,6 @@ func TestSignProposal(t *testing.T) { newProposal(height, round-1, block1), // round regression newProposal(height-1, round, block1), // height regression newProposal(height-2, round+4, block1), // height regression and different round - newProposal(height, round, block2), // different block } for _, c := range cases { From 9c03c58de2c7cb0da0598905b98fb45002a22c70 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Thu, 21 Dec 2017 15:37:27 -0500 Subject: [PATCH 2/3] priv validator checks if only difference is timestamp; else error --- types/priv_validator.go | 112 ++++++++++++++++++++++++++++------- types/priv_validator_test.go | 18 ++++++ 2 files changed, 108 insertions(+), 22 deletions(-) diff --git a/types/priv_validator.go b/types/priv_validator.go index 5f4507e43..0e3050e51 100644 --- a/types/priv_validator.go +++ b/types/priv_validator.go @@ -8,6 +8,7 @@ import ( "io/ioutil" "os" "sync" + "time" crypto "github.com/tendermint/go-crypto" data "github.com/tendermint/go-wire/data" @@ -198,7 +199,8 @@ func (privVal *PrivValidatorFS) Reset() { func (privVal *PrivValidatorFS) SignVote(chainID string, vote *Vote) error { privVal.mtx.Lock() defer privVal.mtx.Unlock() - signature, err := privVal.signBytesHRS(vote.Height, vote.Round, voteToStep(vote), SignBytes(chainID, vote)) + signature, err := privVal.signBytesHRS(vote.Height, vote.Round, voteToStep(vote), + SignBytes(chainID, vote), checkVotesOnlyDifferByTimestamp) if err != nil { return errors.New(cmn.Fmt("Error signing vote: %v", err)) } @@ -211,7 +213,8 @@ func (privVal *PrivValidatorFS) SignVote(chainID string, vote *Vote) error { func (privVal *PrivValidatorFS) SignProposal(chainID string, proposal *Proposal) error { privVal.mtx.Lock() defer privVal.mtx.Unlock() - signature, err := privVal.signBytesHRS(proposal.Height, proposal.Round, stepPropose, SignBytes(chainID, proposal)) + signature, err := privVal.signBytesHRS(proposal.Height, proposal.Round, stepPropose, + SignBytes(chainID, proposal), checkProposalsOnlyDifferByTimestamp) if err != nil { return fmt.Errorf("Error signing proposal: %v", err) } @@ -219,55 +222,76 @@ func (privVal *PrivValidatorFS) SignProposal(chainID string, proposal *Proposal) return nil } -// signBytesHRS signs the given signBytes if the height/round/step (HRS) are -// greater than the latest state. If the HRS are equal, it returns the -// privValidator.LastSignature. -func (privVal *PrivValidatorFS) signBytesHRS(height int64, round int, step int8, signBytes []byte) (crypto.Signature, error) { - sig := crypto.Signature{} - +// returns error if HRS regression. returns true if HRS is unchanged +func (privVal *PrivValidatorFS) checkHRS(height int64, round int, step int8) (bool, error) { if privVal.LastHeight > height { - return sig, errors.New("Height regression") + return false, errors.New("Height regression") } if privVal.LastHeight == height { if privVal.LastRound > round { - return sig, errors.New("Round regression") + return false, errors.New("Round regression") } if privVal.LastRound == round { if privVal.LastStep > step { - return sig, errors.New("Step regression") + return false, errors.New("Step regression") } else if privVal.LastStep == step { if privVal.LastSignBytes != nil { if privVal.LastSignature.Empty() { panic("privVal: LastSignature is nil but LastSignBytes is not!") } - // NOTE: we might crash between writing the priv val and writing to - // wal. since we have time in votes and proposals, signBytes can be - // different from LastSignBytes. we might be signing conflicting - // bytes here. - return privVal.LastSignature, nil + return true, nil } - return sig, errors.New("No LastSignature found") + return false, errors.New("No LastSignature found") } } } + return false, nil +} - // Sign - sig, err := privVal.Sign(signBytes) +// signBytesHRS signs the given signBytes if the height/round/step (HRS) are +// greater than the latest state. If the HRS are equal and the only thing changed is the timestamp, +// it returns the privValidator.LastSignature. Else it returns an error. +func (privVal *PrivValidatorFS) signBytesHRS(height int64, round int, step int8, + signBytes []byte, checkFn checkOnlyDifferByTimestamp) (crypto.Signature, error) { + sig := crypto.Signature{} + + sameHRS, err := privVal.checkHRS(height, round, step) if err != nil { return sig, err } - // Persist height/round/step + // We might crash before writing to the wal, + // causing us to try to re-sign for the same HRS + if sameHRS { + // if they're the same or only differ by timestamp, + // return the LastSignature. Otherwise, error + if bytes.Equal(signBytes, privVal.LastSignBytes) || + checkFn(privVal.LastSignBytes, signBytes) { + return privVal.LastSignature, nil + } + return sig, fmt.Errorf("Conflicting data") + } + + sig, err = privVal.Sign(signBytes) + if err != nil { + return sig, err + } + privVal.saveSigned(height, round, step, signBytes, sig) + return sig, nil +} + +// Persist height/round/step and signature +func (privVal *PrivValidatorFS) saveSigned(height int64, round int, step int8, + signBytes []byte, sig crypto.Signature) { + privVal.LastHeight = height privVal.LastRound = round privVal.LastStep = step privVal.LastSignature = sig privVal.LastSignBytes = signBytes privVal.save() - - return sig, nil } // SignHeartbeat signs a canonical representation of the heartbeat, along with the chainID. @@ -302,3 +326,47 @@ func (pvs PrivValidatorsByAddress) Swap(i, j int) { pvs[i] = pvs[j] pvs[j] = it } + +//------------------------------------- + +type checkOnlyDifferByTimestamp func([]byte, []byte) bool + +// returns true if the only difference in the votes is their timestamp +func checkVotesOnlyDifferByTimestamp(lastSignBytes, newSignBytes []byte) bool { + var lastVote, newVote CanonicalJSONOnceVote + if err := json.Unmarshal(lastSignBytes, &lastVote); err != nil { + panic(fmt.Sprintf("LastSignBytes cannot be unmarshalled into vote: %v", err)) + } + if err := json.Unmarshal(newSignBytes, &newVote); err != nil { + panic(fmt.Sprintf("signBytes cannot be unmarshalled into vote: %v", err)) + } + + // set the times to the same value and check equality + now := CanonicalTime(time.Now()) + lastVote.Vote.Timestamp = now + newVote.Vote.Timestamp = now + lastVoteBytes, _ := json.Marshal(lastVote) + newVoteBytes, _ := json.Marshal(newVote) + + return bytes.Equal(newVoteBytes, lastVoteBytes) +} + +// returns true if the only difference in the proposals is their timestamp +func checkProposalsOnlyDifferByTimestamp(lastSignBytes, newSignBytes []byte) bool { + var lastProposal, newProposal CanonicalJSONOnceProposal + if err := json.Unmarshal(lastSignBytes, &lastProposal); err != nil { + panic(fmt.Sprintf("LastSignBytes cannot be unmarshalled into proposal: %v", err)) + } + if err := json.Unmarshal(newSignBytes, &newProposal); err != nil { + panic(fmt.Sprintf("signBytes cannot be unmarshalled into proposal: %v", err)) + } + + // set the times to the same value and check equality + now := CanonicalTime(time.Now()) + lastProposal.Proposal.Timestamp = now + newProposal.Proposal.Timestamp = now + lastProposalBytes, _ := json.Marshal(lastProposal) + newProposalBytes, _ := json.Marshal(newProposal) + + return bytes.Equal(newProposalBytes, lastProposalBytes) +} diff --git a/types/priv_validator_test.go b/types/priv_validator_test.go index c14773c51..2fefee606 100644 --- a/types/priv_validator_test.go +++ b/types/priv_validator_test.go @@ -99,6 +99,7 @@ func TestSignVote(t *testing.T) { privVal := GenPrivValidatorFS(tempFilePath) block1 := BlockID{[]byte{1, 2, 3}, PartSetHeader{}} + block2 := BlockID{[]byte{3, 2, 1}, PartSetHeader{}} height, round := int64(10), 1 voteType := VoteTypePrevote @@ -116,12 +117,20 @@ func TestSignVote(t *testing.T) { 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 } for _, c := range cases { err = privVal.SignVote("mychainid", c) assert.Error(err, "expected error on signing conflicting vote") } + + // try signing a vote with a different time stamp + sig := vote.Signature + vote.Timestamp = vote.Timestamp.Add(time.Duration(1000)) + err = privVal.SignVote("mychainid", vote) + assert.NoError(err) + assert.Equal(sig, vote.Signature) } func TestSignProposal(t *testing.T) { @@ -131,6 +140,7 @@ func TestSignProposal(t *testing.T) { privVal := GenPrivValidatorFS(tempFilePath) block1 := PartSetHeader{5, []byte{1, 2, 3}} + block2 := PartSetHeader{10, []byte{3, 2, 1}} height, round := int64(10), 1 // sign a proposal for first time @@ -147,12 +157,20 @@ func TestSignProposal(t *testing.T) { newProposal(height, round-1, block1), // round regression newProposal(height-1, round, block1), // height regression newProposal(height-2, round+4, block1), // height regression and different round + newProposal(height, round, block2), // different block } for _, c := range cases { err = privVal.SignProposal("mychainid", c) assert.Error(err, "expected error on signing conflicting proposal") } + + // try signing a proposal with a different time stamp + sig := proposal.Signature + proposal.Timestamp = proposal.Timestamp.Add(time.Duration(1000)) + err = privVal.SignProposal("mychainid", proposal) + assert.NoError(err) + assert.Equal(sig, proposal.Signature) } func newVote(addr data.Bytes, idx int, height int64, round int, typ byte, blockID BlockID) *Vote { From f81025631e949fe44ff7a8ab1d598bcd6e4e8daf Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Thu, 21 Dec 2017 16:28:05 -0500 Subject: [PATCH 3/3] update comment [ci skip] --- types/priv_validator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/types/priv_validator.go b/types/priv_validator.go index 0e3050e51..31c65eeb4 100644 --- a/types/priv_validator.go +++ b/types/priv_validator.go @@ -222,7 +222,7 @@ func (privVal *PrivValidatorFS) SignProposal(chainID string, proposal *Proposal) return nil } -// returns error if HRS regression. returns true if HRS is unchanged +// returns error if HRS regression or no LastSignBytes. returns true if HRS is unchanged func (privVal *PrivValidatorFS) checkHRS(height int64, round int, step int8) (bool, error) { if privVal.LastHeight > height { return false, errors.New("Height regression")