-
Notifications
You must be signed in to change notification settings - Fork 11
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
untrusted peer downloader validation #67
untrusted peer downloader validation #67
Conversation
les/fetcher.go
Outdated
for n != nil { | ||
if td = f.chain.GetTd(n.hash, n.number); td != nil { | ||
if n.hash == h.Hash() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a few comments here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case handling of lastTrustedHeader
is okay document how it is ensured. Otherwise later maintainer could be sceptical too.
les/fetcher.go
Outdated
@@ -523,6 +524,7 @@ func (f *lightFetcher) newFetcherDistReqForSync(bestHash common.Hash) *distReq { | |||
go func() { | |||
p := dp.(*peer) | |||
p.Log().Debug("Synchronisation started") | |||
f.lastTrustedHeader = f.chain.CurrentHeader() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safe setting this shared field asynchronously in goroutines?
les/fetcher.go
Outdated
for last != nil { | ||
if f.isTrustedHash(last.hash) { | ||
current := f.chain.CurrentHeader() | ||
for current != nil || current != f.lastTrustedHeader { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is where I meant the comparison of the lastTrustedHeader
set async in a goroutine.
@themue fixed |
@b00ris Seen my comment regarding setting it inside the goroutine? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now. Thanks.
closes #63