fix: mempool lane size check on CheckTx (backport #561) (#565)

* fix: mempool lane size check on `CheckTx` (#561) * push * init * fix setup * format * fix test * use lane * ok * finalize * fix everything * lint fix: * Update abci/checktx/mempool_parity_check_tx.go Co-authored-by: David Terpay <[email protected]> * lint fix * tidy * remove * cleanup --------- Co-authored-by: David Terpay <[email protected]> Co-authored-by: David Terpay <[email protected]> (cherry picked from commit f1cde2a) # Conflicts: # README.md # go.mod # go.sum # x/auction/types/mocks/account_keeper.go # x/auction/types/mocks/bank_keeper.go # x/auction/types/mocks/distribution_keeper.go # x/auction/types/mocks/rewards_address_provider.go # x/auction/types/mocks/staking_keeper.go * fix * tidy --------- Co-authored-by: Alex Johnson <[email protected]>
skip-mev · Jul 2, 2024 · 1ea62f3 · 1ea62f3
1 parent 9d3be7b
commit 1ea62f3
Show file tree

Hide file tree

Showing 34 changed files with 584 additions and 345 deletions.
diff --git a/abci/abci.go b/abci/abci.go
@@ -8,6 +8,7 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
 	"github.com/cosmos/cosmos-sdk/baseapp"
+
 	"github.com/skip-mev/block-sdk/v2/block"
 	"github.com/skip-mev/block-sdk/v2/block/proposals"
 	"github.com/skip-mev/block-sdk/v2/block/utils"

diff --git a/abci/checktx/check_tx_test.go b/abci/checktx/check_tx_test.go
@@ -43,6 +43,18 @@ func (s *CheckTxTestSuite) TestCheckTxMempoolParity() {
 	)
 	s.Require().NoError(err)
 
+	hugeBidTx, _, err := testutils.CreateNAuctionTx(
+		s.EncCfg.TxConfig,
+		s.Accounts[0],
+		sdk.NewCoin(s.GasTokenDenom, math.NewInt(100)),
+		0,
+		0,
+		[]testutils.Account{s.Accounts[0]},
+		100,
+		100000,
+	)
+	s.Require().NoError(err)
+
 	// create a tx that should not be inserted in the mev-lane
 	bidTx2, _, err := testutils.CreateAuctionTx(
 		s.EncCfg.TxConfig,
@@ -56,10 +68,11 @@ func (s *CheckTxTestSuite) TestCheckTxMempoolParity() {
 	s.Require().NoError(err)
 
 	txs := map[sdk.Tx]bool{
-		bidTx: true,
+		bidTx:     true,
+		hugeBidTx: true,
 	}
 
-	mevLane := s.InitLane(math.LegacyOneDec(), txs)
+	mevLane := s.InitLane(math.LegacyOneDec(), txs, true)
 	mempool, err := block.NewLanedMempool(s.Ctx.Logger(), []block.Lane{mevLane})
 	s.Require().NoError(err)
 
@@ -69,6 +82,7 @@ func (s *CheckTxTestSuite) TestCheckTxMempoolParity() {
 	ba := &baseApp{
 		s.Ctx,
 	}
+
 	mevLaneHandler := checktx.NewMEVCheckTxHandler(
 		ba,
 		cacheDecoder.TxDecoder(),
@@ -82,6 +96,7 @@ func (s *CheckTxTestSuite) TestCheckTxMempoolParity() {
 		mempool,
 		cacheDecoder.TxDecoder(),
 		mevLaneHandler,
+		ba,
 	).CheckTx()
 
 	// test that a bid can be successfully inserted to mev-lane on CheckTx
@@ -99,6 +114,36 @@ func (s *CheckTxTestSuite) TestCheckTxMempoolParity() {
 		s.Require().True(mevLane.Contains(bidTx))
 	})
 
+	// test that a bid will fail to be inserted as it is too large
+	s.Run("test bid insertion failure on CheckTx - too large", func() {
+		txBz, err := s.EncCfg.TxConfig.TxEncoder()(hugeBidTx)
+		s.Require().NoError(err)
+
+		// check tx
+		res, err := handler(&cometabci.RequestCheckTx{Tx: txBz, Type: cometabci.CheckTxType_New})
+		s.Require().NoError(err)
+
+		s.Require().Equal(uint32(1), res.Code)
+
+		// check that the mev-lane does not contain the bid
+		s.Require().False(mevLane.Contains(bidTx))
+	})
+
+	// test that a bid can be successfully inserted to mev-lane on CheckTx
+	s.Run("test bid insertion on CheckTx", func() {
+		txBz, err := s.EncCfg.TxConfig.TxEncoder()(bidTx)
+		s.Require().NoError(err)
+
+		// check tx
+		res, err := handler(&cometabci.RequestCheckTx{Tx: txBz, Type: cometabci.CheckTxType_New})
+		s.Require().NoError(err)
+
+		s.Require().Equal(uint32(0), res.Code)
+
+		// check that the mev-lane contains the bid
+		s.Require().True(mevLane.Contains(bidTx))
+	})
+
 	// test that a bid-tx (not in mev-lane) can be removed from the mempool on ReCheck
 	s.Run("test bid removal on ReCheckTx", func() {
 		// assert that the mev-lane does not contain the bidTx2
@@ -128,13 +173,17 @@ func (s *CheckTxTestSuite) TestRemovalOnRecheckTx() {
 	)
 	s.Require().NoError(err)
 
-	mevLane := s.InitLane(math.LegacyOneDec(), nil)
+	mevLane := s.InitLane(math.LegacyOneDec(), nil, true)
 	mempool, err := block.NewLanedMempool(s.Ctx.Logger(), []block.Lane{mevLane})
 	s.Require().NoError(err)
 
 	cacheDecoder, err := utils.NewDefaultCacheTxDecoder(s.EncCfg.TxConfig.TxDecoder())
 	s.Require().NoError(err)
 
+	ba := &baseApp{
+		s.Ctx,
+	}
+
 	handler := checktx.NewMempoolParityCheckTx(
 		s.Ctx.Logger(),
 		mempool,
@@ -143,6 +192,7 @@ func (s *CheckTxTestSuite) TestRemovalOnRecheckTx() {
 			// always fail
 			return &cometabci.ResponseCheckTx{Code: 1}, nil
 		},
+		ba,
 	).CheckTx()
 
 	s.Run("tx is removed on check-tx failure when re-check", func() {
@@ -169,11 +219,16 @@ func (s *CheckTxTestSuite) TestMempoolParityCheckTx() {
 		cacheDecoder, err := utils.NewDefaultCacheTxDecoder(s.EncCfg.TxConfig.TxDecoder())
 		s.Require().NoError(err)
 
+		ba := &baseApp{
+			s.Ctx,
+		}
+
 		handler := checktx.NewMempoolParityCheckTx(
 			s.Ctx.Logger(),
 			nil,
 			cacheDecoder.TxDecoder(),
 			nil,
+			ba,
 		)
 
 		res, err := handler.CheckTx()(&cometabci.RequestCheckTx{Tx: []byte("invalid-tx")})
@@ -186,7 +241,7 @@ func (s *CheckTxTestSuite) TestMempoolParityCheckTx() {
 func (s *CheckTxTestSuite) TestMEVCheckTxHandler() {
 	txs := map[sdk.Tx]bool{}
 
-	mevLane := s.InitLane(math.LegacyOneDec(), txs)
+	mevLane := s.InitLane(math.LegacyOneDec(), txs, true)
 	mempool, err := block.NewLanedMempool(s.Ctx.Logger(), []block.Lane{mevLane})
 	s.Require().NoError(err)
 
@@ -222,6 +277,7 @@ func (s *CheckTxTestSuite) TestMEVCheckTxHandler() {
 		mempool,
 		cacheDecoder.TxDecoder(),
 		mevLaneHandler,
+		ba,
 	).CheckTx()
 
 	// test that a normal tx can be successfully inserted to the mempool
@@ -287,7 +343,7 @@ func (s *CheckTxTestSuite) TestValidateBidTx() {
 		invalidBidTx: true,
 	}
 
-	mevLane := s.InitLane(math.LegacyOneDec(), txs)
+	mevLane := s.InitLane(math.LegacyOneDec(), txs, true)
 
 	cacheDecoder, err := utils.NewDefaultCacheTxDecoder(s.EncCfg.TxConfig.TxDecoder())
 	s.Require().NoError(err)
@@ -347,7 +403,7 @@ func (ba *baseApp) CommitMultiStore() storetypes.CommitMultiStore {
 
 // CheckTx is baseapp's CheckTx method that checks the validity of a
 // transaction.
-func (baseApp) CheckTx(_ *cometabci.RequestCheckTx) (*cometabci.ResponseCheckTx, error) {
+func (ba *baseApp) CheckTx(_ *cometabci.RequestCheckTx) (*cometabci.ResponseCheckTx, error) {
 	return nil, fmt.Errorf("not implemented")
 }
 
@@ -362,8 +418,17 @@ func (ba *baseApp) LastBlockHeight() int64 {
 }
 
 // GetConsensusParams is utilized to retrieve the consensus params.
-func (baseApp) GetConsensusParams(ctx sdk.Context) cmtproto.ConsensusParams {
-	return ctx.ConsensusParams()
+func (ba *baseApp) GetConsensusParams(_ sdk.Context) cmtproto.ConsensusParams {
+	return cmtproto.ConsensusParams{
+		Block: &cmtproto.BlockParams{
+			MaxBytes: 10000,
+			MaxGas:   10000,
+		},
+		Evidence:  nil,
+		Validator: nil,
+		Version:   nil,
+		Abci:      nil,
+	}
 }
 
 // ChainID is utilized to retrieve the chain ID.

diff --git a/abci/checktx/mempool_parity_check_tx.go b/abci/checktx/mempool_parity_check_tx.go
@@ -3,6 +3,8 @@ package checktx
 import (
 	"fmt"
 
+	cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
+
 	"cosmossdk.io/log"
 
 	cmtabci "github.com/cometbft/cometbft/abci/types"
@@ -26,6 +28,10 @@ type MempoolParityCheckTx struct {
 
 	// checkTxHandler to wrap
 	checkTxHandler CheckTx
+
+	// baseApp is utilized to retrieve the latest committed state and to call
+	// baseapp's CheckTx method.
+	baseApp BaseApp
 }
 
 // NewMempoolParityCheckTx returns a new MempoolParityCheckTx handler.
@@ -34,12 +40,14 @@ func NewMempoolParityCheckTx(
 	mempl block.Mempool,
 	txDecoder sdk.TxDecoder,
 	checkTxHandler CheckTx,
+	baseApp BaseApp,
 ) MempoolParityCheckTx {
 	return MempoolParityCheckTx{
 		logger:         logger,
 		mempl:          mempl,
 		txDecoder:      txDecoder,
 		checkTxHandler: checkTxHandler,
+		baseApp:        baseApp,
 	}
 }
 
@@ -79,29 +87,119 @@ func (m MempoolParityCheckTx) CheckTx() CheckTx {
 			), nil
 		}
 
-		// run the checkTxHandler
-		res, checkTxError := m.checkTxHandler(req)
-
-		// if re-check fails for a transaction, we'll need to explicitly purge the tx from
-		// the app-side mempool
-		if isInvalidCheckTxExecution(res, checkTxError) && isReCheck {
-			// check if the tx exists first
-			if txInMempool {
+		// prepare cleanup closure to remove tx if marked
+		removeTx := false
+		defer func() {
+			if removeTx {
 				// remove the tx
 				if err := m.mempl.Remove(tx); err != nil {
 					m.logger.Debug(
 						"failed to remove tx from app-side mempool when purging for re-check failure",
 						"removal-err", err,
-						"check-tx-err", checkTxError,
 					)
 				}
 			}
+		}()
+
+		// run the checkTxHandler
+		res, checkTxError := m.checkTxHandler(req)
+		// if re-check fails for a transaction, we'll need to explicitly purge the tx from
+		// the app-side mempool
+		if isInvalidCheckTxExecution(res, checkTxError) && isReCheck && txInMempool {
+			removeTx = true
+		}
+
+		sdkCtx := m.GetContextForTx(req)
+		lane, err := m.matchLane(sdkCtx, tx)
+		if err != nil {
+			if isReCheck && txInMempool {
+				removeTx = true
+			}
+
+			m.logger.Debug("failed to match lane", "lane", lane, "err", err)
+			return sdkerrors.ResponseCheckTxWithEvents(
+				err,
+				0,
+				0,
+				nil,
+				false,
+			), nil
+		}
+
+		consensusParams := sdkCtx.ConsensusParams()
+		laneSize := lane.GetMaxBlockSpace().MulInt64(consensusParams.GetBlock().GetMaxBytes()).TruncateInt64()
+
+		txSize := int64(len(req.Tx))
+		if txSize > laneSize {
+			if isReCheck && txInMempool {
+				removeTx = true
+			}
+
+			m.logger.Debug(
+				"tx size exceeds max block bytes",
+				"tx", tx,
+				"tx size", txSize,
+				"max bytes", laneSize,
+			)
+
+			return sdkerrors.ResponseCheckTxWithEvents(
+				fmt.Errorf("tx size exceeds max bytes for lane %s", lane.Name()),
+				0,
+				0,
+				nil,
+				false,
+			), nil
 		}
 
 		return res, checkTxError
 	}
 }
 
+// matchLane returns a Lane if the given tx matches the Lane.
+func (m MempoolParityCheckTx) matchLane(ctx sdk.Context, tx sdk.Tx) (block.Lane, error) {
+	var lane block.Lane
+	// find corresponding lane for this tx
+	for _, l := range m.mempl.Registry() {
+		if l.Match(ctx, tx) {
+			lane = l
+			break
+		}
+	}
+
+	if lane == nil {
+		m.logger.Debug(
+			"failed match tx to lane",
+			"tx", tx,
+		)
+
+		return nil, fmt.Errorf("failed match tx to lane")
+	}
+
+	return lane, nil
+}
+
 func isInvalidCheckTxExecution(resp *cmtabci.ResponseCheckTx, checkTxErr error) bool {
 	return resp == nil || resp.Code != 0 || checkTxErr != nil
 }
+
+// GetContextForTx is returns the latest committed state and sets the context given
+// the checkTx request.
+func (m MempoolParityCheckTx) GetContextForTx(req *cmtabci.RequestCheckTx) sdk.Context {
+	// Retrieve the commit multi-store which is used to retrieve the latest committed state.
+	ms := m.baseApp.CommitMultiStore().CacheMultiStore()
+
+	// Create a new context based off of the latest committed state.
+	header := cmtproto.Header{
+		Height:  m.baseApp.LastBlockHeight(),
+		ChainID: m.baseApp.ChainID(),
+	}
+	ctx, _ := sdk.NewContext(ms, header, true, m.baseApp.Logger()).CacheContext()
+
+	// Set the remaining important context values.
+	ctx = ctx.
+		WithTxBytes(req.Tx).
+		WithEventManager(sdk.NewEventManager()).
+		WithConsensusParams(m.baseApp.GetConsensusParams(ctx))
+
+	return ctx
+}
diff --git a/abci/checktx/mev_check_tx.go b/abci/checktx/mev_check_tx.go
@@ -17,7 +17,7 @@ import (
 	"github.com/skip-mev/block-sdk/v2/x/auction/types"
 )
 
-// MevCheckTxHandler is a wrapper around baseapp's CheckTx method that allows us to
+// MEVCheckTxHandler is a wrapper around baseapp's CheckTx method that allows us to
 // verify bid transactions against the latest committed state. All other transactions
 // are executed normally using base app's CheckTx. This defines all of the
 // dependencies that are required to verify a bid transaction.
@@ -69,7 +69,7 @@ type BaseApp interface {
 	ChainID() string
 }
 
-// NewCheckTxHandler constructs a new CheckTxHandler instance. This method fails if the given LanedMempool does not have a lane
+// NewMEVCheckTxHandler constructs a new CheckTxHandler instance. This method fails if the given LanedMempool does not have a lane
 // adhering to the MevLaneI interface
 func NewMEVCheckTxHandler(
 	baseApp BaseApp,
@@ -87,7 +87,7 @@ func NewMEVCheckTxHandler(
 	}
 }
 
-// CheckTxHandler is a wrapper around baseapp's CheckTx method that allows us to
+// CheckTx is a wrapper around baseapp's CheckTx method that allows us to
 // verify bid transactions against the latest committed state. All other transactions
 // are executed normally. We must verify each bid tx and all of its bundled transactions
 // before we can insert it into the mempool against the latest commit state because

diff --git a/abci/utils_test.go b/abci/utils_test.go
@@ -31,7 +31,7 @@ func (s *ProposalsTestSuite) setUpAnteHandler(expectedExecution map[sdk.Tx]bool)
 		txCache[hashStr] = pass
 	}
 
-	anteHandler := func(ctx sdk.Context, tx sdk.Tx, simulate bool) (newCtx sdk.Context, err error) {
+	anteHandler := func(ctx sdk.Context, tx sdk.Tx, _ bool) (newCtx sdk.Context, err error) {
 		bz, err := s.encodingConfig.TxConfig.TxEncoder()(tx)
 		s.Require().NoError(err)