search

lightningdevkit / rust-lightning

A highly modular Bitcoin Lightning library written in Rust. It's rust-lightning, not Rusty's Lightning!
Other
1.16k stars 366 forks source link

Modify NetAddress Enum naming #2358

Closed yanganto closed 1 year ago

yanganto commented 1 year ago

Hi there,

I have a proposal.

IPv4 socket addresses consist of an IPv4 address and a 16-bit port number, as stated in IETF RFC 793

It may be good to use SocketV4 and SocketV6 here, and also the same naming in Rust std SocketAddr

https://github.com/lightningdevkit/rust-lightning/blob/74a9ed9c1978da15ca49b64fc81e01bbcb3f6ae1/lightning/src/ln/msgs.rs#L745-L759

If this proposal is good, I am happy to do this and also #2183, #2056 at the same time. Some these are implemented in our project. https://github.com/kuutamolabs/lightning-knd/blob/e5f05755be1c702c56b469964c7d9586bce15e5d/api/src/lib.rs

TheBlueMatt commented 1 year ago

Sorry I missed this before the long weekend. I'm not sure that SocketAddress is right here - a socket address is the address of your socket, and it wouldn't be unreasonable to call a named pipe or even an onion address (with the right API) a "socket address". If we want to be overly precise I believe the proper naming would be "TCPv4" and "TCPv6" as those are the direction protocols in use here (and the type of socket address).

yanganto commented 1 year ago

Yeh, if they are not real sockets and are protocols, I think TcpV4 and TcpV6 are good here. Would I open a PR on this?

TheBlueMatt commented 1 year ago

Sure if you want, I'm not super convinced its confusing to users and worth changing, but if you feel strongly about it feel free!

arik-so commented 1 year ago

I think it's actually less confusing to users to stick with IPv4 and IPv6.

yanganto commented 1 year ago

I think it's actually less confusing to users to stick with IPv4 and IPv6.

The reason is here https://github.com/lightningdevkit/rust-lightning/pull/2380#issuecomment-1636228798

TheBlueMatt commented 1 year ago

I wonder if some of the confusion isn't enum/variant mismatch - the enum is really a sockaddr in the traditional sense, the variants just describe the different potential types of socket addresses. Would you be happy simply renaming the enum, as a socket address if type ipv4 makes sense to me.

TheBlueMatt commented 1 year ago

Closing this for inactivity - I'm definitely happy to keep discussing this (any thoughts on my last suggestion above?) but it seems the consensus outside of the reporter was that its not super confusing as-is, so doesn't seem worth keeping open waiting for the OP to respond.

yanganto commented 1 year ago

Sorry about the delay, I just had a trip and forgot the thread, and wait for someone to own this project to decide the final result.

@TheBlueMatt Did you suggest SocketAddrV4 or TcpIPv4SockAddr or anything else?

I don't understand what you mean by what naming you accept. For me, SocketAddrV4 is better than TcpIPv4SockAddr, and both are correct. IPv4 with port does not make sense and is unprofessional to miss up ip and socket.

If you have a final result better than IPv4, I can modify it.

TheBlueMatt commented 1 year ago

Sorry about the delay, I just had a trip and forgot the thread, and wait for someone to own this project to decide the final result.

No problem!

@TheBlueMatt Did you suggest SocketAddrV4 or TcpIPv4SockAddr or anything else?

My suggestion was mostly to rename the enum itself, not just the variants. So eg the enum would be SocketAddr (rather than NetAddress) and the variant could then be TcpIPv4 or whatever. Certainly not "final", but folks may be more interested in that, pending you thinking it resolves the confusion.

yanganto commented 1 year ago

Your solution looks good to me. Would we set a timer here? if there is no other voice and I can prepare the PR for this.

SocketAddress is better than NetAddress, because each net card within the same protocol can have more than one socket address and only one net address. So the SocketAddress here is better than NetAddress.

jkczyz commented 1 year ago

SocketAddr is fine by me. For the variant names, I'd suggest using TcpIpV4 and TcpIpV6. Note the lowercase p in Ip and uppercase V for consistency with Tcp and typically how acronyms are cased in Rust identifiers found in the standard library. At very least it would be internally consistent within the enum.

yanganto commented 1 year ago

Good, I think it is good to implement now. :smiley: 

pub enum SocketAddress {
    /// An IPv4 address/port on which the peer is listening.
    TcpIpV4 { .. },
    /// An IPv6 address/port on which the peer is listening.
    TcpIpV6 { .. },
    /// An old-style Tor onion address/port on which the peer is listening.
    ///
    /// This field is deprecated and the Tor network generally no longer supports V2 Onion
    /// addresses. Thus, the details are not parsed here.
    OnionV2([u8; 12]),
    /// A new-style Tor onion address/port on which the peer is listening.
    ///
    /// To create the human-readable "hostname", concatenate the ED25519 pubkey, checksum, and version,
    /// wrap as base32 and append ".onion".
    OnionV3 { .. },
    /// A hostname/port on which the peer is listening.
    Hostname { .. },
}