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.

Sign up for GitHub

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

Jump to bottom

refactor downloader common ancestor span/binary search fns #21063

Closed
wants to merge 6 commits into from
Closed

refactor downloader common ancestor span/binary search fns #21063

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
12f6c76
downloader: refactor common ancestor algos and logic
meowsbits May 11, 2020
db63f1e
downloader: only attempt common ancestor negotiation if > 0 local head
meowsbits May 11, 2020
ca59775
downloader: attempt span search if <= (+or equal) to maxheaderfetch
meowsbits May 11, 2020
9a9d2aa
downloader: use Go conventional if (remove 'else')
meowsbits May 27, 2020
58dd36b
downloader: use absolute value of remote vs. local height
meowsbits May 27, 2020
4122416
downloader: handle light sync case as findAncestor condition
meowsbits Jun 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 55 additions & 7 deletions eth/downloader/downloader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ package downloader
import (
"errors"
"fmt"
"math"
"math/big"
"sync"
"sync/atomic"
Expand Down Expand Up @@ -81,6 +82,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")
Expand Down Expand Up @@ -440,10 +442,16 @@ 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.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
}
}

d.syncStatsLock.Lock()
if d.syncStatsChainHeight <= origin || d.syncStatsChainOrigin > origin {
d.syncStatsChainOrigin = origin
Expand Down Expand Up @@ -711,8 +719,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:
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@meowsbits meowsbits May 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

log.Crit("unknown sync mode", "mode", d.mode)
}
p.log.Debug("Looking for common ancestor", "local", localHeight, "remote", remoteHeight)

Expand Down Expand Up @@ -741,9 +751,37 @@ func (d *Downloader) findAncestor(p *peerConnection, remoteHeader *types.Header)
}
}

// Only attempt span search if local head is "close" to reported remote
if math.Abs(float64(remoteHeight-localHeight)) <= float64(MaxHeaderFetch) {
a, err := d.findAncestorSpanSearch(p, remoteHeight, localHeight, floor)
if err == nil {
if a > localHeight {
a = localHeight
Copy link
Contributor

Choose a reason for hiding this comment

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

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...

}
return a, nil
}
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
Expand Down Expand Up @@ -794,8 +832,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
Expand All @@ -821,13 +861,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
Expand Down Expand Up @@ -867,8 +913,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
Expand Down