Integrate T TokenStaking with RandomBeacon #2903

nkuba · 2022-03-29T08:54:44Z

Refs: #2850
Closes: #2715
Depends on: #2906

This PR integrates Random Beacon with Threshold Network TokenStaking contract.

It implements functions to let the operator join the sortition pool and update its status in the sortition pool. StakingStub contract has been removed and all tests operate on a real T TokenStaking now.

The changes were already implemented in ECDSA Wallets in #2850 and this PR is based on it.

Operators To Staking Providers Mapping

Please note that due to 8e9b2a4 a gas cost of executing functions that call seize and slash grew significantly:

        method            |   min   |   max   |   avg
-------------------------------------------------------
submitRelayEntry          | 213867  | 3693768 | 1760490 
reportRelayEntryTimeout   | 3641465 | 3659466 | 3654018
reportUnauthorizedSigning |    -    |    -    | 3774342

We integrate RandomBeacon application with TokenStaking contract defined for Threshold Network. The source code is copied from work that was already done for ECDSA Wallets in #2850

We don't want to validate TokenStaking size as it's an external contract that we expect to be deployed separately.

TokenStaking contract expects amounts for slashing to be uint96. This is size of number exceeds the T token total supply enyways and is so big that will fit the slashing amount for sure.

Compile dependencies from external package to use in deployment and tests here.

We will use actual T token implementation instead of this TestToken.

Stores ethers Signer in the Operator instead of just an address to simplify usage.

We use minimumAuthorization parameters instead which is an application specific parameter.

We integrated with TokenStaking defined in Threshold Network's contracts so we can switch our tests to use the proper contracts. We use hardhat deployment fixture `TokenStaking` that is defined in @threshold-network/solidity-contracts to simplify deployment of contracts for tests.

The tests are covered byt the Authorization tests.

TokenStaking contract operates on staking providers addresses. Group members are operators that are internal to RandomBeacon application. We need to map operator addresses to staking provider addresses for seize and slash functions calls.

To have the tests that are more reliable we neeed to ensure unique address is used for each of the roles registered in Token Staking. We need an unique address for 64 group members for each role: owner, staking provider, operator, beneficiary and authorizer. We introduced `unnamedSignersOffset` to specify from which unnamed signer account we should start obtaining signers. This is for a case where we want to pull some other accounts e.g. for third party and don't want to overlap.

getUnnamedSigners returns a signer so it simplifies the way we want to obtain the signer.

It's easier to modify the value in parameters without having to update all the hardcoded places.

We don't want to error on this check.

Our contracts grew to the point where a RandomBeaconStub is problematic to deploy. The RandomBeacon size is fine though. We can safely ignore contract size on deployment in hardhat network as contract size is verified by `contractSizer` and it will alert us if any of the contracts is too big.

We want to have unique signers for operators registration for each of 5 roles. This requires hardhat network configuration account count to be aligned. We want to ensure the configuration is correct and output a meaningfull error if it's not. Without that the test execution fails with an error that doesn't give much clue.

The rewards are calculated based on the multiplier and queue of slashed staking providers. We fix how the expected value is calculated.

We set minimumAuthorization to a value different than unauthorizedSigningSlashingAmount to not have false positive results in tests when the parameters would be confused.

…ng-integration

pdyraga

First round, more to come.

solidity/random-beacon/.eslintrc

solidity/random-beacon/contracts/RandomBeacon.sol

solidity/random-beacon/contracts/libraries/Authorization.sol

nkuba · 2022-04-04T09:28:49Z

solidity/random-beacon/contracts/RandomBeacon.sol

                .getIDOperators(groupMembers);

