Add redeem_vote_credits to runtime #7910
Conversation
rob-solana
force-pushed
the
Move-redeem_vote_credits-into-runtime
branch
2 times, most recently
from
d575773 to
c33b229
Compare
Codecov Report
@@ Coverage Diff @@
## master #7910 +/- ##
========================================
Coverage ? 82%
========================================
Files ? 244
Lines ? 52684
Branches ? 0
========================================
Hits ? 43233
Misses ? 9451
Partials ? 0 |
| /// iterate over all stakes, redeem vote credits for each stake we can | ||
| /// successfully load and parse, return total payout | ||
| fn pay_validator_rewards(&self, validator_point_value: f64) -> u64 { | ||
| let stake_history = self.stakes.read().unwrap().history().clone(); |
There was a problem hiding this comment.
the self.store() below needs a write lock on self.stakes, so if I hold a ref to the stake_history, I hold a read lock and get a deadlock
There was a problem hiding this comment.
Can't inline it? Maybe I'm missing a loop that makes that impractical.
stake.calculate_rewards(
validator_point_value,
&vote_state,
Some(&self.stakes.read().unwrap().history()),
)There was a problem hiding this comment.
that's exactly what I had that caused deadlock ;)
runtime/src/bank.rs
Outdated
| .map( | ||
| |( | ||
| stake_pubkey, | ||
| Delegation { |
There was a problem hiding this comment.
How about just calling this delegation and using delegation.voter_pubkey below?
There was a problem hiding this comment.
I thought the vote_pubkey form looked better, matched stake_pubkey. voter_pubkey is an ancient name, and changing it perturbs cli, rpc
There was a problem hiding this comment.
I'm not a fan of this style, but I suppose it's just personal preference. I don't feel strongly that it needs to change.
There was a problem hiding this comment.
I changed it and prefer your style
runtime/src/bank.rs
Outdated
| }, | ||
| )| { | ||
| match ( | ||
| self.get_account(&stake_pubkey), |
There was a problem hiding this comment.
Since you return zero in all the edge cases, how about moving all this code into a function that returns an Option, so that you can get rid of all the nested blocks:
let mut stake_state = self.get_account(&stake_pubkey)?.state().ok()?;
let mut vote_state = self.get_account(&vote_pubkey)?.state().ok()?;There was a problem hiding this comment.
when I moved the meat of the function into a separate function, I ended up with a 7 or 8 parameter function...
There was a problem hiding this comment.
Sounds like a strong indictor something needs to be done here. Maybe multiple functions!
There was a problem hiding this comment.
I considered collecting what needed to be stored to flatten out the code. I also tried a single, 4-parameter match but ran afoul of the borrow checker.
| }, | ||
| ) | ||
| .sum() | ||
| } |
There was a problem hiding this comment.
much of testing is covered by test_bank_update_rewards(). bit that's missing is balance update verifications
There was a problem hiding this comment.
Is it though? This thing has a whole lot of branches.
There was a problem hiding this comment.
it has essentially 1 branch: whether or not there's meaningful rewards to redeem (tested to death in the stake program). All the corner cases are there to handle the types and not abort.
maybe I should replace all the matches with simple unwrap() or expect(). there are no valid cases where information coming out of bank.stakes can disagree with what's in the accounts_db.
rob-solana
force-pushed
the
Move-redeem_vote_credits-into-runtime
branch
3 times, most recently
from
6229578 to
fe17b9e
Compare
rob-solana
force-pushed
the
Move-redeem_vote_credits-into-runtime
branch
from
fe17b9e to
ed8d6f3
Compare
rob-solana
force-pushed
the
Move-redeem_vote_credits-into-runtime
branch
from
ed8d6f3 to
48694ee
Compare
Problem
RedeemVoteCredits has a number of problems:
Somebody must issue this instruction every MAX_EPOCH_CREDITS_HISTORY epochs for each stake/vote account or rewards evaporate.
Reward point values are computed based on the latest epoch, which can produce confusing and hard to understand results as this example from TdS DR6 shows:
After the first 24 hours of idling, redeeming votes
using a ~1 SOL stake produced ~6221 SOL in voter rewards. Redeeming
that same stake 12 hours later produced ~54 SOL in voter rewards. The reason
for this huge reward drop is because the total stake increased from 435 SOL to 25695
SOL in those 12 hours, so the credit point value was much smaller.
The stake/vote accounts/programs as well as runtime contains code/data bloat that exists only to enable RedeemVoteCredits in its current form.
The first two points highlight the unintentional game that will be played around RedeemVoteCredits: if you see observe stake decreasing in the cluster than you're better off delaying RedeemVoteCredits if possible to capitalize on the increasing credit point value. But don't delay too long or you'll lost it all. Worse, RedeemVoteCredits can be run by anybody so others can inflict less desirable credit point values on you.
The alternative is to dispense of RedeemVoteCredits entirely, and have banks automatically redeem vote credits at the end of each epoch for all parties.
Summary of Changes
add redeem_vote_credits to runtime
CC #7835