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-6988: Slashed validator cannot be elected as a block proposer #3371

Open
wants to merge 18 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,19 @@ jobs:
command: make citest fork=eip6110
- store_test_results:
path: tests/core/pyspec/test-reports
test-eip6988:
docker:
- image: circleci/python:3.9
working_directory: ~/specs-repo
steps:
- restore_cache:
key: v3-specs-repo-{{ .Branch }}-{{ .Revision }}
- restore_pyspec_cached_venv
- run:
name: Run py-tests
command: make citest fork=eip6988
- store_test_results:
path: tests/core/pyspec/test-reports
table_of_contents:
docker:
- image: circleci/node:10.16.3
Expand Down Expand Up @@ -291,6 +304,9 @@ workflows:
- test-eip6110:
requires:
- install_pyspec_test
- test-eip6988:
requires:
- install_pyspec_test
- table_of_contents
- codespell
- lint:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/run-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ jobs:
needs: [preclear,lint,codespell,table_of_contents]
strategy:
matrix:
version: ["phase0", "altair", "bellatrix", "capella", "deneb", "eip6110"]
version: ["phase0", "altair", "bellatrix", "capella", "deneb", "eip6110", "eip6988"]
steps:
- name: Checkout this repo
uses: actions/checkout@v3.2.0
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ tests/core/pyspec/eth2spec/bellatrix/
tests/core/pyspec/eth2spec/capella/
tests/core/pyspec/eth2spec/deneb/
tests/core/pyspec/eth2spec/eip6110/
tests/core/pyspec/eth2spec/eip6988/

