From faeb6665362e35f573ad715ade0ef2db62d71839 Mon Sep 17 00:00:00 2001
From: MarcoFalke <falke.marco@gmail.com>
Date: Mon, 23 Sep 2019 14:45:51 -0400
Subject: [PATCH] util: Add CHECK_NONFATAL and use it in src/rpc

---
 src/Makefile.am             |  1 +
 src/rpc/blockchain.cpp      |  3 +--
 src/rpc/misc.cpp            |  8 ++++++--
 src/rpc/util.h              | 13 ++++++------
 src/util/check.h            | 41 +++++++++++++++++++++++++++++++++++++
 test/functional/rpc_misc.py |  7 +++++++
 6 files changed, 63 insertions(+), 10 deletions(-)
 create mode 100644 src/util/check.h

diff --git a/src/Makefile.am b/src/Makefile.am
index d50524a8ae7..69d380175d7 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -206,6 +206,7 @@ BITCOIN_CORE_H = \
   undo.h \
   util/bip32.h \
   util/bytevectorhash.h \
+  util/check.h \
   util/error.h \
   util/fees.h \
   util/spanparsing.h \
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 4bbd4aaf64f..fac6bcd60db 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -10,11 +10,11 @@
 #include <chain.h>
 #include <chainparams.h>
 #include <coins.h>
-#include <node/coinstats.h>
 #include <consensus/validation.h>
 #include <core_io.h>
 #include <hash.h>
 #include <index/blockfilterindex.h>
+#include <node/coinstats.h>
 #include <policy/feerate.h>
 #include <policy/policy.h>
 #include <policy/rbf.h>
@@ -34,7 +34,6 @@
 #include <validationinterface.h>
 #include <warnings.h>
 
-#include <assert.h>
 #include <stdint.h>
 
 #include <univalue.h>
diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp
index d289274a379..d73dd6e52d9 100644
--- a/src/rpc/misc.cpp
+++ b/src/rpc/misc.cpp
@@ -3,15 +3,16 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-#include <key_io.h>
 #include <httpserver.h>
+#include <key_io.h>
 #include <outputtype.h>
 #include <rpc/blockchain.h>
 #include <rpc/server.h>
 #include <rpc/util.h>
 #include <script/descriptor.h>
-#include <util/system.h>
+#include <util/check.h>
 #include <util/strencodings.h>
+#include <util/system.h>
 #include <util/validation.h>
 
 #include <stdint.h>
@@ -540,6 +541,7 @@ static UniValue echo(const JSONRPCRequest& request)
         throw std::runtime_error(
             RPCHelpMan{"echo|echojson ...",
                 "\nSimply echo back the input arguments. This command is for testing.\n"
+                "\nIt will return an internal bug report when exactly 100 arguments are passed.\n"
                 "\nThe difference between echo and echojson is that echojson has argument conversion enabled in the client-side table in "
                 "bitcoin-cli and the GUI. There is no server-side difference.",
                 {},
@@ -548,6 +550,8 @@ static UniValue echo(const JSONRPCRequest& request)
             }.ToString()
         );
 
+    CHECK_NONFATAL(request.params.size() != 100);
+
     return request.params;
 }
 
diff --git a/src/rpc/util.h b/src/rpc/util.h
index 72fc7b6286d..ec36956c958 100644
--- a/src/rpc/util.h
+++ b/src/rpc/util.h
@@ -7,14 +7,15 @@
 
 #include <node/transaction.h>
 #include <outputtype.h>
-#include <pubkey.h>
 #include <protocol.h>
+#include <pubkey.h>
 #include <rpc/protocol.h>
 #include <rpc/request.h>
 #include <script/script.h>
 #include <script/sign.h>
 #include <script/standard.h>
 #include <univalue.h>
+#include <util/check.h>
 
 #include <string>
 #include <vector>
@@ -146,7 +147,7 @@ struct RPCArg {
           m_oneline_description{oneline_description},
           m_type_str{type_str}
     {
-        assert(type != Type::ARR && type != Type::OBJ);
+        CHECK_NONFATAL(type != Type::ARR && type != Type::OBJ);
     }
 
     RPCArg(
@@ -165,7 +166,7 @@ struct RPCArg {
           m_oneline_description{oneline_description},
           m_type_str{type_str}
     {
-        assert(type == Type::ARR || type == Type::OBJ);
+        CHECK_NONFATAL(type == Type::ARR || type == Type::OBJ);
     }
 
     bool IsOptional() const;
@@ -194,14 +195,14 @@ struct RPCResult {
     explicit RPCResult(std::string result)
         : m_cond{}, m_result{std::move(result)}
     {
-        assert(!m_result.empty());
+        CHECK_NONFATAL(!m_result.empty());
     }
 
     RPCResult(std::string cond, std::string result)
         : m_cond{std::move(cond)}, m_result{std::move(result)}
     {
-        assert(!m_cond.empty());
-        assert(!m_result.empty());
+        CHECK_NONFATAL(!m_cond.empty());
+        CHECK_NONFATAL(!m_result.empty());
     }
 };
 
diff --git a/src/util/check.h b/src/util/check.h
new file mode 100644
index 00000000000..d18887ae958
--- /dev/null
+++ b/src/util/check.h
@@ -0,0 +1,41 @@
+// Copyright (c) 2019 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_UTIL_CHECK_H
+#define BITCOIN_UTIL_CHECK_H
+
+#include <tinyformat.h>
+
+#include <stdexcept>
+
+class NonFatalCheckError : public std::runtime_error
+{
+    using std::runtime_error::runtime_error;
+};
+
+/**
+ * Throw a NonFatalCheckError when the condition evaluates to false
+ *
+ * This should only be used
+ * - where the condition is assumed to be true, not for error handling or validating user input
+ * - where a failure to fulfill the condition is recoverable and does not abort the program
+ *
+ * For example in RPC code, where it is undersirable to crash the whole program, this can be generally used to replace
+ * asserts or recoverable logic errors. A NonFatalCheckError in RPC code is caught and passed as a string to the RPC
+ * caller, which can then report the issue to the developers.
+ */
+#define CHECK_NONFATAL(condition)                                 \
+    do {                                                          \
+        if (!(condition)) {                                       \
+            throw NonFatalCheckError(                             \
+                strprintf("%s:%d (%s)\n"                          \
+                          "Internal bug detected: '%s'\n"         \
+                          "You may report this issue here: %s\n", \
+                    __FILE__, __LINE__, __func__,                 \
+                    (#condition),                                 \
+                    PACKAGE_BUGREPORT));                          \
+        }                                                         \
+    } while (false)
+
+#endif // BITCOIN_UTIL_CHECK_H
diff --git a/test/functional/rpc_misc.py b/test/functional/rpc_misc.py
index 8a3f8c6f061..3da9f05ca53 100755
--- a/test/functional/rpc_misc.py
+++ b/test/functional/rpc_misc.py
@@ -23,6 +23,13 @@ class RpcMiscTest(BitcoinTestFramework):
     def run_test(self):
         node = self.nodes[0]
 
+        self.log.info("test CHECK_NONFATAL")
+        assert_raises_rpc_error(
+            -1,
+            "Internal bug detected: 'request.params.size() != 100'",
+            lambda: node.echo(*[0] * 100),
+        )
+
         self.log.info("test getmemoryinfo")
         memory = node.getmemoryinfo()['locked']
         assert_greater_than(memory['used'], 0)