Merge bitcoin#30617: net: Clarify that m_addr_local is only set once

fa6fe43 net: Clarify that m_addr_local is only set once (MarcoFalke)

Pull request description:

  The function is supposed to be only called once when the version msg arrives (a single time). Calling it twice would be an internal logic bug. However, the `LogError` in this function has many issues:

  * If the error happens in tests, as is the case for the buggy fuzz test, it will go unnoticed
  * It is dead code, unless a bug is introduced to execute it

  Fix all issues by using `Assume(!m_addr_local.IsValid())` instead. Idea taken from bitcoin#30364 (comment)

ACKs for top commit:
  achow101:
    ACK fa6fe43
  mzumsande:
    utACK fa6fe43
  glozow:
    ACK fa6fe43

Tree-SHA512: 8c1e8c524768f4f36cc50110ae54ee423e057a963ff78f736f3bf92df1ce5af28e3e0149153780897944e1d5c22ddbca9dac9865d9f4d44afffa152bc8559405

Author: glozow

src/net.cpp

@@ -576,16 +576,14 @@ CService CNode::GetAddrLocal() const
 {
     AssertLockNotHeld(m_addr_local_mutex);
     LOCK(m_addr_local_mutex);
-    return addrLocal;
+    return m_addr_local;
 }
 
 void CNode::SetAddrLocal(const CService& addrLocalIn) {
     AssertLockNotHeld(m_addr_local_mutex);
     LOCK(m_addr_local_mutex);
-    if (addrLocal.IsValid()) {
-        LogError("Addr local already set for node: %i. Refusing to change from %s to %s\n", id, addrLocal.ToStringAddrPort(), addrLocalIn.ToStringAddrPort());
-    } else {
-        addrLocal = addrLocalIn;
+    if (Assume(!m_addr_local.IsValid())) { // Addr local can only be set once during version msg processing
+        m_addr_local = addrLocalIn;
     }
 }
 

src/net.h

@@ -963,7 +963,7 @@ class CNode
     size_t m_msg_process_queue_size GUARDED_BY(m_msg_process_queue_mutex){0};
 
     // Our address, as reported by the peer
-    CService addrLocal GUARDED_BY(m_addr_local_mutex);
+    CService m_addr_local GUARDED_BY(m_addr_local_mutex);
     mutable Mutex m_addr_local_mutex;
 
     mapMsgTypeSize mapSendBytesPerMsgType GUARDED_BY(cs_vSend);

src/test/fuzz/net.cpp

@@ -33,6 +33,11 @@ FUZZ_TARGET(net, .init = initialize_net)
     SetMockTime(ConsumeTime(fuzzed_data_provider));
     CNode node{ConsumeNode(fuzzed_data_provider)};
     node.SetCommonVersion(fuzzed_data_provider.ConsumeIntegral<int>());
+    if (const auto service_opt =
+            ConsumeDeserializable<CService>(fuzzed_data_provider, ConsumeDeserializationParams<CNetAddr::SerParams>(fuzzed_data_provider)))
+    {
+        node.SetAddrLocal(*service_opt);
+    }
     LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) {
         CallOneOf(
             fuzzed_data_provider,
@@ -52,13 +57,6 @@ FUZZ_TARGET(net, .init = initialize_net)
                     node.Release();
                 }
             },
-            [&] {
-                const std::optional<CService> service_opt = ConsumeDeserializable<CService>(fuzzed_data_provider, ConsumeDeserializationParams<CNetAddr::SerParams>(fuzzed_data_provider));
-                if (!service_opt) {
-                    return;
-                }
-                node.SetAddrLocal(*service_opt);
-            },
             [&] {
                 const std::vector<uint8_t> b = ConsumeRandomLengthByteVector(fuzzed_data_provider);
                 bool complete;