rpc: Assert that RPCArg names are equal to CRPCCommand ones (mining,zmq,rpcdump) #19717

1. 
   MarcoFalke commented at 9:22 am on August 14, 2020: member

   This is split out from #18531 to just touch the RPC methods in misc. Description from the main pr:

   Motivation

   RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the CRPCCommands and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section “bugs found” for a list of bugs that have been found as a result of the changes here.

   Changes

   The changes here add an assert in the CRPCCommand constructor that the RPCArg names are identical to the ones in the CRPCCommand.

   Future work

   Here or follow up, makes sense to also assert type of returned UniValue?

   Sure, but let’s not get ahead of ourselves. I am going to submit any further works as follow-ups, including:

   • Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant
   • Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table
   • Auto-formatting and sanity checking the RPCExamples with RPCMan
   • Checking passed-in json in self-check. Removing redundant checks
   • Checking returned json against documentation to avoid regressions or false documentation
   • Compile the RPC documentation at compile-time to ensure it doesn’t change at runtime and is completely static

   Bugs found

   • The assert identified issue #18607
   • The changes itself fixed bug #19250
2. DrahtBot added the label Needs rebase on Aug 14, 2020
3. MarcoFalke force-pushed on Aug 14, 2020
4. in src/rpc/server.cpp:266 in fa7eceeb7f outdated

   260@@ -261,13 +261,11 @@ CRPCTable::CRPCTable()
   261     }
   262 }
   263 
   264-bool CRPCTable::appendCommand(const std::string& name, const CRPCCommand* pcmd)
   265+void CRPCTable::appendCommand(const std::string& name, const CRPCCommand* pcmd)
   266 {
   267-    if (IsRPCRunning())
   268-        return false;
   269+    CHECK_NONFATAL(!IsRPCRunning()); // Only add commnds before rpc is running
   

   ---

   ---

   

   theStack commented at 10:28 am on August 14, 2020:

   s/commnds/commands

   ---

   

   MarcoFalke commented at 10:38 am on August 14, 2020:

   thx, fixed
5. 
   theStack commented at 10:30 am on August 14, 2020: member

   Concept ACK
6. rpc: Remove unused return type from appendCommand fa93bc14c7
7. rpc: Assert that RPCArg names are equal to CRPCCommand ones (mining) faaa46dc20
8. rpc: Assert that RPCArg names are equal to CRPCCommand ones (zmq) fa32c1d5ec
9. rpc: Assert that RPCArg names are equal to CRPCCommand ones (rpcdump) fa3d9ce325
10. MarcoFalke force-pushed on Aug 14, 2020
11. MarcoFalke added the label Refactoring on Aug 14, 2020
12. MarcoFalke added the label RPC/REST/ZMQ on Aug 14, 2020
13. DrahtBot removed the label Needs rebase on Aug 14, 2020
14. 
    DrahtBot commented at 11:49 am on August 14, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19602 (wallet: Migrate legacy wallets to descriptor wallets by achow101)
    • #19101 (refactor: remove ::vpwallets and related global variables by ryanofsky)
    • #15129 (rpc: Added ability to remove watch only addresses by benthecarman)

    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.

15. 
    practicalswift commented at 3:14 pm on August 14, 2020: contributor

    Concept ACK
16. 
    promag commented at 0:00 am on August 17, 2020: member

    Code review ACK fa3d9ce3254882c545d700990fe8e9a678f31eed.
17. jonasschnelli added this to the "Blockers" column in a project

    ---

18. 
    fjahr commented at 11:12 am on August 31, 2020: member

    tested ACK fa3d9ce3254882c545d700990fe8e9a678f31eed

    Reviewed code, used help on a few RPCs, changed an RPC arg to see the failure.

19. MarcoFalke merged this on Aug 31, 2020
20. MarcoFalke closed this on Aug 31, 2020

    ---

21. MarcoFalke deleted the branch on Aug 31, 2020
22. laanwj removed this from the "Blockers" column in a project

    ---

23. sidhujag referenced this in commit 4c2ef86224 on Aug 31, 2020
24. Fabcien referenced this in commit 87aa73d03b on Sep 20, 2021
25. Fabcien referenced this in commit 9fe6e28e86 on Sep 20, 2021
26. Fabcien referenced this in commit bf87003a21 on Sep 20, 2021
27. Fabcien referenced this in commit 72a4597820 on Sep 20, 2021
28. DrahtBot locked this on Feb 15, 2022

Contributors
MarcoFalke theStack DrahtBot practicalswift promag fjahr

Labels
Refactoring RPC/REST/ZMQ