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

Feature flag to disable head update on attestation basis #4802

Merged
merged 4 commits into from Feb 9, 2020

Conversation

terencechain
Copy link
Member

@terencechain terencechain commented Feb 9, 2020

Computing head for every new attestation is a nice to have but may be too excessive, we can change the strategy to updating head every second. Opening this as an option to not compute head on every attestation.
Another option is to remove the tracing for the following methods. They seem to be the main bottlenecks.

Screen Shot 2020-02-08 at 11 10 54 PM

@codecov
Copy link

codecov bot commented Feb 9, 2020

Codecov Report

Merging #4802 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4802   +/-   ##
=======================================
  Coverage   56.87%   56.87%           
=======================================
  Files         246      246           
  Lines       18394    18394           
=======================================
  Hits        10462    10462           
  Misses       6456     6456           
  Partials     1476     1476           

Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add it as a feature flag? Like we have --disable-strict-attestation-pubsub-verification which gives us the option to disable this in an emergency.

@terencechain
Copy link
Member Author

Can you add it as a feature flag? Like we have --disable-strict-attestation-pubsub-verification which gives us the option to disable this in an emergency.

this is a good idea, I'm leaning towards default behavior is compute head on per attestation, the feature flag will disable that.

It's generally good for the node to get head updated as frequently incoming attestation if there's not much penalties to go with it

@terencechain terencechain changed the title [Optional ]Don't update head on every attestation Feature flag to disable head update on attestation basis Feb 9, 2020
@terencechain terencechain self-assigned this Feb 9, 2020
@terencechain terencechain added Forkchoice Enhancement New feature or request labels Feb 9, 2020
@terencechain terencechain merged commit 8a02003 into master Feb 9, 2020
@delete-merged-branch delete-merged-branch bot deleted the dont-update-head-on-new-att branch February 9, 2020 19:44
cryptomental pushed a commit to cryptomental/prysm that referenced this pull request Feb 24, 2020
cryptomental pushed a commit to cryptomental/prysm that referenced this pull request Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants