validation: assert that pindexPrev is non-null when required #14834

In ContextualCheckBlock, we are checking if pindexPrev == nullptr conditionally at the start, but then assume it is non-null later. This removes the latter assumption.

Is it even hypothetically possible for LOCKTIME_MEDIAN_TIME_PAST to be set on a genesis block?

Precedence is non-obvious in this case, so prefer adding parenthesis for clarity at least.

That's a good point. Is that the only case ContextualCheckBlock is called with pindexPrev == nullptr? (When block is the genesis block)

     const int nHeight = pindexPrev == nullptr ? 0 : pindexPrev->nHeight + 1;

Yeah, that pretty much proves it. It could change in the future, though, so I think the change is still warranted. It also plugs a static analyzer warning about null dereferencing, so there is that.

If it changes in the future, your modification here is a security vulnerability, since the block must be checked with the correct min locktime.

@luke-jr I changed the approach and am now asserting that pindexPrev is non-null for the case where the lock time flags have LOCKTIME_MEDIAN_TIME_PAST. That seems more aligned with your reasoning, I think, and I agree makes more sense in this case.

I guess that looks okay, although I found the logic relatively hard to follow.

Actually, I can just move it into the if block above. Doing.

utACK

utACK fbaaf782cea54dc433e72129ee1088b3169cdfd4

Very nice to get rid of this warning and thereby increase the signal-to-noise in static analyzer output! Explicit assumptions are better than implicit assumptions. Thanks!

utACK fbaaf782.

@promag Maybe I'm not looking closely enough -- how does that improve the patch?

@kallewoof yeah, it doesn't improve this patch, I've updated my comment shortly after, but now I've removed the suggestion.

Gotcha!

utACK fbaaf782cea54dc433e72129ee1088b3169cdfd4

utACK fbaaf782cea54dc433e72129ee1088b3169cdfd4

utACK fbaaf782cea54dc433e72129ee1088b3169cdfd4