Consensus: Sync speedup #4
Conversation
The method 'validateSetHashes' was added to fastSync for better code readability. This method now also validates the block header, in addition to the hash validation. Finally, it no longer compares the block number against latest block number as blocks are added to currentSet only if block number is greater than latest.
Remove redundant sorting of toFinalize. Reduce the size of synchronization blocks. Rename variables to add mroe clarity.
|
Do you think these tunings are appropriate for all hardware/bandwidth? Anyway, approved via last review |
I guess I can add those parameters to the config file, so everyone can tune it for his hardware and bandwidth but I think that for 99% of the users these will work just fine. Anyway, even in the case of a really bad bandwidth, this may result in a slower sync wbut the sync will still work (a timed-out block is added back to the download queue, but unless there is an error, the block will arrive eventually. So in the worst case there will be several requests of the same block). For very large blocks 2 seconds isn't enough, but the vast majority of the blocks contain only a few transactions. I would say that when we get to the point where the average block size is > 2MB we might need to change this. |
|
well, I guess I'm more concerned about the memory implications. With 1MB block size, I'd expect an extra load of 256mb+ (depending on buffering and how we unmarshal the request (and some folks are already rather prone to complaints of memory usage). Now, we're nowhere near 1mb full blocks, so unlikely to be an issue, just more thinking out loud. 2 seconds is rather a short period too, since they're done in parallel, the 10s is likely a fine limit. Again, at full 1mb blocks, this requires 128MB/s from around the world, which will be unlikely to work as block size grows. |
|
Well the fast sync needs atleast 200 blocks as it first validates all the hashes in the validator set and only then adds the blocks to the chain. About the block timeout, I thought the same at first but I noticed a dramatic boost in performance when I changed it from 10 to 2 seconds. I guess 5 seconds might work as well. |
|
Ok. I trust your tests. I just worry that this value will need tuning as block size increases over time. Maybe a configuration value (with default if not set) would be worthwhile. I don't think that should block this PR though. |
| TreeSet<Pair<Block, Channel>> toProcess = new TreeSet<>( | ||
| Comparator.comparingLong(o -> o.getKey().getNumber())); | ||
|
|
||
| for (int i = 0; i < 200; i++) { |
There was a problem hiding this comment.
This and the 199 below should probably at least be static constants (or config values later on).
'Magic numbers' are an anti-pattern.
There's nothing special about 200 afaik, just a convenient number for sync.
There was a problem hiding this comment.
Yes thank you. I was playing with the numbers and forgot to change it back to getValidatorUpdateInterval().
|
|
||
| for (int i = 0; i < 200; i++) { | ||
| Block block = null; | ||
| ; |
Rename sync parameters to prevent confusion: MAX_DOWNLOAD_TIME -> DOWNLOAD_TIMEOUT MAX_QUEUED_BLOCKS -> MAX_QUEUED_JOBS MAX_UNFINISHED_JOBS -> MAX_PENDING_JOBS Add those three parameters along with MAX_PENDING_BLOCKS to the config to allow future tuning.
|
@orogvany I've added all sync parameters to the config so they can be easily tuned in the future. |
| lastBlockInSet = latest + config.getValidatorUpdateInterval(); | ||
| fastSync = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm concerned that if more and more people are using fastsync, votes for blocks inside an interval are less-likely validated. Eventually, we may never be able to sync the blocks in [start_block, last_block_of_interval]
|
Why would we not be able to sync those other blocks? Clients would still hold them, just trusting they were valid, ya? I wonder if there would be possibility of faking those blocks in between but getting enough votes at %200 would be tough. Not clear where abuse would happen |
|
I don't see how this could be abused either. If @semuxgo, Could you please elaborate on how exactly can we get to a point where we are unable to sync? |
|
@karr2 the votes are not hashed. |
|
@semuxgo yes that is correct. But why do those votes matter? Are you concerned that we may need those votes in a future implementation and they might be lost if everyone start using fastSync? |
Adding an option to enable/disable fastSync. Adding a safe gap from the target block in which fastSync will be disabled and syncing will be performed in the normal fashion.
|
I've added an option to enable/disable fastSync from the config file. Also, fastSync will be disabled for the last 1024 blocks, as requested. About the conflicting file, it's because of the usage of NTP time which was introduced in a later commit. Changing line 172 (long timestamp = System.currentTimeMillis();) to "long timestamp = TimeUtil.currentTimeMillis();" and removing lines 174 and 175 will fix the problem. |
|
This has stagnated a bit. I vote we approve with default=off so we at least have it available for folks initial sync |
|
LGTM. Finished syncing in 40 minutes. |
|
Merged |
I'm reopening the PR from the old repo.
I've added tests and changed the default values of
MAX_DOWNLOAD_TIME(from 10 seconds to 2 seconds) andMAX_UNFINISHED_JOBS(from 16 to 256.) Those values yielded best results in my tests.