Skip to content
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

[DRB] - Verify the DRB result from the computation one epoch in advance #3948

Open
wants to merge 67 commits into
base: main
Choose a base branch
from

Conversation

shenkeyao
Copy link
Member

@shenkeyao shenkeyao commented Dec 6, 2024

Closes #3858.

Note: This PR is based on the lr/double-quorum branch corresponding to #3922, which if merged first, this PR will be merged directly to main.

This PR:

  • Splits DrbComputations into DrbSeedsAndResults and DrbComputation.
    • Moves the former to the shared Consensus but keeps the latter at the same place.
    • Moves most contents in quorum_vote/drb_computations.rs to types/src/drb.rs, and some to the quorum_vote/handlers.rs.
  • Updates the proposal task to include the DRB result for the next epoch at the last block.
  • Updates the vote task.
    • Third from the last block:
      • Stores the computed result for nodes in epoch e - 1.
    • Last block:
      • Stores the received DRB result from the proposal for nodes in epoch e.
      • Verifies received result with computed results for nodes in epoch e - 1.
  • Fixes how the third from last block is defined with a helper function in utils.rs.

This PR does not:

  • Consumes the DRB result for the leader election, which will be done in the next PR.

Key places to review:

  • quorum_proposal/handlers.rs.
  • quorum_vote/handlers.rs.
  • types/src/drb.rs.
  • (Many other files only contain format changes, probably due to merging both lr/double-quorum and main.)

TYPES: NodeType,
I: NodeImplementation<TYPES>,
V: Versions,
>(
epoch_number: TYPES::Epoch,
task_state: &mut QuorumVoteTaskState<TYPES, I, V>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels a bit over-coupled to make this take the whole task_state when all we need is drb_computation and consensus, but those also don't really exist in any other place that makes sense. Might make it make understanding what the function can mutate more obvious though

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we also need the shared consensus field of task_state in this function to get the results table, so I prefer to keep the argument as is. But let me know if you think there's a better way though!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of a better way to pass the arguments, I'd say leave it as is. Otherwise everything looks good to me!

Base automatically changed from lr/double-quorum to main December 18, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DRB] - Verify the DRB result from the computation one epoch in advance
5 participants