Impl ToSocketAddrs for SocketAddress #2636

slanesuke · 2023-10-01T19:37:44Z

Resolves #2535

codecov-commenter · 2023-10-01T19:47:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (989304e) 89.02% compared to head (e9ff38f) 89.94%.
Report is 50 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2636      +/-   ##
==========================================
+ Coverage   89.02%   89.94%   +0.91%     
==========================================
  Files         112      112              
  Lines       87168    94312    +7144     
  Branches    87168    94312    +7144     
==========================================
+ Hits        77605    84826    +7221     
+ Misses       7327     7240      -87     
- Partials     2236     2246      +10

Files	Coverage Δ
lightning/src/ln/msgs.rs	`77.50% <100.00%> (+0.68%)`	⬆️

... and 29 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dunxen

Thanks! I believe @tnull has the most context here based on the usage in ldk-node.

dunxen · 2023-10-02T08:33:21Z

lightning/src/ln/msgs.rs

+	#[test]
+	#[cfg(feature = "std")]
+	fn test_socket_address_to_socket_addrs() {
+		assert_eq!(SocketAddress::TcpIpV4 {addr:[0u8; 4], port: 1337,}.to_socket_addrs().unwrap().next().unwrap(),
+				   SocketAddr::V4(std::net::SocketAddrV4::new(std::net::Ipv4Addr::new(0,0,0,0),
+											   1337)));
+		assert_eq!(SocketAddress::TcpIpV6 {addr:[0u8; 16], port: 1337,}.to_socket_addrs().unwrap().next().unwrap(),
+				   SocketAddr::V6(std::net::SocketAddrV6::new(std::net::Ipv6Addr::from([0u8; 16]),
+											   1337, 0, 0)));
+		assert_eq!(SocketAddress::Hostname { hostname: Hostname::try_from("0.0.0.0".to_string()).unwrap(), port: 0
+					}.to_socket_addrs().unwrap().next().unwrap(),
+				   std::net::SocketAddr::V4(std::net::SocketAddrV4::new(
+					   std::net::Ipv4Addr::from([0u8; 4]),0)));
+		assert!(SocketAddress::OnionV2([0u8; 12]).to_socket_addrs().is_err());
+		assert!(SocketAddress::OnionV3{
+			ed25519_pubkey: [37, 24, 75, 5, 25, 73, 117, 194, 139, 102, 182, 107, 4, 105, 247, 246, 85,
+				111, 177, 172, 49, 137, 167, 155, 64, 221, 163, 47, 31, 33, 71, 3],
+			checksum: 48326,
+			version: 121,
+			port: 1234
+		}.to_socket_addrs().is_err());
+	}
 }


nit: I see we can actually remove the std::net:: qualifying prefixes here and just import in the feature gated use above.

Suggested change

#[test]

#[cfg(feature = "std")]

fn test_socket_address_to_socket_addrs() {

assert_eq!(SocketAddress::TcpIpV4 {addr:[0u8; 4], port: 1337,}.to_socket_addrs().unwrap().next().unwrap(),

SocketAddr::V4(std::net::SocketAddrV4::new(std::net::Ipv4Addr::new(0,0,0,0),

1337)));

assert_eq!(SocketAddress::TcpIpV6 {addr:[0u8; 16], port: 1337,}.to_socket_addrs().unwrap().next().unwrap(),

SocketAddr::V6(std::net::SocketAddrV6::new(std::net::Ipv6Addr::from([0u8; 16]),

1337, 0, 0)));

assert_eq!(SocketAddress::Hostname { hostname: Hostname::try_from("0.0.0.0".to_string()).unwrap(), port: 0

}.to_socket_addrs().unwrap().next().unwrap(),

std::net::SocketAddr::V4(std::net::SocketAddrV4::new(

std::net::Ipv4Addr::from([0u8; 4]),0)));

assert!(SocketAddress::OnionV2([0u8; 12]).to_socket_addrs().is_err());

assert!(SocketAddress::OnionV3{

ed25519_pubkey: [37, 24, 75, 5, 25, 73, 117, 194, 139, 102, 182, 107, 4, 105, 247, 246, 85,

111, 177, 172, 49, 137, 167, 155, 64, 221, 163, 47, 31, 33, 71, 3],

checksum: 48326,

version: 121,

port: 1234

}.to_socket_addrs().is_err());

}

}

#[test]

#[cfg(feature = "std")]

fn test_socket_address_to_socket_addrs() {

assert_eq!(SocketAddress::TcpIpV4 {addr:[0u8; 4], port: 1337,}.to_socket_addrs().unwrap().next().unwrap(),

SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::new(0,0,0,0),

1337)));

assert_eq!(SocketAddress::TcpIpV6 {addr:[0u8; 16], port: 1337,}.to_socket_addrs().unwrap().next().unwrap(),

SocketAddr::V6(SocketAddrV6::new(Ipv6Addr::from([0u8; 16]),

1337, 0, 0)));

assert_eq!(SocketAddress::Hostname { hostname: Hostname::try_from("0.0.0.0".to_string()).unwrap(), port: 0

}.to_socket_addrs().unwrap().next().unwrap(),

SocketAddr::V4(SocketAddrV4::new(

Ipv4Addr::from([0u8; 4]),0)));

assert!(SocketAddress::OnionV2([0u8; 12]).to_socket_addrs().is_err());

assert!(SocketAddress::OnionV3{

ed25519_pubkey: [37, 24, 75, 5, 25, 73, 117, 194, 139, 102, 182, 107, 4, 105, 247, 246, 85,

111, 177, 172, 49, 137, 167, 155, 64, 221, 163, 47, 31, 33, 71, 3],

checksum: 48326,

version: 121,

port: 1234

}.to_socket_addrs().is_err());

}

}

