rpc: Use CHECK_NONFATAL over Assert #30429

maflcko · 2024-07-11T15:36:15Z

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 · 2024-07-11T15:36:18Z

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.

stickies-v

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

test/lint/lint-assertions.py

tdb3

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.

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

maflcko · 2024-07-12T07:23:47Z

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:

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

maflcko · 2024-07-12T07:33:48Z

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.

stickies-v

ACK fa62707

test/lint/lint-assertions.py

tdb3

ACK fa62707

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

ACK fa62707

Ran linter without the C++ change:

$ ./test/lint/lint-assertions.py 
CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be used instead of assert for RPC code.
src/rpc/blockchain.cpp:804:    const auto& first_unpruned{*Assert(blockman.GetFirstBlock(*chain_tip, /*status_mask=*/BLOCK_HAVE_MASK, first_block))};
src/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 · 2024-07-16T19:40:56Z

ACK fa62707

Summary: From the PR description: > The function GetFirstStoredBlock() helps us find the first block for which we have data. So far this function only looked for a block with BLOCK_HAVE_DATA. However, this doesn't mean that we also have the undo data of that block, and undo data might be required for what a user would like to do with those blocks. One example of how this might happen is if some blocks were fetched using the getblockfrompeer RPC. Blocks fetched from a peer will have data but no undo data. > > In the second commit I am applying the undo check to the RPCs that report pruneheight to the user. I find this much more intuitive because I think the user expects to be able to do all operations on blocks up until the pruneheight but that is not the case if undo data is missing. I personally ran into this once before and now again when testing for assumeutxo when I had used getblockfrompeer. The following commit adds test coverage for this change of behavior. This is a partial backport of [[bitcoin/bitcoin#29668 | core#29668]] and [[bitcoin/bitcoin#30429 | core#30429]] bitcoin/bitcoin@4a19750 bitcoin/bitcoin@8789dc8 bitcoin/bitcoin@fa62707 Depends on D17924 Test Plan: `ninja all check-extended` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D17925

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

stickies-v reviewed Jul 11, 2024

View reviewed changes

test/lint/lint-assertions.py Show resolved Hide resolved

DrahtBot added the CI failed label Jul 11, 2024

tdb3 reviewed Jul 12, 2024

View reviewed changes

rpc: Use CHECK_NONFATAL over Assert

fa62707

maflcko force-pushed the 2407-rpc-no-assert branch from a183627 to fa62707 Compare July 12, 2024 07:35

DrahtBot removed the CI failed label Jul 12, 2024

stickies-v approved these changes Jul 12, 2024

View reviewed changes

test/lint/lint-assertions.py Show resolved Hide resolved

DrahtBot requested a review from tdb3 July 12, 2024 11:33

tdb3 approved these changes Jul 12, 2024

View reviewed changes

DrahtBot mentioned this pull request Jul 13, 2024

index: Check all necessary block data is available before starting to sync #29770

Open

hodlinator approved these changes Jul 13, 2024

View reviewed changes

achow101 merged commit ad5579e into bitcoin:master Jul 16, 2024

maflcko deleted the 2407-rpc-no-assert branch July 16, 2024 20:13

bitcoin locked and limited conversation to collaborators Jul 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

rpc: Use CHECK_NONFATAL over Assert #30429

rpc: Use CHECK_NONFATAL over Assert #30429

Uh oh!

maflcko commented Jul 11, 2024

Uh oh!

DrahtBot commented Jul 11, 2024 •

edited

Loading

Uh oh!

stickies-v left a comment

Uh oh!

Uh oh!

tdb3 left a comment •

edited

Loading

Uh oh!

maflcko commented Jul 12, 2024

Uh oh!

maflcko commented Jul 12, 2024

Uh oh!

stickies-v left a comment

Uh oh!

Uh oh!

tdb3 left a comment

Uh oh!

hodlinator left a comment

Uh oh!

achow101 commented Jul 16, 2024

Uh oh!

Uh oh!

rpc: Use CHECK_NONFATAL over Assert #30429

rpc: Use CHECK_NONFATAL over Assert #30429

Uh oh!

Conversation

maflcko commented Jul 11, 2024

Uh oh!

DrahtBot commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tdb3 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 12, 2024

Uh oh!

maflcko commented Jul 12, 2024

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jul 16, 2024

Uh oh!

Uh oh!

DrahtBot commented Jul 11, 2024 •

edited

Loading

tdb3 left a comment •

edited

Loading