Merge #7348: fix: penalize oversized notfound messages

a97b7b4 fix: align oversized notfound penalty (PastaClaw)
a43a27f test: avoid hardcoded oversized notfound count (PastaClaw)
f84962b fix: penalize oversized notfound messages (PastaClaw)

Pull request description:

  # fix: penalize oversized notfound messages

  ## Issue being fixed or feature implemented

  Oversized `notfound` inventory vectors were ignored after deserialization. This
  change gives them a small misbehavior score, matching the defensive treatment
  used for other inventory-vector messages, and avoids holding `cs_main` while
  deserializing the vector.

  ## What was done?

  - Deserialize `notfound` inventory vectors before taking `cs_main`.
  - Return early with a small misbehavior score when a `notfound` vector exceeds
    the maximum outstanding object/block request count.
  - Add functional coverage for the oversized `notfound` path.

  ## How Has This Been Tested?

  Environment: macOS 15.6 arm64, local Dash Core autotools build from this branch.

  - `git diff --check`
  - Local build configured with:

    ```bash
    ./configure --without-gui --disable-bench --disable-fuzz-binary \
      --disable-tests --disable-wallet
    ```

  - `make -j8 src/dashd src/dash-cli`
    - Initial parallel build hit a generated dependency-file race while building
      `dash-cli`; `dashd` linked successfully.
  - `make -j1 src/dash-cli`
  - `test/functional/p2p_tx_download.py --cachedir=/tmp/dash_func_cache_notfound`

  ## Breaking Changes

  None.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone
    *(for repository code-owners and collaborators only)*

ACKs for top commit:
  UdjinM6:
    utACK a97b7b4

Tree-SHA512: 35ff6b0b9e12e377e5600f4b0b2caee9582d0b4022dad04727fab54599807a5659c1837e43d30205386be34ce4dda33d4d9f687101ce67e8a694fb1a614d3914

Author: PastaPastaPasta

src/net_processing.cpp

@@ -5451,24 +5451,27 @@ void PeerManagerImpl::ProcessMessage(
     }
     if (msg_type == NetMsgType::NOTFOUND) {
         // Remove the NOTFOUND transactions from the peer
-        LOCK(cs_main);
-        CNodeState *state = State(pfrom.GetId());
         std::vector<CInv> vInv;
         vRecv >> vInv;
-        if (vInv.size() <= MAX_PEER_OBJECT_IN_FLIGHT + MAX_BLOCKS_IN_TRANSIT_PER_PEER) {
-            for (CInv &inv : vInv) {
-                if (inv.IsKnownType()) {
-                    // If we receive a NOTFOUND message for a txid we requested, erase
-                    // it from our data structures for this peer.
-                    auto in_flight_it = state->m_object_download.m_object_in_flight.find(inv);
-                    if (in_flight_it == state->m_object_download.m_object_in_flight.end()) {
-                        // Skip any further work if this is a spurious NOTFOUND
-                        // message.
-                        continue;
-                    }
-                    state->m_object_download.m_object_in_flight.erase(in_flight_it);
-                    state->m_object_download.m_object_announced.erase(inv);
+        if (vInv.size() > MAX_PEER_OBJECT_IN_FLIGHT + MAX_BLOCKS_IN_TRANSIT_PER_PEER) {
+            Misbehaving(pfrom.GetId(), 20, strprintf("notfound message size = %u", vInv.size()));
+            return;
+        }
+
+        LOCK(cs_main);
+        CNodeState *state = State(pfrom.GetId());
+        for (CInv &inv : vInv) {
+            if (inv.IsKnownType()) {
+                // If we receive a NOTFOUND message for a txid we requested, erase
+                // it from our data structures for this peer.
+                auto in_flight_it = state->m_object_download.m_object_in_flight.find(inv);
+                if (in_flight_it == state->m_object_download.m_object_in_flight.end()) {
+                    // Skip any further work if this is a spurious NOTFOUND
+                    // message.
+                    continue;
                 }
+                state->m_object_download.m_object_in_flight.erase(in_flight_it);
+                state->m_object_download.m_object_announced.erase(inv);
             }
         }
         return;

test/functional/p2p_tx_download.py

@@ -40,6 +40,8 @@ def on_getdata(self, message):
 MAX_GETDATA_RANDOM_DELAY = 2  # seconds
 INBOUND_PEER_TX_DELAY = 2  # seconds
 MAX_GETDATA_IN_FLIGHT = 100
+MAX_BLOCKS_IN_TRANSIT_PER_PEER = 16
+MAX_NOTFOUND_SIZE = MAX_GETDATA_IN_FLIGHT + MAX_BLOCKS_IN_TRANSIT_PER_PEER
 TX_EXPIRY_INTERVAL = GETDATA_TX_INTERVAL * 10
 
 # Python test constants
@@ -142,13 +144,21 @@ def test_spurious_notfound(self):
         self.log.info('Check that spurious notfound is ignored')
         self.nodes[0].p2ps[0].send_message(msg_notfound(vec=[CInv(1, 1)]))
 
+    def test_oversized_notfound(self):
+        self.log.info('Check that oversized notfound increases misbehavior score')
+        oversized_notfound_count = MAX_NOTFOUND_SIZE + 1
+        invs = [CInv(t=1, h=i) for i in range(oversized_notfound_count)]
+        with self.nodes[0].assert_debug_log(["Misbehaving", f"notfound message size = {oversized_notfound_count}"]):
+            self.nodes[0].p2ps[0].send_message(msg_notfound(vec=invs))
+            self.nodes[0].p2ps[0].sync_with_ping()
+
     def run_test(self):
         self.wallet = MiniWallet(self.nodes[0])
         self.wallet.rescan_utxos()
 
         # Run each test against new bitcoind instances, as setting mocktimes has long-term effects on when
         # the next trickle relay event happens.
-        for test in [self.test_spurious_notfound, self.test_in_flight_max, self.test_inv_block, self.test_tx_requests]:
+        for test in [self.test_spurious_notfound, self.test_oversized_notfound, self.test_in_flight_max, self.test_inv_block, self.test_tx_requests]:
             self.stop_nodes()
             self.start_nodes()
             self.connect_nodes(1, 0)