Skip to content
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

Clear out header hash on incoming blocks #855

Merged
merged 1 commit into from
Jan 10, 2022
Merged

Conversation

dguenther
Copy link
Member

Summary

Nulls out the hash field on headers of incoming blocks. There's no known bug as a result of this, but we should remove this field in the future because nodes should be calculating it themselves vs. assuming that other nodes have provided a correct hash.

Testing Plan

Synced a chain from scratch on this PR with an account with transactions

Breaking Change

Is this a breaking change? If yes, add notes below on why this is breaking and
what additional work is required, if any.

[ ] Yes
[x] No

@NullSoldier
Copy link
Contributor

I think this change makes the code more confusing, and doesn't have any actual affect on the code base. We recalculate the hash before verification. I'm willing to merge it in but it does make the code harder for other developers to work with.

@dguenther
Copy link
Member Author

The current implementation of blockHeader hashing is confusing and error-prone -- it's difficult to determine where it's necessary to call recomputeHash without looking through the whole code path for adding and verifying a block, so it's not easy to determine if this change has an effect on the codebase. For example, we call recomputeHash in verifyTarget, which gets called in verifyBlockHeader, which gets called in addBlock and verifyBlock. In addBlock we also call recomputeHash. It's tough to answer questions like whether one of those recomputeHash calls is redundant, or whether there any places where we use block header hash before calling addBlock or verifyBlock.

We don't have any in-depth code for verifying incoming network messages or stripping extra fields, like a yup schema. I could go down that route instead, but I do think it's a more substantial change that we'll expect to delete either way when we rework our network messages.

@NullSoldier NullSoldier enabled auto-merge (squash) January 10, 2022 23:33
@NullSoldier NullSoldier merged commit 3363362 into staging Jan 10, 2022
@NullSoldier NullSoldier deleted the null-header-hash branch January 10, 2022 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants