evidence: both MaxAgeDuration and MaxAgeNumBlocks need to be surpassed (#4667)

for evidence to be considered expired. otherwise, a cabal group can
  manipulate block time to make a particular evidence too old.

Refs https://github.com/tendermint/tendermint/issues/2565#issuecomment-432896645
Refs https://github.com/tendermint/tendermint/issues/2653
spec PR: tendermint/spec#87

evidence/pool.go

@@ -163,8 +163,7 @@ func (evpool *Pool) removeEvidence(
 
 		// Remove the evidence if it's already in a block or if it's now too old.
 		if _, ok := blockEvidenceMap[evMapKey(ev)]; ok ||
-			ageNumBlocks > params.MaxAgeNumBlocks ||
-			ageDuration > params.MaxAgeDuration {
+			(ageDuration > params.MaxAgeDuration && ageNumBlocks > params.MaxAgeNumBlocks) {
 			// remove from clist
 			evpool.evidenceList.Remove(e)
 			e.DetachPrev()

evidence/pool_test.go

@@ -58,7 +58,7 @@ func TestEvidencePool(t *testing.T) {
 
 	var (
 		valAddr      = []byte("val1")
-		height       = int64(5)
+		height       = int64(100002)
 		stateDB      = initializeValidatorState(valAddr, height)
 		evidenceDB   = dbm.NewMemDB()
 		pool         = NewPool(stateDB, evidenceDB)
@@ -66,7 +66,7 @@ func TestEvidencePool(t *testing.T) {
 	)
 
 	goodEvidence := types.NewMockEvidence(height, time.Now(), 0, valAddr)
-	badEvidence := types.NewMockEvidence(height, evidenceTime, 0, valAddr)
+	badEvidence := types.NewMockEvidence(1, evidenceTime, 0, valAddr)
 
 	// bad evidence
 	err := pool.AddEvidence(badEvidence)
@@ -134,10 +134,10 @@ func TestAddEvidence(t *testing.T) {
 		evDescription string
 	}{
 		{height, time.Now(), false, "valid evidence"},
-		{height, evidenceTime, true, "evidence created at 2019-01-01 00:00:00 +0000 UTC has expired"},
-		{int64(1), time.Now(), true, "evidence from height 1 is too old"},
+		{height, evidenceTime, false, "valid evidence (despite old time)"},
+		{int64(1), time.Now(), false, "valid evidence (despite old height)"},
 		{int64(1), evidenceTime, true,
-			"evidence from height 1 is too old & evidence created at 2019-01-01 00:00:00 +0000 UTC has expired"},
+			"evidence from height 1 (created at: 2019-01-01 00:00:00 +0000 UTC) is too old"},
 	}
 
 	for _, tc := range testCases {

evidence/reactor.go

@@ -194,7 +194,7 @@ func (evR Reactor) checkSendEvidenceMessage(
 
 	if peerHeight < evHeight { // peer is behind. sleep while he catches up
 		return nil, true
-	} else if ageNumBlocks > params.MaxAgeNumBlocks ||
+	} else if ageNumBlocks > params.MaxAgeNumBlocks &&
 		ageDuration > params.MaxAgeDuration { // evidence is too old, skip
 
 		// NOTE: if evidence is too old for an honest peer, then we're behind and

state/validation.go

@@ -162,18 +162,19 @@ func VerifyEvidence(stateDB dbm.DB, state State, evidence types.Evidence) error
 	var (
 		height         = state.LastBlockHeight
 		evidenceParams = state.ConsensusParams.Evidence
-	)
 
-	ageNumBlocks := height - evidence.Height()
-	if ageNumBlocks > evidenceParams.MaxAgeNumBlocks {
-		return fmt.Errorf("evidence from height %d is too old. Min height is %d",
-			evidence.Height(), height-evidenceParams.MaxAgeNumBlocks)
-	}
+		ageDuration  = state.LastBlockTime.Sub(evidence.Time())
+		ageNumBlocks = height - evidence.Height()
+	)
 
-	ageDuration := state.LastBlockTime.Sub(evidence.Time())
-	if ageDuration > evidenceParams.MaxAgeDuration {
-		return fmt.Errorf("evidence created at %v has expired. Evidence can not be older than: %v",
-			evidence.Time(), state.LastBlockTime.Add(evidenceParams.MaxAgeDuration))
+	if ageDuration > evidenceParams.MaxAgeDuration && ageNumBlocks > evidenceParams.MaxAgeNumBlocks {
+		return fmt.Errorf(
+			"evidence from height %d (created at: %v) is too old; min height is %d and evidence can not be older than %v",
+			evidence.Height(),
+			evidence.Time(),
+			height-evidenceParams.MaxAgeNumBlocks,
+			state.LastBlockTime.Add(evidenceParams.MaxAgeDuration),
+		)
 	}
 
 	valset, err := LoadValidators(stateDB, evidence.Height())