R4R: Piggy-bank distribution spec

[rigelrozanski]

Oink Oink!

🐷

I would recommend reviewing this in your editor as I've moved the old spec into a WIP subdir and it's confusing the github PR diff explorer

Related #1671

• Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
• Updated all relevant documentation (docs/)
• Updated all relevant code comments
• Wrote tests
• Added entries in PENDING.md that include links to the relevant issue or PR that most accurately describes the change.
• Updated cmd/gaia and examples/

---

For Admin Use:

• Added appropriate labels to PR (ex. wip, ready-for-review, docs)
• Reviewers Assigned
• Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

[codecov]

Codecov Report

Merging #1944 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #1944   +/-   ##
========================================
  Coverage    63.85%   63.85%           
========================================
  Files          134      134           
  Lines         8175     8175           
========================================
  Hits          5220     5220           
  Misses        2605     2605           
  Partials       350      350

[rigelrozanski]

I would recommend reviewing this in your editor as I've moved the old spec into a WIP subdir and it's confusing the github PR diff explorer

[jaekwon]

I think GetDelegatorAllWithdraws has too many iterations.. a single for-loop overall delegations already sort of expensive… looping over all val updates in a second for-loop seems dangerous.

Instead, the validator could be charging commission right away as it earns continuously. We can require a TX for a validator change its commission rate, and each time we can charge all the commissions from the last time we updated the validator info until now. This would remove the need for the inner loop in GetDelegatorAllWithdraws.

Also, I wonder if we could also support a partial delegator withdraw in case of any max gas issues.

[rigelrozanski]

looping over all val updates in a second for-loop seems dangerous.
Instead, the validator could be charging commission right away as it earns continuously. We can require a TX for a validator change its commission rate, and each time we can charge all the commissions from the last time we updated the validator info until now. This would remove the need for the inner loop in GetDelegatorAllWithdraws.

note, this is only for validator set commission updates here - which we can easily rate limit (1/day seems fine) - however I think you're right that we can and should avoid this loop by taking commission immediately! - just need to think if anything else special needs to happen - but seems to make sense..... thanks!

Also, I wonder if we could also support a partial delegator withdraw in case of any max gas issues.

There are two ways to interpret this:

1. partial withdrawal means withdrawing from only some of the validators which a delegator is delegated too - implementing this is a slice of cake - everything is already setup to allow for this.
2. partial withdrawal means withdrawing some but not all of the fees from a single validator -
   I think it may be possible in the piggy bank version, but I think it will really weird, breaks a bunch of the simplicity which is why we're going with this model - I'd like to avoid implementing this is possible - I suspect this won't be necessary if we can avoid the val-loop

[cwgoes]

Would it be possible for us to characterize the exact difference between what the rewards ought to be if we calculated the dumb iteration way versus in this approximate manner - both in the spec and in some testcases?

I think understanding the approximation error would be helpful in deciding when (possibly if ever) we would need to upgrade to the more complex model, whether any strange incentives are introduced, etc.

[rigelrozanski]

@cwgoes totally agree - documentation of where the error comes from will be useful, and I'll include a blurb about it - from a high level the reason error is introduced is because the accum reflects an average of what's in the pool - so if you happen to know that fees are about to go way down or way up, you could withdraw or not withdraw to your advantage compared to everyone else. With random actors this model should approach the same distribution of fees as "dumb-slow" distribution - but with scheming actors, some may be able to cheat the system a tiny bit

[rigelrozanski]

@jaekwon - I've updated the spec:

• there is now actively calculate commission - there is no longer record of commission rate changes required, commission is taken as rewards enter the validator
• added a special tx for delegators to withdraw from only a single validator - should address possible max gas issues

[rigelrozanski]

@cwgoes I've added a section called "shortcomings" into overview.md I think this sufficiently describes the shortcomings of this system

[cwgoes]

Thanks @rigelrozanski. I think having rewards not be bonded is quite helpful here, as it somewhat counteracts the incentive not to withdraw in hope of more fees.

[jaekwon]

just an FYI that these receivers should be pointer receivers since they mutate self...

[rigelrozanski]

yeah - to stick with the design pattern used elsewhere in the SDK, rather than using pointers, to address this comment, I've added returns for the modified object for each of these functions

[ValarDragon]

Are we strongly opposed to pointers? Pointers are definitely more efficient, and this is cost that we can't really charge through gas. Additionally the decimals are pointers internally, so were not getting much additional safety by not using a pointer here. (e.g. pointers to the same dec are copied regardless)

[ValarDragon]

From my estimates, copying the struct means copying about 150 bytes, so I guess its not that much of a performance saving. (300 bytes total if you include the return value) I still would prefer pointers though.

[rigelrozanski]

If there are considerably-large efficiency gains which will affect performance then it seems like a good idea. However it doesn't sound that way from your final comment. In terms of code organization I AM strongly for non-pointers specifically because they enforce a code pattern which makes it very obvious where things are being written. In the past there has been considerable development time wasted just deciphering what certain spaghetti code is doing because object writes are hidden several tiers down with no clear way to determine where things are being written. For this reason - even a moderate-small efficiency boost doesn't justify using pointers throughout as it will make the code more prone to bugs. - I think there may be a situation in the future where we can use some special forms of linting or other tooling to aid code comprehension - after this is integrated it may be an appropriate time to switch to more pointer based code. THIS SAID under specific situations in the SDK pointers should of course still be used - we just need to make sure that we're hella explicit to make write locations obvious in the caller functions (and nested caller functions)

[ValarDragon]

Sounds good to me, thank you for the explanation!

[rigelrozanski]

@jaekwon mentioned "basically looks OK to me to merge" - Jae's comments now addressed - Let's merge this unless there are new comments (CC @cwgoes)

[cwgoes]

utACK (spec).

Maybe in simulation or just by hand we can calculate numerically different scenarios of how this will differ from the "ideal" fee distribution.