From 8638918be4ab4b6e23cc796c647c1c00d95d2d57 Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Mon, 17 Jan 2022 22:38:05 +0900 Subject: [PATCH 1/5] Credits auto rewind on vote recreation --- ledger-tool/src/main.rs | 69 ++++++++++++++++++++++++++ programs/stake/src/stake_state.rs | 82 ++++++++++++++++++++++--------- runtime/src/bank.rs | 15 ++++-- sdk/src/feature_set.rs | 6 +-- 4 files changed, 142 insertions(+), 30 deletions(-) diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs index 11b65c7f2fd..046503eabd8 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -52,6 +52,8 @@ use { account::{AccountSharedData, ReadableAccount, WritableAccount}, account_utils::StateMut, clock::{Epoch, Slot}, + feature::{self, Feature}, + feature_set, genesis_config::{ClusterType, GenesisConfig}, hash::Hash, inflation::Inflation, @@ -1537,6 +1539,13 @@ fn main() { .possible_values(&["pico", "full", "none"]) .help("Overwrite inflation when warping"), ) + .arg( + Arg::with_name("enable_credits_auto_rewind") + .required(false) + .long("enable-credits-auto-rewind") + .takes_value(false) + .help("Enable credits auto rewind"), + ) .arg( Arg::with_name("recalculate_capitalization") .required(false) @@ -2734,6 +2743,66 @@ fn main() { .lazy_rent_collection .store(true, std::sync::atomic::Ordering::Relaxed); + let feature_account_balance = std::cmp::max( + genesis_config.rent.minimum_balance(Feature::size_of()), + 1, + ); + if arg_matches.is_present("enable_credits_auto_rewind") { + base_bank.unfreeze(); + let mut force_enabled_count = 0; + if base_bank + .get_account(&feature_set::credits_auto_rewind::id()) + .is_none() + { + base_bank.store_account( + &feature_set::credits_auto_rewind::id(), + &feature::create_account( + &Feature { activated_at: None }, + feature_account_balance, + ), + ); + force_enabled_count += 1; + } + if force_enabled_count == 0 { + warn!( + "Already credits_auto_rewind is activated (or scheduled)" + ); + } + let mut store_failed_count = 0; + if force_enabled_count >= 1 { + if base_bank + .get_account(&feature_set::deprecate_rewards_sysvar::id()) + .is_some() + { + // steal some lamports from the pretty old feature not to affect + // capitalizaion, which doesn't affect inflation behavior! + base_bank.store_account( + &feature_set::deprecate_rewards_sysvar::id(), + &AccountSharedData::default(), + ); + force_enabled_count -= 1; + } else { + store_failed_count += 1; + } + } + assert_eq!(force_enabled_count, store_failed_count); + if store_failed_count >= 1 { + // we have no choice; maybe locally created blank cluster with + // not-Development cluster type. + let old_cap = base_bank.set_capitalization(); + let new_cap = base_bank.capitalization(); + warn!( + "Skewing capitalization a bit to enable credits_auto_rewind as \ + requested: increasing {} from {} to {}", + feature_account_balance, old_cap, new_cap, + ); + assert_eq!( + old_cap + feature_account_balance * store_failed_count, + new_cap + ); + } + } + #[derive(Default, Debug)] struct PointDetail { epoch: Epoch, diff --git a/programs/stake/src/stake_state.rs b/programs/stake/src/stake_state.rs index 7cc4cc16c07..6df8a96865f 100644 --- a/programs/stake/src/stake_state.rs +++ b/programs/stake/src/stake_state.rs @@ -43,6 +43,7 @@ pub enum SkippedReason { ZeroReward, ZeroCreditsAndReturnZero, ZeroCreditsAndReturnCurrent, + ZeroCreditsAndReturnRewinded, } impl From for InflationPointCalculationEvent { @@ -164,7 +165,7 @@ fn redeem_stake_rewards( vote_state: &VoteState, stake_history: Option<&StakeHistory>, inflation_point_calc_tracer: Option, - fix_activating_credits_observed: bool, + credits_auto_rewind: bool, ) -> Option<(u64, u64)> { if let Some(inflation_point_calc_tracer) = inflation_point_calc_tracer.as_ref() { inflation_point_calc_tracer(&InflationPointCalculationEvent::CreditsObserved( @@ -179,7 +180,7 @@ fn redeem_stake_rewards( vote_state, stake_history, inflation_point_calc_tracer.as_ref(), - fix_activating_credits_observed, + credits_auto_rewind, ) .map(|(stakers_reward, voters_reward, credits_observed)| { if let Some(inflation_point_calc_tracer) = inflation_point_calc_tracer { @@ -205,6 +206,8 @@ fn calculate_stake_points( vote_state, stake_history, inflation_point_calc_tracer, + true, // this is safe because this flag shouldn't affect the first + // element of the returned tuple in any way ) .0 } @@ -217,15 +220,37 @@ fn calculate_stake_points_and_credits( new_vote_state: &VoteState, stake_history: Option<&StakeHistory>, inflation_point_calc_tracer: Option, -) -> (u128, u64) { + credits_auto_rewind: bool, +) -> (u128, u64, bool) { let credits_in_stake = stake.credits_observed; let credits_in_vote = new_vote_state.credits(); // if there is no newer credits since observed, return no point if credits_in_vote <= credits_in_stake { - if let Some(inflation_point_calc_tracer) = inflation_point_calc_tracer.as_ref() { - inflation_point_calc_tracer(&SkippedReason::ZeroCreditsAndReturnCurrent.into()); - } - return (0, credits_in_stake); + if credits_auto_rewind && credits_in_vote < credits_in_stake { + if let Some(inflation_point_calc_tracer) = inflation_point_calc_tracer.as_ref() { + inflation_point_calc_tracer(&SkippedReason::ZeroCreditsAndReturnRewinded.into()); + } + // Don't adjust stake.activation_epoch for simplicity: + // - generally back-dating stake.activation_epoch forcibly skews + // the stake history sysvar. And properly handle all the cases + // regarding deactivation epoch/warm-up/cool-down without + // introducing incentive skew is hard. + // - Conceptually, it should be acceptable for the staked SOLs at + // the recreated vote to receive rewards again after rewind even + // if it's kind of sudden. That's because it must have had + // warmed-up to now. + // - Also such a stake account remains to be a part of overall + // effective stake calculation even while the vote account is + // missing for (indefinite) time. It should be treated equally + // with no difference regarding to staking with delinquent + // validator. + return (0, credits_in_vote, true); + } else { + if let Some(inflation_point_calc_tracer) = inflation_point_calc_tracer.as_ref() { + inflation_point_calc_tracer(&SkippedReason::ZeroCreditsAndReturnCurrent.into()); + } + return (0, credits_in_stake, false); + }; } let mut points = 0; @@ -268,7 +293,7 @@ fn calculate_stake_points_and_credits( } } - (points, new_credits_observed) + (points, new_credits_observed, false) } /// for a given stake and vote_state, calculate what distributions and what updates should be made @@ -284,17 +309,17 @@ fn calculate_stake_rewards( vote_state: &VoteState, stake_history: Option<&StakeHistory>, inflation_point_calc_tracer: Option, - _fix_activating_credits_observed: bool, // this unused flag will soon be justified by an upcoming feature pr + credits_auto_rewind: bool, ) -> Option<(u64, u64, u64)> { // ensure to run to trigger (optional) inflation_point_calc_tracer - // this awkward flag variable will soon be justified by an upcoming feature pr - let mut forced_credits_update_with_skipped_reward = false; - let (points, credits_observed) = calculate_stake_points_and_credits( - stake, - vote_state, - stake_history, - inflation_point_calc_tracer.as_ref(), - ); + let (points, credits_observed, mut forced_credits_update_with_skipped_reward) = + calculate_stake_points_and_credits( + stake, + vote_state, + stake_history, + inflation_point_calc_tracer.as_ref(), + credits_auto_rewind, + ); // Drive credits_observed forward unconditionally when rewards are disabled // or when this is the stake's activation epoch @@ -1302,7 +1327,7 @@ pub fn redeem_rewards( point_value: &PointValue, stake_history: Option<&StakeHistory>, inflation_point_calc_tracer: Option, - fix_activating_credits_observed: bool, + credits_auto_rewind: bool, ) -> Result<(u64, u64), InstructionError> { if let StakeState::Stake(meta, mut stake) = stake_state { if let Some(inflation_point_calc_tracer) = inflation_point_calc_tracer.as_ref() { @@ -1326,7 +1351,7 @@ pub fn redeem_rewards( vote_state, stake_history, inflation_point_calc_tracer, - fix_activating_credits_observed, + credits_auto_rewind, ) { stake_account.checked_add_lamports(stakers_reward)?; stake_account.set_state(&StakeState::Stake(meta, stake))?; @@ -2546,10 +2571,23 @@ mod tests { ) ); - // assert the previous behavior is preserved where fix_stake_deactivate=false assert_eq!( - (0, 4), - calculate_stake_points_and_credits(&stake, &vote_state, None, null_tracer()) + (0, 4, false), + calculate_stake_points_and_credits(&stake, &vote_state, None, null_tracer(), true) + ); + + // credits_observed is auto-rewinded when vote_state credits are assumed to have been + // recreated + stake.credits_observed = 1000; + // this is old behavior; return pre-recreation (large) vote credits from stake account + assert_eq!( + (0, 1000, false), + calculate_stake_points_and_credits(&stake, &vote_state, None, null_tracer(), false) + ); + // this is new behavior; return post-recreation rewinded vote credits from the vote account + assert_eq!( + (0, 4, true), + calculate_stake_points_and_credits(&stake, &vote_state, None, null_tracer(), true) ); // get rewards and credits observed when not the activation epoch diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index e2001aa3fce..31a2ff529d4 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -2634,7 +2634,7 @@ impl Bank { prev_epoch, validator_rewards, reward_calc_tracer, - self.stake_program_advance_activating_credits_observed(), + self.credits_auto_rewind(), thread_pool, metrics, update_rewards_from_cached_accounts, @@ -2968,7 +2968,7 @@ impl Bank { rewarded_epoch: Epoch, rewards: u64, reward_calc_tracer: Option, - fix_activating_credits_observed: bool, + credits_auto_rewind: bool, thread_pool: &ThreadPool, metrics: &mut RewardsMetrics, update_rewards_from_cached_accounts: bool, @@ -3084,7 +3084,7 @@ impl Bank { &point_value, Some(&stake_history), reward_calc_tracer.as_ref(), - fix_activating_credits_observed, + credits_auto_rewind, ); if let Ok((stakers_reward, voters_reward)) = redeemed { // track voter rewards @@ -3324,6 +3324,11 @@ impl Bank { } } + // dangerous + pub fn unfreeze(&self) { + self.freeze_started.store(false, Relaxed); + } + pub fn epoch_schedule(&self) -> &EpochSchedule { &self.epoch_schedule } @@ -6718,9 +6723,9 @@ impl Bank { .is_active(&feature_set::versioned_tx_message_enabled::id()) } - pub fn stake_program_advance_activating_credits_observed(&self) -> bool { + pub fn credits_auto_rewind(&self) -> bool { self.feature_set - .is_active(&feature_set::stake_program_advance_activating_credits_observed::id()) + .is_active(&feature_set::credits_auto_rewind::id()) } pub fn leave_nonce_on_success(&self) -> bool { diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index ba829dc4d58..b2d579b6477 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -161,8 +161,8 @@ pub mod instructions_sysvar_owned_by_sysvar { solana_sdk::declare_id!("H3kBSaKdeiUsyHmeHqjJYNc27jesXZ6zWj3zWkowQbkV"); } -pub mod stake_program_advance_activating_credits_observed { - solana_sdk::declare_id!("SAdVFw3RZvzbo6DvySbSdBnHN4gkzSTH9dSxesyKKPj"); +pub mod credits_auto_rewind { + solana_sdk::declare_id!("8kN95iBpwFBFe76VgLPG5QeHJnBHDnmYgMZBbnA6ZCcu"); } pub mod demote_program_write_locks { @@ -398,7 +398,7 @@ lazy_static! { (versioned_tx_message_enabled::id(), "enable versioned transaction message processing"), (libsecp256k1_fail_on_bad_count::id(), "fail libsec256k1_verify if count appears wrong"), (instructions_sysvar_owned_by_sysvar::id(), "fix owner for instructions sysvar"), - (stake_program_advance_activating_credits_observed::id(), "Enable advancing credits observed for activation epoch #19309"), + (credits_auto_rewind::id(), "Auto rewind stake's credits_observed if (accidental) vote recreation is detected #22546"), (demote_program_write_locks::id(), "demote program write locks to readonly, except when upgradeable loader present #19593 #20265"), (ed25519_program_enabled::id(), "enable builtin ed25519 signature verify program"), (return_data_syscall_enabled::id(), "enable sol_{set,get}_return_data syscall"), From 6161aa6aed598a3bb4879759675956a87f1f2ea7 Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Tue, 1 Feb 2022 23:05:06 +0900 Subject: [PATCH 2/5] Update comment --- programs/stake/src/stake_state.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/programs/stake/src/stake_state.rs b/programs/stake/src/stake_state.rs index 6df8a96865f..d570973de73 100644 --- a/programs/stake/src/stake_state.rs +++ b/programs/stake/src/stake_state.rs @@ -231,19 +231,21 @@ fn calculate_stake_points_and_credits( inflation_point_calc_tracer(&SkippedReason::ZeroCreditsAndReturnRewinded.into()); } // Don't adjust stake.activation_epoch for simplicity: - // - generally back-dating stake.activation_epoch forcibly skews - // the stake history sysvar. And properly handle all the cases + // - generally fast-forwarding stake.activation_epoch forcibly (for + // artifical re-activation with re-warm-up) skews the stake + // history sysvar. And properly handling all the cases // regarding deactivation epoch/warm-up/cool-down without // introducing incentive skew is hard. // - Conceptually, it should be acceptable for the staked SOLs at - // the recreated vote to receive rewards again after rewind even - // if it's kind of sudden. That's because it must have had - // warmed-up to now. + // the recreated vote to receive rewards again immediately after + // rewind even if it looks like instant activation. That's + // because it must have passed the required warmed-up at least + // once in the past already // - Also such a stake account remains to be a part of overall // effective stake calculation even while the vote account is - // missing for (indefinite) time. It should be treated equally - // with no difference regarding to staking with delinquent - // validator. + // missing for (indefinite) time or remains to be pre-remove + // credits score. It should be treated equally to staking with + // delinquent validator with no differenciation. return (0, credits_in_vote, true); } else { if let Some(inflation_point_calc_tracer) = inflation_point_calc_tracer.as_ref() { From d58fe61bc4636b7f3670b5aa9d0c471b2c74b8e6 Mon Sep 17 00:00:00 2001 From: Ryo Onodera Date: Thu, 3 Feb 2022 00:40:01 +0900 Subject: [PATCH 3/5] Improve comments and tests --- programs/stake/src/stake_state.rs | 20 +++++++++++++++++--- runtime/src/bank.rs | 2 +- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/programs/stake/src/stake_state.rs b/programs/stake/src/stake_state.rs index d570973de73..53a4a684211 100644 --- a/programs/stake/src/stake_state.rs +++ b/programs/stake/src/stake_state.rs @@ -207,7 +207,7 @@ fn calculate_stake_points( stake_history, inflation_point_calc_tracer, true, // this is safe because this flag shouldn't affect the first - // element of the returned tuple in any way + // element (i.e. `points`) of the returned tuple in any way ) .0 } @@ -246,11 +246,19 @@ fn calculate_stake_points_and_credits( // missing for (indefinite) time or remains to be pre-remove // credits score. It should be treated equally to staking with // delinquent validator with no differenciation. + + // hint with true to indicate some exceptional credits handling is needed return (0, credits_in_vote, true); } else { + // change the above `else` to `else if credits_in_vote == credits_in_stake` + // (and remove the outermost enclosing `if`) when cleaning credits_auto_rewind + // after activation + if let Some(inflation_point_calc_tracer) = inflation_point_calc_tracer.as_ref() { inflation_point_calc_tracer(&SkippedReason::ZeroCreditsAndReturnCurrent.into()); } + // don't hint the caller and return current value if credits_auto_rewind is off or + // credits remain to be unchanged (= delinquent) return (0, credits_in_stake, false); }; } @@ -2581,16 +2589,22 @@ mod tests { // credits_observed is auto-rewinded when vote_state credits are assumed to have been // recreated stake.credits_observed = 1000; - // this is old behavior; return pre-recreation (large) vote credits from stake account + // this is old behavior; return the pre-recreation (large) credits from stake account assert_eq!( (0, 1000, false), calculate_stake_points_and_credits(&stake, &vote_state, None, null_tracer(), false) ); - // this is new behavior; return post-recreation rewinded vote credits from the vote account + // this is new behavior 1; return the post-recreation rewinded credits from the vote account assert_eq!( (0, 4, true), calculate_stake_points_and_credits(&stake, &vote_state, None, null_tracer(), true) ); + // this is new behavior 2; don't hint when credits both from stake and vote are identical + stake.credits_observed = 4; + assert_eq!( + (0, 4, false), + calculate_stake_points_and_credits(&stake, &vote_state, None, null_tracer(), true) + ); // get rewards and credits observed when not the activation epoch vote_state.commission = 0; diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 31a2ff529d4..186b84deaf0 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -3324,7 +3324,7 @@ impl Bank { } } - // dangerous + // dangerous; don't use this; this is only needed for ledger-tool's special command pub fn unfreeze(&self) { self.freeze_started.store(false, Relaxed); } From 3e6748bf25da69a537c90eb8eea222a639e6bece Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Mon, 25 Apr 2022 15:13:53 -0600 Subject: [PATCH 4/5] Recommended fn rename --- ledger-tool/src/main.rs | 2 +- runtime/src/bank.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs index 046503eabd8..51cfa29b325 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -2748,7 +2748,7 @@ fn main() { 1, ); if arg_matches.is_present("enable_credits_auto_rewind") { - base_bank.unfreeze(); + base_bank.unfreeze_for_ledger_tool(); let mut force_enabled_count = 0; if base_bank .get_account(&feature_set::credits_auto_rewind::id()) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 186b84deaf0..ecc6c7afee5 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -3325,7 +3325,7 @@ impl Bank { } // dangerous; don't use this; this is only needed for ledger-tool's special command - pub fn unfreeze(&self) { + pub fn unfreeze_for_ledger_tool(&self) { self.freeze_started.store(false, Relaxed); } From aca12b0174272a98cb8917af08e35ae8955375c8 Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Mon, 25 Apr 2022 15:27:27 -0600 Subject: [PATCH 5/5] Restore old feature, and replace new feature key --- sdk/src/feature_set.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index b2d579b6477..dda51d819d0 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -161,8 +161,12 @@ pub mod instructions_sysvar_owned_by_sysvar { solana_sdk::declare_id!("H3kBSaKdeiUsyHmeHqjJYNc27jesXZ6zWj3zWkowQbkV"); } +pub mod stake_program_advance_activating_credits_observed { + solana_sdk::declare_id!("SAdVFw3RZvzbo6DvySbSdBnHN4gkzSTH9dSxesyKKPj"); +} + pub mod credits_auto_rewind { - solana_sdk::declare_id!("8kN95iBpwFBFe76VgLPG5QeHJnBHDnmYgMZBbnA6ZCcu"); + solana_sdk::declare_id!("BUS12ciZ5gCoFafUHWW8qaFMMtwFQGVxjsDheWLdqBE2"); } pub mod demote_program_write_locks { @@ -398,6 +402,7 @@ lazy_static! { (versioned_tx_message_enabled::id(), "enable versioned transaction message processing"), (libsecp256k1_fail_on_bad_count::id(), "fail libsec256k1_verify if count appears wrong"), (instructions_sysvar_owned_by_sysvar::id(), "fix owner for instructions sysvar"), + (stake_program_advance_activating_credits_observed::id(), "Enable advancing credits observed for activation epoch #19309"), (credits_auto_rewind::id(), "Auto rewind stake's credits_observed if (accidental) vote recreation is detected #22546"), (demote_program_write_locks::id(), "demote program write locks to readonly, except when upgradeable loader present #19593 #20265"), (ed25519_program_enabled::id(), "enable builtin ed25519 signature verify program"),