Refactoring CRPCCommand with enum category #13945

pull isghe wants to merge 2 commits into bitcoin:master from isghe:refactoring-CRPCCommand-with-enum-category changing 14 files +217 −153
  1. isghe commented at 11:21 pm on August 11, 2018: contributor

    refactoring CRPCCommand changing the type of its member category from std::string to a typedef enum ERPCCategory. The behaviour should be exactly the same with a very very small, in any way irrelevant, speed dump.

    I am doing this, in preparation to add attributes, likes visibility and state to CRPCCommand structure, to avoid using category names, for overridden informations like visibility or deprecation.

    For example, from v0.16.0 the command getinfo switched its category from control to hidden, while I think the idea was to let it to control category, with attributes similar to visibility = false and state = deprecated

    Thanks, Isidoro Ghezzi

    p.s:

    • Take care on the fact, that I had no warning assigning the enum to a std::string :-( Also take care I’m never testing on QT. (I simply never configure nor compile on QT); anyway I was able to catch the QT command rpcNestedTest.
    • I tested only on regtest and macOS

    EDITs:

    1. Using enum class it’s giving right error trying to assign by mistake to a std::string :-) https://github.com/bitcoin/bitcoin/pull/13945/commits/cf190f57a078df1d37bd1822e3fa69ba0d9c90e6
    2. I tested on regtest on ubuntu and macOS
    3. I never tested nor compiled on windows.
  2. isghe commented at 0:06 am on August 12, 2018: contributor
    CI failed because of tabs instead o spaces, sorry I am not a python (which version?!) developer; going to solve
  3. in src/rpc/server.h:131 in ff89b8f286 outdated 
    127@@ -128,13 +128,29 @@ void RPCRunLater(const std::string& name, std::function<void(void)> func, int64_
128 
129 typedef UniValue(*rpcfn_type)(const JSONRPCRequest& jsonRequest);
130 
131+typedef enum{
    promag commented at 0:07 am on August 12, 2018:
    Use enum class.
    domob1812 commented at 9:32 am on August 12, 2018:
    Also, if you use enum class, get rid of the eRPCCategory_ prefix (since then it will be namespaced already).
    laanwj commented at 11:38 am on August 12, 2018:
    Please don’t add bitcoin-specific things (such as these categories) in rpc/server.h, the base RPC code is supposed to be independent of any specific use.
  4. in src/rpc/server.h:132 in ff89b8f286 outdated 
    127@@ -128,13 +128,29 @@ void RPCRunLater(const std::string& name, std::function<void(void)> func, int64_
128 
129 typedef UniValue(*rpcfn_type)(const JSONRPCRequest& jsonRequest);
130 
131+typedef enum{
132+	eRPCCategory_blockchain = 0,
    promag commented at 0:07 am on August 12, 2018:
    Don’t use tabs.
  5. promag commented at 0:11 am on August 12, 2018: member

    Kind of weak concept ACK, but current implementation is a bit off.

    Please read developer-notes.md and update code according.

    Also squash commits.

  6. isghe commented at 1:03 am on August 12, 2018: contributor

    now CI it is failing because of:

    00.04s$ test/lint/check-rpc-mappings.py .
1Traceback (most recent call last):
2  File "test/lint/check-rpc-mappings.py", line 158, in <module>
3    main()
4  File "test/lint/check-rpc-mappings.py", line 98, in main
5    cmds += process_commands(os.path.join(root, fname))
6  File "test/lint/check-rpc-mappings.py", line 58, in process_commands
7    assert m, 'No match to table expression: %s' % line
8AssertionError: No match to table expression:     { eRPCCategory_control,            "help",                   &help,                   {"command"}  },
9The command "test/lint/check-rpc-mappings.py ." exited with 1.

    I don’t know how to instruct test/lint/check-rpc-mappings.py teaching to him that a field should be an enum, not a std::string.

    EDIT: should be solved (now using enum class) with https://github.com/bitcoin/bitcoin/pull/13945/commits/4d56e9b55dc61a3a73ca3d4cee95ad580e66ed19

  7. DrahtBot commented at 1:19 am on August 12, 2018: member
    • #14121 (Index for BIP 157 block filters by jimpo)
    • #14053 (Add address-based index (attempt 4?) by marcinja)
    • #14035 (Utxoscriptindex by mgrychow)
    • #13932 (Additional utility RPCs for PSBT by achow101)
    • #13926 ([WIP] [Tools] bitcoin-wallet-tool by jnewbery)
    • #13836 (RPC: Add new ‘clearmempool’ method by domob1812)
    • #13751 (Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency by 251Labs)
    • #13541 (wallet/rpc: sendrawtransaction maxfeerate by kallewoof)
    • #13152 ([rpc] Add getnodeaddresses RPC command by chris-belcher)
    • #12674 (RPC: Support addnode onetry without making the connection priviliged by luke-jr)
    • #12490 ([Wallet] [RPC] Remove deprecated wallet rpc features from bitcoin_server by jnewbery)
    • #10973 (Refactor: separate wallet from node by ryanofsky)
    • #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

    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.

  8. in src/rpc/server.cpp:55 in 01afe8b4a5 outdated 
    50+        "rawtransactions",
51+        "util",
52+        "wallet",
53+        "zmq",
54+        "test"
55+    };
    domob1812 commented at 9:31 am on August 12, 2018:
    This implementation means that the list here has to be in sync with the enum definition. I think it would be much more robust and readable if you had here a big switch instead.
  9. laanwj commented at 11:40 am on August 12, 2018: member

    A drawback of this is that it centralizes knowledge of the command categories in the source code; right now, a module such as the wallet can register its own commands in its own category, without having to update a central file. This is a good form of decoupling.

    This is not as bad for categories as it would be for, say, commands themselves, but still.

  10. laanwj added the label RPC/REST/ZMQ on Aug 12, 2018
  11. isghe commented at 5:07 pm on August 12, 2018: contributor

    A drawback of this is that it centralizes knowledge of the command categories in the source code; right now, a module such as the wallet can register its own commands in its own category, without having to update a central file. This is a good form of decoupling.

    This is not as bad for categories as it would be for, say, commands themselves, but still. @laanwj the target of this refactoring, is right to centralise the knowledge of the command categories, to avoid misuse of categories, like it was done for hidden category.

  12. isghe commented at 6:45 pm on August 12, 2018: contributor
    I am going to squash commits, let me know if everything is ok
  13. isghe force-pushed on Aug 12, 2018
  14. in src/rpc/rpccategory.cpp:11 in cbfa3a0eb1 outdated 
     6+#include <rpc/rpccategory.h>
 7+
 8+namespace rpccategory
 9+{
10+
11+std::string Label (const RPCCategory category);
    ken2812221 commented at 9:31 pm on August 12, 2018:
    Remove this line
    isghe commented at 0:39 am on August 13, 2018:
    do you mean the empty line (line 10) between { and std::string?
    ken2812221 commented at 0:52 am on August 13, 2018:
    No, I mean removing std::string Label (const RPCCategory category);, you already declare it at rpccategory.h
  15. in src/rpc/rpccategory.h:29 in cbfa3a0eb1 outdated 
    24+};
25+
26+namespace rpccategory
27+{
28+
29+extern std::string Label (const RPCCategory category);
    ken2812221 commented at 9:32 pm on August 12, 2018:
    remove extern
    isghe commented at 0:43 am on August 13, 2018:
    hm… extern (ok I’m old style C developer) means declared elsewhere, infact the prototype is declared in src/rpc/rpccategory.cpp, and the definition also in src/rpc/rpccategory.cpp. What’s wrong?
    ken2812221 commented at 1:10 am on August 13, 2018:
    extern on function is redundant, it is already extern if you don’t implement it.
  16. in src/rpc/rpccategory.cpp:13 in cbfa3a0eb1 outdated 
     8+namespace rpccategory
 9+{
10+
11+std::string Label (const RPCCategory category);
12+
13+std::string Label (const RPCCategory category)
    ken2812221 commented at 9:33 pm on August 12, 2018:
    Please remove extra space between Label and (. There are many other places need to do this.
  17. in src/rpc/rpccategory.cpp:16 in cbfa3a0eb1 outdated 
    11+std::string Label (const RPCCategory category);
12+
13+std::string Label (const RPCCategory category)
14+{
15+    #ifdef HANDLE_CASE_RETURN
16+    #error "HANDLE_CASE_RETURN already defined"
    ken2812221 commented at 9:35 pm on August 12, 2018:
    I don’t think this is necessary
    isghe commented at 0:49 am on August 13, 2018:
    you will miss it, in case of unpredictable mistakes ;-)
  18. in src/rpc/rpccategory.cpp:37 in cbfa3a0eb1 outdated 
    31+            HANDLE_CASE_RETURN (RPCCategory, wallet);
32+            HANDLE_CASE_RETURN (RPCCategory, zmq);
33+            HANDLE_CASE_RETURN (RPCCategory, test);
34+            // if you are missing a case, you'll have a warning here
35+    }
36+    #undef HANDLE_CASE_RETURN
    ken2812221 commented at 9:40 pm on August 12, 2018:
    I think we don’t need to undef this
    isghe commented at 0:51 am on August 13, 2018:
    ok right now, it’s not necessary, but i meant to define a local MACRO: define and use as you like, and undefined when you don’t need anymore.
  19. in src/rpc/rpccategory.h:2 in cbfa3a0eb1 outdated 
    0@@ -0,0 +1,33 @@
1+// Copyright (c) 2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2018 The Bitcoin Core developers
    ken2812221 commented at 9:40 pm on August 12, 2018:
    // Copyright (c) 2018 The Bitcoin Core developers
  20. in src/rpc/rpccategory.cpp:2 in cbfa3a0eb1 outdated 
    0@@ -0,0 +1,39 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2018 The Bitcoin Core developers
    ken2812221 commented at 9:41 pm on August 12, 2018:
    // Copyright (c) 2018 The Bitcoin Core developers
  21. in src/rpc/server.cpp:161 in cbfa3a0eb1 outdated 
    157@@ -158,7 +158,7 @@ std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest&
158     std::vector<std::pair<std::string, const CRPCCommand*> > vCommands;
159 
160     for (const auto& entry : mapCommands)
161-        vCommands.push_back(make_pair(entry.second->category + entry.first, entry.second));
162+        vCommands.push_back(std::make_pair(rpccategory::Label (entry.second->category) + entry.first, entry.second));
    ken2812221 commented at 9:41 pm on August 12, 2018:
    nit: emplace_back
    isghe commented at 0:45 am on August 13, 2018:
    can you explain better? thanks
    ken2812221 commented at 0:58 am on August 13, 2018:
    Since you’re touching this line, you can use c++11 emplace_back to have better performance: vCommands.emplace_back(rpccategory::Label(entry.second->category) + entry.first, entry.second);
  22. in src/rpc/server.h:20 in cbfa3a0eb1 outdated 
    16@@ -17,6 +17,8 @@
17 
18 #include <univalue.h>
19 
20+#include <rpc/rpccategory.h>
    ken2812221 commented at 9:42 pm on August 12, 2018:
    Move this after #include <rpc/protocol.h>
    isghe commented at 10:34 pm on August 12, 2018:
    doing in the next commit, thanks
  23. ken2812221 commented at 9:43 pm on August 12, 2018: contributor
    Concept ACK. A few nits.
  24. in src/rpc/rpccategory.h:13 in cbfa3a0eb1 outdated 
     8+
 9+#include <string>
10+
11+enum class RPCCategory
12+{
13+    blockchain = 0,
    MarcoFalke commented at 9:53 pm on August 12, 2018:
    I don’t think you have to set this to 0, also compile time constants should be ALL_UPPER_CASE.
    isghe commented at 10:24 pm on August 12, 2018:

    yes, it’s not necessary infact, I am going to remove the zero initialization (it was necessary before, when I was using an array to transform enum to std::string); (done in https://github.com/bitcoin/bitcoin/pull/13945/commits/7659882f3e64582cb404d6c7ed129cbc06a0011c)

    About ALL_UPPER_CASE for compile time constants, should be doesn’t mean must and as it is written in https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md I think this is near the case:

    are not required when doing so would need changes to significant pieces of existing code.

    and worst and more: consider the fact that, in the case those constants to be uppercase, an extra runtime (!) code would be neccessary to the function rpccategory::Label to make lowercase the result.

    But ok, because of that, I’ll explain in a comment in enum class RPCCategory that the constant identifiers are used exactly as the output

  25. isghe commented at 10:50 pm on August 12, 2018: contributor
    wip: please don’t merge
  26. isghe commented at 1:01 am on August 13, 2018: contributor
    wip: waiting more suggestions, before new squash
  27. isghe commented at 1:10 am on August 13, 2018: contributor

    Now I don’t understand why CI is failing. But I have to go to sleep

    EDIT: it was for missing #include <cassert> https://github.com/bitcoin/bitcoin/pull/13945/commits/c1418aa41c9df3e2fe7019146c2a2d4785e40eaf

  28. in src/rpc/rpccategory.cpp:36 in c9f15c12b2 outdated 
    31+            HANDLE_CASE_RETURN (RPCCategory, wallet);
32+            HANDLE_CASE_RETURN (RPCCategory, zmq);
33+            HANDLE_CASE_RETURN (RPCCategory, test);
34+            // if you are missing a case, you'll have a warning here
35+    }
36+    assert (0); // never fall down here
    ken2812221 commented at 1:13 am on August 13, 2018:
    You need to #include <cassert>
  29. isghe commented at 3:54 pm on August 13, 2018: contributor

    I am going to squash commits. I think there is only one open point: the compile time constants in enum class RPCCategory, are lowercase instead of (should use to be) uppercase.

    The point is that if, I’ll make those identifiers uppercase, I have to make them lowercase in same place; and I have these options:

    1. Hard coding lowercase in the source code, in the function rpccategory::Label; e.g. somethings similar to case RPCCategory::BLOCKCHAIN: return “blockchain”;, and there is the risk for example to introduce typos, and an extra maintenance step, for new categories.
    2. Lowercasing them at runtime in function rpccategory::Label; somethings similar to:
     0#include <boost/algorithm/string/case_conv.hpp>
 1
 2static std::string LabelPrivate (const RPCCategory category){
 3	/* exactly the actual code of rpccategory::Label */
 4}
 5
 6std::string Label (const RPCCategory category){
 7	std::string ret = LabelPrivate (category);
 8	boost::algorithm::to_lower (ret);
 9	return ret;
10}
    1. Lowercasing them at runtime in function CRPCTable::help

    Another option could be, to leave them in upper case, for example:

    0$ /src/bitcoin-cli help
1== BLOCKCHAIN ==
2getbestblockhash

    But we cannot know the impact and the implications, such modification in the output, could have in external software.

    Because of those points, I think it should be better to leave them in lowercase.

    Anyway let, me know

  30. isghe force-pushed on Aug 13, 2018
  31. isghe commented at 5:05 pm on August 13, 2018: contributor
    squashed
  32. ken2812221 commented at 9:50 am on August 14, 2018: contributor
    utACK 4eec7a1c3421acc9d31c4c0f93ca6ec376e0847a
  33. DrahtBot added the label Needs rebase on Aug 15, 2018
  34. ken2812221 commented at 4:50 pm on August 15, 2018: contributor
    Please cherry-pick your commits on master, not just merge master branch.
  35. isghe force-pushed on Aug 15, 2018
  36. isghe commented at 5:57 pm on August 15, 2018: contributor

    Please cherry-pick your commits on master, not just merge master branch.

    done

  37. DrahtBot removed the label Needs rebase on Aug 15, 2018
  38. in src/rpc/server.cpp:172 in 59804165de outdated 
    168@@ -169,7 +169,7 @@ std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest&
169     {
170         const CRPCCommand *pcmd = command.second;
171         std::string strMethod = pcmd->name;
172-        if ((strCommand != "" || pcmd->category == "hidden") && strMethod != strCommand)
173+        if ((strCommand != "" || pcmd->category == RPCCategory::hidden) && strMethod != strCommand)
    isghe commented at 1:55 am on August 18, 2018:

    This is the volta stone of this refactoring: avoiding a strange behavior in the semantics of category identifiers. E.g. the hidden property shouldn’t be in the name of its category, but elsewhere; after this merge a new ones will come removing the RPCCategory::hidden, and will introducing the appropriate property/attribute.

    I suppose that in the future the above instruction will be somethings similar to

    0// pseudo code
1RPCCategoryVisibility::VISIBLE != pcmd->attributes.visibility

    removing ambiguity and adding security to the Bitcoin protocol we love :-)

    If not, how we will handle the next category names, for example hiddden (with triple, by mistake/typo, d) or not-visible, or crash-everythings?

  39. in src/rpc/rpccategory.cpp:28 in 59804165de outdated 
    33+            HANDLE_CASE_RETURN(RPCCategory, test);
34+            // if you are missing a case, you'll have a warning here
35+    }
36+    assert(0); // never fall down here
37+    #undef HANDLE_CASE_RETURN
38+}
    l2a5b1 commented at 1:53 pm on August 19, 2018:

    I would prefer a more straightforward implementation like the one below if we include a category label function:

     0std::string Label(const RPCCategory category)
 1{
 2    switch (category) {
 3        case RPCCategory::blockchain: return "Blockchain";
 4        case RPCCategory::control: return "Control";
 5        case RPCCategory::generating: return "Generating";
 6        case RPCCategory::hidden: return "Hidden";
 7        case RPCCategory::mining: return "Mining";
 8        case RPCCategory::network: return "Network";
 9        case RPCCategory::rawtransactions: return "Rawtransactions";
10        case RPCCategory::util: return "Util";
11        case RPCCategory::wallet: return "Wallet";
12        case RPCCategory::zmq: return "Zmq";
13        case RPCCategory::test: return "Test";
14        default: assert(false);
15    }
16}

    which would also enable you to simplify:

    https://github.com/bitcoin/bitcoin/blob/1f470a8916aabbb7f03afda26d41ebfd8d1a2263/src/rpc/server.cpp#L195-L197

    isghe commented at 6:45 pm on August 19, 2018:

    Yes and now we can see how much is important this refactoring, before making any others modifications, in this area: try figuring out how would be hard now and prone to errors, without this refactoring, to capitalise the category’s identifiers to remove that unwanted boost::to_upper. Consider that without this refactoring, there is no easy way, to know wich and how many categories we have in the source code. For similar reasons, it is necessary before working on properties like visibility or status etc… for CRPCCommand.

    That is why I created this refactoring PR: to make easier, less prone errors, forward modifications.

    Anyway now, knowing that it will help for next PRs, I’m going to uppercase the enum RPCCategory constants, and modify the “template” HANDLE_CASE_RETURN(RPCCategory, blockchain) used in rpccategory::Label to case RPCCategory::BLOCKCHAIN: return "blockchain" (observe the first char is lowercase not capitalised: that’s because the purpose of this PR is just refactoring, not fixing bug; removing that unwanted boost::to_upper deserve the importance of a new dedicated fixing PR).

    About the default: assert(false);, with that we are losing compiler warning in the case we are not handling a new category

    0rpc/rpccategory.cpp: In function 'std::__cxx11::string rpccategory::Label(RPCCategory)':
1rpc/rpccategory.cpp:21:12: warning: enumeration value 'test2' not handled in switch [-Wswitch]
2     switch (category)

    So I think it should be better, not to handle the default case.

  40. l2a5b1 commented at 4:08 pm on August 19, 2018: contributor

    I am leaning towards NACK unfortunately.

    I am doing this, in preparation to add attributes, likes visibility and state to CRPCCommand structure, to avoid using category names, for overridden informations like visibility or deprecation.

    Although I agree that this could be useful, the introduction of a new source and header file rpccategory to fulfil the future objective of being able to extend CRPCCommand feels a bit like over-engineering in the current state 5980416.

    I would really prefer that the pull request focusses on the end goal first by trying to achieve that specifically.

    One way could be to add the intended new “visibility” attribute to CRPCCommand and initialize the various CRPCCommand tables accordingly and take it from there.

    Can we do this without having to add rpccategory?

    Please also consider my inline feedback about the implementation of std::string Label(const RPCCategory category) which addresses #13945 (review) and would allow you to address #13945 (review) (both of which are valid comments IMO).

  41. l2a5b1 commented at 8:44 pm on August 19, 2018: contributor

    Thanks for the update @isghe!

    I am leaning towards NACK because it seems that what it is you want to lay the foundation for can also be achieved without rpccategory.

    Per your motivation:

    I am doing this, in preparation to add attributes, likes visibility and state to CRPCCommand structure, to avoid using category names, for overridden informations like visibility or deprecation.

    Let’s say you want to add that visibility / deprecated flag:

    0class CRPCCommand
1{
2public:
3    const std::string category;
4    const std::string name;
5    const bool visibility;
6    ...
7    ...
8};

    0static const CRPCCommand vRPCCommands[] =
1{ //  category              name                      visible / deprecated     ... etc ...
2  //  --------------------- ------------------------  -----------------------  ----------
3    /* Overall control/query calls */
4    { "control",            "help",                   true,           ...},
5    { "control",            "stop",                   true,           ...},
6    { "control",            "uptime",                 true,           ...},
7    { "control",            "foo",                    false,          ...}
8};

    Am I missing something obvious why we need rpccategory in this case?

    If we do need rpccategory I am happy to review again; I am not against the idea perse.

  42. isghe commented at 10:32 pm on August 19, 2018: contributor

    Thanks for your attention @251Labs!

    For simplicity, just speaking about "hidden" category and visibility property:

    Without enum RPCCategory, how will I proceed to add the visible property? First I will add the property visibility field to the structure CRPCCommand, then I will search for the the string “hidden”: in the last commit df660aa7717a6f4784e90535a13a95d82244565a on master branch, now I have 16 results (only searching on *.cpp/h files) where 13 are related directly to CRPCCommand, and 1 used to decide to show or not those command in the help command: pcmd->category == "hidden". And ok than I will modify those 13 lines of code, adding the false for the visibility field (maybe I can add a CRPCCommand constructor so, the default value for visibility will be true).

    Then I’ll modify the name of those hidden category, e.g: { "hidden", "invalidateblock", &invalidateblock, {"blockhash"} }

    will become: { "blockhain", "invalidateblock", &invalidateblock, {"blockhash"}, true }

    and then I’ll modify: pcmd->category == "hidden"

    to: false == pcmd->visibility // yes putting the constant on left it can helps (but ok it’s another argument)

    Everything is ok, it compiles; make check, tests and CI Travis are ok, and we are all happy…

    …but:

    • I wouldn’t be sure 100% to catch all the “hidden” category: infact:
      • I made the search only in *.cpp/h files;
      • how would I caught the string "hi" "dden" searching for "hidden" (it could be a command hiding itself to be in “hidden” category!)? And this is just a way to obfuscate to be in “hidden” category;
    • By mistake I introduced a mispelled category: “blockhain”.

    So without enum RPCCategory, we cannot be 100% sure:

    • we forgot some commands in “hidden” category;
    • we introduced typos in category’s identifiers.

    Having enum RPCCategory we can be 100% sure, at compile time (!):

    • we are not forgetting some commands in “hidden” category.
    • we are not introducing typos in category’s name.

    and finally (in a new PR) removing the RPCCategory::HIDDEN, we’ll be sure 100%, at compile time, nobody is abusing its semantic.

    isidoro

  43. in src/rpc/rpccategory.cpp:16 in b852456724 outdated 
    11+
12+std::string Label(const RPCCategory category)
13+{
14+    switch (category) {
15+        case RPCCategory::BLOCKCHAIN: return "blockchain";
16+        case RPCCategory::CONTROL: return "control";
    l2a5b1 commented at 10:11 am on August 20, 2018:

    If we introduce a freestanding utility function that returns category labels, I think we should at least make it return the correct labels:

     0        case RPCCategory::BLOCKCHAIN: return "Blockchain";
 1        case RPCCategory::CONTROL: return "Control";
 2        case RPCCategory::GENERATING: return "Generating";
 3        case RPCCategory::HIDDEN: return "Hidden";
 4        case RPCCategory::MINING: return "Mining";
 5        case RPCCategory::NETWORK: return "Network";
 6        case RPCCategory::RAWTRANSACTIONS: return "Rawtransactions";
 7        case RPCCategory::UTIL: return "Util";
 8        case RPCCategory::WALLET: return "Wallet";
 9        case RPCCategory::ZMQ: return "Zmq";
10        case RPCCategory::TEST: return "Test";
    isghe commented at 7:50 pm on August 20, 2018:
    done in d8e5619c9242943e2ae1cfba9c96230965d17c0b
  44. in src/rpc/rpccategory.cpp:26 in b852456724 outdated 
    21+        case RPCCategory::RAWTRANSACTIONS: return "rawtransactions";
22+        case RPCCategory::UTIL: return "util";
23+        case RPCCategory::WALLET: return "wallet";
24+        case RPCCategory::ZMQ: return "zmq";
25+        case RPCCategory::TEST: return "test";
26+        // if you are missing a case, you'll have a warning here
    l2a5b1 commented at 10:11 am on August 20, 2018:
    nit: remove comment?
    isghe commented at 7:50 pm on August 20, 2018:
    done in fa6d149eb264e8594eabbe1f6bd31b7ca5dd0a66
  45. in src/rpc/rpccategory.h:10 in b852456724 outdated 
     5+#ifndef BITCOIN_RPC_RPCCATEGORY_H
 6+#define BITCOIN_RPC_RPCCATEGORY_H
 7+
 8+#include <string>
 9+
10+enum class RPCCategory
    l2a5b1 commented at 10:12 am on August 20, 2018:
    To what extend are RPCCategory and OptionsCategory in util.h redundant?
    isghe commented at 8:13 pm on August 20, 2018:
    In the way they are implemented and used, there seems to be no relationship between RPCCategory and OptionsCategory.
  46. in src/rpc/server.cpp:190 in b852456724 outdated 
    186@@ -187,11 +187,11 @@ std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest&
187                 if (strHelp.find('\n') != std::string::npos)
188                     strHelp = strHelp.substr(0, strHelp.find('\n'));
189 
190-                if (category != pcmd->category)
191+                if (category != rpccategory::Label(pcmd->category))
    l2a5b1 commented at 10:12 am on August 20, 2018:

    If we introduce RPC category enumerations, I think we should also use them and not map their values back to labels.

    By declaring std::string category as RPCCategory category and making rpccategory::Label return the correct labels you will be able to optimize this if statement:

    0                if (category != rpccategory::Label(pcmd->category))
1                {
2                    if (!category.empty())
3                        strRet += "\n";
4                    category = rpccategory::Label(pcmd->category);
5                    std::string firstLetter = category.substr(0,1);
6                    boost::to_upper(firstLetter);
7                    strRet += "== " + firstLetter + category.substr(1) + " ==\n";
8                }

    to:

    0                if (category != pcmd->category) {
1                    category = pcmd->category;
2                    strRet += "\n== " + rpccategory::Label(category) + " ==\n";
3                }

    and remove:

    0#include <boost/algorithm/string/case_conv.hpp> // for to_upper()
    isghe commented at 7:56 pm on August 20, 2018:

    done in two steps: the first in 3a61034793287b1d23655d32fe55d14aba768a91 and the second in d8e5619c9242943e2ae1cfba9c96230965d17c0b, because I consider the first a refactoring, and the second a fix. I had to handle the uninitialized state of RPCCategory category and I left the output exactly the same we had before of this refactoring (not adding a new empty line at the beginning)

    0$ ./src/bitcoin-cli help | sha256
12ae3a94e05bbb98323f72f19272bde01f075db16c7e4357bc46815a1393cd44a
  47. in src/rpc/server.h:135 in b852456724 outdated 
    132@@ -131,10 +133,10 @@ typedef UniValue(*rpcfn_type)(const JSONRPCRequest& jsonRequest);
133 class CRPCCommand
134 {
135 public:
    l2a5b1 commented at 10:19 am on August 20, 2018:
    nit: while we are here, maybe make CRPCCommand a struct and remove the public access modifier?
    isghe commented at 8:06 pm on August 20, 2018:
    done in 7f997989bc63fe14ada94094751c8b4551a4178c, but consider that usually, old style C++ developer prefer class public to struct for portability (struct forward declarations were a little bit tricky). In the same file server.h there are others class public
  48. l2a5b1 commented at 11:04 am on August 20, 2018: contributor

    Between NACK / weak concept ACK b852456.

    I still have the same reservations that made me NACK this before, but b852456 looks better.

    I have added some additional review comments that are hopefully helpful.

  49. isghe commented at 10:48 pm on August 20, 2018: contributor

    Hi @251Labs, thanks for your attention :-)

    Let’s see the last 3 commits:

    1. std::string category to RPCCategory category 3a61034793287b1d23655d32fe55d14aba768a91
    2. remove src/rpc/server.cpp*to_upper d8e5619c9242943e2ae1cfba9c96230965d17c0b
    3. class CRPCCommand to struct CRPCCommand 7f997989bc63fe14ada94094751c8b4551a4178c

    Mine considerations:

    • I think this PR should be close/merge/refuse/squash at point 1 (or others following commits related to that);
    • Commit at point 2 is a very good improvement, but it’s not related to this refactoring: I would say that the very good improvement of point 2 was only possible thanks to this refactoring (point 1); I mean point 1 can live without point 2, viceversa point 2 will hard live without point 1.
    • Commit at point 3 is not related to this PR, and maybe it’s wrong considering that public class is not perfect equivalent to struct, at the end it’s another argument, not related to this refactoring PR; point 3 can be committed independently from this refactoring PR.

    So my idea is to squash this PR to point 1, and if you agree to merge or trash: if merged we can create a new PR for point 2 else, you can try to create an equivalent PR, without this refactoring (at point 1), and than you will understand why this PR squashed at point 1 is important.

    Consider that over point 1 it’s possibile to create improvement independently from point 2, and (why not?) maybe another point 2 better than the actual you can see here.

    I’m planning to begin to work on CRPCCommand properties on point 1 (obviously not in this PR)

    Let me know :-)

  50. in src/rpc/server.cpp:156 in 7f997989bc outdated 
    151@@ -153,12 +152,13 @@ std::vector<unsigned char> ParseHexO(const UniValue& o, std::string strKey)
152 std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest& helpreq) const
153 {
154     std::string strRet;
155-    std::string category;
156+    RPCCategory category;
157+    bool uninitializedCategory = true;
    l2a5b1 commented at 11:14 pm on August 20, 2018:
    Create enumerator RPCCategory::NONE and initialize category with that constant as a replacement for uninitializedCategory?
    isghe commented at 8:17 pm on August 21, 2018:

    I solved using:

    0boost::optional<RPCCategory> category;

    without adding RPCCategory::NONE; what do you think about using boost::optional in these situations? For what I know anyway std::optional will be available in C++17 without using boost.

  51. in src/rpc/server.cpp:197 in 7f997989bc outdated 
    197                         strRet += "\n";
198                     category = pcmd->category;
199-                    std::string firstLetter = category.substr(0,1);
200-                    boost::to_upper(firstLetter);
201-                    strRet += "== " + firstLetter + category.substr(1) + " ==\n";
202+                    strRet += "== " + rpccategory::Label (category) + " ==\n";
    l2a5b1 commented at 11:16 pm on August 20, 2018:
    nit: space between ::Label and (category)
    isghe commented at 8:09 pm on August 21, 2018:
    done
  52. l2a5b1 commented at 0:06 am on August 21, 2018: contributor

    Thanks for the updates @isghe!

    I think I would squash all commits up to d8e5619 and keep 7f99798 as a separate commit.

    Maybe other reviewers can give their opinion on this PR.

  53. DrahtBot added the label Needs rebase on Aug 21, 2018
  54. isghe force-pushed on Aug 21, 2018
  55. isghe commented at 8:38 pm on August 21, 2018: contributor
    waiting for more reviews before squashing, thanks :-)
  56. DrahtBot removed the label Needs rebase on Aug 21, 2018
  57. promag commented at 10:03 pm on August 21, 2018: member

    The performance gain is not a that much beside the fact that it’s not the main motivation.

    I am doing this, in preparation to add attributes, likes visibility and state to CRPCCommand structure, to avoid using category names, for overridden informations like visibility or deprecation.

    Like @251Labs said, this change is not required for that.

    Regarding visibility and state, I think you should not change CRPCCommand, instead I suggest to add separate data structures for that. For instance, there could be

    0typedef int version_t;
1// maps command to version which deprecation started
2std::map<std::string, version_t> g_rpc_deprecated_commands;
3
4// set of hidden commands
5std::set<std::string> g_rpc_hidden_commands;

    The advantage of the above is, for instance, to see all deprecated commands in one place.

    Summing up, and after @laanwj and @251Labs, I’m sorry but I also NACK this change.

  58. isghe commented at 11:45 pm on August 21, 2018: contributor

    Hi @promag thanks for your review.

    I would say, the only place where I spoke about performance, were together with the adjective irrelevant in this PR: this PR should be related only on the organisation, of the informations, related to the actual CRPCCommand::category, and now (without this PR) is sparse and not measurable (it’s a string…).

    This PR, title starts with Refactoring and (the good idea) it should not change the behaviour (I hope) in Bitcoin.

    Which categories and how many we have now in Bitcoin RPC commands? Actually it’s not easy to respond in deterministic way to the above question; maybe the only way, is to run ./bitcoin-cli help and parsing the results (missing the “hidden” category)!

    Adding a global variable for each possible properties (hidden, deprecated, etc… properties) as you suggest, can be a good solution, but it can be a nightmare for maintenance, or maybe (if necessary) it can be done, in next steps, not now.

    Like an example, consider also, that without this enum RPCCategory PR, it will be very hard (or very tedious and prone to errors) to delete the dependancy of src/rpc/server.cpp from the unwanted (for security locale reasons) “*.to_upper”.

    Without this PR; how Bitcoin will handle the next “blockhain” category? (take care about the typo). Or, security reasons: how Bitcoin will handle a category (now it’s just a string) sendtoaddress…?

    Thanks :-)

  59. promag commented at 11:56 pm on August 21, 2018: member

    Without this PR; how Bitcoin will handle the next “blockhain” category? (take care about the typo). Or, security reasons, how Bitcoin will handle a category (now it’s just a string) sendtoaddress…?

    You can write a test that asserts the possible categories.

  60. promag commented at 11:58 pm on August 21, 2018: member

    Actually it’s not easy to respond in deterministic way to the above question; maybe the only way, is to run ./bitcoin-cli help and parsing the results!

    And documentation. Or the above test.

  61. isghe commented at 0:54 am on August 22, 2018: contributor

    @promag do you really think that adding a PR documentation similar to this, will help?

    0Joking-Bitcoin documentation:
1just for developer: please take care on writing the string field of `CRPCCommand::category`; take care about typos, and  [SQL, script, bash, awk , regex or others] injections.
2Also take care that categories should be in space definition, we don't know how to determinate; anyway it's delegated to an external tools (our unit test tools), we cannot verify if it is fully synchronized with the real source code.

    just joking… do you? :-p

    EDIT: renamed to Joking-Bitcoin documentation for security reasons.

  62. isghe commented at 8:47 pm on August 22, 2018: contributor

    Another consideration from another perspective: right now in branch master 271b379e636afa419c5208cb462c07090490266c searching for the hard coded string "wallet" we have 61 occurrences found, and are all (we are lucky) related to CRPCCommand. So a good programming practice, I think should be to use a constant, instead of 61 hard coded strings, e.g.:

    0namespace rpccommand
1{
2    const std::string WALLET="wallet";
3} // namespace "rpccommand"

    and substitute the 61 hard coded "wallet" with rpccommand::WALLET

    Am I wrong?

  63. DrahtBot added the label Needs rebase on Aug 26, 2018
  64. isghe force-pushed on Aug 27, 2018
  65. DrahtBot removed the label Needs rebase on Aug 27, 2018
  66. DrahtBot added the label Needs rebase on Aug 27, 2018
  67. isghe force-pushed on Aug 27, 2018
  68. DrahtBot removed the label Needs rebase on Aug 27, 2018
  69. DrahtBot added the label Needs rebase on Aug 29, 2018
  70. Refactoring CRPCCommand with enum category 
    This is a squash of the below commits:

- enum ERPCCategory, CRPCCommand::Label
- wip: CRPCCommand.category from type string to enum
- wip: CRPCCommands construct with enum
- refactoring with enum ok
- missing test category in qt for cmd rpcNestedTest
- wip: text formatting: tabs to spaces.
- wip: refactor using `enum class` C++11.
Update CRPCCommand::Label () using a switch case
- wip: check-rpc-mappings.py fix regex to new format
Now it's catching the format:
'    { RPCCategory::control,            "help",                   &help,                   {"command"}  },'

before was catching:
'    { "control",            "help",                   &help,                   {"command"}  },'

- wip: add src/rpc/rpccategory.h|cpp
- removed CRCPCommand::Label for rpccategory::Label
- moved `#include <rpc/rpccategory.h>`
- removed unneccessary RPCCategory initializiation
- comments and one assert(0)
- // Copyright (c) 2018 The Bitcoin Core developers

for files: src/rpc/rpccategory.h|cpp
as suggested from github user ken2812221
- wip: missing `#include <cassert>`
- using `emplace_back` C++11
- remove old-style C `extern`
- remove spaces after function names
- remove unnecessary call to RPCCategory::Label

(cherry picked from commit 4eec7a1c3421acc9d31c4c0f93ca6ec376e0847a)

enum RPCCategory uppercase, rpccategory:Label lowercase

fix test/lint/check-rpc-mappings.py to uppercase

remove unnecessary comments, and  false instead of 0

std::string category to RPCCategory category

Using boost::optional<RPCCategory>

remove src/rpc/server.cpp Capitalize() call

Capitalization is done by hand in
rpccategory::Label()
    a7437c5636
  71. class CRPCCommand to struct CRPCCommand 
    removed also an un unnecessary forward declaration
    04e70f2e25
  72. isghe force-pushed on Aug 30, 2018
  73. DrahtBot removed the label Needs rebase on Aug 30, 2018
  74. promag commented at 8:03 am on September 1, 2018: member

    and substitute the 61 hard coded “wallet” with rpccommand::WALLET @isghe that’s unrelated to this PR, and IMO this can be closed.

  75. laanwj commented at 8:31 am on September 1, 2018: member
    Sorry, there seems to be no agreement to do this, in which case we prefer not making a change to the code. Closing.
  76. laanwj closed this on Sep 1, 2018
  77. isghe commented at 0:25 am on September 2, 2018: contributor
    Thanks for your reviews; I am happy because this PR, inspired #14020 , and I did learn new things on C++ programming (emplace and optional), and about test in python. But I think that Refactoring CRPCCommand with enum category (this PR) should help (at compile time) security for Bitcoin. If don’t you see that, it’s not my problem.
  78. DrahtBot locked this on Feb 15, 2022


isghe promag domob1812 laanwj DrahtBot ken2812221 MarcoFalke l2a5b1

Labels
RPC/REST/ZMQ

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-22 00:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me