Fix endpoint crashes due to non-replay protected EVM transactions #846
Conversation
WalkthroughThe changes update how chain IDs are handled for Ethereum transactions, ensuring the correct signer is used for legacy (non-replay protected) transactions without a chain ID. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant JSON-RPC Handler
participant Transaction
participant DeriveTxSender
Client->>JSON-RPC Handler: Submit transaction (may lack chainId)
JSON-RPC Handler->>Transaction: Parse transaction
JSON-RPC Handler->>DeriveTxSender: Derive sender address
alt chainId is non-zero
DeriveTxSender->>Transaction: Use LatestSignerForChainID
else chainId is zero (legacy)
DeriveTxSender->>Transaction: Use HomesteadSigner
end
DeriveTxSender-->>JSON-RPC Handler: Return sender address
JSON-RPC Handler-->>Client: Respond with result or error
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ 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). (1)
🔇 Additional comments (6)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
eth/types/types.go (1)
398-420: Consider consolidating redundant ChainID assignments.The ChainID is being set multiple times in different conditional blocks (lines 398, 407, 413, 420). Since all non-legacy transaction types require a chain ID, consider setting it once after the legacy transaction handling.
if tx.Type() > types.LegacyTxType { al := tx.AccessList() yparity := hexutil.Uint64(v.Sign()) result.Accesses = &al result.YParity = &yparity - result.ChainID = (*hexutil.Big)(tx.ChainId()) } if tx.Type() > types.AccessListTxType { result.GasFeeCap = (*hexutil.Big)(tx.GasFeeCap()) result.GasTipCap = (*hexutil.Big)(tx.GasTipCap()) // Since BaseFee is `0`, this is the max gas price // the sender is willing to pay. result.GasPrice = (*hexutil.Big)(tx.GasFeeCap()) - result.ChainID = (*hexutil.Big)(tx.ChainId()) } if tx.Type() > types.DynamicFeeTxType { result.MaxFeePerBlobGas = (*hexutil.Big)(tx.BlobGasFeeCap()) result.BlobVersionedHashes = tx.BlobHashes() - result.ChainID = (*hexutil.Big)(tx.ChainId()) } // The `AuthorizationList` field became available with the introduction // of https://eip7702.io/#specification, under the `SetCodeTxType` if tx.Type() > types.BlobTxType { result.AuthorizationList = tx.SetCodeAuthorizations() - result.ChainID = (*hexutil.Big)(tx.ChainId()) } + + // Set ChainID for all non-legacy transaction types + if tx.Type() > types.LegacyTxType { + result.ChainID = (*hexutil.Big)(tx.ChainId()) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
eth/types/types.go(3 hunks)models/transaction.go(4 hunks)services/requester/batch_tx_pool.go(1 hunks)services/requester/requester.go(1 hunks)tests/fixtures/multicall3.byte(1 hunks)tests/web3js/eth_filter_endpoints_test.js(0 hunks)tests/web3js/eth_multicall3_contract_test.js(2 hunks)
💤 Files with no reviewable changes (1)
- tests/web3js/eth_filter_endpoints_test.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:50-62
Timestamp: 2025-03-07T01:35:09.751Z
Learning: In the flow-evm-gateway codebase, panics are acceptable in scenarios where immediate detection of critical bugs is desired during development and testing, particularly for invariant violations that should never occur in a correctly functioning system (e.g., when a key is available but locked in the keystore implementation).
services/requester/batch_tx_pool.go (1)
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/batch_tx_pool.go:0-0
Timestamp: 2025-06-19T11:36:25.478Z
Learning: In Go, when copying maps that contain slices (like `map[gethCommon.Address][]pooledEvmTx`), perform deep copies by iterating over the map and copying each slice individually using `make()` and `copy()` to avoid shared references that could lead to race conditions and data corruption.
🧬 Code Graph Analysis (3)
services/requester/requester.go (1)
models/transaction.go (1)
DeriveTxSender(314-328)
services/requester/batch_tx_pool.go (1)
models/transaction.go (1)
DeriveTxSender(314-328)
models/transaction.go (1)
eth/types/types.go (1)
Transaction(276-302)
⏰ 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). (1)
- GitHub Check: Test
🔇 Additional comments (9)
tests/fixtures/multicall3.byte (1)
1-1: Confirm multicall3.byte fixture is correctThe
multicall3.bytefixture is only used by tests/web3js/eth_multicall3_contract_test.js. Please:• Verify that
tests/fixtures/multicall3.bytematches the official Multicall3 contract bytecode (e.g. compile the exact source version you expect or fetch from a trusted on-chain verifier like Etherscan).
• Ensuretests/web3js/eth_multicall3_contract_test.jscorrectly loads and compares against this fixture.
• Run the full test suite to confirm there are no mismatches or failures.services/requester/requester.go (1)
211-214: LGTM! Centralized sender derivation fixes legacy transaction handling.The change to use
models.DeriveTxSender(tx)correctly addresses the issue with non-replay protected transactions by using the appropriate signer based on the transaction's chain ID.services/requester/batch_tx_pool.go (1)
123-127: LGTM! Consistent implementation of centralized sender derivation.The changes align with the updates in
requester.go, ensuring uniform handling of transaction sender derivation across the codebase.models/transaction.go (3)
47-47: LGTM! Essential interface addition for chain ID handling.Adding
ChainId()to the Transaction interface enables consistent chain ID access across all transaction types, which is crucial for the legacy transaction fix.
121-123: Appropriate implementation for DirectCall transactions.Returning a zero chain ID for DirectCall is correct as these are special state transitions in Flow EVM that don't have a traditional chain ID.
314-328: Core fix correctly implements legacy transaction handling.The
DeriveTxSenderfunction properly addresses the PR objective by:
- Using
HomesteadSignerfor transactions with zero chain ID (legacy, non-replay protected)- Using
LatestSignerForChainIDfor transactions with non-zero chain ID- Centralizing sender derivation logic for consistency
This prevents the crashes mentioned in issue #845 by handling legacy transactions appropriately.
eth/types/types.go (1)
365-368: LGTM! Correctly handles legacy transactions with EIP-155 chain ID.The check for non-zero chain ID before including it in the result is appropriate for legacy transactions that may or may not have replay protection.
tests/web3js/eth_multicall3_contract_test.js (2)
44-51: LGTM! Correctly verifies legacy transaction handlingThe test properly verifies that
chainIdis undefined for both the transaction and block data, which aligns with the PR's objective of handling legacy (non-replay protected) transactions.
52-96: LGTM! Multicall3 contract interaction is properly implementedThe test correctly:
- Instantiates the multicall3 contract with appropriate options
- Uses aggregate3 to batch multiple calls
- Properly decodes and verifies the results
m-Peter
force-pushed
the
mpeter/guard-against-zero-chain-id-transactions
branch
from
a9fbe65 to
00aaa70
Compare
Closes: #845
Description
Due to the Geth changes for Pectra, a chain ID of
0is no longer valid. And for such legacy transactions, theHomesteadSignerhas to be used instead. See https://github.com/ethereum/go-ethereum/blob/v1.16.1/core/types/transaction_signing.go#L379-L382 for more info.For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Bug Fixes
Tests