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.

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

includes rent_epoch in vote-accounts sanity checks #29861

Merged

Conversation

behzadnouri
Copy link
Contributor

Problem

#26479 preserves rent epoch for rent-exempt accounts; since the feature got activated, vote accounts in stakes cache have identical rent_epoch field as the accounts in accounts-db. The commit verifies this in stakes-cache sanity checks.

Summary of Changes

includes rent_epoch in vote-accounts sanity checks

solana-labs#26479
preserves rent epoch for rent-exempt accounts; since the feature got
activated, vote accounts in stakes cache have identical rent_epoch field
as the accounts in accounts-db. The commit verifies this in stakes-cache
sanity checks.
pubkey, vote_account, account
);
if vote_account != &account {
error!("vote account mismatch: {pubkey}, {vote_account:?}, {account:?}");
return Err(Error::VoteAccountMismatch(*pubkey));
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this halt the validator? IIRC, using the cache is not enabled yet on mnb?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this halt the validator?

It does halt the validator at start up.

This code is only invoked at startup when deserializing bank:
https://github.com/solana-labs/solana/blob/0be194145/runtime/src/bank.rs#L1828-L1841
in order to obtain Stakes<StakeAccount> from Stakes<Delegation> for backward compatibility reasons mentioned in the link. As part of that, it performs these sanity checks.

IIRC, using the cache is not enabled yet on mnb?

We are not using stakes-cache for rewards calculations (yet), but stakes-cache is already used so many other places, including epoch-stakes calculations. Pubkeys for stakes accounts used in rewards calculations are also already obtained from stakes-cache.

Copy link
Contributor

Choose a reason for hiding this comment

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

as a side note, I wonder why we aren't building this cache on startup instead of serializing it. We scan all accounts to generate the index. It seems we could recreate this then. Bank deserialization at load does take a while and all of this is duplicate information derivable from append vecs at startup. Then we would have no inconsistency checks and it seems we could remove code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure where to obtain correct stake_history: StakeHistory field in stakes-cache, but, yeah, as for stake/vote accounts, those can be obtained from accounts-db. It would need a full accounts-db scan though.

I think since we already do serialize it, it would be just easier/faster to go with it.

Then we would have no inconsistency checks and it seems we could remove code.

Any inconsistency spotted by sanity checks here is only a symptom that there is a bug somewhere. The actual culprit of that inconsistency is somewhere else.

Copy link
Contributor

@jeffwashington jeffwashington left a comment

Choose a reason for hiding this comment

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

ok

@behzadnouri behzadnouri merged commit 8a14636 into solana-labs:master Jan 24, 2023
@behzadnouri behzadnouri deleted the vote-account-cmp-rent-epoch branch January 24, 2023 19:47
mergify bot pushed a commit that referenced this pull request Jan 24, 2023
#26479
preserves rent epoch for rent-exempt accounts. Since the feature got
activated, vote accounts in stakes-cache have identical rent_epoch field
as the accounts in accounts-db. The commit verifies this in stakes-cache
sanity checks.

(cherry picked from commit 8a14636)
behzadnouri added a commit that referenced this pull request Jan 25, 2023
…29874)

includes rent_epoch in vote-accounts sanity checks (#29861)

#26479
preserves rent epoch for rent-exempt accounts. Since the feature got
activated, vote accounts in stakes-cache have identical rent_epoch field
as the accounts in accounts-db. The commit verifies this in stakes-cache
sanity checks.

(cherry picked from commit 8a14636)

Co-authored-by: behzad nouri <behzadnouri@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants