refactor: reuse CompareAncestorFeeRate to remove duplicate fee rate calculation #1650
Conversation
WalkthroughA forward declaration for the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (1)
src/miner.h (1)
89-94: Confirm strict-weak ordering in CompareAncestorFeeRate and simplify tie-checkI wasn’t able to locate the definition of
CompareAncestorFeeRate. Please verify that it implements a strict comparator (using<via cross-multiplication of fees/sizes and never<=) and satisfies strict-weak ordering. Once confirmed, insrc/miner.h(lines 89–94) replace:if (CompareAncestorFeeRate(aFees, aSize, bFees, bSize) == CompareAncestorFeeRate(bFees, bSize, aFees, aSize)) { return CTxMemPool::CompareIteratorByHash()(a.iter, b.iter); } return CompareAncestorFeeRate(aFees, aSize, bFees, bSize);with:
const bool a_lt_b = CompareAncestorFeeRate(aFees, aSize, bFees, bSize); const bool b_lt_a = CompareAncestorFeeRate(bFees, bSize, aFees, aSize); if (!a_lt_b && !b_lt_a) { return CTxMemPool::CompareIteratorByHash()(a.iter, b.iter); } return a_lt_b;This reduces it to two helper calls and makes the tie-case explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/miner.h(2 hunks)
🔇 Additional comments (2)
src/miner.h (2)
80-81: Comment update LGTMAccurately reflects the refactor intent to centralize fee rate comparison.
12-14: Incorrect suggestion: keep the forward declaration in miner.hThe search across the entire repository (using
rg -n CompareAncestorFeeRate -S) shows that the only declaration ofCompareAncestorFeeRateis the one insrc/miner.h. There is no matching declaration intxmempool.h, so removing the forward declaration would break the build. Please ignore the previous recommendation to drop it.Likely an incorrect or invalid review comment.
| double aFees = (double)a.nModFeesWithAncestors; | ||
| double aSize = (double)a.nSizeWithAncestors; | ||
| double bFees = (double)b.nModFeesWithAncestors; | ||
| double bSize = (double)b.nSizeWithAncestors; | ||
|
|
There was a problem hiding this comment.
Do not cast fees/size to double; keep integer types for exactness
Casting nModFeesWithAncestors and sizes to double risks precision loss and subtle ordering bugs. Keep everything in integral types and pass them directly to the helper (which should also operate on integral types).
Apply:
- double aFees = (double)a.nModFeesWithAncestors;
- double aSize = (double)a.nSizeWithAncestors;
- double bFees = (double)b.nModFeesWithAncestors;
- double bSize = (double)b.nSizeWithAncestors;
+ const CAmount aFees = a.nModFeesWithAncestors;
+ const uint64_t aSize = a.nSizeWithAncestors;
+ const CAmount bFees = b.nModFeesWithAncestors;
+ const uint64_t bSize = b.nSizeWithAncestors;Note: This assumes the helper takes integral types; align with the actual signature in txmempool.h.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| double aFees = (double)a.nModFeesWithAncestors; | |
| double aSize = (double)a.nSizeWithAncestors; | |
| double bFees = (double)b.nModFeesWithAncestors; | |
| double bSize = (double)b.nSizeWithAncestors; | |
| const CAmount aFees = a.nModFeesWithAncestors; | |
| const uint64_t aSize = a.nSizeWithAncestors; | |
| const CAmount bFees = b.nModFeesWithAncestors; | |
| const uint64_t bSize = b.nSizeWithAncestors; |
🤖 Prompt for AI Agents
In src/miner.h around lines 84 to 88, the code casts fee and size values from
integers to double, risking precision loss and ordering bugs. Remove the casts
and keep these variables as their original integral types, passing them directly
to the helper function. Also, verify and align the helper function's signature
in txmempool.h to accept integral types accordingly.
PR intention
Refactor fee rate comparison logic in CompareModifiedEntry to use the shared helper function CompareAncestorFeeRate from txmempool.h, eliminating code duplication and improving maintainability.
Code changes brief