From 2f77e65e86a589a67d0376102f9ce3caf6468667 Mon Sep 17 00:00:00 2001
From: Bart <11445373+bthomee@users.noreply.github.com>
Date: Mon, 27 Jul 2026 21:27:25 -0400
Subject: [PATCH] Improve invalid node handling and fee charging

---
 include/xrpl/shamap/SHAMap.h                  |  7 +++--
 src/xrpld/app/ledger/detail/InboundLedger.cpp | 30 +++++++++----------
 src/xrpld/overlay/detail/PeerImp.cpp          | 14 ++++++---
 3 files changed, 29 insertions(+), 22 deletions(-)

diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h
index fe2ddddbe6..e198c472fa 100644
--- a/include/xrpl/shamap/SHAMap.h
+++ b/include/xrpl/shamap/SHAMap.h
@@ -96,14 +96,17 @@ enum class SHAMapState {
  */
 
 /**
- * Holds a SHAMap node's identity, serialized data, and leaf status. Used by
+ * Holds a SHAMap node's identity, leaf status, and serialized data. Used by
  * getNodeFat to return node data for peer synchronization.
  */
 struct SHAMapNodeData
 {
     SHAMapNodeID nodeID;
+    // The `data` field (a Blob, 8-byte aligned) needs 4 bytes of padding after the `nodeID` field
+    // (36 bytes, 4-byte aligned) regardless of what comes between them, so `isLeaf` costs nothing
+    // extra here. Moving it after `data` would add 8 bytes to the size of this struct instead.
     bool isLeaf;
-    Blob data;  // Placed last, so `isLeaf` can fit into the alignment padding of `nodeID`.
+    Blob data;
 };
 
 class SHAMap
diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp
index 006a01f668..b3dafcf5e6 100644
--- a/src/xrpld/app/ledger/detail/InboundLedger.cpp
+++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp
@@ -894,16 +894,12 @@ InboundLedger::receiveNode(
                 return;
             }
 
-            if (nodeID->isRoot())
-            {
-                san += map.addRootNode(rootHash, std::move(treeNode), f);
-            }
-            else
-            {
-                san += map.addKnownNode(*nodeID, std::move(treeNode), f);
-            }
+            auto const result = nodeID->isRoot()
+                ? map.addRootNode(rootHash, std::move(treeNode), f)
+                : map.addKnownNode(*nodeID, std::move(treeNode), f);
+            san += result;
 
-            if (!san.isGood())
+            if (result.isInvalid())
             {
                 JLOG(journal_.warn()) << "Got invalid node " << *nodeID << " for ledger " << hash_
                                       << " from peer " << peer->id();
@@ -970,9 +966,10 @@ InboundLedger::takeAsRootNode(std::string_view data, SHAMapAddNode& san)
     }
 
     AccountStateSF filter(ledger_->stateMap().family().db(), app_.getLedgerMaster());
-    san += ledger_->stateMap().addRootNode(
+    auto const result = ledger_->stateMap().addRootNode(
         SHAMapHash{ledger_->header().accountHash}, std::move(treeNode), &filter);
-    return san.isGood();
+    san += result;
+    return !result.isInvalid();
 }
 
 /**
@@ -1005,9 +1002,10 @@ InboundLedger::takeTxRootNode(std::string_view data, SHAMapAddNode& san)
     }
 
     TransactionStateSF filter(ledger_->txMap().family().db(), app_.getLedgerMaster());
-    san += ledger_->txMap().addRootNode(
+    auto const result = ledger_->txMap().addRootNode(
         SHAMapHash{ledger_->header().txHash}, std::move(treeNode), &filter);
-    return san.isGood();
+    san += result;
+    return !result.isInvalid();
 }
 
 std::vector<InboundLedger::neededHash_t>
@@ -1162,11 +1160,11 @@ InboundLedger::processData(std::shared_ptr<Peer> peer, protocol::TMLedgerData co
                                << ((packet.type() == protocol::liTX_NODE) ? "TX" : "AS")
                                << " node stats: " << san.get();
 
+        // `san` accumulates across the whole packet, so `isInvalid()` (bad_ > 0) does not mean the
+        // packet had no useful nodes: credit whatever good/useful nodes were sent rather than
+        // discarding everything because one node in an otherwise-good packet was bad.
         // Note: Peer charges for invalid/malformed data are issued from within receiveNode at the
         // exact failure site, so the peer is only charged for problems they are responsible for.
-        if (san.isInvalid())
-            return -1;
-
         if (san.isUseful())
             progress_ = true;
 
diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp
index d31998c80c..d9bc4d554c 100644
--- a/src/xrpld/overlay/detail/PeerImp.cpp
+++ b/src/xrpld/overlay/detail/PeerImp.cpp
@@ -1538,14 +1538,20 @@ PeerImp::onMessage(std::shared_ptr<protocol::TMGetLedger> const& m)
             }
         }
 
-        // Charge once per request. Requesting too many node IDs is charged even for a relay
-        // response, since it's a distinct infraction from the "get ledger request" charge below,
-        // which is skipped for relay responses.
+        // These are two distinct infractions and are charged independently: requesting too many
+        // node IDs is charged even for a relay response, while the base "get ledger request" charge
+        // below is skipped for relay responses.
         if (tooManyNodeIds)
         {
             peer->charge(Resource::kFeeModerateBurdenPeer, "TMGetLedger: too many node IDs");
+
+            // Truncate the request to what was actually parsed and charged for, so that if this
+            // request ends up being relayed to another peer, we don't forward the oversized list.
+            m->mutable_nodeids()->DeleteSubrange(
+                static_cast<int>(nodeIDs.size()),
+                m->nodeids_size() - static_cast<int>(nodeIDs.size()));
         }
-        else if (!m->has_requestcookie())
+        if (!m->has_requestcookie())
         {
             peer->charge(Resource::kFeeModerateBurdenPeer, "TMGetLedger: get ledger request");
         }