Remove pointer cast in CRPCTable::dumpArgMap #21035

pull ryanofsky wants to merge 3 commits into bitcoin:master from ryanofsky:pr/argmap changing 7 files +49 −34
  1. ryanofsky commented at 11:52 pm on January 29, 2021: member

    This change is needed to fix the rpc_help.py test failing in #10102: https://cirrus-ci.com/task/5469433013469184?command=ci#L2275

    The CRPCTable::dumpArgMap method currently works by casting RPC unique_id integer field to a function pointer, and then calling it. The unique_id field wasn’t supposed to be used this way (it’s meant to be used to detect RPC aliases) and as a result, this code segfaults in the rpc_help.py test in multiprocess PR #10102 because wallet RPC functions aren’t directly accessible from the node process.

    Fix this by adding a new GET_ARGS RPC request mode to retrieve argument information similar to the way the GET_HELP mode retrieves help information.

    This PR is part of the process separation project.

  2. refactor: Replace JSONRPCRequest fHelp field with mode field 
    No change in behavior
    6158a6d397
  3. refactor: Add RPC server ExecuteCommands function 
    No change in behavior. New function is split from CRPCTable::execute and
used in the next commit.
    14f3d9b908
  4. Remove pointer cast in CRPCTable::dumpArgMap 
    CRPCTable::dumpArgMap currently works by casting RPC command unique_id
integer field to a function pointer, and then calling the function. The
unique_id field wasn't supposed to be used this way (it's meant to be
used to detect RPC aliases), and this code segfaults in the rpc_help.py
test in multiprocess PR https://github.com/bitcoin/bitcoin/pull/10102
because wallet RPC functions aren't directly accessible from the node
process.

Fix this by adding a new GET_ARGS request mode to retrieve argument
information similar to the way the GET_HELP mode retrieves help
information.
    9048c58e10
  5. ryanofsky added this to the "In progress" column in a project
  6. DrahtBot added the label RPC/REST/ZMQ on Jan 30, 2021
  7. DrahtBot added the label Wallet on Jan 30, 2021
  8. MarcoFalke commented at 7:39 am on January 30, 2021: member
    Thanks, Concept ACK
  9. MarcoFalke removed the label Wallet on Jan 31, 2021
  10. MarcoFalke added the label Refactoring on Jan 31, 2021
  11. in src/rpc/request.h:37 in 8a6e870cbb outdated 
    33@@ -34,19 +34,19 @@ class JSONRPCRequest
34     UniValue id;
35     std::string strMethod;
36     UniValue params;
37-    bool fHelp;
38+    enum { EXECUTE, GET_HELP, GET_ARGS } mode = EXECUTE;
    laanwj commented at 1:02 pm on February 1, 2021:
    I don’t think it strictly matters in this case because no other values of this type are ever instantiated outside of this class, but debugging experience has made me prefer named types to anonymous ones.
    ryanofsky commented at 6:48 pm on February 20, 2021:

    re: #21035 (review)

    I don’t think it strictly matters in this case because no other values of this type are ever instantiated outside of this class, but debugging experience has made me prefer named types to anonymous ones.

    I slightly prefer not defining an name that’s never used for a type that’s never referenced, but happy to apply a suggestion if you have a specific one. Maybe enum Mode { EXECUTE, GET_HELP, GET_ARGS } mode = EXECUTE;?

    laanwj commented at 9:55 am on February 23, 2021:
    Sounds good to me.
    ryanofsky commented at 1:22 am on March 9, 2021:

    re: #21035 (review)

    Sounds good to me.

    Updated

  12. in src/rpc/server.cpp:492 in 8a6e870cbb outdated 
    500+    request.params = UniValue();
