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

eth/downloader: fix light client cht binary search issue #18196

Merged
merged 1 commit into from
Nov 28, 2018

Conversation

karalabe
Copy link
Member

The binary search based ancestor lookup doesn't work for light clients. The reason is that a light client will inject a random header (the one stored in the CHT) and will want to sync from that point onward. A binary search will however try to find the common ancestor from [HEAD-90K, REMOTE_HEAD]. Mapping that interval to our header chain would look something like [nil, nil, nil, nil, nil, CHT_HEADER, nil]. You can't binary search on this.

The only proper solution is to start from the current header and set the floor of the binary search either to the HEAD-90K limit, or to the earliest ancestor that we do have in our database. Unfortunately there's no way to actually find that apart from traversing the chain backward, so that's what this PR does. It also caches the result so we don't do it on every sync cycle.

PS: The reason this worked previously is because the ancestor lookup shortcut always retrieved the current header too. A previous polish of this code however resulted in the current head never being retrieved, only it's parent.

@karalabe karalabe added this to the 1.8.19 milestone Nov 28, 2018

// If we're doing a light sync, ensure the floor doesn't go below the CHT, as
// all headers before that point will be missing.
if d.mode == LightSync {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be conditional on LightSync? Other sync modes might set a non-zero genesis in the future.

Copy link
Contributor

@holiman holiman Nov 28, 2018

Choose a reason for hiding this comment

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

But other modes don't suffer from gaps in the ancestor list, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently only light sync is broken. The reason I named it genesis was indeed so we may move the genesis block for full/fast sync too eventually, but that would be a lot more change and potentially breaking at it. I didn't want to screw around adding more stuff to a pre-release hotfix.

Copy link
Member Author

Choose a reason for hiding this comment

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

And we currently don't support non-0 genesis blocks, so we can't even test it that it works. Better to be restrictive imho than to accidentally break something.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM. I might have picked another name than genesis, which already have a well-defined meaning (maybe bootstrap or chtblock), but that's just a nitpick.

@karalabe karalabe merged commit 8fdbbef into ethereum:master Nov 28, 2018
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.

3 participants