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

@@ -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
-}

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,
 	}
 }