remove invalid transactions from the mempool (#3701)

Otherwise, we'll be trying to include them in each consecutive block.

The downside is that evil proposers will be able to drop valid
transactions (see #3322).

Reverts https://github.com/tendermint/tendermint/pull/3625

Fixes #3699

CHANGELOG_PENDING.md

@@ -19,3 +19,5 @@
 ### IMPROVEMENTS:
 
 ### BUG FIXES:
+- [mempool] \#3699 Revert the change where we only remove valid transactions
+  from the mempool once a block is committed.

mempool/clist_mempool.go

@@ -538,24 +538,23 @@ func (mem *CListMempool) Update(
 		if deliverTxResponses[i].Code == abci.CodeTypeOK {
 			// Add valid committed tx to the cache (if missing).
 			_ = mem.cache.Push(tx)
-
-			// Remove valid committed tx from the mempool.
-			if e, ok := mem.txsMap.Load(txKey(tx)); ok {
-				mem.removeTx(tx, e.(*clist.CElement), false)
-			}
 		} else {
 			// Allow invalid transactions to be resubmitted.
 			mem.cache.Remove(tx)
+		}
 
-			// Don't remove invalid tx from the mempool.
+		// Remove committed tx from the mempool.
-			// Otherwise evil proposer can drop valid txs.
+		//
-			// Example:
+		// Note an evil proposer can drop valid txs!
-			//   100 -> 101 -> 102
+		// Mempool before:
-			// Block, proposed by evil proposer:
+		//   100 -> 101 -> 102
-			//   101 -> 102
+		// Block, proposed by an evil proposer:
-			// Mempool (if you remove txs):
+		//   101 -> 102
-			//   100
+		// Mempool after:
-			// https://github.com/tendermint/tendermint/issues/3322.
+		//   100
+		// https://github.com/tendermint/tendermint/issues/3322.
+		if e, ok := mem.txsMap.Load(txKey(tx)); ok {
+			mem.removeTx(tx, e.(*clist.CElement), false)
 		}
 	}
 

mempool/clist_mempool_test.go

@@ -200,12 +200,15 @@ func TestMempoolUpdate(t *testing.T) {
 		assert.Zero(t, mempool.Size())
 	}
 
-	// 3. Removes invalid transactions from the cache, but leaves them in the mempool (if present)
+	// 3. Removes invalid transactions from the cache and the mempool (if present)
 	{
 		err := mempool.CheckTx([]byte{0x03}, nil)
 		require.NoError(t, err)
 		mempool.Update(1, []types.Tx{[]byte{0x03}}, abciResponses(1, 1), nil, nil)
-		assert.Equal(t, 1, mempool.Size())
+		assert.Zero(t, mempool.Size())
+
+		err = mempool.CheckTx([]byte{0x03}, nil)
+		assert.NoError(t, err)
 	}
 }