-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix(engine): is_fork
header traversal
#11368
Conversation
d1ed0a2
to
8fedc6b
Compare
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.
nit,
checking against the canon hash and then the height is correct
while let Some(current_block) = self.sealed_header_by_hash(current_hash)? { | ||
if current_block.hash() == self.state.tree_state.canonical_block_hash() { | ||
if current_block.hash() == canonical_head.hash { | ||
return Ok(false) | ||
} | ||
// We already passed the canonical head | ||
if current_block.number <= canonical_head.number { | ||
break | ||
} |
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.
this loop is now safe and cuts off at the canonical head which is correct because this is the section we need to check here.
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.
lgtm pending removing the finalized hash
8fedc6b
to
f09fa29
Compare
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 see
lgtm
Description
If the header does not extend the canonical chain, on
is_fork
method invocation the engine might start traversing all of headers in the database and hit transaction timeout.