rpc: Add RPCContext #20017

promag commented at 6:54 PM on September 25, 2020: member

This change allows to do

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

instead of

[&](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:

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

and now

int 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:

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

Or even do custom parameter handling:

    int verbosity = ctx.param(1, [](const UniValue& param) -> int {
        if (param.isNull()) return 1;
        if (param.isNum()) return param.get_int();
        return param.get_bool() ? 1 : 0;
    });

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

DrahtBot added the label RPC/REST/ZMQ on Sep 25, 2020

promag force-pushed on Sep 25, 2020

kallewoof commented at 6:35 AM on September 30, 2020: member

Concept ACK

I think this looks nicer btw: https://github.com/kallewoof/bitcoin/commit/fe349cf4410892b62e98c4f119ac005254b4292e

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 12:08 AM on October 1, 2020:

Fixed.

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.

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.

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.

promag force-pushed on Sep 30, 2020

promag force-pushed on Oct 1, 2020

promag commented at 1:02 AM on October 1, 2020: member

Updated with IMO a better API. @ryanofsky care to take a look too?

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.

promag force-pushed on Oct 1, 2020

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?

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.

    RPCHelpMan(std::string name, std::string description, std::vector<RPCArg> args, RPCResults results, RPCExamples examples, RPCLegacyMethodImpl fun)
        : RPCHelpMan(std::move(name), std::move(description), std::move(args), std::move(results), std::move(examples), [&](const RPCContext& ctx) -> UniValue {
            return fun(ctx.m_helpman, ctx.m_request);
        })
    {}

promag marked this as ready for review on Oct 1, 2020

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.

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.)

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. ;)

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.

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

promag force-pushed on Oct 1, 2020

promag commented at 5:23 PM on October 1, 2020: member

See last commit @MarcoFalke.

maflcko commented at 9:43 AM on December 3, 2020: member

Concept ACK with last commit included

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:

    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.

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.)

ajtowns commented at 8:39 AM on January 13, 2021: contributor

Concept ACK, particularly with pulling defaults from the rpc help.

DrahtBot added the label Needs rebase on Jan 21, 2021

promag commented at 3:46 PM on April 11, 2021: member

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

fanquake added the label Waiting for author on Apr 12, 2021

fanquake commented at 12:43 AM on April 12, 2021: member

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

Yes, please do.

promag force-pushed on Apr 13, 2021

DrahtBot removed the label Needs rebase on Apr 14, 2021

promag force-pushed on Apr 14, 2021

promag renamed this:
~~rpc: Add RPCContext~~
WIP: rpc: Add RPCContext
on Apr 14, 2021

promag renamed this:
~~WIP: rpc: Add RPCContext~~
rpc: Add RPCContext
on Apr 14, 2021

promag marked this as a draft on Apr 14, 2021

promag force-pushed on Apr 14, 2021

maflcko removed the label Waiting for author on Apr 19, 2021

maflcko referenced this in commit d4300a10dd on Apr 19, 2021

promag force-pushed on Apr 19, 2021

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.

promag force-pushed on Apr 19, 2021

promag marked this as ready for review on Apr 20, 2021

maflcko commented at 10:29 AM on April 21, 2021: member

Concept ACK

kallewoof commented at 10:10 AM on August 3, 2021: member

reACK f473a17740f57eaecd884b59333e6e44e1695286

LarryRuane commented at 2:15 AM on August 4, 2021: contributor

reACK f473a17740f57eaecd884b59333e6e44e1695286

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:

struct RPCContext {
    const RPCHelpMan& method;
    const JSONRPCRequest& request;

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:

diff --git a/src/rpc/util.h b/src/rpc/util.h
index f7a1acd1b5..1d5b7fc318 100644
--- a/src/rpc/util.h
+++ b/src/rpc/util.h
@@ -348,11 +348,6 @@ struct RPCParam
     const RPCArg& m_arg;
     const UniValue& m_value;
     std::optional<T> m_defaults = {};
-    RPCParam<T>& defaults(const T& defaults) {
-        CHECK_NONFATAL(m_arg.m_fallback.index() != 2);
-        m_defaults = defaults;
-        return *this;
-    }
     T get() const {
         if (!m_value.isNull()) return value(m_value);
         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:

https://github.com/bitcoin/bitcoin/pull/20017/files#r682710976 @MarcoFalke yup, not used in this branch.

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.

maflcko commented at 3:13 PM on August 4, 2021: member

Concept ACK

rpc: Introduce RPCContext and RPCParam e96155f144

rpc: Refactor getnetworkhashps to use RPCContext 7a7b05079d

promag force-pushed on Sep 29, 2021

promag commented at 8:01 AM on September 30, 2021: member

@MarcoFalke updated, I think fail is unrelated.

fanquake commented at 12:34 PM on April 20, 2022: member

Concept ACK

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 12:27 AM on April 21, 2022:

Any reason not to use UpperCamelCase?

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 12:27 AM on April 21, 2022:

Parameter name shadows member variable. Shouldn't this method be static anyway?

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 12:31 AM on April 21, 2022:

CHECK_NONFATAL(std::holds_alternative<Default>(arg.m_fallback)) ?

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

class RPCHelpMan
{
    ...
    template <typename T>
    RPCParam<T> GetParam(size_t index, const JSONRPCRequest& request) const
    {
        return {m_args[index], request.params[index]};
    }
    ...
};

class RPCContext
{
...
    template <typename T=UniValue>
    RPCParam<T> param(size_t index) const
    {
        return method.GetParam<T>(index, request);
    }
};

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 12: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 ?

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 12: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?

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 12:57 AM on April 21, 2022:

if 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

ajtowns commented at 1:09 AM on April 21, 2022: contributor

Approach ACK

DrahtBot added the label Needs rebase on May 19, 2022

DrahtBot commented at 6:10 AM on May 19, 2022: contributor

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

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>

achow101 commented at 6:18 PM on October 12, 2022: member

Are you still working on this?

achow101 added the label Up for grabs on Oct 12, 2022

fanquake closed this on Oct 21, 2022

maflcko removed the label Up for grabs on Aug 7, 2023

maflcko removed the label Needs rebase on Aug 7, 2023

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

vijaydasmp referenced this in commit bafcb123df on Apr 22, 2024

vijaydasmp referenced this in commit 7242c3ca98 on Jul 6, 2024

vijaydasmp referenced this in commit ae93d8b079 on Jul 6, 2024

bitcoin locked this on Aug 6, 2024