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] - Store DRB seed and epoch root with consistent block info #4019

Merged
merged 10 commits into from
Jan 14, 2025

Conversation

shenkeyao
Copy link
Member

@shenkeyao shenkeyao commented Jan 9, 2025

Closes #3998. Closes #4008.

This PR:

  • Moves store_received_drb_result inside store_drb_seed_and_result to handle storing the seed, the received result, and the computed result together.
  • Fixes the logic in store_drb_seed_and_result to use the decided block rather than the newly proposed block for storing the DRB seed.
  • Removes the condition to block voting if failed to store the received result.
  • Rename decide_from_proposal_add_epoch_root to decide_epoch_root.
  • Fixes the logic in decide_epoch_root for adding epoch root.

@ss-es ss-es mentioned this pull request Jan 10, 2025
crates/task-impls/src/helpers.rs Outdated Show resolved Hide resolved
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
crates/task-impls/src/quorum_vote/handlers.rs Outdated Show resolved Hide resolved
crates/task-impls/src/helpers.rs Outdated Show resolved Hide resolved
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 to me, just a few minor comments

Comment on lines 334 to 337
} else {
tracing::warn!(
"No decided leaf while a view has been decided, which should be impossible."
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this might sometimes happen with the way our code is written, but I'm not 100% sure either way. I don't think we necessarily need to log here, but no strong opinion

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed this to an info log and removed "which should be impossible", to still log something but not indicate an error.

Comment on lines 495 to 496
tracing::warn!(
"No decided leaf while a view has been decided, which should be impossible."
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment

Copy link
Member Author

Choose a reason for hiding this comment

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

Same change as above.

Comment on lines 277 to 285
// Skip if we are not in the committee of the next epoch.
if !task_state
.membership
.read()
.await
.has_stake(&task_state.public_key, current_epoch_number + 1)
{
return Ok(());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense to remove the has_stake check here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed!

@shenkeyao shenkeyao merged commit 92b32f1 into main Jan 14, 2025
18 checks passed
@shenkeyao shenkeyao deleted the keyao/fix-drb-store branch January 14, 2025 17:57
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.

Store epoch root with consistent block and block header [DRB] - Store DRB seed with consistent block and QC
2 participants