Merge bitcoin#34267: net: avoid unconditional privatebroadcast logging (+ warn for debug logs)

b39291f doc: fix `-logips` description to clarify that non-debug logs can also contain IP addresses (Lőrinc)
c7028d3 init: log that additional logs may contain privacy-sensitive information (Lőrinc)
31b771a net: move `privatebroadcast` logs to debug category (Lőrinc)

Pull request description:

  ### Motivation
  The recently merged [private broadcast](bitcoin#29415) is a privacy feature, and users may share `debug.log` with support.
  Unconditional `LogInfo()` messages that mention private broadcast and/or include (w)txids can leak sensitive context (e.g. which transactions a user originated).
  Since it's meant to be a private broadcast, we should minimize leaks.
  It's a best effort, it's not invalidated by other logs possibly leaking identifiable information, those can be addressed separately.
  We're not promising that the logs won't ever contain data that could be used against the user, but we should still try to minimize that data, especially for a feature that's advertised as privacy-focused.

  Follow up to [bitcoin#29415 (comment)](bitcoin#29415 (comment))

  ### Changes
  * Move private-broadcast event logs from `LogInfo()` to `LogDebug(BCLog::PRIVBROADCAST, ...)`, so they are only emitted when `-debug=privatebroadcast` was explicitly provided.
  * Remove hardcoded `"[privatebroadcast]"` log-string prefixes (category logging already adds the prefix).
  * Keep warning at the default log level for startup failures.
  * Add an init log (not a warning since that would require excessive test framework updates) when any `-debug` categories are enabled that additional logs may contain privacy-sensitive information and should not be shared publicly.
  * Update a related startup arg (`-logips`) to clarify that clarify that non-debug logs can also contain IP addresses.

  ### Reproducer
  The new warning can be checked with:
  ```bash
  ./build/bin/bitcoind -printtoconsole=1 -stopatheight=1 -listen=0 -connect=0 | grep 'Debug logging is enabled' | wc -l
         0
  ./build/bin/bitcoind -printtoconsole=1 -stopatheight=1 -listen=0 -connect=0 -debug | grep 'Debug logging is enabled' | wc  -l
         1
  ```

ACKs for top commit:
  janb84:
    re ACK b39291f
  vasild:
    ACK b39291f
  andrewtoth:
    ACK b39291f
  frankomosh:
    crACK b39291f .The approach and implementation look good. Moving private broadcast logs to debug only would effectively reduce privacy leaks for users sharing logs.
  sedited:
    ACK b39291f

Tree-SHA512: feca25ebe72a03948ba436e25f9a682947966c4c09627e8f20201ef3872ddbce1c636cd82f06be1afdc09cb80da305058667c0c2eaeadeb351311155325ea06f

Author: sedited

src/init/common.cpp

@@ -31,7 +31,7 @@ void AddLoggingArgs(ArgsManager& argsman)
         "If <category> is not supplied or if <category> is 1 or \"all\", output all debug logging. If <category> is 0 or \"none\", any other categories are ignored. Other valid values for <category> are: " + LogInstance().LogCategoriesString() + ". This option can be specified multiple times to output multiple categories.",
         ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-debugexclude=<category>", "Exclude debug and trace logging for a category. Can be used in conjunction with -debug=1 to output debug and trace logging for all categories except the specified category. This option can be specified multiple times to exclude multiple categories. This takes priority over \"-debug\"", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
-    argsman.AddArg("-logips", strprintf("Include IP addresses in debug output (default: %u)", DEFAULT_LOGIPS), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
+    argsman.AddArg("-logips", strprintf("Include IP addresses in log output (default: %u)", DEFAULT_LOGIPS), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-loglevel=<level>|<category>:<level>", strprintf("Set the global or per-category severity level for logging categories enabled with the -debug configuration option or the logging RPC. Possible values are %s (default=%s). The following levels are always logged: error, warning, info. If <category>:<level> is supplied, the setting will override the global one and may be specified multiple times to set multiple category-specific levels. <category> can be: %s.", LogInstance().LogLevelsString(), LogInstance().LogLevelToStr(BCLog::DEFAULT_LOG_LEVEL), LogInstance().LogCategoriesString()), ArgsManager::DISALLOW_NEGATION | ArgsManager::DISALLOW_ELISION | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-logtimestamps", strprintf("Prepend debug output with timestamp (default: %u)", DEFAULT_LOGTIMESTAMPS), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-logthreadnames", strprintf("Prepend debug output with name of the originating thread (default: %u)", DEFAULT_LOGTHREADNAMES), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
@@ -98,6 +98,11 @@ util::Result<void> SetLoggingCategories(const ArgsManager& args)
             return util::Error{strprintf(_("Unsupported logging category %s=%s."), "-debugexclude", cat)};
         }
     }
+
+    if (LogInstance().GetCategoryMask() != BCLog::NONE) {
+        LogInfo("Debug logging is enabled (-debug). Additional log output may contain privacy-sensitive information. Be cautious when sharing logs.");
+    }
+
     return {};
 }
 

src/net.cpp

@@ -3225,7 +3225,7 @@ void CConnman::ThreadPrivateBroadcast()
         std::optional<Proxy> proxy;
         const std::optional<Network> net{m_private_broadcast.PickNetwork(proxy)};
         if (!net.has_value()) {
-            LogWarning("[privatebroadcast] Connections needed but none of the Tor or I2P networks is reachable");
+            LogWarning("Unable to open -privatebroadcast connections: neither Tor nor I2P is reachable");
             m_interrupt_net->sleep_for(5s);
             continue;
         }

src/net_processing.cpp

@@ -1653,9 +1653,9 @@ void PeerManagerImpl::ReattemptPrivateBroadcast(CScheduler& scheduler)
                          stale_tx->GetHash().ToString(), stale_tx->GetWitnessHash().ToString());
                 ++num_for_rebroadcast;
             } else {
-                LogInfo("[privatebroadcast] Giving up broadcast attempts for txid=%s wtxid=%s: %s",
-                        stale_tx->GetHash().ToString(), stale_tx->GetWitnessHash().ToString(),
-                        mempool_acceptable.m_state.ToString());
+                LogDebug(BCLog::PRIVBROADCAST, "Giving up broadcast attempts for txid=%s wtxid=%s: %s",
+                         stale_tx->GetHash().ToString(), stale_tx->GetWitnessHash().ToString(),
+                         mempool_acceptable.m_state.ToString());
                 m_tx_for_private_broadcast.Remove(stale_tx);
             }
         }
