Skip to content

fix: harden mempool proposal gas accounting #3892

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

Closed
codchen wants to merge 1 commit into from
Closed

fix: harden mempool proposal gas accounting #3892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
59 changes: 38 additions & 21 deletions sei-tendermint/internal/mempool/mempool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,26 @@ const (
MinGasEVMTx = 21000
)

type checkedTxMetadata struct {
priority int64
gasWanted int64
estimatedGas int64
}

// metadataFromCheckTx normalizes application metadata identically for initial checks and
// rechecks. Reusing this step prevents proposal selection from retaining stale gas values.
func metadataFromCheckTx(res *abci.ResponseCheckTxV2) checkedTxMetadata {
estimatedGas := res.GasEstimated
if estimatedGas < MinGasEVMTx || estimatedGas > res.GasWanted {
estimatedGas = res.GasWanted
}
return checkedTxMetadata{
priority: res.Priority,
gasWanted: res.GasWanted,
estimatedGas: estimatedGas,
}
}

type Config struct {
// Maximum number of transactions in the mempool
Size int
Expand Down Expand Up @@ -353,18 +373,14 @@ func (txmp *TxMempool) CheckTx(ctx context.Context, tx types.Tx) (*abci.Response
Global.NumberOfSuccessfulCheckTxsAt().Add(1)
Global.observeCheckTxPriorityDistribution(res.Priority, false, "", false)

// Normalize the estimate.
estimatedGas := res.GasEstimated
if estimatedGas < MinGasEVMTx || estimatedGas > res.GasWanted {
estimatedGas = res.GasWanted
}
metadata := metadataFromCheckTx(res)
wtx := &WrappedTx{
hashedTx: hTx,
timestamp: time.Now().UTC(),
height: txmp.height,
priority: res.Priority,
estimatedGas: estimatedGas,
gasWanted: res.GasWanted,
priority: metadata.priority,
estimatedGas: metadata.estimatedGas,
gasWanted: metadata.gasWanted,
}
if res.IsEVM {
wtx.evm = utils.Some(evmTx{
Expand Down Expand Up @@ -415,11 +431,13 @@ func (txmp *TxMempool) Flush() {
// The returned list starts with EVM transactions (in priority order),
// followed by non-EVM transactions (in priority order).
// There are 4 types of constraints.
// 1. maxBytes - stops pulling txs from mempool once maxBytes is hit.
// 2. maxGasWanted - stops pulling txs from mempool once total gas wanted exceeds maxGasWanted.
// 1. maxTxs - limits the number of transactions returned from the mempool.
// 2. maxBytes - limits the total transaction bytes returned from the mempool.
// 3. maxGasWanted - limits total gas wanted; candidates that do not fit are skipped.
// Can be set to -1 to be ignored.
// 3. maxGasEstimated - similar to maxGasWanted but will use the estimated gas used for EVM txs
// while still using gas wanted for cosmos txs. Can be set to -1 to be ignored.
// 4. maxGasEstimated - similar to maxGasWanted but will use the estimated gas used for EVM txs
// while still using gas wanted for cosmos txs. Candidates that do not fit are skipped. Can be
// set to -1 to be ignored.
//
// NOTE: Transactions are removed from the mempool iff remove == true.
// Either way, the transactions stay in the LRU cache.
Expand Down Expand Up @@ -461,7 +479,7 @@ func (txmp *TxMempool) Update(
for i, tx := range blockTxs {
txResults[tx.Hash()] = execTxResult[i].Code == abci.CodeTypeOK
}
newPriorities := map[types.TxHash]int64{}
recheckedTxs := map[types.TxHash]checkedTxMetadata{}
invalidTxs := map[types.TxHash]bool{}
if recheck {
for _, wtx := range txmp.txStore.ReadyTxs() {
Expand All @@ -476,18 +494,17 @@ func (txmp *TxMempool) Update(
if err != nil || !res.IsOK() {
invalidTxs[wtx.Hash()] = true
} else {
// If succeeds, we just care about the new priority.
newPriorities[wtx.Hash()] = res.Priority
recheckedTxs[wtx.Hash()] = metadataFromCheckTx(res)
}
}
}
txmp.txStore.Update(updateSpec{
Now: time.Now(),
Height: blockHeight,
TxResults: txResults,
NewPriorities: newPriorities,
InvalidTxs: invalidTxs,
Constraints: txConstraints,
Now: time.Now(),
Height: blockHeight,
TxResults: txResults,
RecheckedTxs: recheckedTxs,
InvalidTxs: invalidTxs,
Constraints: txConstraints,
})
txmp.notifyTxsAvailable()
Global.SizeAt().Set(int64(txmp.NumTxsNotPending()))
Expand Down
58 changes: 58 additions & 0 deletions sei-tendermint/internal/mempool/mempool_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,64 @@ func TestTxMempool_ReapMaxBytesMaxGas_FallbackToGasWanted(t *testing.T) {
wg.Wait()
}

func TestTxMempool_RejectsGasWantedAboveProposalLimit(t *testing.T) {
gasWanted := int64(11)
client := &application{Application: kvstore.NewApplication(), gasWanted: &gasWanted}
constraints := NopTxConstraints()
constraints.MaxGasWanted = 10
txmp := setup(TestConfig(), proxy.New(client), func() (TxConstraints, error) {
return constraints, nil
})

_, err := txmp.CheckTx(t.Context(), types.Tx("sender=key=1"))
require.Error(t, err)
require.Zero(t, txmp.Size())
}

func TestTxMempool_ZeroProposalGasLimitDoesNotRejectAtAdmission(t *testing.T) {
gasWanted := int64(1)
client := &application{Application: kvstore.NewApplication(), gasWanted: &gasWanted}
constraints := NopTxConstraints()
constraints.MaxGasWanted = 0
txmp := setup(TestConfig(), proxy.New(client), func() (TxConstraints, error) {
return constraints, nil
})

_, err := txmp.CheckTx(t.Context(), types.Tx("sender=key=1"))
require.NoError(t, err)
require.Equal(t, 1, txmp.Size())
reaped, _ := txmp.ReapTxs(ReapLimits{MaxGasWanted: utils.Some(int64(0))}, false)
require.Empty(t, reaped)
}

func TestTxMempool_RecheckRefreshesGasMetadata(t *testing.T) {
gasWanted := int64(1)
gasEstimated := int64(1)
client := &application{
Application: kvstore.NewApplication(),
gasWanted: &gasWanted,
gasEstimated: &gasEstimated,
}
txmp := setup(TestConfig(), proxy.New(client), NopTxConstraintsFetcher)
tx := types.Tx("sender=key=1")

_, err := txmp.CheckTx(t.Context(), tx)
require.NoError(t, err)
reaped, _ := txmp.ReapTxs(ReapLimits{MaxGasWanted: utils.Some(int64(1))}, false)
require.Equal(t, types.Txs{tx}, reaped)

gasWanted = 9
gasEstimated = 9
txmp.Lock()
require.NoError(t, txmp.Update(t.Context(), 1, nil, nil, NopTxConstraints(), true))
txmp.Unlock()

reaped, _ = txmp.ReapTxs(ReapLimits{MaxGasWanted: utils.Some(int64(8))}, false)
require.Empty(t, reaped)
reaped, _ = txmp.ReapTxs(ReapLimits{MaxGasWanted: utils.Some(int64(9))}, false)
require.Equal(t, types.Txs{tx}, reaped)
}

func TestTxMempool_ReapMaxTxs(t *testing.T) {
ctx := t.Context()

Expand Down
3 changes: 3 additions & 0 deletions sei-tendermint/internal/mempool/reactor/reactor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ func TestReactorFailedCheckTxCountEvictsPeer(t *testing.T) {
return mempool.TxConstraints{
MaxDataBytes: 10,
MaxGas: -1,
MaxGasWanted: -1,
}, nil
})
t.Cleanup(leaktest.Check(t))
Expand Down Expand Up @@ -275,6 +276,7 @@ func TestReactorPeerDownClearsFailedCheckTxCount(t *testing.T) {
return mempool.TxConstraints{
MaxDataBytes: 10,
MaxGas: -1,
MaxGasWanted: -1,
}, nil
},
)
Expand Down Expand Up @@ -314,6 +316,7 @@ func TestReactorMissingFailedCheckTxCountIsNotRecreated(t *testing.T) {
return mempool.TxConstraints{
MaxDataBytes: 10,
MaxGas: -1,
MaxGasWanted: -1,
}, nil
},
)
Expand Down
56 changes: 42 additions & 14 deletions sei-tendermint/internal/mempool/tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ func (wtx *WrappedTx) check(c TxConstraints) error {
if c.MaxGas >= 0 && wtx.gasWanted > c.MaxGas {
return fmt.Errorf("gas wanted exceeds max gas: gas wanted %d is greater than max gas %d", wtx.gasWanted, c.MaxGas)
}
if c.MaxGasWanted > 0 && wtx.gasWanted > c.MaxGasWanted {

@seidroid seidroid Bot Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[nit] The > 0 guard makes MaxGasWanted == 0 mean "unchecked", while the MaxGas check directly above uses >= 0 so MaxGas == 0 rejects everything. The asymmetry is intentional and documented in types.go, but the resulting state is a trap: at zero, admission accepts every transaction while Reap produces only empty proposals (exactly what TestTxMempool_ZeroProposalGasLimitDoesNotRejectAtAdmission pins), so the mempool fills with transactions that can never be proposed. A one-time warning log or a metric when a node observes MaxGasWanted == 0 would make that misconfiguration diagnosable instead of presenting as unexplained empty blocks.

Separately: rejection here calls txmp.txStore.MarkInvalid, so a transaction rejected under the current limit stays cache-blacklisted even if MaxGasWanted is later raised via consensus params, until LRU eviction. Minor, and consistent with how MaxGas already behaves.

Comment thread
seidroid[bot] marked this conversation as resolved.
return fmt.Errorf("gas wanted exceeds max gas wanted: gas wanted %d is greater than max gas wanted %d", wtx.gasWanted, c.MaxGasWanted)
}
return nil
}

Expand Down Expand Up @@ -550,12 +553,12 @@ func (s *txStore) compact(inner *txStoreInner, clearAccounts bool) {
}

type updateSpec struct {
Now time.Time
Height int64
TxResults map[types.TxHash]bool // true - success, false - failed, missing - not executed
Constraints TxConstraints
NewPriorities map[types.TxHash]int64
InvalidTxs map[types.TxHash]bool
Now time.Time
Height int64
TxResults map[types.TxHash]bool // true - success, false - failed, missing - not executed
Constraints TxConstraints
RecheckedTxs map[types.TxHash]checkedTxMetadata
InvalidTxs map[types.TxHash]bool
}

func (s *txStore) Update(spec updateSpec) {
Expand Down Expand Up @@ -592,6 +595,11 @@ func (s *txStore) Update(spec updateSpec) {
if expired {
Global.ExpiredTxsAt().Add(1)
}
if metadata, ok := spec.RecheckedTxs[wtx.Hash()]; ok {
wtx.priority = metadata.priority
wtx.gasWanted = metadata.gasWanted
wtx.estimatedGas = metadata.estimatedGas
}
invalid := spec.InvalidTxs[wtx.Hash()] || wtx.check(spec.Constraints) != nil
_, executed := spec.TxResults[wtx.Hash()]
remove := invalid || executed || (expired && (s.config.RemoveExpiredTxsFromQueue || !inner.isReady(wtx)))
Expand All @@ -606,8 +614,6 @@ func (s *txStore) Update(spec updateSpec) {
if el, ok := wtx.readyEl.Get(); ok {
s.readyTxs.Remove(el)
}
} else if newPriority, ok := spec.NewPriorities[wtx.Hash()]; ok {
wtx.priority = newPriority
}
}
start := time.Now()
Expand All @@ -624,6 +630,14 @@ type ReapLimits struct {
MaxGasEstimated utils.Option[int64]
}

// fitsReapCapacity reports whether a transaction fits every remaining proposal budget.
// The budgets are independent, so a candidate must fit all three to be selectable.
func (wtx *WrappedTx) fitsReapCapacity(remainingBytes, remainingGasWanted, remainingGasEstimated int64) bool {
return wtx.protoSize <= remainingBytes &&
wtx.gasWanted <= remainingGasWanted &&
wtx.estimatedGas <= remainingGasEstimated
}

// Reap returns a list of transactions within the provided tx,
// byte, and gas constraints together with the total estimated gas for the
// returned transactions. Reaped txs are removed iff remove == true.
Expand All @@ -649,18 +663,32 @@ func (s *txStore) Reap(l ReapLimits, remove bool) (types.Txs, int64) {
var wtxs []*WrappedTx
for inner := range s.inner.Lock() {
if uint64(inner.state.Load().ready.count) >= s.config.TxNotifyThreshold { //nolint:gosec // count is non-negative
blockedEVMAccounts := map[common.Address]struct{}{}
for _, wtx := range inner.inInclusionOrder() {
if uint64(len(wtxs)) >= maxTxs || !inner.isReady(wtx) {
if uint64(len(wtxs)) >= maxTxs {
break
}
if maxBytes-totalSize < wtx.protoSize {
if !inner.isReady(wtx) {
break
}
if maxGasWanted-totalGasWanted < wtx.gasWanted {
break
evm, isEVM := wtx.evm.Get()
// Non-EVM ordering assumes transactions come from different accounts. If that
Comment thread
seidroid[bot] marked this conversation as resolved.
Comment thread
seidroid[bot] marked this conversation as resolved.
Comment thread
seidroid[bot] marked this conversation as resolved.

@seidroid seidroid Bot Aug 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[suggestion] Consolidating the same point raised across several earlier rounds, since it survives the rebase (Codex flags it too).

The EVM half of this is correct — I checked that inInclusionOrder assigns each EVM tx txPrio = min(priority over nonces <= n), which is non-increasing in nonce, and the stable sort therefore preserves per-account nonce order. So blocking on the first non-fitting tx of an account does suppress every later nonce, and TestTxStore_ReapDoesNotSkipEVMNonceDependency pins it.

The Cosmos half has no equivalent guard, and this comment documents the hazard instead of closing it: a signer with sequences N and N+1 in the mempool can now have N skipped for capacity and N+1 selected, which fails CheckSignatures with a sequence mismatch at delivery and wastes the block space it was selected for. Under break that specific interleaving was unreachable. Non-EVM ordering is by priority alone (txPrio[tx] = tx.priority, seeded from a range inner.byHash map iteration), so same-account Cosmos txs could already come out misordered — this widens an existing hole rather than opening one, which is why it is a suggestion and not a blocker.

The blocker to fixing it properly is that WrappedTx carries signer identity only inside wtx.evm, so there is no key to block on for Cosmos txs. Three options, in increasing cost: keep break when !isEVM so only EVM candidates are skipped; add a counter on skipped-for-capacity non-EVM candidates so the frequency is observable in production; or plumb the fee payer through ResponseCheckTxV2 so blockedEVMAccounts can key on it for Cosmos txs too. Any of these puts the invariant at the choke point instead of leaving it as a comment the next reader has to trust.

// assumption is violated, skipping sequence N can select N+1 and fail delivery.
if isEVM {
if _, blocked := blockedEVMAccounts[evm.address]; blocked {
continue
}
}
if maxGasEstimated-totalGasEstimated < wtx.estimatedGas {
break
if !wtx.fitsReapCapacity(
maxBytes-totalSize,
maxGasWanted-totalGasWanted,
maxGasEstimated-totalGasEstimated,
) {
if isEVM {
// A later nonce cannot be selected when this account's current nonce is skipped.
blockedEVMAccounts[evm.address] = struct{}{}
Comment thread
seidroid[bot] marked this conversation as resolved.
}
continue
Comment thread
seidroid[bot] marked this conversation as resolved.
}
// include tx and update totals
totalSize += wtx.protoSize
Expand Down
Loading
Loading