rpc: Add RPCContext #20017

promag · 2020-09-25T18:54:49Z

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 · 2020-09-25T18:59:39Z

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

kallewoof · 2020-09-30T06:35:46Z

Concept ACK

I think this looks nicer btw: kallewoof@fe349cf

src/rpc/util.h

promag · 2020-09-30T08:35:17Z

@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 · 2020-09-30T08:47:48Z

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 · 2020-09-30T19:36:07Z

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 · 2020-10-01T01:02:23Z

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

src/rpc/util.h

kallewoof · 2020-10-01T07:17:39Z

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

promag · 2020-10-01T07:49:36Z

@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);
        })
    {}

src/rpc/blockchain.cpp

maflcko · 2020-10-01T11:43:47Z

Concept ACK. Would be nice if the default was already filled in by rpc help man. ;)

promag · 2020-10-01T14:58:25Z

@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 · 2020-10-01T15:00:11Z

The benefit of doing it in one step would be to avoid changing the same line of code twice

promag · 2020-10-01T17:23:54Z

See last commit @MarcoFalke.

promag · 2021-04-19T10:31:32Z

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.

maflcko · 2021-04-21T10:29:39Z

Concept ACK

kallewoof

reACK f473a17

LarryRuane · 2021-08-04T02:15:41Z

reACK f473a17

maflcko

Concept ACK

maflcko · 2021-08-04T14:58:28Z

src/rpc/util.h

