Merge bitcoin/bitcoin#34741: refactor: Return std::optional from GetNameProxy/GetProxy

fa73ed4 refactor: Fix redundant conversion to std::string and then to std::string_view [performance-string-view-conversions] (MarcoFalke)
fa270fd refactor: Return std::optional from GetProxy (MarcoFalke)
faeac1a refactor: Return std::optional from GetNameProxy (MarcoFalke)

Pull request description:

  Currently the getters have a mutable reference as inout param and return a bool to indicate success. This is confusing, because the success bool is redundant with the `IsValid()` state on the proxy object.

  So in theory, the functions could reset the mutable proxy object to an invalid state and return `void`.

  However, this would also be confusing, because devs can forget to check `IsValid()`.

  Fix all issues by using `std::optional<Proxy>`, where devs no longer have to check `IsValid()` manually, or a separate bool. Note that new code in the repo is already using `std::optional<Proxy>`, see `git grep 'std::optional<Proxy>' bitcoin-core/master`. Also, `std::optional<Proxy>` will enforce checking at compile time, whereas calling `Proxy::IsValid` is not enforced.

ACKs for top commit:
  achow101:
    ACK fa73ed4
  sedited:
    ACK fa73ed4
  ViniciusCestarii:
    ACK fa73ed4
  frankomosh:
    Code Review ACK fa73ed4. Good refactor, correctly replaces the bool + mutable reference output parameter pattern with `std::optional<Proxy>` across `GetProxy` and `GetNameProxy`. Semantics are preserved.

Tree-SHA512: c6a1e1d1691958d2e6507e32e3484f96703fba03ccc710145ae2fb84b1254fb0e6e1d8d78e9b572daf5ea485247b73568704881762379b50bcf939a35494dd13

Author: achow101

src/interfaces/node.h

@@ -19,6 +19,7 @@
 #include <cstdint>
 #include <functional>
 #include <memory>
+#include <optional>
 #include <string>
 #include <tuple>
 #include <vector>
@@ -123,7 +124,7 @@ class Node
     virtual void mapPort(bool enable) = 0;
 
     //! Get proxy.
-    virtual bool getProxy(Network net, Proxy& proxy_info) = 0;
+    virtual std::optional<Proxy> getProxy(Network net) = 0;
 
     //! Get number of connections.
     virtual size_t getNodeCount(ConnectionDirection flags) = 0;

src/net.cpp

