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

downloader: extract findAncestor search functions #21744

Merged
merged 3 commits into from
Jan 20, 2021

Conversation

meowsbits
Copy link
Contributor

This is a simple refactoring, extracting common ancestor
negotiation logic to named functions.

This commit addresses the concern at the following link:
#21063 (comment)

Can you give us some explanation why refactor? This PR touches a delicate part of the code, so unless there is a good reason to do so, we would rather not change it now.

[This is] a refactoring intended to help repay some technical debt, which may be contributing, at least in part, to the case of this code's delicateness. The findAncestor method currently houses two algorithms: span search and binary search. These are clearly differentiable and logically distinct tasks. The tests currently rely on testing findAncestor at large, which includes the work of testing not only both algorithms, but the contextual logic around them as well.

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.

Looks good to me, aside from a nitpick

eth/downloader/downloader.go Outdated Show resolved Hide resolved
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

@holiman
Copy link
Contributor

holiman commented Nov 24, 2020

I'll merge this after #21482, to not add more rebase-work on Peter

@holiman holiman self-assigned this Nov 24, 2020
meowsbits and others added 3 commits January 20, 2021 19:41
This is a simple refactoring, extracting common ancestor
negotiation logic to named functions.

This commit addresses the concern at the following link:
ethereum#21063 (comment)

---

This is a combination of 6 commits.
This is the 1st commit message:

downloader: extract span search to function

This is a plain refactoring, extracting span search
logic to its own function.

An error 'errNoAncestor' is introduced to handle
the case when the function does not find a common
ancestor.

Signed-off-by: meows <b5c6@protonmail.com>

downloader: extract binary search to function

This is a simple refactoring extracting
the binary search for common ancestor logic
to its own function.

A tweak was made to the 'floor' variable(s) which
I'm still not happy with, but logic is functional --
passing tests -- for now.

Signed-off-by: meows <b5c6@protonmail.com>

downloader: fixup floor type and logic to minimize change

This minimizes the diff, keeping the logic as nearly
the same as possible, limiting the span and binary search
logic extraction to functions as cleanly as possible.

Signed-off-by: meows <b5c6@protonmail.com>

downloader: remove limiting ancestor to localheight

This is logic not existing at ethereum/go-ethereum master,
and thus is not pertinent to a just-refactor.

Signed-off-by: meows <b5c6@protonmail.com>

downloader: tweaks to get the diff as clean as possible

Signed-off-by: meows <b5c6@protonmail.com>

downloader: rename error noAncestor -> noAncestorFound

Signed-off-by: meows <b5c6@protonmail.com>

sq
This is preferred to the value equality check.

Co-authored-by: Martin Holst Swende <martin@swende.se>
@holiman holiman force-pushed the feat/downloader-refactor2-squash branch from b956722 to 5f00c08 Compare January 20, 2021 18:42
@holiman holiman merged commit 81bf9f9 into ethereum:master Jan 20, 2021
@holiman holiman added this to the 1.10.0 milestone Jan 20, 2021
bulgakovk pushed a commit to bulgakovk/go-ethereum that referenced this pull request Jan 26, 2021
This is a simple refactoring, extracting common ancestor
negotiation logic to named function
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