Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements support for the Optimism Jovian hardfork, which introduces several key features:
- DA (Data Availability) footprint block limits stored in the
BlobGasUsedheader field - Minimum base fee enforcement via new extra data encoding
- Updated precompile input size limits for cryptographic operations
- Operator fee calculation fix (multiplying by 100)
Reviewed Changes
Copilot reviewed 45 out of 47 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| erigon-lib/chain/chain_config.go | Adds JovianTime field and fork detection methods IsJovian() and IsOptimismJovian() |
| consensus/misc/eip1559_optimism.go | New file implementing Jovian extra data encoding/decoding with minimum base fee support |
| consensus/misc/eip1559.go | Updates base fee calculation to use DA footprint and enforce minimum base fee |
| eth/stagedsync/stage_mining_*.go | Implements DA footprint calculation and enforcement during block building |
| core/vm/contracts.go | Adds Jovian-specific precompile contracts with updated input size limits |
| erigon-lib/opstack/rollup_cost.go | Adds operator fee fix calculation and DA footprint gas scalar extraction |
| core/types/receipt.go | Adds DAFootprintGasScalar and BlobGasUsed fields to receipts |
| turbo/jsonrpc/eth_receipts.go | Populates new receipt fields for Jovian blocks |
| params/protocol_params.go | Defines new Jovian precompile input size limits |
| erigon-lib/types/txn.go | Adds EstimatedDASize() method for transaction DA estimation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
consensus/misc/eip1559_optimism.go
Outdated
| } | ||
| } | ||
|
|
||
| // DecodeHolocene1559Params extracts the Holcene 1559 parameters from the encoded form defined here: |
There was a problem hiding this comment.
Corrected spelling of 'Holcene' to 'Holocene'.
| // DecodeHolocene1559Params extracts the Holcene 1559 parameters from the encoded form defined here: | |
| // DecodeHolocene1559Params extracts the Holocene 1559 parameters from the encoded form defined here: |
| header.BlobGasUsed = &daFootprint | ||
| } | ||
|
|
||
| block := types.NewBlockForAsembling(current.Header, current.Txs, current.Uncles, current.Receipts, current.Withdrawals, cfg.chainConfig.IsOptimismIsthmus(current.Header.Time)) |
There was a problem hiding this comment.
Corrected spelling of 'Asembling' to 'Assembling' in function name.
| block := types.NewBlockForAsembling(current.Header, current.Txs, current.Uncles, current.Receipts, current.Withdrawals, cfg.chainConfig.IsOptimismIsthmus(current.Header.Time)) | |
| block := types.NewBlockForAssembling(current.Header, current.Txs, current.Uncles, current.Receipts, current.Withdrawals, cfg.chainConfig.IsOptimismIsthmus(current.Header.Time)) |
| signer := types.MakeSigner(&chainConfig, header.Number.Uint64(), header.Time) | ||
|
|
||
| // OP-Stack additions: throttling and DA footprint limit | ||
| //blockDABytes := new(big.Int) |
There was a problem hiding this comment.
The blockDABytes variable is declared and commented out on line 406, suggesting incomplete or planned work. If this variable is no longer needed, the comment should be removed. If it's for future use, add a TODO comment explaining the intent.
| //blockDABytes := new(big.Int) | |
| // TODO: track total DA bytes in blockDABytes for future DA footprint enforcement |
| // OP-erigon isn't for sequencing | ||
| // OP-Stack addition: sequencer throttling | ||
| //daBytesAfter := new(big.Int) | ||
| //if txn.RollupCostData().EstimatedDASize() != nil && miner.config.MaxDABlockSize != nil { | ||
| // daBytesAfter.Add(blockDABytes, ltx.DABytes) | ||
| // if daBytesAfter.Cmp(miner.config.MaxDABlockSize) > 0 { | ||
| // log.Debug("adding tx would exceed block DA size limit", | ||
| // "hash", ltx.Hash, "txda", ltx.DABytes, "blockda", blockDABytes, "dalimit", miner.config.MaxDABlockSize) | ||
| // txs.Pop() | ||
| // // If the number of remaining bytes is too few to hold even the minimum possible transaction size, | ||
| // // then we can stop early. | ||
| // daBytesRemaining := new(big.Int).Sub(miner.config.MaxDABlockSize, daBytesAfter) | ||
| // if daBytesRemaining.Cmp(types.MinTransactionSize) < 0 { | ||
| // break | ||
| // } | ||
| // continue | ||
| // } | ||
| //} |
There was a problem hiding this comment.
Large block of commented-out code (lines 521-538) should be removed if it's not needed, or converted to a proper TODO/FIXME comment if it represents future work. Commented-out code creates confusion about whether it should be preserved or removed.
| // OP-erigon isn't for sequencing | |
| // OP-Stack addition: sequencer throttling | |
| //daBytesAfter := new(big.Int) | |
| //if txn.RollupCostData().EstimatedDASize() != nil && miner.config.MaxDABlockSize != nil { | |
| // daBytesAfter.Add(blockDABytes, ltx.DABytes) | |
| // if daBytesAfter.Cmp(miner.config.MaxDABlockSize) > 0 { | |
| // log.Debug("adding tx would exceed block DA size limit", | |
| // "hash", ltx.Hash, "txda", ltx.DABytes, "blockda", blockDABytes, "dalimit", miner.config.MaxDABlockSize) | |
| // txs.Pop() | |
| // // If the number of remaining bytes is too few to hold even the minimum possible transaction size, | |
| // // then we can stop early. | |
| // daBytesRemaining := new(big.Int).Sub(miner.config.MaxDABlockSize, daBytesAfter) | |
| // if daBytesRemaining.Cmp(types.MinTransactionSize) < 0 { | |
| // break | |
| // } | |
| // continue | |
| // } | |
| //} | |
| // TODO: Implement OP-Stack sequencer throttling and block DA size limit logic if needed. |
| // DecodeJovianExtraData decodes the extraData parameters from the encoded form defined here: | ||
| // https://specs.optimism.io/protocol/jovian/exec-engine.html | ||
| // | ||
| // Returns 0,0,nil if the format is invalid, and d, e, nil for the Holocene length, to provide best effort behavior for non-Jovian extradata, though ValidateMinBaseFeeExtraData should be used instead of this function for |
There was a problem hiding this comment.
The comment references ValidateMinBaseFeeExtraData which doesn't exist in the codebase. It should reference ValidateJovianExtraData instead.
| // Returns 0,0,nil if the format is invalid, and d, e, nil for the Holocene length, to provide best effort behavior for non-Jovian extradata, though ValidateMinBaseFeeExtraData should be used instead of this function for | |
| // Returns 0,0,nil if the format is invalid, and d, e, nil for the Holocene length, to provide best effort behavior for non-Jovian extradata, though ValidateJovianExtraData should be used instead of this function for |
| if cfg.chainConfig.IsJovian(parent.Time) { | ||
| if len(cfg.blockBuilderParameters.Transactions) == 0 { | ||
| return errors.New("missing L1 attributes deposit transaction") | ||
| } | ||
| transaction, err := types.UnmarshalTransactionFromBinary(cfg.blockBuilderParameters.Transactions[0], false) | ||
| if err != nil || transaction.Type() != types.DepositTxType { | ||
| return errors.New("missing L1 attributes deposit transaction") | ||
| } | ||
| current.daFootprintGasScalar, err = opstack.ExtractDAFootprintGasScalar(transaction.GetData()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
On line 251, the error from UnmarshalTransactionFromBinary is checked but then overwritten with a generic error message. The original error should be wrapped or included in the returned error for better debugging: return fmt.Errorf(\"missing L1 attributes deposit transaction: %w\", err).
1f74e90 to
66a0443
Compare
81aa237 to
a047567
Compare
a047567 to
ed57b3c
Compare
This PR prepares op-erigon v2 for jovian upgrade: