Skip to content
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

staking: Fix reward distribution when common pool is exhausted #5319

Merged
merged 1 commit into from Jul 12, 2023

Conversation

abukosek
Copy link
Contributor

This PR fixes the scenario when the reward schedule hasn't completed yet, but the common pool has been exhausted.

@abukosek abukosek force-pushed the andrej/bugfix/rewards-dont-panic branch from de23845 to 3d2aa62 Compare July 11, 2023 14:31
@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Merging #5319 (3d2aa62) into master (0961dcc) will decrease coverage by 0.33%.
The diff coverage is 74.36%.

❗ Current head 3d2aa62 differs from pull request most recent head f7246af. Consider uploading reports for the commit f7246af to get more accurate results

@@            Coverage Diff             @@
##           master    #5319      +/-   ##
==========================================
- Coverage   66.97%   66.64%   -0.33%     
==========================================
  Files         524      524              
  Lines       55370    55776     +406     
==========================================
+ Hits        37082    37173      +91     
- Misses      13771    14017     +246     
- Partials     4517     4586      +69     
Impacted Files Coverage Δ
go/consensus/api/api.go 38.88% <ø> (ø)
go/consensus/cometbft/abci/state.go 76.94% <ø> (ø)
go/consensus/cometbft/common/common.go 53.33% <ø> (ø)
go/keymanager/api/policy_sgx.go 46.66% <ø> (+13.33%) ⬆️
go/registry/api/api.go 55.93% <ø> (+0.15%) ⬆️
go/runtime/host/mock/mock.go 87.40% <0.00%> (-1.40%) ⬇️
go/runtime/host/protocol/types.go 54.54% <ø> (ø)
go/runtime/host/sgx/sgx.go 68.75% <0.00%> (-0.62%) ⬇️
go/worker/keymanager/api/api.go 0.00% <ø> (-32.44%) ⬇️
go/worker/keymanager/metrics.go 100.00% <ø> (ø)
... and 22 more

... and 71 files with indirect coverage changes

@abukosek abukosek marked this pull request as ready for review July 11, 2023 15:45
@@ -1219,6 +1219,11 @@ func (s *MutableState) AddRewards(
continue
}

if q.Cmp(commonPool) == 1 {
Copy link
Contributor

@peternose peternose Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this condition is true (q > commonPool), should we lower the reward and give as much as we can (q = commmonPool.Clone())?

// If an error occurs, the pool and affected accounts are left in an invalid state.
// This may fail due to the common pool running out of stake.

The method's comment is not correct anymore, as the pool will never run out of stake.

This method is not fair. For example:

  • If I have an unlimited escrow balance, my rewards will always be too big and therefore skipped while others will receive their awards until they drain the pool.
  • Assuming equal balances and too small pool to cover all rewards, the first in the list will get more rewards than the last one.

However, I'm not sure if this is a problem. If we want fairness, we should distribute the reminder of the pool proportional to stakes. But that is probably too much.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should discuss what to do about this and implement it in a separate PR. This PR just fixes the panic that would have happened otherwise :)

@abukosek abukosek force-pushed the andrej/bugfix/rewards-dont-panic branch from 3d2aa62 to f7246af Compare July 12, 2023 07:43
@abukosek abukosek enabled auto-merge July 12, 2023 08:37
@abukosek abukosek merged commit d81131e into master Jul 12, 2023
3 checks passed
@abukosek abukosek deleted the andrej/bugfix/rewards-dont-panic branch July 12, 2023 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants