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

1. 
   maflcko commented at 12:34 pm on March 5, 2026: member

   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.

2. DrahtBot renamed this:
   refactor: Return std::optional from GetNameProxy/GetProxy
   refactor: Return std::optional from GetNameProxy/GetProxy
   on Mar 5, 2026
3. DrahtBot added the label Refactoring on Mar 5, 2026
4. 
   DrahtBot commented at 12:34 pm on March 5, 2026: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	sedited, ViniciusCestarii, frankomosh, achow101

   If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #34520 (refactor: Add [[nodiscard]] to functions returning bool+mutable ref by maflcko)
   • #34486 (net: Reduce local network activity when networkactive=0 by willcl-ark)
   • #32065 (i2p: make a time gap between creating transient sessions and using them by vasild)
   • #30951 (net: option to disallow v1 connection on ipv4 and ipv6 peers by stratospher)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

5. DrahtBot added the label CI failed on Mar 5, 2026
6. DrahtBot removed the label CI failed on Mar 5, 2026
7. 
   luke-jr commented at 6:52 am on March 6, 2026: member

   Wouldn’t util::Result be a better fit? But since IsValid is already there, why introduce a second way to indicate none?
8. 
   maflcko commented at 3:18 pm on March 10, 2026: member

   Wouldn’t util::Result be a better fit?

   Why? Result is primarily intended for high-level errors that should be passed to the user, possibly even translated. I think this is low-level enough to not need translated error strings, or error messages at all. Also, adding error strings won’t be possible in a refactor-only pull anyway.

   But since IsValid is already there, why introduce a second way to indicate none?

   It is what other new code is using. You can use git grep 'std::optional<Proxy>' bitcoin-core/master to find those places. Also, std::optional is easier to read and is enforced at compile-time, whereas for Proxy, checking IsValid is not enforced.

9. refactor: Return std::optional from GetNameProxy faeac1a931
10. refactor: Return std::optional from GetProxy fa270fdacf
11. in src/qt/clientmodel.cpp:291 in fa66d1ad49

    286@@ -287,10 +287,11 @@ void ClientModel::unsubscribeFromCoreSignals()
    287 
    288 bool ClientModel::getProxyInfo(std::string& ip_port) const
    289 {
    290-    Proxy ipv4, ipv6;
    291-    if (m_node.getProxy((Network) 1, ipv4) && m_node.getProxy((Network) 2, ipv6)) {
    292-      ip_port = ipv4.proxy.ToStringAddrPort();
    293-      return true;
    294+    const auto ipv4 = m_node.getProxy((Network)1);
    295+    const auto ipv6 = m_node.getProxy((Network)2);
    

    ---

    ---

    

    ViniciusCestarii commented at 6:48 pm on March 10, 2026:

    nit: since this section is already being refactored, it might be a good opportunity to replace the magic numbers with Network::NET_IPV4 / Network::NET_IPV6 for clarity.

    0    const auto ipv4 = m_node.getProxy(Network::NET_IPV4);
    1    const auto ipv6 = m_node.getProxy(Network::NET_IPV6);
    

    ---

    

    maflcko commented at 8:22 pm on March 10, 2026:

    thx, done
12. maflcko force-pushed on Mar 10, 2026
13. maflcko requested review from ViniciusCestarii on Mar 13, 2026
14. 
    ViniciusCestarii commented at 4:22 pm on March 13, 2026: contributor

    ACK fa270fd
15. in src/net.cpp:502 in faeac1a931

    503-            sock = ConnectThroughProxy(proxy, host, port, proxyConnectionFailed);
    504+        } else if (pszDest) {
    505+            if (const auto name_proxy = GetNameProxy()) {
    506+                std::string host;
    507+                uint16_t port{default_port};
    508+                SplitHostPort(std::string(pszDest), port, host);
    

    ---

    ---

    

    sedited commented at 9:48 pm on March 23, 2026:

    Since you are touching this, my clang-tidy 23 seems to think the extra std::string is redundant:

    0/home/drgrid/bitcoin/src/net.cpp:502:31: error: redundant conversion to 'std::string' (aka 'basic_string<char>') and then back to '__sv_type' (aka 'basic_string_view<char, std::char_traits<char>>') [performance-string-view-conversions,-warnings-as-errors]
    1  502 |                 SplitHostPort(std::string(pszDest), port, host);
    2      |                               ^~~~~~~~~~~~~~~~~~~~
    3      |                               pszDest
    

    ---

    

    maflcko commented at 6:08 am on March 24, 2026:

    Probably not performance critical here, but I pushed the unrelated style nit
16. sedited approved
17. 
    sedited commented at 9:51 pm on March 23, 2026: contributor

    ACK fa270fdacf5b06bfeecf37ae3fe95f96a0e406cd
18. refactor: Fix redundant conversion to std::string and then to std::string_view [performance-string-view-conversions] fa73ed467c
19. sedited approved
20. 
    sedited commented at 7:54 am on March 24, 2026: contributor

    ACK fa73ed467cba598dac7468cffab82605745b0265
21. DrahtBot requested review from ViniciusCestarii on Mar 24, 2026
22. 
    ViniciusCestarii commented at 1:01 pm on March 24, 2026: contributor

    ACK fa73ed467cba598dac7468cffab82605745b0265
23. 
    frankomosh commented at 12:27 pm on March 25, 2026: contributor

    Code Review ACK fa73ed467cba598dac7468cffab82605745b0265. Good refactor, correctly replaces the bool + mutable reference output parameter pattern with std::optional<Proxy> across GetProxy and GetNameProxy. Semantics are preserved.
24. 
    achow101 commented at 6:30 pm on March 26, 2026: member

    ACK fa73ed467cba598dac7468cffab82605745b0265
25. achow101 merged this on Mar 26, 2026
26. achow101 closed this on Mar 26, 2026

    ---

27. maflcko deleted the branch on Mar 26, 2026

Contributors
maflcko DrahtBot luke-jr ViniciusCestarii sedited frankomosh achow101

Labels
Refactoring