Move special CAddress-without-nTime logic to net_processing #20541

"services" must go before the address and port.

            (uint64_t)addr.nServices, addr_you, // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime)
            (uint64_t)nLocalNodeServices, CService(), // Together the pre-version-31402 serialization of CAddress "addrMe" (without nTime)

Yes. No test caught this.

Fixed.

What is 31402?

$ git grep 31402
$

31402 is the protocol version that introduced the nTime field in CAddress. INIT_PROTO_VERSION (=209) being below it is the reason why the addresses inside the version message don't have it. There is no code left that depends on this, as all connections must be MIN_PEER_PROTO_VERSION (31800) after version negotiation now.

• 209 is from may 2010
• 31402 is from october 2010
• 31800 is from december 2010

ditto: swap services and address

Fixed.

ditto: swap services and address

Fixed.

By removing this we assume that for non-disk serialization s.GetType() & SER_GETHASH is always false. I.e. SER_GETHASH is not used in non-disk ser/deser of CAddress. I think this is the case indeed.

Yes, this is the case. #20516 adds an assert for this, actually.

Since addFrom is just thrown away, maybe make this more explicit:

--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2439,7 +2439,6 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
 
         int64_t nTime;
         CAddress addrMe;
-        CAddress addrFrom;
         uint64_t nNonce = 1;
         ServiceFlags nServices;
         int nVersion;
@@ -2472,8 +2471,12 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         }
 
         if (!vRecv.empty()) {
-            // The pre-31402 serialization of addrFrom (which didn't include nTime).
-            vRecv >> Using<CustomUintFormatter<8>>(addrFrom.nServices) >> static_cast<CService&>(addrFrom);
+            // The version message includes information about the sending node which we don't use:
+            //   - 8 bytes (service bits)
+            //   - 16 bytes (ipv6 address)
+            //   - 2 bytes (port)
+            uint8_t unused[26];
+            vRecv >> unused;
             vRecv >> nNonce;
         }
         if (!vRecv.empty()) {

Agree that addrFrom can be removed completely if it is never read from

Done (using vRecv.Ignore(26); instead).

Thanks for teaching me about CDataStream.ignore()!

The service bits here are never used (we don't care what the peer thinks our services are), so we can be more explicit in discarding them and only using the CService data:

--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2438,7 +2438,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         }
 
         int64_t nTime;
-        CAddress addrMe;
+        CService addrMe;
         CAddress addrFrom;
         uint64_t nNonce = 1;
         ServiceFlags nServices;
@@ -2451,8 +2451,10 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         if (nTime < 0) {
             nTime = 0;
         }
-        // The pre-31402 serialization of addrMe (which didn't include nTime):
-        vRecv >> Using<CustomUintFormatter<8>>(addrMe.nServices) >> static_cast<CService&>(addrMe);
+        // Ignore the addrMe service bits sent by the peer
+        uint8_t unused_service_bits[8];
+        vRecv >> unused_service_bits;
+        vRecv >> addrMe;
         if (!pfrom.IsInboundConn())
         {
             m_addrman.SetServices(pfrom.addr, nServices);

Done (using vRecv.ignore(8); instead).

Rather than having these casts in the Make() call, perhaps it's a bit more legible as:

--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1034,18 +1034,19 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, int64_t nTime)
     // Note that pnode->GetLocalServices() is a reflection of the local
     // services we were offering when the CNode object was created for this
     // peer.
-    ServiceFlags nLocalNodeServices = pnode.GetLocalServices();
+    auto my_services = static_cast<uint64_t>(pnode.GetLocalServices());
     uint64_t nonce = pnode.GetLocalNonce();
     const int nNodeStartingHeight{m_best_height};
     NodeId nodeid = pnode.GetId();
     CAddress addr = pnode.addr;
 
     CService addr_you = addr.IsRoutable() && !IsProxy(addr) && addr.IsAddrV1Compatible() ? addr : CService();
+    auto your_services = static_cast<uint64_t>(addr.nServices);
 
     const bool tx_relay = !m_ignore_incoming_txs && pnode.m_tx_relay != nullptr;
-    m_connman.PushMessage(&pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, PROTOCOL_VERSION, (uint64_t)nLocalNodeServices, nTime,
-            (uint64_t)addr.nServices, addr_you, // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime)
-            (uint64_t)nLocalNodeServices, CService(), // Together the pre-version-31402 serialization of CAddress "addrMe" (without nTime)
+    m_connman.PushMessage(&pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, PROTOCOL_VERSION, my_services, nTime,
+            your_services, addr_you, // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime)
+            my_services, CService(), // Together the pre-version-31402 serialization of CAddress "addrMe" (without nTime)
             nonce, strSubVersion, nNodeStartingHeight, tx_relay));
 
     if (fLogIPs) {

Done.

I'm not sure that references to version 31402 are very useful (beyond being a historical footnote). The version message contains {services, ipv6 addr, port} elements - the fact that that also used to be how address records used to be serialized in addr messages is not helpful in understanding the current protocol.

I think they're useful for the time being, as I'd expect more descriptions and documentations of the protocol will keep referring to these fields as CAddresses.

Seems reasonable

nNonce uninitialized?

Fixed.

uint64_t my_services{pnode.GetLocalServices()}; is shorter, safer, and looks better

Agree, done.

same

Done.

nit, can be const

-    uint64_t my_services{pnode.GetLocalServices()};
-    uint64_t nonce = pnode.GetLocalNonce();
+    const uint64_t my_services{pnode.GetLocalServices()};
+    const uint64_t nonce{pnode.GetLocalNonce()};
     const int nNodeStartingHeight{m_best_height};
-    NodeId nodeid = pnode.GetId();
+    const NodeId nodeid{pnode.GetId()};
     CAddress addr = pnode.addr;
 
-    CService addr_you = addr.IsRoutable() && !IsProxy(addr) && addr.IsAddrV1Compatible() ? addr : CService();
-    uint64_t your_services{addr.nServices};
+    const CService addr_you{addr.IsRoutable() && !IsProxy(addr) && addr.IsAddrV1Compatible() ? addr : CService()};
+    const uint64_t your_services{addr.nServices};

Will include this if I have to retouch for other reasons.