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

Questions about get_justification_and_finalization_deltas #882

Closed
JustinDrake opened this issue Apr 6, 2019 · 8 comments
Closed

Questions about get_justification_and_finalization_deltas #882

JustinDrake opened this issue Apr 6, 2019 · 8 comments
Labels
general:proofread spelling, grammar, accuracy general:question Further information is requested

Comments

@JustinDrake
Copy link
Contributor

  1. What is the // 2 quotient in INACTIVITY_PENALTY_QUOTIENT // 2 (as part of get_inactivity_penalty)?
  2. Should adjusted_quotient // 5 be adjusted_quotient // 4?
  3. Can I confirm the above quotient is the max number of base rewards that every validator is entitled? (Those base rewards being a) expected FFG source b) expected FFG source c) expected head d) inclusion speed bonus.) Should we create a MAX_BASE_REWARDS_PER_EPOCH constant?
  4. Can I confirm that 4 in epochs_since_finality > 4 corresponds to the maximum age of a source epoch that can be used for finality? (Specifically, the finality condition being "The 2nd/3rd/4th most recent epochs are all justified, the 2nd using the 4th as source".) Should we create a MAX_SOURCE_EPOCH_AGE_FOR_FINALITY (or better named) constant?
  5. In the context of Withdrawal queue -> exit queue #850, can we simplify eligible_validators? Specifically, can we remove the validator.slashed and current_epoch < validator.withdrawable_epoch condition and use get_active_validator_indices instead? (See also Question about eligible validators in rewards/penalties processing #857.)
  6. Most helper variables in get_justification_and_finalization_deltas are relative to the previous epoch. (This includes boundary_attestations, boundary_attesting_balance, total_balance, total_attesting_balance, matching_head_attestations, etc.) The only exception is eligible_validators which is defined relative to current_epoch. Is this mismatch necessary?
  7. "Take away max rewards if the beacon chain is not finalizing" => Why are the penalties from get_inactivity_penalty not sufficient?
  8. Why is there a + 1 (as opposed to + 0 or - 1) in epochs_since_finality?
  9. What is the status of Attestation inclusion rewards #335?
@JustinDrake JustinDrake added general:question Further information is requested general:proofread spelling, grammar, accuracy labels Apr 6, 2019
@vbuterin
Copy link
Contributor

vbuterin commented Apr 7, 2019

In the context of #850, can we simplify eligible_validators? Specifically, can we remove the validator.slashed and current_epoch < validator.withdrawable_epoch condition and use get_active_validator_indices instead? (See also #857.)

There was an explicit intention that validators that are inactive but have been slashed are "eligible" to receive penalties. I suppose the idea is that we can remove that condition because we've replaced the withdrawal queue with an exit queue? If so, I would disagree, because being slashed also means you are put into a waiting period for 36 days, during which the intent is that you suffer offline penalties and additionally in the middle of this period suffer a penalty proportional to the number of other validators that have been slashed within 18 days of yourself in either direction.

@vbuterin
Copy link
Contributor

vbuterin commented Apr 7, 2019

"Take away max rewards if the beacon chain is not finalizing" => Why are the penalties from get_inactivity_penalty not sufficient?

The goal here is that if a majority coalition is censoring more than 1/3, its chance to earn any validator profits is taken away in order to penalize it.

The only exception is eligible_validators which is defined relative to current_epoch. Is this mismatch necessary?

Doing it based on previous epoch does seem correct; can change.

What is the // 2 quotient in INACTIVITY_PENALTY_QUOTIENT // 2 (as part of get_inactivity_penalty)?

Now that I think about it this may be worth removing; we can just adjust the inactivity penalty quotient as needed later.

Why is there a + 1 (as opposed to + 0 or - 1) in epochs_since_finality?

This seems weird; I think +0 is fine.

Should adjusted_quotient // 5 be adjusted_quotient // 4?

Let's rethink the reward coefficients after we nail down the beacon slot / shard slot / min attestation inclusion relationship? We also need to think about crosslink rewards.

@JustinDrake
Copy link
Contributor Author

being slashed also means you are put into a waiting period for 36 days, during which the intent is that you suffer offline penalties and additionally in the middle of this period suffer a penalty proportional to the number of other validators that have been slashed within 18 days of yourself in either direction

To recap, there are the slashing-specific penalties:

  1. The whistleblowing reward which is immediately taken in slash_validator.
  2. The inactivity penalty during the exit queue (which is forced because get_unslashed_attesting_indices filters slashed validators).
  3. The partial slashing based on 18 days in either direction.
  4. The inactivity penalty during the withdrawal queue.

My gut feel is that 1), 2), 3) combined is sufficiently harsh. (And as you say, 4) is partially covered by 3).)

For simplicity, removing 4) also allows us to replace the following 7 lines of code with just 1. (It also removes the mismatch between eligible_validators and things like total_balance.)

    eligible_validators = [
        index for index, validator in enumerate(state.validator_registry)
        if (
            is_active_validator(validator, current_epoch) or
            (validator.slashed and current_epoch < validator.withdrawable_epoch)
        )
    ]

its chance to earn any validator profits is taken away in order to penalize it

What about rewards[index] = 0 (as opposed to penalties[index] += base_reward * 4)?

Let's rethink the reward coefficients after we nail down the beacon slot / shard slot / min attestation inclusion relationship?

👍

@vbuterin
Copy link
Contributor

vbuterin commented Apr 8, 2019

My gut feel is that 1), 2), 3) combined is sufficiently harsh. (And as you say, 4) is partially covered by 3).)

The goal is to prevent self-slashing from being a way to escape if you are being censored. If you do that, and you do that before everyone else, then (2) is tiny and (3) is tiny, so we need (4).

In the normal case, 36 days of inactivity penalty is only ~0.5% of your balance; it's not a very large penalty.

@vbuterin
Copy link
Contributor

vbuterin commented Apr 8, 2019

What about rewards[index] = 0 (as opposed to penalties[index] += base_reward * 4)?

The problem with that approach is that it takes away the incentive to perform well. The effect of the current code is that your net reward is exactly 0 if you do everything perfectly, and goes negative otherwise. If we just do rewards[index] = 0 then the incentive to get messages included on time etc also disappears.

@djrtwo
Copy link
Contributor

djrtwo commented Apr 17, 2019

Can I confirm that 4 in epochs_since_finality > 4 corresponds to the maximum age of a source epoch that can be used for finality? (Specifically, the finality condition being "The 2nd/3rd/4th most recent epochs are all justified, the 2nd using the 4th as source".) Should we create a MAX_SOURCE_EPOCH_AGE_FOR_FINALITY (or better named) constant?

Although these numbers are the same, I don't think the numbers actually should be coupled. You can tune the > 4 to be more or less strict on when inactivity penalty begins and it has nothing to do with MAX_SOURCE_EPOCH_AGE_FOR_FINALITY

@djrtwo
Copy link
Contributor

djrtwo commented Apr 17, 2019

"Take away max rewards if the beacon chain is not finalizing" => Why are the penalties from get_inactivity_penalty not sufficient?

The idea here is that during an inactivity penalty leak, if you are performing optimally, you do not make or lose any money.

@JustinDrake
Copy link
Contributor Author

Closing in favour of #949 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general:proofread spelling, grammar, accuracy general:question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants