Merge #17192: util: Add CHECK_NONFATAL and use it in src/rpc
faeb666536
util: Add CHECK_NONFATAL and use it in src/rpc (MarcoFalke) Pull request description: Fixes #17181 Currently, we use `assert` in RPC code to document logic and code assumptions. However, it seems a bit extreme to abort all of Bitcoin Core on an assert failure in one of the RPC threads. I suggest to replace all `assert`s with a macro `CHECK_NONFATAL(condition)` that throws a runtime error when the condition evaluates to `false`. That runtime error will then be returned to the rpc caller and will include instructions to report the error to our issue tracker. ACKs for top commit: practicalswift: ACKfaeb666536
laanwj: ACKfaeb666536
ryanofsky: Code review ACKfaeb666536
Tree-SHA512: 9b748715a5e0767ac11f1324a95a3a6ec672a0e0658013492219223bda83ce4b1b447fd8183bbb235f7df5ef7dddda7666ad569544b4d61cc65f232ca7a800ec
This commit is contained in:
commit
9ae468a6d5
6 changed files with 63 additions and 10 deletions
|
@ -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 \
|
||||
|
|
|
@ -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>
|
||||
|
|
|
@ -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;
|
||||
}
|
||||
|
||||
|
|
|
@ -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());
|
||||
}
|
||||
};
|
||||
|
||||
|
|
41
src/util/check.h
Normal file
41
src/util/check.h
Normal file
|
@ -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
|
|
@ -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)
|
||||
|
|
Loading…
Reference in a new issue