Handle Syncing Executon Client #13597
Conversation
nisdas
force-pushed
the
fixPanicWithEL
branch
from
cde5ea4 to
c2666c7
Compare
| @@ -649,6 +651,10 @@ func (s *Service) retrievePayloadsFromExecutionHashes( | |||
| return nil, fmt.Errorf("could not fetch payload bodies by hash %#x: %v", executionHashes, err) | |||
| } | |||
|
|
|||
| if len(payloadBodies) != len(executionHashes) { | |||
There was a problem hiding this comment.
Should not this check be directly in the GetPayloadBodiesByHash function?
According to the specification, is such a length mismatch happens, it is a bug in the EL.
Client software MUST place responses in the order given in the request, using null for any missing blocks. For instance, if the request is [A.block_hash, B.block_hash, C.block_hash] and client software has data of payloads A and C, but doesn't have data of B, the response MUST be [A.body, null, C.body].
If this check is written directly in the GetPayloadBodiesByHash function, then all calls to this function will benefit of this check.
In the contrary, all callers of GetPayloadBodiesByHashwill need to do the same check.
There was a problem hiding this comment.
In the case referenced in the issue description the node is optimistically syncing which indicates that the EL might have just come online, which is why they return nothing instead of empty payloads. It might be a race condition on the EL's side but in the event of bugs across different clients we can just handle it on our end. I have pushed up the change to handle the mismatch in the function
What type of PR is this?
Bug Fix
What does this PR do? Why is it needed?
Handles the case where an EL is syncing and cannot provide us with the desired payloads.
Which issues(s) does this PR fix?
Fixes #13566
Other notes for review