Merged
Conversation
Contributor
Performance Report✔️ no performance regression detected Full benchmark results
|
dapplion
changed the title
g11tech
reviewed
Dec 6, 2022
| // Capella and later forks have withdrawals on their ExecutionPayload | ||
| if (fork === ForkName.capella || fork === ForkName.eip4844) { | ||
| // TODO EIP-4844 Remove this when the EC includes `withdrawals` | ||
| (blockBody as capella.BeaconBlockBody).executionPayload.withdrawals = []; |
Contributor
There was a problem hiding this comment.
this will break the withdrawals, so we should do this only in an unmerged PR
Contributor
There was a problem hiding this comment.
removing from here, so that we can only keep this in the unmerged eip-4844 parent PR
Contributor
Author
There was a problem hiding this comment.
Sure, we should merge withdrawal logic here ASAP. Is it there already on unstable or no-where yet?
Contributor
There was a problem hiding this comment.
merge withdrawal logic is already in the executin/http where withdrawals are bundeled on execution payload depending on fork
Contributor
There was a problem hiding this comment.
we set withdrawal to attributes here: https://github.com/ChainSafe/lodestar/pull/4864/files#diff-c331f7b8e7121839c55f5f20f21d1efe1375b69c6fbbe5db12a4baf1ed4acf5dR350, and rest execution apis handle it appropriately
g11tech
force-pushed
the
dapplion/eip-4844-blockproduction
branch
from
86b4b94 to
95739bd
Compare
twoeths
reviewed
Dec 6, 2022
| */ | ||
| getBlobsSidecar(beaconBlock: eip4844.BeaconBlock): eip4844.BlobsSidecar { | ||
| const blockHash = toHex(beaconBlock.body.executionPayload.blockHash); | ||
| const blobsSidecar = this.producedBlobsSidecarCache.get(blockHash); |
Contributor
There was a problem hiding this comment.
seems here we get from beacon block hash while we cache by payload block hash
Contributor
There was a problem hiding this comment.
const blockHash = toHex(beaconBlock.body.executionPayload.blockHash); is payload blockHash
twoeths
reviewed
Dec 6, 2022
| const blobsBundle = await this.executionEngine.getBlobsBundle(payloadId); | ||
|
|
||
| // Sanity check consistency between getPayload() and getBlobsBundle() | ||
| const blockHash = toHex(payload.blockHash); |
Contributor
There was a problem hiding this comment.
here we return payload block hash but seems later on we get by beacon block hash
Contributor
Author
There was a problem hiding this comment.
Get by execution payload blockHash
lodestar/packages/beacon-node/src/chain/chain.ts
Lines 424 to 425 in 95739bd
Set by execution payload blockHash
Which is computed here from the payload
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Description