-
Notifications
You must be signed in to change notification settings - Fork 85
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
chore(consensus): allow running multiple simulations of consensus #2227
Conversation
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.
Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @asmaastarkware and @dan-starkware)
crates/sequencing/papyrus_consensus/run_consensus.py
line 99 at r1 (raw file):
def build_node(base_layer_node_url, temp_dir, num_validators, i): monitoring_gateway_server_port = find_free_port()
Why the empty line?
Suggestion:
def build_node(base_layer_node_url, temp_dir, num_validators, i):
monitoring_gateway_server_port = find_free_port()
crates/sequencing/papyrus_consensus/run_consensus.py
line 100 at r1 (raw file):
monitoring_gateway_server_port = find_free_port() tcp_port = BOOTNODE_TCP_PORT if i == 1 else find_free_port()
Let's create a variable so the code is self documenting (use below also)
Suggestion:
is_bootstrap = (i == 1)
tcp_port = BOOTNODE_TCP_PORT if is_bootstrap else find_free_port()
crates/sequencing/papyrus_consensus/run_consensus.py
line 116 at r1 (raw file):
if i == 1: specific_command = f"--network.secret_key {SECRET_KEY} "
let's just create command
not common_command
and do command += XXX
Code quote:
specific_command = f"--network.secret_key {SECRET_KEY} "
crates/sequencing/papyrus_consensus/run_consensus.py
line 151 at r1 (raw file):
# Ensure validator 0 runs last nodes.append(build_node(base_layer_node_url, temp_dir, num_validators, 0))
We already have the comment at the top of the function, so let's keep these terse.
Suggestion:
nodes.append(build_node(base_layer_node_url, temp_dir, num_validators, 1)) # Bootstrap
for i in range(2, num_validators):
nodes.append(build_node(base_layer_node_url, temp_dir, num_validators, i))
nodes.append(build_node(base_layer_node_url, temp_dir, num_validators, 0)) # Proposer
9f0ea57
to
103af7d
Compare
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.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @dan-starkware and @matan-starkware)
crates/sequencing/papyrus_consensus/run_consensus.py
line 99 at r1 (raw file):
Previously, matan-starkware wrote…
Why the empty line?
Done.
crates/sequencing/papyrus_consensus/run_consensus.py
line 100 at r1 (raw file):
Previously, matan-starkware wrote…
Let's create a variable so the code is self documenting (use below also)
Done.
crates/sequencing/papyrus_consensus/run_consensus.py
line 116 at r1 (raw file):
Previously, matan-starkware wrote…
let's just create
command
notcommon_command
and docommand += XXX
Done.
crates/sequencing/papyrus_consensus/run_consensus.py
line 151 at r1 (raw file):
Previously, matan-starkware wrote…
We already have the comment at the top of the function, so let's keep these terse.
Done.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2227 +/- ##
=======================================
Coverage 66.33% 66.33%
=======================================
Files 139 139
Lines 18346 18346
Branches 18346 18346
=======================================
Hits 12169 12169
Misses 4884 4884
Partials 1293 1293 ☔ View full report in Codecov by Sentry. |
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.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dan-starkware)
The base branch was changed.
103af7d
to
e6716d4
Compare
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.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dan-starkware)
This change is