From f822c99eb8dff737fdf607e72b0a31312b4b9253 Mon Sep 17 00:00:00 2001 From: Emil Georgiev Date: Fri, 17 May 2024 08:55:18 +0300 Subject: [PATCH 1/3] refactor(abci/checktx): remove duplication code, fix shadow errors, follow Go comments best practices --- abci/checktx/mempool_parity_check_tx.go | 21 +---- abci/checktx/mev_check_tx.go | 102 +++++++----------------- 2 files changed, 33 insertions(+), 90 deletions(-) diff --git a/abci/checktx/mempool_parity_check_tx.go b/abci/checktx/mempool_parity_check_tx.go index df761340..0b06aa9b 100644 --- a/abci/checktx/mempool_parity_check_tx.go +++ b/abci/checktx/mempool_parity_check_tx.go @@ -7,8 +7,6 @@ import ( cmtabci "github.com/cometbft/cometbft/abci/types" sdk "github.com/cosmos/cosmos-sdk/types" - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" - "github.com/skip-mev/block-sdk/v2/block" ) @@ -45,13 +43,7 @@ func (m MempoolParityCheckTx) CheckTx() CheckTx { // decode tx tx, err := m.txDecoder(req.Tx) if err != nil { - return sdkerrors.ResponseCheckTxWithEvents( - fmt.Errorf("failed to decode tx: %w", err), - 0, - 0, - nil, - false, - ), nil + return errorResponse(fmt.Errorf("failed to decode tx: %w", err)), nil } isReCheck := req.Type == cmtabci.CheckTxType_Recheck @@ -63,14 +55,7 @@ func (m MempoolParityCheckTx) CheckTx() CheckTx { "tx from comet mempool not found in app-side mempool", "tx", tx, ) - - return sdkerrors.ResponseCheckTxWithEvents( - fmt.Errorf("tx from comet mempool not found in app-side mempool"), - 0, - 0, - nil, - false, - ), nil + return errorResponse(fmt.Errorf("tx from comet mempool not found in app-side mempool")), nil } // run the checkTxHandler @@ -82,7 +67,7 @@ func (m MempoolParityCheckTx) CheckTx() CheckTx { // check if the tx exists first if m.mempl.Contains(tx) { // remove the tx - if err := m.mempl.Remove(tx); err != nil { + 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, diff --git a/abci/checktx/mev_check_tx.go b/abci/checktx/mev_check_tx.go index fa024968..a2c10f6c 100644 --- 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 @@ -95,67 +95,34 @@ func NewMEVCheckTxHandler( // during this process. func (handler *MEVCheckTxHandler) CheckTx() CheckTx { return func(req *cometabci.RequestCheckTx) (resp *cometabci.ResponseCheckTx, err error) { + l := handler.baseApp.Logger() defer func() { if rec := recover(); rec != nil { - handler.baseApp.Logger().Error( - "panic in check tx handler", - "err", rec, - ) - + l.Error("panic in check tx handler", "err", rec) err = fmt.Errorf("panic in check tx handler: %s", rec) - resp = sdkerrors.ResponseCheckTxWithEvents( - err, - 0, - 0, - nil, - false, - ) + resp = errorResponse(err) } }() tx, err := handler.txDecoder(req.Tx) if err != nil { - handler.baseApp.Logger().Info( - "failed to decode tx", - "err", err, - ) - - return sdkerrors.ResponseCheckTxWithEvents( - fmt.Errorf("failed to decode tx: %w", err), - 0, - 0, - nil, - false, - ), nil + l.Info("failed to decode tx", "err", err) + return errorResponse(fmt.Errorf("failed to decode tx: %w", err)), nil } // Attempt to get the bid info of the transaction. bidInfo, err := handler.mevLane.GetAuctionBidInfo(tx) if err != nil { - handler.baseApp.Logger().Info( - "failed to get auction bid info", - "err", err, - ) - - return sdkerrors.ResponseCheckTxWithEvents( - fmt.Errorf("failed to get auction bid info: %w", err), - 0, - 0, - nil, - false, - ), nil + l.Info("failed to get auction bid info", "err", err) + return errorResponse(fmt.Errorf("failed to get auction bid info: %w", err)), nil } // If this is not a bid transaction, we just execute it normally. if bidInfo == nil { - resp, err := handler.checkTxHandler(req) + resp, err = handler.checkTxHandler(req) if err != nil { - handler.baseApp.Logger().Info( - "failed to execute check tx", - "err", err, - ) + l.Info("failed to execute check tx", "err", err) } - return resp, err } @@ -167,7 +134,7 @@ func (handler *MEVCheckTxHandler) CheckTx() CheckTx { // Verify the bid transaction. gasInfo, err := handler.ValidateBidTx(ctx, tx, bidInfo) if err != nil { - handler.baseApp.Logger().Info( + l.Info( "invalid bid tx", "err", err, "height", ctx.BlockHeight(), @@ -179,24 +146,18 @@ func (handler *MEVCheckTxHandler) CheckTx() CheckTx { // attempt to remove the bid from the MEVLane (if it exists) if handler.mevLane.Contains(tx) { - if err := handler.mevLane.Remove(tx); err != nil { - handler.baseApp.Logger().Error( - "failed to remove bid transaction from mev-lane", - "err", err, - ) + if err = handler.mevLane.Remove(tx); err != nil { + l.Error("failed to remove bid transaction from mev-lane", "err", err) } } - return sdkerrors.ResponseCheckTxWithEvents( - fmt.Errorf("invalid bid tx: %w", err), - gasInfo.GasWanted, - gasInfo.GasUsed, - nil, - false, - ), nil + resp = errorResponse(fmt.Errorf("invalid bid tx: %w", err)) + resp.GasWanted = int64(gasInfo.GasWanted) + resp.GasUsed = int64(gasInfo.GasUsed) + return resp, nil } - handler.baseApp.Logger().Info( + l.Info( "valid bid tx", "height", ctx.BlockHeight(), "bid_height", bidInfo.Timeout, @@ -206,19 +167,12 @@ func (handler *MEVCheckTxHandler) CheckTx() CheckTx { ) // If the bid transaction is valid, we know we can insert it into the mempool for consideration in the next block. - if err := handler.mevLane.Insert(ctx, tx); err != nil { - handler.baseApp.Logger().Info( - "invalid bid tx; failed to insert bid transaction into mempool", - "err", err, - ) - - return sdkerrors.ResponseCheckTxWithEvents( - fmt.Errorf("invalid bid tx; failed to insert bid transaction into mempool: %w", err), - gasInfo.GasWanted, - gasInfo.GasUsed, - nil, - false, - ), nil + if err = handler.mevLane.Insert(ctx, tx); err != nil { + l.Info("invalid bid tx; failed to insert bid transaction into mempool", "err", err) + resp = errorResponse(fmt.Errorf("invalid bid tx; failed to insert bid transaction into mempool: %w", err)) + resp.GasWanted = int64(gasInfo.GasWanted) + resp.GasUsed = int64(gasInfo.GasUsed) + return resp, nil } return &cometabci.ResponseCheckTx{ @@ -299,3 +253,7 @@ func (handler *MEVCheckTxHandler) GetContextForBidTx(req *cometabci.RequestCheck return ctx } + +func errorResponse(err error) *cometabci.ResponseCheckTx { + return sdkerrors.ResponseCheckTxWithEvents(err, 0, 0, nil, false) +} From f0c87dce0d1716a97362852218d27d5da3991366 Mon Sep 17 00:00:00 2001 From: Emil Georgiev Date: Sat, 18 May 2024 19:22:20 +0300 Subject: [PATCH 2/3] refactor(abci/checktx): remove duplication and simplify the code --- abci/checktx/mempool_parity_check_tx.go | 36 ++++++++++++------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/abci/checktx/mempool_parity_check_tx.go b/abci/checktx/mempool_parity_check_tx.go index 0b06aa9b..499e4783 100644 --- a/abci/checktx/mempool_parity_check_tx.go +++ b/abci/checktx/mempool_parity_check_tx.go @@ -40,17 +40,19 @@ func NewMempoolParityCheckTx(logger log.Logger, mempl block.Mempool, txDecoder s // in the app-side mempool on ReCheckTx. func (m MempoolParityCheckTx) CheckTx() CheckTx { return func(req *cmtabci.RequestCheckTx) (*cmtabci.ResponseCheckTx, error) { - // decode tx + // This method check the tx only if the CheckTxType mode is ReCheck. Otherwise, we continue to the next checkHandler. + if req.Type != cmtabci.CheckTxType_Recheck { + return m.checkTxHandler(req) + } + tx, err := m.txDecoder(req.Tx) if err != nil { return errorResponse(fmt.Errorf("failed to decode tx: %w", err)), nil } - isReCheck := req.Type == cmtabci.CheckTxType_Recheck - - // if the mode is ReCheck and the app's mempool does not contain the given tx, we fail - // immediately, to purge the tx from the comet mempool. - if isReCheck && !m.mempl.Contains(tx) { + // In the ReCheck mode the app's mempool should contain the given tx. + // If not, we fail immediately, to purge the tx from the comet mempool. + if !m.mempl.Contains(tx) { m.logger.Debug( "tx from comet mempool not found in app-side mempool", "tx", tx, @@ -61,19 +63,15 @@ func (m MempoolParityCheckTx) CheckTx() CheckTx { // 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 m.mempl.Contains(tx) { - // 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, - ) - } + // if re-check fails for a transaction, we'll need to explicitly purge the tx from the app-side mempool + if isInvalidCheckTxExecution(res, checkTxError) { + // 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, + ) } } From f45cdfcaeaa1504cef848369b1a8edee0f99355e Mon Sep 17 00:00:00 2001 From: Emil Georgiev Date: Sun, 19 May 2024 11:32:35 +0300 Subject: [PATCH 3/3] fix unit test --- abci/checktx/check_tx_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/abci/checktx/check_tx_test.go b/abci/checktx/check_tx_test.go index 39c259fc..b4353f43 100644 --- a/abci/checktx/check_tx_test.go +++ b/abci/checktx/check_tx_test.go @@ -172,7 +172,7 @@ func (s *CheckTxTestSuite) TestMempoolParityCheckTx() { nil, ) - res, err := handler.CheckTx()(&cometabci.RequestCheckTx{Tx: []byte("invalid-tx")}) + res, err := handler.CheckTx()(&cometabci.RequestCheckTx{Type: cometabci.CheckTxType_Recheck, Tx: []byte("invalid-tx")}) s.Require().NoError(err) s.Require().Equal(uint32(1), res.Code)