Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

client: Fix double runs of the block execution #2730

Merged
merged 5 commits into from
May 31, 2023
Merged

Conversation

g11tech
Copy link
Contributor

@g11tech g11tech commented May 26, 2023

On the 4844 devnet5 a weird error was noticed:

  1. on new payload the client runs vmExecution.runWithoutSetBlock first and
    image
  2. then on fcU once the block is part of canonical chain it runs vmExecution.run which runs block in an iterator loop
    image

Run on 1. gives VALID execution while 2nd fails with invalid state root

This PR aims to solve it:
Synopsis of the issue: the vmExecution's run and runWithoutSetHead can end up running the same block one after another leading to state manager's incorrect caches

This PR updates clearCache flag in runBlock if such a scenario comes up during client execution.

  • Add a testcase to repro
  • Fix the issue
  • Testing
    • sync mainnet
    • sync local post merge devnet
    • beacon sync and stay sync in a devnet

PS:
the issue is resolved with this diff which might give an idea about where the problem could be i.e. by running the block again in the vm copy of vmexecution

--- a/packages/client/src/execution/vmexecution.ts
+++ b/packages/client/src/execution/vmexecution.ts
@@ -353,7 +353,8 @@ export class VMExecution extends Execution {
                 }
                 const beforeTS = Date.now()
                 this.stats(this.vm)
-                const result = await this.vm.runBlock({
+                const ivm = await this.vm.copy()
+                const result = await ivm.runBlock({
                   block,
                   root: parentState,
                   clearCache: reorg ? true : false,

@codecov
Copy link

codecov bot commented May 26, 2023

Codecov Report

Merging #2730 (879cbad) into master (9c8f407) will decrease coverage by 0.31%.
The diff coverage is 62.88%.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 90.82% <ø> (ø)
blockchain 90.72% <ø> (ø)
client 86.82% <63.21%> (+<0.01%) ⬆️
common 97.05% <ø> (ø)
devp2p ?
ethash ∅ <ø> (∅)
evm 79.58% <ø> (ø)
rlp ∅ <ø> (∅)
statemanager 80.53% <ø> (ø)
trie 89.92% <ø> (-0.34%) ⬇️
tx 95.76% <ø> (ø)
util 81.13% <ø> (ø)
vm 81.36% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@g11tech g11tech force-pushed the fix-execution-reruns branch from 29ce5e7 to ddae95f Compare May 30, 2023 12:14
@g11tech
Copy link
Contributor Author

g11tech commented May 30, 2023

a lot of diff is because of the formatting changes by moving runWithLock a bit out, but the real PR work is till this commit: 810aa54

@holgerd77 holgerd77 force-pushed the fix-execution-reruns branch from 616f29f to 879cbad Compare May 31, 2023 12:12
@holgerd77
Copy link
Member

Rebased this via UI

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Lot of great smallish additions/clean-ups and nice partial VM execution refactoring, this is still very much needed to have improvements there!

Code looks good, and I guess execution-flow-code can also best be tested by the test cases we have + continued client runs, where I am at right now anyhow.

So will approve and merge. 🙂

* @param tag - The tag to save the headHash to
* @param headHash - The head hash to save
*/
setIteratorHead(tag: string, headHash: Uint8Array): Promise<void>
Copy link
Member

Choose a reason for hiding this comment

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

👍 🙏


closeRPC(server)
t.end()
})
Copy link
Member

Choose a reason for hiding this comment

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

Nice new test case, VM set iterator head execution tests very much needed, great! 👍

@holgerd77 holgerd77 merged commit ae55576 into master May 31, 2023
@holgerd77 holgerd77 deleted the fix-execution-reruns branch May 31, 2023 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants