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

Relax gossip to banking stage filtering to allow refreshed votes #31879

Merged
merged 1 commit into from
May 31, 2023

Conversation

AshwinSekar
Copy link
Contributor

@AshwinSekar AshwinSekar commented May 30, 2023

Problem

The feature flag allow_votes_to_directly_update_vote_state allows an optimization of only storing one vote per validator before forwarding to banking stage. We store votes for the latest slot - which implicitly drops refreshed votes.

Summary of Changes

Create a tiebreaker, if the slot is the same as the latest vote only accept it if it has a later timestamp (refreshed vote)

Fixes #

@AshwinSekar AshwinSekar force-pushed the refresh-votes-gossip branch from e71ca56 to 393e005 Compare May 30, 2023 20:26
@AshwinSekar AshwinSekar marked this pull request as ready for review May 30, 2023 20:37
@AshwinSekar AshwinSekar force-pushed the refresh-votes-gossip branch 2 times, most recently from b70d380 to fe9ba09 Compare May 30, 2023 20:52
@AshwinSekar AshwinSekar force-pushed the refresh-votes-gossip branch 2 times, most recently from effb3bb to 6354df6 Compare May 30, 2023 21:17
@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Merging #31879 (800f5b6) into master (9216ff8) will increase coverage by 0.0%.
The diff coverage is 97.3%.

❗ Current head 800f5b6 differs from pull request most recent head be5c1b0. Consider uploading reports for the commit be5c1b0 to get more accurate results

@@           Coverage Diff           @@
##           master   #31879   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         757      755    -2     
  Lines      206984   207057   +73     
=======================================
+ Hits       169543   169620   +77     
+ Misses      37441    37437    -4     

core/src/verified_vote_packets.rs Outdated Show resolved Hide resolved
core/src/verified_vote_packets.rs Show resolved Hide resolved
core/src/verified_vote_packets.rs Outdated Show resolved Hide resolved
@AshwinSekar AshwinSekar force-pushed the refresh-votes-gossip branch 3 times, most recently from a511a43 to 800f5b6 Compare May 31, 2023 18:21
t-nelson
t-nelson previously approved these changes May 31, 2023
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

steviez
steviez previously approved these changes May 31, 2023
Copy link
Contributor

@steviez steviez left a comment

Choose a reason for hiding this comment

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

LGTM!

carllin
carllin previously approved these changes May 31, 2023
Copy link
Contributor

@carllin carllin left a comment

Choose a reason for hiding this comment

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

just some nits, lgtm

@@ -185,6 +187,13 @@ impl SingleValidatorVotes {
}
}

fn get_latest_gossip_timestamp(&self) -> Option<UnixTimestamp> {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: think we can just name this to get_timestamp, since it's not necessarily from gossip

assert_eq!(slot, vote_later_ts.last_voted_slot().unwrap());
assert_eq!(timestamp, vote_later_ts.timestamp);

// Same vote with no ts should not override
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, spell out ts to timestamp

assert_eq!(slot, vote_later_ts.last_voted_slot().unwrap());
assert_eq!(timestamp, vote_later_ts.timestamp);

// Same vote with earlier ts should not override
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: expand ts to timestamp

@AshwinSekar AshwinSekar dismissed stale reviews from carllin, steviez, and t-nelson via be5c1b0 May 31, 2023 22:57
@AshwinSekar AshwinSekar force-pushed the refresh-votes-gossip branch from 800f5b6 to be5c1b0 Compare May 31, 2023 22:57
@AshwinSekar AshwinSekar requested a review from carllin May 31, 2023 22:58
@AshwinSekar AshwinSekar added the automerge Merge this Pull Request automatically once CI passes label May 31, 2023
@mergify mergify bot merged commit 1b79875 into solana-labs:master May 31, 2023
mergify bot pushed a commit that referenced this pull request May 31, 2023
AshwinSekar added a commit that referenced this pull request Jun 1, 2023
AshwinSekar added a commit that referenced this pull request Jun 1, 2023
@steviez steviez added the v1.16 PRs that should be backported to v1.16 label Jun 1, 2023
mergify bot pushed a commit that referenced this pull request Jun 1, 2023
AshwinSekar added a commit that referenced this pull request Jun 1, 2023
…es (backport of #31879) (#31907)

Relax gossip to banking stage filtering to allow refreshed votes (#31879)

(cherry picked from commit 1b79875)

Co-authored-by: Ashwin Sekar <[email protected]>
AshwinSekar added a commit that referenced this pull request Jun 1, 2023
…es (backport of #31879) (#31906)

Relax gossip to banking stage filtering to allow refreshed votes (#31879)

(cherry picked from commit 1b79875)

Co-authored-by: Ashwin Sekar <[email protected]>
bw-solana pushed a commit to bw-solana/solana that referenced this pull request Jan 10, 2025
…es (backport of solana-labs#31879) (solana-labs#31906)

Relax gossip to banking stage filtering to allow refreshed votes (solana-labs#31879)

(cherry picked from commit 1b79875)

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
automerge Merge this Pull Request automatically once CI passes v1.16 PRs that should be backported to v1.16
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants