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

64 epoch cleanup by @justindrake #2212

Closed
wants to merge 11 commits into from
Closed

Conversation

djrtwo
Copy link
Contributor

@djrtwo djrtwo commented Feb 23, 2021

From @JustinDrake :

Pushed my review/cleanup. Largely just polishing :)

Cosmetic fixes

  • rename leak_score to leak_scores (for consistency with validators, balances, etc.)
  • rename "regular penalties" to "flag penalties" (for consistency with flag rewards)
  • rename ValidatorFlag to ParticipationFlags (more precise; also pluralised because as there are 8 flags)
  • initialise ParticipationFlags with 0b0000_0000 (instead of 0)
  • introduce the concept of "flag mask" (e.g. rename TIMELY_HEAD_FLAG to TIMELY_HEAD_FLAG_MASK)
  • disambiguate uses of "new" which sometimes means "modified" and sometimes "added"
  • rename is_activation_exit_period_boundary to is_activation_exit_epoch (for consistency with compute_activation_exit_epoch)
  • isolate leak updates in process_leak_updates (extracting logic from process_rewards_and_penalties) and call directly from process_epoch
  • change type of leak_epoch_counter to Epoch
  • rename leak_epoch_counter to leak_epochs_counter
  • rename SYNC_COMMITTEE_PUBKEY_AGGREGATES_SIZE to SYNC_SUBCOMMITTEE_SIZE
  • add is_leak_active (replacing and deprecating is_in_inactivity_leak)
  • add LEAK_PENALTY_QUOTIENT (merging with LEAK_SCORE_BIAS; replacing and deprecating INACTIVITY_PENALTY_QUOTIENT)
  • add EPOCHS_TO_LEAK_PENALTIES (replacing and deprecating MIN_EPOCHS_TO_INACTIVITY_PENALTY)
  • remove unnecessary for index in get_eligible_validator_indices(state) in process_leak_updates and get_flag_rewards

Substantive fixes

  • run process_effective_balance_updates only at activation-exit epochs
  • properly round up in compute_activation_exit_epoch (notice special case when epoch % EPOCHS_PER_ACTIVATION_EXIT_PERIOD == 0)
  • remove and is_matching_target in process_attestation when append(TIMELY_HEAD_FLAG_MASK)

djrtwo added a commit that referenced this pull request Mar 2, 2021
djrtwo added a commit that referenced this pull request Mar 2, 2021
djrtwo added a commit that referenced this pull request Mar 2, 2021
djrtwo added a commit that referenced this pull request Mar 2, 2021
djrtwo added a commit that referenced this pull request Mar 2, 2021
djrtwo added a commit that referenced this pull request Mar 2, 2021
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.

My first scan. 👀

  • remove and is_matching_target in process_attestation when append(TIMELY_HEAD_FLAG_MASK)

I can't find it in this PR. Was it left during the previous cleanups?

# 2**26 (= 67,108,864)
INACTIVITY_PENALTY_QUOTIENT: 67108864
# 2**28 (= 268,435,456)
LEAK_PENALTY_QUOTIENT: 268435456
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. INACTIVITY_PENALTY_QUOTIENT was renamed here but it was not modified in phase0/beacon-chain.md.
  2. The phase 0 value should be 2**26 (= 67,108,864).
  3. Double check: is it necessary to rename the variable of phase0?
    • get_inactivity_penalty_deltas will be rewritten anyway. So we only have to rename it in HF1.
    • Besides that the client implementations have to change their phase 0 code, the additional documents may also need to sync the differences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we should rename any variables from phase 0. It's way to easy to bork test vectors and cause issues in clients, imo


### Time parameters

| Name | Value | Unit | Duration |
| - | - | :-: | :-: |
| `EPOCHS_PER_SYNC_COMMITTEE_PERIOD` | `Epoch(2**8)` (= 256) | epochs | ~27 hours |
| `EPOCHS_PER_ACTIVATION_EXIT_PERIOD` | `Epoch(2**6)` (= 64) | epochs | ~6.8 hours |
| `EPOCHS_TO_LEAK_PENALTIES` | `uint64(2**2)` (= 4) | epochs | 25.6 minutes |
Copy link
Contributor

Choose a reason for hiding this comment

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

It was defined/renamed in phase0 spec in this PR and the value was unchanged. This line could be removed:

Suggested change
| `EPOCHS_TO_LEAK_PENALTIES` | `uint64(2**2)` (= 4) | epochs | 25.6 minutes |

Copy link
Contributor Author

Choose a reason for hiding this comment

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

leaving in here for now until we decide about renaming in general

@@ -99,7 +99,7 @@ MIN_VALIDATOR_WITHDRAWABILITY_DELAY: 256
# 2**8 (= 256) epochs ~27 hours
SHARD_COMMITTEE_PERIOD: 256
# 2**2 (= 4) epochs 25.6 minutes
MIN_EPOCHS_TO_INACTIVITY_PENALTY: 4
EPOCHS_TO_LEAK_PENALTIES: 4
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, want to make sure if it's a necessary change in phase 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

specs/lightclient/beacon-chain.md Outdated Show resolved Hide resolved
specs/lightclient/beacon-chain.md Outdated Show resolved Hide resolved
Comment on lines +238 to +243
#### `is_leak_active`

