-
Notifications
You must be signed in to change notification settings - Fork 783
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: ensure executed block does not get pruned if head=final FCU #3153
Conversation
Codecov ReportAttention:
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
@jochem-brouwer what is the status of this PR? |
Ah right I intended this to include more hive fixes. We can use this PR to fix the Cancun failing hive tests, which I assigned myself to #3275. |
@jochem-brouwer ah, can we then please rather take this one in and open a new one instead of re-cycle? This re-purposing of PRs is always a bit problematic. I would then merge this in here. |
Sure, that is fine! |
Updated this via UI |
Updated this via UI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR fixes:
./hive --client ethereumjs --sim ethereum/engine --sim.limit="engine-cancun/Valid NewPayload->ForkchoiceUpdated on Syncing Client
In this test, first FCU is called (after receiving the VALID newPayload):
We reply VALID
Then, FCU is called again, but now with:
B,C are known and VALID. However, before this PR we replied Syncing, since the
pruneCachedBlocks
will actually deleted the just-executed block A from the executed blocks. So if we FCU again (without newPayload which executes the block if this is possible) then it sees that the block is not executed (we thus start executing it) but reply early here with SYNCING.Note: PR still draft because might fix other hive tests here too.