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

Update head before block proposal #11992

Merged
merged 6 commits into from
Feb 17, 2023
Merged

Update head before block proposal #11992

merged 6 commits into from
Feb 17, 2023

Conversation

terencechain
Copy link
Member

Updating head before block proposal

@terencechain terencechain requested a review from a team as a code owner February 15, 2023 00:34
nisdas
nisdas previously approved these changes Feb 15, 2023
View reviewed changes
@terencechain terencechain added the Blocked Blocked by research or external factors label Feb 15, 2023
potuz
potuz requested changes Feb 15, 2023
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.

I don't like this, we process attestations and update head on a slot ticker, this happens right before block proposal. If there's a race then it should be fixed in a different way instead of updating head and processing attestations twice immediately.

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go Outdated
@@ -58,6 +58,9 @@ func (vs *Server) GetBeaconBlock(ctx context.Context, req *ethpb.BlockRequest) (
if err != nil {
return nil, status.Errorf(codes.Internal, "Could not prepare block: %v", err)
}
if err := vs.HeadUpdater.UpdateHead(ctx); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should go before the check for optimistic status

potuz
potuz previously approved these changes Feb 15, 2023
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.

Approving now, but probably better to revisit this design. We are calling effectively the same forkchoice runner twice at the same time

@potuz potuz mentioned this pull request Feb 15, 2023
Open
@terencechain terencechain added Ready For Review A pull request ready for code review OK to merge and removed Blocked Blocked by research or external factors labels Feb 15, 2023
@prylabs-bulldozer prylabs-bulldozer bot merged commit 481d24c into develop Feb 17, 2023
@delete-merged-branch delete-merged-branch bot deleted the update-head-block-proposal branch February 17, 2023 08:41
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

@nisdas nisdas nisdas left review comments

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

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

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

Assignees
No one assigned
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Milestone
No milestone
Development

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