Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Unify major syncing detection #2699

Merged
merged 9 commits into from Oct 19, 2016
Merged

Unify major syncing detection #2699

merged 9 commits into from Oct 19, 2016

Conversation

keorn
Copy link

@keorn keorn commented Oct 18, 2016

Improves #2639.
Tested on two nodes in various sync conditions (early, mid, import), it is reliable when repeatedly querying RPC. Still returns false for a few seconds of initial sync, but thats rather unavoidable.
Should also improve situations similar to the attack when lots of uncles were increasing queue size, possibly leading to "Syncing" console output.

Its not used for mission critical stuff so block delay can be adjusted later (4 behind now).

@keorn keorn added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Oct 18, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 71281cc on eth-syncing into * on master*.

@tomusdrw
Copy link
Collaborator

tomusdrw commented Oct 19, 2016

RPC test is failing. Looks good apart from that.

@tomusdrw tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 19, 2016
@gavofyork gavofyork added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Oct 19, 2016
@keorn keorn added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Oct 19, 2016
@keorn keorn added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Oct 19, 2016
@keorn keorn mentioned this pull request Oct 19, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 86.225% when pulling 3ad8c4f on eth-syncing into b039323 on master.

@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 19, 2016
@gavofyork gavofyork merged commit aa52b04 into master Oct 19, 2016
@gavofyork gavofyork deleted the eth-syncing branch October 19, 2016 16:35
@arkpar
Copy link
Collaborator

arkpar commented Oct 20, 2016

This does not seem right to me. Block queue can still have a few thousand blocks to process after the sync modules completes a download. eth_syncing should return false only after the block queue has been cleared out.

@keorn
Copy link
Author

keorn commented Oct 20, 2016

In this case isn't it SyncState::Waiting? SyncState::Waiting implies major sync here.

@arkpar
Copy link
Collaborator

arkpar commented Oct 20, 2016

No, the sync does not track the queue status after it is done downloading blocks. That's why there was a check for the queue status in the original code.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants