util: Add CHECK_NONFATAL and use it in src/rpc #17192

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:1910-utilCHECK_NONFATAL changing 6 files +63 −10

MarcoFalke commented at 6:51 PM on October 18, 2019: member

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 asserts 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.
MarcoFalke added the label RPC/REST/ZMQ on Oct 18, 2019
MarcoFalke force-pushed on Oct 18, 2019
MarcoFalke force-pushed on Oct 18, 2019
DrahtBot commented at 7:31 PM on October 18, 2019: contributor


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #16899 (UTXO snapshot creation (dumptxoutset) by jamesob)
- #16807 (Let validateaddress locate error in Bech32 address by meshcollider)
- #16440 (BIP-322: Generic signed message format by kallewoof)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/util/check.h:30 in fa052fd301 outdated

  25 | +#define CHECK_NONFATAL(condition)                                     \
  26 | +    do {                                                              \
  27 | +        if (!(condition)) {                                           \
  28 | +            throw NonFatalCheckError(                                 \
  29 | +                strprintf("%s:%d (%s)\n"                              \
  30 | +                          "Internal non-fatal error detected: '%s'\n" \

ryanofsky commented at 7:55 PM on October 18, 2019:

User-facing message might be clearer if it said "internal bug detected".

in src/util/check.h:20 in fa052fd301 outdated

  15 | +};
  16 | +
  17 | +/**
  18 | + * Throw a NonFatalCheckError when the condition evaluates to false
  19 | + *
  20 | + * This can be used in places where an error is recoverable and should not abort the program. This should also be used

ryanofsky commented at 8:02 PM on October 18, 2019:

Maybe say explicitly that the check macro is meant to check conditions that should always be true, not for normal error handling or things like validating user input.

ryanofsky commented at 8:13 PM on October 18, 2019: contributor

Concept ACK. Is this work in progress? I figured #17181 was about more asserts in rpc code than just these, though maybe these are enough for now.
ryanofsky cross-referenced this on Oct 18, 2019 from issue rpc: Report reason for replaceable txpool transactions by MarcoFalke
MarcoFalke commented at 8:47 PM on October 18, 2019: member

Is this work in progress

No. I am happy to do a scripted-diff replacement of all asserts in rpc code as a follow up or directly here. Though, I wanted to wait for at least one ACK before working on the scripted-diff.
util: Add CHECK_NONFATAL and use it in src/rpc faeb666536
MarcoFalke force-pushed on Oct 18, 2019
MarcoFalke commented at 9:20 PM on October 18, 2019: member

Addressed wording issues pointed out by @ryanofsky
practicalswift commented at 2:36 PM on October 19, 2019: contributor

ACK faeb6665362e35f573ad715ade0ef2db62d71839

Thanks for taking a step towards making the RPC interface more robust. That is needed :)
bitcoin deleted a comment on Oct 22, 2019
ryanofsky approved
ryanofsky commented at 5:46 PM on October 22, 2019: contributor

Code review ACK faeb6665362e35f573ad715ade0ef2db62d71839

in src/rpc/misc.cpp:544 in faeb666536

 540 | @@ -540,6 +541,7 @@ static UniValue echo(const JSONRPCRequest& request)
 541 |          throw std::runtime_error(
 542 |              RPCHelpMan{"echo|echojson ...",
 543 |                  "\nSimply echo back the input arguments. This command is for testing.\n"
 544 | +                "\nIt will return an internal bug report when exactly 100 arguments are passed.\n"

laanwj commented at 10:59 AM on October 28, 2019:

LOL

laanwj commented at 11:00 AM on October 28, 2019: member

ACK faeb6665362e35f573ad715ade0ef2db62d71839
laanwj referenced this in commit 9ae468a6d5 on Oct 28, 2019
laanwj merged this on Oct 28, 2019
laanwj closed this on Oct 28, 2019
MarcoFalke deleted the branch on Oct 28, 2019
sidhujag referenced this in commit 6d28260ebf on Oct 28, 2019
laanwj cross-referenced this on Oct 30, 2019 from issue test: Add generatetodescriptor RPC by MarcoFalke
adamjonas cross-referenced this on Oct 30, 2019 from issue replace asserts in RPC code with CHECK_NONFATAL and add linter by adamjonas
MarcoFalke referenced this in commit 94a26b192f on Nov 4, 2019
sidhujag referenced this in commit 4e0427ddd6 on Nov 7, 2019
MarkLTZ cross-referenced this on Apr 4, 2020 from issue Bitcoin PR tracking by MarkLTZ
deadalnix referenced this in commit f7690c0f38 on Apr 20, 2020
promag cross-referenced this on Aug 1, 2020 from issue rpc/blockchain: Reset scantxoutset progress before inferring descriptors by prusnak
jasonbcox referenced this in commit f0bbedc289 on Sep 8, 2020
sidhujag referenced this in commit a5c20ba4d9 on Nov 10, 2020
sidhujag referenced this in commit 8c1e3c13b2 on Nov 10, 2020
bitcoin locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#16490 rpc: Report reason for replaceable txpool transactions #16943 test: Add generatetodescriptor RPC #17181 rpc: Logic errors and asserts in rpc code #17318 replace asserts in RPC code with CHECK_NONFATAL and add linter #19362 rpc/blockchain: Reset scantxoutset progress before inferring descriptors

util: Add CHECK_NONFATAL and use it in src/rpc #17192

Conflicts