Clean up socket allocation logic in cluster_info.rs by alexpyattaev · Pull Request #5832 · anza-xyz/agave

alexpyattaev · 2025-04-15T16:42:25Z

Problem

#5824

Socket binding logic is a disaster of code duplication (because we have had nearly identical logic for localhost and public IP binding) with sudden flakiness mixed in (since multiple tests can be trying to book the same port ranges at the same time).
Tests would also flake because we were relying on port binding logic to never try to bind an already bound port while actually not blocking this properly

Summary of Changes

Merge the socket binding code into one codepath (down from 3) move some of the logic into net-utils for clarity (-200 LOC of boilerplate)

codecov-commenter · 2025-04-22T21:35:47Z

Codecov Report

Attention: Patch coverage is 81.89655% with 21 lines in your changes missing coverage. Please review.

Project coverage is 83.3%. Comparing base (4e33b78) to head (909c6f4).
Report is 6 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master    #5832     +/-   ##
=========================================
- Coverage    83.3%    83.3%   -0.1%     
=========================================
  Files         853      853             
  Lines      378158   377982    -176     
=========================================
- Hits       315300   315110    -190     
- Misses      62858    62872     +14

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bw-solana

I'm having a tough time reviewing thoroughly. It might help to break this up into separate PRs for some of the major changes:

Reorganizing the socket binding APIs
Changing the reuse port usage
Switching VALIDATOR_PORT_RANGE --> localhost_port_range_for_tests
Moving get_gossip_port

All but the socket binding reorganization should be trivial to review. For that one, it might be nice to understand the current hierarchy of functions and the new hierarchy w/ your changes to help guide the review

alexpyattaev · 2025-04-23T20:16:50Z

* Reorganizing the socket binding APIs

* Changing the reuse port usage

* Switching `VALIDATOR_PORT_RANGE` --> `localhost_port_range_for_tests`

* Moving `get_gossip_port`

This plan can not work because stuff will fail CI and we will be committing broken code, so first two steps would essentialy have to be merged into one.

We can definitely move get_gossip_port and switch to localhost_port_range in a separate PRs though.

gregcusack

mostly looking good, but it appears new_single_bind() is no longer doing single bind

gregcusack · 2025-06-27T12:44:14Z

    }

+    /// create localhost node for tests with provided pubkey
+    /// unlike the public IP version, this will also bind RPC sockets.


what is the "public IP version"? are you referring to new_with_external_ip()? Can we refer to it directly by name in this comment

yes, good idea!

gregcusack · 2025-06-27T12:49:31Z

-    ) -> (u16, UdpSocket) {
-        bind_in_range_with_config(bind_ip_addr, port_range, config).expect("Failed to bind")
+        let addr = IpAddr::V4(Ipv4Addr::LOCALHOST);
+        let gossip_addr = SocketAddr::new(addr, 0);


before we were binding to gossip localhost within the port range provided by localhost_port_range_for_tests(). now we are just binding to a random port, possibly outside the port range. this is probably fine since it's just for tests and stuff but does change behavior

Thank you for catching this one, it is a bug! It would not "break" anything but would just cause flaky tests again.

Inlined & deprecated in 909c6f4

gregcusack · 2025-06-27T13:33:43Z

-        let (tvu_port, tvu) = Self::bind_with_config(bind_ip_addr, port_range, socket_config);
-        let (tvu_quic_port, tvu_quic) =
-            Self::bind_with_config(bind_ip_addr, port_range, socket_config);
-        let ((tpu_port, tpu), (_tpu_quic_port, tpu_quic)) =


maybe i'm reading this wrong, but in this old code, tpu gets just a single UdpSocket. but then in the new code we call:

let ((tpu_port, tpu_sockets), (_tpu_port_quic, tpu_quic)) = bind_two_in_range_with_offset_and_config( bind_ip_addr, port_range, QUIC_PORT_OFFSET, socket_config, socket_config, ) .expect("tou_socket primary bind"); let tpu_sockets = bind_more_with_config(tpu_sockets, 32, socket_config).expect("tpu_sockets multi_bind");

so now for new_single_bind() we are binding 32 sockets for tpu even though we only want one

same goes for tvu, tpu_forwards, tpu_vote, broadcast, retransmit_sockets

Yes, it is different, but it is a distinction without a difference. new_single_bind is only used in test_validator, and it simply does not derive any meaningful benefit from multiple binds, but it does not hurt it either. This does remind that we need to finally deprecate TPU UDP for good though!

ok so we are good binding multiple sockets to the same ip/port for test validator? that works for me. but then probably would change name from new_single_bind() to match the fact that it is no longer a single bind

its public API =( I guess we can go ahead and inline it to its sole caller instead.

Co-authored-by: Greg Cusack <greg.cusack@anza.xyz>

lijunwangs · 2025-06-27T21:35:08Z

+            )
+            .expect("tou_socket primary bind");
+        let tpu_sockets =
+            bind_more_with_config(tpu_sockets, 32, socket_config).expect("tpu_sockets multi_bind");


Why hard coded the count? Should be based on the request

it has been hardcoded before. Also, do we even use use a single one of those 32? I thought TPU UDP is not used anymore.

@sakridge confirmed it is going away so we do not care at the moment.

Co-authored-by: Greg Cusack <greg.cusack@anza.xyz>

lijunwangs · 2025-06-28T22:25:29Z

+                .expect("Number of QUIC endpoints can not be zero"),
+            vortexor_receiver_addr: None,
+        };
+        let mut node = Self::new_with_external_ip(pubkey, config);


