Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider allowing PeerPoolSubscribers to specify types of messages they're not interested in #1055

Closed
gsalgado opened this issue Jul 20, 2018 · 3 comments · Fixed by #1137
Closed

Comments

@gsalgado
Copy link
Collaborator

gsalgado commented Jul 20, 2018

What is wrong?

Some PeerPoolSubscribers (e.g. LightPeerChain, TxPool) are only meant to handle a subset of msg types, so it is rather wasteful to have PeerPool deliver all messages to their queues.

How can it be fixed

We could make it possible for PeerPoolSubscribers to specify what types of msgs they aren't interested in, so that they never get those. Or they could specify the types of msgs they are interested in, but the problem with that is we need to remember to update the list if/when they're extended to handle more msg types

@pipermerriam
Copy link
Member

I think it makes a lot of sense for us to update the signature of Peer.add_subscribe to take a list of commands and to only add messages to the queue that are of those types. Should be very easy to implement.

class PeerPoolSubscriber:
    @property
    def subscription_msg_types(self):
        # returns a tuple of `Command` classes that it is interested in.
        ...

class PeerPool:
    def subscribe(self, subscriber):
        for peer in ...:
            peer.add_subscriber(subscriber.msg_queue, subscriber.subscription_msg_types):

Considerations:

  • Potentially make PoolSubscriber.subcriber_msg_types a method which takes the Peer as an argument so that the subscriber can select what message types it wants based on the type of peer it receives.

@gsalgado
Copy link
Collaborator Author

My concern with having subscribers specify the types of messages they want is that it's not easy to ensure that each msg type is handled by at least one subscriber, but if we make BasePeer log a warning when it receives a msg of a type for which there are no subscribers, that should not be that big of a deal

@pipermerriam
Copy link
Member

I think your comment is saying that you think a whitelist can work, but just in case it isn't, here's my reasoning.

A whitelist should be:

  • future proof against new commands showing up
  • most reliable reduction in p2p cmd traffic over the queues
  • subscribers can be written to throw errors on unexpected commands making things more explicit

It should be easy for the Peer class to detect when it has no subscribers for a message type and to log a warning (like I think you were saying above).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment