-
Notifications
You must be signed in to change notification settings - Fork 47
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
Use pre-defined stake-table in orchestrator #3685
Conversation
note: there are still some clippy errors. I'll handle them before merging (and after merging main in) |
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.
Thanks for doing this, approved but with a couple questions
crates/orchestrator/src/lib.rs
Outdated
fn register_from_list( | ||
&mut self, | ||
pubkey: &mut Vec<u8>, | ||
_da_requested: bool, |
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.
I think we should consider checking if or if not the node should be DA, and only let them connect if it matches their request
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.
done!
note that this may require some (mild?) testing, just to make sure that the is_da
flag is being correctly set in the sequencer. I think it may not have mattered previously
crates/orchestrator/src/lib.rs
Outdated
@@ -768,8 +833,29 @@ where | |||
network_config.manual_start_password = env_password.ok(); | |||
} | |||
|
|||
network_config.config.known_nodes_with_stake = vec![]; | |||
network_config.config.known_da_nodes = vec![]; | |||
if network_config.enable_registration_verification { |
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.
I know this wasn't introduced in this PR, but:
What do you think about enabling registration verification automatically if the list is nonzero? And having it be off if it's zero?
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.
oh yes, great idea! done
I've updated the PR and removed the enable_registration_verification
field altogether
This PR:
Updates the orchestrator's verification mode to now use the given stake table and pre-defined DA committee. In this mode,
run-config.toml
must provide:state_ver_key
before startup. Note that this replaces the previous verification mode, meaning the previous behaviour of allowing nodes from a whitelist (but not necessarily requiring the whole list) no longer exists.
One (welcome?) side-effect is that each node's index is now just its position in
run-config.toml
.This PR does not:
Key places to review: