Mutli-Block Finalization #2155

dorothy-zbornak · 2019-09-13T23:51:01Z

Description

At long last, multi-block finalization is here!

Overview

Multi-Block finalization provides an asynchronous method for finalizing pool rewards that can be done over the span of multiple transactions, without halting the next epoch.

Every time a protocol fee is paid, the pool associated with it is credited, tracked, and emitted in an event.
When a keeper calls endEpoch(), the epoch will immediately advance and finalization begins.
- During this time, the next epoch continues to accrue fees, as normal.
Keepers gather up all the pool IDs that were emitted during the previous epoch and pass them into finalizePools()-- potentially over several transactions.
- Each time a pool is finalized, the rewards owed to that pool are paid to pool members (as a group) and to operators (individually).
- If a delegator touches their stake in a pool that has yet to be finalized, it will be finalized on the spot.

Notes

An epoch cannot be ended until the previous epoch has completed finalization.
In contrast to the original, synchronous finalization, epoch rewards are now paid in the context of the new epoch, not the epoch in which rewards were earned in. This turned out to be easier said than done and required significant adjustments to the stake accounting:
- Prior, we maintained "cumulative reward" balances for currentEpoch and currentEpoch-1. Now we maintain them for currentEpoch and currentEpoch+1 (I know, it's weird).
- The behavior of reference counting had to be altered to properly accommodate this, and you'll witness this in their totally different tests.
- We have to take care to factor in unfinalized rewards as well as rewards from previous epochs when computing a delegator's balance.
There is a new mixin, MixinFinalizer, which contains the bulk of the finalization logic. Notable functions include:
- endEpoch(), which anyone can call to end an epoch and begin the finalization process.
- finalizePools(), which finalizes multiple pools at once.
- _finalizePool(), an internal function that finalizes a single pool. Called by MixinStakingPoolRewards when syncing stake.
- _getUnfinalizedPoolRewards(), an internal function that calculates the unfinalized rewards owed to a pool. Also called by MixinStakingPoolRewards in order to return accurate balances while pools are still being finalized.
MixinExchangeFees has been pretty heavily modified to integrate the tracking we need.
This PR comes with fresh unit tests for delegator/operator rewards, finalization, and protocol fees, ~~but I'm still missing ones for the new logic added to payProtocolFee(). Any takers?~~
A notable change was made to EthVault and StakingPoolRewardVault to support efficient batching of finalization:
- We're back to the recordDepositFor() pattern and just bulk transferring funds via their fallbacks.
The FinalizerActor has been updated to accurately and more robustly compute finalization balances.
There is now a minimum cap on stake for a pool to be considered active. It was a whopping 1 wei. I think this makes more sense than edge casing pools with zero stake.
The cobbDouglas() function is now in its own library named, surprise, LibCobbDouglas.
I added some dynamic scaling of values to LibFractions to fix precision issues I was running into.
I clip cobb douglas results to the remaining reward balance in _getUnfinalizedPoolRewards() of MixinFinalizer.
My tests are ugly. Brace yourselves.

Testing instructions

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

Prefix PR title with [WIP] if necessary.
Add tests to cover changes as needed.
Update documentation as needed.
Add new entries to the relevant CHANGELOG.jsons.

coveralls · 2019-09-17T18:53:23Z

Coverage remained the same at 75.804% when pulling 3883297 on feat/staking/hurts-like-a-mbf into 29f4d69 on 3.0.

dorothy-zbornak · 2019-09-19T11:20:18Z

contracts/test-utils/src/lang_utils.ts

+/**
+ * _.zip() that clips to the shortest array.
+ */
+export function shortZip<T1, T2>(a: T1[], b: T2[]): Array<[T1, T2]> {


This is better than _.zip() because the return type isn't some weird Array<[T1 | undefined, T2 | undefined]>, which makes _.zip() pretty inconvenient to use in TS.

abandeali1

I'm not finished with my review but wanted to get something in ASAP. I will continue later today!

abandeali1 · 2019-09-19T16:21:22Z

contracts/staking/contracts/src/Staking.sol

+    MixinStakeStorage,
+    MixinStakingPoolMakers,
+    MixinStakeBalances,
+    MixinCumulativeRewards,


I see you got a hold of Greg's linearization script

I suppose we can give imports another pass after all big features are merged.

Come to the dark side, Amir. These aren't the imports you're looking for.

contracts/staking/contracts/src/ReadOnlyProxy.sol

contracts/staking/contracts/src/fees/MixinExchangeFees.sol

contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol

contracts/staking/contracts/src/sys/MixinParams.sol

contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol

moodlezoup

Awesome job overall! Not done reading the tests, but here are some comments in the meantime

contracts/staking/contracts/src/interfaces/IStakingPoolRewardVault.sol

contracts/staking/test/unit_tests/lib_proxy_unit_test.ts

contracts/staking/contracts/src/sys/MixinAbstract.sol

contracts/staking/test/utils/api_wrapper.ts

contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol

contracts/staking/test/rewards_test.ts

contracts/staking/test/unit_tests/delegator_reward_test.ts

hysz

I'm partway through my review. I think the overall approach looks good, and I like that there's relatively little logic outside of finalization had to change +1.

contracts/staking/contracts/src/fees/MixinExchangeFees.sol

hysz · 2019-09-19T19:34:30Z

contracts/staking/contracts/src/fees/MixinExchangeFees.sol

-        internal
+    /// @dev Returns the total balance of this contract, including WETH.
+    /// @return totalBalance Total balance.
+    function getTotalBalance()


Could we add that this is the fee balance? This will add context to the greater staking API.

~~As in, add it to totalFeesCollected?~~ idk, aren't they two different things? The contract balance could include subsidies, remaining rewards, etc.

Edit: misread

Yeah I guess they are. Basically, just a name to disambiguate between this balance and the stake balances. Maybe getTotalPayableBalance or something?

I could just remove this function. All it's doing is suming ETH and WETH balance, and we could use the space.

contracts/staking/contracts/src/fees/MixinExchangeFees.sol

contracts/staking/contracts/src/immutable/MixinStorage.sol

hysz · 2019-09-19T20:31:44Z

contracts/staking/contracts/src/sys/MixinAbstract.sol

+
+/// @dev Exposes some internal functions from various contracts to avoid
+///      cyclical dependencies.
+contract MixinAbstract {


I guess I don't have full context, but I feel like this could be solved more cleanly by reorganizing the mixins.

idk how to fix the cyclical deps without introducing something like this or an interface, and we don't use the interface convention for mixins in this package so I went with this. 🤔

Kk I'll look into it a bit more during the final pass. If not then could be good to make this an interface since it's declarations only.

contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol

contracts/staking/contracts/src/sys/MixinFinalizer.sol

contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol

abandeali1

Added a review for the remaining Solidity!

contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol

contracts/staking/contracts/src/sys/MixinFinalizer.sol

contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol

contracts/staking/test/utils/api_wrapper.ts

xianny

I only looked over changes to base-contract and order-utils. Those look good to me :)

contracts/staking/test/rewards_test.ts

contracts/staking/test/unit_tests/delegator_reward_test.ts

…ult to `_rewards_test`.

…s in `getTopicsForIndexedArgs()`.

…tests.

… our own.

…actorings. `@0x/contracts-staking`: Add finalization-related protocol fees unit tests.

`@0x/contracts-staking`: Convert all rewards to WETH. `@0x/contracts-staking`: Style changes. `@0x/contracts-staking`: Address misc. review comments. `@0x/contracts-staking`: Make `LibFractions` scaling a separate step.

…able.

abandeali1

This looks good to go. I did not leave any comments on any code related to the EthVault or StakingPoolRewardsVault because of #2186 . Please do not make any further changes to these contracts, since they will be removed.

jalextowle

Nice work on this one! I had a couple of nits, but it looks good to go.

contracts/staking/contracts/src/fees/MixinExchangeFees.sol

contracts/utils/contracts/src/LibFractions.sol

contracts/staking/contracts/src/vaults/EthVault.sol

contracts/staking/test/unit_tests/finalizer_test.ts

`@0x/contracts-utils`: Use `safeDiv()` in `LibFractions.normalize()`.

hysz

This is looking really solid! I left four comments; three of which we can address in the final pass. Although one regarding reward intervals should be addressed before we merge.

hysz · 2019-09-23T16:38:50Z

contracts/staking/contracts/src/fees/MixinExchangeFees.sol

+
+            // Emit an event so keepers know what pools to pass into
+            // `finalize()`.
+            emit StakingPoolActivated(currentEpoch_, poolId);


I feel like there's more information we may want to communicate in this function. What would you think of emitting a single event that communicates: the epoch, pool id, whether payments was in ETH or WETH, and whether or not the fee was attributed to the pool?

hysz · 2019-09-23T16:41:26Z

contracts/staking/contracts/src/fees/MixinExchangeFees.sol

-        internal
+    /// @dev Returns the total balance of this contract, including WETH.
+    /// @return totalBalance Total balance.
+    function getTotalBalance()


Yeah I guess they are. Basically, just a name to disambiguate between this balance and the stake balances. Maybe getTotalPayableBalance or something?

hysz · 2019-09-23T16:53:37Z

contracts/staking/contracts/src/sys/MixinAbstract.sol

+
+/// @dev Exposes some internal functions from various contracts to avoid
+///      cyclical dependencies.
+contract MixinAbstract {


Kk I'll look into it a bit more during the final pass. If not then could be good to make this an interface since it's declarations only.

hysz · 2019-09-23T17:48:45Z

contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol

+        if (unsyncedStake.currentEpoch >= lastRewardEpoch) {
+            return reward;
+        }
+        // From here we know: `unsyncedStake.currentEpoch < currentEpoch > 0`.


It looks like the intervals have been shifted. Is this on purpose?

# Previously the ranges were: [unsyncedStake.currentEpoch-1 .. unsyncedStake.currentEpoch] [unsyncedStake.currentEpoch .. lastRewardEpoch] # Now the ranges are: [unsyncedStake.currentEpoch .. unsyncedStake.currentEpoch + 1] [unsyncedStake.currentEpoch + 1 .. lastRewardEpoch + 1]

Yup. This is from crediting rewards at the top of an epoch rather than at the end, and folding in unfinalized rewards.

moodlezoup

LGTM modulo nits! 🚀 🚀 🚀

contracts/staking/contracts/src/libs/LibCobbDouglas.sol

contracts/utils/contracts/src/LibFractions.sol

contracts/staking/test/utils/types.ts

contracts/staking/test/utils/cumulative_reward_tracking_simulation.ts

contracts/staking/contracts/src/StakingProxy.sol

contracts/staking/contracts/src/vaults/EthVault.sol

`@0x/contracts-staking`: Fix linter errors.

contracts/staking/contracts/src/fees/MixinExchangeFees.sol

dorothy-zbornak force-pushed the feat/staking/hurts-like-a-mbf branch from 2e0af0f to eb9b9b3 Compare September 18, 2019 16:23

dorothy-zbornak commented Sep 19, 2019

View reviewed changes

dorothy-zbornak changed the title ~~[WIP] Mutli-Block Finalization~~ Mutli-Block Finalization Sep 19, 2019

dorothy-zbornak assigned hysz Sep 19, 2019

dorothy-zbornak marked this pull request as ready for review September 19, 2019 11:35

dorothy-zbornak requested review from abandeali1, fabioberger, hysz and xianny as code owners September 19, 2019 11:35

abandeali1 suggested changes Sep 19, 2019

View reviewed changes

moodlezoup suggested changes Sep 19, 2019

View reviewed changes

hysz suggested changes Sep 19, 2019

View reviewed changes

abandeali1 suggested changes Sep 20, 2019

View reviewed changes

dorothy-zbornak commented Sep 20, 2019

View reviewed changes

contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol Outdated Show resolved Hide resolved

dorothy-zbornak commented Sep 20, 2019

View reviewed changes

contracts/staking/test/utils/api_wrapper.ts Outdated Show resolved Hide resolved

xianny approved these changes Sep 20, 2019

View reviewed changes

moodlezoup reviewed Sep 21, 2019

View reviewed changes

dorothy-zbornak force-pushed the feat/staking/hurts-like-a-mbf branch 2 times, most recently from 5be0eb0 to dde306a Compare September 22, 2019 03:51

merklejerk added 10 commits September 22, 2019 12:05

@0x/contracts-staking: Starting MBF.

bbae6b3

@0x/contracts-staking: More work on MixinFinalizer.

73f1aca

@0x/contracts-staking: Tinkering with crediting rewards.

e4b9d14

@0x/contracts-staking: Add some tests to check rewards in the EthVa…

9c47d22

…ult to `_rewards_test`.

@0x/contracts-staking: Add another delegator payout test.

94909f1

@0x/contracts-staking: Fully implement MBF (I hope).

b57c0a2

@0x/contracts-staking: Got the solidity compiling.

294be37

@0x/contracts-staking: Fix solidity linter errors.

712b256

@0x/contracts-staking: Working on MBF v2

06b4d24

@0x/contracts-staking: Almost compiling...

102ca6b

merklejerk and others added 18 commits September 22, 2019 12:11

@0x/contracts-staking: Fixing tests.

993f05d

@0x/contracts-test-utils: Add shortZip().

b4b6d4d

@0x/contracts-staking: Finalizer tests passing.

d33080c

@0x/base-contract: Properly encode BigNumber indexed filter value…

54ac1c2

…s in `getTopicsForIndexedArgs()`.

@0x/contracts-staking: All tests but rewards_test working.

b43fa88

@0x/base-contract: Fix linter errors.

d5bbbe8

@0x/contracts-staking: Gag! All tests passing?

8610671

@0x/contracts-test-utils: Add hexHash() to hex_utils.

0270777

@0x/contracts-staking: Add some computeRewardBalanceOfOperator() …

d064543

…tests.

@0x/contracts-test-utils: Ran prettier.

2bb9b9a

@0x/contracts-staking: Import randomAddress() instead of defining…

2eff213

… our own.

@0x/contracts-staking: Add some extra finalizer tests and light ref…

14c4491

…actorings. `@0x/contracts-staking`: Add finalization-related protocol fees unit tests.

@0x/contracts-staking: Remove IStructs.CumulativeRewardInfo, etc.

3ad7728

`@0x/contracts-staking`: Convert all rewards to WETH. `@0x/contracts-staking`: Style changes. `@0x/contracts-staking`: Address misc. review comments. `@0x/contracts-staking`: Make `LibFractions` scaling a separate step.

@0x/contracts-staking: Updating tests and making the contracts test…

6a29654

…able.

@0x/contracts-staking: All tests back up and running.

c72a15b

@0x/dev-utils: Increase gas limit to 9e6

aa4ee2c

@0x/order-utils: Fix staking error codes after rebase.

e412618

@0x/contracts-staking: Rebase against 3.0

eac4520

dorothy-zbornak force-pushed the feat/staking/hurts-like-a-mbf branch from b180d75 to eac4520 Compare September 22, 2019 17:11

@0x/contracts-staking: Update compiler version in truffle config.

6d20f0e

abandeali1 approved these changes Sep 23, 2019

View reviewed changes

jalextowle approved these changes Sep 23, 2019

View reviewed changes

dorothy-zbornak added 2 commits September 23, 2019 11:55

@0x/contract-staking: Address review nits.

9dd8c61

`@0x/contracts-utils`: Use `safeDiv()` in `LibFractions.normalize()`.

@0x/contract-staking: Make solidity uniformly hideous ;-).

196cc43

hysz reviewed Sep 23, 2019

View reviewed changes

moodlezoup approved these changes Sep 23, 2019

View reviewed changes

@0x/contracts-staking: Remove getTotalBalance() function.

3883297

`@0x/contracts-staking`: Fix linter errors.

hysz reviewed Sep 23, 2019

View reviewed changes

contracts/staking/contracts/src/fees/MixinExchangeFees.sol Show resolved Hide resolved

hysz approved these changes Sep 23, 2019

View reviewed changes

dorothy-zbornak merged commit 8ddcf88 into 3.0 Sep 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mutli-Block Finalization #2155

Mutli-Block Finalization #2155

dorothy-zbornak commented Sep 13, 2019 •

edited

Loading

coveralls commented Sep 17, 2019 •

edited

Loading

dorothy-zbornak Sep 19, 2019

abandeali1 left a comment

abandeali1 Sep 19, 2019

abandeali1 Sep 19, 2019

hysz Sep 19, 2019

moodlezoup left a comment

hysz left a comment

hysz Sep 19, 2019

dorothy-zbornak Sep 20, 2019 •

edited

Loading

hysz Sep 23, 2019

dorothy-zbornak Sep 23, 2019

hysz Sep 19, 2019

dorothy-zbornak Sep 20, 2019

hysz Sep 23, 2019

abandeali1 left a comment

xianny left a comment

abandeali1 left a comment

jalextowle left a comment

hysz left a comment

hysz Sep 23, 2019

hysz Sep 23, 2019

hysz Sep 23, 2019

hysz Sep 23, 2019

dorothy-zbornak Sep 23, 2019

moodlezoup left a comment

Mutli-Block Finalization #2155

Mutli-Block Finalization #2155

Conversation

dorothy-zbornak commented Sep 13, 2019 • edited Loading

Description

Overview

Notes

Further Reading

Testing instructions

Types of changes

Checklist:

coveralls commented Sep 17, 2019 • edited Loading

Choose a reason for hiding this comment

abandeali1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moodlezoup left a comment

Choose a reason for hiding this comment

hysz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorothy-zbornak Sep 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abandeali1 left a comment

Choose a reason for hiding this comment

xianny left a comment

Choose a reason for hiding this comment

abandeali1 left a comment

Choose a reason for hiding this comment

jalextowle left a comment

Choose a reason for hiding this comment

hysz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moodlezoup left a comment

Choose a reason for hiding this comment

dorothy-zbornak commented Sep 13, 2019 •

edited

Loading

coveralls commented Sep 17, 2019 •

edited

Loading

dorothy-zbornak Sep 20, 2019 •

edited

Loading