This repository was archived by the owner on Jun 25, 2024. It is now read-only.
Open
Conversation
Any workspace that cares for security should run at least num_shards validators (potentially even factor_of_redundancy * num_shards). The proposed solution is to assign at least one validator from a workspace to each shard (depending on factor_of_redundancy, this can obviously be at least factor_of_redundancy). For workspaces that do not care about security, their validators will be assigned falling back to the nearprotocol Test plan: Described here https://docs.google.com/document/d/1Wc9b5eM3oQ57Ibe4RkzK3lUEZh3AAhQcx68_bcKH8yc/edit
mercepluka
approved these changes
Aug 11, 2023
mercepluka
left a comment
mercepluka
left a comment
There was a problem hiding this comment.
I left a few style comments, I think that the rest of the code is good.
| min_validators_per_shard: usize, | ||
| validator_account_id_to_workspace_id: HashMap<AccountId, usize>, | ||
| workspace_to_validator_ids: HashMap<usize, Vec<ValidatorId>>, | ||
| workspace_id_validators: HashMap<usize, Vec<AccountId>>, |
There was a problem hiding this comment.
Is there any difference between those 2 arguments? workspace_to_validator_ids and workspace_to_validator_ids
| let workspace_id: usize = if workspace_to_id.contains_key(&workspace_name) { | ||
| *workspace_to_id.get(&workspace_name).unwrap() | ||
| } else { | ||
| workspace_to_id.insert(workspace_name,num_workspaces); |
There was a problem hiding this comment.
Why are you converting workspace references to numbers?
|
|
||
| let mut result: Vec<Vec<T>> = (0..num_shards).map(|_| Vec::new()).collect(); | ||
|
|
||
| let mut index_to_insert: usize = 0; |
There was a problem hiding this comment.
I think this function would be more readable if an initial assignment is also done in a separate function (like 2 original functions are).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Any workspace that cares for security should run at least num_shards validators (potentially even factor_of_redundancy * num_shards). The proposed solution is to assign at least one validator from a workspace to each shard (depending on factor_of_redundancy, this can obviously be at least factor_of_redundancy).
For workspaces that do not care about security, their validators will be assigned falling back to the nearprotocol
Test plan:
Described here
https://docs.google.com/document/d/1Wc9b5eM3oQ57Ibe4RkzK3lUEZh3AAhQcx68_bcKH8yc/edit