Implement data columns by network boilerplate #6224
Conversation
jimmygchen
added
waiting-on-author
dapplion
added
ready-for-review
| @@ -1142,6 +1131,10 @@ impl<E: EthSpec> BeaconProcessor<E> { | |||
| self.spawn_worker(item, idle_tx); | |||
| } else if let Some(item) = blbroots_queue.pop() { | |||
| self.spawn_worker(item, idle_tx); | |||
| } else if let Some(item) = dcbroots_queue.pop() { | |||
There was a problem hiding this comment.
Looks like the intent before was to prioritize all by roots requests ahead of all by range requests?
// Prioritize by_root requests over by_range as the former are time sensitive for recovery
There was a problem hiding this comment.
I have no evidence this is useful, so let's just extend the deneb priorization
| "Invalid fork name for data columns by root".to_string(), | ||
| )), | ||
| Some(fork_name) => { | ||
| if fork_name.deneb_enabled() { |
There was a problem hiding this comment.
we want electra_enabled here right? otherwise we'll be incorrectly advertising this protocol as supported immediately on mainnet when we release
There was a problem hiding this comment.
Good catch! we can perhaps add a peer_das_enabled function on chain_spec so we can test this on deneb-based devnets.
if self.fork_context.spec.peer_das_enabled()
There was a problem hiding this comment.
PeerDAS is currently supported for both deneb and electra. This check does not advertise the topic on deneb, simply allows it to decode it. Advertise logic is in SupportedTopic::currently_supported.
There was a problem hiding this comment.
This check does not advertise the topic on deneb, simply allows it to decode it. Advertise logic is in SupportedTopic::currently_supported.
Ah right, thanks
jimmygchen
added
waiting-on-author
| if fork_context.spec.peerdas_scheduled() { | ||
| supported.extend_from_slice(&[ | ||
| ProtocolId::new(SupportedProtocol::DataColumnsByRootV1, Encoding::SSZSnappy), | ||
| ProtocolId::new(SupportedProtocol::DataColumnsByRangeV1, Encoding::SSZSnappy), | ||
| ]); | ||
| } |
There was a problem hiding this comment.
This logic is the one modifying the advertising rules. Because PeerDAS is still a "soft fork" not tied to Electra this check will advertise the topics in this cases:
| EIP7594_FORK_EPOCH | topics advertised |
|---|---|
| no value | no |
| 18446744073709551615 | no |
| 0 | yes |
| 128 | yes |
So, not advertised in mainnet, and advertised immediately on devnets.
dapplion
added
ready-for-review
jimmygchen
added
waiting-on-author
jimmygchen
added
ready-for-merge
|
@mergify queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at ff15c78 |
* Implement data columns by network boilerplate * Use correct quota values * Address PR review * Update currently_supported * Merge remote-tracking branch 'sigp/unstable' into peerdas-network-boilerplate * PR reviews * Fix data column rpc request not being sent due to incorrect limits set. (sigp#6000)
Issue Addressed
Start porting network changes of
dasbranch. This PR introduces mindless boilerplate around the new RPC routes data columns by range and by root. Both will be used by PeerDAS custody sync on both range sync and lookup sync.Part of
Proposed Changes
Upstream PRs:
DataColumnSidecarsByRootreq/resp protocol #5196