Rather rename it to CheckBlockPoW then... but considering that's already covered by CheckProofOfWork, perhaps CheckBlockHeader should just be removed entirely and inlined where applicable.

I'm all for unifying CheckBlockHeader with CheckProofOfWork in pow.o, but one step at a time.
Here are more future suggestions:
jtimon/bitcoin@pre-0.14-dont-call-me...jtimon:pre-0.14-encapsulate-pow

Currently the only difference between CheckProofOfWork and CheckBlockHeader and thus prevent from unifying the two functions are:

1. Using CValidationState to give more detailed errors with CheckBlockHeader.
   Possible solution: More detailed errors are good, let's call with CheckBlockHeader and bring CValidationState down to CheckProofOfWork too.

2. Not all CheckProofOfWork callers use CBlockHeader, this is can be a performance problem for refactor (see 'Pow: Performance hit: pre-static-CheckProofOfWork...').

We can rename CheckBlockHeader to CheckProofOfWork, CheckBlockPoW or whatever, no problem.

utACK.

I'm not strongly in favor of this change though, given that if more functionality gets added to checkblockheader ever you would want to add fCheckPOW again.

I am opposed to the other changes mentioned, ie, making checkproofofwork add the dos scores, seems better to keep checkproofofwork a pure function.

If there are plans to add more functionality back to this functions (which I wouldn't oppose too), and I can only think that you are referring to this check https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L2967 then we definitely don't want either this PR or @luke-jr 's suggestion of going further and fully remove CheckBlockHeader in one step.

But if we don't plan to put that code back there, then the fCheckPOW parameter doesn't make any sense in CheckBlockHeader. Better is better. This was supposed to be the simplest fix for something that honestly looks embarrassing when I think of reviewers reading this function without looking at the history or thinking about https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L2967 .

Let's please first decide whether we want to remove fCheckPOW from this function or we want to bring the check block.GetBlockTime() > nAdjustedTime + MAX_FUTURE_BLOCK_TIME) back here (which I'm not opposed to and I'm happy to write myself if that's what's preferred, I don't care).

After this, as @luke-jr points out CheckBlockHeader is just a wrapper for CheckProofOfWork but passing CValidationState. An option would be to remove it, another option would be to use CValidationState or maybe put it in https://github.com/bitcoin/bitcoin/blob/master/src/script/script_error.h (and maybe rename script_error.h to consensus/errors.o too, who knows).

But if we can't decide whether fCheckPOW here is pointless or not (and I don't want to bet, but maybe the compiler has already decided for us at a lower level), I don't want to put more decisions in the same PR.

@luke-jr @JeremyRubin Does that sound reasonable to you?

For more context:
The comments are really appreciated in the context of #9177 which I'm trying to complete while rewriting in a simpler and less disruptive way (it will remain on top of #8994 though, since that currently allows to reuse all existing rpc/python tests except segwit.py for any custom chain [or even testnet3 and mainchain] instead of having to replace regtest with your new testchain to do so).

At the same time, I understand neither this nor #9177 are a priority for bitcoin core itself, since I believe it is unlikely #9177 will ever be merged on mainnet unless I find a way that doesn't affect performance at all on mainchain and at the same time is not very disruptive (which as said I believe it's unlikely). It would be very useful for rebasing elements to have #9177 completed and merged in bitcoin core, but bitcoin core shouldn't care about that since not all it's users care about elements.
On the other hand I think #8855 #8994 can be very useful for testing within bitcoin core itself, but have no relation with this PR, only with #9177.

PS: Sorry for the text wall, this shouldn't happen in a +4-4 PR.

utACK 7b3b201

If needed later, can add again.

Agree with @NicolasDorier - if CheckBlockHeader() is needed in future, it can be added.

This seems to be strictly an improvement, so utACK 7b3b201.

@NicolasDorier @jnewbery By "CheckBlockHeader() can be added back if needed" I understand that you're not only ok with this but also with inlining CheckBlockHeader as suggested by @luke-jr in a future PR and therefore not bringing the MAX_FUTURE_BLOCK_TIME check back from ContextualCheckBlockHeader to CheckBlockHeader in the foreseeable future.

@jtimon yes, I am fine with both. Indeed no much point to have CheckBlockHeader anymore after this PR.

We can also pass the blockHash to CheckBlockHeader, that would make it closer to CheckProofOfWork, which I imagine would be more interesting to @luke-jr ?

utACK 7b3b201

utACK 7b3b201, good simplification.

Sorry, forgot to make the function static while at it.

utACK 9855ecaf880840a42f2a9678dc6d9e753c11c531. Only change since last review is making the function static.

This seems like it is ready to be merged. It's very minor, 4 people acked it, and #10339 depends on it.

NACK, there is no value in doing this, and it only makes the code less readable because it conflates the block header with the PoW.

@luke-jr so lets just remove CheckBlockHeader instead and call CheckProofOfWork where we need to?

@TheBlueMatt I don't see how that's an improvement, but at least it sounds non-confusing, and as such I wouldn't object.

Needed rebase. Rebased on top of #10339 instead of the other way around.

@TheBlueMatt I don't see how that's an improvement, but at least it sounds non-confusing, and as such I wouldn't object.

I'm confused, I thought removing CheckBlockHeader was your preference #9717 (comment)

makes the code less readable because it conflates the block header with the PoW

Right now they are the same thing (except for CValidationState and the fDontDoAnything param that this PR tries to remove).

I'm confused, I thought removing CheckBlockHeader was your preference #9717 (comment)

I'd rather that than making the code more confusing, but I don't see how either change is useful.

Right now they are the same thing

They are conceptually different things. If we add an additional check to CheckBlockHeader, we would not expect it to also be skipped when PoW checks are skipped.

Sorry, removed the commit from #10339 again.

I'd rather that than making the code more confusing, but I don't see how either change is useful.

It is useful because it simplifies an interface that is more complex than it needs to be. Currently, if you're calling the function with fCheckPOW=false (could as well be named fActuallyDoSomethingWhenCallingThisFunction), you can just not call it.
This doesn't make the code more confusing, the opposite is true.
At a minimum, we could mae the nonsense argument non optional as a start.

They are conceptually different things. If we add an additional check to CheckBlockHeader, we would not expect it to also be skipped when PoW checks are skipped.

This argument actually makes sense to me.
Do you have a a specific example of some check we may want to add to this function in the future or is it a "maybe 5 years from now we realize we may want to undo this change so better not do it just in case" kind of concern?

If you had specific examples it would be much simpler for me to close this PR. If you don't, I think your opposition to this PR is unreasonable.

A specific example we could put there right now, would be to check the nVersion is >0. But either way, they are still conceptually different things, and so long as it says CheckBlockHeader, it should be called whenever the block header is to be checked, independently of whether the PoW check ought to be skipped.

A specific example we could put there right now, would be to check the nVersion is >0.

Thank you. I guess that would currently be an optimization. mhmm, didn't we used to check that block.version > 0 in ContextualCheckBlockHeader ? where are we checking it now? If in the future we want to use the "hardfork bit" to signal hardforks, I think ContextualCheckBlockHeader is the right place (assuming the hf is coordinated with bip9/bip8).
EDIT: never mind, it seems that check only existed in my head, it was just implicit after moving to version 2 or more.

Do you have more examples? An example of something that we currently don't validate would serve.
I can only think of the "time-too-new" check as example, which used to be here, but since we considered that time was "context" anyway, I don't think we will ever put that back in CheckBlockHeader.

ping this, is it really controversial or only for @luke-jr ?

Closing for now, but don't discard to reopen if I ever touch or review the header of main::CheckBlockHeader again, my fault or not.