-
Notifications
You must be signed in to change notification settings - Fork 201
feat(l1): enforce per-transaction wire-size cap in mempool admission #6599
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
Changes from 1 commit
4570a8a
b2054ac
8833853
a731790
3ee3f72
9d21d06
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,6 +72,7 @@ use ethrex_common::types::{ | |
| }; | ||
| use ethrex_common::types::{ELASTICITY_MULTIPLIER, P2PTransaction}; | ||
| use ethrex_common::types::{Fork, MempoolTransaction}; | ||
| use ethrex_common::types::{MAX_BLOB_TX_SIZE, MAX_TX_SIZE}; | ||
| use ethrex_common::utils::keccak; | ||
| use ethrex_common::{Address, H256, TrieLogger, U256}; | ||
| pub use ethrex_common::{ | ||
|
|
@@ -2432,7 +2433,23 @@ impl Blockchain { | |
| .ok_or(MempoolError::NoBlockHeaderError)?; | ||
| let config = self.storage.get_chain_config(); | ||
|
|
||
| // NOTE: We could add a tx size limit here, but it's not in the actual spec | ||
| // Wire size cap: peer-policy default, not consensus. Matches geth | ||
| // `txMaxSize` (legacypool / blobpool), reth `DEFAULT_MAX_TX_INPUT_BYTES`, | ||
| // nethermind `MaxTxSize` / `MaxBlobTxSize`. For EIP-4844 the canonical | ||
| // encoding of `Transaction` excludes the sidecar (which lives in the | ||
| // adjacent `BlobsBundle`), so this caps the core tx only. | ||
| let encoded_len = tx.encode_canonical_to_vec().len(); | ||
|
Collaborator
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. We might want to find another way to compute this.
Collaborator
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. |
||
| let size_limit = if matches!(tx, Transaction::EIP4844Transaction(_)) { | ||
| MAX_BLOB_TX_SIZE | ||
| } else { | ||
| MAX_TX_SIZE | ||
| }; | ||
| if encoded_len > size_limit { | ||
| return Err(MempoolError::TxSizeExceeded { | ||
| actual: encoded_len, | ||
| limit: size_limit, | ||
| }); | ||
| } | ||
|
|
||
| // Check init code size | ||
| // [EIP-7954] - Amsterdam increases the limit | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3764,4 +3764,31 @@ mod tests { | |
| "blob-gas term missing from cost_without_base_fee() for EIP-4844" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_encoded_size_exceeds_max_tx_size() { | ||
| // Confirms the wire-size signal used by the mempool admission cap: | ||
| // an EIP-1559 tx with > 128 KiB of `data` encodes to > MAX_TX_SIZE bytes. | ||
| use crate::types::MAX_TX_SIZE; | ||
|
|
||
| let payload = vec![0u8; MAX_TX_SIZE + 1]; | ||
| let tx = Transaction::EIP1559Transaction(EIP1559Transaction { | ||
| data: Bytes::from(payload), | ||
| ..Default::default() | ||
| }); | ||
|
|
||
| assert!( | ||
| tx.encode_canonical_to_vec().len() > MAX_TX_SIZE, | ||
| "tx with > 128 KiB calldata must encode larger than MAX_TX_SIZE" | ||
| ); | ||
|
MegaRedHand marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| #[test] | ||
| fn test_encoded_size_below_max_tx_size() { | ||
| // A small tx encodes well below the cap. | ||
| use crate::types::MAX_TX_SIZE; | ||
|
|
||
| let tx = Transaction::EIP1559Transaction(EIP1559Transaction::default()); | ||
| assert!(tx.encode_canonical_to_vec().len() <= MAX_TX_SIZE); | ||
| } | ||
|
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.
Both new tests exercise Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/common/types/transaction.rs
Line: 3786-3793
Comment:
**No test for the `MAX_BLOB_TX_SIZE` cap**
Both new tests exercise `MAX_TX_SIZE` via EIP-1559 transactions, but there is no corresponding test that constructs an `EIP4844Transaction` with an encoded size just over `MAX_BLOB_TX_SIZE` (1 MiB) and verifies the signal fires. The blob tx branch in `validate_transaction` is the riskier path (1 MiB limit vs 128 KiB), so a parallel "exceeds / below" pair for blob txs would meaningfully close the coverage gap.
How can I resolve this? If you propose a fix, please make it concise. |
||
| } | ||
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.
a731790