fix(mempool): a memory leak in the PriorityMempool
#384
Conversation
📝 WalkthroughWalkthroughThe changes update how a MEV lane is instantiated and how mempools are managed. In Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Caller
participant MEV as NewMEVLane
participant Mempool as NewMempool
participant BaseLane as NewBaseLane
participant Proposal as NewProposalHandler
Client->>MEV: Call NewMEVLane(cfg, factory, matchHandler)
MEV->>Mempool: Initialize mempool with config & factory
Mempool-->>MEV: Return mempool instance
MEV->>BaseLane: Create base lane with config & options
BaseLane-->>MEV: Return base lane
MEV->>Proposal: Instantiate proposal handler for base lane
Proposal-->>MEV: Return proposal handler
MEV-->>Client: Return new MEVLane object
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/lanes/mempool.go (1)
65-71: Validate local vs external function usage.
Switching fromblockbase.NewPriorityMempoolto a locally definedNewPriorityMempoolmight introduce differences in configuration or behavior. Please ensure that all parameters (e.g.,TxPriority,MaxTx) align with the intended mempool constraints and that you have sufficient test coverage for this new implementation.app/lanes/priority_nonce.go (1)
331-353: Reordering logic complexity.
ThereorderPriorityTiesmethod recalculates weights for tied priorities. This approach, while functional, has an O(n) scan for reordering. Evaluate whether this might become a performance bottleneck for large mempool sizes and consider more efficient data structures or strategies if necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (4)
app/blocksdk.go(1 hunks)app/lanes/mempool.go(1 hunks)app/lanes/mev.go(2 hunks)app/lanes/priority_nonce.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
app/lanes/mempool.go (1)
app/lanes/priority_nonce.go (1)
NewPriorityMempool(115-131)
app/blocksdk.go (1)
app/lanes/mev.go (1)
NewMEVLane(50-88)
app/lanes/mev.go (1)
app/lanes/mempool.go (1)
NewMempool(51-77)
app/lanes/priority_nonce.go (1)
app/lanes/mempool.go (1)
Mempool(29-47)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run test and upload codecov
- GitHub Check: golangci-lint
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
app/blocksdk.go (1)
52-59: Check new MEV lane integration.
Replacingmevlane.NewMEVLanewithapplanes.NewMEVLaneshifts the creation logic for the MEV lane. Confirm that the new lane’s configuration (particularlyMaxBlockSpaceandMaxTxs) still satisfies your performance and capacity requirements, and verify no unexpected behaviors occur in downstream code that consumes the MEV lane.app/lanes/priority_nonce.go (2)
19-19: Thread-safety of the LRU cache.
You are usinglrucache.New[...]fromgithub.com/hashicorp/golang-lru/v2at line 19. Ensure that the chosen LRU implementation is safe under concurrent access or that you protect it with synchronization if the mempool can be accessed concurrently (e.g., multiple goroutines appending and removing transactions).
437-462: Validate the “IsEmpty” integrity check.
TheIsEmptyfunction thoroughly checks multiple data structures for emptiness. If concurrency is possible, these checks can be invalidated by concurrent inserts or removes. Ensure that calls toIsEmptyhappen under a safe synchronization mechanism or in test environments only.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/lanes/priority_mempool.go (3)
185-188: Clarify intended ordering in the per-sender skiplist.The comparator
skiplist.LessThanFunc(func(a, b any) int { return skiplist.Uint64.Compare(b.(txMeta[C]).nonce, a.(txMeta[C]).nonce) })reverses the usual ascending order for nonce. Confirm that starting at the front is meant to retrieve the highest nonce first. If ascending order is needed, switch the arguments to compareawithb.
361-378: Consider optimizingsenderWeightfor large sender chains.The
senderWeightfunction scans onward through the sender’s skiplist until it finds a different priority. For senders with numerous same-priority transactions, this may be inefficient. Consider tracking the next distinct priority in a more direct manner to avoid scanning the entire chain.
439-464: Account for type assertions inIsEmpty.
mempool.(*PriorityNonceMempool[C])may panic ifmempoolis not actually a*PriorityNonceMempool[C]. Either ensure that this function is only called with aPriorityNonceMempool, or add defensive checks before casting to avoid runtime errors in unexpected usage scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
app/lanes/priority_mempool.go(1 hunks)
🔇 Additional comments (1)
app/lanes/priority_mempool.go (1)
163-167: Re-check negative MaxTx logic.When
mp.cfg.MaxTxis less than zero, the current code allows unlimited transactions by returning early. Verify whether unlimited acceptance is truly intended or if this could lead to unbounded mempool growth. If this scenario is unintended, guard against negative values or throw an error.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #384 +/- ##
==========================================
+ Coverage 39.86% 40.03% +0.16%
==========================================
Files 293 294 +1
Lines 27210 27508 +298
==========================================
+ Hits 10847 11012 +165
- Misses 14687 14796 +109
- Partials 1676 1700 +24
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/lanes/priority_mempool.go (2)
121-131: Consider pruning zero-count priorities to reclaim map space.Inside the eviction callback, each transaction’s priority is decremented in
mp.priorityCounts, but the ma 10000 p entry remains even if the count hits zero. Removing zero-count entries would help keep memory usage low for ephemeral priorities.--- a/app/lanes/priority_mempool.go +++ b/app/lanes/priority_mempool.go @@ -127,6 +127,10 @@ senderIndices, err := lrucache.NewWithEvict(MaxMempoolSenderIndexSize, func(key priorityCounts[k.priority]-- delete(scores, txMeta[C]{nonce: k.nonce, sender: k.sender}) + if priorityCounts[k.priority] == 0 { + delete(priorityCounts, k.priority) + } + cursor = cursor.Next()
1-478: Enhance testing coverage.It’s unclear if there is an associated test suite for this mempool implementation. Testing edge cases—such as LRU evictions, operational concurrency (if applicable), and priority tie reordering—would help ensure correctness and maintain reliability over time.
Do you need help drafting unit tests for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
app/lanes/priority_mempool.go(1 hunks)
🔇 Additional comments (3)
app/lanes/priority_mempool.go (3)
34-34: MakeMaxMempoolSenderIndexSizeconfigurable.Hard-coding
MaxMempoolSenderIndexSizeto 1000 could inadvertently limit valid use cases or lead to poor memory usage in scenarios requiring a different capacity. Using a dynamic limit from config or environment variables can offer more flexibility.
400-432: Overall removal logic seems solid!The
Removefunction is straightforward and properly updates the priority index, sender index, and score map. This helps keep the mempool consistent.
452-477: “IsEmpty” check is well-structured.Using multiple checks for each index, map, and skiplist ensures the mempool has truly zero transactions remaining. This robust approach lowers the risk of hidden data.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/lanes/mempool_test.go (2)
143-143: Consider adding underscore to unused loop variableThe Go convention for iterating a specific number of times is typically
for _ := range Norfor i := range Nwhen the index is used.- for range 2000 { + for _ := range 2000 {
161-171: Check for duplicate transactionsThe test verifies that the transactions can be retrieved, but it doesn't check for potential duplicates in the selected transactions. Since we're testing an eviction mechanism, it might be worth verifying that all returned transactions are unique.
You could add a simple check using a map to ensure all transactions are unique:
numTxs := 0 txes := make([]sdk.Tx, 2000) + txMap := make(map[string]bool) for iter := mempool.Select(ctx, nil); iter != nil; iter = iter.Next() { tx := iter.Tx() txes[numTxs] = tx signers, err := signerExtractor.GetSigners(tx) require.NoError(t, err) require.Equal(t, 1, len(signers)) + // Convert tx to bytes to check for uniqueness + txBytes, err := txEncoder(tx) + require.NoError(t, err) + txKey := string(txBytes) + require.False(t, txMap[txKey], "Found duplicate transaction") + txMap[txKey] = true numTxs++ }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
app/lanes/mempool_test.go(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
app/lanes/mempool_test.go (1)
app/lanes/mempool.go (1)
NewMempool(51-77)
🔇 Additional comments (2)
app/lanes/mempool_test.go (2)
102-117: Good utility function for transaction creationThis is a well-structured utility function that extracts the common transaction creation logic for tests. It helps reduce code duplication and makes the test setup cleaner.
173-178: Good test coverage for transaction removalThe test properly validates that all transactions can be removed from the mempool, ensuring that the eviction mechanism doesn't interfere with basic mempool operations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/lanes/mempool_test.go (1)
142-156: Consider making the LRU eviction behavior more explicit in commentsThe comments partially explain the relationship between creating 4000 transactions and expecting 2000 to remain, but could be more explicit about the eviction policy.
-// total create 4000 txs, but we have lrucache to limit the number of sender indices to 1000 -// so we should evict some txs from the mempool +// Total: creating 4000 txs (2 txs per sender × 2000 senders), but the LRU cache limits unique senders to 1000. +// When a sender is evicted from the cache, all their transactions are removed from the mempool. +// Therefore, we expect only 2000 txs to remain (2 txs per sender × 1000 most recent senders).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
⛔ Files ignored due to path filters (1)
go.modis excluded by!**/*.mod
📒 Files selected for processing (8)
app/lanes/mempool_test.go(2 hunks)app/upgrade.go(1 hunks)crypto/keyring/options.go(1 hunks)x/dynamic-fee/abci_test.go(1 hunks)x/dynamic-fee/ante/fee.go(1 hunks)x/dynamic-fee/keeper/common_test.go(1 hunks)x/dynamic-fee/keeper/gas_price_test.go(1 hunks)x/dynamic-fee/keeper/keeper.go(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- app/upgrade.go
- x/dynamic-fee/keeper/keeper.go
- x/dynamic-fee/keeper/gas_price_test.go
- x/dynamic-fee/abci_test.go
- crypto/keyring/options.go
- x/dynamic-fee/keeper/common_test.go
- x/dynamic-fee/ante/fee.go
🧰 Additional context used
🧬 Code Definitions (1)
app/lanes/mempool_test.go (2)
x/dynamic-fee/keeper/common_test.go (1)
MakeEncodingConfig(151-175)app/lanes/mempool.go (1)
NewMempool(51-77)
🔇 Additional comments (4)
app/lanes/mempool_test.go (4)
103-118: Well-designed helper function for test transaction creationThe
createTxfunction is a good refactoring that extracts the common code for creating test transactions, improving code reusability and readability in test functions.
120-141: Appropriate test setup for LRU cache validationThe test properly configures a mempool with parameters suitable for testing the eviction behavior: sufficient block size, gas limits, and transaction capacity to isolate the LRU cache behavior.
158-172: Test correctly verifies LRU eviction behaviorThe test properly verifies that exactly 2000 transactions remain in the mempool after 4000 insertions, confirming that the LRU cache is working as expected by evicting older senders.
174-183: Well-structured cleanup and verificationThe test thoroughly verifies that all transactions can be removed and that the mempool is empty afterward, providing good test coverage for the transaction removal functionality.
| } | ||
|
|
||
| mp.priorityIndex.Remove(tk) | ||
| senderTxs.Remove(tk) |
There was a problem hiding this comment.
Rather than using LRU, can we just keep the existing structure, and simply remove sender's skiplist when the list becomes empty here?
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/lanes/mempool_test.go (1)
120-188:⚠️ Potential issueContradictory test expectations regarding LRU eviction.
Lines 142–143 comment that an LRU should evict some transactions, but line 158 checks for 4000 total transactions instead of 2000. This discrepancy suggests the test either isn’t covering the intended eviction behavior or the comment is incorrect.Below is one potential fix if you expect half the transactions (those from the excess 1000 senders) to be evicted:
- // it must report 4000 txs - require.Equal(t, 4000, mempool.CountTx()) + // it must report 2000 txs, since we've evicted half the senders + require.Equal(t, 2000, mempool.CountTx())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
⛔ Files ignored due to path filters (1)
go.modis excluded by!**/*.mod
📒 Files selected for processing (3)
app/lanes/mempool.go(2 hunks)app/lanes/mempool_test.go(2 hunks)app/lanes/priority_mempool.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/lanes/mempool.go
🧰 Additional context used
🧬 Code Definitions (2)
app/lanes/mempool_test.go (2)
x/dynamic-fee/keeper/common_test.go (1)
MakeEncodingConfig(151-175)app/lanes/mempool.go (1)
NewMempool(51-77)
app/lanes/priority_mempool.go (1)
app/lanes/mempool.go (1)
Mempool(29-47)
🔇 Additional comments (5)
app/lanes/mempool_test.go (3)
8-9: Use of CometBFT import is consistent.
No concerns with the new import path for CometBFT, as it's aligned with the newer CometBFT rebranding.
13-13: Adequate cryptotypes import.
This import correctly enables usage of private key types for signing and aligns with usage in the new helper function.
103-118: New helper function for transaction creation is clearly structured.
The function’s step-by-step approach to gas, memo, messages, and signatures is coherent, making it straightforward to reuse.app/lanes/priority_mempool.go (2)
1-14: File header and disclaimers acknowledged.
Copying and modifying the Cosmos-SDK priority nonce mempool is documented. No immediate concerns.
154-237: Insertion logic is sound, but capacity checks only limit total tx count.
The function properly handles overall mempool capacity (MaxTx), priority changes, and replacement-logic checks. However, there’s no code to evict older or inactive senders when the sender index grows beyond a certain threshold, potentially undermining the intended LRU behavior.Could you confirm if a separate mechanism will handle sender expiry or if the design expects the skip list buffer alone to suffice?
PriorityMempool
Description
To prevent indefinite memory increase due to senderIndices of mempool, introduce skiplist buffer to prevent it.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...