rpc: Add RPCContext #20017

pull promag wants to merge 2 commits into bitcoin:master from promag:2020-09-rpcontext changing 3 files +102 −6
  1. promag commented at 6:54 pm on September 25, 2020: member

    This change allows to do

    0[&](const RPCContext& ctx) -> UniValue
    

    instead of

    0[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
    

    So RPCContext ties RPCHelpMan and JSONRPCRequest. Then ctx is used to get actual parameter values. For instance, before:

    0int timeout = 0;
    1if (!request.params[0].isNull())	
    2    timeout = request.params[0].get_int();
    

    and now

    0int timeout = ctx.param<int>(0);
    

    Not that the default value defined in the RPC spec is used.

    It is also possible to iterate an array parameter:

    0    std::set<std::string> stats;
    1    ctx.param(1).forEach([&](const UniValue& stat) {
    2        stats.insert(stat.get_str());
    3    });
    

    Or even do custom parameter handling:

    0    int verbosity = ctx.param(1, [](const UniValue& param) -> int {
    1        if (param.isNull()) return 1;
    2        if (param.isNum()) return param.get_int();
    3        return param.get_bool() ? 1 : 0;
    4    });
    
  2. promag commented at 6:59 pm on September 25, 2020: member

    Here’s more goals of this change:

    • get param by name, like ctx.param<bool>("involves_watchonly") and have it with the correct format and make it non-breaking change,
    • try to use the default value specified in RPCArg (Done ✓)
    • support chunked response - don’t rely on the return UniValue
    • this simplifies code review as request.param is no longer used in the new variant
    • detect unused arguments @MarcoFalke @kallewoof
  3. DrahtBot added the label RPC/REST/ZMQ on Sep 25, 2020
  4. promag force-pushed on Sep 25, 2020
  5. kallewoof commented at 6:35 am on September 30, 2020: member
  6. in src/rpc/util.h:337 in c6ecc3587e outdated
    328@@ -329,18 +329,46 @@ struct RPCExamples {
    329     std::string ToDescriptionString() const;
    330 };
    331 
    332+class RPCHelpMan;
    333+class RPCContext
    334+{
    335+public:
    336+    const RPCHelpMan& m_helpman;
    337+    const JSONRPCRequest& m_request;
    


    kallewoof commented at 8:32 am on September 30, 2020:
    I believe the errors appearing are due to one or both of these going out of scope before the function call happens.

    promag commented at 0:08 am on October 1, 2020:
    Fixed.
  7. promag commented at 8:35 am on September 30, 2020: member
    @kallewoof thanks for taking a look! I’ll wait for more concept acks before going forward. Regarding your suggestion to use operator(), I tend to see it used for functors. In this case, to get param values, it’s not that expressiv.
  8. kallewoof commented at 8:47 am on September 30, 2020: member

    Yeah, it may be confusing too. I do think we can afford to use one function that takes a null default, though. Overhead negligible. Gotcha on the concept ACKs.

    Edit: alternatively, operator[] for the no default case, and ParamOrDefault() for the default case..

    Edit again: I dug some more, and it doesn’t seem like the RPCHelpMan or JSONRPCRequest are being deallocated, but some memory violation is happening, for sure. Anyway, will get back when we know if this is the way.

  9. DrahtBot commented at 7:36 pm on September 30, 2020: contributor

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #25093 (rpc: Check for omitted, but required parameters by MarcoFalke)

    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.

  10. promag force-pushed on Sep 30, 2020
  11. promag force-pushed on Oct 1, 2020
  12. promag commented at 1:02 am on October 1, 2020: member
    Updated with IMO a better API. @ryanofsky care to take a look too?
  13. in src/rpc/util.h:364 in 6e1fecd39f outdated
    359+};
    360+
    361+template <> RPCParam<bool>::operator bool() const { return value.get_bool(); }
    362+template <> RPCParam<int>::operator int() const { return value.get_int(); }
    363+template <> RPCParam<std::string>::operator std::string() const { return value.get_str(); }
    364+template <> RPCParam<uint256>::operator uint256() const { return ParseHashV(value, arg.GetName()); }
    


    promag commented at 1:04 am on October 1, 2020:

    6e1fecd39fdadbfd1ea3a2eb3e2c74db43cecb65

    This allows to throw with correct parameter name in ParseHashV.

  14. promag force-pushed on Oct 1, 2020
  15. kallewoof commented at 7:17 am on October 1, 2020: member
    @promag Nice. I can’t figure out what was wrong with the old code. Care to explain?
  16. promag commented at 7:49 am on October 1, 2020: member

    @promag Nice. I can’t figure out what was wrong with the old code. Care to explain?

    Before fun was captured by reference, and it kabooms when called.

    0    RPCHelpMan(std::string name, std::string description, std::vector<RPCArg> args, RPCResults results, RPCExamples examples, RPCLegacyMethodImpl fun)
    1        : RPCHelpMan(std::move(name), std::move(description), std::move(args), std::move(results), std::move(examples), [&](const RPCContext& ctx) -> UniValue {
    2            return fun(ctx.m_helpman, ctx.m_request);
    3        })
    4    {}
    
  17. promag marked this as ready for review on Oct 1, 2020
  18. in src/rpc/blockchain.cpp:340 in a25df4d924 outdated
    347-
    348-    int height = request.params[0].get_int();
    349-
    350-    if (!request.params[1].isNull())
    351-        timeout = request.params[1].get_int();
    352+    int height = ctx.param<int>(0);
    


    kallewoof commented at 8:47 am on October 1, 2020:
    This is a behavior change, as height is not optional; it would throw before, and will silently use height 0 now.

    promag commented at 8:53 am on October 1, 2020:

    and will silently use height 0 now.

    How? param<int>(index) results in the same code as before.


    kallewoof commented at 8:57 am on October 1, 2020:
    I thought the 0 was a default value, not the index. You’re right.
  19. in src/rpc/blockchain.cpp:598 in a25df4d924 outdated
    602 {
    603-    bool fVerbose = false;
    604-    if (!request.params[1].isNull())
    605-        fVerbose = request.params[1].get_bool();
    606+    bool fVerbose = ctx.param<bool>(1).defaults(false);
    607+    uint256 hash = ctx.param<uint256>(0);
    


    kallewoof commented at 8:48 am on October 1, 2020:
    ~Same as elsewhere; this is not an optional value, so it shouldn’t silently pass. (I won’t mention these anymore, but there are others.)~

    LarryRuane commented at 2:09 am on December 18, 2020:
    nit: reverse the order of these two lines. (Same below, getmempooldescendants.)
  20. maflcko commented at 11:43 am on October 1, 2020: member
    Concept ACK. Would be nice if the default was already filled in by rpc help man. ;)
  21. promag commented at 2:58 pm on October 1, 2020: member

    @MarcoFalke yes, it was already suggested in #20017 (comment) but I think that can be done next.

    I also wonder if this should be an exhaustive change, at the moment I’m inclined to do it.

  22. maflcko commented at 3:00 pm on October 1, 2020: member
    The benefit of doing it in one step would be to avoid changing the same line of code twice
  23. promag force-pushed on Oct 1, 2020
  24. promag commented at 5:23 pm on October 1, 2020: member
    See last commit @MarcoFalke.
  25. maflcko commented at 9:43 am on December 3, 2020: member
    Concept ACK with last commit included
  26. in src/rpc/util.cpp:493 in 95453d72b0 outdated
    488+template <> RPCParam<bool>::operator bool() const { return value.get_bool(); }
    489+template <> RPCParam<int>::operator int() const { return value.get_int(); }
    490+template <> RPCParam<std::string>::operator std::string() const
    491+{
    492+    if (!value.isNull()) return value.get_str();
    493+    CHECK_NONFATAL(arg.m_fallback.which() == 1);
    


    LarryRuane commented at 7:07 pm on December 21, 2020:
    0    CHECK_NONFATAL(arg.m_fallback.type() == typeid(std::string));
    

    and the two other places within this file that call m_fallback.which(). However, you’re not changing those other two places in this PR, so if you don’t want to change them (personally I would favor doing so, as this is much easier to understand), you may want to not take this suggestion.

  27. LarryRuane commented at 7:24 pm on December 21, 2020: contributor

    Concept, review, and tested ACK, this is nice! I was curious to see if the mini-API you’ve created here would be sufficient for other RPC code, so I made the changes to rpc/misc.cpp here: https://github.com/LarryRuane/bitcoin/commit/9270424ea70f62b733c0de780c022821f32b7d4f

    As you can see, it went well, there were only a couple of places that weren’t completely obvious. (I made a few small extensions to the API you’ve created that I think are slight improvements.)

  28. ajtowns commented at 8:39 am on January 13, 2021: contributor
    Concept ACK, particularly with pulling defaults from the rpc help.
  29. DrahtBot added the label Needs rebase on Jan 21, 2021
  30. promag commented at 3:46 pm on April 11, 2021: member
    I guess it’s time to rebase and push this forward?
  31. fanquake added the label Waiting for author on Apr 12, 2021
  32. fanquake commented at 0:43 am on April 12, 2021: member

    I guess it’s time to rebase and push this forward?

    Yes, please do.

  33. promag force-pushed on Apr 13, 2021
  34. DrahtBot removed the label Needs rebase on Apr 14, 2021
  35. promag force-pushed on Apr 14, 2021
  36. promag renamed this:
    rpc: Add RPCContext
    WIP: rpc: Add RPCContext
    on Apr 14, 2021
  37. promag renamed this:
    WIP: rpc: Add RPCContext
    rpc: Add RPCContext
    on Apr 14, 2021
  38. promag marked this as a draft on Apr 14, 2021
  39. promag force-pushed on Apr 14, 2021
  40. maflcko removed the label Waiting for author on Apr 19, 2021
  41. maflcko referenced this in commit d4300a10dd on Apr 19, 2021
  42. promag force-pushed on Apr 19, 2021
  43. promag commented at 10:31 am on April 19, 2021: member

    Rebased now that #21679 is merged.

    Not sure what is the best approach going forward, whether this should be an exhaustive change or not. I’ve included a commit to just change getnetworkhashps RPC to serve as example.

  44. promag force-pushed on Apr 19, 2021
  45. promag force-pushed on Apr 19, 2021
  46. promag marked this as ready for review on Apr 20, 2021
  47. maflcko commented at 10:29 am on April 21, 2021: member
    Concept ACK
  48. kallewoof commented at 10:10 am on August 3, 2021: member
    reACK f473a17740f57eaecd884b59333e6e44e1695286
  49. LarryRuane commented at 2:15 am on August 4, 2021: contributor
    reACK f473a17740f57eaecd884b59333e6e44e1695286
  50. in src/rpc/util.h:404 in f473a17740 outdated
    399+
    400+class RPCContext
    401+{
    402+public:
    403+    const RPCHelpMan& helpman;
    404+    const JSONRPCRequest& request;
    


    maflcko commented at 2:58 pm on August 4, 2021:

    62047855691e1f1e07c99db259177b2fdb82cb2c: It looks like you are using this like a struct (without m_ prefix and scoped access ctx.request), so it could make sense to make it a struct. Also, with RPCHelpMan being renamed to RPCMethod (https://github.com/bitcoin/bitcoin/pull/19386#discussion_r447743298), it could make sese to name the variable appropriately:

    0struct RPCContext {
    1    const RPCHelpMan& method;
    2    const JSONRPCRequest& request;
    
  51. in src/rpc/util.h:348 in f473a17740 outdated
    343+    std::optional<T> m_defaults = {};
    344+    RPCParam<T>& defaults(const T& defaults) {
    345+        CHECK_NONFATAL(m_arg.m_fallback.index() != 2);
    346+        m_defaults = defaults;
    347+        return *this;
    348+    }
    


    maflcko commented at 3:12 pm on August 4, 2021:

    I don’t understand how any of this works. I removed this and everything compiled just fine:

     0diff --git a/src/rpc/util.h b/src/rpc/util.h
     1index f7a1acd1b5..1d5b7fc318 100644
     2--- a/src/rpc/util.h
     3+++ b/src/rpc/util.h
     4@@ -348,11 +348,6 @@ struct RPCParam
     5     const RPCArg& m_arg;
     6     const UniValue& m_value;
     7     std::optional<T> m_defaults = {};
     8-    RPCParam<T>& defaults(const T& defaults) {
     9-        CHECK_NONFATAL(m_arg.m_fallback.index() != 2);
    10-        m_defaults = defaults;
    11-        return *this;
    12-    }
    13     T get() const {
    14         if (!m_value.isNull()) return value(m_value);
    15         if (m_defaults) return *m_defaults;
    

    I think it would make sense to split this into at least two commits:

    • First one to introduce RPCContext
    • Second one to introduce RPCParam

    Finally, would be nice to use clang-format on new code.


    promag commented at 6:05 pm on August 29, 2021:

    maflcko commented at 9:01 am on August 30, 2021:
    Then maybe remove the dead, (not even compiled) code?

    maflcko commented at 5:54 am on September 28, 2021:
    @promag are you still working on this?

    promag commented at 6:24 am on September 28, 2021:
    I’ll rebase and remove dead code.
  52. maflcko commented at 3:13 pm on August 4, 2021: member
    Concept ACK
  53. rpc: Introduce RPCContext and RPCParam e96155f144
  54. rpc: Refactor getnetworkhashps to use RPCContext 7a7b05079d
  55. promag force-pushed on Sep 29, 2021
  56. promag commented at 8:01 am on September 30, 2021: member
    @MarcoFalke updated, I think fail is unrelated.
  57. fanquake commented at 12:34 pm on April 20, 2022: member
    Concept ACK
  58. in src/rpc/util.h:361 in e96155f144 outdated
    356+    operator T() const { return get(); }
    357+    bool isNull() const { return value.isNull(); }
    358+    bool isNum() const { return value.isNum(); }
    359+    bool isStr() const { return value.isStr(); }
    360+    template <typename F>
    361+    void forEach(F fn) const {
    


    ajtowns commented at 0:27 am on April 21, 2022:
    Any reason not to use UpperCamelCase?
  59. in src/rpc/util.h:368 in e96155f144 outdated
    363+        for (const UniValue& elem : value.get_array().getValues()) {
    364+            fn(elem);
    365+        }
    366+    }
    367+private:
    368+    T convert(const UniValue& value) const;
    


    ajtowns commented at 0:27 am on April 21, 2022:
    Parameter name shadows member variable. Shouldn’t this method be static anyway?
  60. in src/rpc/util.h:353 in e96155f144 outdated
    348+    const RPCArg& arg;
    349+    const UniValue& value;
    350+    T get() const {
    351+        if (!value.isNull()) return convert(value);
    352+        // if (m_defaults) return *m_defaults;
    353+        CHECK_NONFATAL(arg.m_fallback.index() == 2);
    


    ajtowns commented at 0:31 am on April 21, 2022:
    CHECK_NONFATAL(std::holds_alternative<Default>(arg.m_fallback)) ?
  61. in src/rpc/util.h:398 in e96155f144 outdated
    394@@ -365,6 +395,25 @@ class RPCHelpMan
    395     const std::vector<RPCArg> m_args;
    396     const RPCResults m_results;
    397     const RPCExamples m_examples;
    398+    friend struct RPCContext;
    


    ajtowns commented at 0:48 am on April 21, 2022:

    Rather than having a friend accessing this class’s privates directly, why not have RPCHelpMan be able to create a parameter directly and just call that from RPCContext:

     0class RPCHelpMan
     1{
     2    ...
     3    template <typename T>
     4    RPCParam<T> GetParam(size_t index, const JSONRPCRequest& request) const
     5    {
     6        return {m_args[index], request.params[index]};
     7    }
     8    ...
     9};
    10
    11class RPCContext
    12{
    13...
    14    template <typename T=UniValue>
    15    RPCParam<T> param(size_t index) const
    16    {
    17        return method.GetParam<T>(index, request);
    18    }
    19};
    
  62. in src/rpc/util.h:409 in e96155f144 outdated
    404+    const JSONRPCRequest& request;
    405+
    406+    template <typename T=UniValue>
    407+    RPCParam<T> param(size_t index) const
    408+    {
    409+        return {method.m_args[index], request.params[index]};
    


    ajtowns commented at 0:48 am on April 21, 2022:
    CHECK_NONFATAL(index < m_args.size()); ?

    ajtowns commented at 1:08 am on April 21, 2022:
    When constructing an RPCParam<T> wouldn’t it be worth checking that T aligns with method.m_args[index].m_type along similar lines to RPCResult::MatchesType ?
  63. in src/rpc/util.h:416 in e96155f144 outdated
    411+
    412+    template <typename F>
    413+    auto param(size_t index, F fn) const -> decltype(fn(UniValue{}))
    414+    {
    415+        return fn(request.params[index]);
    416+    }
    


    ajtowns commented at 0:50 am on April 21, 2022:
    Why would you call ctx.param(i, fn) instead of fn(ctx.param(i))ctx.param(i) defaults to being UniValue and automatically converts to UniValue, no?
  64. in src/rpc/util.h:351 in e96155f144 outdated
    346+struct RPCParam
    347+{
    348+    const RPCArg& arg;
    349+    const UniValue& value;
    350+    T get() const {
    351+        if (!value.isNull()) return convert(value);
    


    ajtowns commented at 0:57 am on April 21, 2022:
    0if constexpr (std::is_same_v<T, UniValue>()) return convert(value);
    

    would allow you to get a VNULL value out cleanly if you’re treating the param as UniValue

  65. ajtowns commented at 1:09 am on April 21, 2022: contributor
    Approach ACK
  66. DrahtBot added the label Needs rebase on May 19, 2022
  67. DrahtBot commented at 6:10 am on May 19, 2022: contributor

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

  68. achow101 commented at 6:18 pm on October 12, 2022: member
    Are you still working on this?
  69. achow101 added the label Up for grabs on Oct 12, 2022
  70. fanquake closed this on Oct 21, 2022

  71. maflcko removed the label Up for grabs on Aug 7, 2023
  72. maflcko removed the label Needs rebase on Aug 7, 2023
  73. maflcko commented at 11:17 am on August 7, 2023: member
    I took the concept from here (Thanks!), but implemented it differently (in just three lines of code in the header file). See https://github.com/bitcoin/bitcoin/pull/28230
  74. vijaydasmp referenced this in commit bafcb123df on Apr 22, 2024
  75. vijaydasmp referenced this in commit 7242c3ca98 on Jul 6, 2024
  76. vijaydasmp referenced this in commit ae93d8b079 on Jul 6, 2024
  77. bitcoin locked this on Aug 6, 2024

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-21 12:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me