@@ -3536,9 +3536,9 @@ void PeerManagerImpl::PushPrivateBroadcastTx(CNode& node)
     }
     const CTransactionRef& tx{*opt_tx};
 
-    LogInfo("[privatebroadcast] P2P handshake completed, sending INV for txid=%s%s, peer=%d%s",
-            tx->GetHash().ToString(), tx->HasWitness() ? strprintf(", wtxid=%s", tx->GetWitnessHash().ToString()) : "",
-            node.GetId(), node.LogIP(fLogIPs));
+    LogDebug(BCLog::PRIVBROADCAST, "P2P handshake completed, sending INV for txid=%s%s, peer=%d%s",
+             tx->GetHash().ToString(), tx->HasWitness() ? strprintf(", wtxid=%s", tx->GetWitnessHash().ToString()) : "",
+             node.GetId(), node.LogIP(fLogIPs));
 
     MakeAndPushMessage(node, NetMsgType::INV, std::vector<CInv>{{CInv{MSG_TX, tx->GetHash().ToUint256()}}});
 }
@@ -3677,8 +3677,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
             if (fRelay) {
                 MakeAndPushMessage(pfrom, NetMsgType::VERACK);
             } else {
-                LogInfo("[privatebroadcast] Disconnecting: does not support transactions relay (connected in vain), peer=%d%s",
-                        pfrom.GetId(), pfrom.LogIP(fLogIPs));
+                LogDebug(BCLog::PRIVBROADCAST, "Disconnecting: does not support transaction relay (connected in vain), peer=%d%s",
+                         pfrom.GetId(), pfrom.LogIP(fLogIPs));
                 pfrom.fDisconnect = true;
             }
             return;
