-
Notifications
You must be signed in to change notification settings - Fork 600
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
feat: use shutdown height as sync stop height #11373
Conversation
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.
I like the idea but expected_shutdown
is taken already and has a different meaning. Can you make this a separate mutable config?
chain/client/src/sync/header.rs
Outdated
@@ -147,8 +154,10 @@ impl HeaderSync { | |||
self.syncing_peer = None; | |||
// Pick a new random peer to request the next batch of headers. | |||
if let Some(peer) = highest_height_peers.choose(&mut thread_rng()).cloned() { | |||
// TODO: This condition should always be true, otherwise we can already complete header sync. | |||
if peer.highest_block_height > header_head.height { | |||
let shutdown_height = self.shutdown_height.get(); |
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.
nit: I would put the unwrap_or here.
I don't introduce new parameter, I actually reuse existing expected_shutdown from config.json. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #11373 +/- ##
==========================================
+ Coverage 71.09% 71.28% +0.18%
==========================================
Files 783 784 +1
Lines 156826 157682 +856
Branches 156826 157682 +856
==========================================
+ Hits 111492 112400 +908
+ Misses 40515 40437 -78
- Partials 4819 4845 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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
Gotcha, my idea was to have one config for one meaning. Then you have more control over it as you can set each independently. Anyway since it's only for debugging I'm totally fine with this as is. |
If one wants to take old backup to run some blocks on top of its head, node will first run header sync, and its default behaviour is to download **all** headers from chain. If backup is couple days old, this takes a while. The solution is to use `expected_shutdown` config, which will stop header sync after shutdown height is reached and let the node download and process blocks. I tested this on my node, it worked nicely, allowing to change shutdown height on the fly.
If one wants to take old backup to run some blocks on top of its head, node will first run header sync, and its default behaviour is to download all headers from chain. If backup is couple days old, this takes a while.
The solution is to use
expected_shutdown
config, which will stop header sync after shutdown height is reached and let the node download and process blocks. I tested this on my node, it worked nicely, allowing to change shutdown height on the fly.