From a2188b704de6190ffe34ca9cac07d4ead8ae786f Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Mon, 17 Aug 2020 14:22:16 +0900 Subject: [PATCH 1/3] Re-do rent collection check on rent-exempt account (#11349) * wip: re-do rent collection check on rent-exempt account * Let's see how the ci goes * Restore previous code * Well, almost all new changes are revertable * Update doc * Add test and gating * Fix tests * Fix tests, especially avoid to change abi... * Fix more tests... * Fix snapshot restore * Align to _new_ with better uninitialized detection (cherry picked from commit 23fa84b322b09defe1447e439aa60bcc69eff539) # Conflicts: # core/src/rpc_subscriptions.rs # core/tests/bank_forks.rs # runtime/src/bank.rs --- core/src/rpc_pubsub.rs | 6 +- core/src/rpc_subscriptions.rs | 20 ++++ core/tests/bank_forks.rs | 59 ++++++++-- docs/src/implemented-proposals/rent.md | 6 +- programs/bpf/c/src/invoked/invoked.c | 6 +- programs/bpf/rust/invoked/src/lib.rs | 6 +- runtime/src/accounts.rs | 2 + runtime/src/bank.rs | 156 ++++++++++++++++++++++++- runtime/src/rent_collector.rs | 76 +++++++++++- 9 files changed, 306 insertions(+), 31 deletions(-) diff --git a/core/src/rpc_pubsub.rs b/core/src/rpc_pubsub.rs index af57ee8c0695f1..d3685d15138e63 100644 --- a/core/src/rpc_pubsub.rs +++ b/core/src/rpc_pubsub.rs @@ -602,7 +602,7 @@ mod tests { "lamports": 51, "data": bs58::encode(expected_data).into_string(), "executable": false, - "rentEpoch": 1, + "rentEpoch": 0, }, }, "subscription": 0, @@ -720,7 +720,7 @@ mod tests { "lamports": 100, "data": expected_data, "executable": false, - "rentEpoch": 1, + "rentEpoch": 0, }, }, "subscription": 0, @@ -899,7 +899,7 @@ mod tests { "lamports": 100, "data": "", "executable": false, - "rentEpoch": 1, + "rentEpoch": 0, }, }, "subscription": 0, diff --git a/core/src/rpc_subscriptions.rs b/core/src/rpc_subscriptions.rs index 15d7ed777b8f37..6391fa5f94893b 100644 --- a/core/src/rpc_subscriptions.rs +++ b/core/src/rpc_subscriptions.rs @@ -1098,8 +1098,13 @@ pub(crate) mod tests { "data": "1111111111111111", "executable": false, "lamports": 1, +<<<<<<< HEAD "owner": "Budget1111111111111111111111111111111111111", "rentEpoch": 1, +======= + "owner": "Stake11111111111111111111111111111111111111", + "rentEpoch": 0, +>>>>>>> 23fa84b32... Re-do rent collection check on rent-exempt account (#11349) }, }, "subscription": 0, @@ -1184,8 +1189,13 @@ pub(crate) mod tests { "data": "1111111111111111", "executable": false, "lamports": 1, +<<<<<<< HEAD "owner": "Budget1111111111111111111111111111111111111", "rentEpoch": 1, +======= + "owner": "Stake11111111111111111111111111111111111111", + "rentEpoch": 0, +>>>>>>> 23fa84b32... Re-do rent collection check on rent-exempt account (#11349) }, "pubkey": alice.pubkey().to_string(), }, @@ -1606,8 +1616,13 @@ pub(crate) mod tests { "data": "1111111111111111", "executable": false, "lamports": 1, +<<<<<<< HEAD "owner": "Budget1111111111111111111111111111111111111", "rentEpoch": 1, +======= + "owner": "Stake11111111111111111111111111111111111111", + "rentEpoch": 0, +>>>>>>> 23fa84b32... Re-do rent collection check on rent-exempt account (#11349) }, }, "subscription": 0, @@ -1640,8 +1655,13 @@ pub(crate) mod tests { "data": "1111111111111111", "executable": false, "lamports": 1, +<<<<<<< HEAD "owner": "Budget1111111111111111111111111111111111111", "rentEpoch": 1, +======= + "owner": "Stake11111111111111111111111111111111111111", + "rentEpoch": 0, +>>>>>>> 23fa84b32... Re-do rent collection check on rent-exempt account (#11349) }, }, "subscription": 1, diff --git a/core/tests/bank_forks.rs b/core/tests/bank_forks.rs index 5b55ebb94c1c76..28bf5f5f26f89e 100644 --- a/core/tests/bank_forks.rs +++ b/core/tests/bank_forks.rs @@ -1,31 +1,32 @@ // Long-running bank_forks tests macro_rules! DEFINE_SNAPSHOT_VERSION_PARAMETERIZED_TEST_FUNCTIONS { - ($x:ident) => { + ($x:ident, $y:ident, $z:ident) => { #[allow(non_snake_case)] - mod $x { + mod $z { use super::*; const SNAPSHOT_VERSION: SnapshotVersion = SnapshotVersion::$x; + const OPERATING_MODE: OperatingMode = OperatingMode::$y; #[test] fn test_bank_forks_status_cache_snapshot_n() { - run_test_bank_forks_status_cache_snapshot_n(SNAPSHOT_VERSION) + run_test_bank_forks_status_cache_snapshot_n(SNAPSHOT_VERSION, OPERATING_MODE) } #[test] fn test_bank_forks_snapshot_n() { - run_test_bank_forks_snapshot_n(SNAPSHOT_VERSION) + run_test_bank_forks_snapshot_n(SNAPSHOT_VERSION, OPERATING_MODE) } #[test] fn test_concurrent_snapshot_packaging() { - run_test_concurrent_snapshot_packaging(SNAPSHOT_VERSION) + run_test_concurrent_snapshot_packaging(SNAPSHOT_VERSION, OPERATING_MODE) } #[test] fn test_slots_to_snapshot() { - run_test_slots_to_snapshot(SNAPSHOT_VERSION) + run_test_slots_to_snapshot(SNAPSHOT_VERSION, OPERATING_MODE) } } }; @@ -52,7 +53,7 @@ mod tests { }; use solana_sdk::{ clock::Slot, - genesis_config::GenesisConfig, + genesis_config::{GenesisConfig, OperatingMode}, hash::hashv, pubkey::Pubkey, signature::{Keypair, Signer}, @@ -61,8 +62,14 @@ mod tests { use std::{fs, path::PathBuf, sync::atomic::AtomicBool, sync::mpsc::channel, sync::Arc}; use tempfile::TempDir; +<<<<<<< HEAD DEFINE_SNAPSHOT_VERSION_PARAMETERIZED_TEST_FUNCTIONS!(V1_1_0); DEFINE_SNAPSHOT_VERSION_PARAMETERIZED_TEST_FUNCTIONS!(V1_2_0); +======= + DEFINE_SNAPSHOT_VERSION_PARAMETERIZED_TEST_FUNCTIONS!(V1_2_0, Development, V1_2_0_Development); + DEFINE_SNAPSHOT_VERSION_PARAMETERIZED_TEST_FUNCTIONS!(V1_2_0, Preview, V1_2_0_Preview); + DEFINE_SNAPSHOT_VERSION_PARAMETERIZED_TEST_FUNCTIONS!(V1_2_0, Stable, V1_2_0_Stable); +>>>>>>> 23fa84b32... Re-do rent collection check on rent-exempt account (#11349) struct SnapshotTestConfig { accounts_dir: TempDir, @@ -76,12 +83,14 @@ mod tests { impl SnapshotTestConfig { fn new( snapshot_version: SnapshotVersion, + operating_mode: OperatingMode, snapshot_interval_slots: u64, ) -> SnapshotTestConfig { let accounts_dir = TempDir::new().unwrap(); let snapshot_dir = TempDir::new().unwrap(); let snapshot_output_path = TempDir::new().unwrap(); - let genesis_config_info = create_genesis_config(10_000); + let mut genesis_config_info = create_genesis_config(10_000); + genesis_config_info.genesis_config.operating_mode = operating_mode; let bank0 = Bank::new_with_paths( &genesis_config_info.genesis_config, vec![accounts_dir.path().to_path_buf()], @@ -161,6 +170,7 @@ mod tests { // `last_slot` bank fn run_bank_forks_snapshot_n( snapshot_version: SnapshotVersion, + operating_mode: OperatingMode, last_slot: Slot, f: F, set_root_interval: u64, @@ -169,7 +179,7 @@ mod tests { { solana_logger::setup(); // Set up snapshotting config - let mut snapshot_test_config = SnapshotTestConfig::new(snapshot_version, 1); + let mut snapshot_test_config = SnapshotTestConfig::new(snapshot_version, operating_mode, 1); let bank_forks = &mut snapshot_test_config.bank_forks; let accounts_dir = &snapshot_test_config.accounts_dir; @@ -212,11 +222,20 @@ mod tests { restore_from_snapshot(bank_forks, last_slot, &[accounts_dir.path().to_path_buf()]); } +<<<<<<< HEAD fn run_test_bank_forks_snapshot_n(snapshot_version: SnapshotVersion) { // create banks upto slot 4 and create 1 new account in each bank. test that bank 4 snapshots +======= + fn run_test_bank_forks_snapshot_n( + snapshot_version: SnapshotVersion, + operating_mode: OperatingMode, + ) { + // create banks up to slot 4 and create 1 new account in each bank. test that bank 4 snapshots +>>>>>>> 23fa84b32... Re-do rent collection check on rent-exempt account (#11349) // and restores correctly run_bank_forks_snapshot_n( snapshot_version, + operating_mode, 4, |bank, mint_keypair| { let key1 = Keypair::new().pubkey(); @@ -247,11 +266,14 @@ mod tests { } } - fn run_test_concurrent_snapshot_packaging(snapshot_version: SnapshotVersion) { + fn run_test_concurrent_snapshot_packaging( + snapshot_version: SnapshotVersion, + operating_mode: OperatingMode, + ) { solana_logger::setup(); // Set up snapshotting config - let mut snapshot_test_config = SnapshotTestConfig::new(snapshot_version, 1); + let mut snapshot_test_config = SnapshotTestConfig::new(snapshot_version, operating_mode, 1); let bank_forks = &mut snapshot_test_config.bank_forks; let accounts_dir = &snapshot_test_config.accounts_dir; @@ -396,7 +418,10 @@ mod tests { ); } - fn run_test_slots_to_snapshot(snapshot_version: SnapshotVersion) { + fn run_test_slots_to_snapshot( + snapshot_version: SnapshotVersion, + operating_mode: OperatingMode, + ) { solana_logger::setup(); let num_set_roots = MAX_CACHE_ENTRIES * 2; @@ -405,6 +430,7 @@ mod tests { // Make sure this test never clears bank.slots_since_snapshot let mut snapshot_test_config = SnapshotTestConfig::new( snapshot_version, + operating_mode, (*add_root_interval * num_set_roots * 2) as u64, ); let mut current_bank = snapshot_test_config.bank_forks[0].clone(); @@ -438,8 +464,16 @@ mod tests { } } +<<<<<<< HEAD fn run_test_bank_forks_status_cache_snapshot_n(snapshot_version: SnapshotVersion) { // create banks upto slot (MAX_CACHE_ENTRIES * 2) + 1 while transferring 1 lamport into 2 different accounts each time +======= + fn run_test_bank_forks_status_cache_snapshot_n( + snapshot_version: SnapshotVersion, + operating_mode: OperatingMode, + ) { + // create banks up to slot (MAX_CACHE_ENTRIES * 2) + 1 while transferring 1 lamport into 2 different accounts each time +>>>>>>> 23fa84b32... Re-do rent collection check on rent-exempt account (#11349) // this is done to ensure the AccountStorageEntries keep getting cleaned up as the root moves // ahead. Also tests the status_cache purge and status cache snapshotting. // Makes sure that the last bank is restored correctly @@ -448,6 +482,7 @@ mod tests { for set_root_interval in &[1, 4] { run_bank_forks_snapshot_n( snapshot_version, + operating_mode, (MAX_CACHE_ENTRIES * 2 + 1) as u64, |bank, mint_keypair| { let tx = system_transaction::transfer( diff --git a/docs/src/implemented-proposals/rent.md b/docs/src/implemented-proposals/rent.md index d3d357a898de86..649d20b8e558f2 100644 --- a/docs/src/implemented-proposals/rent.md +++ b/docs/src/implemented-proposals/rent.md @@ -24,11 +24,11 @@ Even if those processes are out of scope of rent collection, all of manipulated ## 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`. +Rent is due for one epoch's worth of time, and accounts have `Account::rent_epoch` of `current_epoch` or `current_epoch + 1` depending on the rent regime. -If the account is in the exempt regime, `Account::rent_epoch` is simply pushed to `current_epoch + 1`. +If the account is in the exempt regime, `Account::rent_epoch` is simply updated to `current_epoch`. -If the account is non-exempt, the difference between the next epoch and `Account::rent_epoch` is used to calculate the amount of rent owed by this account \(via `Rent::due()`\). Any fractional lamports of the calculation are truncated. Rent due is deducted from `Account::lamports` and `Account::rent_epoch` is updated to the next epoch. If the amount of rent due is less than one lamport, no changes are made to the account. +If the account is non-exempt, the difference between the next epoch and `Account::rent_epoch` is used to calculate the amount of rent owed by this account \(via `Rent::due()`\). Any fractional lamports of the calculation are truncated. Rent due is deducted from `Account::lamports` and `Account::rent_epoch` is updated to `current_epoch + 1` (= next epoch). If the amount of rent due is less than one lamport, no changes are made to the account. Accounts whose balance is insufficient to satisfy the rent that would be due simply fail to load. diff --git a/programs/bpf/c/src/invoked/invoked.c b/programs/bpf/c/src/invoked/invoked.c index ece9591a5e3ea7..3e72256a604e8a 100644 --- a/programs/bpf/c/src/invoked/invoked.c +++ b/programs/bpf/c/src/invoked/invoked.c @@ -36,7 +36,7 @@ extern uint64_t entrypoint(const uint8_t *input) { sol_assert(accounts[ARGUMENT_INDEX].data_len == 100); sol_assert(accounts[ARGUMENT_INDEX].is_signer); sol_assert(accounts[ARGUMENT_INDEX].is_writable); - sol_assert(accounts[ARGUMENT_INDEX].rent_epoch == 1); + sol_assert(accounts[ARGUMENT_INDEX].rent_epoch == 0); sol_assert(!accounts[ARGUMENT_INDEX].executable); for (int i = 0; i < accounts[ARGUMENT_INDEX].data_len; i++) { sol_assert(accounts[ARGUMENT_INDEX].data[i] == i); @@ -48,7 +48,7 @@ extern uint64_t entrypoint(const uint8_t *input) { sol_assert(accounts[INVOKED_ARGUMENT_INDEX].data_len == 10); sol_assert(accounts[INVOKED_ARGUMENT_INDEX].is_signer); sol_assert(accounts[INVOKED_ARGUMENT_INDEX].is_writable); - sol_assert(accounts[INVOKED_ARGUMENT_INDEX].rent_epoch == 1); + sol_assert(accounts[INVOKED_ARGUMENT_INDEX].rent_epoch == 0); sol_assert(!accounts[INVOKED_ARGUMENT_INDEX].executable); sol_assert( @@ -57,7 +57,7 @@ extern uint64_t entrypoint(const uint8_t *input) { &bpf_loader_id)); sol_assert(!accounts[INVOKED_PROGRAM_INDEX].is_signer); sol_assert(!accounts[INVOKED_PROGRAM_INDEX].is_writable); - sol_assert(accounts[INVOKED_PROGRAM_INDEX].rent_epoch == 1); + sol_assert(accounts[INVOKED_PROGRAM_INDEX].rent_epoch == 0); sol_assert(accounts[INVOKED_PROGRAM_INDEX].executable); sol_assert(SolPubkey_same(accounts[INVOKED_PROGRAM_INDEX].key, diff --git a/programs/bpf/rust/invoked/src/lib.rs b/programs/bpf/rust/invoked/src/lib.rs index 11a1f11c478a33..b976697a88f9bd 100644 --- a/programs/bpf/rust/invoked/src/lib.rs +++ b/programs/bpf/rust/invoked/src/lib.rs @@ -42,7 +42,7 @@ fn process_instruction( assert_eq!(accounts[ARGUMENT_INDEX].data_len(), 100); assert!(accounts[ARGUMENT_INDEX].is_signer); assert!(accounts[ARGUMENT_INDEX].is_writable); - assert_eq!(accounts[ARGUMENT_INDEX].rent_epoch, 1); + assert_eq!(accounts[ARGUMENT_INDEX].rent_epoch, 0); assert!(!accounts[ARGUMENT_INDEX].executable); { let data = accounts[ARGUMENT_INDEX].try_borrow_data()?; @@ -59,14 +59,14 @@ fn process_instruction( assert_eq!(accounts[INVOKED_ARGUMENT_INDEX].data_len(), 10); assert!(accounts[INVOKED_ARGUMENT_INDEX].is_signer); assert!(accounts[INVOKED_ARGUMENT_INDEX].is_writable); - assert_eq!(accounts[INVOKED_ARGUMENT_INDEX].rent_epoch, 1); + assert_eq!(accounts[INVOKED_ARGUMENT_INDEX].rent_epoch, 0); assert!(!accounts[INVOKED_ARGUMENT_INDEX].executable); assert_eq!(accounts[INVOKED_PROGRAM_INDEX].key, program_id); assert_eq!(accounts[INVOKED_PROGRAM_INDEX].owner, &bpf_loader::id()); assert!(!accounts[INVOKED_PROGRAM_INDEX].is_signer); assert!(!accounts[INVOKED_PROGRAM_INDEX].is_writable); - assert_eq!(accounts[INVOKED_PROGRAM_INDEX].rent_epoch, 1); + assert_eq!(accounts[INVOKED_PROGRAM_INDEX].rent_epoch, 0); assert!(accounts[INVOKED_PROGRAM_INDEX].executable); assert_eq!( diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index f1cb4652a7082c..2b2b9a13ca3ea7 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -794,6 +794,7 @@ mod tests { account::Account, epoch_schedule::EpochSchedule, fee_calculator::FeeCalculator, + genesis_config::OperatingMode, hash::Hash, instruction::CompiledInstruction, message::Message, @@ -1027,6 +1028,7 @@ mod tests { lamports_per_byte_year: 42, ..Rent::default() }, + OperatingMode::Development, ); let min_balance = rent_collector.rent.minimum_balance(nonce::State::size()); let fee_calculator = FeeCalculator::new(min_balance); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index fc5ed3478ad20d..b8834e1ac3317f 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -418,7 +418,9 @@ impl Bank { slots_per_year: parent.slots_per_year, epoch_schedule, collected_rent: AtomicU64::new(0), - rent_collector: parent.rent_collector.clone_with_epoch(epoch), + rent_collector: parent + .rent_collector + .clone_with_epoch(epoch, parent.operating_mode()), max_tick_height: (slot + 1) * parent.ticks_per_slot, block_height: parent.block_height + 1, fee_calculator: fee_rate_governor.create_fee_calculator(), @@ -504,6 +506,143 @@ impl Bank { new } +<<<<<<< HEAD +======= + /// Create a bank from explicit arguments and deserialized fields from snapshot + #[allow(clippy::float_cmp)] + pub(crate) fn new_from_fields( + bank_rc: BankRc, + genesis_config: &GenesisConfig, + fields: BankFieldsToDeserialize, + ) -> Self { + fn new() -> T { + T::default() + } + let mut bank = Self { + rc: bank_rc, + src: new(), + blockhash_queue: RwLock::new(fields.blockhash_queue), + ancestors: fields.ancestors, + hash: RwLock::new(fields.hash), + parent_hash: fields.parent_hash, + parent_slot: fields.parent_slot, + hard_forks: Arc::new(RwLock::new(fields.hard_forks)), + transaction_count: AtomicU64::new(fields.transaction_count), + tick_height: AtomicU64::new(fields.tick_height), + signature_count: AtomicU64::new(fields.signature_count), + capitalization: AtomicU64::new(fields.capitalization), + max_tick_height: fields.max_tick_height, + hashes_per_tick: fields.hashes_per_tick, + ticks_per_slot: fields.ticks_per_slot, + ns_per_slot: fields.ns_per_slot, + genesis_creation_time: fields.genesis_creation_time, + slots_per_year: fields.slots_per_year, + unused: genesis_config.unused, + slot: fields.slot, + epoch: fields.epoch, + block_height: fields.block_height, + collector_id: fields.collector_id, + collector_fees: AtomicU64::new(fields.collector_fees), + fee_calculator: fields.fee_calculator, + fee_rate_governor: fields.fee_rate_governor, + collected_rent: AtomicU64::new(fields.collected_rent), + // clone()-ing is needed to consider a gated behavior in rent_collector + rent_collector: fields + .rent_collector + .clone_with_epoch(fields.epoch, genesis_config.operating_mode), + epoch_schedule: fields.epoch_schedule, + inflation: Arc::new(RwLock::new(fields.inflation)), + stakes: RwLock::new(fields.stakes), + epoch_stakes: fields.epoch_stakes, + is_delta: AtomicBool::new(fields.is_delta), + message_processor: new(), + entered_epoch_callback: new(), + last_vote_sync: new(), + rewards: new(), + skip_drop: new(), + operating_mode: Some(genesis_config.operating_mode), + lazy_rent_collection: new(), + }; + bank.finish_init(); + + // Sanity assertions between bank snapshot and genesis config + // Consider removing from serializable bank state ([Ref]BankFields) and initializing + // from the passed in genesis_config instead (as new()/new_with_paths() already do) + assert_eq!( + bank.hashes_per_tick, + genesis_config.poh_config.hashes_per_tick + ); + assert_eq!(bank.ticks_per_slot, genesis_config.ticks_per_slot); + assert_eq!( + bank.ns_per_slot, + genesis_config.poh_config.target_tick_duration.as_nanos() + * genesis_config.ticks_per_slot as u128 + ); + assert_eq!(bank.genesis_creation_time, genesis_config.creation_time); + assert_eq!(bank.unused, genesis_config.unused); + assert_eq!(bank.max_tick_height, (bank.slot + 1) * bank.ticks_per_slot); + assert_eq!( + bank.slots_per_year, + years_as_slots( + 1.0, + &genesis_config.poh_config.target_tick_duration, + bank.ticks_per_slot, + ) + ); + assert_eq!(bank.epoch_schedule, genesis_config.epoch_schedule); + assert_eq!(bank.epoch, bank.epoch_schedule.get_epoch(bank.slot)); + assert_eq!( + bank.rent_collector, + RentCollector::new( + bank.epoch, + &bank.epoch_schedule, + bank.slots_per_year, + &genesis_config.rent, + genesis_config.operating_mode, + ) + ); + + bank + } + + /// Return subset of bank fields representing serializable state + pub(crate) fn get_fields_to_serialize(&self) -> BankFieldsToSerialize { + BankFieldsToSerialize { + blockhash_queue: &self.blockhash_queue, + ancestors: &self.ancestors, + hash: *self.hash.read().unwrap(), + parent_hash: self.parent_hash, + parent_slot: self.parent_slot, + hard_forks: &*self.hard_forks, + transaction_count: self.transaction_count.load(Ordering::Relaxed), + tick_height: self.tick_height.load(Ordering::Relaxed), + signature_count: self.signature_count.load(Ordering::Relaxed), + capitalization: self.capitalization.load(Ordering::Relaxed), + max_tick_height: self.max_tick_height, + hashes_per_tick: self.hashes_per_tick, + ticks_per_slot: self.ticks_per_slot, + ns_per_slot: self.ns_per_slot, + genesis_creation_time: self.genesis_creation_time, + slots_per_year: self.slots_per_year, + unused: self.unused, + slot: self.slot, + epoch: self.epoch, + block_height: self.block_height, + collector_id: self.collector_id, + collector_fees: self.collector_fees.load(Ordering::Relaxed), + fee_calculator: self.fee_calculator.clone(), + fee_rate_governor: self.fee_rate_governor.clone(), + collected_rent: self.collected_rent.load(Ordering::Relaxed), + rent_collector: self.rent_collector.clone(), + epoch_schedule: self.epoch_schedule, + inflation: *self.inflation.read().unwrap(), + stakes: &self.stakes, + epoch_stakes: &self.epoch_stakes, + is_delta: self.is_delta.load(Ordering::Relaxed), + } + } + +>>>>>>> 23fa84b32... Re-do rent collection check on rent-exempt account (#11349) pub fn collector_id(&self) -> &Pubkey { &self.collector_id } @@ -961,6 +1100,7 @@ impl Bank { &self.epoch_schedule, self.slots_per_year, &genesis_config.rent, + self.operating_mode(), ); // Add additional native programs specified in the genesis config @@ -4428,7 +4568,7 @@ mod tests { bank.get_account(&rent_exempt_pubkey).unwrap().lamports, large_lamports ); - assert_eq!(bank.get_account(&rent_exempt_pubkey).unwrap().rent_epoch, 6); + assert_eq!(bank.get_account(&rent_exempt_pubkey).unwrap().rent_epoch, 5); assert_eq!( bank.slots_by_pubkey(&rent_due_pubkey, &ancestors), vec![genesis_slot, some_slot] @@ -7643,19 +7783,31 @@ mod tests { if bank.slot == 32 { assert_eq!( bank.hash().to_string(), +<<<<<<< HEAD "DV3oAdp4qTQr9eU2s9F1mHMypGAHzSrRvkoSgc9Tgr5R" +======= + "GDH7kUpcQuMT23pPeU9vZdmyMSPQPwzoqdNgFaLga7x3" +>>>>>>> 23fa84b32... Re-do rent collection check on rent-exempt account (#11349) ); } if bank.slot == 64 { assert_eq!( bank.hash().to_string(), +<<<<<<< HEAD "3hRFavWgtFfhKojc8hJqeJWMcyxdt8DbXWME3p6Zes36" +======= + "J4L6bT3KnMMXSufcUSy6Lg9TNi2pFVsYNvQ1Fzms2j1Z" +>>>>>>> 23fa84b32... Re-do rent collection check on rent-exempt account (#11349) ); } if bank.slot == 128 { assert_eq!( bank.hash().to_string(), +<<<<<<< HEAD "4pXHMCcRwVJgkKNMysho5riYNhdS4JAj1budis41EopU" +======= + "BiCUyj8PsbsLW79waf1ifr3wDuZSFwLBhTkdbgHFjrtJ" +>>>>>>> 23fa84b32... Re-do rent collection check on rent-exempt account (#11349) ); break; } diff --git a/runtime/src/rent_collector.rs b/runtime/src/rent_collector.rs index cc3b648b525759..d7af38d5dc667f 100644 --- a/runtime/src/rent_collector.rs +++ b/runtime/src/rent_collector.rs @@ -1,7 +1,13 @@ //! calculate and collect rent from Accounts use solana_sdk::{ - account::Account, clock::Epoch, epoch_schedule::EpochSchedule, genesis_config::GenesisConfig, - incinerator, pubkey::Pubkey, rent::Rent, sysvar, + account::Account, + clock::Epoch, + epoch_schedule::EpochSchedule, + genesis_config::{GenesisConfig, OperatingMode}, + incinerator, + pubkey::Pubkey, + rent::Rent, + sysvar, }; #[derive(Serialize, Deserialize, Clone)] @@ -10,6 +16,11 @@ pub struct RentCollector { pub epoch_schedule: EpochSchedule, pub slots_per_year: f64, 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 operating_mode: Option, } impl Default for RentCollector { @@ -20,6 +31,7 @@ impl Default for RentCollector { // derive default value using GenesisConfig::default() slots_per_year: GenesisConfig::default().slots_per_year(), rent: Rent::default(), + operating_mode: Option::default(), } } } @@ -30,21 +42,33 @@ impl RentCollector { epoch_schedule: &EpochSchedule, slots_per_year: f64, rent: &Rent, + operating_mode: OperatingMode, ) -> Self { Self { epoch, epoch_schedule: *epoch_schedule, slots_per_year, rent: *rent, + operating_mode: Some(operating_mode), } } - pub fn clone_with_epoch(&self, epoch: Epoch) -> Self { + pub fn clone_with_epoch(&self, epoch: Epoch, operating_mode: OperatingMode) -> Self { Self { epoch, + operating_mode: Some(operating_mode), ..self.clone() } } + + fn enable_new_behavior(&self) -> bool { + match self.operating_mode.unwrap() { + OperatingMode::Development => true, + OperatingMode::Preview => self.epoch >= Epoch::max_value(), + OperatingMode::Stable => self.epoch >= Epoch::max_value(), + } + } + // updates this account's lamports and status and returns // the account rent collected, if any // @@ -69,7 +93,15 @@ impl RentCollector { if exempt || rent_due != 0 { if account.lamports > rent_due { - account.rent_epoch = self.epoch + 1; + account.rent_epoch = self.epoch + + if self.enable_new_behavior() && exempt { + // Rent isn't collected for the next epoch + // Make sure to check exempt status later in curent epoch again + 0 + } else { + // Rent is collected for next epoch + 1 + }; account.lamports -= rent_due; rent_due } else { @@ -110,21 +142,55 @@ mod tests { (account.clone(), account) }; - let rent_collector = RentCollector::default().clone_with_epoch(new_epoch); + let rent_collector = + RentCollector::default().clone_with_epoch(new_epoch, OperatingMode::Development); + // collect rent on a newly-created account 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); + // collect rent on a already-existing account 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); + // newly created account should be collected for less rent; thus more remaining balance assert!(created_account.lamports > existing_account.lamports); assert_eq!(created_account.rent_epoch, existing_account.rent_epoch); } + + #[test] + fn test_rent_exempt_temporal_escape() { + let mut account = Account::default(); + let epoch = 3; + let huge_lamports = 123_456_789_012; + let tiny_lamports = 789_012; + let mut collected; + let pubkey = Pubkey::new_rand(); + + account.lamports = huge_lamports; + assert_eq!(account.rent_epoch, 0); + + // create a tested rent collector + let rent_collector = + RentCollector::default().clone_with_epoch(epoch, OperatingMode::Development); + + // first mark account as being collected while being rent-exempt + collected = rent_collector.collect_from_existing_account(&pubkey, &mut account); + assert_eq!(account.lamports, huge_lamports); + assert_eq!(collected, 0); + + // decrease the balance not to be rent-exempt + account.lamports = tiny_lamports; + + // ... 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); + assert_eq!(account.lamports, tiny_lamports - collected); + assert_ne!(collected, 0); + } } From 241fd7bd6563a2574d1b0abd133d0907e37cd466 Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Mon, 17 Aug 2020 15:24:56 +0900 Subject: [PATCH 2/3] Fix conflicts --- core/src/rpc_subscriptions.rs | 20 ---- core/tests/bank_forks.rs | 33 +++---- ledger/src/snapshot_utils.rs | 1 + runtime/src/bank.rs | 173 +++++----------------------------- runtime/src/rent_collector.rs | 2 +- 5 files changed, 35 insertions(+), 194 deletions(-) diff --git a/core/src/rpc_subscriptions.rs b/core/src/rpc_subscriptions.rs index 6391fa5f94893b..8a92836695fc66 100644 --- a/core/src/rpc_subscriptions.rs +++ b/core/src/rpc_subscriptions.rs @@ -1098,13 +1098,8 @@ pub(crate) mod tests { "data": "1111111111111111", "executable": false, "lamports": 1, -<<<<<<< HEAD "owner": "Budget1111111111111111111111111111111111111", - "rentEpoch": 1, -======= - "owner": "Stake11111111111111111111111111111111111111", "rentEpoch": 0, ->>>>>>> 23fa84b32... Re-do rent collection check on rent-exempt account (#11349) }, }, "subscription": 0, @@ -1189,13 +1184,8 @@ pub(crate) mod tests { "data": "1111111111111111", "executable": false, "lamports": 1, -<<<<<<< HEAD "owner": "Budget1111111111111111111111111111111111111", - "rentEpoch": 1, -======= - "owner": "Stake11111111111111111111111111111111111111", "rentEpoch": 0, ->>>>>>> 23fa84b32... Re-do rent collection check on rent-exempt account (#11349) }, "pubkey": alice.pubkey().to_string(), }, @@ -1616,13 +1606,8 @@ pub(crate) mod tests { "data": "1111111111111111", "executable": false, "lamports": 1, -<<<<<<< HEAD "owner": "Budget1111111111111111111111111111111111111", - "rentEpoch": 1, -======= - "owner": "Stake11111111111111111111111111111111111111", "rentEpoch": 0, ->>>>>>> 23fa84b32... Re-do rent collection check on rent-exempt account (#11349) }, }, "subscription": 0, @@ -1655,13 +1640,8 @@ pub(crate) mod tests { "data": "1111111111111111", "executable": false, "lamports": 1, -<<<<<<< HEAD "owner": "Budget1111111111111111111111111111111111111", - "rentEpoch": 1, -======= - "owner": "Stake11111111111111111111111111111111111111", "rentEpoch": 0, ->>>>>>> 23fa84b32... Re-do rent collection check on rent-exempt account (#11349) }, }, "subscription": 1, diff --git a/core/tests/bank_forks.rs b/core/tests/bank_forks.rs index 28bf5f5f26f89e..c07f6c837fed29 100644 --- a/core/tests/bank_forks.rs +++ b/core/tests/bank_forks.rs @@ -62,14 +62,12 @@ mod tests { use std::{fs, path::PathBuf, sync::atomic::AtomicBool, sync::mpsc::channel, sync::Arc}; use tempfile::TempDir; -<<<<<<< HEAD - DEFINE_SNAPSHOT_VERSION_PARAMETERIZED_TEST_FUNCTIONS!(V1_1_0); - DEFINE_SNAPSHOT_VERSION_PARAMETERIZED_TEST_FUNCTIONS!(V1_2_0); -======= + DEFINE_SNAPSHOT_VERSION_PARAMETERIZED_TEST_FUNCTIONS!(V1_1_0, Development, V1_1_0_Development); + DEFINE_SNAPSHOT_VERSION_PARAMETERIZED_TEST_FUNCTIONS!(V1_1_0, Preview, V1_1_0_Preview); + DEFINE_SNAPSHOT_VERSION_PARAMETERIZED_TEST_FUNCTIONS!(V1_1_0, Stable, V1_1_0_Stable); DEFINE_SNAPSHOT_VERSION_PARAMETERIZED_TEST_FUNCTIONS!(V1_2_0, Development, V1_2_0_Development); DEFINE_SNAPSHOT_VERSION_PARAMETERIZED_TEST_FUNCTIONS!(V1_2_0, Preview, V1_2_0_Preview); DEFINE_SNAPSHOT_VERSION_PARAMETERIZED_TEST_FUNCTIONS!(V1_2_0, Stable, V1_2_0_Stable); ->>>>>>> 23fa84b32... Re-do rent collection check on rent-exempt account (#11349) struct SnapshotTestConfig { accounts_dir: TempDir, @@ -122,6 +120,7 @@ mod tests { fn restore_from_snapshot( old_bank_forks: &BankForks, old_last_slot: Slot, + old_genesis_config: &GenesisConfig, account_paths: &[PathBuf], ) { let (snapshot_path, snapshot_package_output_path) = old_bank_forks @@ -146,7 +145,7 @@ mod tests { &CompressionType::Bzip2, ), CompressionType::Bzip2, - &GenesisConfig::default(), + old_genesis_config, ) .unwrap(); @@ -182,7 +181,6 @@ mod tests { let mut snapshot_test_config = SnapshotTestConfig::new(snapshot_version, operating_mode, 1); let bank_forks = &mut snapshot_test_config.bank_forks; - let accounts_dir = &snapshot_test_config.accounts_dir; let mint_keypair = &snapshot_test_config.genesis_config_info.mint_keypair; let (s, _r) = channel(); @@ -198,6 +196,7 @@ mod tests { bank_forks.set_root(bank.slot(), &sender, None); } } + // Generate a snapshot package for last bank let last_bank = bank_forks.get(last_slot).unwrap(); let snapshot_config = &snapshot_test_config.snapshot_config; @@ -216,22 +215,19 @@ mod tests { snapshot_version, ) .unwrap(); - snapshot_utils::archive_snapshot_package(&snapshot_package).unwrap(); - restore_from_snapshot(bank_forks, last_slot, &[accounts_dir.path().to_path_buf()]); + // Restore bank from snapshot + let account_paths = &[snapshot_test_config.accounts_dir.path().to_path_buf()]; + let genesis_config = &snapshot_test_config.genesis_config_info.genesis_config; + restore_from_snapshot(bank_forks, last_slot, genesis_config, account_paths); } -<<<<<<< HEAD - fn run_test_bank_forks_snapshot_n(snapshot_version: SnapshotVersion) { - // create banks upto slot 4 and create 1 new account in each bank. test that bank 4 snapshots -======= fn run_test_bank_forks_snapshot_n( snapshot_version: SnapshotVersion, operating_mode: OperatingMode, ) { - // create banks up to slot 4 and create 1 new account in each bank. test that bank 4 snapshots ->>>>>>> 23fa84b32... Re-do rent collection check on rent-exempt account (#11349) + // create banks upto slot 4 and create 1 new account in each bank. test that bank 4 snapshots // and restores correctly run_bank_forks_snapshot_n( snapshot_version, @@ -464,16 +460,11 @@ mod tests { } } -<<<<<<< HEAD - fn run_test_bank_forks_status_cache_snapshot_n(snapshot_version: SnapshotVersion) { - // create banks upto slot (MAX_CACHE_ENTRIES * 2) + 1 while transferring 1 lamport into 2 different accounts each time -======= fn run_test_bank_forks_status_cache_snapshot_n( snapshot_version: SnapshotVersion, operating_mode: OperatingMode, ) { - // create banks up to slot (MAX_CACHE_ENTRIES * 2) + 1 while transferring 1 lamport into 2 different accounts each time ->>>>>>> 23fa84b32... Re-do rent collection check on rent-exempt account (#11349) + // create banks upto slot (MAX_CACHE_ENTRIES * 2) + 1 while transferring 1 lamport into 2 different accounts each time // this is done to ensure the AccountStorageEntries keep getting cleaned up as the root moves // ahead. Also tests the status_cache purge and status cache snapshotting. // Makes sure that the last bank is restored correctly diff --git a/ledger/src/snapshot_utils.rs b/ledger/src/snapshot_utils.rs index 86ab71f0ae75c6..443bfc821a19ed 100644 --- a/ledger/src/snapshot_utils.rs +++ b/ledger/src/snapshot_utils.rs @@ -749,6 +749,7 @@ where bank.rc = bankrc; bank.operating_mode = Some(genesis_config.operating_mode); + bank.init_rent_collector_after_deserialize(genesis_config); bank.finish_init(); Ok(bank) })?; diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index b8834e1ac3317f..591713221bdb80 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -506,143 +506,6 @@ impl Bank { new } -<<<<<<< HEAD -======= - /// Create a bank from explicit arguments and deserialized fields from snapshot - #[allow(clippy::float_cmp)] - pub(crate) fn new_from_fields( - bank_rc: BankRc, - genesis_config: &GenesisConfig, - fields: BankFieldsToDeserialize, - ) -> Self { - fn new() -> T { - T::default() - } - let mut bank = Self { - rc: bank_rc, - src: new(), - blockhash_queue: RwLock::new(fields.blockhash_queue), - ancestors: fields.ancestors, - hash: RwLock::new(fields.hash), - parent_hash: fields.parent_hash, - parent_slot: fields.parent_slot, - hard_forks: Arc::new(RwLock::new(fields.hard_forks)), - transaction_count: AtomicU64::new(fields.transaction_count), - tick_height: AtomicU64::new(fields.tick_height), - signature_count: AtomicU64::new(fields.signature_count), - capitalization: AtomicU64::new(fields.capitalization), - max_tick_height: fields.max_tick_height, - hashes_per_tick: fields.hashes_per_tick, - ticks_per_slot: fields.ticks_per_slot, - ns_per_slot: fields.ns_per_slot, - genesis_creation_time: fields.genesis_creation_time, - slots_per_year: fields.slots_per_year, - unused: genesis_config.unused, - slot: fields.slot, - epoch: fields.epoch, - block_height: fields.block_height, - collector_id: fields.collector_id, - collector_fees: AtomicU64::new(fields.collector_fees), - fee_calculator: fields.fee_calculator, - fee_rate_governor: fields.fee_rate_governor, - collected_rent: AtomicU64::new(fields.collected_rent), - // clone()-ing is needed to consider a gated behavior in rent_collector - rent_collector: fields - .rent_collector - .clone_with_epoch(fields.epoch, genesis_config.operating_mode), - epoch_schedule: fields.epoch_schedule, - inflation: Arc::new(RwLock::new(fields.inflation)), - stakes: RwLock::new(fields.stakes), - epoch_stakes: fields.epoch_stakes, - is_delta: AtomicBool::new(fields.is_delta), - message_processor: new(), - entered_epoch_callback: new(), - last_vote_sync: new(), - rewards: new(), - skip_drop: new(), - operating_mode: Some(genesis_config.operating_mode), - lazy_rent_collection: new(), - }; - bank.finish_init(); - - // Sanity assertions between bank snapshot and genesis config - // Consider removing from serializable bank state ([Ref]BankFields) and initializing - // from the passed in genesis_config instead (as new()/new_with_paths() already do) - assert_eq!( - bank.hashes_per_tick, - genesis_config.poh_config.hashes_per_tick - ); - assert_eq!(bank.ticks_per_slot, genesis_config.ticks_per_slot); - assert_eq!( - bank.ns_per_slot, - genesis_config.poh_config.target_tick_duration.as_nanos() - * genesis_config.ticks_per_slot as u128 - ); - assert_eq!(bank.genesis_creation_time, genesis_config.creation_time); - assert_eq!(bank.unused, genesis_config.unused); - assert_eq!(bank.max_tick_height, (bank.slot + 1) * bank.ticks_per_slot); - assert_eq!( - bank.slots_per_year, - years_as_slots( - 1.0, - &genesis_config.poh_config.target_tick_duration, - bank.ticks_per_slot, - ) - ); - assert_eq!(bank.epoch_schedule, genesis_config.epoch_schedule); - assert_eq!(bank.epoch, bank.epoch_schedule.get_epoch(bank.slot)); - assert_eq!( - bank.rent_collector, - RentCollector::new( - bank.epoch, - &bank.epoch_schedule, - bank.slots_per_year, - &genesis_config.rent, - genesis_config.operating_mode, - ) - ); - - bank - } - - /// Return subset of bank fields representing serializable state - pub(crate) fn get_fields_to_serialize(&self) -> BankFieldsToSerialize { - BankFieldsToSerialize { - blockhash_queue: &self.blockhash_queue, - ancestors: &self.ancestors, - hash: *self.hash.read().unwrap(), - parent_hash: self.parent_hash, - parent_slot: self.parent_slot, - hard_forks: &*self.hard_forks, - transaction_count: self.transaction_count.load(Ordering::Relaxed), - tick_height: self.tick_height.load(Ordering::Relaxed), - signature_count: self.signature_count.load(Ordering::Relaxed), - capitalization: self.capitalization.load(Ordering::Relaxed), - max_tick_height: self.max_tick_height, - hashes_per_tick: self.hashes_per_tick, - ticks_per_slot: self.ticks_per_slot, - ns_per_slot: self.ns_per_slot, - genesis_creation_time: self.genesis_creation_time, - slots_per_year: self.slots_per_year, - unused: self.unused, - slot: self.slot, - epoch: self.epoch, - block_height: self.block_height, - collector_id: self.collector_id, - collector_fees: self.collector_fees.load(Ordering::Relaxed), - fee_calculator: self.fee_calculator.clone(), - fee_rate_governor: self.fee_rate_governor.clone(), - collected_rent: self.collected_rent.load(Ordering::Relaxed), - rent_collector: self.rent_collector.clone(), - epoch_schedule: self.epoch_schedule, - inflation: *self.inflation.read().unwrap(), - stakes: &self.stakes, - epoch_stakes: &self.epoch_stakes, - is_delta: self.is_delta.load(Ordering::Relaxed), - } - } - ->>>>>>> 23fa84b32... Re-do rent collection check on rent-exempt account (#11349) pub fn collector_id(&self) -> &Pubkey { &self.collector_id } @@ -2471,6 +2334,24 @@ impl Bank { } } + pub fn init_rent_collector_after_deserialize(&mut self, genesis_config: &GenesisConfig) { + self.rent_collector = self + .rent_collector + .clone_with_epoch(self.epoch(), genesis_config.operating_mode); + + // This is minimal backport of https://github.com/solana-labs/solana/pull/10581, needed by https://github.com/solana-labs/solana/pull/11349 + assert_eq!( + self.rent_collector, + RentCollector::new( + self.epoch, + &self.epoch_schedule, + self.slots_per_year, + &genesis_config.rent, + genesis_config.operating_mode, + ) + ); + } + pub fn set_parent(&mut self, parent: &Arc) { self.rc.parent = RwLock::new(Some(parent.clone())); } @@ -7783,31 +7664,19 @@ mod tests { if bank.slot == 32 { assert_eq!( bank.hash().to_string(), -<<<<<<< HEAD - "DV3oAdp4qTQr9eU2s9F1mHMypGAHzSrRvkoSgc9Tgr5R" -======= - "GDH7kUpcQuMT23pPeU9vZdmyMSPQPwzoqdNgFaLga7x3" ->>>>>>> 23fa84b32... Re-do rent collection check on rent-exempt account (#11349) + "9rsnmUCTg2wFnKVnaUSEQcSicvPUyXPTmbsakB2o3eTu" ); } if bank.slot == 64 { assert_eq!( bank.hash().to_string(), -<<<<<<< HEAD - "3hRFavWgtFfhKojc8hJqeJWMcyxdt8DbXWME3p6Zes36" -======= - "J4L6bT3KnMMXSufcUSy6Lg9TNi2pFVsYNvQ1Fzms2j1Z" ->>>>>>> 23fa84b32... Re-do rent collection check on rent-exempt account (#11349) + "B1JNAeiR89kcqwqeSSQXFN3spcYW2qVJwuu9hQ9dxn1V" ); } if bank.slot == 128 { assert_eq!( bank.hash().to_string(), -<<<<<<< HEAD - "4pXHMCcRwVJgkKNMysho5riYNhdS4JAj1budis41EopU" -======= - "BiCUyj8PsbsLW79waf1ifr3wDuZSFwLBhTkdbgHFjrtJ" ->>>>>>> 23fa84b32... Re-do rent collection check on rent-exempt account (#11349) + "9iUEmDnWQmjtkoMuR3hJAwQMpCGwHaJAEsfK131rww8y" ); break; } diff --git a/runtime/src/rent_collector.rs b/runtime/src/rent_collector.rs index d7af38d5dc667f..7a63d26941bd93 100644 --- a/runtime/src/rent_collector.rs +++ b/runtime/src/rent_collector.rs @@ -10,7 +10,7 @@ use solana_sdk::{ sysvar, }; -#[derive(Serialize, Deserialize, Clone)] +#[derive(Serialize, Debug, Deserialize, Clone, PartialEq)] pub struct RentCollector { pub epoch: Epoch, pub epoch_schedule: EpochSchedule, From 341490a245134cdf38bbbf02b3c4e410fd7688bc Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Mon, 17 Aug 2020 18:43:28 +0900 Subject: [PATCH 3/3] Add missing comment --- runtime/src/bank.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 591713221bdb80..326681f86eafd6 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -2335,6 +2335,7 @@ impl Bank { } pub fn init_rent_collector_after_deserialize(&mut self, genesis_config: &GenesisConfig) { + // clone()-ing is needed to consider a gated behavior in rent_collector self.rent_collector = self .rent_collector .clone_with_epoch(self.epoch(), genesis_config.operating_mode);