Sync error handling #3912
Sync error handling #3912
Conversation
Co-authored-by: realbigsean <[email protected]>
Co-authored-by: realbigsean <[email protected]>
Co-authored-by: realbigsean <[email protected]>
…vailable according to the spec
| RPCResponseErrorCode::ResourceUnavailable => 3, | ||
| RPCResponseErrorCode::Unknown => 255, | ||
| RPCResponseErrorCode::RateLimited => 139, | ||
| RPCResponseErrorCode::BlobsNotFoundForBlock => 140, |
There was a problem hiding this comment.
it's being coded to 140 and decoded from 142
| Arc<SignedBeaconBlock<T::EthSpec>>, | ||
| Arc<BlobsSidecar<T::EthSpec>>, |
There was a problem hiding this comment.
is it not easier here to return the wrapper directly?
| /// Returns `Ok(None)` if the blobs and associated block are not found. The block referenced by | ||
| /// the blob doesn't exist in our database. |
There was a problem hiding this comment.
| /// Returns `Ok(None)` if the blobs and associated block are not found. The block referenced by | |
| /// the blob doesn't exist in our database. | |
| /// Returns `Ok(None)` if the blobs and associated block are not found. |
| if block_epoch >= boundary_epoch { | ||
| error!( | ||
| self.log, | ||
| "Peer requested block and blob that should be available, but no blob found"; |
| "peer" => %peer_id, | ||
| "request_root" => ?root, | ||
| "finalized_data_availability_boundary" => ?finalized_data_availability_boundary, | ||
| ); |
There was a problem hiding this comment.
Needs to send an error as well (ResourceUnavailable?)
| debug!(self.log, "Eip4844 fork is disabled"); | ||
| self.send_error_response( | ||
| peer_id, | ||
| RPCResponseErrorCode::ResourceUnavailable, |
There was a problem hiding this comment.
I don't think this should ever happen after we correctly advertise the libp2p protocols, so in a way to me it feels like libp2p logic leaking up. To be able to identify this scenarios I suggest a specific error, prob enough with InternalError (I think?)
| RPCResponseErrorCode::ResourceUnavailable, | ||
| "Backfilling".into(), |
There was a problem hiding this comment.
This is how we handle the same scenario in blocks by range requests. The spec says
Peers that are unable to reply to block requests within the MIN_EPOCHS_FOR_BLOCK_REQUESTS epoch range SHOULD respond with error code 3: ResourceUnavailable. Such peers that are unable to successfully reply to this range of requests MAY get descored or disconnected at any time.
I think it applies here cause we don't have blocks during backfill
| ); | ||
| } | ||
| Err(e) => { | ||
| return error!(self.log, "Unable to obtain root iter"; |
There was a problem hiding this comment.
this should probably be an internal server error response
There was a problem hiding this comment.
It's the same handling as we have for blocks, should I update the handling there as well?
| "peer" => %peer_id, | ||
| "block_root" => ?root | ||
| ); | ||
| break; |
There was a problem hiding this comment.
id this is an error (from the log) don't we want to send some kind of error to the peer?
There was a problem hiding this comment.
this is also the same as how we handle blocks by range, i'll send you a prior conversation about this
| "start_slot" => %req.start_slot, | ||
| "current_slot" => %current_slot, | ||
| "requested" => %req.count, | ||
| "returned" => %blobs_sent |
| "start_slot" => %req.start_slot, | ||
| "current_slot" => %current_slot, | ||
| "requested" => %req.count, | ||
| "returned" => %blobs_sent |
3 participants
Issue Addressed
Which issue # does this PR address?
Proposed Changes
Please list or describe the changes introduced by this PR.
Additional Info
Please provide any additional information. For example, future considerations
or information useful for reviewers.