search

ethereum / consensus-spec-tests

Common tests for the Ethereum proof-of-stake consensus layer
MIT License
75 stars 24 forks source link

Add more optimistic sync tests #30

Open mkalinin opened 2 years ago

mkalinin commented 2 years ago

Issue

We need tests covering more sophisticated scenarios of Optimistic Sync.

Details

Follow up to https://github.com/ethereum/consensus-specs/pull/2982 Test format https://github.com/ethereum/consensus-specs/pull/2965

An idea by @potuz is to have multiple block tree branches that a client is syncing optimistically with, then all these branches but one should be invalidated making client to jump from one invalid branch to the other (LMD weights should be set accordingly, with an edge case where we have the same weight for valid and for invalid branch). Client should end up on a valid branch as canonical chain.

potuz commented 2 years ago

Here's another weird case:

1 Block A, SYNCING, it's head

  1. Block A <- B SYNCING, is head 3 Block C arrives, also SYNCING (ACCEPTED), forks and B is still head. 
    A <- B
\_____C

    Block D arrives SYNCING, but it has enough attestations to C so that C becomes head (it's a self forking block)

    A <- B <--- D
\______C

    Block E arrives its INVALID (LHV: B). It includes a lot of attestations to D so that D would have become head if this block would have been VALID

    A <- B <--- D <--- E
\______C

    Check that C is head.

There are a few things that this test covers, 1) that clients can handle self forking blocks, 2) that they are calling FCU with the right head block and not the just imported one.

There are other checks included here that are a little more subtle: invalid blocks may include valid attestations, but they may be treated different in applications. In the example above, the attestations included by D were all counted in C's weight. That weight will not be removed when the chain D <- E is pruned. The attestations included in E on the other hand is a bit trickier: it's not clear to me if all implementations would agree here, some may process attestations in blocks before realizing the block is INVALID, others may realize it's INVALID and stop processing and ditching attestations from it or not.

mkalinin commented 2 years ago

ditching attestations from it or not

I don't know for sure but it sounds like if attestations are applied to the fork choice it's really hard (implementation-wise) to revert votes back to the previous state. If so it would mean that attestations made by validators to valid block B and then overridden by attestations made to invalid block D by the same validators won't be weighed in when A <- B chain balance is computed. I don't think it's an issue though, but it would be good to have clients on the same page with respect to this