Conversation
| // TODO: do we even do recheck anymore? | ||
| if request.Type == abci.CheckTxType_Recheck { | ||
| return &abci.ResponseCheckTx{Code: abci.CodeTypeOK}, nil | ||
| } |
There was a problem hiding this comment.
we can expect this to never be called with this mempool, yes? cc @mattac21
technicallyty
requested review from
Eric-Warehime,
aljo242,
mattac21 and
vladjdk
|
@greptile whats the scoop man |
|
@greptile |
|
@greptile hello |
Greptile SummaryThis PR adapts the EVM mempool's
Issues found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant C as CometBFT
participant H as CheckTxHandler
participant Q as InsertQueue (async worker)
participant EVM as LegacyPool (EVM)
participant COS as RecheckMempool (Cosmos)
C->>H: RequestCheckTx (New)
H->>H: Decode tx bytes
alt Decode error
H-->>C: ResponseCheckTx (error code)
else Decoded OK
H->>Q: insert(ctx, tx)
note over Q: EVM tx → evmQueue.Push()<br/>Cosmos tx → cosmosQueue.Push()
Q-->>H: errC (channel)
H->>H: block on <-errC (no timeout)
Q->>EVM: txPool.Add() [EVM path]
Q->>COS: insertCosmosTx() [Cosmos path]
alt Insert error
Q->>H: errC ← error
H-->>C: ResponseCheckTx (error code)
else Insert OK
Q->>H: errC ← nil
H-->>C: ResponseCheckTx (OK, GasWanted=0)
end
end
C->>H: RequestCheckTx (Recheck)
H-->>C: ResponseCheckTx (OK, no-op)
Last reviewed commit: 4e043cf |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Data: result.Data, | ||
| Events: types.MarkEventsToIndex(result.Events, nil), | ||
| }, nil | ||
| return &abci.ResponseCheckTx{Code: abci.CodeTypeOK}, nil |
There was a problem hiding this comment.
GasWanted / GasUsed always return 0 — breaks CometBFT mempool gas accounting
The previous handler forwarded the gas values computed by runTx:
return &abci.ResponseCheckTx{
GasWanted: int64(gInfo.GasWanted),
GasUsed: int64(gInfo.GasUsed),
...
}The new handler always returns GasWanted: 0 and GasUsed: 0. CometBFT uses GasWanted from ResponseCheckTx to enforce the block-level mempool gas cap (max_gas in config). When every transaction reports wanting 0 gas, CometBFT's gas cap is never triggered, which effectively disables that safeguard for the entire mempool.
This is safe only if the mempool always operates in exclusive mode (CometBFT's own clist-mempool is disabled). If it is possible for the app to run with OperateExclusively = false, clients and validators will see incorrect gas estimates from CheckTx and the CometBFT-side gas cap will not function.
At a minimum, the function should document that GasWanted is intentionally omitted and under what assumptions this is valid. If the gas values are still meaningful, consider computing them from the decoded tx's declared gas limit before the async insert:
// For EVM transactions the gas limit is embedded in the transaction itself.
// Return it so that CometBFT's mempool gas cap still fires correctly.
return &abci.ResponseCheckTx{Code: abci.CodeTypeOK, GasWanted: int64(gasLimit)}, nilThere was a problem hiding this comment.
@greptile gas values aren't populated in regular checktx anyways. this is not a regression
There was a problem hiding this comment.
Looking at the diff, the old code explicitly called runTx and used the returned gInfo.GasWanted and gInfo.GasUsed values in the response:
gInfo, result, anteEvents, err := runTx(request.Tx, nil)
// ...
return &abci.ResponseCheckTx{
GasWanted: int64(gInfo.GasWanted),
GasUsed: int64(gInfo.GasUsed),
...
}So the old handler was populating gas values from runTx. Are you saying that in practice runTx was returning 0 for those fields, or that there's a different reason they weren't meaningful?
If the gas values were genuinely unused or always zero before, then you're right this isn't a regression. But if runTx was returning valid gas values, then this change does remove that information from CheckTx responses.
There was a problem hiding this comment.
is this true that the values were always zero? I thought you do get gas wanted and used values back from checktx?
There was a problem hiding this comment.
No, that's not typically true in standard Cosmos SDK implementations. Looking at the old code:
gInfo, result, anteEvents, err := runTx(request.Tx, nil)
// ...
return &abci.ResponseCheckTx{
GasWanted: int64(gInfo.GasWanted),
GasUsed: int64(gInfo.GasUsed),
...
}In a typical Cosmos SDK app during CheckTx:
GasWantedis set to the transaction's declared gas limitGasUsedis the actual gas consumed by the ante handlers (signature verification, fee deduction, etc.)
These values are normally non-zero and are used by:
- CometBFT's mempool gas cap (
max_gasin config) - Clients to get accurate gas estimates from CheckTx queries
- Fee estimation and transaction simulation
However, there could be something specific to your EVM implementation where:
- The ante handlers don't consume gas the same way
- Gas tracking is handled differently for EVM transactions
- The values genuinely weren't being set correctly before
If runTx was genuinely returning zero values in practice, that would indicate a pre-existing issue with your gas accounting. But based on standard Cosmos SDK behavior, the old code should have been returning valid gas values.
There was a problem hiding this comment.
@greptile why would those values be populated if checkTx is just a mempool admission api and not a tx execution check?
There was a problem hiding this comment.
In standard Cosmos SDK, CheckTx does execute the ante handlers (signature verification, fee deduction, sequence checks, etc.), which:
- Consume gas - AnteHandlers use gas for crypto operations, state reads, etc. That's the
GasUsedvalue - Validate the transaction - They check if the tx is properly formatted, has sufficient fees, valid signatures, etc.
- Return the declared gas limit - That's
GasWanted, read from the transaction itself
So CheckTx is more than just "should we admit this to the mempool?" - it's "can this transaction pass ante handlers?" The gas values from that execution are used for:
- CometBFT's mempool gas cap - prevents the mempool from accepting too many high-gas transactions
- Client gas estimation - wallets query CheckTx to estimate gas before broadcasting
- Fee market calculations - some chains use CheckTx gas usage for dynamic fee calculations
In your new implementation:
You're bypassing runTx entirely and just routing to the async insert worker. If the insert worker doesn't run ante handlers synchronously, then you're right - there's no gas consumed to report.
However, GasWanted (the declared gas limit) is still available from the decoded transaction and is still meaningful for CometBFT's mempool gas cap. You could extract it from the decoded tx before the async insert if you want to preserve that safeguard.
There was a problem hiding this comment.
@greptile cometbft reduces the fields in the response anyway: https://github.com/cometbft/cometbft/blob/1bb8b386fc366bc4655dede0535e16d1ad669c7d/rpc/core/mempool.go#L58-L64. gas has never been returned here
There was a problem hiding this comment.
You're right, my apologies. I can see that:
OperateExclusively = trueis hardcoded in your codebase (evmd/mempool.go:19)- You're using
SetInsertTxHandlerandSetReapTxsHandler, which completely bypasses CometBFT's clist-mempool - The RPC endpoint does strip gas fields before returning to clients
When operating exclusively, CometBFT's internal mempool is disabled and never sees or uses the GasWanted values from ResponseCheckTx. All gas accounting is handled by your app-side ExperimentalEVMMempool.
So you're correct - this change doesn't break anything that was previously working. The gas values were never meaningfully used in CheckTx for your setup.
mattac21
left a comment
mattac21
left a comment
There was a problem hiding this comment.
could you also rebase this off of main? the tests are pretty broken on feat/krakatoa but if you go to main you should be able to get all of the system unit and integration tests passing
mempool/check_tx.go
Outdated
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), timeout) | ||
| defer cancel() | ||
| errC, err := mempool.insert(ctx, tx) |
There was a problem hiding this comment.
the mempool.Insert function already waits on the errC or ctx to be done like you are doing here. could we use that instead of using the private mempool.insert?
| Data: result.Data, | ||
| Events: types.MarkEventsToIndex(result.Events, nil), | ||
| }, nil | ||
| return &abci.ResponseCheckTx{Code: abci.CodeTypeOK}, nil |
There was a problem hiding this comment.
is this true that the values were always zero? I thought you do get gas wanted and used values back from checktx?
| index map[sdk.Tx]int | ||
| keys map[string]int |
There was a problem hiding this comment.
keys is now serving the same purpose as index right? we can remove index now I think
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
|
|
||
| if key, ok := cosmosTxKey(tx); ok { |
There was a problem hiding this comment.
hm thinking through this, im not actually sure this replacement is safe to do without rechecking all txs for this account, when we replace a tx we may want to just remove all txs > replaced nonce from the tx store for this account and then let them just be included in the next block.
I think this isn't safe because we would need to recheck all txs after the replaced one onto of the state of this new tx, which we are not doing. for example if we recheck txs 4 5 and 6 of an an account and include them in the tx store, then someone can replace tx 4 with a completely different tx that may have invalidated 5 and 6, but we are not rechecking those against the new tx 4's context, which may then cause the proposal to be invalid.
I think this is an issue for evm txs in the legacypool too that we need to address.
There was a problem hiding this comment.
working on a separate PR to address this issue in both evm and cosmos tx stores
| select { | ||
| case err := <-errC: | ||
| if err != nil { | ||
| return sdkerrors.ResponseCheckTxWithEvents(err, gInfo.GasWanted, gInfo.GasUsed, anteEvents, false), nil |
There was a problem hiding this comment.
were the anteEvents also always nil here? we are missing out on events now as well with this? should we modify the response of mempool.Insert to return some of this info?
There was a problem hiding this comment.
i checked on v0.53.x and main, calling broadcast tx sync never returned anything other than code, tx hash, and a log if it failed.
you can see comet stripping down the response here: https://github.com/cometbft/cometbft/blob/1bb8b386fc366bc4655dede0535e16d1ad669c7d/rpc/core/mempool.go#L58-L64
There was a problem hiding this comment.
got it, tested on gaia as well earlier and this was the case too
technicallyty
force-pushed
the
technicallyty/STACK-2402-krakatoa-recheck
branch
from
f7e5c47 to
c54ef7b
Compare
4 participants
Description
we recently updated comet to no longer lock on recheck, pushing concurrency responsibility to the application.
changes:
Closes: STACK-2455
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
mainbranch