Add custody by root request to sync#6275
Conversation
dapplion
added
ready-for-review
|
This was automatically closed by mergify as the upstream branch was merged. I'll reopen this PR against |
jimmygchen
changed the base branch from
peerdas-request-data-columns-by-root
to
unstable
jimmygchen
changed the base branch from
unstable
to
peerdas-request-data-columns-by-root
…est-custody-by-root
jimmygchen
changed the base branch from
peerdas-request-data-columns-by-root
to
unstable
|
The refactoring on these PRs are making merging back to Some of the new function signatures require reworking parts of the existing code on |
jimmygchen
added
do-not-merge
and removed
ready-for-review
dapplion
added
work-in-progress
dapplion
added
ready-for-review
| DataColumnSubnetId::from_column_index::<E>(column_index as usize, spec), | ||
| )) | ||
| .cloned() | ||
| .collect::<Vec<_>>() |
There was a problem hiding this comment.
Why not take the latest change from dasbranch and just use good_custody_subnet_peer?
This undos most the changes from #6218 - i understand selecting peers from entire peers is not the right solution, but even this doesn't fix it and just revert back to the previous version.
There was a problem hiding this comment.
Also happy if you'd like to change how it works on the das branch but i think it's better to target rework on the das branch before we finish the merge to unstable.
| peer.insert_subnet(Subnet::DataColumn(subnet.into())); | ||
| } | ||
|
|
||
| peer_id |
There was a problem hiding this comment.
Is all this change required? Why not just take the latest change from das branch? The change in #6218 fixes the custody lookup tests.
|
|
||
| const FAILED_PEERS_CACHE_EXPIRY_SECONDS: u64 = 5; | ||
|
|
||
| type DataColumnSidecarVec<E> = Vec<Arc<DataColumnSidecar<E>>>; |
There was a problem hiding this comment.
For consistency:
| type DataColumnSidecarVec<E> = Vec<Arc<DataColumnSidecar<E>>>; | |
| type DataColumnSidecarList<E> = Vec<Arc<DataColumnSidecar<E>>>; |
Also updated on the das branch custody.rs
|
|
||
| pub struct ActiveCustodyRequest<T: BeaconChainTypes> { | ||
| block_root: Hash256, | ||
| requester: SingleLookupReqId, |
There was a problem hiding this comment.
What's the reason to change this from CustodyId? (on das branch)
| indices: Vec<ColumnIndex>, | ||
| } | ||
|
|
||
| type CustodyRequestResult<E> = Result<Option<DataColumnSidecarVec<E>>, Error>; |
There was a problem hiding this comment.
Are we dumping the peer group?
das branch:
type CustodyRequestResult<E> = Result<Option<(DataColumnSidecarList<E>, PeerGroup)>, Error>;
| column_request.on_download_success( | ||
| req_id, | ||
| peer_id, | ||
| // Safe to cast, self.column_requests only contains indexes for columns we must custody |
There was a problem hiding this comment.
This comment can be removed as we're no longer casting, although I'm not sure why we removed the CustodyDataColumn type
| .network_globals | ||
| .custody_peers_for_column(i, &self.harness.spec); | ||
| println!("peers column {} {:?}", i, peers); | ||
| } |
There was a problem hiding this comment.
Is this just for debugging? it doesn't actually assert anything but just prints
| &mut self, | ||
| req_id: DataColumnsByRootRequestId, | ||
| _requester: SingleLookupReqId, | ||
| requester: SingleLookupReqId, |
There was a problem hiding this comment.
This parameter type isn't compatible with the das branch, and if we want to change this type i think we should change it there instead, as it would affect other part of the code (e.g. sampling) and make merging back and conga line difficult
| // TODO(das): this is not the correct peer to attribute to a response. It's | ||
| // just the last peer of multiple potential requests. We should switch to a | ||
| // concept of "peer group" to attribute fault to the correct peer. | ||
| peer_id, |
There was a problem hiding this comment.
We already have this on das branch, is there a reason why you don't want it included in this PR? (this would diverge with das branch)
jimmygchen
added
waiting-on-author
jimmygchen
left a comment
jimmygchen
left a comment
There was a problem hiding this comment.
Nice, another 900+ lines down 🎉
I've went through the code and I don't see anything impacting mainnet code path, so we should be good to go once comments are addressed and CI passes.
|
@dapplion Is there any refactor in this PR that you'd like to merge to |
|
I ported a change to else nothing relevant so I'll close |
2 participants
Issue Addressed
Part of
dastounstable#6072Build on / depends on
Proposed Changes
Adds a new higher order request to fetch all custody columns of a given block root.
It's a stateful request that may trigger one or more data_columns_by_root internally with features: