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

maflcko · 2019-10-18T18:51:22Z

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.

DrahtBot · 2019-10-18T19:31:54Z

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.

ryanofsky

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.

src/util/check.h

maflcko · 2019-10-18T20:47:48Z

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.

maflcko · 2019-10-18T21:20:26Z

Addressed wording issues pointed out by @ryanofsky

practicalswift · 2019-10-19T14:36:06Z

ACK faeb666

Thanks for taking a step towards making the RPC interface more robust. That is needed :)

ryanofsky

Code review ACK faeb666

laanwj · 2019-10-28T10:59:46Z

src/rpc/misc.cpp

        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"


laanwj · 2019-10-28T11:00:12Z

ACK faeb666

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

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

… 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

…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

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

…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

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

…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

maflcko added the RPC/REST/ZMQ label Oct 18, 2019

maflcko force-pushed the 1910-utilCHECK_NONFATAL branch 2 times, most recently from faad2c2 to fa052fd Compare October 18, 2019 19:06

ryanofsky reviewed Oct 18, 2019

View reviewed changes

src/util/check.h Outdated Show resolved Hide resolved

src/util/check.h Outdated Show resolved Hide resolved

ryanofsky mentioned this pull request Oct 18, 2019

rpc: Report reason for replaceable txpool transactions #16490

Closed

util: Add CHECK_NONFATAL and use it in src/rpc

faeb666

maflcko force-pushed the 1910-utilCHECK_NONFATAL branch from fa052fd to faeb666 Compare October 18, 2019 21:19

bitcoin deleted a comment from wiranphowan Oct 22, 2019

ryanofsky approved these changes Oct 22, 2019

View reviewed changes

laanwj reviewed Oct 28, 2019

View reviewed changes

laanwj merged commit faeb666 into bitcoin:master Oct 28, 2019

maflcko deleted the 1910-utilCHECK_NONFATAL branch October 28, 2019 16:35

laanwj mentioned this pull request Oct 30, 2019

test: Add generatetodescriptor RPC #16943

Merged

adamjonas mentioned this pull request Oct 30, 2019

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

Merged

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

promag mentioned this pull request Aug 1, 2020

rpc/blockchain: Reset scantxoutset progress before inferring descriptors #19362

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

maflcko commented Oct 18, 2019

Uh oh!

DrahtBot commented Oct 18, 2019 •

edited

Loading

Uh oh!

ryanofsky left a comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Oct 18, 2019

Uh oh!

maflcko commented Oct 18, 2019

Uh oh!

practicalswift commented Oct 19, 2019

Uh oh!

ryanofsky left a comment

Uh oh!

laanwj Oct 28, 2019

Uh oh!

laanwj commented Oct 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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

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

Uh oh!

Conversation

maflcko commented Oct 18, 2019

Uh oh!

DrahtBot commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Oct 18, 2019

Uh oh!

maflcko commented Oct 18, 2019

Uh oh!

practicalswift commented Oct 19, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj commented Oct 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DrahtBot commented Oct 18, 2019 •

edited

Loading