fix(prover): accept reth L1 message tx fields#969
Conversation
Allow prover transaction traces to deserialize reth-style L1 message RPC responses that omit signature fields and use the standard transaction hash key. Constraint: Preserve existing Morph trace field names while accepting reth RPC aliases Confidence: high Scope-risk: narrow
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
📝 WalkthroughWalkthroughThe ChangesTransactionTrace deserialization robustness
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🧹 Nitpick comments (1)
prover/crates/primitives/src/types/tx.rs (1)
198-226: ⚡ Quick winConsider expanding test coverage to validate all defaulted fields.
The test successfully validates that the reth L1 message transaction deserializes and that
ty(),queue_index(), andtx_hash()work correctly. However, it doesn't verify that the defaulted fields (nonce,v,r,s) are actually set to their default values.Consider adding assertions to confirm:
tx.nonce()returns 0tx.sig_v()returns 0tx.signature()returns a signature with zero scalars (or document expected behavior)🧪 Suggested additional assertions
assert_eq!( tx.tx_hash(), "0x4c03b51e272570d42b0135d48ac612ef59c670e5759df97971be35b93c6310f9" .parse::<B256>() .unwrap() ); + // Verify defaulted fields + assert_eq!(tx.nonce(), 0, "nonce should default to 0"); + assert_eq!(tx.sig_v(), 0, "v should default to 0"); + // Signature with zero scalars - document expected behavior + let sig = tx.signature().expect("signature method should not error"); + assert_eq!(sig.r(), alloy_primitives::U256::ZERO); + assert_eq!(sig.s(), alloy_primitives::U256::ZERO); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prover/crates/primitives/src/types/tx.rs` around lines 198 - 226, Add assertions to the test deserializes_reth_l1_message_transaction_without_signature_fields to validate defaulted signature-related fields: call TransactionTrace::nonce() and assert it equals 0, call TransactionTrace::sig_v() (or the accessor used in the code) and assert it equals 0, and call TransactionTrace::signature() (or the associated accessors like sig_r()/sig_s() if present) and assert the signature components are zero/default (e.g., zero-length or zero-value scalars) so the test verifies nonce, v, r, s default values after deserialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@prover/crates/primitives/src/types/tx.rs`:
- Around line 19-20: The tx_hash field uses serde(default) which can silently
produce a zero B256 when "txHash" or "hash" are missing; add validation to
reject zero hashes by either (a) implementing a validate(&self) -> Result<(), _>
on TransactionTrace that checks self.tx_hash != B256::ZERO and calling it after
deserialization, or (b) replace the default with a custom deserializer (e.g.
deserialize_non_zero_hash) wired via #[serde(deserialize_with = "...", rename =
"txHash", alias = "hash")] that returns an error when the deserialized B256
equals B256::ZERO; reference tx_hash, TransactionTrace, B256::ZERO, validate,
and deserialize_non_zero_hash when making the change.
- Around line 85-92: The v/r/s fields now default to zero which allows
deserializing Reth L1 message txs but leads signature() (method signature()) to
build an invalid all-zero Signature; add a doc comment on the transaction struct
explaining that v, r, s default to zero for L1 message (type 0x7e) and that an
all-zero signature denotes “no signature” for that type; add a helper method
has_signature(&self) -> bool (or requires_signature(&self) -> bool) that checks
the tx type and whether v/r/s are non-zero as appropriate, update documentation
for signature() to state callers must call has_signature()/check tx type before
using signature() for verification, and update any call sites to guard
signature() with the new helper.
---
Nitpick comments:
In `@prover/crates/primitives/src/types/tx.rs`:
- Around line 198-226: Add assertions to the test
deserializes_reth_l1_message_transaction_without_signature_fields to validate
defaulted signature-related fields: call TransactionTrace::nonce() and assert it
equals 0, call TransactionTrace::sig_v() (or the accessor used in the code) and
assert it equals 0, and call TransactionTrace::signature() (or the associated
accessors like sig_r()/sig_s() if present) and assert the signature components
are zero/default (e.g., zero-length or zero-value scalars) so the test verifies
nonce, v, r, s default values after deserialization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a82fc1d-2c3e-47bc-8651-3d406bd4cc9f
📒 Files selected for processing (1)
prover/crates/primitives/src/types/tx.rs
| #[serde(default, rename = "txHash", alias = "hash")] | ||
| pub(crate) tx_hash: B256, |
There was a problem hiding this comment.
Consider validating that tx_hash is non-zero after deserialization.
Adding default means that if neither "txHash" nor "hash" is present in the JSON, the field will silently become a zero hash (B256::default()). A zero hash is never a valid transaction hash and could mask missing data, leading to subtle bugs downstream.
Consider one of these approaches:
- Add a post-deserialization validation step that rejects zero hashes
- Document explicitly that zero hash is expected/acceptable for certain transaction types
- Use a custom deserializer that returns an error when both field names are absent
🛡️ Example validation approach
Add a validation method and call it after deserialization:
impl TransactionTrace {
fn validate(&self) -> Result<(), &'static str> {
if self.tx_hash == B256::ZERO {
return Err("tx_hash must be non-zero");
}
Ok(())
}
}Or use serde's deserialize_with for custom validation:
#[serde(default, rename = "txHash", alias = "hash", deserialize_with = "deserialize_non_zero_hash")]
pub(crate) tx_hash: B256,
fn deserialize_non_zero_hash<'de, D>(deserializer: D) -> Result<B256, D::Error>
where
D: serde::Deserializer<'de>,
{
let hash = B256::deserialize(deserializer)?;
if hash == B256::ZERO {
return Err(serde::de::Error::custom("tx_hash cannot be zero"));
}
Ok(hash)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@prover/crates/primitives/src/types/tx.rs` around lines 19 - 20, The tx_hash
field uses serde(default) which can silently produce a zero B256 when "txHash"
or "hash" are missing; add validation to reject zero hashes by either (a)
implementing a validate(&self) -> Result<(), _> on TransactionTrace that checks
self.tx_hash != B256::ZERO and calling it after deserialization, or (b) replace
the default with a custom deserializer (e.g. deserialize_non_zero_hash) wired
via #[serde(deserialize_with = "...", rename = "txHash", alias = "hash")] that
returns an error when the deserialized B256 equals B256::ZERO; reference
tx_hash, TransactionTrace, B256::ZERO, validate, and deserialize_non_zero_hash
when making the change.
| #[serde(default)] | ||
| pub(crate) v: U64, | ||
| /// signature r | ||
| #[serde(default)] | ||
| pub(crate) r: U256, | ||
| /// signature s | ||
| #[serde(default)] | ||
| pub(crate) s: U256, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Document that signature fields default for L1 message transactions.
The signature fields (v, r, s) now default to zero when missing, which enables deserialization of reth L1 message transactions (type 0x7e) that lack traditional ECDSA signatures. However, the signature() method at lines 160-166 will construct a Signature with zero scalars and default parity, which is invalid for cryptographic verification.
Consider:
- Adding a doc comment to the struct explaining that zero signature fields are valid for L1 message transactions
- Adding a helper method like
has_signature(&self) -> boolthat checks if the transaction type requires/has a signature - Documenting that callers must check transaction type before calling
signature()for validation
📝 Suggested documentation
/// Transaction Trace
+///
+/// Note: Signature fields (v, r, s) default to zero for transaction types
+/// that don't require signatures (e.g., L1 message transactions, type 0x7e).
+/// Callers should check the transaction type before attempting signature
+/// verification via the `signature()` method.
#[serde_as]
#[derive(serde::Serialize, serde::Deserialize, Default, Debug, Clone, Hash, PartialEq, Eq)]
pub struct TransactionTrace {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@prover/crates/primitives/src/types/tx.rs` around lines 85 - 92, The v/r/s
fields now default to zero which allows deserializing Reth L1 message txs but
leads signature() (method signature()) to build an invalid all-zero Signature;
add a doc comment on the transaction struct explaining that v, r, s default to
zero for L1 message (type 0x7e) and that an all-zero signature denotes “no
signature” for that type; add a helper method has_signature(&self) -> bool (or
requires_signature(&self) -> bool) that checks the tx type and whether v/r/s are
non-zero as appropriate, update documentation for signature() to state callers
must call has_signature()/check tx type before using signature() for
verification, and update any call sites to guard signature() with the new
helper.
|
Closing this PR because the compatibility fix is being handled in morph-reth instead of the prover. |
1 participant
Summary
hashfor transaction hashes while preserving existingtxHashoutput.nonce,v,r, andsfields so reth0x7eL1 message transactions can be parsed.Test plan
cargo test -p prover-primitives --libDEVNET=true SHOULD_THROTTLE_REQUESTS=false RUST_BACKTRACE=1 cargo test -p morph-prove --lib -- execute::tests::test_execute_local_traces --exact --nocaptureDEVNET=true SHOULD_THROTTLE_REQUESTS=false RUST_BACKTRACE=0 cargo test -p morph-prove --lib -- execute::tests::test_execute_range --exact --nocapture -- --start-block 209 --end-block 209 --rpc http://morph-panos-fullnode-test01.bitkeep.tools:8545now reaches the expected historical proof window error instead of failing to deserialize the L1 message transaction.Summary by CodeRabbit