perf(l1): replace FIFO mempool eviction with min-heap keyed by tip#6607
perf(l1): replace FIFO mempool eviction with min-heap keyed by tip#6607ilitteri wants to merge 4 commits into
Conversation
Replace the `VecDeque<H256>` FIFO order queue in `MempoolInner` with a `BinaryHeap<Reverse<(u64, H256)>>` min-heap keyed by `Transaction::gas_tip_cap()` saturated to `u64`. When the pool is full we now evict the *lowest-tip* transaction instead of the oldest, so under fee pressure high-fee newcomers are admitted at the expense of low-fee residents. Lazy deletion: normal removals leave heap entries as tombstones; the eviction loop skips popped hashes that are no longer in `transaction_pool`. The top-of-`add_transaction` prune rebuilds the heap from the live pool once the heap grows past `MEMPOOL_PRUNE_THRESHOLD_NUM / MEMPOOL_PRUNE_THRESHOLD_DEN` (1.5x) `max_mempool_size`, dropping accumulated tombstones in O(n).
🤖 Kimi Code ReviewThe PR correctly transitions mempool eviction from FIFO to a tip-based priority scheme using a lazy-deletion min-heap. The logic is sound and the test coverage is comprehensive. Minor Defensive Programming Suggestions:
Code Quality Observations:
Verification Questions:
The lazy deletion pattern is well-implemented and the tests verify tombstone handling, heap rebuild thresholds, and correct eviction priority. No critical issues identified. 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
This PR changes the mempool’s behavior under capacity pressure by evicting the lowest-tip transaction instead of the oldest, improving fee-based prioritization during congestion.
Changes:
- Replaces FIFO eviction tracking (
VecDeque) with a min-heap (BinaryHeap<Reverse<(u64, H256)>>) keyed byTransaction::gas_tip_cap()projected intou64. - Implements lazy deletion for heap entries and adds a periodic heap rebuild when tombstones accumulate past a named threshold.
- Adds unit tests covering lowest-tip eviction, newcomer retention, lazy deletion behavior, and heap rebuild pruning.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| inner.txs_order.retain(|tx| txpool.contains_key(tx)); | ||
| inner.transaction_pool = txpool; | ||
| // Rebuild the eviction heap if tombstones have accumulated past the | ||
| // configured threshold (heap-size > MEMPOOL_PRUNE_THRESHOLD_FACTOR * max_mempool_size). |
| /// Min-heap (via `Reverse`) of `(effective tip, hash)` used to pick the | ||
| /// lowest-tip transaction to evict when the mempool is full. Entries are |
Greptile SummaryThis PR replaces the FIFO
Confidence Score: 3/5The eviction logic has a correctness gap that can actively degrade pool quality under fee pressure — the scenario this PR was written to fix. The tip-comparison check before eviction is missing: a sender submitting a zero-tip transaction into a full pool of high-tip transactions will evict the current minimum-tip holder and get admitted, leaving the pool in a worse state than before. crates/blockchain/mempool.rs — specifically the add_transaction / evict_lowest_tip_transaction interaction around lines 195–197.
|
| Filename | Overview |
|---|---|
| crates/blockchain/mempool.rs | Replaces FIFO eviction with a lazy min-heap keyed by tip; contains a correctness gap where a low-tip newcomer can unconditionally evict a higher-tip existing transaction because the incoming tip is never compared against the heap minimum before eviction proceeds. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[add_transaction called] --> B{heap_size > prune_threshold?}
B -- Yes --> C[rebuild_tip_heap from transaction_pool]
C --> D
B -- No --> D{pool_size >= max_mempool_size?}
D -- Yes --> E[evict_lowest_tip_transaction]
E --> F{heap.pop?}
F -- None --> G[warn: pool full break]
F -- Some hash --> H{hash in transaction_pool?}
H -- No tombstone --> F
H -- Yes --> I[remove_transaction_with_lock]
I --> J{pool_size still >= max?}
J -- Yes --> F
J -- No --> K
G --> K
D -- No --> K[push tip/hash onto heap, insert tx into pool]
K --> L[notify tx_added waiters]
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
crates/blockchain/mempool.rs:195-197
**Missing tip comparison before eviction**
`evict_lowest_tip_transaction()` unconditionally evicts the current minimum-tip entry from the pool without first checking whether the newcomer's tip is actually higher. If a sender submits a transaction with `tip=1` while the pool is full of `tip=100` transactions, the tip-100 entry is removed and the tip-1 newcomer is admitted — the opposite of the stated goal. The newcomer's tip must be compared against the live heap minimum before any eviction takes place; if the newcomer would be the new minimum, it should be rejected (or the eviction skipped) rather than displacing a better-priced transaction.
### Issue 2 of 2
crates/blockchain/mempool.rs:624-637
**`make_tx` generates a random sender per call, masking same-sender nonce conflicts**
Because each call to `make_tx` creates a fresh `Address::random()` sender, every transaction is from a different address and the `txs_by_sender_nonce` map never sees a collision. The tests therefore do not cover the replacement flow (`find_tx_to_replace` → remove old → add new), where the old entry becomes a heap tombstone while the sender-nonce mapping still points to the new hash. A test that reuses the same sender with an increasing nonce (or a tip-bump replacement) would catch any interaction between `txs_by_sender_nonce` and the lazy-deletion heap.
Reviews (1): Last reviewed commit: "perf(l1): replace FIFO mempool eviction ..." | Re-trigger Greptile
| if inner.transaction_pool.len() >= inner.max_mempool_size { | ||
| inner.remove_oldest_transaction()?; | ||
| inner.evict_lowest_tip_transaction()?; | ||
| } |
There was a problem hiding this comment.
Missing tip comparison before eviction
evict_lowest_tip_transaction() unconditionally evicts the current minimum-tip entry from the pool without first checking whether the newcomer's tip is actually higher. If a sender submits a transaction with tip=1 while the pool is full of tip=100 transactions, the tip-100 entry is removed and the tip-1 newcomer is admitted — the opposite of the stated goal. The newcomer's tip must be compared against the live heap minimum before any eviction takes place; if the newcomer would be the new minimum, it should be rejected (or the eviction skipped) rather than displacing a better-priced transaction.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/mempool.rs
Line: 195-197
Comment:
**Missing tip comparison before eviction**
`evict_lowest_tip_transaction()` unconditionally evicts the current minimum-tip entry from the pool without first checking whether the newcomer's tip is actually higher. If a sender submits a transaction with `tip=1` while the pool is full of `tip=100` transactions, the tip-100 entry is removed and the tip-1 newcomer is admitted — the opposite of the stated goal. The newcomer's tip must be compared against the live heap minimum before any eviction takes place; if the newcomer would be the new minimum, it should be rejected (or the eviction skipped) rather than displacing a better-priced transaction.
How can I resolve this? If you propose a fix, please make it concise.| let inner = EIP1559Transaction { | ||
| nonce, | ||
| max_priority_fee_per_gas: tip, | ||
| // Keep `max_fee_per_gas >= max_priority_fee_per_gas` so the tx is | ||
| // structurally valid even though we bypass mempool admission here. | ||
| max_fee_per_gas: tip, | ||
| gas_limit: 21_000, | ||
| to: TxKind::Call(Address::from_low_u64_be(1)), | ||
| ..Default::default() | ||
| }; | ||
| let tx = Transaction::EIP1559Transaction(inner); | ||
| let sender = Address::random(); | ||
| let hash = H256::random(); | ||
| (hash, sender, MempoolTransaction::new(tx, sender)) |
There was a problem hiding this comment.
make_tx generates a random sender per call, masking same-sender nonce conflicts
Because each call to make_tx creates a fresh Address::random() sender, every transaction is from a different address and the txs_by_sender_nonce map never sees a collision. The tests therefore do not cover the replacement flow (find_tx_to_replace → remove old → add new), where the old entry becomes a heap tombstone while the sender-nonce mapping still points to the new hash. A test that reuses the same sender with an increasing nonce (or a tip-bump replacement) would catch any interaction between txs_by_sender_nonce and the lazy-deletion heap.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/mempool.rs
Line: 624-637
Comment:
**`make_tx` generates a random sender per call, masking same-sender nonce conflicts**
Because each call to `make_tx` creates a fresh `Address::random()` sender, every transaction is from a different address and the `txs_by_sender_nonce` map never sees a collision. The tests therefore do not cover the replacement flow (`find_tx_to_replace` → remove old → add new), where the old entry becomes a heap tombstone while the sender-nonce mapping still points to the new hash. A test that reuses the same sender with an increasing nonce (or a tip-bump replacement) would catch any interaction between `txs_by_sender_nonce` and the lazy-deletion heap.
How can I resolve this? If you propose a fix, please make it concise.
🤖 Claude Code ReviewNow I have the full picture. Let me write the review. Review:
|
🤖 Codex Code Review
The lazy-deletion/rebuild tests are useful, but they miss Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Phase 2 review (Copilot): - The `txs_by_tip` doc described entries as "(effective tip, hash)" but the key is the raw `gas_tip_cap` (not base-fee-adjusted). - The `tip_key` doc said large tips "order to the top of the heap"; the `BinaryHeap<Reverse<...>>` is a min-heap so the largest tips actually rank highest and are evicted last. - The prune-threshold comment referenced a stale `MEMPOOL_PRUNE_THRESHOLD_FACTOR` identifier; the code uses the `NUM / DEN` integer ratio (3/2 = 1.5x). Wording updates only; no behavior change.
Phase 2 review (greptile P1): when the pool was full,
`evict_lowest_tip_transaction` unconditionally evicted the current
minimum-tip entry without checking whether the incoming tx's tip was
actually higher. Submitting a tip=1 tx to a pool full of tip=100
entries would evict a tip=100 and admit the tip=1 — the opposite of
the stated goal of tip-based eviction.
`add_transaction` now peeks the live minimum tip (skipping tombstones
via a new `live_min_tip` helper) before evicting. If the incoming tip
is less than or equal to the heap minimum, the incoming tx is rejected
with the new `MempoolError::PoolFullAndUnderpriced { incoming_tip,
min_pool_tip }`. Equal tips are rejected too — admitting at-tie would
needlessly churn the pool without economic benefit.
`add_transaction` return type changed from `Result<(), StoreError>` to
`Result<(), MempoolError>` to surface the new variant; existing
`?`-using callers continue to work because `StoreError` already has a
`From` impl into `MempoolError`.
Three new tests:
- `underpriced_newcomer_is_rejected_when_pool_is_full`
- `equal_tip_newcomer_is_rejected_when_pool_is_full`
- `same_sender_replacement_clears_heap_tombstone` (addresses the
greptile P2 test-coverage gap on heap tombstone replacement flow)
Benchmark Block Execution Results Comparison Against Main
|
|
| 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)
Motivation
When the mempool is full (
transaction_pool.len() >= max_mempool_size), ethrex currently drops the oldest transaction (VecDeque<H256>FIFO). That is wrong under fee pressure: low-fee transactions cling on while high-fee newcomers get rejected. The block builder should reward the highest-tip senders, not the earliest arrivals.Description
Replace the FIFO
txs_order: VecDeque<H256>inMempoolInnerwith a min-heaptxs_by_tip: BinaryHeap<Reverse<(u64, H256)>>keyed byTransaction::gas_tip_cap()saturated tou64(astronomically-large tips rank highest and are evicted last by the min-heap).evict_lowest_tip_transactionreplacesremove_oldest_transactionand pops from the min-heap untiltransaction_poolis under capacity.add_transactionpeeks the live heap minimum via a newlive_min_tiphelper (skipping tombstones) before evicting. If the incoming tx's tip is<=the heap minimum, the incoming tx is rejected with the newMempoolError::PoolFullAndUnderpriced { incoming_tip, min_pool_tip }— admitting a cheaper tx by evicting a more expensive one would invert tip-based eviction.add_transaction's return type changes fromResult<(), StoreError>toResult<(), MempoolError>so the new variant can surface.remove_transaction_with_lock) do not touch the heap. The eviction loop checkstransaction_pool.contains_keyon each popped entry and skips tombstones.add_transactionwhen the heap exceedsMEMPOOL_PRUNE_THRESHOLD_NUM / MEMPOOL_PRUNE_THRESHOLD_DEN(3/2 = 1.5x)max_mempool_size. Both numerator and denominator are named constants — no magic numbers.Behavioral change