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

p2p: peer update subscription model should take channels into consideration #6598

Closed
4 tasks
cmwaters opened this issue Jun 18, 2021 · 4 comments
Closed
4 tasks
Assignees
Labels
C:p2p Component: P2P pkg stale for use by stalebot

Comments

@cmwaters
Copy link
Contributor

Summary

We need to think about the peer update subscription model. At the moment, the reactor allows for anyone to open a subscription to listen to peer status updates. However most reactors only care for updates that relate to the channels they are operating across i.e. The block chain reactor doesn't care if a peer status comes through for a new peer that only runs the PEX reactor. We can leave this as it will be caught by the router when the reactor tries to send that peer a message but we should really find a better solution.

We could have reactors subscribe to peer status updates based on the channels that interest them. Thus we'd have to keep channels in the peer updates struct and only fire the update if the peer in question had (all/at least one) of these channels.

Do people have any other suggestions?
(I also thought of dumping the subscription model and just keeping the peer updates within the channel itself).


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@cmwaters cmwaters self-assigned this Jun 18, 2021
@alexanderbez alexanderbez added the C:p2p Component: P2P pkg label Jun 18, 2021
@alexanderbez
Copy link
Contributor

You mean the router, right? Not reactor.

@alexanderbez
Copy link
Contributor

Also, I'm not quite making the connection between PeerUpdate and Channel...The router sends a PeerUpdate to all of it's subscribers. This has nothing to do with a Channel, it only cares about the peer's Status.

@cmwaters
Copy link
Contributor Author

cmwaters commented Jul 7, 2021

Other than the reactors, what other services will subscribe to the PeerUpdates? I can't see any other use but I might be missing something. Since all reactors use channels and we want to find some way to send peer information per channel why not use the Channel struct itself as the instrument to sending PeerUpdates. The only problem with this is that one reactor might have several channels in which case it would receive the update several times and need to be able to handle it.

@cmwaters
Copy link
Contributor Author

cmwaters commented Jul 7, 2021

If I think about this more, it's odd that we communicate which channels are open but in fact it should be which services are available i.e. fastsync, statesync, mempool. The NodeInfo shouldn't really contain which channels a peer has but rather which services it is running. Imagine if a node were to say that is was running the snapshot channel but not the chunk channel (within the state sync service).

@github-actions github-actions bot added the stale for use by stalebot label Jul 18, 2021
@tac0turtle tac0turtle reopened this Jul 26, 2021
@cmwaters cmwaters removed the stale for use by stalebot label Sep 13, 2021
@github-actions github-actions bot added the stale for use by stalebot label Jul 2, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:p2p Component: P2P pkg stale for use by stalebot
Projects
None yet
Development

No branches or pull requests

3 participants