Merge bitcoin/bitcoin#30538: Doc: add a comment referencing past vulnerability next to where it was fixed

eb0724f doc: banman: reference past vuln due to unbounded banlist (Antoine Poinsot)
ad616b6 doc: net: mention past vulnerability as rationale to limit incoming message size (Antoine Poinsot)
4489117 doc: txrequest: point to past censorship vulnerability in tx re-request handling (Antoine Poinsot)
68ac954 doc: net_proc: reference past DoS vulnerability in orphan processing (Antoine Poinsot)
c02d9f6 doc: net_proc: reference past defect regarding invalid GETDATA types (Antoine Poinsot)
5e3d9f2 doc: validation: add a reference to historical header spam vulnerability (Antoine Poinsot)

Pull request description:

  It is useful when reading code to have context about why it is written or behaves the way it does. Some instances in this PR may seem obvious but i think nonetheless offer important context to anyone willing to change (or review a change to) this code.

ACKs for top commit:
  ryanofsky:
    Code review ACK eb0724f. No changes since last review other than rebase

Tree-SHA512: 271902f45b8130d44153d793bc1096cd22b6ce05494e67c665a5bc45754e3fc72573d303ec8fc7db4098d473760282ddbf0c1cf316947539501dfd8d7d5b8828

Author: ryanofsky

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(std::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

@@ -2425,6 +2425,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);
@@ -3068,6 +3071,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)
 {