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

feat: implement chunk validator assignment #9983

Merged
merged 14 commits into from Nov 1, 2023

Conversation

mooori
Copy link
Contributor

@mooori mooori commented Oct 20, 2023

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

  • Introduces EpochInfoV4 which contains the epoch’s ValidatorMandates.
  • Adds EpochInfo::sample_chunk_validators which allows sampling for a given height.
  • Module 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:

  • Properly integrating stateless validator assignment with the rest of the system.
  • Dynamically calculating and updating stake_per_mandate and min_mandates_per_shard.

Hence there are some open TODOs in the diff, they are marked with #10014.

@mooori mooori marked this pull request as ready for review October 20, 2023 23:30
@mooori mooori requested a review from a team as a code owner October 20, 2023 23:30
@mooori mooori requested a review from wacban October 20, 2023 23:30
@wacban
Copy link
Contributor

wacban commented Oct 23, 2023

I'm not too familiar with this topic.
@robin-near, @pugachAG or @Longarithm can you have a look?

@wacban wacban requested review from pugachAG, Longarithm and robin-near and removed request for wacban October 23, 2023 12:45
Copy link
Contributor

@robin-near robin-near left a 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!

core/primitives/src/epoch_manager.rs Outdated Show resolved Hide resolved
core/primitives/src/types.rs Outdated Show resolved Hide resolved
core/primitives/src/validator_mandates.rs Outdated Show resolved Hide resolved
core/primitives/src/validator_mandates.rs Outdated Show resolved Hide resolved
core/primitives/src/types.rs Outdated Show resolved Hide resolved
core/primitives/src/epoch_manager.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@robin-near robin-near left a 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!

@@ -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")]
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in a2415c1.

Comment on lines +659 to +668
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"))]
Copy link
Contributor Author

@mooori mooori Oct 31, 2023

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 ifs and feature checking here.

Copy link
Contributor

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.

Comment on lines +659 to +668
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"))]
Copy link
Contributor

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.

@robin-near robin-near added this pull request to the merge queue Nov 1, 2023
Merged via the queue into near:master with commit 5e80b0b Nov 1, 2023
18 of 19 checks passed
@mooori mooori deleted the chunk-validator-assignment branch November 30, 2023 10:40
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

3 participants