```python
def is_leak_active(state: BeaconState) -> bool:
return get_finality_delay(state) > EPOCHS_TO_LEAK_PENALTIES
```
Copy link
Contributor

@hwwhww hwwhww Mar 2, 2021

Choose a reason for hiding this comment

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

It's a 100% duplicate of the phase 0 version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

leaving in until we decide on cross-cutting changes on phases

specs/lightclient/beacon-chain.md Outdated Show resolved Hide resolved
specs/lightclient/beacon-chain.md Outdated Show resolved Hide resolved
Comment on lines -716 to -717
process_rewards(state)
process_penalties(state)
Copy link
Contributor

@hwwhww hwwhww Mar 2, 2021

Choose a reason for hiding this comment

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

I can see why this change makes sense, but IMHO it's good to have multiple purpose-focused functions. It would be more flexible when we have to modify it in the future forks.

Copy link
Contributor Author

@djrtwo djrtwo Mar 5, 2021

Choose a reason for hiding this comment

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

yeah, I would tend to agree given the surgery we've had to do across forks thus far

@@ -602,21 +555,19 @@ def process_sync_committee(state: BeaconState, body: BeaconBlockBody) -> None:

### Modified helpers

#### New `compute_activation_exit_epoch`
#### Modified `compute_activation_exit_epoch`
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: need to add some unit tests for it.

@JustinDrake JustinDrake added the Altair aka HF1 label Mar 3, 2021
@djrtwo
Copy link
Contributor Author

djrtwo commented Mar 5, 2021

I'm against changing constants names from phase 0 (and especially values e.g. INACTIVITY_PENALTY_QUOTIENT).

The general policy has been "never change constants at forks, only add new ones and update calling code". This helps keep configuration clean and avoid hard to track down changes and issues in testing (that might also surface in clients)

Copy link
Contributor Author

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

minor

def add_validator_flags(flags: ValidatorFlag, add: ValidatorFlag) -> ValidatorFlag:
return flags | add
def add_flag(flags: ParticipationFlags, flag_index: int) -> ParticipationFlags:
flag = ParticipationFlags(2**flag_index)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
flag = ParticipationFlags(2**flag_index)
flag = ParticipationFlags(2 ** flag_index)

tests/core/pyspec/eth2spec/test/helpers/rewards.py Outdated Show resolved Hide resolved
tests/core/pyspec/eth2spec/test/helpers/rewards.py Outdated Show resolved Hide resolved
# 2**26 (= 67,108,864)
INACTIVITY_PENALTY_QUOTIENT: 67108864
# 2**28 (= 268,435,456)
LEAK_PENALTY_QUOTIENT: 268435456
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we should rename any variables from phase 0. It's way to easy to bork test vectors and cause issues in clients, imo

@@ -99,7 +99,7 @@ MIN_VALIDATOR_WITHDRAWABILITY_DELAY: 256
# 2**8 (= 256) epochs ~27 hours
SHARD_COMMITTEE_PERIOD: 256
# 2**2 (= 4) epochs 25.6 minutes
MIN_EPOCHS_TO_INACTIVITY_PENALTY: 4
EPOCHS_TO_LEAK_PENALTIES: 4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto


### Time parameters

| Name | Value | Unit | Duration |
| - | - | :-: | :-: |
| `EPOCHS_PER_SYNC_COMMITTEE_PERIOD` | `Epoch(2**8)` (= 256) | epochs | ~27 hours |
| `EPOCHS_PER_ACTIVATION_EXIT_PERIOD` | `Epoch(2**6)` (= 64) | epochs | ~6.8 hours |
| `EPOCHS_TO_LEAK_PENALTIES` | `uint64(2**2)` (= 4) | epochs | 25.6 minutes |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

leaving in here for now until we decide about renaming in general

specs/lightclient/beacon-chain.md Outdated Show resolved Hide resolved
penalties = [Gwei(0)] * len(state.validators)

# Only give penalties on period boundaries
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with removing this blocking condition is that test vectors can very strangely be generated. That is, the vectors for get_leak_penalties specifically if run not on an epoch boundary will return non-zero values. I'd prefer keeping the blocking condition in to avoid misuse in alternative contexts even though the calling function in the spec had the conditional

@djrtwo
Copy link
Contributor Author

djrtwo commented Mar 5, 2021

remove and is_matching_target in process_attestation when append(TIMELY_HEAD_FLAG_MASK)

this is purposeful

  1. it matches phase 0 functionality, see get_matching_head_attestations which calls get_matching_target_attestations as its base
  2. this functionality was put into phase 0 to ensure that there was not a perverse scenario where you can essentially net 0 reward/penalty and not contribute to finality (e.g. get correct head but insert a bad target). It is instead natural for correctness to stem from source into target up to head (voting on a cohesive chain) rather than each being independent

@djrtwo
Copy link
Contributor Author

djrtwo commented Mar 11, 2021

This optimization will not go into Altair. More R&D is desired before making such spec optimizations.

See call notes here ethereum/eth2.0-pm#208

@djrtwo djrtwo closed this Mar 11, 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.

3 participants