dergoegge noted here that CBlock::GetHash() is a footgun since it only calculates the hash of the header and not the full block. I agree with that sentiment and think it would be beneficial to make this explicit by renaming the inherited function.
The same goes for ConstructBlockHash().
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process. A summary of reviews will appear here.
Reviewers, this pull request conflicts with the following ones:
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.
I don’t think that this is a good idea. The term “block hash” is used in dozens of RPCs, BIPs etc., basically everywhere all over the world. Renaming a single function within bitcoin core won’t change that and will only lead to confusion that the header hash might be different from what the rest of the world calls the “block hash”.
The rest of the world also doesn’t think about stuff like block maleation, verification of the merkle root etc., but we do here in the code. If this helps us with reasoning about these things easier and prevents footguns, we should do it. If it’s confusing someone who is contributing to this project that there is a distinction between hashing just header and hashing the whole block, I think that is a great learning opportunity for them and this might actually prevent them from shooting themselves in the foot in the future :)
The function only hashes the header of the block but not the full block as the name implies.
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
I agree with @mzumsande. It’d have been nice perhaps is the adopted terminology was “block header hash” rather than “block hash”, but making that point now through the code I think will do more harm than good. People might equally be surprised by seeing something referred to as block header hash now, if they expect it to be called block hash.
If it’s confusing someone who is contributing to this project that there is a distinction between hashing just header and hashing the whole block
I don’t think I agree with that. The block header hash, as you call it, is the hash of the whole block (the merkle root commits to the transactions). In fact, the block header hash is in fact a hash of the entire chain up to that point too.
I agree with @mzumsande. It’d have been nice perhaps is the adopted terminology was “block header hash” rather than “block hash”, but making that point now through the code I think will do more harm than good. People might equally be surprised by seeing something referred to as block header hash now, if they expect it to be called block hash.
I don’t think anyone aside from regular contributors would even notice since this doesn’t change any exposed APIs.
If it’s confusing someone who is contributing to this project that there is a distinction between hashing just header and hashing the whole block
I don’t think I agree with that. The block header hash, as you call it, is the hash of the whole block (the merkle root commits to the transactions). In fact, the block header hash is in fact a hash of the entire chain up to that point too.
But the whole issue is that the merke root is not validated here, so we don’t get the hash of the whole block, we only get the hash of the header. It is literally the GetHash() inherited from CBlockHeader that we are actually calling. It is a good point though that the hash also contains the prevblock header hash that is also not validated.
Closing this due to lack of support but I still think this would be a good idea.
But the whole issue is that the merke root is not validated here, so we don’t get the hash of the whole block, we only get the hash of the header. It is literally the
GetHash()inherited fromCBlockHeaderthat we are actually calling.
Ah I see now where you’re coming from. Before a block header is validated, its hash is indeed just the hash of its header and it cannot be considered yet to be the hash of the full block. I still don’t think a change like this is desirable, though.
Labels
Refactoring