-
Notifications
You must be signed in to change notification settings - Fork 116
feat(driver): do not schedule the same timeout twice #1501
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
base: main
Are you sure you want to change the base?
Changes from all commits
ccd02fc
4d031cf
cb9c578
7cd5092
023fe6f
8262369
76ec324
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,6 +62,10 @@ where | |
| /// The first element of the tuple is the round at which that input has been emitted. | ||
| pending_inputs: Vec<(Round, RoundInput<Ctx>)>, | ||
|
|
||
| /// The set of timeouts already scheduled by the consensus logic. | ||
| /// Intended to avoid scheduling the same timeout multiple times. | ||
| scheduled_timeouts: Vec<Timeout>, | ||
|
|
||
| last_prevote: Option<Ctx::Vote>, | ||
| last_precommit: Option<Ctx::Vote>, | ||
|
|
||
|
|
@@ -98,6 +102,7 @@ where | |
| round_state, | ||
| proposer: None, | ||
| pending_inputs: vec![], | ||
| scheduled_timeouts: vec![], | ||
| commit_certificates: vec![], | ||
| polka_certificates: vec![], | ||
| last_prevote: None, | ||
|
|
@@ -119,7 +124,6 @@ where | |
| // Reset the round state | ||
| let round_state = RoundState::new(height, Round::Nil); | ||
| self.round_state = round_state; | ||
| self.round_certificate = None; | ||
|
|
||
| // Reset the proposal keeper | ||
| let proposal_keeper = ProposalKeeper::new(); | ||
|
|
@@ -130,8 +134,10 @@ where | |
| self.polka_certificates = vec![]; | ||
|
|
||
| // Reset additional internal state | ||
| self.scheduled_timeouts.clear(); | ||
| self.last_prevote = None; | ||
| self.last_precommit = None; | ||
| self.round_certificate = None; | ||
| } | ||
|
|
||
| /// Return the height of the consensus. | ||
|
|
@@ -370,17 +376,29 @@ where | |
|
|
||
| RoundOutput::Vote(vote) => self.lift_vote_output(vote, outputs), | ||
|
|
||
| RoundOutput::ScheduleTimeout(timeout) => outputs.push(Output::ScheduleTimeout(timeout)), | ||
| RoundOutput::ScheduleTimeout(timeout) => self.lift_timeout_output(timeout, outputs), | ||
|
|
||
| RoundOutput::GetValueAndScheduleTimeout(height, round, timeout) => { | ||
| outputs.push(Output::ScheduleTimeout(timeout)); | ||
| self.lift_timeout_output(timeout, outputs); | ||
| outputs.push(Output::GetValue(height, round, timeout)); | ||
| } | ||
|
|
||
| RoundOutput::Decision(round, proposal) => outputs.push(Output::Decide(round, proposal)), | ||
| } | ||
| } | ||
|
|
||
| fn lift_timeout_output(&mut self, timeout: Timeout, outputs: &mut Vec<Output<Ctx>>) { | ||
| if timeout.round < self.round() || self.scheduled_timeouts.contains(&timeout) { | ||
| return; | ||
| } | ||
| debug_assert!( | ||
| timeout.is_consensus(), | ||
| "lift_timeout_output received {timeout:?}" | ||
| ); | ||
| self.scheduled_timeouts.push(timeout); | ||
| outputs.push(Output::ScheduleTimeout(timeout)); | ||
| } | ||
|
|
||
| fn lift_vote_output(&mut self, vote: Ctx::Vote, outputs: &mut Vec<Output<Ctx>>) { | ||
| if vote.validator_address() != self.address() { | ||
| return; | ||
|
|
@@ -482,6 +500,10 @@ where | |
| // Update the proposer for the new round | ||
| self.proposer = Some(proposer); | ||
|
|
||
| // Remove useless timeouts from previous rounds | ||
| self.scheduled_timeouts | ||
| .retain(|timeout| timeout.round >= round); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar here, could this just be clear()? It is minor ofc, I just want to understand if we expect to see any future rounds here, since I don't see how
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also a good point. But the concern here is to limit the growth of this set.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we can schedule timeouts for future rounds - otherwise, |
||
|
|
||
| self.apply_input(round, RoundInput::NewRound(round)) | ||
| } | ||
|
|
||
|
|
||
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.
Minor: Can this ever be true
timeout.round < self.round()? Timeouts should only be for the current round. If purely defensive, I'd add awarn!here so we notice if it ever triggers.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.
Good point.