Add vote participation api #3916
Conversation
lukaszrzasik
requested review from
bfish713,
imabdulbasit,
jbearer,
rob-maron,
ss-es and
sveitser
as code owners
Summary of ChangesHello @lukaszrzasik, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a comprehensive system for tracking and exposing node vote participation within the HotShot consensus mechanism. It adds new data structures and logic to the core consensus engine to monitor voting activity across epochs and provides new API endpoints for external systems to query this participation data. The changes aim to improve visibility into validator behavior and overall network health. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces a vote participation API, which is a valuable addition for monitoring node health. The implementation is well-structured, primarily through the new VoteParticipation struct within the Consensus state, and the API endpoints are correctly defined. I've noted several opportunities to refactor duplicated code, which would enhance the maintainability of the new functionality. Overall, this is a strong contribution.
| let (stake_table, success_threshold) = if let Ok(epoch_membership) = self | ||
| .membership_coordinator | ||
| .stake_table_for_epoch(Some(next_epoch)) | ||
| .await | ||
| { | ||
| ( | ||
| epoch_membership.stake_table().await, | ||
| epoch_membership.success_threshold().await, | ||
| ) | ||
| } else { | ||
| tracing::warn!( | ||
| "Failed to get stake table for epoch {} while updating vote \ | ||
| participation", | ||
| next_epoch | ||
| ); | ||
| (HSStakeTable::default(), U256::MAX) | ||
| }; |
There was a problem hiding this comment.
This logic for fetching the stake table and success threshold is also present in crates/hotshot/hotshot/src/lib.rs. To improve maintainability and reduce code duplication, consider adding a helper function to EpochMembershipCoordinator that encapsulates this logic.
| fn current_vote_participation( | ||
| &self, | ||
| ) -> HashMap<<TYPES::SignatureKey as SignatureKey>::VerificationKeyType, f64> { | ||
| self.current_epoch_participation | ||
| .iter() | ||
| .map(|(key, votes)| { | ||
| ( | ||
| key.clone(), | ||
| if self.current_epoch_num_views == 0 { | ||
| 0.0 | ||
| } else { | ||
| *votes as f64 / self.current_epoch_num_views as f64 | ||
| }, | ||
| ) | ||
| }) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
The logic for calculating the vote ratio is repeated in get_participation, current_vote_participation, and previous_vote_participation. To improve code clarity and reduce duplication, consider extracting this logic into a private helper function.
For example:
fn calculate_ratio(num_votes: &u64, total_views: u64) -> f64 {
if total_views == 0 {
0.0
} else {
*num_votes as f64 / total_views as f64
}
}You can then use this helper in current_vote_participation and other methods.
| fn signers( | ||
| &self, | ||
| stake_table: &[<TYPES::SignatureKey as SignatureKey>::StakeTableEntry], | ||
| threshold: U256, | ||
| ) -> Result<Vec<<TYPES::SignatureKey as SignatureKey>::VerificationKeyType>> { | ||
| if self.view_number == TYPES::View::genesis() { | ||
| return Ok(vec![]); | ||
| } | ||
| let real_qc_pp = | ||
| <TYPES::SignatureKey as SignatureKey>::public_parameter(stake_table, threshold); | ||
|
|
||
| <TYPES::SignatureKey as SignatureKey>::signers( | ||
| &real_qc_pp, | ||
| self.signatures.as_ref().unwrap(), | ||
| ) | ||
| .wrap() | ||
| .context(|e| warn!("Tracing signers: {e}")) | ||
| } |
There was a problem hiding this comment.
The signers method implementation is duplicated across three Certificate trait implementations in this file (DaData, DaData2, and QuorumMarker). To avoid code duplication, you could move this logic into a method on SimpleCertificate itself, and then have the trait implementations call this shared method.
| /// Get the current vote participation. | ||
| async fn current_vote_participation(&self) -> HashMap<PubKey, f64> { | ||
| self.consensus() | ||
| .await | ||
| .read() | ||
| .await | ||
| .consensus() | ||
| .read() | ||
| .await | ||
| .current_vote_participation() | ||
| } |
There was a problem hiding this comment.
The pattern for accessing the consensus state is repeated for current_proposal_participation, previous_proposal_participation, and the new current_vote_participation and previous_vote_participation methods. To reduce boilerplate and improve readability, consider adding a helper method to ApiState that encapsulates this access pattern.
For example:
async fn get_consensus_read_lock(&self) -> async_lock::RwLockReadGuard<'_, hotshot_types::consensus::Consensus<SeqTypes>> {
self.consensus()
.await
.read()
.await
.consensus()
.read()
.await
}Then you can simplify this method and others like it:
async fn current_vote_participation(&self) -> HashMap<PubKey, f64> {
self.get_consensus_read_lock().await.current_vote_participation()
}
Closes #<ISSUE_NUMBER>
This PR:
Adds vote participation API.
The API can be used to show each's node vote participation in the current and previous epoch.
This PR does not:
Key places to review: