This repository was archived by the owner on Jan 22, 2025. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Further expand last_voted_slot terminology #10747
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -185,9 +185,9 @@ impl Tower { | |
| } | ||
| let start_root = vote_state.root_slot; | ||
|
|
||
| // Add the latest vote to update the `heaviest_subtree_fork_choice` | ||
| if let Some(latest_vote) = vote_state.votes.back() { | ||
| pubkey_votes.push((key, latest_vote.slot)); | ||
| // Add the last vote to update the `heaviest_subtree_fork_choice` | ||
| if let Some(last_voted_slot) = vote_state.last_voted_slot() { | ||
| pubkey_votes.push((key, last_voted_slot)); | ||
| } | ||
|
|
||
| vote_state.process_slot_vote_unchecked(bank_slot); | ||
|
|
@@ -265,45 +265,45 @@ impl Tower { | |
|
|
||
| fn new_vote( | ||
| local_vote_state: &VoteState, | ||
| slot: u64, | ||
| slot: Slot, | ||
| hash: Hash, | ||
| last_bank_slot: Option<Slot>, | ||
| last_voted_slot_in_bank: Option<Slot>, | ||
| ) -> (Vote, usize) { | ||
| let mut local_vote_state = local_vote_state.clone(); | ||
| let vote = Vote::new(vec![slot], hash); | ||
| local_vote_state.process_vote_unchecked(&vote); | ||
| let slots = if let Some(last_bank_slot) = last_bank_slot { | ||
| let slots = if let Some(last_voted_slot_in_bank) = last_voted_slot_in_bank { | ||
| local_vote_state | ||
| .votes | ||
| .iter() | ||
| .map(|v| v.slot) | ||
| .skip_while(|s| *s <= last_bank_slot) | ||
| .skip_while(|s| *s <= last_voted_slot_in_bank) | ||
| .collect() | ||
| } else { | ||
| local_vote_state.votes.iter().map(|v| v.slot).collect() | ||
| }; | ||
| trace!( | ||
| "new vote with {:?} {:?} {:?}", | ||
| last_bank_slot, | ||
| last_voted_slot_in_bank, | ||
| slots, | ||
| local_vote_state.votes | ||
| ); | ||
| (Vote::new(slots, hash), local_vote_state.votes.len() - 1) | ||
| } | ||
|
|
||
| fn last_bank_vote(bank: &Bank, vote_account_pubkey: &Pubkey) -> Option<Slot> { | ||
| fn last_voted_slot_in_bank(bank: &Bank, vote_account_pubkey: &Pubkey) -> Option<Slot> { | ||
| let vote_account = bank.vote_accounts().get(vote_account_pubkey)?.1.clone(); | ||
| let bank_vote_state = VoteState::deserialize(&vote_account.data).ok()?; | ||
| bank_vote_state.votes.iter().map(|v| v.slot).last() | ||
| bank_vote_state.last_voted_slot() | ||
| } | ||
|
|
||
| pub fn new_vote_from_bank(&self, bank: &Bank, vote_account_pubkey: &Pubkey) -> (Vote, usize) { | ||
| let last_vote = Self::last_bank_vote(bank, vote_account_pubkey); | ||
| Self::new_vote(&self.lockouts, bank.slot(), bank.hash(), last_vote) | ||
| let voted_slot = Self::last_voted_slot_in_bank(bank, vote_account_pubkey); | ||
| Self::new_vote(&self.lockouts, bank.slot(), bank.hash(), voted_slot) | ||
| } | ||
|
|
||
| pub fn record_bank_vote(&mut self, vote: Vote) -> Option<Slot> { | ||
| let slot = *vote.slots.last().unwrap_or(&0); | ||
| let slot = vote.last_voted_slot().unwrap_or(0); | ||
| trace!("{} record_vote for {}", self.node_pubkey, slot); | ||
| let root_slot = self.lockouts.root_slot; | ||
| self.lockouts.process_vote_unchecked(&vote); | ||
|
|
@@ -344,19 +344,19 @@ impl Tower { | |
| self.lockouts.root_slot | ||
| } | ||
|
|
||
| // a slot is not recent if it's older than the newest vote we have | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inverted the description for more simpler definition. Also, old description should be older than or equal to... |
||
| pub fn is_recent(&self, slot: u64) -> bool { | ||
| if let Some(last_vote) = self.lockouts.votes.back() { | ||
| if slot <= last_vote.slot { | ||
| // a slot is recent if it's newer than the last vote we have | ||
| pub fn is_recent(&self, slot: Slot) -> bool { | ||
| if let Some(last_voted_slot) = self.lockouts.last_voted_slot() { | ||
| if slot <= last_voted_slot { | ||
| return false; | ||
| } | ||
| } | ||
| true | ||
| } | ||
|
|
||
| pub fn has_voted(&self, slot: u64) -> bool { | ||
| pub fn has_voted(&self, slot: Slot) -> bool { | ||
| for vote in &self.lockouts.votes { | ||
| if vote.slot == slot { | ||
| if slot == vote.slot { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -283,11 +283,10 @@ impl VoteState { | |
| let mut i = 0; // index into the vote's slots | ||
| let mut j = slot_hashes.len(); // index into the slot_hashes | ||
| while i < vote.slots.len() && j > 0 { | ||
| // find the most recent "new" slot in the vote | ||
| // find the last slot in the vote | ||
| if self | ||
| .votes | ||
| .back() | ||
| .map_or(false, |old_vote| old_vote.slot >= vote.slots[i]) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reversed the condition operands for readability. Loop variable dependant expression should be first to be like a subject in a sentence. |
||
| .last_voted_slot() | ||
| .map_or(false, |last_voted_slot| vote.slots[i] <= last_voted_slot) | ||
| { | ||
| i += 1; | ||
| continue; | ||
|
|
@@ -341,9 +340,8 @@ impl VoteState { | |
| pub fn process_slot(&mut self, slot: Slot, epoch: Epoch) { | ||
| // Ignore votes for slots earlier than we already have votes for | ||
| if self | ||
| .votes | ||
| .back() | ||
| .map_or(false, |old_vote| old_vote.slot >= slot) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reversed the condition operands for readability. function arguments should be first to be like a subject in a sentence. |
||
| .last_voted_slot() | ||
| .map_or(false, |last_voted_slot| slot <= last_voted_slot) | ||
| { | ||
| return; | ||
| } | ||
|
|
@@ -410,6 +408,14 @@ impl VoteState { | |
| } | ||
| } | ||
|
|
||
| pub fn last_lockout(&self) -> Option<&Lockout> { | ||
| self.votes.back() | ||
| } | ||
|
|
||
| pub fn last_voted_slot(&self) -> Option<Slot> { | ||
| self.last_lockout().map(|v| v.slot) | ||
| } | ||
|
|
||
| fn current_epoch(&self) -> Epoch { | ||
| if self.epoch_credits.is_empty() { | ||
| 0 | ||
|
|
@@ -513,7 +519,7 @@ impl VoteState { | |
|
|
||
| fn pop_expired_votes(&mut self, slot: Slot) { | ||
| loop { | ||
| if self.votes.back().map_or(false, |v| v.is_expired(slot)) { | ||
| if self.last_lockout().map_or(false, |v| v.is_expired(slot)) { | ||
| self.votes.pop_back(); | ||
| } else { | ||
| break; | ||
|
|
@@ -1279,7 +1285,8 @@ mod tests { | |
| // the root_slot should change to the | ||
| // second vote | ||
| let top_vote = vote_state.votes.front().unwrap().slot; | ||
| vote_state.process_slot_vote_unchecked(vote_state.votes.back().unwrap().expiration_slot()); | ||
| vote_state | ||
| .process_slot_vote_unchecked(vote_state.last_lockout().unwrap().expiration_slot()); | ||
| assert_eq!(Some(top_vote), vote_state.root_slot); | ||
|
|
||
| // Expire everything except the first vote | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I don't know how much rustc optimizer is clever. But I simply changed the algorithm from map-then-last to last-then-map, considering this would be more straight-forward in this case: https://github.com/solana-labs/solana/pull/10747/files#diff-3666262446ea129567c6d96960c7e23cR411-R418
New code might eliminate this unneeded iteration here.