Add DataColumnSidecar gossip topic and message handling
#6147
Conversation
jimmygchen
added
work-in-progress
# Conflicts: # beacon_node/beacon_chain/src/data_column_verification.rs # beacon_node/beacon_chain/src/lib.rs
… for consistency.
jimmygchen
changed the title
DataColumnSidecar gossip topic and verification (#5050 and #5783)DataColumnSidecar gossip topic and verification
jimmygchen
added
ready-for-review
jimmygchen
changed the title
DataColumnSidecar gossip topic and verificationDataColumnSidecar gossip topic and message handling
There was a problem hiding this comment.
Looks mostly good. The das logic seems sane, but I have mainly focused on ensuring that we don't trigger any of the das related code if this is run on mainnet.
Just to be extra safe, I would prefer to remove all instances of panic macros and just let the TODO(das) comments guide us to incomplete code.
Also confirmed that we are not subscribing to any data column subnet topics in this PR so any of the upstream code wouldn't get triggered
jimmygchen
added
ready-for-merge
| @@ -107,6 +107,9 @@ pub enum SyncMessage<E: EthSpec> { | |||
| /// A blob with an unknown parent has been received. | |||
| UnknownParentBlob(PeerId, Arc<BlobSidecar<E>>), | |||
|
|
|||
| /// A data column with an unknown parent has been received. | |||
| UnknownParentDataColumn(PeerId, Arc<DataColumnSidecar<E>>), | |||
There was a problem hiding this comment.
It's actually useless to have to forward the payload in this error :)
There was a problem hiding this comment.
I think you're referring to GossipDataColumnError::ParentUnknown instead of SyncMessage here? we do need to keep the data column in the BlockComponent when doing a parent lookup.
There was a problem hiding this comment.
I've removed the unused error types in GossipDataColumnError and leaving this line unchanged. Let me know if you still want the payload removed for this error?
jimmygchen
added
ready-for-review
| where | ||
| E: EthSpec, | ||
| { | ||
| let log_clone = log.clone(); | ||
| let spec_clone = spec.clone(); |
There was a problem hiding this comment.
We can do this later, but we should probably avoid cloning the ChainSpec so frequently by Arcing it
There was a problem hiding this comment.
To do it properly we should probably Arc it everywhere, but we could probably do it in a separate PR as it will be quite a big change. I've Arc'd the spec only in Discovery and NetworkService to avoid the performance regression for now.
…ding to clone it repeatedly.
|
@mergify queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at 4e5a363 |
Issue Addressed
Part of #6072.
This PR contains:
Upstream PRs: