Skip to content

Conversation

@fmoletta
Copy link
Contributor

@fmoletta fmoletta commented Apr 25, 2025

Motivation
PR #2426 changed how fork choice & new payload interact with the syncer and also introduced a bug. If snap sync is enabled, then fork choice update will never attempt to trigger a sync, so the sync process never gets started.
This PR fixes the bug and also refactors the sync manager api to better suit the new use cases

  • Combine commonly used together SyncManager methods set_head & start_sync into sync_to_head
  • Remove unused SyncManager method status and associated struct
  • Make sure sync is triggered during fcu when needed even if snap sync is enabled
  • Re-enable snap sync hive test suite
    Description

Closes #2521

@github-actions
Copy link

Lines of code report

Total lines added: 229
Total lines removed: 82
Total lines changed: 311

Detailed view
+----------------------------------------------------+-------+------+
| File                                               | Lines | Diff |
+----------------------------------------------------+-------+------+
| ethrex/cmd/ef_tests/state/runner/levm_runner.rs    | 368   | -13  |
+----------------------------------------------------+-------+------+
| ethrex/crates/l2/contracts/deployer.rs             | 766   | -32  |
+----------------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/l1_committer.rs         | 394   | -29  |
+----------------------------------------------------+-------+------+
| ethrex/crates/l2/utils/config/committer.rs         | 24    | -1   |
+----------------------------------------------------+-------+------+
| ethrex/crates/l2/utils/config/toml_parser.rs       | 260   | -3   |
+----------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/sync_manager.rs       | 127   | -3   |
+----------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/engine/fork_choice.rs | 393   | +3   |
+----------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/engine/payload.rs     | 698   | -1   |
+----------------------------------------------------+-------+------+
| ethrex/crates/vm/backends/revm/execution_db.rs     | 123   | +123 |
+----------------------------------------------------+-------+------+
| ethrex/crates/vm/backends/revm/mod.rs              | 607   | +14  |
+----------------------------------------------------+-------+------+
| ethrex/crates/vm/backends/revm/mods.rs             | 38    | +38  |
+----------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/call_frame.rs            | 135   | +12  |
+----------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/constants.rs             | 51    | +4   |
+----------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/environment.rs           | 38    | +10  |
+----------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/errors.rs                | 241   | +13  |
+----------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/utils.rs                 | 540   | +12  |
+----------------------------------------------------+-------+------+

@fmoletta fmoletta force-pushed the fix-fcu-not-syncing-if-snap branch from af1fa70 to 21296b8 Compare April 25, 2025 19:34
@fmoletta fmoletta marked this pull request as ready for review April 25, 2025 19:50
@fmoletta fmoletta requested a review from a team as a code owner April 25, 2025 19:50
Copy link
Collaborator

@mpaulucci mpaulucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch!

Comment on lines +317 to +319
context
.syncer
.sync_to_head(fork_choice_state.head_block_hash);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great that now the syncer decides to start the sync!

@fmoletta fmoletta added this pull request to the merge queue Apr 25, 2025
Merged via the queue into main with commit 521a9b6 Apr 25, 2025
37 of 43 checks passed
@fmoletta fmoletta deleted the fix-fcu-not-syncing-if-snap branch April 25, 2025 21:44
pedrobergamini pushed a commit to pedrobergamini/ethrex that referenced this pull request Aug 24, 2025
…sync hive test (lambdaclass#2605)

**Motivation**
PR lambdaclass#2426 changed how fork choice & new payload interact with the syncer
and also introduced a bug. If snap sync is enabled, then fork choice
update will never attempt to trigger a sync, so the sync process never
gets started.
This PR fixes the bug and also refactors the sync manager api to better
suit the new use cases
<!-- Why does this pull request exist? What are its goals? -->
* Combine commonly used together `SyncManager` methods `set_head` &
`start_sync` into `sync_to_head`
* Remove unused `SyncManager` method `status` and associated struct
* Make sure sync is triggered during fcu when needed even if snap sync
is enabled
* Re-enable snap sync hive test suite
**Description**

<!-- A clear and concise general description of the changes this PR
introduces -->

<!-- Link to issues: Resolves lambdaclass#111, Resolves lambdaclass#222 -->

Closes lambdaclass#2521
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sync snap test is flaky

4 participants