feat(mempool)!: respect gas wanted returned by ante handler #25338
Conversation
* Problem: mempool don't respect gas wanted returned by ante handler Solution: - support custom gas wanted in mempool * Update CHANGELOG.md Signed-off-by: yihuang <huang@crypto.com> * cleanup * cleanup * fix priorityIndex * fix process proposal * fix lint * fix lint --------- Signed-off-by: yihuang <huang@crypto.com> Problem: mempool.GasTx interface is not reused (cosmos#536) * Problem: redundant mutex for InsertWithGasWanted cfg of PriorityNonceMempool remains unchanged once assigned, so no lock is required * make mocks * cleanup * keep order of check MaxTx
📝 WalkthroughWalkthroughPropagates gasWanted from ante handling through mempool and proposal flows. Updates ProposalTxVerifier and TxSelector signatures to carry gasWanted, refactors BaseApp’s ProcessProposalVerifyTx and CheckTx insertion, introduces a mempool.Tx wrapper with gasWanted, and updates mempool interfaces, iterators, and implementations (noop, priority_nonce, sender_nonce) along with tests. Adds CHANGELOG note. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Proposer
participant BaseApp
participant Mempool
participant TxSelector
participant ProposalTxVerifier as TxVerifier
participant Ante as AnteHandler
Proposer->>BaseApp: PrepareProposal
BaseApp->>Mempool: SelectBy(ctx, txs, callback(memTx))
activate Mempool
loop For each memTx
Mempool-->>BaseApp: memTx (Tx + GasWanted)
BaseApp->>TxVerifier: PrepareProposalVerifyTx(memTx.Tx)
TxVerifier-->>BaseApp: ok/error
alt ok
BaseApp->>TxSelector: SelectTxForProposal(..., memTx.Tx, txBz, memTx.GasWanted)
TxSelector-->>BaseApp: include?
alt include
BaseApp-->>Mempool: (continue)
else skip
end
else invalid
BaseApp-->>Mempool: (continue)
end
end
deactivate Mempool
BaseApp-->>Proposer: Proposal with selected txs
sequenceDiagram
autonumber
actor Validator
participant BaseApp
participant TxVerifier as ProposalTxVerifier
participant Ante as AnteHandler
Validator->>BaseApp: ProcessProposal(txs)
Note over BaseApp: maxBlockGas check
loop For each txBz
BaseApp->>TxVerifier: ProcessProposalVerifyTx(txBz)
activate TxVerifier
TxVerifier->>BaseApp: (sdk.Tx, gasWanted, err)
deactivate TxVerifier
alt err
BaseApp-->>Validator: REJECT
else ok
BaseApp->>BaseApp: totalTxGas += gasWanted (if maxBlockGas>0)
alt totalTxGas > maxBlockGas
BaseApp-->>Validator: REJECT
end
end
end
BaseApp-->>Validator: ACCEPT
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
types/mempool/priority_nonce.go (1)
182-193: Fix potential nil-deref in NextSenderTx.If the sender index exists but is empty, cursor will be nil and this panics.
func (mp *PriorityNonceMempool[C]) NextSenderTx(sender string) sdk.Tx { senderIndex, ok := mp.senderIndices[sender] if !ok { return nil } cursor := senderIndex.Front() - return cursor.Value.(Tx).Tx + if cursor == nil { + return nil + } + return cursor.Value.(Tx).Tx }types/mempool/sender_nonce.go (1)
106-117: Fix potential nil-deref in NextSenderTx.Empty per-sender list yields nil cursor; guard it.
func (snm *SenderNonceMempool) NextSenderTx(sender string) sdk.Tx { senderIndex, ok := snm.senders[sender] if !ok { return nil } cursor := senderIndex.Front() - return cursor.Value.(Tx).Tx + if cursor == nil { + return nil + } + return cursor.Value.(Tx).Tx }
🧹 Nitpick comments (11)
types/mempool/mempool_test.go (1)
142-145: Unwrap via helper to decouple tests from wrapper field layoutAccessing
.Txdirectly ties tests to the struct field name. Prefer a small helper to improve resilience if the wrapper evolves. Functionally identical.- txs = append(txs, iterator.Tx().Tx) + txs = append(txs, unwrapTx(iterator.Tx()))Add once in this test file:
// unwrapTx returns the underlying sdk.Tx from the mempool wrapper. func unwrapTx(t mempool.Tx) sdk.Tx { return t.Tx }types/mempool/sender_nonce_property_test.go (1)
95-103: Same unwrapping helper to avoid direct field access in testsMirror the helper approach to reduce coupling to the wrapper’s field name.
- tx := iterator.Tx() - txs = append(txs, tx.Tx.(testTx)) + tx := iterator.Tx() + txs = append(txs, unwrapTx(tx).(testTx))Add once in this test file if not shared:
func unwrapTx(t mempool.Tx) sdk.Tx { return t.Tx }types/mempool/noop.go (1)
19-24: No-op additions look good; add compile-time check for Mempool tooYou already assert ExtMempool conformance. Add Mempool as well to catch drift if interfaces change.
Add near the existing assertion:
var _ Mempool = (*NoOpMempool)(nil)types/mempool/mempool.go (3)
10-20: Add brief GoDoc to clarify wrapper semantics.Document that GasWanted is the ante-derived value (when available) or implementation-derived default.
Apply:
type Tx struct { - Tx sdk.Tx - GasWanted uint64 + // Tx is the original SDK transaction. + Tx sdk.Tx + // GasWanted is the gas value to be respected by selection logic (e.g., from ante). + // Implementations MAY derive this from tx if not provided via InsertWithGasWanted. + GasWanted uint64 } -func NewMempoolTx(tx sdk.Tx, gasWanted uint64) Tx { +// NewMempoolTx wraps an sdk.Tx with its gasWanted value for mempool iteration/selection. +func NewMempoolTx(tx sdk.Tx, gasWanted uint64) Tx {
22-24: Clarify how GasTx is used by Insert implementations.State that Insert may populate GasWanted by checking this interface when InsertWithGasWanted isn’t used.
Apply:
type GasTx interface { - GetGas() uint64 + // GetGas returns the gas wanted for the transaction. + GetGas() uint64 }
31-33: Document InsertWithGasWanted override behavior.Make it explicit that this bypasses interface-based derivation.
Apply:
- // Insert with a custom gas wanted value + // Insert with a custom gasWanted value. + // Implementations MUST use this gasWanted value as authoritative for the inserted tx. InsertWithGasWanted(context.Context, sdk.Tx, uint64) errortypes/mempool/priority_nonce.go (2)
213-214: Avoid duplicate lookups in senderIndex.Get(key).Cache the element to prevent two map/skiplist traversals and simplify formatting.
- if mp.cfg.TxReplacement != nil && !mp.cfg.TxReplacement(oldScore.priority, priority, senderIndex.Get(key).Value.(Tx).Tx, tx) { + oldEl := senderIndex.Get(key) + oldTx := oldEl.Value.(Tx).Tx + if mp.cfg.TxReplacement != nil && !mp.cfg.TxReplacement(oldScore.priority, priority, oldTx, tx) { return fmt.Errorf( "tx doesn't fit the replacement rule, oldPriority: %v, newPriority: %v, oldTx: %v, newTx: %v", oldScore.priority, priority, - senderIndex.Get(key).Value.(Tx).Tx, + oldTx, tx, ) }Also applies to: 251-259
283-290: Name consistency: use gasWanted, not gasLimit.This wrapper derives gas from GasTx. Rename local var for clarity and consistency with InsertWithGasWanted.
func (mp *PriorityNonceMempool[C]) Insert(ctx context.Context, tx sdk.Tx) error { - var gasLimit uint64 + var gasWanted uint64 if gasTx, ok := tx.(GasTx); ok { - gasLimit = gasTx.GetGas() + gasWanted = gasTx.GetGas() } - return mp.InsertWithGasWanted(ctx, tx, gasLimit) + return mp.InsertWithGasWanted(ctx, tx, gasWanted) }types/mempool/sender_nonce.go (2)
119-129: Good: gas-aware insertion path; minor naming nit.Functionality is correct; consider renaming gasLimit -> gasWanted for consistency with the rest of the API and PR intent.
-func (snm *SenderNonceMempool) InsertWithGasWanted(_ context.Context, tx sdk.Tx, gasLimit uint64) error { +func (snm *SenderNonceMempool) InsertWithGasWanted(_ context.Context, tx sdk.Tx, gasWanted uint64) error { @@ - memTx := NewMempoolTx(tx, gasLimit) + memTx := NewMempoolTx(tx, gasWanted) @@ - senderTxs.Set(nonce, memTx) + senderTxs.Set(nonce, memTx)Also applies to: 131-132, 154-155
162-169: Name consistency in wrapper Insert.Rename local variable to gasWanted for clarity.
func (snm *SenderNonceMempool) Insert(ctx context.Context, tx sdk.Tx) error { - var gasLimit uint64 + var gasWanted uint64 if gasTx, ok := tx.(GasTx); ok { - gasLimit = gasTx.GetGas() + gasWanted = gasTx.GetGas() } - return snm.InsertWithGasWanted(ctx, tx, gasLimit) + return snm.InsertWithGasWanted(ctx, tx, gasWanted) }baseapp/abci_utils.go (1)
506-507: Nit: parameter name.The parameter named memTx is actually an sdk.Tx; consider renaming to tx for clarity.
-func (ts *defaultTxSelector) SelectTxForProposal(_ context.Context, maxTxBytes, maxBlockGas uint64, memTx sdk.Tx, txBz []byte, gasWanted uint64) bool { +func (ts *defaultTxSelector) SelectTxForProposal(_ context.Context, maxTxBytes, maxBlockGas uint64, tx sdk.Tx, txBz []byte, gasWanted uint64) bool {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (11)
CHANGELOG.md(1 hunks)baseapp/abci_utils.go(7 hunks)baseapp/baseapp.go(2 hunks)baseapp/testutil/mock/mocks.go(2 hunks)types/mempool/mempool.go(4 hunks)types/mempool/mempool_test.go(1 hunks)types/mempool/noop.go(1 hunks)types/mempool/priority_nonce.go(7 hunks)types/mempool/priority_nonce_test.go(4 hunks)types/mempool/sender_nonce.go(5 hunks)types/mempool/sender_nonce_property_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: test-system
- GitHub Check: test-system-legacy
- GitHub Check: tests (01)
- GitHub Check: tests (03)
- GitHub Check: tests (00)
- GitHub Check: tests (02)
- GitHub Check: Check up to date mocks
- GitHub Check: test-e2e
- GitHub Check: test-sim-nondeterminism
- GitHub Check: test-integration
- GitHub Check: build (amd64)
- GitHub Check: build (arm64)
- GitHub Check: Analyze
- GitHub Check: golangci-lint
- GitHub Check: Gosec
- GitHub Check: dependency-review
- GitHub Check: Summary
🔇 Additional comments (21)
baseapp/baseapp.go (2)
875-880: LGTM: mempool insertion now carries gasWantedSwitching to InsertWithGasWanted on CheckTx correctly threads ante-derived gasWanted into the mempool.
1071-1082: LGTM — ProcessProposalVerifyTx now returns gasWanted; callers & mocks updatedrg output shows the interface, call site handling gasWanted, and mock implementation updated (no other references found).
types/mempool/priority_nonce_test.go (4)
391-391: Iterator now returns wrapper mempool.Tx — access pattern looks correct.Using iterator.Tx().Tx.(testTx) aligns with the new wrapper. No issues.
467-469: SelectBy callback updated to accept mempool.Tx — usage is correct.Unwrapping via memTx.Tx.(testTx) is consistent with the new API.
938-938: Tx replacement test assertion updated for wrapper — OK.Comparing against iter.Tx().Tx matches the new iterator contract.
973-973: Final iterator assertion updated for wrapper — OK.Accessing iter.Tx().Tx for equality is correct.
baseapp/testutil/mock/mocks.go (4)
138-145: Mock return signature extended to include gasWanted — implementation matches interface.Return indexes and types (types.Tx, uint64, error) are correct.
220-231: TxSelector mock updated to accept gasWanted — signature and recorder are consistent.Parameter threading (ctx, maxTxBytes, maxBlockGas, memTx, txBz, gasWanted) is correct.
148-151: No action required — callers already handle the extra return value.
Only invocation found: baseapp/abci_utils.go:417 —_, gasWanted, err := h.txVerifier.ProcessProposalVerifyTx(txBytes).
220-231: Confirm gomock expectations include gasWanted in SelectTxForProposalMocks were regenerated (baseapp/testutil/mock/mocks.go:219–231) and callers pass gasWanted (baseapp/abci_utils.go:284,345); no gomock EXPECT(...).SelectTxForProposal(...) usages were found — verify and update any tests/stubs to include the new gasWanted argument (or use gomock.Any()/matchers).
types/mempool/mempool.go (2)
53-54: ExtMempool SelectBy callback type updated to wrapper Tx — OK.Matches the new iterator return type; enables passing gasWanted to selectors.
75-86: SelectBy helper preserves fallback behavior — OK.Wrapper callback is propagated; fallback remains not thread-safe as documented.
types/mempool/priority_nonce.go (3)
195-205: Good: gas-aware insertion entry point.Renamed/added InsertWithGasWanted aligns with the new gasWanted flow.
355-357: Iterator now returns mempool.Tx.API shift looks consistent; downstream selection uses the wrapper.
390-398: SelectBy callback type change acknowledged.Signature updates to func(Tx) bool match the wrapper-based iteration.
types/mempool/sender_nonce.go (1)
211-219: Iterator and SelectBy now operate on mempool.Tx.Changes are coherent with the wrapper storage.
Also applies to: 304-306
baseapp/abci_utils.go (5)
279-285: No-op path: uses gas from tx when available.Good fallback for non-mempool selection; consistent with new TxSelector signature.
417-427: ProcessProposal gas accounting uses ante-returned gasWanted.This matches PR intent; guards maxBlockGas strictly.
205-207: Resolved — ProcessProposalVerifyTx now returns (sdk.Tx, uint64, error).
Search shows updated signatures/usages in baseapp/abci_utils.go, baseapp/baseapp.go and baseapp/testutil/mock/mocks.go; no old two-value implementations were found.
481-483: Breaking change: TxSelector.SelectTxForProposal signature now includes gasWanted — add migration noteCustom TxSelector implementations and tests must be updated to accept and handle the new gasWanted (uint64) parameter; add an UPGRADE/MIGRATION note calling out this breaking change.
Locations: baseapp/abci_utils.go — interface (lines 481–483) and default implementation (lines 506–527); baseapp/testutil/mock/mocks.go — mock (lines ~220–230).
299-346: PrepareProposal mempool path wired to memTx.GasWanted — CheckTx passes ante GasMeter limitConfirmed: baseapp.runTx sets gasWanted = ctx.GasMeter().Limit() after the AnteHandler and calls app.mempool.InsertWithGasWanted(ctx, tx, gasWanted) on CheckTx (baseapp/baseapp.go:858,876). PrepareProposal selection reads memTx.GasWanted (baseapp/abci_utils.go; mempool types). Ensure your AnteHandler sets the GasMeter (e.g., include SetUpContextDecorator or call ctx.WithGasMeter(NewGasMeter(tx.GetGas()))) so InsertWithGasWanted receives the intended non-zero GasWanted; mempool Insert wrappers also pass tx.GetGas when used directly (types/mempool/sender_nonce.go:168, priority_nonce.go:289).
| Tx() Tx | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Iterator.Tx now returns wrapper Tx — breaking change: verify all usages are updated.
Audit for code still asserting sdk.Tx directly from Iterator.Tx().
Run:
Expected: call sites use iter.Tx().Tx (or equivalent) after this change.
🏁 Script executed:
#!/bin/bash
# Find Iterator.Tx() usages and spot unwraps expecting sdk.Tx.
rg -nP --type=go -C3 '\.Tx\(\)\s*[\),]' # method call contexts
rg -nP --type=go -C3 '\.Tx\(\)\.' # chained member access implying wrapper usage
rg -nP --type=go -C3 '=\s*iter\.Tx\(\)' # direct assignment patternsLength of output: 3411
Iterator.Tx now returns wrapper Tx — update all call sites that still treat it as sdk.Tx
Call sites still passing/assigning iter.Tx() must unwrap (.Tx) or accept the wrapper:
- types/mempool/mempool.go:83 — for iter != nil && callback(iter.Tx()) {
- types/mempool/sender_nonce.go:216 — for iter != nil && callback(iter.Tx()) {
- types/mempool/priority_nonce.go:395 — for iter != nil && callback(iter.Tx()) {
Tests already access the inner Tx in several places (e.g., types/mempool/mempool_test.go:142; types/mempool/priority_nonce_test.go:391, 938, 973) — ensure expectations/ assertions match the wrapper change.
🤖 Prompt for AI Agents
In types/mempool/mempool.go around lines 65-66 the interface method Tx() now
returns the wrapper type Tx instead of sdk.Tx; update all call sites that still
treat iter.Tx() as sdk.Tx to either unwrap the inner sdk.Tx via iter.Tx.Tx or
change the consumer to accept the wrapper type. Specifically, modify the three
call sites (types/mempool/mempool.go:83, types/mempool/sender_nonce.go:216,
types/mempool/priority_nonce.go:395) to call callback(iter.Tx().Tx) if callback
expects sdk.Tx, or change the callback signature to accept the wrapper; also
update tests that inspect the inner transaction (e.g.,
types/mempool/mempool_test.go:142; types/mempool/priority_nonce_test.go:391,
938, 973) to access the inner sdk.Tx through the wrapper before making
assertions.
|
@mmsqe looks like there are some CI issues |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Seems got github rate limited. |
|
@mmsqe we should note that this is an API breaking change - is there any way to make this non-api breaking? |
seems inevitable, since we can't add property in sdk.Tx which is an interface, we need to save |
|
i will have a look here next week |
technicallyty
changed the title
baseapp/abci_utils.go
Outdated
| ProposalTxVerifier interface { | ||
| PrepareProposalVerifyTx(tx sdk.Tx) ([]byte, error) | ||
| ProcessProposalVerifyTx(txBz []byte) (sdk.Tx, error) | ||
| ProcessProposalVerifyTx(txBz []byte) (sdk.Tx, uint64, error) |
There was a problem hiding this comment.
uint64 is a bit ambiguous, maybe a named return or comment to help the developer understand the variable
baseapp/baseapp.go
Outdated
| // returned if the transaction cannot be decoded. <Tx, nil> will be returned if | ||
| // the transaction is valid, otherwise <Tx, err> will be returned. | ||
| func (app *BaseApp) ProcessProposalVerifyTx(txBz []byte) (sdk.Tx, error) { | ||
| func (app *BaseApp) ProcessProposalVerifyTx(txBz []byte) (sdk.Tx, uint64, error) { |
There was a problem hiding this comment.
same here and comments need to updated above too
types/mempool/mempool.go
Outdated
| // Insert with a custom gas wanted value | ||
| InsertWithGasWanted(context.Context, sdk.Tx, uint64) error | ||
|
|
There was a problem hiding this comment.
can you explain why this extra method is needed? i think the reason is here:
Line 876 in 4cd29a5
There was a problem hiding this comment.
yes, we need to fill mempoolTx with gasWanted from gas meter when runTx
There was a problem hiding this comment.
what is the difference between the tx.Gas() vs the gasWanted from the meter here?
There was a problem hiding this comment.
what do you think of removing this method and instead adding an argument to Insert such as:
type InsertOption struct {
GasWanted uint64
}
type Mempool interface {
// ....
Insert(context.Context, sdk.Tx, InsertOption) error
}this way, if more is needed in the future, it will be nonbreaking.
There was a problem hiding this comment.
There are cases that tx.Gas() is different from gasWanted returned from context GasMeter() and we need to override with it. I think we keep Insert mainly for the tests.
types/mempool/mempool.go
Outdated
| type GasTx interface { | ||
| GetGas() uint64 | ||
| } |
There was a problem hiding this comment.
maybe this should be defined with the other tx interfaces: types/tx_msg.go?
There was a problem hiding this comment.
but this interface is mainly used in mempool
There was a problem hiding this comment.
i think even if it is only used here, i would prefer to house all Tx interfaces in one place
upstream the fix for gas wanted
Summary by CodeRabbit