net: Don't log own ips during discover #34458

sedited commented at 9:59 AM on January 30, 2026: contributor

The -logips option seems to imply that ip addresses are only logged when it is set. This seems like an obvious case for not logging these by default. There might be prior discussion around this, but I was unable to find why these might be exempt. There are also a few issues (both open and closed) where printing these lines was useful.

DrahtBot added the label P2P on Jan 30, 2026

DrahtBot commented at 10:00 AM on January 30, 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	willcl-ark, l0rinc, danielabrozzoni, achow101

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34812 (net: advertise CJDNS addresses when -externalip disables discovery by w0xlt)

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.

in src/net.cpp:291 in ca21fc8c28 outdated

 286 | @@ -287,7 +287,9 @@ bool AddLocal(const CService& addr_, int nScore)
 287 |      if (!g_reachable_nets.Contains(addr))
 288 |          return false;
 289 |  
 290 | -    LogInfo("AddLocal(%s,%i)\n", addr.ToStringAddrPort(), nScore);
 291 | +    if (fLogIPs) {
 292 | +        LogInfo("AddLocal(%s,%i)\n", addr.ToStringAddrPort(), nScore);

l0rinc commented at 10:17 AM on January 30, 2026:

nit: while we're here, let's remove the trailing newlines nit2: we might as well format this similarly to the actual method call nit3: we can safely use %s for both nit4: we could still show nScore when fLogIPs is disabled

        LogInfo("AddLocal(%s, %s)", fLogIPs ? addr.ToStringAddrPort() : "[redacted]", nScore);

We should likely do the same with RemoveLocal:

void RemoveLocal(const CService& addr)
{
    LOCK(g_maplocalhost_mutex);
    LogInfo("RemoveLocal(%s)", fLogIPs ? addr.ToStringAddrPort() : "[redacted]");
    mapLocalHost.erase(addr);
}

sedited commented at 12:40 PM on January 30, 2026:

I am not even sure why we log these in the first place. Could alternatively just relegate them to debug? Just redacting the IPs seems to make the log line useless.

in src/net.cpp:3351 in ca21fc8c28 outdated

3346 | @@ -3345,8 +3347,9 @@ void Discover()
3347 |          return;
3348 |  
3349 |      for (const CNetAddr &addr: GetLocalAddresses()) {
3350 | -        if (AddLocal(addr, LOCAL_IF))
3351 | +        if (AddLocal(addr, LOCAL_IF) && fLogIPs) {
3352 |              LogInfo("%s: %s\n", __func__, addr.ToStringAddr());

l0rinc commented at 10:53 AM on January 30, 2026:

this will also avoid logging the whole line now - which is probably fine

willcl-ark commented at 10:58 AM on February 3, 2026:

I think here we might log twice if fLogIPs is true? Once in AddLocal and once in the following line 3351? (not tested)

sedited commented at 6:40 AM on February 25, 2026:

Yes, that is the current behaviour too. I could remove the line in Discover, but it might also make sense to keep it, since we do use AddLocal in another context.

l0rinc changes_requested

l0rinc commented at 11:17 AM on January 30, 2026: contributor

Concept ACK on taking PII logging more seriously. We can't fully fix it, but I agree we should be able to reduce it.

The PR title should probably generalize to sanitizing LogInfo/LogWarning address logging - or show "Discover" at the end to note that it's a method name.

Can we cover any of these cases as well (I have skipped all debug logs, we can do those separately)?

These were my local changes corresponding to the above:

<details><summary>Patch</summary>

diff --git a/src/httprpc.cpp b/src/httprpc.cpp
index 56a243085c..6e73469ada 100644
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -120,7 +120,7 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
     jreq.context = context;
     jreq.peerAddr = req->GetPeer().ToStringAddrPort();
     if (!RPCAuthorized(authHeader.second, jreq.authUser)) {
-        LogWarning("ThreadRPCServer incorrect password attempt from %s", jreq.peerAddr);
+        LogWarning("ThreadRPCServer incorrect password attempt%s", fLogIPs ? strprintf(" from %s", jreq.peerAddr) : "");
 
         /* Deter brute-forcing
            If this results in a DoS the user really
diff --git a/src/i2p.cpp b/src/i2p.cpp
index 04093ea100..b99f6e6349 100644
--- a/src/i2p.cpp
+++ b/src/i2p.cpp
@@ -452,10 +452,10 @@ void Session::CreateIfNotCreatedAlready()
     m_session_id = session_id;
     m_control_sock = std::move(sock);
 
-    LogInfo("%s I2P SAM session %s created, my address=%s",
+    LogInfo("%s I2P SAM session %s created%s",
         Capitalize(session_type),
         m_session_id,
-        m_my_addr.ToStringAddrPort());
+        fLogIPs ? strprintf(", my address=%s", m_my_addr.ToStringAddrPort()) : "");
 }
 
 std::unique_ptr<Sock> Session::StreamAccept()
diff --git a/src/net.cpp b/src/net.cpp
index 16591461ef..635d4dff90 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -287,7 +287,7 @@ bool AddLocal(const CService& addr_, int nScore)
     if (!g_reachable_nets.Contains(addr))
         return false;
 
-    LogInfo("AddLocal(%s,%i)\n", addr.ToStringAddrPort(), nScore);
+    LogInfo("AddLocal(%s, %s)", fLogIPs ? addr.ToStringAddrPort() : "[redacted]", nScore);
 
     {
         LOCK(g_maplocalhost_mutex);
@@ -310,7 +310,7 @@ bool AddLocal(const CNetAddr &addr, int nScore)
 void RemoveLocal(const CService& addr)
 {
     LOCK(g_maplocalhost_mutex);
-    LogInfo("RemoveLocal(%s)\n", addr.ToStringAddrPort());
+    LogInfo("RemoveLocal(%s)", fLogIPs ? addr.ToStringAddrPort() : "[redacted]");
     mapLocalHost.erase(addr);
 }
 
@@ -385,7 +385,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect,
 
         // Look for an existing connection
         if (AlreadyConnectedToAddressPort(addrConnect)) {
-            LogInfo("Failed to open new connection to %s, already connected", addrConnect.ToStringAddrPort());
+            LogInfo("Failed to open new connection to %s, already connected", fLogIPs ? addrConnect.ToStringAddrPort() : "[redacted]");
             return nullptr;
         }
     }
@@ -416,7 +416,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect,
                 // It is possible that we already have a connection to the IP/port pszDest resolved to.
                 // In that case, drop the connection that was just created.
                 if (AlreadyConnectedToAddressPort(addrConnect)) {
-                    LogInfo("Not opening a connection to %s, already connected to %s\n", pszDest, addrConnect.ToStringAddrPort());
+                    LogInfo("Not opening a connection to %s, already connected to %s", pszDest, fLogIPs ? addrConnect.ToStringAddrPort() : "[redacted]");
                     return nullptr;
                 }
                 // Add the address to the resolved addresses vector so we can try to connect to it later on
@@ -1783,7 +1783,7 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
     }
 
     if (!sock->IsSelectable()) {
-        LogInfo("connection from %s dropped: non-selectable socket\n", addr.ToStringAddrPort());
+        LogInfo("connection from %s dropped: non-selectable socket", fLogIPs ? addr.ToStringAddrPort() : "[redacted]");
         return;
     }
 
@@ -3325,7 +3325,7 @@ bool CConnman::BindListenPort(const CService& addrBind, bilingual_str& strError,
         LogError("%s\n", strError.original);
         return false;
     }
-    LogInfo("Bound to %s\n", addrBind.ToStringAddrPort());
+    LogInfo("Bound to %s", fLogIPs ? addrBind.ToStringAddrPort() : "[redacted]");
 
     // Listen for incoming connections
     if (sock->Listen(SOMAXCONN) == SOCKET_ERROR)
@@ -3345,8 +3345,8 @@ void Discover()
         return;
 
     for (const CNetAddr &addr: GetLocalAddresses()) {
-        if (AddLocal(addr, LOCAL_IF))
-            LogInfo("%s: %s\n", __func__, addr.ToStringAddr());
+        if (AddLocal(addr, LOCAL_IF) && fLogIPs)
+            LogInfo("%s: %s", __func__, addr.ToStringAddr());
     }
 }
 
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 16b4735e5e..bd8ba085e6 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -3620,7 +3620,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         // Disconnect if we connected to ourself
         if (pfrom.IsInboundConn() && !m_connman.CheckIncomingNonce(nNonce))
         {
-            LogInfo("connected to self at %s, disconnecting\n", pfrom.addr.ToStringAddrPort());
+            LogInfo("connected to self%s, disconnecting\n", fLogIPs ? strprintf(" at %s", pfrom.addr.ToStringAddrPort()) : "");
             pfrom.fDisconnect = true;
             return;
         }
diff --git a/src/netbase.cpp b/src/netbase.cpp
index c1c03c5725..365e0a2cf9 100644
--- a/src/netbase.cpp
+++ b/src/netbase.cpp
@@ -644,9 +644,10 @@ static bool ConnectToSocket(const Sock& sock, struct sockaddr* sockaddr, socklen
 
 std::unique_ptr<Sock> ConnectDirectly(const CService& dest, bool manual_connection)
 {
+    const std::string dest_str{fLogIPs ? dest.ToStringAddrPort() : "[redacted]"};
     auto sock = CreateSock(dest.GetSAFamily(), SOCK_STREAM, IPPROTO_TCP);
     if (!sock) {
-        LogError("Cannot create a socket for connecting to %s\n", dest.ToStringAddrPort());
+        LogError("Cannot create a socket for connecting%s", fLogIPs ? strprintf(" to %s", dest.ToStringAddrPort()) : "");
         return {};
     }
 
@@ -654,11 +655,11 @@ std::unique_ptr<Sock> ConnectDirectly(const CService& dest, bool manual_connecti
     struct sockaddr_storage sockaddr;
     socklen_t len = sizeof(sockaddr);
     if (!dest.GetSockAddr((struct sockaddr*)&sockaddr, &len)) {
-        LogInfo("Cannot get sockaddr for %s: unsupported network\n", dest.ToStringAddrPort());
+        LogInfo("Cannot get sockaddr%s: unsupported network", fLogIPs ? strprintf(" for %s", dest.ToStringAddrPort()) : "");
         return {};
     }
 
-    if (!ConnectToSocket(*sock, (struct sockaddr*)&sockaddr, len, dest.ToStringAddrPort(), manual_connection)) {
+    if (!ConnectToSocket(*sock, (struct sockaddr*)&sockaddr, len, dest_str, manual_connection)) {
         return {};
     }
 
diff --git a/test/functional/feature_anchors.py b/test/functional/feature_anchors.py
index e82f28f4cc..1a2f0aa89c 100755
--- a/test/functional/feature_anchors.py
+++ b/test/functional/feature_anchors.py
@@ -139,7 +139,7 @@ class AnchorsTest(BitcoinTestFramework):
 
         self.log.info("Restarting node attempts to reconnect to anchors")
         with self.nodes[0].assert_debug_log([f"Trying to make an anchor connection to {ONION_ADDR}"]):
-            self.start_node(0, extra_args=[f"-onion={onion_conf.addr[0]}:{onion_conf.addr[1]}"])
+            self.start_node(0, extra_args=[f"-onion={onion_conf.addr[0]}:{onion_conf.addr[1]}", "-logips"])
 
 
 if __name__ == "__main__":
diff --git a/test/functional/p2p_addr_selfannouncement.py b/test/functional/p2p_addr_selfannouncement.py
index 8ab75aaa06..88e73a2a15 100755
--- a/test/functional/p2p_addr_selfannouncement.py
+++ b/test/functional/p2p_addr_selfannouncement.py
@@ -65,7 +65,7 @@ class SelfAnnouncementReceiver(P2PInterface):
 class AddrSelfAnnouncementTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 1
-        self.extra_args = [[f"-externalip={IP_TO_ANNOUNCE}"]]
+        self.extra_args = [[f"-externalip={IP_TO_ANNOUNCE}", "-logips"]]
 
     def run_test(self):
         # populate addrman to have some addresses for a GETADDR response

</details>

sedited commented at 12:45 PM on January 30, 2026: contributor

Thanks for taking a look @l0rinc! I'm not sure if I want to litigate all of those yet. The example here just seems particularly egregious, because it leaks PII. Maybe we can figure out what to do with these here and then use that to inform us on the rest?

willcl-ark commented at 10:59 AM on February 3, 2026: member

Concept ACK

I get not wanting to litigate all instances in this PR, so happy to ack this as-is as it's a clear improvement on logging at startup all our PII, which is often the most-shared part of a debug log.

I would add my own suggestion of including RemoveLocal for inclusion in this change, mostly because you touch "opposing"" function AddLocal:

https://github.com/bitcoin/bitcoin/blob/be35408c5ad72885af1d0bab3f01dcee2435f74b/src/net.cpp#L310-L315

And this this protects against logging PII when Tor/I2P backends go up or down as part of normal operation.

chriszeng1010 commented at 6:18 PM on February 24, 2026: none

RemoveLocal() at line 313 also logs addr.ToStringAddrPort() unconditionally.

void RemoveLocal(const CService& addr)
{
    LOCK(g_maplocalhost_mutex);
    LogInfo("RemoveLocal(%s)\n", addr.ToStringAddrPort());
    mapLocalHost.erase(addr);
}

Fixed here https://github.com/bitcoin/bitcoin/pull/34665

net: Don't log own ips during discover

The -logips option seems to imply that ip addresses are only logged when
it is set. This seems like an obvious case for not logging these by
default.

a49f97ff4a

sedited force-pushed on Feb 25, 2026

sedited commented at 7:32 AM on February 25, 2026: contributor

Updated ca21fc8c2884932327af306d1dd89b14527d7076 -> a49f97ff4a3f20bb4e73e625de1522d21752fe25 (logips_self_discover_0 -> logips_self_discover_1, compare)

Addressed @willcl-ark's comment, only logging the RemoveLocal line conditionally.

I was hoping this would get quick review, since this logs users' own clearnet IPs (which is probably the clearest example of PII that bitcoind produces), and I found many examples of users pasting these logs. At this point I might just go for a more comprehensive take on it. I skipped RemoveLocal before, since there are a few other places where we leak our own privacy overlay networks identifier too. Should those also really be covered by something called -logips? Similarly, we log the dns names, which might include non-seednode nodes. Should those be covered as well? @chriszeng1010 your comment, was made by both l0rinc and willcl-ark too. There is no need to open a PR for a simple review comment while the PR it is targeted at is still being worked on.

willcl-ark approved

willcl-ark commented at 8:26 AM on February 25, 2026: member

tACK a49f97ff4a3f20bb4e73e625de1522d21752fe25

# Bitcoin Core 30.2
will in 🌐 nix-desktop in ~
❯ /run/current-system/sw/bin/bitcoind -daemon=0 -port=9333 -rpcport=9332 | rg 'AddLocal|RemoveLocal'
2026-02-25T08:22:38Z AddLocal(xxx.xxx.xxx.xxx:9333,4)
2026-02-25T08:22:42Z AddLocal(<onion_addr>.onion:8333,4)
^C⏎

# This PR
will in 🌐 nix-desktop in ~
❯ /home/will/src/core/worktrees/pr-34458/build/bin/bitcoind -daemon=0 -port=9333 -rpcport=9332 | rg 'AddLocal|RemoveLocal'
# no logs
^C⏎

will in 🌐 nix-desktop in ~ took 15s
❯ /home/will/src/core/worktrees/pr-34458/build/bin/bitcoind -daemon=0 -port=9333 -rpcport=9332 -logips | rg 'AddLocal|RemoveLocal'
2026-02-25T08:23:56Z AddLocal(xxx.xxx.xxx.xxx:9333,4)
2026-02-25T08:24:00Z AddLocal(<onion_addr>.onion:8333,4)

DrahtBot requested review from l0rinc on Feb 25, 2026

l0rinc commented at 9:03 AM on February 25, 2026: contributor

tested ACK a49f97ff4a3f20bb4e73e625de1522d21752fe25

I'm mostly OK with the change. I'm fine with merging as-is, but would also gladly reACK if you decide to add a test for the new behavior:

<details><summary>LocalAddress_BasicLifecycle test addition</summary>

diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
--- a/src/test/net_tests.cpp	(revision 6e93028e4a346871535b14aa4cbb5a008f7d1a73)
+++ b/src/test/net_tests.cpp	(date 1772010060874)
@@ -6,6 +6,7 @@
 #include <clientversion.h>
 #include <common/args.h>
 #include <compat/compat.h>
+#include <logging.h>
 #include <net.h>
 #include <net_processing.h>
 #include <netaddress.h>
@@ -792,12 +793,22 @@
 
     g_reachable_nets.Add(NET_IPV4);
 
-    BOOST_CHECK(!IsLocal(addr));
-    BOOST_CHECK(AddLocal(addr, 1000));
-    BOOST_CHECK(IsLocal(addr));
+    for (const bool should_log_ips : {!fLogIPs, fLogIPs}) {
+        fLogIPs = should_log_ips;
+
+        bool ip_logged{false};
+        auto cb{LogInstance().PushBackCallback([&](const std::string& s) { ip_logged |= s.find(addr.ToStringAddrPort()) != std::string::npos; })};
+
+        BOOST_CHECK(!IsLocal(addr));
+        BOOST_CHECK(AddLocal(addr, 1000));
+        BOOST_CHECK(IsLocal(addr));
 
-    RemoveLocal(addr);
-    BOOST_CHECK(!IsLocal(addr));
+        RemoveLocal(addr);
+        BOOST_CHECK(!IsLocal(addr));
+        BOOST_CHECK_EQUAL(ip_logged, should_log_ips);
+
+        LogInstance().DeleteCallback(cb);
+    }
 }
 
 BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)

</details>

l0rinc approved

sedited requested review from vasild on Mar 19, 2026

danielabrozzoni approved

danielabrozzoni commented at 6:00 PM on March 25, 2026: member

tACK a49f97ff4a3f20bb4e73e625de1522d21752fe25

I tested it the same way Will did:

on master, without -logips, I checked that my address is printed in AddLocal()
on this PR, I checked that my address is printed only when -logips is passed.

There are other instances where my address is printed, but they can be fixed in subsequent PRs.

achow101 commented at 6:20 PM on March 26, 2026: member

ACK a49f97ff4a3f20bb4e73e625de1522d21752fe25

achow101 merged this on Mar 26, 2026

achow101 closed this on Mar 26, 2026