Refactor subnets subscriptions. #14711
Conversation
nalepae
force-pushed
the
refactor-subnets-subscriptions
branch
from
c453cc3 to
2d92c53
Compare
beacon-chain/sync/subscriber.go
Outdated
| @@ -87,18 +107,23 @@ func (s *Service) registerSubscribers(epoch primitives.Epoch, digest [4]byte) { | |||
| ) | |||
| if flags.Get().SubscribeToAllSubnets { | |||
| s.subscribeStaticWithSubnets( | |||
| "Attestation", | |||
There was a problem hiding this comment.
Can we provide the actual topic or object type here ? Its better than using hardcoded strings here
There was a problem hiding this comment.
This description string is only used in:
log.WithField("digest", digest).Infof("%s subnets with are no longer valid, unsubscribing from all of them.", description)Before, we had the log hardcoded version:
log.Warnf("Attestation subnets with digest %#x are no longer valid, unsubscribing from all of them.", digest)So, it was already hard-coded before, but not at the same location.
I can for sure use the actual topic instead, but the log will be less human readable.
There was a problem hiding this comment.
Could you possibly use a helper function with a switch of all topics to derive this (ex: If the topic is the attestation subnet, then the description will always be Attestation . And so on. This way we don't need to add in an additional argument
beacon-chain/sync/subscriber.go
Outdated
| if flags.Get().SubscribeToAllSubnets { | ||
| s.subscribeStaticWithSyncSubnets( | ||
| s.subscribeStaticWithSubnets( | ||
| "Sync committee", |
There was a problem hiding this comment.
We already add in the SyncCommitteeSubnetTopicFormat topic, so do we need to add this again ?
nalepae
force-pushed
the
refactor-subnets-subscriptions
branch
from
7a52666 to
260a1f3
Compare
beacon-chain/sync/subscriber.go
Outdated
| // subscribeDynamicWithSyncSubnets subscribes to a dynamically changing list of subnets. | ||
| func (s *Service) subscribeDynamicWithSyncSubnets( | ||
| // subscribeSubnets subscribes to a list of subnets. | ||
| func (s *Service) subscribeSubnets( |
There was a problem hiding this comment.
can you rename this or subscribeToSubnets ? They sound too similar to each other which makes it easy to confuse between both of them
==> So we have something symmetrical with subscriptions.
nalepae
force-pushed
the
refactor-subnets-subscriptions
branch
from
a0144ba to
3f49c6a
Compare
What type of PR is this?
Other
What does this PR do? Why is it needed?
This pull requests refactors subnets subscriptions.
It removes
subscribeDynamicWithSubnetsandsubscribeStaticWithSyncSubnetsto only usesubscribeDynamicWithSubnetsandsubscribeStaticWithSubnets.It also paves the way of future data columns (for peerDAS) subscriptions without the need to add specifics functions.
Acknowledgements