Add sync lookup custody request state #6257
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dapplion
requested review from
jimmygchen and
realbigsean
and removed request for
jimmygchen
dapplion
added
ready-for-review
realbigsean
requested changes
Aug 14, 2024
| @@ -6777,6 +6777,27 @@ impl<T: BeaconChainTypes> BeaconChain<T> { | |||
| self.data_availability_checker.data_availability_boundary() | |||
| } | |||
|
|
|||
| /// Returns true if epoch is within the data availability boundary | |||
| pub fn is_within_data_availability_boundary(&self, epoch: Epoch) -> bool { | |||
There was a problem hiding this comment.
We do already have a method, we could pass through here self.data_availability_checker.da_check_required_for_epoch()
There was a problem hiding this comment.
Switched to da_check_required_for_epoch in 96e2363
| /// Compute custody data columns the node is assigned to custody. | ||
| pub fn custody_columns(&self, _spec: &ChainSpec) -> Vec<ColumnIndex> { | ||
| let _enr = self.local_enr(); | ||
| todo!("implement ENR changes"); |
There was a problem hiding this comment.
Suggested change
| todo!("implement ENR changes"); | |
| //TODO(das): implement ENR changes | |
| vec![] |
| @@ -46,6 +46,10 @@ pub enum RangeRequestId { | |||
| }, | |||
| } | |||
|
|
|||
| /// 0: expected blob count | |||
| /// 1: block slot | |||
| pub type DownloadedBlockSummary = (/* expected blob count */ usize, Slot); | |||
There was a problem hiding this comment.
a struct might be nice here
pub struct DownloadedBlockSummary {
expected_blob_count: usize,
block_slot: Slot
}
There was a problem hiding this comment.
Removed the type / struct altogether by just passing the full block around in 96e2363
| @@ -307,19 +328,19 @@ impl<E: EthSpec> BlobRequestState<E> { | |||
| } | |||
| } | |||
|
|
|||
| /// The state of the block request component of a `SingleBlockLookup`. | |||
| /// The state of the blob request component of a `SingleBlockLookup`. | |||
There was a problem hiding this comment.
Suggested change
| /// The state of the blob request component of a `SingleBlockLookup`. | |
| /// The state of the custody request component of a `SingleBlockLookup`. |
| @@ -723,6 +723,7 @@ impl TestRig { | |||
| (ev.work_type() == beacon_processor::RPC_BLOBS).then_some(()) | |||
| }) | |||
| .unwrap_or_else(|e| panic!("Expected blobs work event: {e}")), | |||
| ResponseType::CustodyColumn => todo!(), | |||
There was a problem hiding this comment.
Suggested change
| ResponseType::CustodyColumn => todo!(), | |
| ResponseType::CustodyColumn => todo!(), //TODO(das) |
There was a problem hiding this comment.
Added comment in 96e2363
…erdas-network-lookup
realbigsean
approved these changes
Aug 15, 2024
|
@mergify queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at 9fc0a66 |
5 tasks
This was referenced Aug 16, 2024
AgeManning
pushed a commit
to AgeManning/lighthouse
that referenced
this pull request
Sep 3, 2024
* Add sync lookup custody request state * Review PR * clippy * Merge branch 'unstable' of https://github.com/sigp/lighthouse into peerdas-network-lookup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Addressed
Part of
Proposed Changes
Extend
SingleBlockLookupwith a new request typeCustodyRequestState.Note that this is not functionally complete, in the diff I'll comment which code will be extended in other PRs. In summary
custody_lookup_request: expects the network context to issue N requests to fetch all the columns the node has to custody for a given block root. To see how this "super-request" will look like, check out this filelighthouse/beacon_node/network/src/sync/network_context/custody.rs
Line 24 in b638019
send_custody_columns_for_processing: expects the beacon processor to import those columns into the da_checker