Implement Stream Duties Functionality #5685
Conversation
rauljordan
added
API
rauljordan
requested review from
prestonvanloon,
terencechain,
0xKiwi,
farazdagi,
nisdas and
shayzluf
beacon-chain/rpc/validator/server.go
Outdated
| @@ -29,6 +29,8 @@ import ( | |||
| pbp2p "github.com/prysmaticlabs/prysm/proto/beacon/p2p/v1" | |||
| "github.com/prysmaticlabs/prysm/shared/bytesutil" | |||
| "github.com/prysmaticlabs/prysm/shared/params" | |||
| "github.com/prysmaticlabs/prysm/shared/slotutil" | |||
|
|
|||
Codecov Report
@@ Coverage Diff @@
## master #5685 +/- ##
=======================================
Coverage 20.47% 20.47%
=======================================
Files 239 239
Lines 20849 20849
=======================================
Hits 4268 4268
Misses 15776 15776
Partials 805 805 |
|
|
||
| // Compute the validator duties from the head state's corresponding epoch | ||
| // for validators public key / indices requested. | ||
| func (vs *Server) duties(ctx context.Context, req *ethpb.DutiesRequest) (*ethpb.DutiesResponse, error) { |
There was a problem hiding this comment.
we could possibly send nextCommitteeAssignments over the stream. that way validators will know about their assignments ahead of time
There was a problem hiding this comment.
I want to do this in a separate PR to not further complicate the logic and line count, but yep we definitely need to compute next epoch assignments
beacon-chain/blockchain/head.go
Outdated
| @@ -108,6 +112,17 @@ func (s *Service) saveHead(ctx context.Context, headRoot [32]byte) error { | |||
| return errors.Wrap(err, "could not save head root in DB") | |||
| } | |||
|
|
|||
| // A chain re-org occurred, so we fire an event notifying the rest of the services. | |||
| if newHeadState.Slot() < currentHeadSlot { | |||
There was a problem hiding this comment.
Not sure this is fully correct, while this would be one way to determine a re-org the correct way would be to check whether the incoming block is a direct child of the current head. If it's not then we can mark it as a re-org.
You could have re-orgs also when
newHeadState.Slot() > currentHeadSlot where the new head isn't a descendant of the current chain head.
cc @terencechain
There was a problem hiding this comment.
Yeah my bad on this. I was only thinking reorg that affect assignments so it's not complete. Nishant is right. We can have the following:
100 - 101 - 103
\- 102
Head can change from 101 to 102 to 103. From 102 to 103 it's a reorg but slot isn't right.
Checking the direct child sounds good to me
|
Code LGTM. I didn't review @nisdas point about whether or not this correctly detects reorgs. |
| // ReorgData is the data alongside a reorg event. | ||
| type ReorgData struct { | ||
| // NewSlot is the slot of new state after the reorg. | ||
| NewSlot uint64 |
There was a problem hiding this comment.
Is this correct ? shouldn't it be new block root instead
There was a problem hiding this comment.
This is fine. ReorgData is just to notify downstream service and based on downstream service's need. Currently OldRoot and NewRoot are not needed by the downstream services. They can be added later if its necessary
| // ReorgData is the data alongside a reorg event. | ||
| type ReorgData struct { | ||
| // NewSlot is the slot of new state after the reorg. | ||
| NewSlot uint64 |
There was a problem hiding this comment.
This is fine. ReorgData is just to notify downstream service and based on downstream service's need. Currently OldRoot and NewRoot are not needed by the downstream services. They can be added later if its necessary
What type of PR is this?
What does this PR do? Why is it needed?
Currently, validators request duties by using an internal ticker on the client side. This makes it impossible for validators to be aware of any chain reorgs that may have occurred. We can solve this problem by using StreamDuties as a server-side stream from the beacon node to connected validators and simply sending over new duties if there was a chain reorg accordingly. This PR is only the server-side implementation.
This PR also introduces the notion of a reorg event in the blockchain service, sending out a
statefeed.Reorgevent over a StateFeed(). The RPC validator service verifies if this reorg occurred across epoch boundaries, and resends duties accordingly. Thanks for the tip @terencechain.Which issues(s) does this PR fix?
Fixes #5306