docs: Improve netaddress comments #15718

 223 | @@ -194,6 +224,10 @@ bool CNetAddr::IsLocal() const
 224 |      return false;
 225 |  }
 226 |  
 227 | +/**
 228 | + * @returns Whether or not this network address is a valid address that @a could

practicalswift commented at 5:33 PM on April 2, 2019:

@a a typo? :-)

dongcarl commented at 5:37 PM on April 2, 2019:

Nope, it italicizes I believe.

  46 | + * addresses have a prefix of fd6b:88c0:8724::/48 and are guaranteed to not be
  47 | + * publicly routable as it falls under RFC4193's fc00::/7 subnet allocated to
  48 | + * unique-local addresses.
  49 | + *
  50 | + * This is useful for having some source address to identify the hostname of DNS
  51 | + * seeds with in CAddrMan without doing a resolve.

jonatack commented at 11:09 AM on April 4, 2019:

Nit: "with in" -> "within" ?

dongcarl commented at 2:58 PM on April 4, 2019:

Yeah this is confusing, will reword.

  65 | @@ -52,6 +66,16 @@ bool CNetAddr::SetInternal(const std::string &name)
  66 |      return true;
  67 |  }
  68 |  
  69 | +/**
  70 | + * Try to make this a dummy address that maps the specified onion address into
  71 | + * IPv6 using OnionCat's range and encoding. Such dummy addresses has a prefix

jonatack commented at 11:10 AM on April 4, 2019:

Nit: "has" -> "have".

  65 | @@ -52,6 +66,16 @@ bool CNetAddr::SetInternal(const std::string &name)
  66 |      return true;
  67 |  }
  68 |  
  69 | +/**
  70 | + * Try to make this a dummy address that maps the specified onion address into
  71 | + * IPv6 using OnionCat's range and encoding. Such dummy addresses has a prefix
  72 | + * of fd87:d87e:eb43::/48 and is guaranteed to not be publicly routable as it

jonatack commented at 11:11 AM on April 4, 2019:

Nit: "is" -> "are" and "it" -> "they".

  65 | @@ -52,6 +66,16 @@ bool CNetAddr::SetInternal(const std::string &name)
  66 |      return true;
  67 |  }
  68 |  
  69 | +/**
  70 | + * Try to make this a dummy address that maps the specified onion address into
  71 | + * IPv6 using OnionCat's range and encoding. Such dummy addresses has a prefix
  72 | + * of fd87:d87e:eb43::/48 and is guaranteed to not be publicly routable as it
  73 | + * falls under RFC4193's fc00::/7 subnet allocated to unique-local addresses.

jonatack commented at 11:11 AM on April 4, 2019:

Nit: "falls" -> "fall".

  45 | + * so: (0xFD + %sha256("bitcoin")[0:5]) + %sha256(name)[0:10]. Such dummy
  46 | + * addresses have a prefix of fd6b:88c0:8724::/48 and are guaranteed to not be
  47 | + * publicly routable as it falls under RFC4193's fc00::/7 subnet allocated to
  48 | + * unique-local addresses.
  49 | + *
  50 | + * CAddrMan can use this as a source address to identify DNS seed hostnames.

ryanofsky commented at 5:22 PM on April 5, 2019:

In commit "docs: netaddress: Improve {Is,Set}Internal docs" (ef22bf11b71a5b5e396fd15de5c9db979c9fe501)

While it's seems helpful to mention CAddrMan, the new comment seems less clear than the original "Useful for mapping resolved addresses back to their source". Would maybe suggest: "CAddrMan uses these fake addresses to keep track of which DNS seeds were used."

The next sentence about extra resolves doesn't also make sense to me, and I'd probably suggest dropping it. I'd expect if this method were not available, the obvious alternative for CAddrMan would be to track strings in addition to addresses, not to start doing extra resolves.

 224 | @@ -225,6 +225,10 @@ bool CNetAddr::IsLocal() const
 225 |      return false;
 226 |  }
 227 |  
 228 | +/**
 229 | + * @returns Whether or not this network address is a valid address that @a could
 230 | + *          be used to refer to an actual host.

ryanofsky commented at 5:28 PM on April 5, 2019:

In commit "docs: netaddress: Add IsValid docs" (2473fef57e54aaa2513cb0ff90ccfce3138fc66a)

Can the comment say how this is different than IsRoutable, or related to it?

 379 | @@ -380,8 +380,16 @@ bool CNetAddr::GetIn6Addr(struct in6_addr* pipv6Addr) const
 380 |      return true;
 381 |  }
 382 |  
 383 | -// get canonical identifier of an address' group
 384 | -// no two connections will be attempted to addresses with the same group
 385 | +/**
 386 | + * Get the canonical identifier of our network group
 387 | + *
 388 | + * Networks groups aim to group addresses by the (approximate) cost to obtain

ryanofsky commented at 6:58 PM on April 5, 2019:

In commit "docs: netaddress: Improve GetGroup docs" (862a5579d99e95bb159f4dfdca2d525ff239337d)

I think a more direct way of saying this would be: "The groups are assigned in a way where it should be costly for an attacker to obtain addresses with many different group identifiers, even if it is cheap to obtain addresses with the same identifier."

The way it's written now it sounds like this is trying to divide addresses up by how much the addresses cost (like expensive addresses grouped with expensive addresses, cheap ones grouped with cheap ones).

 651 |       std::vector<unsigned char> vKey;
 652 |       vKey.resize(18);
 653 |       memcpy(vKey.data(), ip, 16);
 654 | -     vKey[16] = port / 0x100;
 655 | -     vKey[17] = port & 0x0FF;
 656 | +     vKey[16] = port / 0x100; // first (most significant) byte of our port

ryanofsky commented at 7:36 PM on April 5, 2019:

In commit "docs: netaddress: Add CService::GetKey docs" (b3697888fcff02eccc84521c2b64e0ae21a8236a)

Would drop first/second and just say most significant/least significant or high/low.

With first/second it sounds like this is making an assumption about the byte order.