Add syncing configuration options #892
Conversation
dutterbutter
requested review from
austinabell,
ec2 and
timvermeulen
as code owners
| @@ -54,14 +54,15 @@ fn peer_manager_update() { | |||
| local_sender, | |||
| event_receiver, | |||
| genesis_ts.clone(), | |||
| SyncConfig::default(), | |||
There was a problem hiding this comment.
this should spawn 0 workers, or else the test has unnecessary overhead
| @@ -1029,6 +1031,7 @@ mod tests { | |||
| genesis: genesis_ts, | |||
| bad_blocks: Default::default(), | |||
| verifier: Default::default(), | |||
| req_window: Default::default(), | |||
There was a problem hiding this comment.
this will be 0, seems like that would cause issues with the test?
| @@ -83,6 +83,7 @@ async fn space_race_full_sync() { | |||
| genesis, | |||
| bad_blocks: Default::default(), | |||
| verifier: PhantomData::<FullVerifier>::default(), | |||
| req_window: Default::default(), | |||
| fn default() -> Self { | ||
| Self { | ||
| req_window: 200, | ||
| worker_tasks: 1, |
There was a problem hiding this comment.
can you please add the TODO that existed to revisit this value, as this certainly shouldn't stay once side effects fixed?
blockchain/chain_sync/src/sync.rs
Outdated
| impl SyncConfig { | ||
| pub fn new(req_window: i64, worker_tasks: usize) -> Self { | ||
| Self { | ||
| req_window, | ||
| worker_tasks | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think a constructor like this is necessary, but doesn't matter much
There was a problem hiding this comment.
I can remove if you want and just reference the struct directly in peer_test.rs I just defaulted to adding a constructor instead. Give a thumbs up if you want that changed.
There was a problem hiding this comment.
It's just config, it can change later if more are added (probably would be swapped for a builder pattern or something so don't worry about it)
Summary of changes
Changes introduced in this pull request:
SyncConfigto theChainSyncerworker_tasksandreq_windowDaemonOptsfor command-line usageReference issue to close (if applicable)
Closes #801
Other information and links
Command-line usage: