replace asserts in RPC code with CHECK_NONFATAL and add linter #17318

pull adamjonas wants to merge 1 commits into bitcoin:master from adamjonas:replace-rpc-asserts-for-CHECK_NONFATAL changing 6 files +34 −23

adamjonas commented at 2:06 PM on October 30, 2019: member
- Replace instances of assert in /rpc files and rpcwallet with CHECK_NONFATAL(condition)
- Add a linter to prevent future usage of assert being used in RPC code
ref https://github.com/bitcoin/bitcoin/pull/17192
fanquake added the label RPC/REST/ZMQ on Oct 30, 2019
MarcoFalke commented at 2:13 PM on October 30, 2019: member

Concept ACK. Reviewers should verify that the two requirements of the macro are fullfilled: https://dev.visucore.com/bitcoin/doxygen/check_8h.html#a46a3e27097aa5e94bbf62075bad7016f
in src/rpc/server.cpp:317 in db98d6f647 outdated
```
 313 | @@ -314,7 +314,7 @@ void SetRPCWarmupStatus(const std::string& newStatus)
 314 |  void SetRPCWarmupFinished()
 315 |  {
 316 |      LOCK(cs_rpcWarmup);
 317 | -    assert(fRPCInWarmup);
 318 | +    CHECK_NONFATAL(fRPCInWarmup);
```
laanwj commented at 2:14 PM on October 30, 2019:

I don't think you should change this one, it's used as part of initialization not RPC.

adamjonas commented at 2:30 PM on October 30, 2019:

Updated.

adamjonas commented at 2:44 PM on October 30, 2019:

Hmm. Looks like the linter is working (a little too well). Any suggestions on how I might relax the linter requirements? cc @MarcoFalke?

MarcoFalke commented at 2:47 PM on October 30, 2019:

My suggestion would be to add an ASSERT macro to check.h that just does assert() for now.

In the short term this helps with documentation to explicitly distinguish cases CHECK_NONFATAL vs ASSERT (and to fix the linter). In the long term we can get rid of the default assert and replace it with our own shiny Bitcoin Core ASSERT (doing fancy things like logging or writing the traceback to stderr, ...)

laanwj commented at 2:48 PM on October 30, 2019:

or exclude rpcserver.cpp, it contains mostly meta-code

MarcoFalke commented at 2:53 PM on October 30, 2019:

or exclude rpcserver.cpp, it contains mostly meta-code

Fine in the short term. I'd still prefer to get rid of assert() in the long term.

laanwj commented at 2:57 PM on October 30, 2019:

Fine in the short term. I'd still prefer to get rid of assert() in the long term.

Sure, I agree.

adamjonas commented at 3:30 PM on October 30, 2019:

OK. I'll take the easy way out on this one and will look into getting rid of the assert().

in test/lint/lint-assertions.sh:25 in db98d6f647 outdated

  19 | @@ -20,4 +20,14 @@ if [[ ${OUTPUT} != "" ]]; then
  20 |      EXIT_CODE=1
  21 |  fi
  22 |  
  23 | +# "Macro CHECK_NONFATAL(condition) should be used instead of assert for RPC code, where it
  24 | +# is undersirable to crash the whole program. See: src/util/check.h"
  25 | +OUTPUT=$(git grep -nE 'assert(.*);' -- "src/rpc" "src/wallet/rpcwallet.*")

MarcoFalke commented at 2:14 PM on October 30, 2019:

OUTPUT=$(git grep -nE 'assert(.*);' -- "src/rpc" "src/wallet/rpc*")

adamjonas commented at 2:30 PM on October 30, 2019:

Updated.

adamjonas force-pushed on Oct 30, 2019

in test/lint/lint-assertions.sh:24 in d68f7cdde6 outdated

  19 | @@ -20,4 +20,14 @@ if [[ ${OUTPUT} != "" ]]; then
  20 |      EXIT_CODE=1
  21 |  fi
  22 |  
  23 | +# "Macro CHECK_NONFATAL(condition) should be used instead of assert for RPC code, where it
  24 | +# is undersirable to crash the whole program. See: src/util/check.h"

