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

stake: index consensus set based on minimum stake requirement #3609

Merged
merged 23 commits into from
Jan 18, 2024

Conversation

erwanor
Copy link
Member

@erwanor erwanor commented Jan 13, 2024

This is a work-in-progress, here is a high-level overview of what this PR does:

  • Add a min_validator_stake stake parameters specifying the minimum amount of stake required for a validator to be Inactive (default is 100penumbra).
  • Add a Defined state for validator that is the initial state for all post-genesis validators
  • Keep a single source-of-truth for validator definitions in the JMT.
  • Create a consensus_set nonverifiable index that tracks the identity keys of validators that have reached a minimum stake threshold
  • Remove usage of the validator_list_* methods that returns (and collect) every validator definition.1

This is the current validator state machine diagram:

///                         ┌──────────────────────────────────────┐     
///         ┌────────────┐  │  ┌─────────────────────┐             │     
///         │            │  ▼  ▼                     │             │     
///         ▼          ┌─┴──────┐               ┌────────┐         │     
///    ┌─────────┐     │        ├──────────────▶│        │    ┌────────┐ 
///    │ Defined │────▶│Inactive│    ┌──────────│ Active │───▶│ Jailed ││
///    └─────────┘     │        │    │          │        │    └────────┘│
///         │  ▲       └────────┘    │          └────────┘         │    │
///         │  │            ▲        │               │             │    │
///         │  │            │        │               │             │    │
///         │  │            │        │               │             │    │
///         │  │            │        │               │             │    │
///         │  │            │        ▼               │             │    │
///         │  │            │  ┌──────────┐          ▼             │    │
///         │  │            └──┤          │    ┌──────────┐        │    │
///         │  │               │ Disabled │    │Tombstoned│◀───────┘    │
///         │  └───────────────┤          │    └──────────┘             │
///         │                  └──────────┘                             │
///         │                     ▲     ▲                               │
///         └─────────────────────┘     └───────────────────────────────┘

Footnotes

  1. The idea here is to keep a state key that stores all validator definitions, but abrogate direct iteration on that store (because it could be really large).

@erwanor erwanor force-pushed the erwan/2921_min_threshold_stake branch from a2da79a to 7cc75c8 Compare January 13, 2024 07:47
@erwanor erwanor requested a review from hdevalence January 13, 2024 08:04
@hdevalence
Copy link
Member

hdevalence commented Jan 14, 2024

I haven't looked at the changes in detail yet, but I wanted to make one initial comment:

Remove usage of the validator_list_* methods that returns (and collect) every validator definition.

This is a non-goal of the change, the point of introducing the Defined state is that we don't want to make extensive changes to the staking component state machine we already have, we just want to gate access to it. That's why Defined is a new "precursor" state.

Similarly, we don't want to create new indexes for validators above the minimum threshold. We want to use the indexes we already have.

@erwanor
Copy link
Member Author

erwanor commented Jan 15, 2024

Thanks for giving it a look, let me try to workshop an alternative.

Currently, we have:

  • A state_key::validator that maps identity keys to validator definitions
  • StateReadExt::validator_list which is a prefix query that collects all the validator definitions
  • StateReadExt::validator_identity_list which is a prefix_keys query that collect all the validator identity keys

I don't think we can escape adding another index because even if we gate access to state_key::validator for post-Defined validators, we still have to track Defined validator definitions some place else. If that's alright, then we could:

  • Add a state_key::defined_validator that tracks defined validators.
  • Gate access to state_key::validator on being post-Defined
  • Keep the validator_list and validator_list_identity methods, perhaps turning them into streams.

The entry point for transitions in-out of the precursor Defined state:

  • Executing a Delegate action
  • Processing undelegations

@erwanor
Copy link
Member Author

erwanor commented Jan 16, 2024

We discussed this and will go with the original approach because the alternative approach outlined in the previous comment is more complicated. To understand why, walk through the case where a validator is Defined and we process a delegation that triggers a state transition to Inactive. We need to pull the validator's definition from storage. If we store the Defined validators separately this actually creates more I/O than promoting the IdentityKey of the validator to an entry in a consensus_set index, because we have to remove the definition from that store and add it to the list of validators.

Another thing, is that we don't actually have a validator index. We have a state_key::validator_list that records validator definitions, and perform prefix_key queries over it to iterate over the validator set.

@erwanor erwanor marked this pull request as ready for review January 17, 2024 17:11
@erwanor
Copy link
Member Author

erwanor commented Jan 17, 2024

There are some "architectural" TODOs that I plan to tackle as part of #3615.

@erwanor erwanor merged commit eccf899 into main Jan 18, 2024
6 of 7 checks passed
@erwanor erwanor deleted the erwan/2921_min_threshold_stake branch January 18, 2024 02:26
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