use net utils for binding UDP sockets #3705

gregcusack · 2024-11-19T17:24:50Z

Problem

We are in the process of properly sizing UDP socket buffers. Buffer sizing will happen in net-utils. Also, the code base is fragmented in the sense that some services use net-utils to bind to sockets and some use UdpSocket::bind.

Summary of Changes

switch all cases of UdpSocket::bind to use solana_net_utils::bind_to and all cases of tokio::new::UdpSocket::bind() to use solana_net_utils::bind_to_async

Next PR will update socket binds to take in a SocketConfig to properly size buffers: #3618

t-nelson · 2024-11-25T19:52:30Z

we can leverage clippy's disallowed_methods lint to prevent future introduction of UdpSocket::bind

steviez

Minor requested change that will save us from pasting the default values in 10's of place + potentially save us from touching the same spots should we add a new parameter to bind_to() (which seems possible given that I believe you'll be extending SocketConfig).

In some other parts of the codebase, we have helpers like new_for_tests() and new_for_benches(); not sure if that is appropriate here or not (maybe as part of the next PR where you actually tune buffer sizes) ?

client/src/connection_cache.rs

bench-streamer/src/main.rs

steviez

Thanks for making the changes - just a couple more minor things

steviez · 2024-11-26T15:20:49Z

gossip/src/cluster_info.rs

+        let broadcast = vec![bind_to_localhost().unwrap()];
+        let retransmit_socket = bind_to_localhost().unwrap();


These were both previously unspecified but localhost now. I'm not sure which is more proper, but I'm leaning towards keeping same behavior in this PR and we can change in a separate one (where the change in behavior will be much more apparent)

steviez · 2024-11-26T15:21:06Z

gossip/src/cluster_info.rs

+        let ancestor_hashes_requests = bind_to_localhost().unwrap();
+        let ancestor_hashes_requests_quic = bind_to_localhost().unwrap();


Same as above - these were both previously unspecified

steviez · 2024-11-26T15:25:00Z

streamer/src/nonblocking/recvmmsg.rs

