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

Global quotient penalties [isolated] #2177

Closed
wants to merge 2 commits into from
Closed

Global quotient penalties [isolated] #2177

wants to merge 2 commits into from

Conversation

djrtwo
Copy link
Contributor

@djrtwo djrtwo commented Jan 5, 2021

Isolated penalties via a global quotient feature.
Pulled out of #2159. Branch based on/dependent on account-reform (#2176)


Copied from #2150:

This PR changes the way that we process inactivity penalties (both in the "normal case" and in the "inactivity leak" case). Currently, we process such penalties by directly adjusting all active validators' balances, going through all validators in a loop at the end of each epoch. However, this poses the risk that even a chain with a very low level of participation (eg. 100 empty epochs followed by a single block) requires O(N) work per epoch, leading to a potential DoS vector via an attacker proposing many such chains.

This PR fixes this by moving all validator penalties into a single global quotient, balance_denominator, which starts at 10**9 gwei and increments from there. For example, if in some epoch, participating validators are to gain a reward of 0.02% and nonparticipating validators a penalty of 0.03%, the balance_denominator increases by 1.0003x, and participating validators are assigned a reward of 0.05%. If no one (or almost no one) is participating, then very little work needs to be done except for the single O(1) operation of adjusting the balance_denominator.

We make effective balances reflect the denominator as part of hysteresis: when the balance_denominator increases, the thresholds needed for a validator to be assigned a particular EB level change. For example, for an EB of 20 ETH, at the start (balance_denominator == 1), the hysteresis thresholds are 19.75 ETH to drop below and 21.25 ETH to go above 20 ETH. But when the balance_denominator increases by 1.0003x, those thresholds shift to 19.755925 ETH to drop below and 21.256375 ETH to go above. The buckets themselves shift.

When the balance_denominator goes above 2, we perform the extremely rare procedure of halving both the balance_denominator and everyone's balances (so the effective balances stay the same).

This approach is slightly different from #1340 in that balances are reconciled during hysteresis and not during the first block after a series of empty slots; this arguably increases efficiency for low-participation chains (and not just "nearly zero participation chains") further.

Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

Porting previous comments from #2159 and #2150 to here.

@@ -81,6 +82,7 @@ The reward fractions add up to 7/8, leaving the remaining 1/8 for proposer rewar
| - | - |
| `PARTICIPATION_FLAGS_LENGTH` | `8` |
| `G2_POINT_AT_INFINITY` | `BLSSignature(b'\xc0' + b'\x00' * 95)` |
| `GWEI_PER_ETH` | `10**9` |
Copy link
Contributor

Choose a reason for hiding this comment

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

((porting previous comments to here))

Maybe use ether instead of eth in the denomination context?

  • GWEI_PER_ETH -> GWEI_PER_ETHER
  • get_base_reward_per_eth -> get_base_reward_per_ether

@@ -155,6 +157,8 @@ class BeaconState(Container):
# Light client sync committees
current_sync_committee: SyncCommittee
next_sync_committee: SyncCommittee
# Denominator to real-time-updated balances (NOT effective balances!)
balance_denominator: uint64
Copy link
Contributor

Choose a reason for hiding this comment

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

((porting previous comments to here))
https://github.com/ethereum/eth2.0-specs/pull/2150/files#r550251888
@dankrad: This means balances is not denoted in GWei anymore, should be changed to uint64

Comment on lines +533 to +538
state.balance_denominator = (
state.balance_denominator
+ penalty
+ (state.balance_denominator - GWEI_PER_ETH) * penalty // GWEI_PER_ETH
)
Copy link
Contributor

Choose a reason for hiding this comment

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

((porting previous comments to here))
https://github.com/ethereum/eth2.0-specs/pull/2150/files#r550256452

@dankrad:

Suggested change
state.balance_denominator = (
state.balance_denominator
+ penalty
+ (state.balance_denominator - GWEI_PER_ETH) * penalty // GWEI_PER_ETH
)
state.balance_denominator = state.balance_denominator + state.balance_denominator * penalty // GWEI_PER_ETH

HYSTERESIS_INCREMENT = uint64(EFFECTIVE_BALANCE_INCREMENT // HYSTERESIS_QUOTIENT)
DOWNWARD_THRESHOLD = HYSTERESIS_INCREMENT * HYSTERESIS_DOWNWARD_MULTIPLIER
UPWARD_THRESHOLD = HYSTERESIS_INCREMENT * HYSTERESIS_UPWARD_MULTIPLIER
balance_increments = state.balances[index] * HYSTERESIS_QUOTIENT // state.balance_denominator
Copy link
Contributor

Choose a reason for hiding this comment

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

((porting previous comments to here))
https://github.com/ethereum/eth2.0-specs/pull/2150/files#r550261538
@dankrad: Suggest name balance_in_increments to make it clear that increment is menat as a unit here

@djrtwo
Copy link
Contributor Author

djrtwo commented Jan 14, 2021

Some comments raised during a discussion with @paulhauner

  1. not-yet-activated and exited validators are currently subject to changes in the penalty quotient even though they can’t get rewards [for exits, maybe balances can be calculated against current penalty quotient and frozen at exit. similar but reverse for activation]
  2. deposited ETH amount needs to be adjusted for the current penalty quotient when creating / topping up a validator
  3. there are still a couple of O(N) loops (i.e. updating effective balances and process registry updates) <— maybe these can be optimized more on client side but unclear and cost could be significant at times
  4. [related to 3] depending on what might change in the state due to some of the loops still present, intermittent calculation of state_roots might still be a problem [thus not storing of empty state roots would be a huge savings but at a cost to funtionality]

1, 2, and similar issues can probably be massaged in the spec. Issues such as 3 and 4 need clearer answers to better understand if this actually gives us the comptuational savings we are looking for

@vbuterin
Copy link
Contributor

I think the remaining O(N) loops can be optimized by using priority queues to store who will soon be activated/exited and who is the closest to needing an effective balance update. Agree that (1) and (2) can be managed with changes to the spec (though I'll probably make a different PR that uses a multiplication factor instead of a balance numerator; can fix those issues while doing that).

Base automatically changed from accounting-reform to dev February 4, 2021 17:16
@djrtwo djrtwo closed this Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants