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

bump nim-eth, extend empty block fallback for Capella #4357

Merged
merged 2 commits into from
Nov 28, 2022

Conversation

etan-status
Copy link
Contributor

Implements emptyPayloadToBlockHeader for Capella. status-im/nim-eth#562

Implements `emptyPayloadToBlockHeader` for Capella.
status-im/nim-eth#562
@etan-status etan-status requested a review from tersec November 25, 2022 11:35
mixDigest : payload.prev_randao, # EIP-4399 `mixDigest` -> `prevRandao`
nonce : default(BlockNonce),
fee : some payload.base_fee_per_gas,
withdrawalsRoot: withdrawalsRoot)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it work to unconditionally include this, regardless of fork or engine API version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the RLP serializer checks whether the field is a some or a none, and emits the correct block hash.

I tested the logic using the test script here:
status-im/nim-eth#562 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For Capella blocks you need engine API v2 calls. For pre-Capella blocks, you may also use v1 but v2 also works.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fortunately, once ELs support Capella in any production capacity, then Nimbus can start getting rid of the v1 calls then. Presumably gating on Gnosis capella fork schedule, but fundamentally see no reason to keep using v1 calls, ever, once the ELs support v2 calls.

build_empty_execution_payload[typeof forkyState.data, T](
forkyState.data, feeRecipient)
when stateFork >= BeaconStateFork.Capella:
raiseAssert $capellaImplementationMissing
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this PR add such an implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem here is that the various when return different types. So, either the entire function needs to change to ForkyState, or there needs to be a ForkedExecutionPayload. Too big to just extend the empty payload fallback.

Also, regardless of empty vs non-empty, we need to pass expected withdrawals into this function so that engine_getPayload or the empty block fallback are correct. Again too big for the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Plan is to make the whole makeBeaconBlockForHeadAndSlot call tree generic, avoid yet another forked type.

Copy link
Contributor

Choose a reason for hiding this comment

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

But, sure, agree with scope of PR.

@github-actions
Copy link

github-actions bot commented Nov 25, 2022

Unit Test Results

         9 files  ±0       873 suites  ±0   25m 27s ⏱️ + 3m 4s
  2 703 tests +1    2 510 ✔️ +1  193 💤 ±0  0 ±0 
11 780 runs  +3  11 561 ✔️ +3  219 💤 ±0  0 ±0 

Results for commit 341f6bb. ± Comparison against base commit 60e01dc.

♻️ This comment has been updated with latest results.

@etan-status etan-status merged commit c941dc8 into unstable Nov 28, 2022
@etan-status etan-status deleted the dev/etan/el-capellafallback branch November 28, 2022 13:41
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.

2 participants