adding grpc endpoints for attester, proposer, sync duties, and ptc duties - #16416
Conversation
34d4ec0 to
455f3d5
Compare
| return nil | ||
| } | ||
|
|
||
| func (c *beaconApiValidatorClient) AttesterDuties(ctx context.Context, epoch primitives.Epoch, validatorIndices []primitives.ValidatorIndex) (*ethpb.AttesterDutiesResponse, error) { |
There was a problem hiding this comment.
THis currently doesn't have any tests as the call itself is just an API call, calling the endpoint is tested on the endpoint itself
| }, nil | ||
| } | ||
|
|
||
| func (c *grpcValidatorClient) AttesterDuties(ctx context.Context, epoch primitives.Epoch, validatorIndices []primitives.ValidatorIndex) (*ethpb.AttesterDutiesResponse, error) { |
There was a problem hiding this comment.
same reason of not having tests as beacon-api folder
| } | ||
|
|
||
| // Deprecated: Use the REST API instead. GetProposerDutiesV2 retrieves proposer duties for the given epoch. | ||
| rpc GetProposerDutiesV2(ProposerDutiesRequest) returns (ProposerDutiesResponse) { |
There was a problem hiding this comment.
I called this V2 to match the beacon api one
**What type of PR is this?** Other **What does this PR do? Why is it needed?** This PR refactors the way we store different validator duties into a duties store for easier splitting of tasks and in a future pr processing duties for split endpoints. This PR will reduce the number of changes when we start calling the different endpoints introduced in #16416 pr is part of #16421 **Which issues(s) does this PR fix?** Fixes # **Other notes for review** **Acknowledgements** - [x] I have read [CONTRIBUTING.md](https://github.com/prysmaticlabs/prysm/blob/develop/CONTRIBUTING.md). - [x] I have included a uniquely named [changelog fragment file](https://github.com/prysmaticlabs/prysm/blob/develop/CONTRIBUTING.md#maintaining-changelogmd). - [x] I have added a description with sufficient context for reviewers to understand this PR. - [x] I have tested that my changes work as expected and I added a testing plan to the PR description (if applicable). --------- Co-authored-by: Preston Van Loon <pvanloon@offchainlabs.com>
|
Note for reviewer ethereum/beacon-APIs#586 might affect this pr regarding ptc duties |
| } | ||
| s, err = vs.stateForEpoch(ctx, s, req.Epoch) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Why not wrapping the error for this case?
| var dependentRoot []byte | ||
| if req.Epoch <= 1 { | ||
| r, err := vs.BeaconDB.GenesisBlockRoot(ctx) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "Could not get genesis block root: %v", err) | ||
| } | ||
| dependentRoot = r[:] | ||
| } else { | ||
| dependentRoot, err = core.AttestationDependentRoot(s, req.Epoch) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "Could not get dependent root: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
It's a matter of taste here, bu the
var dependentRoot []byte and the else could be avoided by defining:
func (vs *Server) dependentRoot(req *ethpb.AttesterDutiesRequest) ([]byte, error) {
if req.Epoch <= 1 {
root, err := vs.BeaconDB.GenesisBlockRoot(ctx)
if err != nil {
return nil, status.Errorf(codes.Internal, "Could not get genesis block root: %v", err)
}
return root[:]
}
root, err := core.AttestationDependentRoot(s, req.Epoch)
if err != nil {
return nil, status.Errorf(codes.Internal, "Could not get dependent root: %v", err)
}
return root
}And then calling
dependentRoot := dependentRoot(req)| // Pre-Fulu epoch 1 can be computed normally via ProposalDependentRoot. | ||
| useGenesisRoot := req.Epoch == 0 || (req.Epoch == 1 && s.Version() >= version.Fulu) | ||
| var dependentRoot []byte | ||
| if useGenesisRoot { |
| } | ||
|
|
||
| var dependentRoot []byte | ||
| if req.Epoch <= 1 { |
| } | ||
|
|
||
| type ValidatorClient interface { | ||
| // Duties is the pre-GLOAS combined endpoint (GetDuties/GetDutiesV2). |
There was a problem hiding this comment.
Maybe we can write here that this will be eventually replaced.
| ptcDuties := structs.GetPTCDutiesResponse{} | ||
| if err = c.handler.Post( | ||
| ctx, | ||
| fmt.Sprintf("/eth/v1/validator/duties/ptc/%d", epoch), |
There was a problem hiding this comment.
Can we make it a constant somewhere?
There was a problem hiding this comment.
all the other endpoints are handled in the same way without a constant, if we want to break it out I think we should do it together
…s#16479) **What type of PR is this?** Other **What does this PR do? Why is it needed?** This PR refactors the way we store different validator duties into a duties store for easier splitting of tasks and in a future pr processing duties for split endpoints. This PR will reduce the number of changes when we start calling the different endpoints introduced in OffchainLabs#16416 pr is part of OffchainLabs#16421 **Which issues(s) does this PR fix?** Fixes # **Other notes for review** **Acknowledgements** - [x] I have read [CONTRIBUTING.md](https://github.com/prysmaticlabs/prysm/blob/develop/CONTRIBUTING.md). - [x] I have included a uniquely named [changelog fragment file](https://github.com/prysmaticlabs/prysm/blob/develop/CONTRIBUTING.md#maintaining-changelogmd). - [x] I have added a description with sufficient context for reviewers to understand this PR. - [x] I have tested that my changes work as expected and I added a testing plan to the PR description (if applicable). --------- Co-authored-by: Preston Van Loon <pvanloon@offchainlabs.com>
…ties (OffchainLabs#16416) <!-- Thanks for sending a PR! Before submitting: 1. If this is your first PR, check out our contribution guide here https://docs.prylabs.network/docs/contribute/contribution-guidelines You will then need to sign our Contributor License Agreement (CLA), which will show up as a comment from a bot in this pull request after you open it. We cannot review code without a signed CLA. 2. Please file an associated tracking issue if this pull request is non-trivial and requires context for our team to understand. All features and most bug fixes should have an associated issue with a design discussed and decided upon. Small bug fixes and documentation improvements don't need issues. 3. New features and bug fixes must have tests. Documentation may need to be updated. If you're unsure what to update, send the PR, and we'll discuss in review. 4. Note that PRs updating dependencies and new Go versions are not accepted. Please file an issue instead. 5. A changelog entry is required for user facing issues. --> **What type of PR is this?** Other **What does this PR do? Why is it needed?** ~~DEPENDS ON OffchainLabs#16402 Adding grpc endpoints for attester, proposer, and sync duties. This pr doesn't utilize the apis. In a future pr we will include a transition from using duties v2 endpoint to the split duties endpoints for the fork. TESTING Both the GetDutiesV2 endpoint and the new ones added ( GetAttesterDuties, GetProposerDutiesV2, GetSyncCommitteeDuties, GetPTCDuties) use the same underlying helper function under coreservice.duty function, so all core duties should be the same. Note that the new gRPC endpoints use validator indices instead of pubkeys to match REST api endpoints. Using grpcurl (install with brew install grpcurl): ``` # GetDutiesV2 - composite endpoint # Note: public_keys are base64-encoded BLS pubkeys grpcurl -plaintext -d '{ "epoch": '$EPOCH', "public_keys": ["<base64_pubkey_1>", "<base64_pubkey_2>"] }' localhost:4000 ethereum.eth.v1alpha1.BeaconNodeValidator/GetDutiesV2 ``` ``` # GetAttesterDuties grpcurl -plaintext -d '{ "epoch": '$EPOCH', "validator_indices": [0, 1, 2, 3, 4] }' localhost:4000 ethereum.eth.v1alpha1.BeaconNodeValidator/GetAttesterDuties ``` ``` # GetProposerDutiesV2 grpcurl -plaintext -d '{ "epoch": '$EPOCH' }' localhost:4000 ethereum.eth.v1alpha1.BeaconNodeValidator/GetProposerDutiesV2 ``` ``` # GetSyncCommitteeDuties grpcurl -plaintext -d '{ "epoch": '$EPOCH', "validator_indices": [0, 1, 2, 3, 4] }' localhost:4000 ethereum.eth.v1alpha1.BeaconNodeValidator/Get SyncCommitteeDuties ``` ``` # GetPTCDuties (Gloas+ only) grpcurl -plaintext -d '{ "epoch": '$EPOCH', "validator_indices": [0, 1, 2, 3, 4] }' localhost:4000 ethereum.eth.v1alpha1.BeaconNodeValidator/GetPTCDuties ``` Verification Checklist 1. Attester duty data match: For each validator in GetDutiesV2.CurrentEpochDuties, confirm attester_slot, committee_index, committee_length, committees_at_slot, validator_committee_index match the corresponding AttesterDuty from GetAttesterDuties 2. Proposer duty data match: For each validator with non-empty proposer_slots in GetDutiesV2, confirm those slots appear in GetProposerDutiesV2 response for that validator index 3. Sync committee flag match: For each validator where is_sync_committee=true in GetDutiesV2, confirm that validator appears in GetSyncCommitteeDuties response 4. PTC duty data match: For each validator with non-empty ptc_slots in GetDutiesV2, confirm those slots appear in GetPTCDuties response for that validator index 5. Dependent root — attester: GetDutiesV2.PreviousDutyDependentRoot == GetAttesterDuties.dependent_root (for the same epoch) 6. Dependent root — proposer pre-Fulu: GetDutiesV2.CurrentDutyDependentRoot == GetProposerDutiesV2.dependent_root 7. Dependent root — proposer post-Fulu: ( DutiesV2 still has a bug that doesn't take into account the proposer look ahead) GetDutiesV2.CurrentDutyDependentRoot ≠ GetProposerDutiesV2.dependent_root. The V2 value should match GetAttesterDuties.dependent_root instead (both use (E-1)_start - 1) 8. Dependent root — PTC: GetPTCDuties.dependent_root == GetAttesterDuties.dependent_root (both use AttestationDependentRoot) 9. Next epoch: Repeat checks 1-4 using GetDutiesV2.NextEpochDuties vs individual endpoints queried with epoch+1 10. Edge cases: Epoch 0, epoch 1, sync committee period boundary, pre-Gloas epoch for PTC (should error) **Which issues(s) does this PR fix?** Fixes # **Other notes for review** **Acknowledgements** - [x] I have read [CONTRIBUTING.md](https://github.com/prysmaticlabs/prysm/blob/develop/CONTRIBUTING.md). - [x] I have included a uniquely named [changelog fragment file](https://github.com/prysmaticlabs/prysm/blob/develop/CONTRIBUTING.md#maintaining-changelogmd). - [x] I have added a description with sufficient context for reviewers to understand this PR. - [x] I have tested that my changes work as expected and I added a testing plan to the PR description (if applicable).
What type of PR is this?
Other
What does this PR do? Why is it needed?
DEPENDS ON #16402Adding grpc endpoints for attester, proposer, and sync duties. This pr doesn't utilize the apis.
In a future pr we will include a transition from using duties v2 endpoint to the split duties endpoints for the fork.
TESTING
Both the GetDutiesV2 endpoint and the new ones added ( GetAttesterDuties, GetProposerDutiesV2, GetSyncCommitteeDuties, GetPTCDuties) use the same underlying helper function under coreservice.duty function, so all core duties should be the same. Note that the new gRPC endpoints use validator indices instead of pubkeys to match REST api endpoints.
Using grpcurl (install with brew install grpcurl):
Verification Checklist
Attester duty data match: For each validator in
GetDutiesV2.CurrentEpochDuties, confirm attester_slot,
committee_index, committee_length, committees_at_slot,
validator_committee_index match the corresponding AttesterDuty
from GetAttesterDuties
Proposer duty data match: For each validator with non-empty
proposer_slots in GetDutiesV2, confirm those slots appear in
GetProposerDutiesV2 response for that validator index
Sync committee flag match: For each validator where
is_sync_committee=true in GetDutiesV2, confirm that validator
appears in GetSyncCommitteeDuties response
PTC duty data match: For each validator with non-empty
ptc_slots in GetDutiesV2, confirm those slots appear in
GetPTCDuties response for that validator index
Dependent root — attester:
GetDutiesV2.PreviousDutyDependentRoot ==
GetAttesterDuties.dependent_root (for the same epoch)
Dependent root — proposer pre-Fulu:
GetDutiesV2.CurrentDutyDependentRoot ==
GetProposerDutiesV2.dependent_root
Dependent root — proposer post-Fulu: ( DutiesV2 still has a bug that doesn't take into account the proposer look ahead)
GetDutiesV2.CurrentDutyDependentRoot ≠
GetProposerDutiesV2.dependent_root. The V2 value should match
GetAttesterDuties.dependent_root instead (both use (E-1)_start - 1)
Dependent root — PTC: GetPTCDuties.dependent_root ==
GetAttesterDuties.dependent_root (both use
AttestationDependentRoot)
Next epoch: Repeat checks 1-4 using
GetDutiesV2.NextEpochDuties vs individual endpoints queried
with epoch+1
Edge cases: Epoch 0, epoch 1, sync committee period
boundary, pre-Gloas epoch for PTC (should error)
Which issues(s) does this PR fix?
Fixes #
Other notes for review
Acknowledgements