feat: consider all proposals for chunk validators #11252

Longarithm · 2024-05-07T20:35:37Z

Solve #11202 and continue simplifying proposals processing logic.

I want to make epoch info generation as straightforward as possible, by moving code for old protocol versions to local submodule so it won't be distracting. Now it should be more clear that epoch info generation consists of couple independent steps.

Couple notes:

FixStakingThreshold isn't stabilised so we can use whatever protocol version. Version of epoch being generated makes much more sense.
I expected chunk validators set to be the biggest, but there is subtle case when we can't select chunk validators due to small stake ratio but can select chunk producers instead. It adds a bit of complexity.
There is subtle change in validator indexing and I believe the new indexing - basically, sorting by descending stake in majority of cases - makes more sense.

wacban · 2024-05-08T06:37:27Z

hey @Longarithm this PR is still in draft but I see you requested reviews, just to make sure, is it ready for review?

tayfunelmas · 2024-05-08T15:41:00Z

core/primitives/src/epoch_manager.rs

+    pub num_chunk_producer_seats: NumSeats,
+    #[default(300)]
+    pub num_chunk_validator_seats: NumSeats,
+    // Deprecated, should be set to zero for future protocol versions.


Can we make this a TODO (instead of saying deprecated, say deprecate this and set to zero ...) and also indicate after which protocol versions this can change (in case someone considers doing it later)?

chain/epoch-manager/src/shard_assignment.rs

tayfunelmas · 2024-05-08T19:19:24Z

chain/epoch-manager/src/validator_selection.rs

+            // is not enough to iterate over chunk validators.
+            // So unfortunately we have to look over all roles to get unselected
+            // proposals.
+            // TODO: must be simplified.


can you expand this TODO to indicate simplify in which direction?

codecov · 2024-05-08T20:38:30Z

Codecov Report

Attention: Patch coverage is 95.70552% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 71.03%. Comparing base (ff2385d) to head (1cb511a).
Report is 1 commits behind head on master.

Files	Patch %	Lines
core/chain-configs/src/test_genesis.rs	40.00%	6 Missing ⚠️
core/primitives/src/epoch_manager.rs	71.42%	4 Missing ⚠️
chain/epoch-manager/src/validator_selection.rs	99.29%	2 Missing ⚠️
tools/fork-network/src/cli.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11252      +/-   ##
==========================================
+ Coverage   71.01%   71.03%   +0.02%     
==========================================
  Files         782      782              
  Lines      156476   156618     +142     
  Branches   156476   156618     +142     
==========================================
+ Hits       111123   111259     +136     
- Misses      40525    40530       +5     
- Partials     4828     4829       +1

