Submitter: add blob version check for blobTx #800
Conversation
WalkthroughRemoves proof generation from the internal blob helper and sidecar construction, adds header-aware blob sidecar versioning by threading a block header through fee/tip retrieval and rollup transaction creation, and adds blob hashing/proof utilities and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Rollup.rollup()
participant Gas as GetGasTipAndCap()
participant RollupTx as createRollupTx()
participant BlobTx as createBlobTx()
participant Version as DetermineBlobVersion()
participant Sidecar as BlobTxSidecar
Caller->>Gas: request fees
Gas-->>Caller: return (tip, gasFeeCap, blobFee, head, err)
Caller->>RollupTx: create rollup tx (head, fees, calldata)
RollupTx->>Version: determine blobVersion(head)
Version-->>RollupTx: blobVersion
RollupTx->>BlobTx: create blob tx (head, blobVersion, sidecar)
BlobTx->>Sidecar: build/convert sidecar (versioning, proofs)
Sidecar-->>BlobTx: versioned sidecar
BlobTx-->>RollupTx: tx with sidecar
RollupTx-->>Caller: signed tx ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
node/types/blob.go (1)
68-75: Critical inconsistency: Empty blob includes Proofs while non-empty blobs don't.The empty blob case still populates the
Proofsfield (line 73), but the non-empty cases (lines 101-104) omit it entirely. This creates an inconsistent BlobTxSidecar structure.Apply this diff to make the behavior consistent:
if len(blobBytes) == 0 { return ð.BlobTxSidecar{ Blobs: []kzg4844.Blob{*emptyBlob}, Commitments: []kzg4844.Commitment{emptyBlobCommit}, - Proofs: []kzg4844.Proof{emptyBlobProof}, }, nil }tx-submitter/services/rollup.go (2)
1866-1894: Blob cancellation always uses Version0, potentially causing version mismatch.The
CancelTxfunction ignores theheadparameter (line 1828) and always creates a Version0 blob hash usingkZGToVersionedHash(line 1888). If the network has upgraded to support Version1 blobs (Osaka), the cancellation transaction may fail due to version mismatch.Consider applying this diff to use the correct version:
- tip, gasFeeCap, blobFeeCap, _, err := r.GetGasTipAndCap() + tip, gasFeeCap, blobFeeCap, head, err := r.GetGasTipAndCap() if err != nil { return nil, fmt.Errorf("get gas tip and cap error:%w", err) } // ... existing bump logic ... case ethtypes.BlobTxType: // For blob transactions, we need to keep one empty blob var emptyBlob kzg4844.Blob emptyCommitment, err := kzg4844.BlobToCommitment(&emptyBlob) if err != nil { return nil, fmt.Errorf("failed to create empty blob commitment: %w", err) } emptyProof, err := kzg4844.ComputeBlobProof(&emptyBlob, emptyCommitment) if err != nil { return nil, fmt.Errorf("failed to create empty blob proof: %w", err) } + + // Determine blob version based on chain state + version := r.DetermineBlobVersion(head) + var blobHash common.Hash + if version == ethtypes.BlobSidecarVersion1 { + hasher := sha256.New() + blobHash = common.Hash(kzg4844.CalcBlobHashV1(hasher, &emptyCommitment)) + } else { + blobHash = kZGToVersionedHash(emptyCommitment) + } newTx = ethtypes.NewTx(ðtypes.BlobTx{ ChainID: uint256.MustFromBig(tx.ChainId()), Nonce: tx.Nonce(), GasTipCap: uint256.MustFromBig(tip), GasFeeCap: uint256.MustFromBig(gasFeeCap), Gas: tx.Gas(), To: *tx.To(), Value: uint256.MustFromBig(tx.Value()), Data: []byte{}, // Empty calldata for cancellation BlobFeeCap: uint256.MustFromBig(blobFeeCap), - BlobHashes: []common.Hash{kZGToVersionedHash(emptyCommitment)}, + BlobHashes: []common.Hash{blobHash}, Sidecar: ðtypes.BlobTxSidecar{ + Version: version, Blobs: []kzg4844.Blob{emptyBlob}, Commitments: []kzg4844.Commitment{emptyCommitment}, Proofs: []kzg4844.Proof{emptyProof}, }, })
1156-1183: Add nil check for batch.Sidecar before accessing Blobs field.Line 1150 accesses
batch.Sidecar.Blobswithout verifyingbatch.Sidecaris not nil. When batches originate from the oracle path and don't meet the parent copy conditions (line 180 inoracle/oracle/batch.go),batch.Sidecarremains nil, causing a panic. Additionally, verify thatbatch.Sidecar.Proofsis properly populated for both derivation and oracle paths—the oracle path may leave this field incomplete.func (r *Rollup) createRollupTx(batch *eth.RPCRollupBatch, nonce, gas uint64, tip, gasFeeCap, blobFee *big.Int, calldata []byte, head *ethtypes.Header) (*ethtypes.Transaction, error) { if batch.Sidecar != nil && len(batch.Sidecar.Blobs) > 0 { return r.createBlobTx(batch, nonce, gas, tip, gasFeeCap, blobFee, calldata, head) } return r.createDynamicFeeTx(nonce, gas, tip, gasFeeCap, calldata) }
🧹 Nitpick comments (1)
tx-submitter/services/rollup_test.go (1)
67-72: Consider verifying the head parameter in successful test cases.While the current implementation correctly captures the new
headreturn value, the tests should assert thatheadis not nil when the call succeeds to validate the API contract.Apply this diff:
- tip, feecap, blobfee, _, err := r.GetGasTipAndCap() + tip, feecap, blobfee, head, err := r.GetGasTipAndCap() require.NoError(t, err) require.NotNil(t, tip) require.NotNil(t, feecap) require.NotNil(t, blobfee) + require.NotNil(t, head) require.Equal(t, initTip, tip)Apply similar changes to line 80.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
node/types/blob.go(2 hunks)tx-submitter/services/blob_hash_test.go(1 hunks)tx-submitter/services/rollup.go(14 hunks)tx-submitter/services/rollup_test.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tx-submitter/services/rollup.go (1)
tx-submitter/types/blob_params.go (2)
ChainConfigMap(10-14)DefaultBlobConfig(8-8)
node/types/blob.go (1)
node/derivation/blobs.go (1)
Blob(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: check
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
node/types/blob.go (2)
55-66: LGTM: Clean refactor to remove proof generation.The rename to
makeBlobCommitmentbetter reflects the function's purpose, and removing proof generation simplifies the API appropriately.
86-100: LGTM: Call sites correctly updated.All calls to
makeBlobCommitmentproperly handle the new return signature without the proof value.tx-submitter/services/rollup.go (4)
845-849: LGTM: Finalize correctly ignores blob-related parameters.Since
finalize()creates aDynamicFeeTx(line 878), it appropriately ignores theheadandblobFeereturn values fromGetGasTipAndCap().
1250-1307: LGTM: GetGasTipAndCap correctly returns head parameter.The function properly retrieves the header (line 1251) and threads it through all return paths, returning it on success (line 1306) or nil on error (lines 1253, 1258, 1264, 1273).
1586-1650: LGTM: ReSubmitTx correctly handles blob version conversion.The function properly uses the
headparameter (line 1586) to determine the blob version (line 1644) and conditionally converts from Version0 to Version1 (lines 1645-1650), ensuring resubmitted blob transactions use the correct version.
1921-1933: LGTM: DetermineBlobVersion has sound versioning logic.The function safely defaults to Version0 when the head is unavailable (lines 1922-1924), uses chain-specific configuration (lines 1925-1928), and correctly identifies when to use Version1 based on Osaka activation (lines 1929-1931).
tx-submitter/services/blob_hash_test.go (1)
1-126: The review comment is incorrect—kZGToVersionedHash is properly defined and accessible.The function
kZGToVersionedHashis defined attx-submitter/services/utils.go:27with a complete implementation. Since both the test file (blob_hash_test.go) and the utility file (utils.go) are in the sameservicespackage, the unexported function is accessible to the tests. The function signature matches all three call sites in the test file (lines 58, 99, 124), and the tests will compile and run without issues.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tx-submitter/services/rollup.go (2)
1156-1192: Compute blob hashes after choosing the sidecar version
createBlobTxgrabs the blob hashes before the version switch, so even with the helper fixed we still end up hashing with the wrong version info. For v0 submits this continues to emit Osaka (v1) hashes; for v1 submits we never recompute afterBlobSidecarVersionToV1enriches the proofs. Please derive the hashes after the version branch and pass the finalsidecar.VersionintoBlobHashesso commitments, proofs, and hashes all agree.(eips.ethereum.org)- versionedHashes := types.BlobHashes(batch.Sidecar.Blobs, batch.Sidecar.Commitments) sidecar := ðtypes.BlobTxSidecar{ Blobs: batch.Sidecar.Blobs, Commitments: batch.Sidecar.Commitments, } switch types.DetermineBlobVersion(head, r.chainId.Uint64()) { @@ default: return nil, fmt.Errorf("unsupported blob version") } + versionedHashes := types.BlobHashes(sidecar.Blobs, sidecar.Commitments, sidecar.Version) return ethtypes.NewTx(ðtypes.BlobTx{ @@ - BlobHashes: versionedHashes, + BlobHashes: versionedHashes, Sidecar: sidecar, }), nil
1653-1673: Resubmits upgrade proofs but leave stale blob hashesWhen we bump a v0 sidecar to v1 in
ReSubmitTx,BlobSidecarVersionToV1swaps in cell proofs but we still reuse the original blob hashes (0x01 prefix). Osaka validators will reject that payload. After the potential version upgrade, recompute theBlobHasheswith the sidecar’s actual version before building the new blob tx.(eips.ethereum.org)case ethtypes.BlobTxType: sidecar := tx.BlobTxSidecar() version := types.DetermineBlobVersion(head, r.chainId.Uint64()) if sidecar.Version == ethtypes.BlobSidecarVersion0 && version == ethtypes.BlobSidecarVersion1 { err = types.BlobSidecarVersionToV1(sidecar) if err != nil { return nil, err } + blobHashes := types.BlobHashes(sidecar.Blobs, sidecar.Commitments, sidecar.Version) + sidecar.Proofs = sidecar.Proofs + tx = tx.WithBlobHashes(blobHashes) } newTx = ethtypes.NewTx(ðtypes.BlobTx{ @@ - BlobHashes: tx.BlobHashes(), + BlobHashes: types.BlobHashes(sidecar.Blobs, sidecar.Commitments, sidecar.Version), Sidecar: sidecar, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tx-submitter/services/rollup.go(14 hunks)tx-submitter/types/blob.go(1 hunks)tx-submitter/types/blob_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tx-submitter/types/blob.go (2)
node/derivation/blobs.go (1)
Blob(9-9)tx-submitter/types/blob_params.go (2)
ChainConfigMap(10-14)DefaultBlobConfig(8-8)
tx-submitter/types/blob_test.go (3)
node/derivation/blobs.go (1)
Blob(9-9)tx-submitter/types/blob.go (2)
MakeBlobProof(21-31)MakeCellProof(33-43)node/derivation/beacon.go (1)
VerifyBlobProof(217-219)
tx-submitter/services/rollup.go (2)
tx-submitter/types/blob.go (5)
BlobHashes(12-19)DetermineBlobVersion(45-57)MakeBlobProof(21-31)MakeCellProof(33-43)BlobSidecarVersionToV1(60-73)tx-submitter/types/blob_params.go (1)
ChainConfigMap(10-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: check
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: check
- GitHub Check: check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tx-submitter/types/blob_config_test.go(2 hunks)tx-submitter/types/blob_params.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tx-submitter/types/blob_config_test.go (2)
tx-submitter/types/blob_params.go (1)
HoodiChainConfig(40-54)tx-submitter/types/blob_config.go (1)
GetBlobFeeDenominator(92-136)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
tx-submitter/types/blob_params.go (1)
28-35: LGTM! Mainnet fork configurations added correctly.The Osaka, BPO1, and BPO2 fork times and configurations have been properly added to MainnetChainConfig, bringing it in line with the test network configurations. The timestamps are in chronological order and the configuration structure is consistent.
Summary by CodeRabbit
Enhancements
Tests