-
Notifications
You must be signed in to change notification settings - Fork 201
feat(l1): reject gapped-nonce txs under heavy mempool pressure #6609
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 1 commit
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 |
|---|---|---|
|
|
@@ -118,6 +118,9 @@ use ethrex_common::types::BlobsBundle; | |
|
|
||
| const MAX_PAYLOADS: usize = 10; | ||
| const MAX_MEMPOOL_SIZE_DEFAULT: usize = 10_000; | ||
| /// Default mempool occupancy percentage (0-100) at which gapped-nonce | ||
| /// transaction admission is denied. Set to 100 to disable the check. | ||
| pub const DEFAULT_GAP_ADMIT_OCCUPANCY_THRESHOLD: u8 = 90; | ||
|
|
||
| /// Background thread for dropping large tree structures off the critical path. | ||
| /// Accepts any `Send` value and drops it on a dedicated thread, avoiding | ||
|
|
@@ -231,6 +234,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, | ||
| /// Mempool occupancy percentage (0-100) at or above which incoming | ||
| /// transactions with a nonce gap relative to the sender's on-chain nonce | ||
| /// are rejected. Setting to 100 disables the check. | ||
| pub gap_admit_occupancy_threshold: u8, | ||
| } | ||
|
|
||
| impl Default for BlockchainOptions { | ||
|
|
@@ -242,6 +249,7 @@ impl Default for BlockchainOptions { | |
| max_blobs_per_block: None, | ||
| precompute_witnesses: false, | ||
| precompile_cache_enabled: true, | ||
| gap_admit_occupancy_threshold: DEFAULT_GAP_ADMIT_OCCUPANCY_THRESHOLD, | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -2486,7 +2494,7 @@ impl Blockchain { | |
|
|
||
| let maybe_sender_acc_info = self.storage.get_account_info(header_no, sender).await?; | ||
|
|
||
| if let Some(sender_acc_info) = maybe_sender_acc_info { | ||
| let sender_acc_nonce = if let Some(sender_acc_info) = &maybe_sender_acc_info { | ||
| if nonce < sender_acc_info.nonce || nonce == u64::MAX { | ||
| return Err(MempoolError::NonceTooLow); | ||
| } | ||
|
|
@@ -2498,10 +2506,11 @@ impl Blockchain { | |
| if tx_cost > sender_acc_info.balance { | ||
| return Err(MempoolError::NotEnoughBalance); | ||
| } | ||
| sender_acc_info.nonce | ||
| } else { | ||
| // An account that is not in the database cannot possibly have enough balance to cover the transaction cost | ||
| return Err(MempoolError::NotEnoughBalance); | ||
| } | ||
| }; | ||
|
|
||
| // Check the nonce of pendings TXs in the mempool from the same sender | ||
| // If it exists check if the new tx has higher fees | ||
|
|
@@ -2514,6 +2523,24 @@ impl Blockchain { | |
| return Err(MempoolError::InvalidChainId(config.chain_id)); | ||
| } | ||
|
|
||
| // When the mempool is heavily occupied, reject incoming transactions | ||
| // whose nonce is not contiguous with the sender's on-chain nonce. This | ||
| // prevents a flood of gapped-nonce spam txs from pinning pool budget | ||
| // that productive txs could use. Replacements (same nonce as a tx | ||
| // already in the pool) bypass this rule since they are not gapped. | ||
| let threshold = self.options.gap_admit_occupancy_threshold; | ||
| if tx_to_replace_hash.is_none() | ||
| && nonce != sender_acc_nonce | ||
| && self.mempool.is_heavily_occupied(threshold)? | ||
| { | ||
| let occupancy_pct = (self.mempool.occupancy_ratio()? * 100.0).round() as u8; | ||
| let nonce_gap = nonce.saturating_sub(sender_acc_nonce); | ||
| return Err(MempoolError::GapAdmissionDeniedUnderPressure { | ||
| occupancy_pct, | ||
| nonce_gap, | ||
| }); | ||
|
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.
Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/blockchain/blockchain.rs
Line: 2532-2541
Comment:
**TOCTOU between admission check and error-message occupancy**
`is_heavily_occupied` acquires and releases the read lock internally (via `occupancy_ratio`), then the `if`-body calls `occupancy_ratio` a second time under a fresh lock. Between those two lock acquisitions the pool can drain or fill, so the `occupancy_pct` printed in the error can be lower than the threshold — e.g. "Mempool 88% full; rejecting gapped-nonce tx" when the pool was actually at 91% when the check fired. Operators using this message to diagnose admission behaviour will see inconsistent numbers. Consider capturing the ratio from inside `is_heavily_occupied` (return it alongside the bool, or compute both from a single lock hold) so that the same snapshot drives both the decision and the diagnostic.
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. |
||
| } | ||
|
|
||
| Ok(tx_to_replace_hash) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -377,6 +377,32 @@ impl Mempool { | |
| Ok((txs_size as u64, blobs_size as u64)) | ||
| } | ||
|
|
||
| /// Returns the current occupancy of the transaction pool as a fraction of | ||
| /// its configured maximum size, in the range `[0.0, 1.0]`. | ||
| /// | ||
| /// Returns `0.0` when the pool has unlimited capacity (`max_mempool_size == 0`) | ||
| /// to avoid a division by zero and to signal that pressure-gated admission | ||
| /// rules should treat the pool as empty in that configuration. | ||
| pub fn occupancy_ratio(&self) -> Result<f64, MempoolError> { | ||
| let inner = self.read()?; | ||
| if inner.max_mempool_size == 0 { | ||
| return Ok(0.0); | ||
| } | ||
| let ratio = inner.transaction_pool.len() as f64 / inner.max_mempool_size as f64; | ||
| Ok(ratio.min(1.0)) | ||
| } | ||
|
|
||
| /// Returns true when the transaction pool occupancy is at or above the | ||
| /// given threshold percentage (0-100). A threshold of 100 effectively | ||
| /// disables the check, since occupancy can never exceed 100%. | ||
| pub fn is_heavily_occupied(&self, threshold_pct: u8) -> Result<bool, MempoolError> { | ||
|
Contributor
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.
So this method's only consumer is its own tests. Two options:
Leaning toward (1) — the inline comparison is cheaper and the percentage value is needed for the error message regardless. Not blocking. |
||
| if threshold_pct >= 100 { | ||
| return Ok(false); | ||
| } | ||
| let ratio = self.occupancy_ratio()?; | ||
| Ok(ratio * 100.0 >= threshold_pct as f64) | ||
|
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. |
||
| } | ||
|
Comment on lines
+405
to
+417
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.
When Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/blockchain/mempool.rs
Line: 398-404
Comment:
**`is_heavily_occupied(0)` always returns `true` even for unlimited pools**
When `max_mempool_size == 0` (unlimited), `occupancy_ratio()` returns `0.0` — intentionally, per the doc-comment, to signal that pressure-gated rules should treat the pool as empty. However, `is_heavily_occupied(0)` evaluates `0.0 * 100.0 >= 0.0`, which is `true`, so all gapped transactions are rejected even when the pool has no capacity limit. The "treat as empty" contract documented on `occupancy_ratio` is silently broken for the edge case of threshold=0 combined with an unlimited pool. Adding a fast-path `if inner.max_mempool_size == 0 { return Ok(false); }` at the top of `is_heavily_occupied` would make the unlimited-pool guarantee explicit and consistent with the `occupancy_ratio` doc-comment.
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. |
||
|
|
||
| /// Returns all transactions currently in the pool | ||
| pub fn content(&self) -> Result<Vec<Transaction>, MempoolError> { | ||
| let pooled_transactions = &self.read()?.transaction_pool; | ||
|
|
@@ -574,3 +600,77 @@ pub fn transaction_intrinsic_gas( | |
|
|
||
| Ok(gas) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use ethrex_common::types::{EIP1559Transaction, Transaction, TxKind}; | ||
| use ethrex_common::{Address, Bytes}; | ||
|
|
||
| fn dummy_mempool_tx(sender: Address, nonce: u64) -> MempoolTransaction { | ||
| let tx = Transaction::EIP1559Transaction(EIP1559Transaction { | ||
| nonce, | ||
| max_priority_fee_per_gas: 0, | ||
| max_fee_per_gas: 0, | ||
| gas_limit: 21_000, | ||
| to: TxKind::Call(Address::from_low_u64_be(1)), | ||
| value: U256::zero(), | ||
| data: Bytes::default(), | ||
| access_list: Default::default(), | ||
| ..Default::default() | ||
| }); | ||
| MempoolTransaction::new(tx, sender) | ||
| } | ||
|
|
||
| fn fill_mempool(mempool: &Mempool, count: usize) { | ||
| for i in 0..count { | ||
| let sender = Address::from_low_u64_be(i as u64 + 1); | ||
| let hash = H256::from_low_u64_be(i as u64 + 1); | ||
| mempool | ||
| .add_transaction(hash, sender, dummy_mempool_tx(sender, 0)) | ||
| .expect("Failed to add transaction"); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn occupancy_ratio_empty_pool() { | ||
| let mempool = Mempool::new(100); | ||
| assert_eq!(mempool.occupancy_ratio().unwrap(), 0.0); | ||
| } | ||
|
|
||
| #[test] | ||
| fn occupancy_ratio_half_full_pool() { | ||
| let mempool = Mempool::new(100); | ||
| fill_mempool(&mempool, 50); | ||
| assert!((mempool.occupancy_ratio().unwrap() - 0.5).abs() < f64::EPSILON); | ||
| } | ||
|
|
||
| #[test] | ||
| fn occupancy_ratio_full_pool() { | ||
| let mempool = Mempool::new(100); | ||
| fill_mempool(&mempool, 100); | ||
| assert_eq!(mempool.occupancy_ratio().unwrap(), 1.0); | ||
| } | ||
|
|
||
| #[test] | ||
| fn is_heavily_occupied_disabled_at_threshold_100() { | ||
| let mempool = Mempool::new(100); | ||
| fill_mempool(&mempool, 100); | ||
| // Threshold of 100 disables the check. | ||
| assert!(!mempool.is_heavily_occupied(100).unwrap()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn is_heavily_occupied_below_threshold() { | ||
| let mempool = Mempool::new(100); | ||
| fill_mempool(&mempool, 50); | ||
| assert!(!mempool.is_heavily_occupied(90).unwrap()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn is_heavily_occupied_at_or_above_threshold() { | ||
| let mempool = Mempool::new(100); | ||
| fill_mempool(&mempool, 91); | ||
| assert!(mempool.is_heavily_occupied(90).unwrap()); | ||
| } | ||
| } | ||
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.
348b680