Skip to content

fix(slashing): keep old-key signing info after rotation - #111

Open
julienrbrt wants to merge 2 commits into
mainfrom
julien/fix-slash-consensus
Open

fix(slashing): keep old-key signing info after rotation#111
julienrbrt wants to merge 2 commits into
mainfrom
julien/fix-slash-consensus

Conversation

@julienrbrt

Copy link
Copy Markdown
Collaborator

Description

Closes: #XXXX


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • run make lint and make test
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@julienrbrt
julienrbrt marked this pull request as ready for review August 28, 2026 14:30
@giunatale

giunatale commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Nitpick:

  • The CHANGELOG entry is tagged (x/staking), but every code change in this PR is in x/slashing (hooks.go, signing_info.go); the tag should be (x/slashing).

Couple comments/issues:

  • Shouldn't we eventually prune the old ValidatorSigningInfo, after it matures? Right now it's left indefinitely, and unnecessarily I might add, once the slashing window is elapsed, this entry is useless and just bloats state.
  • double genesis-exported SigningInfo/ValidatorMissedBlocks entries carry byte-identical missed-block data for what is really one physical bitmap. This doesn't break re-import (InitGenesis re-applies the same bits idempotently), but it silently doubles genesis payload size per historical rotation and could double-count downtime if any tooling sums missed blocks across all exported entries.
  • Optional: HandleValidatorSignature (from fix!: cherry-pick 19038 and add tests #108) resolves rotated addresses caller-side via ValidatorByConsAddr with no fallback, while this PR adds callee-side resolution with an old-address fallback (signingInfoAddr) for the jail/tombstone paths. Both land on the same record in every reachable state, but consider either reusing signingInfoAddr in HandleValidatorSignature or adding a short comment noting why the liveness path intentionally keeps strict no-fallback semantics, so the two mechanisms don't drift apart.

Also checked for logic bugs in the new address-resolution helpers (currentConsAddr/signingInfoAddr), consistency with the prior key-rotation fixes (#100, #108), genesis export/import behavior with the retained frozen records, and comment/behavior mismatches — no blocking issues found. The retained-record design and its rationale are clearly documented in the code and covered by the new TestHandleDoubleSign_AfterConsKeyRotation integration test.

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.

2 participants