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

More efficient list indexed attestation endpoint #5441

Merged
merged 58 commits into from
Apr 19, 2020

Conversation

shayzluf
Copy link
Contributor

@shayzluf shayzluf commented Apr 15, 2020

[Resolves] #5373
[Resolves] #5486


Description

list indexed attestations endpoint i using beacondb get state function that only returns the lates few epochs states . using the new state-gen will enable to retrieve committees using old states

  • added grouping by blockroot to make the endpoint more efficiant
  • added handling for new stategen disabled

@shayzluf shayzluf requested a review from a team as a code owner April 15, 2020 11:27
@shayzluf shayzluf added the Ready For Review A pull request ready for code review label Apr 15, 2020
Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

Looks like it was already using state gen for state:
https://github.com/prysmaticlabs/prysm/pull/5441/files#diff-c767c9e73a5894791f1a09e7b26fd917R142

What did this PR change?

@prestonvanloon
Copy link
Member

What happens when new state management is not enabled?

@shayzluf shayzluf changed the title Simplify and handle state gen featureconfig in list indexed attestations More efficient list indexed attestation endpoint Apr 19, 2020
…abs/prysm into fix-list-indexed-attestations
@codecov
Copy link

codecov bot commented Apr 19, 2020

Codecov Report

Merging #5441 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5441   +/-   ##
=======================================
  Coverage   18.88%   18.88%           
=======================================
  Files         235      235           
  Lines       20239    20239           
=======================================
  Hits         3822     3822           
  Misses      15823    15823           
  Partials      594      594           

beacon-chain/rpc/beacon/attestations.go Outdated Show resolved Hide resolved
beacon-chain/rpc/beacon/attestations.go Outdated Show resolved Hide resolved
beacon-chain/rpc/beacon/attestations.go Outdated Show resolved Hide resolved
beacon-chain/rpc/beacon/attestations.go Outdated Show resolved Hide resolved
beacon-chain/rpc/beacon/attestations.go Outdated Show resolved Hide resolved
beacon-chain/rpc/beacon/attestations.go Outdated Show resolved Hide resolved
@shayzluf
Copy link
Contributor Author

@0xKiwi fixed test to cover the bug you pointed to

@shayzluf
Copy link
Contributor Author

thanks @terencechain for the feedback

Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

We still should add back old archival functionality but i think that's out of the scope of this PR. Can be addressed in the next PR

@shayzluf shayzluf merged commit acd7dd1 into master Apr 19, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix-list-indexed-attestations branch April 19, 2020 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants