-
Notifications
You must be signed in to change notification settings - Fork 919
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
Cache ETH1 headers on startup for eth1data voting #7541
Conversation
# Conflicts: # beacon-chain/powchain/service.go
beacon-chain/powchain/service.go
Outdated
blocksPerVotingPeriod := params.BeaconConfig().EpochsPerEth1VotingPeriod * params.BeaconConfig().SlotsPerEpoch * | ||
params.BeaconConfig().SecondsPerSlot / params.BeaconConfig().SecondsPerETH1Block | ||
|
||
end := lastKnownHeader.Number.Uint64() - params.BeaconConfig().Eth1FollowDistance |
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.
Check for underflow, even though this cant happen after block height 1024, it can mess up some people running dev goerli chains
beacon-chain/powchain/service.go
Outdated
|
||
end := lastKnownHeader.Number.Uint64() - params.BeaconConfig().Eth1FollowDistance | ||
// We fetch twice the number of headers just to be safe. | ||
start := end - 2*blocksPerVotingPeriod |
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.
same here
beacon-chain/powchain/service.go
Outdated
blocksPerVotingPeriod := params.BeaconConfig().EpochsPerEth1VotingPeriod * params.BeaconConfig().SlotsPerEpoch * | ||
params.BeaconConfig().SecondsPerSlot / params.BeaconConfig().SecondsPerETH1Block | ||
|
||
end := lastKnownHeader.Number.Uint64() - params.BeaconConfig().Eth1FollowDistance |
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.
Please use the helper to calculate this:
prysm/beacon-chain/powchain/service.go
Line 341 in f31d40c
func (s *Service) followBlockHeight(ctx context.Context) (uint64, error) { |
beacon-chain/powchain/service.go
Outdated
_, err := s.batchRequestHeaders(start, end) | ||
if err != nil { | ||
// Caching failure is not critical, so we just log a warning. | ||
log.Warningf("Unable to cache headers: %v", err) |
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 should be an error, failure to request headers could lead to a block proposal with invalid eth1data.
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.
Right. We no longer call the ETH1 endpoint when we have a cache miss.
if err != nil { | ||
return false, nil, err | ||
} | ||
span.AddAttributes(trace.BoolAttribute("blockCacheHit", true)) |
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.
Shouldn't this be a metric maintained with prometheus?
What type of PR is this?
Optimization
What does this PR do? Why is it needed?
This PR caches ETH1 headers on POW initialization so that the cache can be utilized when voting for eth1data. This will optimize the voting algorithm because an RPC call to the ETH1 endpoint is made for every non-cached header.
Zinken testnet cache miss difference:
Which issues(s) does this PR fix?
N/A
Other notes for review
Unblocks #7362