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

EIP-7251: Update correlation penalty computation #3882

Merged
merged 2 commits into from
Sep 5, 2024

Conversation

mkalinin
Copy link
Collaborator

@mkalinin mkalinin commented Aug 14, 2024

Fixes an issue in the correlation penalty computation that under certain total slashed balances (sum(state.slashings)) lead to zero penalty for 32 ETH validators and non-zero penalty for 2048 ETH validators, and as a result imbalance between the penalty for 64 x 32 ETH (= 0) and 1 x 2048 ETH (> 0) validators. It also fixes potential overflow in the same computation.

Many thanks to Pavel for finding the issue.
Many thanks to @fradamt for co-authoring the fix.

Problem

If validator effective_balance = 2048 ETH and > 30.024M ETH is slashed, the overflow can occur in the first line of the following code :

penalty_numerator = validator.effective_balance // increment * adjusted_total_slashing_balance
penalty = penalty_numerator // total_balance * increment

The cause of the imbalance problem is in the integer division in the second line of the above code.
Whenever validator.effective_balance // increment * adjusted_total_slashing_balance < total_balance the penalty would be zero. Let us use real numbers for effective balance to highlight the discrepancy:

penalty_32eth   =   32 * adjusted_total_slashing_balance // total_balance * increment
penalty_2048eth = 2048 * adjusted_total_slashing_balance // total_balance * increment

Obviously, for 32 ETH validator to experience non-zero penalty the adjusted_total_slashing_balance value should be 64 times higher than in the 2048 ETH case. Which is also illustrated by the following plot:

image

Fix

The idea of the fix is to compute penalty per EB increment in a way that never overflows and then multiply it by a number of increments to alleviate the imbalance between 64 x 32 ETH and 1 x 2048 ETH validators:

penalty_per_effective_balance_increment = adjusted_total_slashing_balance // (total_balance // increment)
effective_balance_increments = validator.effective_balance // increment
penalty = penalty_per_effective_balance_increment * effective_balance_increments

Analysis

Notebook with more details can be found here.

The fix has the following effects:

  1. Correlation penalty becomes linearly dependent on the amount of total slashed balance and number of effective balance increments. Which in its turn makes the penalty unaffected by effective balance distribution between validators, and makes the total effective balance of all slashed validators as the main factor for the penalty computation. In other words, 64 validators each having 32 ETH effective balance accrue the same penalty as a single 2048 ETH validator.
  2. Correlation penalty is always non-zero, even when only one validator is slashed, but the amount of it in this case is negligible with respect to the initial and attestation penalties that validator is experiencing when it gets slashed.
  3. Correlation penalty becomes a bit higher on average, but the total slashing penalty in Electra (with this fix) is reduced, especially for the cases when less than 320,000 ETH gets slashed. This is achieved by the initial penalty reduction.

image

image

image

image

@mkalinin mkalinin requested a review from ralexstokes August 14, 2024 10:40
@fradamt
Copy link
Contributor

fradamt commented Aug 14, 2024

Lgtm! Besides fixing the overflow and the issue of penalties not being uniform between 32 and 2048 validators (and anything in between), I think it's quite nice that it makes the penalty computation more precise, resulting in penalties being actually linear in the slashed amount rather than a step function. About this, one note to other reviewers is that penalties are now actually in Gwei, rather than increasing in 1 ETH increments.

@mkalinin Maybe you could link the notebook?

@mkalinin
Copy link
Collaborator Author

@mkalinin Maybe you could link the notebook?

Sure! Notebook with more details is available here (also put a link to the PR description)

Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

looks great!

AFAICT this is mathematically equivalent to the bellatrix spec if we are working over say R (real numbers)

and the issue arises due to working over the integers (both the issues with actual penalty amounts at the extremes and also the overflow)

this PR seems to handle it well

Copy link

@twoeths twoeths left a comment

Choose a reason for hiding this comment

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

the change looks good to me! lodestar tracks everything in increment value so not likely to face the overflow issue just fyi

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.

6 participants