[codex] add evm-only staking precompile#3616
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 @@
## codex/sei-v3-evm-only-scaffold #3616 +/- ##
==================================================================
- Coverage 58.30% 58.18% -0.12%
==================================================================
Files 2179 2190 +11
Lines 177070 178750 +1680
==================================================================
+ Hits 103238 104008 +770
- Misses 64762 65422 +660
- Partials 9070 9320 +250
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 executor now builds a registry-aware precompile map (implemented contracts run; address-only entries still fail closed), adapts them through The staking package implements create/edit validator, delegate/redelegate/undelegate, queries, commission guardrails, JSON-indexed module state, escrow for payable stake (usei-aligned wei), and end-block validator-set / unbonding / redelegation maturity—documented as no rewards, slashing, or jailing. Shared Reviewed by Cursor Bugbot for commit 58207d2. 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}
codchen
force-pushed
the
codex/sei-v3-evm-only-scaffold
branch
2 times, most recently
from
ab82ec3 to
23fc6d3
Compare
| record, ok, err := getUnbondingDelegation(ctx.Store, pair.DelegatorAddress, pair.ValidatorAddress) | ||
| if err != nil || !ok { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Missing unbonding record skips payout
Medium Severity
In completeUnbonding and completeRedelegation, when the store lookup returns ok == false with a nil error, the functions return success instead of failing. The mature-queue loop still deletes queue entries, so a queued unbonding pair without a matching record can be dropped without releasing escrowed stake or cleaning redelegation indexes.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b90eb37. Configure here.
There was a problem hiding this comment.
this is consistent with cosmos behavior
codchen
force-pushed
the
codex/evmonly-staking-precompile
branch
2 times, most recently
from
27e3acc to
550f6fa
Compare
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds the first SDK-free staking custom precompile for the evm-only executor, with byte-keyed storage-backed state, an end-block hook, and a thorough test suite including a full delegate→redelegate→undelegate lifecycle. The implementation closely mirrors Cosmos staking semantics and looks correct; the main open concern is flat gas accounting for state-growing operations. No blocking correctness bugs found.
Findings: 0 blocking | 9 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Gas accounting (Codex finding #1): custom precompile execution charges only the flat RequiredGas (writeGas=20000 + 16/byte). Staking writes JSON-encoded indexes (validators index, per-validator/per-delegator delegation lists, queues) that are re-read, sorted, and re-serialized into chunked storage slots on every mutation — O(n) SSTOREs charged as a flat fee. Block gas therefore does not bound precompile state growth or runtime, allowing cheap unbounded index growth. This mirrors existing Sei precompile flat-gas behavior and the evm-only path is documented as early-integration, so non-blocking — but should be revisited before production (per-operation/per-byte-of-stored-state metering).
- Disagree with Codex finding #2 (editValidator minSelfDelegation vs validator.Tokens): Cosmos x/staking msg_server.EditValidator also checks
MinSelfDelegation.GT(validator.Tokens), not self-delegation. The implementation here is faithful Cosmos parity, not a new invariant break. - Disagree with Codex finding #3 (commission max-change uses signed delta, not abs): Cosmos
Commission.ValidateNewRateuses signednewRate.Sub(c.Rate).GT(MaxChangeRate)and also permits unbounded decreases. The code (and its comment) intentionally matches upstream, so this is parity rather than a bug. - Cursor second-opinion review file (cursor-review.md) is empty — that pass produced no output. REVIEW_GUIDELINES.md is also empty/absent, so no repo-specific standards were applied.
- Documented limitations (no rewards/slashing/jailing, shares track tokens 1:1, historical info recorded in end-block rather than begin-block) are clearly noted in the README; just confirm downstream consumers/indexers tolerate the zero-amount DelegationRewardsWithdrawn events and the deferred historical-info availability.
- 4 suggestion(s)/nit(s) flagged inline on specific lines.
| if isTransaction(method.Name) { | ||
| gas = writeGas | ||
| } | ||
| return gas + inputByteGas*uint64(len(input)) //nolint:gosec // input length is bounded by memory. |
There was a problem hiding this comment.
[suggestion] Gas is flat per call (writeGas + 16/byte of input) regardless of how much module-like state the operation rewrites. delegate/redelegate/undelegate rewrite O(n) JSON index lists (validators index, per-validator/per-delegator delegation lists, time queues) into chunked storage slots, so the real SSTORE/CPU cost can far exceed the charged gas, and block gas no longer bounds state growth. This matches existing Sei precompile flat-gas semantics and the path is experimental, so not blocking — but worth per-operation metering before this backs production state. (Codex finding #1.)
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if minSelfDelegation.Cmp(tokens) > 0 { |
There was a problem hiding this comment.
[nit] Codex flags this as comparing minSelfDelegation against validator.Tokens (which includes third-party delegations). Note this matches Cosmos x/staking msg_server.EditValidator, which also checks msg.MinSelfDelegation.GT(validator.Tokens). So this is faithful parity, not a newly introduced invariant break — no change needed unless intentional divergence from Cosmos is desired here.
| return errCommissionNegative | ||
| case newRate.Cmp(maxRate) > 0: | ||
| return errCommissionGTMaxRate | ||
| case new(big.Rat).Sub(newRate, oldRate).Cmp(maxChange) > 0: |
There was a problem hiding this comment.
[nit] Codex flags that large commission decreases bypass the max-change check because this uses signed newRate - oldRate rather than the absolute delta. This actually matches Cosmos Commission.ValidateNewRate, which likewise uses the signed delta and allows unbounded decreases. Given the stated goal of Cosmos parity (and the function doc), this is correct as written — not a bug.
| } else if err := addPoolNotBonded(ctx.Store, useiAmount); err != nil { | ||
| return nil, err | ||
| } | ||
| p.emit(ctx, "Delegate", ctx.Caller, validatorAddress, util.CloneBig(ctx.ApparentValue)) |
There was a problem hiding this comment.
[nit] Minor unit inconsistency: the Delegate event emits the raw wei ApparentValue (18 decimals), while Undelegate (line 392) and Redelegate (line 334) emit the usei amount arg (6 decimals). Worth confirming this matches the canonical Sei staking precompile event semantics so indexers decode all three consistently.
codchen
force-pushed
the
codex/evmonly-staking-precompile
branch
from
550f6fa to
accacbf
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit accacbf. Configure here.
There was a problem hiding this comment.
Adds a substantial, well-tested SDK-free staking precompile for the evm-only executor. The main blocker is that createValidator accepts an arbitrary/empty consensus pubkey, which can later be emitted as an invalid consensus ValidatorUpdate; there are also a few divergences from Cosmos staking semantics (tie-break ordering, unenforced MaxVotingPowerRatio) and a flat gas model worth addressing.
Findings: 1 blocking | 7 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- MaxVotingPowerRatio / MaxVotingPowerEnforcementThreshold are surfaced in Params but never enforced anywhere in delegate/createValidator/redelegate/endblock, so a validator can accumulate more than the configured maximum voting-power share. Defaults are "0" (disabled) so there's no immediate impact, but if a non-zero ratio is ever loaded it is silently ignored. (Raised by Codex.) Either enforce it or document the gap explicitly in README's limitations list.
- Gas is charged as a flat readGas/writeGas plus per-input-byte, independent of how much state is actually written. Mutating handlers rewrite whole JSON blobs and re-sort/rewrite entire string-list indexes (validators/index, delegator/validator delegation indexes, queues) — O(n) storage slots for a fixed 20000 gas. On a live chain this underprices unbounded state growth and is a potential DoS vector; consider metering by bytes/slots written before this path is production-wired.
- util.EmitEvent silently swallows Pack errors (returns without emitting), so a future event/ABI mismatch would drop logs with no signal. A test or at least a comment documenting the intentional best-effort behavior would help.
- Cursor's second-opinion review (cursor-review.md) produced no output / was empty.
- Codex's review aligned with the findings here: consensus-pubkey validation (reported as the inline blocker), the equal-power tie-break ordering, and the unenforced MaxVotingPowerRatio.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| commissionMaxRate := args[3].(string) | ||
| commissionMaxChangeRate := args[4].(string) | ||
| minSelfDelegation := args[5].(*big.Int) | ||
| pubKey, err := hex.DecodeString(pubKeyHex) |
There was a problem hiding this comment.
[blocker] createValidator hex-decodes pubKeyHex with no length/format validation, so an empty string or any wrong-length byte string is accepted as the consensus pubkey. That key is stored on the Validator and later emitted verbatim as precompiles.ValidatorUpdate{PubKey: ...} from endblock.go, which feeds consensus validator-set updates. The Cosmos path requires a valid cryptotypes.PubKey of a type supported by the consensus params; here an empty or malformed (e.g. non-32-byte ed25519) key produces an invalid validator update. Validate the decoded length against the expected consensus key type before accepting. (Also flagged by Codex.)
| if left != right { | ||
| return left > right | ||
| } | ||
| return validators[i].OperatorAddress < validators[j].OperatorAddress |
There was a problem hiding this comment.
[suggestion] Equal-power ties are broken by OperatorAddress in ascending order. Cosmos's power index orders tied validators by operator address in descending byte order (addresses are stored complemented in the ValidatorsByPower index). When tied candidates straddle the MaxValidators cutoff, this can select a different bonded set than the staking rules being replicated. Ordering is deterministic, but to match Cosmos consider reversing the tie-break (or document the intentional divergence). (Also flagged by Codex.)
| } else if err := addPoolNotBonded(ctx.Store, useiAmount); err != nil { | ||
| return nil, err | ||
| } | ||
| p.emit(ctx, "Delegate", ctx.Caller, validatorAddress, util.CloneBig(ctx.ApparentValue)) |
There was a problem hiding this comment.
[nit] The Delegate event amount is emitted in wei (ctx.ApparentValue), whereas Undelegate/Redelegate emit the usei amount. This unit inconsistency across the events is a footgun for log consumers — confirm it intentionally mirrors the legacy Sei staking precompile, and consider a comment noting it.
codchen
force-pushed
the
codex/evmonly-staking-precompile
branch
from
accacbf to
93a6625
Compare
There was a problem hiding this comment.
Adds a well-structured, extensively tested SDK-free staking precompile for the evm-only executor. No confirmed runtime-breaking bugs in the wired path, but there are a few correctness/fidelity gaps worth addressing (notably missing consensus-pubkey validation before it flows into validator-set updates).
Findings: 0 blocking | 7 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor produced no output (cursor-review.md is empty) and REVIEW_GUIDELINES.md is empty, so this synthesis is based on the diff plus Codex's three findings only.
- Accepted-but-worth-restating limitations (documented in README): jailing/slashing are not modeled, so self-delegation dropping below minSelfDelegation via undelegate/redelegate never jails or unbonds the validator (Cosmos does); delegation shares track tokens 1:1; and DelegationRewardsWithdrawn events are always emitted with a zero amount. Fine as a first pass, but downstream consumers relying on these semantics should be aware.
- Registering any custom precompile disables OCC (useOCC returns false via hasCustomPrecompiles), so every block runs sequentially once staking is registered. This matches prior behavior but is a notable throughput implication now that a real precompile exists — worth a follow-up.
- No negative/adversarial tests around consensus-pubkey validation (empty/short/over-length hex) or the createValidator payable-value edge cases; consider adding them alongside the fix below.
- 3 suggestion(s)/nit(s) flagged inline on specific lines.
| commissionMaxRate := args[3].(string) | ||
| commissionMaxChangeRate := args[4].(string) | ||
| minSelfDelegation := args[5].(*big.Int) | ||
| pubKey, err := hex.DecodeString(pubKeyHex) |
There was a problem hiding this comment.
[suggestion] pubKeyHex is only hex-decoded here with no length/format validation, so an empty or wrong-length key (e.g. the 4-byte "01020304" used in tests) is accepted, stored on the validator, and later emitted verbatim as ValidatorUpdate.PubKey in validatorUpdate (endblock.go). Once these executor updates are consumed by the runtime, a malformed key becomes an invalid consensus update. Cosmos enforces the key type/length (ed25519 = 32 bytes) at create time; consider validating the decoded pubkey length here before persisting. (Matches Codex's medium finding.)
| record.ValidatorSrcAddress = srcValidator | ||
| record.ValidatorDstAddress = dstValidator | ||
| record.Entries = append(record.Entries, RedelegationEntry{ | ||
| CreationHeight: 0, |
There was a problem hiding this comment.
[nit] Redelegation entries are always recorded with CreationHeight: 0, unlike addUnbondingDelegation which correctly threads saturatingInt64FromUint64(creationHeight). The ABI exposes creationHeight via redelegations, so clients get incorrect history. Thread the block height through addRedelegation like the unbonding path does. (Matches Codex's low finding.)
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if minSelfDelegation.Cmp(tokens) > 0 { |
There was a problem hiding this comment.
[nit] minSelfDelegation is validated against validator.Tokens, which includes all delegators' tokens, not just the operator's self-delegation — so an operator can raise the minimum above their actual self-stake if others delegated enough. This mirrors Cosmos's own EditValidator behavior (it also checks validator.Tokens), so it's not a regression, but combined with the absence of jailing (undelegate/redelegate can drop self-stake below the minimum with no consequence) the guardrail is effectively cosmetic here. Worth a comment noting the intentional divergence. (Codex flagged this as high; downgrading since it matches upstream semantics.)
seidroid
Bot
dismissed
their stale review
Superseded: latest AI review found no blocking issues.
codchen
force-pushed
the
codex/evmonly-staking-precompile
branch
from
93a6625 to
8548c28
Compare
There was a problem hiding this comment.
Adds a well-structured, SDK-free staking precompile for the evm-only executor with solid test coverage. No hard blockers, but there are a few fidelity/correctness gaps versus Cosmos staking (consensus-pubkey validation, redelegation completion-time semantics, and a hardcoded redelegation creation height) worth addressing.
Findings: 0 blocking | 6 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- State-mutating precompile methods and O(n) query methods (e.g.
validators,delegatorDelegations, pluscreateValidator/editValidatorwhich callsetHistoricalInfoiterating every validator) are charged only a flatreadGas/writeGasfee regardless of the number of validators/delegations scanned, and the per-blockEndBlockwork is entirely unmetered. This under-prices computation relative to ordinary EVM gas; consider scaling gas with the amount of state touched or documenting the DoS/pricing tradeoff. cursor-review.mdwas empty, so the Cursor second-opinion pass produced no findings to merge; only Codex's review was available.- Consider a focused unit test covering the redelegation-from-unbonded-source and redelegation-creation-height behaviors once the semantics are settled, since the current e2e test only exercises bonded-source redelegation.
- 3 suggestion(s)/nit(s) flagged inline on specific lines.
| commissionMaxRate := args[3].(string) | ||
| commissionMaxChangeRate := args[4].(string) | ||
| minSelfDelegation := args[5].(*big.Int) | ||
| pubKey, err := hex.DecodeString(pubKeyHex) |
There was a problem hiding this comment.
[suggestion] The consensus pubkey is only hex-decoded here — there is no length/validity check. The raw bytes are later emitted verbatim as ValidatorUpdate.PubKey (see validatorUpdate in endblock.go), so a malformed or empty key (e.g. pubKeyHex == "" decodes to a zero-length slice with no error) can enter the validator-set output. Tendermint ed25519 consensus keys must be 32 bytes; validate the length/format before accepting the validator, matching the SDK staking precompile which constructs a proper typed pubkey. (Flagged by Codex.)
| if err := movePoolsForRedelegation(ctx.Store, src.Status, dst.Status, amount); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := addRedelegation(ctx.Store, delegator, srcValidator, dstValidator, amount, util.SaturatingCompletionTime(ctx.Block.Time, params.UnbondingTime)); err != nil { |
There was a problem hiding this comment.
[suggestion] Redelegation completion is unconditionally set to now + UnbondingTime. Cosmos getBeginInfo(srcValidator) instead completes the redelegation immediately when the source validator is unbonded, and reuses the source's existing UnbondingTime/UnbondingHeight when it is unbonding; only a bonded source uses now + UnbondingTime. Because the entry lives until this (over-long) completion time, hasReceivingRedelegation also blocks transitive redelegations longer than intended. Consider branching on the source validator's status. (Flagged by Codex.)
| record.ValidatorSrcAddress = srcValidator | ||
| record.ValidatorDstAddress = dstValidator | ||
| record.Entries = append(record.Entries, RedelegationEntry{ | ||
| CreationHeight: 0, |
There was a problem hiding this comment.
[nit] CreationHeight is hardcoded to 0 for redelegation entries, whereas addUnbondingDelegation threads the real block height through. addRedelegation isn't passed the block number, so redelegations queries will report a creation height of 0. Consider plumbing ctx.Block.Number through here for consistency with the unbonding path. (Flagged by Codex.)
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if err := movePoolsForRedelegation(ctx.Store, src.Status, dst.Status, amount); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := addRedelegation(ctx.Store, delegator, srcValidator, dstValidator, amount, util.SaturatingCompletionTime(ctx.Block.Time, params.UnbondingTime)); err != nil { |
There was a problem hiding this comment.
🟡 Both redelegate() (staking.go:331) and undelegate() (staking.go:381) unconditionally set the completion time to ctx.Block.Time + params.UnbondingTime, regardless of the source validator's Status. Cosmos getBeginInfo picks a different completion time in three cases: Bonded → now + UnbondingTime; Unbonded → complete immediately; Unbonding → inherit the source validator's remaining UnbondingTime. Given this PR's stated goal of Cosmos parity for the semantics it models (unbonding is explicitly modeled), consider branching on src.Status before computing the entry's completion time.
Extended reasoning...
The divergence
Cosmos x/staking getBeginInfo (see sei-cosmos/x/staking/keeper/delegation.go) computes the completion time for a new unbonding/redelegation entry based on the source validator's status:
- Bonded (or not found) →
ctx.BlockHeader().Time.Add(UnbondingTime)— the standard "start unbonding now" case - Unbonded → returns
completeNow=true, so the entry is released immediately with no delay - Unbonding → uses the validator's already-in-flight
validator.UnbondingTime/UnbondingHeight, so the delegator finishes with the validator, not later
This PR's redelegate() and undelegate() both call:
util.SaturatingCompletionTime(ctx.Block.Time, params.UnbondingTime)with no branching on src.Status. Effectively the Bonded case is applied in all three states.
Why this is reachable
The end-block logic in this same PR (applyAndReturnValidatorSetUpdates in endblock.go) actively moves validators through bondStatusUnbonding and bondStatusUnbonded when they drop out of the top-MaxValidators set. So the Unbonded and Unbonding source paths are not dead code — an operator whose validator gets outbid by other stakers will end up in one of those states, and any delegator undelegating/redelegating from them hits the divergent path.
Impact
Funds are still eventually released (the entry is queued with a valid completion time and completeMatureUnbondings / completeMatureRedelegations process it correctly), so this is not a safety issue. The user-visible effect is:
- Unbonded source: delegator waits a full unbonding period unnecessarily. In Cosmos the tokens are already free; here they get locked up again.
- Unbonding source: delegator's completion time can exceed the validator's own
UnbondingTime, so the delegator finishes unbonding after the validator has fully unbonded.
Pool bookkeeping stays consistent because undelegate() only shifts bonded→notBonded when Status==Bonded, and completeUnbonding only debits notBonded, so no invariant is violated. This is purely a delegator-UX / refund-timing divergence.
Step-by-step proof (Unbonded source)
- Assume
params.UnbondingTime = 1_814_400(default, 21 days). - Validator V self-stakes 10 SEI at block 1 (t=100).
V.Status = bondStatusBondedafter end-block. - Many higher-stake validators join; at block 100 (t=X) V drops out of the top-
MaxValidatorsset.applyAndReturnValidatorSetUpdatesmoves V through the queue:bondStatusBonded → bondStatusUnbondingwithV.UnbondingTime = X + 1_814_400. unbondAllMatureValidatorsruns some blocks later at t=Y whereY >= V.UnbondingTime, flipping V tobondStatusUnbonded.- Delegator D now calls
undelegate(V, amount)at t=Z (t=Y or later). Cosmos would setcompletionTime = Z(immediate release, since V is already Unbonded — the unbonding window has already elapsed). This PR setscompletionTime = Z + 1_814_400, forcing D through another full 21-day window. - D's tokens sit in escrow for an extra 21 days that Cosmos would not require.
Fix
Before calling addRedelegation / addUnbondingDelegation, branch on src.Status (already loaded in redelegate; would need to be loaded from validator in undelegate):
bondStatusBonded→util.SaturatingCompletionTime(ctx.Block.Time, params.UnbondingTime)(current behavior)bondStatusUnbonded→ complete immediately (either release synchronously like Cosmos does, or queue with a completion time ofctx.Block.Time)bondStatusUnbonding→ usesrc.UnbondingTime(already on the Validator struct in this codebase)
Downgrading to nit because: (a) this is an experimental/opt-in precompile with no runtime consuming its ValidatorUpdates yet, (b) the current behavior is strictly more conservative than Cosmos (longer wait, not shorter), and (c) tests only exercise Bonded-source paths so nothing regresses. But it's worth fixing before this backs production state, since it's a genuine parity gap in the exact semantics the PR is trying to model.
codchen
force-pushed
the
codex/evmonly-staking-precompile
branch
from
8548c28 to
04870c2
Compare
There was a problem hiding this comment.
Adds the first SDK-free staking custom precompile for the evm-only executor, with a byte-keyed storage-backed store, end-block validator-set/unbonding/redelegation processing, and a good lifecycle e2e test. No hard blockers, but there are a couple of behavioral divergences from Cosmos staking (redelegation-from-unbonded, self-delegation below MinSelfDelegation) worth noting, most of which are already flagged as unmodeled in the README.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion coverage was partial:
cursor-review.mdis empty (Cursor produced no output) andREVIEW_GUIDELINES.mdis empty, so no repo-specific guideline overrides were applied. Codex's pass produced two findings, both incorporated below. - Broad SDK-parity caveat: the precompile does not model slashing, jailing, or rewards, and shares track tokens 1:1. This is documented in the README's limitations, but reviewers/integrators should treat the staking precompile as not yet safe for production validator economics — several security-relevant invariants (min-self-delegation enforcement via jailing, slash-driven share/token divergence) are intentionally absent.
- Tests cover the happy-path lifecycle and guardrails well, but there is no coverage for the two divergences noted inline (redelegation away from an already-unbonded source validator, and self-undelegation below MinSelfDelegation). Consider adding cases once the intended behavior is decided.
DelegationRewardsWithdrawnevents are emitted with a hard-coded zero amount on delegate/redelegate/undelegate; fine as a placeholder but callers relying on this event will always see zero, which could be misleading — worth documenting on the ABI/event side too (already partially noted in the README).- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| if err := movePoolsForRedelegation(ctx.Store, src.Status, dst.Status, amount); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := addRedelegation(ctx.Store, delegator, srcValidator, dstValidator, amount, util.SaturatingCompletionTime(ctx.Block.Time, params.UnbondingTime)); err != nil { |
There was a problem hiding this comment.
[suggestion] Divergence from Cosmos staking: this always schedules the redelegation entry with block.Time + UnbondingTime, regardless of the source validator's bond status. In the SDK, getBeginInfo returns completeNow = true when the source validator is already Unbonded, so the redelegation completes immediately and no in-progress redelegation record is created. Here, redelegating away from an unbonded source leaves a full-unbonding-period redelegation record, which then blocks further redelegation of the destination tokens via the transitive-redelegation check (hasReceivingRedelegation) for the entire period. Consider branching on src.Status == bondStatusUnbonded to complete immediately (matching the SDK) rather than always using SaturatingCompletionTime(...).
| if err := validateDelegationAmount(ctx.Store, delegator, validatorAddress, amount); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := addDelegation(ctx.Store, delegator, validatorAddress, new(big.Int).Neg(amount)); err != nil { |
There was a problem hiding this comment.
[suggestion] Self-undelegation is allowed to drop a validator's self-delegation below its MinSelfDelegation with no consequence. In the SDK, when self-delegation falls below the minimum the validator is jailed (and removed from the active set); here the validator can keep validating with only external stake and zero self-bond. This follows from the README's documented decision not to model jailing, but it removes a real skin-in-the-game invariant. At minimum consider documenting this on the undelegate path, or adding an explicit guard/rejection until jailing is implemented.
| maxValidators = 0 | ||
| } | ||
| if maxValidators > len(candidates) { | ||
| maxValidators = len(candidates) | ||
| } | ||
|
|
||
| updates := make([]precompiles.ValidatorUpdate, 0) | ||
| totalPower := int64(0) | ||
| amtFromBondedToNotBonded := new(big.Int) | ||
| amtFromNotBondedToBonded := new(big.Int) | ||
|
|
||
| for i := 0; i < maxValidators; i++ { | ||
| validator := candidates[i] | ||
| newPower, err := validatorPower(validator) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if newPower == 0 { | ||
| break |
There was a problem hiding this comment.
🟡 The staking precompile exposes MaxVotingPowerRatio and MaxVotingPowerEnforcementThreshold in Params (types.go:109-110) and returns them from the ABI params() method, but applyAndReturnValidatorSetUpdates (endblock.go:37-169) never consults them — validator selection is pure top-MaxValidators by power with no fraction cap, unlike Cosmos's Sei fork which does enforce MaxVotingPowerRatio. Currently benign because the defaults are 0.000000000000000000 (the disabled sentinel) and no writer exists, but the ABI is misleading — either implement the cap or add a note to README.md alongside the existing 'no slashing / no jailing / no rewards' limitations.
Extended reasoning...
The gap
Params declares two fields that shape validator-set updates in Cosmos's Sei fork:
MaxVotingPowerRatio— cap on any single validator's fraction of total voting powerMaxVotingPowerEnforcementThreshold— threshold below which the cap does not kick in
Both are populated with "0.000000000000000000" defaults in loadParams (state.go:241-250) and returned to EVM callers via the ABI-exposed params() method (staking.go:889, abi.json). But grepping the giga/evmonly tree shows they appear only in those three sites — types.go, state.go, and abi.json. No code reads them.
Specifically, applyAndReturnValidatorSetUpdates (endblock.go:37-169) walks validatorsByPower, takes the first MaxValidators entries by (power, operator-address-asc), and emits ValidatorUpdate records with each validator's raw power. There is no post-selection pass that scales down concentrations above MaxVotingPowerRatio or that applies the enforcement threshold.
Why this is inconsistent
Every other field on Params is actually enforced by this PR:
MinCommissionRate— checked invalidateInitialCommissionandvalidateCommissionUpdate(commission.go)HistoricalEntries— used intrackHistoricalInfopruning (state.go)MaxEntries— checked inredelegate/undelegate(staking.go)MaxValidators— bounds the top-N cut inapplyAndReturnValidatorSetUpdatesUnbondingTime— threaded into completion-time math
Only MaxVotingPowerRatio and MaxVotingPowerEnforcementThreshold are dead surface. An EVM caller reading params() reasonably assumes these caps are active, but they are not.
Why it's benign today
The defaults are 0.000000000000000000, which is Cosmos Sei's 'cap disabled' sentinel. There is also no setter method or genesis import that writes these fields today — loadParams always returns the hardcoded defaults. So a caller reading params() sees a disabled cap and correctly observes no cap being enforced. The runtime behavior is currently self-consistent.
Additionally the README already lists other Cosmos-parity gaps (no slashing, no jailing, no rewards), and this PR's ValidatorUpdates have no runtime consumer yet, so nothing hard-fails.
Step-by-step proof
- Create two validators A and B via
createValidatorwith self-stake 90 SEI and 10 SEI respectively. (For the sake of illustration assume params.MaxVotingPowerRatio has been set to"0.500000000000000000"— 50% cap.) - End-block runs:
validatorsByPowerreturns[A (power 90), B (power 10)]. applyAndReturnValidatorSetUpdatesloops through the topMaxValidatorsentries and emitsValidatorUpdate{PubKey: A.PubKey, Power: 90}andValidatorUpdate{PubKey: B.PubKey, Power: 10}.- A gets 90% of consensus power — no cap applied — even though
MaxVotingPowerRatiois set to 50%. Cosmos's Sei fork would scale A down to 50% here.
Because there's no writer today, step 1's "has been set" is not currently reachable — that's why this is a nit rather than a blocker. But the moment someone adds a setter (or genesis import) for these params, callers will observe values that the endblock code silently ignores.
Fix
Either (a) implement the cap in applyAndReturnValidatorSetUpdates — after the top-N selection, scale down any validator whose power exceeds totalPower * MaxVotingPowerRatio when totalPower >= MaxVotingPowerEnforcementThreshold — or (b) drop the two fields from Params (and abi.json) — or (c) add a note to README.md alongside the existing 'does not model slashing / jailing / rewards' list explicitly stating that MaxVotingPowerRatio/MaxVotingPowerEnforcementThreshold are exposed for parity with the Cosmos ABI but not enforced in the evm-only path.
Nit severity because this is an experimental precompile whose ValidatorUpdates aren't consumed yet, the defaults render the enforcement moot today, and other Cosmos-parity gaps are already documented in the README.
| if err := addUnbondingDelegation(ctx.Store, delegator, validatorAddress, amount, ctx.Block.Number, util.SaturatingCompletionTime(ctx.Block.Time, params.UnbondingTime)); err != nil { | ||
| return nil, err | ||
| } | ||
| if validator.Status == bondStatusBonded { | ||
| if err := addPoolBonded(ctx.Store, new(big.Int).Neg(amount)); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := addPoolNotBonded(ctx.Store, amount); err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| p.emit(ctx, "Undelegate", ctx.Caller, validatorAddress, amount) | ||
| p.emit(ctx, "DelegationRewardsWithdrawn", ctx.Caller, validatorAddress, new(big.Int)) | ||
| return method.Outputs.Pack(true) | ||
| } |
There was a problem hiding this comment.
🟡 Orphan Unbonded validators with zero shares are never garbage-collected. A validator created with self-stake < 1M usei never reaches Bonded (validatorsByPower filters power==0), never enters validatorQueueIndexKey (only populated in the Bonded→Unbonding branch of applyAndReturnValidatorSetUpdates), and thus never hits removeValidator (only wired to unbondAllMatureValidators in endblock.go). After the operator undelegates their self-stake, the validator record persists forever in validatorsIndexKey and validatorConsensusPubkeyKey — permanently bricking that EVM operator address and consensus pubkey from ever creating a new validator (createValidator rejects both duplicates). Cosmos x/staking calls RemoveValidator when shares reach 0 and status is Unbonded; consider mirroring that from addValidatorTokens (state.go:130) or undelegate (staking.go:381) when shares hit 0 and Status != Bonded.
Extended reasoning...
The bug
removeValidator (state.go:118) is the only function that deletes a validator record from validatorsIndexKey and validatorConsensusPubkeyKey. It is called from exactly one place: unbondAllMatureValidators at endblock.go:257, which iterates validatorQueueIndexKey.
That queue is populated in exactly one place: insertValidatorQueue inside the noLongerBonded branch of applyAndReturnValidatorSetUpdates (endblock.go), which fires only when a Bonded validator transitions to Unbonding.
A validator that never reaches Bonded therefore never enters the queue and can never be removed — regardless of what happens to its tokens/shares.
The trigger
With powerReduction = 1_000_000, any self-stake < 1M usei yields validatorPower = 0. validatorsByPower filters those out (endblock.go, if power == 0 { continue }), so the validator never becomes a bonding candidate. minSelfDelegation on createValidator is only required to be positive (staking.go:434, ValidatePositiveAmount(minSelfDelegation, ...)), so minSelfDelegation=1 combined with value=1e12 wei (= 1 usei) passes.
The permanent lock
Once the operator calls undelegate and the entry matures, the validator sits in validatorsIndexKey with Tokens=0, Shares=0, Status=Unbonded. createValidator at staking.go:432-441 then rejects any retry:
getValidator(store, validatorAddress)→exists→"validator already exists"getValidatorByConsensusPubkey(store, pubKey)→exists→errDuplicateConsensusKey
That EVM address and pubkey are permanently bricked from validator creation.
Cosmos parity gap
sei-cosmos x/staking/keeper/delegation.go (Unbond) does:
if validator.DelegatorShares.IsZero() && validator.IsUnbonded() {
k.RemoveValidator(ctx, validator.GetOperator())
}This PR's undelegate (staking.go:340-395) has no equivalent zero-shares removal path. Given the PR's explicit goal of Cosmos parity for the semantics it models, this is a genuine divergence.
Step-by-step proof
- Operator
OcallscreateValidator(pubKey, ..., minSelfDelegation=1, value=1e12 wei). AfterstakingValue,selfDelegation = 1 usei. ValidatorVis persisted withTokens=1, Shares=1, Status=bondStatusUnbonded(staking.go:466-482). - End-block runs. In
validatorsByPower(endblock.go),validatorPower(V) = 1/1_000_000 = 0, soVis filtered out.Vnever entersvalidatorQueueIndexKey. - Operator calls
undelegate(V, 1).addDelegationreduces (O,V) delegation to 0 and deletes the delegation record.addValidatorTokens(state.go:130) reducesV.TokensandV.DelegatorSharesto 0 and callssetValidator— no removal path exists here.V.Status != Bonded, so no pool moves.addUnbondingDelegationqueues an entry. - At maturity,
completeUnbondingtransfers 1 usei back to O and deletes the unbonding record.Vis untouched. - Operator retries
createValidatorwith samepubKey.staking.go:432-441—getValidatorreturnsexists=true(V still invalidatorsIndexKey); if the operator regenerated pubkey,getValidatorByConsensusPubkeystill rejects the old one. Both operator address and consensus pubkey are permanently locked.
Fix
The straightforward Cosmos-parity fix is in addValidatorTokens (state.go:130): after updating tokens/shares, if shares.Sign() == 0 && validator.Status != bondStatusBonded, call removeValidator instead of setValidator. Alternatively, add the check in undelegate (staking.go:381) after addValidatorTokens returns. Cosmos does the former.
Why nit
- The evm-only staking precompile is explicitly experimental; the README documents no slashing/rewards/jailing yet, and
ValidatorUpdatesis not yet consumed by the consensus runtime. - No fund safety issue:
completeUnbondingcorrectly returns escrowed usei. - Blast radius is bounded to the operator's own EVM address plus their pubkey — no cross-user impact.
- The fix is small and localized. Worth landing before this backs production state.
codchen
force-pushed
the
codex/evmonly-staking-precompile
branch
from
04870c2 to
83c1b62
Compare
There was a problem hiding this comment.
Adds the first SDK-free staking custom precompile for the evm-only executor, with storage-backed module state and an end-block hook; the implementation is careful and well-tested (including a full delegate→redelegate→undelegate lifecycle e2e). No blocking issues found — only minor fidelity gaps, mostly documented divergences from Cosmos.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Self-undelegate/redelegate paths do not enforce MinSelfDelegation on the operator's own delegation (Codex #2). This diverges from Cosmos, which jails the validator when self-delegation drops below the minimum. The README documents that jailing/slashing are not modeled, so this is an accepted limitation rather than a bug, but it is worth calling out explicitly in the code near undelegate/redelegate so future readers know the omission is intentional.
- Registering any custom precompile forces the whole executor onto the sequential path (useOCC returns false whenever hasCustomPrecompiles() is true), disabling OCC parallelism for all blocks. This appears intentional/by-design given the storage-backed store and end-block hook, but it is a meaningful performance tradeoff worth flagging.
- cursor-review.md is empty (Cursor produced no output) and REVIEW_GUIDELINES.md is empty/missing, so no repo-specific guidelines were applied. No prompt-injection content was found in the PR.
- Reward-related events (DelegationRewardsWithdrawn) are emitted with a zero amount and shares track tokens 1:1 with no slashing divergence; these are documented in the README limitations and are fine for this stage.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| record.ValidatorSrcAddress = srcValidator | ||
| record.ValidatorDstAddress = dstValidator | ||
| record.Entries = append(record.Entries, RedelegationEntry{ | ||
| CreationHeight: 0, |
There was a problem hiding this comment.
[suggestion] Redelegation entries hardcode CreationHeight: 0, unlike addUnbondingDelegation (line 164) which records the actual block height via saturatingInt64FromUint64(creationHeight). Since redelegate also never threads the block number through, redelegations/redelegation query responses will always report a creation height of 0. Consider passing ctx.Block.Number into addRedelegation and storing saturatingInt64FromUint64(...) here for parity with unbonding entries and Cosmos. (Low severity — data fidelity only.)
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if minSelfDelegation.Cmp(tokens) > 0 { |
There was a problem hiding this comment.
[nit] Note (not a bug): this validates the new minSelfDelegation against validator.Tokens, which includes external delegations, not just the operator's self-delegation — so a validator with tiny self-stake but large external delegations can raise its minimum above its real self-delegation. This exactly mirrors Cosmos SDK msgServer.EditValidator (which also compares against validator.Tokens), so it's faithful replication rather than a defect. Flagging for awareness in case stricter semantics are desired for the evm-only path.
Address review findings on the SDK-free staking precompile: - redelegate/undelegate now enforce Cosmos guardrails: reject self-redelegation, transitive redelegation, and over-MaxEntries unbonding/redelegation entries. - delegation shares are reported as an sdk.Dec (scaled by 10^precision) to match the keeper-backed precompile. - create/editValidator validate commission bounds, the 24h rate-change rule, and min-self-delegation increase/cap; stricter decimal parsing rejects fraction/scientific forms. - historicalInfo query is read-only again; historical info is tracked and pruned in the end-block hook. - powerReduction set to 1_000_000 (Sei DefaultPowerReduction); tests use SEI-scale stakes. - validators query no longer re-reads validators; README documents the staking parity limitations. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
codchen
force-pushed
the
codex/evmonly-staking-precompile
branch
from
83c1b62 to
58207d2
Compare
There was a problem hiding this comment.
A large, well-structured PR adding an SDK-free staking precompile to the evm-only executor with thorough lifecycle tests. The main concern is that validator consensus pubkeys are stored and emitted to consensus without any length/type validation, which can produce malformed consensus-critical validator updates.
Findings: 1 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- ValidatorUpdates are lost on the legacy
ResultSinkpath (Codex #1):sinkBlockResultin executor.go only persists ChangeSet+Receipts for sinks that are not aBlockResultSink, so validator-set output (now populated onBlockResult) is silently dropped. No productionResultSink-only implementer exists yet, so this is a latent design gap rather than an active bug — but since these updates are consensus-critical, consider either extendingResultSinkto carry them or having the executor refuse to register custom precompiles (which emit validator updates) when only a plainResultSinkis configured. - Missing negative test coverage for invalid consensus pubkeys:
guardrails_test.gocovers duplicate keys but not empty/wrong-length keys. Add a test assertingcreateValidatorrejects an empty and a non-32-byte pubkey once validation is added. - Second-opinion passes:
cursor-review.mdis empty (Cursor produced no output) andREVIEW_GUIDELINES.mdis empty, so no repo-specific guidelines were applied. Codex's two findings are both incorporated above and inline. - Minor:
RequiredGasfully unpacks the input viaprepareandRununpacks again, so every state-changing call ABI-decodes its arguments twice — acceptable, but worth noting for a hot path.
| commissionMaxRate := args[3].(string) | ||
| commissionMaxChangeRate := args[4].(string) | ||
| minSelfDelegation := args[5].(*big.Int) | ||
| pubKey, err := hex.DecodeString(pubKeyHex) |
There was a problem hiding this comment.
[blocker] The consensus pubkey is only validated for hex decodability — there is no length/type check (Codex #2). hex.DecodeString("") returns an empty slice with no error, so a validator can be created with an empty (or arbitrary-length) ConsensusPubkey, which is then persisted and later emitted verbatim as ValidatorUpdate.PubKey in endblock.go:222. Cosmos/Tendermint require a valid 32-byte ed25519 consensus key; emitting an empty or malformed key into the validator-set update is consensus-critical and can corrupt or halt the set. Validate the expected key type and length (e.g. reject empty and enforce the 32-byte ed25519 length) before storing the validator.
1 participant
Summary
Adds the first SDK-free custom precompile for the evm-only executor: staking at
0x0000000000000000000000000000000000001005.This PR wires custom precompile execution into the evm-only executor, stores precompile module-like state as storage owned by the precompile address, and adds an end-block hook for staking validator-set updates and delayed redelegation/undelegation completion. It also keeps staking token handling explicitly usei-only for the evm-only path.
Details
giga/evmonly/precompiles/util.Validation
go test ./giga/evmonly/...