Calculates voting reward using an off curve account #694
Conversation
akhi3030
force-pushed
the
akhi3030/calc-reward-off-curve-account
branch
from
fb1f9ce to
9f40259
Compare
akhi3030
force-pushed
the
akhi3030/calc-reward-off-curve-account
branch
4 times, most recently
from
8513336 to
e862c89
Compare
akhi3030
force-pushed
the
akhi3030/calc-reward-off-curve-account
branch
from
e862c89 to
0f486b5
Compare
akhi3030
changed the title
runtime/src/bank/tests.rs
Outdated
| assert_eq!( | ||
| bank.hash().to_string(), | ||
| "6h1KzSuTW6MwkgjtEbrv6AyUZ2NHtSxCQi8epjHDFYh8" | ||
| "BURo5b1ZCkgvjE4z9KviRgz6iRoQRad9JKWE82wjXPSc" |
There was a problem hiding this comment.
I am not sure if this is the right way to fix this test. Suggestions are welcome.
There was a problem hiding this comment.
Hmm... this test is succeeding for me locally but failing on CI. I am not sure how to fix it yet. Investigating... if someone has hints, suggestions welcome.
There was a problem hiding this comment.
Do we know what precise change triggers this bank hash change?
There was a problem hiding this comment.
I am a bit stumped with this test. It is failing on CI and locally. The hash my local machine computes is BURo5b1ZCkgvjE4z9KviRgz6iRoQRad9JKWE82wjXPSc and CI computes GAcpLy2beH4eyygZaprWWzn4geSCd3xvLvnn2tudvhy1.
I think the hash is changing because we are adding a new account to the bank. But why are the computed hashes different locally and on CI?
CC: @AshwinSekar
| let per_slot_inflation_lamports = | ||
| epoch_validator_rewards_lamports as f64 / slots_per_epoch as f64; | ||
| let fractional_stake_lamports = validator_stake_lamports as f64 / total_stake_lamports as f64; | ||
| round(fractional_stake_lamports * per_slot_inflation_lamports) |
There was a problem hiding this comment.
As discussed in #682, please avoid using f64's in rewards calculations. f64 calculations are not deterministic across all CPU architectures, which may lead to non-determinism in rewards calculations, which could in turn lead to bank hash mismatches.
Instead:
let num = (epoch_validator_rewards_lamports as u128) * (validator_stake_lamports as u128);
let denom = (slots_per_epoch as u128) * (total_stake_lamports as u128);
num / denomWe should hard unwrap num / denom to a u64, since it's mathematically impossible for num / denom to not be able to fit into a u64 (probably a good idea to test this).
There was a problem hiding this comment.
I like the suggestion and made the change.
since it's mathematically impossible for num / denom to not be able to fit into a u64 (probably a good idea to test this).
Not sure I understand this point though. I would imagine that the result should fit in u64 because we expect the inflation to fit in u64. Or to put it another way, could you provide some details on how I can do this test please?
There was a problem hiding this comment.
since it's mathematically impossible for num / denom to not be able to fit into a u64
We're saying the same thing - the result must fit into a u64.
runtime/src/bank/tests.rs
Outdated
| assert_eq!( | ||
| bank.hash().to_string(), | ||
| "6h1KzSuTW6MwkgjtEbrv6AyUZ2NHtSxCQi8epjHDFYh8" | ||
| "BURo5b1ZCkgvjE4z9KviRgz6iRoQRad9JKWE82wjXPSc" |
There was a problem hiding this comment.
Do we know what precise change triggers this bank hash change?
| if enable_alpenglow && enable_vat { | ||
| // VAT should be burned | ||
| assert!(capitalization_epoch_1 < capitalization_epoch_0); | ||
| // We expect the VAT to be burned so epoch 1's capitalization should be lower however |
There was a problem hiding this comment.
I suppose a more elegant way to fix this test would be to have it run over 3 epochs. In the first epoch the vote reward account doesn't exist, in the second epoch it is created, and in the first epoch we enable AG and burn the VAT. Should I try to make that change or is this way of handling the issue good enough?
Problem
Based on the reward certs, we need to calculate and pay the voting rewards.
Summary of changes
This PR tackles the calculation part of the problem.