stake: Don't pay out rewards for epochs where inflation was not enabled#13680
Conversation
t-nelson
force-pushed
the
no-preinflation-stake-rewards
branch
from
07448b2 to
e0b3db4
Compare
Codecov Report
@@ Coverage Diff @@
## master #13680 +/- ##
=======================================
Coverage 82.1% 82.1%
=======================================
Files 381 381
Lines 91948 91955 +7
=======================================
+ Hits 75561 75574 +13
+ Misses 16387 16381 -6 |
|
Thanks for coming up with this. @t-nelson I like this approach. Here's a bit of context and my reasoning for that. When I originally wrote #13233, the situation was rather limited, not needing a gating for stake program. What a good old peaceful day. I was only aware of small inflation skew..... Also, I was completely new to the relevant code. So, I originally approached to the solution of not going outside inflation code (thereby not modifying accounts so that we can deploy changes without gating) and opted for not rewriting stake accounts. But, the situation changed significantly.... now that we have whole switch for the stake native program and we'll change staking calculation for stuck deactivating ones, fixing spoof bugs, and even more we'll rewrite bunch of stake accounts. And now I've been really deep into the code. So, why not updating like this pr in aggressive implementation change way? ;) Especially, this is a blocker for the SoM and stake merges at the same time. |
t-nelson
force-pushed
the
no-preinflation-stake-rewards
branch
from
e0b3db4 to
c695366
Compare
mergify
Bot
dismissed
CriesofCarrots’s stale review
Pull request has been modified.
|
Updated with a much simpler change set after some great discussions with @ryoqun and @CriesofCarrots. Many thanks to both! |
CriesofCarrots
left a comment
CriesofCarrots
left a comment
There was a problem hiding this comment.
😍 how small this fix ended up!
ryoqun
left a comment
ryoqun
left a comment
There was a problem hiding this comment.
LGTM! Also, I checked this like before by running ledger-tool with mainnet-beta snapshot; I confirmed this pr actually updates all stake delegations' credits_observed only when inflation is disabled.
t-nelson
added
the
automerge
seems like my check was wrong in some way..: #13836. |
well, it turns out mainnet-beta ledger didn't have those problematic stake accounts in fact. So, my testing with mb was rather correct by itself. the testnet had those problematic stake accounts.... I really wish I had tested with testnet as well at the time......... Well, I thought we really care about mainnet-beta and mainnet-beta has been used by real customers and I tended to think it has most exhaustive set of various accounts state, regarding testing only it is enough to stamp LGTM (this laziness was combined with time-pressures as well). I learnt an important lesson here.... |
3 participants
Problem
The stake program will redeem rewards for all unpaid epochs in the delegate's vote history up to the current one, including those for which inflation was disabled
Summary of Changes
An alternative approach to #13233
Always update the stake state's
observed_creditsstamp, when inflation is disabled.closes #13233