util: Add CHECK_NONFATAL and use it in src/rpc #17192
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
faad2c2 to
fa052fd
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
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. |
fa052fd to
faeb666
Compare
|
Addressed wording issues pointed out by @ryanofsky |
|
ACK faeb666 Thanks for taking a step towards making the RPC interface more robust. That is needed :) |
laanwj
reviewed
Oct 28, 2019
| @@ -540,6 +541,7 @@ static UniValue echo(const JSONRPCRequest& request) | |||
| throw std::runtime_error( | |||
| RPCHelpMan{"echo|echojson ...", | |||
| "\nSimply echo back the input arguments. This command is for testing.\n" | |||
| "\nIt will return an internal bug report when exactly 100 arguments are passed.\n" | |||
|
ACK faeb666 |
laanwj
added a commit
that referenced
this pull request
Oct 28, 2019
faeb666 util: Add CHECK_NONFATAL and use it in src/rpc (MarcoFalke) Pull request description: 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 `assert`s 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. ACKs for top commit: practicalswift: ACK faeb666 laanwj: ACK faeb666 ryanofsky: Code review ACK faeb666 Tree-SHA512: 9b748715a5e0767ac11f1324a95a3a6ec672a0e0658013492219223bda83ce4b1b447fd8183bbb235f7df5ef7dddda7666ad569544b4d61cc65f232ca7a800ec
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Oct 28, 2019
faeb666 util: Add CHECK_NONFATAL and use it in src/rpc (MarcoFalke) Pull request description: Fixes bitcoin#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 `assert`s 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. ACKs for top commit: practicalswift: ACK faeb666 laanwj: ACK faeb666 ryanofsky: Code review ACK faeb666 Tree-SHA512: 9b748715a5e0767ac11f1324a95a3a6ec672a0e0658013492219223bda83ce4b1b447fd8183bbb235f7df5ef7dddda7666ad569544b4d61cc65f232ca7a800ec
maflcko
pushed a commit
that referenced
this pull request
Nov 4, 2019
… linter c98bd13 replace asserts in RPC code with CHECK_NONFATAL and add linter (Adam Jonas) Pull request description: - 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 #17192 ACKs for top commit: practicalswift: ACK c98bd13 -- diff looks correct Tree-SHA512: a16036b6bbcca73a5334665f66e17e1756377d582317568291da1d727fc9cf8c84bac9d9bd099534e1be315345336e5f7b66b93793135155f320dc5862a2d875
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Nov 7, 2019
…and add linter c98bd13 replace asserts in RPC code with CHECK_NONFATAL and add linter (Adam Jonas) Pull request description: - 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 bitcoin#17192 ACKs for top commit: practicalswift: ACK c98bd13 -- diff looks correct Tree-SHA512: a16036b6bbcca73a5334665f66e17e1756377d582317568291da1d727fc9cf8c84bac9d9bd099534e1be315345336e5f7b66b93793135155f320dc5862a2d875
deadalnix
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Apr 20, 2020
Summary:
faeb6665362e35f573ad715ade0ef2db62d71839 util: Add CHECK_NONFATAL and use it in src/rpc (MarcoFalke)
Pull request description:
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 `assert`s 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.
ACKs for top commit:
practicalswift:
ACK faeb6665362e35f573ad715ade0ef2db62d71839
laanwj:
ACK faeb6665362e35f573ad715ade0ef2db62d71839
ryanofsky:
Code review ACK faeb6665362e35f573ad715ade0ef2db62d71839
Tree-SHA512: 9b748715a5e0767ac11f1324a95a3a6ec672a0e0658013492219223bda83ce4b1b447fd8183bbb235f7df5ef7dddda7666ad569544b4d61cc65f232ca7a800ec
Backport of Core [[bitcoin/bitcoin#17192 | PR17192]]
This is an out-of-order backport so that the older backports can be done without the asserts like [[ https://reviews.bitcoinabc.org/D5586#inline-34624 | here ]].
Test Plan:
make
test_runner.py
ninja
test_runner.py
Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix
Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix
Subscribers: deadalnix
Differential Revision: https://reviews.bitcoinabc.org/D5746
jasonbcox
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Sep 8, 2020
…add linter Summary: replace asserts in RPC code with CHECK_NONFATAL and add linter (Adam Jonas) Pull request description: - 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 bitcoin/bitcoin#17192 --- Depends on D7402 Backport of Core [[bitcoin/bitcoin#17318 | PR17318]] Test Plan: ninja check check-functional For the linter, change a CHECK_NONFATAL back to assertion on a file inside the src/rpc diretory, an rpcXXXXX.cpp file in src/wallet and another file in the latter directory whose filename _does not_ start with 'rpc' only the first two should trigger the new linter's error Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7405
sidhujag
pushed a commit
to syscoin-core/syscoin
that referenced
this pull request
Nov 10, 2020
faeb666 util: Add CHECK_NONFATAL and use it in src/rpc (MarcoFalke) Pull request description: Fixes bitcoin#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 `assert`s 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. ACKs for top commit: practicalswift: ACK faeb666 laanwj: ACK faeb666 ryanofsky: Code review ACK faeb666 Tree-SHA512: 9b748715a5e0767ac11f1324a95a3a6ec672a0e0658013492219223bda83ce4b1b447fd8183bbb235f7df5ef7dddda7666ad569544b4d61cc65f232ca7a800ec
sidhujag
pushed a commit
to syscoin-core/syscoin
that referenced
this pull request
Nov 10, 2020
…and add linter c98bd13 replace asserts in RPC code with CHECK_NONFATAL and add linter (Adam Jonas) Pull request description: - 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 bitcoin#17192 ACKs for top commit: practicalswift: ACK c98bd13 -- diff looks correct Tree-SHA512: a16036b6bbcca73a5334665f66e17e1756377d582317568291da1d727fc9cf8c84bac9d9bd099534e1be315345336e5f7b66b93793135155f320dc5862a2d875
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #17181
Currently, we use
assertin 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 allasserts with a macroCHECK_NONFATAL(condition)that throws a runtime error when the condition evaluates tofalse. That runtime error will then be returned to the rpc caller and will include instructions to report the error to our issue tracker.