Add data column lookup and response handling #5350
Conversation
jimmygchen
added
ready-for-review
jimmygchen
added
work-in-progress
| /// We know for certain these data columns are missing. | ||
| KnownMissing(Vec<DataColumnIdentifier>), | ||
| /// We think these data columns might be missing. | ||
| PossibleMissing(Vec<DataColumnIdentifier>), |
There was a problem hiding this comment.
This state is dangerous because it can trigger us to fetch all samples. If we haven't received the block yet we should not fetch anything and just wait for the block.
| @@ -157,6 +157,10 @@ const MAX_RPC_BLOCK_QUEUE_LEN: usize = 1_024; | |||
| /// will be stored before we start dropping them. | |||
| const MAX_RPC_BLOB_QUEUE_LEN: usize = 1_024; | |||
|
|
|||
| /// The maximum number of queued `DataColumnSidecar` objects received from the network RPC that | |||
| /// will be stored before we start dropping them. | |||
There was a problem hiding this comment.
Add a TODO(das) to refine this value
| pub fn send_rpc_data_columns( | ||
| self: &Arc<Self>, | ||
| block_root: Hash256, | ||
| data_columns: FixedDataColumnSidecarList<T::EthSpec>, |
There was a problem hiding this comment.
I wonder of the need to send a fixed vector instead of just a vec of sidecars
| fn make_request( | ||
| id: SingleLookupReqId, | ||
| peer_id: PeerId, | ||
| request: Self::RequestType, | ||
| cx: &SyncNetworkContext<T>, | ||
| ) -> Result<(), LookupRequestError> { | ||
| cx.data_column_lookup_request(id, peer_id, request, L::lookup_type()) | ||
| .map_err(LookupRequestError::SendFailed) | ||
| } |
There was a problem hiding this comment.
This won't work because the set of requested_ids is not custodied by a single peer. Instead we need to figure out which peer has each column and do multiple requests
|
Looks like the same approach may not work very well with DAS, so the lookup may be re-designed. Will mark this PR as |
|
Closing this PR as the branch is very outdated. |
Issue Addressed
Part of #4983.
Add data column lookup and response handling logic, mostly copied from blob lookup. Lots of boilerplate here, need to think about whether we need all of this.