Pow: Remove fCheckPOW from CheckBlockHeader

jtimon commented at 3:18 am on February 8, 2017: contributor

After the time checks were removed from CheckBlockHeader, , bool fCheckPOW makes no sense. Nobody is calling it with fCheckPOW=false and if they were, they could simply not call CheckBlockHeader.

luke-jr commented at 3:25 am on February 8, 2017: member

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.

dcousens approved

jtimon commented at 6:00 am on February 8, 2017: contributor

I’m all for unifying CheckBlockHeader with CheckProofOfWork in pow.o, but one step at a time. Here are more future suggestions: https://github.com/jtimon/bitcoin/compare/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:

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.
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.

fanquake added the label Validation on Feb 8, 2017

JeremyRubin commented at 8:56 pm on March 29, 2017: contributor

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.

jtimon commented at 10:32 pm on April 6, 2017: contributor

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.

NicolasDorier commented at 10:26 am on April 7, 2017: contributor

utACK 7b3b201f1ba5db4265363f29e3bea98686b6b5c3

If needed later, can add again.

jnewbery commented at 4:47 pm on April 7, 2017: member

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

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

jtimon commented at 2:32 am on April 11, 2017: contributor

@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.

NicolasDorier commented at 11:08 am on April 11, 2017: contributor

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

jtimon commented at 2:12 am on May 4, 2017: contributor

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 ?

TheBlueMatt commented at 8:36 pm on May 5, 2017: member

utACK 7b3b201f1ba5db4265363f29e3bea98686b6b5c3

ryanofsky commented at 7:11 pm on May 8, 2017: member

utACK 7b3b201f1ba5db4265363f29e3bea98686b6b5c3, good simplification.

jtimon force-pushed on May 9, 2017

jtimon commented at 4:14 pm on May 9, 2017: contributor

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

ryanofsky commented at 8:54 pm on June 1, 2017: member

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

ryanofsky commented at 9:06 pm on June 1, 2017: member

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

luke-jr commented at 2:35 am on June 2, 2017: member

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.

TheBlueMatt commented at 7:36 pm on June 7, 2017: member

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

luke-jr commented at 7:45 pm on June 7, 2017: member

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

jtimon force-pushed on Jun 7, 2017

jtimon commented at 9:58 pm on June 7, 2017: contributor

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).

luke-jr commented at 10:16 pm on June 7, 2017: member

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.

ryanofsky commented at 3:09 pm on June 8, 2017: member

utACK eb1e328e86d46d74121e8254114363115b6c854c. I don’t know if this an improvement or not, but change seems fine if others want it.

Pow: Remove fCheckPOW from CheckBlockHeader b93cf7829b

jtimon force-pushed on Jun 24, 2017

jtimon commented at 0:06 am on June 25, 2017: contributor

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.

luke-jr commented at 1:31 am on June 25, 2017: member

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.

jtimon commented at 1:52 am on June 25, 2017: contributor

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.

jtimon closed this on Jun 25, 2017

jtimon reopened this on Jun 25, 2017

jtimon commented at 2:33 am on August 26, 2017: contributor

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

jtimon commented at 11:55 pm on September 30, 2017: contributor

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.

jtimon closed this on Sep 30, 2017

DrahtBot locked this on Sep 8, 2021

Pow: Remove fCheckPOW from CheckBlockHeader #9717