feat: decode tx_input directly from RLP

[f3l1ph3s]

PR Type

Enhancement, Tests

---

Description

• Add signature_hash() method to TransactionInput

  • Supports Legacy and EIP-2930/1559/4844/7702 types

• Refactor RLP decoding flow

  • Introduce build_transaction_input_from_envelope()
  • Replace try_from_alloy_transaction() logic

• Remove redundant transaction conversion code

• Add unit tests for signature_hash() correctness

---

Diagram Walkthrough

flowchart LR
  A["RLP bytes"] -- "Decodable.decode()" --> B["TxEnvelope"]
  B -- "build_transaction_input_from_envelope()" --> C["TransactionInput"]
  C -- "signature_hash()" --> D["B256 signature hash"]

Loading

File Walkthrough

	Relevant files
Enhancement	transaction_input.rs Add signature_hash and refactor decoding                                  src/eth/types/transaction/transaction_input.rs Introduce signature_hash() computing B256 from stored fields Refactor RLP decoding into build_transaction_input_from_envelope() Replace old try_from_alloy_transaction() conversion path Add unit tests verifying signature hash matches envelope +144/-26

---

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 6b243b5)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Incomplete signature hash The new signature_hash() method defaults several important fields—access_list, blob‐related fields (blob_versioned_hashes, max_fee_per_blob_gas), and authorization_list—to empty or zero. It also uses the same gas_price for both max_fee_per_gas and max_priority_fee_per_gas. As a result, transactions that include a non‐empty access list, blob data, or distinct priority fees will produce an incorrect signing hash. This mismatch will only surface on real transactions with those fields populated. fn signature_hash(&self) -> B256 { match self.transaction_info.tx_type.map(|t| t.as_u64()).unwrap_or(0) { // EIP-2930 1 => TxEip2930 { chain_id: self.execution_info.chain_id.unwrap_or_default().into(), nonce: self.execution_info.nonce.into(), gas_price: self.execution_info.gas_price, gas_limit: self.execution_info.gas_limit.into(), to: TxKind::from(self.execution_info.to.map(Into::into)), value: self.execution_info.value.into(), input: self.execution_info.input.clone().into(), access_list: AccessList::default(), } .signature_hash(), // EIP-1559 2 => TxEip1559 { chain_id: self.execution_info.chain_id.unwrap_or_default().into(), nonce: self.execution_info.nonce.into(), max_fee_per_gas: self.execution_info.gas_price, max_priority_fee_per_gas: self.execution_info.gas_price, gas_limit: self.execution_info.gas_limit.into(), to: TxKind::from(self.execution_info.to.map(Into::into)), value: self.execution_info.value.into(), input: self.execution_info.input.clone().into(), access_list: AccessList::default(), } .signature_hash(), // EIP-4844 3 => TxEip4844 { chain_id: self.execution_info.chain_id.unwrap_or_default().into(), nonce: self.execution_info.nonce.into(), max_fee_per_gas: self.execution_info.gas_price, max_priority_fee_per_gas: self.execution_info.gas_price, gas_limit: self.execution_info.gas_limit.into(), to: self.execution_info.to.map(Into::into).unwrap_or_default(), value: self.execution_info.value.into(), input: self.execution_info.input.clone().into(), access_list: AccessList::default(), blob_versioned_hashes: Vec::default(), max_fee_per_blob_gas: 0, } .signature_hash(), // EIP-7702 4 => TxEip7702 { chain_id: self.execution_info.chain_id.unwrap_or_default().into(), nonce: self.execution_info.nonce.into(), gas_limit: self.execution_info.gas_limit.into(), max_fee_per_gas: self.execution_info.gas_price, max_priority_fee_per_gas: self.execution_info.gas_price, to: self.execution_info.to.map(Into::into).unwrap_or_default(), value: self.execution_info.value.into(), input: self.execution_info.input.clone().into(), access_list: AccessList::default(), authorization_list: Vec::default(), } .signature_hash(), // Legacy (default) _ => TxLegacy { chain_id: self.execution_info.chain_id.map(Into::into), nonce: self.execution_info.nonce.into(), gas_price: self.execution_info.gas_price, gas_limit: self.execution_info.gas_limit.into(), to: TxKind::from(self.execution_info.to.map(Into::into)), value: self.execution_info.value.into(), input: self.execution_info.input.clone().into(), } .signature_hash(), } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 6b243b5
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Use TxKind for to field Use a TxKind conversion for the to field in EIP-4844 and EIP-7702 arms to match how other types are handled and ensure the signature hash aligns with to_tx_envelope. src/eth/types/transaction/transaction_input.rs [187-203] // EIP-4844 3 => TxEip4844 { ... - to: self.execution_info.to.map(Into::into).unwrap_or_default(), + to: TxKind::from(self.execution_info.to.map(Into::into)), ... } // EIP-7702 4 => TxEip7702 { ... - to: self.execution_info.to.map(Into::into).unwrap_or_default(), + to: TxKind::from(self.execution_info.to.map(Into::into)), ... } Suggestion importance[1-10]: 8 __ Why: Converting to with TxKind aligns EIP-4844 and EIP-7702 arms with other transaction types, ensuring consistent signature hash logic and preventing subtle bugs.	Medium
General	Qualify AccessList path Qualify AccessList with its full path to avoid missing import errors and ensure the code compiles without adding a new import. This makes it explicit where AccessList is coming from. src/eth/types/transaction/transaction_input.rs [162] -access_list: AccessList::default(), +access_list: alloy_consensus::AccessList::default(), Suggestion importance[1-10]: 6 __ Why: Qualifying AccessList ensures the code compiles without adding a new import and prevents ambiguity, improving reliability with minimal change.	Low

---

Previous suggestions

Suggestions up to commit 7db6dee

Category	Suggestion	Impact
Possible issue	Separate EIP-1559 fee fields Collapsing both fee fields into gas_price loses the original max and priority fee distinction and will break EIP-1559 signature hashes when they differ. Split ExecutionInfo.gas_price into max_fee_per_gas and max_priority_fee_per_gas, and use them here. src/eth/types/transaction/transaction_input.rs [170-171] -// Inside the EIP-1559 arm of signature_hash: -max_fee_per_gas: self.execution_info.gas_price, -max_priority_fee_per_gas: self.execution_info.gas_price, +max_fee_per_gas: self.execution_info.max_fee_per_gas, +max_priority_fee_per_gas: self.execution_info.max_priority_fee_per_gas, Suggestion importance[1-10]: 8 __ Why: Using the same gas_price for both EIP-1559 fee fields will produce incorrect signature hashes when original max_fee_per_gas ≠ max_priority_fee_per_gas, so this is an important correctness fix.	Medium

[cloudwalk-review-agent]

Summary

Nice refactor overall: decoding now builds TransactionInput directly from TxEnvelope, and the new signature_hash() path is covered by a focused consistency test against to_tx_envelope().signature_hash() across tx types 0–4.

I reviewed for correctness around signer recovery and hash derivation, and the implementation stays behaviorally aligned with the previous envelope-based hash flow for the fields currently persisted in TransactionInput.

[github-actions]

Persistent review updated to latest commit 6b243b5

[carneiro-cw]

Wrong idea. The idea is to decode transaction input directly from the received rlp. This means that we never even create the alloy types in the first place, the rlp gets decoded directly to TransactionInput, this means we don't ever even decode to TxEnvelope.

Before doing this however, a better issue to solve first is #2548 . Since then when receiving an eth_sendRawTransaction we wouldn't have to do any conversions with alloy at all.

[cloudwalk-review-agent]

Thanks for the refactor — moving decode toward TransactionInput directly is a good direction. I found one blocking correctness issue in signer recovery:

Blocking: signature_hash() now reconstructs typed txs with fields that are not persisted in TransactionInput (access list, 1559 priority fee, 4844 blob fields, 7702 authorization list) using defaults. For any real tx where those fields are non-default, recovered prehash will differ from the original signed prehash, so signer recovery can fail or recover the wrong address.

Concretely in this diff:

• type 1/2: access_list: AccessList::default()
• type 2/3/4: max_priority_fee_per_gas = gas_price
• type 3: blob_versioned_hashes = [], max_fee_per_blob_gas = 0
• type 4: authorization_list = []

Because build_transaction_input_from_envelope() now calls recover_signer_address() (which uses this new hash path), this can break immediately on decode of valid raw txs carrying those fields.

Suggested fix options:

1. Keep using envelope-native signing hash for recovery when decoding from raw tx (envelope.signature_hash()), and only use field-derived hash where you can guarantee complete persisted fields; or
2. Extend TransactionInput/ExecutionInfo to store all signing-relevant fields per tx type, then compute hash from fully faithful data.

Also, current test only checks signature_hash() vs to_tx_envelope() built from the same reduced fields, so it can’t catch this class of mismatch. Please add a test with a real typed tx (e.g. 1559 with non-empty access list / different priority fee, and/or 4844 with blob hashes) and assert recovered signer matches the signer from the original envelope/raw tx.

[cloudwalk-review-agent]

Summary

Thanks for pushing this forward — decoding TransactionInput directly from raw tx bytes is a good direction and the new decode tests are valuable. I found one blocking correctness issue in signer recovery/hash derivation for typed transactions.

[cloudwalk-review-agent]

Summary

Thanks for the refactor toward direct raw-transaction decoding; the structure is clearer and the decode-path tests are useful. I found one blocking correctness issue that can break signer recovery for valid typed transactions.

Blocking: signature_hash() is reconstructed from a reduced field set and injects defaults/placeholders for typed-tx fields that are actually signed. For tx types 1/2/3/4 this can change the signing preimage (e.g., non-empty access list, differing 1559 tip vs max fee, 4844 blob fields, 7702 authorization list), so recover_signer_address() may recover the wrong address or fail.

Current tests don’t catch this because they only exercise default/degenerate values (empty lists and equal fee fields), where reconstructed and original preimages happen to match.

[cloudwalk-review-agent]

Summary

Thanks for the refactor — decoding from raw bytes and removing the rlp crate dependency is a good direction. However, there is still one blocking correctness issue in signer recovery for typed transactions.

Blocking

TransactionInput::signature_hash() reconstructs typed preimages with placeholder/default values for signed fields that are not preserved in TransactionInput (e.g. access list, 1559 priority fee, 4844 blob fields, 7702 authorization list). Because recover_signer_address() now depends on this reconstructed hash, valid transactions with non-default values can recover the wrong signer or fail recovery.

Current tests only cover degenerate/default cases (empty lists and equal tip/max-fee), so they do not exercise this failure mode.

[cloudwalk-review-agent]

Summary

Thanks for the refactor toward direct raw-byte decoding and for adding decode/signature-hash tests. I still found one blocking correctness issue in signer recovery for typed transactions.

Blocking

TransactionInput::signature_hash() reconstructs typed signing payloads using placeholder/default values for signed fields that are not persisted in TransactionInput (e.g. access list, 1559 priority fee, 4844 blob fields, 7702 authorization list). Because recover_signer_address() now uses this reconstructed hash, valid transactions with non-default values in those fields can recover the wrong signer or fail recovery.

Current new tests only validate degenerate/default cases (empty lists and equal tip/max-fee), so this failure mode is not covered.

[cloudwalk-review-agent]

Summary

Nice refactor overall — moving to direct alloy-rlp decoding and adding coverage for all tx families is a good step. I found one blocking correctness issue in signature recovery for typed transactions.

Blocking

• Typed transactions are storing v directly from RLP and later converting it to AlloySignature with a boolean parity check (value.v == U64::ONE). For EIP-1559/2930/4844/7702 this can be encoded as 27/28 by some producers, and your current conversion will treat both as false, producing wrong signer recovery/hashes in those cases.

If you normalize typed v to parity (0/1) at decode time (or normalize inside From<Signature> for AlloySignature), this should be safe.

[cloudwalk-review-agent]

Summary

Good refactor direction overall (direct alloy RLP decode + dedicated error type + broad tx-family tests), but I found one blocking correctness issue that can cause signer recovery failures and wrong signatures for valid typed transactions.

Blocking: typed transaction v/y-parity is not normalized before conversion to AlloySignature.

[cloudwalk-review-agent]

Summary

Good direction overall (direct alloy-rlp decoding, typed tx coverage, and signature-hash parity tests), but there is one blocking correctness issue still present in signer recovery for typed transactions.

Blocking: typed v (y-parity) is stored without normalization and then converted to AlloySignature with value.v == U64::ONE, which misinterprets valid 27/28 encodings as false parity.