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

Allow for null reorg distance #127

Closed
wants to merge 1 commit into from

Conversation

paulhauner
Copy link
Contributor

We've been having a conversation about the reorg event depth in sigp/lighthouse#2090.

The problem is that computing the re-org distance involves finding the highest common ancestor of two blocks. This is a notoriously difficult problem in blockchains which is resistant to optimization and typically involves an O(n) walk back through the chain.

In Eth2, computing that re-org distance is quite easy if the re-org is not deeper than SLOTS_PER_HISTORICAL_ROOT (8,192) blocks, since clients probably have all those block roots sitting in memory (they're in the BeaconState). However, once we go past 8,192 we're entering the territory of reading BeaconStates from the database or doing block-by-block reads from the DB.

My concern is that if the chain is very unhealthy and the head is bouncing around then the BN is committed to constantly computing the correct re-org depth for consumers of the event stream. In this PR I've explicitly added the ability for nodes to return null and save themselves from entering this O(n) search.

@ajsutton
Copy link
Contributor

I believe Teku will just report the finalized slot as the common ancestor if the fork is longer than SLOTS_PER_HISTORICAL_ROOT. It seemed like enough of a weird corner case that just reporting the worst possible slot seemed like a reasonable answer even if it's not entirely accurate.

I'm not against allowing null here but I suspect it's a case users are not going to handle because it's such an unexpected corner case. Reporting the finalized slot I suspect would just work without needing any special case handling.
But I also doubt such an 8193 block re-org will ever happen so probably not a big deal.

@paulhauner
Copy link
Contributor Author

I believe Teku will just report the finalized slot as the common ancestor if the fork is longer than SLOTS_PER_HISTORICAL_ROOT. It seemed like enough of a weird corner case that just reporting the worst possible slot seemed like a reasonable answer even if it's not entirely accurate.

Oh yeah, this is a good idea. It's important to report the finalized slot of the old head, since (in LH at least) it's possible for a new head to have an arbitrarily higher finalized slot. We had considered just returning a distance of the entire chain, which is a similar approach (e.g., overkill but safe).

I'll go back to the drawing board and potentially open a new PR that makes it explicit that it's a maximum re-org distance.

@paulhauner paulhauner closed this Feb 25, 2021
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

Successfully merging this pull request may close these issues.

2 participants