@@ -4203,8 +4203,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         if (pfrom.IsPrivateBroadcastConn()) {
             const auto pushed_tx_opt{m_tx_for_private_broadcast.GetTxForNode(pfrom.GetId())};
             if (!pushed_tx_opt) {
-                LogInfo("[privatebroadcast] Disconnecting: got GETDATA without sending an INV, peer=%d%s",
-                        pfrom.GetId(), fLogIPs ? strprintf(", peeraddr=%s", pfrom.addr.ToStringAddrPort()) : "");
+                LogDebug(BCLog::PRIVBROADCAST, "Disconnecting: got GETDATA without sending an INV, peer=%d%s",
+                         pfrom.GetId(), fLogIPs ? strprintf(", peeraddr=%s", pfrom.addr.ToStringAddrPort()) : "");
                 pfrom.fDisconnect = true;
                 return;
             }
@@ -4220,8 +4220,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
                 peer->m_ping_queued = true; // Ensure a ping will be sent: mimic a request via RPC.
                 MaybeSendPing(pfrom, *peer, GetTime<std::chrono::microseconds>());
             } else {
-                LogInfo("[privatebroadcast] Disconnecting: got an unexpected GETDATA message, peer=%d%s",
-                        pfrom.GetId(), fLogIPs ? strprintf(", peeraddr=%s", pfrom.addr.ToStringAddrPort()) : "");
+                LogDebug(BCLog::PRIVBROADCAST, "Disconnecting: got an unexpected GETDATA message, peer=%d%s",
+                         pfrom.GetId(), fLogIPs ? strprintf(", peeraddr=%s", pfrom.addr.ToStringAddrPort()) : "");
                 pfrom.fDisconnect = true;
             }
             return;
@@ -4465,9 +4465,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         AddKnownTx(*peer, hash);
 
         if (const auto num_broadcasted{m_tx_for_private_broadcast.Remove(ptx)}) {
-            LogInfo("[privatebroadcast] Received our privately broadcast transaction (txid=%s) from the "
-                    "network from peer=%d%s; stopping private broadcast attempts",
-                    txid.ToString(), pfrom.GetId(), pfrom.LogIP(fLogIPs));
+            LogDebug(BCLog::PRIVBROADCAST, "Received our privately broadcast transaction (txid=%s) from the "
+                                           "network from peer=%d%s; stopping private broadcast attempts",
+                     txid.ToString(), pfrom.GetId(), pfrom.LogIP(fLogIPs));
             if (NUM_PRIVATE_BROADCAST_PER_TX > num_broadcasted.value()) {
                 // Not all of the initial NUM_PRIVATE_BROADCAST_PER_TX connections were needed.
                 // Tell CConnman it does not need to start the remaining ones.
@@ -4981,8 +4981,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
                         pfrom.PongReceived(ping_time);
                         if (pfrom.IsPrivateBroadcastConn()) {
                             m_tx_for_private_broadcast.NodeConfirmedReception(pfrom.GetId());
-                            LogInfo("[privatebroadcast] Got a PONG (the transaction will probably reach the network), marking for disconnect, peer=%d%s",
-                                    pfrom.GetId(), pfrom.LogIP(fLogIPs));
+                            LogDebug(BCLog::PRIVBROADCAST, "Got a PONG (the transaction will probably reach the network), marking for disconnect, peer=%d%s",
+                                     pfrom.GetId(), pfrom.LogIP(fLogIPs));
                             pfrom.fDisconnect = true;
                         }
                     } else {
@@ -5712,8 +5712,8 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
     // not sent. This here is just an optimization.
     if (pto->IsPrivateBroadcastConn()) {
         if (pto->m_connected + PRIVATE_BROADCAST_MAX_CONNECTION_LIFETIME < current_time) {
-            LogInfo("[privatebroadcast] Disconnecting: did not complete the transaction send within %d seconds, peer=%d%s",
-                    count_seconds(PRIVATE_BROADCAST_MAX_CONNECTION_LIFETIME), pto->GetId(), pto->LogIP(fLogIPs));
+            LogDebug(BCLog::PRIVBROADCAST, "Disconnecting: did not complete the transaction send within %d seconds, peer=%d%s",
+                     count_seconds(PRIVATE_BROADCAST_MAX_CONNECTION_LIFETIME), pto->GetId(), pto->LogIP(fLogIPs));
             pto->fDisconnect = true;
         }
         return true;