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

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

2. MarcoFalke added the label RPC/REST/ZMQ on Oct 18, 2019
3. MarcoFalke force-pushed on Oct 18, 2019
4. MarcoFalke force-pushed on Oct 18, 2019
5. 
   DrahtBot commented at 7:31 pm on October 18, 2019: member

   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.

6. 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”.
7. 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.
8. 
   ryanofsky commented at 8:13 pm on October 18, 2019: member

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

10. util: Add CHECK_NONFATAL and use it in src/rpc faeb666536
11. MarcoFalke force-pushed on Oct 18, 2019
12. 
    MarcoFalke commented at 9:20 pm on October 18, 2019: member

    Addressed wording issues pointed out by @ryanofsky
13. 
    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 :)

14. MarcoFalke deleted a comment on Oct 22, 2019
15. ryanofsky approved
16. 
    ryanofsky commented at 5:46 pm on October 22, 2019: member

    Code review ACK faeb6665362e35f573ad715ade0ef2db62d71839
17. 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
18. 
    laanwj commented at 11:00 am on October 28, 2019: member

    ACK faeb6665362e35f573ad715ade0ef2db62d71839
19. laanwj referenced this in commit 9ae468a6d5 on Oct 28, 2019
20. laanwj merged this on Oct 28, 2019
21. laanwj closed this on Oct 28, 2019

    ---

22. MarcoFalke deleted the branch on Oct 28, 2019
23. sidhujag referenced this in commit 6d28260ebf on Oct 28, 2019
24. MarcoFalke referenced this in commit 94a26b192f on Nov 4, 2019
25. sidhujag referenced this in commit 4e0427ddd6 on Nov 7, 2019
26. deadalnix referenced this in commit f7690c0f38 on Apr 20, 2020
27. jasonbcox referenced this in commit f0bbedc289 on Sep 8, 2020
28. sidhujag referenced this in commit a5c20ba4d9 on Nov 10, 2020
29. sidhujag referenced this in commit 8c1e3c13b2 on Nov 10, 2020
30. MarcoFalke locked this on Dec 16, 2021

Contributors
MarcoFalke DrahtBot ryanofsky practicalswift laanwj

Labels
RPC/REST/ZMQ