From 0ad31535927b339bcff1b23da7a429855f99bfd6 Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Wed, 12 Aug 2020 00:04:32 +0900 Subject: [PATCH 1/3] Fix bad rent in Bank::deposit as if since epoch 0 (#10468) * Fix bad rent in Bank::deposit as if since epoch 0 * Remove redundant predicate * Rename * Start to add tests with some cleanup * Forgot to add refactor code... * Enchance test * Really fix rent timing in deposit with robust test * Simplify new behavior by disabling rent altogether (cherry picked from commit 6c242f3fec406c3626c23882b8eccec1c1be75a6) # Conflicts: # runtime/src/accounts.rs # runtime/src/rent_collector.rs --- docs/src/implemented-proposals/rent.md | 8 +++ runtime/src/accounts.rs | 11 ++++- runtime/src/bank.rs | 40 +++++++++------ runtime/src/rent_collector.rs | 67 ++++++++++++++++++++++++-- sdk/src/genesis_config.rs | 21 ++++++++ 5 files changed, 128 insertions(+), 19 deletions(-) diff --git a/docs/src/implemented-proposals/rent.md b/docs/src/implemented-proposals/rent.md index 4465e44c1870aa..4e5fd57a05e58f 100644 --- a/docs/src/implemented-proposals/rent.md +++ b/docs/src/implemented-proposals/rent.md @@ -14,6 +14,14 @@ Currently, the rent cost is fixed at the genesis. However, it's anticipated to b There are two timings of collecting rent from accounts: \(1\) when referenced by a transaction, \(2\) periodically once an epoch. \(1\) includes the transaction to create the new account itself, and it happens during the normal transaction processing by the bank as part of the load phase. \(2\) exists to ensure to collect rents from stale accounts, which aren't referenced in recent epochs at all. \(2\) requires the whole scan of accounts and is spread over an epoch based on account address prefix to avoid load spikes due to this rent collection. +On the contrary, rent collection isn't applied to accounts that are directly manipulated by any of protocol-level bookkeeping processes including: + +- The distribution of rent collection itself (Otherwise, it may cause recursive rent collection handling) +- The distribution of staking rewards at the start of every epoch (To reduce as much as processing spike at the start of new epoch) +- The distribution of transaction fee at the end of every epoch + +Even if those processes are out of scope of rent collection, all of manipulated accounts will eventually be handled by the \(2\) mechanism. + ## Actual processing of collecting rent Rent is due for one epoch's worth of time, and accounts always have `Account::rent_epoch` of `current_epoch + 1`. diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 42db83b9bc86c4..f5da9f379de969 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -154,10 +154,18 @@ impl Accounts { } let (account, rent) = AccountsDB::load(storage, ancestors, accounts_index, key) +<<<<<<< HEAD .and_then(|(mut account, _)| { if message.is_writable(i) && !account.executable { let rent_due = rent_collector.update(&key, &mut account); Some((account, rent_due)) +======= + .map(|(mut account, _)| { + if message.is_writable(i) { + let rent_due = rent_collector + .collect_from_existing_account(&key, &mut account); + (account, rent_due) +>>>>>>> 6c242f3fe... Fix bad rent in Bank::deposit as if since epoch 0 (#10468) } else { Some((account, 0)) } @@ -751,8 +759,7 @@ impl Accounts { ); if message.is_writable(i) { if account.rent_epoch == 0 { - account.rent_epoch = rent_collector.epoch; - acc.2 += rent_collector.update(&key, account); + acc.2 += rent_collector.collect_from_created_account(&key, account); } accounts.push((key, &*account)); } diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 8ecf956db1c907..dc7037c41f5ff4 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -945,18 +945,13 @@ impl Bank { .unwrap() .genesis_hash(&genesis_config.hash(), &self.fee_calculator); - self.hashes_per_tick = genesis_config.poh_config.hashes_per_tick; - self.ticks_per_slot = genesis_config.ticks_per_slot; - self.ns_per_slot = genesis_config.poh_config.target_tick_duration.as_nanos() - * genesis_config.ticks_per_slot as u128; + self.hashes_per_tick = genesis_config.hashes_per_tick(); + self.ticks_per_slot = genesis_config.ticks_per_slot(); + self.ns_per_slot = genesis_config.ns_per_slot(); self.genesis_creation_time = genesis_config.creation_time; self.unused = genesis_config.unused; self.max_tick_height = (self.slot + 1) * self.ticks_per_slot; - self.slots_per_year = years_as_slots( - 1.0, - &genesis_config.poh_config.target_tick_duration, - self.ticks_per_slot, - ); + self.slots_per_year = genesis_config.slots_per_year(); self.epoch_schedule = genesis_config.epoch_schedule; @@ -1859,7 +1854,9 @@ impl Bank { // parallelize? let mut rent = 0; for (pubkey, mut account) in accounts { - rent += self.rent_collector.update(&pubkey, &mut account); + rent += self + .rent_collector + .collect_from_existing_account(&pubkey, &mut account); // Store all of them unconditionally to purge old AppendVec, // even if collected rent is 0 (= not updated). self.store_account(&pubkey, &account); @@ -2296,10 +2293,25 @@ impl Bank { pub fn deposit(&self, pubkey: &Pubkey, lamports: u64) { let mut account = self.get_account(pubkey).unwrap_or_default(); - self.collected_rent.fetch_add( - self.rent_collector.update(pubkey, &mut account), - Ordering::Relaxed, - ); + + let should_be_in_new_behavior = match self.operating_mode() { + OperatingMode::Development => true, + OperatingMode::Preview => self.epoch() >= Epoch::max_value(), + OperatingMode::Stable => self.epoch() >= Epoch::max_value(), + }; + + // 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) + // there are too many and subtly nuanced modification codepaths + if !should_be_in_new_behavior { + // previously we're too much collecting rents as if it existed since epoch 0... + self.collected_rent.fetch_add( + self.rent_collector + .collect_from_existing_account(pubkey, &mut account), + Ordering::Relaxed, + ); + } + account.lamports += lamports; self.store_account(pubkey, &account); } diff --git a/runtime/src/rent_collector.rs b/runtime/src/rent_collector.rs index 6d95a457953660..2b3be64b00e28d 100644 --- a/runtime/src/rent_collector.rs +++ b/runtime/src/rent_collector.rs @@ -1,10 +1,14 @@ //! calculate and collect rent from Accounts use solana_sdk::{ - account::Account, clock::Epoch, epoch_schedule::EpochSchedule, incinerator, pubkey::Pubkey, - rent::Rent, sysvar, + account::Account, clock::Epoch, epoch_schedule::EpochSchedule, genesis_config::GenesisConfig, + incinerator, pubkey::Pubkey, rent::Rent, sysvar, }; +<<<<<<< HEAD #[derive(Default, Serialize, Deserialize, Clone)] +======= +#[derive(Serialize, Deserialize, Clone, PartialEq, Debug, AbiExample)] +>>>>>>> 6c242f3fe... Fix bad rent in Bank::deposit as if since epoch 0 (#10468) pub struct RentCollector { pub epoch: Epoch, pub epoch_schedule: EpochSchedule, @@ -12,6 +16,18 @@ pub struct RentCollector { pub rent: Rent, } +impl Default for RentCollector { + fn default() -> Self { + Self { + epoch: Epoch::default(), + epoch_schedule: EpochSchedule::default(), + // derive default value using GenesisConfig::default() + slots_per_year: GenesisConfig::default().slots_per_year(), + rent: Rent::default(), + } + } +} + impl RentCollector { pub fn new( epoch: Epoch, @@ -36,7 +52,8 @@ impl RentCollector { // updates this account's lamports and status and returns // the account rent collected, if any // - pub fn update(&self, address: &Pubkey, account: &mut Account) -> u64 { + #[must_use = "add to Bank::collected_rent"] + pub fn collect_from_existing_account(&self, address: &Pubkey, account: &mut Account) -> u64 { if account.executable || account.rent_epoch > self.epoch || sysvar::check_id(&account.owner) @@ -70,4 +87,48 @@ impl RentCollector { } } } + + #[must_use = "add to Bank::collected_rent"] + pub fn collect_from_created_account(&self, address: &Pubkey, account: &mut Account) -> u64 { + // initialize rent_epoch as created at this epoch + account.rent_epoch = self.epoch; + self.collect_from_existing_account(address, account) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_collect_from_account_created_and_existing() { + let old_lamports = 1000; + let old_epoch = 1; + let new_epoch = 3; + + let (mut created_account, mut existing_account) = { + let mut account = Account::default(); + account.lamports = old_lamports; + account.rent_epoch = old_epoch; + + (account.clone(), account) + }; + + let rent_collector = RentCollector::default().clone_with_epoch(new_epoch); + + let collected = + rent_collector.collect_from_created_account(&Pubkey::new_rand(), &mut created_account); + assert!(created_account.lamports < old_lamports); + assert_eq!(created_account.lamports + collected, old_lamports); + assert_ne!(created_account.rent_epoch, old_epoch); + + let collected = rent_collector + .collect_from_existing_account(&Pubkey::new_rand(), &mut existing_account); + assert!(existing_account.lamports < old_lamports); + assert_eq!(existing_account.lamports + collected, old_lamports); + assert_ne!(existing_account.rent_epoch, old_epoch); + + assert!(created_account.lamports > existing_account.lamports); + assert_eq!(created_account.rent_epoch, existing_account.rent_epoch); + } } diff --git a/sdk/src/genesis_config.rs b/sdk/src/genesis_config.rs index 8a350da68b3a55..6d2f893e0f5c38 100644 --- a/sdk/src/genesis_config.rs +++ b/sdk/src/genesis_config.rs @@ -14,6 +14,7 @@ use crate::{ shred_version::compute_shred_version, signature::{Keypair, Signer}, system_program, + timing::years_as_slots, }; use bincode::{deserialize, serialize}; use chrono::{TimeZone, Utc}; @@ -182,6 +183,26 @@ impl GenesisConfig { pub fn add_rewards_pool(&mut self, pubkey: Pubkey, account: Account) { self.rewards_pools.insert(pubkey, account); } + + pub fn hashes_per_tick(&self) -> Option { + self.poh_config.hashes_per_tick + } + + pub fn ticks_per_slot(&self) -> u64 { + self.ticks_per_slot + } + + pub fn ns_per_slot(&self) -> u128 { + self.poh_config.target_tick_duration.as_nanos() * self.ticks_per_slot() as u128 + } + + pub fn slots_per_year(&self) -> f64 { + years_as_slots( + 1.0, + &self.poh_config.target_tick_duration, + self.ticks_per_slot(), + ) + } } impl fmt::Display for GenesisConfig { From 0754a44040c5079f15d893977b41b13fcf58c485 Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Wed, 12 Aug 2020 00:13:17 +0900 Subject: [PATCH 2/3] Fix conflict --- runtime/src/accounts.rs | 9 +-------- runtime/src/rent_collector.rs | 6 +----- 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index f5da9f379de969..f1cb4652a7082c 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -154,18 +154,11 @@ impl Accounts { } let (account, rent) = AccountsDB::load(storage, ancestors, accounts_index, key) -<<<<<<< HEAD .and_then(|(mut account, _)| { - if message.is_writable(i) && !account.executable { - let rent_due = rent_collector.update(&key, &mut account); - Some((account, rent_due)) -======= - .map(|(mut account, _)| { if message.is_writable(i) { let rent_due = rent_collector .collect_from_existing_account(&key, &mut account); - (account, rent_due) ->>>>>>> 6c242f3fe... Fix bad rent in Bank::deposit as if since epoch 0 (#10468) + Some((account, rent_due)) } else { Some((account, 0)) } diff --git a/runtime/src/rent_collector.rs b/runtime/src/rent_collector.rs index 2b3be64b00e28d..cc3b648b525759 100644 --- a/runtime/src/rent_collector.rs +++ b/runtime/src/rent_collector.rs @@ -4,11 +4,7 @@ use solana_sdk::{ incinerator, pubkey::Pubkey, rent::Rent, sysvar, }; -<<<<<<< HEAD -#[derive(Default, Serialize, Deserialize, Clone)] -======= -#[derive(Serialize, Deserialize, Clone, PartialEq, Debug, AbiExample)] ->>>>>>> 6c242f3fe... Fix bad rent in Bank::deposit as if since epoch 0 (#10468) +#[derive(Serialize, Deserialize, Clone)] pub struct RentCollector { pub epoch: Epoch, pub epoch_schedule: EpochSchedule, From 701ce5b0bc7935c27705321da6520d4e978d60d7 Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Wed, 12 Aug 2020 00:35:17 +0900 Subject: [PATCH 3/3] Fix clippy --- runtime/src/bank.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index dc7037c41f5ff4..fc5ed3478ad20d 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -54,7 +54,6 @@ use solana_sdk::{ slot_history::SlotHistory, system_transaction, sysvar::{self, Sysvar}, - timing::years_as_slots, transaction::{Result, Transaction, TransactionError}, }; use solana_stake_program::stake_state::{self, Delegation, PointValue}; @@ -2911,7 +2910,7 @@ mod tests { signature::{Keypair, Signer}, system_instruction, system_program, sysvar::{fees::Fees, rewards::Rewards}, - timing::duration_as_s, + timing::{duration_as_s, years_as_slots}, }; use solana_stake_program::{ stake_instruction,