Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 4, 2019
Merged

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

merged 1 commit into from
Nov 4, 2019

Conversation

adamjonas
Copy link
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 #17192

@maflcko
Copy link
Member

maflcko commented Oct 30, 2019

Concept ACK. Reviewers should verify that the two requirements of the macro are fullfilled: https://dev.visucore.com/bitcoin/doxygen/check_8h.html#a46a3e27097aa5e94bbf62075bad7016f

laanwj
laanwj reviewed Oct 30, 2019
View reviewed changes
maflcko
maflcko reviewed Oct 30, 2019
View reviewed changes
@adamjonas adamjonas force-pushed the replace-rpc-asserts-for-CHECK_NONFATAL branch from db98d6f to d68f7cd Compare October 30, 2019 14:29
practicalswift
practicalswift reviewed Oct 30, 2019
View reviewed changes
Copy link
Contributor

@practicalswift practicalswift left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

Very nice with the added linter!

@adamjonas adamjonas force-pushed the replace-rpc-asserts-for-CHECK_NONFATAL branch from d68f7cd to 5465071 Compare October 30, 2019 15:25
practicalswift
practicalswift reviewed Oct 30, 2019
View reviewed changes
@adamjonas adamjonas force-pushed the replace-rpc-asserts-for-CHECK_NONFATAL branch from 5465071 to c98bd13 Compare October 30, 2019 16:05
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 30, 2019 •
edited
Loading

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

Conflicts

No conflicts as of last run.

practicalswift
practicalswift approved these changes Nov 2, 2019
View reviewed changes
Copy link
Contributor

@practicalswift practicalswift left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK c98bd13 -- diff looks correct

maflcko pushed a commit that referenced this pull request Nov 4, 2019
Merge #17318: replace asserts in RPC code with CHECK_NONFATAL and add…
94a26b1 
… 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
@maflcko maflcko merged commit c98bd13 into bitcoin:master Nov 4, 2019
@adamjonas adamjonas deleted the replace-rpc-asserts-for-CHECK_NONFATAL branch November 4, 2019 16:38
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 7, 2019
Merge bitcoin#17318: replace asserts in RPC code with CHECK_NONFATAL …
4e0427d 
…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
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 8, 2020
@majcosta
[backport#17318] replace asserts in RPC code with CHECK_NONFATAL and …
f0bbedc 
…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
Merge bitcoin#17318: replace asserts in RPC code with CHECK_NONFATAL …
8c1e3c1 
…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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laanwj laanwj laanwj left review comments

@maflcko maflcko Awaiting requested review from maflcko

+1 more reviewer

@practicalswift practicalswift practicalswift approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
RPC/REST/ZMQ
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@adamjonas @maflcko @DrahtBot @laanwj @practicalswift @fanquake