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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Jul 12, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .changelog/5319.bugfix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
staking: Fix reward distribution when common pool is exhausted
17 changes: 12 additions & 5 deletions go/consensus/cometbft/apps/staking/state/state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1164,11 +1164,8 @@ func (s *MutableState) DiscardGovernanceDeposit(
}

// AddRewards computes and transfers a staking reward to active escrow accounts.
// 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. In this case, the
// returned error's cause will be `staking.ErrInsufficientBalance`, and it should
// be safe for the caller to roll back to an earlier state tree and continue from
// there.
// If the common pool runs out, the rewards that are too big to be paid out are
// skipped and no error is returned.
func (s *MutableState) AddRewards(
ctx *abciAPI.Context,
time beacon.EpochTime,
Expand Down Expand Up @@ -1219,6 +1216,11 @@ func (s *MutableState) AddRewards(
continue
}

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

@peternose peternose Jul 11, 2023 •
edited

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 :)

// Not enough balance left in common pool, skip.
continue
}

rate := ent.Escrow.CommissionSchedule.CurrentRate(time)
var com *quantity.Quantity
com, q, err = s.computeCommission(ctx, rate, q)
Expand Down Expand Up @@ -1349,6 +1351,11 @@ func (s *MutableState) AddRewardSingleAttenuated(
return nil
}

if q.Cmp(commonPool) == 1 {
// Not enough balance left in common pool, skip.
return nil
}

rate := acct.Escrow.CommissionSchedule.CurrentRate(time)
com, q, err := s.computeCommission(ctx, rate, q)
if err != nil {
Expand Down
113 changes: 113 additions & 0 deletions go/consensus/cometbft/apps/staking/state/state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,119 @@ func TestDebondingDelegation(t *testing.T) {
require.EqualValues(expectedDds, dds, "expected debonding delegations should exist")
}

func TestRewardsWithoutEnoughInCommonPool(t *testing.T) {
require := require.New(t)

delegatorSigner, err := memorySigner.NewSigner(rand.Reader)
require.NoError(err, "generating delegator signer")
delegatorAddr := staking.NewAddress(delegatorSigner.Public())
delegatorAccount := &staking.Account{}
delegatorAccount.General.Nonce = 10
err = delegatorAccount.General.Balance.FromBigInt(big.NewInt(300))
require.NoError(err, "initialize delegator account general balance")

escrowSigner, err := memorySigner.NewSigner(rand.Reader)
require.NoError(err, "generating escrow signer")
escrowAddr := staking.NewAddress(escrowSigner.Public())
escrowAddrAsList := []staking.Address{escrowAddr}
escrowAccount := &staking.Account{}
escrowAccount.Escrow.CommissionSchedule = staking.CommissionSchedule{
Rates: []staking.CommissionRateStep{
{
Start: 0,
Rate: mustInitQuantity(t, 20_000), // 20%
},
},
Bounds: []staking.CommissionRateBoundStep{
{
Start: 0,
RateMin: mustInitQuantity(t, 0),
RateMax: mustInitQuantity(t, 100_000),
},
},
}
err = escrowAccount.Escrow.CommissionSchedule.PruneAndValidate(
&staking.CommissionScheduleRules{
RateChangeInterval: 10,
RateBoundLead: 30,
MaxRateSteps: 4,
MaxBoundSteps: 12,
}, 0)
require.NoError(err, "prune and validate commission schedule")

del := &staking.Delegation{}
_, err = escrowAccount.Escrow.Active.Deposit(&del.Shares, &delegatorAccount.General.Balance, mustInitQuantityP(t, 100))
require.NoError(err, "active escrow deposit")

var deb staking.DebondingDelegation
deb.DebondEndTime = 21
_, err = escrowAccount.Escrow.Debonding.Deposit(&deb.Shares, &delegatorAccount.General.Balance, mustInitQuantityP(t, 100))
require.NoError(err, "debonding escrow deposit")

appState := abciAPI.NewMockApplicationState(&abciAPI.MockApplicationStateConfig{})
ctx := appState.NewContext(abciAPI.ContextEndBlock)
defer ctx.Close()

s := NewMutableState(ctx.State())

err = s.SetConsensusParameters(ctx, &staking.ConsensusParameters{
DebondingInterval: 21,
RewardSchedule: []staking.RewardStep{
{
Until: 30,
Scale: mustInitQuantity(t, 1000),
},
{
Until: 40,
Scale: mustInitQuantity(t, 500),
},
},
CommissionScheduleRules: staking.CommissionScheduleRules{
RateChangeInterval: 10,
RateBoundLead: 30,
MaxRateSteps: 4,
MaxBoundSteps: 12,
},
})
require.NoError(err, "SetConsensusParameters")
// Init common pool with insufficient balance on purpose.
err = s.SetCommonPool(ctx, mustInitQuantityP(t, 300))
require.NoError(err, "SetCommonPool")

err = s.SetAccount(ctx, delegatorAddr, delegatorAccount)
require.NoError(err, "SetAccount")
err = s.SetAccount(ctx, escrowAddr, escrowAccount)
require.NoError(err, "SetAccount")
err = s.SetDelegation(ctx, delegatorAddr, escrowAddr, del)
require.NoError(err, "SetDelegation")
err = s.SetDebondingDelegation(ctx, delegatorAddr, escrowAddr, 1, &deb)
require.NoError(err, "SetDebondingDelegation")

require.NoError(s.AddRewards(ctx, 10, mustInitQuantityP(t, 100_000), escrowAddrAsList), "add rewards 1")
evs := ctx.GetEvents()
require.Len(evs, 3, "adding rewards should emit 3 events")

require.NoError(s.AddRewards(ctx, 30, mustInitQuantityP(t, 100_000), escrowAddrAsList), "add rewards 2")
evs = ctx.GetEvents()
require.Len(evs, 6, "adding rewards should emit 3 new events")

require.NoError(s.AddRewards(ctx, 35, mustInitQuantityP(t, 100_000), escrowAddrAsList), "add rewards 3")
evs = ctx.GetEvents()
require.Len(evs, 6, "adding rewards with not enough in common pool should not emit any new events")

require.NoError(s.AddRewards(ctx, 38, mustInitQuantityP(t, 80_000), escrowAddrAsList), "add rewards 4")
evs = ctx.GetEvents()
require.Len(evs, 6, "adding rewards with not enough in common pool should not emit any new events")

require.NoError(s.AddRewardSingleAttenuated(ctx, 10, mustInitQuantityP(t, 10_000), 5, 10, escrowAddr), "add rewards attenuated 1")
evs = ctx.GetEvents()
require.Len(evs, 9, "adding attenuated rewards should emit 3 new events")

require.NoError(s.AddRewardSingleAttenuated(ctx, 10, mustInitQuantityP(t, 100_000), 5, 10, escrowAddr), "add rewards attenuated 2")
evs = ctx.GetEvents()
require.Len(evs, 9, "adding attenuated rewards with not enough in common pool should not emit any new events")
}

func TestRewardAndSlash(t *testing.T) {
require := require.New(t)

Expand Down