updates rewards at epoch boundary using cached accounts #24200

behzadnouri · 2022-04-08T17:27:36Z

Problem

Updating rewards at epoch boundary is slow; and Loading vote and stake accounts from accounts-db takes a significant portion of that computation time.

Summary of Changes

This change bypasses accounts-db on the read path and instead uses vote and stake accounts cached in bank stakes:
https://github.com/solana-labs/solana/blob/d2702201c/runtime/src/stakes.rs#L148-L152
These cached accounts are synchronized with accounts-db after each transaction, and so there should not be any change in the resulting computation:
https://github.com/solana-labs/solana/blob/d2702201c/runtime/src/bank.rs#L4526
Nevertheless, to avoid any chances of introducing a consensus issue, the switch to cached account is feature gated.
In order to maintain backward compatibility, values in stake_delegations map in Stakes struct are now generic. Stakes<Delegation> is equivalent to the old code and is used for backward compatibility in BankFieldsTo{Serialize,Deserialize}.
But banks cache Stakes<StakeAccount> which includes the entire stake account and StakeState deserialized from the account. Doing so, will remove the need to load the stake account from accounts-db when working with stake-delegations.
The change shows ~30% improvement in update_rewards_with_thread_pool_us metric.

Feature Gate Issue: #24278

behzadnouri · 2022-04-08T17:30:07Z

@jstarry still doing more testings but you may want to take a look in case of comments.

codecov · 2022-04-10T16:53:59Z

Codecov Report

Merging #24200 (d950b39) into master (3155270) will increase coverage by 11.9%.
The diff coverage is 80.6%.

❗ Current head d950b39 differs from pull request most recent head 634b353. Consider uploading reports for the commit 634b353 to get more accurate results

@@             Coverage Diff             @@
##           master   #24200       +/-   ##
===========================================
+ Coverage    70.0%    82.0%    +11.9%     
===========================================
  Files          36      594      +558     
  Lines        2255   164444   +162189     
  Branches      322        0      -322     
===========================================
+ Hits         1580   134891   +133311     
- Misses        560    29553    +28993     
+ Partials      115        0      -115

jeffwashington · 2022-04-11T20:11:09Z

what did you learn?

jstarry

Great work, the serialization compatibility in particular looks really good. I'm hoping that updating StakeAccount a bit will help remove a lot of the fallible methods when converting from StakeAccount to Delegation.

runtime/src/stakes.rs

runtime/src/stake_account.rs

runtime/src/bank.rs

runtime/src/stakes.rs

runtime/src/bank.rs

runtime/src/epoch_stakes.rs

runtime/src/bank.rs

jstarry · 2022-04-12T08:36:27Z

@behzadnouri since this PR introduces a new feature gate, please create a new feature tracker issue and link it to this PR, thanks! Details here: https://github.com/solana-labs/solana/blob/master/CONTRIBUTING.md#the-pr--issue-labels

behzadnouri · 2022-04-12T14:01:07Z

@behzadnouri since this PR introduces a new feature gate, please create a new feature tracker issue and link it to this PR, thanks! Details here: https://github.com/solana-labs/solana/blob/master/CONTRIBUTING.md#the-pr--issue-labels

done.
#24278

behzadnouri · 2022-04-12T14:13:27Z

what did you learn?

@jeffwashington As mentioned in the description, the change shows ~30% improvement in update_rewards_with_thread_pool_us metric. However that was done before some further optimizations I did regarding storing stakes in EpochStakes;

Note that this change will save both on loading accounts (by bypassing accounts-db altogether) and deserializing state from accounts (because the state is already deserialized in these cached accounts). Also does not force evicting other accounts from readonly cache.

t-nelson · 2022-04-15T04:46:25Z

runtime/src/vote_account.rs

@@ -196,6 +196,12 @@ impl From<AccountSharedData> for VoteAccount {
    }
 }

