feat: same signer recovery path#2520
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
![cloudwalk-review-agent[bot]](https://avatars.githubusercontent.com/u/6581007?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Good refactor direction to centralize signer recovery, but this implementation changes the signing payload before recovery for typed transactions. That can derive the wrong signer (or fail recovery) for valid transactions, so this needs to recover from the original envelope/transaction data path.
There was a problem hiding this comment.
Thanks for the refactor—centralizing signer recovery is a good goal. However there is one blocking correctness issue still present:
Blocking
recover_signer() reconstructs an AlloyTransaction from TransactionInput (self.clone().into()) and then recovers from that rebuilt tx.
This path is not payload-preserving for signed fields across tx types (e.g., EIP-2930 access list defaults, EIP-1559 priority fee currently mirrored from max fee, and 4844/7702-specific fields not represented in TransactionInput). That means recovery can be executed over a different signing payload than the original envelope, yielding wrong signer or false recovery failure for valid txs.
Impact: signer identity may be incorrect or rejected depending on tx type/content, which is security-sensitive and can break authorization/accounting paths.
Suggested fix: recover signer from the original alloy_rpc_types_eth::Transaction / TxEnvelope directly (as before), or make TransactionInput fully capture every signable field before doing any round-trip-based recovery.
Test gap (non-blocking but important)
Add coverage proving signer recovery matches original for at least:
- legacy
- EIP-2930 with non-empty access list
- EIP-1559 with
max_priority_fee_per_gas != max_fee_per_gas - EIP-4844 transaction
Without this, regressions in signer derivation are easy to reintroduce.
There was a problem hiding this comment.
Nice fix. This addresses the earlier blocking concern by recovering the signer from the original TxEnvelope rather than from a reconstructed transaction payload.
What improved
recover_signer(&TxEnvelope)now usesenvelope.recover_signer()directly, avoiding lossy round-trips.Decodable::decodenow buildsTransactionInputwith placeholder signer and then recovers from the decoded envelope.TryFrom<ExternalTransaction>andTryFrom<AlloyTransaction>follow the same envelope-based recovery path.- Removed signer extraction from
try_from_alloy_transaction, so construction is now clearly separated from recovery.
This keeps signer derivation consistent across origins and preserves correctness for typed txs with fields not fully represented in TransactionInput.
Non-blocking follow-up: tests are still important here; I’d add explicit coverage for legacy, 2930 (non-empty access list), 1559 (different priority/max fee), and 4844/7702 to guard this path long-term.
There was a problem hiding this comment.
We can add tests to avoid regressions for this. In the e2e testing suite try creating each existing transaction type and using all fields that alloy can use to create a signature. Right now we're blocking certain tx types though, so maybe put the conditional that blocks the tx types in rpc_server behind a #cfg[not(dev)]. This tx would need to be sent to the leader and imported to the follower, so leader-follower tests are the right place to do this.
There was a problem hiding this comment.
Summary
Looks good to approve. The core correctness risk called out in prior review (recovering signer from a reconstructed transaction payload) appears addressed: signer recovery is now done from the original TxEnvelope path, and conversion/deserialization flows consistently call recover_signer before returning usable TransactionInput values.
The new leader/follower E2E regression coverage across tx types 0/1/2/3 with non-trivial fields is meaningful and directly targets the previous class of signer-recovery regressions.
I did not find concrete blocking issues in the provided diff. Remaining risk is mostly around uninspected paths not included in the excerpt (e.g., any constructors/usages of TransactionInput outside these conversion paths that might call signer() before recovery), but nothing concrete in this PR indicates a live bug.
There was a problem hiding this comment.
Blocking issue
The #[cfg(feature = "dev")] gate on type-4 rejection in eth_send_raw_transaction appears to invert the safety behavior and can re-enable the known follower-crash path in non-dev builds.
In this PR, the existing guard:
if tx.transaction_info.tx_type.is_some_and(|t| t > 3) {
tracing::warn!(%tx_hash, "rejecting unsuported transaction type");
return Err(RpcError::ParameterInvalid.into());
}was changed to:
#[cfg(feature = "dev")]
if tx.transaction_info.tx_type.is_some_and(|t| t > 3) { ... }Given the in-code comment says this guard is a temporary stopgap to prevent follower crashes, compiling it only when dev is enabled means production/non-dev builds no longer reject tx type 4 and may hit the crash condition.
Please switch this to the opposite condition (#[cfg(not(feature = "dev"))]) or otherwise ensure the crash-prevention guard remains active in the environments where followers are vulnerable.
There was a problem hiding this comment.
Blocking issue
The tx-type safety guard in eth_send_raw_transaction is now compiled only with feature = "dev", which appears to invert the intended protection.
In this diff (src/eth/rpc/rpc_server.rs), the hotfix comment says this guard prevents tx type 4 from crashing followers, but the code is now:
#[cfg(feature = "dev")]
if tx.transaction_info.tx_type.is_some_and(|t| t > 3) { ... }That means non-dev/prod builds will skip the rejection path entirely and can accept type 4 again.
If the crash condition still exists, this is deploy-blocking for non-dev environments. Please switch the cfg condition (likely #[cfg(not(feature = "dev"))]) or otherwise ensure the guard is active where followers are vulnerable.
There was a problem hiding this comment.
Blocking issue
The tx-type rejection guard in eth_send_raw_transaction is now gated behind #[cfg(feature = "dev")], which appears to invert the intended safety behavior.
In src/eth/rpc/rpc_server.rs, this block:
#[cfg(feature = "dev")]
if tx.transaction_info.tx_type.is_some_and(|t| t > 3) {
tracing::warn!(%tx_hash, "rejecting unsuported transaction type");
return Err(RpcError::ParameterInvalid.into());
}means non-dev/prod builds won’t execute the stopgap rejection for tx type > 3. The nearby comment says this guard exists to prevent follower crashes for type 4 transactions, so compiling it only in dev re-opens the crash path where it matters most.
Please keep this protection active in non-dev environments (e.g. invert cfg condition or remove the dev-only gate).
There was a problem hiding this comment.
Blocking issue
The tx-type safety guard in eth_send_raw_transaction appears inverted by cfg-gating and is now only compiled for dev builds:
#[cfg(feature = "dev")]
if tx.transaction_info.tx_type.is_some_and(|t| t > 3) {
tracing::warn!(%tx_hash, "rejecting unsuported transaction type");
return Err(RpcError::ParameterInvalid.into());
}Given the nearby comment says this is a temporary stopgap to prevent follower crashes on type-4 txs, compiling it only in dev removes that protection from non-dev/prod builds.
Impact: non-dev environments can accept tx type > 3 again and re-open the known follower-crash path.
Please keep this guard active in the vulnerable environments (e.g. invert cfg to not(feature = "dev") or remove the dev-only gate).
There was a problem hiding this comment.
LGTM — I did not find a concrete blocking issue in the provided diff.
Summary
- The signer recovery path is now centralized and consistently invoked after deserialization/conversion (
Decodable,TryFrom<ExternalTransaction>,TryFrom<AlloyTransaction>), which addresses the prior correctness risk of inconsistent recovery paths. ExecutionInfonow carries typed-tx fields (priority fee, blob fee/hashes, access/authorization lists), and those fields are propagated throughTransactionInput <-> AlloyTransactionand persisted in RocksDB mappings.TransactionExecutionnow preserves originalexecution_infoinstead of reconstructing it fromEvmInput, reducing lossy transformations across transaction types.- Added meaningful E2E regression coverage across tx types 0/1/2/3 in leader/follower flows, including signer checks and receipt parity.
No VictoriaLogs production logging anti-patterns were identified in backend runtime code changes (test console.log usage is test-only and allow-listed).
carneiro-cw
left a comment
carneiro-cw
left a comment
There was a problem hiding this comment.
As I said before, we won't be updating the rocks types for this. Instead we'll compute the singer ignoring the fields we don't support for now.
There was a problem hiding this comment.
LGTM.
I reviewed the signer-recovery refactor and related persistence/execution wiring changes and did not find a concrete blocking issue in the provided diff.
What looks good
- Signer recovery is now centralized via
TransactionInputand consistently used across decode/TryFrom/EVM input paths, reducing prior divergence risk. - The
tx type > 3safety guard ineth_sendRawTransactionis now correctly gated with#[cfg(not(feature = "dev"))], preserving non-dev protection. TransactionExecutionnow carriesexecution_info, and Rocks serialization paths use it, reducing lossy reconstruction fromevm_input.- New E2E coverage across tx types 0/1/2/3/4 (leader/follower signer + receipt parity) is meaningful and directly targets this regression class.
No concrete VictoriaLogs production logging anti-pattern was found in backend runtime code touched by this PR (test console.log usage ignored per allow-list).
There was a problem hiding this comment.
LGTM — I did not find a concrete blocking issue in the provided diff.
What I checked
- Signer recovery correctness across decode/conversion/execution/storage paths
- RPC safety behavior for tx type gating
- Data consistency risks introduced by
Signer::{Recovered,Unrecovered}and lazysigner()access - Regression test coverage for multi-type tx handling in leader/follower flows
- Logging changes for VictoriaLogs anti-patterns (none in production runtime paths; test
console.logis allow-listed)
Summary
- Centralizing signer recovery via
TransactionInputand reusingto_tx_envelope()removes prior path divergence and keeps behavior consistent inDecodable,TryFrom, executor, and Rocks serialization. TransactionExecutionnow carriesexecution_infoexplicitly, avoiding lossy reconstruction fromevm_input.- The tx-type safety guard in
eth_send_raw_transactionis now#[cfg(not(feature = "dev"))], preserving non-dev protection against unsupported type > 3. - Added E2E coverage for signer/receipt parity across tx types (including typed tx variants) is meaningful and directly aligned with the regression class.
No concrete security/correctness/data-loss/deploy-safety issue is evidenced in the current diff beyond previously addressed concerns.
There was a problem hiding this comment.
LGTM.
I reviewed the signer-recovery refactor and surrounding execution/storage/RPC wiring in the provided diff and did not find a concrete blocking issue.
What looks solid
- Signer recovery is centralized in
TransactionInput(recover_signer_address/to_tx_envelope) and consistently used across decode and conversion paths. ExecutionInfo.signernow models recovered-vs-unrecovered state explicitly, reducing accidental reliance on placeholder addresses.TransactionExecutionnow carriesexecution_infodirectly, avoiding lossy reconstruction fromEvmInputduring round-trips.- Storage mappings were updated to persist/rebuild signer with the new enum shape.
- The non-dev tx-type safety guard for
tx_type > 3is present ineth_send_raw_transaction. - Added/updated integration coverage for signer consistency and leader/follower parity across tx types is meaningful and aligned with the regression class.
No concrete VictoriaLogs production logging anti-pattern was identified in backend runtime paths touched by this PR (test console.log usage is allow-listed).
There was a problem hiding this comment.
LGTM.
I reviewed the provided diff with focus on signer recovery correctness, tx-type safety behavior, persistence consistency, and regression coverage. I did not find a concrete blocking issue in the current head.
Summary
- Signer recovery is now centralized in
TransactionInputand reused across decode/conversion/execution paths, which reduces prior divergence risk. ExecutionInfo.signernow explicitly models recovered vs unrecovered state, and call sites were updated to usetx.signer()where a concrete address is required.TransactionExecutionnow carriesexecution_infodirectly, avoiding lossy reconstruction fromEvmInputand improving storage round-trip fidelity.- Non-dev tx-type guard for
tx_type > 3is present ineth_send_raw_transactionvia#[cfg(not(feature = "dev"))], preserving the follower crash stopgap in non-dev/prod. - Added E2E coverage across tx types 0/1/2/3/4 for leader/follower signer and receipt parity is meaningful for this regression class.
No concrete VictoriaLogs production logging anti-patterns were identified in backend runtime code touched by this PR (test-side console.log usage is allow-listed).
There was a problem hiding this comment.
LGTM — I did not find a concrete blocking issue in the provided diff.
What I checked
- Signer recovery correctness across decode /
TryFrom/ envelope reconstruction paths - Safety around fallback signer access (
TransactionInput::signer()) - Execution/storage round-trip consistency after introducing
execution_infointoTransactionExecution - RPC tx-type guard behavior for follower crash stopgap
- Regression test coverage for tx types 0/1/2/3/4 leader-follower parity
- VictoriaLogs anti-patterns in production logging paths touched by this PR
Summary
- Centralized signer recovery via
recover_signer_address()+to_tx_envelope()is consistently applied in conversion paths and avoids prior path divergence. ExecutionInfo.signerbecomingSigner::{Recovered,Unrecovered}is wired through executor/storage/RPC call sites without obvious contract break in touched code.TransactionExecutionnow carriesexecution_info, reducing lossy reconstruction fromEvmInputand improving persistence fidelity.- The non-dev tx-type rejection guard (
#[cfg(not(feature = "dev"))]fortx_type > 3) is present ineth_send_raw_transaction, preserving the stated stopgap. - New/updated E2E coverage for transaction-type signer recovery and leader/follower receipt parity is meaningful and aligned with the regression risk.
- No concrete VictoriaLogs production logging issue found (test
console.logusage is allow-listed).
There was a problem hiding this comment.
LGTM — I did not find a concrete blocking issue in the provided diff.
What I checked
- Signer recovery correctness across decode/conversion/executor/storage/RPC paths
- Potential behavior regressions from introducing
Signer::{Recovered, Unrecovered} - Temporary-storage and RocksDB round-trip consistency after adding
execution_infointoTransactionExecution - Non-dev tx-type safety guard behavior in
eth_send_raw_transaction - New/updated E2E coverage relevance for the signer-recovery regression class
- VictoriaLogs anti-patterns in touched backend runtime logging paths
Summary
- Centralized signer recovery in
TransactionInputis wired consistently throughTryFrom, RLP decode, EVM input creation, and RPC tracing usage. TransactionExecutionnow preservingexecution_infoavoids lossy reconstruction fromevm_inputand is reflected in storage mappings.- The tx-type
> 3stopgap ineth_send_raw_transactionis gated for non-dev builds (#[cfg(not(feature = "dev"))]), preserving production protection. - E2E additions/updates meaningfully exercise multi-type signer consistency (including 7702 path) and leader/follower parity.
- No actionable VictoriaLogs production logging issue was found in backend runtime code touched by this PR (test/debug
console.logusage is in allow-listed test paths).
2 participants
PR Type
Enhancement
Description
Add
recover_signermethod for consistent ECDSA signer recoveryUpdate RLP decoding to invoke
recover_signeron deserializationModify
TryFromconversions to callrecover_signerRemove direct signer extraction in
try_from_alloy_transactionDiagram Walkthrough
File Walkthrough
transaction_input.rs
Add unified ECDSA signer recoverysrc/eth/primitives/transaction_input.rs
recover_signermethod for ECDSA recoveryDecodable::decodeto callrecover_signerTryFromandTryFromto invoke recoverytry_from_alloy_transaction, usingplaceholder ZERO