practicalswift commented at 2:38 PM on October 30, 2019:

Should be "undesirable" :)

in test/lint/lint-assertions.sh:23 in d68f7cdde6 outdated

  19 | @@ -20,4 +20,14 @@ if [[ ${OUTPUT} != "" ]]; then
  20 |      EXIT_CODE=1
  21 |  fi
  22 |  
  23 | +# "Macro CHECK_NONFATAL(condition) should be used instead of assert for RPC code, where it

practicalswift commented at 2:38 PM on October 30, 2019:

Nit: Could remove surrounding quote :)

in test/lint/lint-assertions.sh:25 in d68f7cdde6 outdated

  19 | @@ -20,4 +20,14 @@ if [[ ${OUTPUT} != "" ]]; then
  20 |      EXIT_CODE=1
  21 |  fi
  22 |  
  23 | +# "Macro CHECK_NONFATAL(condition) should be used instead of assert for RPC code, where it
  24 | +# is undersirable to crash the whole program. See: src/util/check.h"
  25 | +OUTPUT=$(git grep -nE 'assert(.*);' -- "src/rpc" "src/wallet/rpc*")

practicalswift commented at 2:43 PM on October 30, 2019:

Nit: git grep -nE 'assert *\(.*\);' -- "src/rpc/" "src/wallet/rpc*"

Backslahes to match the quotes.
src/rpc/ instead of src/rpc to make it clear it is a directory.

practicalswift commented at 2:44 PM on October 30, 2019: contributor

Concept ACK

Very nice with the added linter!
adamjonas force-pushed on Oct 30, 2019
replace asserts in RPC code with CHECK_NONFATAL and add linter c98bd13e67

in test/lint/lint-assertions.sh:26 in 546507134f outdated

  19 | @@ -20,4 +20,15 @@ if [[ ${OUTPUT} != "" ]]; then
  20 |      EXIT_CODE=1
  21 |  fi
  22 |  
  23 | +# Macro CHECK_NONFATAL(condition) should be used instead of assert for RPC code, where it
  24 | +# is undesirable to crash the whole program. See: src/util/check.h
  25 | +# src/rpc/server.cpp is excluded from this check since it's mostly meta-code.
  26 | +OUTPUT=$(git grep -nE 'assert *\(.*\);' -- "src/rpc/" "src/wallet/rpc*" | grep -v -- "src/rpc/server.cpp")

practicalswift commented at 3:50 PM on October 30, 2019:

Nit:

git grep -nE 'assert *\(.*\);' -- "src/rpc/" "src/wallet/rpc*" ":(exclude)src/rpc/server.cpp"

adamjonas commented at 4:05 PM on October 30, 2019:

Oh, that's much better. Thanks.

adamjonas force-pushed on Oct 30, 2019
DrahtBot commented at 5:37 PM on October 30, 2019: member



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



Conflicts

No conflicts as of last run.
adamjonas requested review from practicalswift on Nov 2, 2019
adamjonas requested review from MarcoFalke on Nov 2, 2019
practicalswift approved
practicalswift commented at 2:41 PM on November 2, 2019: contributor

ACK c98bd13e675fbf5641ed64d551b63aaf55a1a8e9 -- diff looks correct
MarcoFalke referenced this in commit 94a26b192f on Nov 4, 2019
MarcoFalke merged this on Nov 4, 2019
MarcoFalke closed this on Nov 4, 2019
adamjonas deleted the branch on Nov 4, 2019
sidhujag referenced this in commit 4e0427ddd6 on Nov 7, 2019
jasonbcox referenced this in commit f0bbedc289 on Sep 8, 2020
sidhujag referenced this in commit 8c1e3c13b2 on Nov 10, 2020
MarcoFalke locked this on Dec 16, 2021

Contributors

Review Requested
MarcoFalke

Labels

RPC/REST/ZMQ

Linked (view graph)

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

replace asserts in RPC code with CHECK_NONFATAL and add linter #17318

Conflicts