fix: make proposal gas accounting state-independent - #3892
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3892 +/- ##
==========================================
- Coverage 59.45% 58.51% -0.94%
==========================================
Files 2319 2227 -92
Lines 198379 187944 -10435
==========================================
- Hits 117946 109976 -7970
+ Misses 69235 67589 -1646
+ Partials 11198 10379 -819
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryHigh Risk Overview The app adds Gasless policy narrows to unassociated native Tendermint mempool enforces Reviewed by Cursor Bugbot for commit 28caad0. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The core fix is sound — decoupling execution gas from reported GasWanted via a wrapping no-consumption meter is the right shape, and the CheckTx/DeliverTx/recheck/proposal paths are now consistent. However, counting fee-exempt transactions' self-declared gas toward the block budget without any cap creates a free block-gas reservation vector that can crowd out oracle votes, and the Reap break→continue change lacks a same-signer guard for non-EVM transactions.
Findings: 1 blocking | 9 non-blocking | 4 posted inline
Blockers
- Fee-exempt transactions can now reserve block gas for free.
checkTotalBlockGasno longer excludes gasless txs (app/app.go, removed block), andReap/wtx.checknow honor their declaredgasWanted. A gaslessMsgAssociatepays nothing:CheckAndChargeFeesreturns early onisGasless(no min-gas-price check), andChargeFeesinCosmosDeliverTxAntededucts only the fee the sender declared (0). So--gas 50000000 --fees 0from any fresh unassociated address now reserves the entireMaxGasWanted(50M) budget at zero cost. Amplification is large: one paid multi-send can bootstrap many accounts, each yielding one free 50M-gas reservation. Mitigating factors are real but partial — gasless txs getpriority = 0so they sort last ininInclusionOrder— but that is exactly the same tier as validator oracle votes, so this spam competes directly with (and under the new skip-don't-breakReapcan crowd out) oracle votes, risking missed-vote jailing. Suggest clamping the reported limit for fee-exempt txs to a fixed constant (still fully state-independent, so the original bug stays fixed) rather than honoring an attacker-chosen--gas.
Non-blocking
- Cursor's second-opinion file (
cursor-review.md) is empty — that pass produced no output. Codex reported no material issues. - Oracle-vote liveness: votes now consume the block
gasWantedbudget at priority 0 and, withcontinueinstead ofbreak, can be individually skipped when the budget is tight. Worth confirming the price feeder declares--gas 0(or adding a reserved carve-out) before rollout, since systematically skipped votes lead to miss-count penalties. checkTotalBlockGasruns inProcessProposalHandler(app/app.go:1293) and decides proposal accept/reject, so this change alters consensus-visible voting behavior with no version gate in the diff. Per REVIEW_GUIDELINES §2 this is normally handled by Sei's coordinated version-by-version rollout and theapp-hash-breakinglabel suggests that is intended — just flagging so the release cut is explicit about it.- Godoc style vs
AGENTS.md("Explain WHAT, not WHY or HOW", "Document the subject, not the system"): several new doc comments carry rationale that belongs inline or nowhere —checkGaslessAndSetGasMeter("...so CheckTx, recheck, and proposal processing agree..."),NewNoConsumptionGasMeter(2nd sentence),metadataFromCheckTx("Reusing this step prevents..."),fitsReapCapacity("The budgets are independent, so..."), andcheckGaslessWithoutConsumingGas("The caller separately preserves..."). - Test coverage gap: no test exercises the exact regression described in the PR body end-to-end — a
MsgAssociatethat is gasless atCheckTxbut associated byProcessProposaltime, asserting that mempoolgasWantedandcheckTotalBlockGasstill agree.TestCheckTxGaslessAssociatePreservesDeclaredGasAccountingonly covers the CheckTx half, and the twocheck_total_block_gas_test.gotests only flip an assertion. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| // A later nonce cannot be selected when this account's current nonce is skipped. | ||
| blockedEVMAccounts[evm.address] = struct{}{} | ||
| } | ||
| continue |
There was a problem hiding this comment.
[suggestion] The break→continue change adds blockedEVMAccounts to preserve EVM nonce dependencies, but there is no equivalent guard for non-EVM txs. Cosmos txs carry account sequence numbers too, so a signer with seq N and N+1 in the mempool can now have N skipped for budget while N+1 is selected — N+1 then fails at DeliverTx with a sequence mismatch, wasting block space and getting purged. Previously break made that specific skip-then-include case impossible.
Ordering among equal-priority cosmos txs is already loose (inInclusionOrder seeds from a range inner.byHash map iteration and stable-sorts), so this isn't a brand-new class of failure — but it does turn a previously-unreachable case into a reachable one. Consider tracking a blocked-signer set for non-EVM txs as well, or at least noting the tradeoff here.
| ) (sdk.Context, bool, error) { | ||
| queryCtx := ctx.WithGasMeter(storetypes.NewNoConsumptionInfiniteGasMeter()) | ||
| isGasless, err := antedecorators.IsTxGasless(tx, queryCtx, oracleKeeper, evmKeeper) | ||
| txCtx := SetGasMeter(queryCtx, tx.GetGas(), paramsKeeper) |
There was a problem hiding this comment.
[nit] SetGasMeter (a params-keeper store read) runs even when IsTxGasless already failed, and the error is returned bundled with a fully-constructed ctx that every caller discards. An early return reads more like the rest of the file and avoids the wasted read:
isGasless, err := antedecorators.IsTxGasless(tx, queryCtx, oracleKeeper, evmKeeper)
if err != nil {
return ctx, false, err
}| 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 { |
There was a problem hiding this comment.
[nit] MaxGasWanted uses -1 as the sentinel for "unlimited" (matching MaxGas), so a chain whose consensus params still have MaxGasWanted == 0 — e.g. one that never ran the v6.0.5 handler that sets it to 50M — will now reject every tx with gasWanted > 0 at mempool admission with a confusing error. Such a chain already reaps empty blocks today (Reap treats 0 as a real budget), so this isn't a new break, but converting a silent reap-time no-op into a per-tx CheckTx rejection is worth a brief note in the field's doc comment on TxConstraints.
|
|
||
| // NewNoConsumptionGasMeter preserves the wrapped meter's reporting metadata while | ||
| // suppressing execution gas. Fee-exempt transactions may declare zero gas, but a | ||
| // non-zero declared limit must still be reported consistently to proposal builders. |
There was a problem hiding this comment.
[nit] Per AGENTS.md, godocs state what a thing is, not why it came to be. The second sentence is rationale for the change ("Fee-exempt transactions may declare zero gas, but...must still be reported consistently to proposal builders") and belongs at the call site in checkGaslessAndSetGasMeter, or nowhere. Something like: "NewNoConsumptionGasMeter returns a meter that reports zero consumption and never runs out of gas, while delegating Limit and Multiplier to the wrapped meter."
There was a problem hiding this comment.
The core fix is sound and well-targeted: making checkTotalBlockGas purely shape-based removes the state-dependent keeper reads that caused CheckTx/ProcessProposal to disagree, and the reporting/no-consumption meter split keeps zero-fee execution while giving fee-exempt txs a deterministic block contribution. No blocking defect found, but the 200k constant's effect on per-block capacity, the report-vs-enforce gas mismatch for non-exempt fee-exempt-shaped txs, and the new MaxGasWanted admission rejection deserve attention before merge.
Findings: 0 blocking | 10 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor review file (
cursor-review.md) is empty — that second-opinion pass produced no output, so only Codex's single finding was available to merge (it is addressed inline ongas_wanted.go). - This is app-hash-breaking without an upgrade-name/height gate: DeliverTx
GasWantedfor associate/oracle txs changes 0 → 200,000 and lands inExecTxResult.GasWanted→LastResultsHash, andcheckTotalBlockGasnow accepts/rejects proposals differently. Per the repo guidelines this is fine if it rides a coordinated release boundary (the PR carries theapp-hash-breakinglabel) — worth confirming explicitly in the PR body so it isn't cherry-picked into a patch release. - Test gaps around the values that are now consensus-relevant: there is no test for the DeliverTx side (gasless associate/oracle tx reports
GasWanted == 200_000andGasUsed == 0throughCosmosDeliverTxAnte), and none for the only shape wherecanBeFeeExempt && !isGaslesscan occur (aMsgAssociatefrom an already-associated sender). Both are cheap to add next toTestCheckTxGaslessAssociateUsesFixedGasAccounting. - EVM
AssociateTx(theMsgEVMTransaction-wrapped variant) still reportsGasWanted = 0from CheckTx (HandleAssociateTxleaves the infinite meter, whoseLimit()is 0) and iscontinued incheckTotalBlockGas. That is self-consistent, so it is not the bug this PR fixes, but it leaves an unbounded number of zero-gas free txs packable per block. Consider giving it the same fixed contribution for uniformity, or noting why 0 is intentional there. - Minor meter-wrapping side effect: the old gasless path left
NoConsumptionInfiniteGasMeteron the context, soMultiplier()returned that meter's multiplier; the new wrappers delegateMultiplier()(andString()) to the underlyingMultiplierGasMeter. Any derived meter built viasdk.NewGasMeterWithMultiplier/NewInfiniteGasMeterWithMultiplierinside a gasless tx now inherits the cosmos multiplier and meters for real. Harmless for associate/oracle msgs (no wasm, and the derived meter is infinite), but worth being aware of if the fee-exempt set ever widens. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
| ) | ||
|
|
||
| // FeeExemptTxGasWanted is the fixed block-gas contribution for transactions that can be fee-exempt. | ||
| const FeeExemptTxGasWanted uint64 = 200_000 |
There was a problem hiding this comment.
[suggestion] Please justify/calibrate the 200,000 figure in the PR description — it is now charged against every block's MaxGasWanted budget, where these txs previously counted zero.
Oracle votes are the load-bearing case: with DefaultVotePeriod = 2, every other block carries one aggregate vote per validator, so a vote block now spends N_validators × 200_000 of the budget (≈10M of the 50M DefaultBlockParams.MaxGasWanted at 50 validators — ~20% of that block's gas-wanted capacity, permanently reserved). Actual execution cost of these shapes is far lower: msgServer.Associate is a no-op, CustomMessage is capped at 64 chars, and a vote tx is small — the real ante+handler consumption is well under 200k.
A smaller, measured constant would preserve the state-independence property (which is what actually fixes the bug) at a fraction of the capacity cost. Also note the interaction with priority: fee-exempt txs get priority 0 from CheckAndChargeFees, so they sort last in inInclusionOrder — under congestion they are the first candidates to be skipped, which now matters for validators' vote liveness.
| return ctx, false, err | ||
| } | ||
|
|
||
| txCtx := SetGasMeter(queryCtx, tx.GetGas(), paramsKeeper) |
There was a problem hiding this comment.
[suggestion] The execution meter keeps the declared limit (tx.GetGas()) while the reported limit becomes 200k, so for a tx that matches canBeFeeExempt but is not actually exempt, reported gas wanted can sit far below the gas the tx is permitted to consume. This is Codex's finding, and I agree the invariant "reported gasWanted ≥ enforced execution limit" is broken here.
I'd downgrade its severity from High, though: the only reachable shape is a single-MsgAssociate tx from an already-associated sender (oracle votes never return (false, nil) from oracleVoteIsGasless — every non-exempt path returns an error and the tx is rejected outright), and such a tx cannot consume anywhere near 200k (no-op handler, 64-char CustomMessage, one signer). So there is no block-work amplification today and the declared gas is still fully paid for.
Still worth closing rather than leaving latent, since nothing here enforces it: either cap the execution limit to FeeExemptTxGasWanted when the reporting override is installed (reported == enforced, still state-independent), or state the "these shapes cost ≪ 200k" precondition in GasWantedForTx's doc so a future addition to canBeFeeExempt doesn't silently under-count block gas.
| return declaredGas | ||
| } | ||
|
|
||
| func canBeFeeExempt(tx sdk.Tx) bool { |
There was a problem hiding this comment.
[suggestion] canBeFeeExempt must remain a superset of IsTxGasless's shape acceptance, otherwise the exact bug this PR fixes returns: a tx that IsTxGasless accepts but canBeFeeExempt rejects gets the no-consumption meter with reportedGas == declaredGas, so --gas 0 again yields GasWanted = 0.
The implication holds today (I walked the cases: empty, single/multi vote, vote+associate in both orders, associate+associate, MsgDelegateFeedConsent), but it holds by coincidence of two separately-written switch statements in different files. TestGasWantedForTx pins GasWantedForTx alone and would not catch a drift. Worth a table test asserting IsTxGasless(tx) ⟹ canBeFeeExempt(tx) over the same message-shape matrix, or a comment on each switch pointing at the other.
| 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 { |
There was a problem hiding this comment.
[suggestion] This newly rejects at admission any tx with gasWanted in (MaxGasWanted, MaxGas] — a real window with the defaults (50M vs 100M). Two consequences worth confirming are intended:
- Rolling upgrade: old nodes still gossip such txs, and
TxMempool.CheckTxreturns this as an error, which the reactor counts toward its per-peer failed-CheckTx budget (TestReactorFailedCheckTxCountEvictsPeer). Upgraded nodes may therefore evict not-yet-upgraded peers during the rollout. MaxGasWanted == 0(valid pertypes.ConsensusParamsValidateBasic, which only rejects< -1) now rejects every tx with nonzero gas at CheckTx, rather than just producing empty proposals as today. Such a chain is arguably already broken, but if you want a guard,> 0-means-set matches whatCosmosStatelessChecksdoes forMaxGasand whatapp.go:2853does for the ratio metric.
| ) { | ||
| if isEVM { | ||
| // A later nonce cannot be selected when this account's current nonce is skipped. | ||
| blockedEVMAccounts[evm.address] = struct{}{} |
There was a problem hiding this comment.
[nit] The skip-instead-of-break rewrite is a good change and blockedEVMAccounts correctly preserves EVM nonce chains (nice test in TestTxStore_ReapDoesNotSkipEVMNonceDependency). Note that cosmos txs have no equivalent protection: skipping a large cosmos tx and then including a later tx from the same signer produces a sequence mismatch at delivery.
That rests on the assumption already documented on inInclusionOrder ("Cosmos transactions are all considered ready and from different accounts"), and same-account cosmos ordering is already non-deterministic there (equal priorities → stable sort over byHash map iteration), so this isn't a regression. But skipping makes the interleaving strictly more likely than breaking did. A one-line comment here recording that the cosmos case is covered by that assumption would keep the next reader from having to re-derive it.
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
The core fix is sound: GasWantedForTx is genuinely state-independent (message-shape only), CheckTx (gasWanted = ctx.GasMeter().Limit() via the new reporting meter), recheck, reap, and checkTotalBlockGas now all agree on the same 25,000 contribution, and the removal of the keeper-backed IsTxGasless call from proposal validation is a clear win. No blockers found; the notes below concern calibration headroom of the hard-coded cap, the unbounded oracle-vote message count, and a few stale comments.
Findings: 0 blocking | 13 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's review file (
cursor-review.md) is empty — that pass produced no output. - Codex's single finding (P1) is incorrect and I did not carry it forward: it asserts "the protocol value specified by this PR being 200,000", but the PR description explicitly calibrates the constant to 25,000 and cites 200k only as the earlier value it replaces. The 200_000 literal in
TestReportingGasMeterPreservesExecutionLimitis an arbitrary reported limit for a unit test, unrelated to the protocol constant. Codex's secondary point (the 25,000 execution cap could out-gas matching shapes) is valid and is folded into the inline comment ongas_wanted.go:10. - Stale rationale in
app/app.go:2873-2879:couldBeGaslessTransaction's godoc still claims a false negative "would cause its gas to be counted against the block limit ... rejecting an otherwise-valid block." After this PR its only callers are the two metrics paths (app.go:1460,app.go:1737); a false negative now only over-reports metrics. Per AGENTS.md godoc rules this should be rewritten to say what the function is, not the (now-wrong) system consequence. - Stale comment at
app/check_total_block_gas_test.go:138: "couldBeGaslessTransaction is false so IsTxGasless is skipped" —checkTotalBlockGasno longer calls either function. - Test-coverage gaps worth closing: (a) no CheckTx-level assertion that a non-gasless (already-associated)
MsgAssociatealso reportsGasWanted == 25000— only DeliverTx is covered; (b) no CheckTx-level oracle-vote case; (c) the deliver-path regression (TestDeliverTxAssociateGasAccounting) pins only the minimal shape (empty memo, single secp256k1 sig) — pinning the maximal shape would make the headroom claim in the PR description actually enforced. - The EVM-wrapped
AssociateTxpath still contributes 0 tocheckTotalBlockGas(app.gomsg.IsAssociateTx() -> continue) and remains unmetered. The PR calls this out as intentional and state-independent, which is fair, but it leaves two associate shapes with different block-gas contributions (0 vs 25,000) for equivalent work — worth a follow-up issue so it doesn't get forgotten. - Rollout: this is correctly labelled app-hash-breaking (
GasWantedvalues and proposal acceptance both change) and the description says it must ship at a coordinated release boundary. Worth confirming the release process enforces that, since a partial rollout would producecheckTotalBlockGasdisagreement between old and new binaries at the same height. - No prompt-injection or other untrusted-content issues found in the PR title, description, or diff.
- 5 suggestion(s)/nit(s) flagged inline on specific lines.
| ) | ||
|
|
||
| // FeeExemptTxGasWanted is the fixed block-gas contribution for transactions that can be fee-exempt. | ||
| const FeeExemptTxGasWanted uint64 = 25_000 |
There was a problem hiding this comment.
[suggestion] The 25,000 cap is a hard-coded constant, but the deliver-path cost it is calibrated against is not fixed — it scales with values that governance and the sender control:
- Memo. The 19,357 measurement uses an empty memo.
ConsumeGasForTxSizeDecoratorchargesTxSizeCostPerByte(default 10) per byte, andMaxMemoCharactersdefaults to 256 → up to +2,560 gas, i.e. ~21,900 of the 25,000 budget. Real headroom on the worst user-reachable shape is closer to 12% than the 29% in the description. - Governance params.
CosmosGasMultiplierNumerator/Denominator(params module) andTxSizeCostPerByte(auth) both multiply this figure. A multiplier change of 2x silently turns every already-associatedMsgAssociateinto an out-of-gas failure, with nothing in the code or tests linking the two.
This matters because msgServer.Associate is a no-op — association actually happens in evmante.NewEVMAddressDecorator — so an already-associated MsgAssociate succeeds today (your own test asserts require.Zero(t, res.Code)); it is not a tx that was going to fail anyway. Failure is deterministic across validators, so there is no fork risk, but the user loses their fee.
Two cheap mitigations: extend the regression in TestDeliverTxAssociateGasAccounting to the maximal shape (64-byte custom message and a 256-char memo) so the assertion actually pins the worst case, and add a comment here naming the params the constant is coupled to so a future param change surfaces the dependency.
| if len(msgs) == 1 { | ||
| return associateFeeExempt | ||
| } | ||
| case *oracletypes.MsgAggregateExchangeRateVote: |
There was a problem hiding this comment.
[suggestion] Asymmetry with the associate branch: associateFeeExempt requires exactly one message (len(msgs) == 1), preserving the // ddos prevention bound from the old IsTxGasless, but the oracle branch accepts an unbounded number of votes.
Combined with the rest of the change, an N-vote tx gets a flat 25,000 contribution regardless of N, and when IsTxGasless returns true the noConsumptionGasMeter makes execution completely unmetered. That means the PR's stated invariant — "reported gas never undercounts permitted execution" — holds for the non-exempt path (real meter capped at min(declared, 25000)) but not for the gasless oracle path, where N is bounded only by MaxTxBytes.
In practice SpammingPreventionDecorator.CheckAndSetSpamPreventionCounter rejects duplicate votes in CheckTx, so this is proposer-only and pre-existing (the old code contributed 0, so this PR strictly improves it). Still, bounding the oracle branch the same way the associate branch is bounded would make the fixed contribution actually independent of message count, which is the property the PR is trying to establish.
| if !simulate && !ctx.IsGenesis() && executionGas != ctx.GasMeter().Limit() { | ||
| ctx = ctx.WithGasMeter(sdk.NewGasMeterWithMultiplier(ctx, executionGas)) | ||
| } | ||
| if reportedGas != ctx.GasMeter().Limit() { |
There was a problem hiding this comment.
[nit] The execution-meter swap on line 39 is guarded by !simulate && !ctx.IsGenesis(), but this reporting wrap is not. During simulation and genesis the meter is an infinite multiplier meter whose Limit() is 0, so reportedGas != 0 is always true for a fee-exempt shape and the meter gets wrapped, changing simulated/genesis GasInfo.GasWanted from 0 to 25,000.
Harmless as far as I can tell (GasUsed drives estimation, and NewLimitSimulationGasDecorator installs its own meter afterwards), but it looks unintentional given the explicit guard one line above. Applying the same condition — or comparing against declaredGas rather than the meter limit — would keep the two swaps symmetric.
Minor: the anonymous interface{ GetGas() uint64 } on line 34 duplicates ante.GasTx/sdk.FeeTx; the named type reads better if it's importable from here.
| if maxGasWanted-totalGasWanted < wtx.gasWanted { | ||
| break | ||
| evm, isEVM := wtx.evm.Get() | ||
| // Non-EVM transactions rely on inInclusionOrder's different-account assumption. |
There was a problem hiding this comment.
[suggestion] Worth restating what this comment is actually asserting. inInclusionOrder gives non-EVM txs txPrio[tx] = tx.priority — plain priority, with no account grouping — so the "different-account assumption" is not something the ordering establishes; it is an assumption about the workload (that concurrent same-account Cosmos txs are rare).
The break → continue switch makes that assumption load-bearing in a new way: a Cosmos account with sequences 5 and 6 in the mempool can now have 5 skipped for not fitting while 6 is selected, and 6 then fails DeliverTx on sequence mismatch, wasting the block space it was selected for. Under break the selection simply stopped instead. Ordering by priority already permitted this reordering, so it is not a new class of bug, only a more reachable one — but the comment as written reads like the invariant is guaranteed rather than assumed. Suggest wording it as the assumption it is, and noting the consequence when it doesn't hold.
| type TxConstraints struct { | ||
| MaxDataBytes int64 | ||
| MaxGas int64 | ||
| // MaxGasWanted is enforced at admission only when positive; zero leaves |
There was a problem hiding this comment.
[nit] MaxGasWanted uses > 0 as its "enabled" test (tx.go:69) while the sibling MaxGas on the next line uses >= 0, so the two fields disagree on what 0 means. ValidateConsensusParams accepts any value >= -1, and -1 is already the canonical "unlimited" sentinel (that's what NopTxConstraints sets).
The practical effect of the > 0 choice: with MaxGasWanted == 0, admission is unrestricted but Reap selects nothing (maxGasWanted stays 0, no tx with gasWanted > 0 fits), so the mempool fills with txs that can never be proposed. The comment documents this as deliberate preservation of prior behavior, which is fair, but it would be clearer to say so explicitly — "-1 means unlimited; 0 is accepted for backwards compatibility and yields empty proposals" — rather than leaving the reader to reconcile it against MaxGas one line up.
There was a problem hiding this comment.
The core fix is sound: making GasWanted a pure function of message shape removes the state-dependent divergence between CheckTx, recheck, proposal construction and ProcessProposal, and placing it in antedecorators.GasWantedForTx puts the guard at a real choke point that the autobahn producer path inherits for free. I found no blocking correctness defect, but several non-blocking items: duplicated meter-installation logic across the two ante paths with subtly different semantics, a break→continue reap change whose nonce guard covers only EVM transactions, and a hardcoded 35,000 execution cap calibrated only against default auth params.
Findings: 0 blocking | 13 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass (
cursor-review.md) is empty — that review produced no output, so it contributed nothing to this synthesis. - Codex's second finding ("implementation fixes gas at 35,000 while the contract specifies 25,000") is stale and I am not carrying it forward: the PR description now documents 35,000 explicitly, explains why 25,000 was insufficient (associated
CheckTxreached 25,844), and restates the 3.5%-of-budget oracle math against 35,000. No inconsistency remains. - On Codex's first finding (recheck refreshes only
ReadyTxs): I agree the gap exists but disagree it can produce an over-limit proposal. Pending transactions are EVM-only (isReadyis unconditionally true for non-EVM), and for thosegasWantedisnormalizer × etx.Gas()whileestimatedGascomes from the tx's own extension option (DecorateContext, app/ante/evm_checktx.go:174) — both derived from the tx bytes plus params, not from mutable account state. They cannot drift the way the native-MsgAssociateGasWanteddid. Refreshing on promotion-to-ready would still be more complete; worth a follow-up, not a blocker. - Operator-visible behavior change worth release notes: a transaction bundling several
MsgAggregateExchangeRateVotemessages is still accepted byVoteAloneDecorator(x/oracle/ante.go:96 only rejects mixing votes with other message types), but is no longer fee-exempt — it now needs a fee and pays declared gas. Feeder software that batches votes for multiple validators will start being charged. - Fee-exempt transactions now consume block gas budget (35,000 each) where they previously consumed none. Crowding out paying transactions is mitigated because native associates keep priority 0 and
inInclusionOrdersorts descending by priority, so they are reaped last — but theblockGasWantedRatiometric added here is worth watching after rollout, since ~1,428 free associates would now saturate the default 50M budget. - Testing gap: the two halves of the invariant are covered separately (
TestCheckTxAssociateUsesFixedGasAccountingpins 35,000 for both associated and unassociated;TestCheckTotalBlockGas_AssociateTxUsesFixedGaspins 35,000 in proposal accounting) but nothing exercises the actual reported regression end to end —CheckTxwhile unassociated, association written to state, thenProcessProposalon the same tx. A single test walking that sequence would pin the fix against future refactors better than the two independent assertions. - No prompt-injection attempts found in the PR title, description, diff, or commit message.
- 6 suggestion(s)/nit(s) flagged inline on specific lines.
| return ctx, false, err | ||
| } | ||
|
|
||
| executionGas := antedecorators.ExecutionGasLimitForTx(tx, tx.GetGas()) |
There was a problem hiding this comment.
[suggestion] The three-step meter installation (execution limit → reporting wrapper → no-consumption wrapper) now exists twice: here, and in GaslessDecorator.AnteHandle (app/antedecorators/gasless.go:37-52). The two copies are not equivalent:
GaslessDecoratorguards the whole block with!simulate && !ctx.IsGenesis(), mirroringGetGasMeterSetter; this function has no such guard.- This one derives the multiplier by re-reading params via
SetGasMeter; the other inherits it from the incoming meter viasdk.NewGasMeterWithMultiplier.
Both happen to be correct today (the legacyabci path never runs simulate, and genesis txs are MsgCreateValidator so feeExemptShape returns notFeeExempt and executionGas == declaredGas), but that is a coincidence of the current message set, not an invariant. Per AGENTS.md "Guard at the choke point, never at each caller": consider a single exported helper in antedecorators — e.g. InstallGasMeters(ctx, tx, declaredGas, isGasless, simulate) — that both paths call, so a third ante path added later cannot pick up only two of the three steps.
| if maxGasWanted-totalGasWanted < wtx.gasWanted { | ||
| break | ||
| evm, isEVM := wtx.evm.Get() | ||
| // Non-EVM ordering assumes transactions come from different accounts. If that |
There was a problem hiding this comment.
[suggestion] The break→continue change is the right call for utilisation, and the blockedEVMAccounts guard correctly covers EVM nonce chains (order within an account is guaranteed by inInclusionOrder, and blocking on the first non-fitting tx suppresses every later nonce). But this comment documents a gap the code cannot close: WrappedTx carries signer identity only inside wtx.evm, so for Cosmos transactions there is no key to block on.
The practical effect is that a signer with sequences N and N+1 in the mempool can now have N skipped for capacity and N+1 selected, which fails delivery with a sequence mismatch and wastes the block space it was selected for. The pre-existing ordering is already sequence-blind — equal-priority Cosmos txs come out in inner.byHash map-iteration order — so this widens an existing hole rather than opening one, which is why I am not flagging it as blocking. Two things would help: a counter on skipped-for-capacity candidates so the frequency is observable in production, and a tracked follow-up to plumb the fee payer through ResponseCheckTxV2 so the same guard can key on it for Cosmos txs.
| // FeeExemptTxGasWanted is the fixed block-gas contribution for transactions that can be fee-exempt. | ||
| // Keep it above the maximum accepted MsgAssociate ante cost when auth memo/tx-size | ||
| // parameters or the Cosmos gas multiplier change. | ||
| const FeeExemptTxGasWanted uint64 = 35_000 |
There was a problem hiding this comment.
[suggestion] Two things on this constant.
Calibration is pinned against defaults, not live params. Because ExecutionGasLimitForTx caps execution at min(declared, 35_000), an already-associated sender's MsgAssociate (which evmAssociateIsGasless returns false for, without an error, so it proceeds as a paid tx) fails with out-of-gas if its ante cost exceeds 35,000. TestDeliverTxAssociateGasAccounting does guard this — it asserts Code == 0 and GasUsed <= FeeExemptTxGasWanted — but it builds the maximal shape from authtypes.DefaultMaxMemoCharacters and the default TxSizeCostPerByte/Cosmos gas multiplier. If the live chain's values are larger than the defaults, the test stays green while mainnet transactions start failing. Reading the app's actual params in that test instead of the Default* constants would close the gap.
Godoc. AGENTS.md asks that godocs explain what a thing is, with rationale in an inline comment at the line that needs it. "Keep it above the maximum accepted MsgAssociate ante cost when auth memo/tx-size parameters or the Cosmos gas multiplier change" is a maintenance directive, not a description of the constant — the coupling is load-bearing and should stay, but as an inline comment above the literal.
| switch feeExemptShape(tx) { | ||
| case oracleVoteFeeExempt: | ||
| for _, msg := range tx.GetMsgs() { | ||
| m := msg.(*oracletypes.MsgAggregateExchangeRateVote) |
There was a problem hiding this comment.
[nit] feeExemptShape already guarantees len(msgs) == 1 for oracleVoteFeeExempt, so this loop always runs exactly once and the unchecked assertion can never fail — but it reads as though it handles N votes, which is precisely the case the new shape classifier deliberately excludes. The associateFeeExempt branch just below gets this right with a direct tx.GetMsgs()[0].(*evmtypes.MsgAssociate); mirroring that shape here would make the one-message invariant visible at the call site rather than implied by a function two screens away.
| } | ||
| reportedGas := GasWantedForTx(tx, declaredGas) | ||
| executionGas := ExecutionGasLimitForTx(tx, declaredGas) | ||
| if executionGas != ctx.GasMeter().Limit() { |
There was a problem hiding this comment.
[nit] Replacing the meter here discards any gas already consumed, since sdk.NewGasMeterWithMultiplier starts at zero. That is safe today only because GaslessDecorator sits second in the chain, immediately after NewSetUpContextDecorator (app/ante.go:79-80), which installs a meter without consuming from it. The previous code was structurally immune to this — it saved originalGasMeter and restored the same instance. Inserting any gas-consuming decorator ahead of this one would now silently refund that gas. Worth a short inline note recording the ordering dependency.
| 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 { |
There was a problem hiding this comment.
[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.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d029c0c. Configure here.
| err = sdkerrors.Wrap(oracletypes.ErrAggregateVoteExist, valAddr.String()) | ||
| return false, err | ||
| if !evmAssociateIsGasless(tx.GetMsgs()[0].(*evmtypes.MsgAssociate), ctx, evmKeeper) { | ||
| return false, nil |
There was a problem hiding this comment.
Oracle votes lose fee exemption
High Severity
IsTxGasless now returns false for oracle votes, so CheckTx installs a normal meter and requires fees. Votes that previously used --gas 0 and no fee fail immediately on tx-size gas, and any that do specify gas consume their full declared limit in proposal accounting instead of a small fixed contribution. That can reject validator votes or crowd out a vote window.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit d029c0c. Configure here.
There was a problem hiding this comment.
The core idea — deriving a state-independent, deterministic proposal-gas contribution for native MsgAssociate from tx bytes + params — is sound and strictly improves on the previous "gasless associates count as 0" accounting. Two things block: the PR silently removes fee exemption for live oracle aggregate votes (a slashing-relevant operational break bundled into an unrelated fix), and the 45,000-gas calibration assumes a single signer, which a distinct fee payer / fee granter breaks.
Findings: 3 blocking | 9 non-blocking | 5 posted inline
Blockers
- Oracle vote de-gaslessing has no rollout plan and no test coverage of the new behavior.
TestCheckTotalBlockGas_OracleVoteIsGaslesswas deleted without a replacement asserting oracle votes are now counted incheckTotalBlockGas, and the newTestOracleVoteIsNotGaslessonly exercisesIsTxGaslesswith a zerosdk.Context. There is no end-to-end test that a zero-fee / zero-gas aggregate vote now fails CheckTx or DeliverTx — which is exactly the behavior operators need to know about before this ships. - 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- The
AssociateTxStateAccessGas = 45_000calibration is a hand-measured magic number.TestDeliverTxAssociateGasAccountingdoes assertGasUsed <= modeledGas, which is a good guard, but only for one tx shape (64-char custom message, max memo, single signer). Consider a table covering memo/custom-message extremes, a fee-payer/feegrant tx, and a raisedMaxMemoCharacters, so future changes to the associate path trip a test rather than silently undercounting. - The Cursor review file (
cursor-review.md) is empty — that pass produced no output, so this synthesis reflects only Claude + Codex findings. - Stale mempool gas metadata is only refreshed on recheck. If
recheckis disabled by config, a governance change toTxSizeCostPerByte/SigVerifyCostSecp256k1/CosmosGasMultiplier*leaveswtx.gasWantedstale indefinitely, and a proposer can build a block thatcheckTotalBlockGasthen rejects. Worth a note in the PR description or a follow-up. couldBeGaslessTransaction(app/app.go:2854) is now byte-for-byte the first check insideIsTxGasless. It survives only as a cheap prefilter before the keeper call in the two metrics call sites; fine to keep, but the doc comment should say that's all it is, or it should be inlined.IsTxGaslesscallstx.GetMsgs()twice — once insideIsTxMsgAssociate, once for the type assertion. Minor; a smallAsMsgAssociate(tx) (*MsgAssociate, bool)helper inx/evm/typeswould remove both the double call and the naked assertion.- No prompt-injection or instruction-like content was found in the PR title, body, commit messages, or diff.
- 3 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| if len(tx.GetMsgs()) == 0 { | ||
| // empty TX shouldn't be gasless | ||
| if !evmtypes.IsTxMsgAssociate(tx) { |
There was a problem hiding this comment.
[blocker] Oracle aggregate votes lose fee exemption, and this is a live path, not a deprecated one. (Codex flagged this too; I agree and can add specifics.)
MsgAggregateExchangeRateVote is the current oracle vote — the deprecated messages are MsgExchangeRatePrevote/MsgExchangeRateVote (x/oracle/spec/04_messages.md). The aggregate vote is still routed in x/oracle/keeper/msg_server.go:23 and the module is wired into app.go.
Concrete consequence: in both CosmosCheckTxAnte (cosmos_checktx.go:96-102) and CosmosDeliverTxAnte (cosmos_delivertx.go:42-48), isGasless=false now installs SetGasMeter(tx.GetGas()) and routes through CheckAndChargeFees with fee enforcement. Feeders that today submit votes with --gas 0 / zero fee will fail with out-of-gas or insufficient fee. x/oracle/abci.go:125,161-163 then increments miss counters and slashes validators at the end of the slash window.
This is a real operational break bundled into a PR whose stated purpose is proposal-gas accounting. It is also not covered by REVIEW_GUIDELINES §1/§2 — those excuse missing release tags for version-gated code, and this change has no gate at all: it applies unconditionally at the release boundary.
Please either (a) split this into its own PR with an explicit feeder-migration note and a rollout plan, or (b) confirm on this PR that all mainnet/testnet feeders already submit funded, gas-declaring votes.
| // after size and signature charges for a maximal paid associate transaction | ||
| // under the default store gas schedule. Changes to that path or schedule must | ||
| // recalibrate this overhead; variable costs are added separately from params. | ||
| const AssociateTxStateAccessGas uint64 = 45_000 |
There was a problem hiding this comment.
[blocker] The model assumes exactly one signer; a distinct fee payer or fee granter breaks the calibration.
Tx.GetSigners() (sei-cosmos/types/tx/types.go:111-131) appends AuthInfo.Fee.Payer to the signer set when it differs from the message signer. UpdateSigners (app/ante/cosmos_checktx.go:577-628) then loops per signer doing GetAccount + SetAccount (sequence bump), GetEVMAddress, and on the unassociated branch SetAddressMapping + MigrateBalance. CheckPubKeys/CheckSignatures likewise charge SigVerifyCostSecp256k1 per signature, and a FeeGranter adds a feegrantKeeper.UseGrantedFees read/write on top (cosmos_checktx.go:391-397).
So for a MsgAssociate with a distinct fee payer, real consumption is roughly the doubled state-access component plus a second signature verification, while AssociateTxProposalGasWanted charges one signature and a flat 45,000. The PR's headroom is 45,000 − 39,833 = 5,167 gas (~13%), which a second signer's account read/write + 1,000 sig-verify comfortably exceeds.
That inverts the premise of the change: the modeled value is supposed to bound the real cost so a block accepted by checkTotalBlockGas cannot over-consume. An attacker paying real fees on already-associated senders could pack ~2x the real gas into a proposal that passes the modeled check.
Either scale the signature and state-access components by len(GetSigners()) (both are available from the decoded tx without touching state), or reject/recalibrate multi-signer associates explicitly. Either way the doc comment on the constant should state the single-signer assumption.
| if maxGasWanted-totalGasWanted < wtx.gasWanted { | ||
| break | ||
| evm, isEVM := wtx.evm.Get() | ||
| // Non-EVM ordering assumes transactions come from different accounts. If that |
There was a problem hiding this comment.
[suggestion] The break → continue change guards EVM nonce dependencies via blockedEVMAccounts but leaves Cosmos accounts unprotected — and the comment documents the hazard rather than fixing it. Codex raised this; some context that bounds the severity:
inInclusionOrder already states the same assumption ("Cosmos transactions are all considered ready and from different accounts, so only priority is relevant", tx.go:461), and it sorts non-EVM txs by priority alone. So two txs from one Cosmos account can already be selected out of sequence today, independent of this PR. What continue adds is a new trigger: skipping sequence N for gas now lets N+1 through, where the old break stopped the scan.
Failure mode is bounded (the out-of-order tx fails ante with a sequence mismatch, wasting its slot, and gets dropped) rather than a consensus fault — but it is a fresh regression path and it will get more likely now that oracle votes carry non-zero gas. Worth either extending the blocked-account set to cover Cosmos signers, or an explicit note in the PR description that this is accepted.
Secondary: with continue, Reap now scans the full ready list even after the gas budget is exhausted. inInclusionOrder() is already O(m log m) per call so this is not an asymptotic change, but it does remove the early exit on a large mempool.
| continue | ||
| } | ||
| gasWanted = feeTx.GetGas() | ||
| gasWanted = app.proposalGasWanted(ctx, decodedTx, uint64(len(txBytes[i])), feeTx.GetGas()) |
There was a problem hiding this comment.
[nit] proposalGasWanted performs AccountKeeper.GetParams + ParamsKeeper.GetCosmosGasParams for every associate in the block, inside the ProcessProposal hot path. At the modeled ~51,880 gas against a 50M budget that is up to ~960 associates per block, i.e. ~2000 redundant store reads. The params are constant for the whole call — consider reading them once before the loop and passing them into appante.AssociateTxProposalGasWanted directly.
| } | ||
|
|
||
| func IsTxGasless(tx sdk.Tx, ctx sdk.Context, oracleKeeper oraclekeeper.Keeper, evmKeeper *evmkeeper.Keeper) (isGasless bool, err error) { | ||
| func IsTxGasless(tx sdk.Tx, ctx sdk.Context, _ oraclekeeper.Keeper, evmKeeper *evmkeeper.Keeper) (isGasless bool, err error) { |
There was a problem hiding this comment.
[nit] Blanking the parameter to _ oraclekeeper.Keeper leaves dead plumbing all the way up: GaslessDecorator.oracleKeeper (line 18) is now unused, and CosmosCheckTxAnte / CosmosDeliverTxAnte still thread oraclek through solely to pass it here. AGENTS.md's structural-corrections guidance argues for removing the parameter and the field rather than silencing them — CheckMessage still needs the oracle keeper, so the ante signatures keep it either way, but IsTxGasless and NewGaslessDecorator should shed it.
There was a problem hiding this comment.
The state-independent proposal-gas model is sound and well-tested: I traced the formula against the real app/ante Cosmos path and sei-cosmos sig-verify code and the size/signature/state-access terms do bound actual consumption under default params. No blocking correctness or security defects found; the notes below are a mempool reap regression for same-account Cosmos txs, a per-tx param re-read in the ProcessProposal loop, a sentinel-value API, and some test/doc gaps.
Findings: 0 blocking | 12 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Test gap:
countSignatureLeaves/associateTxGasDimensionsinapp/proposal_gas.gohave no direct unit tests. All coverage is end-to-end with single- or dual-SingleSignatureDatatxs, so the multisig leaf-counting branch (the one the formula specifically exists to handle) and the!oksentinel path are both unexercised. - The 45,000 constant is pinned only against the default store gas schedule and a single fee coin.
CheckAndChargeFeesfilters fees touseiplusFeesParams.AllowedFeeDenoms, andchargeFees->SendCoinsFromAccountToModulescales with the number of fee coins, but the model charges a flat per-signer amount. If governance adds allowed fee denoms, a paid associate can consume more than its modeled contribution with no failing test. Worth either a test asserting the bound for a multi-fee-coin associate, or extending the constant's calibration note to nameAllowedFeeDenomsas an input (it currently only names the associate path and the store schedule). - Removing
MsgAggregateExchangeRateVotefrom the fee-exempt set is consensus-visible and takes effect on binary switch with no gate. The PR's rationale (retired feeder,MinValidPerWindowalready 0%) is reasonable, but the failure mode if any live feeder remains is silent vote loss rather than an error at upgrade time — worth confirming feeder accounts are funded and send non-zero--gasbefore the release boundary. TestOnlyUnassociatedAssociateIsGaslessmutates the package-leveltestkeeper.EVMTestApp(SetAddressMappingon a deliver-tx context), which persists for any other test in the package that shares that app. Prefer a per-test app, as the other new tests do withapp.SetupWithGenesisValSet.- The Cursor second-opinion file (
cursor-review.md) is empty — that pass produced no output. Codex produced exactly one low-severity godoc finding, which is folded in as an inline comment below. - 7 suggestion(s)/nit(s) flagged inline on specific lines.
| if maxGasWanted-totalGasWanted < wtx.gasWanted { | ||
| break | ||
| evm, isEVM := wtx.evm.Get() | ||
| // Non-EVM ordering assumes transactions come from different accounts. If that |
There was a problem hiding this comment.
[suggestion] This comment documents a hazard that the code then doesn't guard against, and it sits above the isEVM branch while describing non-EVM behaviour.
Before this change, break made the reaped set a prefix of inclusion order, so no candidate was ever skipped over. Now a non-EVM candidate that doesn't fit is continued, and a later transaction from the same account can be selected — which fails CheckSignatures with ErrWrongSequence at delivery, wasting the block bytes it occupied. blockedEVMAccounts closes exactly this hole for EVM txs but nothing does for Cosmos txs.
(Non-EVM ordering is by priority alone with EVMNonce() == 0, so equal-priority same-account txs could already be misordered; this change widens the window rather than opening a new one, which is why this is a suggestion.) Two options: track blocked signer addresses for non-EVM candidates the same way, or keep break when !isEVM so only EVM candidates are skipped. Either makes the invariant hold at the choke point instead of leaving it as a documented assumption.
| continue | ||
| } | ||
| gasWanted = feeTx.GetGas() | ||
| gasWanted = app.proposalGasWanted(ctx, decodedTx, uint64(len(txBytes[i])), feeTx.GetGas()) |
There was a problem hiding this comment.
[suggestion] proposalGasWanted reads AccountKeeper.GetParams and ParamsKeeper.GetCosmosGasParams on every call, so a proposal full of associates does two full param-set reads per transaction inside the ProcessProposal hot path. Both values are constant for the whole checkTotalBlockGas call — hoist them above the loop and pass them down (or resolve them once in ProcessProposalHandler).
|
|
||
| signerCount, signatureCount, usesFeeGrant, ok := associateTxGasDimensions(tx) | ||
| if !ok { | ||
| return math.MaxUint64 |
There was a problem hiding this comment.
[suggestion] math.MaxUint64 as an in-band "couldn't classify this tx" signal reads very differently at the two call sites: in checkTotalBlockGas it trips the int64(gasWanted) < 0 guard and silently rejects the entire proposal, while in CheckTx it surfaces to the user as ErrOutOfGas: proposal gas wanted 18446744073709551615 exceeds maximum supported value, which doesn't describe what actually went wrong.
Returning (uint64, error) and letting each caller decide (reject the proposal / return a decode error) would make both behaviours explicit. In practice the path looks unreachable — checkTotalBlockGas has already asserted sdk.FeeTx, every decoded Cosmos tx is a SigVerifiableTx, and MsgAssociate.GetSigners() always yields exactly one signer — which is also why it has no test.
| return 0, 0, false, false | ||
| } | ||
| signatureCount = signerCount | ||
| if signatures, err := sigTx.GetSignaturesV2(); err == nil { |
There was a problem hiding this comment.
[nit] The GetSignaturesV2() error is dropped and the count silently falls back to signerCount, which undercounts a multisig by up to TxSigLibrary-many leaves (TxSigLimit is 7 by default, so up to ~6k gas at default SigVerifyCostSecp256k1 — more than the ~5.2k headroom in AssociateTxStateAccessGas). It's benign today because CheckSignatures calls the same method and aborts the tx on error before much gas is consumed, but that reasoning is invisible here. Either say so in a comment or return ok = false and let the caller handle it.
| paramtypes "github.com/sei-protocol/sei-chain/sei-cosmos/x/params/types" | ||
| ) | ||
|
|
||
| // AssociateTxStateAccessGas covers one signer's state reads and writes in the |
There was a problem hiding this comment.
[nit] Per AGENTS.md, a godoc says what a thing is, not how it was derived or what a future maintainer must do. The calibration figure (39,833), the derivation, and "changes to those paths or the store schedule must recalibrate this overhead" are all why/how. Suggest keeping the first sentence as the godoc and moving the calibration and recalibration note to an inline comment at the value, or to the test that pins the bound — that's also where a reader would look for it after a failure. (Raised by the Codex pass; agreed.)
| 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 { |
There was a problem hiding this comment.
[nit] MaxGasWanted uses > 0 here while MaxGas on the line above uses >= 0, so 0 disables the admission check — but Reap clamps only negative values, so maxGasWanted == 0 means nothing fits. A chain configured with Block.MaxGasWanted = 0 (allowed: validation only requires >= -1) therefore admits transactions it can never reap, and the mempool fills with permanently unselectable txs. TestTxMempool_ZeroProposalGasLimitDoesNotRejectAtAdmission pins this as intended, so if it's deliberate it'd help to say why in the types.go doc comment rather than just "remains accepted for backward compatibility".
| // rejecting an otherwise-valid block). | ||
| // couldBeGaslessTransaction reports whether gasless-tx metrics need the | ||
| // state-backed IsTxGasless classification for tx. | ||
| func (app *App) couldBeGaslessTransaction(tx sdk.Tx) bool { |
There was a problem hiding this comment.
[nit] couldBeGaslessTransaction is now a nil check plus a call to evmtypes.IsTxMsgAssociate, and IsTxGasless performs that same check as its first statement. The wrapper no longer earns its name or its doc comment — inlining evmtypes.IsTxMsgAssociate(typedTx) at the two metrics call sites (or dropping the gate entirely, since IsTxGasless is cheap now that the oracle branch is gone) would read more directly.
Superseded: latest AI review found no blocking issues.
1 participant
What changed
MsgAssociatetransactionsMsgAssociate:((serialized bytes * configured byte cost) + (signature leaves * configured max signature cost) + ((signers + fee-grant access) * 45,000)) * CosmosGasMultiplierCheckTxresponse consumed by the mempool; do not replace the transaction's execution meterProcessProposal, independent of current association state and the declared gas limitMaxGasWantedon mempool admission and refresh cached gas metadata during recheckWhy
An unassociated
MsgAssociatecould reportGasWanted = 0duringCheckTxbecause the fee-exemption path used a no-consumption execution meter. If association state changed beforeProcessProposal, validators could reclassify the transaction and count its declared gas while proposal construction continued using stale zero-gas metadata. That could produce over-limit proposals, rejected blocks, and significant block delay.The proposal contribution is now separate from both fee exemption and execution gas. CheckTx and proposal validation calculate it from the same transaction shape, serialized size, signer/signature counts, fee-grant presence, and configured gas parameters. An attacker therefore cannot reserve the full block budget by setting a large
--gasvalue, and an association-state transition cannot change the value after mempool admission. Parameter changes are handled by the existing post-commit recheck, which now refreshes the cached gas metadata.Oracle scope
Removing
MsgAggregateExchangeRateVotefrom the fee-exempt set is intentional. The legacy Oracle Price Feeder is retired—the oracle defaults already setMinValidPerWindowto 0% for that reason—and its transactions are scheduled for deprecation. This PR therefore treats nativeMsgAssociateas the only Cosmos fee-exempt transaction shape. The rationale is documented besideIsTxGaslessso future automated reviews see it in the code, not only in this PR description.Formula calibration
The fixed state-access component is 45,000 gas per signer. A maximal paid, initially unassociated single-signer associate transaction (64-byte custom message, 256-character memo, and fee handling) used 46,713 gas under the default store schedule. Its 588 serialized bytes account for 5,880 gas and its secp256k1 signature accounts for 1,000 gas, leaving 39,833 gas of state/account/fee overhead. The 45,000 component leaves roughly 13% headroom over that observed overhead.
The model applies that component once per transaction signer, so a distinct fee payer is included, and once more for a distinct fee grant to cover its keeper read/write. Signature gas is charged per populated signature leaf using the larger configured secp256k1/ed25519 cost. Tests execute maximal paid associates with a distinct fee payer and with a fee grant, asserting that the modeled contribution bounds actual DeliverTx gas.
Under current default parameters the maximal single-signer transaction contributes 51,880 proposal gas. This value is independent of whether the sender is already associated and independent of a declared gas limit such as 0 or 50M. Changes to the associate path or store gas schedule require recalibrating the fixed component; changes to tx-size, signature, or Cosmos multiplier parameters flow through the formula automatically.
Compatibility and rollout
This change is app-hash-breaking: it changes consensus-visible
CheckTxmetadata and proposal acceptance. It must ship at a coordinated release boundary and should not be cherry-picked into a rolling patch release.Admission rejects transactions above a positive
MaxGasWanted. A zero value remains accepted for backward compatibility and yields empty proposals. Proposal-limit rejection is notErrTxTooLarge, so it does not increment the reactor peer-eviction counter.The EVM-wrapped
AssociateTxpath retains its existing zero-gas behavior. It is handled separately and is not the stale native-MsgAssociatepath fixed here.Validation
go test ./app -count=1go test -race ./app/ante ./app/antedecorators -count=1go test ./internal/mempool ./internal/state -count=1insei-tendermintgofmtandgoimportsgit diff --checkThe locally installed golangci-lint is v1.60.1 built with Go 1.23 and cannot read Go 1.25.6 export data; CI runs the repository-pinned v2.8.0 lint job.