-
Notifications
You must be signed in to change notification settings - Fork 618
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
feat: implement chunk validator assignment #9983
Conversation
b243fb4
to
f32715b
Compare
f32715b
to
20d225f
Compare
I'm not too familiar with this topic. |
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.
Looks really good! Thanks!
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.
Looks great - just that one change left and we should be good to go! Thanks!
core/primitives/src/epoch_manager.rs
Outdated
@@ -615,6 +656,28 @@ pub mod epoch_info { | |||
let block_producers_sampler = stake_weights(&block_producers_settlement); | |||
let chunk_producers_sampler = | |||
chunk_producers_settlement.iter().map(|vs| stake_weights(vs)).collect(); | |||
#[cfg(feature = "protocol_feature_chunk_validation")] |
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.
We should use checked_feature!("protocol_feature_chunk_validation", ChunkValidation, protocol_version) as the condition. This checks not only the feature flag, but that the protocol version is new enough.
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.
Addressed in a2415c1.
if checked_feature!( | ||
"protocol_feature_chunk_validation", | ||
ChunkValidation, | ||
protocol_version | ||
) { | ||
// This block is entered only if feature `protocol_feature_chunk_validation` is | ||
// enabled. In that case `validator_mandates` is a parameter of the function and | ||
// the variable shadowing below is not included. Still, the following | ||
// declaration of `validator_mandates` is needed to satisfy the compiler. | ||
#[cfg(not(feature = "protocol_feature_chunk_validation"))] |
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.
Assuming the path returning EpochInfo::V2
is still needed, I couldn't see a better way than nesting if
s and feature checking here.
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.
Yeah this is expected. And yes you're right, all prior code paths are still needed.
if checked_feature!( | ||
"protocol_feature_chunk_validation", | ||
ChunkValidation, | ||
protocol_version | ||
) { | ||
// This block is entered only if feature `protocol_feature_chunk_validation` is | ||
// enabled. In that case `validator_mandates` is a parameter of the function and | ||
// the variable shadowing below is not included. Still, the following | ||
// declaration of `validator_mandates` is needed to satisfy the compiler. | ||
#[cfg(not(feature = "protocol_feature_chunk_validation"))] |
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.
Yeah this is expected. And yes you're right, all prior code paths are still needed.
Implements chunk validator assignment as described in
The term seat
In
nearcore
it is already used:seat_price
is the minimum stake required to become a validator.NumSeats
is the number of seats available for block producers.This might lead to conflicts and confusion with the concept of seat to be introduced for chunk validator assignment. To avoid that, this PR uses the term mandate for now. Accordingly the stake of chunk validators is splitt into mandates which are randomly shuffled and assigned to shards.
I’m happy to rename mandate to anything else, though if possible I would try to avoid conflicts with the existing usage of seat.
Overview
EpochInfoV4
which contains the epoch’sValidatorMandates
.EpochInfo::sample_chunk_validators
which allows sampling for a given height.core::primitives::validator_mandates
encapsulates splitting validator stake into mandates, shuffling them, and assigning them to shards.Follow-up concerns
As discussed offline (ref), the following are separate concerns:
stake_per_mandate
andmin_mandates_per_shard
.Hence there are some open
TODO
s in the diff, they are marked with #10014.