Prevent rent-paying account creation (#22292)

* Fixup typo

* Add new feature

* Add new TransactionError

* Add framework for checking account state before and after transaction processing

* Fail transactions that leave new rent-paying accounts

* Only check rent-state of writable tx accounts

* Review comments: combine process_result success behavior; log and metrics before feature activation

* Fix tests that assume rent-exempt accounts are okay

* Remove test no longer relevant

* Remove native/sysvar special case

* Move metrics submission to report legacy->legacy rent paying transitions as well
This commit is contained in:
Tyera Eulberg
2022-01-11 11:32:25 -07:00
committed by GitHub
parent a49ef49f87
commit 637e366b18
24 changed files with 819 additions and 179 deletions

View File

@ -19,6 +19,7 @@ bzip2 = "0.4.3"
dashmap = { version = "5.0.0", features = ["rayon", "raw-api"] }
crossbeam-channel = "0.5"
dir-diff = "0.3.2"
enum-iterator = "0.7.0"
flate2 = "1.0.22"
fnv = "1.0.7"
index_list = "0.2.7"

View File

@ -0,0 +1,135 @@
use {
enum_iterator::IntoEnumIterator,
log::*,
solana_sdk::{
account::{AccountSharedData, ReadableAccount},
rent::Rent,
transaction::{Result, TransactionError},
transaction_context::TransactionContext,
},
};
#[derive(Debug, PartialEq, IntoEnumIterator)]
pub(crate) enum RentState {
Uninitialized, // account.lamports == 0
RentPaying, // 0 < account.lamports < rent-exempt-minimum
RentExempt, // account.lamports >= rent-exempt-minimum
}
impl RentState {
pub(crate) fn from_account(account: &AccountSharedData, rent: &Rent) -> Self {
if account.lamports() == 0 {
Self::Uninitialized
} else if !rent.is_exempt(account.lamports(), account.data().len()) {
Self::RentPaying
} else {
Self::RentExempt
}
}
pub(crate) fn transition_allowed_from(&self, pre_rent_state: &RentState) -> bool {
// Only a legacy RentPaying account may end in the RentPaying state after message processing
!(self == &Self::RentPaying && pre_rent_state != &Self::RentPaying)
}
}
pub(crate) fn submit_rent_state_metrics(pre_rent_state: &RentState, post_rent_state: &RentState) {
match (pre_rent_state, post_rent_state) {
(&RentState::Uninitialized, &RentState::RentPaying) => {
inc_new_counter_info!("rent_paying_err-new_account", 1);
}
(&RentState::RentPaying, &RentState::RentPaying) => {
inc_new_counter_info!("rent_paying_ok-legacy", 1);
}
(_, &RentState::RentPaying) => {
inc_new_counter_info!("rent_paying_err-other", 1);
}
_ => {}
}
}
pub(crate) fn check_rent_state(
pre_rent_state: Option<&RentState>,
post_rent_state: Option<&RentState>,
transaction_context: &TransactionContext,
index: usize,
) -> Result<()> {
if let Some((pre_rent_state, post_rent_state)) = pre_rent_state.zip(post_rent_state) {
submit_rent_state_metrics(pre_rent_state, post_rent_state);
if !post_rent_state.transition_allowed_from(pre_rent_state) {
debug!(
"Account {:?} not rent exempt, state {:?}",
transaction_context.get_key_of_account_at_index(index),
transaction_context.get_account_at_index(index).borrow(),
);
return Err(TransactionError::InvalidRentPayingAccount);
}
}
Ok(())
}
#[cfg(test)]
mod tests {
use {super::*, solana_sdk::pubkey::Pubkey};
#[test]
fn test_from_account() {
let program_id = Pubkey::new_unique();
let uninitialized_account = AccountSharedData::new(0, 0, &Pubkey::default());
let account_data_size = 100;
let rent = Rent::free();
let rent_exempt_account = AccountSharedData::new(1, account_data_size, &program_id); // if rent is free, all accounts with non-zero lamports and non-empty data are rent-exempt
assert_eq!(
RentState::from_account(&uninitialized_account, &rent),
RentState::Uninitialized
);
assert_eq!(
RentState::from_account(&rent_exempt_account, &rent),
RentState::RentExempt
);
let rent = Rent::default();
let rent_minimum_balance = rent.minimum_balance(account_data_size);
let rent_paying_account = AccountSharedData::new(
rent_minimum_balance.saturating_sub(1),
account_data_size,
&program_id,
);
let rent_exempt_account = AccountSharedData::new(
rent.minimum_balance(account_data_size),
account_data_size,
&program_id,
);
assert_eq!(
RentState::from_account(&uninitialized_account, &rent),
RentState::Uninitialized
);
assert_eq!(
RentState::from_account(&rent_paying_account, &rent),
RentState::RentPaying
);
assert_eq!(
RentState::from_account(&rent_exempt_account, &rent),
RentState::RentExempt
);
}
#[test]
fn test_transition_allowed_from() {
for post_rent_state in RentState::into_enum_iter() {
for pre_rent_state in RentState::into_enum_iter() {
if post_rent_state == RentState::RentPaying
&& pre_rent_state != RentState::RentPaying
{
assert!(!post_rent_state.transition_allowed_from(&pre_rent_state));
} else {
assert!(post_rent_state.transition_allowed_from(&pre_rent_state));
}
}
}
}
}

View File

@ -213,6 +213,7 @@ pub struct ErrorCounters {
pub invalid_program_for_execution: usize,
pub not_allowed_during_cluster_maintenance: usize,
pub invalid_writable_account: usize,
pub invalid_rent_paying_account: usize,
}
#[derive(Debug, Default, Clone, Copy)]

View File

@ -160,6 +160,8 @@ use {
},
};
mod transaction_account_state_info;
pub const SECONDS_PER_YEAR: f64 = 365.25 * 24.0 * 60.0 * 60.0;
pub const MAX_LEADER_SCHEDULE_STAKES: Epoch = 5;
@ -214,7 +216,7 @@ impl RentDebits {
}
type BankStatusCache = StatusCache<Result<()>>;
#[frozen_abi(digest = "2r36f5cfgP7ABq7D3kRkRfQZWdggGFUnnhwTrVEWhoTC")]
#[frozen_abi(digest = "Gr2MTwWyUdkbF6FxM6TSwGaC3c5buUirHmh64oAPgg7Z")]
pub type BankSlotDelta = SlotDelta<Result<()>>;
// Eager rent collection repeats in cyclic manner.
@ -3697,6 +3699,9 @@ impl Bank {
compute_budget.max_invoke_depth.saturating_add(1),
);
let pre_account_state_info =
self.get_transaction_account_state_info(&transaction_context, tx.message());
let instruction_recorder = if enable_cpi_recording {
Some(InstructionRecorder::new_ref(
tx.message().instructions().len(),
@ -3746,11 +3751,28 @@ impl Bank {
);
let status = process_result
.and_then(|info| {
let post_account_state_info =
self.get_transaction_account_state_info(&transaction_context, tx.message());
self.verify_transaction_account_state_changes(
&pre_account_state_info,
&post_account_state_info,
&transaction_context,
)
.map(|_| info)
})
.map(|info| {
self.store_accounts_data_len(info.accounts_data_len);
})
.map_err(|err| {
error_counters.instruction_error += 1;
match err {
TransactionError::InvalidRentPayingAccount => {
error_counters.invalid_rent_paying_account += 1;
}
_ => {
error_counters.instruction_error += 1;
}
}
err
});
@ -6254,7 +6276,7 @@ impl Bank {
// Adjust capitalization.... it has been wrapping, reducing the real capitalization by 1-lamport
self.capitalization.fetch_add(1, Relaxed);
info!(
"purged rewards pool accont: {}, new capitalization: {}",
"purged rewards pool account: {}, new capitalization: {}",
reward_pubkey,
self.capitalization()
);
@ -7085,6 +7107,12 @@ pub(crate) mod tests {
bootstrap_validator_stake_lamports,
)
.genesis_config;
// While we are preventing new accounts left in a rent-paying state, not quite ready to rip
// out all the rent assessment tests. Just deactivate the feature for now.
genesis_config
.accounts
.remove(&feature_set::require_rent_exempt_accounts::id())
.unwrap();
genesis_config.epoch_schedule = EpochSchedule::custom(
MINIMUM_SLOTS_PER_EPOCH,
@ -15526,4 +15554,282 @@ pub(crate) mod tests {
7
);
}
#[derive(Serialize, Deserialize)]
enum MockTransferInstruction {
Transfer(u64),
}
fn mock_transfer_process_instruction(
_first_instruction_account: usize,
data: &[u8],
invoke_context: &mut InvokeContext,
) -> result::Result<(), InstructionError> {
let transaction_context = &invoke_context.transaction_context;
let instruction_context = transaction_context.get_current_instruction_context()?;
if let Ok(instruction) = bincode::deserialize(data) {
match instruction {
MockTransferInstruction::Transfer(amount) => {
instruction_context
.try_borrow_instruction_account(transaction_context, 1)?
.checked_sub_lamports(amount)?;
instruction_context
.try_borrow_instruction_account(transaction_context, 2)?
.checked_add_lamports(amount)?;
Ok(())
}
}
} else {
Err(InstructionError::InvalidInstructionData)
}
}
fn create_mock_transfer(
payer: &Keypair,
from: &Keypair,
to: &Keypair,
amount: u64,
mock_program_id: Pubkey,
recent_blockhash: Hash,
) -> Transaction {
let account_metas = vec![
AccountMeta::new(payer.pubkey(), true),
AccountMeta::new(from.pubkey(), true),
AccountMeta::new(to.pubkey(), true),
];
let transfer_instruction = Instruction::new_with_bincode(
mock_program_id,
&MockTransferInstruction::Transfer(amount),
account_metas,
);
Transaction::new_signed_with_payer(
&[transfer_instruction],
Some(&payer.pubkey()),
&[payer, from, to],
recent_blockhash,
)
}
#[test]
fn test_invalid_rent_state_changes_existing_accounts() {
let GenesisConfigInfo {
mut genesis_config,
mint_keypair,
..
} = create_genesis_config_with_leader(sol_to_lamports(100.), &Pubkey::new_unique(), 42);
genesis_config.rent = Rent::default();
let mock_program_id = Pubkey::new_unique();
let account_data_size = 100;
let rent_exempt_minimum = genesis_config.rent.minimum_balance(account_data_size);
// Create legacy accounts of various kinds
let rent_paying_account = Keypair::new();
genesis_config.accounts.insert(
rent_paying_account.pubkey(),
Account::new(rent_exempt_minimum - 1, account_data_size, &mock_program_id),
);
let rent_exempt_account = Keypair::new();
genesis_config.accounts.insert(
rent_exempt_account.pubkey(),
Account::new(rent_exempt_minimum, account_data_size, &mock_program_id),
);
// Activate features, including require_rent_exempt_accounts
activate_all_features(&mut genesis_config);
let mut bank = Bank::new_for_tests(&genesis_config);
bank.add_builtin(
"mock_program",
&mock_program_id,
mock_transfer_process_instruction,
);
let recent_blockhash = bank.last_blockhash();
let check_account_is_rent_exempt = |pubkey: &Pubkey| -> bool {
let account = bank.get_account(pubkey).unwrap();
Rent::default().is_exempt(account.lamports(), account.data().len())
};
// RentPaying account can be left as Uninitialized, in other RentPaying states, or RentExempt
let tx = create_mock_transfer(
&mint_keypair, // payer
&rent_paying_account, // from
&mint_keypair, // to
1,
mock_program_id,
recent_blockhash,
);
let result = bank.process_transaction(&tx);
assert!(result.is_ok());
assert!(!check_account_is_rent_exempt(&rent_paying_account.pubkey()));
let tx = create_mock_transfer(
&mint_keypair, // payer
&rent_paying_account, // from
&mint_keypair, // to
rent_exempt_minimum - 2,
mock_program_id,
recent_blockhash,
);
let result = bank.process_transaction(&tx);
assert!(result.is_ok());
assert!(bank.get_account(&rent_paying_account.pubkey()).is_none());
bank.store_account(
// restore program-owned account
&rent_paying_account.pubkey(),
&AccountSharedData::new(rent_exempt_minimum - 1, account_data_size, &mock_program_id),
);
let result = bank.transfer(1, &mint_keypair, &rent_paying_account.pubkey());
assert!(result.is_ok());
assert!(check_account_is_rent_exempt(&rent_paying_account.pubkey()));
// RentExempt account can only remain RentExempt or be Uninitialized
let tx = create_mock_transfer(
&mint_keypair, // payer
&rent_exempt_account, // from
&mint_keypair, // to
1,
mock_program_id,
recent_blockhash,
);
let result = bank.process_transaction(&tx);
assert!(result.is_err());
assert!(check_account_is_rent_exempt(&rent_exempt_account.pubkey()));
let result = bank.transfer(1, &mint_keypair, &rent_exempt_account.pubkey());
assert!(result.is_ok());
assert!(check_account_is_rent_exempt(&rent_exempt_account.pubkey()));
let tx = create_mock_transfer(
&mint_keypair, // payer
&rent_exempt_account, // from
&mint_keypair, // to
rent_exempt_minimum + 1,
mock_program_id,
recent_blockhash,
);
let result = bank.process_transaction(&tx);
assert!(result.is_ok());
assert!(bank.get_account(&rent_exempt_account.pubkey()).is_none());
}
#[test]
fn test_invalid_rent_state_changes_new_accounts() {
let GenesisConfigInfo {
mut genesis_config,
mint_keypair,
..
} = create_genesis_config_with_leader(sol_to_lamports(100.), &Pubkey::new_unique(), 42);
genesis_config.rent = Rent::default();
let mock_program_id = Pubkey::new_unique();
let account_data_size = 100;
let rent_exempt_minimum = genesis_config.rent.minimum_balance(account_data_size);
// Activate features, including require_rent_exempt_accounts
activate_all_features(&mut genesis_config);
let mut bank = Bank::new_for_tests(&genesis_config);
bank.add_builtin(
"mock_program",
&mock_program_id,
mock_transfer_process_instruction,
);
let recent_blockhash = bank.last_blockhash();
let check_account_is_rent_exempt = |pubkey: &Pubkey| -> bool {
let account = bank.get_account(pubkey).unwrap();
Rent::default().is_exempt(account.lamports(), account.data().len())
};
// Try to create RentPaying account
let rent_paying_account = Keypair::new();
let tx = system_transaction::create_account(
&mint_keypair,
&rent_paying_account,
recent_blockhash,
rent_exempt_minimum - 1,
account_data_size as u64,
&mock_program_id,
);
let result = bank.process_transaction(&tx);
assert!(result.is_err());
assert!(bank.get_account(&rent_paying_account.pubkey()).is_none());
// Try to create RentExempt account
let rent_exempt_account = Keypair::new();
let tx = system_transaction::create_account(
&mint_keypair,
&rent_exempt_account,
recent_blockhash,
rent_exempt_minimum,
account_data_size as u64,
&mock_program_id,
);
let result = bank.process_transaction(&tx);
assert!(result.is_ok());
assert!(check_account_is_rent_exempt(&rent_exempt_account.pubkey()));
}
#[test]
fn test_rent_state_changes_sysvars() {
let GenesisConfigInfo {
mut genesis_config,
mint_keypair,
..
} = create_genesis_config_with_leader(sol_to_lamports(100.), &Pubkey::new_unique(), 42);
genesis_config.rent = Rent::default();
// Activate features, including require_rent_exempt_accounts
activate_all_features(&mut genesis_config);
let validator_pubkey = solana_sdk::pubkey::new_rand();
let validator_stake_lamports = sol_to_lamports(1.);
let validator_staking_keypair = Keypair::new();
let validator_voting_keypair = Keypair::new();
let validator_vote_account = vote_state::create_account(
&validator_voting_keypair.pubkey(),
&validator_pubkey,
0,
validator_stake_lamports,
);
let validator_stake_account = stake_state::create_account(
&validator_staking_keypair.pubkey(),
&validator_voting_keypair.pubkey(),
&validator_vote_account,
&genesis_config.rent,
validator_stake_lamports,
);
genesis_config.accounts.insert(
validator_pubkey,
Account::new(
genesis_config.rent.minimum_balance(0),
0,
&system_program::id(),
),
);
genesis_config.accounts.insert(
validator_staking_keypair.pubkey(),
Account::from(validator_stake_account),
);
genesis_config.accounts.insert(
validator_voting_keypair.pubkey(),
Account::from(validator_vote_account),
);
let bank = Bank::new_for_tests(&genesis_config);
// Ensure transactions with sysvars succeed, even though sysvars appear RentPaying by balance
let tx = Transaction::new_signed_with_payer(
&[stake_instruction::deactivate_stake(
&validator_staking_keypair.pubkey(),
&validator_staking_keypair.pubkey(),
)],
Some(&mint_keypair.pubkey()),
&[&mint_keypair, &validator_staking_keypair],
bank.last_blockhash(),
);
let result = bank.process_transaction(&tx);
assert!(result.is_ok());
}
}

View File

@ -0,0 +1,71 @@
use {
crate::{
account_rent_state::{check_rent_state, RentState},
bank::Bank,
},
solana_sdk::{
account::ReadableAccount, feature_set, message::SanitizedMessage, native_loader,
transaction::Result, transaction_context::TransactionContext,
},
};
pub(crate) struct TransactionAccountStateInfo {
rent_state: Option<RentState>, // None: readonly account
}
impl Bank {
pub(crate) fn get_transaction_account_state_info(
&self,
transaction_context: &TransactionContext,
message: &SanitizedMessage,
) -> Vec<TransactionAccountStateInfo> {
(0..transaction_context.get_number_of_accounts())
.map(|i| {
let rent_state = if message.is_writable(i) {
let account = transaction_context.get_account_at_index(i).borrow();
// Native programs appear to be RentPaying because they carry low lamport
// balances; however they will never be loaded as writable
debug_assert!(!native_loader::check_id(account.owner()));
Some(RentState::from_account(
&account,
&self.rent_collector().rent,
))
} else {
None
};
TransactionAccountStateInfo { rent_state }
})
.collect()
}
pub(crate) fn verify_transaction_account_state_changes(
&self,
pre_state_infos: &[TransactionAccountStateInfo],
post_state_infos: &[TransactionAccountStateInfo],
transaction_context: &TransactionContext,
) -> Result<()> {
let require_rent_exempt_accounts = self
.feature_set
.is_active(&feature_set::require_rent_exempt_accounts::id());
for (i, (pre_state_info, post_state_info)) in
pre_state_infos.iter().zip(post_state_infos).enumerate()
{
if let Err(err) = check_rent_state(
pre_state_info.rent_state.as_ref(),
post_state_info.rent_state.as_ref(),
transaction_context,
i,
) {
// Feature gate only wraps the actual error return so that the metrics and debug
// logging generated by `check_rent_state()` can be examined before feature
// activation
if require_rent_exempt_accounts {
return Err(err);
}
}
}
Ok(())
}
}

View File

@ -1,6 +1,7 @@
#![cfg_attr(RUSTC_WITH_SPECIALIZATION, feature(min_specialization))]
#![allow(clippy::integer_arithmetic)]
pub mod account_info;
pub mod account_rent_state;
pub mod accounts;
pub mod accounts_background_service;
pub mod accounts_cache;