-
Notifications
You must be signed in to change notification settings - Fork 105
feat(l1): handle non-canonical headers by hash in GetBlockHeaders
#4576
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR enhances the GetBlockHeaders
functionality to handle requests that specify a starting block by hash when the hash is not in the canonical chain. Previously, such requests returned empty responses, but now they can return the block header if found in the database.
- Refactors the header fetching logic to preserve hash-based lookups when the block isn't in the canonical chain
- Adds support for retrieving block headers by hash from storage
- Implements proper error handling and logging for database access issues
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Lines of code reportTotal lines added: Detailed view
|
// We don't support fetching headers by hash, unless the hash is | ||
// part of the canonical chain, so we break here. | ||
// TODO: we could support fetching by hash in descending order, | ||
// by fetching the parent of each block. |
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.
According to https://github.com/ethereum/devp2p/blob/master/caps/eth.md#getblockheaders-0x03 , even when fetching by hash, it should be part of the canonical chain, so maybe we should adapt the code/comments to that?
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 didn't catch that part of the spec before, but Geth apparently services the request even if the hash is not part of the canonical chain:
We can keep the logic as it was before, since it already supports returning headers in the canonical chain. I think we should add this, however, since the current behavior could cause problems in our syncing algorithm when handling reorgs in an ethrex-only network.
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.
Motivation
We're currently returning empty header responses when we receive a
GetBlockHeaders
request specifying the start with the block's hash, and the hash isn't part of our canonical view of the chain.Description
This PR adds support for this case by simply looking for the starting block in the DB and returning it. This stays simple enough and, though suboptimal, should make the simple case of a peer asking for a sidechain to work.
It could be further extended by looking for the block's parent through the parent hash, which should allow us to support cases where
reverse
istrue
.