Flag	Coverage Δ
backward-compatibility	`0.24% <0.00%> (-0.01%)`	⬇️
db-migration	`0.24% <0.00%> (-0.01%)`	⬇️
genesis-check	`1.39% <0.00%> (-0.01%)`	⬇️
integration-tests	`37.20% <73.92%> (+0.04%)`	⬆️
linux	`68.93% <61.96%> (-0.15%)`	⬇️
linux-nightly	`70.50% <91.10%> (+0.03%)`	⬆️
macos	`52.34% <52.45%> (-0.19%)`	⬇️
pytests	`1.62% <0.00%> (-0.01%)`	⬇️
sanity-checks	`1.41% <0.00%> (-0.01%)`	⬇️
unittests	`65.46% <87.42%> (+<0.01%)`	⬆️
upgradability	`0.29% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

chain/epoch-manager/src/shard_assignment.rs

wacban · 2024-05-09T07:08:26Z

chain/epoch-manager/src/validator_selection.rs

+    // Returns unselected proposals, validator lists for all roles and stake
+    // threshold to become a validator.
+    let (unselected_proposals, chunk_producers, block_producers, chunk_validators, threshold) =
+        if checked_feature!("stable", StatelessValidationV0, next_version) {


nit: Can you refactor the body of this if to a helper method? Perhaps something like new_validator_selection::select_validators_from_proposals for symmetry.

wacban · 2024-05-09T07:12:10Z

chain/epoch-manager/src/validator_selection.rs

+            let mut block_producer_proposals = order_proposals(proposals.values().cloned());
+            let (block_producers, bp_stake_threshold) = select_block_producers(
+                &mut block_producer_proposals,
+                epoch_config.num_block_producer_seats as usize,


I'm curious why chunk producers and chunk validators are configured in validator_selection_config but block producers aren't. Some legacy stuff?

Yeah, legacy. There should be a separate storage for EpochConfigs #11265

wacban · 2024-05-09T07:15:25Z

chain/epoch-manager/src/validator_selection.rs

+            let max_validators_for_role = cmp::max(
+                chunk_producers.len(),
+                cmp::max(block_producers.len(), chunk_validators.len()),
            );


nit: maybe a bit shorter version but up to you
chunk_producers.len().max(block_producers.len()).max(chunk_validators.len())

wacban · 2024-05-09T07:18:45Z

chain/epoch-manager/src/validator_selection.rs

+            let unselected_proposals = if chunk_producers.len() == max_validators_for_role {
+                chunk_producer_proposals
+            } else if block_producers.len() == max_validators_for_role {
+                block_producer_proposals
+            } else {
+                chunk_validator_proposals
+            };


Can you explain what are the unselected proposals and why is it the ones with max validator role? In a typical configuration we're going to have the most chunk validators. Why do we make all chunk validator proposals "unselected"?

This follows current implementation style. When one calls select_validators, passed proposals ( e.g. chunk_validator_proposals) are mutated: if proposal is selected, it is popped out of set, so the only ones remaining are unselected.

Then, I use the fact that all roles are selected by stake, so the unselected proposals will come from the role with maximal amount of validators. All other proposals will have at least one role assigned.

This is not ideal but I don't want to tweak this part of implementation as well for now.

wacban · 2024-05-09T07:26:10Z

chain/epoch-manager/src/validator_selection.rs

@@ -341,11 +381,140 @@ impl Ord for OrderedValidatorStake {
 mod old_validator_selection {
    use super::*;

-    pub fn assign_chunk_producers_to_shards(
+    pub(crate) fn select_validators_from_proposals(


nit: Can you add comments to the pub functions?

wacban · 2024-05-09T07:26:39Z

chain/epoch-manager/src/validator_selection.rs

+        BinaryHeap<OrderedValidatorStake>,
+        Vec<ValidatorStake>,
+        Vec<ValidatorStake>,
+        Vec<ValidatorStake>,
+        Balance,


nit: Can you put it in a struct with nice names and comments?

tayfunelmas · 2024-05-15T01:22:56Z

chain/epoch-manager/src/validator_selection.rs

+    } = if checked_feature!("stable", StatelessValidationV0, next_version) {
+        select_validators_from_proposals(epoch_config, proposals, next_version)
+    } else {
+        old_validator_selection::select_validators_from_proposals(


very nit. maybe not say "old" yet but something like stateful_validator_selection, as this will still be effective until the launch? or V1 vs V2 as in the case of other evolving structs in the code?

I would keep old_validator_selection for now because this is existing naming for old versions and easy to fix in the future.

tayfunelmas · 2024-05-15T01:33:21Z

chain/epoch-manager/src/validator_selection.rs

+    // TODO: getting unselected proposals must be simpler. For example,
+    // we can track all roles assign to each validator in some structure
+    // and then return all validators which don't have any role.
+    let max_validators_for_role =


I do not get this part. What does unselected proposals represent? I first thought these are the validators that we are unable to find a role but you are now assigning the entire set of block or chunk producers/validators to that set. Do I misunderstand the semantics of unselected proposals?

I stated the purpose in comment:

ValidatorRoles { /// Proposals which were not given any role. unselected_proposals: BinaryHeap<OrderedValidatorStake>,

So "validators that we are unable to find a role" is right.

you are not assigning the entire set of block or chunk producers/validators to that set.

I don't get the question either... The goal is to get validators without roles, not the opposite.
One way to do it is to take one of chunk_validator_proposals/... corresponding to maximal number of roles set because all proposals are selected by stake. After select_validators/... is called, only unselected proposals are left in this set. Selected proposals go to chunk_validators.

Please note that this is how legacy selection logic currently works and I want to make changes iteratively to make it better with time.

I agree with @tayfunelmas that this is very confusing to read. How about :

a) Rather than mutating the x_proposals you pass it into the select_x and have the unselected ones returned?

let ordered_proposals = order_proposals(proposals.values().cloned()); let (chunk_producers, not_chunk_producers, cp_stake_threshold) = select_chunk_producers( ordered_proposals, ... );

or

b) To find the unselected ones just iterate over all proposals and check which ones are not in bp, cp, val sets.

for proposal in ordered_proposals { if chunk_producers.contains(proposal) continue; if block_producers.contains(proposal) continue; if chunk_validators.contains(proposal) continue; unselected.insert(proposal); }

wacban

LGTM
I focused on the new stuff assuming that the old selection code is just moved around to a new method and that it's covered by enough tests. Please let me know if you'd like me to have a proper look at the old_* methods.

wacban · 2024-05-15T09:22:20Z

chain/epoch-manager/src/validator_selection.rs

+/// Helper struct which is a result of proposals processing.
+struct ValidatorRoles {
+    /// Proposals which were not given any role.
+    unselected_proposals: BinaryHeap<OrderedValidatorStake>,
+    /// Validators which are assigned to produce chunks.
+    chunk_producers: Vec<ValidatorStake>,
+    /// Validators which are assigned to produce blocks.
+    block_producers: Vec<ValidatorStake>,
+    /// Validators which are assigned to validate chunks.
+    chunk_validators: Vec<ValidatorStake>,
+    /// Stake threshold to become a validator.
+    threshold: Balance,
+}
+
+/// Helper struct which is a result of assigning chunk producers to shards.
+struct ChunkProducersAssignment {
+    /// List of all validators in the epoch.
+    /// Note that it doesn't belong here, but in the legacy code it is computed
+    /// together with chunk producers assignment.
+    all_validators: Vec<ValidatorStake>,
+    /// Maps validator account names to local indices throughout the epoch.
+    validator_to_index: HashMap<AccountId, ValidatorId>,
+    /// Maps chunk producers to shards, where i-th list contains local indices
+    /// of validators producing chunks for i-th shard.
+    chunk_producers_settlement: Vec<Vec<ValidatorId>>,
+}


That's fantastic, thank you!

wacban · 2024-05-15T09:33:55Z

chain/epoch-manager/src/validator_selection.rs

+    // TODO: getting unselected proposals must be simpler. For example,
+    // we can track all roles assign to each validator in some structure
+    // and then return all validators which don't have any role.
+    let max_validators_for_role =


I agree with @tayfunelmas that this is very confusing to read. How about :

a) Rather than mutating the x_proposals you pass it into the select_x and have the unselected ones returned?

let ordered_proposals = order_proposals(proposals.values().cloned()); let (chunk_producers, not_chunk_producers, cp_stake_threshold) = select_chunk_producers( ordered_proposals, ... );

or

b) To find the unselected ones just iterate over all proposals and check which ones are not in bp, cp, val sets.

for proposal in ordered_proposals { if chunk_producers.contains(proposal) continue; if block_producers.contains(proposal) continue; if chunk_validators.contains(proposal) continue; unselected.insert(proposal); }

wacban · 2024-05-15T09:35:05Z

chain/epoch-manager/src/validator_selection.rs

+}
+
+/// Selects validator roles for the given proposals.
+fn select_validators_from_proposals(


This is really pretty now :)

Longarithm added 6 commits May 7, 2024 23:30

debug

fcba344

correct numbers

c4c2f28

use bps

f1ddf8b

interface fix

1d53a49

fix tests

74393f2

no debug

2d3cd6e

Longarithm added the A-stateless-validation Area: stateless validation label May 7, 2024

Longarithm linked an issue May 7, 2024 that may be closed by this pull request

Change validator assignment to consider all validator proposals instead of chunk producer only #11202

Closed

Longarithm added 4 commits May 8, 2024 02:12

warnd

31aad60

debug

3761136

fix cp numbers

6e73a84

fix cp numbers

141e495

Longarithm requested review from wacban, pugachAG and robin-near May 7, 2024 23:42

Longarithm marked this pull request as ready for review May 8, 2024 07:00

Longarithm requested a review from a team as a code owner May 8, 2024 07:00

Longarithm changed the title ~~draft: consider all proposals for chunk validators~~ feat: consider all proposals for chunk validators May 8, 2024

Longarithm mentioned this pull request May 8, 2024

Standardize epoch configs #11265

Open

put cv in config

f0e5fa9

tayfunelmas reviewed May 8, 2024

View reviewed changes

Longarithm added 2 commits May 8, 2024 19:47

todo

6ee3d97

todo

bab624c

tayfunelmas reviewed May 8, 2024

View reviewed changes

chain/epoch-manager/src/shard_assignment.rs Show resolved Hide resolved

tayfunelmas reviewed May 8, 2024

View reviewed changes

Longarithm added 2 commits May 8, 2024 23:34

todo

8048e32

config

79f0ea7

Longarithm mentioned this pull request May 8, 2024

draft: sticky chunk producers assignment Longarithm/nearcore#2

Draft

wacban reviewed May 9, 2024

View reviewed changes

comments

e5fa9b0

Longarithm requested review from tayfunelmas and wacban May 13, 2024 11:58

Longarithm and others added 3 commits May 13, 2024 14:58

Merge branch 'master' into epoch-2

c30bd89

fix derived setup

ffb08e1

fix 2

b4f6660

tayfunelmas reviewed May 15, 2024

View reviewed changes

wacban approved these changes May 15, 2024

View reviewed changes

Longarithm and others added 3 commits May 15, 2024 17:27

feedback

ba3d8e2

Merge branch 'master' into epoch-2

157c112

feedback

1cb511a

Longarithm enabled auto-merge May 15, 2024 14:28

Longarithm added this pull request to the merge queue May 15, 2024

Merged via the queue into near:master with commit 7befdbd May 15, 2024
29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: consider all proposals for chunk validators #11252

feat: consider all proposals for chunk validators #11252

Longarithm commented May 7, 2024 •

edited

wacban commented May 8, 2024

tayfunelmas May 8, 2024 •

edited

tayfunelmas May 8, 2024

codecov bot commented May 8, 2024 •

edited

wacban May 9, 2024

wacban May 9, 2024

Longarithm May 13, 2024

wacban May 9, 2024

wacban May 9, 2024

Longarithm May 13, 2024

wacban May 9, 2024

wacban May 9, 2024

tayfunelmas May 15, 2024

Longarithm May 15, 2024

tayfunelmas May 15, 2024 •

edited

Longarithm May 15, 2024 •

edited

wacban May 15, 2024

wacban left a comment

wacban May 15, 2024

wacban May 15, 2024

wacban May 15, 2024

feat: consider all proposals for chunk validators #11252

feat: consider all proposals for chunk validators #11252

Conversation

Longarithm commented May 7, 2024 • edited

wacban commented May 8, 2024

tayfunelmas May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 8, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tayfunelmas May 15, 2024 • edited

Choose a reason for hiding this comment

Longarithm May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wacban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Longarithm commented May 7, 2024 •

edited

tayfunelmas May 8, 2024 •

edited

codecov bot commented May 8, 2024 •

edited

tayfunelmas May 15, 2024 •

edited

Longarithm May 15, 2024 •

edited