fix(l1): added checks to newpayload and forkchoiceupdated #2831

fedacking · 2025-05-16T18:37:03Z

Motivation

This should fix some sync and inconsistent behaviour problems with the hive engine tests. The problem was happening when a sync process had begun, and a block from that sync process entered the server. Some checks for that scenario have been added.

Also made some tests in the CI easier to read and edit, while adding a couple of them.

Description

Made the CI tests have 1 by 1.
Added some fixed CI tests to "Paris Engine tests" and "Engine withdrawal tests"
Added a check to forkchoiceupdatet for the body to be present before forking.
Added a check to execute_payload for the body of the parent to be present before executing.

Fixes some tests in #1285

Made it so that we check the parent body is there before adding new payloads and forking. If it's missing but the header is there, we are syncing.

github-actions · 2025-05-16T18:38:09Z

Lines of code report

Total lines added: 10
Total lines removed: 0
Total lines changed: 10

Detailed view

+-----------------------------------------+-------+------+
| File                                    | Lines | Diff |
+-----------------------------------------+-------+------+
| ethrex/crates/blockchain/blockchain.rs  | 514   | +9   |
+-----------------------------------------+-------+------+
| ethrex/crates/blockchain/fork_choice.rs | 142   | +1   |
+-----------------------------------------+-------+------+

crates/blockchain/blockchain.rs

crates/blockchain/fork_choice.rs

fmoletta

Could you comment in the code that the need to fetch the parent's block body is due to executing blocks in batch during full-sync? This will be relevant if we choose to remove it or put it under a feature in the near future

crates/blockchain/blockchain.rs

crates/blockchain/fork_choice.rs

mpaulucci · 2025-05-20T13:58:13Z

.github/workflows/pr-main_l1.yaml

+              Transaction Re-Org|
+              Unique Payload ID|
+              Unknown|
+              Valid NewPayload->ForkchoiceUpdated" # |Invalid P9 -> flaky


mpaulucci · 2025-05-20T14:24:35Z

crates/blockchain/blockchain.rs

-            return Err(ChainError::ParentNotFound);
+        // Feda (#2831): We search for the entire block because during full/batch sync
+        // we can have the header without the body indicating it's still syncing.
+        let parent = self


so let me get this straight: the only change is that you are searching for the whole block instead of just the header. Any reason you decided to do this inline instead of with a helper function? I think it was actually more readable before.

Ah, I see that the previous function was confusing and the comments outdated. Maybe the solution is a better name and a better comment instead of inlining and repeating code?

I'll do that. Do I return the old function for the other places in the code where they were used? With a better comment

yeah, and maybe a better naming. But only if you think it makes the code more readable, this is a suggestion

After implementing it I didn't think it was more readable for this section, but the other places it actually was better, so I have reintroduced the function.

**Motivation** This should fix some sync and inconsistent behaviour problems with the hive engine tests. The problem was happening when a sync process had begun, and a block from that sync process entered the server. Some checks for that scenario have been added. Also made some tests in the CI easier to read and edit, while adding a couple of them. **Description** - Made the CI tests have 1 by 1. - Added some fixed CI tests to "Paris Engine tests" and "Engine withdrawal tests" - Added a check to forkchoiceupdatet for the body to be present before forking. - Added a check to execute_payload for the body of the parent to be present before executing. Fixes some tests in #1285

**Motivation** When adding blocks in batches, transactions in blocks may need to read the hash of a previous block in the batch. This poses an issue, as we add all blocks after the full batch has been executed ad we don't have those hashes stored. Previously, this issue was tackled by storing the block headers before executing and storing the full block batch. This also required us to make the fixes on #2831 (as we might have the headers of blocks we have not yet executed during full sync). This stopped being a suitable solution once #3022 was merged, as block hashes are now searched using the block the state is based on's ancestors. As the state is based on the first block in the batch's parents, later blocks are not able to access the hashes of previous blocks in the batch. This PR aims to fix both issues by adding a `block_hash_cache` to the `StoreVmDatabase` were we would add the hashes of all blocks in the batch, and use it when fetching block hashes before looking at the Store. This way we can access block hashes for previously executed blocks in the batch without prematurely adding data from blocks yet to be executed to the state. Allowing us to rollback fixes from #2831  **Description** * Add `block_hash_cache` to `VmStoreDatabase` * Cache block hashes of the full batch before executing in `Blockchain::add_block_batch` * No longer store block headers before execution during full sync * Rollback fixes from #2831 (no longer needed due to the above change)   Closes #issue_number

…s#2831) **Motivation** This should fix some sync and inconsistent behaviour problems with the hive engine tests. The problem was happening when a sync process had begun, and a block from that sync process entered the server. Some checks for that scenario have been added. Also made some tests in the CI easier to read and edit, while adding a couple of them. **Description** - Made the CI tests have 1 by 1. - Added some fixed CI tests to "Paris Engine tests" and "Engine withdrawal tests" - Added a check to forkchoiceupdatet for the body to be present before forking. - Added a check to execute_payload for the body of the parent to be present before executing. Fixes some tests in lambdaclass#1285

