Skip to content

feat(staking): add basic key rotation#26440

Merged
mattac21 merged 21 commits into
mainfrom
ma/key-rotation
Jun 1, 2026
Merged

feat(staking): add basic key rotation#26440
mattac21 merged 21 commits into
mainfrom
ma/key-rotation

Conversation

@mattac21

@mattac21 mattac21 commented May 21, 2026

Copy link
Copy Markdown
Contributor

Description

Adds the ability for validators to rotate their consensus keys. This is currently just the happy path implementation and is explicitly not feature complete. This is missing major features that will be implemented as follow up such as:

  1. Slashing/evidence handling for infractions or downtime on the old keys.
  2. CLI integration.
  3. System tests (relies on CLI integration).
  4. Configurable key rotation fee as part of staking modules params (hardcoded as 1000000 of DefaultBondDenom currently).
  5. Configurable max rotations within unbonding period (hardcoded at 1 currently).
  6. x/poa key rotations
  7. Maintaining key rotation state when importing and exporting genesis.

The design implemented here and in future PR's is following ADR-16. Reading the ADR would be a very helpful starting point to review this PR. There was a previous implementation of this here. This PR and future PR's are also largely using this as a reference point.

The main files to review here are:

  • x/staking/keeper/msg_server.go - The RotateConsPubKey method does basic validation to ensure that the validator is allowed to do a rotation, and then schedules the rotation to happen during the EndBlocker (since the rotation is really a set of ABCI ValidatorUpdate's, it must happen during the end blocker and not during the msg handling).
  • x/staking/types/keys.go - There are four new keys implemented to facilitate key rotation.
    • UnappliedConsKeyRotationKey - A queue of rotations that needs to happen during the next EndBlocker
    • RotatedConsAddrIndexKey - Lookup of old keys to new keys. Pruned during when the rotation of old -> new happened longer than 1 unbending period ago.
    • ValidatorConsKeyRotationKey - Lookup of validators that have rotated during within 1 unbonding period ago.
    • ConsKeyRotationQueueKey - Queue of rotations that have happened within this unbending period. Used to prune RotatedConsAddrIndexKey and ValidatorConsKeyRotationKey when the rotation is no longer relevant.
  • x/staking/keeper/val_state_change.go - EndBlocker implementation. Calls to do the actual key rotation in state, creating the ValidatorUpdate's, cleaning up the stores.
  • x/staking/keeper/rotation.go - Keeper helpers related to key rotation.

Closes: STACK-2805

@codecov

codecov Bot commented May 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.33333% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.86%. Comparing base (ebb120a) to head (de8f4c6).

Files with missing lines Patch % Lines
x/staking/keeper/rotation.go 81.66% 22 Missing ⚠️
x/staking/keeper/msg_server.go 79.48% 8 Missing ⚠️
x/staking/keeper/val_state_change.go 66.66% 2 Missing ⚠️
x/staking/keeper/genesis.go 50.00% 1 Missing ⚠️
x/staking/keeper/keeper.go 50.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #26440      +/-   ##
==========================================
+ Coverage   64.82%   64.86%   +0.03%     
==========================================
  Files         810      811       +1     
  Lines       56157    56357     +200     
==========================================
+ Hits        36406    36555     +149     
- Misses      19751    19802      +51     
Files with missing lines Coverage Δ
simapp/app.go 91.97% <ø> (ø)
testutil/configurator/configurator.go 85.14% <100.00%> (+0.07%) ⬆️
testutil/integration/router.go 90.00% <100.00%> (ø)
x/staking/keeper/pool.go 83.63% <100.00%> (+0.61%) ⬆️
x/staking/types/expected_keepers.go 0.00% <ø> (ø)
x/staking/types/keys.go 84.46% <100.00%> (+2.44%) ⬆️
x/staking/types/params.go 63.82% <ø> (ø)
x/staking/types/pool.go 100.00% <ø> (ø)
x/staking/keeper/genesis.go 67.12% <50.00%> (-0.24%) ⬇️
x/staking/keeper/keeper.go 70.42% <50.00%> (-0.60%) ⬇️
... and 3 more

... and 3 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@linear-code

linear-code Bot commented May 21, 2026

Copy link
Copy Markdown

STACK-2805

@mattac21 mattac21 marked this pull request as ready for review May 21, 2026 20:46
@mattac21 mattac21 requested a review from a team as a code owner May 21, 2026 20:46
@greptile-apps

greptile-apps Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR introduces the foundational "happy path" consensus key rotation for validators, as specified in ADR-16. The msg server queues rotations into four new KV-store indexes; the EndBlocker applies them as paired ABCI ValidatorUpdate pairs (zero-power for the old key, live-power for the new key) and prunes entries after their unbonding window elapses.

  • Msg handler (RotateConsPubKey): validates in the correct order (rotation-lock check, live-key uniqueness, validator existence, jailed guard, per-window cap), charges and burns the rotation fee through a dedicated key_rotation_fee_pool burner module account, then queues the rotation via SetConsKeyRotation.
  • Store design: four prefixes (0x91–0x94) covering the maturity queue, per-validator window flag, rotation-locked address index (guards both old key post-rotation and new key while pending), and the unapplied drain queue; key parsing/encoding appears correct.
  • EndBlocker integration: ApplyPendingConsKeyRotations is called inside ApplyAndReturnValidatorSetUpdates (after normal bonding transitions), followed by PruneMaturedConsKeyRotations in BlockValidatorUpdates; the previous iterator-mutation bug is fixed by materialising entries into a slice before the callback.

Confidence Score: 4/5

Safe to merge as an explicitly incomplete foundation; no blocking bugs found in the happy-path flow.

The core state machine — queuing, applying, and pruning rotations — is logically correct. Previous thread concerns (iterator mutation, same-block collision, fee routing, jailed guard) have all been addressed. The two remaining findings are non-blocking quality issues: an exported constant that is never read by the enforcement path, and a dead fee parameter in SetConsKeyRotation that is silently ignored. Neither affects correctness of the current implementation.

x/staking/keeper/rotation.go — the MaxConsKeyRotations constant and the unused fee parameter in SetConsKeyRotation are the two items worth a second look before this code is extended toward the configurable-limit and historical-recording follow-ups.

Important Files Changed

Filename Overview
x/staking/keeper/msg_server.go Adds RotateConsPubKey msg handler with correct validation ordering: rotation lock check, live-key check, validator existence, jailed guard, unbonding-window cap, fee charge, and store write. Previous thread concerns (jailed guard, fee routing, same-block collision) are addressed.
x/staking/keeper/rotation.go New file implementing all rotation keeper helpers. Iterator-mutation bug from previous review is fixed (entries are materialized into a slice before the callback). Two P2 issues: MaxConsKeyRotations constant is exported but never used in enforcement, and SetConsKeyRotation's fee parameter is dead code.
x/staking/keeper/val_state_change.go Adds ApplyPendingConsKeyRotations call inside ApplyAndReturnValidatorSetUpdates and PruneMaturedConsKeyRotations call inside BlockValidatorUpdates; ordering is correct (apply then prune).
x/staking/types/keys.go Adds four new store key prefixes (0x91–0x94) with correct key builders and a ParseConsKeyRotationQueueKey decoder. Length-prefix encoding and time-prefix range bounds appear correct.
tests/integration/staking/keeper/cons_key_rotation_test.go New integration tests cover the happy path (queue + end-blocker apply), pruning after unbonding window, and the per-window rotation cap lifting after pruning. Good block-level coverage using real ABCI FinalizeBlock/Commit flow.
x/staking/keeper/rotation_test.go Unit tests for ApplyConsKeyRotation, IterateUnappliedConsKeyRotations, and PruneMaturedConsKeyRotations; edge cases (empty store, error propagation, mixed matured/future entries) are covered.
x/staking/types/pool.go Adds KeyRotationFeePoolName constant; dedicated burner module account keeps rotation fees isolated from bonded/not-bonded staking pools.
simapp/app.go Registers KeyRotationFeePoolName with Burner permission in maccPerms; mirrored in enterprise simapp and POA migrate-from-pos app.

Sequence Diagram

sequenceDiagram
    participant V as Validator (TX)
    participant MS as MsgServer
    participant Bank as BankKeeper
    participant Store as KV Store
    participant EB as EndBlocker
    participant Comet as CometBFT

    V->>MS: RotateConsPubKey(valAddr, newPk)
    MS->>Store: IsConsAddrLockedByRotation(newConsAddr)?
    Store-->>MS: false
    MS->>Store: GetValidatorByConsAddr(newConsAddr)?
    Store-->>MS: not found
    MS->>Store: GetValidator(valAddr)
    Store-->>MS: validator
    MS->>MS: IsJailed? HasRotationInWindow?
    MS->>Bank: "SendCoinsFromAccountToModule(valAddr -> KeyRotationFeePool)"
    Bank-->>MS: ok
    MS->>Bank: BurnCoins(KeyRotationFeePool)
    Bank-->>MS: ok
    MS->>Store: SetConsKeyRotation(valAddr, oldPk, newPk)
    Store-->>MS: ok
    MS-->>V: MsgRotateConsPubKeyResponse

    Note over EB: Same block - EndBlocker
    EB->>Store: unappliedConsKeyRotations() snapshot
    Store-->>EB: [(valAddr, newPk)]
    EB->>Store: ApplyConsKeyRotation: delete old index, set new index
    EB->>Store: delete LockedIndex(newConsAddr), delete UnappliedQueue(valAddr)
    EB->>Comet: "ValidatorUpdate{oldKey, power=0}"
    EB->>Comet: "ValidatorUpdate{newKey, power=current}"

    Note over EB: Block at maturity (unbondingTime later)
    EB->>Store: PruneMaturedConsKeyRotations()
    Store-->>EB: deletes MaturityQueue, ValidatorWindow, LockedIndex(oldConsAddr)
Loading

Reviews (7): Last reviewed commit: "dont delete during iteration" | Re-trigger Greptile

Comment thread x/staking/keeper/msg_server.go Outdated
Comment thread x/staking/keeper/msg_server.go Outdated
Comment thread x/staking/keeper/msg_server.go Outdated
Comment thread x/staking/keeper/msg_server.go
Comment thread x/staking/keeper/rotation.go Outdated
mattac21 and others added 3 commits May 21, 2026 17:10
@mattac21

Copy link
Copy Markdown
Contributor Author

@greptile re review

Comment thread x/staking/keeper/msg_server.go
@mattac21

Copy link
Copy Markdown
Contributor Author

@greptile review again, that test you are referencing has already been fixed

Comment thread x/staking/keeper/rotation.go Outdated
@mattac21

Copy link
Copy Markdown
Contributor Author

@greptile review again

Comment thread x/staking/keeper/genesis.go
Comment thread x/staking/keeper/msg_server.go Outdated
@mattac21

Copy link
Copy Markdown
Contributor Author

@greptile review again. review specifically looking for other issues similar to the previous issue you found Same-block rotation collision corrupts validator state

Comment thread x/staking/keeper/rotation.go
@mattac21

Copy link
Copy Markdown
Contributor Author

@greptile review again

@aljo242

aljo242 commented May 26, 2026

Copy link
Copy Markdown
Contributor

approving. a few things to track for follow-up:

genesis import/export missing, rotation state won't survive an export/import cycle.

fee + max-rotations hardcoded, may be fine depending on whether these need to be governable.

possible block-stm gap: lock check in msg_server reads RotationLockedConsAddrIndexKey before SetConsKeyRotation writes it. under parallel execution two txns targeting the same new key in the same block could both pass validation. worth verifying.

Comment thread x/staking/keeper/msg_server.go
@aljo242

aljo242 commented May 27, 2026

Copy link
Copy Markdown
Contributor

doesn't call IsConsAddrLockedByRotation before accepting a consensus key — RotateConsPubKey does but CreateValidator doesn't. once evidence routing for old keys is wired up, could a new validator register with a recently-rotated-away key and intercept that routing? byConsAddr[oldConsAddr] would point at the new validator, so evidence for the original double-signer routes to them instead, either panicking (no signing info) or tombstoning the wrong validator.

@mattac21

Copy link
Copy Markdown
Contributor Author

doesn't call IsConsAddrLockedByRotation before accepting a consensus key — RotateConsPubKey does but CreateValidator doesn't. once evidence routing for old keys is wired up, could a new validator register with a recently-rotated-away key and intercept that routing? byConsAddr[oldConsAddr] would point at the new validator, so evidence for the original double-signer routes to them instead, either panicking (no signing info) or tombstoning the wrong validator.

@aljo242 good callout here, im making a ticket to track this as a follow up (dont allow CreateValidator on locked consensus keys)

@mattac21 mattac21 added this pull request to the merge queue Jun 1, 2026
Merged via the queue into main with commit 9b89052 Jun 1, 2026
43 checks passed
@mattac21 mattac21 deleted the ma/key-rotation branch June 1, 2026 17:50
@mattac21 mattac21 restored the ma/key-rotation branch June 1, 2026 18:07
@mattac21 mattac21 deleted the ma/key-rotation branch June 1, 2026 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants