Remove checks for nullptr from BlockAssembler::CreateNewBlock

cculianu commented at 12:21 pm on June 14, 2020: none

Motivation/rationale: Minor nit.

Background:

See: https://en.cppreference.com/w/cpp/memory/new/operator_new

operator new() does not normally ever return nullptr unless one explicitly uses the std::nothrow version or unless one redefines it. Neither is the case in this entire codebase, hence the check for nullptr for the return value from BlockAssembler::CreateNewBlock was entirely superfluous and all branches that test it would always evaluate to false. So, it can be safely removed in the interests of code quality.

tl;dr: operator new() either succeeds or throws std::bad_alloc, it can never return nullptr.

Remove checks for nullptr from BlockAssembler::CreateNewBlock

operator new() does not normally ever return nullptr unless one
explicitly uses the `std::nothrow` version or unless one redefines it.  Neither
is the case in this entire codebase, hence the check for nullptr for the
return value from BlockAssembler::CreateNewBlock was entirely
superfluous and all brances that test it would always evaluate to
false. So, it can be safely removed in the interests of code quality.

See: https://en.cppreference.com/w/cpp/memory/new/operator_new

tl;dr: operator new() either succeeds or throws std::bad_alloc, it can
never return nullptr.

125ce3b256

DrahtBot added the label Mining on Jun 14, 2020

DrahtBot added the label RPC/REST/ZMQ on Jun 14, 2020

MarcoFalke commented at 12:48 pm on June 14, 2020: member

If it is impossible to return a nullptr, then why not return a plain CBlockTemplate?

cculianu commented at 1:05 pm on June 14, 2020: none

@MarcoFalke Good question. I’m not sure what the motivation is behind the BlockAssembler class and its design. Outside of tests, it’s only ever used in getblocktemplate and generateBlocks in rpc/mining.cpp. It is always created as an ephemeral object on the stack and then after it is done creating a CBlockTemplate, it is immediately destroyed after the stack frame ends.

Why it allocates a CBlockTemplate as a pointer and then returns it via a unique_ptr is a good question. I guess because a block template is potentially very “heavy” and the people designing the subsystem didn’t want to mess with std::move and/or rvalue reference && semantics, and unique_ptr is just easier to think about for some people? Or maybe this evolved from some older code that did it with raw pointers? I don’t know… If I had done it perhaps I would have just used a CBlockTemplate and ensured it had proper std::move semantics and returned it directly by value, leveraging RVO and && move semantics.

MarcoFalke commented at 1:25 pm on June 14, 2020: member

What I meant is that simply removing nullptr checks is scary and will also fail to compile on some compilers/sanitizers/static analysers with warnings and errors enabled. Also, if the template ever only exists on the stack, then I don’t see why it should live in the heap.

I am not sure if there is a way in C++ to ensure an object can be efficiently moved, but a pre-C++11 alternative would be:

0CBlockTemplate on_the_stack;
1BlockAssembler().CreateNewBlock(coinbase_script, on_the_stack);

cculianu commented at 1:37 pm on June 14, 2020: none

Yeah that was the pre-C++11 canonical way to do it. Also just returning the value was pretty much always guaranteed to not produce any copies and be equivalent to the “pass down a non-const ref” way.. (RVO/copy elision basically does that for you internally anyway).

The example you show is the cheapest way (as is a proper RVO return by value), for sure. Move semantics are “almost free” too – you just have to pay the extra cost of swapping pointers, etc (CBlockTemplate’s heaviest members are the vector of tx refs and other stuff and so you pay the cost of that pointer swap…).

What I meant is that simply removing nullptr checks is scary and will also fail to compile

:) It is only scary if you are thinking about it as just a fancy malloc. In C, definitely, not checking NULL is scary.

However, in C++, new can never return nullptr (at least not the new used in this entire codebase). It’s guaranteed to throw or succeed. Those are its only two postconditions. In light of that, there are no sanitizers or compilers that will error out on it or complain – since the standard in C++ specifies exactly what new does…

If you find this PR useless or scary, feel free to close it. I figured rather than complain I could help out and fix the little nits here and there.

No worries either way.

MarcoFalke commented at 2:11 pm on June 14, 2020: member

new can never return nullptr

Oh, I meant that compilers can’t know that CreateNewBlock never returns a nullptr.

0const auto b = CreateNewBlock();
1b->Get();  // <-- Warning/Error here

If you find this PR useless

We do appreciate refactoring changes that improve the code base. My feedback was about the current version of the pull request, which I believe can not be merged as-is.

cculianu commented at 4:03 pm on June 14, 2020: none

No static analyzer or compiler expects you to check every single pointer you get given or receive as a return value. Having such a strict requirement would mean your code would be riddled with boilerplate checks everywhere. (instead, at best, some static analyzers sometimes use heuristics to warn you about obviously bad usages).

Just to reiterate – code like this:

0auto ptr = std::make_unique<MyClass>(arg1, arg2);
1if (!ptr)
2  // handle error here

Is unidiomatic and kind of silly… since it can never be false. For that reason no static analyzer that I know of expects you to check unique_ptr return value either.

Background: Usually static analyzers do not expect you to check for NULL for every single pointer return type you use. Doing so would be boilerplate city. They usually expect you to check for NULL before using only if the analyzer concludes (based on heuristics) that the pointer may be NULL sometimes. For example, if you later check for NULL in the function – this indicates to the analyzer that you expect it to be NULL sometimes. If it cannot conclude that the pointer may be NULL, it will not complain.

MarcoFalke commented at 5:48 pm on June 14, 2020: member

For our codebase, unique_ptr means that the pointer can be null. E.g.

So I think it would be confusing to have one exception where a pointer type is used but at the same time it can not be null.

cculianu commented at 6:44 pm on June 14, 2020: none

Right, but in this case the contract is that it cannot be null.

I get it – this is a minor nit and not worth worrying about – no worries. I still maintain that it would be a code quality improvement. It’s a bit silly to see people checking new’s return value as if it can ever return nullptr. If this doesn’t utterly make you “wtf” when you read it:

0    pblocktemplate.reset(new CBlockTemplate());
1
2    if(!pblocktemplate.get())
3        return nullptr;

Then that’s your prerogative, I guess.

But I get it. You don’t want to worry about this. Feel free to close this. Like I said – no harm done, no offense taken, and no worries.

DrahtBot commented at 7:52 pm on June 15, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19283 (refactor: Remove unused BlockAssembler::pblock member var by MarcoFalke)

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.

laanwj commented at 6:19 pm on July 9, 2020: member

I agree with @MarcoFalke. Please make it either impossible to return a nullptr (by returning a while object) or keep the checks somehow. Simply removing them is scary. Even though the check might, with some reasoning, be unnecessary right now, for maintainability and robustness some defense in depth doesn’t hurt so NACK on this change as it is.

DrahtBot commented at 6:38 pm on July 9, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the label Needs rebase on Jul 9, 2020

cculianu commented at 8:02 am on July 10, 2020: none

Please make it either impossible to return a nullptr (

My point is – It’s already impossible to return a nullptr. Which is why the check is silly. It’s the equivalent of doing:

0   int a = 2;
1   if (a != 2) {
2      // .... code here to throw
3   }

The above example may seem like an exaggeration, of course – but if you stop to think about it – it really is that silly. Anyway, I’m closing this. I feel like I’m fighting against the wind here. Good luck.

cculianu closed this on Jul 10, 2020

DrahtBot locked this on Feb 15, 2022

Remove checks for nullptr from BlockAssembler::CreateNewBlock #19273

Conflicts