From 7bc3a7274a9224c6ce766bc11cb9679f8029d074 Mon Sep 17 00:00:00 2001 From: William Banfield <4561443+williambanfield@users.noreply.github.com> Date: Wed, 19 Jan 2022 12:42:09 -0500 Subject: [PATCH] privval: do not use old proposal timestamp (#7621) After #7592, @cmwaters noticed that the logic for re-using old timestamps for proposals may not work with proposer-based timestamps. This change removes the logic to re-use old proposal timestamps since it is no longer correct. Two proposals with different timestamps can no longer be treated as equivalent. Signing a proposal that only differs by timestamp in the new algorithm can be thought of as roughly equivalent to signing a proposal that only differs by `BlockID` in the old scheme. I also investigated the codebase and checked for any place we updated a timestamp using the pattern `(Timestamp = |Timestamp: )` and saw no additional places where we are updating the timestamp of a proposal message. Here is the output of that search: ``` privval/file.go:372: vote.Timestamp = timestamp privval/file.go:453: lastVote.Timestamp = now privval/file.go:454: newVote.Timestamp = now internal/test/factory/commit.go:25: Timestamp: now, internal/test/factory/vote.go:34: Timestamp: time, internal/consensus/state.go:2261: Timestamp: cs.voteTime(), internal/consensus/state.go:2286: vote.Timestamp = v.Timestamp light/detector.go:414: ev.Timestamp = common.Time light/detector.go:418: ev.Timestamp = trusted.Time types/block.go:616: Timestamp: ts, types/block.go:725: Timestamp: cs.Timestamp, types/block.go:736: cs.Timestamp = csp.Timestamp types/block.go:800: Timestamp: commitSig.Timestamp, types/evidence.go:84: Timestamp: blockTime, types/evidence.go:190: dve.Timestamp = evidenceTime types/evidence.go:202: Timestamp: dve.Timestamp, types/evidence.go:228: Timestamp: pb.Timestamp, types/evidence.go:382: Timestamp: %v}#%X`, types/evidence.go:491: l.Timestamp = evidenceTime types/evidence.go:517: Timestamp: l.Timestamp, types/evidence.go:546: Timestamp: lpb.Timestamp, types/evidence.go:722: Timestamp: time, types/vote.go:80: Timestamp: vote.Timestamp, types/vote.go:216: Timestamp: vote.Timestamp, types/vote.go:240: vote.Timestamp = pv.Timestamp types/test_util.go:27: Timestamp: now, types/proposal.go:44: Timestamp: tmtime.Now(), types/proposal.go:132: pb.Timestamp = p.Timestamp types/proposal.go:157: p.Timestamp = pp.Timestamp types/canonical.go:49: Timestamp: proposal.Timestamp, types/canonical.go:62: Timestamp: vote.Timestamp, test/e2e/runner/evidence.go:186: Timestamp: evTime, ``` --- privval/file.go | 49 ++++++++------------------------------------ privval/file_test.go | 49 +++++++++----------------------------------- 2 files changed, 18 insertions(+), 80 deletions(-) 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, } }