Credits auto rewind on vote recreation#22546
Credits auto rewind on vote recreation#22546CriesofCarrots merged 5 commits intosolana-labs:masterfrom
Conversation
| } | ||
| return (0, stake.credits_observed); | ||
| if credits_in_vote <= credits_in_stake { | ||
| let (referenced_credits, forced_credits_update_with_skipped_reward) = if credits_auto_rewind |
There was a problem hiding this comment.
readability: enum NoCredit:{Delinquent, Recreated} or should_rewrite
There was a problem hiding this comment.
or out_of_sync/outdated? is_stale_credits
ryoqun
force-pushed
the
credits-auto-rewind
branch
from
aff230a to
de5bf81
Compare
Codecov Report
@@ Coverage Diff @@
## master #22546 +/- ##
===========================================
+ Coverage 70.0% 82.0% +11.9%
===========================================
Files 36 596 +560
Lines 2255 165080 +162825
Branches 322 0 -322
===========================================
+ Hits 1580 135460 +133880
- Misses 560 29620 +29060
+ Partials 115 0 -115 |
|
I ran note that no reward before&after the feature activation, new_credits_observed, and skipped_reasons. |
fyi: i've rebased out-of-tree pretty hacky inflation validation subcommand of |
|
Also, around +0.007% (currently stalled due to the accidental vote account recreation) stakes will be started to earn staking rewards after this feature activation. this should be negligible overall |
|
(status: this pr is almost ready for review. it went through rather rigorous self-review including full re-audit of staking code) |
ryoqun
force-pushed
the
credits-auto-rewind
branch
from
70bb7c0 to
8e3962f
Compare
| stake_history, | ||
| inflation_point_calc_tracer.as_ref(), | ||
| ); | ||
| let (points, credits_observed, mut forced_credits_update_with_skipped_reward) = |
There was a problem hiding this comment.
a bit contrived flag coding. but i rather wanted some encapsulation for maintainability (place new code at lowest-level to be touched by all codepath even including currently-hypothetical code, which isn't via this fn calculate_stake_rewards) and separation-of-concerns (no visible muddy credits handling with feature activation here and put it into calculate_stake_point_and_credits)
ryoqun
changed the title
|
@CriesofCarrots this is finally ready for review. :) |
CriesofCarrots
left a comment
CriesofCarrots
left a comment
There was a problem hiding this comment.
Thank you, @ryoqun , this looks great! Just wondering if it's time for some helper return structs to keep values clear, but I'm okay to ship as-is, if you want to keep the changeset smaller.
|
status: this pr is pended because of questioned risk/benefits for changing on this (#22512 (comment) ; as a stake-related engineer, i think this pr is safe and might be beneficial but not so strongly opinionated to ship this) and i requested audits to @bennofs (indicated he can find something interesting in this change) |
|
quite busy ATM, but I'll look at this early next week. |
There was a problem hiding this comment.
Thanks for waiting for me, I reviewed the changes and it looks good to me. My reasoning for why this is safe:
- the rewind code is only triggered when there is a vote account with lower credits than observed in a rooted bank. This means that there is no way that any vote state with higher credits observed is obtainable.
- the codepath that calls the rewind code reloads the stake account from the bank, so even if there were inconsistencies in the stakes cache these wouldn't matter here
- rewinding only happens at the end of the epoch, and no rewards are paid for ending epoch. So the outcome is the same as you would get by just unstaking and restaking
I also did a quick search for credits_observed in the code base just to make sure that we didn't forget about something that uses that field and would break, but only found the stake program and some cli tools/tests so that's good as well.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
stale
Bot
added
the
stale
|
@ryoqun , what are you thinking about this one? |
|
I still think we should make this fix, as current behavior is objectively broken. Assuming I can push to this branch, I will rebase here. |
|
oh i didn't realize we never landed this 😕 |
stale
Bot
removed
the
stale
CriesofCarrots
force-pushed
the
credits-auto-rewind
branch
from
570ec9b to
3e6748b
Compare
mergify
Bot
dismissed stale reviews from t-nelson and CriesofCarrots
Pull request has been modified.
CriesofCarrots
added
feature-gate
@CriesofCarrots thanks for taking over and rebasing this. it's embarrassing for me not to be able to work on this for such long time. i have been focusing on the prolonged cluster-wide instability with primary focus of assisting affected ecosystem partners. maybe, it's time for me to take a breather and address not-highest-priority tasks... as @mvines reacted with
as said above, i haven't spent time on this regarding testing... And, I think it's required, considering the touched code. Once this lands (with guarantee of shipping eventually), i'll report back here with results with procedures (i.e. csv, its Also, I'll monitor few epochs before and after around devnet/testnet feature activation. lastly, sorry for not properly communicating about this pr's stalemate. |
Thank you, that sounds perfect. Yes, I'm planning to let this ride in on v1.11 without backporting. |
* Credits auto rewind on vote recreation * Update comment * Improve comments and tests * Recommended fn rename * Restore old feature, and replace new feature key Co-authored-by: Tyera Eulberg <tyera@solana.com>
fyi, I'm still not forgetting this pr and this live-cluster verification chore. :) I'm just waiting v1.11 upgrade for devnet and testnet. |
* Credits auto rewind on vote recreation * Update comment * Improve comments and tests * Recommended fn rename * Restore old feature, and replace new feature key Co-authored-by: Tyera Eulberg <tyera@solana.com>
* Credits auto rewind on vote recreation * Update comment * Improve comments and tests * Recommended fn rename * Restore old feature, and replace new feature key Co-authored-by: Tyera Eulberg <tyera@solana.com>
@CriesofCarrots this is done with no concerns found. I've found no not-explainable wierdness after carefully investigating actual inflation distributions after the feature activation on testnet. (for the very curious stranger who is eager to redo the verification step, this is done with rebased out-of-tree ugly inflation validation code, which I'm carrying in my bag for years: (found here: ryoqun@5343644) and the dataset used was here https://drive.google.com/drive/folders/13DE2dvdfhfhN5VWNIDaJQKGgOLmWr4k9?usp=sharing) So, devnet and mainnet-beta is remaining.. |
|
this feature finally was activated. in short, it seems working as expected: (this is redo of #22546 (comment))
1.5 years has passed. +0.006% started to earn rewards thanks to this feature activation; seems the total staked amount is mostly unchanged and the stuck stake amount reduced because some of newly recreated vote accounts surpassed credits of deleted vote accounts. |
as this sampled vote account had already been earning staking rewards prior to this feature activation, i've chosen different sampled stake account: you can see this account started to earn reward only after epoch 464 (this feature got activated) and onwards. After that, these stake accounts are just normal stake accounts. so, I've seen no rewound stakes accounts at epoch 465. |
|
also, I've checked all stuck stake accounts are normalized to same credits by vote accounts (there were 4 bad vote accounts at the time of feature activation): |
4 participants
still draft; exploring best-desired least-intrusive code changes with some preparatory code cleanings (should be separated into its own pr)...todo:
Problem
(See #22512)
Summary of Changes
(See #22512)
Fixes #22512