+impl From<VoteAccount> for AccountSharedData {
+    fn from(account: VoteAccount) -> Self {
+        Self::from(account.0.account.clone())


wen stable unwrap_or_clone 🥺

runtime/src/stakes.rs

t-nelson · 2022-04-15T05:22:34Z

runtime/src/stake_account.rs

+        };
+        let stake_state = StakeState::Stake(Meta::example(), Stake::example());
+        let mut account = Account::example();
+        account.data.resize(256, 0u8);


does the extra 56 bytes make a difference in how this thing works?

no it does not. reduced the size to 196 bytes.

size_of<StakeState>() is 200, fwiw

maybe because of struct paddings?

Yeah, there's some padding, I added a constant to prevent this issue in the future: #24416

runtime/src/stakes.rs

t-nelson · 2022-04-15T06:33:57Z

runtime/src/bank.rs

+            // vote_accounts_cache_miss_count is shown to be always zero.
+            let account = self.get_account_with_fixed_root(vote_pubkey)?;
+            if account.owner() == &solana_vote_program
+                && VoteState::deserialize(account.data()).is_ok()


should we reuse this work to construct the VoteAccount?

The code already has comments here that this is only for sanity check and this path should never return a valid vote-account. My intention is to remove this piece all together once it is confirmed vote_accounts_cache_miss_count is always zero.

@behzadnouri I missed this earlier but I think that the code here will produce false positives for the vote_accounts_cache_miss_count counter. We only add vote accounts to the cache if they are initialized so it's possible that an uninitialized vote account has a stake delegation and deserializes correctly. If that happens, this counter would indicate that we had a cache miss but the cache was actually correct. This all stems from the fact that the vote state doesn't have an intuitive "uninitialized" state. Best we can do is check if the authorized voter is non-zero:

solana/programs/vote/src/vote_state/vote_state_versions.rs

Lines 54 to 62 in 7cd7173

pub fn is_uninitialized(&self) -> bool {

match self {

VoteStateVersions::V0_23_5(vote_state) => {

vote_state.authorized_voter == Pubkey::default()

}

VoteStateVersions::Current(vote_state) => vote_state.authorized_voters.is_empty(),

}

}

So then shouldn't this code also check if the vote-account is initialized?
https://github.com/solana-labs/solana/blob/30d2b112e/runtime/src/bank.rs#L2848-L2888

Seems to me either we should skip uninitialized vote accounts there as well, or store them in the cache as well.

Yes, exactly. I brought this up because I think this code needs to check for initialization but it isn't doing that right now.

t-nelson

lgtm! just a nit

t-nelson · 2022-04-15T18:41:22Z

runtime/src/bank.rs

+                        Err(stake_account::Error::InvalidDelegation(_)) => {
+                            // This should not happen because StakeAccount<()>
+                            // does not check if stake state is a Delegation.
+                            return;


log if it happens anyway?

jstarry · 2022-04-16T04:25:57Z

runtime/src/bank.rs

+            let (account, _slot) = bank_rc.accounts.load_with_fixed_root(&ancestors, pubkey)?;
+            Some(account)
+        })
+        .unwrap();


This will panic if the stakes cache and accountsdb are inconsistent right? Seems like we should should rebuild the cache in this case, what do you think?

Well it is a major logic bug if these are inconsistent, and can indicate bugs in the cache or accounts-db or a corrupted snapshot.
I think having this panic here will actually help to detect such inconsistencies earlier.
Should this actually happen, I think the best would be to hold on going down this path until we can debug this inconsistency.

I think having this panic here will actually help to detect such inconsistencies earlier.

This is true, can you change the unwrap to an expect with a message describing that it "can indicate bugs in the cache or accounts-db or a corrupted snapshot"

Should this actually happen, I think the best would be to hold on going down this path until we can debug this inconsistency.

You mean try this on our nodes first to see if it's happening?

can you change the unwrap to an expect with a message describing that it "can indicate bugs in the cache or accounts-db or a corrupted snapshot"

done.

Should this actually happen, I think the best would be to hold on going down this path until we can debug this inconsistency.

You mean try this on our nodes first to see if it's happening?

For all nodes which submit metrics we can monitor panics through the panic hook:
https://github.com/solana-labs/solana/blob/6c5a3ca4a/metrics/src/metrics.rs#L442-L454

