From 12f6c7642c4127e8a89043faf5cf4dde088fcbc7 Mon Sep 17 00:00:00 2001 From: meows Date: Mon, 11 May 2020 15:28:44 -0500 Subject: [PATCH 1/6] downloader: refactor common ancestor algos and logic 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 --- eth/downloader/downloader.go | 48 +++++++++++++++++++++++++++++++++--- 1 file changed, 44 insertions(+), 4 deletions(-) diff --git a/eth/downloader/downloader.go b/eth/downloader/downloader.go index 2d0807194862..40ee78f90f0d 100644 --- a/eth/downloader/downloader.go +++ b/eth/downloader/downloader.go @@ -81,6 +81,7 @@ var ( errTimeout = errors.New("timeout") errEmptyHeaderSet = errors.New("empty header set by peer") errPeersUnavailable = errors.New("no peers available or all tried for download") + errNoAncestor = errors.New("no common ancestor") errInvalidAncestor = errors.New("retrieved ancestor is invalid") errInvalidChain = errors.New("retrieved hash chain is invalid") errInvalidBody = errors.New("retrieved block body is invalid") @@ -711,8 +712,10 @@ func (d *Downloader) findAncestor(p *peerConnection, remoteHeader *types.Header) localHeight = d.blockchain.CurrentBlock().NumberU64() case FastSync: localHeight = d.blockchain.CurrentFastBlock().NumberU64() - default: + case LightSync: localHeight = d.lightchain.CurrentHeader().Number.Uint64() + default: + log.Crit("unknown sync mode", "mode", d.mode) } p.log.Debug("Looking for common ancestor", "local", localHeight, "remote", remoteHeight) @@ -741,9 +744,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) { + a, err := d.findAncestorSpanSearch(p, remoteHeight, localHeight, floor) + if err == nil { + if a > localHeight { + a = localHeight + } + return a, nil + } else if err != errNoAncestor { + return 0, err + } + } + + // Ancestor not found, we need to binary search over our chain + a, err := d.findAncestorBinarySearch(p, remoteHeight, floor) + if err != nil { + return 0, err + } + if a > localHeight { + a = localHeight + } + return a, nil +} + +// findAncestorSpanSearch looks for a common ancestor by requesting a spaced set of headers between ours and theirs. +func (d *Downloader) findAncestorSpanSearch(p *peerConnection, remoteHeight, localHeight uint64, floor int64) (uint64, error) { from, count, skip, max := calculateRequestSpan(remoteHeight, localHeight) p.log.Trace("Span searching for common ancestor", "count", count, "from", from, "skip", skip) + go p.peer.RequestHeadersByNumber(uint64(from), count, skip, false) // Wait for the remote response to the head fetch @@ -794,8 +824,10 @@ func (d *Downloader) findAncestor(p *peerConnection, remoteHeader *types.Header) known = d.blockchain.HasBlock(h, n) case FastSync: known = d.blockchain.HasFastBlock(h, n) - default: + case LightSync: known = d.lightchain.HasHeader(h, n) + default: + log.Crit("unknown sync mode", "mode", d.mode) } if known { number, hash = n, h @@ -821,13 +853,19 @@ func (d *Downloader) findAncestor(p *peerConnection, remoteHeader *types.Header) p.log.Debug("Found common ancestor", "number", number, "hash", hash) return number, nil } - // Ancestor not found, we need to binary search over our chain + return 0, errNoAncestor +} + +// findAncestorBinarySearch negotiates a binary search using their reported chain. +func (d *Downloader) findAncestorBinarySearch(p *peerConnection, remoteHeight uint64, floor int64) (uint64, error) { start, end := uint64(0), remoteHeight if floor > 0 { start = uint64(floor) } p.log.Trace("Binary searching for common ancestor", "start", start, "end", end) + // Wait for the remote response to the head fetch + hash := common.Hash{} for start+1 < end { // Split our chain interval in two, and request the hash to cross check check := (start + end) / 2 @@ -867,8 +905,10 @@ func (d *Downloader) findAncestor(p *peerConnection, remoteHeader *types.Header) known = d.blockchain.HasBlock(h, n) case FastSync: known = d.blockchain.HasFastBlock(h, n) - default: + case LightSync: known = d.lightchain.HasHeader(h, n) + default: + log.Crit("unknown sync mode", "mode", d.mode) } if !known { end = check From db63f1ee420f86bcf85fb8c6bb34e88ac10cddc9 Mon Sep 17 00:00:00 2001 From: meows Date: Mon, 11 May 2020 15:30:23 -0500 Subject: [PATCH 2/6] downloader: only attempt common ancestor negotiation if > 0 local head Eliminates a redundant negotiation if we can already deduce that our only common block is genesis. Signed-off-by: meows --- eth/downloader/downloader.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/eth/downloader/downloader.go b/eth/downloader/downloader.go index 40ee78f90f0d..6bd4706915d7 100644 --- a/eth/downloader/downloader.go +++ b/eth/downloader/downloader.go @@ -441,10 +441,18 @@ func (d *Downloader) syncWithPeer(p *peerConnection, hash common.Hash, td *big.I } height := latest.Number.Uint64() - origin, err := d.findAncestor(p, latest) - if err != nil { - return err + var origin = uint64(0) + + if d.blockchain != nil && + d.blockchain.CurrentHeader() != nil && + d.blockchain.CurrentHeader().Number != nil && + d.blockchain.CurrentHeader().Number.Uint64() > 0 { + origin, err = d.findAncestor(p, latest) + if err != nil { + return err + } } + d.syncStatsLock.Lock() if d.syncStatsChainHeight <= origin || d.syncStatsChainOrigin > origin { d.syncStatsChainOrigin = origin From ca59775e1aaa62e904a3057f47efd706e60abc79 Mon Sep 17 00:00:00 2001 From: meows Date: Mon, 11 May 2020 15:44:39 -0500 Subject: [PATCH 3/6] downloader: attempt span search if <= (+or equal) to maxheaderfetch Signed-off-by: meows --- eth/downloader/downloader.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eth/downloader/downloader.go b/eth/downloader/downloader.go index 6bd4706915d7..a13afbb31afb 100644 --- a/eth/downloader/downloader.go +++ b/eth/downloader/downloader.go @@ -753,7 +753,7 @@ 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) { + if remoteHeight-localHeight <= uint64(MaxHeaderFetch) { a, err := d.findAncestorSpanSearch(p, remoteHeight, localHeight, floor) if err == nil { if a > localHeight { From 9a9d2aadca61ee56cae1a84f6a3f2f502bca1e76 Mon Sep 17 00:00:00 2001 From: meowsbits <45600330+meowsbits@users.noreply.github.com> Date: Wed, 27 May 2020 15:19:02 -0500 Subject: [PATCH 4/6] downloader: use Go conventional if (remove 'else') Co-authored-by: Martin Holst Swende Signed-off-by: meows --- eth/downloader/downloader.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/eth/downloader/downloader.go b/eth/downloader/downloader.go index a13afbb31afb..07d2c0f9c9b8 100644 --- a/eth/downloader/downloader.go +++ b/eth/downloader/downloader.go @@ -760,7 +760,8 @@ func (d *Downloader) findAncestor(p *peerConnection, remoteHeader *types.Header) a = localHeight } return a, nil - } else if err != errNoAncestor { + } + if err != errNoAncestor { return 0, err } } From 58dd36b66f6ab27e93527869a417e8edaab3dd31 Mon Sep 17 00:00:00 2001 From: meows Date: Wed, 27 May 2020 15:43:44 -0500 Subject: [PATCH 5/6] downloader: use absolute value of remote vs. local height 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 --- eth/downloader/downloader.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/eth/downloader/downloader.go b/eth/downloader/downloader.go index 07d2c0f9c9b8..0853e8da487b 100644 --- a/eth/downloader/downloader.go +++ b/eth/downloader/downloader.go @@ -20,6 +20,7 @@ package downloader import ( "errors" "fmt" + "math" "math/big" "sync" "sync/atomic" @@ -753,7 +754,7 @@ 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) { + if math.Abs(float64(remoteHeight-localHeight)) <= float64(MaxHeaderFetch) { a, err := d.findAncestorSpanSearch(p, remoteHeight, localHeight, floor) if err == nil { if a > localHeight { From 4122416ba30e248d473bf5d67930168bc7608142 Mon Sep 17 00:00:00 2001 From: meows Date: Wed, 24 Jun 2020 06:24:41 -0500 Subject: [PATCH 6/6] downloader: handle light sync case as findAncestor condition 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 --- eth/downloader/downloader.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/eth/downloader/downloader.go b/eth/downloader/downloader.go index 0853e8da487b..34f267736637 100644 --- a/eth/downloader/downloader.go +++ b/eth/downloader/downloader.go @@ -444,10 +444,8 @@ func (d *Downloader) syncWithPeer(p *peerConnection, hash common.Hash, td *big.I var origin = uint64(0) - if d.blockchain != nil && - d.blockchain.CurrentHeader() != nil && - d.blockchain.CurrentHeader().Number != nil && - d.blockchain.CurrentHeader().Number.Uint64() > 0 { + if (d.mode != LightSync && d.blockchain.CurrentHeader().Number.Uint64() > 0) || + d.lightchain.CurrentHeader().Number.Uint64() > 0 { origin, err = d.findAncestor(p, latest) if err != nil { return err