This section code is almost identical to the one in new_single_bind. except for the gossip port is set. Maybe put it in a common code?

I believe new_single_bind should get deprecated. Just axing it is also an option but in this case we can follow procedure. I think it has no practical purpose, just more code duplication. Am I missing something?

lijunwangs

LGTM. it much reduced code duplication

alexpyattaev mentioned this pull request Apr 15, 2025

Refactor cluster_info module to reduce complexity of the code #5824

Open

alexpyattaev force-pushed the cleanup_cluster_info branch from e8ce52e to a34fa4a Compare April 15, 2025 18:43

alexpyattaev marked this pull request as ready for review April 15, 2025 18:45

alexpyattaev requested a review from gregcusack April 15, 2025 19:26

alexpyattaev force-pushed the cleanup_cluster_info branch 2 times, most recently from 957f9a4 to 7e8b292 Compare April 15, 2025 21:54

alexpyattaev changed the title ~~Cleanup cluster info~~ Cleanup socket allocation logic in cluster info (and in tests in general) Apr 16, 2025

alexpyattaev force-pushed the cleanup_cluster_info branch from 0b78585 to b34f81c Compare April 16, 2025 10:35

alexpyattaev marked this pull request as draft April 17, 2025 09:33

alexpyattaev force-pushed the cleanup_cluster_info branch from b34f81c to 47861e7 Compare April 17, 2025 11:18

This was referenced Apr 17, 2025

Start test validator bound to localhost by default (rather than all interfaces) #5862

Merged

reduce buffer size of "unused" side of socket #4313

Closed

alexpyattaev force-pushed the cleanup_cluster_info branch 2 times, most recently from f436b15 to 1745647 Compare April 22, 2025 15:46

alexpyattaev changed the title ~~Cleanup socket allocation logic in cluster info (and in tests in general)~~ Clean up socket allocation logic in cluster_info.rs Apr 22, 2025

alexpyattaev force-pushed the cleanup_cluster_info branch 2 times, most recently from e4fbce3 to 44f1034 Compare April 22, 2025 19:01

alexpyattaev marked this pull request as ready for review April 22, 2025 19:27

alexpyattaev requested a review from bw-solana April 22, 2025 19:27

alexpyattaev requested a review from lijunwangs April 23, 2025 06:28

bw-solana reviewed Apr 23, 2025

View reviewed changes

Comment thread net-utils/src/lib.rs Outdated

Comment thread net-utils/src/lib.rs Outdated

Comment thread net-utils/src/lib.rs Outdated

Comment thread net-utils/src/lib.rs Outdated

Comment thread net-utils/src/lib.rs Outdated

Comment thread net-utils/src/lib.rs Outdated

This was referenced Apr 23, 2025

switch tests in cluster_info.rs to new port allocation logic #5947

Merged

move get_gossip_port, rename it appropriately #5948

Merged

Remove direct access to SO_REUSEPORT #5965

Closed

alexpyattaev marked this pull request as draft April 24, 2025 13:12

remove the dedicated logic for localhost bind

f47ea7f

alexpyattaev force-pushed the cleanup_cluster_info branch from e6e16d6 to f47ea7f Compare June 26, 2025 20:31

alexpyattaev marked this pull request as ready for review June 26, 2025 21:26

gregcusack reviewed Jun 27, 2025

View reviewed changes

alexpyattaev and others added 2 commits June 27, 2025 23:31

Apply suggestions from code review

e79fb56

Co-authored-by: Greg Cusack <greg.cusack@anza.xyz>

address more review comments

8b56785

alexpyattaev requested a review from gregcusack June 27, 2025 20:58

lijunwangs reviewed Jun 27, 2025

View reviewed changes

alexpyattaev and others added 4 commits June 27, 2025 21:41

deprecate new_single_bind

00fc1dc

Update gossip/src/cluster_info.rs

a110c52

Co-authored-by: Greg Cusack <greg.cusack@anza.xyz>

fix formatting

245e1e0

unbreak RPC binding for test validator

909c6f4

alexpyattaev requested a review from lijunwangs June 28, 2025 07:26

lijunwangs reviewed Jun 28, 2025

View reviewed changes

lijunwangs approved these changes Jul 3, 2025

View reviewed changes

alexpyattaev merged commit ea9f28e into anza-xyz:master Jul 6, 2025
28 checks passed

alexpyattaev deleted the cleanup_cluster_info branch July 6, 2025 18:16

Conversation

alexpyattaev commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

codecov-commenter commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bw-solana left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexpyattaev commented Apr 23, 2025

Uh oh!

gregcusack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lijunwangs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alexpyattaev commented Apr 15, 2025 •

edited

Loading

codecov-commenter commented Apr 22, 2025 •

edited

Loading