-
Notifications
You must be signed in to change notification settings - Fork 38.8k
refactor: Move mempool RPCs to rpc/mempool #24656
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
The head ref may contain hidden characters: "2203-more-mempool-\u{1F362}"
Conversation
Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
cf50a75 to
fac5a51
Compare
promag
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.
Code review ACK fac5a51.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
shaavan
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.
Concept ACK
- The static keyword is used appropriately in
rpc/mempool.cppfile. I checked for any other instances where a static keyword could be used and found none. - The moved code looks good to me. I was able to verify the move-only changes in the second commit successfully.
- I was able to compile the PR successfully on Ubuntu 20.04
| {"rawtransactions", &sendrawtransaction}, | ||
| {"rawtransactions", &testmempoolaccept}, |
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.
nit:
I would suggest putting these two lines after the rest of the blockchain category functions to match the proper lexicographical ordering.
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.
I am expecting that everything will move to a new mempool category later, so I am leaving as-is for now.
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.
I think that's acceptable reasoning.
Other than this, I can't find any blockers for this PR, and hence I think it can be merged.
ACK fac5a51
This moves the remaining mempool RPCs to
rpc/mempool. Previously all mempool RPCs from theblockchaincategory have been moved. This patch moves the ones from therawtransactionscategory.In the future, as a follow-up to this refactoring patch, it could be considered whether a new
mempoolcategory should be introduced.Beside a clearer code organization, this pull request should also reduce the compile time and space of the
rawtransactions.cppfile.