Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: mempool lane size check on CheckTx #561

Merged
merged 17 commits into from
Jul 2, 2024
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@

> **Note**: The BlockSDK is open source software that Skip maintains. We strive to be responsive to questions and issues within 1-2 weeks - please ask in our [#block-sdk-support discord](https://discord.com/invite/hFeHVAE26P) channel, or open a GitHub issue!

**Please note the status of BlockSDK maintainenace:**
**Please note the status of BlockSDK maintenance:**

1. We are not currently providing hands-on support for new integrations.
2. We have not yet completed our entire testing process for the FreeLane. We recommend integrators who want to have the best experience utilize the MEV Lane and the Default Lane only at this time.
Expand Down
1 change: 1 addition & 0 deletions abci/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
81 changes: 73 additions & 8 deletions abci/checktx/check_tx_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)

Expand All @@ -69,6 +82,7 @@ func (s *CheckTxTestSuite) TestCheckTxMempoolParity() {
ba := &baseApp{
s.Ctx,
}

mevLaneHandler := checktx.NewMEVCheckTxHandler(
ba,
cacheDecoder.TxDecoder(),
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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() {
Expand All @@ -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")})
Expand All @@ -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)

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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")
}

Expand All @@ -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.
Expand Down
97 changes: 97 additions & 0 deletions abci/checktx/mempool_parity_check_tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import (
"fmt"

cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"

"cosmossdk.io/log"

cmtabci "github.com/cometbft/cometbft/abci/types"
Expand All @@ -26,6 +28,10 @@

// 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.
Expand All @@ -34,12 +40,14 @@
mempl block.Mempool,
txDecoder sdk.TxDecoder,
checkTxHandler CheckTx,
baseApp BaseApp,
) MempoolParityCheckTx {
return MempoolParityCheckTx{
logger: logger,
mempl: mempl,
txDecoder: txDecoder,
checkTxHandler: checkTxHandler,
baseApp: baseApp,
}
}

Expand Down Expand Up @@ -98,10 +106,99 @@
}
}

sdkCtx := m.GetContextForTx(req)
aljo242 marked this conversation as resolved.
Show resolved Hide resolved
lane, err := m.matchLane(sdkCtx, tx)
if err != nil {
m.logger.Debug("failed to match lane", "lane", lane, "err", err)
return sdkerrors.ResponseCheckTxWithEvents(
err,
0,
0,
nil,
false,
), nil

Check warning on line 119 in abci/checktx/mempool_parity_check_tx.go

View check run for this annotation

Codecov / codecov/patch

abci/checktx/mempool_parity_check_tx.go#L112-L119

Added lines #L112 - L119 were not covered by tests
}

consensusParams := sdkCtx.ConsensusParams()
aljo242 marked this conversation as resolved.
Show resolved Hide resolved
laneSize := lane.GetMaxBlockSpace().MulInt64(consensusParams.GetBlock().GetMaxBytes()).TruncateInt64()

txSize := int64(len(req.Tx))
if txSize > laneSize {
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 block bytes"),
aljo242 marked this conversation as resolved.
Show resolved Hide resolved
0,
0,
nil,
false,
), nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to do this check before the entire ante-handler sequence is run?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure - I think the case with a huge Tx is much less likely than the case where youre Tx just fails the antehandler. So i would rather run that and have the matching logic done later only when we need to


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,
)

Check warning on line 162 in abci/checktx/mempool_parity_check_tx.go

View check run for this annotation

Codecov / codecov/patch

abci/checktx/mempool_parity_check_tx.go#L159-L162

Added lines #L159 - L162 were not covered by tests

return nil, fmt.Errorf("failed match tx to lane")

Check warning on line 164 in abci/checktx/mempool_parity_check_tx.go

View check run for this annotation

Codecov / codecov/patch

abci/checktx/mempool_parity_check_tx.go#L164

Added line #L164 was not covered by tests
}

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 context to the correct checking mode.
switch req.Type {
case cmtabci.CheckTxType_New:
ctx = ctx.WithIsCheckTx(true)
case cmtabci.CheckTxType_Recheck:
ctx = ctx.WithIsReCheckTx(true)
default:
panic("unknown check tx type")

Check warning on line 194 in abci/checktx/mempool_parity_check_tx.go

View check run for this annotation

Codecov / codecov/patch

abci/checktx/mempool_parity_check_tx.go#L193-L194

Added lines #L193 - L194 were not covered by tests
}

// Set the remaining important context values.
ctx = ctx.
WithTxBytes(req.Tx).
WithEventManager(sdk.NewEventManager()).
WithConsensusParams(m.baseApp.GetConsensusParams(ctx))

return ctx
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you benchmarket how long this takes just so we have a reference on the slowdown we expected? I imagine some of this is not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you imagine benchmarking this?

Start a small in mem test app and call this on it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

benchmarking with a real simapp would be better, but probably not important to get this shipped. fine as is

6 changes: 3 additions & 3 deletions abci/checktx/mev_check_tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions block/base/lane.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,12 @@ type BaseLane struct { //nolint
// that are waiting to be processed.
block.LaneMempool

// matchHandler is the function that determines whether or not a transaction
// matchHandler is the function that determines whether a transaction
// should be processed by this lane.
matchHandler MatchHandler

// prepareLaneHandler is the function that is called when a new proposal is being
// requested and the lane needs to submit transactions it wants included in the block.
// requested and the lane needs to submit transactions it wants to be included in the block.
prepareLaneHandler PrepareLaneHandler

// processLaneHandler is the function that is called when a new proposal is being
Expand Down
2 changes: 1 addition & 1 deletion block/base/match.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
// DefaultMatchHandler returns a default implementation of the MatchHandler. It matches all
// transactions.
func DefaultMatchHandler() MatchHandler {
return func(ctx sdk.Context, tx sdk.Tx) bool {
return func(_ sdk.Context, _ sdk.Tx) bool {

Check warning on line 12 in block/base/match.go

View check run for this annotation

Codecov / codecov/patch

block/base/match.go#L12

Added line #L12 was not covered by tests
return true
}
}
Expand Down
Loading
Loading