Only take the latest vote for each validator in banking stage

[AshwinSekar]

Problem

The new votes are not incremental so there's no value in storing and processing intermediate transactions.

Summary of Changes

Once the transactions are deserialized, for each batch filter out extraneous votes.

Feature gate issue: #24717

[bji]

I believe that the effects of VoteStateUpdate transactions (it's getting harder and harder to call these "votes" anymore given how for this is straying from the concept of voting!) on credits earned may in fact be incremental, at least to some degree.

After my recent change, vote credits are awarded by counting votes present in the previous vote state that were retired in the new vote state. But if you collapse a bunch of these vote states together, you miss intermediate states which would have retired votes.

Have you considered how this change will alter the incentives for sending VoteStateUpdate transactions?

Actually as I think about this more, maybe credits are not affected by this?

I think if there is an effect on credits it would only occur if there were a large number of very delayed VoteStateUpdate transactions from a single validator all landing in the same slot. In that case I think that it's possible that slots may be missed but it would have to be a span of at least 32 slots missed via intermediate VoteStateUpdate eliding, and that seems really unlikely.

[AshwinSekar]

it's getting harder and harder to call these "votes" anymore given how for this is straying from the concept of voting!

I disagree, each VoteStateUpdate is the result of one vote. The difference is it contains the full vote state rather than the incremental votes we previously had. If anything the old schema was less representative of a validators votes as any dropped/rearranged votes would result in a divergence impacting all further votes until lockout expiry.

After my recent change, vote credits are awarded by counting votes present in the previous vote state that were retired in the new vote state. But if you collapse a bunch of these vote states together, you miss intermediate states which would have retired votes.

If I understand correctly "retired" here means any votes in the old state that are older than the root of the new state. Since roots are monotonically increasing, the end result should be the same.

but it would have to be a span of at least 32 slots missed via intermediate VoteStateUpdate eliding, and that seems really unlikely.

For this to happen > 32 VoteStateUpdates would have to arrive and be processed in the same batch for the same bank. Also this already can happen with your credit scheme if a validator is partitioned away for 32 votes.

[carllin]

it would be nice to include this _filter_extraneous_votes_time in the slot_metrics_tracker

[carllin]

This logic is correct I think, but perhaps in the wrong place.

So the issue here is that where filter_extraneous_votes is currently called, it will only analyze across the 128 packets that are passed to process_packets_transactions from the MinMaxHeap of pending work here:

solana/core/src/banking_stage.rs

Lines 665 to 689 in 980d056

	let mut retryable_packets: MinMaxHeap<Rc<ImmutableDeserializedPacket>> = retryable_packets
	.drain_desc()
	.chunks(num_packets_to_process_per_iteration)
	.into_iter()
	.flat_map(|packets_to_process| {
	let packets_to_process = packets_to_process.into_iter().collect_vec();
	// TODO: Right now we iterate through buffer and try the highest weighted transaction once
	// but we should retry the highest weighted transactions more often.
	let (bank_start, poh_recorder_lock_time) = measure!(
	poh_recorder.lock().unwrap().bank_start(),
	"poh_recorder_lock",
	);
	slot_metrics_tracker.increment_consume_buffered_packets_poh_recorder_lock_us(
	poh_recorder_lock_time.as_us(),
	);
	let packets_to_process_len = packets_to_process.len();
	if let Some(BankStart {
	working_bank,
	bank_creation_time,
	}) = bank_start
	{
	let (process_transactions_summary, process_packets_transactions_time) =
	measure!(
	Self::process_packets_transactions(

I think what we want instead, is to only keep the latest vote for each validator across the entire buffer.

I think to do this properly is quite involved:

1. I think for voting threads, banking stage should ideally use a different data structure to track the unprocessed work. I would imagine it could just be a latest_votes: HashMap<Pubkey, Packet> where we keep track of the latest packet.

2. We update latest_votes when we get a new vote transaction for a particular validator. It would be awesome here if the vote transaction format supported letting us quickly parse out what the latest vote here was, kind of like how we can quickly determine if a vote is a simple vote transaction: https://github.com/solana-labs/solana/blame/980d056ce196ec21acdb983afe0ecfd6567aabb4/perf/src/sigverify.rs#L423-L429. That way we can quickly figure out when adding to the pending buffer whether this vote should be added/dropped

3. With this latest_votes structure, we can then also always prioritize the highest staked validators, and drop votes from non-staked validators

cc @taozhu-chicago. For context this PR is for processing a new type of vote where only the latest vote is necessary to update the entire vote account.

[carllin]

That being said, a easy first win would be to optimize the gossip vote sender to just send the latest vote per validator here:

solana/core/src/verified_vote_packets.rs

Lines 81 to 128 in 980d056

	let validator_votes = self
	.verified_vote_packets
	.0
	.get(&vote_account_key)
	.and_then(|validator_gossip_votes| {
	// Fetch the validator's vote state from the bank
	self.my_leader_bank
	.vote_accounts()
	.get(&vote_account_key)
	.and_then(|(_stake, vote_account)| {
	vote_account.vote_state().as_ref().ok().map(|vote_state| {
	let start_vote_slot =
	vote_state.last_voted_slot().map(|x| x + 1).unwrap_or(0);
	// Filter out the votes that are outdated
	validator_gossip_votes
	.range((start_vote_slot, Hash::default())..)
	.filter_map(|((slot, hash), (packet, tx_signature))| {
	if self.previously_sent_to_bank_votes.contains(tx_signature)
	{
	return None;
	}
	// Don't send the same vote to the same bank multiple times
	self.previously_sent_to_bank_votes.insert(*tx_signature);
	// Filter out votes on the wrong fork (or too old to be)
	// on this fork
	if self
	.slot_hashes
	.get(slot)
	.map(|found_hash| found_hash == hash)
	.unwrap_or(false)
	{
	Some(packet.clone())
	} else {
	None
	}
	})
	.collect::<Vec<PacketBatch>>()
	})
	})
	});
	if let Some(validator_votes) = validator_votes {
	if !validator_votes.is_empty() {
	return Some(validator_votes);
	}
	}
	}
	None
	}

😃

Edit: Oh I see you beat me to the punch here: https://github.com/solana-labs/solana/pull/25934/files

[tao-stones]

I'd second the thought of putting vote into its own processing within banking stage. It'd be a project by itself, but I can see benefits.

[AshwinSekar]

We update latest_votes when we get a new vote transaction for a particular validator. It would be awesome here if the vote transaction format supported letting us quickly parse out what the latest vote here was, kind of like how we can quickly determine if a vote is a simple vote transaction: https://github.com/solana-labs/solana/blame/980d056ce196ec21acdb983afe0ecfd6567aabb4/perf/src/sigverify.rs#L423-L429. That way we can quickly figure out when adding to the pending buffer whether this vote should be added/dropped

Should it be filtered out in sigverify before it even makes it to banking stage?

[AshwinSekar]

I can use a thread safe data structure to keep track of latest slot voted per validator and have the banking stage threads drop extraneous votes, however it seems like it would be nice if we could drop them earlier?

Problem is we need to be able to find the slot before we deserialize the transaction...

[tao-stones]

Filtering votes here within a batch works, functionally, but agree with @carllin that ideally the filtering should be done once on the packets in buffer. Perhaps can add additional plumbing to mark deserialized packet in unprocessed_packet_batches as discard if its sanitized transaction is identified as extraneous vote.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[AshwinSekar]

Closing this and picking it up in #26722