-
Notifications
You must be signed in to change notification settings - Fork 197
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
banking_stage: do not drain votes that cannot land on our leader fork #2465
Conversation
@@ -115,9 +126,6 @@ impl LatestValidatorVotePacket { | |||
} | |||
} | |||
|
|||
// TODO: replace this with rand::seq::index::sample_weighted once we can update rand to 0.8+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this comment because it is no longer accurate, we cannot use sample_weighted
(see #2072 (comment))
.filter_map(|pubkey| { | ||
self.get_entry(pubkey).and_then(|lock| { | ||
let mut latest_vote = lock.write().unwrap(); | ||
if !Self::is_valid_for_our_fork(&latest_vote, &slot_hashes) { | ||
return None; | ||
} | ||
latest_vote.take_vote().map(|vote| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
side note: do we ever clear the validator keys from self.latest_votes_per_pubkey
after we've taken thte vote?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't, see #2079
I think on epoch boundary we can probably remove unstaked nodes? What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah after a root is made in the new epoch, that would make sense
.filter_map(|pubkey| { | ||
self.get_entry(pubkey).and_then(|lock| { | ||
let mut latest_vote = lock.write().unwrap(); | ||
if !Self::is_valid_for_our_fork(&latest_vote, &slot_hashes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought about throwing a small cache in here of (vote, hash) -> bool
results in case number of validators every blows up or slothashes gets large, but even at 432,000 SlotHashes and 10,000 validators, binary searching the SlotHashes would only take 20,000 lookups
First part of addressing #2183
Problem
Votes from the banking stage buffer will be drained to the leader bank regardless of whether they were casted for the fork the leader bank is on.
This leads us to draining votes that we know will fail. This could also lead to votes being lost if a leader resets and decides to build a later leader block off a different fork.
Summary of Changes
Perform the slot hashes check when we drain votes to the bank.