Add transaction size validation to RPC endpoints#1807
Add transaction size validation to RPC endpoints#1807manuelmauro wants to merge 7 commits intopolkadot-evm:masterfrom
Conversation
| // Default TransactionRequest converts to EIP-1559 (no gas_price, no access_list) | ||
| let request = TransactionRequest::default(); | ||
| let size = request.encoded_length(); | ||
| let size = request.encoded_length().unwrap(); |
There was a problem hiding this comment.
Please never use unwrap in production code, the error should be propagated instead
There was a problem hiding this comment.
Not even in tests?
There was a problem hiding this comment.
In tests it's ok but I prefer expect even in tests
librelois
left a comment
librelois
left a comment
There was a problem hiding this comment.
eth_sendTransaction validates size before the node fills in nonce, gas limit, fee fields, and chain ID, so near-limit requests can still become oversized after signing. The new guard runs at client/rpc/src/eth/submit.rs#L51 via request.validate_size(), but validate_size() computes length from the pre-default TransactionRequest at client/rpc-core/src/types/transaction_request.rs#L192. A request that omits nonce, gas, gasPrice/maxFeePerGas, or chainId is measured with zero/default values, then later expanded at client/rpc/src/eth/submit.rs#L71, client/rpc/src/eth/submit.rs#L79, client/rpc/src/eth/submit.rs#L91, and client/rpc/src/eth/submit.rs#L102. That means the exact 128 KiB limit is not actually enforced on the final signed transaction for the send_transaction path.
Address review feedback: validate_size() was called on the raw TransactionRequest before nonce, gas limit, gas price, and chain ID were filled in. Near-limit transactions could pass validation then become oversized after field population. Move encoded_length(), validate_size(), and SIGNATURE_RLP_OVERHEAD from TransactionRequest to TransactionMessage. In send_transaction, validate the fully-populated TransactionMessage (after nonce, gas, fees, chain ID are set) right before signing. This ensures the 128 KiB limit is enforced on the actual final transaction size. TransactionRequest::encoded_length() is kept as a convenience method that delegates to TransactionMessage::encoded_length() via conversion.
Addressed in 1b2f4fc |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds RLP-encoded transaction size calculation and enforces a 128 KiB submission limit; exposes size constants via the public types module; integrates validation into RPC transaction submission paths. Also adds a temporary Cargo patch to use a git revision of the ethereum crate. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can get early access to new features in CodeRabbit.Enable the |
![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
🧹 Nitpick comments (1)
client/rpc-core/src/types/transaction_request.rs (1)
529-681: Add exact 128 KiB boundary tests.The current cases only prove "well under" and "well over". Since the contract is
> DEFAULT_MAX_TX_INPUT_BYTES, please add coverage for exactlyDEFAULT_MAX_TX_INPUT_BYTESandDEFAULT_MAX_TX_INPUT_BYTES + 1so off-by-one regressions are caught.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/rpc-core/src/types/transaction_request.rs` around lines 529 - 681, Add two unit tests that assert boundary behavior at DEFAULT_MAX_TX_INPUT_BYTES and DEFAULT_MAX_TX_INPUT_BYTES + 1: construct TransactionRequest instances whose access_list serialised size is exactly DEFAULT_MAX_TX_INPUT_BYTES and one byte larger, convert into Option<TransactionMessage> (via Into) and call TransactionMessage::validate_size to assert Ok for the equal case and Err for the +1 case; use DEFAULT_MAX_TX_INPUT_BYTES and TransactionMessage::SIGNATURE_RLP_OVERHEAD (and existing AccessListItem/H256/H160 helpers used in nearby tests) to compute the number of storage_keys needed so the encoded_length() hits the exact boundary (and +1) rather than hardcoding counts, and add these tests near the other size tests so they run with the existing test harness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/rpc-core/src/types/transaction_request.rs`:
- Around line 269-317: The current TransactionMessage::encoded_length() and
validate_size() use a fixed 67-byte signature allowance and therefore only
produce an estimate; change validate_size() to be explicitly a pre-signing guard
(e.g., only check against a conservative threshold or return a non-fatal
warning) and stop using it as the final hard reject for eth_sendTransaction;
instead, in the submit path (client/rpc/src/eth/submit.rs) after creating the
actual signed transaction bytes call the definitive check against the
signed-byte limit (128 KiB) and return an error if signed_bytes.len() >
128*1024. Update comments on TransactionMessage::encoded_length and
validate_size to state they are estimates and reference
TransactionMessage::encoded_length, TransactionMessage::validate_size and
DEFAULT_MAX_TX_INPUT_BYTES so reviewers can find the changed logic.
---
Nitpick comments:
In `@client/rpc-core/src/types/transaction_request.rs`:
- Around line 529-681: Add two unit tests that assert boundary behavior at
DEFAULT_MAX_TX_INPUT_BYTES and DEFAULT_MAX_TX_INPUT_BYTES + 1: construct
TransactionRequest instances whose access_list serialised size is exactly
DEFAULT_MAX_TX_INPUT_BYTES and one byte larger, convert into
Option<TransactionMessage> (via Into) and call TransactionMessage::validate_size
to assert Ok for the equal case and Err for the +1 case; use
DEFAULT_MAX_TX_INPUT_BYTES and TransactionMessage::SIGNATURE_RLP_OVERHEAD (and
existing AccessListItem/H256/H160 helpers used in nearby tests) to compute the
number of storage_keys needed so the encoded_length() hits the exact boundary
(and +1) rather than hardcoding counts, and add these tests near the other size
tests so they run with the existing test harness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f6b1d25-9cd6-4cb9-b827-7a4d8efa2535
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomlclient/rpc-core/src/types/mod.rsclient/rpc-core/src/types/transaction_request.rsclient/rpc/src/eth/submit.rs
| impl TransactionMessage { | ||
| /// RLP overhead for signature fields (yParity + r + s) | ||
| /// - yParity: 1 byte (0x00 or 0x01 encoded as single byte) | ||
| /// - r: typically 33 bytes (0x80 + 32 bytes, or less if leading zeros) | ||
| /// - s: typically 33 bytes (0x80 + 32 bytes, or less if leading zeros) | ||
| const SIGNATURE_RLP_OVERHEAD: usize = 1 + 33 + 33; | ||
|
|
||
| /// Calculates the RLP-encoded size of the signed transaction for DoS protection. | ||
| /// | ||
| /// This mirrors geth's `tx.Size()` and reth's `transaction.encoded_length()` which use | ||
| /// actual RLP encoding to determine transaction size. We use the `encoded_len()` method | ||
| /// from the ethereum crate on the fully-populated message. | ||
| pub fn encoded_length(&self) -> usize { | ||
| match self { | ||
| // Legacy: RLP([nonce, gasPrice, gasLimit, to, value, data, v, r, s]) | ||
| TransactionMessage::Legacy(msg) => msg.encoded_len() + Self::SIGNATURE_RLP_OVERHEAD, | ||
| // EIP-2930: 0x01 || RLP([chainId, nonce, gasPrice, gasLimit, to, value, data, accessList, yParity, r, s]) | ||
| TransactionMessage::EIP2930(msg) => { | ||
| 1 + msg.encoded_len() + Self::SIGNATURE_RLP_OVERHEAD | ||
| } | ||
| // EIP-1559: 0x02 || RLP([chainId, nonce, maxPriorityFeePerGas, maxFeePerGas, gasLimit, to, value, data, accessList, yParity, r, s]) | ||
| TransactionMessage::EIP1559(msg) => { | ||
| 1 + msg.encoded_len() + Self::SIGNATURE_RLP_OVERHEAD | ||
| } | ||
| // EIP-7702: 0x04 || RLP([chainId, nonce, maxPriorityFeePerGas, maxFeePerGas, gasLimit, to, value, data, accessList, authorizationList, yParity, r, s]) | ||
| TransactionMessage::EIP7702(msg) => { | ||
| 1 + msg.encoded_len() + Self::SIGNATURE_RLP_OVERHEAD | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Validates that the estimated signed transaction size is within limits. | ||
| /// | ||
| /// This prevents DoS attacks via oversized transactions before they enter the pool. | ||
| /// The limit matches geth's `txMaxSize` and reth's `DEFAULT_MAX_TX_INPUT_BYTES`. | ||
| /// | ||
| /// This should be called on the fully-populated message (after nonce, gas limit, | ||
| /// gas price, chain ID, etc. have been filled in) to ensure the final transaction | ||
| /// size is accurately measured. | ||
| pub fn validate_size(&self) -> Result<(), String> { | ||
| let size = self.encoded_length(); | ||
|
|
||
| if size > DEFAULT_MAX_TX_INPUT_BYTES { | ||
| return Err(format!( | ||
| "oversized data: transaction size {size} exceeds limit {DEFAULT_MAX_TX_INPUT_BYTES}" | ||
| )); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
validate_size() is still an estimate, not the final signed size.
encoded_length() adds a fixed 67-byte signature allowance, but signed transaction RLP is not fixed-width: r/s can shrink when they have leading zeroes, and legacy v is variable. Because client/rpc/src/eth/submit.rs now uses this as the hard reject for eth_sendTransaction, near-limit requests can be rejected here even though the signed bytes would fit and pass the raw path. Please keep this as a pre-sign guard only, and enforce the definitive 128 KiB limit on the signed transaction bytes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/rpc-core/src/types/transaction_request.rs` around lines 269 - 317, The
current TransactionMessage::encoded_length() and validate_size() use a fixed
67-byte signature allowance and therefore only produce an estimate; change
validate_size() to be explicitly a pre-signing guard (e.g., only check against a
conservative threshold or return a non-fatal warning) and stop using it as the
final hard reject for eth_sendTransaction; instead, in the submit path
(client/rpc/src/eth/submit.rs) after creating the actual signed transaction
bytes call the definitive check against the signed-byte limit (128 KiB) and
return an error if signed_bytes.len() > 128*1024. Update comments on
TransactionMessage::encoded_length and validate_size to state they are estimates
and reference TransactionMessage::encoded_length,
TransactionMessage::validate_size and DEFAULT_MAX_TX_INPUT_BYTES so reviewers
can find the changed logic.
There was a problem hiding this comment.
It is meant to be an estimate. Also on the etherum side the encoded size is just estimated.
There was a problem hiding this comment.
@manuelmauro, that's a fair point — thank you for the clarification. Since SIGNATURE_RLP_OVERHEAD = 67 is the maximum possible signature size (r and s can only be ≤ 33 bytes each, never more), encoded_length() is a conservative upper-bound estimate. This means:
- If
estimated_size > 128 KiB→ reject (the actual signed tx could be slightly smaller, so rare false positives are possible at the very edge) - If
estimated_size ≤ 128 KiB→ the actual signed tx is guaranteed to fit (no false negatives)
For DoS protection, this is the right trade-off — you never let an oversized transaction slip through; you might conservatively reject a few borderline ones. The approach is sound.
One minor suggestion: it may be worth updating the validate_size() doc comment to explicitly note that it uses a conservative upper-bound and that false positives are possible within SIGNATURE_RLP_OVERHEAD bytes of the limit. This would help future reviewers understand the intent without needing to dig into the constants.
(´• ω •`) 🐇
✏️ Learnings added
Learnt from: manuelmauro
Repo: polkadot-evm/frontier PR: 1807
File: client/rpc-core/src/types/transaction_request.rs:269-317
Timestamp: 2026-03-18T15:37:52.728Z
Learning: In `client/rpc-core/src/types/transaction_request.rs`, `TransactionMessage::encoded_length()` and `validate_size()` are intentionally conservative upper-bound estimates. `SIGNATURE_RLP_OVERHEAD = 67` bytes is the maximum possible RLP signature size (r and s ≤ 33 bytes each). This guarantees no false negatives for the 128 KiB DoS guard (oversized txs are never let through), though rare false positives are possible within 67 bytes of the limit. The `ethereum` crate's `encoded_len()` is also an estimate.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
2 participants
Summary
eth_sendTransactionandeth_sendRawTransactionRPC endpointstxMaxSizelimit)Changes
encoded_length()andvalidate_size()methods toTransactionRequestTX_SLOT_BYTE_SIZE(32 KiB) andDEFAULT_MAX_TX_INPUT_BYTES(128 KiB) constantssend_transactionbefore signingsend_raw_transactionbefore decodingReferences
Note
Depends on: rust-ethereum/ethereum#76
Uses a temporary patch for the ethereum crate to access
encoded_len(). This should be removed once the changes are released.