EIP-6988: Slashed validator cannot be elected as a block proposer #3371

mkalinin · 2023-05-18T11:38:24Z

Substitutes #3175 by moving the proposed change to a _features/ folder under the specified EIP number (ethereum/EIPs#6988).

This PR adds new tests for earlier phases like Phase0 and Altair, and makes tiny modification (not affecting test vectors) to already existing tests to work properly around the proposed change. It does also add a couple of unittests to the validator suite.

What's not done:

eip6988 isn't enabled in fork tests as it doesn't considered as a standalone fork and assumed to be a part of Deneb
test generators aren't enable for this feature as well as no new tests producing test vectors are introduced

.circleci/config.yml

Previously the number of subnets is equal to MAX_BLOBS_PER_BLOCK which specifies the number of blobs per block. This commit now makes the number of subnets equal to BLOB_SIDECAR_SUBNET_COUNT instead. The advantage of doing this is that we can change MAX_BLOBS_PER_BLOCK without worrying about the p2p network structure and the number of subnets.

mkalinin · 2023-05-19T06:50:39Z

This fix is based on Capella with the intuition that Capella-based features comprise Deneb. I don't mind to re-base it to Deneb if that is required

hwwhww

Generally looks good! 👍

Some test case ideas:

[sanity/block] a proposer slashes themselves in the block
[sanity/block] slash most validators and try to find the case where compute_proposer_index hits effective_balance * MAX_RANDOM_BYTE >= MAX_EFFECTIVE_BALANCE * random_byte and slashed.

hwwhww · 2023-05-22T13:16:33Z

tests/core/pyspec/eth2spec/test/phase0/unittests/validator/test_validator_unittest.py

+@with_phases([PHASE0, ALTAIR, BELLATRIX, CAPELLA])
+@spec_state_test
+def test_slashed_validator_elected_for_proposal(spec, state):
+    proposer_index = spec.get_beacon_proposer_index(state)
+    state.validators[proposer_index].slashed = True
+
+    assert spec.get_beacon_proposer_index(state) == proposer_index


This test case actually works for DENEB and EIP-6988 now because the default state is genesis and state.latest_block_header.slot is 0.

We should skip one slot first.

Suggested change

@with_phases([PHASE0, ALTAIR, BELLATRIX, CAPELLA])

@spec_state_test

def test_slashed_validator_elected_for_proposal(spec, state):

proposer_index = spec.get_beacon_proposer_index(state)

state.validators[proposer_index].slashed = True

assert spec.get_beacon_proposer_index(state) == proposer_index

@with_phases([PHASE0, ALTAIR, BELLATRIX, CAPELLA, DENEB])

@spec_state_test

def test_slashed_validator_elected_for_proposal(spec, state):

next_slot(spec, state)

proposer_index = spec.get_beacon_proposer_index(state)

state.validators[proposer_index].slashed = True

assert spec.get_beacon_proposer_index(state) == proposer_index

Good catch! I've fixed the test, didn't add DENEB as a target because this change should eventually be applied to Deneb and this test will fail then

mkalinin · 2023-05-24T11:03:23Z

Generally looks good! 👍

Some test case ideas:

[sanity/block] a proposer slashes themselves in the block

I think that this is already checked by test_proposer_index_slashed and test_slashed_and_proposer_index_the_same.

[sanity/block] slash most validators and try to find the case where compute_proposer_index hits effective_balance * MAX_RANDOM_BYTE >= MAX_EFFECTIVE_BALANCE * random_byte and slashed.

Good idea, will add this test!

mkalinin · 2023-05-31T12:50:36Z

I have added the following test cases to sanity/blocks:

All phases
- test_proposer_slashes_itself
- test_proposer_with_lack_of_effective_balance
Specific to EIP-6988
- test_slashed_proposer_with_lack_of_effective_balance
- test_slashed_validator_is_not_proposing_block

djrtwo · 2023-05-31T21:55:05Z

specs/_features/eip6988/beacon-chain.md

+        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:


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

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

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

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

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

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.

arnetheduck · 2023-06-02T14:31:07Z

Why not simply drop the block (in block validation for example)? This avoids the epoch stability issues..

djrtwo · 2023-06-02T16:48:01Z

Why not simply drop the block (in block validation for example)? This avoids the epoch stability issues..

We already do drop the block if the proposer is slashed -- https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/beacon-chain.md#block-header assert not proposer.slashed. This PR is an attempt to not have this have a huge impact on missed porposals in the event that there is a mass slashing (e.g. 50% slashed validators results in a 50% reduction in blocks until the exit queue is cleared).

