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.

Sign up for GitHub

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: Gas limit check on PrepareProposal #277

Merged
merged 7 commits into from
Dec 28, 2023
Merged

fix: Gas limit check on PrepareProposal #277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4b69847
init
davidterpay Dec 28, 2023
d10d063
size check after all other checks
davidterpay Dec 28, 2023
514bf7f
nit
davidterpay Dec 28, 2023
7431245
nit v2
davidterpay Dec 28, 2023
4144c0b
test set up
davidterpay Dec 28, 2023
c104919
go work nit
davidterpay Dec 28, 2023
223c5ba
max block gas with -1
davidterpay Dec 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 47 additions & 5 deletions abci/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,21 +58,32 @@ func NewProposalHandler(
func (h *ProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHandler {
return func(ctx sdk.Context, req abci.RequestPrepareProposal) abci.ResponsePrepareProposal {
var (
selectedTxs [][]byte
totalTxBytes int64
selectedTxs [][]byte
totalTxBytes int64
totalGasLimit int64
)

bidTxIterator := h.mempool.AuctionBidSelect(ctx)
txsToRemove := make(map[sdk.Tx]struct{}, 0)
seenTxs := make(map[string]struct{}, 0)

maxGasLimit := ctx.ConsensusParams().Block.MaxGas

// Attempt to select the highest bid transaction that is valid and whose
// bundled transactions are valid.
selectBidTxLoop:
for ; bidTxIterator != nil; bidTxIterator = bidTxIterator.Next() {
cacheCtx, write := ctx.CacheContext()
tmpBidTx := bidTxIterator.Tx()

// Retrieve the gas limit of the transaction
feeTx, ok := tmpBidTx.(sdk.FeeTx)
if !ok {
txsToRemove[tmpBidTx] = struct{}{}
continue selectBidTxLoop
}
gasLimit := feeTx.GetGas()

bidTxBz, err := h.PrepareProposalVerifyTx(cacheCtx, tmpBidTx)
if err != nil {
txsToRemove[tmpBidTx] = struct{}{}
Expand Down Expand Up @@ -103,6 +114,15 @@ func (h *ProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHandler {
continue selectBidTxLoop
}

feeTx, ok := refTx.(sdk.FeeTx)
if !ok {
// Malformed bundled transaction, so we remove the bid transaction
// and try the next top bid.
txsToRemove[tmpBidTx] = struct{}{}
continue selectBidTxLoop
}
gasLimit += feeTx.GetGas()
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if PrepareProposalVerifyTx fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gas limit will be reset on the next iteration so this is safe


txBz, err := h.PrepareProposalVerifyTx(cacheCtx, refTx)
if err != nil {
// Invalid bundled transaction, so we remove the bid transaction
Expand All @@ -114,6 +134,15 @@ func (h *ProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHandler {
sdkTxBytes[index] = txBz
}

if maxGasLimit > 0 && int64(gasLimit) > maxGasLimit {
// The gas limit of the bundled transactions exceeds the maximum gas
// limit of the block, so we remove the bid transaction and try the
// next top bid.
txsToRemove[tmpBidTx] = struct{}{}
continue selectBidTxLoop
}
totalGasLimit = int64(gasLimit)

// At this point, both the bid transaction itself and all the bundled
// transactions are valid. So we select the bid transaction along with
// all the bundled transactions. We also mark these transactions as seen and
Expand Down Expand Up @@ -177,13 +206,26 @@ func (h *ProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHandler {
}

txSize := int64(len(txBz))
if totalTxBytes += txSize; totalTxBytes <= req.MaxTxBytes {
selectedTxs = append(selectedTxs, txBz)
} else {
if totalTxBytes += txSize; totalTxBytes > req.MaxTxBytes {
// We've reached capacity per req.MaxTxBytes so we cannot select any
// more transactions.
break selectTxLoop
}

// Gas Limit check
feeTx, ok := memTx.(sdk.FeeTx)
if !ok {
txsToRemove[memTx] = struct{}{}
continue selectTxLoop
}
gasLimit := feeTx.GetGas()
if totalGasLimit += int64(gasLimit); maxGasLimit > 0 && totalGasLimit > maxGasLimit {
// We've reached capacity per maxGasLimit so we cannot select any more
// transactions.
break selectTxLoop
}

selectedTxs = append(selectedTxs, txBz)
}

// Remove all invalid transactions from the mempool.
Expand Down
162 changes: 162 additions & 0 deletions abci/abci_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@ import (
"encoding/hex"
"fmt"
"math/rand"
"os"
"testing"
"time"

abcitypes "github.com/cometbft/cometbft/abci/types"
"github.com/cometbft/cometbft/libs/log"
tmproto "github.com/cometbft/cometbft/proto/tendermint/types"
storetypes "github.com/cosmos/cosmos-sdk/store/types"
"github.com/cosmos/cosmos-sdk/testutil"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -69,6 +71,13 @@ func (suite *ABCITestSuite) SetupTest() {
testCtx := testutil.DefaultContextWithDB(suite.T(), suite.key, storetypes.NewTransientStoreKey("transient_test"))
suite.ctx = testCtx.Ctx.WithBlockHeight(1)

cp := tmproto.ConsensusParams{
Block: &tmproto.BlockParams{
MaxGas: int64(1000000000000000000),
},
}
suite.ctx = suite.ctx.WithConsensusParams(&cp)

// Mempool set up
suite.config = mempool.NewDefaultAuctionFactory(suite.encodingConfig.TxConfig.TxDecoder())
suite.mempool = mempool.NewAuctionMempool(suite.encodingConfig.TxConfig.TxDecoder(), suite.encodingConfig.TxConfig.TxEncoder(), 0, suite.config)
Expand Down Expand Up @@ -768,3 +777,156 @@ func (suite *ABCITestSuite) isTopBidValid() bool {
_, err := suite.anteHandler(suite.ctx, iterator.Tx(), true)
return err == nil
}

func (suite *ABCITestSuite) TestPrepareProposalLimits() {
suite.Run("rejects a single tx that is over the limit", func() {
suite.SetupTest() // reset
gasLimit := uint64(1000)
tx1, err := testutils.CreateTxWithLimit(suite.encodingConfig.TxConfig, suite.accounts[0], 0, 1000, testutils.CreateRandomMsgs(suite.accounts[0].Address, 1), gasLimit)
suite.Require().NoError(err)

mempool := mempool.NewAuctionMempool(suite.encodingConfig.TxConfig.TxDecoder(), suite.encodingConfig.TxConfig.TxEncoder(), 0, suite.config)
err = mempool.Insert(suite.ctx, tx1)
suite.Require().NoError(err)

suite.proposalHandler = abci.NewProposalHandler(
mempool,
log.NewTMLogger(os.Stdout),
suite.anteHandler,
suite.encodingConfig.TxConfig.TxEncoder(),
suite.encodingConfig.TxConfig.TxDecoder(),
)

// Update the consensus params to have a lower gas limit
cp := tmproto.ConsensusParams{
Block: &tmproto.BlockParams{
MaxGas: int64(1000) - 1,
},
}
ctx := suite.ctx.WithConsensusParams(&cp)

res := suite.proposalHandler.PrepareProposalHandler()(ctx, abcitypes.RequestPrepareProposal{
MaxTxBytes: 1000000000000000000,
})
suite.Require().Len(res.Txs, 0)
})

suite.Run("accepts a single tx that is at the limit", func() {
suite.SetupTest() // reset
gasLimit := uint64(1000)
tx1, err := testutils.CreateTxWithLimit(suite.encodingConfig.TxConfig, suite.accounts[0], 0, 1000, testutils.CreateRandomMsgs(suite.accounts[0].Address, 1), gasLimit)
suite.Require().NoError(err)
nivasan1 marked this conversation as resolved.
Show resolved Hide resolved

mempool := mempool.NewAuctionMempool(suite.encodingConfig.TxConfig.TxDecoder(), suite.encodingConfig.TxConfig.TxEncoder(), 0, suite.config)
err = mempool.Insert(suite.ctx, tx1)
suite.Require().NoError(err)

suite.proposalHandler = abci.NewProposalHandler(
mempool,
log.NewTMLogger(os.Stdout),
suite.anteHandler,
suite.encodingConfig.TxConfig.TxEncoder(),
suite.encodingConfig.TxConfig.TxDecoder(),
)

// Update the consensus params to have a lower gas limit
cp := tmproto.ConsensusParams{
Block: &tmproto.BlockParams{
MaxGas: int64(1000),
},
}
ctx := suite.ctx.WithConsensusParams(&cp)

req := abcitypes.RequestPrepareProposal{
MaxTxBytes: 1000000000000000000,
}
res := suite.proposalHandler.PrepareProposalHandler()(ctx, req)

suite.Require().Len(res.Txs, 1)

decodedTx, err := suite.encodingConfig.TxConfig.TxEncoder()(tx1)
suite.Require().NoError(err)
suite.Require().Equal(res.Txs[0], decodedTx)
})

suite.Run("rejects a bid that is over the limit", func() {
suite.SetupTest() // reset
gasLimit := uint64(1000)
tx1, err := testutils.CreateAuctionTxWithSignersAndLimit(
suite.encodingConfig.TxConfig,
suite.accounts[0],
sdk.NewCoin("foo", sdk.NewInt(1000000000000000000)),
0,
1000,
suite.accounts[0:10],
gasLimit,
)
suite.Require().NoError(err)

mempool := mempool.NewAuctionMempool(suite.encodingConfig.TxConfig.TxDecoder(), suite.encodingConfig.TxConfig.TxEncoder(), 0, suite.config)
err = mempool.Insert(suite.ctx, tx1)
suite.Require().NoError(err)

suite.proposalHandler = abci.NewProposalHandler(
mempool,
log.NewTMLogger(os.Stdout),
suite.anteHandler,
suite.encodingConfig.TxConfig.TxEncoder(),
suite.encodingConfig.TxConfig.TxDecoder(),
)

// Update the consensus params to have a lower gas limit
cp := tmproto.ConsensusParams{
Block: &tmproto.BlockParams{
MaxGas: int64(1000) - 1,
},
}
ctx := suite.ctx.WithConsensusParams(&cp)

res := suite.proposalHandler.PrepareProposalHandler()(ctx, abcitypes.RequestPrepareProposal{
MaxTxBytes: 1000000000000000000,
})
suite.Require().Len(res.Txs, 0)
})

suite.Run("rejects a tx that is too large but accepts a smaller one", func() {
suite.SetupTest() // reset
gasLimit := uint64(1000)
tx1, err := testutils.CreateTxWithLimit(suite.encodingConfig.TxConfig, suite.accounts[0], 1, 1000, testutils.CreateRandomMsgs(suite.accounts[0].Address, 1), gasLimit)
suite.Require().NoError(err)
nivasan1 marked this conversation as resolved.
Show resolved Hide resolved

tx2, err := testutils.CreateTxWithLimit(suite.encodingConfig.TxConfig, suite.accounts[0], 0, 1000, testutils.CreateRandomMsgs(suite.accounts[0].Address, 1), gasLimit-1)
suite.Require().NoError(err)

mempool := mempool.NewAuctionMempool(suite.encodingConfig.TxConfig.TxDecoder(), suite.encodingConfig.TxConfig.TxEncoder(), 0, suite.config)
err = mempool.Insert(suite.ctx, tx1)
suite.Require().NoError(err)
err = mempool.Insert(suite.ctx, tx2)
suite.Require().NoError(err)

suite.proposalHandler = abci.NewProposalHandler(
mempool,
log.NewTMLogger(os.Stdout),
suite.anteHandler,
suite.encodingConfig.TxConfig.TxEncoder(),
suite.encodingConfig.TxConfig.TxDecoder(),
)

// Update the consensus params to have a lower gas limit
cp := tmproto.ConsensusParams{
Block: &tmproto.BlockParams{
MaxGas: int64(1000) - 1,
},
}
ctx := suite.ctx.WithConsensusParams(&cp)

res := suite.proposalHandler.PrepareProposalHandler()(ctx, abcitypes.RequestPrepareProposal{
MaxTxBytes: 1000000000000000000,
})
suite.Require().Len(res.Txs, 1)

decodedTx, err := suite.encodingConfig.TxConfig.TxEncoder()(tx2)
suite.Require().NoError(err)
suite.Require().Equal(res.Txs[0], decodedTx)
})
}
Loading
Loading