-
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 all commits
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 |
|---|---|---|
|
|
@@ -377,6 +377,45 @@ impl Mempool { | |
| Ok((txs_size as u64, blobs_size as u64)) | ||
| } | ||
|
|
||
| /// Returns the current occupancy of the transaction pool as an integer | ||
| /// percentage of its configured maximum size, in the range `[0, 100]`. | ||
| /// | ||
| /// Returns `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. | ||
| /// | ||
| /// The computation uses integer arithmetic (`len * 100 / max`) so threshold | ||
| /// boundary comparisons are deterministic — float rounding from a prior | ||
| /// `f64`-based implementation could misclassify near-boundary cases. | ||
| pub fn occupancy_pct(&self) -> Result<u8, MempoolError> { | ||
| let inner = self.read()?; | ||
| if inner.max_mempool_size == 0 { | ||
| return Ok(0); | ||
| } | ||
| let pct = inner.transaction_pool.len().saturating_mul(100) / inner.max_mempool_size; | ||
| Ok(pct.min(100) as u8) | ||
| } | ||
|
|
||
| /// Returns `true` when the transaction pool occupancy is at or above the | ||
| /// given threshold percentage (`0..=100`). A threshold of 100 disables | ||
| /// the check entirely (occupancy can never exceed 100%); a threshold of | ||
| /// 0 on an unlimited pool (`max_mempool_size == 0`) also returns | ||
| /// `false`, treating "no cap" as "never under pressure" so gapped | ||
| /// admission isn't blanket-rejected when capacity is unbounded. | ||
| pub fn is_heavily_occupied(&self, threshold_pct: u8) -> Result<bool, MempoolError> { | ||
| if threshold_pct >= 100 { | ||
| return Ok(false); | ||
| } | ||
| let inner = self.read()?; | ||
| if inner.max_mempool_size == 0 { | ||
| return Ok(false); | ||
| } | ||
| let pool_len = inner.transaction_pool.len(); | ||
| let max = inner.max_mempool_size; | ||
| // `pool_len * 100 >= threshold_pct * max`, in integer arithmetic. | ||
| Ok(pool_len.saturating_mul(100) >= max.saturating_mul(threshold_pct as usize)) | ||
| } | ||
|
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 +613,87 @@ 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_pct_empty_pool() { | ||
| let mempool = Mempool::new(100); | ||
| assert_eq!(mempool.occupancy_pct().unwrap(), 0); | ||
| } | ||
|
|
||
| #[test] | ||
| fn occupancy_pct_half_full_pool() { | ||
| let mempool = Mempool::new(100); | ||
| fill_mempool(&mempool, 50); | ||
| assert_eq!(mempool.occupancy_pct().unwrap(), 50); | ||
| } | ||
|
|
||
| #[test] | ||
| fn occupancy_pct_full_pool() { | ||
| let mempool = Mempool::new(100); | ||
| fill_mempool(&mempool, 100); | ||
| assert_eq!(mempool.occupancy_pct().unwrap(), 100); | ||
| } | ||
|
|
||
| #[test] | ||
| fn is_heavily_occupied_on_unlimited_pool_is_false() { | ||
| // max_mempool_size == 0 means unlimited; pressure-gated rules must | ||
| // NOT fire under that configuration even when threshold_pct is 0. | ||
| let mempool = Mempool::new(0); | ||
| assert!(!mempool.is_heavily_occupied(0).unwrap()); | ||
| assert!(!mempool.is_heavily_occupied(50).unwrap()); | ||
| assert!(!mempool.is_heavily_occupied(99).unwrap()); | ||
| } | ||
|
|
||
| #[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.
is_heavily_occupiedis exposed as a public API and has dedicated unit tests, but it's not called from anywhere outside its own tests. The admission gate atblockchain.rs:2537-2546deliberately usesoccupancy_pct()directly + inline>= thresholdcomparison (per the TOCTOU comment), which is the right call.So this method's only consumer is its own tests. Two options:
occupancy_pct()and let callers do the comparison inline (matching what the admission gate already does). One fewer method to maintain.let occupancy_pct = self.mempool.occupancy_pct()?; if occupancy_pct >= threshold { ... }withif self.mempool.is_heavily_occupied(threshold)? { ... }— but then you lose theoccupancy_pctvalue for the error message and would need to re-read (the TOCTOU concern the comment cites).Leaning toward (1) — the inline comparison is cheaper and the percentage value is needed for the error message regardless. Not blocking.