rpc: Fail to return undocumented return values

MarcoFalke commented at 10:51 am on November 23, 2020: member

Currently a few return values are undocumented. This is causing confusion at the least. See for example #18476

Fix this by treating it as an internal bug to return undocumented return values.

MarcoFalke added the label RPC/REST/ZMQ on Nov 23, 2020

MarcoFalke added the label Needs backport (0.21) on Nov 23, 2020

laanwj commented at 11:14 am on November 23, 2020: member

Concept ACK but it would be much better if checks like this could be done at compile time instead of run time. This seems to me like a kind of a python-esque solution instead of a static language one.

MarcoFalke commented at 11:44 am on November 23, 2020: member

UniValue::getType() is not a compile-time constant, so I don’t think this works at compile time (without replacing UniValue with something else)

MarcoFalke removed the label Needs backport (0.21) on Nov 23, 2020

MarcoFalke force-pushed on Nov 23, 2020

DrahtBot commented at 9:43 pm on November 23, 2020: member

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

Conflicts

No conflicts as of last run.

amitiuttarwar commented at 11:31 pm on December 24, 2020: contributor

concept ACK, very nice! having a programatic check that the docs are in sync with the actual output seems like a win for maintaining up-to-date docs & reducing review burden

in src/rpc/mining.cpp:532 in fa6135b1cb outdated

552-                },
553-                RPCResult{
554-                    RPCResult::Type::OBJ, "", "",
555+        },
556+        {
557+            RPCResult{"When mode=='proposal'", RPCResult::Type::STR, "", ""},

sipa commented at 7:10 am on December 27, 2020:

Nit: using single quotes isn’t valid JSON or C++. Maybe use double quotes?

MarcoFalke commented at 9:06 am on December 27, 2020:

The existing docs use single quotes:

https://github.com/bitcoin/bitcoin/blob/02cf20b9f571474c939d18a8b9d6b5d22479a222/src/rpc/mining.cpp#L508

https://github.com/bitcoin/bitcoin/blob/02cf20b9f571474c939d18a8b9d6b5d22479a222/src/rpc/mining.cpp#L521

So I used that. Also, a lot of RPC clients are implemented in python, which also allows single quotes.

in src/rpc/blockchain.cpp:2181 in 9999759ecc outdated

2182-                },
2183-                RPCResult{
2184-                    RPCResult::Type::OBJ, "", "",
2185+        },
2186+        {
2187+            RPCResult{"When action=='abort'", RPCResult::Type::BOOL, "", ""},

sipa commented at 7:11 am on December 27, 2020:

Same here: single quotes isn’t valid JSON.

MarcoFalke commented at 9:06 am on December 27, 2020:

#20459 (review)

in src/rpc/util.cpp:754 in faca73c074 outdated

748+    case Type::STR:
749+    case Type::STR_HEX: {
750+        return UniValue::VSTR == result.getType();
751+    }
752+    case Type::NUM:
753+    case Type::NUM_AMOUNT:

sipa commented at 7:16 am on December 27, 2020:

Suggestion for a follow-up: for NUM_AMOUNT, you can do a stronger test (parse with AmountFromValue, convert back to JSON using ValueFromAmount, and the string representation must match exactly).

sipa commented at 7:17 am on December 27, 2020: member

utACK faca73c07461f3344c7f88ea63727d9ac1c70b2a

MarcoFalke marked this as a draft on Dec 27, 2020

MarcoFalke marked this as ready for review on Mar 15, 2021

MarcoFalke force-pushed on Mar 15, 2021

in src/rpc/server.cpp:152 in fabf0515c6 outdated

148@@ -149,6 +149,7 @@ static RPCHelpMan help()
149     }
150     if (strCommand == "dump_all_command_conversions") {
151         // Used for testing only, undocumented
152+        const_cast<JSONRPCRequest&>(jsonRequest).mode = JSONRPCRequest::GET_ARGS;

MarcoFalke commented at 9:57 am on March 20, 2021:

This is currently safe to do, because JSONRPCRequest isn’t const where it is (copy-)constructed. I don’t like the const_cast myself, but the only alternative I came up with was to change every rpc method with the new signature. Any other ideas? Maybe @ryanofsky

ryanofsky commented at 2:19 am on April 1, 2021:

re: #20459 (review)

This is currently safe to do, because JSONRPCRequest isn’t const where it is (copy-)constructed. I don’t like the const_cast myself, but the only alternative I came up with was to change every rpc method with the new signature. Any other ideas? Maybe @ryanofsky

I think it would be better add ANY to list of acceptable result types: 4d692e282d404ce3d25a51261a1c6958ebd8b196 (tag). Advantages of this approach:

No need for const_cast
No need to change dumpArgMap implementation and make callers care how it works internally
GET_ARGS can still mean “get arguments” instead of “get arguments and ignore result type”

MarcoFalke commented at 10:24 am on April 1, 2021:

Thanks, done

DrahtBot added the label Needs rebase on Mar 31, 2021

ryanofsky approved

ryanofsky commented at 2:23 am on April 1, 2021: member

Code review ACK fabf0515c6c58096aafb6685daf1830a34bf5c75. Suggested a tweak to get rid of const_cast, but current implementation also looks good, so feel free to ignore suggestion.

rpc: Fail to return undocumented return values fa8192f42e

MarcoFalke force-pushed on Apr 1, 2021

DrahtBot removed the label Needs rebase on Apr 1, 2021

ryanofsky approved

ryanofsky commented at 2:11 pm on April 1, 2021: member

Code review ACK fa8192f42e1d24444f1d0433c96dbce1adf76967. Only changes: rebase, no const_cast suggestion, and tostring cleanups needed after suggestion

MarcoFalke merged this on Apr 3, 2021

MarcoFalke closed this on Apr 3, 2021

MarcoFalke deleted the branch on Apr 3, 2021

sidhujag referenced this in commit c38b140703 on Apr 3, 2021

DrahtBot locked this on Aug 18, 2022

rpc: Fail to return undocumented return values #20459

Conflicts