Put undocumented JSON failure mode behind a runtime flag

fanquake commented at 12:43 pm on March 28, 2022: member

Following the merge of #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 before opening your pull request.

fanquake added the label RPC/REST/ZMQ on Mar 28, 2022

fanquake added the label good first issue on Mar 28, 2022

fanquake added this to the milestone 24.0 on Mar 28, 2022

MarcoFalke commented at 12:53 pm on March 28, 2022: member

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.

fanquake deleted a comment on Mar 29, 2022

laanwj commented at 1:09 pm on April 5, 2022: member

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).

MarcoFalke commented at 3:27 pm on April 5, 2022: member

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.)

 0diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
 1index 9c9e6e9f11..9371ea3421 100644
 2--- a/src/rpc/util.cpp
 3+++ b/src/rpc/util.cpp
 4@@ -918,20 +918,25 @@ bool RPCResult::MatchesType(const UniValue& result) const
 5         std::map<std::string, UniValue> result_obj;
 6         result.getObjMap(result_obj);
 7         for (const auto& result_entry : result_obj) {
 8+            std::cout << "r_ " << result_entry.first << std::endl; // ----------------
 9             if (doc_keys.find(result_entry.first) == doc_keys.end()) {
10+                std::cout << "missing doc" << std::endl; // -------------
11                 return false; // missing documentation
12             }
13         }
14 
15         for (const auto& doc_entry : m_inner) {
16+            std::cout << doc_entry.m_key_name << std::endl; // ---------------
17             const auto result_it{result_obj.find(doc_entry.m_key_name)};
18             if (result_it == result_obj.end()) {
19                 if (!doc_entry.m_optional) {
20+                    std::cout << "missing a required key" << std::endl; // -------------
21                     return false; // result is missing a required key
22                 }
23                 continue;
24             }
25             if (!doc_entry.MatchesType(result_it->second)) {
26+                std::cout << "wrong type" << std::endl; // --------------
27                 return false; // wrong type
28             }
29         }

suhailsaqan commented at 5:14 pm on May 7, 2022: contributor

Is this still open? If so, could someone provide context on what is left?

laanwj commented at 1:11 pm on May 10, 2022: member

It’s still entirely open AFAIK.

suhailsaqan referenced this in commit 5a90593462 on May 17, 2022

MarcoFalke referenced this in commit bb83aba6c9 on May 19, 2022

MarcoFalke closed this on May 19, 2022

sidhujag referenced this in commit bb830284e8 on May 28, 2022

DrahtBot locked this on May 19, 2023

Put undocumented JSON failure mode behind a runtime flag #24695

Useful skills:

Want to work on this issue?