Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Store and compute node/stake state in EpochStakes struct#8958

Merged
carllin merged 7 commits intosolana-labs:masterfrom
carllin:FixClusterInfo
Mar 25, 2020
Merged

Store and compute node/stake state in EpochStakes struct#8958
carllin merged 7 commits intosolana-labs:masterfrom
carllin:FixClusterInfo

Conversation

@carllin
Copy link
Copy Markdown
Contributor

@carllin carllin commented Mar 19, 2020

Problem

  1. EpochStakes cloned Stakes on every new bank, potentially expensive
  2. No convenient way to find epoch-based information critical to consensus like mappings from node ids to vote accounts, vote accounts to authorized voters, total epoch stake.

Summary of Changes

  1. Wrap EpochStakes in Arc's to prevent expensive clones on each Bank, only needs to be recomputed when crossing leader_schedule_epoch boundaries
  2. Save off Epoch-based information and add convenience functions

Note @mvines this breaks the ABI (serializing/deserializing the bank), I've add this to track here: https://github.com/solana-labs/solana/projects/45

Fixes #

@carllin carllin requested a review from mvines March 19, 2020 07:58
@mvines
Copy link
Copy Markdown
Contributor

mvines commented Mar 19, 2020

😢 we'll need to halt and restart devnet, tds, and mainnet-beta with a snapshot rewrite. I'll look more closely at this PR tomorrow @carllin

@carllin
Copy link
Copy Markdown
Contributor Author

carllin commented Mar 19, 2020

😢 we'll need to halt and restart devnet, tds, and mainnet-beta with a snapshot rewrite. I'll look more closely at this PR tomorrow @carllin

Ugh, I know hehe. I debated stashing this info elsewhere but it started getting really ugly/hacky, so I bit the bullet.

@carllin carllin force-pushed the FixClusterInfo branch 2 times, most recently from afd3865 to d68b0f4 Compare March 20, 2020 02:47
Comment thread runtime/src/epoch_stakes.rs Outdated
.get_authorized_voter(leader_schedule_epoch)
.expect("Authorized voter for current epoch must be known");

node_id_to_vote_accounts
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@aeyakovenko, fyi, the mapping from the node_id used in gossip to the set of vote_account's is fixed here at leader_schedule_epoch boundaries (Aka the mappinig for epoch n+2 is fixed halfway through epoch n). Hopefully that should be ok

@carllin carllin force-pushed the FixClusterInfo branch 4 times, most recently from 51ec328 to c1cfad7 Compare March 20, 2020 03:02
@carllin carllin marked this pull request as ready for review March 20, 2020 03:02
mvines
mvines previously requested changes Mar 20, 2020
Copy link
Copy Markdown
Contributor

@mvines mvines left a comment

Choose a reason for hiding this comment

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

I'd like to find a way to roll this out without having to hard fork mainnet-beta

@sakridge
Copy link
Copy Markdown
Contributor

I'd like to find a way to roll this out without having to hard fork mainnet-beta

+1, these kinds of things are just kind of a cache/wrapper over some account state, we need to figure out how to handle that sooner than later I think.

@aeyakovenko
Copy link
Copy Markdown
Member

@mvines @carllin if the state is derived from the account state, do we need to store it in the snapshot? Can it be recomputed at load?

@carllin
Copy link
Copy Markdown
Contributor Author

carllin commented Mar 23, 2020

@aeyakovenko, because the EpochStakes for the current epoch are saved off from the stakes 1.5 epochs ago, it's not entirely derivable, you need the stake information from that point in time.

@carllin carllin mentioned this pull request Mar 23, 2020
@mergify mergify Bot dismissed mvines’s stale review March 24, 2020 03:05

Pull request has been modified.

pub const TAR_VERSION_FILE: &str = "version";

pub const SNAPSHOT_VERSION: &str = "1.0.0";
pub const SNAPSHOT_VERSION: &str = "1.1.0";
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mvines bumped it here

@carllin carllin force-pushed the FixClusterInfo branch 4 times, most recently from 85601a5 to e343480 Compare March 25, 2020 01:17
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 25, 2020

Codecov Report

Merging #8958 into master will decrease coverage by <.1%.
The diff coverage is 73.4%.

@@           Coverage Diff            @@
##           master   #8958     +/-   ##
========================================
- Coverage    80.4%   80.3%   -0.1%     
========================================
  Files         267     268      +1     
  Lines       58543   58684    +141     
========================================
+ Hits        47073   47174    +101     
- Misses      11470   11510     +40

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants