rpc: Use CHECK_NONFATAL over Assert

maflcko commented at 3:36 pm on July 11, 2024: member

Any RPC method should not abort the whole node when an internal logic error happens.

Fix it by just aborting this single RPC method call when an error happens.

Also, fix the linter to find the fixed cases.

DrahtBot commented at 3:36 pm on July 11, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, tdb3, hodlinator, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29770 (index: Check all necessary block data is available before starting to sync by fjahr)

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.

DrahtBot added the label RPC/REST/ZMQ on Jul 11, 2024

in test/lint/lint-assertions.py:32 in fa8b587b91 outdated

26@@ -27,8 +27,9 @@ def main():
27     # checks should be used over assert. See: src/util/check.h
28     # src/rpc/server.cpp is excluded from this check since it's mostly meta-code.
29     exit_code = git_grep([
30-        "-nE",
31-        r"\<(A|a)ss(ume|ert) *\(.*\);",
32+        "--line-number",
33+        "--extended-regexp",
34+        r"\<(A|a)ss(ume|ert)\(",

stickies-v commented at 5:39 pm on July 11, 2024:

I think assuming no whitespace between a macro and the parenthesis is a regression, since that’s allowed? (same for BOOST_ASSERT)

 0#include <iostream>
 1#include <cstdlib>
 2
 3#define Assume(x) do { if (!(x)) std::terminate(); } while(0)
 4
 5int main() {
 6    Assume(true);  // Without space
 7    Assume (true); // With space
 8
 9    std::cout << "Both assertions passed." << std::endl;
10    return 0;
11}

stickies-v commented at 5:49 pm on July 11, 2024:

Also, when I run this new lint-assertions.py on the old rpc/blockchain.cpp it still doesn’t seem to catch the issue, I think because of the \< prefix? r"(A|a)ss(ume|ert) *\(", works fine for me.

maflcko commented at 6:57 pm on July 11, 2024:

I think assuming no whitespace between a macro and the parenthesis is a regression, since that’s allowed?

Yes, also a newline, but both are “forbidden” by clang-format. If you want to be more accurate you’ll have to write a clang-tidy plugin, but I won’t be doing this myself.

I think because of the \< prefix?

Keep in mind that macOS is not supported. What is grep --version | grep GNU on your machine?

stickies-v commented at 11:32 am on July 12, 2024:

If you want to be more accurate you’ll have to write a clang-tidy plugin, but I won’t be doing this myself.

The issue doesn’t seem very likely to happen, so I don’t think we should spend too much time on this indeed, but since clang-format doesn’t red the CI I feel like not removing two characters is a sensible approach to catch this. Either way is fine for me, so up to you.

Keep in mind that macOS is not supported.

Ugh, sorry didn’t consider that. I confirmed that the new test works fine on the old src code on my debian (grep GNU 3.8) machine.

0./test/lint/lint-assertions.py
1CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be used instead of assert for RPC code.
2src/rpc/blockchain.cpp:804:    const auto& first_unpruned{*Assert(blockman.GetFirstBlock(*chain_tip, /*status_mask=*/BLOCK_HAVE_MASK, first_block))};
3src/rpc/blockchain.cpp:811:    return Assert(first_unpruned.pprev)->nHeight;

stickies-v commented at 5:51 pm on July 11, 2024: contributor

Approach ACK. Code fixes fa8b587b91dfa8df28fc13589c511b871902670b LGTM but I think the linter needs some further changes.

DrahtBot added the label CI failed on Jul 11, 2024

tdb3 commented at 3:21 am on July 12, 2024: contributor

Approach ACK Ran lint-assertions.py against the old rpc/blockchain.cpp and similarly to what @stickies-v observed the Asserts did not appear to be caught.

0(Debian)$ grep --version | grep GNU
1grep (GNU grep) 3.8

maflcko commented at 7:23 am on July 12, 2024: member

Can you add exact steps to reproduce, starting from a fresh install of your operating system? Otherwise there is nothing that can be done. For a fresh install of Ubuntu 24.04 LTS (noble) or Debian GNU/Linux 12 (bookworm) it passes:

0git switch --detach fa8b587b91dfa8df28fc13589c511b871902670b && ( git show src | git apply --reverse ) && ./test/lint/lint-assertions.py 
1HEAD is now at fa8b587 rpc: Use CHECK_NONFATAL over Assert
2CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be used instead of assert for RPC code.
3src/rpc/blockchain.cpp:804:    const auto& first_unpruned{*Assert(blockman.GetFirstBlock(*chain_tip, /*status_mask=*/BLOCK_HAVE_MASK, first_block))};
4src/rpc/blockchain.cpp:811:    return Assert(first_unpruned.pprev)->nHeight;

rpc: Use CHECK_NONFATAL over Assert fa6270737e

maflcko commented at 7:33 am on July 12, 2024: member

Same in the CI https://cirrus-ci.com/task/6075434675208192?logs=lint#L755 and when using ( cd ./test/lint/test_runner/ && RUST_BACKTRACE=1 cargo run -- --lint=all_python_linters ) locally. Again, I’ll need exact steps to reproduce, otherwise there is little that can be done.

maflcko force-pushed on Jul 12, 2024

DrahtBot removed the label CI failed on Jul 12, 2024

stickies-v approved

stickies-v commented at 11:33 am on July 12, 2024: contributor

ACK fa6270737eb9655bfb4e29b7070ecb6cd2087b7f

DrahtBot requested review from tdb3 on Jul 12, 2024

tdb3 approved

tdb3 commented at 4:42 pm on July 12, 2024: contributor

ACK fa6270737eb9655bfb4e29b7070ecb6cd2087b7f

Can you add exact steps to reproduce, starting from a fresh install of your operating system? Otherwise there is nothing that can be done. For a fresh install of Ubuntu 24.04 LTS (noble) or Debian GNU/Linux 12 (bookworm) it passes:

Was unable to reproduce the original issue. I had manually changed the CHECK_NONFATALs back to Assert. It’s possible it was manual error, but not 100% sure. Either way, it doesn’t seem to happen now on a non-fresh/fresh Debian 12 install.

hodlinator approved

hodlinator commented at 12:35 pm on July 13, 2024: contributor

ACK fa6270737eb9655bfb4e29b7070ecb6cd2087b7f

Ran linter without the C++ change:

0$ ./test/lint/lint-assertions.py 
1CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be used instead of assert for RPC code.
2src/rpc/blockchain.cpp:804:    const auto& first_unpruned{*Assert(blockman.GetFirstBlock(*chain_tip, /*status_mask=*/BLOCK_HAVE_MASK, first_block))};
3src/rpc/blockchain.cpp:811:    return Assert(first_unpruned.pprev)->nHeight;

Including the C++ change silences the linter. ✅

Concept: Assert()/Assume() should be possible to use to document/enforce constraints on logic, but such logic should probably live in the code which the RPC code calls out to, not directly in the RPCs themselves. This was already the policy before this PR, just that the previous regexps required that matches end with );, so it missed the lines in question.

achow101 commented at 7:40 pm on July 16, 2024: member

ACK fa6270737eb9655bfb4e29b7070ecb6cd2087b7f

achow101 merged this on Jul 16, 2024

achow101 closed this on Jul 16, 2024

maflcko deleted the branch on Jul 16, 2024

rpc: Use CHECK_NONFATAL over Assert #30429

Code Coverage

Reviews

Conflicts