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

Implement timely vote credits feature. #32957

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

bji
Copy link
Contributor

@bji bji commented Aug 23, 2023

Re-submission of the timely_vote_credits feature, with a guard around credits accounting to ensure that credit counting is not altered before the feature is enabled.

@bji bji requested a review from AshwinSekar August 23, 2023 17:52
// can be in this set are those oldest slots in the current vote state that are not present in the
// new vote state; these have been "popped off the back" of the tower and thus represent finalized slots
let mut finalized_slot_count = 1_u64;
// Accumulate credits earned by newly rooted slots. The behavior changes with timely_vote_credits: prior to
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the updated comment for the additional credits accounting fix.

let timely_vote_credits = feature_set.map_or(false, |f| {
f.is_active(&feature_set::timely_vote_credits::id())
});
let mut earned_credits = if timely_vote_credits { 0_u64 } else { 1_u64 };
Copy link
Contributor Author

@bji bji Aug 23, 2023

Choose a reason for hiding this comment

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

Lines 616-619 are changed from the original timely_vote_credits submission as part of the credits accounting fix.


if let Some(new_root) = new_root {
for current_vote in &vote_state.votes {
// Find the first vote in the current vote state for a slot greater
// than the new proposed root
if current_vote.slot() <= new_root {
if timely_vote_credits || (current_vote.slot() != new_root) {
Copy link
Contributor Author

@bji bji Aug 23, 2023

Choose a reason for hiding this comment

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

This conditional is added as part of the credits accounting fix.

@bji
Copy link
Contributor Author

bji commented Aug 23, 2023

I have added comments to the sections that were changed from the original timely_vote_credits change. These are the only differences with that original PR, otherwise this PR is identical to the timely_vote_credits change already accepted (I authored this new change with: git revert -n 427b8b133203dd363a4b2a22018abd66ae3693d4 then added in the fix plus new test case).

@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Merging #32957 (0df05a9) into master (ece376f) will increase coverage by 0.0%.
The diff coverage is 96.8%.

@@           Coverage Diff            @@
##           master   #32957    +/-   ##
========================================
  Coverage    82.1%    82.1%            
========================================
  Files         786      786            
  Lines      211634   211880   +246     
========================================
+ Hits       173794   174036   +242     
- Misses      37840    37844     +4     

📢 Have feedback on the report? Share it here.

@bji bji force-pushed the timely_vote_credits_2_plus_fix branch 2 times, most recently from e02824b to ff23b22 Compare September 8, 2023 05:24
@AshwinSekar
Copy link
Contributor

Currently running this against mnb, will approve if there are no issues over the weekend

Copy link
Contributor

@AshwinSekar AshwinSekar left a comment

Choose a reason for hiding this comment

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

Confirmed no mnb divergence over the weekend with the fix.

@AshwinSekar AshwinSekar merged commit bdf7207 into solana-labs:master Sep 12, 2023
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants