refactor downloader common ancestor span/binary search fns #21063

meowsbits · 2020-05-11T21:11:56Z

breaks these functions (SpanSearch, BinarySearch) off into their own functions
only attempt any common ancestor negotiation if we're not at genesis
try SpanSearch only if we're within MaxHeaderFetch range of the peer's reported head

This breaks the common ancestor search functions into SpanSearch and BinarySearch functions, ensuring that a negotiated common ancestor is never above our local head. It makes switch cases conditional on sync mode use LightSync explicitly, which makes the code more readable (instead of relying on LightSync as tacit default). Signed-off-by: meows <b5c6@protonmail.com>

Eliminates a redundant negotiation if we can already deduce that our only common block is genesis. Signed-off-by: meows <b5c6@protonmail.com>

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

holiman · 2020-05-27T17:34:55Z

eth/downloader/downloader.go

 		localHeight = d.lightchain.CurrentHeader().Number.Uint64()
+	default:


You've added these in three places, I can't say I think it's a worthwhile improvement, really. Mind reverting that and make the diff smaller?

The reason I like this is that it removes what I would call "incidental" logic, where a reader/developer has to know and use an implicit context; ie. that there are 3 and only 3 downloader sync modes and what they are. And then read between the lines to understand that the d.lightchain assignment happens tacitly in the case of the downloader's LightSync configuration (despite that LightSync isn't the application default). IMO It's taking advantage of a terse logic pattern at the price of readability, and I would prefer explicit readability at the cost of a few lines.

Hypothetically, with the proposed change, if a developer wants to try adding a 4th downloader sync mode on a given weekend, they won't have to spend any time working to understand a mysterious bug that will turn out to be the switch statement not complaining about an unhandled case.

Of course, you're the owner here, and I'm willing to concede the change with no hard feelings, but those are my two cents.

holiman · 2020-05-27T17:40:35Z

eth/downloader/downloader.go

@@ -741,9 +752,36 @@ func (d *Downloader) findAncestor(p *peerConnection, remoteHeader *types.Header)
 		}
 	}

