feat: use static subscribe instead of subscribeWithParameters for blob sidecar topics #15895
Conversation
|
|
|
Please use the defined template for PR. |
| }) | ||
| s.spawn(func() { | ||
| s.subscribe(p2p.AggregateAndProofSubnetTopicFormat, s.validateAggregateAndProof, s.beaconAggregateProofSubscriber, nse) | ||
| s.subscribe(p2p.AggregateAndProofSubnetTopicFormat, s.buildTopicWithoutSubnet(p2p.AggregateAndProofSubnetTopicFormat, nse.ForkDigest), s.validateAggregateAndProof, s.beaconAggregateProofSubscriber, nse) |
There was a problem hiding this comment.
s.buildTopicWithoutSubnet could be called and the result extracted in a variable to avoid extra long lines of code.
| if !strings.Contains(topicFormat, "%x") { | ||
| log.Error("Topic does not have appropriate formatter for digest") | ||
| } | ||
| return (fmt.Sprintf(topicFormat, digest)) + s.cfg.p2p.Encoding().ProtocolSuffix() |
There was a problem hiding this comment.
Why the extra parenthesis around fmt.Sprintf?
|
|
||
| func (s *Service) buildTopicWithoutSubnet(topicFormat string, digest [4]byte) string { | ||
| if !strings.Contains(topicFormat, "%x") { | ||
| log.Error("Topic does not have appropriate formatter for digest") |
There was a problem hiding this comment.
If we are here, the user have no idea of which subnet was wrong.
IMO, it's better to return an error here.
| // subscribe to a given topic with a given validator and subscription handler. | ||
| // The base protobuf message is used to initialize new messages for decoding. | ||
| func (s *Service) subscribe(topic string, validator wrappedVal, handle subHandler, nse params.NetworkScheduleEntry) { | ||
| func (s *Service) subscribe(topicFormat string, fullTopic string, validator wrappedVal, handle subHandler, nse params.NetworkScheduleEntry) { |
There was a problem hiding this comment.
Why using both topicFormat and fullTopic here?
It seems topicFormat is only used in logs and in p2p.GossipTopicMappings, where the result is only checked against nil.
| s.subscribeWithBase(fullTopic, validator, handle) | ||
| } | ||
|
|
||
| func (s *Service) buildTopicWithoutSubnet(topicFormat string, digest [4]byte) string { |
There was a problem hiding this comment.
Without taking tests into account, buildTopicWithoutSubnet is only used when using subscribe.
==> The call to buildTopicWithoutSubnet could be directly done in subscribe, and thus avoiding an extra parameter in subscribe.
| s.spawn(func() { | ||
| s.subscribe( | ||
| p2p.SyncContributionAndProofSubnetTopicFormat, | ||
| s.buildTopicWithoutSubnet(p2p.SyncContributionAndProofSubnetTopicFormat, nse.ForkDigest), |
There was a problem hiding this comment.
- All the parameters used in
s.buildTopicWihoutSubnetare also parameters ofsubscribe. - All calls of
s.subscribealso uses.builTopicWithoutSubnet
==> s.buildTopicWithoutSubnet should be directly called in s.subscribe.
| return (fmt.Sprintf(topicFormat, digest)) + s.cfg.p2p.Encoding().ProtocolSuffix() | ||
| } | ||
|
|
||
| func (s *Service) buildTopicWithSubnet(topicFormat string, digest [4]byte, subnet uint64) string { |
There was a problem hiding this comment.
Same comments as in buildTopicWithoutSubnet.
There was a problem hiding this comment.
The names buildTopicWithSubnet and buildTopicWithoutSubnet are not so clear.
Maybe something like buildStaticSubnet and buildDynamicSubnet?
Also, please add a godoc with an example of suitable topic format, so we can understand what the function does without reading it.
3 participants
What type of PR is this?
Improvement/Refactor
What does this PR do? Why is it needed?
Since the number of blob sidecar subnets are constant for a given fork, we can use the subscribe call instead of subscribeWithParameters call and avoid the overhead of spinning up go-routines and looking for peers every slot since all peers subscribe to all configured blob sidecar topics anyways.
This PR also includes some refactoring to make the topic building homogenous across the code.
Acknowledgements