Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

staking: emit event when tombstoning active/jailed/inactive vals #4277

Merged
merged 1 commit into from
Apr 28, 2024

Conversation

erwanor
Copy link
Member

@erwanor erwanor commented Apr 26, 2024

Describe your changes

This PR adds a EventTombstoneValidator message that is emitted on the first validated evidence message for validators that are Active | Inactive | Jailed.

The evidence event is defined as follow:

message EventTombstoneValidator {
  // The height at which the offense occurred.
  uint64 evidence_height = 1;
  // The height at which the evidence was processed.
  uint64 current_height = 2;
  // The validator identity key.
  keys.v1.IdentityKey identity_key = 4;
  // The validator's Comet address.
  bytes address = 5;
  // The voting power for the validator.
  uint64 voting_power = 6;
}

My first instinct was to reach for storing the evidence in nonverifiable storage, hoping it would make it easier for node runners to query it. But I later realized, that comet only gives us a filtered version that does not include the actual double signed headers, or offending signatures.

Checklist before requesting a review

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

    Event emission

@erwanor erwanor added A-staking Area: Design and implementation of staking and delegation protobuf-changes Makes changes to the protobuf definitions. labels Apr 26, 2024
@erwanor erwanor self-assigned this Apr 26, 2024
@erwanor
Copy link
Member Author

erwanor commented Apr 26, 2024

We'll need to port and complete the event coverage for the staking component as part of #3504 and #3797

@conorsch conorsch self-requested a review April 26, 2024 15:57
@conorsch
Copy link
Contributor

This looks great, @erwanor, and provides us with a nice shape for adding the other state transitions, as described in #3746 (comment). The only thing I'm unclear on is why the emission is gated on transitions out of Active | Inactive | Jailed. The state machine diagram makes it clear that Defined and Disabled validators can also be Tombstoned—why not also emit the event in those scenarios?

@erwanor
Copy link
Member Author

erwanor commented Apr 26, 2024

Good point, my reasoning is that those transitions are less relevant because they don't affect the consensus view at all. Something else that I had in mind is that generating valid equivocation evidence rounds down to "free", hence why we only record the first offense, and tie it to 1/ a validator definition upload 2/ having reached the minimum stake threshold 3/ being enabled.

@conorsch
Copy link
Contributor

Thanks, that's clearly explained!

@TalDerei
Copy link
Collaborator

what do tombstoning and jailed mean here?

@erwanor
Copy link
Member Author

erwanor commented Apr 26, 2024

There's a good overview in the spec: https://protocol.penumbra.zone/main/concepts/validators.html, happy to chat about it offline as well!

@erwanor erwanor merged commit dc8eff9 into main Apr 28, 2024
8 checks passed
@erwanor erwanor deleted the erwan/store_evidence branch April 28, 2024 22:32
@cratelyn cratelyn added this to the Sprint 5 milestone May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-staking Area: Design and implementation of staking and delegation protobuf-changes Makes changes to the protobuf definitions.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants