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

Use timestamp to tiebreak votes in banking_stage #31925

Merged
merged 1 commit into from
Jun 5, 2023

Conversation

AshwinSekar
Copy link
Contributor

Problem

Refreshed votes are dropped as the deduplication logic doesn't allow new votes for the same slot to entire the queue.
Same bug as #31879 further in the pipeline in banking_stage. This affects both tpu and gossip votes.

Summary of Changes

Allow refreshed votes in banking stage queue by using timestamp to tiebreak.

Note: only affects v1.16.0+ with the allow_votes_to_directly_update_vote_state feature enabled.

Fixes #

@AshwinSekar AshwinSekar force-pushed the refresh-votes-banking branch from 814cfbe to 39e576d Compare June 1, 2023 21:16
@AshwinSekar AshwinSekar added the v1.16 PRs that should be backported to v1.16 label Jun 1, 2023
Copy link
Contributor

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

looks reasonable to me, but i am not too sure if timestamp is reliable for the purpose.

sdk/program/src/vote/instruction.rs Show resolved Hide resolved
sdk/program/src/vote/instruction.rs Show resolved Hide resolved
carllin
carllin previously approved these changes Jun 3, 2023
Comment on lines 209 to 210
let latest_slot = latest_vote.read().unwrap().slot();
if slot > latest_slot {
let latest_timestamp = latest_vote.read().unwrap().timestamp;
Copy link
Contributor

Choose a reason for hiding this comment

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

think we can just take the lock once and grab both these fields

@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

Merging #31925 (a59357e) into master (c1d68d3) will increase coverage by 0.0%.
The diff coverage is 96.6%.

@@           Coverage Diff            @@
##           master   #31925    +/-   ##
========================================
  Coverage    81.9%    81.9%            
========================================
  Files         760      760            
  Lines      207407   207513   +106     
========================================
+ Hits       169968   170070   +102     
- Misses      37439    37443     +4     

@AshwinSekar AshwinSekar requested a review from carllin June 5, 2023 13:01
Copy link
Contributor

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

lgtm

@AshwinSekar AshwinSekar merged commit 9f62cc1 into solana-labs:master Jun 5, 2023
mergify bot pushed a commit that referenced this pull request Jun 5, 2023
AshwinSekar added a commit that referenced this pull request Jun 5, 2023
…31925) (#31973)

Use timestamp to tiebreak votes in banking_stage (#31925)

(cherry picked from commit 9f62cc1)

Co-authored-by: Ashwin Sekar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.16 PRs that should be backported to v1.16
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants