feat: txpool pending limit #1107
Conversation
WalkthroughThis pull request introduces a new configuration parameter Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (2)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/tx_pool.go (1)
447-454: Consider adding metrics for pending transaction eviction.The implementation of lifetime cleanup for pending transactions is good, but would benefit from metrics tracking.
if time.Since(pool.beats[addr]) > pool.config.Lifetime { list := pool.pending[addr].Flatten() for _, tx := range list { log.Trace("Evicting transaction due to timeout", "account", addr.Hex(), "hash", tx.Hash().Hex(), "lifetime sec", time.Since(pool.beats[addr]).Seconds(), "lifetime limit sec", pool.config.Lifetime.Seconds()) pool.removeTx(tx.Hash(), true) } - queuedEvictionMeter.Mark(int64(len(list))) + pendingEvictionMeter.Mark(int64(len(list))) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/geth/main.go(1 hunks)cmd/geth/usage.go(1 hunks)cmd/utils/flags.go(2 hunks)core/tx_pool.go(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: check
🔇 Additional comments (7)
core/tx_pool.go (3)
181-182: LGTM! Clear and well-documented configuration field.The new
AccountPendingLimitfield inTxPoolConfigis appropriately placed and documented.
976-980: LGTM! Proper limit enforcement during transaction promotion.The implementation correctly checks the account's pending transaction count against the limit before promotion.
1598-1620: LGTM! Comprehensive implementation of pending transaction truncation.The implementation properly enforces the account pending limit during periodic cleanup, updates nonces correctly, and maintains proper metrics.
cmd/geth/usage.go (1)
111-111: LGTM! Flag properly added to transaction pool section.The new flag is correctly placed in the transaction pool flags group.
cmd/geth/main.go (1)
93-93: LGTM! Flag properly added to node flags.The new flag is correctly added to the node configuration flags.
cmd/utils/flags.go (2)
405-409: LGTM! Clear and well-documented flag definition.The flag is properly defined with clear usage description and correct default value.
1527-1529: LGTM! Proper configuration handling.The flag value is correctly applied to the transaction pool configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/tx_pool.go (1)
1608-1630: Consider optimizing the truncation logic.While the implementation correctly enforces the account pending limit, there are a few suggestions for improvement:
- The truncation could be more efficient by calculating the exact number of transactions to remove instead of iterating one by one.
- Consider adding metrics to track the number of transactions dropped due to account limits.
Here's a suggested optimization:
func (pool *TxPool) truncatePending() { for addr, list := range pool.pending { - if list.Len() > int(pool.config.AccountPendingLimit) { - caps := list.Cap(int(pool.config.AccountPendingLimit)) + excess := list.Len() - int(pool.config.AccountPendingLimit) + if excess > 0 { + caps := list.Cap(list.Len() - excess) for _, tx := range caps { hash := tx.Hash() pool.all.Remove(hash) pool.calculateTxsLifecycle(types.Transactions{tx}, time.Now()) pool.pendingNonces.setIfLower(addr, tx.Nonce()) log.Trace("Removed pending transaction to comply with hard limit", "hash", hash.Hex()) } pool.priced.Removed(len(caps)) pendingGauge.Dec(int64(len(caps))) + // Add new metric + pendingAccountLimitMeter.Mark(int64(len(caps))) if pool.locals.contains(addr) { localGauge.Dec(int64(len(caps))) } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/tx_pool.go(8 hunks)core/tx_pool_test.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: check
🔇 Additional comments (6)
core/tx_pool_test.go (2)
1100-1107: LGTM! Test case correctly verifies pending transaction handling for non-local transactions.The test case properly validates that when
nolocalsis true, pending transactions are correctly evicted, ensuring the transaction pool's behavior aligns with the expected functionality.
1167-1167: Implement the TODO test case for global pending limits.The TODO comment indicates a missing test case for transaction pool's global pending limits. This test is important to verify the correct enforcement of global transaction limits across multiple accounts.
Would you like me to help implement the test case for global pending limits? The test should verify:
- Transactions exceeding global limits are properly dropped
- The selection of which transactions to drop is fair across accounts
- Local transactions are preserved when appropriate
core/tx_pool.go (4)
182-183: LGTM! Well-documented configuration parameter addition.The new
AccountPendingLimitparameter is appropriately placed in the configuration struct with a clear purpose - limiting the number of executable transactions per account.
201-202: LGTM! Reasonable default value.The default value of 1024 for
AccountPendingLimitis a reasonable choice that balances resource usage with transaction throughput.
238-241: LGTM! Proper configuration validation.The sanitization logic correctly ensures the
AccountPendingLimitis at least 1, maintaining the pool's operational integrity.
986-990: LGTM! Early limit check in promoteTx.The implementation efficiently checks the account's pending transaction limit before attempting promotion, preventing unnecessary processing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/tx_pool.go (1)
1612-1634: Consider optimizing the truncation logic.The truncation logic could be improved in two ways:
- Consider transaction priority/gas price when selecting which transactions to drop
- Add a warning log when transactions are dropped due to the limit to help with debugging
Apply this diff to add warning logs:
if list.Len() > int(pool.config.AccountPendingLimit) { caps := list.Cap(int(pool.config.AccountPendingLimit)) for _, tx := range caps { hash := tx.Hash() pool.all.Remove(hash) pool.calculateTxsLifecycle(types.Transactions{tx}, time.Now()) + log.Warn("Dropping transaction due to account pending limit", + "account", addr, + "hash", hash, + "limit", pool.config.AccountPendingLimit) pool.pendingNonces.setIfLower(addr, tx.Nonce()) - log.Trace("Removed pending transaction to comply with hard limit", "hash", hash.Hex()) }core/tx_pool_test.go (1)
1212-1321: LGTM! The test implementation is thorough and well-structured.The test effectively verifies the account pending limit feature by checking:
- Basic limit enforcement
- Nonce gap handling
- Global transaction counts
Consider adding test cases for:
- Edge cases (limit=0, limit=1)
- Transaction removal behavior
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/tx_pool.go(8 hunks)core/tx_pool_test.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: check
🔇 Additional comments (7)
core/tx_pool.go (5)
182-183: LGTM: Configuration parameter addition looks good.The new
AccountPendingLimitconfiguration parameter is well-placed in theTxPoolConfigstruct with a reasonable default value of 1024 transactions.Also applies to: 201-202
238-241: LGTM: Proper configuration validation.The sanitization logic correctly validates the
AccountPendingLimitparameter and falls back to the default value if invalid.
107-107: LGTM: Added eviction metric.The
pendingEvictionMetermetric will help monitor transactions dropped due to lifetime limits.
Line range hint
434-465: LGTM: Lifetime-based eviction for both queued and pending transactions.The implementation correctly applies lifetime-based eviction to both queued and pending transactions, which helps prevent transaction pool bloat.
986-994: Verify the impact of early transaction rejection.The early rejection of transactions when an account's pending list is full could potentially lead to:
- Higher priority transactions being rejected if they arrive after the limit is reached
- Possible reordering of transactions that could affect user experience
Consider adding a priority-based eviction strategy instead of outright rejection.
core/tx_pool_test.go (2)
1100-1107: LGTM! The conditional check for nolocals is correctly implemented.The changes properly handle the case when nolocals is enabled, ensuring pending transactions are cleared as expected.
1212-1321: Verify test coverage for the new account pending limit feature.Let's check if all critical paths in the implementation are covered by tests.
✅ Verification successful
Test coverage for account pending limit feature is comprehensive ✅
The test suite thoroughly covers all critical paths including limit enforcement, nonce gap handling, and multi-account scenarios. Consider adding a test for config sanitization as a minor improvement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for account pending limit feature # Find all references to AccountPendingLimit in the codebase echo "Searching for AccountPendingLimit usage..." rg -A 5 "AccountPendingLimit" # Find all test functions related to transaction pending limits echo "Searching for related test functions..." rg -l "TestTransaction.*Pending.*Limiting"Length of output: 6925
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
core/tx_list.go (1)
442-446: Consider documenting the nil check and reviewing other methods.While adding a nil check is a good defensive programming practice, it would be helpful to:
- Document why a nil receiver is a valid state for txList
- Review other methods of txList that might need similar nil checks to maintain consistency
func (l *txList) Len() int { + // Handle nil receiver to prevent panic if l == nil { return 0 } else { return l.txs.Len() } }core/tx_pool.go (3)
1191-1195: Consolidate duplicate beats map cleanup logic.The cleanup of the beats map appears in two places. Consider extracting this into a helper method to avoid duplication and ensure consistent cleanup.
+func (pool *TxPool) cleanupBeats(addr common.Address) { + if pool.queue[addr].Empty() && pool.pending[addr].Empty() { + delete(pool.beats, addr) + } +} func (pool *TxPool) removeTx(hash common.Hash, outofbound bool) { // ... - defer func(addr common.Address) { - if pool.queue[addr].Empty() && pool.pending[addr].Empty() { - delete(pool.beats, addr) - } - }(addr) + defer func(addr common.Address) { pool.cleanupBeats(addr) }(addr) // ... } func (pool *TxPool) promoteExecutables(accounts []common.Address) []*types.Transaction { // ... - if pool.queue[addr].Empty() && pool.pending[addr].Empty() { - delete(pool.beats, addr) - } + pool.cleanupBeats(addr) // ... }Also applies to: 1587-1589
1619-1641: Optimize truncatePending implementation.The current implementation has a few areas for optimization:
- The loop could be combined with the global slots check
- The caps slice allocation could be optimized for large lists
func (pool *TxPool) truncatePending() { + pending := uint64(0) + oversizedAccounts := make(map[common.Address]struct{}) + for addr, list := range pool.pending { + size := uint64(list.Len()) + pending += size if list.Len() > int(pool.config.AccountPendingLimit) { + oversizedAccounts[addr] = struct{}{} + } + } + + // First handle oversized accounts + for addr := range oversizedAccounts { + list := pool.pending[addr] caps := list.Cap(int(pool.config.AccountPendingLimit)) for _, tx := range caps { hash := tx.Hash() pool.all.Remove(hash) pool.calculateTxsLifecycle(types.Transactions{tx}, time.Now()) pool.pendingNonces.setIfLower(addr, tx.Nonce()) log.Trace("Removed pending transaction to comply with hard limit", "hash", hash.Hex()) } pool.priced.Removed(len(caps)) pendingGauge.Dec(int64(len(caps))) if pool.locals.contains(addr) { localGauge.Dec(int64(len(caps))) } } - pending := uint64(0) - for _, list := range pool.pending { - pending += uint64(list.Len()) - } if pending <= pool.config.GlobalSlots { return }
986-994: Enhance logging for transaction rejections.When a transaction is rejected due to the account pending limit, it would be helpful to log additional context to aid in debugging.
// Account pending list is full if uint64(list.Len()) >= pool.config.AccountPendingLimit { + log.Debug("Rejecting transaction - account pending limit reached", + "account", addr.Hex(), + "hash", hash.Hex(), + "current", list.Len(), + "limit", pool.config.AccountPendingLimit) pool.all.Remove(hash) pool.calculateTxsLifecycle(types.Transactions{tx}, time.Now()) pool.priced.Removed(1) pendingDiscardMeter.Mark(1) return false }core/tx_pool_test.go (2)
964-1046: LGTM with suggestions for improvement.The test comprehensively verifies the transaction pool's behavior regarding account pending limits and transaction lifetime. However, consider these improvements:
- Extract magic numbers into named constants for better readability:
- tx := pricedTransaction(0, 100000, big.NewInt(1), remote) + const ( + testGasLimit = 100000 + testGasPrice = 1 + ) + tx := pricedTransaction(0, testGasLimit, big.NewInt(testGasPrice), remote)
- Consider using
testing.T.Run()to organize the test into logical sub-tests:t.Run("Initial Transaction Acceptance", func(t *testing.T) { ... }) t.Run("Lifetime Eviction", func(t *testing.T) { ... }) t.Run("Account Pending Limit", func(t *testing.T) { ... })
1187-1194: Enhance test coverage for transaction size validation.While the test covers basic scenarios, consider adding these test cases for better coverage:
- Edge case exactly at the limit
- Transaction with empty payload
- Multiple transactions that collectively exceed the block size
tests := []struct { name string tx *types.Transaction want error }{ {"Valid transaction", validTx, nil}, {"Oversized transaction", oversizedTx, ErrOversizedData}, + {"Edge case - exactly at limit", exactSizeTx, nil}, + {"Empty payload", emptyTx, nil}, }Also, consider adding assertions to verify the actual payload size:
assert.Equal(t, tt.tx.Size(), uint64(float64(*pool.chainconfig.Scroll.MaxTxPayloadBytesPerBlock)*float64(0.9)), "Unexpected transaction size")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/tx_list.go(1 hunks)core/tx_pool.go(10 hunks)core/tx_pool_test.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (2)
core/tx_pool.go (2)
182-183: LGTM! Well-documented configuration parameter.The new configuration parameter is well-documented and has a reasonable default value.
Also applies to: 201-202
238-241: LGTM! Proper validation of configuration parameter.The sanitize method properly validates the new configuration parameter and provides appropriate warnings.
1. Purpose or design rationale of this PR
This change aims to fix the recent issues where skipped transactions lead to congested mempools on follower nodes, leading to inaccurate gas price estimates. We address the issue by enforcing a hard cap on pending transactions per account, and dropping long-term pending transactions.
--txpool.accountpendinglimitwhich specifies a hard limit on pending txs per account.promoteTxandtruncatePending.pending(previously only applied toqueue).Note: This PR is currently based on
scroll-v5.8.0so that we can easily release a hot fix without including other changes.2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?TBD
4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Chores