# coverage reports
.htmlcov
Expand Down
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ MARKDOWN_FILES = $(wildcard $(SPEC_DIR)/*/*.md) \
$(wildcard $(SPEC_DIR)/_features/*/*/*.md) \
$(wildcard $(SSZ_DIR)/*.md)

ALL_EXECUTABLE_SPECS = phase0 altair bellatrix capella deneb eip6110
ALL_EXECUTABLE_SPECS = phase0 altair bellatrix capella deneb eip6110 eip6988
# The parameters for commands. Use `foreach` to avoid listing specs again.
COVERAGE_SCOPE := $(foreach S,$(ALL_EXECUTABLE_SPECS), --cov=eth2spec.$S.$(TEST_PRESET_TYPE))
PYLINT_SCOPE := $(foreach S,$(ALL_EXECUTABLE_SPECS), ./eth2spec/$S)
Expand Down
3 changes: 3 additions & 0 deletions configs/mainnet.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ DENEB_FORK_EPOCH: 18446744073709551615
# EIP6110
EIP6110_FORK_VERSION: 0x05000000 # temporary stub
EIP6110_FORK_EPOCH: 18446744073709551615
# EIP6988
EIP6988_FORK_VERSION: 0x04000000 # temporary stub
EIP6988_FORK_EPOCH: 18446744073709551615


# Time parameters
Expand Down
3 changes: 3 additions & 0 deletions configs/minimal.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ DENEB_FORK_EPOCH: 18446744073709551615
# EIP6110
EIP6110_FORK_VERSION: 0x05000001
EIP6110_FORK_EPOCH: 18446744073709551615
# EIP6988
EIP6988_FORK_VERSION: 0x04000001
EIP6988_FORK_EPOCH: 18446744073709551615


# Time parameters
Expand Down
28 changes: 23 additions & 5 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ def installPackage(package: str):
CAPELLA = 'capella'
DENEB = 'deneb'
EIP6110 = 'eip6110'
EIP6988 = 'eip6988'


# The helper functions that are used when defining constants
Expand Down Expand Up @@ -680,10 +681,22 @@ def imports(cls, preset_name: str):
from eth2spec.deneb import {preset_name} as deneb
'''

#
# EIP6988SpecBuilder
#
class EIP6988SpecBuilder(CapellaSpecBuilder):
fork: str = EIP6988

@classmethod
def imports(cls, preset_name: str):
return super().imports(preset_name) + f'''
from eth2spec.capella import {preset_name} as capella
'''


spec_builders = {
builder.fork: builder
for builder in (Phase0SpecBuilder, AltairSpecBuilder, BellatrixSpecBuilder, CapellaSpecBuilder, DenebSpecBuilder, EIP6110SpecBuilder)
for builder in (Phase0SpecBuilder, AltairSpecBuilder, BellatrixSpecBuilder, CapellaSpecBuilder, DenebSpecBuilder, EIP6110SpecBuilder, EIP6988SpecBuilder)
}


Expand Down Expand Up @@ -982,14 +995,14 @@ def finalize_options(self):
if len(self.md_doc_paths) == 0:
print("no paths were specified, using default markdown file paths for pyspec"
" build (spec fork: %s)" % self.spec_fork)
if self.spec_fork in (PHASE0, ALTAIR, BELLATRIX, CAPELLA, DENEB, EIP6110):
if self.spec_fork in (PHASE0, ALTAIR, BELLATRIX, CAPELLA, DENEB, EIP6110, EIP6988):
self.md_doc_paths = """
specs/phase0/beacon-chain.md
specs/phase0/fork-choice.md
specs/phase0/validator.md
specs/phase0/weak-subjectivity.md
"""
if self.spec_fork in (ALTAIR, BELLATRIX, CAPELLA, DENEB, EIP6110):
if self.spec_fork in (ALTAIR, BELLATRIX, CAPELLA, DENEB, EIP6110, EIP6988):
self.md_doc_paths += """
specs/altair/light-client/full-node.md
specs/altair/light-client/light-client.md
Expand All @@ -1001,7 +1014,7 @@ def finalize_options(self):
specs/altair/validator.md
specs/altair/p2p-interface.md
"""
if self.spec_fork in (BELLATRIX, CAPELLA, DENEB, EIP6110):
if self.spec_fork in (BELLATRIX, CAPELLA, DENEB, EIP6110, EIP6988):
self.md_doc_paths += """
specs/bellatrix/beacon-chain.md
specs/bellatrix/fork.md
Expand All @@ -1010,7 +1023,7 @@ def finalize_options(self):
specs/bellatrix/p2p-interface.md
sync/optimistic.md
"""
if self.spec_fork in (CAPELLA, DENEB, EIP6110):
if self.spec_fork in (CAPELLA, DENEB, EIP6110, EIP6988):
self.md_doc_paths += """
specs/capella/light-client/fork.md
specs/capella/light-client/full-node.md
Expand Down Expand Up @@ -1044,6 +1057,11 @@ def finalize_options(self):
specs/_features/eip6110/beacon-chain.md
specs/_features/eip6110/fork.md
"""
if self.spec_fork == EIP6988:
self.md_doc_paths += """
specs/_features/eip6988/beacon-chain.md
specs/_features/eip6988/fork.md
"""
if len(self.md_doc_paths) == 0:
raise Exception('no markdown files specified, and spec fork "%s" is unknown', self.spec_fork)

Expand Down
127 changes: 127 additions & 0 deletions specs/_features/eip6988/beacon-chain.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
# EIP-6988 -- The Beacon Chain

## Table of contents

<!-- TOC -->
<!-- START doctoc generated TOC please keep comment here to allow auto update -->
<!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE -->

- [Introduction](#introduction)
- [Helper functions](#helper-functions)
- [Misc](#misc)
- [Modified `compute_proposer_index`](#modified-compute_proposer_index)
- [Modified `get_beacon_proposer_index`](#modified-get_beacon_proposer_index)
- [Testing](#testing)

<!-- END doctoc generated TOC please keep comment here to allow auto update -->
<!-- /TOC -->

## Introduction

This is the beacon chain specification of [EIP-6988](https://github.com/ethereum/EIPs/pull/6988).
EIP-6988 modifies the protocol to prevent slashed validator from becoming a block proposer.

*Note:* This specification is built upon [Capella](../../capella/beacon-chain.md) and is under active development.

## Helper functions

### Misc

#### Modified `compute_proposer_index`

*Note:* Disallowing slashed validator to become a proposer is the only modification of this function.

```python
def compute_proposer_index(state: BeaconState, indices: Sequence[ValidatorIndex], seed: Bytes32) -> ValidatorIndex:
"""
Return from ``indices`` a random index sampled by effective balance.
"""
assert len(indices) > 0
MAX_RANDOM_BYTE = 2**8 - 1
i = uint64(0)
total = uint64(len(indices))
while True:
candidate_index = indices[compute_shuffled_index(i % total, total, seed)]
random_byte = hash(seed + uint_to_bytes(uint64(i // 32)))[i % 32]
effective_balance = state.validators[candidate_index].effective_balance
# [Modified in EIP6988]
slashed = state.validators[candidate_index].slashed
if effective_balance * MAX_RANDOM_BYTE >= MAX_EFFECTIVE_BALANCE * random_byte and not slashed:
Copy link
Contributor

@djrtwo djrtwo May 31, 2023

Choose a reason for hiding this comment

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

This actually has a strange side effect --- the shuffling for an epoch is no longer fixed at the start of the epoch as a slashing could remove a validator from a previously selected slot and replace them with someone else.

This isn't a deal breaker but likely breaks engineering invariants. We need to discuss with engineers and make sure to have specific tests for this (i.e. predict the epoch's shuffling, then move a coupel of blocks forward with a slashing for the next slot proposer, then show that the proposer changes)

EDIT: this is directionally the test -- test_slashed_validator_is_not_proposing_block -- but I'd want it to be a multi-block test to ensure some epoch proposer shuffling cache doesn't become an issue. That is, have a block that does the transition into the epoch, have a block that adds the slashing, then have the block of the next slot that has the changed proposer

This might also likely have interactions with VC proposer duties API which might expect epoch stability. It might actulaly ripple into a bunch of small things like that

Copy link
Contributor Author

@mkalinin mkalinin Jun 1, 2023

Choose a reason for hiding this comment

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

the shuffling for an epoch is no longer fixed at the start of the epoch as a slashing could remove a validator from a previously selected slot and replace them with someone else

Very good point! 👍

Strictly, the shuffling for an epoch is not fixed before this change was applied, and in the status quo slashing can become a reason of a proposer switch because of its economics component. This change moves this effect from probabilistic to deterministic area and because of that it should be taken with care.

EDIT: the shuffling for an epoch is actually fixed because effective_balance is updated only during epoch processing, thus, balance decrease because of slashing takes into effect only at the end of the epoch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but I'd want it to be a multi-block test to ensure some epoch proposer shuffling cache doesn't become an issue

I've added test_proposer_shuffling_changes_within_epoch

Copy link
Contributor

Choose a reason for hiding this comment

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

Strictly, the shuffling for an epoch is not fixed before this change was applied

but it is due to effective_balance only being updated at epoch boundaries! let's discuss on call today

Copy link
Member

@dapplion dapplion Jun 2, 2023

Choose a reason for hiding this comment

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

We could compute the proposers in the epoch transition and persist the result in the state. Citing the EIP motivation

The impact of the proposed fix in the case of a single slashing on Ethereum Mainnet is negligible but it becomes significant in the case of correlated slashings. For instance, a correlated slashing of 1/10th of a validator set can lead to 1/10th of missed proposals in a number of epochs after the slashing.

This PR reduces the impact to 0 slots of potential missed proposes. With my suggestion it would be reduced to max of 32 slots.

At least in Lodestar and probably in all clients, breaking dependant_root patterns will have a significant impact on the validator client's design.

EDIT: significant in time consuming but not a breaking deal. I can try to quantify it when there's bandwidth. But definitely the EIP will require more dev work than I originally imagined

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've attempted to spec out proposer shuffling in state #3405. It doesn't seem like a big change after all.

This modification requires beacon state initialization to be finished with a call to update_proposer_shuffling like it is done in initialize_beacon_state_from_eth1 and upgrade_to_6988. I am wondering about other implication that this requirement can have.

return candidate_index
i += 1
```

#### Modified `get_beacon_proposer_index`

*Note:* Modified to read proposer index from the state to ensure the output stays unaffected if proposing validator gets slashed.

```python
def get_beacon_proposer_index(state: BeaconState) -> ValidatorIndex:
"""
Return the beacon proposer index at the current slot.
"""
# [New in EIP6988]
if state.latest_block_header.slot == state.slot:
return state.latest_block_header.proposer_index

epoch = get_current_epoch(state)
seed = hash(get_seed(state, epoch, DOMAIN_BEACON_PROPOSER) + uint_to_bytes(state.slot))
indices = get_active_validator_indices(state, epoch)
return compute_proposer_index(state, indices, seed)
```

## Testing

*Note*: The function `initialize_beacon_state_from_eth1` is modified for pure EIP-6988 testing only.
Modifications include:
1. Use `EIP6988_FORK_VERSION` as the previous and current fork version.
2. Utilize the EIP-6988 `BeaconBlockBody` when constructing the initial `latest_block_header`.
3. Add `deposit_receipts_start_index` variable to the genesis state initialization.

```python
def initialize_beacon_state_from_eth1(eth1_block_hash: Hash32,
eth1_timestamp: uint64,
deposits: Sequence[Deposit],
execution_payload_header: ExecutionPayloadHeader=ExecutionPayloadHeader()
) -> BeaconState:
fork = Fork(
previous_version=EIP6988_FORK_VERSION, # [Modified in EIP6988] for testing only
current_version=EIP6988_FORK_VERSION, # [Modified in EIP6988]
epoch=GENESIS_EPOCH,
)
state = BeaconState(
genesis_time=eth1_timestamp + GENESIS_DELAY,
fork=fork,
eth1_data=Eth1Data(block_hash=eth1_block_hash, deposit_count=uint64(len(deposits))),
latest_block_header=BeaconBlockHeader(body_root=hash_tree_root(BeaconBlockBody())),
randao_mixes=[eth1_block_hash] * EPOCHS_PER_HISTORICAL_VECTOR, # Seed RANDAO with Eth1 entropy
)

# Process deposits
leaves = list(map(lambda deposit: deposit.data, deposits))
for index, deposit in enumerate(deposits):
deposit_data_list = List[DepositData, 2**DEPOSIT_CONTRACT_TREE_DEPTH](*leaves[:index + 1])
state.eth1_data.deposit_root = hash_tree_root(deposit_data_list)
process_deposit(state, deposit)

# Process activations
for index, validator in enumerate(state.validators):
balance = state.balances[index]
validator.effective_balance = min(balance - balance % EFFECTIVE_BALANCE_INCREMENT, MAX_EFFECTIVE_BALANCE)
if validator.effective_balance == MAX_EFFECTIVE_BALANCE:
validator.activation_eligibility_epoch = GENESIS_EPOCH
validator.activation_epoch = GENESIS_EPOCH

# Set genesis validators root for domain separation and chain versioning
state.genesis_validators_root = hash_tree_root(state.validators)

# Fill in sync committees
# Note: A duplicate committee is assigned for the current and next committee at genesis
state.current_sync_committee = get_next_sync_committee(state)
state.next_sync_committee = get_next_sync_committee(state)

# Initialize the execution payload header
state.latest_execution_payload_header = execution_payload_header

return state
```
Loading