-
Notifications
You must be signed in to change notification settings - Fork 20.7k
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
eth: update highest block we know during the sync if a higher was found #16283
Conversation
ecdba2e
to
ea04ad0
Compare
internal/ethapi/api.go
Outdated
@@ -80,7 +80,7 @@ func (s *PublicEthereumAPI) Syncing() (interface{}, error) { | |||
progress := s.b.Downloader().Progress() | |||
|
|||
// Return not syncing if the synchronisation already completed | |||
if progress.CurrentBlock >= progress.HighestBlock { | |||
if !s.b.Downloader().Synchronising() { |
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.
This won't be a good change:
// Synchronising returns whether the downloader is currently retrieving blocks.
func (d *Downloader) Synchronising() bool {
return atomic.LoadInt32(&d.synchronising) > 0
}
During the node's initial sync (and maybe even afterwards), sync will abort and restart a few times if the master peer we're connected to disappears. The current code progress.CurrentBlock >= progress.HighestBlock
doesn't care about that, and will just report syncing until we're actually in sync. Your change would report sync done whenever the downloader aborts for a few second.
92ed5c4
to
12c61d8
Compare
@karalabe Updated. |
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!
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.
Hmm, I think the fast sync stall check you added causes some test to fail. Me might just need to patch that test up to cater for this extra check.
12c61d8
to
548eb6b
Compare
@karalabe Please take a look again |
eth/sync.go
Outdated
// Make sure the peer's total difficulty we are synchronizing is higher. | ||
if pm.blockchain.GetTdByHash(pm.blockchain.CurrentFastBlock().Hash()).Cmp(pTd) >= 0 { | ||
return | ||
} |
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.
This is only one path where fast sync is enabled, the above if branch also enables fast sync.
Please move these two lines below outside of both branches, and do something like
if mode == downloader.FastSync {
if pm.blockchain.GetTdByHash(pm.blockchain.CurrentFastBlock().Hash()).Cmp(pTd) >= 0 {
return
}
}
548eb6b
to
920c49d
Compare
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, lets hope nothing breaks with that extra check :)
ethereum#16283) * eth: update higest block we know during the sync if a higher was found * eth: avoid useless sync in fast sync
ethereum#16283) * eth: update higest block we know during the sync if a higher was found * eth: avoid useless sync in fast sync
ethereum#16283) * eth: update higest block we know during the sync if a higher was found * eth: avoid useless sync in fast sync
ethereum#16283) * eth: update higest block we know during the sync if a higher was found * eth: avoid useless sync in fast sync
In this PR, fix two problems:
Avoid useless synchronization during the fast sync if the peer doesn't have higher difficulty than our fast block head.
Update highest block we know during the sync