So any node which runs the master branch, ours or not ours, will do. And then testnet nodes once this is backported to testnet.

Cool, sounds good

runtime/src/stake_account.rs

Pull request has been modified.

behzadnouri · 2022-04-19T16:25:21Z

@carllin @ryoqun It will be great if you can also take a look in case I am missing something. thanks.

runtime/src/bank.rs

Pull request has been modified.

The added type does sanity checks on the Account and stores deserialized StakeState. Following commits will use this type instead of Delegation in Stakes.

The commit makes values in stake_delegations map in Stakes struct generic. Stakes<Delegation> is equivalent to the old code and is used for backward compatibility in BankFieldsTo{Serialize,Deserialize}. But banks cache Stakes<StakeAccount> which includes the entire stake account and StakeState deserialized from account. Doing so, will remove the need to load stake account from accounts-db when working with stake-delegations.

For backward compatibility, we can only serialize and deserialize Stakes<Delegation>. However Bank caches Stakes<StakeAccount>. This type mismatch incurs a conversion cost at epoch boundary when updating EpochStakes. This commit adds StakesEnum which allows EpochStakes to include either a Stakes<StakeAccount> or Stakes<Delegation> and so bypass the conversion cost between the two at the epoch boundary.

Loading vote and stake accounts from accounts-db takes a significant portion of time updating rewards at epoch boundary. This commit bypasses accounts-db and instead uses vote and stake accounts cached in bank stakes: https://github.com/solana-labs/solana/blob/d2702201c/runtime/src/stakes.rs#L148-L152 These cached accounts are synchronized with accounts-db after each transaction, and so there should not be any change in the resulting computation: https://github.com/solana-labs/solana/blob/d2702201c/runtime/src/bank.rs#L4526 Nevertheless, to avoid any chances of introducing a consensus issue, the switch to cached account is feature gated.

StakeAccount<Delegation> can only wrap a stake-state which is a Delegation; whereas StakeAccount<()> wraps any account with stake state. As a result, StakeAccount::<Delegation>::delegation() will return Delegation instead of Option<Delegation>.

Tracking mismatches between cached vote/stake accounts and accounts-db in preparation of activating feature for updating rewards at epoch-boundary using cached vote/stake accounts.

behzadnouri force-pushed the cache-stake-accounts-lite branch 2 times, most recently from b5d9c53 to d1ff7ac Compare April 8, 2022 20:27

behzadnouri force-pushed the cache-stake-accounts-lite branch from d1ff7ac to fa6e579 Compare April 11, 2022 19:42

behzadnouri force-pushed the cache-stake-accounts-lite branch from fa6e579 to 00a1173 Compare April 11, 2022 22:01

jstarry reviewed Apr 12, 2022

View reviewed changes

jstarry added the feature-gate Pull Request adds or modifies a runtime feature gate label Apr 12, 2022

behzadnouri mentioned this pull request Apr 12, 2022

Feature Gate: updates rewards at epoch boundary using cached accounts #24278

Closed

behzadnouri force-pushed the cache-stake-accounts-lite branch 7 times, most recently from 4cafe3c to d83c161 Compare April 14, 2022 15:48

behzadnouri requested review from carllin, ryoqun, sakridge and t-nelson April 14, 2022 17:23

behzadnouri mentioned this pull request Apr 14, 2022

implement vote_accounts::update #24350

Closed

t-nelson reviewed Apr 15, 2022

View reviewed changes

jstarry mentioned this pull request Apr 15, 2022

Lack of consistency checks for stakes cache #24376

Closed

behzadnouri force-pushed the cache-stake-accounts-lite branch from d83c161 to abb347f Compare April 15, 2022 17:26

t-nelson previously approved these changes Apr 15, 2022

View reviewed changes

jstarry reviewed Apr 16, 2022

View reviewed changes

behzadnouri force-pushed the cache-stake-accounts-lite branch from abb347f to 7757c08 Compare April 16, 2022 16:23

