vote_storage: use the correct epoch to filter authorized voter#10522
vote_storage: use the correct epoch to filter authorized voter#10522apfitzge merged 2 commits intoanza-xyz:masterfrom
Conversation
| self.cached_epoch_authorized_voters = bank | ||
| .epoch_stakes(bank.epoch()) | ||
| .map(|stakes| stakes.epoch_authorized_voters().clone()) | ||
| .unwrap_or_else(|| current_epoch_stakes.epoch_authorized_voters().clone()); |
There was a problem hiding this comment.
should be fine to expect but didn't want to intro a new panic in a backport PR
There was a problem hiding this comment.
Sounds good but please add a comment for this. We should clean it up later on master
|
Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10522 +/- ##
========================================
Coverage 83.2% 83.2%
========================================
Files 847 847
Lines 320054 320205 +151
========================================
+ Hits 266440 266591 +151
Misses 53614 53614 🚀 New features to boost your workflow:
|
| cached_epoch_stakes: VersionedEpochStakes, | ||
| /// Authorized voters for the current epoch. This is separate from | ||
| /// cached_epoch_stakes because stakes are offset by one epoch (we use | ||
| /// epoch E + 1 for stake in epoch E), but authorized voters must |
There was a problem hiding this comment.
I'm not understanding why we use E+1 for the cached epoch stakes.
This has to do with stakes changes per epoch right? Can you reply with a timeline of how this changes?
Let's say in bank for slot N (in epoch E) A has 100_000 SOL staked. Is this keyed into E+1, why is that?
In slot N if I stake an additional 50_000 SOL to A, is that keyed in E+1?
There was a problem hiding this comment.
I believe the reason is because the stake is computed in advance which is why it's keyed by the "leader schedule epoch" rather than the bank epoch:
at the beginning of epoch E we compute all stake delegations and these take effect protocol wise in epoch E + 1.
during epoch E if I stake an additional 50k, it's noticed at the start of epoch E + 1 and takes effect in epoch E + 2.
the reason (i think?) why it can't take effect immediately is because there could be forks across the boundary with different stake delegations active, we need a rooted bank for the protocol to use to query stake.
@jstarry is more of an expert in this than I am, so i'll let him confirm.
There was a problem hiding this comment.
during epoch E if I stake an additional 50k, it's noticed at the start of epoch E + 1
Right, so would that additional 50k show up in the current_epoch_stakes value returned? If so then, shouldn't we not be using it here since it doesn't reflect current stake?
Is the current stake of a node not keyed at epoch E, like the current authorized voter key is? Seems like we should be using that for everything
There was a problem hiding this comment.
Right, so would that additional 50k show up in the
current_epoch_stakesvalue returned? If so then, shouldn't we not be using it here since it doesn't reflect current stake?
restating ashwin's statement: "during epoch E if I stake an additional 50k, it's noticed at the start of epoch E + 1 [and stored in the map at E+2]". so, the stake active in E+1 is stored in E+2.
There was a problem hiding this comment.
okay, so so stake changes occuring in E show up in index E+2? But authorized voters apply for index E?
There's a similar epoch delay for the authorized voter change. So why are we using different epochs for authorized voters and stake?
There was a problem hiding this comment.
So why are we using different epochs for authorized voters and stake?
Historically the agave implementation has inconsistently epoch stakes in many places. For example, newly activated stake in epoch E starts earning rewards in epoch E + 1. It should actually not earn rewards until E + 2 because that's when the stake applies for leader schedule slots and consensus vote weight. The historical inconsistencies have been cleaned up partially but not completely and so some things still use the "current epoch" stakes which is arguably poorly named because as you pointed out current stake can be interpreted as the snapshot of activated stake from the previous epoch rather than the stake distribution activated as of the beginning of the current epoch.
So since we still consider stake to be "active" (reward earning) in epoch E + 1 but new authorized voters not active until E + 2, we end up using different epoch stakes here.
There was a problem hiding this comment.
So since we still consider stake to be "active" (reward earning) in epoch E + 1 but new authorized voters not active until E + 2, we end up using different epoch stakes here.
But we don't care about rewards here for the vote-worker, we care about the impact on consensus weight.
These 2 comments:
It should actually not earn rewards until E + 2 because that's when the stake applies for leader schedule slots and consensus vote weight.
but new authorized voters not active until E + 2
make it sound like we should be aligned on the two here, not unaligned?
Is the stake-weight we're using in the vote-worker, i.e. keyed on E+1 from Bank::current_epoch_stakes representing the actual consensus weight applied for whatever epoch we're processing in?
There was a problem hiding this comment.
I think you're right that consensus doesn't use the "current" epoch stakes but it's still important for rewards for votes to get filtered by current epoch stake. If you're in a reward period, your votes should be able to land even if they don't count for consensus until the next epoch.
| self.cached_epoch_authorized_voters = bank | ||
| .epoch_stakes(bank.epoch()) | ||
| .map(|stakes| stakes.epoch_authorized_voters().clone()) | ||
| .unwrap_or_else(|| current_epoch_stakes.epoch_authorized_voters().clone()); |
There was a problem hiding this comment.
Sounds good but please add a comment for this. We should clean it up later on master
c5a6d03 to
b210ab4
Compare
* vote_storage: use the correct epoch to filter authorized voter * pr feedback: comments (cherry picked from commit 87167f5)
…(backport of #10522) (#10537) * vote_storage: use the correct epoch to filter authorized voter (#10522) * vote_storage: use the correct epoch to filter authorized voter * pr feedback: comments (cherry picked from commit 87167f5) * VoteStateV4::new_with_defaults -> new --------- Co-authored-by: Ashwin Sekar <[email protected]>
Problem
We use the
current_epoch_stakesto cache epoch stakes. For a bank in epochEthis uses the epoch stakes fromE + 1.This is required because of how stake computation is performed.
However we also abuse
epoch_stakesto check the authorized voter information. This information should not be offset by 1 epoch. Instead we need to use the authorized voters from epochE.Summary of Changes
Cache the authorized voters separately for use in the filter. Add tests.