+            .map_err(|e| io::Error::new(io::ErrorKind::InvalidInput, e))?;
+        let reader = bind_to_async(
+            sock_addr.ip(),
+            /*port*/ sock_addr.port(),


nit: I think sock_addr.port() is telling enough that we don't need the /*port*/ comment.

steviez · 2024-11-26T15:25:22Z

streamer/src/nonblocking/recvmmsg.rs

-        let sender = UdpSocket::bind(ip_str).await?;
+        let sender = bind_to_async(
+            sock_addr.ip(),
+            /*port*/ sock_addr.port(),


Same as above - I think sock_addr.port() is telling enough that we don't need the /*port*/ comment

steviez · 2024-11-26T15:28:32Z

streamer/src/recvmmsg.rs

+            .map_err(|e| io::Error::new(io::ErrorKind::InvalidInput, e))?;
+        let reader = bind_to(
+            sock_addr.ip(),
+            /*port:*/ sock_addr.port(),


Same as above - I think sock_addr.port() is telling enough that we don't need the /*port*/ comment

steviez · 2024-11-26T15:28:38Z

streamer/src/recvmmsg.rs

-        let sender = UdpSocket::bind(ip_str)?;
+        let sender = bind_to(
+            sock_addr.ip(),
+            /*port:*/ sock_addr.port(),


Same as above - I think sock_addr.port() is telling enough that we don't need the /*port*/ comment

steviez · 2024-11-26T15:29:55Z

streamer/src/nonblocking/testing_utilities.rs

+                    bind_to(
+                        IpAddr::V4(Ipv4Addr::LOCALHOST),
+                        /*port*/ 0,
+                        /*reuseport:*/ true,


Just making sure - we couldn't use the convenience helper since reuseport = true here ?

correct, although we could change the helper to always accept a reuse port argument. or add a separate helper that is something like: bind_to_localhost_reuse_port() but that feels excessive.

steviez · 2024-11-26T15:34:11Z

streamer/src/nonblocking/sendmmsg.rs

@@ -177,7 +177,13 @@ mod tests {
        ];
        let dest_refs: Vec<_> = vec![&ip4, &ip6, &ip4];

-        let sender = UdpSocket::bind("0.0.0.0:0").await.expect("bind");
+        let sender = bind_to_async(


Given that we have a bind_to_localhost_async(), it wouldn't seem too crazy to add a bind_to_unspecified_async(). But, this test looks to contain the only two instances so I don't think this is a "must-do" either

true. tbh probably worth it to add it for future development use if needed

steviez · 2024-11-26T17:14:15Z

local-cluster/Cargo.toml

@@ -23,6 +23,7 @@ solana-entry = { workspace = true }
 solana-gossip = { workspace = true }
 solana-ledger = { workspace = true }
 solana-logger = { workspace = true }
+solana-net-utils = { workspace = true }


Replying to this question: #3705 (comment)

side note, bind_to_localhost is used directly in LocalCluster. And while LocalCluster is technically only used for testing, it is its own standalone crate. based on my understanding, it seems like we can't make bind_to_localhost #[cfg(feature = "dev-context-only-utils")]. is that right?

I think you could if you do:

solana-net-utils = { workspace = true, features = ["dev-context-only-utils"] }

Like you said, LocalCluster itself is primarily testing so I think it falls in the category of core development crate

agave/scripts/check-dev-context-only-utils.sh

Lines 17 to 20 in 81e2c0d

# Firstly, detect any misuse of dev-context-only-utils as normal/build

# dependencies. Also, allow some exceptions for special purpose crates. This

# white-listing mechanism can be used for core-development-oriented crates like

# bench bins.

I see some dcou in dev-dependencies, but given that you're modifying "source" and not tests, I don't think you could include it in dev-dep list

agave/local-cluster/Cargo.toml

Lines 44 to 53 in 81e2c0d

[dev-dependencies]

assert_matches = { workspace = true }

fs_extra = { workspace = true }

gag = { workspace = true }

serial_test = { workspace = true }

solana-core = { workspace = true, features = ["dev-context-only-utils"] }

solana-download-utils = { workspace = true }

solana-ledger = { workspace = true, features = ["dev-context-only-utils"] }

solana-local-cluster = { path = ".", features = ["dev-context-only-utils"] }

solana-runtime = { workspace = true, features = ["dev-context-only-utils"] }

gregcusack force-pushed the use-net-utils-to-bind branch 5 times, most recently from b0f3959 to 767e5a1 Compare November 19, 2024 18:48

gregcusack marked this pull request as ready for review November 19, 2024 19:18

gregcusack force-pushed the use-net-utils-to-bind branch from 767e5a1 to 87ede38 Compare November 19, 2024 22:21

gregcusack requested review from behzadnouri and t-nelson November 20, 2024 01:47

steviez reviewed Nov 25, 2024

View reviewed changes

client/src/connection_cache.rs Outdated Show resolved Hide resolved

bench-streamer/src/main.rs Outdated Show resolved Hide resolved

gregcusack added 3 commits November 26, 2024 00:35

use net utils for binding sockets

15d2f33

use clippy disallowed methods to prevent direct calls to UdpSocket::bind

461ab50

build helper fcn for bind to local host and unspecified

4917d36

gregcusack force-pushed the use-net-utils-to-bind branch 3 times, most recently from be45088 to 506e152 Compare November 26, 2024 01:03

steviez reviewed Nov 26, 2024

View reviewed changes

gregcusack force-pushed the use-net-utils-to-bind branch 5 times, most recently from 2462284 to ed809ba Compare November 26, 2024 20:37

enforce dev-context-only-utils in net-utils

b0f2135

gregcusack force-pushed the use-net-utils-to-bind branch from ed809ba to b0f2135 Compare November 26, 2024 21:00

gregcusack requested a review from steviez November 26, 2024 22:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use net utils for binding UDP sockets #3705

use net utils for binding UDP sockets #3705

gregcusack commented Nov 19, 2024 •

edited

Loading

t-nelson commented Nov 25, 2024

steviez left a comment

steviez left a comment

steviez Nov 26, 2024

steviez Nov 26, 2024

steviez Nov 26, 2024

steviez Nov 26, 2024

steviez Nov 26, 2024

steviez Nov 26, 2024

steviez Nov 26, 2024

gregcusack Nov 26, 2024

steviez Nov 26, 2024

gregcusack Nov 26, 2024

steviez Nov 26, 2024

		let broadcast = vec![bind_to_localhost().unwrap()];
		let retransmit_socket = bind_to_localhost().unwrap();

		let ancestor_hashes_requests = bind_to_localhost().unwrap();
		let ancestor_hashes_requests_quic = bind_to_localhost().unwrap();

	# Firstly, detect any misuse of dev-context-only-utils as normal/build
	# dependencies. Also, allow some exceptions for special purpose crates. This
	# white-listing mechanism can be used for core-development-oriented crates like
	# bench bins.

	[dev-dependencies]
	assert_matches = { workspace = true }
	fs_extra = { workspace = true }
	gag = { workspace = true }
	serial_test = { workspace = true }
	solana-core = { workspace = true, features = ["dev-context-only-utils"] }
	solana-download-utils = { workspace = true }
	solana-ledger = { workspace = true, features = ["dev-context-only-utils"] }
	solana-local-cluster = { path = ".", features = ["dev-context-only-utils"] }
	solana-runtime = { workspace = true, features = ["dev-context-only-utils"] }

use net utils for binding UDP sockets #3705

Are you sure you want to change the base?

use net utils for binding UDP sockets #3705

Conversation

gregcusack commented Nov 19, 2024 • edited Loading

Problem

Summary of Changes

t-nelson commented Nov 25, 2024

steviez left a comment

Choose a reason for hiding this comment

steviez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregcusack commented Nov 19, 2024 •

edited

Loading