-            try staking.slash(slashingAmount, groupMembersAddresses) {
+            address[] memory stakingProvidersAddresses = new address[](


I tried to extract this piece of code to the Authorizations library (code snippet below) to reduce transaction costs, but unfortunately, it ended up being even more expensive:

method | min | max | avg ------------------------------------------------------- submitRelayEntry | 213867 | 3735913 | 1779221 reportRelayEntryTimeout | 3683609 | 3701610 | 3696162 reportUnauthorizedSigning | - | - | 3816486

function operatorsToStakingProviders( Data storage self, address[] memory operators ) external returns (address[] memory) { address[] memory stakingProviders = new address[](operators.length); for (uint256 i = 0; i < operators.length; i++) { stakingProviders[i] = self.operatorToStakingProvider[operators[i]]; } return stakingProviders; }

We will need to do some diligence here and figure out where does the cost of gas come from. If it is real TokenStaking or something else. TokenStaking writes to the queue so the cost could increase by 1.2M for a whole group slashed but what we see is an increase by 3.28M for reportUnauthorizedSigning.

For a group of 100 operators, the total cost of reportUnauthorizedSigning would be 5 726 857. It turns out that:

sortitionPool.getIDOperators costs 326 864

operatorToStakingProvider loop costs 249 402

tokenStaking.seize costs 4 858 014

From the TokenStaking perspective it's slashingQueue.push that produces such a cost burden. For tokenStaking.seize call mentioned above the slashingQueue.push execution consumes 4 546 619 out of the 4 858 014.

Addressed in threshold-network/solidity-contracts#87.

We no longer need this setting as there are no more matches in the tests.

pdyraga · 2022-04-04T09:43:39Z

solidity/random-beacon/hardhat.config.ts

+      // unique addresses in staking for each operator.
+      accounts: { count: 10 + 5 * 64 },
      tags: ["local"],
+      allowUnlimitedContractSize: true,


This is only for RandomBeaconStub, right? It is quite dangerous because we may end up with non-deployable RandomBeacon and not even notice it. Would it be possible to branch-off from this PR, remove this line, and try to make tests passing by, for example, dividing RandomBeaconStub into several per-test stubs?

Yeah, the RandomBeaconStub is problematic. I already tried to reduce the size of the stub in #2906. There is not much room for RandomBeacon to grow as it's 24.091 KB already.

As I mentioned in the commit 90e6594 we're protected by the hardhat-contract-sizer. Whenever we run yarn build the plugin will verify the size of the RandomBeacon.

Would it be possible to branch-off from this PR, remove this line, and try to make tests passing by, for example, dividing RandomBeaconStub into several per-test stubs?

If the contract sizer is not enough in your opinion, I can try slicing the contract.

Let's try slicing and we'll see where we end up with. I am not a fan of stubs adding a lot of functionality because in the end, we do not know what we are testing - the final contract or stub. On the other hand, having just one stub could (unsure) be better for aggregating gas usage reports by hardhat plugin.

FWIW hardhat-contract-sizer will fail contracts build in case of a too big contract. It means that CI will fail and PR won't be mergeable. See example PR #2912

solidity/random-beacon/package.json

In keep-network/sortition-pools#149 we discontinued publishing `dev` packages for sortition pools, but use `pre` instead.

solidity/random-beacon/contracts/RandomBeacon.sol

solidity/random-beacon/test/RandomBeacon.Relay.test.ts

solidity/random-beacon/test/fixtures/index.ts

solidity/random-beacon/test/RandomBeacon.Relay.test.ts

solidity/random-beacon/test/system/e2e.test.ts

solidity/random-beacon/test/RandomBeacon.Authorization.test.ts

For consistency I aliased BeaconInactivity as we do for other Beacon* libraries. Also I gathered aliased imports together to make it prettier.

Some adjustments to tests to make them even more fabulous! 🌟

…ng-integration

To be more clear about accounts configuration for tests we defined testConfig object that holds the config and those config are reused in operators registration. Additionally we validate on hardhat test start if the parameters are configured properly.

pdyraga · 2022-04-06T13:43:59Z

The problem of slashing gas costs mentioned in the PR description is addressed in threshold-network/solidity-contracts#87.

nkuba requested review from dimpar and pdyraga March 29, 2022 10:29

nkuba force-pushed the random-beacon-t-staking-integration branch from 05ec8a2 to 8436925 Compare March 29, 2022 15:04

nkuba mentioned this pull request Mar 29, 2022

Make Relay a linked library #2904

Closed

nkuba added 11 commits March 30, 2022 14:51

Intergrate RandomBeacon with T TokenStaking

94cc6e1

We integrate RandomBeacon application with TokenStaking contract defined for Threshold Network. The source code is copied from work that was already done for ECDSA Wallets in #2850

Exclude TokenStaking from contract sizer check

b77b267

We don't want to validate TokenStaking size as it's an external contract that we expect to be deployed separately.

Define slashing amounts as uint96

6539ba1

TokenStaking contract expects amounts for slashing to be uint96. This is size of number exceeds the T token total supply enyways and is so big that will fit the slashing amount for sure.

Compile dependencies from @threshold-network/solidity-contracts

6989645

Compile dependencies from external package to use in deployment and tests here.

Remove TestToken

6ec7c35

We will use actual T token implementation instead of this TestToken.

Store signer in Operator object

82282c1

Stores ethers Signer in the Operator instead of just an address to simplify usage.

Remove minimumStake constant

5250a96

We use minimumAuthorization parameters instead which is an application specific parameter.

Provide types for contract factories

8a5c835

Update authorization parameters used in tests

215465a

Remove Poll tests

043a28f

The tests are covered byt the Authorization tests.

nkuba force-pushed the random-beacon-t-staking-integration branch from 8436925 to 043a28f Compare March 30, 2022 12:52

nkuba changed the base branch from main to beacon-stub-improve March 30, 2022 12:53

Commit latest yarn.lock

5eec8e6

nkuba mentioned this pull request Mar 30, 2022

Removing slashing factor #2840

Merged

nkuba added 10 commits March 30, 2022 15:20

Remove duplicated SignerWithAddress

3acd836

Use getUnnamedSigners function

0abf1b2

getUnnamedSigners returns a signer so it simplifies the way we want to obtain the signer.

Use params for amount instead of hardcoded value

a77274d

It's easier to modify the value in parameters without having to update all the hardcoded places.

Eslint warn on prefer-destructuring

973c704

We don't want to error on this check.

Fix code formatting problems

2727dae

Minor imprevements to operators registration script

bfc8a2b

nkuba mentioned this pull request Mar 31, 2022

ECDSA Wallets: Use unique accounts for staking and operator registration roles in tests #2909

Merged

nkuba added 3 commits March 31, 2022 17:23

Fix how reward is calculated in tests

e8d1b4c

The rewards are calculated based on the multiplier and queue of slashed staking providers. We fix how the expected value is calculated.

Reconfigure minimumAuthorization in tests

684e0ef

We set minimumAuthorization to a value different than unauthorizedSigningSlashingAmount to not have false positive results in tests when the parameters would be confused.

Merge remote-tracking branch 'origin/main' into random-beacon-t-staki…

a6e48dd

…ng-integration

pdyraga reviewed Apr 4, 2022

View reviewed changes

nkuba commented Apr 4, 2022

View reviewed changes

nkuba added 2 commits April 4, 2022 11:46

Remove prefer-destructuring:warn from eslintrc

ea31a1c

We no longer need this setting as there are no more matches in the tests.

Replace application with beacon in the doc

472fcdf

pdyraga reviewed Apr 4, 2022

View reviewed changes

nkuba added 4 commits April 4, 2022 12:06

Improve documentation

1421d09

Rename Authorization library to Beacon Authorization

41a16e6

Improve documentation

cf8dc12

Use pre package for @keep-network/sortition-pools

9fbb133

In keep-network/sortition-pools#149 we discontinued publishing `dev` packages for sortition pools, but use `pre` instead.

pdyraga reviewed Apr 4, 2022

View reviewed changes

solidity/random-beacon/test/RandomBeacon.Authorization.test.ts Outdated Show resolved Hide resolved

solidity/random-beacon/test/RandomBeacon.Authorization.test.ts Show resolved Hide resolved

solidity/random-beacon/test/RandomBeacon.Authorization.test.ts Outdated Show resolved Hide resolved

pdyraga mentioned this pull request Apr 4, 2022

Authorizations: integration with T staking contract #2766

Closed

nkuba added 5 commits April 4, 2022 15:33

Impreve Beacon* libraries import

88df9b8

For consistency I aliased BeaconInactivity as we do for other Beacon* libraries. Also I gathered aliased imports together to make it prettier.

Test improvements

f931643

Some adjustments to tests to make them even more fabulous! 🌟

Merge remote-tracking branch 'origin/main' into random-beacon-t-staki…

7421d1f

…ng-integration

Prettier ignore hardhat-dependency-compiler

2209398

nkuba requested a review from pdyraga April 6, 2022 12:04

pdyraga mentioned this pull request Apr 6, 2022

Involuntarily decrease all authorizations in case of slashing threshold-network/solidity-contracts#87

Merged

pdyraga approved these changes Apr 6, 2022

View reviewed changes

pdyraga merged commit d5f99af into main Apr 6, 2022

pdyraga deleted the random-beacon-t-staking-integration branch April 6, 2022 13:43

This was referenced Apr 6, 2022

Reimbursements in Random Beacon from the ETH pool #2860

Merged

Synchronising authorisations between the beacon and the T staking contract #2747

Closed

pdyraga added this to the solidity/v2.0.0 milestone Jun 2, 2022

pdyraga added the ⛓️ solidity label Sep 29, 2022

Uh oh!

Integrate T TokenStaking with RandomBeacon #2903

Integrate T TokenStaking with RandomBeacon #2903

Uh oh!

Conversation

nkuba commented Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Operators To Staking Providers Mapping

Uh oh!

pdyraga left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nkuba Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pdyraga Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

nkuba Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkuba Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

pdyraga Apr 6, 2022

Choose a reason for hiding this comment

Uh oh!

pdyraga Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

nkuba Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

pdyraga Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

nkuba Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pdyraga commented Apr 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nkuba commented Mar 29, 2022 •

edited

Loading

nkuba Apr 4, 2022 •

edited

Loading

nkuba Apr 4, 2022 •

edited

Loading

nkuba Apr 4, 2022 •

edited

Loading