[ETCM-103] Restartable state sync#730
Conversation
| # Current Size of ETC state trie is aroud 150M Nodes, so 200M is set to have some reserve | ||
| # If the number of elements inserted into bloom filter would be significally higher that expected, then number | ||
| # of false positives would rise which would degrade performance of state sync | ||
| state-sync-bloomFilter-size = 200000000 |
There was a problem hiding this comment.
state-sync-bloom-filter-size? to be consistent
|
|
||
| # Max number of mpt nodes held in memory in state sync, before saving them into database | ||
| # 100k is around 60mb (each key-value pair has around 600bytes) | ||
| state-sync-persistBatch-size = 100000 |
There was a problem hiding this comment.
state-sync-persist-batch-size
| # If new pivot block received from network will be less than fast sync current pivot block, the re-try to chose new | ||
| # pivot will be scheduler after this time. Avarage block time in etc/eth is around 15s so after this time, most of | ||
| # network peers should have new best block | ||
| pivot-block-reSchedule-interval = 15.seconds |
There was a problem hiding this comment.
pivot-block-reschedule-interval
| def waitingForPivotBlockUpdate(updateReason: PivotBlockUpdateReason): Receive = handleCommonMessages orElse { | ||
| case PivotBlockSelector.Result(pivotBlockHeader) => | ||
| log.info(s"New pivot block with number ${pivotBlockHeader.number} received") | ||
| if (pivotBlockHeader.number >= syncState.pivotBlock.number) { |
There was a problem hiding this comment.
It will be more readable if you use pattern matching instead of nested ifs
| reScheduleAskForNewPivot(updateReason) | ||
| } else { | ||
| updatePivotSyncState(updateReason, pivotBlockHeader) | ||
| syncState = syncState.copy(updatingPivotBlock = false) |
There was a problem hiding this comment.
I think syncState = syncState.copy(updatingPivotBlock = false) should be done in updatePivotSyncState method
| reqType match { | ||
| case _: CodeRequest => | ||
| blockchain.storeEvmCode(hash, data).commit() | ||
| bloomFilter.put(hash) |
There was a problem hiding this comment.
Very minor: You could call bloomFilter.put(hash) before the pattern matching
| // restart. This can be done by exposing RockDb iterator to traverse whole mptnode storage. | ||
| // Another possibility is that there is some light way alternative in rocksdb to check key existence | ||
| state.memBatch.contains(req.nodeHash) || isInDatabase(req) | ||
| if (state.memBatch.contains(req.nodeHash)) { |
There was a problem hiding this comment.
Simpler: state.memBatch.contains(req.nodeHash) || (bloomFilter.mightContain(req.nodeHash) && isInDatabase(req))
| } | ||
|
|
||
| private def updatePivotBlock(state: FinalBlockProcessingResult): Unit = { | ||
| private def updatePivotBlock(state: PivotBlockUpdateReason): Unit = { |
There was a problem hiding this comment.
minor: state or reason then?
There was a problem hiding this comment.
reason - forgot to change
| syncState = | ||
| syncState.updatePivotBlock(pivotBlockHeader, syncConfig.fastSyncBlockValidationX, updateFailures = true) | ||
|
|
||
| case NodeRestart => |
There was a problem hiding this comment.
Shouldn't it be named SyncRestart? If fast-sync actor gets restarted due to some failure catched by supervisor, it's going to be restarted with clean state the same way is if it was restarted whole node.
That also makes me think - shouldn't SyncController watch for Fast-Sync restarts and start it once such happens?
There was a problem hiding this comment.
So i agree that SyncRestart is more compelling name.
Question about supervision is more nuanced, in general I am not sure we handle it well across all codebase, but for this particual case we are fine as: FastySync is child of SyncController and default strategy for uncatched Exception in child is just restart it. It will probably mean some of the request in flight will be later ignored and some of the peers gets unnecessry blacklis. Those missed requests may triger some weird error condition. In my view this whole class was not designed with restarts in mind, but rather with handing all excpetion by itself.
| (info.maxBlockNumber - syncConfig.pivotBlockOffset) - state.pivotBlock.number >= syncConfig.maxPivotBlockAge | ||
| } | ||
|
|
||
| private def getPeerWithTooFreshNewBlock( |
There was a problem hiding this comment.
is it really too fresh or more fresh enough to update to?
|
|
||
| sealed abstract class FinalBlockProcessingResult | ||
| sealed abstract class PivotBlockUpdateReason { | ||
| def nodeRestart: Boolean = this match { |
kapke
left a comment
kapke
left a comment
There was a problem hiding this comment.
The code looks good!
I'd like to see how it works though. How much time should I expect to wait on mainnet before state sync starts?
|
So currently sync time looks like on average:
And state sync starts only after blockchain is downloaded. There is also still one issue with blockchain which makes blockchain state to be stuck, and restart with some config tweak is needed to resume it, if it happen to you let me know. (we already have ticket to track it, and I suspect what is the issue here) State Sync has higher variability in sync times as it is more parallel and it depends on number of peers , which for now is totally random due to our random walk nature of current disovery. On my machine i had finished state sync in 6h when i got 9-10 peers, and 10h when i got 3-4peers. |
3 participants
Description
Makes it possible to restart state sync if the pivot block goes stale during it.
Proposed Solution
The way it works is:
Possible improvements
The best possible improvement would be to have concurrent blockchain and state download, then this whole dance with updating pivot would be unnecessary, only thing the state sync would need to do would be to track best synced block and restart when best synced block is larger by some margin than current state sync pivot. This would require a little rehaul of the way we are handling available peers in upper layers of Mantis to avoid simultaneous concurrent request for state and blockchain data.
Bonus
SyncControllerSpec test have beed refactored to use autopilot.
Testing
I was able to sync to mainnet 4 times now with this setup. (for now without node restarts during state sync, as proper restarting requires one more ticket https://jira.iohk.io/browse/ETCM-213 i.e refiling bloom filter after node restart. Without it it should theroticly be possible to do but can be painfully slow due to large number of false positives from bloom filter which not coresspond to database content)