feat(l1): periodic mempool sweep for stale and dormant transactions#6610
feat(l1): periodic mempool sweep for stale and dormant transactions#6610ilitteri wants to merge 8 commits into
Conversation
Add a 1-minute background task that evicts mempool entries on two criteria: TTL (drop txs older than --mempool.lifetime, default 3h) and dormancy (drop all entries from senders whose top pending nonce exceeds the on-chain nonce by more than --mempool.max-nonce-gap and have been dormant for --mempool.dormancy, defaults 64 and 3h). The sweep is spawned in init_blockchain for both L1 and L2; it holds a Weak<Blockchain> so it terminates automatically once the blockchain is dropped. MempoolTransaction already tracks insertion time, so the evict methods read tx.time() directly; a new_with_timestamp constructor is added for test-time injection. Constants DEFAULT_MEMPOOL_LIFETIME, DEFAULT_DORMANCY, DEFAULT_MAX_NONCE_GAP, and MEMPOOL_SWEEP_INTERVAL live in crates/blockchain/constants.rs and back both the CLI defaults and the BlockchainOptions defaults. Duration flags are parsed via the existing ethrex_common::serde_utils::parse_duration.
ilitteri
requested review from
a team,
ManuelBilbao and
avilagaston9
as code owners
🤖 Kimi Code ReviewOverall Assessment: Solid implementation of mempool eviction logic with good test coverage and clear documentation. Two main issues need attention: blocking synchronization in async context and inefficient sender-based lookup. Critical Issues1. Blocking locks in async context (Performance/Risk)File: The mempool uses Recommendation: Either:
// In run_mempool_sweep
let result = tokio::task::spawn_blocking({
let mempool = chain.mempool.clone();
move || mempool.evict_stale(ttl)
}).await.expect("spawn_blocking failed");2. Inefficient sender lookup in dormancy sweep (Performance)File:
// Instead of scanning all transactions:
let hashes: Vec<H256> = inner
.txs_by_address // Use index if available
.get(&sender)
.cloned()
.unwrap_or_default();Minor Issues3. Fragile test pathFile: let genesis = serde_json::from_str::<Genesis>(include_str!(concat!(
env!("CARGO_MANIFEST_DIR"),
"/../../fixtures/genesis/l1.json"
))).expect("parse genesis");4. Clock drift sensitivityFile: Positive Feedback
NitpickFile: Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
Lines of code reportTotal lines added: Detailed view |
There was a problem hiding this comment.
Pull request overview
Adds a periodic, low-frequency mempool sweep to evict (1) stale transactions by TTL and (2) “dormant” senders with large nonce gaps whose pool entries have all aged past a dormancy window. This is wired into both L1 and L2 initialization and exposed via new CLI flags with documented defaults.
Changes:
- Implemented
Mempool::evict_stale(ttl)and asyncMempool::evict_dormant(store, max_nonce_gap, dormancy), plus unit tests. - Spawned a background sweep loop in
Blockchain(weakly-referenced) running everyMEMPOOL_SWEEP_INTERVAL. - Added CLI flags/env vars and docs for
--mempool.lifetime,--mempool.max-nonce-gap,--mempool.dormancy, backed by new constants.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/CLI.md | Documents the new mempool sweep flags and defaults for both node modes. |
| crates/common/types/transaction.rs | Adds MempoolTransaction::new_with_timestamp to support timestamp injection in tests. |
| crates/blockchain/mempool.rs | Implements TTL + dormancy eviction logic and adds unit tests for both sweeps. |
| crates/blockchain/constants.rs | Defines default durations/thresholds and sweep interval constants. |
| crates/blockchain/Cargo.toml | Adds serde_json dev-dependency for new mempool tests. |
| crates/blockchain/blockchain.rs | Adds sweep task spawning and the periodic sweep loop. |
| cmd/ethrex/utils.rs | Adds a duration parser wrapper for clap value parsing. |
| cmd/ethrex/l2/initializers.rs | Wires new mempool options into L2 BlockchainOptions and spawns the sweep task. |
| cmd/ethrex/initializers.rs | Wires new mempool options into L1 BlockchainOptions and spawns the sweep task. |
| cmd/ethrex/cli.rs | Adds new CLI flags/env vars, defaults, and passes options into blockchain init. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mut inner = self.write()?; | ||
| let mut evicted = 0usize; | ||
| for sender in dormant_senders { | ||
| let hashes: Vec<H256> = inner | ||
| .transaction_pool |
| let mut evicted = 0usize; | ||
| for sender in dormant_senders { | ||
| let hashes: Vec<H256> = inner | ||
| .transaction_pool | ||
| .iter() | ||
| .filter_map(|(hash, tx)| { | ||
| if tx.sender() == sender { | ||
| Some(*hash) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .collect(); | ||
| for hash in &hashes { | ||
| inner.remove_transaction_with_lock(hash)?; | ||
| evicted += 1; | ||
| } |
| // Snapshot per-sender (top_nonce, oldest_timestamp) under the read | ||
| // lock so we can do async storage lookups without holding it. | ||
| struct SenderStats { | ||
| top_nonce: u64, | ||
| newest_timestamp: u128, |
| let mut interval = tokio::time::interval(MEMPOOL_SWEEP_INTERVAL); | ||
| // Drop the first immediate tick — let the node finish initialising before | ||
| // doing any work. | ||
| interval.tick().await; | ||
| loop { |
🤖 Claude Code ReviewNow I have everything I need for a thorough review. Review: feat(l1): periodic mempool sweep for stale and dormant transactionsThe overall architecture is sound —
|
Greptile SummaryThis PR adds a 1-minute background task that periodically evicts stale and dormant transactions from the mempool, addressing two failure modes: unbounded lifetime of abandoned transactions and senders holding pool capacity with unreachable nonces. Two configurable sweeps are introduced — a TTL sweep (
Confidence Score: 3/5The sweep infrastructure and TTL eviction are safe, but the dormancy sweep has two correctness defects that can drop valid transactions in production before the fix is applied. The nonce gap check compares the sender's top pool nonce against the on-chain nonce rather than the minimum pool nonce, meaning a sender who queued 65+ sequential transactions starting at nonce 1 would be incorrectly evicted after 3 hours. In the same function, the read-lock snapshot is released before async storage lookups and re-acquired as a write lock, so any transaction submitted by the sender in that window is swept without the dormancy check being re-applied. crates/blockchain/mempool.rs — both the gap-computation logic and the snapshot/eviction lock split in evict_dormant need attention before merge.
|
| Filename | Overview |
|---|---|
| crates/blockchain/mempool.rs | Core eviction logic added — two bugs: gap uses top_nonce instead of min_nonce (evicts valid consecutive-nonce queues), and TOCTOU between snapshot read-lock and write-lock eviction can drop fresh transactions. |
| crates/blockchain/blockchain.rs | Adds spawn_mempool_sweep() and run_mempool_sweep(); Weak lifecycle and interval handling look correct. |
| cmd/ethrex/cli.rs | Adds three new CLI flags and wires them into BlockchainOptions; default duration helpers display as raw seconds (e.g. 10800s) instead of the more readable 3h form. |
| crates/blockchain/constants.rs | Adds well-documented sweep constants; all values look sensible. |
| crates/common/types/transaction.rs | Adds new_with_timestamp constructor for test injection of synthetic timestamps; correctly scoped and documented. |
| cmd/ethrex/initializers.rs | Passes new options into BlockchainOptions and calls spawn_mempool_sweep() for L1; straightforward wiring. |
| cmd/ethrex/l2/initializers.rs | Same pattern as L1 initializer — passes options and spawns sweep for L2. |
| cmd/ethrex/utils.rs | Thin wrapper around ethrex_common::serde_utils::parse_duration for clap value_parser; correct and well-documented. |
| crates/blockchain/Cargo.toml | Adds serde_json as a dev-dependency for genesis parsing in tests; appropriate placement. |
| docs/CLI.md | Documents the three new CLI flags in both L1 and L2 sections; defaults shown as 10800s instead of 3h, mirroring the CLI helper issue. |
Sequence Diagram
sequenceDiagram
participant Init as init_blockchain
participant BC as Blockchain (Arc)
participant Sweep as run_mempool_sweep (Weak)
participant MP as Mempool
participant Store as Storage
Init->>BC: spawn_mempool_sweep()
BC->>Sweep: tokio::spawn(run_mempool_sweep(Weak))
Note over Sweep: interval tick (dropped — init grace)
loop Every MEMPOOL_SWEEP_INTERVAL (60s)
Sweep->>BC: Weak::upgrade()
alt Arc still alive
BC-->>Sweep: "Arc<Blockchain>"
Sweep->>MP: evict_stale(ttl)
MP-->>Sweep: Ok(n_evicted)
Sweep->>MP: evict_dormant(store, max_gap, dormancy)
Note over MP,Store: Read lock - snapshot sender stats, Release lock
MP->>Store: get_latest_block_number()
loop per sender in snapshot
MP->>Store: get_account_info(block, sender)
Store-->>MP: Option AccountInfo
MP->>MP: check gap and dormancy
end
Note over MP: Write lock - evict dormant senders
MP-->>Sweep: Ok(n_evicted)
else Arc dropped
Sweep->>Sweep: return (task tears down)
end
end
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
crates/blockchain/mempool.rs:567-606
**Incorrect nonce used for gap check — flags legitimate senders**
`SenderStats` tracks `top_nonce`, and the gap is computed as `top_nonce - on_chain_nonce`. This catches the described "high-nonce wall" attack, but it also flags any sender whose queue of **consecutive** transactions happens to be longer than `max_nonce_gap`. For example: on-chain nonce = 0, pool nonces = 1 … 65. Gap = 65 > 64, but nonce 1 is the very next executable tx — nothing is unreachable. These transactions would be evicted even though they're entirely valid.
The check that correctly distinguishes an unreachable wall from a large-but-valid queue is `min_nonce_in_pool - on_chain_nonce > max_nonce_gap`. A min-nonce field needs to be added to `SenderStats` and tracked in the accumulation loop the same way `top_nonce` is.
### Issue 2 of 3
crates/blockchain/mempool.rs:611-631
**TOCTOU: fresh transactions can be evicted after dormancy snapshot**
The function releases the read lock after taking the snapshot, does async storage lookups (potentially hundreds of milliseconds), then acquires a write lock to remove transactions. Any transaction submitted by a "dormant" sender in that window will be collected in the per-sender `hashes` vector and removed unconditionally, even though it was fresh and was never observed in the snapshot that triggered the eviction decision.
Re-checking `tx.time() < cutoff` for each hash inside the write-lock phase before calling `remove_transaction_with_lock` would close the race without needing to hold the read lock across the awaits.
### Issue 3 of 3
cmd/ethrex/cli.rs:69-79
The helpers convert the constant to `{secs}s`, so both `--mempool.lifetime` and `--mempool.dormancy` display as `10800s` in `--help` and in `CLI.md`. Expressing the default as `3h` directly matches the format the parser already accepts and is far more readable to operators.
```suggestion
/// Format the default mempool lifetime as a clap-compatible string,
/// reusing the [`DEFAULT_MEMPOOL_LIFETIME`] constant so it stays in sync.
fn default_mempool_lifetime_str() -> clap::builder::OsStr {
let secs = DEFAULT_MEMPOOL_LIFETIME.as_secs();
let h = secs / 3600;
let m = (secs % 3600) / 60;
let s = secs % 60;
match (h, m, s) {
(h, 0, 0) => format!("{}h", h),
(0, m, 0) => format!("{}m", m),
(0, 0, s) => format!("{}s", s),
(h, m, 0) => format!("{}h{}m", h, m),
_ => format!("{}s", secs),
}
.into()
}
/// Format the default mempool dormancy as a clap-compatible string,
/// reusing the [`DEFAULT_DORMANCY`] constant so it stays in sync.
fn default_mempool_dormancy_str() -> clap::builder::OsStr {
let secs = DEFAULT_DORMANCY.as_secs();
let h = secs / 3600;
let m = (secs % 3600) / 60;
let s = secs % 60;
match (h, m, s) {
(h, 0, 0) => format!("{}h", h),
(0, m, 0) => format!("{}m", m),
(0, 0, s) => format!("{}s", s),
(h, m, 0) => format!("{}h{}m", h, m),
_ => format!("{}s", secs),
}
.into()
}
```
Reviews (1): Last reviewed commit: "feat(l1): periodic mempool sweep for sta..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let snapshot: FxHashMap<Address, SenderStats> = { | ||
| let inner = self.read()?; | ||
| let mut acc: FxHashMap<Address, SenderStats> = | ||
| FxHashMap::with_capacity_and_hasher(128, Default::default()); | ||
| for tx in inner.transaction_pool.values() { | ||
| let entry = acc.entry(tx.sender()).or_insert(SenderStats { | ||
| top_nonce: tx.nonce(), | ||
| newest_timestamp: tx.time(), | ||
| }); | ||
| if tx.nonce() > entry.top_nonce { | ||
| entry.top_nonce = tx.nonce(); | ||
| } | ||
| if tx.time() > entry.newest_timestamp { | ||
| entry.newest_timestamp = tx.time(); | ||
| } | ||
| } | ||
| acc | ||
| }; | ||
|
|
||
| let latest_block_number = store.get_latest_block_number().await?; | ||
|
|
||
| let mut dormant_senders: Vec<Address> = Vec::new(); | ||
| for (sender, stats) in &snapshot { | ||
| // Newest entry must itself be older than the dormancy window, | ||
| // which is equivalent to "all entries are older than dormancy". | ||
| if stats.newest_timestamp >= cutoff { | ||
| continue; | ||
| } | ||
|
|
||
| let on_chain_nonce = store | ||
| .get_account_info(latest_block_number, *sender) | ||
| .await? | ||
| .map(|info| info.nonce) | ||
| .unwrap_or(0); | ||
|
|
||
| let gap = stats.top_nonce.saturating_sub(on_chain_nonce); | ||
| if gap > max_nonce_gap { | ||
| dormant_senders.push(*sender); | ||
| } | ||
| } |
There was a problem hiding this comment.
SenderStats tracks top_nonce, and the gap is computed as top_nonce - on_chain_nonce. This catches the described "high-nonce wall" attack, but it also flags any sender whose queue of consecutive transactions happens to be longer than max_nonce_gap. For example: on-chain nonce = 0, pool nonces = 1 … 65. Gap = 65 > 64, but nonce 1 is the very next executable tx — nothing is unreachable. These transactions would be evicted even though they're entirely valid.
The check that correctly distinguishes an unreachable wall from a large-but-valid queue is min_nonce_in_pool - on_chain_nonce > max_nonce_gap. A min-nonce field needs to be added to SenderStats and tracked in the accumulation loop the same way top_nonce is.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/mempool.rs
Line: 567-606
Comment:
**Incorrect nonce used for gap check — flags legitimate senders**
`SenderStats` tracks `top_nonce`, and the gap is computed as `top_nonce - on_chain_nonce`. This catches the described "high-nonce wall" attack, but it also flags any sender whose queue of **consecutive** transactions happens to be longer than `max_nonce_gap`. For example: on-chain nonce = 0, pool nonces = 1 … 65. Gap = 65 > 64, but nonce 1 is the very next executable tx — nothing is unreachable. These transactions would be evicted even though they're entirely valid.
The check that correctly distinguishes an unreachable wall from a large-but-valid queue is `min_nonce_in_pool - on_chain_nonce > max_nonce_gap`. A min-nonce field needs to be added to `SenderStats` and tracked in the accumulation loop the same way `top_nonce` is.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| let mut inner = self.write()?; | ||
| let mut evicted = 0usize; | ||
| for sender in dormant_senders { | ||
| let hashes: Vec<H256> = inner | ||
| .transaction_pool | ||
| .iter() | ||
| .filter_map(|(hash, tx)| { | ||
| if tx.sender() == sender { | ||
| Some(*hash) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .collect(); | ||
| for hash in &hashes { | ||
| inner.remove_transaction_with_lock(hash)?; | ||
| evicted += 1; | ||
| } | ||
| } | ||
| Ok(evicted) |
There was a problem hiding this comment.
The function releases the read lock after taking the snapshot, does async storage lookups (potentially hundreds of milliseconds), then acquires a write lock to remove transactions. Any transaction submitted by a "dormant" sender in that window will be collected in the per-sender hashes vector and removed unconditionally, even though it was fresh and was never observed in the snapshot that triggered the eviction decision.
Re-checking tx.time() < cutoff for each hash inside the write-lock phase before calling remove_transaction_with_lock would close the race without needing to hold the read lock across the awaits.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/mempool.rs
Line: 611-631
Comment:
**TOCTOU: fresh transactions can be evicted after dormancy snapshot**
The function releases the read lock after taking the snapshot, does async storage lookups (potentially hundreds of milliseconds), then acquires a write lock to remove transactions. Any transaction submitted by a "dormant" sender in that window will be collected in the per-sender `hashes` vector and removed unconditionally, even though it was fresh and was never observed in the snapshot that triggered the eviction decision.
Re-checking `tx.time() < cutoff` for each hash inside the write-lock phase before calling `remove_transaction_with_lock` would close the race without needing to hold the read lock across the awaits.
How can I resolve this? If you propose a fix, please make it concise.| /// Format the default mempool lifetime as a clap-compatible string, | ||
| /// reusing the [`DEFAULT_MEMPOOL_LIFETIME`] constant so it stays in sync. | ||
| fn default_mempool_lifetime_str() -> clap::builder::OsStr { | ||
| format!("{}s", DEFAULT_MEMPOOL_LIFETIME.as_secs()).into() | ||
| } | ||
|
|
||
| /// Format the default mempool dormancy as a clap-compatible string, | ||
| /// reusing the [`DEFAULT_DORMANCY`] constant so it stays in sync. | ||
| fn default_mempool_dormancy_str() -> clap::builder::OsStr { | ||
| format!("{}s", DEFAULT_DORMANCY.as_secs()).into() | ||
| } |
There was a problem hiding this comment.
{secs}s, so both --mempool.lifetime and --mempool.dormancy display as 10800s in --help and in CLI.md. Expressing the default as 3h directly matches the format the parser already accepts and is far more readable to operators.
| /// Format the default mempool lifetime as a clap-compatible string, | |
| /// reusing the [`DEFAULT_MEMPOOL_LIFETIME`] constant so it stays in sync. | |
| fn default_mempool_lifetime_str() -> clap::builder::OsStr { | |
| format!("{}s", DEFAULT_MEMPOOL_LIFETIME.as_secs()).into() | |
| } | |
| /// Format the default mempool dormancy as a clap-compatible string, | |
| /// reusing the [`DEFAULT_DORMANCY`] constant so it stays in sync. | |
| fn default_mempool_dormancy_str() -> clap::builder::OsStr { | |
| format!("{}s", DEFAULT_DORMANCY.as_secs()).into() | |
| } | |
| /// Format the default mempool lifetime as a clap-compatible string, | |
| /// reusing the [`DEFAULT_MEMPOOL_LIFETIME`] constant so it stays in sync. | |
| fn default_mempool_lifetime_str() -> clap::builder::OsStr { | |
| let secs = DEFAULT_MEMPOOL_LIFETIME.as_secs(); | |
| let h = secs / 3600; | |
| let m = (secs % 3600) / 60; | |
| let s = secs % 60; | |
| match (h, m, s) { | |
| (h, 0, 0) => format!("{}h", h), | |
| (0, m, 0) => format!("{}m", m), | |
| (0, 0, s) => format!("{}s", s), | |
| (h, m, 0) => format!("{}h{}m", h, m), | |
| _ => format!("{}s", secs), | |
| } | |
| .into() | |
| } | |
| /// Format the default mempool dormancy as a clap-compatible string, | |
| /// reusing the [`DEFAULT_DORMANCY`] constant so it stays in sync. | |
| fn default_mempool_dormancy_str() -> clap::builder::OsStr { | |
| let secs = DEFAULT_DORMANCY.as_secs(); | |
| let h = secs / 3600; | |
| let m = (secs % 3600) / 60; | |
| let s = secs % 60; | |
| match (h, m, s) { | |
| (h, 0, 0) => format!("{}h", h), | |
| (0, m, 0) => format!("{}m", m), | |
| (0, 0, s) => format!("{}s", s), | |
| (h, m, 0) => format!("{}h{}m", h, m), | |
| _ => format!("{}s", secs), | |
| } | |
| .into() | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cmd/ethrex/cli.rs
Line: 69-79
Comment:
The helpers convert the constant to `{secs}s`, so both `--mempool.lifetime` and `--mempool.dormancy` display as `10800s` in `--help` and in `CLI.md`. Expressing the default as `3h` directly matches the format the parser already accepts and is far more readable to operators.
```suggestion
/// Format the default mempool lifetime as a clap-compatible string,
/// reusing the [`DEFAULT_MEMPOOL_LIFETIME`] constant so it stays in sync.
fn default_mempool_lifetime_str() -> clap::builder::OsStr {
let secs = DEFAULT_MEMPOOL_LIFETIME.as_secs();
let h = secs / 3600;
let m = (secs % 3600) / 60;
let s = secs % 60;
match (h, m, s) {
(h, 0, 0) => format!("{}h", h),
(0, m, 0) => format!("{}m", m),
(0, 0, s) => format!("{}s", s),
(h, m, 0) => format!("{}h{}m", h, m),
_ => format!("{}s", secs),
}
.into()
}
/// Format the default mempool dormancy as a clap-compatible string,
/// reusing the [`DEFAULT_DORMANCY`] constant so it stays in sync.
fn default_mempool_dormancy_str() -> clap::builder::OsStr {
let secs = DEFAULT_DORMANCY.as_secs();
let h = secs / 3600;
let m = (secs % 3600) / 60;
let s = secs % 60;
match (h, m, s) {
(h, 0, 0) => format!("{}h", h),
(0, m, 0) => format!("{}m", m),
(0, 0, s) => format!("{}s", s),
(h, m, 0) => format!("{}h{}m", h, m),
_ => format!("{}s", secs),
}
.into()
}
```
How can I resolve this? If you propose a fix, please make it concise.
🤖 Codex Code Review
The diff does not touch EVM execution, gas accounting, trie updates, or RLP logic directly; the risk here is isolated to mempool policy/concurrency. I could not run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
… one-pass scan) Phase 2 review (Copilot + greptile P1) flagged three related issues: - The dormancy gap test used `top_nonce - on_chain_nonce`, which fires on senders with a long but CONTIGUOUS queue (e.g. on_chain=0, pool=1..=65 → gap=65>64) even though every nonce is executable. - TOCTOU between the read-lock snapshot and the write-lock removal let freshly-arrived txs be evicted alongside the stale ones. - The removal phase scanned `transaction_pool.iter()` once per dormant sender — O(pool_size × dormant_senders). Single rewrite addresses all three: - Compare against `lowest_pending_nonce` instead of `top_nonce`. A contiguous queue from on-chain+1 has lowest=on-chain+1 and gap=1, so it doesn't fire the rule. Only queues whose bottom is unreachable (>max_nonce_gap past on-chain) are flagged. - The snapshot now records the exact `(hash, timestamp)` pairs per sender, and the write phase removes only those hashes whose pool entry still has the snapshot timestamp. Newly-arrived (or re-added) txs are safe. - The hash collection is done in the single read-phase pass over `transaction_pool.iter()` — no per-sender re-scans. New regression test `evict_dormant_keeps_long_contiguous_queue` covers the false-positive case. Existing fixtures adjusted to the new lowest-nonce semantics.
Phase 2 review (Copilot): `tokio::time::interval` defaults to `MissedTickBehavior::Burst`. If a sweep iteration ever runs longer than `MEMPOOL_SWEEP_INTERVAL` (many senders, slow storage), the loop fires back-to-back ticks trying to catch up — exactly when the node is already under load. `Skip` instead drops the missed ticks; we'd rather miss a sweep cycle than burst. The "stale-vs-fresh timestamp" doc-comment mismatch flagged in the same review was already addressed when the snapshot was rewritten in the previous commit (`SenderStats.newest_timestamp` now matches the code that reads `tx.time() > entry.newest_timestamp`).
Phase 2 review (greptile P2): `--mempool.lifetime` and `--mempool.dormancy` defaults rendered as `10800s` in `--help` and `docs/CLI.md`, but the parser already accepts `3h` / `30m` / `45s`. Show the human-readable form so operators don't have to convert. Added `duration_default_str(d: Duration)` that picks the largest whole unit (`Nh`, `Nm`, or `Ns`) for the displayed default. Both existing helpers route through it, and `docs/CLI.md` is regenerated with the new defaults.
…cstring Reviewer attribution belongs in PR bodies and commit messages, not in test docstrings. Rephrased the comment to describe the failure mode it guards against without naming the reviewer who flagged it.
| let ttl_micros = ttl.as_micros(); | ||
| let cutoff = now_micros.saturating_sub(ttl_micros); | ||
|
|
||
| let mut inner = self.write()?; |
There was a problem hiding this comment.
evict_stale takes the write lock for the entire scan + remove, so admissions are blocked for the duration of the iteration. evict_dormant (line 580-599) deliberately uses a read-lock snapshot to avoid this same blocking, then re-acquires the write lock only for the actual removals with a per-entry timestamp double-check.
For consistency (and because evict_stale will face the same scan cost for a full pool), consider mirroring the snapshot-then-evict pattern:
let stale_with_ts: Vec<(H256, u128)> = {
let inner = self.read()?;
inner.transaction_pool.iter()
.filter(|(_, tx)| tx.time() < cutoff)
.map(|(h, tx)| (*h, tx.time()))
.collect()
};
// then write-lock + per-entry timestamp double-check, same as evict_dormantFor a 10k-tx pool this drops the worst-case admission-block window from "O(N) under write lock" to "O(N) under read lock + O(K) under write lock" where K is the count actually evicted. Not blocking — the work per tx is already cheap — but it's a free consistency win with the dormancy path.
| /// occupancy. | ||
| pub const DEFAULT_MEMPOOL_LIFETIME: Duration = Duration::from_secs(3 * 60 * 60); | ||
|
|
||
| /// Default maximum gap allowed between a sender's top pending nonce and |
There was a problem hiding this comment.
Doc-vs-impl mismatch: this comment says "between a sender's top pending nonce and their on-chain nonce", but the evict_dormant implementation (mempool.rs:621) explicitly measures the gap at the lowest pending nonce — which the PR description confirms is intentional ("avoids false positives on long contiguous queues").
Swap "top" → "lowest" here so the constant's docstring matches the implementation. Otherwise a future reader tuning --mempool.max-nonce-gap will reason about the wrong queue end.
# Conflicts: # cmd/ethrex/utils.rs # crates/blockchain/blockchain.rs # crates/blockchain/constants.rs # crates/blockchain/mempool.rs
|
| EIP | Bucket | Count |
|---|---|---|
| EIP-7702 | set_code_txs |
24 |
| EIP-7702 | set_code_txs_2 |
15 |
| EIP-7702 | gas |
1 |
| EIP-8037 | state_gas_set_code |
17 |
| EIP-8037 | state_gas_pricing |
1 |
| EIP-8037 | state_gas_sstore |
1 |
| EIP-7928 | block_access_lists_eip7702 |
8 |
| EIP-7928 | block_access_lists |
1 |
| EIP-7778 | gas_accounting |
3 |
| EIP-7708 | transfer_logs |
1 |
| EIP-7976 | refunds |
1 |
| EIP-1344 | chainid (Amsterdam fork-transition fixture) |
1 |
| Total | 74 |
Re-enable once we either:
- (a) bump fixtures to a snobal-devnet-7 release that locks in the new
accounting; or - (b) revert the bal-devnet-7-prep subtraction for bal-devnet-6
compatibility.
Full test list (74)
EIP-7702 — for_amsterdam/prague/eip7702_set_code_tx/set_code_txs/
delegation_clearingdelegation_clearing_and_setdelegation_clearing_failing_txdelegation_clearing_tx_toeoa_tx_after_set_codeext_code_on_chain_delegating_set_codeext_code_on_self_delegating_set_codeext_code_on_self_set_codeext_code_on_set_codemany_delegationsnonce_overflow_after_first_authorizationnonce_validityreset_codeself_code_on_set_codeself_sponsored_set_codeset_code_multiple_valid_authorization_tuples_same_signer_increasing_nonceset_code_multiple_valid_authorization_tuples_same_signer_increasing_nonce_self_sponsoredset_code_to_logset_code_to_non_empty_storage_non_zero_nonceset_code_to_self_destructset_code_to_self_destructing_account_deployed_in_same_txset_code_to_sstoreset_code_to_sstore_then_sloadset_code_to_system_contract
EIP-7702 — for_amsterdam/prague/eip7702_set_code_tx/set_code_txs_2/
call_pointer_to_created_from_create_after_oog_call_againcall_to_precompile_in_pointer_contextcontract_storage_to_pointer_with_storagedelegation_replacement_call_previous_contractdouble_authpointer_measurementspointer_normalpointer_reentrypointer_resets_an_empty_code_account_with_storagepointer_revertspointer_to_pointerpointer_to_precompilepointer_to_staticpointer_to_static_reentrystatic_to_pointer
EIP-7702 — for_amsterdam/prague/eip7702_set_code_tx/gas/
account_warming
EIP-8037 — for_amsterdam/amsterdam/eip8037_state_creation_gas_cost_increase/state_gas_set_code/
auth_refund_block_gas_accountingauth_refund_bypasses_one_fifth_capauth_with_calldata_and_access_listauth_with_multiple_sstoresauthorization_exact_state_gas_boundaryauthorization_to_precompile_addressauthorization_with_sstoreduplicate_signer_authorizationsexisting_account_auth_header_gas_used_uses_worst_caseexisting_account_refundexisting_account_refund_enables_sstoreexisting_auth_with_reverted_execution_preserves_intrinsicmany_authorizations_state_gasmixed_auths_header_gas_used_uses_worst_casemixed_new_and_existing_authsmixed_valid_and_invalid_authsmulti_tx_block_auth_refund_and_sstore
EIP-8037 — state_gas_pricing/
auth_state_gas_scales_with_cpsb
EIP-8037 — state_gas_sstore/
sstore_state_gas_all_tx_types
EIP-7928 — for_amsterdam/amsterdam/eip7928_block_level_access_lists/block_access_lists_eip7702/
bal_7702_delegation_clearbal_7702_delegation_createbal_7702_delegation_updatebal_7702_double_auth_resetbal_7702_double_auth_swapbal_7702_null_address_delegation_no_code_changebal_selfdestruct_to_7702_delegationbal_withdrawal_to_7702_delegation
EIP-7928 — block_access_lists/
bal_all_transaction_types
EIP-7778 — for_amsterdam/amsterdam/eip7778_block_gas_accounting_without_refunds/gas_accounting/
multiple_refund_types_in_one_txsimple_gas_accountingvarying_calldata_costs
EIP-7708 — for_amsterdam/amsterdam/eip7708_eth_transfer_logs/transfer_logs/
transfer_with_all_tx_types
EIP-7976 — for_amsterdam/amsterdam/eip7976_increase_calldata_floor_cost/refunds/
gas_refunds_from_data_floor
EIP-1344 — for_amsterdam/istanbul/eip1344_chainid/chainid/
chainid(Amsterdam fork-transition fixture)
Rust 1.91's clippy::manual_is_multiple_of lint rejects `secs % N == 0` when a built-in method exists. Swaps the two checks in duration_default_str.
Motivation
Two failure modes accumulate dead state in the mempool over time:
Add a low-frequency background sweep that evicts both classes.
Description
Blockchain::spawn_mempool_sweep. Holds only aWeak<Blockchain>so the task tears itself down on shutdown.MissedTickBehavior::Skipso a slow iteration doesn't trigger a burst of catch-up ticks.Mempool::evict_stale(ttl)— drops txs older thanttlbased on the existingMempoolTransaction.timestamp.Mempool::evict_dormant(store, max_nonce_gap, dormancy)— flags senders whose lowest pending nonce exceeds the on-chain nonce by more thanmax_nonce_gapAND every queue entry is older thandormancy. The lowest-nonce gap (rather than top-nonce - on-chain) avoids false positives on long contiguous queues. The eviction is atomic with respect to fresh arrivals: the read-lock snapshot records exact(hash, timestamp)pairs and the write-lock removal step only drops hashes whose pool entry STILL has the snapshot timestamp — txs that arrived between snapshot and removal are safe.--mempool.lifetime(default 3h),--mempool.max-nonce-gap(default 64),--mempool.dormancy(default 3h). Duration defaults render as3hin--help, not raw seconds.Behavioral change
Pool occupancy drops over time as inactive senders' txs age out. Sweep is rate-limited (1-minute tick) and bounded (per-sender scan via
txs_by_sender_nonceindex, one pass).Deferred
End-to-end test of the spawned sweep loop (timing-dependent). The eviction methods themselves have direct unit-test coverage including the contiguous-queue regression case.