fix(auction): Adding extra check on bundler timeouts (#156)

* validate bundler txs

* nits

* lint

lanes/mev/check_tx.go

@@ -166,7 +166,7 @@ func (handler *CheckTxHandler) CheckTx() CheckTx {
 				"bid_height", bidInfo.Timeout,
 				"bidder", bidInfo.Bidder,
 				"bid", bidInfo.Bid,
-				"removing tx from mempool", true,
+				"is_recheck_tx", ctx.IsReCheckTx(),
 			)
 
 			// attempt to remove the bid from the MEVLane (if it exists)

lanes/mev/factory.go

@@ -79,25 +79,37 @@ func (config *DefaultAuctionFactory) GetAuctionBidInfo(tx sdk.Tx) (*types.BidInf
 		return nil, fmt.Errorf("invalid bidder address (%s): %w", msg.Bidder, err)
 	}
 
-	timeoutTx, ok := tx.(TxWithTimeoutHeight)
-	if !ok {
-		return nil, fmt.Errorf("cannot extract timeout; transaction does not implement TxWithTimeoutHeight")
+	height, err := config.GetTimeoutHeight(tx)
+	if err != nil {
+		return nil, err
 	}
 
-	signers, err := config.getBundleSigners(msg.Transactions)
+	signers, timeouts, err := config.getBundleInfo(msg.Transactions)
 	if err != nil {
 		return nil, err
 	}
 
 	return &types.BidInfo{
-		Bid:          msg.Bid,
-		Bidder:       bidder,
-		Transactions: msg.Transactions,
-		Timeout:      timeoutTx.GetTimeoutHeight(),
-		Signers:      signers,
+		Bid:                 msg.Bid,
+		Bidder:              bidder,
+		Transactions:        msg.Transactions,
+		TransactionTimeouts: timeouts,
+		Timeout:             height,
+		Signers:             signers,
 	}, nil
 }
 
+// GetTimeoutHeight returns the timeout height of the transaction.
+func (config *DefaultAuctionFactory) GetTimeoutHeight(tx sdk.Tx) (uint64, error) {
+	timeoutTx, ok := tx.(TxWithTimeoutHeight)
+	if !ok {
+		return 0, fmt.Errorf("cannot extract timeout; transaction does not implement TxWithTimeoutHeight")
+	}
+
+	return timeoutTx.GetTimeoutHeight(), nil
+}
+
+// MatchHandler defines a default function that checks if a transaction matches the mev lane.
 func (config *DefaultAuctionFactory) MatchHandler() base.MatchHandler {
 	return func(ctx sdk.Context, tx sdk.Tx) bool {
 		bidInfo, err := config.GetAuctionBidInfo(tx)
@@ -105,31 +117,38 @@ func (config *DefaultAuctionFactory) MatchHandler() base.MatchHandler {
 	}
 }
 
-// getBundleSigners defines a default function that returns the signers of all transactions in
-// a bundle. In the default case, each bundle transaction will be an sdk.Tx and the
-// signers are the signers of each sdk.Msg in the transaction.
-func (config *DefaultAuctionFactory) getBundleSigners(bundle [][]byte) ([]map[string]struct{}, error) {
-	bundleSigners := make([]map[string]struct{}, 0)
+// getBundleInfo defines a default function that returns the signers of all transactions in
+// a bundle as well as each bundled txs timeout. In the default case, each bundle transaction
+// will be an sdk.Tx and the signers are the signers of each sdk.Msg in the transaction.
+func (config *DefaultAuctionFactory) getBundleInfo(bundle [][]byte) ([]map[string]struct{}, []uint64, error) {
+	bundleSigners := make([]map[string]struct{}, len(bundle))
+	timeouts := make([]uint64, len(bundle))
 
-	for _, tx := range bundle {
+	for index, tx := range bundle {
 		sdkTx, err := config.txDecoder(tx)
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 
 		txSigners := make(map[string]struct{})
 
 		signers, err := config.signerExtractor.GetSigners(sdkTx)
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 
 		for _, signer := range signers {
 			txSigners[signer.Signer.String()] = struct{}{}
 		}
 
-		bundleSigners = append(bundleSigners, txSigners)
+		timeout, err := config.GetTimeoutHeight(sdkTx)
+		if err != nil {
+			return nil, nil, err
+		}
+
+		bundleSigners[index] = txSigners
+		timeouts[index] = timeout
 	}
 
-	return bundleSigners, nil
+	return bundleSigners, timeouts, nil
 }

tests/app/app.go

@@ -268,9 +268,9 @@ func New(
 		Logger:          app.Logger(),
 		TxEncoder:       app.txConfig.TxEncoder(),
 		TxDecoder:       app.txConfig.TxDecoder(),
-		MaxBlockSpace:   math.LegacyZeroDec(), // This means the lane has no limit on block space.
+		MaxBlockSpace:   math.LegacyMustNewDecFromStr("0.2"),
 		SignerExtractor: signer_extraction.NewDefaultAdapter(),
-		MaxTxs:          0, // This means the lane has no limit on the number of transactions it can store.
+		MaxTxs:          1000,
 	}
 	mevLane := mev.NewMEVLane(
 		mevConfig,
@@ -282,9 +282,9 @@ func New(
 		Logger:          app.Logger(),
 		TxEncoder:       app.txConfig.TxEncoder(),
 		TxDecoder:       app.txConfig.TxDecoder(),
-		MaxBlockSpace:   math.LegacyZeroDec(),
+		MaxBlockSpace:   math.LegacyMustNewDecFromStr("0.2"),
 		SignerExtractor: signer_extraction.NewDefaultAdapter(),
-		MaxTxs:          0,
+		MaxTxs:          1000,
 	}
 	freeLane := free.NewFreeLane(
 		freeConfig,
@@ -297,9 +297,9 @@ func New(
 		Logger:          app.Logger(),
 		TxEncoder:       app.txConfig.TxEncoder(),
 		TxDecoder:       app.txConfig.TxDecoder(),
-		MaxBlockSpace:   math.LegacyZeroDec(),
+		MaxBlockSpace:   math.LegacyMustNewDecFromStr("0.6"),
 		SignerExtractor: signer_extraction.NewDefaultAdapter(),
-		MaxTxs:          0,
+		MaxTxs:          1000,
 	}
 	defaultLane := defaultlane.NewDefaultLane(defaultConfig)
 

tests/integration/block_sdk_integration_test.go

@@ -8,6 +8,7 @@ import (
 	"github.com/strangelove-ventures/interchaintest/v7"
 	"github.com/strangelove-ventures/interchaintest/v7/chain/cosmos"
 	"github.com/strangelove-ventures/interchaintest/v7/ibc"
+	ictestutil "github.com/strangelove-ventures/interchaintest/v7/testutil"
 	"github.com/stretchr/testify/suite"
 
 	"github.com/skip-mev/block-sdk/tests/integration"
@@ -16,7 +17,7 @@ import (
 
 var (
 	// config params
-	numValidators = 4
+	numValidators = 1
 	numFullNodes  = 0
 	denom         = "stake"
 
@@ -36,6 +37,10 @@ var (
 		},
 	}
 
+	consensusParams = ictestutil.Toml{
+		"timeout_commit": "3500ms",
+	}
+
 	// interchain specification
 	spec = &interchaintest.ChainSpec{
 		ChainName:     "block-sdk",
@@ -63,6 +68,7 @@ var (
 			NoHostMount:            noHostMount,
 			UsingNewGenesisCommand: true,
 			ModifyGenesis:          cosmos.ModifyGenesis(genesisKV),
+			ConfigFileOverrides:    map[string]any{"config/config.toml": ictestutil.Toml{"consensus": consensusParams}},
 		},
 	}
 )

tests/integration/chain_setup.go

@@ -47,7 +47,7 @@ type KeyringOverride struct {
 // and returns the associated chain
 func ChainBuilderFromChainSpec(t *testing.T, spec *interchaintest.ChainSpec) ibc.Chain {
 	// require that NumFullNodes == NumValidators == 4
-	require.Equal(t, *spec.NumValidators, 4)
+	require.Equal(t, *spec.NumValidators, 1)
 
 	cf := interchaintest.NewBuiltinChainFactory(zaptest.NewLogger(t), []*interchaintest.ChainSpec{spec})
 
@@ -106,9 +106,7 @@ func (s *IntegrationTestSuite) CreateTx(ctx context.Context, chain *cosmos.Cosmo
 	}
 
 	// get gas for tx
-	_, gas, err := tx.CalculateGas(cc, txf, msgs...)
-	s.Require().NoError(err)
-	txf.WithGas(gas)
+	txf.WithGas(25000000)
 
 	// update sequence number
 	txf = txf.WithSequence(txf.Sequence() + seqIncrement)
@@ -181,8 +179,8 @@ func (s *IntegrationTestSuite) CreateAuctionBidMsg(ctx context.Context, searcher
 // BroadcastTxs broadcasts the given messages for each user. This function returns the broadcasted txs. If a message
 // is not expected to be included in a block, set SkipInclusionCheck to true and the method
 // will not block on the tx's inclusion in a block, otherwise this method will block on the tx's inclusion
-func (s *IntegrationTestSuite) BroadcastTxs(ctx context.Context, chain *cosmos.CosmosChain, msgsPerUser []Tx) [][]byte {
-	return s.BroadcastTxsWithCallback(ctx, chain, msgsPerUser, nil)
+func (s *IntegrationTestSuite) BroadcastTxs(ctx context.Context, chain *cosmos.CosmosChain, txs []Tx) [][]byte {
+	return s.BroadcastTxsWithCallback(ctx, chain, txs, nil)
 }
 
 // BroadcastTxs broadcasts the given messages for each user. This function returns the broadcasted txs. If a message
@@ -192,13 +190,13 @@ func (s *IntegrationTestSuite) BroadcastTxs(ctx context.Context, chain *cosmos.C
 func (s *IntegrationTestSuite) BroadcastTxsWithCallback(
 	ctx context.Context,
 	chain *cosmos.CosmosChain,
-	msgsPerUser []Tx,
+	txs []Tx,
 	cb func(tx []byte, resp *rpctypes.ResultTx),
 ) [][]byte {
-	txs := make([][]byte, len(msgsPerUser))
+	rawTxs := make([][]byte, len(txs))
 
-	for i, msg := range msgsPerUser {
-		txs[i] = s.CreateTx(ctx, chain, msg.User, msg.SequenceIncrement, msg.Height, msg.GasPrice, msg.Msgs...)
+	for i, msg := range txs {
+		rawTxs[i] = s.CreateTx(ctx, chain, msg.User, msg.SequenceIncrement, msg.Height, msg.GasPrice, msg.Msgs...)
 	}
 
 	// broadcast each tx
@@ -210,12 +208,12 @@ func (s *IntegrationTestSuite) BroadcastTxsWithCallback(
 
 	s.T().Logf("broadcasting transactions at latest height of %d", statusResp.SyncInfo.LatestBlockHeight)
 
-	for i, tx := range txs {
+	for i, tx := range rawTxs {
 		// broadcast tx
 		resp, err := client.BroadcastTxSync(ctx, tx)
 
 		// check execution was successful
-		if !msgsPerUser[i].ExpectFail {
+		if !txs[i].ExpectFail {
 			s.Require().Equal(resp.Code, uint32(0))
 		} else {
 			if resp != nil {
@@ -224,14 +222,13 @@ func (s *IntegrationTestSuite) BroadcastTxsWithCallback(
 				s.Require().Error(err)
 			}
 		}
-
 	}
 
 	// block on all txs being included in block
 	eg := errgroup.Group{}
-	for i, tx := range txs {
+	for i, tx := range rawTxs {
 		// if we don't expect this tx to be included.. skip it
-		if msgsPerUser[i].SkipInclusionCheck || msgsPerUser[i].ExpectFail {
+		if txs[i].SkipInclusionCheck || txs[i].ExpectFail {
 			continue
 		}
 
@@ -254,7 +251,7 @@ func (s *IntegrationTestSuite) BroadcastTxsWithCallback(
 
 	s.Require().NoError(eg.Wait())
 
-	return txs
+	return rawTxs
 }
 
 // QueryAuctionParams queries the x/auction module's params
@@ -373,6 +370,20 @@ func VerifyBlock(t *testing.T, block *rpctypes.ResultBlock, offset int, bidTxHas
 	}
 }
 
+// VerifyBlockWithExpectedBlock takes in a list of raw tx bytes and compares each tx hash to the tx hashes in the block.
+// The expected block is the block that should be returned by the chain at the given height.
+func VerifyBlockWithExpectedBlock(t *testing.T, chain *cosmos.CosmosChain, height uint64, txs [][]byte) {
+	block := Block(t, chain, int64(height))
+	blockTxs := block.Block.Data.Txs[1:]
+
+	t.Logf("verifying block %d", height)
+	require.Equal(t, len(txs), len(blockTxs))
+	for i, tx := range txs {
+		t.Logf("verifying tx %d; expected %s, got %s", i, TxHash(tx), TxHash(blockTxs[i]))
+		require.Equal(t, TxHash(tx), TxHash(blockTxs[i]))
+	}
+}
+
 func TxHash(tx []byte) string {
 	return strings.ToUpper(hex.EncodeToString(comettypes.Tx(tx).Hash()))
 }
@@ -446,3 +457,7 @@ func (s *IntegrationTestSuite) keyringDirFromNode() string {
 
 	return localDir
 }
+
+func escrowAddressIncrement(bid math.Int, proposerFee math.LegacyDec) int64 {
+	return int64(bid.Sub(math.Int(math.LegacyNewDecFromInt(bid).Mul(proposerFee).RoundInt())).Int64())
+}

x/auction/ante/utils.go

@@ -41,9 +41,9 @@ func ValidateTimeout(ctx sdk.Context, timeout int64) error {
 		height++
 	}
 
-	if height > timeout {
+	if height != timeout {
 		return fmt.Errorf(
-			"timeout height cannot be less than the current block height (timeout: %d, current block height: %d)",
+			"you must set the timeout height to be the next block height got %d, expected %d",
 			timeout,
 			height,
 		)

x/auction/keeper/auction.go

@@ -37,7 +37,8 @@ func (k Keeper) ValidateBidInfo(ctx sdk.Context, highestBid sdk.Coin, bidInfo *t
 		}
 	}
 
-	return nil
+	// Validate the timeouts of the transactions in the bundle.
+	return k.ValidateBundleTimeouts(bidInfo)
 }
 
 // ValidateAuctionBid validates that the bidder has sufficient funds to participate in the auction and that the bid amount
@@ -180,6 +181,31 @@ func (k Keeper) ValidateAuctionBundle(bidder sdk.AccAddress, bundleSigners []map
 	return nil
 }
 
+// ValidateBundleTimeouts validates that the timeouts of the transactions in the bundle are valid. We consider
+// the timeouts valid iff all of the bidders txs in the bundle are equal to the timeout of the bid. This is
+// mandatory as otherwise other searchers can potentially include other searcher's transactions in their bundles.
+func (k Keeper) ValidateBundleTimeouts(bidInfo *types.BidInfo) error {
+	bidder := bidInfo.Bidder.String()
+	bidTimeout := bidInfo.Timeout
+
+	for index, bundleTimeout := range bidInfo.TransactionTimeouts {
+		signers := bidInfo.Signers[index]
+		if _, ok := signers[bidder]; !ok {
+			continue
+		}
+
+		if bundleTimeout != bidTimeout {
+			return fmt.Errorf(
+				"searchers must set the timeout of all of their transactions in the bundle to be the same as the bid timeout; got %d, expected %d",
+				bundleTimeout,
+				bidTimeout,
+			)
+		}
+	}
+
+	return nil
+}
+
 // filterSigners removes any signers from the currentSigners map that are not in the txSigners map.
 func filterSigners(currentSigners, txSigners map[string]struct{}) {
 	for signer := range currentSigners {

x/auction/keeper/auction_test.go

@@ -226,3 +226,59 @@ func (s *KeeperTestSuite) TestValidateBundle() {
 		})
 	}
 }
+
+func (s *KeeperTestSuite) TestValidateBundleTimeouts() {
+	s.Run("can validate valid bundle timeouts", func() {
+		bidder := sdk.AccAddress([]byte("bidder"))
+		other := sdk.AccAddress([]byte("other"))
+
+		bidInfo := &types.BidInfo{
+			Bidder:              sdk.AccAddress([]byte("bidder")),
+			Timeout:             10,
+			TransactionTimeouts: []uint64{0, 10, 0},
+			Signers: []map[string]struct{}{
+				{other.String(): {}},
+				{bidder.String(): {}},
+				{other.String(): {}},
+			},
+		}
+
+		err := s.auctionkeeper.ValidateBundleTimeouts(bidInfo)
+		s.Require().NoError(err)
+	})
+
+	s.Run("can invalidate invalid bundle timeouts", func() {
+		bidder := sdk.AccAddress([]byte("bidder"))
+		other := sdk.AccAddress([]byte("other"))
+
+		bidInfo := &types.BidInfo{
+			Bidder:              sdk.AccAddress([]byte("bidder")),
+			Timeout:             10,
+			TransactionTimeouts: []uint64{0, 10, 0},
+			Signers: []map[string]struct{}{
+				{other.String(): {}},
+				{bidder.String(): {}},
+				{bidder.String(): {}},
+			},
+		}
+
+		err := s.auctionkeeper.ValidateBundleTimeouts(bidInfo)
+		s.Require().Error(err)
+	})
+
+	s.Run("can invalidate invalid bundle timeout with single tx", func() {
+		bidder := sdk.AccAddress([]byte("bidder"))
+
+		bidInfo := &types.BidInfo{
+			Bidder:              sdk.AccAddress([]byte("bidder")),
+			Timeout:             10,
+			TransactionTimeouts: []uint64{0},
+			Signers: []map[string]struct{}{
+				{bidder.String(): {}},
+			},
+		}
+
+		err := s.auctionkeeper.ValidateBundleTimeouts(bidInfo)
+		s.Require().Error(err)
+	})
+}

x/auction/types/bid_info.go

@@ -4,9 +4,16 @@ import sdk "github.com/cosmos/cosmos-sdk/types"
 
 // BidInfo defines the information about a bid to the auction house.
 type BidInfo struct {
-	Bidder       sdk.AccAddress
-	Bid          sdk.Coin
+	// Bidder is the address of the bidder.
+	Bidder sdk.AccAddress
+	// Bid is the amount of coins that the bidder is bidding.
+	Bid sdk.Coin
+	// Transactions is the bundle of transactions that the bidder is committing to.
 	Transactions [][]byte
-	Timeout      uint64
-	Signers      []map[string]struct{}
+	// Timeout is the block height at which the bid transaction will be executed. This must be the next block height.
+	Timeout uint64
+	// Signers is the list of signers for each transaction in the bundle.
+	Signers []map[string]struct{}
+	// TransactionTimeouts is the list of timeouts for each transaction in the bundle.
+	TransactionTimeouts []uint64
 }