feat(chunk-validator-assignment): shuffle shard ids #10298
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #10298 +/- ##
=======================================
Coverage 71.82% 71.83%
=======================================
Files 712 712
Lines 143059 143143 +84
Branches 143059 143143 +84
=======================================
+ Hits 102749 102823 +74
- Misses 35610 35612 +2
- Partials 4700 4708 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
mooori
requested review from
wacban,
birchmd and
robin-near
and removed request for
wacban
There was a problem hiding this comment.
Thank you very much! It is my first time reading code related to validator assignment, and it was quite nice, thanks to comments. Leaving some readability comments, hope they can improve it even more.
P.S. Could you add a definition of what a "mandate" is somewhere to the code? After some searching I found in #9983 that "mandate" is a liability of validator to validate some shard for each block in epoch, similar to "seat". But that was a long way. This is a common issue in a codebase - I think there is no strict definition of "seat" as well.
|
|
||
| // For the current `shard_id`, collect mandates with index `i` such that | ||
| // For the current `mandates_shard_id`, collect mandates with index `i` such that |
There was a problem hiding this comment.
There is no mandates_shard_id around, could we consider mandates_shard_id = shard_ids_for_mandates.get_alias(shard_id)? Same below for partials_shard_id.
There was a problem hiding this comment.
mandates_shard_id in the comment is a leftover from a previous version of the code, sorry for the confusion. Hope that’s fixed with 7cc02da.
| @@ -125,35 +131,40 @@ impl ValidatorMandates { | |||
| // assigned the mandates with indices 1, 5, and 9. | |||
| // | |||
| // TODO(#10014) shuffle shard ids to avoid a bias towards smaller shard ids | |||
| let mut mandates_per_shard = Vec::with_capacity(self.config.num_shards); | |||
| let mut mandates_per_shard: ValidatorMandatesAssignment = | |||
| vec![HashMap::new(); self.config.num_shards]; | |||
| for shard_id in 0..self.config.num_shards { | |||
There was a problem hiding this comment.
I would even consider renaming shard_id with abstract i to indicate that it is not the actual shard id to be impacted.
There was a problem hiding this comment.
The name shard_id helps to point out what is interated. I think it’s not problematic since mandates are actually collected for shard_id and then written to position *.get_alias(shard_id). With an abstract i that might be harder to follow since there’s a lot going on here.
| /// When assigning mandates first to shards with lower ids, the shards with higher ids might end up | ||
| /// with fewer assigned mandates. | ||
| /// | ||
| /// Assumes shard ids are in `[0, num_shards)`. | ||
| /// | ||
| /// # Example | ||
| /// | ||
| /// Assume there are 3 shards and 5 mandates. Assigning to shards with lower ids first, the first | ||
| /// two shards get 2 mandates each. For the third shard only 1 mandate remains. | ||
| /// | ||
| /// # Shuffling to avoid bias | ||
| /// | ||
| /// When mandates cannot be distributed evenly across shards, some shards will be assigned one | ||
| /// mandata less than others. Shuffling shard ids prevents a bias towards lower shard ids, as it is | ||
| /// no longer predictable which shard(s) will be assigned one mandate less. |
There was a problem hiding this comment.
I think this explanation belongs more to fn sample than here. ShuffledShardIds is just a shuffled vector which doesn't carry much context, so I'd just say "Used for assigning validator mandates to shards".
There was a problem hiding this comment.
7cc02da extends the fn sample doc comment to mention the shard id shuffle and links to ShuffledShardIds. Moving this comment to fn sample might overwhelm other devs who just want to call fn sample and don’t really care about the details of shard id shuffling.
Good point! Explaining it in the comment added in 36177d6. |
Introduces a shuffling of shard ids when assigning chunk validators to shards. Without that shuffling it is predictable which shards receive fewer (partial) mandates when they cannot be distributed evenly across shards. See this comment which also brings up the idea of shard id shuffling.
Separate shuffles for full and partial mandate assignment
If there was only one shuffling for both full and partial mandates, then the shard(s) that get a full mandate less may also get a partial mandate less. Separate shufflings counter that and ideally (depending on randomness) stake is distributed more evenly as shards that receive a full mandate less may still get the maximum amount of partial mandates.
Related issue
#10014