Skip to content

Conversation

@Fuzzbawls
Copy link
Collaborator

Revives/replaces #1720, backporting bitcoin#7766.

Split out methods to every module, apart from 'help' and 'stop' which
are implemented in rpcserver.cpp itself.

  • This makes it easier to add or remove RPC commands - no longer
    everything that includes rpcserver.h has to be rebuilt when there's a
    change there.
  • Cleans up rpc/server.h by getting rid of the huge cluttered list of
    function definitions.
  • Move zPIV RPC commands to their own file
  • Move getstakingstatus to the more appropriate rpcwallet.cpp
  • Hide commands intended for testing environments only
  • Split up the ambiguous "pivx" command category

@Fuzzbawls Fuzzbawls added this to the 5.0.0 milestone Oct 17, 2020
@Fuzzbawls Fuzzbawls self-assigned this Oct 17, 2020
random-zebra
random-zebra previously approved these changes Oct 20, 2020
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase needed (new getcachedblockhashes introduced in #1904 can be added to the hidden category), otherwise looking good. Left just a couple notes.

@random-zebra random-zebra dismissed their stale review October 20, 2020 15:14

rebase required

Split out methods to every module, apart from 'help' and 'stop' which
are implemented in rpcserver.cpp itself.

- This makes it easier to add or remove RPC commands - no longer
  everything that includes rpcserver.h has to be rebuilt when there's a
  change there.
- Cleans up `rpc/server.h` by getting rid of the huge cluttered list of
  function definitions.
All of these commands are set to be deprecated/removed post-5.0, might
as well extract them out of rpcwallet to a dedicated file to reduce
bloat in rpcwallet.cpp
This is a better place to have this command, especially since it is
already categorized as a "wallet" command.
These commands are primarily only used during regtest functional
testing, or are otherwise not relevant to our current mainnet/testnet
chains.
Split into two primary categories ("masternode" and "budget") to better
reflect the purpose. `mnsync` and `spork` are now in the "control"
category.
Keeping them in `rpc/blockchain.cpp` because they aren't strictly wallet
commands, and `findserials` relies on the `validaterange` function here,
no sense in duplicating that code elsewhere.

Both are set to be deleted after 5.0 is locked in.
@Fuzzbawls
Copy link
Collaborator Author

rebased

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good, travis is still running but utACK 903b618.
Left a tiny nit that can be tackled in another work.

void RegisterBlockchainRPCCommands(CRPCTable &tableRPC)
{
for (unsigned int vcidx = 0; vcidx < ARRAYLEN(commands); vcidx++)
tableRPC.appendCommand(commands[vcidx].name, &commands[vcidx]);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tiny nit, could be written:

for (const auto& command : commands)
        tableRPC.appendCommand(command.name, &command);

(And same for the others RegisterxxxRPCCommands)

@furszy furszy requested a review from random-zebra October 30, 2020 04:29
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 903b618 and merging...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants