net: change CNetAddr::ip to have flexible size #19628

Are you sure the use of prevector is warranted here? Measurements would be nice.

Generally I think we should try stick with to the standard containers unless we have clear quantitative evidence showing that they are unsuitable for the scenario being considered. Reviewers, static analysers and compilers tend to be better at analysing use of the standard containers :)

FWIW:

$ git grep -l prevector -- "*.cpp" "*.h" ":(exclude)src/bench/" ":(exclude)src/test/"
src/hash.h
src/memusage.h
src/prevector.h
src/script/script.h
src/serialize.h

I suggested this, so let me back it up.

Currently an address in CAddrMan consumes 256 bytes (cost in mapInfo and mapAddr together, plus allocation overhead).

This PR as-is raises that to 288 bytes, for addresses that take up to 16 bytes (which is all of them, for now).

Using an std::vector<uint8_t> instead would change that to 352 bytes per address, or an extra 64 on top. It would also increase memory fragmentation and (slightly) increase CPU usage because of two malloc's per address.

FTR, my node has 69691 addresses in CAddrMan.

@sipa Thanks for quantifying! That answers my question :)

Thanks for the question and the elaborate explanation! Indeed smaller size and avoiding an extra heap allocation is better. FWIW I moved m_addr definition before m_net to save 8 bytes from the size of CNetAddr (32 vs 40 bytes on x64).

Slightly more modern:

return std::all_of(m_addr.begin(), m_addr.end(), [](uint8_t b) { return b == 0; });

Changed.

Not something you introduced, but this only works because those addresses have the same byte representation in every byte order.

Right! Even though the values of those constants are never going to change (I hope!), does the following look better?

