feat(octane/evmengine): support blob txs #2498
Merged
+396
−45
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
corverroos
requested review from
chmllr,
kc1116 and
kevinhalliday
as code owners
corverroos
commented
Nov 18, 2024
octane/evmengine/keeper/helpers.go
Outdated
|
|
||
| hasher := sha256.New() | ||
| for i := range sidecar.Blobs { | ||
| if err := kzg4844.VerifyBlobProof(&sidecar.Blobs[i], sidecar.Commitments[i], sidecar.Proofs[i]); err != nil { |
There was a problem hiding this comment.
Is verifying the whole blob, proof, and commitment required for all validators? Since only Commitments are required to calculate versioned hashes which are provided to geth via NewPayload, other fields (Blobs and Proofs) are technically unused. Can they not be removed?
There was a problem hiding this comment.
Did you find the answer to this?
There was a problem hiding this comment.
Yeah, we only include Commitments for minimal but safe support
corverroos
commented
Nov 18, 2024
octane/evmengine/keeper/helpers.go
Outdated
| hashes := make([]common.Hash, 0, len(sidecar.Blobs)) | ||
|
|
||
| hasher := sha256.New() | ||
| for i := range sidecar.Blobs { |
There was a problem hiding this comment.
Ethereum consensus layer has a MAX_BLOBS_PER_BLOCK=6. We should maybe also enforce something like this.
corverroos
force-pushed
the
corver/blobtx
branch
from
9eaf418
to
2796042
Compare
chmllr
approved these changes
Nov 19, 2024
kevinhalliday
approved these changes
Nov 19, 2024
corverroos
force-pushed
the
corver/blobtx
branch
from
2796042
to
b48b1d9
Compare
corverroos
force-pushed
the
corver/blobtx
branch
from
b48b1d9
to
bac9c75
Compare
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.
Adds minimal support for blob txs.
Only include
BlobCommitmentsin consensus chain blocks. This is required to calculate blob hashes and provide toengine.NewPayload. This adds minimum support for blobs, i.e., that they can be submitted to EVM and included in blocks without problems.But we don't include blobs themselves (or proofs) in the consensus chains. Neither do we provide an API to query them. This limits consensus chain size by not including blobs. Submitted blobs can therefore not be queried from the consensus chain. This does render BlobTxs useless, but this isn't a problem since we don't have a use-case that require it.
Minimal support for blobs does means that stock standard geth is still supported and that forking geth to disable blob txs isn't required.
issue: https://github.com/omni-network/ops/issues/613