-
Notifications
You must be signed in to change notification settings - Fork 65
apollo_consensus: update consensus sim test to simulate network msg drops #10869
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
apollo_consensus: update consensus sim test to simulate network msg drops #10869
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
fc26f31 to
ceb5ac4
Compare
f8e0428 to
4a7dfad
Compare
ceb5ac4 to
0e51b51
Compare
4a7dfad to
a8f5824
Compare
matanl-starkware
left a comment
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.
@matanl-starkware reviewed 1 file and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @asmaastarkware and @dafnamatsry).
crates/apollo_consensus/src/simulation_test.rs line 100 at r2 (raw file):
node_votes: HashMap<Round, Vote>, /// The drop rate for the network. drop_rate: f64,
Consider keeping as 'keep_ratio', to avoid 1-P everywhere
Code quote:
drop_ratecrates/apollo_consensus/src/simulation_test.rs line 153 at r2 (raw file):
let leader_id = Self::get_leader(round); if leader_id != *VALIDATOR_ID && self.rng.gen_bool(1.0 - self.drop_rate) {
Move this part into self.schedule
Code quote:
&& self.rng.gen_bool(1.0 - self.drop_rate)crates/apollo_consensus/src/simulation_test.rs line 167 at r2 (raw file):
for i in 1..self.validators.len() { let voter = self.validators[i]; let commitment = Some(PROPOSAL_COMMITMENT);
We should have different commitments per round
Code quote:
PROPOSAL_COMMITMENTcrates/apollo_consensus/src/simulation_test.rs line 201 at r2 (raw file):
fn check_and_generate_next_round(&mut self, request_round: Round) { if request_round > self.current_max_round {
Suggestion:
if self.current_max_round < request_rounda8f5824 to
ce997f5
Compare
0e51b51 to
7e7b39b
Compare
e294bb6 to
fcb800e
Compare
asmaastarkware
left a comment
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.
@asmaastarkware made 3 comments.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @dafnamatsry and @matanl-starkware).
crates/apollo_consensus/src/simulation_test.rs line 153 at r2 (raw file):
Previously, matanl-starkware (Matan Lior) wrote…
Move this part into self.schedule
Done.
crates/apollo_consensus/src/simulation_test.rs line 167 at r2 (raw file):
Previously, matanl-starkware (Matan Lior) wrote…
We should have different commitments per round
Done.
crates/apollo_consensus/src/simulation_test.rs line 201 at r2 (raw file):
fn check_and_generate_next_round(&mut self, request_round: Round) { if request_round > self.current_max_round {
This just reads more naturally/readable to me..
matanl-starkware
left a comment
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.
@matanl-starkware reviewed 1 file and all commit messages, made 2 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @asmaastarkware and @dafnamatsry).
crates/apollo_consensus/src/simulation_test.rs line 201 at r2 (raw file):
Previously, asmaastarkware (asmaa-starkware) wrote…
This just reads more naturally/readable to me..
crates/apollo_consensus/src/simulation_test.rs line 170 at r3 (raw file):
} } }
Split logic and operation
Code quote (i):
match &event {
InputEvent::Internal(_) => {
self.pending_events.push(TimedEvent { tick: self.current_tick + delay, event });
}
_ => {
if self.rng.gen_bool(self.keep_ratio) {
self.pending_events.push(TimedEvent { tick: self.current_tick + delay, event });
}
}
}Code snippet (ii):
let should_enqueue = matches!(event, InputEvent::Internal(_))
|| self.rng.gen_bool(self.keep_ratio);
if should_enqueue {
self.pending_events.push(TimedEvent { tick: self.current_tick + delay, event });
}fcb800e to
70fb297
Compare
7e7b39b to
2b93b33
Compare
70fb297 to
178a8c3
Compare
matanl-starkware
left a comment
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.
@matanl-starkware reviewed 1 file and all commit messages, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry).
2b93b33 to
e9629cb
Compare
178a8c3 to
d6d2d07
Compare
d6d2d07 to
900acc4
Compare
e9629cb to
5274c34
Compare
9278d7c to
32a5ce3
Compare
a4df0f9 to
1500d7a
Compare
32a5ce3 to
a95546c
Compare
1500d7a to
54525f0
Compare
a95546c to
877d8d7
Compare
54525f0 to
7834218
Compare
877d8d7 to
4ee399d
Compare
7834218 to
db5fd8e
Compare
matanl-starkware
left a comment
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.
@matanl-starkware made 1 comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @asmaastarkware and @dafnamatsry).
crates/apollo_consensus/src/simulation_test.rs line 179 at r9 (raw file):
/// For rounds where NODE_0 is the proposer, peer votes are scheduled after /// the build finish event (which will be determined dynamically during simulation). fn pre_generate_all_rounds(&mut self) {
Most of the changes related to PR 10868.
Can you check why it happened?
Code quote:
fn pre_generate_all_rounds
matanl-starkware
left a comment
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.
@matanl-starkware reviewed 1 file and all commit messages, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry).
db5fd8e to
4af840c
Compare
4ee399d to
8cfadb5
Compare
4af840c to
c367da5
Compare
996a755 to
f4e8619
Compare
c367da5 to
a9f614d
Compare
f4e8619 to
dbcb7f4
Compare
a9f614d to
4e8a975
Compare
4e8a975 to
c5575e4
Compare
dbcb7f4 to
22eb9d0
Compare
dafnamatsry
left a comment
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.
@dafnamatsry reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @asmaastarkware).
fc7d85b

No description provided.