Repository: bitcoin/bitcoin (language: cpp)

Issue:
Put undocumented JSON failure mode behind a runtime flag
Following the merge of https://github.com/bitcoin/bitcoin/pull/23083, we'll throw a non-fatal error at runtime if the documentation for an RPC is incorrect. This behaviour has found numerous issues over a very short period (of the PR being opened). However, this is not something that we want in production / release builds, instead, we want the ability to have it enabled in the CI and when running tests. A runtime option could be added, which controls this behaviour (disabled by default).

#### Useful skills:

- C++
- Understanding of Bitcoin Core's initialization sequence
- Understanding of Bitcoin Core's RPC interface

#### Want to work on this issue?

For guidance on contributing, please read [CONTRIBUTING.md](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md) before opening your pull request.



Discussion / hints:
For background. This check is not done at compile time, nor at startup time. It is done at run time, and only if the JSON return value is actually generated, which may not be the case for our test suite. Reasons might be: Uncovered code, Racy code or tests, ...

It would be undesirable to crash a production system intermittently if there is a typo in the doc.
Concept ACK. 

> It would be undesirable to crash a production system intermittently if there is a typo in the doc.

Right. See #24716 for such a case that was luckily found by testing, not in production.

I also think the assertion is pretty uninformative. It would be great if it logged what the difference from the doc is (though out of scope here).
I use the following hack locally, which can be improved by returning the last error only.

(Edit: An alternative to returning only the last error would be to collect all errors and return all of them in the exception.)


```diff
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index 9c9e6e9f11..9371ea3421 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -918,20 +918,25 @@ bool RPCResult::MatchesType(const UniValue& result) const
         std::map<std::string, UniValue> result_obj;
         result.getObjMap(result_obj);
         for (const auto& result_entry : result_obj) {
+            std::cout << "r_ " << result_entry.first << std::endl; // ----------------
             if (doc_keys.find(result_entry.first) == doc_keys.end()) {
+                std::cout << "missing doc" << std::endl; // -------------
                 return false; // missing documentation
             }
         }
 
         for (const auto& doc_entry : m_inner) {
+            std::cout << doc_entry.m_key_name << std::endl; // ---------------
             const auto result_it{result_obj.find(doc_entry.m_key_name)};
             if (result_it == result_obj.end()) {
                 if (!doc_entry.m_optional) {
+                    std::cout << "missing a required key" << std::endl; // -------------
                     return false; // result is missing a required key
                 }
                 continue;
             }
             if (!doc_entry.MatchesType(result_it->second)) {
+                std::cout << "wrong type" << std::endl; // --------------
                 return false; // wrong type
             }
         }

Is this still open? If so, could someone provide context on what is left?
It's still entirely open AFAIK.

Fix patch:
diff --git a/src/init.cpp b/src/init.cpp
index e180a2b5cd83f..f06b5bd0d06b0 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -570,6 +570,7 @@ void SetupServerArgs(ArgsManager& argsman)
     argsman.AddArg("-rpcallowip=<ip>", "Allow JSON-RPC connections from specified source. Valid for <ip> are a single IP (e.g. 1.2.3.4), a network/netmask (e.g. 1.2.3.4/255.255.255.0) or a network/CIDR (e.g. 1.2.3.4/24). This option can be specified multiple times", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
     argsman.AddArg("-rpcauth=<userpw>", "Username and HMAC-SHA-256 hashed password for JSON-RPC connections. The field <userpw> comes in the format: <USERNAME>:<SALT>$<HASH>. A canonical python script is included in share/rpcauth. The client then connects normally using the rpcuser=<USERNAME>/rpcpassword=<PASSWORD> pair of arguments. This option can be specified multiple times", ArgsManager::ALLOW_ANY | ArgsManager::SENSITIVE, OptionsCategory::RPC);
     argsman.AddArg("-rpcbind=<addr>[:port]", "Bind to given address to listen for JSON-RPC connections. Do not expose the RPC server to untrusted networks such as the public internet! This option is ignored unless -rpcallowip is also passed. Port is optional and overrides -rpcport. Use [host]:port notation for IPv6. This option can be specified multiple times (default: 127.0.0.1 and ::1 i.e., localhost)", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY | ArgsManager::SENSITIVE, OptionsCategory::RPC);
+    argsman.AddArg("-rpcdoccheck", strprintf("Throw a non-fatal error at runtime if the documentation for an RPC is incorrect (default: %u)", DEFAULT_RPC_DOC_CHECK), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::RPC);
     argsman.AddArg("-rpccookiefile=<loc>", "Location of the auth cookie. Relative paths will be prefixed by a net-specific datadir location. (default: data dir)", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
     argsman.AddArg("-rpcpassword=<pw>", "Password for JSON-RPC connections", ArgsManager::ALLOW_ANY | ArgsManager::SENSITIVE, OptionsCategory::RPC);
     argsman.AddArg("-rpcport=<port>", strprintf("Listen for JSON-RPC connections on <port> (default: %u, testnet: %u, signet: %u, regtest: %u)", defaultBaseParams->RPCPort(), testnetBaseParams->RPCPort(), signetBaseParams->RPCPort(), regtestBaseParams->RPCPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::RPC);
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index ef3e58494e984..48ce33e7ebdda 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -12,6 +12,7 @@
 #include <util/check.h>
 #include <util/strencodings.h>
 #include <util/string.h>
+#include <util/system.h>
 #include <util/translation.h>
 
 #include <tuple>
@@ -581,7 +582,9 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const
         throw std::runtime_error(ToString());
     }
     const UniValue ret = m_fun(*this, request);
-    CHECK_NONFATAL(std::any_of(m_results.m_results.begin(), m_results.m_results.end(), [ret](const RPCResult& res) { return res.MatchesType(ret); }));
+    if (gArgs.GetBoolArg("-rpcdoccheck", DEFAULT_RPC_DOC_CHECK)) {
+        CHECK_NONFATAL(std::any_of(m_results.m_results.begin(), m_results.m_results.end(), [ret](const RPCResult& res) { return res.MatchesType(ret); }));
+    }
     return ret;
 }
 
diff --git a/src/rpc/util.h b/src/rpc/util.h
index e16fed75bcf2f..6e23caff6c3a1 100644
--- a/src/rpc/util.h
+++ b/src/rpc/util.h
@@ -22,6 +22,8 @@
 #include <variant>
 #include <vector>
 
+static constexpr bool DEFAULT_RPC_DOC_CHECK{false};
+
 /**
  * String used to describe UNIX epoch time in documentation, factored out to a
  * constant for consistency.


Test patch:
diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
index 8651bcf636dac..b043d1a70db34 100644
--- a/test/functional/test_framework/util.py
+++ b/test/functional/test_framework/util.py
@@ -378,6 +378,7 @@ def write_config(config_path, *, n, chain, extra_config="", disable_autoconnect=
             f.write("[{}]\n".format(chain_name_conf_section))
         f.write("port=" + str(p2p_port(n)) + "\n")
         f.write("rpcport=" + str(rpc_port(n)) + "\n")
+        f.write("rpcdoccheck=1\n")
         f.write("fallbackfee=0.0002\n")
         f.write("server=1\n")
         f.write("keypool=1\n")