pump go-ethereum version & support parsing setCodeTx #782
Conversation
WalkthroughBumps the go-ethereum target to morph-v2.0.7 and aligns multiple go.mod/submodule references; replaces blob-based transaction decoding with a bytes-based DecodeTxsFromBytes (adds SetCodeTx); adds Viridian chain/hardfork support; adds tracing reasons and previous-state returns to MemoryStateDB APIs; updates genesis deploy-configs and related code. Changes
Sequence DiagramsequenceDiagram
participant Client
participant BatchProcessor
participant Decoder
participant Signer
participant MemoryStateDB
rect rgb(200,230,255)
note right of BatchProcessor: Old (blob-based)
Client->>BatchProcessor: submit blob payload
BatchProcessor->>Decoder: DecodeTxsFromBlob(blob) + decompress
Decoder-->>BatchProcessor: list of txs (legacy types)
end
rect rgb(200,255,200)
note right of BatchProcessor: New (bytes-based + Viridian)
Client->>BatchProcessor: submit raw bytes payload
BatchProcessor->>Decoder: DecodeTxsFromBytes(bytes)
Decoder-->>BatchProcessor: parsed txs (includes SetCodeTx, EIP-7702 auth)
BatchProcessor->>Signer: NewViridianSigner(chainId)
Signer-->>BatchProcessor: signed message for processing
end
rect rgb(255,245,210)
note right of MemoryStateDB: Tracing-enabled state ops
BatchProcessor->>MemoryStateDB: AddBalance(addr, amt, reason)
MemoryStateDB-->>BatchProcessor: previous balance
BatchProcessor->>MemoryStateDB: SetState(addr,key,val)
MemoryStateDB-->>BatchProcessor: previous storage value
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Reason: Heterogeneous changes touching core decoding logic, public API method signatures (state DB), genesis wiring, prover hardfork/types, and multi-module dependency bumps requiring cross-checking and coordinated validation. 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)
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: 5
🧹 Nitpick comments (15)
.gitmodules (1)
4-4: Submodule tracks a feature branch; confirm this is intended for main and consider pinning for reproducibilityTracking
feat/eip-7702can cause non-deterministic builds as the branch moves. For long-lived branches or releases, prefer pinning the submodule to a specific commit/tag and reverting this tomain(or removing thebranchkey) before merge to main.If you’re ready to target main, apply:
-branch = feat/eip-7702 +branch = mainops/l2-genesis/deploy-config/qanet-deploy-config.json (1)
35-36: 0 values likely activate forks at genesis; use null/omit to keep them unsetGiven DeployConfig uses pointer fields (e.g.,
*uint64withomitempty),0will be a non-nil value and typically means “activate at time 0,” which becomes “active from genesis.” If the intent is to leave Morph203/Morph300 disabled until scheduled, set them tonullor omit the keys.- "morph203Time": 0, - "morph300Time": 0, + "morph203Time": null, + "morph300Time": null,If activation-at-genesis is desired for qanet, keep as-is but please confirm.
oracle/oracle/batch.go (3)
98-106: Use block base fee for AsMessage and reuse o.ctx.AsMessage expects the block base fee, not the tx fee cap. Also prefer o.ctx over context.Background for cancellation/timeout propagation.
Apply:
- signer := types.NewMorph300Signer(tx.ChainId()) - msg, err := tx.AsMessage(signer, tx.GasFeeCap()) + signer := types.NewMorph300Signer(tx.ChainId()) + header, err := o.l1Client.HeaderByNumber(o.ctx, big.NewInt(int64(lg.BlockNumber))) + if err != nil { + return fmt.Errorf("get header by number error:%v", err) + } + msg, err := tx.AsMessage(signer, header.BaseFee) if err != nil { return err } - header, err := o.l1Client.HeaderByNumber(context.Background(), big.NewInt(int64(lg.BlockNumber))) - if err != nil { - return fmt.Errorf("get header by number error:%v", err) - }
118-121: Guard against short tx data before slicing.Directly slicing tx.Data()[0:4] can panic if data < 4. Cheap length check avoids that.
- var batch eth.RPCRollupBatch - if bytes.Equal(tx.Data()[0:4], abi.Methods["commitBatch"].ID) { + var batch eth.RPCRollupBatch + if len(tx.Data()) < 4 { + continue + } + if bytes.Equal(tx.Data()[0:4], abi.Methods["commitBatch"].ID) {
186-190: Fix logged error variable.parseErr is the error here; logging err is misleading.
- if parseErr != nil { - log.Error("get l2 BlockNumber", "err", err) - return parseErr - } + if parseErr != nil { + log.Error("get l2 BlockNumber", "err", parseErr) + return parseErr + }Makefile (1)
13-19: Make sed in-place edits OS-portable.BSD sed uses
-i ''; GNU sed prefers-i(or-i''without a space). To avoid CI/host drift, prefergo mod editover sed.Apply:
- @if grep -q '$(ETHEREUM_MODULE_NAME)' $(MODULE)/go.mod; then \ - sed -i '' -e "s|$(ETHEREUM_MODULE_NAME) v[0-9][^[:space:]]*|$(ETHEREUM_MODULE_NAME) $(ETHEREUM_TARGET_VERSION)|" $(MODULE)/go.mod; \ - fi + @if grep -q '$(ETHEREUM_MODULE_NAME)' $(MODULE)/go.mod; then \ + ( cd $(MODULE) && go mod edit -require=$(ETHEREUM_MODULE_NAME)@$(ETHEREUM_TARGET_VERSION) ); \ + fi - @if grep -q '$(TENDERMINT_MODULE_NAME)' $(MODULE)/go.mod; then \ - sed -i '' -e "s|$(TENDERMINT_MODULE_NAME) v[0-9][^[:space:]]*|$(TENDERMINT_MODULE_NAME) $(TENDERMINT_TARGET_VERSION)|" $(MODULE)/go.mod; \ - fi + @if grep -q '$(TENDERMINT_MODULE_NAME)' $(MODULE)/go.mod; then \ + ( cd $(MODULE) && go mod edit -require=$(TENDERMINT_MODULE_NAME)@$(TENDERMINT_TARGET_VERSION) ); \ + fiops/l2-genesis/deploy-config/devnet-deploy-config.json (1)
43-48:l2StakingPksappears unused in the code path.I don’t see this consumed in DeployConfig or genesis builders. Either plumb it through (type/validation) or remove to avoid config drift.
ops/l2-genesis/morph-chain-ops/genesis/config.go (1)
129-132: Adding Morph300Time to DeployConfig: LGTM; add a sanity check vs Morph203Time.Prevent misconfiguration by ensuring 300 ≥ 203 when both provided.
Apply:
func (d *DeployConfig) Check() error { + if d.Morph203Time != nil && d.Morph300Time != nil && *d.Morph300Time < *d.Morph203Time { + return fmt.Errorf("Morph300Time must be >= Morph203Time: %w", ErrInvalidDeployConfig) + }ops/l2-genesis/morph-chain-ops/genesis/genesis.go (1)
43-46: Minor: avoid redundant local for Morph300Time.You can pass
config.Morph300Timedirectly, mirroring existing intent.Apply:
- var morph300Time *uint64 - if config.Morph300Time != nil { - morph300Time = config.Morph300Time - } + // use config.Morph300Time directlyAnd below:
- Morph300Time: morph300Time, + Morph300Time: config.Morph300Time,ops/l2-genesis/morph-chain-ops/genesis/setters.go (2)
31-33: Tag dev account funding as genesis balance increase.Using a specific reason improves trace clarity.
Apply:
- db.AddBalance(account, devBalance, tracing.BalanceChangeUnspecified) + db.AddBalance(account, devBalance, tracing.BalanceIncreaseGenesisBalance)
147-150: StateDB type inconsistency.Other setters accept
vm.StateDB; this one takes*state.MemoryStateDB. Consider takingvm.StateDBfor consistency and easier testing/mocking (MemoryStateDB can satisfy the interface).node/types/blob_test.go (1)
145-151: Also assert the SetCodeTx round-trips.Strengthen the test by checking the 4th tx hash.
Apply:
require.NoError(t, err) require.EqualValues(t, 4, txs.Len()) require.EqualValues(t, transferTx.Hash(), txs[0].Hash()) require.EqualValues(t, legacyContractTx.Hash(), txs[1].Hash()) require.EqualValues(t, contractTx.Hash(), txs[2].Hash()) + require.EqualValues(t, setCodeTx.Hash(), txs[3].Hash())node/types/blob.go (1)
169-177: Remove commented-out codeThese lines contain commented-out legacy code that should be removed to maintain code cleanliness. The new implementation above already handles these cases properly.
- - // we support the tx types of LegacyTxType/AccessListTxType/DynamicFeeTxType - //if firstByte == eth.AccessListTxType || firstByte == eth.DynamicFeeTxType { - // // the firstByte here is used to indicate tx type, so skip it - // if err := binary.Read(reader, binary.BigEndian, &firstByte); err != nil { - // return nil, err - // } - //} else if firstByte <= 0xf7 { // legacy tx first byte must be greater than 0xf7(247) - // return nil, fmt.Errorf("not supported tx type: %d", firstByte) - //} fullTxBytes, err = extractInnerTxFullBytes(firstByte, reader)ops/l2-genesis/morph-chain-ops/state/memory_db.go (2)
164-174: SetNonce should track and return the previous nonce valueFor consistency with other setter methods that now return previous values for tracing,
SetNonceshould also return the previous nonce. The current signature accepts aNonceChangeReasonbut doesn't utilize the tracing capability fully.While the current implementation follows the vm.StateDB interface which has a void return type for SetNonce, consider documenting why this method doesn't return the previous value like the other setters, or consider extending the interface if nonce tracking becomes important for your use case.
119-120: Consider using a consistent pattern for account initialization checksBoth
SubBalanceandAddBalancepanic when the account doesn't exist, but the error handling pattern differs slightly from other methods that return nil or default values.Consider creating accounts automatically in these methods similar to how Ethereum's StateDB typically works, or at least use a consistent error handling approach:
func (db *MemoryStateDB) AddBalance(addr common.Address, amount *big.Int, reason tracing.BalanceChangeReason) *big.Int { db.rw.Lock() defer db.rw.Unlock() account, ok := db.genesis.Alloc[addr] if !ok { - panic(fmt.Sprintf("%s not in state", addr)) + // Create account with zero balance if it doesn't exist + db.genesis.Alloc[addr] = core.GenesisAccount{ + Code: []byte{}, + Storage: make(map[common.Hash]common.Hash), + Balance: big.NewInt(0), + Nonce: 0, + } + account = db.genesis.Alloc[addr] } prev := new(big.Int).Set(account.Balance)Also applies to: 133-135
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
bindings/go.sumis excluded by!**/*.sumcontracts/go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumnode/go.sumis excluded by!**/*.sumops/l2-genesis/go.sumis excluded by!**/*.sumops/tools/go.sumis excluded by!**/*.sumoracle/go.sumis excluded by!**/*.sumtx-submitter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
.gitmodules(1 hunks)Makefile(1 hunks)bindings/go.mod(1 hunks)contracts/go.mod(1 hunks)go-ethereum(1 hunks)node/go.mod(1 hunks)node/types/blob.go(2 hunks)node/types/blob_test.go(3 hunks)ops/l2-genesis/deploy-config/devnet-deploy-config.json(1 hunks)ops/l2-genesis/deploy-config/qanet-deploy-config.json(1 hunks)ops/l2-genesis/go.mod(1 hunks)ops/l2-genesis/morph-chain-ops/genesis/config.go(1 hunks)ops/l2-genesis/morph-chain-ops/genesis/genesis.go(2 hunks)ops/l2-genesis/morph-chain-ops/genesis/setters.go(3 hunks)ops/l2-genesis/morph-chain-ops/state/memory_db.go(6 hunks)ops/tools/go.mod(1 hunks)oracle/go.mod(1 hunks)oracle/oracle/batch.go(1 hunks)tx-submitter/go.mod(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ops/l2-genesis/morph-chain-ops/genesis/setters.go (2)
ops/l2-genesis/morph-chain-ops/state/memory_db.go (1)
MemoryStateDB(25-28)bindings/predeploys/addresses.go (1)
L2CrossDomainMessengerAddr(36-36)
node/types/blob_test.go (2)
node/types/batch.go (1)
BatchData(24-32)node/types/blob.go (1)
DecodeTxsFromBytes(124-188)
⏰ 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: test
🔇 Additional comments (18)
ops/tools/go.mod (1)
8-8: Approve — version bump verified across repoAll go.mod entries reference github.com/morph-l2/go-ethereum v1.10.14-0.20250904090403-2bccdaee12d9. Run repo-wide go mod tidy after merging to refresh go.sum.
tx-submitter/go.mod (1)
10-10: LGTM on go-ethereum bump.No issues spotted. Remember to run go mod tidy and update vendor (if used) for this module.
contracts/go.mod (1)
9-9: Upgrade acknowledged.Contracts module pinned to the new pseudo-version; no concerns.
oracle/oracle/batch.go (1)
98-100: Confirm signer choice against L1 rules.Switching to NewMorph300Signer for L1 tx parsing may be unnecessary if L1 is standard London/Cancun. If Morph300 doesn’t alter signature hashing for these tx types, fine; otherwise consider types.LatestSignerForChainID(tx.ChainId()) to match L1 fork rules.
bindings/go.mod (1)
7-7: Bindings aligned with new go-ethereum version.Looks good; no additional action.
ops/l2-genesis/go.mod (1)
9-9: Genesis tooling bump ok.No issues; ensure generated artifacts remain reproducible with the new Geth fork point.
oracle/go.mod (1)
10-10: Oracle module pinned; matches others.All good. After merge, run tidy to update sums.
node/go.mod (2)
12-12: Promoting uint256 to direct dep makes sense.Given usage in node/types and tests, this is appropriate.
14-14: Node pinned to new go-ethereum; validate cross-module uniformity.Version matches other modules. Consider a quick repo scan to prevent drift (see script in ops/tools/go.mod comment).
ops/l2-genesis/deploy-config/devnet-deploy-config.json (1)
37-38: Confirm fork activation at genesis.
morph203Time: 0andmorph300Time: 0will activate both forks at genesis (non-nil, value 0). Confirm this is intended for devnet; otherwise set them to null or omit.ops/l2-genesis/morph-chain-ops/genesis/setters.go (2)
8-9: Tracing import addition: LGTM.
140-144: Precompile funding reason: LGTM.node/types/blob_test.go (1)
100-121: SetCodeTx auth list is empty—verify if that’s acceptable for decoding scope.If decoding doesn’t require authorization, fine. If you want closer-to-real payloads, consider adding a minimal authorization entry (per EIP-7702/go-ethereum expectations).
node/types/blob.go (2)
124-188: Support for SetCodeTx looks good, but consider adding defensive validationThe implementation correctly handles the new
SetCodeTxType(EIP-7702), which aligns with the PR's objective to support parsing setCodeTx. The switch-case structure properly handles all transaction types.However, consider adding validation for the SetCodeTx fields after decoding to ensure the transaction is well-formed, similar to how other transaction types might have specific validation requirements.
157-161: Verify that eth.SetCodeTxType and eth.SetCodeTx exist in github.com/morph-l2/go-ethereumUsages found at node/types/blob.go:157-161 and node/types/blob_test.go:105, but no local type/const definitions were located; go.mod pins github.com/morph-l2/go-ethereum v1.10.14-0.20250904090403-2bccdaee12d9. Confirm the dependency (likely core/types) declares:
- const SetCodeTxType
- type SetCodeTx struct { … }
If those symbols are absent, update the code to the new API or adjust the dependency version/interface.
ops/l2-genesis/morph-chain-ops/state/memory_db.go (3)
111-126: LGTM! Proper implementation of balance tracking with tracing supportThe
SubBalancemethod correctly:
- Returns the previous balance for tracing/auditing purposes
- Creates a defensive copy to prevent mutation
- Properly handles the zero balance case
128-140: LGTM! Consistent implementation with SubBalanceThe
AddBalancemethod maintains consistency withSubBalanceby properly returning the previous balance and creating defensive copies.
258-270: LGTM! Proper implementation of state trackingThe
SetStatemethod correctly returns the previous value before updating, which is essential for state tracking and rollback operations.
Makefile
Outdated
| ################## update dependencies #################### | ||
|
|
||
| ETHEREUM_TARGET_VERSION := morph-v2.0.5 | ||
| ETHEREUM_TARGET_VERSION := v1.10.14-0.20250904090403-2bccdaee12d9 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Version bump looks fine; ensure it matches the submodule commit across all go.mod files.
Please verify the pseudo-version’s commit (2bccdaee12d9) matches the go-ethereum submodule HEAD and that every module’s go.mod is updated consistently.
Run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target_ver='v1.10.14-0.20250904090403-2bccdaee12d9'
target_commit=$(sed -E 's/.*-([0-9a-f]{12})$/\1/' <<<"$target_ver")
echo "Target commit: $target_commit"
echo "Submodule HEAD:"
( cd go-ethereum && git rev-parse --short=12 HEAD )
echo "go.mod requires:"
fd go.mod --max-depth 3 | xargs -I{} rg -n 'github.com/morph-l2/go-ethereum\s+v' {}
echo "Mismatches (if any):"
fd go.mod --max-depth 3 | xargs -I{} rg -n "github.com/morph-l2/go-ethereum\s+v(?!.*$target_commit)" {} || trueLength of output: 1644
Mismatch: ETHEREUM_TARGET_VERSION pseudo-version ≠ go-ethereum submodule HEAD
Makefile (line 3) sets ETHEREUM_TARGET_VERSION = v1.10.14-0.20250904090403-2bccdaee12d9 (commit 2bccdaee12d9) but the go-ethereum submodule HEAD is d8d3731fd8a0; go.mod files currently reference the pseudo-version — the repo is inconsistent.
- Action: either update the go-ethereum submodule to commit 2bccdaee12d9 and commit the submodule bump, or change the Makefile and all go.mod entries to the submodule’s current commit. Ensure every module’s go.mod uses the same pseudo-version for github.com/morph-l2/go-ethereum.
🤖 Prompt for AI Agents
In Makefile (line 3) and related go.mod files there is a mismatch: Makefile sets
ETHEREUM_TARGET_VERSION to pseudo-version v1.10.14-0.20250904090403-2bccdaee12d9
while the go-ethereum submodule HEAD is d8d3731fd8a0; fix by making the repo
consistent — either (A) update the go-ethereum submodule to commit 2bccdaee12d9
and commit the submodule bump so the submodule HEAD matches the pseudo-version,
or (B) change the Makefile and every module’s go.mod to use the pseudo-version
that corresponds to the current submodule commit (d8d3731f…), ensuring all
github.com/morph-l2/go-ethereum require entries use the same pseudo-version;
after updating, run git add/update the submodule reference and commit the
changes so all files reference the same version.
| setCodeTx := generateSetCodeTx() | ||
| require.NoError(t, err) | ||
| b, err := MakeBlobCanonical(compressedBlobBytes) | ||
| setCodeTxBz, err := setCodeTx.MarshalBinary() | ||
| require.NoError(t, err) | ||
| txs, err := DecodeTxsFromBlob(b) | ||
|
|
There was a problem hiding this comment.
Bug: checking the wrong error variable after generateSetCodeTx.
generateSetCodeTx() doesn’t return an error; require.NoError(t, err) is using the previous err and hides failures.
Apply:
- setCodeTx := generateSetCodeTx()
- require.NoError(t, err)
- setCodeTxBz, err := setCodeTx.MarshalBinary()
+ setCodeTx := generateSetCodeTx()
+ setCodeTxBz, err := setCodeTx.MarshalBinary()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setCodeTx := generateSetCodeTx() | |
| require.NoError(t, err) | |
| b, err := MakeBlobCanonical(compressedBlobBytes) | |
| setCodeTxBz, err := setCodeTx.MarshalBinary() | |
| require.NoError(t, err) | |
| txs, err := DecodeTxsFromBlob(b) | |
| setCodeTx := generateSetCodeTx() | |
| setCodeTxBz, err := setCodeTx.MarshalBinary() | |
| require.NoError(t, err) |
🤖 Prompt for AI Agents
In node/types/blob_test.go around lines 140 to 144, the test wrongly calls
require.NoError(t, err) immediately after generateSetCodeTx(), but
generateSetCodeTx() does not return an error and this assertion is checking a
stale err variable; remove that incorrect require.NoError call (or replace it
with a nil/zero-value check on setCodeTx if you prefer), leaving the subsequent
MarshalBinary call and its require.NoError intact so the actual error from
MarshalBinary is correctly asserted.
| func DecodeTxsFromBytes(txsBytes []byte) (eth.Transactions, error) { | ||
| reader := bytes.NewReader(txsBytes) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding size validation for txsBytes
The function should validate the input size to prevent potential issues with extremely large byte arrays that could cause memory problems.
func DecodeTxsFromBytes(txsBytes []byte) (eth.Transactions, error) {
+ // Add reasonable size limit to prevent memory issues
+ const maxTxsBytesSize = 10 * 1024 * 1024 // 10MB limit
+ if len(txsBytes) > maxTxsBytesSize {
+ return nil, fmt.Errorf("txsBytes size %d exceeds maximum allowed size %d", len(txsBytes), maxTxsBytesSize)
+ }
reader := bytes.NewReader(txsBytes)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func DecodeTxsFromBytes(txsBytes []byte) (eth.Transactions, error) { | |
| reader := bytes.NewReader(txsBytes) | |
| func DecodeTxsFromBytes(txsBytes []byte) (eth.Transactions, error) { | |
| // Add reasonable size limit to prevent memory issues | |
| const maxTxsBytesSize = 10 * 1024 * 1024 // 10MB limit | |
| if len(txsBytes) > maxTxsBytesSize { | |
| return nil, fmt.Errorf("txsBytes size %d exceeds maximum allowed size %d", len(txsBytes), maxTxsBytesSize) | |
| } | |
| reader := bytes.NewReader(txsBytes) |
🤖 Prompt for AI Agents
In node/types/blob.go around lines 124-125, add input size validation for
txsBytes before creating the bytes.Reader: define a reasonable MAX_TXS_BYTES
constant (e.g. configurable or ~10MB), check len(txsBytes) against that limit
and return a clear error if it exceeds the limit (and optionally if it is zero),
and only proceed to create the reader and decode when the size is within bounds;
ensure the error type/message is consistent with the package error handling so
callers can handle it.
| func (db *MemoryStateDB) SetCode(addr common.Address, code []byte) []byte { | ||
| db.rw.Lock() | ||
| defer db.rw.Unlock() | ||
|
|
||
| account, ok := db.genesis.Alloc[addr] | ||
| if !ok { | ||
| return | ||
| return nil | ||
| } | ||
| account.Code = code | ||
| db.genesis.Alloc[addr] = account | ||
| return code | ||
| } |
There was a problem hiding this comment.
SetCode should return the previous code, not the new code
The current implementation returns the newly set code instead of the previous code. For consistency with other setter methods (SetState, AddBalance, SubBalance) which return the previous value, this should return the old code.
func (db *MemoryStateDB) SetCode(addr common.Address, code []byte) []byte {
db.rw.Lock()
defer db.rw.Unlock()
account, ok := db.genesis.Alloc[addr]
if !ok {
return nil
}
+ prevCode := account.Code
account.Code = code
db.genesis.Alloc[addr] = account
- return code
+ return prevCode📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (db *MemoryStateDB) SetCode(addr common.Address, code []byte) []byte { | |
| db.rw.Lock() | |
| defer db.rw.Unlock() | |
| account, ok := db.genesis.Alloc[addr] | |
| if !ok { | |
| return | |
| return nil | |
| } | |
| account.Code = code | |
| db.genesis.Alloc[addr] = account | |
| return code | |
| } | |
| func (db *MemoryStateDB) SetCode(addr common.Address, code []byte) []byte { | |
| db.rw.Lock() | |
| defer db.rw.Unlock() | |
| account, ok := db.genesis.Alloc[addr] | |
| if !ok { | |
| return nil | |
| } | |
| prevCode := account.Code | |
| account.Code = code | |
| db.genesis.Alloc[addr] = account | |
| return prevCode | |
| } |
🤖 Prompt for AI Agents
In ops/l2-genesis/morph-chain-ops/state/memory_db.go around lines 204–215, the
SetCode function currently returns the newly assigned code but should return the
previous code like other setters; to fix, capture the existing account.Code into
a local variable before assigning the new code, then assign account.Code = code,
write the updated account back into db.genesis.Alloc, and finally return the
captured old code (nil if the account didn't exist or had no code), keeping the
existing locking behavior.
Co-authored-by: chengwenxi <22697326+chengwenxi@users.noreply.github.com>
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 (1)
prover/crates/primitives/src/types/tx.rs (1)
552-554: Decoding panics on malformed input.decode() unwraps decode_2718; this will panic on bad bytes. Return the decode error instead.
Apply this diff:
- Ok(TypedTransaction::Enveloped(TxEnvelope::decode_2718(buf).unwrap())) + TxEnvelope::decode_2718(buf).map(TypedTransaction::Enveloped)
🧹 Nitpick comments (13)
prover/tests/bls12381/host/src/main.rs (1)
4-43: Consider consolidating duplicate test structure.This file shares nearly identical structure with
prover/tests/keccak256/host/src/main.rsandprover/tests/zstd/host/src/main.rs. The only differences are the ELF path and test data.Consider extracting common test logic into a shared utility function to reduce duplication:
// In a shared test utility module pub fn run_prover_test(elf: &[u8], stdin_data: &[u8]) { // Common test logic here }Then each test file would only need to provide its specific ELF and data.
prover/crates/core/src/executor/mod.rs (1)
22-30: Simplify the conversion logic.The intermediate
signed_authsvariable is unnecessary and can be removed for more concise code.Apply this diff:
fn convert_authorization_list( auth_list: Option<&[SignedAuthorization]>, ) -> Option<RevmAuthorizationList> { - auth_list.map(|list| { - let signed_auths: Vec<SignedAuthorization> = list.to_vec(); - RevmAuthorizationList::from(signed_auths) - }) + auth_list.map(|list| RevmAuthorizationList::from(list.to_vec())) }prover/crates/morph-executor/client/src/types/blob.rs (1)
78-82: Consider refactoring to a positive condition for better maintainability.The negative condition requires adding a new clause for each supported transaction type. A positive check would be more concise and maintainable.
Apply this diff to refactor using a positive condition:
- // Support transaction types: 0x01, 0x02, 0x04 - if first_byte != 0x01 && first_byte != 0x02 && first_byte != 0x04 { - println!("not supported tx type: 0x{:02x}", first_byte); + // Support transaction types: 0x01, 0x02, 0x04 + if !matches!(first_byte, 0x01 | 0x02 | 0x04) { + println!("not supported tx type: 0x{:02x}", first_byte); break; }prover/crates/core/src/hardfork.rs (5)
21-26: All hardfork heights set to 0 ⇒ VIRIDIAN from genesis on all nets. Confirm.With all entries at 0, get_spec_id will always return VIRIDIAN for any block. If that’s intentional for devnet/testnet/mainnet, all good; if not, please set real heights. Also consider deduping the repeated maps.
Example refactor to remove duplication:
static HARDFORK_HEIGHTS: Lazy<HashMap<u64, HashMap<SpecId, u64>>> = Lazy::new(|| { - let mut map = HashMap::new(); - map.insert(MORPH_DEVNET_CHAIN_ID, HashMap::from([ - (SpecId::BERNOULLI, 0), - (SpecId::CURIE, 0), - (SpecId::MORPH203, 0), - (SpecId::VIRIDIAN, 0), - ])); - map.insert(MORPH_TESTNET_CHAIN_ID, HashMap::from([ /* same */ ])); - map.insert(MORPH_MAINNET_CHAIN_ID, HashMap::from([ /* same */ ])); + fn all_zero_forks() -> HashMap<SpecId, u64> { + HashMap::from([ + (SpecId::BERNOULLI, 0), + (SpecId::CURIE, 0), + (SpecId::MORPH203, 0), + (SpecId::VIRIDIAN, 0), + ]) + } + let mut map = HashMap::new(); + map.insert(MORPH_DEVNET_CHAIN_ID, all_zero_forks()); + map.insert(MORPH_TESTNET_CHAIN_ID, all_zero_forks()); + map.insert(MORPH_MAINNET_CHAIN_ID, all_zero_forks()); map });Also applies to: 30-35, 39-44
56-56: Expose a setter for viridian_block to match existing API.Downstream code can set bernoulli/curie/morph203 but not viridian. Add a setter for consistency and flexibility.
impl HardforkConfig { + /// Set the Viridian block number. + pub fn set_viridian_block(&mut self, viridian_block: u64) -> &mut Self { + self.viridian_block = viridian_block; + self + }Also applies to: 67-67
102-104: Guard against misordered fork heights.If heights are misconfigured (non‑monotonic), spec selection can silently skew. Add a debug assertion.
pub fn get_spec_id(&self, block_number: u64) -> SpecId { + debug_assert!( + self.bernoulli_block <= self.curie_block + && self.curie_block <= self.morph203_block + && self.morph203_block <= self.viridian_block, + "HardforkConfig blocks must be non-decreasing" + ); match block_number { n if n < self.bernoulli_block => SpecId::PRE_BERNOULLI, n if n < self.curie_block => SpecId::BERNOULLI, n if n < self.morph203_block => SpecId::CURIE, n if n < self.viridian_block => SpecId::MORPH203, _ => SpecId::VIRIDIAN, } }
133-135: Confirm slot initializations; consider explicit literal type.Setting IS_CURIE_SLOT and L1_BLOB_BASEFEE_SLOT to 1 looks intentional—please confirm semantics. Minor: make the literal type explicit for readability.
- (l1_gas_price_oracle::IS_CURIE_SLOT, EvmStorageSlot::new(U256::from(1))), - (l1_gas_price_oracle::L1_BLOB_BASEFEE_SLOT, EvmStorageSlot::new(U256::from(1))), + (l1_gas_price_oracle::IS_CURIE_SLOT, EvmStorageSlot::new(U256::from(1u8))), + (l1_gas_price_oracle::L1_BLOB_BASEFEE_SLOT, EvmStorageSlot::new(U256::from(1u8))),
147-147: Avoid temporary HashMap allocation on commit (tiny nit).Slightly cleaner and avoids constructing via array.
- db.commit(HashMap::from([(l1_gas_price_oracle_addr, l1_gas_price_oracle_acc)])); + let mut changes = HashMap::new(); + changes.insert(l1_gas_price_oracle_addr, l1_gas_price_oracle_acc); + db.commit(changes);prover/crates/primitives/src/lib.rs (1)
186-188: Authorization list accessor returns owned Vec.This forces allocation on every call. If hot, consider an additional accessor on concrete types that yields a slice (or iterator) to avoid churn; keep the owned Vec where ownership is required (builder path).
Also applies to: 397-399
prover/crates/primitives/src/types/authorization_list.rs (1)
90-100: Avoid panics on malformed parity/signature during conversion.Both Parity::try_from(...).unwrap() and Signature::from_rs_and_parity(...).unwrap() will panic on bad data. Prefer a fallible conversion and propagate an error, or at least add context.
Apply one of:
- Minimal: replace unwrap with expect for clearer crash logs.
- Better: add TryFrom with error; keep From only if inputs are trusted.
Example (minimal):
- let parity = alloy::primitives::Parity::try_from(v as u64).unwrap(); - let signature = Signature::from_rs_and_parity(auth.r, auth.s, parity).unwrap(); + let parity = alloy::primitives::Parity::try_from(v as u64) + .expect("authorization_list: invalid yParity (expected 0/1)"); + let signature = Signature::from_rs_and_parity(auth.r, auth.s, parity) + .expect("authorization_list: invalid signature r/s or parity");prover/crates/primitives/src/types/tx.rs (3)
178-180: Owned authorization_list on TxTrace.Returns Vec, which allocates per call. If this becomes hot, consider a dedicated conversion point (only where building TxEip7702) and otherwise avoid repeated clones.
242-244: rkyv deserialize unwrap.Unwrap will panic on corrupted archives. If these archives can be untrusted, validate before unwrap or convert to a Result at the call boundary. If trusted, add expect with message.
246-253: AuthorizationList rkyv deserialize unwrap.Same concern as above; prefer validation or an expect with context to ease debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
.github/workflows/prover.yml(1 hunks)contracts/src/deploy-config/holesky.ts(1 hunks)contracts/src/deploy-config/l1.ts(1 hunks)contracts/src/deploy-config/qanetl1.ts(1 hunks)prover/Cargo.toml(0 hunks)prover/README.md(1 hunks)prover/bin/host/src/main.rs(1 hunks)prover/bin/server/src/lib.rs(1 hunks)prover/crates/core/src/batch.rs(2 hunks)prover/crates/core/src/database.rs(4 hunks)prover/crates/core/src/error.rs(1 hunks)prover/crates/core/src/executor/builder.rs(2 hunks)prover/crates/core/src/executor/hooks.rs(1 hunks)prover/crates/core/src/executor/mod.rs(2 hunks)prover/crates/core/src/hardfork.rs(6 hunks)prover/crates/morph-executor/client/src/types/blob.rs(2 hunks)prover/crates/morph-executor/host/src/zstd_util.rs(1 hunks)prover/crates/primitives/src/lib.rs(6 hunks)prover/crates/primitives/src/types/authorization_list.rs(1 hunks)prover/crates/primitives/src/types/mod.rs(10 hunks)prover/crates/primitives/src/types/tx.rs(8 hunks)prover/crates/utils/src/metrics/registry.rs(1 hunks)prover/crates/utils/src/utils/debug.rs(1 hunks)prover/tests/bls12381/host/src/main.rs(1 hunks)prover/tests/keccak256/host/src/main.rs(1 hunks)prover/tests/zstd/host/src/main.rs(2 hunks)
💤 Files with no reviewable changes (1)
- prover/Cargo.toml
✅ Files skipped from review due to trivial changes (7)
- prover/crates/utils/src/metrics/registry.rs
- prover/crates/core/src/batch.rs
- prover/crates/utils/src/utils/debug.rs
- prover/crates/core/src/executor/builder.rs
- prover/crates/core/src/executor/hooks.rs
- prover/bin/server/src/lib.rs
- prover/crates/core/src/database.rs
🧰 Additional context used
🧬 Code graph analysis (7)
prover/tests/keccak256/host/src/main.rs (1)
prover/bin/host/src/lib.rs (2)
public_values(64-64)proof(100-100)
prover/tests/bls12381/host/src/main.rs (1)
prover/bin/host/src/lib.rs (2)
public_values(64-64)proof(100-100)
prover/crates/primitives/src/lib.rs (2)
prover/crates/primitives/src/types/tx.rs (11)
tx_hash(121-123)tx_hash(188-190)tx_hash(475-480)authorization_list(178-180)authorization_list(246-253)authorization_list(359-364)authorization_list(439-441)chain_id(162-164)chain_id(229-231)chain_id(261-266)chain_id(383-385)prover/crates/primitives/src/types/authorization_list.rs (4)
from(77-87)from(91-99)from(121-123)from(127-129)
prover/crates/core/src/executor/mod.rs (1)
prover/crates/primitives/src/types/authorization_list.rs (4)
from(77-87)from(91-99)from(121-123)from(127-129)
prover/crates/primitives/src/types/authorization_list.rs (2)
prover/crates/primitives/src/lib.rs (2)
signature(190-190)signature(401-403)prover/crates/primitives/src/types/tx.rs (2)
signature(182-184)signature(255-257)
prover/crates/primitives/src/types/mod.rs (3)
prover/crates/primitives/src/lib.rs (4)
authorization_list(187-187)authorization_list(397-399)to(172-172)to(377-379)prover/crates/primitives/src/types/tx.rs (8)
authorization_list(178-180)authorization_list(246-253)authorization_list(359-364)authorization_list(439-441)to(153-160)to(220-227)to(317-322)to(415-417)prover/crates/primitives/src/types/authorization_list.rs (4)
from(77-87)from(91-99)from(121-123)from(127-129)
prover/crates/primitives/src/types/tx.rs (2)
prover/crates/primitives/src/lib.rs (2)
authorization_list(187-187)authorization_list(397-399)prover/crates/primitives/src/types/authorization_list.rs (1)
deserialize(48-73)
🪛 Gitleaks (8.28.0)
contracts/src/deploy-config/l1.ts
[high] 20-20: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
contracts/src/deploy-config/holesky.ts
[high] 17-17: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
contracts/src/deploy-config/qanetl1.ts
[high] 17-17: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (32)
.github/workflows/prover.yml (1)
26-28: Clarify the toolchain setup and versioning strategy.Line 27 installs rustfmt from a hardcoded nightly version (
nightly-2024-07-07), while line 26 sets up the stable toolchain. This creates a potential mismatch: the stable toolchain may have a different rustfmt version than the nightly one being explicitly installed.Additionally, this workflow change appears orthogonal to the PR's primary objectives (go-ethereum version bump and SetCodeTx parsing). The enriched summary mentions alignment with "broader dependency updates," but the connection is unclear.
Consider:
- Clarifying why a specific nightly version is needed instead of using the stable toolchain's rustfmt or the latest nightly.
- Ensuring the hardcoded nightly date (
nightly-2024-07-07) won't cause maintenance issues if that version becomes unavailable.- Documenting the rationale for this change if it is intentional.
prover/crates/core/src/error.rs (1)
1-1: LGTM! Clean import consolidation.The import path consolidation correctly groups related types from
revm::primitivesand all imported types are actively used throughout the file.prover/crates/morph-executor/host/src/zstd_util.rs (1)
29-31: LGTM: Formatting improvement.The formatting change to single-line calls for these parameter settings is clean and improves consistency. No functional changes.
contracts/src/deploy-config/l1.ts (1)
20-20: Verify the new programVkey value is correct.The programVkey update appears clean and is consistent across all deploy configs (l1.ts, holesky.ts, qanetl1.ts). This likely relates to updating the ZK verification system to support the new transaction types (SetCodeTx/EIP-7702).
Please confirm that this new verification key matches the intended program verifier for the updated system.
Note: The Gitleaks alert flagging this as a "Generic API Key" is a false positive. Program verification keys are public cryptographic parameters, not secret credentials.
contracts/src/deploy-config/holesky.ts (1)
17-17: LGTM - consistent with other deploy configs.The programVkey update matches the changes in l1.ts and qanetl1.ts, ensuring consistency across network configurations.
contracts/src/deploy-config/qanetl1.ts (1)
17-17: LGTM - consistent with other deploy configs.The programVkey update matches the changes in l1.ts and holesky.ts, maintaining consistency across all network configurations.
prover/tests/keccak256/host/src/main.rs (1)
24-24: LGTM! Formatting consolidation improves conciseness.The multi-line println! statements and method chaining have been consolidated to single lines without affecting behavior.
Also applies to: 27-27, 35-35, 38-38
prover/tests/zstd/host/src/main.rs (2)
2-2: LGTM! Import consolidation is appropriate.The expanded import statement properly includes
File,Read, andPathwhich are used throughout the file (lines 11-14, 52-55).
26-26: LGTM! Formatting consolidation improves conciseness.Consistent with the formatting changes in the other test files.
Also applies to: 29-29, 37-37, 40-40
prover/crates/core/src/executor/mod.rs (2)
9-10: LGTM! Clean import additions for EIP-7702 authorization support.The aliasing of
AuthorizationList as RevmAuthorizationListand the addition ofSignedAuthorizationare appropriate for the new authorization handling functionality.Also applies to: 14-16
121-121: LGTM! Proper integration of authorization list into transaction environment.The authorization list is correctly populated using the conversion helper. This properly wires EIP-7702 authorization data into the REVM transaction environment.
prover/README.md (2)
7-7: LGTM! Documentation link updated.The SP1 installation link has been updated to point to the current documentation site.
12-17: File paths are correct—no changes needed.Both required files exist at the expected locations (
prover/configs/4844_trusted_setup.txtandprover/testdata/viridian/eip7702_traces.json). The relative path change from../../to./correctly reflects that commands run from theprover/directory, which is the natural working directory when following a README in that location. The documentation is clear as-is.prover/bin/host/src/main.rs (1)
13-13: File verification passed.The test data file exists at the correct location and is properly accessible from the binary's execution context. When the
provebinary runs from theprover/directory (standard Cargo workspace execution), the relative path./testdata/viridian/eip7702_traces.jsoncorrectly resolves to./prover/testdata/viridian/eip7702_traces.json. No changes needed.prover/crates/morph-executor/client/src/types/blob.rs (3)
78-82: Verify that silently breaking on unsupported transaction types is the intended behavior.When an unsupported transaction type is encountered, the function breaks the loop and returns whatever transactions were decoded up to that point. While this is consistent with the zero-byte handling pattern (lines 69-73), it could silently mask issues if an unsupported type appears unexpectedly in the middle of a batch.
Consider whether this should return an error or at least log a warning to indicate incomplete decoding.
112-112: Good addition for observability.The logging statement provides useful feedback on the number of successfully decoded transactions, which aids in debugging and monitoring.
79-79: Transaction type code 0x04 for SetCodeTx (EIP-7702) is consistent across implementations.The Rust code at
prover/crates/morph-executor/client/src/types/blob.rsline 79 correctly accepts0x04as a valid transaction type. The Go implementation atnode/types/blob.goline 157 useseth.SetCodeTxTypefrom thegithub.com/morph-l2/go-ethereum/core/typespackage, which is the standard constant for this transaction type.Per EIP-7702 specification,
0x04is the official transaction type code for SetCodeTx. Both implementations are aligned:
- Rust: Accepts
0x01(AccessListTxType),0x02(DynamicFeeTxType), and0x04(SetCodeTxType)- Go: Handles
eth.AccessListTxType,eth.DynamicFeeTxType, andeth.SetCodeTxType(which equals0x04)prover/crates/primitives/src/lib.rs (4)
5-5: Public API surface change: TxEip7702 import and SignedAuthorization re-export.Confirm we intend to expose EIP‑7702 types at the crate root (semver signal to downstreams). If not strictly needed, consider keeping them internal.
Also applies to: 20-21
126-128: DA header hash: base_fee_per_gas defaulting to 0.Using unwrap_or_default() hashes None as 0. Please confirm this matches the DA header spec for pre‑London blocks to avoid cross‑impl mismatches.
135-138: hash_l1_msg iteration is clean and correct.Filtering L1 txs and hashing their tx_hash bytes looks good.
244-259: TxEip7702 construction: field mapping and presence.Looks correct, but please double‑check against alloy’s TxEip7702: that access_list is indeed a field for 7702 and no field order/units mismatch exists. Also verify that signature() covers the 7702 signing domain as implemented in alloy.
prover/crates/primitives/src/types/authorization_list.rs (4)
7-26: ArchivedSignedAuthorization structure LGTM.Derives and field choices align with rkyv/serde needs; using U8 for y‑parity is appropriate.
28-44: JSON compatibility: field names.chainId/yParity mappings look right. Please confirm upstream JSON producers emit yParity (0/1) rather than legacy v to avoid silent misreads.
Also applies to: 46-74
76-88: From → ArchivedSignedAuthorization LGTM.into_parts + signature extraction is correct.
120-130: Vec/Slice conversions are fine.Round‑trips are straightforward; no extra clones beyond what’s necessary.
prover/crates/primitives/src/types/mod.rs (3)
7-10: New module and re‑exports.Authorization list module exposure looks correct; matches usages in tx/lib.
156-164: flatten(): iterator refactor LGTM.Zero‑copy iteration over proofs with Bytes::as_ref is neat and clear.
413-416: Archived vs live tx equivalence test is strong.Zipping try_build_typed_tx() outputs is a good guard for archival fidelity.
prover/crates/primitives/src/types/tx.rs (4)
109-112: Trace field for authorizationList.Serde rename + DefaultOnNull is the right choice to keep older traces compatible.
359-363: Transaction::authorization_list implementation LGTM.Returns EIP‑7702 slice for envelopes, None for L1 messages.
499-502: effective_gas_price for EIP‑7702.Using effective_tip_per_gas + base fee mirrors EIP‑1559 logic; good match for 7702’s fee model.
531-534: data() supports EIP‑7702.Extracting input via as_eip7702().unwrap() is consistent with other variants.
pump go-ethereum version & support parsing setCodeTx (#782)
Summary by CodeRabbit
New Features
Refactor
Chores