fix(distribution): make delegation-reward queries read-only#3738
fix(distribution): make delegation-reward queries read-only#3738codchen wants to merge 2 commits into
Conversation
The distribution reward query handlers (DelegationRewards / DelegationTotalRewards in grpc_query.go, and their amino equivalents in querier.go) call IncrementValidatorPeriod, which mutates distribution state (validator period, historical/current rewards, reference counts, and the zero-token community-pool transfer). Queries are meant to be side-effect free — the amino querier already branches a CacheContext "to isolate state changes", and the gRPC query server runs on a cached context — but the EVM distribution `rewards()` precompile calls the gRPC handler with the live context, so those writes actually persist and commit with the transaction. Replace the "IncrementValidatorPeriod then CalculateDelegationRewards" sequence in the query paths with a read-only computation: - Add currentRewardsEndingPeriodAndRatio, which computes in memory the ending period and cumulative reward ratio that IncrementValidatorPeriod would persist, without writing. - Factor CalculateDelegationRewards into a shared core that accepts the ending ratio; CalculateDelegationRewardsReadOnly passes the in-memory ratio and never writes. The state-changing (withdraw) path passes nil so the core reads the ending ratio from the store lazily in the final period — byte-for-byte the same store accesses and gas as before (verified: precompile withdraw gas unchanged). Tests assert the read-only result equals the write-path result (including the slash-event path) and that neither the helper nor the gRPC query mutates the validator period or historical reference count, and that repeated queries are idempotent. NOTE (gating): the gRPC handlers are called by the EVM distribution precompile (current and every legacy version), so no longer persisting these writes changes committed state for any tx that called `rewards()`. This is state-machine-breaking and must ship gated (precompile version / upgrade height) to preserve historical replay. Gating is intentionally left out of this focused change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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 #3738 +/- ##
==========================================
- Coverage 59.87% 58.95% -0.92%
==========================================
Files 2286 2199 -87
Lines 189603 179806 -9797
==========================================
- Hits 113522 106012 -7510
+ Misses 65959 64470 -1489
+ Partials 10122 9324 -798
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 Read-only path: Upgrade gating (v6.7.0): Tests cover read-only vs write-path equality, query idempotency, and all gating branches (live/tracing, pre/post v6.7.0). Reviewed by Cursor Bugbot for commit 025cce2. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The read-only reward computation is correctly implemented (it mirrors IncrementValidatorPeriod's ratio math exactly and preserves the withdraw path's store accesses/gas), and it's well-tested. However, the change alters committed consensus state for historical blocks without any version/upgrade-height gating, so it must not merge as-is — the PR author acknowledges this.
Findings: 2 blocking | 3 non-blocking | 1 posted inline
Blockers
- Missing consensus/replay gate. The EVM distribution precompile
rewards()(precompiles/distribution/distribution.go:349-350) callsDelegationTotalRewardswith the LIVE context (sdk.WrapSDKContext(ctx), no cache branch), so the previousIncrementValidatorPeriodwrites (validator period, historical/current rewards, reference counts, zero-token community-pool transfer) were actually persisted and committed for any tx that invokedrewards(). Removing them changes committed state — and gas consumed — for those historical txs, so replaying pre-upgrade blocks will diverge. This is confirmed by theapp-hash-breakinglabel and the PR description itself, which states the change 'must ship gated' and that gating was intentionally left out. The change should be gated behind a precompile version snapshot or upgrade-height flag before merge. - 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- cursor-review.md is empty — the Cursor second-opinion pass produced no output; only Codex's P0 (which this review confirms) was available.
- REVIEW_GUIDELINES.md is empty/missing, so no repo-specific standards could be applied.
- The Codex P0 anchors the gating concern to the single-delegation
DelegationRewardshandler (grpc_query.go:155). Note the concrete consensus break is via theDelegationTotalRewardshandler, which is the one the EVM precompile actually invokes with a live context; both query handlers change, but only the precompile-reached path commits state today.
| delReward := k.CalculateDelegationRewards(ctx, val, del, endingPeriod) | ||
| // Read-only: this is a query, so compute rewards without persisting a | ||
| // validator-period increment (see CalculateDelegationRewardsReadOnly). | ||
| delReward := k.CalculateDelegationRewardsReadOnly(ctx, val, del) |
There was a problem hiding this comment.
[blocker] Blocker (consensus/replay): this is the handler the EVM rewards() precompile invokes with a LIVE context (sdk.WrapSDKContext(ctx) at precompiles/distribution/distribution.go:349), so the removed IncrementValidatorPeriod writes were previously persisted and committed for every tx that called rewards(). Dropping them — and reducing the gas consumed on that path — changes committed state and app hashes for historical blocks, breaking replay of every prior version (current + legacy v65…v640). As the PR description states, this must be gated behind a precompile-version snapshot or upgrade-height flag; it should not merge ungated. (The read-only math itself is correct and matches the write path — the only issue is the missing gate.)
There was a problem hiding this comment.
The read-only reward computation is correct, well-factored, and gas-preserving on the withdraw path, with good tests. However the upgrade gate keys the "reproduce old mutating behavior" branch on ctx.IsTracing(), so any non-tracing execution (early binary rollout before the upgrade height, or block-by-block replay/sync of a pre-v6.7.0 block that invoked the rewards() precompile) will diverge from the historically-committed app hash — a consensus-safety concern that must be resolved before merge.
Findings: 1 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
readOnlyRewardsUpgrade = "v6.7.0"is a hardcoded constant; the PR body already notes it must be updated if the shipping upgrade name changes. Since a mismatched name silently mis-gates a state-machine-breaking change, consider deriving/validating it against the actual registered upgrade name (or add a compile-time/test assertion) rather than relying on a manual edit.- Cursor's second-opinion pass (
cursor-review.md) produced no output — no findings were available to merge from it. Codex's single P1 finding is the same consensus-divergence concern flagged inline below. - The read-only refactor itself looks correct:
currentRewardsEndingPeriodAndRatiofaithfully mirrors the ratio math inIncrementValidatorPeriod(historical[period-1] + current, zero-token → zero ratio), and the nil-endingRatiolazy read preserves the withdraw path's exact store accesses and gas. Tests cover match-with-write-path, query idempotency, and the gate.
| // behavior when tracing a block from before the v6.7.0 upgrade. Live execution | ||
| // (not tracing) always uses the read-only path. | ||
| func (k Keeper) DelegationRewardsForQuery(ctx sdk.Context, val stakingtypes.ValidatorI, del stakingtypes.DelegationI) sdk.DecCoins { | ||
| if ctx.IsTracing() && semver.Compare(ctx.ClosestUpgradeName(), readOnlyRewardsUpgrade) < 0 { |
There was a problem hiding this comment.
[blocker] The gate only reproduces the pre-v6.7.0 mutating behavior when ctx.IsTracing() is true, but ClosestUpgradeName is populated only on the RPC/trace context paths (app.go ~2670/2681/2721) — it is empty during normal DeliverTx/BeginBlock. That means all non-tracing execution takes the read-only branch unconditionally, regardless of height/upgrade, which breaks historical determinism in two ways:
- Non-tracing replay/sync of pre-v6.7.0 blocks. A node re-executing (block-by-block sync, not state-sync snapshot) a pre-v6.7.0 block that called the
rewards()precompile in a tx runs withIsTracing()==false, so it skipsIncrementValidatorPeriodand fails to reproduce the writes that were committed to that block's app hash → app-hash mismatch / halt. - Early binary rollout. If this binary runs live before the v6.7.0 upgrade height, it produces read-only blocks while old-binary nodes still mutate → divergence before the upgrade activates.
The existing !IsTracing() idiom in giga/deps/xevm/keeper/params.go is safe under replay only because it reads height-correct state (GetParams); here you're changing a write behavior that height-correct state can't reconstruct, so the analogy doesn't hold. This is Codex's P1.
Suggested fix: gate the live behavior on the upgrade actually being active for the block being executed (e.g. via the upgrade keeper's IsUpgradeActiveAtHeight / comparing ctx.BlockHeight() to the v6.7.0 upgrade height) in addition to the tracing branch, so binary-rollout timing and non-tracing replay both reproduce the correct behavior. The distribution keeper would need a reference to the upgrade keeper (or height) to do this.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The read-only reward-query change is state-machine-breaking on the EVM distribution `rewards()` precompile path (which ran the query on the live context and persisted IncrementValidatorPeriod's writes). Gate it so any pre-v6.7.0 block is re-executed with the old mutating behavior — whether it is re-traced OR replayed block-by-block during non-tracing sync — otherwise a syncing node would skip the writes committed to that block's app hash and halt on an app-hash mismatch. DelegationRewardsForQuery now dispatches via rewardQueryIsReadOnly(ctx): - Tracing: gate on ctx.ClosestUpgradeName() (set by the trace harness; empty during live execution) vs "v6.7.0". - Live / non-tracing replay: gate on the upgrade being active at ctx.BlockHeight() via UpgradeChecker.IsUpgradeActiveAtHeight, which is derived from the persisted upgrade "done" marker and therefore tracks the block being (re-)executed. If the checker is unset (some test harnesses), default to the old, always-reward-correct mutating behavior. Wiring: - Add types.UpgradeChecker (minimal IsUpgradeActiveAtHeight interface) and Keeper.SetUpgradeChecker; app.go injects app.UpgradeKeeper into the distribution keeper before it is copied into hooks/modules (mirroring SetHooks wiring). The reward figure is identical in both paths; only whether the query mutates state differs. TestDelegationRewardsForQueryUpgradeGate covers all four cases: live pre-v6.7.0 (mutates), live v6.7.0-active (read-only), tracing pre-v6.7.0 (mutates), tracing v6.7.0+ (read-only). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
codchen
force-pushed
the
claude/distr-readonly-reward-query
branch
from
1011463 to
025cce2
Compare
There was a problem hiding this comment.
The refactor to make delegation-reward queries read-only is careful, well-documented, and well-tested, and the read-only computation faithfully mirrors the write path. However, the read-only behavior is gated on upgrade name "v6.7.0", which is not registered in app/tags (which ends at v6.6) and does not match the repo's current tag naming convention (v6.5, v6.6 — no patch component), so in production the fix is inert on the live path and risks trace/live divergence.
Findings: 3 blocking | 4 non-blocking | 1 posted inline
Blockers
- Upgrade gating is inert in production.
readOnlyRewardsUpgrade = "v6.7.0"is never registered inapp/tags(the list ends atv6.6), so no upgrade handler and no "done" marker are ever created for it.IsUpgradeActiveAtHeightdoes an exact store-key lookup on that marker, so the live/non-tracing path never flips to read-only and the EVMrewards()precompile keeps persisting distribution state indefinitely — the exact bug this PR sets out to fix. This confirms Codex's P1. Before merge, the v6.7.0 upgrade tag must be registered and the constant must exactly match the on-chain upgrade name. - Naming-convention mismatch creates a determinism hazard. The recent tags are
v6.5andv6.6(no patch), so the next upgrade will almost certainly be namedv6.7, notv6.7.0. The live path (IsUpgradeActiveAtHeight) matches the upgrade name by exact string, so av6.7marker would never satisfy a"v6.7.0"lookup; meanwhile the tracing path (semver.Compare(ClosestUpgradeName(), "v6.7.0")) treatsv6.7as equal tov6.7.0. Result: for a v6.7 block, live/sync would commit the mutating result while tracing reproduces the read-only result — trace/replay divergence, i.e. the precise determinism problem the gating is meant to prevent. The constant and the registered tag must be reconciled (exact-match on both paths). - 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Confirmed the trace-vs-live dispatch is sound:
WithTraceMode/WithIsTracingboth setisTracing=true, so RPC/trace contexts take the semver branch and live block execution takes theupgradeCheckerbranch — matching the PR's stated design. cursor-review.mdis empty — the Cursor second-opinion pass produced no output.REVIEW_GUIDELINES.mdis empty — no repo-specific review standards were available to apply.- Aside from the upgrade-name gating, the change is high quality: the shared
calculateDelegationRewardscore preserves the withdraw path's lazy final-period store read (byte-for-byte gas), the read-only ratio derivation mirrorsIncrementValidatorPeriodexactly, and the new tests cover the read-only equivalence, idempotency, and all four gating quadrants.
|
|
||
| // readOnlyRewardsUpgrade is the upgrade at which delegation-reward queries stopped | ||
| // persisting a validator-period increment (see DelegationRewardsForQuery). | ||
| const readOnlyRewardsUpgrade = "v6.7.0" |
There was a problem hiding this comment.
[blocker] "v6.7.0" is not registered in app/tags (which ends at v6.6), so IsUpgradeActiveAtHeight(ctx, "v6.7.0", ...) — an exact store-key lookup on the upgrade "done" marker — never returns true on the live path. The read-only behavior therefore never activates in production and the rewards() precompile keeps mutating/committing distribution state (the bug this PR fixes). Additionally, the current tag convention is v6.5/v6.6 (no patch), so the next tag is likely v6.7, which would match the tracing gate (semver.Compare treats v6.7==v6.7.0) but NOT the live exact-match gate — causing trace/live app-hash divergence for that block. Register the upgrade tag and make this constant exactly equal the on-chain upgrade name before merge.
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 025cce2. Configure here.
|
|
||
| // readOnlyRewardsUpgrade is the upgrade at which delegation-reward queries stopped | ||
| // persisting a validator-period increment (see DelegationRewardsForQuery). | ||
| const readOnlyRewardsUpgrade = "v6.7.0" |
There was a problem hiding this comment.
Wrong upgrade name format
High Severity
readOnlyRewardsUpgrade is "v6.7.0", but from v6.5 onward Sei upgrade names use "v6.7" with no .0. IsUpgradeActiveAtHeight looks up the done marker by exact string key, so after a real "v6.7" upgrade the live gate stays false and reward queries keep mutating state.
Additional Locations (2)
Triggered by learned rule: app: upgrade names from v6.5+ use v. format — no .0 suffix
Reviewed by Cursor Bugbot for commit 025cce2. Configure here.
1 participant
Problem
The distribution reward query handlers —
DelegationRewards/DelegationTotalRewardsingrpc_query.go, and the amino equivalents inquerier.go— callIncrementValidatorPeriod, which mutates distribution state (validator period, historical/current rewards, reference counts, and the zero-token community-pool transfer).Queries are supposed to be side-effect free, and the codebase's own intent confirms it:
ctx.CacheContext()"to isolate state changes";But the EVM
rewards()distribution precompile calls the gRPC handler with the live context (sdk.WrapSDKContext(ctx), no branch) and is not behind thereadOnlyguard — so those writes actually persist and commit when a contract callsrewards()in a transaction. That's an unintended state mutation from a view method, and it also defeats the empty-layer read optimization in the companion store-cache fix (the writes dirty thedistributioncache layers).Change
Replace the
IncrementValidatorPeriod+CalculateDelegationRewardssequence in the query paths with a read-only computation:currentRewardsEndingPeriodAndRatiocomputes, in memory, the ending period and cumulative reward ratio thatIncrementValidatorPeriodwould persist — without writing. The ratio is identical to the persisted one, so the reward figure matches the state-changing path exactly.CalculateDelegationRewardsis factored into a shared core that takes the ending ratio.CalculateDelegationRewardsReadOnlypasses the in-memory ratio and never writes.Upgrade gating (v6.7.0)
Because the pre-fix query persisted state on the precompile path, flipping to read-only is state-machine-breaking: re-executing an old block where a contract called
rewards()must still reproduce the committed writes, or the node diverges on app hash and halts. That applies to both historical tracing and non-tracing block-by-block sync — so gating onIsTracing()alone is insufficient.DelegationRewardsForQuerydispatches viarewardQueryIsReadOnly(ctx):IsUpgradeActiveAtHeightreads the upgrade module's "done" marker, which is part of versioned state — so replaying a pre-v6.7.0 block (marker not yet written at that height) reproduces the old mutating behavior, and a post-upgrade block uses read-only. This is the case that would otherwise halt a syncing node.ClosestUpgradeName(set byapp.RPCContextProviderfromGetClosestUpgrade), matching the existingx/evm/keeper/params.goidiom.New behavior activates at the v6.7.0 upgrade height. The reward figure is identical in both paths; only whether the query mutates state differs.
Wiring: a minimal
types.UpgradeCheckerinterface (IsUpgradeActiveAtHeight) plusKeeper.SetUpgradeChecker;app.goinjectsapp.UpgradeKeeperinto the distribution keeper right after it is created and before it is copied into hooks/modules (same pattern asStakingKeeper.SetHooks).readOnlyRewardsUpgrade = "v6.7.0"— adjust if the on-chain upgrade name differs.Tests
TestCalculateDelegationRewardsReadOnlyMatchesWritePath— read-only result equals theIncrementValidatorPeriod+CalculateDelegationRewardsresult (over the slash-event path), and the read-only helper mutates neither the validator period nor the historical reference count.TestDelegationTotalRewardsQueryIsReadOnly— with v6.7.0 active, the gRPC query leaves the validator period/reference count unchanged and is idempotent across repeated calls.TestDelegationRewardsForQueryUpgradeGate— four sub-cases: live pre-v6.7.0 mutates (the sync-halt case), live v6.7.0-active is read-only, tracing pre-v6.7.0 mutates, tracing v6.7.0+ is read-only; the reward figure is identical across all four.-race.Companion to the store-cache depth-amplification fix (#3713): keeping
distributionlayers clean lets that fix's read short-circuit actually hold on the reward path.🤖 Generated with Claude Code