Make sure all callers of LookupBlockIndex(...) check for nullptr before dereferencing (CBlockIndex*)

[practicalswift]

Make sure all callers of LookupBlockIndex(...) check for nullptr before dereferencing (CBlockIndex*).

[DrahtBot]

Reviewers, this pull request conflicts with the following ones:

• Refactor: Start to separate wallet from node #14437 (Refactor: Start to separate wallet from node by ryanofsky)
• Refactor: separate wallet from node #10973 (Refactor: separate wallet from node by ryanofsky)

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.

[domob1812]

A few nits, but otherwise utACK 5983a8d9eda7a3e53c4d9ec715f802d434ba7cf8.

[domob1812]

Nit: You could make this const CBlockIndex*.

[domob1812]

While you are here, you could make this const.

[domob1812]

This is a matter of taste, but I think that the more explicit pBestIndex != nullptr is more readable as to the intent. I'm not sure if there's a style rule for how to test pointers against null which goes against that, though.

[domob1812]

Can also be const.

[practicalswift]

@domob1812 Thanks for reviewing. Feedback addressed. Please re-review :-)

[promag]

This is a matter of taste, but I think that the more explicit pBestIndex != nullptr is more readable as to the intent

Not sure but I think the most frequent is assert(pointer) so I would go that way. If you feel otherwise then submit a PR to update developer notes first?

[promag]

Is this even possible? If so then it's new behaviour, should have a test, otherwise should be an assertion.

[promag]

Unrelated to this PR. Above you see that if vHeaders.push is called then pBestIndex = pindex was called. IMO this should be removed.

[practicalswift]

You don't mean that the assertion should be removed? How do you mean "unrelated to this PR" in this context? :-)

[promag]

This is not an assertion of a LookupBlockIndex result, that's done in L3400.

[promag]

Is this even possible? If not, should be an assertion.

[promag]

Should be an assertion? How can it be nullptr since confirms > 0?

[practicalswift]

@promag Thanks for reviewing! Feedback addressed. Please re-review :-)

[promag]

This PR only changes how the process terminates in case there is some bug. An early assertion makes sense when there are side effects until the dereferencing, which I believe is not the case here.

Also, see #13969 (review) above.

[promag]

Why this assertion?

[practicalswift]

@promag stopBlock is dereferenced a few lines down:

response.pushKV("stop_height", stopBlock->nHeight);

[promag]

And that's fine, if by any chance stopBlock is null then the process will terminate. In this simple code the assertion is pointless IMO.

[practicalswift]

@promag I'm not sure I follow. The assumption underlying your reasoning seems to be that the only difference between …

assert(foo);
foo->bar();

… and …

// assert(foo);
foo->bar();

... at runtime would be the line number at which the process terminates (given foo == nullptr). That seems like a very risky assumption to make? :-)

[promag]

@practicalswift I said

which I believe is not the case here.

[practicalswift]

@promag Ah, I think I misunderstood what you wrote. You're simply suggesting that the assertions should be made right before dereferencing and not earlier (assuming no side effects before dereferencing)?

I misunderstood and thought that you meant that the only potential problem caused by null pointer dereferencing would be process termination :-)

[promag]

You're simply suggesting that the assertions should be made right before dereferencing and not earlier

I mean that an assertion makes sense to prevent side effects before dereferencing.

I also think that these particular assertions are not that relevant or useful.

IMHO of course, and I'd wait for more reviews.

[ryanofsky]

I don't think it would be a good idea to make this change without adding a style guideline about when it is actually recommended to assert that a pointer is not null before dereferencing.

Personally, I don't like these asserts because I think they are distracting and make the code verbose and awkward to read, but I guess the benefit of adding them would be to have error messages that make it easier to determine the causes of crashes when core dumps aren't available. Another benefit might be that if there was a guideline requiring asserts before pointer dereferences, it might encourage more uses of c++ references instead of pointers, which I think would be good.

Will add my code utACK 75f360c if other people want this change, but personally would like it better if the change was dropped or accompanied by a developer guideline.

[promag]

it might encourage more uses of c++ references instead of pointers

@ryanofsky +1