New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: Improve naming of CBlock::GetHash() -> GetHeaderHash() #29538
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process. ConflictsReviewers, 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :) |
7de2ee6
to
f683183
Compare
f683183
to
edeacf1
Compare
The function only hashes the header of the block but not the full block as the name implies.
edeacf1
to
7e2ac74
Compare
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being 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.
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 don't think anyone aside from regular contributors would even notice since this doesn't change any exposed APIs.
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 Closing this due to lack of support but I still think this would be a good idea. |
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. |
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()
.