Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements epoch transition logic and refactors proposal handling. Key changes include the introduction of an EpochManager, the addition of DRB result validation, and the replacement of QuorumProposal2 with a new Proposal struct. The Consensus implementation is now asynchronous and includes epoch-aware staking checks. Feedback identifies a critical bug in the Proposal to Leaf2 conversion where the next_epoch_justify_qc field is omitted. Additionally, the reviewer recommends addressing TODOs regarding message filtering for old views and the explicit handling of DrbResult view numbers to ensure protocol robustness.
| view_number: self.view_number, | ||
| epoch: Some(self.epoch), | ||
| justify_qc: self.justify_qc, | ||
| next_epoch_justify_qc: None, |
There was a problem hiding this comment.
In the Into<Leaf2<T>> implementation for Proposal<T>, the next_epoch_justify_qc field is hardcoded to None. This means that if a Proposal contains a next_epoch_justify_qc, it will be lost when converting to a Leaf2. Since the Leaf2 is what gets committed to, this is a critical bug that could lead to consensus failures or state divergence, as nodes will not be committing to the full proposal data.
| next_epoch_justify_qc: None, | |
| next_epoch_justify_qc: self.next_epoch_justify_qc, |
| // TODO: This isn't correct, I think we need to process vote2 for views | ||
| // that haven't timed out, but are before the timeout view | ||
| if !matches!(input, ConsensusInput::DrbResult(_, _)) && view <= self.timeout_view { |
There was a problem hiding this comment.
The TODO comment here highlights a potential correctness issue. While DrbResult is now exempt from the old view check, other messages like Vote2 might also need to be processed for views that are before timeout_view but not yet timed out. Ignoring these could prevent certificate formation and stall progress. This should be addressed to ensure robust consensus.
| // TODO: where else can this cause problems? | ||
| ConsensusInput::DrbResult(..) => ViewNumber::genesis(), |
There was a problem hiding this comment.
The TODO here is valid. While returning ViewNumber::genesis() for DrbResult works for the input filtering logic in handle_input, it's a bit of a hack. If view_number() is ever used for DrbResult in other contexts, this could lead to unexpected behavior. It would be safer to handle DrbResult more explicitly, perhaps by matching on ConsensusInput before calling view_number(), or by restructuring the input handling to not require a view_number for all variants.
1 participant
Closes #<ISSUE_NUMBER>
This PR:
This PR does not:
Key places to review: