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.

Sign up for GitHub

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

Jump to bottom

Blockchain: Use get_block to retrieve block #10688

Merged
merged 6 commits into from
May 15, 2022
Merged

Blockchain: Use get_block to retrieve block #10688

merged 6 commits into from
May 15, 2022

Conversation

terencechain
Copy link
Member

getBlock is better to use than s.cfg.BeaconDB.Block because

  1. It checks DB
  2. It checks the initial sync block cache
  3. It returns error if the block is nil

We should be using getblock as much as we can in blockchain pkg

@terencechain terencechain requested a review from a team as a code owner May 13, 2022 17:12
@terencechain terencechain self-assigned this May 13, 2022
rauljordan
rauljordan previously approved these changes May 13, 2022
View reviewed changes
Copy link
Contributor

@rauljordan rauljordan left a comment

Choose a reason for hiding this comment

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

Ideally the beacon DB itself is refactored to return ErrNotFound, but this is a safe approach for now

potuz
potuz reviewed May 15, 2022
View reviewed changes
Copy link
Contributor

@potuz potuz left a comment

Choose a reason for hiding this comment

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

LGTM

beacon-chain/blockchain/service.go Outdated Show resolved Hide resolved
@terencechain @potuz
Update beacon-chain/blockchain/service.go
48dbe51 
Co-authored-by: Potuz <potuz@prysmaticlabs.com>
@prylabs-bulldozer prylabs-bulldozer bot merged commit a90335b into develop May 15, 2022
@delete-merged-branch delete-merged-branch bot deleted the use-get-block branch May 15, 2022 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@potuz potuz potuz approved these changes

@rauljordan rauljordan rauljordan left review comments

@kasey kasey Awaiting requested review from kasey kasey is a code owner automatically assigned from prysmaticlabs/core-team

@nisdas nisdas Awaiting requested review from nisdas nisdas is a code owner automatically assigned from prysmaticlabs/core-team

@james-prysm james-prysm Awaiting requested review from james-prysm james-prysm is a code owner automatically assigned from prysmaticlabs/core-team

Assignees

@terencechain terencechain

Labels
Cleanup Code health!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@terencechain @rauljordan @potuz