Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Mar 24, 2022

This moves the remaining mempool RPCs to rpc/mempool. Previously all mempool RPCs from the blockchain category have been moved. This patch moves the ones from the rawtransactions category.

In the future, as a follow-up to this refactoring patch, it could be considered whether a new mempool category should be introduced.

Beside a clearer code organization, this pull request should also reduce the compile time and space of the rawtransactions.cpp file.

MarcoFalke added 2 commits March 24, 2022 08:21
Can be reviewed with
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
Copy link
Contributor

@promag promag left a 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.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 24, 2022

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #24661 (refactor: Use clang-tidy syntax for C++ named arguments by fanquake)
  • #24513 (CChainState -> Chainstate by jamesob)
  • #21422 (Add feerate histogram to getmempoolinfo by kiminuo)

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.

Copy link
Contributor

@shaavan shaavan left a 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.cpp file. 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

Comment on lines +677 to +678
{"rawtransactions", &sendrawtransaction},
{"rawtransactions", &testmempoolaccept},
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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

@maflcko maflcko merged commit 3d2f24b into bitcoin:master Mar 28, 2022
@maflcko maflcko deleted the 2203-more-mempool-🍢 branch March 28, 2022 07:01
@bitcoin bitcoin locked and limited conversation to collaborators Mar 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants