feat(node): add morph-reth bin and node wire #28
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "bin/morph-reth (CLI)"
participant Builder as "ComponentsBuilder"
participant Components as "Components (Consensus, Executor, Payload, Pool)"
participant AddOns as "MorphAddOns / RealMorphL2EngineApi"
participant Node as "MorphNode (running)"
CLI->>Builder: build components_builder(args, config)
Builder->>Components: construct consensus, executor, payload, pool
Components-->>Builder: components ready
Builder->>AddOns: register RPC & Engine APIs (RealMorphL2EngineApi)
AddOns-->>Builder: add-ons registered
Builder->>Node: assemble and launch node
Node->>AddOns: start RPC & Engine servers (handle requests)
AddOns->>Node: assemble/validate/submit L2 blocks (calls into payload_builder & provider)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/payload/types/src/attributes.rs (1)
252-264:⚠️ Potential issue | 🟡 MinorTest doesn't isolate what it claims to verify.
create_test_attributes()usesB256::random()andAddress::random(), soattrs1andattrs2already have differentprev_randaoandsuggested_fee_recipient. The IDs would differ regardless of thetransactionsfield, making this test unable to catch a regression where transactions are accidentally excluded from the payload ID computation.Suggested fix: share the same base attributes
fn test_payload_id_different_with_transactions() { let parent = B256::random(); - let attrs1 = create_test_attributes(); - let mut attrs2 = create_test_attributes(); + let attrs1 = create_test_attributes(); + let mut attrs2 = attrs1.clone(); attrs2.transactions = Some(vec![Bytes::from(vec![0x01])]);crates/rpc/src/eth/transaction.rs (1)
169-172:⚠️ Potential issue | 🟡 MinorFix redundant clone flagged by CI.
inneris not used after this call, so the.clone()is unnecessary — consumeinnerdirectly.Proposed fix
- let inner_tx_env = inner - .clone() - .try_into_tx_env(evm_env) + let inner_tx_env = inner + .try_into_tx_env(evm_env) .map_err(EthApiError::from)?;
🧹 Nitpick comments (7)
.gitignore (1)
4-7: LGTM! Clear and well-organized additions.The new
local-test/entry with a descriptive comment is helpful for keeping local test artifacts out of version control.One optional note:
/docsuses an absolute path pattern (matches only at repository root), whilelocal-test/uses a relative pattern (matches anywhere). Iflocal-test/should only be ignored at the root level for consistency, consider using/local-test/instead.crates/node/Cargo.toml (1)
46-48: Nit:clapis miscategorized under "Async".
clapis a CLI argument parser, not an async utility. Consider moving it to a separate section (e.g.,# CLIor# Other).crates/evm/src/engine.rs (1)
34-47: Consider avoiding the intermediateVecallocation.The
.collect::<Vec<_>>().into_iter()on lines 43–44 materializes all(Arc, index)pairs upfront. For blocks with many transactions, this allocates unnecessarily. You could return theMapiterator directly if you restructure the ownership slightly, or just note that this is bounded by block transaction count (which is practically small).That said, this works correctly and the per-item cost is just an Arc clone (cheap).
♻️ Optional: avoid intermediate collect
fn tx_iterator_for_payload( &self, payload: &MorphExecutionData, ) -> Result<impl ExecutableTxIterator<Self>, Self::Error> { let block = payload.block.clone(); - // Create an iterator over (block, index) pairs - // Use into_iter() to get a proper Iterator implementation - let transactions = (0..payload.block.body().transactions.len()) - .map(move |i| (block.clone(), i)) - .collect::<Vec<_>>() - .into_iter(); + let len = payload.block.body().transactions.len(); + let transactions = (0..len).map(move |i| (block.clone(), i)); Ok((transactions, RecoveredInBlock::new)) }crates/node/src/components/pool.rs (1)
88-115: Sync tests don't need#[tokio::test].The first three tests (
test_validate_oversized_transaction,test_l1_message_type_id,test_morph_tx_type_id) don't use any async operations. Using#[tokio::test]works but is misleading and adds unnecessary runtime overhead.bin/morph-reth/src/main.rs (1)
22-24:std::env::set_varinunsafeis correct here but worth a safety comment.Since Rust 1.66,
set_varisunsafedue to thread-safety concerns. Calling it at the top ofmain()before any threads spawn is safe, but a brief// SAFETY:comment would document the invariant.📝 Suggested safety comment
if std::env::var_os("RUST_BACKTRACE").is_none() { - unsafe { std::env::set_var("RUST_BACKTRACE", "1") }; + // SAFETY: Called before any threads are spawned, so this is single-threaded. + unsafe { std::env::set_var("RUST_BACKTRACE", "1") }; }crates/node/src/node.rs (1)
207-213: Consider documenting theunwrap()safety invariant.The
unwrap()onduration_since(UNIX_EPOCH)is practically safe (only fails if the system clock is before 1970), but a brief// SAFETY:orexpect()with a message would make the assumption explicit.Proposed change
fn unix_timestamp_now() -> u64 { std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) - .unwrap() + .expect("system clock is before UNIX epoch") .as_secs() }crates/rpc/src/eth/transaction.rs (1)
354-365:access_listis cloned twice — once at line 354, again at line 365.The clone at line 365 is needed because
access_listis reused at line 382 in theNonebranch. Consider passing by reference totry_build_morph_tx_from_envto avoid the extra allocation, or restructure so theSomebranch consumes it.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@crates/engine-api/src/builder.rs`:
- Around line 420-429: new_l2_block currently calls validate_l2_block and only
logs a warning on validation.success == false, but still proceeds to accept the
block; change new_l2_block to stop and return an error when validation.success
is false (or make this behavior conditional on an explicit config flag), i.e.,
after calling validate_l2_block in new_l2_block, if validation.success == false
then return an Err containing a descriptive message and the block hash (or check
an explicit self.<validate_blocks_flag> to allow override), and ensure any
Ok/Err from validate_l2_block is propagated correctly instead of always
continuing.
- Around line 250-259: The current Err branch for sealed_header_by_hash marks
the block as validated and returns GenericResponse { success: true }, which
wrongly treats DB errors as successful validation; update the Err arm in the
function containing sealed_header_by_hash so it does NOT call
self.validation_cache.insert(data.hash, ()), and instead either propagate the
error (return Err(e) or map it into the function's error type) or return
GenericResponse { success: false } to force a retry; ensure the tracing::warn
stays (or is upgraded to error) and reference the same symbols
(sealed_header_by_hash, self.validation_cache.insert, GenericResponse { success:
true }) when making the change so transient DB errors are not cached as valid.
- Line 566: The current cast base_fee_per_gas: data.base_fee_per_gas.map(|f| f
as u64) silently truncates Option<u128> values above u64::MAX; replace the
unchecked cast with a fallible conversion and propagate or handle the error
(e.g., use f.try_into() and transpose() to produce Result<Option<u64>, _> and
return or map the error), so update the construction for base_fee_per_gas
(referencing data.base_fee_per_gas and the builder that constructs the header)
to use try_into() and proper error handling instead of as u64.
🧹 Nitpick comments (5)
crates/node/Cargo.toml (1)
47-49: Misleading section comment:eyreandclapare not async crates.
eyreis an error-handling library andclapis a CLI argument parser. Consider renaming this section to something like# Miscor# CLI / Utilities, or grouping them with their actual purpose.Proposed fix
-# Async +# Misc eyre.workspace = true clap.workspace = truecrates/evm/src/config.rs (1)
67-105: Consider extracting sharedCfgEnv+ blob placeholder construction.Both
evm_envandnext_evm_envduplicate theCfgEnv::default().with_chain_id(...).with_spec(spec)pattern and the identicalBlobExcessGasAndPriceplaceholder block. A small helper (e.g.,fn make_cfg_env(&self, spec) -> CfgEnv<MorphHardfork>) would reduce the duplication and make it easier to update the blob placeholder logic in one place if Morph ever changes its stance on EIP-4844.♻️ Example helper extraction
+ fn make_cfg_env(&self, spec: MorphHardfork) -> CfgEnv<MorphHardfork> { + CfgEnv::<MorphHardfork>::default() + .with_chain_id(self.chain_spec().chain().id()) + .with_spec(spec) + } + + fn blob_placeholder() -> Option<BlobExcessGasAndPrice> { + Some(BlobExcessGasAndPrice { + excess_blob_gas: 0, + blob_gasprice: 1, + }) + }crates/engine-api/src/builder.rs (3)
262-271: Redundant parent hash check.
sealed_header_by_hash(data.parent_hash)fetches the header bydata.parent_hash, soparent.hash()will always equaldata.parent_hash. This branch is dead code.
126-132:unwrap()onSystemTime::now()can panic if clock is before UNIX epoch.This is extremely unlikely in practice but violates defensive coding. Consider using
unwrap_or(0)orunwrap_or_else(|_| parent.timestamp() + 1)as a safe fallback.
46-48: Unbounded validation cache can grow indefinitely.
DashMap<B256, ()>is never evicted. Over time this will consume increasing memory as every validated block hash is retained. Consider using an LRU cache or periodic eviction.
| Err(e) => { | ||
| tracing::warn!( | ||
| target: "morph::engine", | ||
| error = %e, | ||
| "failed to query parent header" | ||
| ); | ||
| // Database error - but don't fail, just skip validation | ||
| self.validation_cache.insert(data.hash, ()); | ||
| return Ok(GenericResponse { success: true }); | ||
| } |
There was a problem hiding this comment.
DB errors silently pass validation as success: true.
When sealed_header_by_hash returns an Err, the block is cached as validated and success: true is returned. A transient database error (e.g., I/O glitch, corrupted page) would cause an unvalidated block to be permanently cached as valid. This is a different situation from "parent not found" — a DB error means we don't know whether the parent exists.
At minimum, don't cache the result on error and consider returning success: false or propagating the error so the caller can retry.
Proposed fix
Err(e) => {
tracing::warn!(
target: "morph::engine",
error = %e,
"failed to query parent header"
);
- // Database error - but don't fail, just skip validation
- self.validation_cache.insert(data.hash, ());
- return Ok(GenericResponse { success: true });
+ // Database error - can't validate, report failure so caller can retry
+ return Ok(GenericResponse { success: false });
}🤖 Prompt for AI Agents
In `@crates/engine-api/src/builder.rs` around lines 250 - 259, The current Err
branch for sealed_header_by_hash marks the block as validated and returns
GenericResponse { success: true }, which wrongly treats DB errors as successful
validation; update the Err arm in the function containing sealed_header_by_hash
so it does NOT call self.validation_cache.insert(data.hash, ()), and instead
either propagate the error (return Err(e) or map it into the function's error
type) or return GenericResponse { success: false } to force a retry; ensure the
tracing::warn stays (or is upgraded to error) and reference the same symbols
(sealed_header_by_hash, self.validation_cache.insert, GenericResponse { success:
true }) when making the change so transient DB errors are not cached as valid.
| // 4. Optionally validate the block content | ||
| if let Ok(validation) = self.validate_l2_block(data.clone()).await { | ||
| if !validation.success { | ||
| tracing::warn!( | ||
| target: "morph::engine", | ||
| block_hash = %data.hash, | ||
| "block content validation failed" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Block import proceeds after validation failure.
new_l2_block calls validate_l2_block but only logs a warning when validation fails — it still converts and "accepts" the block. While the comment says "optionally validate," silently importing a block that failed content validation seems risky. Consider at least returning an error when validation.success == false, or making the "optional" behavior an explicit configuration flag.
🤖 Prompt for AI Agents
In `@crates/engine-api/src/builder.rs` around lines 420 - 429, new_l2_block
currently calls validate_l2_block and only logs a warning on validation.success
== false, but still proceeds to accept the block; change new_l2_block to stop
and return an error when validation.success is false (or make this behavior
conditional on an explicit config flag), i.e., after calling validate_l2_block
in new_l2_block, if validation.success == false then return an Err containing a
descriptive message and the block hash (or check an explicit
self.<validate_blocks_flag> to allow override), and ensure any Ok/Err from
validate_l2_block is propagated correctly instead of always continuing.
| extra_data: Default::default(), | ||
| mix_hash: alloy_primitives::B256::ZERO, | ||
| nonce: alloy_primitives::B64::ZERO, | ||
| base_fee_per_gas: data.base_fee_per_gas.map(|f| f as u64), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the context around line 566 in the builder.rs file
sed -n '550,580p' crates/engine-api/src/builder.rs | cat -nRepository: morph-l2/morph-reth
Length of output: 1504
🏁 Script executed:
# Now let's find what 'data' is and its type definition around that area
sed -n '530,570p' crates/engine-api/src/builder.rs | cat -nRepository: morph-l2/morph-reth
Length of output: 1920
🏁 Script executed:
# Search for the struct containing the base_fee_per_gas field
rg 'struct.*Data|base_fee_per_gas' crates/engine-api/src/builder.rs -A3 -B3Repository: morph-l2/morph-reth
Length of output: 411
🏁 Script executed:
# Search for imports and type definitions related to U256 and base_fee_per_gas across the engine-api crate
rg 'base_fee_per_gas' crates/engine-api/ --type rust -B2 -A2Repository: morph-l2/morph-reth
Length of output: 497
🏁 Script executed:
# Search for ExecutableL2Data struct definition
rg 'struct ExecutableL2Data' crates/engine-api/ -A20Repository: morph-l2/morph-reth
Length of output: 45
🏁 Script executed:
# Also check if it's imported from elsewhere
rg 'ExecutableL2Data' crates/engine-api/src/builder.rs -B5 -A2 | head -40Repository: morph-l2/morph-reth
Length of output: 1313
🏁 Script executed:
# Search more broadly for the struct definition
find crates/engine-api -name '*.rs' -exec grep -l 'struct ExecutableL2Data' {} \;Repository: morph-l2/morph-reth
Length of output: 45
🏁 Script executed:
# Find the morph_payload_types crate
find . -name 'morph_payload_types' -type d | head -5Repository: morph-l2/morph-reth
Length of output: 45
🏁 Script executed:
# Also search for ExecutableL2Data definition in the entire codebase
rg 'struct ExecutableL2Data' --type rustRepository: morph-l2/morph-reth
Length of output: 139
🏁 Script executed:
# Search for pub struct ExecutableL2Data with various patterns
rg 'ExecutableL2Data' --type rust -B5 -A15 | grep -A15 'pub struct ExecutableL2Data'Repository: morph-l2/morph-reth
Length of output: 1267
🏁 Script executed:
cat crates/payload/types/src/executable_l2_data.rsRepository: morph-l2/morph-reth
Length of output: 6184
Replace silent truncation with explicit overflow handling for base_fee_per_gas conversion.
data.base_fee_per_gas is Option<u128>, and the cast as u64 will silently truncate values exceeding u64::MAX (18,446,744,073,709,551,615). Use try_into() with error handling instead to prevent incorrect header construction:
base_fee_per_gas: data.base_fee_per_gas.map(|f| f.try_into()).transpose()?,or handle the conversion error explicitly.
🤖 Prompt for AI Agents
In `@crates/engine-api/src/builder.rs` at line 566, The current cast
base_fee_per_gas: data.base_fee_per_gas.map(|f| f as u64) silently truncates
Option<u128> values above u64::MAX; replace the unchecked cast with a fallible
conversion and propagate or handle the error (e.g., use f.try_into() and
transpose() to produce Result<Option<u64>, _> and return or map the error), so
update the construction for base_fee_per_gas (referencing data.base_fee_per_gas
and the builder that constructs the header) to use try_into() and proper error
handling instead of as u64.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@crates/engine-api/src/builder.rs`:
- Around line 126-132: The current assemble_l2_block uses SystemTime::now()
causing non-deterministic timestamps; change assemble_l2_block to accept a
timestamp via AssembleL2BlockParams (add a timestamp field) and use that value
instead of calling SystemTime::now(), and update callers accordingly;
specifically, have validate_l2_block supply the original timestamp from the
incoming ExecutableL2Data when it replays the block so the rebuild uses the
exact same timestamp and produces deterministic state roots/gas/hash
comparisons.
- Around line 541-545: The current code silently falls back to Bloom::ZERO when
data.logs_bloom.len() != 256; change this to validate and return an error
instead of masking bad data: check data.logs_bloom.len() and if it is not 256
return an Err (with a clear message including the bad length and context) rather
than using Bloom::ZERO, replacing the else branch where logs_bloom is set;
update the enclosing function (the builder creation function in this file) to
return a Result if it does not already and propagate the error to callers so
malformed bloom data is surfaced; keep the use of Bloom::from_slice on the
success path.
- Around line 547-553: Replace the incorrect zero values: set Header.ommers_hash
to EMPTY_OMMER_ROOT_HASH (imported from alloy_consensus) instead of
alloy_primitives::B256::ZERO, and compute Header.transactions_root from the
block transactions rather than leaving it as B256::ZERO—use
proofs::calculate_transaction_root(data.transactions) (as done in
crates/evm/src/assemble.rs) before calling seal_unchecked so that seal_unchecked
and subsequent block.ensure_transaction_root_valid() see the correct
transactions_root.
🧹 Nitpick comments (3)
crates/evm/src/config.rs (1)
74-96: Consider extracting sharedCfgEnv+ blob-placeholder construction.Both
evm_envandnext_evm_envduplicate theCfgEnvconstruction (lines 35–37 vs 74–76) and theblob_excess_gas_and_priceplaceholder. A small helper (e.g.,fn make_cfg_env(&self, spec) -> CfgEnv<MorphHardfork>and a constant for the blob placeholder) would reduce duplication and ensure both paths stay in sync.crates/engine-api/src/builder.rs (2)
258-267: Redundant parent-hash check — always passes.
parentwas fetched viasealed_header_by_hash(data.parent_hash)(line 229), soparent.hash()is guaranteed to equaldata.parent_hash. This comparison is dead code.
635-657: Consider removing dead legacy builder or marking it#[deprecated].The comment says this builder is "superseded" and "not used in practice," and the
add_ons.rssnippet confirmsNoopEngineApiBuilderis used instead. Keeping dead code without a deprecation marker or#[cfg(test)]gate adds maintenance burden.
| let timestamp = std::cmp::max( | ||
| parent.timestamp() + 1, | ||
| std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .unwrap() | ||
| .as_secs(), | ||
| ); |
There was a problem hiding this comment.
Non-deterministic timestamp will cause validation rebuilds to always mismatch.
assemble_l2_block computes timestamp from SystemTime::now(). When validate_l2_block (line 275) calls assemble_l2_block to re-execute the block, a different wall-clock time will produce a different timestamp, leading to different state roots, gas usage, and block hashes. The rebuild-and-compare validation strategy (lines 290–333) will effectively never match.
The timestamp should either be passed in via AssembleL2BlockParams (so validation can replay with the original timestamp), or validate_l2_block should skip calling assemble_l2_block and instead re-execute with the exact parameters from the incoming ExecutableL2Data.
🤖 Prompt for AI Agents
In `@crates/engine-api/src/builder.rs` around lines 126 - 132, The current
assemble_l2_block uses SystemTime::now() causing non-deterministic timestamps;
change assemble_l2_block to accept a timestamp via AssembleL2BlockParams (add a
timestamp field) and use that value instead of calling SystemTime::now(), and
update callers accordingly; specifically, have validate_l2_block supply the
original timestamp from the incoming ExecutableL2Data when it replays the block
so the rebuild uses the exact same timestamp and produces deterministic state
roots/gas/hash comparisons.
| let logs_bloom = if data.logs_bloom.len() == 256 { | ||
| Bloom::from_slice(&data.logs_bloom) | ||
| } else { | ||
| Bloom::ZERO | ||
| }; |
There was a problem hiding this comment.
Silent fallback to Bloom::ZERO on unexpected bloom length.
If data.logs_bloom has an unexpected length (not 256 bytes), the code silently uses Bloom::ZERO instead of returning an error. This could mask corrupted or malformed block data. Consider returning an error for invalid bloom length.
🤖 Prompt for AI Agents
In `@crates/engine-api/src/builder.rs` around lines 541 - 545, The current code
silently falls back to Bloom::ZERO when data.logs_bloom.len() != 256; change
this to validate and return an error instead of masking bad data: check
data.logs_bloom.len() and if it is not 256 return an Err (with a clear message
including the bad length and context) rather than using Bloom::ZERO, replacing
the else branch where logs_bloom is set; update the enclosing function (the
builder creation function in this file) to return a Result if it does not
already and propagate the error to callers so malformed bloom data is surfaced;
keep the use of Bloom::from_slice on the success path.
| let inner = Header { | ||
| parent_hash: data.parent_hash, | ||
| ommers_hash: alloy_primitives::B256::ZERO, // No ommers in L2 | ||
| beneficiary: data.miner, | ||
| state_root: data.state_root, | ||
| transactions_root: alloy_primitives::B256::ZERO, // Will be calculated when sealing | ||
| receipts_root: data.receipts_root, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if EMPTY_OMMER_ROOT_HASH exists in alloy_consensus or related crates
rg 'EMPTY_OMMER_ROOT_HASH|EMPTY_UNCLES_HASH' --type rust -l
# Check the builder.rs file around the problematic lines
fd builder.rs --type f -x cat -n {} \; | grep -A 30 -B 5 'transactions_root'
# Check what seal_unchecked does
rg 'seal_unchecked' --type rust -A 10 -B 2Repository: morph-l2/morph-reth
Length of output: 2665
🏁 Script executed:
# Examine the full builder.rs file to understand context
wc -l crates/engine-api/src/builder.rsRepository: morph-l2/morph-reth
Length of output: 99
🏁 Script executed:
# Search for imports and constants related to ommer/uncle hashing
rg 'EMPTY.*HASH|empty.*ommer|empty.*uncle' --type rust -iRepository: morph-l2/morph-reth
Length of output: 2120
🏁 Script executed:
# Search for transactions_root computation patterns
rg 'transactions_root' --type rust -B 3 -A 3 -g '!target/'
# Check the ExecutableL2Data structure to see if it has transaction info
rg 'struct ExecutableL2Data|impl.*ExecutableL2Data' --type rust -A 10Repository: morph-l2/morph-reth
Length of output: 4368
🏁 Script executed:
# Check what fields are available in the data being converted
rg 'fn executable_data_to_header' --type rust -A 50 | head -80Repository: morph-l2/morph-reth
Length of output: 3607
🏁 Script executed:
# Look for transaction root calculation using proofs or merkle trees
rg 'calculate.*root|compute.*root|tx.*root|transaction.*root' --type rust -iRepository: morph-l2/morph-reth
Length of output: 3091
🏁 Script executed:
# Get the full ExecutableL2Data struct definition
rg 'pub struct ExecutableL2Data' --type rust -A 50Repository: morph-l2/morph-reth
Length of output: 3854
🏁 Script executed:
# Check if there's a pattern where transactions_root is expected to come from elsewhere (like data.hash)
rg 'data\.hash|hash.*transactions' --type rust -B 3 -A 3 -g '!target/'Repository: morph-l2/morph-reth
Length of output: 5926
🏁 Script executed:
# Check if L2 has special validation rules that skip transactions_root
rg 'ensure_transaction_root|transaction.*root.*validation|skip.*transaction' --type rust -B 2 -A 5Repository: morph-l2/morph-reth
Length of output: 4241
🏁 Script executed:
# Check the validation logic to see if L2 blocks are exempt from transactions_root checking
rg 'fn validate_block_content|validate.*header|transactions_root' --type rust -B 5 -A 10 | head -100Repository: morph-l2/morph-reth
Length of output: 6783
🏁 Script executed:
# Look at the actual block sealing and assembly logic to understand the intent
sed -n '520,580p' crates/engine-api/src/builder.rsRepository: morph-l2/morph-reth
Length of output: 2175
Use EMPTY_OMMER_ROOT_HASH instead of B256::ZERO for ommers_hash, and calculate transactions_root from the transaction list.
-
ommers_hash(line 549): Must beEMPTY_OMMER_ROOT_HASH(the keccak256 hash of an empty RLP-encoded list), notB256::ZERO. The codebase validation enforces this check:header.ommers_hash() != EMPTY_OMMER_ROOT_HASHwill fail.EMPTY_OMMER_ROOT_HASHis exported fromalloy_consensus. -
transactions_root(line 552): The comment says "will be calculated when sealing," butseal_uncheckedon line 529 only stamps the provided hash without recalculating any fields. This leavestransactions_rootas permanentlyB256::ZERO, which will fail validation:block.ensure_transaction_root_valid()will reject the block. Calculate it fromdata.transactionsusingproofs::calculate_transaction_root()as shown incrates/evm/src/assemble.rs.
🤖 Prompt for AI Agents
In `@crates/engine-api/src/builder.rs` around lines 547 - 553, Replace the
incorrect zero values: set Header.ommers_hash to EMPTY_OMMER_ROOT_HASH (imported
from alloy_consensus) instead of alloy_primitives::B256::ZERO, and compute
Header.transactions_root from the block transactions rather than leaving it as
B256::ZERO—use proofs::calculate_transaction_root(data.transactions) (as done in
crates/evm/src/assemble.rs) before calling seal_unchecked so that seal_unchecked
and subsequent block.ensure_transaction_root_valid() see the correct
transactions_root.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@crates/chainspec/src/spec.rs`:
- Around line 36-56: The conversion of genesis.base_fee_per_gas in
make_genesis_header silently turns overflowed values into 0 (via .map(|b|
b.try_into().unwrap_or(0))); change this to surface the error instead of masking
it: update make_genesis_header (and possibly its callers) to handle conversion
failures from Genesis::base_fee_per_gas by either returning a
Result<MorphHeader, Error> or by logging a clear warning/error when b.try_into()
fails, and use a safe fallback only if explicitly desired. Locate the
base_fee_per_gas mapping in make_genesis_header and replace the unwrap_or(0)
usage with explicit error handling (e.g., match or map_err) so overflow is
reported (or propagated) rather than silently producing 0.
- Around line 62-104: The hardfork assembly logic in the From<Genesis> for
MorphChainSpec impl is duplicated; refactor that impl to call the existing
build_morph_hardforks_from_genesis(genesis) helper instead of reimplementing
block_forks/time_forks and Prague insertion: replace the duplicated construction
in the From<Genesis> implementation with a call to
build_morph_hardforks_from_genesis(genesis) and use its returned ChainHardforks
when building the MorphChainSpec, removing the redundant code while preserving
the Prague-at-Emerald insertion and any unwrap/default handling present in
build_morph_hardforks_from_genesis.
In `@crates/payload/types/Cargo.toml`:
- Around line 40-42: The serde feature in Cargo.toml is currently a no-op
causing inconsistent JSON formatting because MorphPayloadAttributes has
unconditional #[derive(Serialize, Deserialize)] while
MorphPayloadAttributes.inner is guarded with #[cfg_attr(feature = "serde",
serde(flatten))]; fix this by removing the cfg_attr guard and making
serde(flatten) unconditional on MorphPayloadAttributes.inner (or alternatively
make the derive conditional on feature "serde" throughout attributes.rs), i.e.,
update attributes.rs to either (preferred) remove cfg_attr and use
serde(flatten) directly on inner so flattening always occurs, or wrap the
Serialize/Deserialize derives with cfg(feature = "serde") to make the feature
meaningful.
🧹 Nitpick comments (3)
crates/chainspec/src/spec.rs (1)
106-115: Duplicate section comment.Lines 106–108 and 113–115 both say "Chain Specification Parser (CLI)". Remove the first one.
Fix
-// ============================================================================= -// Chain Specification Parser (CLI) -// ============================================================================= - /// Chains supported by Morph. First value should be used as the default. pub const SUPPORTED_CHAINS: &[&str] = &["mainnet", "hoodi"];crates/evm/src/config.rs (2)
35-56: Consider extracting sharedCfgEnvand blob-placeholder construction.Both
evm_envandnext_evm_envbuild theCfgEnvandblob_excess_gas_and_priceidentically. A small helper would reduce duplication and keep the placeholder logic in one place.♻️ Example helper extraction
+fn morph_cfg_env(chain_id: u64, spec: MorphHardfork) -> CfgEnv<MorphHardfork> { + CfgEnv::<MorphHardfork>::default() + .with_chain_id(chain_id) + .with_spec_and_mainnet_gas_params(spec) +} + +/// Morph doesn't support EIP-4844 blob transactions, but when SpecId >= CANCUN, +/// revm requires `blob_excess_gas_and_price` to be set. +fn morph_blob_excess_gas_and_price() -> Option<BlobExcessGasAndPrice> { + Some(BlobExcessGasAndPrice { excess_blob_gas: 0, blob_gasprice: 1 }) +}Also applies to: 74-96
82-82: Potentialu64overflow onparent.number() + 1.If
parent.number()isu64::MAX, this wraps. In practice block numbers won't reach that, but achecked_add(1)orsaturating_add(1)would be more defensive.
| pub(crate) fn make_genesis_header(genesis: &Genesis, state_root: B256) -> MorphHeader { | ||
| let inner = Header { | ||
| gas_limit: genesis.gas_limit, | ||
| difficulty: genesis.difficulty, | ||
| nonce: genesis.nonce.into(), | ||
| extra_data: genesis.extra_data.clone(), | ||
| state_root, | ||
| timestamp: genesis.timestamp, | ||
| mix_hash: genesis.mix_hash, | ||
| beneficiary: genesis.coinbase, | ||
| base_fee_per_gas: genesis.base_fee_per_gas.map(|b| b.try_into().unwrap_or(0)), | ||
| withdrawals_root: None, | ||
| parent_beacon_block_root: None, | ||
| blob_gas_used: None, | ||
| excess_blob_gas: None, | ||
| requests_hash: None, | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| MorphHeader::from(inner) | ||
| } |
There was a problem hiding this comment.
base_fee_per_gas conversion silently truncates to 0 on overflow.
Line 46: genesis.base_fee_per_gas.map(|b| b.try_into().unwrap_or(0)) — if the genesis JSON specifies a baseFeePerGas value larger than u64::MAX, it silently becomes 0. While unlikely in practice, this could mask configuration errors. Consider logging a warning or returning an error.
🤖 Prompt for AI Agents
In `@crates/chainspec/src/spec.rs` around lines 36 - 56, The conversion of
genesis.base_fee_per_gas in make_genesis_header silently turns overflowed values
into 0 (via .map(|b| b.try_into().unwrap_or(0))); change this to surface the
error instead of masking it: update make_genesis_header (and possibly its
callers) to handle conversion failures from Genesis::base_fee_per_gas by either
returning a Result<MorphHeader, Error> or by logging a clear warning/error when
b.try_into() fails, and use a safe fallback only if explicitly desired. Locate
the base_fee_per_gas mapping in make_genesis_header and replace the unwrap_or(0)
usage with explicit error handling (e.g., match or map_err) so overflow is
reported (or propagated) rather than silently producing 0.
| pub(crate) fn build_morph_hardforks_from_genesis(genesis: &Genesis) -> ChainHardforks { | ||
| // Start with Ethereum hardforks from genesis | ||
| let base_spec = ChainSpec::from_genesis(genesis.clone()); | ||
| let mut hardforks = base_spec.hardforks; | ||
|
|
||
| // Extract Morph genesis info | ||
| let chain_info = MorphGenesisInfo::extract_from(&genesis.config.extra_fields) | ||
| .expect("failed to extract morph genesis info"); | ||
| let hardfork_info = chain_info | ||
| .hard_fork_info | ||
| .as_ref() | ||
| .cloned() | ||
| .unwrap_or_default(); | ||
|
|
||
| // Add Morph hardforks | ||
| let block_forks = vec![ | ||
| (MorphHardfork::Bernoulli, hardfork_info.bernoulli_block), | ||
| (MorphHardfork::Curie, hardfork_info.curie_block), | ||
| ] | ||
| .into_iter() | ||
| .filter_map(|(fork, block)| block.map(|b| (fork, ForkCondition::Block(b)))); | ||
|
|
||
| let time_forks = vec![ | ||
| (MorphHardfork::Morph203, hardfork_info.morph203_time), | ||
| (MorphHardfork::Viridian, hardfork_info.viridian_time), | ||
| (MorphHardfork::Emerald, hardfork_info.emerald_time), | ||
| (MorphHardfork::MPTFork, hardfork_info.mpt_fork_time), | ||
| ] | ||
| .into_iter() | ||
| .filter_map(|(fork, time)| time.map(|t| (fork, ForkCondition::Timestamp(t)))); | ||
|
|
||
| hardforks.extend(block_forks.chain(time_forks)); | ||
|
|
||
| // Add Prague at Emerald time for EIP-7702 | ||
| if let Some(emerald_time) = hardfork_info.emerald_time { | ||
| hardforks.insert( | ||
| EthereumHardfork::Prague, | ||
| ForkCondition::Timestamp(emerald_time), | ||
| ); | ||
| } | ||
|
|
||
| hardforks | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Hardfork construction logic is duplicated between build_morph_hardforks_from_genesis and From<Genesis> for MorphChainSpec.
The new helper build_morph_hardforks_from_genesis (lines 62–104) contains the exact same hardfork assembly logic as the From<Genesis> impl (lines 222–256): block forks, time forks, Prague-at-Emerald insertion. The helper is only used in morph.rs and morph_hoodi.rs for the static chain specs, while the From<Genesis> impl handles CLI-parsed genesis files.
Consider refactoring From<Genesis> to call build_morph_hardforks_from_genesis instead of reimplementing the same logic:
Proposed refactor sketch
impl From<Genesis> for MorphChainSpec {
fn from(genesis: Genesis) -> Self {
let chain_info = MorphGenesisInfo::extract_from(&genesis.config.extra_fields)
.expect("failed to extract morph genesis info");
- let hardfork_info = chain_info.hard_fork_info.clone().unwrap_or_default();
-
- // Create base chainspec from genesis (already has ordered Ethereum hardforks)
- let mut base_spec = ChainSpec::from_genesis(genesis);
-
- // Add Morph hardforks
- let block_forks = vec![...];
- ...
- base_spec.hardforks.extend(morph_forks);
-
- if let Some(emerald_time) = hardfork_info.emerald_time {
- base_spec.hardforks.insert(...);
- }
+ let hardforks = build_morph_hardforks_from_genesis(&genesis);
+ let mut base_spec = ChainSpec::from_genesis(genesis);
+ base_spec.hardforks = hardforks;
Self {
inner: base_spec.map_header(MorphHeader::from),
info: chain_info,
}
}
}🤖 Prompt for AI Agents
In `@crates/chainspec/src/spec.rs` around lines 62 - 104, The hardfork assembly
logic in the From<Genesis> for MorphChainSpec impl is duplicated; refactor that
impl to call the existing build_morph_hardforks_from_genesis(genesis) helper
instead of reimplementing block_forks/time_forks and Prague insertion: replace
the duplicated construction in the From<Genesis> implementation with a call to
build_morph_hardforks_from_genesis(genesis) and use its returned ChainHardforks
when building the MorphChainSpec, removing the redundant code while preserving
the Prague-at-Emerald insertion and any unwrap/default handling present in
build_morph_hardforks_from_genesis.
| [features] | ||
| default = [] | ||
| serde = [] |
There was a problem hiding this comment.
The serde feature is a no-op marker that causes a serialization inconsistency.
The serde feature gates nothing (serde = []), yet attributes.rs uses #[cfg_attr(feature = "serde", serde(flatten))] on MorphPayloadAttributes.inner. Since serde::Serialize/serde::Deserialize are derived unconditionally (line 17 of attributes.rs), when this crate is compiled without the serde feature (e.g. via default-features = false), the struct will still be (de)serializable but inner won't be flattened — breaking the Engine API JSON format silently.
Either:
- Make the entire serde derive conditional on the feature, or
- Make
serde(flatten)unconditional (remove thecfg_attrguard), since serde is always a dependency anyway.
Option 2 seems simpler given that serde is already unconditional:
Proposed fix in attributes.rs
- #[cfg_attr(feature = "serde", serde(flatten))]
+ #[serde(flatten)]🤖 Prompt for AI Agents
In `@crates/payload/types/Cargo.toml` around lines 40 - 42, The serde feature in
Cargo.toml is currently a no-op causing inconsistent JSON formatting because
MorphPayloadAttributes has unconditional #[derive(Serialize, Deserialize)] while
MorphPayloadAttributes.inner is guarded with #[cfg_attr(feature = "serde",
serde(flatten))]; fix this by removing the cfg_attr guard and making
serde(flatten) unconditional on MorphPayloadAttributes.inner (or alternatively
make the derive conditional on feature "serde" throughout attributes.rs), i.e.,
update attributes.rs to either (preferred) remove cfg_attr and use
serde(flatten) directly on inner so flattening always occurs, or wrap the
Serialize/Deserialize derives with cfg(feature = "serde") to make the feature
meaningful.
1 participant
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores