Implement FromStr for NetAddress

[tnull]

It would be nice to allow for the parsing of NetAddresses from String, as it's likely needed by users of NetAddress, and currently they'd have to create a newtype to do it.

It's really straightforward as we can lean on to_socket_addrs and, in case this fails, on Hostname::try_from.

[tnull]

May also be nice to impl TryFrom/TryInto for SocketAddr.

[jbesraa]

hey
I can try to implement this

[jbesraa]

@tnull how would you identify the input string(to which enum property it should be assigned)?

we can do something like this where key is one of the enum props

fn from_str(s: &str) -> Result<Self, Self::Err> {
  let key = s.split(":")[0]
  let value = s.split(":")[1]
}

we can then use match statement

match key {
 "ipv4": ...,
 "ipv6": ...,
 "hostname": ...,
}

[ayeankit]

hey @tnull , I would like to do it. Please assign me this issue.

[tnull]

hey @tnull , I would like to do it. Please assign me this issue.

Sorry to say, but @jbesraa just picked it up a few days ago. Feel free to pick another issue. :)

[tnull]

@tnull how would you identify the input string(to which enum property it should be assigned)?

we can do something like this where key is one of the enum props

fn from_str(s: &str) -> Result<Self, Self::Err> {
  let key = s.split(":")[0]
  let value = s.split(":")[1]
}

we can then use match statement

match key {
 "ipv4": ...,
 "ipv6": ...,
 "hostname": ...,
}

I think we should first try to parse the IPv4/IPv6 cases and then fall back on Hostname and fail otherwise. I think the onion variants are not really parseable from string unfortunately.

[TheBlueMatt]

It's really straightforward as we can lean on to_socket_addrs and, in case this fails, on Hostname::try_from.

I'm not sure its that straightforward? If you have a hostname, we probably just want to include the singular hostname variant, but that may be a bit surprising to users (which is a more general criticism of FromStr and From - they give you no context about how the conversion is happening, so really only belong for things that are trivially mapped).

[TheBlueMatt]

Can you elaborate on what the specific use-cases is here - are we trying to convert some user-provided configfile-read value to a NetAddress?

[tnull]

Can you elaborate on what the specific use-cases is here - are we trying to convert some user-provided configfile-read value to a NetAddress?

Basically, yes. I want to enable name resolution support in LDK Node and would love to reuse NetAddress for that, since we already have it exposed. Sure I could do a newtype and implement the same parsing logic downstream, but I assume other users of NetAddress might find parsing from String useful.

[tnull]

@jbesraa Still interested in picking this up?

[jbesraa]

yes. I was just waiting for the discussion to come into a conclusion.. can u please review the comment above with the code snippet to validate my thoughts..?

[tnull]

yes. I was just waiting for the discussion to come into a conclusion.. can u please review the comment above with the code snippet to validate my thoughts..?

Great! Yes, as said above the approach basically seems valid. You probably first want to try parse the string into a SocketAddr (see ToSocketAddrs), and if this fails (i.e., returns an Err) try to split it on : and parse the first part it into a Hostname. If this is successful we should finally check if the hostname contains a .onion suffix and in this case reconsider it to be an OnionV3 variant and parse the bytes accordingly, which is probably the trickiest part due to the base32 encoding etc.

[TheBlueMatt]

I don't think we want to try to parse into a socket address at all - if we get a hostname, generally we want to store/announce/etc that hostname and only do the resolution when we actually go to connect. Thus, maybe instead we consider implementing ToSocketAddrs on NetAddress in addition to FromStr?

[tnull]

Ah, I see your point, as ToSocketAddrs indeed would try to do the name resolution right away. What I meant and should have said above was to first try to parse the string as an IP address/port tuple (e.g., using SocketAddr::from_str, IpAddr::from_str, or manually) and if that fails proceed to parse as a hostname etc. In fact, we could follow a pattern similar to the one used in the ToSocketAddr implementation, but omit the resolution step: https://doc.rust-lang.org/src/std/net/socket_addr.rs.html#910-927.

