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 checks with a macro THROW_LOGIC_ERROR_IF_ASSERT_FAILS(condition) that does what it says. That logic error will then be returned to the rpc caller and will include instructions to report the error to our issue tracker.
Any objections or naming suggestions for that macro?
There is also a broader proposal to get rid of asserts in non-rpc code:
I tend to agree. I think assertion errors should be used for critical errors when the sane state of the process can’t be guaranteed anymore. When internal assumptions don’t hold anymore, and when continuing would have a high risk of causing further corruption.
Then it’s valid to crash.
For all other cases, please use some form of error handling. In RPC code, we use exceptions for this.
I would encourage more use of std::logic_error instead of asserts. I see logic errors and asserts as semantically equivalent, both indicating the presence of bugs, but logic_errors working better with unit tests. I think the suggested macro could be a good idea, because if (cond) throw std::logic_error(strprintf(...)) is pretty verbose, and I think that discourages use. (Grepping for logic_error` I think every existing logic_error throw actually comes from me and no one other bitcoin developer has written one.)
However, I don’t think it would be a good idea to ever catch std::logic_error outside of unit tests. I think that defeats the major advantage of logic_errors as logical assertions compatible with having good test coverage.
If the goal here is better isolation from nonfatal bugs in RPCs, I think I’d suggest instead defining a new NonfatalError class inheriting from std::exception, a CHECK_NONFATAL or similar macro throwing nonfatal error exceptions, and code to catch and handle those exceptions somewhere in the RPC framework.
CHECK_NONFATAL / NonFatalCheckError might be more consistent naming
logic_errors from rpc threads and returning them as a verbose string to the caller. We already do that right now. Are you advocating to not catch them and crash? There shouldn’t be any fatal error in rpc code that would warrant a crash. The only case I can think of is a rpc thread calling into validation and running into a database error, but then validation will take care of aborting the whole program.
Every time I have ever used std::logic_error, I’ve used it to mean that I’ve encountered a bug and I want the program to crash. If I found a bug and don’t want the program to crash, I would use a different exception like std::runtime_error or a custom exception like the suggested CHECK_NONFATAL / NonFatalCheckError. Again, I think the nice thing about logic_error is that it can provide the same semantics as assert without getting in the way of unit test coverage. If we start catching logic_error exceptions then it loses those advantages because I no longer know what to expect when I throw it.
We already do that right now.
logic_error doesn’t inherit from runtime_error, so I hope this isn’t the case
There shouldn’t be any fatal error in rpc code that would warrant a crash
This is subjective and is something reasonable people can disagree about. But I think it would be best for authors who add checks to clearly distinguish cases where they do and don’t want crashes by using assert or logic_error in fatal cases, and CHECK_NONFATAL or std::runtime_error or similar in nonfatal cases.
I’m fully onboard with your original suggestion to get rid of asserts in RPC code. All I’m suggesting is that instead of replacing them with logic_error throws, you replace them with something like NonFatalCheckError throws. You already proposed defining a custom THROW_LOGIC_ERROR_IF_ASSERT_FAILS macro, so I don’t see what objection you would have to a shorter, clearer CHECK_NONFATAL macro instead.
logic_error for both fatal and nonfatal errors like you are suggesting. I still think it would be better for clarity to use a distinct exception type if you are going out of your way to define a new custom macro. But it does look like our current code is bad enough that using std::logic_error wouldn’t be worse in a practical way right now.
Labels
RPC/REST/ZMQ