Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove activated feature for removing inactive delegations from stakes cache #21732

Merged
merged 2 commits into from
Dec 14, 2021
Merged

Remove activated feature for removing inactive delegations from stakes cache #21732

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
07f07f9
Remove activated feature for removing inactive delegations from stake…
jstarry Dec 6, 2021
1daca5f
Fix builtin purging
jstarry Dec 9, 2021
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
22 changes: 5 additions & 17 deletions runtime/src/bank.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2792,9 +2792,10 @@ impl Bank {

fn burn_and_purge_account(&self, program_id: &Pubkey, mut account: AccountSharedData) {
self.capitalization.fetch_sub(account.lamports(), Relaxed);
// Resetting account balance to 0 is needed to really purge from AccountsDb and
// flush the Stakes cache
// Both resetting account balance to 0 and zeroing the account data
// is needed to really purge from AccountsDb and flush the Stakes cache
Comment on lines +2795 to +2796
Copy link
Member

Choose a reason for hiding this comment

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

❤️ comment grooming

account.set_lamports(0);
account.data_as_mut_slice().fill(0);
Copy link
Member Author

Choose a reason for hiding this comment

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

@ryoqun can you take a look at this? When the stakes_remove_delegation_if_inactive feature was activated, we no longer remove delegations simply if the account balance is zero for stake accounts. The data must still be large enough to hold stake state AND it must not deserialize into a state that has a delegation.

Copy link
Member Author

Choose a reason for hiding this comment

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

This change will only affect consensus if we create a new builtin for an active stake account.

Copy link
Member

@ryoqun ryoqun Dec 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

haha, you're editing scary part. thanks for pinging :)

to be sure, account with lamport=0 should always be same account hash regardless of its data contents. that's why this doesn't need feature-gated, right?

i mean, there's a risk of differing bank hash in add_builtin_account (

solana/runtime/src/bank.rs

Line 2814 in 1149c18

self.burn_and_purge_account(program_id, account);
) (likewise, in add_precompiled_account) under mixed-version cluster condition unless the above premise holds true.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, exactly, the account hash shouldn't change. The risk in builtin/precompiled accounts is that the stakes cache state is corrupted so maybe worth adding a new feature gate in your opinion? There is a new change (#21654) that prunes corrupt state from the stakes cache but I'd rather not test it.

Copy link
Member

Choose a reason for hiding this comment

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

spoiler:

image

Copy link
Member

Choose a reason for hiding this comment

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

The risk in builtin/precompiled accounts is that the stakes cache state is corrupted so maybe worth adding a new feature gate in your opinion?

i don't think it's worth adding a new feature gate. i always forget the following impracticality of these attacks... xD:

solana/runtime/src/bank.rs

Lines 2807 to 2814 in 1149c18

// it's very unlikely to be squatted at program_id as non-system account because of burden to
// find victim's pubkey/hash. So, when account.owner is indeed native_loader's, it's
// safe to assume it's a genuine program.
if native_loader::check_id(account.owner()) {
Some(account)
} else {
// malicious account is pre-occupying at program_id
self.burn_and_purge_account(program_id, account);

Copy link
Contributor

Choose a reason for hiding this comment

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

// Both resetting account balance to 0 and zeroing the account data
// is needed to really purge from AccountsDb and flush the Stakes cache
account.set_lamports(0);
account.data_as_mut_slice().fill(0);

What does above comment actually mean?
I see zero lamports accounts are not stored in accounts-db even if they have non zero data and data_len.
Also, the code indicates that is the case:
https://github.com/solana-labs/solana/blob/64abd008c/runtime/src/accounts_db.rs#L4971-L4978

This causes stakes cache to be inconsistent with accounts-db. Whereas previously if lamports = 0, the delegation was removed from stakes cache as well.

Copy link
Member Author

@jstarry jstarry Apr 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This burn_and_purge_account method is only called when we forcefully purge an account so this checks ensures that if we purge an active stake account, it actually gets removed from the stakes cache. Simply setting the lamports to 0 is not enough because the stake account state could still be deserialized into a delegation and then update_stake_delegation would be called with Some(0, Delegation) and self.stake_delegations.remove(stake_pubkey) wouldn't be called.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the code comment could be better expressed, because as it is right now, it may falsely imply that setting account balance to 0 may not always remove the account from AccountsDb, which to my understanding it is not true.

But that issue aside and ignoring accounts-db for the moment, regarding the stakes-cache:

  1. old code used to remove delegations if lamports == 0: https://github.com/solana-labs/solana/blob/476124de5/runtime/src/stakes.rs#L181-L185
  2. not removing delegations from stakes-cache when lamports == 0 will cause stakes cache to be inconsistent with accounts-db. ie. there will be zombie delegations in stakes cache with no associated stake account in accounts-db.

Is there any scenario that we don't want to remove the delegation from the stakes-cache when account's lamports == 0?

self.store_account(program_id, &account);
}

Expand Down Expand Up @@ -4763,11 +4764,7 @@ impl Bank {
.accounts
.store_slow_cached(self.slot(), pubkey, account);

self.stakes_cache.check_and_store(
pubkey,
account,
self.stakes_remove_delegation_if_inactive_enabled(),
);
self.stakes_cache.check_and_store(pubkey, account);
}

pub fn force_flush_accounts_cache(&self) {
Expand Down Expand Up @@ -5509,11 +5506,7 @@ impl Bank {
for (_i, (pubkey, account)) in
(0..message.account_keys_len()).zip(loaded_transaction.accounts.iter())
{
self.stakes_cache.check_and_store(
pubkey,
account,
self.stakes_remove_delegation_if_inactive_enabled(),
);
self.stakes_cache.check_and_store(pubkey, account);
}
}
}
Expand Down Expand Up @@ -5777,11 +5770,6 @@ impl Bank {
.is_active(&feature_set::leave_nonce_on_success::id())
}

pub fn stakes_remove_delegation_if_inactive_enabled(&self) -> bool {
self.feature_set
.is_active(&feature_set::stakes_remove_delegation_if_inactive::id())
}

pub fn send_to_tpu_vote_port_enabled(&self) -> bool {
self.feature_set
.is_active(&feature_set::send_to_tpu_vote_port::id())
Expand Down
92 changes: 39 additions & 53 deletions runtime/src/stakes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,7 @@ impl StakesCache {
&& account.data().len() >= std::mem::size_of::<StakeState>()
}

pub fn check_and_store(
&self,
pubkey: &Pubkey,
account: &AccountSharedData,
remove_delegation_on_inactive: bool,
) {
pub fn check_and_store(&self, pubkey: &Pubkey, account: &AccountSharedData) {
if solana_vote_program::check_id(account.owner()) {
let new_vote_account = if account.lamports() != 0
&& VoteState::is_correct_size_and_initialized(account.data())
Expand Down Expand Up @@ -93,17 +88,10 @@ impl StakesCache {
(stake, delegation)
});

let remove_delegation = if remove_delegation_on_inactive {
new_delegation.is_none()
} else {
account.lamports() == 0
};

self.0.write().unwrap().update_stake_delegation(
pubkey,
new_delegation,
remove_delegation,
);
self.0
.write()
.unwrap()
.update_stake_delegation(pubkey, new_delegation);
}
}

Expand Down Expand Up @@ -314,7 +302,6 @@ impl Stakes {
&mut self,
stake_pubkey: &Pubkey,
new_delegation: Option<(u64, Delegation)>,
remove_delegation: bool,
) {
// old_stake is stake lamports and voter_pubkey from the pre-store() version
let old_stake = self.stake_delegations.get(stake_pubkey).map(|delegation| {
Expand All @@ -336,13 +323,13 @@ impl Stakes {
}
}

if remove_delegation {
// when account is removed (lamports == 0), remove it from Stakes as well
// so that given `pubkey` can be used for any owner in the future, while not
if let Some((_stake, delegation)) = new_delegation {
self.stake_delegations.insert(*stake_pubkey, delegation);
} else {
// when stake is no longer delegated, remove it from Stakes so that
// given `pubkey` can be used for any owner in the future, while not
// affecting Stakes.
self.stake_delegations.remove(stake_pubkey);
} else if let Some((_stake, delegation)) = new_delegation {
self.stake_delegations.insert(*stake_pubkey, delegation);
}
}

Expand Down Expand Up @@ -450,8 +437,8 @@ pub mod tests {
let ((vote_pubkey, vote_account), (stake_pubkey, mut stake_account)) =
create_staked_node_accounts(10);

stakes_cache.check_and_store(&vote_pubkey, &vote_account, true);
stakes_cache.check_and_store(&stake_pubkey, &stake_account, true);
stakes_cache.check_and_store(&vote_pubkey, &vote_account);
stakes_cache.check_and_store(&stake_pubkey, &stake_account);
let stake = stake_state::stake_from(&stake_account).unwrap();
{
let stakes = stakes_cache.stakes();
Expand All @@ -464,7 +451,7 @@ pub mod tests {
}

stake_account.set_lamports(42);
stakes_cache.check_and_store(&stake_pubkey, &stake_account, true);
stakes_cache.check_and_store(&stake_pubkey, &stake_account);
{
let stakes = stakes_cache.stakes();
let vote_accounts = stakes.vote_accounts();
Expand All @@ -477,7 +464,7 @@ pub mod tests {

// activate more
let (_stake_pubkey, mut stake_account) = create_stake_account(42, &vote_pubkey);
stakes_cache.check_and_store(&stake_pubkey, &stake_account, true);
stakes_cache.check_and_store(&stake_pubkey, &stake_account);
let stake = stake_state::stake_from(&stake_account).unwrap();
{
let stakes = stakes_cache.stakes();
Expand All @@ -490,7 +477,7 @@ pub mod tests {
}

stake_account.set_lamports(0);
stakes_cache.check_and_store(&stake_pubkey, &stake_account, true);
stakes_cache.check_and_store(&stake_pubkey, &stake_account);
{
let stakes = stakes_cache.stakes();
let vote_accounts = stakes.vote_accounts();
Expand All @@ -509,14 +496,14 @@ pub mod tests {
let ((vote_pubkey, vote_account), (stake_pubkey, stake_account)) =
create_staked_node_accounts(10);

stakes_cache.check_and_store(&vote_pubkey, &vote_account, true);
stakes_cache.check_and_store(&stake_pubkey, &stake_account, true);
stakes_cache.check_and_store(&vote_pubkey, &vote_account);
stakes_cache.check_and_store(&stake_pubkey, &stake_account);

let ((vote11_pubkey, vote11_account), (stake11_pubkey, stake11_account)) =
create_staked_node_accounts(20);

stakes_cache.check_and_store(&vote11_pubkey, &vote11_account, true);
stakes_cache.check_and_store(&stake11_pubkey, &stake11_account, true);
stakes_cache.check_and_store(&vote11_pubkey, &vote11_account);
stakes_cache.check_and_store(&stake11_pubkey, &stake11_account);

let vote11_node_pubkey = VoteState::from(&vote11_account).unwrap().node_pubkey;

Expand All @@ -534,8 +521,8 @@ pub mod tests {
let ((vote_pubkey, mut vote_account), (stake_pubkey, stake_account)) =
create_staked_node_accounts(10);

stakes_cache.check_and_store(&vote_pubkey, &vote_account, true);
stakes_cache.check_and_store(&stake_pubkey, &stake_account, true);
stakes_cache.check_and_store(&vote_pubkey, &vote_account);
stakes_cache.check_and_store(&stake_pubkey, &stake_account);

{
let stakes = stakes_cache.stakes();
Expand All @@ -545,7 +532,7 @@ pub mod tests {
}

vote_account.set_lamports(0);
stakes_cache.check_and_store(&vote_pubkey, &vote_account, true);
stakes_cache.check_and_store(&vote_pubkey, &vote_account);

{
let stakes = stakes_cache.stakes();
Expand All @@ -554,7 +541,7 @@ pub mod tests {
}

vote_account.set_lamports(1);
stakes_cache.check_and_store(&vote_pubkey, &vote_account, true);
stakes_cache.check_and_store(&vote_pubkey, &vote_account);

{
let stakes = stakes_cache.stakes();
Expand All @@ -568,7 +555,7 @@ pub mod tests {
let mut pushed = vote_account.data().to_vec();
pushed.push(0);
vote_account.set_data(pushed);
stakes_cache.check_and_store(&vote_pubkey, &vote_account, true);
stakes_cache.check_and_store(&vote_pubkey, &vote_account);

{
let stakes = stakes_cache.stakes();
Expand All @@ -580,7 +567,7 @@ pub mod tests {
let default_vote_state = VoteState::default();
let versioned = VoteStateVersions::new_current(default_vote_state);
VoteState::to(&versioned, &mut vote_account).unwrap();
stakes_cache.check_and_store(&vote_pubkey, &vote_account, true);
stakes_cache.check_and_store(&vote_pubkey, &vote_account);

{
let stakes = stakes_cache.stakes();
Expand All @@ -589,7 +576,7 @@ pub mod tests {
}

vote_account.set_data(cache_data);
stakes_cache.check_and_store(&vote_pubkey, &vote_account, true);
stakes_cache.check_and_store(&vote_pubkey, &vote_account);

{
let stakes = stakes_cache.stakes();
Expand All @@ -612,11 +599,11 @@ pub mod tests {
let ((vote_pubkey2, vote_account2), (_stake_pubkey2, stake_account2)) =
create_staked_node_accounts(10);

stakes_cache.check_and_store(&vote_pubkey, &vote_account, true);
stakes_cache.check_and_store(&vote_pubkey2, &vote_account2, true);
stakes_cache.check_and_store(&vote_pubkey, &vote_account);
stakes_cache.check_and_store(&vote_pubkey2, &vote_account2);

// delegates to vote_pubkey
stakes_cache.check_and_store(&stake_pubkey, &stake_account, true);
stakes_cache.check_and_store(&stake_pubkey, &stake_account);

let stake = stake_state::stake_from(&stake_account).unwrap();

Expand All @@ -633,7 +620,7 @@ pub mod tests {
}

// delegates to vote_pubkey2
stakes_cache.check_and_store(&stake_pubkey, &stake_account2, true);
stakes_cache.check_and_store(&stake_pubkey, &stake_account2);

{
let stakes = stakes_cache.stakes();
Expand All @@ -659,11 +646,11 @@ pub mod tests {

let (stake_pubkey2, stake_account2) = create_stake_account(10, &vote_pubkey);

stakes_cache.check_and_store(&vote_pubkey, &vote_account, true);
stakes_cache.check_and_store(&vote_pubkey, &vote_account);

// delegates to vote_pubkey
stakes_cache.check_and_store(&stake_pubkey, &stake_account, true);
stakes_cache.check_and_store(&stake_pubkey2, &stake_account2, true);
stakes_cache.check_and_store(&stake_pubkey, &stake_account);
stakes_cache.check_and_store(&stake_pubkey2, &stake_account2);

{
let stakes = stakes_cache.stakes();
Expand All @@ -680,8 +667,8 @@ pub mod tests {
let ((vote_pubkey, vote_account), (stake_pubkey, stake_account)) =
create_staked_node_accounts(10);

stakes_cache.check_and_store(&vote_pubkey, &vote_account, true);
stakes_cache.check_and_store(&stake_pubkey, &stake_account, true);
stakes_cache.check_and_store(&vote_pubkey, &vote_account);
stakes_cache.check_and_store(&stake_pubkey, &stake_account);
let stake = stake_state::stake_from(&stake_account).unwrap();

{
Expand Down Expand Up @@ -714,8 +701,8 @@ pub mod tests {
let ((vote_pubkey, vote_account), (stake_pubkey, stake_account)) =
create_staked_node_accounts(10);

stakes_cache.check_and_store(&vote_pubkey, &vote_account, true);
stakes_cache.check_and_store(&stake_pubkey, &stake_account, true);
stakes_cache.check_and_store(&vote_pubkey, &vote_account);
stakes_cache.check_and_store(&stake_pubkey, &stake_account);

{
let stakes = stakes_cache.stakes();
Expand All @@ -728,7 +715,6 @@ pub mod tests {
stakes_cache.check_and_store(
&stake_pubkey,
&AccountSharedData::new(1, 0, &stake::program::id()),
true,
);
{
let stakes = stakes_cache.stakes();
Expand Down Expand Up @@ -759,8 +745,8 @@ pub mod tests {
let genesis_epoch = 0;
let ((vote_pubkey, vote_account), (stake_pubkey, stake_account)) =
create_warming_staked_node_accounts(10, genesis_epoch);
stakes_cache.check_and_store(&vote_pubkey, &vote_account, true);
stakes_cache.check_and_store(&stake_pubkey, &stake_account, true);
stakes_cache.check_and_store(&vote_pubkey, &vote_account);
stakes_cache.check_and_store(&stake_pubkey, &stake_account);

{
let stakes = stakes_cache.stakes();
Expand Down