Remembering when a validator is slashed or doing something like @dapplion proposes above (#3371 (comment)) could meet the needs without breaking the pre-epoch shuffling stablility invariant

dapplion · 2023-06-15T14:27:20Z

Breaking the shuffling invariant may break some cache designs (or make them more expensive) around proposer shufflings relevant to process incoming beacon_block gossip messages.

consensus-specs/specs/phase0/p2p-interface.md

Lines 345 to 346 in 834f6f7

    
           - _[REJECT]_ The block is proposed by the expected `proposer_index` for the block's slot 
        
             in the context of the current shuffling (defined by `parent_root`/`slot`).

It does for Lodestar, and at least also Lighthouse, which has a proposer shuffling cache indexed by dependant_root https://github.com/sigp/lighthouse/blob/c547a11b0da48db6fdd03bca2c6ce2448bbcc3a9/beacon_node/beacon_chain/src/block_verification.rs#L780-L797

        let proposer_opt = chain
            .beacon_proposer_cache
            .lock()
            .get_slot::<T::EthSpec>(proposer_shuffling_decision_block, block.slot());

dapplion · 2023-07-25T10:38:34Z

Noting that this EIP is future incompatible with Whisk SSLE.

Whisk selects a pool of candidates 256 epochs in advance to the first elected candidate proposing a block. If there's a mass slashing event during a shuffling phase, it is not possible to know which elected candidate trackers correspond to slashed proposers. Only when said proposer publishes its block the network can assert that the slot is assigned to a slashed proposer and thus reject the block.

mkalinin added 3 commits May 17, 2023 19:39

Add eip6988

cef5e7c

Update tests with eip6988

f47defb

Enable eip6988 on CI

28628b8

mkalinin mentioned this pull request May 18, 2023

Not allow slashed validator to become a proposer #3175

Closed

jtraglia and others added 3 commits May 18, 2023 19:33

Add comment about zero elements in batch verification (ethereum#3367)

4da3fe0

Fix CircleCI Python version

8c5777c

Nitpick: blob -> blob sidecar

1f5e17d

hwwhww reviewed May 18, 2023

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

hwwhww and others added 9 commits May 18, 2023 23:39

Fix image version

974019c

Refactor run_generator

405391e

Try multiprocessing

4b0a7d2

Fix and set to MODE_MULTIPROCESSING

620ac52

Add settings.py of testgen

b6ee834

Use SubnetID instead of uint64

b9b2d5e

Clarify blob subnets

0a09f79

Merge branch 'dev' into eip6988

0ad3972

hwwhww reviewed May 22, 2023

View reviewed changes

Fix proposer election tests

4bdc78f

Add sanity block tests

8740967

djrtwo reviewed May 31, 2023

View reviewed changes

mkalinin mentioned this pull request Jun 1, 2023

Consensus-layer Call 110 ethereum/pm#796

Closed

Add more slashing tests

d60ae65

mkalinin mentioned this pull request Jun 7, 2023

EIP-6988: keep proposer shuffling in state #3405

Draft

mkalinin mentioned this pull request Jun 14, 2023

Consensus-layer Call 111 ethereum/pm#809

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP-6988: Slashed validator cannot be elected as a block proposer #3371

EIP-6988: Slashed validator cannot be elected as a block proposer #3371

mkalinin commented May 18, 2023

mkalinin commented May 19, 2023

hwwhww left a comment

hwwhww May 22, 2023

mkalinin May 24, 2023

mkalinin commented May 24, 2023

mkalinin commented May 31, 2023

djrtwo May 31, 2023 •

edited

Loading

mkalinin Jun 1, 2023 •

edited

Loading

mkalinin Jun 1, 2023

djrtwo Jun 1, 2023

dapplion Jun 2, 2023 •

edited

Loading

mkalinin Jun 7, 2023

arnetheduck commented Jun 2, 2023

djrtwo commented Jun 2, 2023

dapplion commented Jun 15, 2023 •

edited

Loading

dapplion commented Jul 25, 2023

EIP-6988: Slashed validator cannot be elected as a block proposer #3371

Are you sure you want to change the base?

EIP-6988: Slashed validator cannot be elected as a block proposer #3371

Conversation

mkalinin commented May 18, 2023

mkalinin commented May 19, 2023

hwwhww left a comment

Choose a reason for hiding this comment

hwwhww May 22, 2023

Choose a reason for hiding this comment

mkalinin May 24, 2023

Choose a reason for hiding this comment

mkalinin commented May 24, 2023

mkalinin commented May 31, 2023

djrtwo May 31, 2023 • edited Loading

Choose a reason for hiding this comment

mkalinin Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

mkalinin Jun 1, 2023

Choose a reason for hiding this comment

djrtwo Jun 1, 2023

Choose a reason for hiding this comment

dapplion Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

mkalinin Jun 7, 2023

Choose a reason for hiding this comment

arnetheduck commented Jun 2, 2023

djrtwo commented Jun 2, 2023

dapplion commented Jun 15, 2023 • edited Loading

dapplion commented Jul 25, 2023

djrtwo May 31, 2023 •

edited

Loading

mkalinin Jun 1, 2023 •

edited

Loading

dapplion Jun 2, 2023 •

edited

Loading

dapplion commented Jun 15, 2023 •

edited

Loading