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

Run mint.BeginBlocker before distr.BeginBlocker #2967

Closed
cwgoes opened this Issue Nov 30, 2018 · 9 comments

Comments

Projects
None yet
5 participants
@cwgoes
Copy link
Contributor

cwgoes commented Nov 30, 2018

Presently we run distr.BeginBlocker before mint.BeginBlocker - this means that if a large-stake validator unbonds, inflationary rewards proportional to their stake will be paid at the start of the next block (since they're just added to the collected fees in x/mint, not yet distributed). Can we swap the ordering so said validator will be able to withdraw the fees (which they deserve, since they signed the block)?

Any problems with doing this (other than needing to change a few invariants) @rigelrozanski?

@rigelrozanski

This comment has been minimized.

Copy link
Member

rigelrozanski commented Nov 30, 2018

nope that sounds fine, I don't foresee any issues

@Slamper

This comment has been minimized.

Copy link
Contributor

Slamper commented Nov 30, 2018

I'm not sure whether this will completely fix the problem since in x/mint we are inflating and then calculating the inflation amount for the next block.

mintedCoin := minter.BlockProvision(params)
k.fck.AddCollectedFees(ctx, sdk.Coins{mintedCoin})
k.sk.InflateSupply(ctx, sdk.NewDecFromInt(mintedCoin.Amount))
if blockTime.Sub(minter.LastUpdate) < time.Hour {
return
}
// adjust the inflation, hourly-provision rate every hour
totalSupply := k.sk.TotalPower(ctx)
bondedRatio := k.sk.BondedRatio(ctx)
minter.Inflation = minter.NextInflationRate(params, bondedRatio)
minter.AnnualProvisions = minter.NextAnnualProvisions(params, totalSupply)
minter.LastUpdate = blockTime
k.SetMinter(ctx, minter)

So with both of these things combined we could currently have a 2 block lag of inflation. Even though I'm wondering because in the block after the unbonding the calculation/TotalPower should be correct.

@Slamper

This comment has been minimized.

Copy link
Contributor

Slamper commented Nov 30, 2018

image

This is a guess of how this went in Gaia-9002.

After the genesis validators were unbonded the inflation was still lagging 2 blocks.

In the first of these we autobonded and consequently were the ones with the by far highest weight. So we accumulated almost 100% of the Accum in the FeePool in the 2nd block and could get almost all of the inflation which was still extremely large because the inflation module was "lagging behind".

That way not only inflation was extremely high for 2 rounds but we were also able to claim almost 100% of the rewards in one of the blocks.

@kauchy

This comment has been minimized.

Copy link

kauchy commented Dec 1, 2018

even mint.BeginBlocker before distr.BeginBlocker,this situation will still happen,because Inflation/AnnualProvisions calculate every 1 hour.Only if a large-stake validator unbonds at Inflation/AnnualProvisions calculate block

@Slamper

This comment has been minimized.

Copy link
Contributor

Slamper commented Dec 1, 2018

With all these things in mind.

I don't think this change is plausible as recalculation of the AnnualProvisions after a large bonded stake change would open a potential DoS vector.

I can only imagine that calculating the inflation rate rather than the amount every hour would solve this even though that increases the effective inflation.

@cwgoes

This comment has been minimized.

Copy link
Contributor

cwgoes commented Dec 1, 2018

even mint.BeginBlocker before distr.BeginBlocker,this situation will still happen,because Inflation/AnnualProvisions calculate every 1 hour.Only if a large-stake validator unbonds at Inflation/AnnualProvisions calculate block

Yes, you are quite correct. In normal situations (no single massive validator unbonding), this doesn't seem too problematic, but in this case it definitely is.

I don't think this change is plausible as recalculation of the AnnualProvisions after a large bonded stake change would open a potential DoS vector.

I don't think this matters, calculating the annual provisions is not very expensive at all (a few multiplications) compared to lots of other things we do every block (tracking signing info, for example).

I suggest we make this BeginBlocker change and additionally recalculate inflation each block.

Thoughts @rigelrozanski?

@dlguddus

This comment has been minimized.

Copy link

dlguddus commented Dec 3, 2018

I want to point out about the global pool distribution logic behind the scene.

  1. The fee pool has 100 stake and every validator's DelAccum is 100(suppose there are 4 validators)
  2. If node0 withdraws, he gets 100 * 100/400 = 25
    ( If the distribution is fair, node0's distribution on 100 stake is DONE. There should be no more rewards to node0 from current pool )
  3. Time passes, 4 validator's DelAccum increased by 100 each
    ( Pool stays at 75 stake )
  4. node0 withdraws again, he gets 75 * 100/700 = 10.7
  5. node1 withdraws, he gets 64.3 * 200/600 = 21.4
  6. node2 withdraws, he gets 42.9 * 200/400 = 21.4
  7. node3 withdraws, he gets 21.4 * 200/200 = 21.4

Therefore, total rewards for each node is 35.7/21.4/21.4/21.4

Considering the same DelAccum at step 1, the distribution result is somewhat different.
I suspect this logic impacted on gaia-9002. If node0 delegate his withdrawal amount(25) right after step 2, he would get more total reward than 35.7, result in larger distribution difference. And if the reward pool is very huge, the difference will explode since node0's power will increase with huge magnitude.

Is it a reasonable thing to happen in distribution logic? This logic makes all the validators to withdraw more frequently, result in heavy tx and network costs.

  • OPINION
    For the design perspective, I think the distribution design should be fixed. The distribution should not be affected by the DelAccum of future times, which is later of the reward happened time.

This logic will finally lead to an equillibrium that all the validators withdraw their rewards when their share is greater than 1 stake. It will be eventually a very competitively withdrawing network.

@cwgoes cwgoes referenced this issue Dec 3, 2018

Closed

Game of Stakes release blockers #2981

3 of 4 tasks complete
@cwgoes

This comment has been minimized.

Copy link
Contributor

cwgoes commented Dec 3, 2018

This logic will finally lead to an equillibrium that all the validators withdraw their rewards when their share is greater than 1 stake. It will be eventually a very competitively withdrawing network.

I also think this is likely - can we discuss on #2764 instead?

@cwgoes cwgoes referenced this issue Dec 3, 2018

Merged

R4R: Inflation bug fixes #2982

5 of 5 tasks complete
@dlguddus

This comment has been minimized.

Copy link

dlguddus commented Dec 3, 2018

ok. thanks for the link! I can see your reply with similar argument already there.

@cwgoes cwgoes changed the title Run mint.BeginBlocker before distr.BeginBlocker R4R: Run mint.BeginBlocker before distr.BeginBlocker Dec 3, 2018

@cwgoes cwgoes changed the title R4R: Run mint.BeginBlocker before distr.BeginBlocker Run mint.BeginBlocker before distr.BeginBlocker Dec 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment