-
Notifications
You must be signed in to change notification settings - Fork 335
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
stake pallet #159
Conversation
…ccountId from inherent
…thod, and revert previous unnecessary changes
pallets/author/src/lib.rs
Outdated
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); |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree
…t param for IsValidator to ref
…t bc it didnt do anything
…egration tests to recognize RoundIndex is u32
pallets/author/src/lib.rs
Outdated
} | ||
|
||
fn on_finalize() { | ||
let author = <Author<T>>::get().expect("author must be set before on_finalize"); |
There was a problem hiding this comment.
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.
🎉 |
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 instake
and only listen to the nodes in the current validator set.Is there something left for follow-up PRs?
stake
and only listen to the nodes in the current validator setWhat alternative implementations were considered?
The alternative is to use substrate's
pallet-staking
, but it is tightly linked withpallet-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
These goals are actually 3 distinct requirements:
The current implementation only does (1) and (2).
Checklist