+class RPCContext
+{
+public:
+    const RPCHelpMan& helpman;
+    const JSONRPCRequest& request;


6204785: 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 (#19386 (comment)), it could make sese to name the variable appropriately:

Suggested change

class RPCContext

{

public:

const RPCHelpMan& helpman;

const JSONRPCRequest& request;

struct RPCContext {

const RPCHelpMan& method;

const JSONRPCRequest& request;

maflcko · 2021-08-04T15:12:03Z

src/rpc/util.h

+        CHECK_NONFATAL(m_arg.m_fallback.index() != 2);
+        m_defaults = defaults;
+        return *this;
+    }


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.

https://github.com/bitcoin/bitcoin/pull/20017/files#r682710976

@MarcoFalke yup, not used in this branch.

Then maybe remove the dead, (not even compiled) code?

@promag are you still working on this?

I’ll rebase and remove dead code.

promag · 2021-09-30T08:01:36Z

@MarcoFalke updated, I think fail is unrelated.

fanquake · 2022-04-20T12:34:59Z

Concept ACK

ajtowns

Approach ACK

ajtowns · 2022-04-21T00:27:01Z

src/rpc/util.h

+    bool isNum() const { return value.isNum(); }
+    bool isStr() const { return value.isStr(); }
+    template <typename F>
+    void forEach(F fn) const {


Any reason not to use UpperCamelCase?

ajtowns · 2022-04-21T00:27:24Z

src/rpc/util.h

+        }
+    }
+private:
+    T convert(const UniValue& value) const;


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

ajtowns · 2022-04-21T00:31:04Z

src/rpc/util.h

+    T get() const {
+        if (!value.isNull()) return convert(value);
+        // if (m_defaults) return *m_defaults;
+        CHECK_NONFATAL(arg.m_fallback.index() == 2);


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

ajtowns · 2022-04-21T00:48:10Z

src/rpc/util.h

@@ -365,6 +395,25 @@ class RPCHelpMan
    const std::vector<RPCArg> m_args;
    const RPCResults m_results;
    const RPCExamples m_examples;
+    friend struct RPCContext;


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

ajtowns · 2022-04-21T00:48:25Z

src/rpc/util.h

+    template <typename T=UniValue>
+    RPCParam<T> param(size_t index) const
+    {
+        return {method.m_args[index], request.params[index]};


CHECK_NONFATAL(index < m_args.size()); ?

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 ?

ajtowns · 2022-04-21T00:50:38Z

src/rpc/util.h

+    auto param(size_t index, F fn) const -> decltype(fn(UniValue{}))
+    {
+        return fn(request.params[index]);
+    }


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?

ajtowns · 2022-04-21T00:57:34Z

src/rpc/util.h

+    const RPCArg& arg;
+    const UniValue& value;
+    T get() const {
+        if (!value.isNull()) return convert(value);


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

DrahtBot · 2022-05-19T06:10:43Z

🐙 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".}

achow101 · 2022-10-12T18:18:17Z

Are you still working on this?

maflcko · 2023-08-07T11:17:07Z

I took the concept from here (Thanks!), but implemented it differently (in just three lines of code in the header file). See #28230

bee56c7 rpc: Check default value type againts argument type (João Barbosa) f81ef43 rpc: Keep default argument value in correct type (João Barbosa) Pull request description: Store default values of RPC arguments in the corresponding type instead of a string. The value is then serialized when the help output is needed. This change simplifies bitcoin#20017. The following examples illustrates how to use the new `RPCArg::Default` and `RPCArg::DefaultHint`: ```diff - {"verbose", RPCArg::Type::BOOL, /* default */ "false", "True for a json object, false for array of transaction ids"} + {"verbose", RPCArg::Type::BOOL, RPCArg::Default(false), "True for a json object, false for array of transaction ids"} ``` ```diff - {"nblocks", RPCArg::Type::NUM, /* default */ "one month", "Size of the window in number of blocks"} + {"nblocks", RPCArg::Type::NUM, RPCArg::DefaultHint("one month"), "Size of the window in number of blocks"} ``` No behavior change is expected. ACKs for top commit: LarryRuane: ACK bee56c7 MarcoFalke: ACK bee56c7 🦅 Tree-SHA512: c47d78c918e996d36631d4ad3c933b270a34c5b446b8d736be94cf4a0a7b8c0e33d954149ec786cf9550639865b79deb6a130ad044de6030f95aac33f524293a

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

promag force-pushed the 2020-09-rpcontext branch from 24654fc to c6ecc35 Compare September 25, 2020 20:41

kallewoof mentioned this pull request Sep 30, 2020

rpc: normalize parameter names #20013

Closed

kallewoof reviewed Sep 30, 2020

View reviewed changes

src/rpc/util.h Outdated Show resolved Hide resolved

maflcko mentioned this pull request Sep 30, 2020

upstream: add and use defaulted getters to UniValue #20045

Closed

promag force-pushed the 2020-09-rpcontext branch 2 times, most recently from 1a07390 to 871487c Compare October 1, 2020 00:58

promag commented Oct 1, 2020

View reviewed changes

src/rpc/util.h Outdated Show resolved Hide resolved

This was referenced Oct 1, 2020

validation: UTXO snapshot activation #19806

Merged

rpc: Add dumpcoinstats #19792

Closed

promag force-pushed the 2020-09-rpcontext branch from 871487c to a25df4d Compare October 1, 2020 07:03

DrahtBot mentioned this pull request Oct 1, 2020

Coinstats Index #19521

Merged

promag marked this pull request as ready for review October 1, 2020 07:52

kallewoof reviewed Oct 1, 2020

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

promag force-pushed the 2020-09-rpcontext branch from 01a1554 to 95453d7 Compare October 1, 2020 17:00

DrahtBot mentioned this pull request Oct 15, 2020

tree-wide: De-globalize ChainstateManager #20158

Closed

12 tasks

DrahtBot mentioned this pull request Oct 31, 2020

Add missing thread safety annotations #20272

Closed

DrahtBot mentioned this pull request Nov 19, 2020

rpc: Remove duplicate name and argNames from CRPCCommand #20012

Merged

promag force-pushed the 2020-09-rpcontext branch 2 times, most recently from 97bc6af to f473a17 Compare April 19, 2021 10:34

promag marked this pull request as ready for review April 20, 2021 00:30

kallewoof reviewed Aug 3, 2021

View reviewed changes

maflcko reviewed Aug 4, 2021

View reviewed changes

maflcko mentioned this pull request Aug 25, 2021

RPC documentation cleanup ideas #22799

Open

promag added 2 commits September 29, 2021 21:22

rpc: Introduce RPCContext and RPCParam

e96155f

rpc: Refactor getnetworkhashps to use RPCContext

7a7b050

promag force-pushed the 2020-09-rpcontext branch from f473a17 to 7a7b050 Compare September 29, 2021 20:24

fanquake mentioned this pull request Apr 20, 2022

refactor: use ternary when creating RPC param variables #24741

Closed

ajtowns reviewed Apr 21, 2022

View reviewed changes

DrahtBot mentioned this pull request May 10, 2022

doc: Fix incorrect sendmany RPC doc #25093

Merged

DrahtBot added the Needs rebase label May 19, 2022

achow101 added the Up for grabs label Oct 12, 2022

fanquake closed this Oct 21, 2022

maflcko mentioned this pull request Jun 15, 2023

rpc: Be able to access RPC parameters by name #27788

Closed

maflcko removed Up for grabs Needs rebase labels Aug 7, 2023

bitcoin locked and limited conversation to collaborators Aug 6, 2024

rpc: Add RPCContext #20017

rpc: Add RPCContext #20017

Uh oh!

Conversation

promag commented Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Sep 30, 2020

Uh oh!

Uh oh!

promag commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag commented Oct 1, 2020

Uh oh!

Uh oh!

kallewoof commented Oct 1, 2020

Uh oh!

promag commented Oct 1, 2020

Uh oh!

Uh oh!

Uh oh!

maflcko commented Oct 1, 2020

Uh oh!

promag commented Oct 1, 2020

Uh oh!

maflcko commented Oct 1, 2020

Uh oh!

promag commented Oct 1, 2020

Uh oh!

promag commented Apr 19, 2021

Uh oh!

maflcko commented Apr 21, 2021

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

LarryRuane commented Aug 4, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Sep 30, 2021

Uh oh!

fanquake commented Apr 20, 2022

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

promag commented Sep 25, 2020 •

edited

Loading

promag commented Sep 25, 2020 •

edited

Loading

promag commented Sep 30, 2020 •

edited

Loading

kallewoof commented Sep 30, 2020 •

edited

Loading

DrahtBot commented Sep 30, 2020 •

edited

Loading