Merge eb0724f0de into a50af6e4c4

src/banman.h

@@ -54,6 +54,11 @@ class CSubNet;
 // transaction that fails a policy check and a future version changes the
 // policy check so the transaction is accepted, then that transaction could
 // cause the network to split between old nodes and new nodes.
+//
+// NOTE: previously a misbehaving peer would get banned instead of discouraged.
+// This meant a peer could unboundedly grow our in-memory map of banned ips. When
+// receiving an ADDR message we would also compare every address received to every
+// item in the map. See https://bitcoincore.org/en/2024/07/03/disclose-unbounded-banlist.
 
 class BanMan
 {

src/net.cpp

@@ -761,6 +761,8 @@ int V1Transport::readHeader(Span<const uint8_t> msg_bytes)
     }
 
     // reject messages larger than MAX_SIZE or MAX_PROTOCOL_MESSAGE_LENGTH
+    // NOTE: failing to perform this check previously allowed a malicious peer to make us allocate 32MiB of memory per
+    // connection. See https://bitcoincore.org/en/2024/07/03/disclose_receive_buffer_oom.
     if (hdr.nMessageSize > MAX_SIZE || hdr.nMessageSize > MAX_PROTOCOL_MESSAGE_LENGTH) {
         LogDebug(BCLog::NET, "Header error: Size too large (%s, %u bytes), peer=%d\n", SanitizeString(hdr.GetMessageType()), hdr.nMessageSize, m_node_id);
         return -1;

src/net_processing.cpp

@@ -2426,6 +2426,9 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic
         }
         // else: If the first item on the queue is an unknown type, we erase it
         // and continue processing the queue on the next call.
+        // NOTE: previously we wouldn't do so and the peer sending us a malformed GETDATA could
+        // result in never making progress and this thread using 100% allocated CPU. See
+        // https://bitcoincore.org/en/2024/07/03/disclose-getdata-cpu.
     }
 
     peer.m_getdata_requests.erase(peer.m_getdata_requests.begin(), it);
@@ -3069,6 +3072,8 @@ void PeerManagerImpl::ProcessPackageResult(const node::PackageToValidate& packag
     }
 }
 
+// NOTE: the orphan processing used to be uninterruptible and quadratic, which could allow a peer to stall the node for
+// hours with specially crafted transactions. See https://bitcoincore.org/en/2024/07/03/disclose-orphan-dos.
 bool PeerManagerImpl::ProcessOrphanTx(Peer& peer)
 {
     AssertLockHeld(g_msgproc_mutex);

src/txrequest.h

@@ -92,6 +92,10 @@
  *   peers with a nonzero number of tracked announcements.
  * - CPU usage is generally logarithmic in the total number of tracked announcements, plus the number of
  *   announcements affected by an operation (amortized O(1) per announcement).
+ *
+ * Context:
+ * - In an earlier version of the transaction request logic it was possible for a peer to prevent us from seeing a
+ *   specific transaction. See https://bitcoincore.org/en/2024/07/03/disclose_already_asked_for.
  */
 class TxRequestTracker {
     // Avoid littering this header file with implementation details.

src/validation.cpp

@@ -4202,6 +4202,10 @@ arith_uint256 CalculateClaimedHeadersWork(std::span<const CBlockHeader> headers)
  *  enforced in this function (eg by adding a new consensus rule). See comment
  *  in ConnectBlock().
  *  Note that -reindex-chainstate skips the validation that happens here!
+ *
+ *  NOTE: failing to check the header's height against the last checkpoint's opened a DoS vector between
+ *  v0.12 and v0.15 (when no additional protection was in place) whereby an attacker could unboundedly
+ *  grow our in-memory block index. See https://bitcoincore.org/en/2024/07/03/disclose-header-spam.
  */
 static bool ContextualCheckBlockHeader(const CBlockHeader& block, BlockValidationState& state, BlockManager& blockman, const ChainstateManager& chainman, const CBlockIndex* pindexPrev) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
 {