-
Notifications
You must be signed in to change notification settings - Fork 201
feat(l1): cap mempool pending transactions per sender (default 16) #6603
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: main
Are you sure you want to change the base?
Changes from 3 commits
1dd07d1
b70ce5b
5838ac8
f6de3cf
00c681e
149b829
3df6bc8
d52d06b
ba25e61
887431b
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 |
|---|---|---|
|
|
@@ -183,6 +183,15 @@ pub struct Options { | |
| env = "ETHREX_MEMPOOL_MAX_SIZE" | ||
| )] | ||
| pub mempool_max_size: usize, | ||
| #[arg( | ||
| help = "Maximum number of pending transactions a single sender may hold in the mempool. Replacements at an existing (sender, nonce) bypass this cap.", | ||
| long = "mempool.max-pending-txs-per-account", | ||
| default_value_t = 16, | ||
| value_name = "MAX_PENDING_TXS_PER_ACCOUNT", | ||
| help_heading = "Node options", | ||
| env = "ETHREX_MEMPOOL_MAX_PENDING_TXS_PER_ACCOUNT" | ||
| )] | ||
| pub mempool_max_pending_txs_per_account: usize, | ||
| #[arg( | ||
| long = "http.addr", | ||
| default_value = "0.0.0.0", | ||
|
|
@@ -450,6 +459,7 @@ impl Default for Options { | |
| dev: Default::default(), | ||
| force: false, | ||
| mempool_max_size: Default::default(), | ||
| mempool_max_pending_txs_per_account: 16, | ||
| tx_broadcasting_time_interval: Default::default(), | ||
|
Comment on lines
473
to
477
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. |
||
| target_peers: Default::default(), | ||
| lookup_interval: Default::default(), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -231,6 +231,10 @@ pub struct BlockchainOptions { | |
| /// warmer thread and the executor. Set to false (via `--no-precompile-cache`) to | ||
| /// disable the cache for benchmarking purposes. | ||
| pub precompile_cache_enabled: bool, | ||
| /// Maximum number of pending transactions a single sender may hold in | ||
| /// the mempool. A replacement at an existing `(sender, nonce)` bypasses | ||
| /// this check. | ||
| pub max_pending_txs_per_account: usize, | ||
| } | ||
|
|
||
| impl Default for BlockchainOptions { | ||
|
|
@@ -242,10 +246,14 @@ impl Default for BlockchainOptions { | |
| max_blobs_per_block: None, | ||
| precompute_witnesses: false, | ||
| precompile_cache_enabled: true, | ||
| max_pending_txs_per_account: DEFAULT_MAX_PENDING_TXS_PER_ACCOUNT, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Default per-account pending-tx cap. | ||
| pub const DEFAULT_MAX_PENDING_TXS_PER_ACCOUNT: usize = 16; | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub struct BatchBlockProcessingFailure { | ||
| pub last_valid_hash: H256, | ||
|
|
@@ -2507,6 +2515,19 @@ impl Blockchain { | |
| // If it exists check if the new tx has higher fees | ||
| let tx_to_replace_hash = self.mempool.find_tx_to_replace(sender, nonce, tx)?; | ||
|
|
||
| // Per-account pending-tx cap. Replacement candidates (same | ||
| // `(sender, nonce)`) bypass the cap — they don't grow the | ||
| // sender's pool footprint. | ||
| if tx_to_replace_hash.is_none() { | ||
| let count = self.mempool.count_for_sender(sender)?; | ||
| if count >= self.options.max_pending_txs_per_account { | ||
| return Err(MempoolError::MaxPendingTxsPerAccountExceeded { | ||
| count, | ||
| limit: self.options.max_pending_txs_per_account, | ||
| }); | ||
| } | ||
| } | ||
|
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. 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.
The count check in Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/blockchain/blockchain.rs
Line: 2521-2529
Comment:
**TOCTOU race allows cap to be exceeded under concurrent load**
The count check in `validate_transaction` and the actual `add_transaction` call hold separate, non-overlapping locks. Between releasing the read lock after `count_for_sender` and acquiring the write lock inside `add_transaction`, another concurrent submission from the same sender can pass the same check. Both threads see `count = 15 < 16`, both pass, and both are inserted, leaving the sender with 17 pending slots instead of the intended 16. Wrapping the validate-and-add sequence in a single write-locked section — or moving the count check inside `add_transaction` where the write lock is already held — would close this window.
How can I resolve this? If you propose a fix, please make it concise.
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. |
||
|
|
||
| if tx | ||
| .chain_id() | ||
| .is_some_and(|chain_id| chain_id != config.chain_id) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -83,6 +83,8 @@ pub enum MempoolError { | |||||||||
| TxMaxInitCodeSizeError, | ||||||||||
| #[error("Transaction max data size exceeded")] | ||||||||||
| TxMaxDataSizeError, | ||||||||||
| #[error("Sender has {count} pending transactions, exceeds the per-account cap of {limit}")] | ||||||||||
|
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. |
||||||||||
| MaxPendingTxsPerAccountExceeded { count: usize, limit: usize }, | ||||||||||
|
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.
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/blockchain/error.rs
Line: 86-87
Comment:
When `count == limit` the message reads "has 16 pending transactions, exceeds the per-account cap of 16", but 16 does not exceed 16. The phrasing should reflect that admitting a new transaction *would* exceed the cap.
```suggestion
#[error("Sender has {count} pending transactions; adding a new one would exceed the per-account cap of {limit}")]
MaxPendingTxsPerAccountExceeded { count: usize, limit: usize },
```
How can I resolve this? If you propose a fix, please make it concise.
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. |
||||||||||
| #[error("Transaction gas limit exceeded")] | ||||||||||
| TxGasLimitExceededError, | ||||||||||
| #[error( | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -451,6 +451,17 @@ impl Mempool { | |
| Ok(contains) | ||
| } | ||
|
|
||
| /// Returns the number of pending transactions currently held in the | ||
| /// mempool for `sender`. Used by the per-sender slot cap at admission. | ||
| pub fn count_for_sender(&self, sender: Address) -> Result<usize, MempoolError> { | ||
| let inner = self.read()?; | ||
| let count = inner | ||
| .txs_by_sender_nonce | ||
| .range((sender, 0)..=(sender, u64::MAX)) | ||
| .count(); | ||
|
Comment on lines
+479
to
+481
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. This is bounded, so I think it's OK for now. |
||
| Ok(count) | ||
| } | ||
|
|
||
| pub fn find_tx_to_replace( | ||
| &self, | ||
| sender: Address, | ||
|
|
@@ -574,3 +585,70 @@ pub fn transaction_intrinsic_gas( | |
|
|
||
| Ok(gas) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use ethrex_common::types::EIP1559Transaction; | ||
|
|
||
| fn build_tx(nonce: u64) -> Transaction { | ||
| Transaction::EIP1559Transaction(EIP1559Transaction { | ||
| nonce, | ||
| ..Default::default() | ||
| }) | ||
| } | ||
|
|
||
| fn add_tx(pool: &Mempool, sender: Address, nonce: u64) -> H256 { | ||
| let tx = build_tx(nonce); | ||
| let mtx = MempoolTransaction::new(tx, sender); | ||
| let hash = mtx.hash(); | ||
| pool.add_transaction(hash, sender, mtx).unwrap(); | ||
| hash | ||
| } | ||
|
|
||
| #[test] | ||
| fn count_for_sender_empty_pool() { | ||
| let pool = Mempool::new(64); | ||
| let sender = Address::from_low_u64_be(1); | ||
| assert_eq!(pool.count_for_sender(sender).unwrap(), 0); | ||
| } | ||
|
|
||
| #[test] | ||
| fn count_for_sender_one_tx() { | ||
| let pool = Mempool::new(64); | ||
| let sender = Address::from_low_u64_be(1); | ||
| add_tx(&pool, sender, 0); | ||
| assert_eq!(pool.count_for_sender(sender).unwrap(), 1); | ||
| } | ||
|
|
||
| #[test] | ||
| fn count_for_sender_many_nonces() { | ||
| let pool = Mempool::new(64); | ||
| let sender = Address::from_low_u64_be(1); | ||
| for nonce in 0..5 { | ||
| add_tx(&pool, sender, nonce); | ||
| } | ||
| assert_eq!(pool.count_for_sender(sender).unwrap(), 5); | ||
| } | ||
|
|
||
| #[test] | ||
| fn count_for_sender_isolates_senders() { | ||
| let pool = Mempool::new(64); | ||
| let a = Address::from_low_u64_be(1); | ||
| let b = Address::from_low_u64_be(2); | ||
| add_tx(&pool, a, 0); | ||
| add_tx(&pool, a, 1); | ||
| add_tx(&pool, b, 0); | ||
| assert_eq!(pool.count_for_sender(a).unwrap(), 2); | ||
| assert_eq!(pool.count_for_sender(b).unwrap(), 1); | ||
| } | ||
|
|
||
| #[test] | ||
| fn count_for_sender_unknown_returns_zero() { | ||
| let pool = Mempool::new(64); | ||
| let a = Address::from_low_u64_be(1); | ||
| let b = Address::from_low_u64_be(2); | ||
| add_tx(&pool, a, 0); | ||
| assert_eq!(pool.count_for_sender(b).unwrap(), 0); | ||
| } | ||
| } | ||
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.
f6de3cf