fix(mempool): legacy subpool panic on skipped header during reset (#668)

* add test to demonstrate subpool reset being called with invalid block hashes

* make test fail on hash mismatch

* update txpool cosmos reorg test to use real legacypool as the subpool

* update comment

* fix chan pointer madness

* remove logic that would only panic for cosmos chains from legacypool when reset was called with a skipped header

* remove unnecessary long sleep from test for success case

* fix race in test

* changelog

* reference pr link in log

Author: mattac21

CHANGELOG.md

@@ -21,6 +21,7 @@
 - [\#658](https://github.com/cosmos/evm/pull/658) Fix race condition between legacypool's RemoveTx and runReorg.
 - [\#687](https://github.com/cosmos/evm/pull/687) Avoid blocking node shutdown when evm indexer is enabled, log startup failures instead of using errgroup.
 - [\#689](https://github.com/cosmos/evm/pull/689) Align debug addr for hex address.
+- [\#668](https://github.com/cosmos/evm/pull/668) Fix panic in legacy mempool when Reset() was called with a skipped header between old and new block.
 
 ### IMPROVEMENTS
 

mempool/txpool/legacypool/mocks/BlockChain.go

@@ -3,8 +3,6 @@
 package legacypool
 
 import (
-	"math"
-
 	"github.com/ethereum/go-ethereum/core/types"
 	"github.com/ethereum/go-ethereum/log"
 )
@@ -16,76 +14,15 @@ func (pool *LegacyPool) reset(oldHead, newHead *types.Header) {
 	var reinject types.Transactions
 
 	if oldHead != nil && oldHead.Hash() != newHead.ParentHash {
-		// If the reorg is too deep, avoid doing it (will happen during fast sync)
-		oldNum := oldHead.Number.Uint64()
-		newNum := newHead.Number.Uint64()
-
-		if depth := uint64(math.Abs(float64(oldNum) - float64(newNum))); depth > 64 {
-			log.Debug("Skipping deep transaction reorg", "depth", depth)
-		} else {
-			// Reorg seems shallow enough to pull in all transactions into memory
-			var (
-				rem = pool.chain.GetBlock(oldHead.Hash(), oldHead.Number.Uint64())
-				add = pool.chain.GetBlock(newHead.Hash(), newHead.Number.Uint64())
-			)
-			if rem == nil {
-				// This can happen if a setHead is performed, where we simply discard the old
-				// head from the chain.
-				if newNum >= oldNum {
-					// If we reorged to a same or higher number, then it's not a case of setHead
-					log.Warn("Transaction pool reset with missing old head",
-						"old", oldHead.Hash(), "oldnum", oldNum, "new", newHead.Hash(), "newnum", newNum)
-					return
-				}
-				// If the reorg ended up on a lower number, it's indicative of setHead being the cause
-				log.Debug("Skipping transaction reset caused by setHead",
-					"old", oldHead.Hash(), "oldnum", oldNum, "new", newHead.Hash(), "newnum", newNum)
-				// We still need to update the current state s.th. the lost transactions can be readded by the user
-			} else {
-				if add == nil {
-					// if the new head is nil, it means that something happened between
-					// the firing of newhead-event and _now_: most likely a
-					// reorg caused by sync-reversion or explicit sethead back to an
-					// earlier block.
-					log.Warn("Transaction pool reset with missing new head", "number", newHead.Number, "hash", newHead.Hash())
-					return
-				}
-				var discarded, included types.Transactions
-				for rem.NumberU64() > add.NumberU64() {
-					discarded = append(discarded, rem.Transactions()...)
-					if rem = pool.chain.GetBlock(rem.ParentHash(), rem.NumberU64()-1); rem == nil {
-						log.Error("Unrooted old chain seen by tx pool", "block", oldHead.Number, "hash", oldHead.Hash())
-						return
-					}
-				}
-				for add.NumberU64() > rem.NumberU64() {
-					included = append(included, add.Transactions()...)
-					if add = pool.chain.GetBlock(add.ParentHash(), add.NumberU64()-1); add == nil {
-						log.Error("Unrooted new chain seen by tx pool", "block", newHead.Number, "hash", newHead.Hash())
-						return
-					}
-				}
-				for rem.Hash() != add.Hash() {
-					discarded = append(discarded, rem.Transactions()...)
-					if rem = pool.chain.GetBlock(rem.ParentHash(), rem.NumberU64()-1); rem == nil {
-						log.Error("Unrooted old chain seen by tx pool", "block", oldHead.Number, "hash", oldHead.Hash())
-						return
-					}
-					included = append(included, add.Transactions()...)
-					if add = pool.chain.GetBlock(add.ParentHash(), add.NumberU64()-1); add == nil {
-						log.Error("Unrooted new chain seen by tx pool", "block", newHead.Number, "hash", newHead.Hash())
-						return
-					}
-				}
-				lost := make([]*types.Transaction, 0, len(discarded))
-				for _, tx := range types.TxDifference(discarded, included) {
-					if pool.Filter(tx) {
-						lost = append(lost, tx)
-					}
-				}
-				reinject = lost
-			}
-		}
+		// this is a strange reorg check from geth, it is possible for cosmos
+		// chains to call this function with newHead=oldHead+2, so
+		// newHead.ParentHash != oldHead.Hash. This would incorrectly be seen
+		// as a reorg on a cosmos chain and would therefore panic. Since this
+		// logic would only panic for cosmos chains in a valid state, we have
+		// removed it and replaced with a debug log.
+		//
+		// see https://github.com/cosmos/evm/pull/668 for more context.
+		log.Debug("leacypool saw skipped block (reorg) on cosmos chain, doing nothing...", "oldHead", oldHead.Hash(), "newHead", newHead.Hash(), "newParent", newHead.ParentHash)
 	}
 	pool.resetInternalState(newHead, reinject)
 }