Also implementing ToSocketAddr sounds like a great idea!

[TheBlueMatt]

What I meant and should have said above was to first try to parse the string as an IP address/port tuple (e.g., using SocketAddr::from_str, IpAddr::from_str, or manually) and if that fails proceed to parse as a hostname etc.

I still don't really see why we'd want to do that? If we get a hostname, we should store + announce it as a hostname. I don't see why we should do resolution of the hostname except considering backwards compat of other nodes not reading the hostname address type, but I think that's increasingly less of a concern these days.

[tnull]

I still don't really see why we'd want to do that? If we get a hostname, we should store + announce it as a hostname.

Agreed. But how do we know the string we got is a hostname, if we don't try to parse it to IpAddr first?

I don't see why we should do resolution of the hostname except considering backwards compat of other nodes not reading the hostname address type, but I think that's increasingly less of a concern these days.

To clarify: I'm not saying we should resolve the hostname during FromStr.

[TheBlueMatt]

Ah! Okay, yes, we're on the same page, we should try to parse it as an address, obviously, but not try to resolve the hostname.

[tnull]

@jbesraa Any news on this? Would be great to get it into the next release.

[jbesraa]

Hey
Im planning to work on this on Sunday(March 26th). When is the next release scheduled?

[tnull]

Hey Im planning to work on this on Sunday(March 26th). When is the next release scheduled?

Great, thanks! The release is scheduled for ~next week or so, let's see how this goes. No worries if it doesn't land in time, as worst case I can still pull it in via a master-branch dependency for now.

[TheBlueMatt]

I think the 115 release will probably be in three or four weeks. We're trying to get all the major PRs up for review this week, but minor things like this can probably slip in after that deadline.

[jbesraa]

I created initial commit here jbesraa@20a2482
still wip as I need to figure out the onion part, and I need to add a couple of tests to cover the different scenarios

[tnull]

I created initial commit here jbesraa@20a2482 still wip as I need to figure out the onion part, and I need to add a couple of tests to cover the different scenarios

Great to hear, feel free to open a draft PR!

[Kixunil]

IIUC NetAddress is the same as [ln_types::P2PAddress] which already has FromStr. Feel free to copy the code if it's useful!

[TheBlueMatt]

Huh, hadn't realized ln_types exists, does it make sense to move some of the lightning types there and make it a workspace crate in rust-lightning? Then again, I'm not sure we'd do it with a serde, postgres-types, slog, etc (optional) dep.

[Kixunil]

WTF, I totally remember mentioning it to you but I can't find the conversation!

It could possibly make sense to collaborate more and I'm totally open to handing over the crate. Regarding integrations, IDK what your policy is, here are some reasons I added them:

• serde - obviously it's pretty much the serialization standard in Rust
• slog - convenient loggig, has very conservative MSRV, it was created and is co-maintained by dpc
• postgres-type - I use postgres in multiple projects so it felt nice to add it. I'm not very attached to this one especially given it's not stable
• parse_arg - my own lightweight crate with very conservative MSRV, makes integration with confiure_me very easy. It didn't change for a while but I do plan some changes and eventually stabilization.
• bitcoin - pretty much only conversion between amounts

I would personally at least want to continue supporting serde and parse_arg since these are "interface types" which humans interact with often and have standardized human-readable representation.

[TheBlueMatt]

Heh, my personal view is generally "don't bother doing any integration with anything in the rust ecosystem cause most of it doesn't care about MSRV and trying to do split MSRV across dependencies is a pain, so let downstream folks do their own integrations if they want" :). Eg we recently had to rip out serde stuff because their MSRV policy is "YOLO", which just isn't practical :/. Stuff that actually has a reasonable MSRV we could probably do, but I honestly just kinda hate the "rust ecosystem" and don't like bothering to interact...