dunxen · 2023-10-02T08:34:27Z

lightning/src/ln/msgs.rs

@@ -2671,7 +2698,7 @@ mod tests {
 	use crate::chain::transaction::OutPoint;

 	#[cfg(feature = "std")]
-	use std::net::{Ipv4Addr, Ipv6Addr};
+	use std::net::{Ipv4Addr, Ipv6Addr, SocketAddr, ToSocketAddrs};


Suggested change

use std::net::{Ipv4Addr, Ipv6Addr, SocketAddr, ToSocketAddrs};

use std::net::{Ipv4Addr, Ipv6Addr, SocketAddr, SocketAddrV4, SocketAddrV6, ToSocketAddrs};

dunxen · 2023-10-02T08:40:14Z

lightning/src/ln/msgs.rs

+				Ok((hostname.as_str(), *port).to_socket_addrs()?.next().into_iter())
+			}
+			SocketAddress::OnionV2(..) => {
+				Err(std::io::Error::from(std::io::ErrorKind::Unsupported))


Ah looks like the MSRV (minimum supported Rust version) tests are failing because the Unsupported variant is only available from version 1.53 up. We'll need to use another variant here, probably Other until we bump the MSRV.

tnull

Thanks for looking into this!

lightning/src/ln/msgs.rs

tnull · 2023-10-12T08:40:28Z

lightning/src/ln/msgs.rs

@@ -46,6 +46,7 @@ use core::fmt::Debug;
 use core::ops::Deref;
 #[cfg(feature = "std")]
 use core::str::FromStr;
+use std::net::SocketAddr;


You'll have to also feature-gate this import.

I'm unsure if I should use the existing [cfg(feature = "std")] or create a new feature flag. I imagine the former is the right approach but I want to make sure. Let me know

Yeah just using that one is good as we just want to ensure it's only imported when we have access to std :)

tnull · 2023-10-12T08:53:13Z

lightning/src/ln/msgs.rs

+	fn test_socket_address_to_socket_addrs() {
+		assert_eq!(SocketAddress::TcpIpV4 {addr:[0u8; 4], port: 1337,}.to_socket_addrs().unwrap().next().unwrap(),
+				   SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::new(0,0,0,0),
+											   1337)));


nit: Indentation is off here and below.

tnull · 2023-10-12T08:53:57Z

lightning/src/ln/msgs.rs

+				Ok(vec![socket_addr].into_iter())
+			}
+			SocketAddress::Hostname { ref hostname, port } => {
+				let socket_addr: Vec<SocketAddr> = (hostname.as_str(), *port).to_socket_addrs()?.collect();


I think here you can now avoid all intermediary steps and just do

(hostname.as_str(), *port).to_socket_addrs()

tnull

LGTM, please squash commits.

Feel free to include the minor log nits below while squashing.

lightning/src/ln/msgs.rs

tnull

LGTM, please squash the commits down to a single one!

dunxen

LGTM

dunxen reviewed Oct 2, 2023

View reviewed changes

tnull reviewed Oct 5, 2023

View reviewed changes

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

slanesuke closed this Oct 6, 2023

slanesuke reopened this Oct 6, 2023

slanesuke marked this pull request as draft October 6, 2023 20:42

slanesuke force-pushed the impl-ToSocketAddrs-for-Hostname branch from f61987a to e507557 Compare October 10, 2023 20:48

slanesuke marked this pull request as ready for review October 10, 2023 20:49

tnull reviewed Oct 12, 2023

View reviewed changes

tnull reviewed Oct 16, 2023

View reviewed changes

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

Impl ToSocketAddrs for SocketAddress

e9ff38f

tnull reviewed Oct 19, 2023

View reviewed changes

slanesuke force-pushed the impl-ToSocketAddrs-for-Hostname branch from 6011d63 to e9ff38f Compare October 19, 2023 23:07

dunxen approved these changes Oct 19, 2023

View reviewed changes

tnull approved these changes Oct 20, 2023

View reviewed changes

tnull merged commit 62fd36d into lightningdevkit:main Oct 20, 2023
14 of 15 checks passed

tnull mentioned this pull request Oct 20, 2023

Impl Display for SocketAddress #2670

Merged

slanesuke deleted the impl-ToSocketAddrs-for-Hostname branch October 20, 2023 22:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Impl ToSocketAddrs for SocketAddress #2636

Impl ToSocketAddrs for SocketAddress #2636

slanesuke commented Oct 1, 2023

codecov-commenter commented Oct 1, 2023 •

edited

dunxen left a comment

dunxen Oct 2, 2023

dunxen Oct 2, 2023

dunxen Oct 2, 2023

tnull left a comment

tnull Oct 12, 2023

slanesuke Oct 12, 2023

dunxen Oct 13, 2023

tnull Oct 12, 2023

tnull Oct 12, 2023

tnull left a comment

tnull left a comment

dunxen left a comment

	use std::net::{Ipv4Addr, Ipv6Addr, SocketAddr, ToSocketAddrs};
	use std::net::{Ipv4Addr, Ipv6Addr, SocketAddr, SocketAddrV4, SocketAddrV6, ToSocketAddrs};

Impl ToSocketAddrs for SocketAddress #2636

Impl ToSocketAddrs for SocketAddress #2636

Conversation

slanesuke commented Oct 1, 2023

codecov-commenter commented Oct 1, 2023 • edited

Codecov Report

dunxen 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

tnull 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

tnull left a comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

dunxen left a comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 1, 2023 •

edited