evidence: prevent proposer from proposing duplicate pieces of evidence (#4839)

prevent proposer from proposing duplicate pieces of evidence

CHANGELOG_PENDING.md

@@ -72,6 +72,7 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi
 - [mempool] [\#4759](https://github.com/tendermint/tendermint/pull/4759) Allow ReapX and CheckTx functions to run in parallel (@melekes)
 - [state] [\#4781](https://github.com/tendermint/tendermint/pull/4781) Export `InitStateVersion` for the initial state version (@erikgrinaker)
 - [p2p/conn] \#4795 Return err on `signChallenge()` instead of panic
+- [evidence] [\#4839](https://github.com/tendermint/tendermint/pull/4839) Reject duplicate evidence from being proposed (@cmwaters)
 
 ### BUG FIXES:
 

node/node_test.go

@@ -262,7 +262,7 @@ func TestCreateProposalBlock(t *testing.T) {
 
 	// fill the evidence pool with more evidence
 	// than can fit in a block
-	for i := 0; i < maxEvidence+1; i++ {
+	for i := 0; i <= maxEvidence; i++ {
 		ev := types.NewMockRandomEvidence(height, time.Now(), proposerAddr, tmrand.Bytes(minEvSize))
 		err := evidencePool.AddEvidence(ev)
 		require.NoError(t, err)

state/helpers_test.go

@@ -146,6 +146,7 @@ func makeConsensusParams(
 	blockBytes, blockGas int64,
 	blockTimeIotaMs int64,
 	evidenceAge int64,
+	maxNumEvidence uint32,
 ) types.ConsensusParams {
 	return types.ConsensusParams{
 		Block: types.BlockParams{
@@ -156,6 +157,7 @@ func makeConsensusParams(
 		Evidence: types.EvidenceParams{
 			MaxAgeNumBlocks: evidenceAge,
 			MaxAgeDuration:  time.Duration(evidenceAge),
+			MaxNum:          maxNumEvidence,
 		},
 	}
 }

state/state_test.go

@@ -986,7 +986,7 @@ func TestConsensusParamsChangesSaveLoad(t *testing.T) {
 }
 
 func TestApplyUpdates(t *testing.T) {
-	initParams := makeConsensusParams(1, 2, 3, 4)
+	initParams := makeConsensusParams(1, 2, 3, 4, 5)
 	const maxAge int64 = 66
 	cases := [...]struct {
 		init     types.ConsensusParams
@@ -1002,15 +1002,16 @@ func TestApplyUpdates(t *testing.T) {
 					MaxGas:   55,
 				},
 			},
-			makeConsensusParams(44, 55, 3, 4)},
+			makeConsensusParams(44, 55, 3, 4, 5)},
 		3: {initParams,
 			abci.ConsensusParams{
 				Evidence: &abci.EvidenceParams{
 					MaxAgeNumBlocks: maxAge,
 					MaxAgeDuration:  time.Duration(maxAge),
+					MaxNum:          10,
 				},
 			},
-			makeConsensusParams(1, 2, 3, maxAge)},
+			makeConsensusParams(1, 2, 3, maxAge, 10)},
 	}
 
 	for i, tc := range cases {

state/validation.go

@@ -127,7 +127,13 @@ func validateBlock(evidencePool EvidencePool, stateDB dbm.DB, state State, block
 	}
 
 	// Validate all evidence.
-	for _, ev := range block.Evidence.Evidence {
+	for idx, ev := range block.Evidence.Evidence {
+		// check that no evidence has been submitted more than once
+		for i := idx + 1; i < len(block.Evidence.Evidence); i++ {
+			if ev.Equal(block.Evidence.Evidence[i]) {
+				return types.NewErrEvidenceInvalid(ev, errors.New("evidence was submitted twice"))
+			}
+		}
 		if evidencePool != nil {
 			if evidencePool.IsCommitted(ev) {
 				return types.NewErrEvidenceInvalid(ev, errors.New("evidence was already committed"))

state/validation_test.go

@@ -203,7 +203,8 @@ func TestValidateBlockEvidence(t *testing.T) {
 	require.NoError(t, proxyApp.Start())
 	defer proxyApp.Stop()
 
-	state, stateDB, privVals := makeState(3, 1)
+	state, stateDB, privVals := makeState(4, 1)
+	state.ConsensusParams.Evidence.MaxNum = 3
 	blockExec := sm.NewBlockExecutor(
 		stateDB,
 		log.TestingLogger(),
@@ -215,8 +216,8 @@ func TestValidateBlockEvidence(t *testing.T) {
 
 	for height := int64(1); height < validationTestsStopHeight; height++ {
 		proposerAddr := state.Validators.GetProposer().Address
-		goodEvidence := types.NewMockEvidence(height, time.Now(), proposerAddr)
 		maxNumEvidence := state.ConsensusParams.Evidence.MaxNum
+		t.Log(maxNumEvidence)
 		if height > 1 {
 			/*
 				A block with too much evidence fails
@@ -225,7 +226,7 @@ func TestValidateBlockEvidence(t *testing.T) {
 			evidence := make([]types.Evidence, 0)
 			// one more than the maximum allowed evidence
 			for i := uint32(0); i <= maxNumEvidence; i++ {
-				evidence = append(evidence, goodEvidence)
+				evidence = append(evidence, types.NewMockEvidence(height, time.Now(), proposerAddr))
 			}
 			block, _ := state.MakeBlock(height, makeTxs(height), lastCommit, evidence, proposerAddr)
 			err := blockExec.ValidateBlock(state, block)
@@ -240,7 +241,9 @@ func TestValidateBlockEvidence(t *testing.T) {
 		evidence := make([]types.Evidence, 0)
 		// precisely the amount of allowed evidence
 		for i := uint32(0); i < maxNumEvidence; i++ {
-			evidence = append(evidence, goodEvidence)
+			// make different evidence for each validator
+			addr, _ := state.Validators.GetByIndex(int(i))
+			evidence = append(evidence, types.NewMockEvidence(height, time.Now(), addr))
 		}
 
 		var err error
@@ -313,27 +316,24 @@ func TestValidateAlreadyPendingEvidence(t *testing.T) {
 	require.NoError(t, err)
 }
 
-// TODO: prevent committing duplicate votes
-//func TestValidateDuplicateEvidenceShouldFail(t *testing.T) {
-//	var height int64 = 1
-//	var evidence []types.Evidence
-//	state, stateDB, _ := makeState(1, int(height))
-//
-//	evpool := evmock.NewDefaultEvidencePool()
-//	blockExec := sm.NewBlockExecutor(
-//		stateDB, log.TestingLogger(),
-//		nil,
-//		nil,
-//		evpool)
-//	// A block with a couple pieces of evidence passes.
-//	block := makeBlock(state, height)
-//	addr, _ := state.Validators.GetByIndex(0)
-//	for i := 0; i < 2; i++ {
-//		evidence = append(evidence, types.NewMockEvidence(height, time.Now(), addr))
-//	}
-//	block.Evidence.Evidence = evidence
-//	block.EvidenceHash = block.Evidence.Hash()
-//	err := blockExec.ValidateBlock(state, block)
-//
-//	require.Error(t, err)
-//}
+func TestValidateDuplicateEvidenceShouldFail(t *testing.T) {
+	var height int64 = 1
+	state, stateDB, _ := makeState(1, int(height))
+	addr, _ := state.Validators.GetByIndex(0)
+	addr2, _ := state.Validators.GetByIndex(1)
+	ev := types.NewMockEvidence(height, defaultTestTime, addr)
+	ev2 := types.NewMockEvidence(height, defaultTestTime, addr2)
+
+	blockExec := sm.NewBlockExecutor(
+		stateDB, log.TestingLogger(),
+		nil,
+		nil,
+		sm.MockEvidencePool{})
+	// A block with a couple pieces of evidence passes.
+	block := makeBlock(state, height)
+	block.Evidence.Evidence = []types.Evidence{ev, ev2, ev2}
+	block.EvidenceHash = block.Evidence.Hash()
+	err := blockExec.ValidateBlock(state, block)
+
+	require.Error(t, err)
+}

types/evidence.go

@@ -1060,6 +1060,8 @@ func (e MockRandomEvidence) Hash() []byte {
 	return []byte(fmt.Sprintf("%d-%x", e.EvidenceHeight, e.randBytes))
 }
 
+func (e MockRandomEvidence) Equal(ev Evidence) bool { return false }
+
 // UNSTABLE
 type MockEvidence struct {
 	EvidenceHeight  int64
@@ -1090,9 +1092,8 @@ func (e MockEvidence) Bytes() []byte {
 }
 func (e MockEvidence) Verify(chainID string, pubKey crypto.PubKey) error { return nil }
 func (e MockEvidence) Equal(ev Evidence) bool {
-	e2 := ev.(MockEvidence)
-	return e.EvidenceHeight == e2.EvidenceHeight &&
-		bytes.Equal(e.EvidenceAddress, e2.EvidenceAddress)
+	return e.EvidenceHeight == ev.Height() &&
+		bytes.Equal(e.EvidenceAddress, ev.Address())
 }
 func (e MockEvidence) ValidateBasic() error { return nil }
 func (e MockEvidence) String() string {