-
Notifications
You must be signed in to change notification settings - Fork 547
Add transaction size validation to RPC endpoints #1807
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 6 commits
4d7f850
c242d68
3bc944f
fc974a5
b5eef63
1b2f4fc
a5c026f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,13 @@ use serde::{Deserialize, Deserializer}; | |
|
|
||
| use crate::types::Bytes; | ||
|
|
||
| /// The default byte size of a transaction slot (32 KiB). | ||
| pub const TX_SLOT_BYTE_SIZE: usize = 32 * 1024; | ||
|
|
||
| /// The default maximum size a single transaction can have (128 KiB). | ||
| /// This is the RLP-encoded size of the signed transaction. | ||
| pub const DEFAULT_MAX_TX_INPUT_BYTES: usize = 4 * TX_SLOT_BYTE_SIZE; | ||
|
|
||
| /// Transaction request from the RPC. | ||
| #[derive(Clone, Debug, Default, Eq, PartialEq, Deserialize)] | ||
| #[serde(rename_all = "camelCase")] | ||
|
|
@@ -174,6 +181,27 @@ impl TransactionRequest { | |
| fn chain_id_u64(&self) -> u64 { | ||
| self.chain_id.map(|id| id.as_u64()).unwrap_or_default() | ||
| } | ||
|
|
||
| /// Calculates the RLP-encoded size of the signed transaction for DoS protection. | ||
| /// | ||
| /// This converts the request to a `TransactionMessage` using default values for any | ||
| /// missing fields, then delegates to `TransactionMessage::encoded_length()`. | ||
| /// | ||
| /// **Note:** For `eth_sendTransaction`, prefer calling `TransactionMessage::validate_size()` | ||
| /// on the fully-populated message (after nonce, gas, fees, chain ID are filled in) | ||
| /// to get an accurate size measurement. | ||
| /// | ||
| /// Returns an error if the transaction request cannot be converted to a valid message type. | ||
| pub fn encoded_length(&self) -> Result<usize, String> { | ||
| let message: Option<TransactionMessage> = self.clone().into(); | ||
|
|
||
| match message { | ||
| Some(msg) => Ok(msg.encoded_length()), | ||
| None => Err( | ||
| "invalid transaction parameters: unable to determine transaction type".to_string(), | ||
| ), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Additional data of the transaction. | ||
|
|
@@ -238,6 +266,57 @@ pub enum TransactionMessage { | |
| EIP7702(EIP7702TransactionMessage), | ||
| } | ||
|
|
||
| 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(()) | ||
| } | ||
| } | ||
|
|
||
| impl From<TransactionRequest> for Option<TransactionMessage> { | ||
| fn from(req: TransactionRequest) -> Self { | ||
| // Common fields extraction - these are used by all transaction types | ||
|
|
@@ -446,4 +525,158 @@ mod tests { | |
| } | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_message_size_validation_large_access_list() { | ||
| use ethereum::AccessListItem; | ||
| use ethereum_types::{H160, H256}; | ||
|
|
||
| // Create access list that exceeds 128KB (131,072 bytes) | ||
| // Each storage key RLP-encodes to ~33 bytes | ||
| // 4000 keys * 33 bytes = 132,000 bytes > 128KB | ||
| let storage_keys: Vec<H256> = (0..4000).map(|_| H256::default()).collect(); | ||
| let access_list = vec![AccessListItem { | ||
| address: H160::default(), | ||
| storage_keys, | ||
| }]; | ||
| let request = TransactionRequest { | ||
| access_list: Some(access_list), | ||
| ..Default::default() | ||
| }; | ||
| let message: Option<TransactionMessage> = request.into(); | ||
| assert!(message.unwrap().validate_size().is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_message_size_validation_valid() { | ||
| use ethereum::AccessListItem; | ||
| use ethereum_types::{H160, H256}; | ||
|
|
||
| // 100 storage keys is well under 128KB | ||
| let request = TransactionRequest { | ||
| access_list: Some(vec![AccessListItem { | ||
| address: H160::default(), | ||
| storage_keys: vec![H256::default(); 100], | ||
| }]), | ||
| ..Default::default() | ||
| }; | ||
| let message: Option<TransactionMessage> = request.into(); | ||
| assert!(message.unwrap().validate_size().is_ok()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_encoded_length_includes_signature_overhead() { | ||
| // A minimal EIP-1559 transaction should include signature overhead | ||
| // Default TransactionRequest converts to EIP-1559 (no gas_price, no access_list) | ||
| let request = TransactionRequest::default(); | ||
| let size = request.encoded_length().unwrap(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please never use unwrap in production code, the error should be propagated instead
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not even in tests?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In tests it's ok but I prefer expect even in tests
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in a5c026f |
||
|
|
||
| // EIP-1559 message RLP: ~11 bytes for minimal fields (all zeros/empty) | ||
| // + 1 byte type prefix + 67 bytes signature overhead = ~79 bytes minimum | ||
| // The signature overhead (67 bytes) is the key verification | ||
| assert!( | ||
| size >= TransactionMessage::SIGNATURE_RLP_OVERHEAD, | ||
| "Size {} should be at least signature overhead {}", | ||
| size, | ||
| TransactionMessage::SIGNATURE_RLP_OVERHEAD | ||
| ); | ||
|
|
||
| // Verify it's a reasonable size for a minimal transaction | ||
| assert!( | ||
| size < 200, | ||
| "Size {size} should be reasonable for minimal tx" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_encoded_length_typed_transaction_overhead() { | ||
| use ethereum::AccessListItem; | ||
| use ethereum_types::H160; | ||
|
|
||
| // EIP-1559 transaction (has max_fee_per_gas) | ||
| let request = TransactionRequest { | ||
| max_fee_per_gas: Some(U256::from(1000)), | ||
| access_list: Some(vec![AccessListItem { | ||
| address: H160::default(), | ||
| storage_keys: vec![], | ||
| }]), | ||
| ..Default::default() | ||
| }; | ||
| let typed_size = request.encoded_length().unwrap(); | ||
|
|
||
| // Legacy transaction | ||
| let legacy_request = TransactionRequest { | ||
| gas_price: Some(U256::from(1000)), | ||
| ..Default::default() | ||
| }; | ||
| let legacy_size = legacy_request.encoded_length().unwrap(); | ||
|
|
||
| // Typed transaction should be larger due to: | ||
| // - Type byte (+1) | ||
| // - Chain ID (+9) | ||
| // - max_priority_fee_per_gas (+33) | ||
| // - Access list overhead | ||
| assert!( | ||
| typed_size > legacy_size, | ||
| "Typed tx {typed_size} should be larger than legacy {legacy_size}" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_encoded_length_access_list_scaling() { | ||
| use ethereum::AccessListItem; | ||
| use ethereum_types::{H160, H256}; | ||
|
|
||
| // Transaction with 10 storage keys | ||
| let request_10 = TransactionRequest { | ||
| access_list: Some(vec![AccessListItem { | ||
| address: H160::default(), | ||
| storage_keys: vec![H256::default(); 10], | ||
| }]), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| // Transaction with 100 storage keys | ||
| let request_100 = TransactionRequest { | ||
| access_list: Some(vec![AccessListItem { | ||
| address: H160::default(), | ||
| storage_keys: vec![H256::default(); 100], | ||
| }]), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let size_10 = request_10.encoded_length().unwrap(); | ||
| let size_100 = request_100.encoded_length().unwrap(); | ||
|
|
||
| // Size should scale roughly linearly with storage keys | ||
| // 90 additional keys * ~34 bytes each ≈ 3060 bytes difference | ||
| let diff = size_100 - size_10; | ||
| assert!( | ||
| diff > 2500 && diff < 4000, | ||
| "Size difference {diff} should be proportional to storage keys" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_constants_match_geth_reth() { | ||
| // Verify our constants match geth/reth exactly | ||
| assert_eq!(TX_SLOT_BYTE_SIZE, 32 * 1024); // 32 KiB | ||
| assert_eq!(DEFAULT_MAX_TX_INPUT_BYTES, 128 * 1024); // 128 KiB | ||
| assert_eq!(DEFAULT_MAX_TX_INPUT_BYTES, 4 * TX_SLOT_BYTE_SIZE); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_encoded_length_invalid_parameters() { | ||
| // Transaction with both max_fee_per_gas and gas_price set (invalid combination) | ||
| let request = TransactionRequest { | ||
| max_fee_per_gas: Some(U256::from(1000)), | ||
| gas_price: Some(U256::from(500)), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| assert!(request.encoded_length().is_err()); | ||
| // Invalid parameters also fail conversion, so validate_size is unreachable | ||
| let message: Option<TransactionMessage> = request.into(); | ||
| assert!(message.is_none()); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/scan shrink when they have leading zeroes, and legacyvis variable. Becauseclient/rpc/src/eth/submit.rsnow uses this as the hard reject foreth_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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is meant to be an estimate. Also on the
etherumside the encoded size is just estimated.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@manuelmauro, that's a fair point — thank you for the clarification. SinceSIGNATURE_RLP_OVERHEAD = 67is 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:estimated_size > 128 KiB→ reject (the actual signed tx could be slightly smaller, so rare false positives are possible at the very edge)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 withinSIGNATURE_RLP_OVERHEADbytes of the limit. This would help future reviewers understand the intent without needing to dig into the constants.(´• ω •`) 🐇
✏️ Learnings added