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.

Sign up for GitHub

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

Jump to bottom

Fetch sysvars from invoke context for vote program #22444

Merged
merged 2 commits into from Jan 13, 2022
Merged

Fetch sysvars from invoke context for vote program #22444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ee552f5
Fetch sysvars from invoke context for vote program
jstarry Jan 12, 2022
f1d530f
Merge branch 'master' into perf/vote-sysvars
jstarry Jan 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
90 changes: 46 additions & 44 deletions programs/vote/src/vote_instruction.rs
View file
Open in desktop
Expand Up @@ -16,11 +16,14 @@ use {
feature_set,
hash::Hash,
instruction::{AccountMeta, Instruction, InstructionError},
keyed_account::{from_keyed_account, get_signers, keyed_account_at_index, KeyedAccount},
keyed_account::{
check_sysvar_keyed_account, from_keyed_account, get_signers, keyed_account_at_index,
KeyedAccount,
},
program_utils::limited_deserialize,
pubkey::Pubkey,
system_instruction,
sysvar::{self, clock::Clock, slot_hashes::SlotHashes},
sysvar::{self, clock::Clock, rent::Rent, slot_hashes::SlotHashes, Sysvar},
},
std::collections::HashSet,
thiserror::Error,
Expand Down Expand Up @@ -379,16 +382,28 @@ pub fn withdraw(

fn verify_rent_exemption(
keyed_account: &KeyedAccount,
rent_sysvar_account: &KeyedAccount,
rent: &Rent,
) -> Result<(), InstructionError> {
let rent: sysvar::rent::Rent = from_keyed_account(rent_sysvar_account)?;
if !rent.is_exempt(keyed_account.lamports()?, keyed_account.data_len()?) {
Err(InstructionError::InsufficientFunds)
} else {
Ok(())
}
}

/// This method facilitates a transition from fetching sysvars from keyed
/// accounts to fetching from the sysvar cache without breaking consensus. In
/// order to keep consistent behavior, it continues to enforce the same checks
/// as `solana_sdk::keyed_account::from_keyed_account` despite dynamically
/// loading them instead of deserializing from account data.
fn get_sysvar_with_keyed_account_check<S: Sysvar>(
keyed_account: &KeyedAccount,
invoke_context: &InvokeContext,
) -> Result<S, InstructionError> {
check_sysvar_keyed_account::<S>(keyed_account)?;
invoke_context.get_sysvar(keyed_account.unsigned_key())
}

pub fn process_instruction(
first_instruction_account: usize,
data: &[u8],
Expand All @@ -407,30 +422,24 @@ pub fn process_instruction(
let signers: HashSet<Pubkey> = get_signers(&keyed_accounts[first_instruction_account..]);
match limited_deserialize(data)? {
VoteInstruction::InitializeAccount(vote_init) => {
verify_rent_exemption(
me,
let rent: Rent = get_sysvar_with_keyed_account_check(
keyed_account_at_index(keyed_accounts, first_instruction_account + 1)?,
invoke_context,
)?;
verify_rent_exemption(me, &rent)?;
let clock: Clock = get_sysvar_with_keyed_account_check(
keyed_account_at_index(keyed_accounts, first_instruction_account + 2)?,
invoke_context,
)?;
vote_state::initialize_account(
me,
&vote_init,
&signers,
&from_keyed_account::<Clock>(keyed_account_at_index(
keyed_accounts,
first_instruction_account + 2,
)?)?,
)
vote_state::initialize_account(me, &vote_init, &signers, &clock)
}
VoteInstruction::Authorize(voter_pubkey, vote_authorize) => {
let clock: Clock = get_sysvar_with_keyed_account_check(
keyed_account_at_index(keyed_accounts, first_instruction_account + 1)?,
invoke_context,
)?;
vote_state::authorize(me, &voter_pubkey, vote_authorize, &signers, &clock)
}
VoteInstruction::Authorize(voter_pubkey, vote_authorize) => vote_state::authorize(
me,
&voter_pubkey,
vote_authorize,
&signers,
&from_keyed_account::<Clock>(keyed_account_at_index(
keyed_accounts,
first_instruction_account + 1,
)?)?,
),
VoteInstruction::UpdateValidatorIdentity => vote_state::update_validator_identity(
me,
keyed_account_at_index(keyed_accounts, first_instruction_account + 1)?.unsigned_key(),
Expand All @@ -441,19 +450,15 @@ pub fn process_instruction(
}
VoteInstruction::Vote(vote) | VoteInstruction::VoteSwitch(vote, _) => {
inc_new_counter_info!("vote-native", 1);
vote_state::process_vote(
me,
&from_keyed_account::<SlotHashes>(keyed_account_at_index(
keyed_accounts,
first_instruction_account + 1,
)?)?,
&from_keyed_account::<Clock>(keyed_account_at_index(
keyed_accounts,
first_instruction_account + 2,
)?)?,
&vote,
&signers,
)
let slot_hashes: SlotHashes = get_sysvar_with_keyed_account_check(
keyed_account_at_index(keyed_accounts, first_instruction_account + 1)?,
invoke_context,
)?;
let clock: Clock = get_sysvar_with_keyed_account_check(
keyed_account_at_index(keyed_accounts, first_instruction_account + 2)?,
invoke_context,
)?;
vote_state::process_vote(me, &slot_hashes, &clock, &vote, &signers)
}
VoteInstruction::UpdateVoteState(vote_state_update)
| VoteInstruction::UpdateVoteStateSwitch(vote_state_update, _) => {
Expand Down Expand Up @@ -522,10 +527,7 @@ mod tests {
invoke_context::{mock_process_instruction, mock_process_instruction_with_sysvars},
sysvar_cache::SysvarCache,
},
solana_sdk::{
account::{self, Account, AccountSharedData},
rent::Rent,
},
solana_sdk::account::{self, Account, AccountSharedData},
std::str::FromStr,
};

Expand Down Expand Up @@ -581,7 +583,7 @@ mod tests {
})
.collect();
let mut sysvar_cache = SysvarCache::default();
let rent = Rent::default();
let rent = Rent::free();
sysvar_cache.push_entry(sysvar::rent::id(), bincode::serialize(&rent).unwrap());
let clock = Clock::default();
sysvar_cache.push_entry(sysvar::clock::id(), bincode::serialize(&clock).unwrap());
Expand Down Expand Up @@ -820,7 +822,7 @@ mod tests {

#[test]
fn test_minimum_balance() {
let rent = solana_sdk::rent::Rent::default();
let rent = Rent::default();
let minimum_balance = rent.minimum_balance(VoteState::size_of());
// golden, may need updating when vote_state grows
assert!(minimum_balance as f64 / 10f64.powf(9.0) < 0.04)
Expand Down
17 changes: 12 additions & 5 deletions sdk/src/keyed_account.rs
View file
Open in desktop
Expand Up @@ -7,6 +7,7 @@ use {
std::{
cell::{Ref, RefCell, RefMut},
iter::FromIterator,
ops::Deref,
rc::Rc,
},
};
Expand Down Expand Up @@ -248,14 +249,20 @@ where
}
}

pub fn from_keyed_account<S: Sysvar>(
keyed_account: &crate::keyed_account::KeyedAccount,
) -> Result<S, InstructionError> {
pub fn check_sysvar_keyed_account<'a, S: Sysvar>(
Copy link
Contributor

@Lichtso Lichtso Jan 12, 2022 •
edited

Choose a reason for hiding this comment

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

Can't this be inlined at its only two use sites?
Because otherwise this will be one more public interface that needs to be removed again,
when KeyedAccounts in general are removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see any problem with removing it then. I'd like to reuse the code here because consensus depends on the checks being the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I thought you wanted to share it for the error messages to stay the same. However, you also intend to replace one of the call sites by the sysvar cache right? So, that would remove the potential error case there.

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR already changes the call site to use the sysvar cache, I'm not sure what you mean

keyed_account: &'a crate::keyed_account::KeyedAccount<'_>,
) -> Result<impl Deref<Target = AccountSharedData> + 'a, InstructionError> {
if !S::check_id(keyed_account.unsigned_key()) {
return Err(InstructionError::InvalidArgument);
}
from_account::<S, AccountSharedData>(&*keyed_account.try_account_ref()?)
.ok_or(InstructionError::InvalidArgument)
keyed_account.try_account_ref()
}

pub fn from_keyed_account<S: Sysvar>(
keyed_account: &crate::keyed_account::KeyedAccount,
) -> Result<S, InstructionError> {
let sysvar_account = check_sysvar_keyed_account::<S>(keyed_account)?;
from_account::<S, AccountSharedData>(&*sysvar_account).ok_or(InstructionError::InvalidArgument)
}

#[cfg(test)]
Expand Down