RPC: make RPCResult::MatchesType return useful errors #26887

ajtowns commented at 11:51 AM on January 13, 2023: contributor

Currently if you don't correctly update the description of the return value for an RPC call, you essentially just get an assertion failure with no useful information; this generates a description of the problems instead.

DrahtBot commented at 11:51 AM on January 13, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	MarcoFalke
Concept ACK	brunoerg

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25429 (refactor: Avoid UniValue copies 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.

DrahtBot added the label RPC/REST/ZMQ on Jan 13, 2023

ajtowns commented at 11:52 AM on January 13, 2023: contributor

eg if I garble up the results of getdeploymentinfo, and run rpc_blockchain.py, prior to this PR, I just get:

test_framework.authproxy.JSONRPCException: Internal bug detected: "std::any_of(m_results.m_results.begin(), m_results.m_results.end(), [&ret](const RPCResult& res) { return res.MatchesType(ret); })" rpc/util.cpp:584 (HandleRequest) Bitcoin Core v24.99.0-38d883a41078 Please report this issue here: https://github.com/bitcoin/bitcoin/issues

with this PR, I instead get:

test_framework.authproxy.JSONRPCException: Internal bug detected: RPC call "getdeploymentinfo" returned incorrect type:
{
    "deployments": {
        "testdummy": {
            "bip9": {
                "awesomeness": "unexpected key",
                "status_next": "non-optional key missing",
                "statistics": {
                    "period": "expected number got null",
                    "threshold": "expected number got string"
                }
            }
        },
        "taproot": {
            "bip9": {
                "awesomeness": "unexpected key",
                "status_next": "non-optional key missing"
            }
        }
    }
}
Bitcoin Core v24.99.0-51b4318e76d3-dirty
Please report this issue here: https://github.com/bitcoin/bitcoin/issues

which seems much more helpful for fixing the problem.

cc @MarcoFalke

in src/rpc/util.h:329 in 2a92980593 outdated

 323 | @@ -324,8 +324,11 @@ struct RPCResult {
 324 |      std::string ToStringObj() const;
 325 |      /** Return the description string, including the result type. */
 326 |      std::string ToDescriptionString() const;
 327 | -    /** Check whether the result JSON type matches. */
 328 | -    bool MatchesType(const UniValue& result) const;
 329 | +    /** Check whether the result JSON type matches.
 330 | +     * Returns value where .isTrue() passes is type matches,
 331 | +     * or object describind error(s) if not.

maflcko commented at 12:24 PM on January 13, 2023:

     * or object describing error(s) if not.

in src/rpc/util.cpp:5 in 2a92980593 outdated

   1 | @@ -2,6 +2,7 @@
   2 |  // Distributed under the MIT software license, see the accompanying
   3 |  // file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 |  
   5 | +#include <clientversion.h>

maflcko commented at 12:25 PM on January 13, 2023:

Not sure. Wouldn't it be better to use one of the existing checks or formatters from util/check?

ajtowns commented at 1:09 PM on January 13, 2023:

Maybe? Could just use StrFormatInternalBug I guess, but the file/line/func info would be kind of misleading as to where the bug is?

(As it is, it backports cleanly to 24.0, which was convenient for what I was trying to debug)

maflcko approved

maflcko commented at 12:25 PM on January 13, 2023: member

Nice. Concept ACK

brunoerg commented at 3:10 PM on January 13, 2023: contributor

Concept ACK

It had helped me to save a lot of time recently btw :)

ajtowns force-pushed on Jan 13, 2023

in src/rpc/util.h:328 in 94adb442fa outdated

 323 | @@ -324,8 +324,11 @@ struct RPCResult {
 324 |      std::string ToStringObj() const;
 325 |      /** Return the description string, including the result type. */
 326 |      std::string ToDescriptionString() const;
 327 | -    /** Check whether the result JSON type matches. */
 328 | -    bool MatchesType(const UniValue& result) const;
 329 | +    /** Check whether the result JSON type matches.
 330 | +     * Returns value where .isTrue() passes is type matches,

maflcko commented at 3:13 PM on January 18, 2023:

     * Returns true if type matches,

nit: Use simple English?

in src/rpc/util.cpp:886 in 94adb442fa outdated

 884 | +static const std::optional<UniValue::VType> ExpectedType(RPCResult::Type type)
 885 |  {
 886 | -    if (m_skip_type_check) {
 887 | -        return true;
 888 | +    switch (type) {
 889 | +    case RPCResult::Type::ELISION:

maflcko commented at 3:27 PM on January 18, 2023:

    using Type = RPCResult::Type;
    switch (type) {
    case Type::ELISION:

nit: not sure if it compiles, though

in src/rpc/util.cpp:934 in 94adb442fa outdated

 951 | +        UniValue errors(UniValue::VOBJ);
 952 |          for (size_t i{0}; i < result.get_array().size(); ++i) {
 953 |              // If there are more results than documented, re-use the last doc_inner.
 954 |              const RPCResult& doc_inner{m_inner.at(std::min(m_inner.size() - 1, i))};
 955 | -            if (!doc_inner.MatchesType(result.get_array()[i])) return false;
 956 | +            UniValue match{doc_inner.MatchesType(result.get_array()[i]).isTrue()};

maflcko commented at 3:33 PM on January 18, 2023:

            UniValue match{doc_inner.MatchesType(result.get_array()[i])};

?

in src/rpc/util.cpp:961 in 94adb442fa outdated

 957 | @@ -916,26 +958,26 @@ bool RPCResult::MatchesType(const UniValue& result) const
 958 |          result.getObjMap(result_obj);
 959 |          for (const auto& result_entry : result_obj) {
 960 |              if (doc_keys.find(result_entry.first) == doc_keys.end()) {
 961 | -                return false; // missing documentation
 962 | +                errors.pushKV(result_entry.first, "unexpected key");

maflcko commented at 3:36 PM on January 18, 2023:

                errors.pushKV(result_entry.first, "unexpected key: Returned key not found in doc");

in src/rpc/util.cpp:926 in 94adb442fa outdated

 942 | +    }
 943 | +
 944 | +    const auto exp_type = ExpectedType(m_type);
 945 | +    if (!exp_type) return true;
 946 | +    if (*exp_type != result.getType()) {
 947 | +        return strprintf("expected %s got %s", uvTypeName(*exp_type), uvTypeName(result.getType()));

maflcko commented at 3:38 PM on January 18, 2023:

        return strprintf("documented type is %s, but returned %s", uvTypeName(*exp_type), uvTypeName(result.getType()));

in src/rpc/util.cpp:969 in 94adb442fa outdated

 966 |          for (const auto& doc_entry : m_inner) {
 967 |              const auto result_it{result_obj.find(doc_entry.m_key_name)};
 968 |              if (result_it == result_obj.end()) {
 969 |                  if (!doc_entry.m_optional) {
 970 | -                    return false; // result is missing a required key
 971 | +                    errors.pushKV(doc_entry.m_key_name, "non-optional key missing");

maflcko commented at 3:39 PM on January 18, 2023:

                    errors.pushKV(doc_entry.m_key_name, "Key is documented as required, but missing from the result");

maflcko approved

maflcko commented at 3:43 PM on January 18, 2023: member

There is one typo. Also, in the presence of bugs it could be confusing to call something "expected". Instead, it could make more sense to just say there is an error and mention the doc-type and the result-type.

ACK 94adb442fac9da04063a57897b44a9623c4fe39e 🔎

<details><summary>Show signature</summary>

Signature:

</details>

RPC: make RPCResult::MatchesType return useful errors 3d1a4d8a45

ajtowns force-pushed on Jan 19, 2023

ajtowns commented at 8:35 PM on January 19, 2023: contributor

Took @MarcoFalke's suggestions (albeit with some wording changes)

maflcko commented at 9:36 AM on January 20, 2023: member

Very nice.

re-ACK 3d1a4d8a45cd91bdfe0ef107c2e9c5e882b34155 🌷

<details><summary>Show signature</summary>

Signature:

</details>

maflcko merged this on Jan 20, 2023

maflcko closed this on Jan 20, 2023

in src/rpc/util.cpp:883 in 3d1a4d8a45

 879 | @@ -860,53 +880,77 @@ void RPCResult::ToSections(Sections& sections, const OuterType outer_type, const
 880 |      NONFATAL_UNREACHABLE();
 881 |  }
 882 |  
 883 | -bool RPCResult::MatchesType(const UniValue& result) const
 884 | +static const std::optional<UniValue::VType> ExpectedType(RPCResult::Type type)

maflcko commented at 10:50 AM on January 20, 2023:

nit: const is confusing and thus wrong?

maflcko commented at 11:12 AM on January 20, 2023:

Done in https://github.com/bitcoin/bitcoin/pull/26929

sidhujag referenced this in commit 11fb1d2c79 on Jan 20, 2023

bitcoin locked this on Jan 20, 2024