Remove activated feature for removing inactive delegations from stakes cache

[jstarry]

Problem

Feature is active on all clusters and can be cleaned up

Summary of Changes

Fixes #

[mergify]

automerge label removed due to a CI failure

[mergify]

automerge label removed due to a CI failure

[jstarry]

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

[jstarry]

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

[ryoqun]

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.

[jstarry]

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.

[ryoqun]

spoiler:

[ryoqun]

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

[behzadnouri]

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

[jstarry]

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.

[behzadnouri]

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?

[codecov]

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]

❤️ comment grooming

[ryoqun]

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

[ryoqun]

lgtm with nit (and after merge conflict cleaning)

Pull request has been modified.

[mergify]

automerge label removed due to a CI failure