@@ -439,20 +439,15 @@ CNode* CConnman::ConnectNode(CAddress addrConnect,
 
     // Connect
     std::unique_ptr<Sock> sock;
-    Proxy proxy;
     CService addr_bind;
     assert(!addr_bind.IsValid());
     std::unique_ptr<i2p::sam::Session> i2p_transient_session;
 
-    for (auto& target_addr: connect_to) {
+    for (auto& target_addr : connect_to) {
         if (target_addr.IsValid()) {
-            bool use_proxy;
-            if (proxy_override.has_value()) {
-                use_proxy = true;
-                proxy = proxy_override.value();
-            } else {
-                use_proxy = GetProxy(target_addr.GetNetwork(), proxy);
-            }
+            const std::optional<Proxy> use_proxy{
+                proxy_override.has_value() ? proxy_override : GetProxy(target_addr.GetNetwork()),
+            };
             bool proxyConnectionFailed = false;
 
             if (target_addr.IsI2P() && use_proxy) {
@@ -469,7 +464,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect,
                         LOCK(m_unused_i2p_sessions_mutex);
                         if (m_unused_i2p_sessions.empty()) {
                             i2p_transient_session =
-                                std::make_unique<i2p::sam::Session>(proxy, m_interrupt_net);
+                                std::make_unique<i2p::sam::Session>(*use_proxy, m_interrupt_net);
                         } else {
                             i2p_transient_session.swap(m_unused_i2p_sessions.front());
                             m_unused_i2p_sessions.pop();
@@ -489,8 +484,8 @@ CNode* CConnman::ConnectNode(CAddress addrConnect,
                     addr_bind = conn.me;
                 }
             } else if (use_proxy) {
-                LogDebug(BCLog::PROXY, "Using proxy: %s to connect to %s\n", proxy.ToString(), target_addr.ToStringAddrPort());
-                sock = ConnectThroughProxy(proxy, target_addr.ToStringAddr(), target_addr.GetPort(), proxyConnectionFailed);
+                LogDebug(BCLog::PROXY, "Using proxy: %s to connect to %s\n", use_proxy->ToString(), target_addr.ToStringAddrPort());
+                sock = ConnectThroughProxy(*use_proxy, target_addr.ToStringAddr(), target_addr.GetPort(), proxyConnectionFailed);
             } else {
                 // no proxy needed (none set for target network)
                 sock = ConnectDirectly(target_addr, conn_type == ConnectionType::MANUAL);
@@ -500,12 +495,14 @@ CNode* CConnman::ConnectNode(CAddress addrConnect,
                 // the proxy, mark this as an attempt.
                 addrman.get().Attempt(target_addr, fCountFailure);
             }
-        } else if (pszDest && GetNameProxy(proxy)) {
-            std::string host;
-            uint16_t port{default_port};
-            SplitHostPort(std::string(pszDest), port, host);
-            bool proxyConnectionFailed;
-            sock = ConnectThroughProxy(proxy, host, port, proxyConnectionFailed);
+        } else if (pszDest) {
+            if (const auto name_proxy = GetNameProxy()) {
+                std::string host;
+                uint16_t port{default_port};
+                SplitHostPort(pszDest, port, host);
+                bool proxyConnectionFailed;
+                sock = ConnectThroughProxy(*name_proxy, host, port, proxyConnectionFailed);
+            }
         }
         // Check any other resolved address (if any) if we fail to connect
         if (!sock) {
@@ -3120,9 +3117,10 @@ void CConnman::PrivateBroadcast::NumToOpenWait() const
 
 std::optional<Proxy> CConnman::PrivateBroadcast::ProxyForIPv4or6() const
 {
-    Proxy tor_proxy;
-    if (m_outbound_tor_ok_at_least_once.load() && GetProxy(NET_ONION, tor_proxy)) {
-        return tor_proxy;
+    if (m_outbound_tor_ok_at_least_once.load()) {
+        if (const auto tor_proxy = GetProxy(NET_ONION)) {
+            return tor_proxy;
+        }
     }
     return std::nullopt;
 }
@@ -3479,10 +3477,11 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
         return false;
     }
 
-    Proxy i2p_sam;
-    if (GetProxy(NET_I2P, i2p_sam) && connOptions.m_i2p_accept_incoming) {
-        m_i2p_sam_session = std::make_unique<i2p::sam::Session>(gArgs.GetDataDirNet() / "i2p_private_key",
-                                                                i2p_sam, m_interrupt_net);
+    if (connOptions.m_i2p_accept_incoming) {
+        if (const auto i2p_sam = GetProxy(NET_I2P)) {
+            m_i2p_sam_session = std::make_unique<i2p::sam::Session>(gArgs.GetDataDirNet() / "i2p_private_key",
+                                                                    *i2p_sam, m_interrupt_net);
+        }
     }
 
     // Randomize the order in which we may query seednode to potentially prevent connecting to the same one every restart (and signal that we have restarted)

src/netbase.cpp

@@ -706,13 +706,14 @@ bool SetProxy(enum Network net, const Proxy &addrProxy) {
     return true;
 }
 
-bool GetProxy(enum Network net, Proxy &proxyInfoOut) {
+std::optional<Proxy> GetProxy(enum Network net)
+{
     assert(net >= 0 && net < NET_MAX);
     LOCK(g_proxyinfo_mutex);
-    if (!proxyInfo[net].IsValid())
-        return false;
-    proxyInfoOut = proxyInfo[net];
-    return true;
+    if (!proxyInfo[net].IsValid()) {
+        return std::nullopt;
+    }
+    return proxyInfo[net];
 }
 
 bool SetNameProxy(const Proxy &addrProxy) {
@@ -723,12 +724,13 @@ bool SetNameProxy(const Proxy &addrProxy) {
     return true;
 }
 
-bool GetNameProxy(Proxy &nameProxyOut) {
+std::optional<Proxy> GetNameProxy()
+{
     LOCK(g_proxyinfo_mutex);
-    if(!nameProxy.IsValid())
-        return false;
-    nameProxyOut = nameProxy;
-    return true;
+    if (!nameProxy.IsValid()) {
+        return std::nullopt;
+    }
+    return nameProxy;
 }
 
 bool HaveNameProxy() {

src/netbase.h

@@ -14,6 +14,7 @@
 #include <cstdint>
 #include <functional>
 #include <memory>
+#include <optional>
 #include <string>
 #include <type_traits>
 #include <unordered_set>
@@ -179,7 +180,7 @@ std::string GetNetworkName(enum Network net);
 /** Return a vector of publicly routable Network names; optionally append NET_UNROUTABLE. */
 std::vector<std::string> GetNetworkNames(bool append_unroutable = false);
 bool SetProxy(enum Network net, const Proxy &addrProxy);
-bool GetProxy(enum Network net, Proxy &proxyInfoOut);
+std::optional<Proxy> GetProxy(enum Network net);
 bool IsProxy(const CNetAddr &addr);
 /**
  * Set the name proxy to use for all connections to nodes specified by a
@@ -199,7 +200,7 @@ bool IsProxy(const CNetAddr &addr);
  */
 bool SetNameProxy(const Proxy &addrProxy);
 bool HaveNameProxy();
-bool GetNameProxy(Proxy &nameProxyOut);
+std::optional<Proxy> GetNameProxy();
 
 using DNSLookupFn = std::function<std::vector<CNetAddr>(const std::string&, bool)>;
 extern DNSLookupFn g_dns_lookup;

src/node/interfaces.cpp

@@ -187,7 +187,7 @@ class NodeImpl : public Node
         args().WriteSettingsFile();
     }
     void mapPort(bool enable) override { StartMapPort(enable); }
-    bool getProxy(Network net, Proxy& proxy_info) override { return GetProxy(net, proxy_info); }
+    std::optional<Proxy> getProxy(Network net) override { return GetProxy(net); }
     size_t getNodeCount(ConnectionDirection flags) override
     {
         return m_context->connman ? m_context->connman->GetNodeCount(flags) : 0;

src/qt/clientmodel.cpp

@@ -287,10 +287,11 @@ void ClientModel::unsubscribeFromCoreSignals()
 
 bool ClientModel::getProxyInfo(std::string& ip_port) const
 {
-    Proxy ipv4, ipv6;
-    if (m_node.getProxy((Network) 1, ipv4) && m_node.getProxy((Network) 2, ipv6)) {
-      ip_port = ipv4.proxy.ToStringAddrPort();
-      return true;
+    const auto ipv4 = m_node.getProxy(NET_IPV4);
+    const auto ipv6 = m_node.getProxy(NET_IPV6);
+    if (ipv4 && ipv6) {
+        ip_port = ipv4->proxy.ToStringAddrPort();
+        return true;
     }
     return false;
 }

src/qt/optionsdialog.cpp

@@ -456,17 +456,14 @@ void OptionsDialog::updateDefaultProxyNets()
         proxyIpText = ui_proxy.ToStringAddrPort();
     }
 
-    Proxy proxy;
-    bool has_proxy;
+    const auto proxy_ipv4 = model->node().getProxy(NET_IPV4);
+    ui->proxyReachIPv4->setChecked(proxy_ipv4 && proxy_ipv4->ToString() == proxyIpText);
 
-    has_proxy = model->node().getProxy(NET_IPV4, proxy);
-    ui->proxyReachIPv4->setChecked(has_proxy && proxy.ToString() == proxyIpText);
+    const auto proxy_ipv6 = model->node().getProxy(NET_IPV6);
+    ui->proxyReachIPv6->setChecked(proxy_ipv6 && proxy_ipv6->ToString() == proxyIpText);
 
-    has_proxy = model->node().getProxy(NET_IPV6, proxy);
-    ui->proxyReachIPv6->setChecked(has_proxy && proxy.ToString() == proxyIpText);
-
-    has_proxy = model->node().getProxy(NET_ONION, proxy);
-    ui->proxyReachTor->setChecked(has_proxy && proxy.ToString() == proxyIpText);
+    const auto proxy_onion = model->node().getProxy(NET_ONION);
+    ui->proxyReachTor->setChecked(proxy_onion && proxy_onion->ToString() == proxyIpText);
 }
 
 ProxyAddressValidator::ProxyAddressValidator(QObject *parent) :

src/rpc/net.cpp

@@ -613,14 +613,17 @@ static UniValue GetNetworksInfo()
     for (int n = 0; n < NET_MAX; ++n) {
         enum Network network = static_cast<enum Network>(n);
         if (network == NET_UNROUTABLE || network == NET_INTERNAL) continue;
-        Proxy proxy;
         UniValue obj(UniValue::VOBJ);
-        GetProxy(network, proxy);
         obj.pushKV("name", GetNetworkName(network));
         obj.pushKV("limited", !g_reachable_nets.Contains(network));
         obj.pushKV("reachable", g_reachable_nets.Contains(network));
-        obj.pushKV("proxy", proxy.IsValid() ? proxy.ToString() : std::string());
-        obj.pushKV("proxy_randomize_credentials", proxy.m_tor_stream_isolation);
+        if (const auto proxy = GetProxy(network)) {
+            obj.pushKV("proxy", proxy->ToString());
+            obj.pushKV("proxy_randomize_credentials", proxy->m_tor_stream_isolation);
+        } else {
+            obj.pushKV("proxy", std::string());
+            obj.pushKV("proxy_randomize_credentials", false);
+        }
         networks.push_back(std::move(obj));
     }
     return networks;