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

jstarry · 2021-12-09T16:17:08Z

Problem

Feature is active on all clusters and can be cleaned up

Summary of Changes

Fixes #

mergify · 2021-12-09T16:25:59Z

automerge label removed due to a CI failure

mergify · 2021-12-09T17:11:31Z

automerge label removed due to a CI failure

jstarry · 2021-12-09T18:23:57Z

runtime/src/bank.rs

        account.set_lamports(0);
+        account.data_as_mut_slice().fill(0);


@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.

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

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.

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.

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);

// 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.

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.

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:

old code used to remove delegations if lamports == 0: https://github.com/solana-labs/solana/blob/476124de5/runtime/src/stakes.rs#L181-L185

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?

codecov · 2021-12-10T17:21:49Z

Codecov Report

Merging #21732 (1daca5f) into master (a400b5e) will increase coverage by 0.0%.
The diff coverage is 80.1%.

@@           Coverage Diff            @@
##           master   #21732    +/-   ##
========================================
  Coverage    81.3%    81.3%            
========================================
  Files         516      516            
  Lines      144033   144155   +122     
========================================
+ Hits       117139   117241   +102     
- Misses      26894    26914    +20

ryoqun · 2021-12-13T13:42:26Z

runtime/src/bank.rs

+        // Both resetting account balance to 0 and zeroing the account data
+        // is needed to really purge from AccountsDb and flush the Stakes cache


❤️ comment grooming

ryoqun · 2021-12-13T14:04:00Z

runtime/src/stakes.rs

-
-            if remove_delegation {
-                // when account is removed (lamports == 0), remove it from Stakes as well
+                // when stake is no longer delegated, remove it from Stakes as well


nits: as well could be removed as it was referring to the 0 lamported ness?

ryoqun

lgtm with nit (and after merge conflict cleaning)

…s cache

Pull request has been modified.

mergify · 2021-12-13T14:43:19Z

automerge label removed due to a CI failure

…s cache (#21732) * Remove activated feature for removing inactive delegations from stakes cache * Fix builtin purging (cherry picked from commit c92c09a) # Conflicts: # runtime/src/bank.rs # runtime/src/stakes.rs

…s cache (#21732) * Remove activated feature for removing inactive delegations from stakes cache * Fix builtin purging (cherry picked from commit c92c09a) # Conflicts: # runtime/src/stakes.rs

jstarry added automerge Merge this Pull Request automatically once CI passes v1.8 labels Dec 9, 2021

mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Dec 9, 2021

jstarry force-pushed the remove-inactive-delegation-fs branch from 61b24c5 to beeba33 Compare December 9, 2021 17:05

jstarry added the automerge Merge this Pull Request automatically once CI passes label Dec 9, 2021

mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Dec 9, 2021

jstarry commented Dec 9, 2021

View reviewed changes

ryoqun reviewed Dec 13, 2021

View reviewed changes

ryoqun previously approved these changes Dec 13, 2021

View reviewed changes

jstarry added 2 commits December 13, 2021 09:34

Remove activated feature for removing inactive delegations from stake…

07f07f9

…s cache

Fix builtin purging

1daca5f

jstarry force-pushed the remove-inactive-delegation-fs branch from e2bd002 to 1daca5f Compare December 13, 2021 14:34

jstarry added the automerge Merge this Pull Request automatically once CI passes label Dec 13, 2021

mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Dec 13, 2021

jstarry merged commit c92c09a into solana-labs:master Dec 14, 2021

jstarry deleted the remove-inactive-delegation-fs branch December 14, 2021 14:23

mergify bot mentioned this pull request Dec 14, 2021

Remove activated feature for removing inactive delegations from stakes cache (backport #21732) #21886

Closed

mergify bot mentioned this pull request Dec 14, 2021

Remove activated feature for removing inactive delegations from stakes cache (backport #21732) #21887

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jstarry commented Dec 9, 2021

mergify bot commented Dec 9, 2021

mergify bot commented Dec 9, 2021

jstarry Dec 9, 2021

jstarry Dec 9, 2021

ryoqun Dec 13, 2021 •

edited

Loading

jstarry Dec 13, 2021

ryoqun Dec 13, 2021

ryoqun Dec 13, 2021

behzadnouri Apr 8, 2022

jstarry Apr 9, 2022 •

edited

Loading

behzadnouri Apr 10, 2022

codecov bot commented Dec 10, 2021 •

edited

Loading

ryoqun Dec 13, 2021

ryoqun Dec 13, 2021

ryoqun left a comment •

edited

Loading

mergify bot commented Dec 13, 2021

		account.set_lamports(0);
		account.data_as_mut_slice().fill(0);

	// 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);

		// Both resetting account balance to 0 and zeroing the account data
		// is needed to really purge from AccountsDb and flush the Stakes cache

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

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

Conversation

jstarry commented Dec 9, 2021

Problem

Summary of Changes

mergify bot commented Dec 9, 2021

mergify bot commented Dec 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryoqun Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstarry Apr 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 10, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryoqun left a comment • edited Loading

Choose a reason for hiding this comment

mergify bot commented Dec 13, 2021

ryoqun Dec 13, 2021 •

edited

Loading

jstarry Apr 9, 2022 •

edited

Loading

codecov bot commented Dec 10, 2021 •

edited

Loading

ryoqun left a comment •

edited

Loading