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.

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

Involuntarily decrease all authorizations in case of slashing #87

Merged
merged 2 commits into from
Apr 22, 2022

Conversation

pdyraga
Copy link
Member

@pdyraga pdyraga commented Apr 6, 2022

The original approach was to decrease authorization on the slashing application and decrease authorizations on other applications only if necessary, that is, if the staked amount after the slashing was lower than the authorization on the other application.

For example, let's assume the staking provider has 1000 T delegated and three applications authorized:

A application authorized for 950 T
B application authorized for 500 T
C application authorized for 100 T

If C slashed for 50 T, authorization on C would be reduced by 50 T, to 100 - 50 = 50 T.
If A slashed for 950 T, authorizations on A would be reduced to 0, and authorizations on B and C would be reduced to 50 T.

The new approach assumes all applications are decreasing authorizations in case of slashing, no matter which application executed the slash.

If C slashed for 50 T, authorization on A would be reduced to 950 - 50 = 900 T, authorization on B would be reduced to 500 - 50 = 450 T, and authorization on C would be reduced to 100 - 50 = 50 T.
If A slashed for 950 T, authorizations on A, B, and C would be reduced to 0 T.

Depending on how staker thinks about setting their risk profile, the first or second approach could make more sense for them. One staker could argue slashing event on one application should not affect their authorizations on other applications, and another staker would argue they set their original risk profile based on another staked amount, and given that the situations changed and they have less tokens now, the risk profile should be adjusted.

Most importantly though, this change is made to avoid storing the address of slashing application in the slashing event structure. This change allows reducing the gas cost of slashing large groups SIGNIFICANTLY given that we store one EVM word less per each member seat in the group. For a group of 64 members, it means storing 64 EVM words less. For a group of 100 members, it means storing 100 EVM words less. This is extremely important for random beacon and tBTC where signing groups have 64 and 100 operators respectively.

This change was tested by @nkuba with the RandomBeacon contract's reportUnauthorizedSigning call with the cost of TokenStaking.seize extracted.

Before this change:

  • for group size 64, gas cost: 3 142 093,
  • for group size 100, gas cost: 4 860 500.

After this change:

  • for group size 64, gas cost: 1 720 849,
  • for group size 100: gas cost: 2 639 758

See keep-network/keep-core#2903 (comment)

I recommend reviewing this PR per-commit, as the diff can be misleading because of the linting changes.

The original approach was to decrease authorization on the slashing
application and decrease authorizations on other applications only
if necessary, that is, if the staked amount after the slashing was
lower than the authorization on the other application.

For example, let's assume the staking provider has 1000 T delegated
and three applications authorized:

A application authorized for 950 T
B application authorized for 500 T
C application authorized for 100 T

If C slashed for 50 T, authorization on C would be reduced by 50 T,
to 100 - 50 = 50 T.

If A slashed for 950 T, authorizations on A would be reduced to 0,
and authorizations on B and C would be reduced to 50 T.

The new approach assumes all applications are decreasing authorizations
in case of slashing, no matter which application executed the slash.

If C slashed for 50 T, authorization on A would be reduced to
950 - 50 = 900 T, authorization on B would be reduced to
500 - 50 = 450 T, and authorization on C would be reduced to
100 - 50 = 50 T.

If A slashed for 950 T, authorizations on A, B, and C would be reduced
to 0 T.

Depending on how staker thinks about setting their risk profile, the
first or second approach could make more sense for them. One staker
could argue slashing event on one application should not affect their
authorizations on other applications, and another staker would argue
they set their original risk profile based on another staked amount,
and given that the situations changed and they have less tokens now,
the risk profile should be adjusted.

Most importantly though, this change is made to avoid storing the
address of slashing application in the slashing event structure. This
change allows reducing the gas cost of slashing large groups
SIGNIFICANTLY given that we store one EVM word less per each member
seat in the group. For a group of 64 members, it means storing 64 EVM
words less. For a group of 100 members, it means storing 100 EVM words
less. This is extremely important for random beacon and tBTC where
signing groups have 64 and 100 operators respectively.

This change was tested with the `RandomBeacon` contract's
`reportUnauthorizedSigning` call with the cost of `TokenStaking.seize`
extracted.

Before this change:
- for group size 64, gas cost: 3 142 093,
- for group size 100, gas cost: 4 860 500.

After this change:
- for group size 64, gas cost: 1 720 849,
- for group size 100: gas cost: 2 639 758
Copy link
Contributor

@vzotova vzotova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except failing test - everything else seems good to me

@pdyraga
Copy link
Member Author

pdyraga commented Apr 21, 2022

Except failing test (...)

I was investigating it and seems it's something about hardhat version after 2.7.1. The version got updated during the work on #82. System tests work fine on f129e20 using 2.7.1 but they stopped working in b8d198e after the update to 2.8.4.