+	// Only attempt span search if local head is "close" to reported remote
+	if remoteHeight-localHeight <= uint64(MaxHeaderFetch) {


Are we, at this point, certain that the remoteHeight is higher than localHeight? What if it's a lower height but higher TD? Shouldn't this check use abs ?

Yeap, I think you're correct. Fixed with 58dd36b

holiman · 2020-05-27T17:41:35Z

eth/downloader/downloader.go

+		a, err := d.findAncestorSpanSearch(p, remoteHeight, localHeight, floor)
+		if err == nil {
+			if a > localHeight {
+				a = localHeight


Hm, what happens here? The found ancestor is above our local height?
Ah, that's when we're aware of a higher block, but we don't consider it canon, for TD reasons...

eth/downloader/downloader.go

Co-authored-by: Martin Holst Swende <martin@swende.se> Signed-off-by: meows <b5c6@protonmail.com>

Span search depends on absolute distance between remote and local heights. This fixes the case where remote head is greater than MaxHeaderFetch beneath local height. Signed-off-by: meows <b5c6@protonmail.com>

meowsbits · 2020-05-27T20:46:15Z

Force pushed to squash a tiny + } patch fixing the suggestion committed via #21063 (comment)

holiman · 2020-06-24T07:23:02Z

eth/downloader/downloader.go

+	if d.blockchain != nil &&
+		d.blockchain.CurrentHeader() != nil &&
+		d.blockchain.CurrentHeader().Number != nil &&
+		d.blockchain.CurrentHeader().Number.Uint64() > 0 {


This looks extremely defensive. Why did you add all these checks?
It looks pretty moot anyway -- if d.blockchain is nul, you'll get a panic later on when this falls through to e.g localHeight = d.blockchain.CurrentBlock().NumberU64

Thanks for pointing this out. Not one of my prouder moments 🤒 . The original reasoning was that les tests failed otherwise, since d.blockchain would be nil. The middle two conditions I can only explain as results of frustrated debugging, and they are unnecessary.

What is currently unaddressed is that light mode is completely excluded from the condition, so common ancestor negotiation would never be attempted during Light sync. Will push a change to fix this very shortly.

See 4122416

In addition to being quite terribly ugly, this condition set erroneously excluded the light sync case entirely. This change handles d._chain cases depending on respective sync modes, and fixes the case that light sync mode would never actually use the findAncestor negotiations. Signed-off-by: meows <b5c6@protonmail.com>

holiman

I think this is ok, but I also think it needs some actual runtime in production for testing.

adamschmideg · 2020-09-24T08:33:22Z

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.

meowsbits · 2020-09-30T13:50:32Z

@adamschmideg It's 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.

holiman · 2020-10-20T08:29:46Z

We don't really feel comfortable with all these changes at once. Could you rewrite this PR to be only the modularization of the findAncestorXX-methods? And leave out any changes in semantics.
Then a follow-up PR could contain changes that modify behaviour.

meowsbits · 2020-10-23T12:56:28Z

@holiman Will do. Please see #21744 for the first pure-refactoring change set, which only does the job of extract-to-functions.

I am going to close this, since it will be superseded by the bespoke and forthcoming PRs.

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

meowsbits added 3 commits May 11, 2020 16:06

downloader: only attempt common ancestor negotiation if > 0 local head

db63f1e

Eliminates a redundant negotiation if we can already deduce that our only common block is genesis. Signed-off-by: meows <b5c6@protonmail.com>

downloader: attempt span search if <= (+or equal) to maxheaderfetch

ca59775

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

meowsbits requested review from holiman, karalabe and rjl493456442 as code owners May 11, 2020 21:11

holiman reviewed May 27, 2020

View reviewed changes

meowsbits and others added 2 commits May 27, 2020 15:35

downloader: use Go conventional if (remove 'else')

9a9d2aa

Co-authored-by: Martin Holst Swende <martin@swende.se> Signed-off-by: meows <b5c6@protonmail.com>

downloader: use absolute value of remote vs. local height

58dd36b

Span search depends on absolute distance between remote and local heights. This fixes the case where remote head is greater than MaxHeaderFetch beneath local height. Signed-off-by: meows <b5c6@protonmail.com>

meowsbits force-pushed the feat/downloader-refactor branch from e017bb4 to 58dd36b Compare May 27, 2020 20:44

adamschmideg added the status:triage label Jun 9, 2020

adamschmideg assigned holiman Jun 23, 2020

adamschmideg removed the status:triage label Jun 23, 2020

holiman reviewed Jun 24, 2020

View reviewed changes

holiman approved these changes Jun 24, 2020

View reviewed changes

adamschmideg added the status:triage label Jul 21, 2020

adamschmideg assigned karalabe Jul 28, 2020

adamschmideg removed the status:triage label Jul 28, 2020

adamschmideg added the status:triage label Sep 24, 2020

adamschmideg unassigned holiman Sep 24, 2020

adamschmideg added need:more-information and removed status:triage labels Sep 24, 2020

adamschmideg added status:triage and removed need:more-information labels Oct 12, 2020

adamschmideg unassigned karalabe Oct 20, 2020

adamschmideg added pr:change-requested and removed status:triage labels Oct 20, 2020

meowsbits mentioned this pull request Oct 23, 2020

downloader: extract findAncestor search functions #21744

Merged

meowsbits closed this Oct 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor downloader common ancestor span/binary search fns #21063

refactor downloader common ancestor span/binary search fns #21063

meowsbits commented May 11, 2020

holiman May 27, 2020

meowsbits May 27, 2020 •

edited

Loading

holiman May 27, 2020

meowsbits May 27, 2020

holiman May 27, 2020

meowsbits commented May 27, 2020 •

edited

Loading

holiman Jun 24, 2020

meowsbits Jun 24, 2020

meowsbits Jun 24, 2020

holiman left a comment

adamschmideg commented Sep 24, 2020

meowsbits commented Sep 30, 2020

holiman commented Oct 20, 2020

meowsbits commented Oct 23, 2020 •

edited

Loading

		localHeight = d.lightchain.CurrentHeader().Number.Uint64()
		default:

refactor downloader common ancestor span/binary search fns #21063

refactor downloader common ancestor span/binary search fns #21063

Conversation

meowsbits commented May 11, 2020

holiman May 27, 2020

Choose a reason for hiding this comment

meowsbits May 27, 2020 • edited Loading

Choose a reason for hiding this comment

holiman May 27, 2020

Choose a reason for hiding this comment

meowsbits May 27, 2020

Choose a reason for hiding this comment

holiman May 27, 2020

Choose a reason for hiding this comment

meowsbits commented May 27, 2020 • edited Loading

holiman Jun 24, 2020

Choose a reason for hiding this comment

meowsbits Jun 24, 2020

Choose a reason for hiding this comment

meowsbits Jun 24, 2020

Choose a reason for hiding this comment

holiman left a comment

Choose a reason for hiding this comment

adamschmideg commented Sep 24, 2020

meowsbits commented Sep 30, 2020

holiman commented Oct 20, 2020

meowsbits commented Oct 23, 2020 • edited Loading

meowsbits May 27, 2020 •

edited

Loading

meowsbits commented May 27, 2020 •

edited

Loading

meowsbits commented Oct 23, 2020 •

edited

Loading