core,miner: add ability to build block with blobs #27875
Merged
Conversation
This file contains 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
lightclient
force-pushed
the
4844-miner
branch
2 times, most recently
from
790c229
to
ba83d18
Compare
|
This is now rebased and waiting on merging #27841 |
lightclient
changed the title
lightclient
force-pushed
the
4844-miner
branch
3 times, most recently
from
2097e28
to
543e90c
Compare
|
Rebased against master, this is now ready for review. |
lightclient
force-pushed
the
4844-miner
branch
from
543e90c
to
1df3885
Compare
holiman
reviewed
Aug 17, 2023
beacon/engine/types.go
Outdated
Comment on lines
263
to
270
| for i := range sidecars { | ||
| for j := range sidecars[i].Blobs { | ||
| blobsBundle.Blobs = append(blobsBundle.Blobs, hexutil.Bytes(sidecars[i].Blobs[j][:])) | ||
| blobsBundle.Commitments = append(blobsBundle.Commitments, hexutil.Bytes(sidecars[i].Commitments[j][:])) | ||
| blobsBundle.Proofs = append(blobsBundle.Proofs, hexutil.Bytes(sidecars[i].Proofs[j][:])) | ||
| } |
There was a problem hiding this comment.
Suggested change
| for i := range sidecars { | |
| for j := range sidecars[i].Blobs { | |
| blobsBundle.Blobs = append(blobsBundle.Blobs, hexutil.Bytes(sidecars[i].Blobs[j][:])) | |
| blobsBundle.Commitments = append(blobsBundle.Commitments, hexutil.Bytes(sidecars[i].Commitments[j][:])) | |
| blobsBundle.Proofs = append(blobsBundle.Proofs, hexutil.Bytes(sidecars[i].Proofs[j][:])) | |
| } | |
| for _, sidecar := range sidecars { | |
| for j, blob := range sidecar.Blobs { | |
| bundle.Blobs = append(bundle.Blobs, hexutil.Bytes(sidecar.Blobs[j][:])) | |
| bundle.Commitments = append(bundle.Commitments, hexutil.Bytes(sidecar.Commitments[j][:])) | |
| bundle.Proofs = append(bundle.Proofs, hexutil.Bytes(sidecar.Proofs[j][:])) | |
| } |
make it a bit less dense, and remove the possibility of mixing up i and j
miner/worker.go
Outdated
Comment on lines
751
to
752
| // TODO (MariusVanDerWijden): Move this check | ||
| if (len(env.sidecars)+len(tx.BlobHashes()))*params.BlobTxBlobGasPerBlob > params.BlobTxMaxBlobGasPerBlock { | ||
| return nil, errors.New("max data blobs reached") | ||
| } |
There was a problem hiding this comment.
Why not move it already? Where do we want it?
lightclient
force-pushed
the
4844-miner
branch
2 times, most recently
from
839644c
to
a9a7e79
Compare
rjl493456442
approved these changes
Aug 23, 2023
| // isn't really a better place right now. The blob gas limit is checked at block validation time | ||
| // and not during execution. This means core.ApplyTransaction will not return an error if the | ||
| // tx has too many blobs. So we have to explicitly check it here. | ||
| if (env.blobs+len(tx.BlobHashes()))*params.BlobTxBlobGasPerBlob > params.MaxBlobGasPerBlock { |
There was a problem hiding this comment.
I think it can be treated as block.GasLeft.
Block has a gasPool and we can create a blobGasPool for blobTx. The benefit of this approach is: this check can be used in both (1) block generation and (2) block import.
Right now, we check the blob validity in (v *BlockValidator) ValidateBody when import a new block, and here when generate a new block. They can be in the same place I think.
There was a problem hiding this comment.
Me and @lightclient tried making this change yesterday, adding blobGas into core.GasPool. I think it's not worth it right now. There are too many places in the code where we would need to suddenly care about blobGas. It would be better to refactor the state transition first, to remove duplication across the codebase.
Co-authored-by: Marius van der Wijden <m.vanderwijden@live.de> Co-authored-by: lightclient <lightclient@protonmail.com>
devopsbo3
pushed a commit
to HorizenOfficial/go-ethereum
that referenced
this pull request
Nov 10, 2023
--------- Co-authored-by: Marius van der Wijden <m.vanderwijden@live.de> Co-authored-by: Felix Lange <fjl@twurst.com>
devopsbo3
added a commit
to HorizenOfficial/go-ethereum
that referenced
this pull request
Nov 10, 2023
This reverts commit 2099a80.
devopsbo3
added a commit
to HorizenOfficial/go-ethereum
that referenced
this pull request
Nov 10, 2023
This reverts commit 2099a80.
sduchesneau
pushed a commit
to streamingfast/go-ethereum
that referenced
this pull request
Feb 26, 2024
miner: add to build block with EIP-4844 blobs Conflicts: miner/worker.go simple conflict because we added a return param. accepted both changes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I'm ripping out the changes that allow the miner to build 4844 blocks from #27511.