test: Fix mining to an invalid target + ensure that a new block has the correct hash internally #18350

Test with block 47 in the feature_block.py creates a block with a hash higher than the target, which is supposed to fail. Now two issues exist there, and both have low probability of showing up:

1. The creation is done with while (hash < target), which is wrong, because hash = target is a valid mined value based on the code in the function CheckProofOfWork() that validates the mining target:

    if (UintToArith256(hash) > bnTarget)
        return false;

2. As we know the hash stored in CBlock class in Python is stateful, unlike how it's in C++, where calling CBlock::GetHash() will actively calculate the hash and not cache it anywhere. With this, blocks that come out of the method next_block can have incorrect hash value when solve=False. This is because the next_block is mostly used with solve=True, and solving does call the function rehash() which calculates the hash of the block, but with solve=False, nothing calls that method. And since the work to be done in regtests is very low, the probably of this problem showing up is very low, but it practically happens (well, with much higher probability compared to issue No. 1 above).

This PR fixes both these issues.

If this was ever the cause of a test failure, it probably means our assumption about hash functions is broken.

Still, ACK. I don't mind fixing it.

Concept ACK fixing issue 1. Like @MarcoFalke says, the probability of this ever causing a failure is negligible, but it's better that the test's assumptions are accurate.

For issue 2 (next_block not recalculating the digest if solve is False), I think we can just remove the solve parameter from that function and always solve the block. The function is only called with solve=False in two places, and in both cases the nonce is recalculated (to be invalid for block 47, and resolved for block 48).

If this was ever the cause of a test failure, it probably means our assumption about hash functions is broken.

Actually you're right about the first fix, but the second one has a much higher likelihood to happen. I was able to simulate it by putting it on the tip of the block 1 and it happened always within a loop of 1000 tests that repeat b47's test case.

@TheQuantumPhysicist let me know if you want to address @jnewbery 's feedback or not

@MarcoFalke Do you mean implement it? If so, I don't mind. I just need a confirmation that this is the solution we wanna go for.

In my opinion, the solve option should remain, because solving and hashing are different requirements. But I don't mind either way.

I think in tests we aim for simplicity, even if that comes at a trivial cost like a few hashes. So I see @jnewbery 's point. I am happy to merge either version.

If you don't mind either way, please merge it as it's, and anyone is welcome to change it, since it's correct in its current form.

If I see the majority over here insisting on changing it to remove solve, I'm all for it. I'm just too lazy to run more and test more spontaneously 😄

I really don't like the second commit. I think it adds complexity for no good reason, and without commenting it makes the code less clear instead of more.

This has been merged, but anyone who feels that the logic needs commenting or a refactor, may open a follow-up pull request.