Start validating blobs closer to RPC response reader #13511
Conversation
|
|
||
| func (sbv *seqBlobValid) nextValid(blob blocks.ROBlob) error { | ||
| if sbv.prev == nil { | ||
| sbv.prev = &blob |
There was a problem hiding this comment.
If there isn't a previous blob, we need to check that the blob index is zero.
| return ErrBlobIndexOutOfBounds | ||
| } | ||
| } else { | ||
| if sbv.prev.Slot() >= blob.Slot() { |
There was a problem hiding this comment.
We also need to check that blob index is zero here.
| } | ||
| } else { | ||
| if sbv.prev.Slot() >= blob.Slot() { | ||
| return errChunkResponseSlotNotAsc |
There was a problem hiding this comment.
Before this, we should check that the prev.BlockRoot() matches blob.ParentBlockRoot(). It's a cheap check that ensures it's a connected chain 😄
| sbv.prev = &blob | ||
| return nil | ||
| } | ||
| if sbv.prev.BlockRoot() == blob.BlockRoot() { |
There was a problem hiding this comment.
Another thing. If the block root is the same, it won't check the blob's slot and parentBlockRoot. It might be useful to have another function which ensures blockRoot, parentBlockRoot, and slot are equal.
There was a problem hiding this comment.
checking slot first since that's a minutely cheaper check, then roots & parent root.
kasey
force-pushed
the
blob-rpc-opt
branch
2 times, most recently
from
3050e60 to
e9e4d49
Compare
| var ( | ||
| // ErrInvalidFetchedData is used to signal that an error occurred which should result in peer downscoring. | ||
| ErrInvalidFetchedData = errors.New("invalid data returned from peer") | ||
| ErrBlobIndexOutOfBounds = errors.Wrap(ErrInvalidFetchedData, "blob index out of range") |
There was a problem hiding this comment.
I think this should be private, ie errBlobIndexOutOfBounds.
| if err := vf[i](blob); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
maybe over-engineering here, but could these be processed in parallel?
There was a problem hiding this comment.
discussed offline, these checks are too trivial to justify parallelization
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
We already perform sufficient checks to ensure that we safely handle RPC responses, but we can improve efficiency by performing some checks closer to the rpc network reads. This is a proof of concept to show the direction we can move, more refactoring is needed to move the entire blob validation chain to this point.