fix: send votes to the immediate next leader #2607
Conversation
|
Backports to the stable branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. |
|
Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis. |
jstarry
force-pushed
the
fix/vote-service-leader
branch
from
5b6c3bd
to
47472a1
Compare
Yeah I was thinking we could use blockstore to find in-progress or potential descendants of the last voted slot. This change felt like a good quick fix though. |
Hi @jstarry, Is this not also applicable for all the transactions? I was assuming the next leader offset logic was the same for all of them so this improvement would apply for everyone. agave/sdk/program/src/clock.rs Line 141 in 5263c9d Regards and thanks, |
core/src/next_leader.rs
Outdated
| upcoming_leaders | ||
| .into_iter() | ||
| .flatten() | ||
| .unique() |
There was a problem hiding this comment.
It will probably be enough to just use dedup instead of unique here:
https://docs.rs/itertools/latest/itertools/trait.Itertools.html#method.dedup
core/src/next_leader.rs
Outdated
| 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)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
any reason not to use filter_map here over flat_map
core/src/next_leader.rs
Outdated
| pub(crate) fn upcoming_leader_tpu_vote_sockets( | ||
| cluster_info: &ClusterInfo, | ||
| poh_recorder: &RwLock<PohRecorder>, | ||
| fanout_slots: usize, |
There was a problem hiding this comment.
Maybe just use u64 here to be consistent with leader_after_n_slots api, and avoid as u64 below.
core/src/voting_service.rs
Outdated
| 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; |
There was a problem hiding this comment.
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.
@StaRkeSolanaValidator yes the offset logic is also the same for general tx forwarding and could benefit from a similar fix. Keeping this PR scoped to votes right now because improving vote latency is a priority given TVC will be enabled soon. |
jstarry
dismissed stale reviews from carllin, behzadnouri, and AshwinSekar
via
6e918e4
* fix: send votes to the immediate next leader * feedback (cherry picked from commit f4e2fa9) # Conflicts: # core/src/next_leader.rs
* fix: send votes to the immediate next leader * feedback (cherry picked from commit f4e2fa9)
Problem
Votes are always sent to the "forward leader" which is defined as the leader of the slot which is 2 slots beyond our latest poh clock slot. This means that a vote for the third slot of a given leader's slot span will always be sent to the following leader for inclusion in the first slot of that leader's slot span. This adds an unnecessary slot of latency to votes because the vote could potentially land in current leader's fourth slot.
Summary of Changes
Fixes #