-
Notifications
You must be signed in to change notification settings - Fork 20.1k
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
consensus/ethash: less lookups of block data #21467
Conversation
@@ -204,11 +204,20 @@ func (ethash *Ethash) VerifyUncles(chain consensus.ChainReader, block *types.Blo | |||
|
|||
number, parent := block.NumberU64()-1, block.ParentHash() | |||
for i := 0; i < 7; i++ { | |||
ancestorHeader := chain.GetHeader(parent, number) | |||
if ancestorHeader == nil { |
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.
Just one concern, if we lose the header because the database reason, we may loosen rules for checking 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.
Yes, I know. It might be worth having a panic
here, but I didn't want to introduce one now -- since the current code just silently does the exact same thing
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.
Just one concern, otherwise LGTM
defd914
to
493100b
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.
SGTM
This PR contains a small optimization picked from #19456
When verifying a block PoW, we need to check an ancestor's uncles. However, most blocks don't have uncles, and in those cases we can avoid loading the full block, and just make do with the header.