Skip to content

[Refactor] Align peer handling between Router and Gateway#3879

Merged
vicsn merged 5 commits intoProvableHQ:stagingfrom
ljedrz:refactor/merge_peers
Sep 25, 2025
Merged

[Refactor] Align peer handling between Router and Gateway#3879
vicsn merged 5 commits intoProvableHQ:stagingfrom
ljedrz:refactor/merge_peers

Conversation

@ljedrz
Copy link
Collaborator

@ljedrz ljedrz commented Sep 23, 2025

The Router and the Gateway have very similar functionalities, so we can benefit from aligning and deduplicating some code between them. This PR was mostly inspired by #3869 (which would otherwise require duplicated code), but other than that, it will allow both objects to benefit from similar bugfixes and optimizations (e.g. those from #3540 and #3713, which had only targeted the Router, but apply to the Gateway as well).

The commits may be read individually (the code compiles after each one) or together. The short list of changes is as follows:

  • decouple the Peer from the Router
  • simplify peer handling in the Gateway by leveraging the Peer
  • introduce a PeerPoolHandling trait which contains methods applicable to both Router and Gateway

This integration can be pushed further (CC #3872), but I wanted to limit the scope of changes for a better reviewing experience. This PR also doesn't contain #3869 just yet, but it will now be trivial to follow up with it.

Closes #3873.

Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
…om Resolver

Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
@ljedrz ljedrz requested review from kaimast and vicsn September 23, 2025 11:57
vicsn
vicsn previously approved these changes Sep 24, 2025
Copy link
Contributor

@kaimast kaimast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me.

I am mostly wondering if we want to export the trait publicly, as it adds a method that allows to side-step Router/Gatways logic and directly modify the peer pool.
Right now, there are not many (any?) users of snarkOS as a library, but would like to minimize the amount of functions that are added to the public API of snarkOS.

Maybe it is better to introduce a PeerPool struct that wraps around the map of peers and Gateway/Router can use to implement their functionality?

Also, what is the difference in functionality between Gateway and Router after we are done with all the cleanups? Could we get to a point where there is only one struct to manage p2p and BFT peers?

Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
@ljedrz
Copy link
Collaborator Author

ljedrz commented Sep 25, 2025

I am mostly wondering if we want to export the trait publicly, as it adds a method that allows to side-step Router/Gatways logic and directly modify the peer pool.

The only intended users of this trait are objects carrying a specific peer pool, and since only the Router and Gateway contain it, only they can implement it. There are likely several other traits that could be misused in the way you're describing, but it couldn't be done by accident. What is more, the peer_pool under the Gateway shouldn't have an individual Arc over itself (only the Gateway needs to be shallowly clonable), but I decided to do it in a small, dedicated follow-up. This will further ensure that misuse is impossible.

Right now, there are not many (any?) users of snarkOS as a library, but would like to minimize the amount of functions that are added to the public API of snarkOS.

To my knowledge there are none, and thus we haven't maintained API stability for snarkOS (otherwise we'd routinely run into such concerns).

Also, what is the difference in functionality between Gateway and Router after we are done with all the cleanups?

Specific handshaking and peering logic, some caches, probably some other small details. Eventually having just one of these objects is likely possible, but it's not the goal of this PR.

@vicsn vicsn dismissed kaimast’s stale review September 25, 2025 09:57

Concerns have been addressed

@vicsn vicsn merged commit b7401ce into ProvableHQ:staging Sep 25, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Proposal] Introduce a peer pool for the validators

3 participants