Add broadcast_validation to block publishing
#12432
Conversation
# Conflicts: # beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go
| } | ||
|
|
||
| func (vs *Server) validateEquivocation(blk interfaces.ReadOnlyBeaconBlock) error { | ||
| if vs.seenProposerIndex(blk.Slot(), blk.ProposerIndex()) { |
There was a problem hiding this comment.
no longer needs to use the cache here, just have to check if there exists a block of same slot in forkchoice store
| if validation == types.Consensus { | ||
| if err = vs.validateConsensus(ctx, blk); err != nil { | ||
| return nil, errors.Wrap(err, types.ErrConsensusValidationFailed.Error()) | ||
| } | ||
| } else if validation == types.ConsensusAndEquivocation { | ||
| if err = vs.validateConsensus(ctx, blk); err != nil { | ||
| return nil, errors.Wrap(err, types.ErrConsensusValidationFailed.Error()) | ||
| } | ||
| if err = vs.validateEquivocation(blk.Block()); err != nil { | ||
| return nil, errors.Wrap(err, types.ErrEquivocationValidationFailed.Error()) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I would prefer these checks to be outside of v1alpha1 pkg. It should be in the beacon-api pkg.
1.) It's less risky to current validator code
2.) v1alpha1 will be deprecated eventually then we shouldn't have to move these checks again
There was a problem hiding this comment.
From the spec:
consensus_and_equivocation: the same asconsensus, with an extra equivocation check immediately before the block is broadcast
Broadcast is done here, so it's hard to move this check anywhere else. Also I believe the function will stay as it is even when we deprecate v1alpha1. It will be just no longer a method on the v1alpha1 server, but rather a regular function either in Beacon APIs or somewhere else.
There was a problem hiding this comment.
Why not break it up and call Broadcast directly like we did here: https://github.com/prysmaticlabs/prysm/pull/12335/files#diff-50db3f4907c1c24a971fc3a1ef6264d8386f7ec2b42cf60928238b6dfc94d058R1044
I think we should delete v1alpha1 eventually so avoid adding more dependency to it will be nice.
rkapka
changed the title
broadcast_validation to block publishing
| isSyncing, syncDetails, err := helpers.ValidateSyncHTTP(r.Context(), bs.SyncChecker, bs.HeadFetcher, bs.TimeFetcher, bs.OptimisticModeFetcher) | ||
| if err != nil { | ||
| errJson := &network.DefaultErrorJson{ | ||
| Message: "Could not check if node is syncing: " + err.Error(), | ||
| Code: http.StatusInternalServerError, | ||
| } | ||
| network.WriteError(w, errJson) | ||
| return | ||
| } | ||
| if isSyncing { | ||
| msg := "Beacon node is currently syncing and not serving request on that endpoint" | ||
| details, err := json.Marshal(syncDetails) | ||
| if err == nil { | ||
| msg += " Details: " + string(details) | ||
| } | ||
| errJson := &network.DefaultErrorJson{ | ||
| Message: msg, | ||
| Code: http.StatusServiceUnavailable, | ||
| } | ||
| network.WriteError(w, errJson) | ||
| return | ||
| } |
There was a problem hiding this comment.
Seems like this whole part is duplicated and we'll use it in a few more places. Might as well abstract into its own function
| } | ||
|
|
||
| var broadcastValidation types.BroadcastValidation | ||
| switch r.URL.Query().Get("broadcast_validation") { |
There was a problem hiding this comment.
magic strings here, same with the cases. Might be useful to make these variables instead
Co-authored-by: terencechain <terence@prysmaticlabs.com>
* day 1 * day 2 * day 2+ * day 3 * day 4 * making bazel happy * PublishBlindedBlockV2 * remove file * use lock in insertSeenProposerIndex * remove EquivocationChecker interface * update deps.bzl * remove middleware json tags * go mod tidy * remove redundant return statements * validate in handler * improvements * extract common code * remove import * sync test fix * Update beacon-chain/rpc/eth/beacon/handlers.go Co-authored-by: terencechain <terence@prysmaticlabs.com> --------- Co-authored-by: terencechain <terence@prysmaticlabs.com>
What type of PR is this?
Feature
What does this PR do? Why is it needed?
This PR implements ethereum/beacon-APIs#317, which defines two new endpoints:
publishBlockV2andpublishBlindedBlockV2.Things to keep in mind when reading the code:
publishBlockandpublishBlindedBlockare implemented as gRPC handlers. To make the code more manageable, it would be a good idea to call the HTTP handler directly from the gRPC handler because the only difference is an additional query parameter. This will be done in a follow-up PR.rpchelpers.ValidateSyncused gRPC-specific constructs, so I renamed it toValidateSyncGRPCand created a new HTTP versionValidateSyncHTTP.structs.go, but maybe it would have been a better idea to reuse the ones from API Middleware. I didn't want to do this because we want to remove all middleware code in the future.oneOfspecification, which means that values representing several versions of blocks can be submitted. This makes JSON unmarshalling problematic because missing fields get ignored (as an example, a JSON representing a Phase 0 block will get unmarshalled to a Capella block without errors, but it shouldn't). To fix the issue I imported the https://github.com/go-playground/validator package to validate that fields are not empty. This package looks very useful and we can later extend validations to check for things like the length of a hex string.