-
Notifications
You must be signed in to change notification settings - Fork 30
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
[DEPENDENCY_REFACTOR] - Fix the ValidatedStateUpdated dependency in the quorum proposal and vote tasks #3322
Conversation
* fix * remove dead test * fix lint * fix test * Fix vote tests --------- Co-authored-by: Keyao Shen <[email protected]>
613c95f
to
5d8f197
Compare
…t handling from quorum proposal
@@ -175,6 +177,30 @@ impl<TYPES: NodeType> HandleDepOutput for ProposalDependencyHandle<TYPES> { | |||
|
|||
#[allow(clippy::no_effect_underscore_binding)] | |||
async fn handle_dep_result(self, res: Self::Output) { | |||
let high_qc_view_number = self.consensus.read().await.high_qc().view_number; |
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.
Followup on an earlier discussion, in all testing, this value, and the provided high qc value were never different, so it seemed like a red herring. I can add the change in though that effectively takes the high qc from the event if it's available, and falls back to the shared state otherwise.
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.
Looks good to me. One suggestion only but I'm not sure if it's correct.
consensus | ||
.write() | ||
.await | ||
.update_saved_leaves(proposed_leaf.clone()); |
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.
Would it make sense to call update_saved_leaves
from within update_validated_state_map
? The only place where both of them are not called one after another is DaTaskState
. It seems to me that they could (and maybe even should) be called together even there and then it means that they are coupled so tightly that we should make sure that they are always called together. What do you think?
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.
Yeah we definitely want to do this. I had originally done it, but the change was not great. I plan to do it as a follow up after evaluating if ViewInner::Da is still relevant, as it made the implementation quite tricky. I’ll tag you in the follow up.
Closes #3296. WIP!
This PR:
This PR does not:
Key places to review: