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

fix: send votes to the immediate next leader #2607

Merged
merged 2 commits into from
Aug 16, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions core/src/next_leader.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,40 @@
use {
itertools::Itertools,
solana_gossip::{cluster_info::ClusterInfo, contact_info::ContactInfo},
solana_poh::poh_recorder::PohRecorder,
solana_sdk::{clock::FORWARD_TRANSACTIONS_TO_LEADER_AT_SLOT_OFFSET, pubkey::Pubkey},
std::{net::SocketAddr, sync::RwLock},
};

/// Returns a list of tpu vote sockets for the leaders of the next N fanout
/// slots. Leaders are deduped but the resulting list could have duplicate
/// sockets if two different leaders share the same tpu vote socket.
pub(crate) fn upcoming_leader_tpu_vote_sockets(
cluster_info: &ClusterInfo,
poh_recorder: &RwLock<PohRecorder>,
fanout_slots: usize,

Choose a reason for hiding this comment

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

Maybe just use u64 here to be consistent with leader_after_n_slots api, and avoid as u64 below.

) -> Vec<SocketAddr> {
let upcoming_leaders = {
AshwinSekar marked this conversation as resolved.
Show resolved Hide resolved
let mut upcoming_leaders = Vec::with_capacity(fanout_slots);
let poh_recorder = poh_recorder.read().unwrap();
for n_slots in 1..=fanout_slots {
upcoming_leaders.push(poh_recorder.leader_after_n_slots(n_slots as u64));

Choose a reason for hiding this comment

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

None values could be filtered out earlier here, for example:

upcoming_leaders.extend((1..=fanout_slots).flat_map(...))

also deduping (instead of unique) here would probably be fine.

Copy link

Choose a reason for hiding this comment

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

any reason not to use filter_map here over flat_map

Choose a reason for hiding this comment

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

yeah i guess both would work

}
upcoming_leaders
};

upcoming_leaders
.into_iter()
.flatten()
.unique()

Choose a reason for hiding this comment

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

It will probably be enough to just use dedup instead of unique here:
https://docs.rs/itertools/latest/itertools/trait.Itertools.html#method.dedup

.filter_map(|leader_pubkey| {
cluster_info
.lookup_contact_info(&leader_pubkey, ContactInfo::tpu_vote)?
.ok()
})
.collect()
}

pub(crate) fn next_leader_tpu_vote(
cluster_info: &ClusterInfo,
poh_recorder: &RwLock<PohRecorder>,
Expand Down
21 changes: 16 additions & 5 deletions core/src/voting_service.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use {
crate::{
consensus::tower_storage::{SavedTowerVersions, TowerStorage},
next_leader::next_leader_tpu_vote,
next_leader::upcoming_leader_tpu_vote_sockets,
},
crossbeam_channel::Receiver,
solana_gossip::cluster_info::ClusterInfo,
Expand Down Expand Up @@ -78,12 +78,23 @@ impl VotingService {
trace!("{measure}");
}

let _ = cluster_info.send_transaction(
vote_op.tx(),
next_leader_tpu_vote(cluster_info, poh_recorder)
.map(|(_pubkey, target_addr)| target_addr),
// Attempt to send our vote transaction to the leaders for the next few slots
const UPCOMING_LEADER_FANOUT_SLOTS: usize = 2;

Choose a reason for hiding this comment

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

I would personally define this constant equal to FORWARD_TRANSACTIONS_TO_LEADER_AT_SLOT_OFFSET, for sake of consistency, and maybe a const_assert that is equal to 2.

let upcoming_leader_sockets = upcoming_leader_tpu_vote_sockets(
cluster_info,
poh_recorder,
UPCOMING_LEADER_FANOUT_SLOTS,
);

if !upcoming_leader_sockets.is_empty() {
for tpu_vote_socket in upcoming_leader_sockets {
let _ = cluster_info.send_transaction(vote_op.tx(), Some(tpu_vote_socket));
}
} else {
// Send to our own tpu vote socket if we cannot find a leader to send to
let _ = cluster_info.send_transaction(vote_op.tx(), None);
}

match vote_op {
VoteOp::PushVote {
tx, tower_slots, ..
Expand Down