-
Notifications
You must be signed in to change notification settings - Fork 179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
eth: Fix active total stake calc in RewardWithHint #2029
Conversation
Thanks Yondon 👍 |
374f09c
to
eff16f8
Compare
33e5588
to
01af092
Compare
@@ -792,7 +794,7 @@ func (c *client) Reward() (*types.Transaction, error) { | |||
return nil, errors.Wrapf(err, "unable to get transcoder pool max size") | |||
} | |||
|
|||
hints := simulateTranscoderPoolUpdate(addr, reward.Add(reward, ep.TotalStake), transcoders, len(transcoders) == int(maxSize.Int64())) | |||
hints := simulateTranscoderPoolUpdate(addr, reward.Add(reward, tr.DelegatedStake), transcoders, len(transcoders) == int(maxSize.Int64())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, we used the TotalStake
field on the earnings pool here. There were two problems with this:
- If the orchestratrator's
LastActiveStakeUpdateRound
is before the current round, then theTotalStake
field would be 0 and empty hints would be returned which results in the increased gas usage described in eth: Fix active total stake calc in RewardWithHint #2029 - Even if the correct earnings pool was fetched, the
TotalStake
field on the earnings pool would reflect the orchestrator's active total stake which remains fixed throughout the current round. If the orchestrator receives delegation during the round, its active total stake would not change, but its total stake would change and its total stake is the value that should be used forsimulateTranscoderPoolUpdate
Both 1 and 2 are resolved by using the DelegatedStake
field on the Transcoder
struct which accurately reflects the orchestrator's total stake at all points in the round.
if err != nil { | ||
return nil, err | ||
} | ||
|
||
ep, err := c.GetTranscoderEarningsPoolForRound(c.accountManager.Account().Address, currentRound) | ||
ep, err := c.GetTranscoderEarningsPoolForRound(addr, tr.LastActiveStakeUpdateRound) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, we always fetched the earnings pool for the current round. When LastActiveStakeUpdateRound
is in the past, the TotalStake
field of the earnings pool for the current round would be 0 and the total stake of the orchestrator would be stored in the earnings pool for LastActiveStakeUpdateRound
. This is resolved by always fetching the earnings pool for LastActiveStakeUpdateRound
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What does this pull request do? Explain your changes. (required)
This PR fixes a bug in the total stake calculations used for hint generation in
eth.client.Reward()
. I've added in-line PR comments to explain the bug and the solutions used.Specific updates (required)
See commit history.
How did you test each of these updates (required)
Tested manually using devtool.
Does this pull request close any open issues?
Fixes #2015
Checklist:
make
runs successfully./test.sh
pass