EIP-7934: RLP Execution Block Size Limit #1249
Conversation
WalkthroughAdds a MaxBlockSize constant and an ErrBlockOversized error, enforces an early oversized-block check in block validation for Galileo, adds a mining-side buffer guard to pre-reject transactions that approach the limit, adjusts blob-sidecar size accounting, adds EIP-4844 block encoding tests, and updates several indirect Go module versions. Changes
Sequence Diagram(s)sequenceDiagram
participant V as BlockValidator
participant S as SizeCheck
participant O as OtherValidations
participant M as Miner
participant P as TxPreCheck
Note over V,S: ValidateBody (new early size guard)
V->>S: compute RLP-encoded block size
alt Galileo && size > MaxBlockSize
S-->>V: ErrBlockOversized (early return)
else
S->>O: continue known-block / linkability checks
O-->>V: validation result
end
Note over M,P: processTxn (mining-side buffer)
M->>P: propose transaction
P->>P: compute currentSize + txSize
alt >= MaxBlockSize - buffer
P-->>M: reject ("tx would exceed block size limit")
else
P-->>M: accept transaction
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)core/types/transaction.go (1)
⏰ 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). (2)
🔇 Additional comments (2)
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rollup/missing_header_fields/export-headers-toolkit/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
core/block_validator.go(2 hunks)core/error.go(1 hunks)core/types/block_test.go(2 hunks)core/types/transaction.go(1 hunks)miner/scroll_worker.go(2 hunks)params/protocol_params.go(1 hunks)rollup/missing_header_fields/export-headers-toolkit/go.mod(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-11T15:54:05.820Z
Learnt from: Thegaram
PR: scroll-tech/go-ethereum#1204
File: miner/scroll_worker.go:836-845
Timestamp: 2025-06-11T15:54:05.820Z
Learning: When `processTxn` rejects a transaction because its calculated L1 data fee is ≥ `fees.MaxL1DataFee()`, this indicates a mis-configured system parameter, not a bad transaction, so the tx must remain in the TxPool rather than being purged.
Applied to files:
miner/scroll_worker.go
🧬 Code graph analysis (4)
core/block_validator.go (2)
params/protocol_params.go (1)
MaxBlockSize(200-200)core/error.go (1)
ErrBlockOversized(40-40)
core/types/block_test.go (8)
common/bytes.go (2)
FromHex(29-37)Hex2Bytes(79-82)core/types/block.go (2)
Block(223-241)Header(69-122)rlp/decode.go (1)
DecodeBytes(91-105)common/hexutil/hexutil.go (2)
MustDecodeUint64(111-117)MustDecode(80-86)common/types.go (3)
HexToAddress(218-218)HexToHash(66-66)BytesToHash(54-58)core/types/transaction.go (3)
NewTx(72-76)Transactions(564-564)BlobTxType(54-54)core/types/blob_tx.go (1)
BlobTx(48-69)core/types/transaction_signing.go (1)
LatestSignerForChainID(96-101)
core/types/transaction.go (1)
rlp/raw.go (1)
ListSize(33-35)
miner/scroll_worker.go (1)
params/protocol_params.go (1)
MaxBlockSize(200-200)
⏰ 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). (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
rollup/missing_header_fields/export-headers-toolkit/go.mod (1)
18-18: Clarify rationale for dependency updates in unrelated module.The PR objective is to implement EIP-7934 block size limits, but this file updates indirect dependencies in the export-headers-toolkit module, which appears orthogonal to the feature. All changes are version bumps across cryptographic and system libraries.
Please clarify:
- Are these dependency updates necessary for the EIP-7934 implementation, or are they a separate maintenance task that should be in its own PR?
- Are there any known breaking changes in these upgraded versions, particularly for
gnark-crypto(v0.12.1 → v0.16.0) andblst(v0.3.12 → v0.3.15)?- Has the corresponding
go.sumfile been updated and tested?Given the scope change, consider verifying:
- Run
go test ./...in this module to ensure no incompatibilities- Confirm all transitive dependencies resolve correctly
- Check if the golang.org/x security updates (crypto, sys) introduce any API changes affecting this toolkit
Also applies to: 21-22, 67-67, 75-78
params/protocol_params.go (1)
199-201: LGTM! Clean addition of the maximum block size constant.The 8 MiB limit is clearly defined and will be enforced by the validator for Galileo nodes.
core/error.go (1)
38-41: LGTM! Clear error definition for oversized blocks.The error message and comment appropriately reference EIP 7934.
core/block_validator.go (1)
70-74: LGTM! Proper early validation for oversized blocks.The check is correctly fork-gated for Galileo and placed early in the validation pipeline to fail fast.
core/types/block_test.go (1)
200-252: LGTM! Comprehensive test coverage for EIP-4844 blob transaction encoding.The test properly validates block structure, blob transaction fields, and round-trip encoding consistency using a reference test vector from the Ethereum test suite.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
core/types/block_test.go (3)
219-219: Consider adding nil safety for WithdrawalsHash dereference.The code dereferences
WithdrawalsHashwithout checking for nil. While the test data is known and should be valid, adding a nil check would make the test more robust against data issues.Apply this diff to add defensive nil checking:
- check("WithdrawalRoot", *block.Header().WithdrawalsHash, common.HexToHash("0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421")) + if block.Header().WithdrawalsHash == nil { + t.Fatal("WithdrawalsHash is nil") + } + check("WithdrawalRoot", *block.Header().WithdrawalsHash, common.HexToHash("0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421"))
241-243: Consider validating all transactions in the block.The test only validates the blob transaction (index 3) but not the other three transactions. While this follows existing test patterns, validating all transaction types in the block would provide more comprehensive coverage, especially given that the reference test is named "blockWithAllTransactionTypes".
Consider adding validation for the other transactions:
check("len(Transactions)", len(block.Transactions()), 4) + // Validate all transaction types are correctly decoded + check("Transactions[0].Type()", block.Transactions()[0].Type(), uint8(LegacyTxType)) + check("Transactions[1].Type()", block.Transactions()[1].Type(), uint8(AccessListTxType)) + check("Transactions[2].Type()", block.Transactions()[2].Type(), uint8(DynamicFeeTxType)) check("Transactions[3].Hash", block.Transactions()[3].Hash(), tx.Hash()) check("Transactions[3].Type()", block.Transactions()[3].Type(), uint8(BlobTxType))
213-222: Consider validating EIP-4844 specific header fields.Since this test validates an EIP-4844 block containing blob transactions, consider also validating the EIP-4844 specific header fields
BlobGasUsedandExcessBlobGasto ensure these fields are correctly encoded/decoded.Add validation for EIP-4844 header fields:
check("Nonce", block.Nonce(), uint64(0)) check("Time", block.Time(), hexutil.MustDecodeUint64("0x79e")) check("Size", block.Size(), uint64(len(blockEnc))) + // Validate EIP-4844 specific fields + if block.Header().BlobGasUsed != nil { + check("BlobGasUsed", *block.Header().BlobGasUsed, uint64(0x20000)) + } + if block.Header().ExcessBlobGas != nil { + check("ExcessBlobGas", *block.Header().ExcessBlobGas, uint64(0)) + }Note: Adjust the expected values based on the actual test data from the ethereum/tests repository.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/types/block_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/types/block_test.go (8)
common/bytes.go (2)
FromHex(29-37)Hex2Bytes(79-82)core/types/block.go (2)
Block(223-241)Header(69-122)rlp/decode.go (1)
DecodeBytes(91-105)common/hexutil/hexutil.go (2)
MustDecodeUint64(111-117)MustDecode(80-86)common/types.go (3)
HexToAddress(218-218)HexToHash(66-66)BytesToHash(54-58)core/types/transaction.go (3)
NewTx(72-76)Transactions(564-564)BlobTxType(54-54)core/types/blob_tx.go (1)
BlobTx(48-69)core/types/transaction_signing.go (1)
LatestSignerForChainID(96-101)
⏰ 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). (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
core/types/block_test.go (5)
26-26: LGTM!The new imports are necessary and appropriate for constructing and validating EIP-4844 blob transactions in the test.
Also applies to: 30-30
200-202: Good test structure following established patterns.The test appropriately validates EIP-4844 block encoding/decoding using reference data from ethereum/tests, consistent with other encoding tests in this file.
224-239: LGTM!The blob transaction construction correctly uses uint256 types for large numeric fields and properly signs the transaction. The absence of a Sidecar is appropriate since it's optional and the test is validating encoding of the transaction without blob data.
245-251: LGTM!The round-trip encoding validation is crucial for ensuring lossless encoding/decoding of EIP-4844 blocks. This validation correctly confirms byte-for-byte equivalence, which is essential for consensus and block size calculations.
229-229: Gas field value verified as correct.The Gas value
0xe8d4a51000is sourced from the ethereum/tests reference test file (blockWithAllTransactionTypes.json) as indicated by the URL comment in the test. The value appears consistently in the RLP-encoded block data (blockEnc) and the manually constructedBlobTx, confirming it is intentional test data for validating EIP-4844 blob transaction encoding. No issues found.
| HistoryStorageCode = common.FromHex("3373fffffffffffffffffffffffffffffffffffffffe14604657602036036042575f35600143038111604257611fff81430311604257611fff9006545f5260205ff35b5f5ffd5b5f35611fff60014303065500") | ||
| HistoryServeWindow = uint64(8191) | ||
|
|
||
| MaxBlockSize = 8_388_608 // maximum size of an RLP-encoded block |
There was a problem hiding this comment.
We already have this:
"maxTxPayloadBytesPerBlock": 122880,Does adding EIP-7934 make any difference?
There was a problem hiding this comment.
// IsValidBlockSize returns whether the given block's transaction payload size is below the limit.
func (s ScrollConfig) IsValidBlockSize(size common.StorageSize) bool {
return s.MaxTxPayloadBytesPerBlock == nil || size <= common.StorageSize(*s.MaxTxPayloadBytesPerBlock)
}
// IsValidBlockSizeForMining is similar to IsValidBlockSize, but it accounts for the confidence factor in Rust CCC
func (s ScrollConfig) IsValidBlockSizeForMining(size common.StorageSize) bool {
return s.IsValidBlockSize(size * (1.0 / 0.95))
}
yeah, the thought is the same.
1. Purpose or design rationale of this PR
...
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Tests
Chores