Fix Range Request Responses #7531
Conversation
nisdas
added
Ready For Review
| // Decrease allowed blocks capacity by the number of streamed blocks. | ||
| if startSlot <= endSlot { | ||
| s.rateLimiter.add(stream, int64(1+(endSlot-startSlot)/m.Step)) | ||
| } | ||
| // Exit in the event we have a disjoint chain to | ||
| // return. | ||
| if err == errInvalidParent { |
There was a problem hiding this comment.
Can we use errors.Is(err, errInvalidParent), so that we can wrap errors?
There was a problem hiding this comment.
That would seem best IMO
| step, startSlot uint64) ([]*ethpb.SignedBeaconBlock, error) { | ||
|
|
||
| newBlks := make([]*ethpb.SignedBeaconBlock, 0, len(blks)) | ||
|
|
| // Decrease allowed blocks capacity by the number of streamed blocks. | ||
| if startSlot <= endSlot { | ||
| s.rateLimiter.add(stream, int64(1+(endSlot-startSlot)/m.Step)) | ||
| } | ||
| // Exit in the event we have a disjoint chain to | ||
| // return. | ||
| if err == errInvalidParent { |
There was a problem hiding this comment.
That would seem best IMO
| newBlks := make([]*ethpb.SignedBeaconBlock, 0, len(blks)) | ||
|
|
||
| for i, b := range blks { | ||
| isRequestedSlotStep := (b.Block.Slot-startSlot)%step == 0 |
There was a problem hiding this comment.
Should we validate start slot <= b.Block.Slot ?
There was a problem hiding this comment.
all this validation is already done and taken care of in validateRangeRequest
|
|
||
| for i, b := range blks { | ||
| isRequestedSlotStep := (b.Block.Slot-startSlot)%step == 0 | ||
| isCanonical, err := s.chain.IsCanonical(ctx, roots[i]) |
There was a problem hiding this comment.
This func will never return err, is that ok?
There was a problem hiding this comment.
Ah I think we might need to fix the api of that method then. It can be done in another PR
| @@ -214,6 +217,37 @@ func (s *Service) validateRangeRequest(r *pb.BeaconBlocksByRangeRequest) error { | |||
| return nil | |||
| } | |||
|
|
|||
| // filters all the provided blocks to ensure they are canonical | |||
| // and are strictly linear. | |||
| func (s *Service) filterBlocks(ctx context.Context, blks []*ethpb.SignedBeaconBlock, roots [][32]byte, prevRoot *[32]byte, | |||
There was a problem hiding this comment.
what does prevRoot mean? is it like a starting head root?
There was a problem hiding this comment.
I added a comment in the main request handler of what prevRoot is and why it is used.
| // and are strictly linear. | ||
| func (s *Service) filterBlocks(ctx context.Context, blks []*ethpb.SignedBeaconBlock, roots [][32]byte, prevRoot *[32]byte, | ||
| step, startSlot uint64) ([]*ethpb.SignedBeaconBlock, error) { | ||
|
|
There was a problem hiding this comment.
perhaps do a defensive check that blks and roots are the same length
There was a problem hiding this comment.
This is actually already done in dedupBlocksAndRoots but I can add it in again too.
| isLinear := *prevRoot == bytesutil.ToBytes32(b.Block.ParentRoot) | ||
| isSingular := step == 1 | ||
| if isRequestedSlotStep && isCanonical { | ||
| // Exit early if our valid block is non linear. |
There was a problem hiding this comment.
do we want to be nicer here and actually return the resorted blocks for linearity? or exit early is good enough
There was a problem hiding this comment.
We do return the blocks if we find the next block is non-linear. In the caller of this function, it checks the error and if it is invalidParent we just exit early and return what we have so far.
Codecov Report
@@ Coverage Diff @@
## master #7531 +/- ##
==========================================
+ Coverage 61.62% 61.76% +0.13%
==========================================
Files 422 423 +1
Lines 29688 29761 +73
==========================================
+ Hits 18296 18381 +85
+ Misses 8466 8452 -14
- Partials 2926 2928 +2 |
What type of PR is this?
Bug Fix
What does this PR do? Why is it needed?
We were responding to range request with invalid blocks which were non linear. This lead to lighthouse nodes to disconnect
from us. This PR adds a new method to filter all blocks to be sent and to ensure their linearity.
Which issues(s) does this PR fix?
Fixes #7464
Other notes for review