refactor(ethexe): Implement BlockLoader
#4870
Conversation
ark0f
added
A0-pleasereview
|
Review changes with Changed Files
|
…-tx-window # Conflicts: # ethexe/observer/src/lib.rs # ethexe/observer/src/sync.rs # ethexe/service/src/fast_sync.rs
…-tx-window # Conflicts: # ethexe/observer/src/sync.rs
ark0f
changed the title
BLOCK_HASHES_WINDOW_SIZE blocks in fast-syncBlockLoader
…-tx-window # Conflicts: # ethexe/service/src/fast_sync.rs
grishasobol
left a comment
grishasobol
left a comment
There was a problem hiding this comment.
Overall I'm not agree with this approach. Writing data in on-chain storage bypassing observer, is unsafe.
Optimisation in this PR does not make a significant contribution. Observer already tries to upload only not-synced blocks. And in almost all cases "synced" means "header set" and "events set" and vice versa.
| header, | ||
| events, | ||
| }, | ||
| (Some(_), None) | (None, Some(_)) => unreachable!("inconsistent database state"), |
There was a problem hiding this comment.
- It's definetely not unreachable, because state of DB can be changed outside. This is error case.
- As soon as you added BlockLoader - this situation can be. When two threads have their own block loaders, so one thread write header, then events. In the same time second thread is in this function. Between two moments when thread1 has already set header, but still not set events - this case would appear on thread2.
| let data = match (header, events) { | ||
| (Some(header), Some(events)) => BlockData { | ||
| hash: block, | ||
| header, | ||
| events, | ||
| }, | ||
| (Some(_), None) | (None, Some(_)) => unreachable!("inconsistent database state"), | ||
| (None, None) => { | ||
| missing_end_block = Some(bn); | ||
| break; | ||
| } | ||
| }; |
There was a problem hiding this comment.
This match looks weird for me.
To identify whether block has on-chain information in db, we have already block_synced(). If you wanna to skip here loading of already loaded data - much more clear solution can be: use observer Stream:
observer.force_block_sync(block);
observer.wait_block_synced(block).await;
Using observer stream would be more safe and services style solution.
| let end_block = self | ||
| .provider | ||
| .get_block_by_number((*range.end()).into()) | ||
| .await? |
There was a problem hiding this comment.
You have additional rpc call here, which is not presented in previous solution, making it less efficient.
| self.request_block_batch(start..=end) | ||
| }); | ||
|
|
||
| let batches = future::try_join_all(batch_futures).await?; |
| .collect(); | ||
| } | ||
|
|
||
| async fn load_many(&self, range: RangeInclusive<u64>) -> Result<HashMap<H256, BlockData>> { |
There was a problem hiding this comment.
It looks dirty for me: writing blocks data in db and returns in the same time. You can write data in db outside of block loader.
No description provided.