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] - Start the DRB calculation two epochs in advance #3911

Merged
merged 6 commits into from
Dec 2, 2024

Conversation

shenkeyao
Copy link
Member

Closes #3857.

This PR:

  • Calls the DRB function in the vote task to start the computation for two views in advance.
  • Updates the vote task type to include spawned DRB computation tasks.
  • Moves and updates the DRB implementation file.

This PR does not:

  • Handles the use of the DRB result, which will be done in the upcoming PRs.

Key places to review:

  • All files, but the main changes are in handlers.rs.

crates/task-impls/src/quorum_vote/handlers.rs Outdated Show resolved Hide resolved
crates/task-impls/src/quorum_vote/handlers.rs Outdated Show resolved Hide resolved
@@ -233,6 +233,7 @@ impl<TYPES: NodeType, I: NodeImplementation<TYPES>, V: Versions> CreateTaskState
network: Arc::clone(&handle.hotshot.network),
quorum_membership: handle.hotshot.memberships.quorum_membership.clone().into(),
da_membership: handle.hotshot.memberships.da_membership.clone().into(),
drb_computations: BTreeMap::new(),
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need a BTreeMap here? I think we only want to run one of these calculations at a time

we can e.g. store the seeds and results in a map and trigger the start of the calculation at block_height % epoch_height = 1 or something

Copy link
Member Author

Choose a reason for hiding this comment

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

block_height % epoch_height = 1

Assuming this is (qc_block_number + 3) % task_state.epoch_height == 0 instead?

Makes sense not to use BTreeMap. I'm going to add a DrbTasks struct for this, containing the previous DRB result and the in-progress DRB calculation task, since we may need both of them in each epoch. Hopefully, it's not an overkill!

Copy link
Member Author

Choose a reason for hiding this comment

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

On second thought, BTreeMap might be more suitable here than a HashMap or the new struct I proposed above. We may have at most three things to store here, two DRB results and one in-progress task. I think BTreeMap makes garbage collection easier because the three epochs may not be consecutive due to the membership.

Lmk if this doesn't make sense or there's a better design 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.

As discussed on Zulip, I'll update the maps in a separate PR. Issue: #3933.

Copy link
Contributor

@ss-es ss-es left a comment

Choose a reason for hiding this comment

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

I think this looks good, but I left some thoughts about the BTreeMap on zulip

@shenkeyao shenkeyao merged commit 78ce7cb into main Dec 2, 2024
17 checks passed
@shenkeyao shenkeyao deleted the keyao/drb-start branch December 2, 2024 17:17
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] - Start the DRB calculation two epochs in advance
2 participants