fix(ethexe): fast-sync allow zero state and hashes request splitting#5146
fix(ethexe): fast-sync allow zero state and hashes request splitting#5146grishasobol wants to merge 6 commits intomasterfrom
Conversation
Changed Files
|
Summary of ChangesHello @grishasobol, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue within the fast synchronization mechanism where the presence of a zero state hash for an actor would erroneously trigger an error and stop the sync process. By eliminating a specific validation check, the system is now able to correctly handle and proceed with zero state hashes, ensuring that fast sync completes successfully in scenarios where such hashes are legitimate. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request aims to allow zero state hashes during fast sync by removing a check in collect_program_states. However, this change is incomplete and introduces a critical issue. The downstream function sync_from_network is not prepared to handle zero hashes and will panic in debug builds. My review includes a critical comment explaining the issue and the necessary changes to sync_from_network to correctly handle zero state hashes.
I am having trouble creating individual review comments. Click here to see my feedback.
ethexe/service/src/fast_sync.rs (243-246)
While removing this check is a valid step towards allowing zero state hashes, this change is incomplete and introduces a critical issue that will cause panics in debug builds.
The program_states map, which can now contain zero hashes, is passed to sync_from_network. This function has two problems when dealing with zero hashes:
-
Panic in
RequestManager::add:sync_from_networkcallsmanager.add(state, ...)for each state hash.RequestManager::addcontains adebug_assert_ne!(hash, H256::zero(), ...)which will panic when a zero hash is passed. -
Panic when enriching program states: Even if the first panic is avoided (e.g., by not adding zero hashes to the manager),
sync_from_networkwill later panic at this line:.expect("program state cached queue size must be restored");
This is because
restored_cached_queue_sizeswill not contain an entry for the zero hash.
To correctly fix this, you need to modify sync_from_network to handle zero state hashes gracefully:
-
In the loop that populates the
RequestManager, you should skip zero hashes:for &state in program_states.values() { if !state.is_zero() { manager.add(state, RequestMetadata::ProgramState); } }
-
When enriching program states with queue sizes, you should handle the zero hash case. For a zero hash, the queue sizes should be 0, which is consistent with
ProgramState::zero():.map(|(program_id, hash)| { let (canonical_queue_size, injected_queue_size) = if hash.is_zero() { (0, 0) } else { *restored_cached_queue_sizes .get(&hash) .expect("program state cached queue size must be restored") }; // ... })
Since these changes are outside the current diff, I cannot provide a direct code suggestion. Please apply these fixes to make the fast sync robust against zero state hashes.
| use tokio::sync::{mpsc, oneshot}; | ||
|
|
||
| const STREAM_PROTOCOL: StreamProtocol = StreamProtocol::new("/ethexe/db-sync/1.0.0"); | ||
| const STREAM_PROTOCOL: StreamProtocol = |
There was a problem hiding this comment.
Temporary - in order to be able to connect to current Vara.eth testnet
No description provided.