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 pallet #159

Merged
merged 21 commits into from
Jan 9, 2021
Merged

stake pallet #159

merged 21 commits into from
Jan 9, 2021

Conversation

4meta5
Copy link
Contributor

@4meta5 4meta5 commented Jan 5, 2021

What does it do?

A minimal staking pallet. Simplifying assumptions include at most one validator per nominator and every validator self-nominates. There are two main goals for this initial implementation (1) selecting canonical validator set by choosing the N candidates with most stake every round and (2) distributing rewards to block authors (who should be in the set of validators at the time when they produced the block).

What important points reviewers should know?

Slot-based consensus is a pre-requisite for closed collator sets that have dynamic membership (allow rotation). Right now, cumulus consensus has an open validator set. Until slot-based consensus is added, we cannot enforce the set of authorities in consensus from the runtime. Once slot-based consensus is added, pallet-sessions can be used. This will enable safe validator rotation with a closed set of validators.

Until then, we can use this stake pallet. If the cumulus nodes only listen to the selected validators, then we get the functionality we're looking for because only those nodes would be authoring blocks. This is a temporary solution though. For this to work, we need to configure the cumulus node so that it watches the runtime state in stake and only listen to the nodes in the current validator set.

Is there something left for follow-up PRs?

  • integration testing for authority reporting => reward payouts
  • configure the cumulus node so that it watches the runtime state in stake and only listen to the nodes in the current validator set
  • replace constant issuance with substrate's reward curve
  • implement slashing for equivocation and some system for counting unattributable faults like going offline and not producing a block
  • benchmarks and weights

What alternative implementations were considered?

The alternative is to use substrate's pallet-staking, but it is tightly linked with pallet-sessions. This will be supported once slot-oriented consensus is added to cumulus, and we may consider it again then.

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

paritytech/cumulus#36

What value does it bring to the blockchain users?

A simple stake pallet may be useful for enforcing shared risk and reward in the context of blockchain validation or any other game designed to secure some resource.

The high-level goals are

  • enable nominators to share profit and risk with validators
  • choose validator set for each session from candidates

These goals are actually 3 distinct requirements:

  1. choosing validator set
  2. paying rewards
  3. enforcing punishment

The current implementation only does (1) and (2).

Checklist

  • Does it require a purge of the network? No
  • You bumped the runtime version if there are breaking changes in the runtime ?
  • Does it require changes in documentation/tutorials ? Not for now

@JoshOrndorff JoshOrndorff mentioned this pull request Jan 6, 2021
3 tasks
@4meta5 4meta5 marked this pull request as ready for review January 7, 2021 16:11
fn set_author(origin, author: T::AccountId) {
ensure_none(origin)?;
ensure!(<Author<T>>::get().is_none(), Error::<T>::AuthorAlreadySet);
ensure!(T::IsAuthority::is_validator(author.clone()), Error::<T>::NotValidator);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check ensures that the author set in this pallet is always a member of the current validator set. This prevents incorrectly awarding points to non-validators in stake for block production (through EventHandler impl). It ensures that all points awarded for block production only go to current validators.

I considered making T::EventHandler::note_author fallible so points are only awarded if the author is in the validator set recognized in stake, but this would lead to situations in which a non-validator is set as the block author in this pallet (author) and no one is awarded points. I feel this outcome would be more confusing than the current implementation, which prevents setting the author unless the author is in the recognized validator set in stake.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree

…egration tests to recognize RoundIndex is u32
tests/package.json Outdated Show resolved Hide resolved
}

fn on_finalize() {
let author = <Author<T>>::get().expect("author must be set before on_finalize");
Copy link
Contributor Author

@4meta5 4meta5 Jan 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check was placed here so every block must have a block author set before it is finalized (see #162 (comment) ), but our integration tests do not call set_author during every block.

Until I write integration tests for stake, I'm going to remove this panic and allow a block author to not be set so the integration tests don't keep failing because of it.

@4meta5 4meta5 merged commit e72de06 into master Jan 9, 2021
@4meta5 4meta5 deleted the amar-stake branch January 9, 2021 18:53
@4meta5 4meta5 mentioned this pull request Jan 9, 2021
4 tasks
@crystalin
Copy link
Collaborator

🎉

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.

None yet

5 participants