501+
502     UniValue ret{UniValue::VARR};
503     for (const auto& cmd : mapCommands) {
504-        for (const auto& c : cmd.second) {
505-            const auto help = RpcMethodFnType(c->unique_id)();
    laanwj commented at 1:08 pm on February 1, 2021:
    Holy crap, I’m happy to get rid of this.
    MarcoFalke commented at 10:23 am on February 2, 2021:
    I was just testing our review process, obviously :sweat_smile:
  13. laanwj commented at 1:08 pm on February 1, 2021: member
    Concept ACK.
  14. fanquake deleted a comment on Feb 2, 2021
  15. in src/rpc/util.h:340 in 8a6e870cbb outdated 
    337@@ -338,7 +338,7 @@ class RPCHelpMan
338     UniValue HandleRequest(const JSONRPCRequest& request);
339     std::string ToString() const;
340     /** Append the named args that need to be converted from string to another JSON type */
    MarcoFalke commented at 9:58 am on February 23, 2021:
    Nit in the last commit: s/Append/Return/
    ryanofsky commented at 1:19 am on March 9, 2021:

    re: #21035 (review)

    Nit in the last commit: s/Append/Return/

    Thanks, updated

  16. in src/rpc/server.cpp:499 in 8a6e870cbb outdated 
    491@@ -492,13 +492,19 @@ std::vector<std::string> CRPCTable::listCommands() const
492     return commandList;
493 }
494 
495-UniValue CRPCTable::dumpArgMap() const
496+UniValue CRPCTable::dumpArgMap(const JSONRPCRequest& args_request) const
497 {
498+    JSONRPCRequest request(args_request);
499+    request.mode = JSONRPCRequest::GET_ARGS;
500+    request.params = UniValue();
    MarcoFalke commented at 10:00 am on February 23, 2021:
    question: Any reason to reset those?
    ryanofsky commented at 1:19 am on March 9, 2021:

    re: #21035 (review)

    question: Any reason to reset those?

    Not really, removed this. This was copied from CRPCTable::help but that method sets method and params fields together, and here it doesn’t make sense to set just the one field.

  17. MarcoFalke approved
  18. MarcoFalke commented at 10:00 am on February 23, 2021: member

    review ACK 8a6e870cbb34586958ca4c17037d41df800a26ef 🥅

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 8a6e870cbb34586958ca4c17037d41df800a26ef 🥅
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUirIAv/aoT2DpLO8cDSa87LK8Wr/CEGuuvUnfZ0B5s1r34wjIxMUI+XWmwV+Hhu
 8UXTQT0P0JY84TXxaYiXYVqH27qiVU6N66AXTi4s6X4W0Ndjl6ZUo9aNYScahyDgX
 9iMbw4TiGrPzjhNH0kxA5De5Q/Frs90eM5kUGzlahVPwyNg+7RI5kZ7Gqi7yVRouX
10WN+pjxRK7RXZTlZDnFQz9vbZQLbMexnnoRA5RZ/7h1mul1PbaEWjCBDybVHncRkA
11a0+WKKA9yPDnLpn26o2/kOu2ChQ+7E4FraE1WIM7+iy6p7JnVtMDlk1VtfvZek1d
12XL9mV6E6aUrqOnTjfssk8yl6VLmjFpiDCW+fYuaoxfBPjvHRTTOqGzKnV+0s8Voh
13IJdfqoIkkCp9GMoQHn2PPWliq99LRGjWlZsYxSer/oUwE6BlZaKqf3cprN1d6DbI
140MmK4Yxpte15H2WuoDG/1IiwFjIQxM8rot7Y1pwOblZMFmi6EeV4tlOYZnnBBqLw
15tieUn9Ym
16=gOJt
17-----END PGP SIGNATURE-----

    Timestamp of file with hash a3e1a8004872dac4b7123281ccd1da57e1dcc6b23121c63d3ced4954ec92a7cb -

  19. DrahtBot commented at 12:27 pm on March 5, 2021: member

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

    Conflicts

    No conflicts as of last run.

  20. ryanofsky force-pushed on Mar 9, 2021
  21. ryanofsky commented at 1:30 am on March 9, 2021: member
    Updated 8a6e870cbb34586958ca4c17037d41df800a26ef -> 9048c58e10841d9e1d709c0a325dd14684cec325 (pr/argmap.1 -> pr/argmap.2, compare) with review suggestions
  22. MarcoFalke commented at 9:09 am on March 15, 2021: member

    re-ACK 9048c58e10841d9e1d709c0a325dd14684cec325 👑

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 9048c58e10841d9e1d709c0a325dd14684cec325 👑
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjdQQv7BRr4swBySG+3b1lpJ+lf203nq9gecZulk16CsimVw5AzWpME6/Wi4bQC
 85aqXT6vaD10E9U6PMcjjg+4lU610LAnJyfBBWwmH/mjP5TL44OQmX7iJpTC4oElf
 9YIYogyNWGE/s2pQX5EyD+M9KO0FUpYv930su/0PbkTCPDhMi/GDNQXi2woF+fC+F
10r2Fcbj2Ngb5hjDp6aIDABKjDqXJIJyAu9oSHCPGkX9pUzOFZMGn+626ItNwU+wG8
116s3JfizTZfCnb87Xe9LVhWPut7m8v0VKSBplyqIWtQJWo4rATcO6wkm3AaIFJfnX
12Olt9P/RWLzwaYz/K6rNkDpN/PQzUgZguutXeUjAv+np8kP0tzFHfuvbfoJw2j3rQ
13z1AVpn3JELnUr3FhIsi9saGuLiU716xphcumOzJxyz09BJxn8T4Xg1sToUxKfLR6
146OP40JYqwXXgY4jjCHbRTbs8/xv4Q9h5/tviL16Jwv5S50MtVf58QEsvBBzoOqPh
15+lPTf56z
16=xSEe
17-----END PGP SIGNATURE-----

    Timestamp of file with hash 1b64b38688b2d7ed0c7469d91d3d76faf4fbfd0c3674dd99533efa3158ae6f63 -

  23. MarcoFalke merged this on Mar 15, 2021
  24. MarcoFalke closed this on Mar 15, 2021
  25. sidhujag referenced this in commit bf84f721b2 on Mar 15, 2021
  26. fanquake moved this from the "In progress" to the "Done" column in a project
  27. Fabcien referenced this in commit a263b60519 on Jan 5, 2022
  28. Fabcien referenced this in commit 8e70b7afa5 on Jan 5, 2022
  29. Fabcien referenced this in commit 64ea6e739c on Jan 5, 2022
  30. DrahtBot locked this on Aug 16, 2022


ryanofsky MarcoFalke laanwj DrahtBot

Labels
Refactoring 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-06-29 07:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me