gaspersic/solana
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Port various rent fixes to runtime feature (#12842) (#13068)

Browse Source 
* Port various rent fixes to runtime feature

* Fix CI

* Use more consistent naming...

(cherry picked from commit 608b81b412)

Co-authored-by: Ryo Onodera <ryoqun@gmail.com>
This commit is contained in:
mergify[bot]
2020-10-21 14:47:48 +00:00
committed by GitHub
parent 8752bf0826
commit 5dd61b5db2
4 changed files with 84 additions and 72 deletions
Download Patch File Download Diff File Expand all files Collapse all files

21
runtime/src/accounts.rs
View File

@ -144,6 +144,8 @@ impl Accounts {
let mut payer_index = None; let mut payer_index = None;
let mut tx_rent: TransactionRent = 0; let mut tx_rent: TransactionRent = 0;
let mut accounts = Vec::with_capacity(message.account_keys.len()); let mut accounts = Vec::with_capacity(message.account_keys.len());
let rent_fix_enabled = feature_set.cumulative_rent_related_fixes_enabled();
for (i, key) in message.account_keys.iter().enumerate() { for (i, key) in message.account_keys.iter().enumerate() {
let account = if Self::is_non_loader_key(message, key, i) { let account = if Self::is_non_loader_key(message, key, i) {
if payer_index.is_none() { if payer_index.is_none() {
@ -163,7 +165,11 @@ impl Accounts {
.map(|(mut account, _)| { .map(|(mut account, _)| {
if message.is_writable(i) { if message.is_writable(i) {
let rent_due = rent_collector let rent_due = rent_collector
.collect_from_existing_account(&key, &mut account); .collect_from_existing_account(
&key,
&mut account,
rent_fix_enabled,
);
(account, rent_due) (account, rent_due)
} else { } else {
(account, 0) (account, 0)
@ -721,6 +727,8 @@ impl Accounts {
} }
/// Store the accounts into the DB /// Store the accounts into the DB
// allow(clippy) needed for various gating flags
#[allow(clippy::too_many_arguments)]
pub fn store_accounts( pub fn store_accounts(
&self, &self,
slot: Slot, slot: Slot,
@ -731,6 +739,7 @@ impl Accounts {
rent_collector: &RentCollector, rent_collector: &RentCollector,
last_blockhash_with_fee_calculator: &(Hash, FeeCalculator), last_blockhash_with_fee_calculator: &(Hash, FeeCalculator),
fix_recent_blockhashes_sysvar_delay: bool, fix_recent_blockhashes_sysvar_delay: bool,
rent_fix_enabled: bool,
) { ) {
let accounts_to_store = self.collect_accounts_to_store( let accounts_to_store = self.collect_accounts_to_store(
txs, txs,
@ -740,6 +749,7 @@ impl Accounts {
rent_collector, rent_collector,
last_blockhash_with_fee_calculator, last_blockhash_with_fee_calculator,
fix_recent_blockhashes_sysvar_delay, fix_recent_blockhashes_sysvar_delay,
rent_fix_enabled,
); );
self.accounts_db.store(slot, &accounts_to_store); self.accounts_db.store(slot, &accounts_to_store);
} }
@ -767,6 +777,7 @@ impl Accounts {
rent_collector: &RentCollector, rent_collector: &RentCollector,
last_blockhash_with_fee_calculator: &(Hash, FeeCalculator), last_blockhash_with_fee_calculator: &(Hash, FeeCalculator),
fix_recent_blockhashes_sysvar_delay: bool, fix_recent_blockhashes_sysvar_delay: bool,
rent_fix_enabled: bool,
) -> Vec<(&'a Pubkey, &'a Account)> { ) -> Vec<(&'a Pubkey, &'a Account)> {
let mut accounts = Vec::with_capacity(loaded.len()); let mut accounts = Vec::with_capacity(loaded.len());
for (i, ((raccs, _hash_age_kind), (_, tx))) in loaded for (i, ((raccs, _hash_age_kind), (_, tx))) in loaded
@ -807,7 +818,11 @@ impl Accounts {
); );
if message.is_writable(i) { if message.is_writable(i) {
if account.rent_epoch == 0 { if account.rent_epoch == 0 {
acc.2 += rent_collector.collect_from_created_account(&key, account); acc.2 += rent_collector.collect_from_created_account(
&key,
account,
rent_fix_enabled,
);
} }
accounts.push((key, &*account)); accounts.push((key, &*account));
} }
@ -1123,7 +1138,6 @@ mod tests {
lamports_per_byte_year: 42, lamports_per_byte_year: 42,
..Rent::default() ..Rent::default()
}, },
ClusterType::Development,
); );
let min_balance = rent_collector.rent.minimum_balance(nonce::State::size()); let min_balance = rent_collector.rent.minimum_balance(nonce::State::size());
let fee_calculator = FeeCalculator::new(min_balance); let fee_calculator = FeeCalculator::new(min_balance);
@ -1801,6 +1815,7 @@ mod tests {
&rent_collector, &rent_collector,
&(Hash::default(), FeeCalculator::default()), &(Hash::default(), FeeCalculator::default()),
true, true,
true,
); );
assert_eq!(collected_accounts.len(), 2); assert_eq!(collected_accounts.len(), 2);
assert!(collected_accounts assert!(collected_accounts

58
runtime/src/bank.rs
View File

@ -807,9 +807,7 @@ impl Bank {
slots_per_year: parent.slots_per_year, slots_per_year: parent.slots_per_year,
epoch_schedule, epoch_schedule,
collected_rent: AtomicU64::new(0), collected_rent: AtomicU64::new(0),
rent_collector: parent rent_collector: parent.rent_collector.clone_with_epoch(epoch),
.rent_collector
.clone_with_epoch(epoch, parent.cluster_type()),
max_tick_height: (slot + 1) * parent.ticks_per_slot, max_tick_height: (slot + 1) * parent.ticks_per_slot,
block_height: parent.block_height + 1, block_height: parent.block_height + 1,
fee_calculator: fee_rate_governor.create_fee_calculator(), fee_calculator: fee_rate_governor.create_fee_calculator(),
@ -927,9 +925,7 @@ impl Bank {
fee_rate_governor: fields.fee_rate_governor, fee_rate_governor: fields.fee_rate_governor,
collected_rent: AtomicU64::new(fields.collected_rent), collected_rent: AtomicU64::new(fields.collected_rent),
// clone()-ing is needed to consider a gated behavior in rent_collector // clone()-ing is needed to consider a gated behavior in rent_collector
rent_collector: fields rent_collector: fields.rent_collector.clone_with_epoch(fields.epoch),
.rent_collector
.clone_with_epoch(fields.epoch, genesis_config.cluster_type),
epoch_schedule: fields.epoch_schedule, epoch_schedule: fields.epoch_schedule,
inflation: Arc::new(RwLock::new(fields.inflation)), inflation: Arc::new(RwLock::new(fields.inflation)),
stakes: RwLock::new(fields.stakes), stakes: RwLock::new(fields.stakes),
@ -1668,7 +1664,6 @@ impl Bank {
&self.epoch_schedule, &self.epoch_schedule,
self.slots_per_year, self.slots_per_year,
&genesis_config.rent, &genesis_config.rent,
self.cluster_type(),
); );
// Add additional native programs specified in the genesis config // Add additional native programs specified in the genesis config
@ -2591,6 +2586,7 @@ impl Bank {
&self.rent_collector, &self.rent_collector,
&self.last_blockhash_with_fee_calculator(), &self.last_blockhash_with_fee_calculator(),
self.fix_recent_blockhashes_sysvar_delay(), self.fix_recent_blockhashes_sysvar_delay(),
self.cumulative_rent_related_fixes_enabled(),
); );
self.collect_rent(executed, loaded_accounts); self.collect_rent(executed, loaded_accounts);
@ -2834,9 +2830,11 @@ impl Bank {
// parallelize? // parallelize?
let mut rent = 0; let mut rent = 0;
for (pubkey, mut account) in accounts { for (pubkey, mut account) in accounts {
rent += self rent += self.rent_collector.collect_from_existing_account(
.rent_collector &pubkey,
.collect_from_existing_account(&pubkey, &mut account); &mut account,
self.cumulative_rent_related_fixes_enabled(),
);
// Store all of them unconditionally to purge old AppendVec, // Store all of them unconditionally to purge old AppendVec,
// even if collected rent is 0 (= not updated). // even if collected rent is 0 (= not updated).
self.store_account(&pubkey, &account); self.store_account(&pubkey, &account);
@ -2954,12 +2952,9 @@ impl Bank {
self.get_epoch_and_slot_index(self.parent_slot()); self.get_epoch_and_slot_index(self.parent_slot());
let should_enable = match self.cluster_type() { let should_enable = match self.cluster_type() {
ClusterType::Development => true,
ClusterType::Devnet => true,
ClusterType::Testnet => current_epoch >= 97,
ClusterType::MainnetBeta => { ClusterType::MainnetBeta => {
#[cfg(not(test))] #[cfg(not(test))]
let should_enable = current_epoch >= Epoch::max_value(); let should_enable = self.cumulative_rent_related_fixes_enabled();
// needed for test_rent_eager_across_epoch_with_gap_under_multi_epoch_cycle, // needed for test_rent_eager_across_epoch_with_gap_under_multi_epoch_cycle,
// which depends on ClusterType::MainnetBeta // which depends on ClusterType::MainnetBeta
@ -2968,6 +2963,7 @@ impl Bank {
should_enable should_enable
} }
_ => self.cumulative_rent_related_fixes_enabled(),
}; };
let mut partitions = vec![]; let mut partitions = vec![];
@ -3314,21 +3310,19 @@ impl Bank {
pub fn deposit(&self, pubkey: &Pubkey, lamports: u64) -> u64 { pub fn deposit(&self, pubkey: &Pubkey, lamports: u64) -> u64 {
let mut account = self.get_account(pubkey).unwrap_or_default(); let mut account = self.get_account(pubkey).unwrap_or_default();
let should_be_in_new_behavior = match self.cluster_type() { let rent_fix_enabled = self.cumulative_rent_related_fixes_enabled();
ClusterType::Development => true,
ClusterType::Devnet => true,
ClusterType::Testnet => self.epoch() >= 97,
ClusterType::MainnetBeta => self.epoch() >= Epoch::max_value(),
};
// don't collect rents if we're in the new behavior; // don't collect rents if we're in the new behavior;
// in genral, it's not worthwhile to account for rents outside the runtime (transactions) // in genral, it's not worthwhile to account for rents outside the runtime (transactions)
// there are too many and subtly nuanced modification codepaths // there are too many and subtly nuanced modification codepaths
if !should_be_in_new_behavior { if !rent_fix_enabled {
// previously we're too much collecting rents as if it existed since epoch 0... // previously we're too much collecting rents as if it existed since epoch 0...
self.collected_rent.fetch_add( self.collected_rent.fetch_add(
self.rent_collector self.rent_collector.collect_from_existing_account(
.collect_from_existing_account(pubkey, &mut account), pubkey,
&mut account,
rent_fix_enabled,
),
Relaxed, Relaxed,
); );
} }
@ -3895,6 +3889,10 @@ impl Bank {
.is_active(&feature_set::no_overflow_rent_distribution::id()) .is_active(&feature_set::no_overflow_rent_distribution::id())
} }
pub fn cumulative_rent_related_fixes_enabled(&self) -> bool {
self.feature_set.cumulative_rent_related_fixes_enabled()
}
// This is called from snapshot restore AND for each epoch boundary // This is called from snapshot restore AND for each epoch boundary
// The entire code path herein must be idempotent // The entire code path herein must be idempotent
fn apply_feature_activations(&mut self, init_finish_or_warp: bool) { fn apply_feature_activations(&mut self, init_finish_or_warp: bool) {
@ -4100,8 +4098,9 @@ mod tests {
use crate::{ use crate::{
accounts_index::{AccountMap, Ancestors}, accounts_index::{AccountMap, Ancestors},
genesis_utils::{ genesis_utils::{
create_genesis_config_with_leader, create_genesis_config_with_vote_accounts, activate_all_features, create_genesis_config_with_leader,
GenesisConfigInfo, ValidatorVoteKeypairs, BOOTSTRAP_VALIDATOR_LAMPORTS, create_genesis_config_with_vote_accounts, GenesisConfigInfo, ValidatorVoteKeypairs,
BOOTSTRAP_VALIDATOR_LAMPORTS,
}, },
native_loader::NativeLoaderError, native_loader::NativeLoaderError,
process_instruction::InvokeContext, process_instruction::InvokeContext,
@ -5071,7 +5070,8 @@ mod tests {
#[test] #[test]
fn test_rent_eager_across_epoch_with_full_gap() { fn test_rent_eager_across_epoch_with_full_gap() {
let (genesis_config, _mint_keypair) = create_genesis_config(1); let (mut genesis_config, _mint_keypair) = create_genesis_config(1);
activate_all_features(&mut genesis_config);
let mut bank = Arc::new(Bank::new(&genesis_config)); let mut bank = Arc::new(Bank::new(&genesis_config));
assert_eq!(bank.rent_collection_partitions(), vec![(0, 0, 32)]); assert_eq!(bank.rent_collection_partitions(), vec![(0, 0, 32)]);
@ -5093,7 +5093,8 @@ mod tests {
#[test] #[test]
fn test_rent_eager_across_epoch_with_half_gap() { fn test_rent_eager_across_epoch_with_half_gap() {
let (genesis_config, _mint_keypair) = create_genesis_config(1); let (mut genesis_config, _mint_keypair) = create_genesis_config(1);
activate_all_features(&mut genesis_config);
let mut bank = Arc::new(Bank::new(&genesis_config)); let mut bank = Arc::new(Bank::new(&genesis_config));
assert_eq!(bank.rent_collection_partitions(), vec![(0, 0, 32)]); assert_eq!(bank.rent_collection_partitions(), vec![(0, 0, 32)]);
@ -5641,7 +5642,8 @@ mod tests {
fn test_rent_eager_collect_rent_in_partition() { fn test_rent_eager_collect_rent_in_partition() {
solana_logger::setup(); solana_logger::setup();
let (genesis_config, _mint_keypair) = create_genesis_config(1); let (mut genesis_config, _mint_keypair) = create_genesis_config(1);
activate_all_features(&mut genesis_config);
let zero_lamport_pubkey = Pubkey::new_rand(); let zero_lamport_pubkey = Pubkey::new_rand();
let rent_due_pubkey = Pubkey::new_rand(); let rent_due_pubkey = Pubkey::new_rand();

5
runtime/src/feature_set.rs
View File

@ -130,6 +130,11 @@ impl FeatureSet {
pub fn is_active(&self, feature_id: &Pubkey) -> bool { pub fn is_active(&self, feature_id: &Pubkey) -> bool {
self.active.contains(feature_id) self.active.contains(feature_id)
} }
pub fn cumulative_rent_related_fixes_enabled(&self) -> bool {
self.is_active(&cumulative_rent_related_fixes::id())
}
/// All features enabled, useful for testing /// All features enabled, useful for testing
pub fn all_enabled() -> Self { pub fn all_enabled() -> Self {
Self { Self {

72
runtime/src/rent_collector.rs
View File

@ -1,13 +1,7 @@
//! calculate and collect rent from Accounts //! calculate and collect rent from Accounts
use solana_sdk::{ use solana_sdk::{
account::Account, account::Account, clock::Epoch, epoch_schedule::EpochSchedule, genesis_config::GenesisConfig,
clock::Epoch, incinerator, pubkey::Pubkey, rent::Rent, sysvar,
epoch_schedule::EpochSchedule,
genesis_config::{ClusterType, GenesisConfig},
incinerator,
pubkey::Pubkey,
rent::Rent,
sysvar,
}; };
#[derive(Serialize, Deserialize, Clone, PartialEq, Debug, AbiExample)] #[derive(Serialize, Deserialize, Clone, PartialEq, Debug, AbiExample)]
@ -16,11 +10,6 @@ pub struct RentCollector {
pub epoch_schedule: EpochSchedule, pub epoch_schedule: EpochSchedule,
pub slots_per_year: f64, pub slots_per_year: f64,
pub rent: Rent, pub rent: Rent,
// serde(skip) is needed not to break abi
// Also, wrap this with Option so that we can spot any uninitialized codepath (like
// snapshot restore)
#[serde(skip)]
pub cluster_type: Option<ClusterType>,
} }
impl Default for RentCollector { impl Default for RentCollector {
@ -31,7 +20,6 @@ impl Default for RentCollector {
// derive default value using GenesisConfig::default() // derive default value using GenesisConfig::default()
slots_per_year: GenesisConfig::default().slots_per_year(), slots_per_year: GenesisConfig::default().slots_per_year(),
rent: Rent::default(), rent: Rent::default(),
cluster_type: Option::default(),
} }
} }
} }
@ -42,39 +30,32 @@ impl RentCollector {
epoch_schedule: &EpochSchedule, epoch_schedule: &EpochSchedule,
slots_per_year: f64, slots_per_year: f64,
rent: &Rent, rent: &Rent,
cluster_type: ClusterType,
) -> Self { ) -> Self {
Self { Self {
epoch, epoch,
epoch_schedule: *epoch_schedule, epoch_schedule: *epoch_schedule,
slots_per_year, slots_per_year,
rent: *rent, rent: *rent,
cluster_type: Some(cluster_type),
} }
} }
pub fn clone_with_epoch(&self, epoch: Epoch, cluster_type: ClusterType) -> Self { pub fn clone_with_epoch(&self, epoch: Epoch) -> Self {
Self { Self {
epoch, epoch,
cluster_type: Some(cluster_type),
..self.clone() ..self.clone()
} }
} }
fn enable_new_behavior(&self) -> bool {
match self.cluster_type.unwrap() {
ClusterType::Development => true,
ClusterType::Devnet => true,
ClusterType::Testnet => self.epoch >= 97,
ClusterType::MainnetBeta => self.epoch >= Epoch::max_value(),
}
}
// updates this account's lamports and status and returns // updates this account's lamports and status and returns
// the account rent collected, if any // the account rent collected, if any
// //
#[must_use = "add to Bank::collected_rent"] #[must_use = "add to Bank::collected_rent"]
pub fn collect_from_existing_account(&self, address: &Pubkey, account: &mut Account) -> u64 { pub fn collect_from_existing_account(
&self,
address: &Pubkey,
account: &mut Account,
rent_fix_enabled: bool,
) -> u64 {
if account.executable if account.executable
|| account.rent_epoch > self.epoch || account.rent_epoch > self.epoch
|| sysvar::check_id(&account.owner) || sysvar::check_id(&account.owner)
@ -100,7 +81,7 @@ impl RentCollector {
if exempt || rent_due != 0 { if exempt || rent_due != 0 {
if account.lamports > rent_due { if account.lamports > rent_due {
account.rent_epoch = self.epoch account.rent_epoch = self.epoch
+ if self.enable_new_behavior() && exempt { + if rent_fix_enabled && exempt {
// Rent isn't collected for the next epoch // Rent isn't collected for the next epoch
// Make sure to check exempt status later in curent epoch again // Make sure to check exempt status later in curent epoch again
0 0
@ -123,10 +104,15 @@ impl RentCollector {
} }
#[must_use = "add to Bank::collected_rent"] #[must_use = "add to Bank::collected_rent"]
pub fn collect_from_created_account(&self, address: &Pubkey, account: &mut Account) -> u64 { pub fn collect_from_created_account(
&self,
address: &Pubkey,
account: &mut Account,
enable_new_behavior: bool,
) -> u64 {
// initialize rent_epoch as created at this epoch // initialize rent_epoch as created at this epoch
account.rent_epoch = self.epoch; account.rent_epoch = self.epoch;
self.collect_from_existing_account(address, account) self.collect_from_existing_account(address, account, enable_new_behavior)
} }
} }
@ -148,19 +134,24 @@ mod tests {
(account.clone(), account) (account.clone(), account)
}; };
let rent_collector = let rent_collector = RentCollector::default().clone_with_epoch(new_epoch);
RentCollector::default().clone_with_epoch(new_epoch, ClusterType::Development);
// collect rent on a newly-created account // collect rent on a newly-created account
let collected = let collected = rent_collector.collect_from_created_account(
rent_collector.collect_from_created_account(&Pubkey::new_rand(), &mut created_account); &Pubkey::new_rand(),
&mut created_account,
true,
);
assert!(created_account.lamports < old_lamports); assert!(created_account.lamports < old_lamports);
assert_eq!(created_account.lamports + collected, old_lamports); assert_eq!(created_account.lamports + collected, old_lamports);
assert_ne!(created_account.rent_epoch, old_epoch); assert_ne!(created_account.rent_epoch, old_epoch);
// collect rent on a already-existing account // collect rent on a already-existing account
let collected = rent_collector let collected = rent_collector.collect_from_existing_account(
.collect_from_existing_account(&Pubkey::new_rand(), &mut existing_account); &Pubkey::new_rand(),
&mut existing_account,
true,
);
assert!(existing_account.lamports < old_lamports); assert!(existing_account.lamports < old_lamports);
assert_eq!(existing_account.lamports + collected, old_lamports); assert_eq!(existing_account.lamports + collected, old_lamports);
assert_ne!(existing_account.rent_epoch, old_epoch); assert_ne!(existing_account.rent_epoch, old_epoch);
@ -183,11 +174,10 @@ mod tests {
assert_eq!(account.rent_epoch, 0); assert_eq!(account.rent_epoch, 0);
// create a tested rent collector // create a tested rent collector
let rent_collector = let rent_collector = RentCollector::default().clone_with_epoch(epoch);
RentCollector::default().clone_with_epoch(epoch, ClusterType::Development);
// first mark account as being collected while being rent-exempt // first mark account as being collected while being rent-exempt
collected = rent_collector.collect_from_existing_account(&pubkey, &mut account); collected = rent_collector.collect_from_existing_account(&pubkey, &mut account, true);
assert_eq!(account.lamports, huge_lamports); assert_eq!(account.lamports, huge_lamports);
assert_eq!(collected, 0); assert_eq!(collected, 0);
@ -195,7 +185,7 @@ mod tests {
account.lamports = tiny_lamports; account.lamports = tiny_lamports;
// ... and trigger another rent collection on the same epoch and check that rent is working // ... and trigger another rent collection on the same epoch and check that rent is working
collected = rent_collector.collect_from_existing_account(&pubkey, &mut account); collected = rent_collector.collect_from_existing_account(&pubkey, &mut account, true);
assert_eq!(account.lamports, tiny_lamports - collected); assert_eq!(account.lamports, tiny_lamports - collected);
assert_ne!(collected, 0); assert_ne!(collected, 0);
} }