Paths

Table of Contentst

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

Authored by nakihito on Apr 16 2020, 19:29.

Details

Reviewers

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Commits

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

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 PR17192

This is an out-of-order backport so that the older backports can be done without the asserts like here.

Test Plan

make
test_runner.py

ninja
test_runner.py

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

nakihito created this revision.Apr 16 2020, 19:29

Owners added a reviewer: Restricted Owners Package.Apr 16 2020, 19:29

Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 16 2020, 19:29

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those PRs have been inserted into the summary for reference.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

nakihito edited the summary of this revision. (Show Details)Apr 16 2020, 19:30

nakihito mentioned this in D5586: Merge #14885: rpc: Assert named arguments are unique in RPCHelpMan.Apr 16 2020, 19:45

nakihito added a child revision: D5586: Merge #14885: rpc: Assert named arguments are unique in RPCHelpMan.Apr 16 2020, 19:57

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10253: Diff 18872.Apr 16 2020, 19:59

deadalnix requested changes to this revision.Apr 17 2020, 00:32

deadalnix added a subscriber: deadalnix.

deadalnix added inline comments.

src/rpc/util.h
101 ↗	(On Diff #18872)	What about `RPCResult` ?

This revision now requires changes to proceed.Apr 17 2020, 00:32

nakihito requested review of this revision.Apr 17 2020, 18:12

nakihito added inline comments.

src/rpc/util.h

101 ↗

(On Diff #18872)

This is an out-of-order backport so that the older backports can be done without the asserts like here.

From the developer notes:

    Assertions should not have side-effects
        *Rationale*: Even though the source code is set to refuse to compile with assertions disabled, having side-effects in assertions is unexpected and makes the code harder to understand

If assert is compiled out the insertion is lost.

It hasn't been backported yet to avoid the above.

deadalnix accepted this revision.Apr 18 2020, 08:33

This revision is now accepted and ready to land.Apr 18 2020, 08:33

Rebased.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10306: Diff 18961.Apr 20 2020, 16:59

Closed by commit rABCf7690c0f382b: Merge #17192: util: Add CHECK_NONFATAL and use it in src/rpc (authored by Wladimir J. van der Laan <laanwj@protonmail.com>, committed by nakihito). · Explain WhyApr 20 2020, 17:09

This revision was automatically updated to reflect the committed changes.

nakihito added a commit: rABCf7690c0f382b: Merge #17192: util: Add CHECK_NONFATAL and use it in src/rpc.

nakihito added a commit: rSTAGINGf7690c0f382b: Merge #17192: util: Add CHECK_NONFATAL and use it in src/rpc.May 5 2020, 19:03