Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

runtime: transpose VoteAccount::vote_state() return to improve ergonomics #31256

Merged
merged 1 commit into from
Apr 18, 2023
Merged

runtime: transpose VoteAccount::vote_state() return to improve ergonomics #31256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 4 additions & 5 deletions core/src/consensus.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ impl Tower {
continue;
}
trace!("{} {} with stake {}", vote_account_pubkey, key, voted_stake);
let mut vote_state = match account.vote_state().as_ref() {
let mut vote_state = match account.vote_state().cloned() {
Err(_) => {
datapoint_warn!(
"tower_warn",
Expand All @@ -314,7 +314,7 @@ impl Tower {
);
continue;
}
Ok(vote_state) => vote_state.clone(),
Ok(vote_state) => vote_state,
};
for vote in &vote_state.votes {
lockout_intervals
Expand Down Expand Up @@ -1284,9 +1284,8 @@ impl Tower {
if let Some(vote_account) = bank.get_vote_account(vote_account_pubkey) {
self.vote_state = vote_account
.vote_state()
.as_ref()
.expect("vote_account isn't a VoteState?")
.clone();
.cloned()
.expect("vote_account isn't a VoteState?");
self.initialize_root(root);
self.initialize_lockouts(|v| v.slot() > root);
trace!(
Expand Down
4 changes: 1 addition & 3 deletions core/src/replay_stage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2894,9 +2894,7 @@ impl ReplayStage {
Tower::is_direct_vote_state_update_enabled(bank),
bank.get_vote_account(my_vote_pubkey),
) {
if let Some(mut bank_vote_state) =
vote_account.vote_state().as_ref().ok().cloned()
{
if let Ok(mut bank_vote_state) = vote_account.vote_state().cloned() {
behzadnouri marked this conversation as resolved.
Show resolved Hide resolved
if bank_vote_state.last_voted_slot()
> tower.vote_state.last_voted_slot()
{
Expand Down
4 changes: 2 additions & 2 deletions ledger-tool/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,7 +520,7 @@ fn graph_forks(bank_forks: &BankForks, config: &GraphConfig) -> String {
.sum();
for (stake, vote_account) in bank.vote_accounts().values() {
let vote_state = vote_account.vote_state();
let vote_state = vote_state.as_ref().unwrap_or(&default_vote_state);
let vote_state = vote_state.unwrap_or(&default_vote_state);
if let Some(last_vote) = vote_state.votes.iter().last() {
let entry = last_votes.entry(vote_state.node_pubkey).or_insert((
last_vote.slot(),
Expand Down Expand Up @@ -561,7 +561,7 @@ fn graph_forks(bank_forks: &BankForks, config: &GraphConfig) -> String {
loop {
for (_, vote_account) in bank.vote_accounts().values() {
let vote_state = vote_account.vote_state();
let vote_state = vote_state.as_ref().unwrap_or(&default_vote_state);
let vote_state = vote_state.unwrap_or(&default_vote_state);
if let Some(last_vote) = vote_state.votes.iter().last() {
let validator_votes = all_votes.entry(vote_state.node_pubkey).or_default();
validator_votes
Expand Down
2 changes: 1 addition & 1 deletion rpc/src/rpc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -939,7 +939,7 @@ impl JsonRpcRequestProcessor {
}

let vote_state = account.vote_state();
let vote_state = vote_state.as_ref().unwrap_or(&default_vote_state);
let vote_state = vote_state.unwrap_or(&default_vote_state);
let last_vote = if let Some(vote) = vote_state.votes.iter().last() {
vote.slot()
} else {
Expand Down
6 changes: 3 additions & 3 deletions runtime/src/bank.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ use {
collections::{HashMap, HashSet},
convert::{TryFrom, TryInto},
fmt, mem,
ops::{AddAssign, Deref, RangeInclusive},
ops::{AddAssign, RangeInclusive},
path::PathBuf,
rc::Rc,
sync::{
Expand Down Expand Up @@ -2759,8 +2759,8 @@ impl Bank {
invalid_vote_keys.insert(vote_pubkey, InvalidCacheEntryReason::WrongOwner);
return None;
}
let vote_state = match vote_account.vote_state().deref() {
Ok(vote_state) => vote_state.clone(),
let vote_state = match vote_account.vote_state().cloned() {
behzadnouri marked this conversation as resolved.
Show resolved Hide resolved
Ok(vote_state) => vote_state,
Err(_) => {
invalid_vote_keys.insert(vote_pubkey, InvalidCacheEntryReason::BadState);
return None;
Expand Down
25 changes: 10 additions & 15 deletions runtime/src/vote_account.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,13 @@ impl VoteAccount {
self.0.account.owner()
}

pub fn vote_state(&self) -> &Result<VoteState, Error> {
pub fn vote_state(&self) -> Result<&VoteState, &Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the tradeoff is that this return type is ugly/weird.
Is the .as_ref() at call-sites really a big deal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i would argue that &Result<T, E> is uglier than Result<&T, &E>. we prefer Option<&T> to &Option<T> in most cases elsewhere to avoid the eye-twitch-inducing &None. ofc, this would all be better if it were Result<&T, E>, but i don't think we can get to that without jumping through hoops

// VoteState::deserialize deserializes a VoteStateVersions and then
// calls VoteStateVersions::convert_to_current.
self.0
.vote_state
.get_or_init(|| VoteState::deserialize(self.0.account.data()).map_err(Error::from))
.as_ref()
}

pub(crate) fn is_deserialized(&self) -> bool {
Expand All @@ -79,7 +80,7 @@ impl VoteAccount {

/// VoteState.node_pubkey of this vote-account.
pub fn node_pubkey(&self) -> Option<Pubkey> {
Some(self.vote_state().as_ref().ok()?.node_pubkey)
Some(self.vote_state().ok()?.node_pubkey)
}
}

Expand Down Expand Up @@ -395,17 +396,17 @@ mod tests {
let lamports = account.lamports();
let vote_account = VoteAccount::try_from(account).unwrap();
assert_eq!(lamports, vote_account.lamports());
assert_eq!(vote_state, *vote_account.vote_state().as_ref().unwrap());
assert_eq!(vote_state, *vote_account.vote_state().unwrap());
// 2nd call to .vote_state() should return the cached value.
assert_eq!(vote_state, *vote_account.vote_state().as_ref().unwrap());
assert_eq!(vote_state, *vote_account.vote_state().unwrap());
}

#[test]
fn test_vote_account_serialize() {
let mut rng = rand::thread_rng();
let (account, vote_state) = new_rand_vote_account(&mut rng, None);
let vote_account = VoteAccount::try_from(account.clone()).unwrap();
assert_eq!(vote_state, *vote_account.vote_state().as_ref().unwrap());
assert_eq!(vote_state, *vote_account.vote_state().unwrap());
// Assert than VoteAccount has the same wire format as Account.
assert_eq!(
bincode::serialize(&account).unwrap(),
Expand All @@ -419,29 +420,23 @@ mod tests {
let (account, vote_state) = new_rand_vote_account(&mut rng, None);
let data = bincode::serialize(&account).unwrap();
let vote_account = VoteAccount::try_from(account).unwrap();
assert_eq!(vote_state, *vote_account.vote_state().as_ref().unwrap());
assert_eq!(vote_state, *vote_account.vote_state().unwrap());
let other_vote_account: VoteAccount = bincode::deserialize(&data).unwrap();
assert_eq!(vote_account, other_vote_account);
assert_eq!(
vote_state,
*other_vote_account.vote_state().as_ref().unwrap()
);
assert_eq!(vote_state, *other_vote_account.vote_state().unwrap());
}

#[test]
fn test_vote_account_round_trip() {
let mut rng = rand::thread_rng();
let (account, vote_state) = new_rand_vote_account(&mut rng, None);
let vote_account = VoteAccount::try_from(account).unwrap();
assert_eq!(vote_state, *vote_account.vote_state().as_ref().unwrap());
assert_eq!(vote_state, *vote_account.vote_state().unwrap());
let data = bincode::serialize(&vote_account).unwrap();
let other_vote_account: VoteAccount = bincode::deserialize(&data).unwrap();
// Assert that serialize->deserialized returns the same VoteAccount.
assert_eq!(vote_account, other_vote_account);
assert_eq!(
vote_state,
*other_vote_account.vote_state().as_ref().unwrap()
);
assert_eq!(vote_state, *other_vote_account.vote_state().unwrap());
}

#[test]
Expand Down