…3034) **Motivation** When adding blocks in batches, transactions in blocks may need to read the hash of a previous block in the batch. This poses an issue, as we add all blocks after the full batch has been executed ad we don't have those hashes stored. Previously, this issue was tackled by storing the block headers before executing and storing the full block batch. This also required us to make the fixes on lambdaclass#2831 (as we might have the headers of blocks we have not yet executed during full sync). This stopped being a suitable solution once lambdaclass#3022 was merged, as block hashes are now searched using the block the state is based on's ancestors. As the state is based on the first block in the batch's parents, later blocks are not able to access the hashes of previous blocks in the batch. This PR aims to fix both issues by adding a `block_hash_cache` to the `StoreVmDatabase` were we would add the hashes of all blocks in the batch, and use it when fetching block hashes before looking at the Store. This way we can access block hashes for previously executed blocks in the batch without prematurely adding data from blocks yet to be executed to the state. Allowing us to rollback fixes from lambdaclass#2831  **Description** * Add `block_hash_cache` to `VmStoreDatabase` * Cache block hashes of the full batch before executing in `Blockchain::add_block_batch` * No longer store block headers before execution during full sync * Rollback fixes from lambdaclass#2831 (no longer needed due to the above change)   Closes #issue_number

fedacking added 2 commits May 16, 2025 12:13

Sync Fix

3a0737b

Made it so that we check the parent body is there before adding new payloads and forking. If it's missing but the header is there, we are syncing.

Made CI less ugly

6775cce

fedacking requested a review from a team as a code owner May 16, 2025 18:37

fedacking changed the title ~~Fix/newpayload sync check~~ fix(l1): added checks to newpayload and forkchoiceupdated May 16, 2025

fmoletta reviewed May 16, 2025

View reviewed changes

crates/blockchain/blockchain.rs Outdated Show resolved Hide resolved

fmoletta reviewed May 16, 2025

View reviewed changes

crates/blockchain/fork_choice.rs Outdated Show resolved Hide resolved

fmoletta reviewed May 16, 2025

View reviewed changes

fedacking marked this pull request as draft May 16, 2025 22:45

fedacking added 8 commits May 19, 2025 11:30

Simplified database call

182ccc2

fixed missing head

908da90

Merge branch 'main' into fix/newpayload-sync-check

4dc3421

Merge branch 'main' into fix/newpayload-sync-check

569ca2a

Removed find_parent_header function

eb14b33

Update block.rs

2dabf49

Documentation

d270912

Clippy

e78704a

fmoletta reviewed May 19, 2025

View reviewed changes

crates/blockchain/blockchain.rs Outdated Show resolved Hide resolved

fmoletta reviewed May 19, 2025

View reviewed changes

crates/blockchain/fork_choice.rs Outdated Show resolved Hide resolved

Moved refs and fixed doc

2ffd4a0

fmoletta approved these changes May 19, 2025

View reviewed changes

fedacking marked this pull request as ready for review May 19, 2025 19:49

Missing Doc

d77303c

mpaulucci reviewed May 20, 2025

View reviewed changes

mpaulucci approved these changes May 20, 2025

View reviewed changes

fedacking added 3 commits May 21, 2025 18:44

Merge branch 'main' into fix/newpayload-sync-check

2244dda

added function to clean up

c6e4ed4

Typ'os

d8096c4

mpaulucci approved these changes May 22, 2025

View reviewed changes

Unused import

04f5e3e

fedacking added this pull request to the merge queue May 22, 2025

Merged via the queue into main with commit 490cd62 May 22, 2025
21 checks passed

fedacking deleted the fix/newpayload-sync-check branch May 22, 2025 15:06

fmoletta added a commit that referenced this pull request Jun 3, 2025

Rollback changes from #2831

438ad07

fmoletta mentioned this pull request Jun 3, 2025

fix(l1): cache block hashes when adding blocks in batch #3034

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(l1): added checks to newpayload and forkchoiceupdated #2831

fix(l1): added checks to newpayload and forkchoiceupdated #2831

Uh oh!

fedacking commented May 16, 2025

Uh oh!

github-actions bot commented May 16, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

fmoletta left a comment

Uh oh!

Uh oh!

Uh oh!

mpaulucci May 20, 2025

Uh oh!

mpaulucci May 20, 2025

Uh oh!

mpaulucci May 20, 2025

Uh oh!

fedacking May 20, 2025 •

edited

Loading

Uh oh!

mpaulucci May 20, 2025

Uh oh!

fedacking May 21, 2025

Uh oh!

Uh oh!

Uh oh!

fix(l1): added checks to newpayload and forkchoiceupdated #2831

fix(l1): added checks to newpayload and forkchoiceupdated #2831

Uh oh!

Conversation

fedacking commented May 16, 2025

Uh oh!

github-actions bot commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Lines of code report

Uh oh!

Uh oh!

Uh oh!

fmoletta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mpaulucci May 20, 2025

Choose a reason for hiding this comment

Uh oh!

mpaulucci May 20, 2025

Choose a reason for hiding this comment

Uh oh!

mpaulucci May 20, 2025

Choose a reason for hiding this comment

Uh oh!

fedacking May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpaulucci May 20, 2025

Choose a reason for hiding this comment

Uh oh!

fedacking May 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented May 16, 2025 •

edited

Loading

fedacking May 20, 2025 •

edited

Loading