[Kixunil]

Don't you find serde worth the trouble of maintaining the pin? I personally totally do for my crates. But yeah, I'm also annoyed by this crazy MSRV bumps from various important crates (e.g. tokio, time, once_cell...)

[TheBlueMatt]

At least tokio maintains MSRVs for a given minor release, so you at least know you'll get bugfixes at your given MSRV as long as its an LTS release. serde I don't really think its worth it, at least for us, because we already have a serialization framework (namely the lightning-native one) so we just re-use that everywhere rather than trying to use something like serde (and forgetting to apply our preconditions when reading :p).

[Kixunil]

I see why it's not useful for most types. I find "interface types" still important:

• Amount
• NetAddress
• P2PAddress
• NodeId
• NodePubkey
• Invoice
• ChannelId

These occur in human-readable interfaces, RPCs, configuration files...

[TheBlueMatt]

Why? In human-readable interfaces, RPCs and configuration files you're probably JSON'ing or hex'ing, or doing something actually human readable.... not using serde (though you may use serde's JSON decoder, but probably not hex). Doubly so for something like invoice where there's a pre-defined human-readable/string encoding.

[Kixunil]

Indeed, that's why having serde impls (de)serialize as strings is important.

[TheBlueMatt]

FromStr is in std it doesn't require serde :p

[Kixunil]

The problem is that defining custom serialization module and slapping #[serde(with = "...")] everywhere is annoying.

[TheBlueMatt]

Right, for all the types you listed we want something for tostring that isn't naive serde, though, and in lightning in general we often have custom serialization we have to use anyway.

[tnull]

I see why it's not useful for most types. I find "interface types" still important:

• Amount
• NetAddress
• P2PAddress
• NodeId
• NodePubkey
• Invoice
• ChannelId

I agree it would be nice to have such types as a sub-crate eventually. FWIW, I'm already introducing some of these type wrappers around [u8; 32] to allow them to be exposed via LDK Node bindings. Also, I'll eventually look into introducing an Amount type into LDK proper (cf. #2076). Having a single crate for all of them would get rid of redundant code and increase type-level compatibility.

[MaxFangX]

Just want to +1 @Kixunil above; there should be serde impls on the most common "interface" types. I don't use the other frameworks so don't have a strong opinion on those.

I honestly just kinda hate the "rust ecosystem" and don't like bothering to interact

Even if you personally dislike the Rust ecosystem, the fact that most Rust + Bitcoin ecosystem libraries and most of LDK's downstream Rust users use serde should be sufficient to establish that LDK should have opt-in serde support as well. Not integrating with serde when most of LDK's downstream Rust users do just ends up wasting their time writing repetitive newtype boilerplate just to get some impls which require only 1 line of code upstream.

A quick list of crates / LDK projects that integrate with serde: bitcoin, secp256k1, esplora_client, electrum_client, Sensei, Mutiny, Kuutamolabs, rust-teos, hidden-lightning-network, rust-dlc. The rust-bitcoin libraries, for example, have saved us time because types like Txid, secp256k1::PublicKey, and Address already have everything we need and we can just use them natively.

Here's a list of LDK types we serialize in some way or another which we've already newtyped to get a serde impl:

• Invoice (with serde_with's SerializeDisplay, DeserializeFromStr derives)
• PaymentHash
• PaymentPreimage
• PaymentSecret
• ConfirmationTarget
• In the past, now removed: ChannelDetails, OutPoint

Granted, this is a short list, and most LDK types don't need it. The logic-heavy / security-critical types like ChannelManager etc should absolutely continue to exclusively use LDK's existing serialization scheme. But a few serde impls here and would make LDK more ergonomic.

serde support could be added conservatively and grow primarily based on user feedback; just add the feature flag and impl it for a few carefully-chosen types which are fundamental or particularly likely to be exposed to end users, then expand gradually as LDK users open issues or #[derive(..)] PRs.