Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Credits auto rewind on vote recreation #22546

Merged
CriesofCarrots merged 5 commits into from
Apr 26, 2022
Merged

Credits auto rewind on vote recreation #22546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8638918
Credits auto rewind on vote recreation
ryoqun Jan 17, 2022
6161aa6
Update comment
ryoqun Feb 1, 2022
d58fe61
Improve comments and tests
ryoqun Feb 2, 2022
3e6748b
Recommended fn rename
Apr 25, 2022
aca12b0
Restore old feature, and replace new feature key
Apr 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 69 additions & 0 deletions ledger-tool/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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_for_ledger_tool();
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,
Expand Down
98 changes: 76 additions & 22 deletions programs/stake/src/stake_state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ pub enum SkippedReason {
ZeroReward,
ZeroCreditsAndReturnZero,
ZeroCreditsAndReturnCurrent,
ZeroCreditsAndReturnRewinded,
}

impl From<SkippedReason> for InflationPointCalculationEvent {
Expand Down Expand Up @@ -164,7 +165,7 @@ fn redeem_stake_rewards(
vote_state: &VoteState,
stake_history: Option<&StakeHistory>,
inflation_point_calc_tracer: Option<impl Fn(&InflationPointCalculationEvent)>,
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(
Expand All @@ -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 {
Expand All @@ -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 (i.e. `points`) of the returned tuple in any way
)
.0
}
Expand All @@ -217,15 +220,47 @@ fn calculate_stake_points_and_credits(
new_vote_state: &VoteState,
stake_history: Option<&StakeHistory>,
inflation_point_calc_tracer: Option<impl Fn(&InflationPointCalculationEvent)>,
) -> (u128, u64) {
credits_auto_rewind: bool,
) -> (u128, u64, bool) {
Comment thread
CriesofCarrots marked this conversation as resolved.
Outdated
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 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 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 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 {
Comment thread
ryoqun marked this conversation as resolved.
Outdated
// change the above `else` to `else if credits_in_vote == credits_in_stake`
// (and remove the outermost enclosing `if`) when cleaning credits_auto_rewind
Comment thread
CriesofCarrots marked this conversation as resolved.
Outdated
// 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);
};
}

let mut points = 0;
Expand Down Expand Up @@ -268,7 +303,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
Expand All @@ -284,17 +319,17 @@ fn calculate_stake_rewards(
vote_state: &VoteState,
stake_history: Option<&StakeHistory>,
inflation_point_calc_tracer: Option<impl Fn(&InflationPointCalculationEvent)>,
_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)> {
Comment thread
CriesofCarrots marked this conversation as resolved.
Outdated
// 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) =
Copy link
Copy Markdown
Contributor Author

@ryoqun ryoqun Feb 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit contrived flag coding. but i rather wanted some encapsulation for maintainability (place new code at lowest-level to be touched by all codepath even including currently-hypothetical code, which isn't via this fn calculate_stake_rewards) and separation-of-concerns (no visible muddy credits handling with feature activation here and put it into calculate_stake_point_and_credits)

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
Expand Down Expand Up @@ -1302,7 +1337,7 @@ pub fn redeem_rewards(
point_value: &PointValue,
stake_history: Option<&StakeHistory>,
inflation_point_calc_tracer: Option<impl Fn(&InflationPointCalculationEvent)>,
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() {
Expand All @@ -1326,7 +1361,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))?;
Expand Down Expand Up @@ -2546,10 +2581,29 @@ 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 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 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
Expand Down
15 changes: 10 additions & 5 deletions runtime/src/bank.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -2968,7 +2968,7 @@ impl Bank {
rewarded_epoch: Epoch,
rewards: u64,
reward_calc_tracer: Option<impl Fn(&RewardCalculationEvent) + Send + Sync>,
fix_activating_credits_observed: bool,
credits_auto_rewind: bool,
thread_pool: &ThreadPool,
metrics: &mut RewardsMetrics,
update_rewards_from_cached_accounts: bool,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -3324,6 +3324,11 @@ impl Bank {
}
}

// dangerous; don't use this; this is only needed for ledger-tool's special command
pub fn unfreeze_for_ledger_tool(&self) {
Comment thread
CriesofCarrots marked this conversation as resolved.
self.freeze_started.store(false, Relaxed);
}

pub fn epoch_schedule(&self) -> &EpochSchedule {
&self.epoch_schedule
}
Expand Down Expand Up @@ -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 {
Expand Down
5 changes: 5 additions & 0 deletions sdk/src/feature_set.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,10 @@ pub mod stake_program_advance_activating_credits_observed {
solana_sdk::declare_id!("SAdVFw3RZvzbo6DvySbSdBnHN4gkzSTH9dSxesyKKPj");
}

pub mod credits_auto_rewind {
solana_sdk::declare_id!("BUS12ciZ5gCoFafUHWW8qaFMMtwFQGVxjsDheWLdqBE2");
}

pub mod demote_program_write_locks {
solana_sdk::declare_id!("3E3jV7v9VcdJL8iYZUMax9DiDno8j7EWUVbhm9RtShj2");
}
Expand Down Expand Up @@ -399,6 +403,7 @@ lazy_static! {
(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"),
Expand Down