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

Add more optimistic sync tests #30

Open
Tracked by #3489
mkalinin opened this issue Sep 12, 2022 · 2 comments
Open
Tracked by #3489

Add more optimistic sync tests #30

mkalinin opened this issue Sep 12, 2022 · 2 comments

Comments

@mkalinin
Copy link

Issue

We need tests covering more sophisticated scenarios of Optimistic Sync.

Details

Follow up to ethereum/consensus-specs#2982
Test format ethereum/consensus-specs#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
Copy link

potuz commented Sep 13, 2022

Here's another weird case:

1 Block A, SYNCING, it's head
2. 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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants
@mkalinin @potuz and others