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

Capture ChainReorg, FinalizedCheckpoint, and NewHead Events in Beacon Node #9011

Merged
merged 12 commits into from
Jun 10, 2021

Conversation

rauljordan
Copy link
Contributor

@rauljordan rauljordan commented Jun 9, 2021

What type of PR is this?

Feature

What does this PR do? Why is it needed?

As part of #9001, we need to capture a few important events in eth2 that are required for the eth/v1/events standard API endpoint. These include chain reorgs, finalized checkpoint, and new chain head.

IMPORTANT

The eth2.0-apis chain reorg event does not capture enough information about whether the reorg was across epoch boundaries. Currently, we have a StreamDuties endpoint that is UNUSED in Prysm which only resends duties if there is a reorg across epoch boundaries. Unfortunately, we can no longer tell this information and therefore we simply send the duties if there is a reorg at all. This should not be be very harmful as reorgs should be very rare and the endpoint is unused in v1alpha1.

@rauljordan rauljordan requested a review from a team as a code owner June 9, 2021 18:09
@rauljordan rauljordan self-assigned this Jun 9, 2021
@prestonvanloon
Copy link
Member

Build failure

beacon-chain/rpc/validator/assignments_test.go:348:11: undefined: "github.com/prysmaticlabs/prysm/beacon-chain/core/feed/state".ReorgData
beacon-chain/rpc/validator/assignments_test.go:356:11: undefined: "github.com/prysmaticlabs/prysm/beacon-chain/core/feed/state".ReorgData

beacon-chain/blockchain/head.go Outdated Show resolved Hide resolved
beacon-chain/blockchain/head.go Outdated Show resolved Hide resolved
Copy link
Member

@nisdas nisdas left a comment

Choose a reason for hiding this comment

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

This PR inserts a lot of new code into our critical processing path, have any benchmarks been done on if there are any effects here on block processing time?

@codecov
Copy link

codecov bot commented Jun 10, 2021

Codecov Report

Merging #9011 (b1864ac) into develop (8f90e91) will decrease coverage by 0.07%.
The diff coverage is 6.45%.

@@             Coverage Diff             @@
##           develop    #9011      +/-   ##
===========================================
- Coverage    60.96%   60.89%   -0.08%     
===========================================
  Files          531      531              
  Lines        37558    37611      +53     
===========================================
+ Hits         22899    22902       +3     
- Misses       11384    11432      +48     
- Partials      3275     3277       +2     

// Notifies a common event feed of a new chain head event. Called right after a new
// chain head is determined, set, and saved to disk.
func (s *Service) notifyNewHeadEvent(
newHeadSlot types.Slot,
Copy link
Member

Choose a reason for hiding this comment

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

Optional feedback:
These arguments could be cleaner and reduced... (ie. get slot from block, get state root from block... etc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can fix in the next PR which integrates this one

@rauljordan rauljordan merged commit 6eb0061 into develop Jun 10, 2021
@delete-merged-branch delete-merged-branch bot deleted the add-new-events branch June 10, 2021 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants