refactor: use ternary when creating RPC param variables #24741

A style we are already using in some RPC code. i.e: https://github.com/bitcoin/bitcoin/blob/55ea6fd2506b84f553c0bd42f48a9cc4393bab47/src/rpc/blockchain.cpp#L864-L866 More concise (about half the lines of code). Variables that should be made const, can be made const. I'd argue that this:

    const bool fVerbose{request.params[0].isNull() ? false : request.params[0].get_bool()};

    const bool include_mempool_sequence{request.params[1].isNull() ? false : request.params[1].get_bool()};

is easier to read / reason about, than:

    bool fVerbose = false;
    if (!request.params[0].isNull())
        fVerbose = request.params[0].get_bool();

    bool include_mempool_sequence = false;
    if (!request.params[1].isNull()) {
        include_mempool_sequence = request.params[1].get_bool();
    }

Concept ACK. I've been suggesting this style in code reviews since a year or two.

Nice! Concept ACK.

Btw, what do you think about further reducing the boilerplate code for RPC param variables using macros?

e.g. the following

const bool fSubtractFeeFromAmount{request.params[4].isNull() ? false : request.params[4].get_bool()};
const int timeout {request.params[1].isNull() ? 0 : request.params[1].get_int()};
const double threshold{request.params[1].isNull() ? 0.95 : request.params[1].get_real()};
const std::string strCommand{request.params[1].isNull() ? "" : request.params[1].get_str()};

becomes:

#define RPC_PARAM_BOOL(i, name, defval) const bool #name {request.params[i].isNull() ? defval : request.params[i].get_bool()}
#define RPC_PARAM_INT(i, name, defval) const int #name {request.params[i].isNull() ? defval : request.params[i].get_int()}
#define RPC_PARAM_DOUBLE(i, name, defval) const double #name {request.params[i].isNull() ? defval : request.params[i].get_real()}
#define RPC_PARAM_STRING(i, name, defval) const std::string #name {request.params[i].isNull() ? defval : request.params[i].get_str()}

RPC_PARAM_BOOL(4, fSubtractFeeFromAmount, false);
RPC_PARAM_INT(1, timeout, 0);
RPC_PARAM_DOUBLE(1, threshold, 0.95);
RPC_PARAM_STRING(1, strCommand, "");

Could just be an inline method for JSONRPCRequest?

class JSONRPCRequest
{
    ...
    bool get_bool_param(size_t id, bool default) const
    {
        return params[id].isNull() ? default : params[id].get_bool();
    }
};
const bool fSubtractFeeFromAmount{request.get_bool_param(4, false)};

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #19825 (rpc: simpler setban and new ban manipulation commands by dhruv)

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.

Could just be an inline method for JSONRPCRequest?

Will this still allow creating a const variable?

Concept ACK

• I like the idea of using the ternary operator to define param variables, as this makes the code clean and easy to understand and reason with. However, I see some excellent update ideas to this PR, and I would be interested to see how they would be taken.

In the meantime, I found some nits that would make the changes in this PR consistent.

Concept ACK.

FWIW #20017 takes a different approach to handle RPC arguments.

@promag Heh, was about to link that pull just now

I don't like these kind of "change style all over the place for sake of changing style" PRs to be honest. That said I think using the tertiary operator is fine, as long as it's only one level deep. It can become nasty and hard to review when nested.

Agree with @laanwj . Ideally we should make those changes so that they give additional run-time or compile-time guarantees. Otherwise, the lines that are changed here will need to be touched again if default arguments are made "documentation-enforced".

I don't like these kind of "change style all over the place for sake of changing style" PRs to be honest.

Ideally we should make those changes so that they give additional run-time or compile-time guarantees.

I think I want to push back against "style for the sake of style" here. It's not often that style-only changes result in half as many lines of (I'll claim somewhat "better") code. IIRC the binary for this branch is also smaller.

I realise there is a spectrum, but I think at some point we have to allow these kind of "cleanup" PRs, that may be slightly closer to stylish than we'd like, because if we do, it should eventually just result in "better", and maybe generally less, code in the repository.

Saying all that, it seems like something like #20017 is probably the better way forward here.