behzadnouri force-pushed the cache-stake-accounts-lite branch from 7757c08 to d10c2f9 Compare April 19, 2022 13:13

behzadnouri force-pushed the cache-stake-accounts-lite branch from d10c2f9 to 20ab672 Compare April 19, 2022 20:06

jstarry reviewed Apr 20, 2022

View reviewed changes

runtime/src/bank.rs Show resolved Hide resolved

behzadnouri force-pushed the cache-stake-accounts-lite branch from 20ab672 to d950b39 Compare April 20, 2022 16:12

jstarry previously approved these changes Apr 21, 2022

View reviewed changes

behzadnouri force-pushed the cache-stake-accounts-lite branch from d950b39 to 56b9a41 Compare April 21, 2022 13:14

behzadnouri added 6 commits April 21, 2022 09:15

adds StakeAccount type embodying an Account and a StakeState

7555e53

The added type does sanity checks on the Account and stores deserialized StakeState. Following commits will use this type instead of Delegation in Stakes.

collects metrics for cached vote/stake accounts consistency

634b353

Tracking mismatches between cached vote/stake accounts and accounts-db in preparation of activating feature for updating rewards at epoch-boundary using cached vote/stake accounts.

behzadnouri force-pushed the cache-stake-accounts-lite branch from 56b9a41 to 634b353 Compare April 21, 2022 13:16

behzadnouri merged commit d524ae2 into solana-labs:master Apr 21, 2022

behzadnouri deleted the cache-stake-accounts-lite branch April 21, 2022 15:28

behzadnouri mentioned this pull request Apr 19, 2023

Rewards Optimization: Optimize Epoch Reward Compute Code - no join of vote accounts with stake accounts #31193

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updates rewards at epoch boundary using cached accounts #24200

updates rewards at epoch boundary using cached accounts #24200

behzadnouri commented Apr 8, 2022 •

edited

Loading

behzadnouri commented Apr 8, 2022

codecov bot commented Apr 10, 2022 •

edited

Loading

jeffwashington commented Apr 11, 2022

jstarry left a comment

jstarry commented Apr 12, 2022

behzadnouri commented Apr 12, 2022

behzadnouri commented Apr 12, 2022

t-nelson Apr 15, 2022

t-nelson Apr 15, 2022

behzadnouri Apr 15, 2022

t-nelson Apr 15, 2022

behzadnouri Apr 16, 2022

jstarry Apr 17, 2022

t-nelson Apr 15, 2022

behzadnouri Apr 15, 2022

jstarry Jun 25, 2022

behzadnouri Jun 25, 2022

jstarry Jun 26, 2022

t-nelson left a comment

t-nelson Apr 15, 2022

behzadnouri Apr 16, 2022

jstarry Apr 16, 2022

behzadnouri Apr 16, 2022

jstarry Apr 16, 2022

behzadnouri Apr 16, 2022

jstarry Apr 20, 2022

behzadnouri commented Apr 19, 2022

	pub fn is_uninitialized(&self) -> bool {
	match self {
	VoteStateVersions::V0_23_5(vote_state) => {
	vote_state.authorized_voter == Pubkey::default()
	}

	VoteStateVersions::Current(vote_state) => vote_state.authorized_voters.is_empty(),
	}
	}

updates rewards at epoch boundary using cached accounts #24200

updates rewards at epoch boundary using cached accounts #24200

Conversation

behzadnouri commented Apr 8, 2022 • edited Loading

Problem

Summary of Changes

behzadnouri commented Apr 8, 2022

codecov bot commented Apr 10, 2022 • edited Loading

Codecov Report

jeffwashington commented Apr 11, 2022

jstarry left a comment

Choose a reason for hiding this comment

jstarry commented Apr 12, 2022

behzadnouri commented Apr 12, 2022

behzadnouri commented Apr 12, 2022

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

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

Choose a reason for hiding this comment

t-nelson left a 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

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

behzadnouri commented Apr 19, 2022

behzadnouri commented Apr 8, 2022 •

edited

Loading

codecov bot commented Apr 10, 2022 •

edited

Loading