for (uint32_t a : {htonl(INADDR_ANY), htonl(INADDR_NONE)}) {

That seems the best option, I think. It's too bad that there's no way to get these constants as byte spans that can be directly compared against m_addr so it wouldn't need the cast and memcmp at all.

Changed to use htonl().

More modern:

return std::tie(a.m_net, a.m_addr) < std::tie(b.m_net, b.m_addr);

Modernized :bulb:

Nit: uppercase v in global constants (here and further).

Changed to uppercase.

This is pretty scary cast expression. Wouldn't a cast to just const uint8_t* work just as well?

Passing const uint8_t* will not work as long as the argument of the method is a reference to an array: CNetAddr::SetLegacyIPv6(const uint8_t (&ipv6)[ADDR_IPV6_SIZE]) which is best of type safety but unfortunately two of the callers don't have an array to pass.

I think either one of the following two is better:

<details> <summary>change argument to void*</summary>

diff --git i/src/netaddress.cpp w/src/netaddress.cpp
index b421e1b11..bb4d2b50b 100644
--- i/src/netaddress.cpp
+++ w/src/netaddress.cpp
@@ -26,32 +26,33 @@ CNetAddr::CNetAddr() {}
 void CNetAddr::SetIP(const CNetAddr& ipIn)
 {
     m_net = ipIn.m_net;
     m_addr = ipIn.m_addr;
 }
 
-void CNetAddr::SetLegacyIPv6(const uint8_t (&ipv6)[ADDR_IPV6_SIZE])
+void CNetAddr::SetLegacyIPv6(const void* ipv6)
 {
-    const uint8_t* ipv6_end = ipv6 + ADDR_IPV6_SIZE;
+    const uint8_t* ipv6_begin = static_cast<const uint8_t*>(ipv6);
+    const uint8_t* ipv6_end = ipv6_begin + ADDR_IPV6_SIZE;
 
-    if (memcmp(ipv6, IPV4_IN_IPV6_PREFIX, sizeof(IPV4_IN_IPV6_PREFIX)) == 0) {
+    if (memcmp(ipv6_begin, IPV4_IN_IPV6_PREFIX, sizeof(IPV4_IN_IPV6_PREFIX)) == 0) {
         // IPv4-in-IPv6
         m_net = NET_IPV4;
-        m_addr.assign(ipv6 + sizeof(IPV4_IN_IPV6_PREFIX), ipv6_end);
-    } else if (memcmp(ipv6, TORV2_IN_IPV6_PREFIX, sizeof(TORV2_IN_IPV6_PREFIX)) == 0) {
+        m_addr.assign(ipv6_begin + sizeof(IPV4_IN_IPV6_PREFIX), ipv6_end);
+    } else if (memcmp(ipv6_begin, TORV2_IN_IPV6_PREFIX, sizeof(TORV2_IN_IPV6_PREFIX)) == 0) {
         // TORv2-in-IPv6
         m_net = NET_ONION;
-        m_addr.assign(ipv6 + sizeof(TORV2_IN_IPV6_PREFIX), ipv6_end);
-    } else if (memcmp(ipv6, INTERNAL_IN_IPV6_PREFIX, sizeof(INTERNAL_IN_IPV6_PREFIX)) == 0) {
+        m_addr.assign(ipv6_begin + sizeof(TORV2_IN_IPV6_PREFIX), ipv6_end);
+    } else if (memcmp(ipv6_begin, INTERNAL_IN_IPV6_PREFIX, sizeof(INTERNAL_IN_IPV6_PREFIX)) == 0) {
         // Internal-in-IPv6
         m_net = NET_INTERNAL;
-        m_addr.assign(ipv6 + sizeof(INTERNAL_IN_IPV6_PREFIX), ipv6_end);
+        m_addr.assign(ipv6_begin + sizeof(INTERNAL_IN_IPV6_PREFIX), ipv6_end);
     } else {
         // IPv6
         m_net = NET_IPV6;
-        m_addr.assign(ipv6, ipv6_end);
+        m_addr.assign(ipv6_begin, ipv6_end);
     }
 }
 
 /**
  * Try to make this a dummy address that maps the specified name into IPv6 like
  * so: (0xFD + %sha256("bitcoin")[0:5]) + %sha256(name)[0:10]. Such dummy
@@ -105,13 +106,13 @@ CNetAddr::CNetAddr(const struct in_addr& ipv4Addr)
     m_addr.assign(ptr, ptr + ADDR_IPV4_SIZE);
 }
 
 CNetAddr::CNetAddr(const struct in6_addr& ipv6Addr, const uint32_t scope)
 {
     assert(sizeof(ipv6Addr) == ADDR_IPV6_SIZE);
-    SetLegacyIPv6(*reinterpret_cast<const uint8_t (*)[ADDR_IPV6_SIZE]>(&ipv6Addr));
+    SetLegacyIPv6(&ipv6Addr);
     scopeId = scope;
 }
 
 bool CNetAddr::IsBindAny() const
 {
     if (!IsIPv4() && !IsIPv6()) {
diff --git i/src/netaddress.h w/src/netaddress.h
index 8924ed0b4..ca048fbf5 100644
--- i/src/netaddress.h
+++ w/src/netaddress.h
@@ -105,13 +105,13 @@ class CNetAddr
         /**
          * Set from a legacy IPv6 address.
          * Legacy IPv6 address may be a normal IPv6 address, or another address
          * (e.g. IPv4) disguised as IPv6. This encoding is used in the legacy
          * `addr` encoding.
          */
-        void SetLegacyIPv6(const uint8_t (&ipv6)[ADDR_IPV6_SIZE]);
+        void SetLegacyIPv6(const void* ipv6);
 
         bool SetInternal(const std::string& name);
 
         bool SetSpecial(const std::string &strName); // for Tor addresses
         bool IsBindAny() const; // INADDR_ANY equivalent
         bool IsIPv4() const;    // IPv4 mapped address (::FFFF:0:0/96, 0.0.0.0/0)
diff --git i/src/test/fuzz/asmap.cpp w/src/test/fuzz/asmap.cpp
index 84902ee2d..98ac82453 100644
--- i/src/test/fuzz/asmap.cpp
+++ w/src/test/fuzz/asmap.cpp
@@ -45,13 +45,13 @@ void test_one_input(const std::vector<uint8_t>& buffer)
     if (!SanityCheckASMap(asmap)) return;
 
     const uint8_t* addr_data = buffer.data() + 1 + asmap_size;
     CNetAddr net_addr;
     if (ipv6) {
         assert(addr_size == ADDR_IPV6_SIZE);
-        net_addr.SetLegacyIPv6(*reinterpret_cast<const uint8_t (*)[ADDR_IPV6_SIZE]>(addr_data));
+        net_addr.SetLegacyIPv6(addr_data);
     } else {
         assert(addr_size == ADDR_IPV4_SIZE);
         in_addr ipv4;
         memcpy(&ipv4, addr_data, addr_size);
         net_addr.SetIP(CNetAddr{ipv4});
     }

</details>

<details> <summary>change argument to Span</summary>

diff --git i/src/netaddress.cpp w/src/netaddress.cpp
index b421e1b11..121b92650 100644
--- i/src/netaddress.cpp
+++ w/src/netaddress.cpp
@@ -26,32 +26,32 @@ CNetAddr::CNetAddr() {}
 void CNetAddr::SetIP(const CNetAddr& ipIn)
 {
     m_net = ipIn.m_net;
     m_addr = ipIn.m_addr;
 }
 
-void CNetAddr::SetLegacyIPv6(const uint8_t (&ipv6)[ADDR_IPV6_SIZE])
+void CNetAddr::SetLegacyIPv6(Span<const uint8_t> ipv6)
 {
-    const uint8_t* ipv6_end = ipv6 + ADDR_IPV6_SIZE;
+    assert(ipv6.size() == ADDR_IPV6_SIZE);
 
-    if (memcmp(ipv6, IPV4_IN_IPV6_PREFIX, sizeof(IPV4_IN_IPV6_PREFIX)) == 0) {
+    if (memcmp(ipv6.begin(), IPV4_IN_IPV6_PREFIX, sizeof(IPV4_IN_IPV6_PREFIX)) == 0) {
         // IPv4-in-IPv6
         m_net = NET_IPV4;
-        m_addr.assign(ipv6 + sizeof(IPV4_IN_IPV6_PREFIX), ipv6_end);
-    } else if (memcmp(ipv6, TORV2_IN_IPV6_PREFIX, sizeof(TORV2_IN_IPV6_PREFIX)) == 0) {
+        m_addr.assign(ipv6.begin() + sizeof(IPV4_IN_IPV6_PREFIX), ipv6.end());
+    } else if (memcmp(ipv6.begin(), TORV2_IN_IPV6_PREFIX, sizeof(TORV2_IN_IPV6_PREFIX)) == 0) {
         // TORv2-in-IPv6
         m_net = NET_ONION;
-        m_addr.assign(ipv6 + sizeof(TORV2_IN_IPV6_PREFIX), ipv6_end);
-    } else if (memcmp(ipv6, INTERNAL_IN_IPV6_PREFIX, sizeof(INTERNAL_IN_IPV6_PREFIX)) == 0) {
+        m_addr.assign(ipv6.begin() + sizeof(TORV2_IN_IPV6_PREFIX), ipv6.end());
+    } else if (memcmp(ipv6.begin(), INTERNAL_IN_IPV6_PREFIX, sizeof(INTERNAL_IN_IPV6_PREFIX)) == 0) {
         // Internal-in-IPv6
         m_net = NET_INTERNAL;
-        m_addr.assign(ipv6 + sizeof(INTERNAL_IN_IPV6_PREFIX), ipv6_end);
+        m_addr.assign(ipv6.begin() + sizeof(INTERNAL_IN_IPV6_PREFIX), ipv6.end());
     } else {
         // IPv6
         m_net = NET_IPV6;
-        m_addr.assign(ipv6, ipv6_end);
+        m_addr.assign(ipv6.begin(), ipv6.end());
     }
 }
 
 /**
  * Try to make this a dummy address that maps the specified name into IPv6 like
  * so: (0xFD + %sha256("bitcoin")[0:5]) + %sha256(name)[0:10]. Such dummy
@@ -104,14 +104,13 @@ CNetAddr::CNetAddr(const struct in_addr& ipv4Addr)
     const uint8_t* ptr = reinterpret_cast<const uint8_t*>(&ipv4Addr);
     m_addr.assign(ptr, ptr + ADDR_IPV4_SIZE);
 }
 
 CNetAddr::CNetAddr(const struct in6_addr& ipv6Addr, const uint32_t scope)
 {
-    assert(sizeof(ipv6Addr) == ADDR_IPV6_SIZE);
-    SetLegacyIPv6(*reinterpret_cast<const uint8_t (*)[ADDR_IPV6_SIZE]>(&ipv6Addr));
+    SetLegacyIPv6(Span<const uint8_t>(reinterpret_cast<const uint8_t*>(&ipv6Addr), sizeof(ipv6Addr)));
     scopeId = scope;
 }
 
 bool CNetAddr::IsBindAny() const
 {
     if (!IsIPv4() && !IsIPv6()) {
diff --git i/src/netaddress.h w/src/netaddress.h
index 8924ed0b4..a76b5e9ce 100644
--- i/src/netaddress.h
+++ w/src/netaddress.h
@@ -105,13 +105,13 @@ class CNetAddr
         /**
          * Set from a legacy IPv6 address.
          * Legacy IPv6 address may be a normal IPv6 address, or another address
          * (e.g. IPv4) disguised as IPv6. This encoding is used in the legacy
          * `addr` encoding.
          */
-        void SetLegacyIPv6(const uint8_t (&ipv6)[ADDR_IPV6_SIZE]);
+        void SetLegacyIPv6(Span<const uint8_t> ipv6);
 
         bool SetInternal(const std::string& name);
 
         bool SetSpecial(const std::string &strName); // for Tor addresses
         bool IsBindAny() const; // INADDR_ANY equivalent
         bool IsIPv4() const;    // IPv4 mapped address (::FFFF:0:0/96, 0.0.0.0/0)
diff --git i/src/test/fuzz/asmap.cpp w/src/test/fuzz/asmap.cpp
index 84902ee2d..e3aefa18a 100644
--- i/src/test/fuzz/asmap.cpp
+++ w/src/test/fuzz/asmap.cpp
@@ -45,13 +45,13 @@ void test_one_input(const std::vector<uint8_t>& buffer)
     if (!SanityCheckASMap(asmap)) return;
 
     const uint8_t* addr_data = buffer.data() + 1 + asmap_size;
     CNetAddr net_addr;
     if (ipv6) {
         assert(addr_size == ADDR_IPV6_SIZE);
-        net_addr.SetLegacyIPv6(*reinterpret_cast<const uint8_t (*)[ADDR_IPV6_SIZE]>(addr_data));
+        net_addr.SetLegacyIPv6(Span<const uint8_t>(addr_data, addr_size));
     } else {
         assert(addr_size == ADDR_IPV4_SIZE);
         in_addr ipv4;
         memcpy(&ipv4, addr_data, addr_size);
         net_addr.SetIP(CNetAddr{ipv4});
     }

</details>

What do you think?

Span span span...

Done

Heh we need to make EncodeBase32 take a span too (not in this PR ofcourse).

Done in https://github.com/bitcoin/bitcoin/pull/19687

You do not need the "hh" here, or any type specifiers ever, strprintf is type safe. Also maybe include the / to avoid an extra string concatenation

network.ToString() + strprintf("/%u", cidr);

Changed.

perhaps place the NET_IPV6 case first to group the similar cases together

suggestion here and line 221 to be the same as the first case line 205:

                assert(prefix_size + m_addr.size() == sizeof(arr));

Done.

per developer-notes.md:

    } // no default case, so the compiler can warn about missing cases
    assert(false);

Done.

Add the assert as well as the comment?

Hmm, I almost added the assert(false); but then realized that the code flow just continues after the switch (it contains break;, not return; as the example in the developer-notes).

Apply this?

<details> <summary>diff</summary>

diff --git i/src/netaddress.h w/src/netaddress.h
index b75eff1b9..8cd2401e2 100644
--- i/src/netaddress.h
+++ w/src/netaddress.h
@@ -200,36 +200,38 @@ class CNetAddr
             size_t prefix_size;
 
             switch (m_net) {
             case NET_IPV6:
                 assert(m_addr.size() == sizeof(arr));
                 memcpy(arr, m_addr.data(), m_addr.size());
-                break;
+                return;
             case NET_IPV4:
                 prefix_size = sizeof(IPV4_IN_IPV6_PREFIX);
                 assert(prefix_size + m_addr.size() == sizeof(arr));
                 memcpy(arr, IPV4_IN_IPV6_PREFIX, prefix_size);
                 memcpy(arr + prefix_size, m_addr.data(), m_addr.size());
-                break;
+                return;
             case NET_ONION:
                 prefix_size = sizeof(TORV2_IN_IPV6_PREFIX);
                 assert(prefix_size + m_addr.size() == sizeof(arr));
                 memcpy(arr, TORV2_IN_IPV6_PREFIX, prefix_size);
                 memcpy(arr + prefix_size, m_addr.data(), m_addr.size());
-                break;
+                return;
             case NET_INTERNAL:
                 prefix_size = sizeof(INTERNAL_IN_IPV6_PREFIX);
                 assert(prefix_size + m_addr.size() == sizeof(arr));
                 memcpy(arr, INTERNAL_IN_IPV6_PREFIX, prefix_size);
                 memcpy(arr + prefix_size, m_addr.data(), m_addr.size());
-                break;
+                return;
             case NET_UNROUTABLE:
             case NET_MAX:
                 memset(arr, 0x0, sizeof(arr));
-                break;
+                return;
             } // no default case, so the compiler can warn about missing cases
+
+            assert(false);
         }
 
         /**
          * Serialize in pre-ADDRv2/BIP155 format to a stream.
          * Some addresses (e.g. TORv3) cannot be serialized in pre-BIP155 format.
          */

</details>

I should have mentioned that. Returning LGTM, nothing else is being done in the function.

Changed break; to return; and added assert(false); after the switch.

nBits is currently given a value a few lines down but seems a good habit to always provide a default.

    int nBits{0};

Default assigning in the case where we are supposed to always assign the variable later will suppress valgrind and compiler warnings if there is a bug in the code and it does not actually initialize the variable in some cases.

(there's nuance to be sure; if helpful:)

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es20-always-initialize-an-object

Some history and reading on uninitialized reads in Bitcoin Core: https://bitcoincore.reviews/17639

See also this comment and thread #17627 (comment)

Initialized to 0. Note that the Cpp Core Guidelines contain an example that resembles our case and admits that "This cannot trivially be rewritten to initialize i and j with initializers".

We have:

int nBits;
... code that is supposed to set nBits ... // [1]
read nBits // [2]

So if there is a bug in [1] and nBits is not set (or if new code is added that reads nBits before [1]), then we would get undefined behavior in [2], but we would also get compiler, msan and valgrind warnings about it, so not likely to remain unnoticed.

OTOH if we initialize nBits with some dummy_value and the same bugs happen, then we would not get undefined behavior, but we would get unexpected results due to [2] reading dummy_value. And the downside is that none of the compiler, msan or valgrind will warn about the bug.

Yes. Attempted at the time to summarize the trade-offs in the review club notes I linked to (LMK if you see any errors, I wanted it to be a resource).

Edit: Thanks @vasild for the offline suggestions on the review club notes.

Maybe it is time to add an init macro that default initializes a variable, but disables the initialization when --enable-debug or so?

consider using the pre-increment (++i) operator

Done.

nit, you could reformat this to more "modern" C++ (though I'm not sure if it is less or more readable)

if(std::equal(std::cbegin(IPV4_IN_IPV6_PREFIX), std::cend(IPV4_IN_IPV6_PREFIX), ipv6.cbegin()))

(you could replace std::cbegin(IPV4_IN_IPV6_PREFIX) with IPV4_IN_IPV6_PREFIX.cbegin() if you make it a std::array and not a C style array)

Agree with @elichai's suggestion, std::equal is nicer.

Given how many of these tests are being done (including the IsRFC... stuff below), perhaps you want to go even further (this is absolutely not a blocker for this PR, however):

template<typename T, int LEN>
bool HasPrefix(const T1& obj, const uint8_t (&prefix)[LEN])
{
    return obj.size() >= LEN && std::equal(std::cbegin(prefix), std::cend(prefix), std::cbegin(obj));
}

...

if (HasPrefix(ipv6, IPV4_IN_IPV6_PREFIX)) ...

If you turn the prefixes into std::array it could be

template<typename T1, typename T2>
bool HasPrefix(const T1& obj, const T2& prefix)
{
    return obj.size() >= prefix.size() && std::equal(std::cbegin(prefix), std::cend(prefix), std::cbegin(obj));
}

instead.

In C++17 HasPrefix will be possible even with C arrays :D (by replacing .size() with std::size(..))

Done - diff together with other suggestions.

Nit: { } around indented blocks, if you're touching this code anyway.

I added { } to all places where the line containing if/for/while is modified for another reason (this one is not).

Placing this assertion here seems weird to me. Shouldn't this check be done when(ever) m_net is modified? Same with IsIPv6() below.

Done - diff together with other suggestions.

The opposite seems more straightforward:

if (IsIPv4()) {
    uint32_t a;
    memcpy(&a, m_addr.data(), sizeof(a));
    if (a == htonl(INADDR_ANY) || a == htonl(INADDR_NONE)) return false;
}

@sipa might yell at me but it is also possible to do uint32_t a = *(uint32_t*)m_addr.data(); to skip the memcpy line.

It seems I've caused a lot of confusion here. Everything works here of course due to INADDR_ANY and INADDR_NONE being the same in LE and BE, but if they weren't, the code here would actually be wrong.

These constants (and the s_addr field of struct in_addr) are uint32_t in name only; they're really 4 bytes in big endian notation storing an IP address. So the IP address 1.2.3.4 would be stored as the bytes {0x01,0x02,0x03,0x04}, which on LE systems would be the integer 0x04030201 while on BE systems it would be 0x01020304.

htonl is converting them from network order (BE) to native order, so it would result in the integer 0x01020304 on both systems. That's surprisingly not what we want, we want it in network order, because that's the order we store in m_addr (but as bytes, rather than a fake-uint32_t).

This would work:

static constexpr uint32_t ANY = INADDR_ANY;
static constexpr uint32_t ALL = INADDR_ALL;
if (memcmp((const uint8_t*)&ANY, m_addr.data(), sizeof(ANY) ||
    memcmp((const uint8_t*)&ALL, m_addr.data(), sizeof(ALL)) ...

this would also work:

uint32_t a;
memcpy((uint8_t*)&a, m_addr.data(), sizeof(a));
if (a == INADDR_ANY || a == INADDR_ALL) ...

@kallewoof Your suggestion is UB, I'm afraid. m_addr is an array of bytes, you can't access it through a pointer cast to another type (except char* or unsigned char*, which can be used to access the in-memory representation of any object).

Those constants are defined like this in the system headers:

typedef uint32_t in_addr_t;
...
#define	INADDR_ANY		((in_addr_t) 0x00000000)
#define	INADDR_NONE		((in_addr_t) 0xffffffff)
...
#define INADDR_ALLHOSTS_GROUP	((in_addr_t) 0xe0000001) /* 224.0.0.1 */

Of course any discussion about the byte order of INADDR_ANY is meaningless, but nevertheless - we can assume that all INADDR_* are in the same byte order. From the above we see that 224.0.0.1 is represented as the integer 3758096385 or 0xe0000001 which on LE systems is stored in memory as 01 00 00 e0 and on BE systems as e0 00 00 01.

This means that whenever we want to compare those constants to something we know is in BE byte order (like m_addr) we have to apply htonl() to them. This is how it is done in the Linux and FreeBSD system headers.

So, leaving this as is at this iteration.

You're right.

I was confused by the fact that struct in_addr's s_addr field stores IP addresses in BE order, but the INADDR_* constants are in native order (and you are supposed to use htonl & friends to convert between them; see the section on sin_addr in https://man7.org/linux/man-pages/man7/ip.7.html).

Arguably you should be using ntohl instead of htonl (as these constants are in native order, and we want them in BE order) - but on every realistic platform these two are equivalent.

If you want to avoid the winsock/inet header dependency you can also use WriteBE32 to convert them to bytes, or:

uint32_t a = ReadBE32(m_data.data());
if (a == INADDR_ANY || a == INADDR_ALL) ...

as @sipa said

uint32_t a = ReadBE32(m_data.data());
if (a == INADDR_ANY || a == INADDR_ALL) ...

is so much more readable :)

re: #19628 (review)

@kallewoof Your suggestion is UB, I'm afraid

Just in case anybody else reading this gets confused like me, this is /not/ saying @kallewoof memcpy suggestion is UB. Only the second suggestion without memcpy is UB

Oops, yes, I guess this confused @kallewoof too (on IRC). Specifially, this is UB:

uint32_t a = *(uint32_t*)m_addr.data();

as it's accessing a byte array through an incompatible type.

Alright, we have a clear winner with

uint32_t a = ReadBE32(m_data.data());
if (a == INADDR_ANY || a == INADDR_ALL) ...

Changed.

It looks like you can do this as a copy operation now that the ordering is the same.

size_t i = nBits >> 3;
vchRet.insert(vchRet.end(), m_addr.begin(), m_addr.begin() + i);
nBits &= 0x7;

I believe that's correct.

Done - diff together with other suggestions.

I don't think this is correct, for two reasons:

• We technically support non-CIDR subnets (which are allowed to contain an arbitrary mix of 0 and 1 bits).
• This test isn't even implementing whether the mask is CIDR, as it's only checking whether no 1-bits-after-0-bits occur within one byte of the mask, rather than the entire mask.

I wouldn't be opposed to dropping non-CIDR netmasks, but if we do, it should be a separate PR, I think.

Uh, oh, you found a bug (the second item above)! Fixed - diff together with other suggestions.

A summary about the netmasks: #19628 (comment)

Braces, please.

(for most rules I don't care that strongly whether you stick to the existing/surrounding style vs. follow the new style, but a braceless if has actually been part of the cause of a nontrivial security bug, see https://www.imperialviolet.org/2014/02/22/applebug.html)

Done - diff together with other suggestions.

Here too it seems you're dropping support for non-CIDR masks. Is that intentional?

Yes, see #19628 (comment) - it is mentioned in the commit message and also some tests are adjusted.

In commit "net: change CNetAddr::ip to have flexible size" (86b2397faedcd9b57649ddb90e98c8cf6a4a6ad3)

Maybe s/16/ADDR_IPV6_SIZE/ since code would be broken using any other size.

Done.

In commit "net: change CNetAddr::ip to have flexible size" (86b2397faedcd9b57649ddb90e98c8cf6a4a6ad3)

Can you add a comment to explain why this is serializing 0's rather than asserting false in both of these cases? Maybe say when this code could be called to serialize unroutable addresses and when it could be called to serialize invalid NET_MAX addresses.

There is no reason to serialize as 0s. NET_UNROUTABLE and NET_MAX are never set to m_net and must never be set. Changed this to assert(false).

I had to google this syntax :/ (for a second I thought it was a C trick to make arrays work, but it's actually C++) could we please use const std::array<uint8_t, N>& instead? :)

or just make this generic over all containers

generic over all containers

Span !?

I can't figure out how to use std::array or Span and keep the usage to just HasPrefix(m_addr, {192, 168}) (which is sweet!), so I am keeping the reference to array.

The current one doesn't compile either on gcc 4.8:

netaddress.cpp: In member function ‘bool CNetAddr::IsRFC3927() const’:

netaddress.cpp:163:52: error: no matching function for call to ‘HasPrefix(const prevector<16u, unsigned char>&, <brace-enclosed initializer list>)’

     return IsIPv4() && HasPrefix(m_addr, {169, 254});

                                                    ^

netaddress.cpp:163:52: note: candidate is:

netaddress.cpp:52:13: note: template<class T1, class T2, unsigned int PREFIX_LEN> bool HasPrefix(const T1&, const T2 (&)[PREFIX_LEN])

 inline bool HasPrefix(const T1& obj, const T2 (&prefix)[PREFIX_LEN])

             ^

netaddress.cpp:52:13: note:   template argument deduction/substitution failed:

netaddress.cpp:163:52: note:   couldn't deduce template parameter ‘T2’

     return IsIPv4() && HasPrefix(m_addr, {169, 254});

                                                    ^

@vasild Nice, that indeed works, and I don't see how that could work with std::array (the on-the-fly constructed array's length template parameter can't be figured out from the list of elements). It certainly can't work for Span as there needs to be some object that actually holds the elements. @elichai I guess you have an answer to the question you asked on IRC yesterday "why would someone use a C-style array over std::array?".

@vasild @MarcoFalke I think that can be fixed by removing the T2 template parameter, and just hardcoding it to an array of uint8_t (but templated size).

@sipa Yep, I guess C arrays are still treated quite differently than anything else (more like initilaizer_list etc) :( (interesting: https://stackoverflow.com/a/61390991/1621927)

I think that can be fixed by removing the T2 template parameter, and just hardcoding it to an array of uint8_t

I think that can be fixed by removing the T2 template parameter, and just hardcoding it to an array of uint8_t (but templated size).

It does not compile either because then gcc 4.8 complained it cannot deduce the length from the initialiser list, so I switched to std::array :-/

This ended up with const std::array<uint8_t, N>& anyway, due to GCC 4.8. Will change it back to reference to an array once the support for GCC 4.8 is removed.

In commit "net: change CNetAddr::ip to have flexible size" (86b2397faedcd9b57649ddb90e98c8cf6a4a6ad3)

Having a ParseUInt8() function would seem more appealing than layering uint8 range checking on top of uint32 range checking done by ParseUInt32

Right, added ParseUint8().

In commit "net: change CNetAddr::ip to have flexible size" (86b2397faedcd9b57649ddb90e98c8cf6a4a6ad3)

Precedes this PR, so maybe not worth changing here, but this method seems more awkward to use for assignment than plain operator=, and the SetIP method name is misleading since it works for all addresses, not just IP addresses. I think it would better to just use plain = for address assignment, and have a separate AssertValid() method if it's helpful to have runtime asserts in places like this.

I agree. I considered swapping this for operator=, but cut it off, trying to keep the patch small. There are so many things that can be improved around this code!

In commit "net: change CNetAddr::ip to have flexible size" (86b2397faedcd9b57649ddb90e98c8cf6a4a6ad3)

It would be nice not to drop this explanation. I think it could move to the SerializeV1Array function or be incorporated in the TORV2_IN_IPV6_PREFIX comment.

Moved the comment to TORV2_IN_IPV6_PREFIX and similar with SetInternal() and INTERNAL_IN_IPV6_PREFIX.

In commit "net: change CNetAddr::ip to have flexible size" (86b2397faedcd9b57649ddb90e98c8cf6a4a6ad3)

Not sure if it's portable, but it seems like you can avoid the casting and span declaration on this line with just SetLegacyIPv6(ipv6Addr.s6_addr);

Looks like it is not: https://docs.microsoft.com/en-us/windows/win32/api/in6addr/ns-in6addr-in6_addr :-(

In commit "net: change CNetAddr::ip to have flexible size" (86b2397faedcd9b57649ddb90e98c8cf6a4a6ad3)

Maybe would be good to static assert that in_addr size is the same as ADDR_IPV4_SIZE

I think that's not necessary. It was also the case before this PR that struct in_addr would be assumed to be 4 bytes. I don't see this ever changing.

In commit "net: change CNetAddr::ip to have flexible size" (86b2397faedcd9b57649ddb90e98c8cf6a4a6ad3)

Now that this is calling memcpy with dynamic size rather than fixed size, I think it'd be good to add an assert to check that it doesn't overflow the in_addr struct. Same suggestion for in6_addr struct below.

Done

In commit "net: change CNetAddr::ip to have flexible size" (86b2397faedcd9b57649ddb90e98c8cf6a4a6ad3)

I think this could be a little safer with bounds checking, and more readable as ReadBE32(MakeSpan(m_addr).last(ADDR_IPV4_SIZE).data()). Similarly for RFC4380 case below.

Done

In commit "net: change CNetAddr::ip to have flexible size" (86b2397faedcd9b57649ddb90e98c8cf6a4a6ad3)

Probably my fault, but I did spend a minute looking at this diff being baffled by nBits >> 3 and nBits &= 0x7. Would suggest changing to size_t bytes = nBits / 8 and nBits = nBits % 8 to give a few more hints for comprehension.

Done

In commit "net: change CNetAddr::ip to have flexible size" (86b2397faedcd9b57649ddb90e98c8cf6a4a6ad3)

It seems like this assert would make more sense moved before the vchRet.insert statement above, and not conditioned on nBits divisibility.

It is here because here we access m_addr[i]. I would normally use m_addr.at(i) if there is a chance it may get out of bounds and not write an explicit separate assert, but prevector does not have an at() method :/

re: #19628 (review)

It is here because here we access m_addr[i]. I would normally use m_addr.at(i) if there is a chance it may get out of bounds and not write an explicit separate assert, but prevector does not have an at() method :/

Right, but if you assume the assert is false and num_bytes >= m_addr.size(), then the expression a few lines earlier m_addr.begin() + num_bytes will read from uninitialized memory. So I'm saying it might make sense to move the assert earlier there. Not very important, though.

In commit "net: change CNetAddr::ip to have flexible size" (86b2397faedcd9b57649ddb90e98c8cf6a4a6ad3)

Might be worth mentioning in commit message CNetAddr now has returns different hash values. It seems like GetHash is only called one place in net_processing. Maybe this is the only change in the PR that's not just refactoring and actually affects externally observable behavior.

Hmm, that hash is only used when relaying addresses and is not saved on disk or sent over the network. The current time is also thrown to the hasher, together with the result of the changed CNetAddr::GetHash() and also with the relay destination. So the final result from the hasher is likely to change even after just a bitcoind restart (because it will connect to other nodes) or after 24h or less. I think it is not necessary to mention this in the commit message.

The hashing (in RelayAddress) is actually salted by a value that's randomly generated at startup (through CConnman::GetDeterministicRandomizer), so it will in fact always be different after restart.

You could add a comment in the commit description, but I don't think it needs more exposure than that.

In commit "net: change CNetAddr::ip to have flexible size" (86b2397faedcd9b57649ddb90e98c8cf6a4a6ad3)

Value of size variable used here seems fragile. Seems like it would be better to not assume any particular network type and use network.m_addr_size() like code above seems to do.

Same suggestion for operator < below.

a.network.m_addr is protected here and can't be accessed. Anyway, since we only do subnetting of IPv4 and IPv6 I added an assert to ensure that this is the case.

Perhaps point out where these are used. I think the -rpcallowip parameter, and the setban RPC are the only ones?

Will do so in a followup.

Not a comment on this PR (it makes sense to not change any further behavior), but 0.2.8 is over 10 years old now, and can't even communicate with current software due to the mandatory checksum change in 2012. It's probably fine to drop this code at some point.

Append " (#19628)"

Will do so in a followup, to keep the current ACKs.

μ-nit: not sure why this is T1 and not simply T

This was T initially, then was changed to T1 and T2, then I dropped the second one but forgot to rename T1 to T. Will take are about it when touching this after GCC 4.8 is no longer supported.

If we for a brief moment pretended that prevector.h:258 had this line added to it:

    static prevector from(std::initializer_list<T> il) { return prevector(il.begin(), il.end()); }

then it would have been worth it to do:

static auto ipv6_loopback = prevector<16, uint8_t>::from({0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1});
if (IsIPv6() && m_addr == ipv6_loopback) {

Alas, that prevector line would need to be added. Not sure it's worth it, or if it should be its own PR perhaps.

Edit: I initially suggested doing an initializer list constructor, but that interferes e.g. with the 2 element constructor messing stuff up and being generally unpredictable. The above may be too messy to be warranted, but I'll leave it up anyway.

I think it is also important to not deviate from std::vector interface so that it is not too much hassle should one decide to swap prevector for std::vector or the other way around.

Btw, std::vector has an initializer list constructor.

Anyway - leaving this as is.

Yeah, it may be that my lines below (the ipNode6) were in fact invalid, and were the cause for the errors I saw when I did this as a prevector constructor, where I didn't initially cast 16 to size_t in ipNone6{16, 0x0}. I much prefer the constructor variant, which is also closer to std::vector. FTR, that was

prevector(std::initializer_list<T> il) : prevector(il.begin(), il.end()) {}

and the line here beomes

static prevector<ADDR_IPV6_SIZE, uint8_t> ipv6_loopback{0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1};

I wonder if we can avoid defining a local variable for IPv6 loopback address and use IN6ADDR_LOOPBACK_INIT or in6addr_loopback...

This could be

    static prevector<ADDR_IPV6_SIZE, uint8_t> ipNone6{ADDR_IPV6_SIZE, 0x0};
    if (IsIPv6() && m_addr == ipNone6) {

Right! I will apply it if I modify this PR.

ADDR_IPV6_SIZE is of type size_t, the typecast should not be needed?

Yeah, I had originally put 16 there but realized ADDR_IPV6_SIZE was better.