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

gossip: remove vote buffer, send all verified votes to banking_stage #2509

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

AshwinSekar
Copy link

@AshwinSekar AshwinSekar commented Aug 8, 2024

Problem

Gossip has a buffer that stores the latest vote from each validator, starting up to 3 * 512 slots before our next leader slot, and sends these votes to banking_stage once we have a working_bank.

This buffer is unnecessary, as banking_stage already holds the latest vote per validator in it's own buffer, latest_unprocessed_votes until it is time to produce a block.

Furthermore, 3 * 512 is a somewhat arbitrary number. We can place no lower bound on the block height of the leader's fork, so votes from before 3 * 512 could be included (and not expired) in the next leader block if a validator has been stuck not voting for a while.

The removal of this buffer allows latest_unprocessed_votes to maintain a consistent view of the latest vote per validator for both gossip and tpu votes, and makes reported metrics for vote ingestion more accurate.

Summary of Changes

Remove verified_vote_packets and the buffering in cluster_info_vote_listener. Forward verified votes directly to banking_stage

As explained here #2183 (comment), verified_vote_packets performed an additional check to only send votes that were part of the leader fork to banking_stage. #2465 added this logic to latest_unprocessed_votes so it still applies to these gossip votes, and now to tpu votes as well.

Fixes #2183

Copy link

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

Code changes look correct for the description you've given.
I added a comment about some behavior change that I'm unsure of but will ultimately defer to consensus team's expertice on whether it's okay to remove that behavior.

Comment on lines -298 to -299
validator_votes.insert((slot, hash), (packet_batch, signature));
if validator_votes.len() > MAX_VOTES_PER_VALIDATOR {
Copy link

Choose a reason for hiding this comment

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

So I'm a bit curious about the change as it relates to this line.

With this code we used to store up to 1000 votes per validator.
Votes on our fork would get sent to banking stage and the others remain here in case we switch forks.
But now, we bypass this and all just go directly to banking stage.
Since banking stage only stores the most recent vote (to my knowledge) we no longer have this reserve for "in case we switch forks".

Copy link
Author

Choose a reason for hiding this comment

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

This code was to deal with pre VoteStateUpdate, back when incremental votes were being sent. This business with the IncrementalVotes and FullTowerVote was to deal with the feature activation and gracefully handle a possible rollback if the feature was busted.

Previously when votes where incremental we stored up to 1000 of the last votes, as we wanted to land as many as possible to ensure that the on chain tower matched the local tower. Now we only store one vote. Even in the situation of switching forks, the most recent vote suffices.

Non modded validators do not send incremental votes, so I think it's fine to just store the most recent vote in banking stage. There is an effort I am working on to remove incremental votes from the wire all together, but for now treating them incorrectly is fine.

@AshwinSekar AshwinSekar merged commit 926e06f into anza-xyz:master Aug 13, 2024
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BankingStage: Gossip votes held
3 participants