Add a feature to disable rent collection (#33945)
* add a feature to disable rent collection * fix a test * fix a test * rekey * should collect rent * Update runtime/src/bank/fee_distribution.rs Co-authored-by: Brooks <brooks@prumo.org> * expand tests to cover both rent collection disabled and enabled * feedbacks * reviews - move should collect rent check out of rent collector into bank * enforce rent_epoch to u64:max when rent collection is disabled * review feedbacks and fix a test When rent fee collection is disabled, we won't collect rent for any account. If there are any rent paying accounts, their `rent_epoch` won't change too. * revise comments * update rent_epoch for rent exempted account * rebase * set rent_epoch in rent collection for rent exempted account * revert test change * don't assert --------- Co-authored-by: HaoranYi <haoran.yi@solana.com> Co-authored-by: Brooks <brooks@prumo.org>
This commit is contained in:
parent
87d20aecbe
commit
60fdd85aed
|
@ -50,6 +50,7 @@ use {
|
||||||
State as NonceState,
|
State as NonceState,
|
||||||
},
|
},
|
||||||
pubkey::Pubkey,
|
pubkey::Pubkey,
|
||||||
|
rent::RentDue,
|
||||||
saturating_add_assign,
|
saturating_add_assign,
|
||||||
slot_hashes::SlotHashes,
|
slot_hashes::SlotHashes,
|
||||||
sysvar::{self, instructions::construct_instructions_data},
|
sysvar::{self, instructions::construct_instructions_data},
|
||||||
|
@ -314,6 +315,7 @@ impl Accounts {
|
||||||
reward_interval: RewardInterval,
|
reward_interval: RewardInterval,
|
||||||
program_accounts: &HashMap<Pubkey, (&Pubkey, u64)>,
|
program_accounts: &HashMap<Pubkey, (&Pubkey, u64)>,
|
||||||
loaded_programs: &LoadedProgramsForTxBatch,
|
loaded_programs: &LoadedProgramsForTxBatch,
|
||||||
|
should_collect_rent: bool,
|
||||||
) -> Result<LoadedTransaction> {
|
) -> Result<LoadedTransaction> {
|
||||||
let in_reward_interval = reward_interval == RewardInterval::InsideInterval;
|
let in_reward_interval = reward_interval == RewardInterval::InsideInterval;
|
||||||
|
|
||||||
|
@ -382,15 +384,31 @@ impl Accounts {
|
||||||
.load_with_fixed_root(ancestors, key)
|
.load_with_fixed_root(ancestors, key)
|
||||||
.map(|(mut account, _)| {
|
.map(|(mut account, _)| {
|
||||||
if message.is_writable(i) {
|
if message.is_writable(i) {
|
||||||
let rent_due = rent_collector
|
if should_collect_rent {
|
||||||
.collect_from_existing_account(
|
let rent_due = rent_collector
|
||||||
key,
|
.collect_from_existing_account(
|
||||||
&mut account,
|
key,
|
||||||
self.accounts_db.filler_account_suffix.as_ref(),
|
&mut account,
|
||||||
set_exempt_rent_epoch_max,
|
self.accounts_db.filler_account_suffix.as_ref(),
|
||||||
)
|
set_exempt_rent_epoch_max,
|
||||||
.rent_amount;
|
)
|
||||||
(account.data().len(), account, rent_due)
|
.rent_amount;
|
||||||
|
|
||||||
|
(account.data().len(), account, rent_due)
|
||||||
|
} else {
|
||||||
|
// When rent fee collection is disabled, we won't collect rent for any account. If there
|
||||||
|
// are any rent paying accounts, their `rent_epoch` won't change either. However, if the
|
||||||
|
// account itself is rent-exempted but its `rent_epoch` is not u64::MAX, we will set its
|
||||||
|
// `rent_epoch` to u64::MAX. In such case, the behavior stays the same as before.
|
||||||
|
if set_exempt_rent_epoch_max
|
||||||
|
&& (account.rent_epoch() != RENT_EXEMPT_RENT_EPOCH
|
||||||
|
&& rent_collector.get_rent_due(&account)
|
||||||
|
== RentDue::Exempt)
|
||||||
|
{
|
||||||
|
account.set_rent_epoch(RENT_EXEMPT_RENT_EPOCH);
|
||||||
|
}
|
||||||
|
(account.data().len(), account, 0)
|
||||||
|
}
|
||||||
} else {
|
} else {
|
||||||
(account.data().len(), account, 0)
|
(account.data().len(), account, 0)
|
||||||
}
|
}
|
||||||
|
@ -641,6 +659,7 @@ impl Accounts {
|
||||||
in_reward_interval: RewardInterval,
|
in_reward_interval: RewardInterval,
|
||||||
program_accounts: &HashMap<Pubkey, (&Pubkey, u64)>,
|
program_accounts: &HashMap<Pubkey, (&Pubkey, u64)>,
|
||||||
loaded_programs: &LoadedProgramsForTxBatch,
|
loaded_programs: &LoadedProgramsForTxBatch,
|
||||||
|
should_collect_rent: bool,
|
||||||
) -> Vec<TransactionLoadResult> {
|
) -> Vec<TransactionLoadResult> {
|
||||||
txs.iter()
|
txs.iter()
|
||||||
.zip(lock_results)
|
.zip(lock_results)
|
||||||
|
@ -675,6 +694,7 @@ impl Accounts {
|
||||||
in_reward_interval,
|
in_reward_interval,
|
||||||
program_accounts,
|
program_accounts,
|
||||||
loaded_programs,
|
loaded_programs,
|
||||||
|
should_collect_rent,
|
||||||
) {
|
) {
|
||||||
Ok(loaded_transaction) => loaded_transaction,
|
Ok(loaded_transaction) => loaded_transaction,
|
||||||
Err(e) => return (Err(e), None),
|
Err(e) => return (Err(e), None),
|
||||||
|
@ -1498,6 +1518,7 @@ mod tests {
|
||||||
RewardInterval::OutsideInterval,
|
RewardInterval::OutsideInterval,
|
||||||
&HashMap::new(),
|
&HashMap::new(),
|
||||||
&LoadedProgramsForTxBatch::default(),
|
&LoadedProgramsForTxBatch::default(),
|
||||||
|
true,
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -3097,6 +3118,7 @@ mod tests {
|
||||||
RewardInterval::OutsideInterval,
|
RewardInterval::OutsideInterval,
|
||||||
&HashMap::new(),
|
&HashMap::new(),
|
||||||
&LoadedProgramsForTxBatch::default(),
|
&LoadedProgramsForTxBatch::default(),
|
||||||
|
true,
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -89,7 +89,7 @@ use {
|
||||||
epoch_accounts_hash::EpochAccountsHash,
|
epoch_accounts_hash::EpochAccountsHash,
|
||||||
nonce_info::{NonceInfo, NoncePartial},
|
nonce_info::{NonceInfo, NoncePartial},
|
||||||
partitioned_rewards::PartitionedEpochRewardsConfig,
|
partitioned_rewards::PartitionedEpochRewardsConfig,
|
||||||
rent_collector::{CollectedInfo, RentCollector},
|
rent_collector::{CollectedInfo, RentCollector, RENT_EXEMPT_RENT_EPOCH},
|
||||||
rent_debits::RentDebits,
|
rent_debits::RentDebits,
|
||||||
sorted_storages::SortedStorages,
|
sorted_storages::SortedStorages,
|
||||||
stake_rewards::{RewardInfo, StakeReward},
|
stake_rewards::{RewardInfo, StakeReward},
|
||||||
|
@ -160,6 +160,7 @@ use {
|
||||||
packet::PACKET_DATA_SIZE,
|
packet::PACKET_DATA_SIZE,
|
||||||
precompiles::get_precompiles,
|
precompiles::get_precompiles,
|
||||||
pubkey::Pubkey,
|
pubkey::Pubkey,
|
||||||
|
rent::RentDue,
|
||||||
saturating_add_assign,
|
saturating_add_assign,
|
||||||
signature::{Keypair, Signature},
|
signature::{Keypair, Signature},
|
||||||
slot_hashes::SlotHashes,
|
slot_hashes::SlotHashes,
|
||||||
|
@ -5255,6 +5256,7 @@ impl Bank {
|
||||||
self.get_reward_interval(),
|
self.get_reward_interval(),
|
||||||
&program_accounts_map,
|
&program_accounts_map,
|
||||||
&programs_loaded_for_tx_batch.borrow(),
|
&programs_loaded_for_tx_batch.borrow(),
|
||||||
|
self.should_collect_rent(),
|
||||||
);
|
);
|
||||||
load_time.stop();
|
load_time.stop();
|
||||||
|
|
||||||
|
@ -5890,6 +5892,13 @@ impl Bank {
|
||||||
.is_active(&feature_set::skip_rent_rewrites::id())
|
.is_active(&feature_set::skip_rent_rewrites::id())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// true if rent fees should be collected (i.e. disable_rent_fees_collection is NOT enabled)
|
||||||
|
fn should_collect_rent(&self) -> bool {
|
||||||
|
!self
|
||||||
|
.feature_set
|
||||||
|
.is_active(&feature_set::disable_rent_fees_collection::id())
|
||||||
|
}
|
||||||
|
|
||||||
/// Collect rent from `accounts`
|
/// Collect rent from `accounts`
|
||||||
///
|
///
|
||||||
/// This fn is called inside a parallel loop from `collect_rent_in_partition()`. Avoid adding
|
/// This fn is called inside a parallel loop from `collect_rent_in_partition()`. Avoid adding
|
||||||
|
@ -5923,15 +5932,29 @@ impl Bank {
|
||||||
.is_active(&solana_sdk::feature_set::set_exempt_rent_epoch_max::id());
|
.is_active(&solana_sdk::feature_set::set_exempt_rent_epoch_max::id());
|
||||||
let mut skipped_rewrites = Vec::default();
|
let mut skipped_rewrites = Vec::default();
|
||||||
for (pubkey, account, _loaded_slot) in accounts.iter_mut() {
|
for (pubkey, account, _loaded_slot) in accounts.iter_mut() {
|
||||||
let (rent_collected_info, measure) =
|
let rent_collected_info = if self.should_collect_rent() {
|
||||||
measure!(self.rent_collector.collect_from_existing_account(
|
let (rent_collected_info, measure) =
|
||||||
pubkey,
|
measure!(self.rent_collector.collect_from_existing_account(
|
||||||
account,
|
pubkey,
|
||||||
self.rc.accounts.accounts_db.filler_account_suffix.as_ref(),
|
account,
|
||||||
set_exempt_rent_epoch_max,
|
self.rc.accounts.accounts_db.filler_account_suffix.as_ref(),
|
||||||
));
|
set_exempt_rent_epoch_max,
|
||||||
time_collecting_rent_us += measure.as_us();
|
));
|
||||||
|
time_collecting_rent_us += measure.as_us();
|
||||||
|
rent_collected_info
|
||||||
|
} else {
|
||||||
|
// When rent fee collection is disabled, we won't collect rent for any account. If there
|
||||||
|
// are any rent paying accounts, their `rent_epoch` won't change either. However, if the
|
||||||
|
// account itself is rent-exempted but its `rent_epoch` is not u64::MAX, we will set its
|
||||||
|
// `rent_epoch` to u64::MAX. In such case, the behavior stays the same as before.
|
||||||
|
if set_exempt_rent_epoch_max
|
||||||
|
&& (account.rent_epoch() != RENT_EXEMPT_RENT_EPOCH
|
||||||
|
&& self.rent_collector.get_rent_due(account) == RentDue::Exempt)
|
||||||
|
{
|
||||||
|
account.set_rent_epoch(RENT_EXEMPT_RENT_EPOCH);
|
||||||
|
}
|
||||||
|
CollectedInfo::default()
|
||||||
|
};
|
||||||
// only store accounts where we collected rent
|
// only store accounts where we collected rent
|
||||||
// but get the hash for all these accounts even if collected rent is 0 (= not updated).
|
// but get the hash for all these accounts even if collected rent is 0 (= not updated).
|
||||||
// Also, there's another subtle side-effect from rewrites: this
|
// Also, there's another subtle side-effect from rewrites: this
|
||||||
|
|
|
@ -273,6 +273,13 @@ impl Bank {
|
||||||
pub(super) fn distribute_rent_fees(&self) {
|
pub(super) fn distribute_rent_fees(&self) {
|
||||||
let total_rent_collected = self.collected_rent.load(Relaxed);
|
let total_rent_collected = self.collected_rent.load(Relaxed);
|
||||||
|
|
||||||
|
if !self.should_collect_rent() {
|
||||||
|
if total_rent_collected != 0 {
|
||||||
|
warn!("Rent fees collection is disabled, yet total rent collect was non zero! Total rent collected: {total_rent_collected}");
|
||||||
|
}
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
let (burned_portion, rent_to_be_distributed) = self
|
let (burned_portion, rent_to_be_distributed) = self
|
||||||
.rent_collector
|
.rent_collector
|
||||||
.rent
|
.rent
|
||||||
|
|
|
@ -1578,13 +1578,17 @@ impl Bank {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test_case(true; "enable rent fees collection")]
|
||||||
fn test_rent_eager_collect_rent_in_partition() {
|
#[test_case(false; "disable rent fees collection")]
|
||||||
|
fn test_rent_eager_collect_rent_in_partition(should_collect_rent: bool) {
|
||||||
solana_logger::setup();
|
solana_logger::setup();
|
||||||
|
|
||||||
let (mut genesis_config, _mint_keypair) = create_genesis_config(1_000_000);
|
let (mut genesis_config, _mint_keypair) = create_genesis_config(1_000_000);
|
||||||
for feature_id in FeatureSet::default().inactive {
|
for feature_id in FeatureSet::default().inactive {
|
||||||
if feature_id != solana_sdk::feature_set::set_exempt_rent_epoch_max::id() {
|
if feature_id != solana_sdk::feature_set::set_exempt_rent_epoch_max::id()
|
||||||
|
&& (should_collect_rent
|
||||||
|
|| feature_id != solana_sdk::feature_set::disable_rent_fees_collection::id())
|
||||||
|
{
|
||||||
activate_feature(&mut genesis_config, feature_id);
|
activate_feature(&mut genesis_config, feature_id);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
@ -1598,7 +1602,11 @@ fn test_rent_eager_collect_rent_in_partition() {
|
||||||
let large_lamports = 123_456_789;
|
let large_lamports = 123_456_789;
|
||||||
// genesis_config.epoch_schedule.slots_per_epoch == 432_000 and is unsuitable for this test
|
// genesis_config.epoch_schedule.slots_per_epoch == 432_000 and is unsuitable for this test
|
||||||
let some_slot = MINIMUM_SLOTS_PER_EPOCH; // chosen to cause epoch to be +1
|
let some_slot = MINIMUM_SLOTS_PER_EPOCH; // chosen to cause epoch to be +1
|
||||||
let rent_collected = 1; // this is a function of 'some_slot'
|
let rent_collected = if bank.should_collect_rent() {
|
||||||
|
1 /* this is a function of 'some_slot' */
|
||||||
|
} else {
|
||||||
|
0
|
||||||
|
};
|
||||||
|
|
||||||
bank.store_account(
|
bank.store_account(
|
||||||
&zero_lamport_pubkey,
|
&zero_lamport_pubkey,
|
||||||
|
@ -1648,9 +1656,9 @@ fn test_rent_eager_collect_rent_in_partition() {
|
||||||
bank.get_account(&rent_due_pubkey).unwrap().lamports(),
|
bank.get_account(&rent_due_pubkey).unwrap().lamports(),
|
||||||
little_lamports - rent_collected
|
little_lamports - rent_collected
|
||||||
);
|
);
|
||||||
assert_eq!(
|
assert!(
|
||||||
bank.get_account(&rent_due_pubkey).unwrap().rent_epoch(),
|
bank.get_account(&rent_due_pubkey).unwrap().rent_epoch() == current_epoch + 1
|
||||||
current_epoch + 1
|
|| !bank.should_collect_rent()
|
||||||
);
|
);
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
bank.get_account(&rent_exempt_pubkey).unwrap().lamports(),
|
bank.get_account(&rent_exempt_pubkey).unwrap().lamports(),
|
||||||
|
@ -10877,6 +10885,7 @@ fn test_rent_state_list_len() {
|
||||||
RewardInterval::OutsideInterval,
|
RewardInterval::OutsideInterval,
|
||||||
&HashMap::new(),
|
&HashMap::new(),
|
||||||
&LoadedProgramsForTxBatch::default(),
|
&LoadedProgramsForTxBatch::default(),
|
||||||
|
true,
|
||||||
);
|
);
|
||||||
|
|
||||||
let compute_budget = bank.runtime_config.compute_budget.unwrap_or_else(|| {
|
let compute_budget = bank.runtime_config.compute_budget.unwrap_or_else(|| {
|
||||||
|
@ -11462,14 +11471,22 @@ fn test_get_rent_paying_pubkeys() {
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Ensure that accounts data size is updated correctly by rent collection
|
/// Ensure that accounts data size is updated correctly by rent collection
|
||||||
#[test]
|
#[test_case(true; "enable rent fees collection")]
|
||||||
fn test_accounts_data_size_and_rent_collection() {
|
#[test_case(false; "disable rent fees collection")]
|
||||||
|
fn test_accounts_data_size_and_rent_collection(should_collect_rent: bool) {
|
||||||
for set_exempt_rent_epoch_max in [false, true] {
|
for set_exempt_rent_epoch_max in [false, true] {
|
||||||
let GenesisConfigInfo {
|
let GenesisConfigInfo {
|
||||||
mut genesis_config, ..
|
mut genesis_config, ..
|
||||||
} = genesis_utils::create_genesis_config(100 * LAMPORTS_PER_SOL);
|
} = genesis_utils::create_genesis_config(100 * LAMPORTS_PER_SOL);
|
||||||
genesis_config.rent = Rent::default();
|
genesis_config.rent = Rent::default();
|
||||||
activate_all_features(&mut genesis_config);
|
for feature_id in FeatureSet::default().inactive {
|
||||||
|
if should_collect_rent
|
||||||
|
|| feature_id != solana_sdk::feature_set::disable_rent_fees_collection::id()
|
||||||
|
{
|
||||||
|
activate_feature(&mut genesis_config, feature_id);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
let bank = Arc::new(Bank::new_for_tests(&genesis_config));
|
let bank = Arc::new(Bank::new_for_tests(&genesis_config));
|
||||||
let slot = bank.slot() + bank.slot_count_per_normal_epoch();
|
let slot = bank.slot() + bank.slot_count_per_normal_epoch();
|
||||||
let bank = Arc::new(Bank::new_from_parent(bank, &Pubkey::default(), slot));
|
let bank = Arc::new(Bank::new_from_parent(bank, &Pubkey::default(), slot));
|
||||||
|
@ -11497,17 +11514,18 @@ fn test_accounts_data_size_and_rent_collection() {
|
||||||
}
|
}
|
||||||
|
|
||||||
// Collect rent for real
|
// Collect rent for real
|
||||||
|
let should_collect_rent = bank.should_collect_rent();
|
||||||
let accounts_data_size_delta_before_collecting_rent = bank.load_accounts_data_size_delta();
|
let accounts_data_size_delta_before_collecting_rent = bank.load_accounts_data_size_delta();
|
||||||
bank.collect_rent_eagerly();
|
bank.collect_rent_eagerly();
|
||||||
let accounts_data_size_delta_after_collecting_rent = bank.load_accounts_data_size_delta();
|
let accounts_data_size_delta_after_collecting_rent = bank.load_accounts_data_size_delta();
|
||||||
|
|
||||||
let accounts_data_size_delta_delta = accounts_data_size_delta_after_collecting_rent
|
let accounts_data_size_delta_delta = accounts_data_size_delta_after_collecting_rent
|
||||||
- accounts_data_size_delta_before_collecting_rent;
|
- accounts_data_size_delta_before_collecting_rent;
|
||||||
assert!(accounts_data_size_delta_delta < 0);
|
assert!(!should_collect_rent || accounts_data_size_delta_delta < 0);
|
||||||
let reclaimed_data_size = accounts_data_size_delta_delta.saturating_neg() as usize;
|
let reclaimed_data_size = accounts_data_size_delta_delta.saturating_neg() as usize;
|
||||||
|
|
||||||
// Ensure the account is reclaimed by rent collection
|
// Ensure the account is reclaimed by rent collection
|
||||||
assert_eq!(reclaimed_data_size, data_size,);
|
assert!(!should_collect_rent || reclaimed_data_size == data_size);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -724,6 +724,10 @@ pub mod validate_fee_collector_account {
|
||||||
solana_sdk::declare_id!("prpFrMtgNmzaNzkPJg9o753fVvbHKqNrNTm76foJ2wm");
|
solana_sdk::declare_id!("prpFrMtgNmzaNzkPJg9o753fVvbHKqNrNTm76foJ2wm");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
pub mod disable_rent_fees_collection {
|
||||||
|
solana_sdk::declare_id!("CJzY83ggJHqPGDq8VisV3U91jDJLuEaALZooBrXtnnLU");
|
||||||
|
}
|
||||||
|
|
||||||
lazy_static! {
|
lazy_static! {
|
||||||
/// Map of feature identifiers to user-visible description
|
/// Map of feature identifiers to user-visible description
|
||||||
pub static ref FEATURE_NAMES: HashMap<Pubkey, &'static str> = [
|
pub static ref FEATURE_NAMES: HashMap<Pubkey, &'static str> = [
|
||||||
|
@ -900,6 +904,7 @@ lazy_static! {
|
||||||
(update_hashes_per_tick5::id(), "Update desired hashes per tick to 9.2M"),
|
(update_hashes_per_tick5::id(), "Update desired hashes per tick to 9.2M"),
|
||||||
(update_hashes_per_tick6::id(), "Update desired hashes per tick to 10M"),
|
(update_hashes_per_tick6::id(), "Update desired hashes per tick to 10M"),
|
||||||
(validate_fee_collector_account::id(), "validate fee collector account #33888"),
|
(validate_fee_collector_account::id(), "validate fee collector account #33888"),
|
||||||
|
(disable_rent_fees_collection::id(), "Disable rent fees collection #33945"),
|
||||||
/*************** ADD NEW FEATURES HERE ***************/
|
/*************** ADD NEW FEATURES HERE ***************/
|
||||||
]
|
]
|
||||||
.iter()
|
.iter()
|
||||||
|
|
Loading…
Reference in New Issue