diff --git a/privval/file.go b/privval/file.go index b11346dc7..36eec7a1c 100644 --- a/privval/file.go +++ b/privval/file.go @@ -99,14 +99,14 @@ type FilePVLastSignState struct { filePath string } -// CheckHRS checks the given height, round, step (HRS) against that of the +// checkHRS checks the given height, round, step (HRS) against that of the // FilePVLastSignState. It returns an error if the arguments constitute a regression, // or if they match but the SignBytes are empty. // The returned boolean indicates whether the last Signature should be reused - // it returns true if the HRS matches the arguments and the SignBytes are not empty (indicating // we have already signed for this HRS, and can reuse the existing signature). // It panics if the HRS matches the arguments, there's a SignBytes, but no Signature. -func (lss *FilePVLastSignState) CheckHRS(height int64, round int32, step int8) (bool, error) { +func (lss *FilePVLastSignState) checkHRS(height int64, round int32, step int8) (bool, error) { if lss.Height > height { return false, fmt.Errorf("height regression. Got %v, last height %v", height, lss.Height) @@ -345,7 +345,7 @@ func (pv *FilePV) signVote(chainID string, vote *tmproto.Vote) error { round := vote.Round lss := pv.LastSignState - sameHRS, err := lss.CheckHRS(height, round, step) + sameHRS, err := lss.checkHRS(height, round, step) if err != nil { return err } @@ -388,14 +388,12 @@ func (pv *FilePV) signVote(chainID string, vote *tmproto.Vote) error { } // signProposal checks if the proposal is good to sign and sets the proposal signature. -// It may need to set the timestamp as well if the proposal is otherwise the same as -// a previously signed proposal ie. we crashed after signing but before the proposal hit the WAL). func (pv *FilePV) signProposal(chainID string, proposal *tmproto.Proposal) error { height, round, step := proposal.Height, proposal.Round, stepPropose lss := pv.LastSignState - sameHRS, err := lss.CheckHRS(height, round, step) + sameHRS, err := lss.checkHRS(height, round, step) if err != nil { return err } @@ -405,23 +403,12 @@ func (pv *FilePV) signProposal(chainID string, proposal *tmproto.Proposal) error // We might crash before writing to the wal, // causing us to try to re-sign for the same HRS. // If signbytes are the same, use the last signature. - // If they only differ by timestamp, use last timestamp and signature - // Otherwise, return error if sameHRS { - if bytes.Equal(signBytes, lss.SignBytes) { - proposal.Signature = lss.Signature - } else { - timestamp, ok, err := checkProposalsOnlyDifferByTimestamp(lss.SignBytes, signBytes) - if err != nil { - return err - } - if !ok { - return errors.New("conflicting data") - } - proposal.Timestamp = timestamp - proposal.Signature = lss.Signature - return nil + if !bytes.Equal(signBytes, lss.SignBytes) { + return errors.New("conflicting data") } + proposal.Signature = lss.Signature + return nil } // It passed the checks. Sign the proposal @@ -467,23 +454,3 @@ func checkVotesOnlyDifferByTimestamp(lastSignBytes, newSignBytes []byte) (time.T return lastTime, proto.Equal(&newVote, &lastVote), nil } - -// returns the timestamp from the lastSignBytes. -// returns true if the only difference in the proposals is their timestamp -func checkProposalsOnlyDifferByTimestamp(lastSignBytes, newSignBytes []byte) (time.Time, bool, error) { - var lastProposal, newProposal tmproto.CanonicalProposal - if err := protoio.UnmarshalDelimited(lastSignBytes, &lastProposal); err != nil { - return time.Time{}, false, fmt.Errorf("LastSignBytes cannot be unmarshalled into proposal: %w", err) - } - if err := protoio.UnmarshalDelimited(newSignBytes, &newProposal); err != nil { - return time.Time{}, false, fmt.Errorf("signBytes cannot be unmarshalled into proposal: %w", err) - } - - lastTime := lastProposal.Timestamp - // set the times to the same value and check equality - now := tmtime.Now() - lastProposal.Timestamp = now - newProposal.Timestamp = now - - return lastTime, proto.Equal(&newProposal, &lastProposal), nil -} diff --git a/privval/file_test.go b/privval/file_test.go index 655a3d9b8..e51b488cb 100644 --- a/privval/file_test.go +++ b/privval/file_test.go @@ -234,7 +234,8 @@ func TestSignProposal(t *testing.T) { height, round := int64(10), int32(1) // sign a proposal for first time - proposal := newProposal(height, round, block1) + ts := tmtime.Now() + proposal := newProposal(height, round, block1, ts) pbp := proposal.ToProto() err = privVal.SignProposal(ctx, "mychainid", pbp) @@ -246,23 +247,17 @@ func TestSignProposal(t *testing.T) { // now try some bad Proposals cases := []*types.Proposal{ - 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 + newProposal(height, round-1, block1, ts), // round regression + newProposal(height-1, round, block1, ts), // height regression + newProposal(height-2, round+4, block1, ts), // height regression and different round + newProposal(height, round, block2, ts), // different block + newProposal(height, round, block1, ts.Add(time.Second)), // different timestamp } for _, c := range cases { - assert.Error(t, privVal.SignProposal(ctx, "mychainid", c.ToProto()), + assert.Errorf(t, privVal.SignProposal(ctx, "mychainid", c.ToProto()), "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(ctx, "mychainid", pbp) - assert.NoError(t, err) - assert.Equal(t, sig, proposal.Signature) } func TestDifferByTimestamp(t *testing.T) { @@ -277,33 +272,9 @@ func TestDifferByTimestamp(t *testing.T) { privVal, err := GenFilePV(tempKeyFile.Name(), tempStateFile.Name(), "") require.NoError(t, err) randbytes := tmrand.Bytes(tmhash.Size) - block1 := types.BlockID{Hash: randbytes, PartSetHeader: types.PartSetHeader{Total: 5, Hash: randbytes}} height, round := int64(10), int32(1) chainID := "mychainid" - // test proposal - { - proposal := newProposal(height, round, block1) - pb := proposal.ToProto() - err := privVal.SignProposal(ctx, chainID, pb) - require.NoError(t, err, "expected no error signing proposal") - signBytes := types.ProposalSignBytes(chainID, pb) - - sig := proposal.Signature - timeStamp := proposal.Timestamp - - // manipulate the timestamp. should get changed back - pb.Timestamp = pb.Timestamp.Add(time.Millisecond) - var emptySig []byte - proposal.Signature = emptySig - err = privVal.SignProposal(ctx, "mychainid", pb) - require.NoError(t, err, "expected no error on signing same proposal") - - assert.Equal(t, timeStamp, pb.Timestamp) - assert.Equal(t, signBytes, types.ProposalSignBytes(chainID, pb)) - assert.Equal(t, sig, proposal.Signature) - } - // test vote { voteType := tmproto.PrevoteType @@ -343,11 +314,11 @@ func newVote(addr types.Address, idx int32, height int64, round int32, } } -func newProposal(height int64, round int32, blockID types.BlockID) *types.Proposal { +func newProposal(height int64, round int32, blockID types.BlockID, t time.Time) *types.Proposal { return &types.Proposal{ Height: height, Round: round, BlockID: blockID, - Timestamp: tmtime.Now(), + Timestamp: t, } }