I was also able to reproduce the same problem in https://github.com/keep-network/coverage-pools. All system tests are passing there (hardhat version 2.6.4) but after updating to 2.8.4 they start failing with the same TypeError: config.chains.has is not a function error. Updating to the most recent hardhat version does not help.

I opened an issue in hardhat repo describing the problem as I was not able to figure out what is wrong - looking at the documentation it should all work fine: NomicFoundation/hardhat#2630

If you are fine about it @vzotova @cygnusv, I suggest we do not block on this problem and ignore this check as the code change has nothing to do with KEEP staking contract.

@cygnusv cygnusv merged commit 07e49aa into main Apr 22, 2022
@cygnusv cygnusv deleted the reduce-all branch April 22, 2022 07:30
pdyraga added a commit to keep-network/keep-core that referenced this pull request Apr 22, 2022
The new version contains changes to TokenStaking allowing to spend less
gas on slashing.

See threshold-network/solidity-contracts#87
pdyraga added a commit to keep-network/keep-core that referenced this pull request Apr 22, 2022
The new version contains changes to TokenStaking allowing to spend less
gas on slashing.

See threshold-network/solidity-contracts#87
pdyraga added a commit to keep-network/keep-core that referenced this pull request Apr 26, 2022
…g contract

The change in the staking contract modified how authorized stake is reduced
after slashing, making the reduction "global", to all authorized applications,
and not only "local" to a single application, as it was initially.

The original approach was to decrease authorization on the slashing application
and decrease authorizations on other applications only if necessary, that is, if
the staked amount after the slashing was lower than the authorization on the
other application.

For example, let's assume the staking provider has 1000 T delegated and three
applications authorized:
- A application authorized for 950 T
- B application authorized for 500 T
- C application authorized for 100 T

If C slashed for 50 T, authorization on C would be reduced by 50 T,
to 100 - 50 = 50 T.

If A slashed for 950 T, authorizations on A would be reduced to 0, and
authorizations on B and C would be reduced to 50 T.

The new approach assumes all applications are decreasing authorizations in case
of slashing, no matter which application executed the slash.

If C slashed for 50 T, authorization on A would be reduced to 950 - 50 = 900 T,
authorization on B would be reduced to 500 - 50 = 450 T, and authorization on C
would be reduced to 100 - 50 = 50 T.

If A slashed for 950 T, authorizations on A, B, and C would be reduced to 0 T.

The unit tests had to be adjusted to reflect the new reality.

See threshold-network/solidity-contracts#87
nkuba added a commit to keep-network/keep-core that referenced this pull request Apr 26, 2022
WalletRegistry.seize function implemented


Allows the wallet owner to add all signing group members of the wallet with the given ID to the slashing queue of the staking contract. The notifier will receive reward per each group member from the staking contract notifiers treasury. The reward is scaled by the `rewardMultiplier` provided as a parameter.

This function will be used by tBTC Bridge contract to slash wallet signing group members.

The gas cost of calling this function when slashing all 100 members of the group is ~3M gas.

This cost was captured after upgrading `@threshold-network/solidity-contracts` dependency to include the gas cost fix from threshold-network/solidity-contracts#87. 

After upgrading the dependency, I had to fix the unit tests for `WalletRegistry` authorizations to take into account the change of the behavior in the staking contract. This was done in the commit a5a83e7.
pdyraga added a commit to keep-network/keep-core that referenced this pull request Apr 26, 2022
The change in the staking contract modified how authorized stake is reduced
after slashing, making the reduction "global", to all authorized applications,
and not only "local" to a single application, as it was initially.

The original approach was to decrease authorization on the slashing application
and decrease authorizations on other applications only if necessary, that is, if
the staked amount after the slashing was lower than the authorization on the
other application.

For example, let's assume the staking provider has 1000 T delegated and three
applications authorized:
- A application authorized for 950 T
- B application authorized for 500 T
- C application authorized for 100 T

If C slashed for 50 T, authorization on C would be reduced by 50 T,
to 100 - 50 = 50 T.

If A slashed for 950 T, authorizations on A would be reduced to 0, and
authorizations on B and C would be reduced to 50 T.

The new approach assumes all applications are decreasing authorizations in case
of slashing, no matter which application executed the slash.

If C slashed for 50 T, authorization on A would be reduced to 950 - 50 = 900 T,
authorization on B would be reduced to 500 - 50 = 450 T, and authorization on C
would be reduced to 100 - 50 = 50 T.

If A slashed for 950 T, authorizations on A, B, and C would be reduced to 0 T.

The unit tests had to be adjusted to reflect the new reality.

See threshold-network/solidity-contracts#87
@pdyraga pdyraga added this to the v1.2.0 milestone Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants