-
Notifications
You must be signed in to change notification settings - Fork 8
fix!: use best_transactions for get_txs #29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @tac0turtle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've implemented a crucial fix that refactors the transaction selection mechanism within the RPC layer. Previously, the system iterated through pending transactions; however, based on recommendations from the reth team, I've switched to using best_transactions. This change ensures that the system prioritizes transactions more effectively, likely leading to better performance and more accurate transaction handling within the pool. This is a targeted improvement to the core transaction processing logic.
Highlights
- Transaction Selection Logic: I've updated the
get_txsfunction to useself.pool.best_transactions()instead ofself.pool.pending_transactions(). This change was suggested by the reth team to improve how transactions are selected from the pool. - Variable Renaming: I've refactored the variable name from
pending_maptoselected_txsto align with the new logic of selecting the "best" transactions. - Code Comments Update: I've updated the inline comments to clearly indicate that the function now iterates over transactions sorted by priority, providing better clarity for future code readers.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
The pull request correctly switches from using pending_transactions to best_transactions to fetch transactions from the pool. This is a good improvement as it ensures transactions are prioritized. I've added a couple of suggestions to improve code clarity and performance within the new implementation.
crates/evolve/src/rpc/txpool.rs
Outdated
| let mut rlp_bytes = Vec::new(); | ||
| tx.encode(&mut rlp_bytes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current RLP encoding creates a new Vec with default capacity and then encodes into it, which may cause reallocations. You can make this more efficient by using encode_to_vec(), which pre-allocates the Vec with the exact required capacity before encoding. This avoids potential reallocations during the encoding process.
| let mut rlp_bytes = Vec::new(); | |
| tx.encode(&mut rlp_bytes); | |
| let rlp_bytes = tx.encode_to_vec(); |
Manav-Aggarwal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left two small comments, curious for your thoughts. LGTM otherwise
Description
after talking to the reth team they proposed using best_transactions instead of pending.
Type of Change
Related Issues
Fixes #(issue)
Checklist
Testing
Additional Notes