I172 service transactions garbage collector#329
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the garbage collection mechanism for service transactions (zero gas price transactions) in the transaction pool. The changes fix a critical logic bug where valid transactions were being removed instead of invalid ones, update documentation for clarity, correct a typo, and adjust logging levels to be more appropriate for routine operations.
- Fixes inverted logic in garbage collection that was incorrectly removing valid transactions
- Updates documentation and parameter naming to clarify the garbage collection behavior
- Reduces logging verbosity for routine GC operations from info/warn to debug/trace levels
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/transaction-pool/src/pool.rs | Fixed critical bug in garbage_collect logic by inverting the should_keep_function check, updated documentation, changed condition check from best_transactions to transactions, renamed parameter for clarity, and reduced logging level from info to debug |
| crates/ethcore/src/client/client.rs | Fixed typo "servive" -> "service" in documentation and reduced logging level from info to trace for garbage collection messages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if self.best_transactions.is_empty() { | ||
| /// Performs garbage collection of the pool for free service transactions (zero gas transactions). | ||
| /// Only checks lowest nonce. | ||
| /// inject "should_keep_function" to decide. |
There was a problem hiding this comment.
The comment says "inject 'should_keep_function' to decide" but should use proper capitalization and clearer phrasing. Consider: "Inject should_keep_function to determine which transactions to keep."
| /// inject "should_keep_function" to decide. | |
| /// Inject should_keep_function to determine which transactions to keep. |
| pub fn garbage_collect<F: Fn(&T) -> bool>(&mut self, service_transaction_check: F) { | ||
| if self.best_transactions.is_empty() { | ||
| /// Performs garbage collection of the pool for free service transactions (zero gas transactions). | ||
| /// Only checks lowest nonce. |
There was a problem hiding this comment.
The comment "Only checks lowest nonce" is misleading. The function actually checks all consecutive zero-gas-price transactions starting from the lowest nonce until it encounters a non-zero gas price transaction. Consider updating to: "Checks consecutive zero-gas-price transactions starting from the lowest nonce."
| /// Only checks lowest nonce. | |
| /// Checks consecutive zero-gas-price transactions starting from the lowest nonce. |
No description provided.