Improve invalid node handling and fee charging

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

include/xrpl/basics/Slice.h

@@ -11,6 +11,7 @@
 #include <limits>
 #include <stdexcept>
 #include <string>
+#include <string_view>
 #include <type_traits>
 #include <vector>
 
@@ -251,4 +252,11 @@ makeSlice(std::basic_string<char, Traits, Alloc> const& s)
     return Slice(s.data(), s.size());
 }
 
+template <class Traits>
+Slice
+makeSlice(std::basic_string_view<char, Traits> s)
+{
+    return Slice(s.data(), s.size());
+}
+
 }  // namespace xrpl

include/xrpl/beast/core/SemanticVersion.h

@@ -17,14 +17,14 @@ namespace beast {
 class SemanticVersion
 {
 public:
-    using identifier_list = std::vector<std::string>;
+    using IdentifierList = std::vector<std::string>;
 
     int majorVersion;
     int minorVersion;
     int patchVersion;
 
-    identifier_list preReleaseIdentifiers;
-    identifier_list metaData;
+    IdentifierList preReleaseIdentifiers;
+    IdentifierList metaData;
 
     SemanticVersion();
 

include/xrpl/proto/xrpl.proto

@@ -246,7 +246,15 @@ message TMGetObjectByHash {
 
 message TMLedgerNode {
   required bytes nodedata = 1;
-  optional bytes nodeid = 2;  // missing for ledger base data
+
+  // Used when protocol version <2.3. Not set for ledger base data.
+  optional bytes nodeid = 2;
+
+  // Used when protocol version >=2.3. Neither value is set for ledger base data.
+  oneof reference {
+    bytes id = 3;      // Set for inner nodes.
+    uint32 depth = 4;  // Set for leaf nodes.
+  }
 }
 
 enum TMLedgerInfoType {

include/xrpl/shamap/SHAMap.h

@@ -3,7 +3,6 @@
 #include <xrpl/basics/Blob.h>
 #include <xrpl/basics/IntrusivePointer.h>
 #include <xrpl/basics/SHAMapHash.h>
-#include <xrpl/basics/Slice.h>
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/beast/utility/Journal.h>
 #include <xrpl/beast/utility/instrumentation.h>
@@ -95,6 +94,21 @@ enum class SHAMapState {
  *
  * See https://en.wikipedia.org/wiki/Merkle_tree
  */
+
+/**
+ * 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;
+};
+
 class SHAMap
 {
 private:
@@ -289,10 +303,10 @@ public:
     std::vector<std::pair<SHAMapNodeID, uint256>>
     getMissingNodes(int maxNodes, SHAMapSyncFilter const* filter);
 
-    bool
+    [[nodiscard]] bool
     getNodeFat(
         SHAMapNodeID const& wanted,
-        std::vector<std::pair<SHAMapNodeID, Blob>>& data,
+        std::vector<SHAMapNodeData>& data,
         bool fatLeaves,
         std::uint32_t depth) const;
 
@@ -321,10 +335,45 @@ public:
     void
     serializeRoot(Serializer& s) const;
 
+    /**
+     * Add a root node to the SHAMap during synchronization.
+     *
+     * This function is used when receiving the root node of a SHAMap from a peer during ledger
+     * synchronization. The node must already have been deserialized.
+     *
+     * @param hash The expected hash of the root node.
+     * @param rootNode A deserialized root node to add.
+     * @param filter Optional sync filter to track received nodes.
+     * @return Status indicating whether the node was useful, duplicate, or invalid.
+     *
+     * @note This function expects the rootNode to be a valid, deserialized SHAMapTreeNode. The
+     *       caller is responsible for deserialization and basic validation before calling this
+     *       function.
+     */
     SHAMapAddNode
-    addRootNode(SHAMapHash const& hash, Slice const& rootNode, SHAMapSyncFilter const* filter);
+    addRootNode(SHAMapHash const& hash, SHAMapTreeNodePtr rootNode, SHAMapSyncFilter const* filter);
+
+    /**
+     * Add a known node at a specific position in the SHAMap during synchronization.
+     *
+     * This function is used when receiving nodes from peers during ledger synchronization. The node
+     * is inserted at the position specified by nodeID. The node must already have been
+     * deserialized.
+     *
+     * @param nodeID The position in the tree where this node belongs.
+     * @param treeNode A deserialized tree node to add.
+     * @param filter Optional sync filter to track received nodes.
+     * @return Status indicating whether the node was useful, duplicate, or invalid.
+     *
+     * @note This function expects the treeNode to be a valid, deserialized SHAMapTreeNode. The
+     *       caller is responsible for deserialization and basic validation before calling this
+     *       function. This also means that the nodeID must be consistent with the node's content.
+     */
     SHAMapAddNode
-    addKnownNode(SHAMapNodeID const& nodeID, Slice const& rawNode, SHAMapSyncFilter const* filter);
+    addKnownNode(
+        SHAMapNodeID const& nodeID,
+        SHAMapTreeNodePtr treeNode,
+        SHAMapSyncFilter const* filter);
 
     // status functions
     void

include/xrpl/shamap/SHAMapLeafNode.h

@@ -1,6 +1,9 @@
 #pragma once
 
 #include <xrpl/basics/SHAMapHash.h>
+#include <xrpl/basics/base_uint.h>
+#include <xrpl/basics/safe_cast.h>
+#include <xrpl/beast/utility/instrumentation.h>
 #include <xrpl/shamap/SHAMapItem.h>
 #include <xrpl/shamap/SHAMapNodeID.h>
 #include <xrpl/shamap/SHAMapTreeNode.h>
@@ -60,4 +63,16 @@ public:
     getString(SHAMapNodeID const&) const final;
 };
 
+/**
+ * Return the key of the item held by a SHAMap leaf node.
+ *
+ * @param node a node known to be a leaf (see SHAMapTreeNode::isLeaf).
+ */
+inline uint256 const&
+leafKey(SHAMapTreeNode const& node)
+{
+    XRPL_ASSERT(node.isLeaf(), "xrpl::leafKey : node is a leaf");
+    return safeDowncast<SHAMapLeafNode const&>(node).peekItem()->key();
+}
+
 }  // namespace xrpl

src/libxrpl/beast/core/SemanticVersion.cpp

@@ -15,7 +15,7 @@
 namespace beast {
 
 std::string
-printIdentifiers(SemanticVersion::identifier_list const& list)
+printIdentifiers(SemanticVersion::IdentifierList const& list)
 {
     std::string ret;
 
@@ -115,7 +115,7 @@ extractIdentifier(std::string& value, bool allowLeadingZeroes, std::string& inpu
 
 bool
 extractIdentifiers(
-    SemanticVersion::identifier_list& identifiers,
+    SemanticVersion::IdentifierList& identifiers,
     bool allowLeadingZeroes,
     std::string& input)
 {

src/libxrpl/shamap/SHAMapNodeID.cpp

@@ -129,7 +129,8 @@ selectBranch(SHAMapNodeID const& id, uint256 const& hash)
 SHAMapNodeID
 SHAMapNodeID::createID(int depth, uint256 const& key)
 {
-    XRPL_ASSERT((depth >= 0) && (depth < 65), "xrpl::SHAMapNodeID::createID : valid branch input");
+    XRPL_ASSERT(
+        depth >= 0 && depth <= SHAMap::kLeafDepth, "xrpl::SHAMapNodeID::createID : valid depth");
     return SHAMapNodeID(depth, key & depthMask(depth));
 }
 

src/libxrpl/shamap/SHAMapSync.cpp

@@ -157,10 +157,7 @@ SHAMap::visitDifferences(
                     if ((have == nullptr) || !have->hasInnerNode(childID, childHash))
                         stack.emplace(safeDowncast<SHAMapInnerNode*>(next), childID);
                 }
-                else if (
-                    (have == nullptr) ||
-                    !have->hasLeafNode(
-                        safeDowncast<SHAMapLeafNode*>(next)->peekItem()->key(), childHash))
+                else if ((have == nullptr) || !have->hasLeafNode(leafKey(*next), childHash))
                 {
                     if (!function(*next))
                         return;
@@ -414,7 +411,7 @@ SHAMap::getMissingNodes(int max, SHAMapSyncFilter const* filter)
 bool
 SHAMap::getNodeFat(
     SHAMapNodeID const& wanted,
-    std::vector<std::pair<SHAMapNodeID, Blob>>& data,
+    std::vector<SHAMapNodeData>& data,
     bool fatLeaves,
     std::uint32_t depth) const
 {
@@ -460,7 +457,7 @@ SHAMap::getNodeFat(
         // Add this node to the reply
         s.erase();
         node->serializeForWire(s);
-        data.emplace_back(nodeID, s.getData());
+        data.emplace_back(nodeID, node->isLeaf(), s.getData());
 
         if (node->isInner())
         {
@@ -490,7 +487,7 @@ SHAMap::getNodeFat(
                             // Just include this node
                             s.erase();
                             childNode->serializeForWire(s);
-                            data.emplace_back(childID, s.getData());
+                            data.emplace_back(childID, childNode->isLeaf(), s.getData());
                         }
                     }
                 }
@@ -508,25 +505,33 @@ SHAMap::serializeRoot(Serializer& s) const
 }
 
 SHAMapAddNode
-SHAMap::addRootNode(SHAMapHash const& hash, Slice const& rootNode, SHAMapSyncFilter const* filter)
+SHAMap::addRootNode(
+    SHAMapHash const& hash,
+    SHAMapTreeNodePtr rootNode,
+    SHAMapSyncFilter const* filter)
 {
+    XRPL_ASSERT(cowid_ >= 1, "xrpl::SHAMap::addRootNode : valid cowid");
+    XRPL_ASSERT(rootNode, "xrpl::SHAMap::addRootNode : non-null root node");
+
     // we already have a root_ node
     if (root_->getHash().isNonZero())
     {
-        JLOG(journal_.trace()) << "got root node, already have one";
-        XRPL_ASSERT(root_->getHash() == hash, "xrpl::SHAMap::addRootNode : valid hash input");
+        JLOG(journal_.trace()) << "Got root node, already have one";
+        XRPL_ASSERT(root_->getHash() == hash, "xrpl::SHAMap::addRootNode : valid hash");
         return SHAMapAddNode::duplicate();
     }
 
-    XRPL_ASSERT(cowid_ >= 1, "xrpl::SHAMap::addRootNode : valid cowid");
-    auto node = SHAMapTreeNode::makeFromWire(rootNode);
-    if (!node || node->getHash() != hash)
+    if (rootNode->getHash() != hash)
+    {
+        JLOG(journal_.warn()) << "Corrupt root node received: expected hash " << hash << ", got "
+                              << rootNode->getHash();
         return SHAMapAddNode::invalid();
+    }
 
     if (backed_)
-        canonicalize(hash, node);
+        canonicalize(hash, rootNode);
 
-    root_ = node;
+    root_ = std::move(rootNode);
 
     if (root_->isLeaf())
         clearSynching();
@@ -543,9 +548,18 @@ SHAMap::addRootNode(SHAMapHash const& hash, Slice const& rootNode, SHAMapSyncFil
 }
 
 SHAMapAddNode
-SHAMap::addKnownNode(SHAMapNodeID const& node, Slice const& rawNode, SHAMapSyncFilter const* filter)
+SHAMap::addKnownNode(
+    SHAMapNodeID const& nodeID,
+    SHAMapTreeNodePtr treeNode,
+    SHAMapSyncFilter const* filter)
 {
-    XRPL_ASSERT(!node.isRoot(), "xrpl::SHAMap::addKnownNode : valid node input");
+    XRPL_ASSERT(!nodeID.isRoot(), "xrpl::SHAMap::addKnownNode : valid node");
+    XRPL_ASSERT(treeNode, "xrpl::SHAMap::addKnownNode : non-null tree node");
+    XRPL_ASSERT(
+        !treeNode->isLeaf() ||
+            SHAMapNodeID::createID(nodeID.getDepth(), leafKey(*treeNode)).getNodeID() ==
+                nodeID.getNodeID(),
+        "xrpl::SHAMap::addKnownNode : leaf position consistent with node ID");
 
     if (!isSynching())
     {
@@ -559,14 +573,15 @@ SHAMap::addKnownNode(SHAMapNodeID const& node, Slice const& rawNode, SHAMapSyncF
 
     while (currNode->isInner() &&
            !safeDowncast<SHAMapInnerNode*>(currNode)->isFullBelow(generation) &&
-           (currNodeID.getDepth() < node.getDepth()))
+           (currNodeID.getDepth() < nodeID.getDepth()))
     {
-        int const branch = selectBranch(currNodeID, node.getNodeID());
+        int const branch = selectBranch(currNodeID, nodeID.getNodeID());
         XRPL_ASSERT(branch >= 0, "xrpl::SHAMap::addKnownNode : valid branch");
         auto inner = safeDowncast<SHAMapInnerNode*>(currNode);
         if (inner->isEmptyBranch(branch))
         {
-            JLOG(journal_.warn()) << "Add known node for empty branch" << node;
+            JLOG(journal_.warn()) << "Add known node " << nodeID << " for empty branch " << branch
+                                  << " at " << currNodeID;
             return SHAMapAddNode::invalid();
         }
 
@@ -582,67 +597,45 @@ SHAMap::addKnownNode(SHAMapNodeID const& node, Slice const& rawNode, SHAMapSyncF
         if (currNode != nullptr)
             continue;
 
-        auto newNode = SHAMapTreeNode::makeFromWire(rawNode);
-
-        if (!newNode || childHash != newNode->getHash())
+        if (childHash != treeNode->getHash())
         {
-            JLOG(journal_.warn()) << "Corrupt node received";
+            JLOG(journal_.warn()) << "Corrupt node " << nodeID << " received: expected hash "
+                                  << childHash << ", got " << treeNode->getHash();
             return SHAMapAddNode::invalid();
         }
 
-        // In rare cases, a node can still be corrupt even after hash
-        // validation. For leaf nodes, we perform an additional check to
-        // ensure the node's position in the tree is consistent with its
-        // content to prevent inconsistencies that could
-        // propagate further down the line.
-        if (newNode->isLeaf())
-        {
-            auto const& actualKey =
-                safeDowncast<SHAMapLeafNode const*>(newNode.get())->peekItem()->key();
-
-            // Validate that this leaf belongs at the target position
-            auto const expectedNodeID = SHAMapNodeID::createID(node.getDepth(), actualKey);
-            if (expectedNodeID.getNodeID() != node.getNodeID())
-            {
-                JLOG(journal_.debug())
-                    << "Leaf node position mismatch: "
-                    << "expected=" << expectedNodeID.getNodeID() << ", actual=" << node.getNodeID();
-                return SHAMapAddNode::invalid();
-            }
-        }
-
         // Inner nodes must be at a level strictly less than 64
         // but leaf nodes (while notionally at level 64) can be
         // at any depth up to and including 64:
         if ((currNodeID.getDepth() > kLeafDepth) ||
-            (newNode->isInner() && currNodeID.getDepth() == kLeafDepth))
+            (treeNode->isInner() && currNodeID.getDepth() == kLeafDepth))
         {
             // Map is provably invalid
             state_ = SHAMapState::Invalid;
             return SHAMapAddNode::useful();
         }
 
-        if (currNodeID != node)
+        if (currNodeID != nodeID)
         {
             // Either this node is broken or we didn't request it (yet)
-            JLOG(journal_.warn()) << "unable to hook node " << node;
+            JLOG(journal_.warn()) << "unable to hook node " << nodeID;
             JLOG(journal_.info()) << " stuck at " << currNodeID;
-            JLOG(journal_.info()) << "got depth=" << node.getDepth()
+            JLOG(journal_.info()) << "got depth=" << nodeID.getDepth()
                                   << ", walked to= " << currNodeID.getDepth();
             return SHAMapAddNode::useful();
         }
 
         if (backed_)
-            canonicalize(childHash, newNode);
+            canonicalize(childHash, treeNode);
 
-        newNode = prevNode->canonicalizeChild(branch, std::move(newNode));
+        treeNode = prevNode->canonicalizeChild(branch, std::move(treeNode));
 
         if (filter != nullptr)
         {
             Serializer s;
-            newNode->serializeWithPrefix(s);
+            treeNode->serializeWithPrefix(s);
             filter->gotNode(
-                false, childHash, ledgerSeq_, std::move(s.modData()), newNode->getType());
+                false, childHash, ledgerSeq_, std::move(s.modData()), treeNode->getType());
         }
 
         return SHAMapAddNode::useful();

src/test/app/LedgerNodeHelpers_test.cpp

@@ -0,0 +1,260 @@
+#include <xrpld/app/ledger/LedgerNodeHelpers.h>
+
+#include <xrpl/basics/IntrusivePointer.h>
+#include <xrpl/basics/base_uint.h>
+#include <xrpl/beast/unit_test/suite.h>
+#include <xrpl/protocol/Serializer.h>
+#include <xrpl/shamap/SHAMap.h>
+#include <xrpl/shamap/SHAMapAccountStateLeafNode.h>
+#include <xrpl/shamap/SHAMapInnerNode.h>
+#include <xrpl/shamap/SHAMapItem.h>
+#include <xrpl/shamap/SHAMapTreeNode.h>
+
+#include <boost/smart_ptr/intrusive_ptr.hpp>
+
+#include <xrpl.pb.h>
+
+#include <cstdint>
+#include <string>
+
+namespace xrpl::tests {
+
+class LedgerNodeHelpers_test : public beast::unit_test::Suite
+{
+    static boost::intrusive_ptr<SHAMapItem>
+    makeTestItem(std::uint32_t seed)
+    {
+        Serializer s;
+        s.add32(seed);
+        s.add32(seed + 1);
+        s.add32(seed + 2);
+        return makeShamapitem(s.getSHA512Half(), s.slice());
+    }
+
+    static std::string
+    serializeNode(SHAMapTreeNodePtr const& node)
+    {
+        Serializer s;
+        node->serializeForWire(s);
+        auto const slice = s.slice();
+        return std::string(slice.begin(), slice.end());
+    }
+
+    void
+    testGetTreeNode()
+    {
+        testcase("getTreeNode");
+
+        // Valid: inner node. It must have at least one child for `serializeNode` to work.
+        {
+            auto const innerNode = intr_ptr::makeShared<SHAMapInnerNode>(1);
+            auto const childNode = intr_ptr::makeShared<SHAMapInnerNode>(1);
+            innerNode->setChild(0, childNode);
+            auto const innerData = serializeNode(innerNode);
+            auto const result = getTreeNode(innerData);
+            BEAST_EXPECT(result && result->isInner());
+        }
+
+        // Valid: leaf node.
+        {
+            auto const leafItem = makeTestItem(12345);
+            auto const leafNode = intr_ptr::makeShared<SHAMapAccountStateLeafNode>(leafItem, 1);
+            auto const leafData = serializeNode(leafNode);
+            auto const result = getTreeNode(leafData);
+            BEAST_EXPECT(result && result->isLeaf());
+        }
+
+        // Invalid: empty data.
+        {
+            auto const result = getTreeNode("");
+            BEAST_EXPECT(!result);
+        }
+
+        // Invalid: garbage data.
+        {
+            auto const result = getTreeNode("invalid");
+            BEAST_EXPECT(!result);
+        }
+
+        // Invalid: truncated data.
+        {
+            auto const leafItem = makeTestItem(54321);
+            auto const leafNode = intr_ptr::makeShared<SHAMapAccountStateLeafNode>(leafItem, 1);
+            // Truncate the data to trigger an exception in SHAMapTreeNode::makeAccountState when
+            // the data is used to deserialize the node.
+            uint256 const tag;
+            auto const leafData = serializeNode(leafNode).substr(0, tag.kBytes - 1);
+            auto const result = getTreeNode(leafData);
+            BEAST_EXPECT(!result);
+        }
+    }
+
+    void
+    testGetSHAMapNodeID()
+    {
+        testcase("getSHAMapNodeID");
+
+        {
+            // Tests using inner nodes at various depths.
+            auto const innerNode = intr_ptr::makeShared<SHAMapInnerNode>(1);
+            auto const childNode = intr_ptr::makeShared<SHAMapInnerNode>(1);
+            innerNode->setChild(0, childNode);
+            auto const innerData = serializeNode(innerNode);
+
+            // Valid: legacy `nodeid` field at arbitrary depth.
+            {
+                auto const innerDepth = 3;
+                auto const innerID = SHAMapNodeID::createID(innerDepth, uint256{});
+
+                protocol::TMLedgerNode ledgerNode;
+                ledgerNode.set_nodedata(innerData);
+                ledgerNode.set_nodeid(innerID.getRawString());
+                auto const result = getSHAMapNodeID(ledgerNode, *innerNode);
+                BEAST_EXPECT(result == innerID);
+            }
+
+            // Valid: new `id` field at minimum depth.
+            {
+                auto const innerDepth = 0;
+                auto const innerID = SHAMapNodeID::createID(innerDepth, uint256{});
+
+                protocol::TMLedgerNode ledgerNode;
+                ledgerNode.set_nodedata(innerData);
+                ledgerNode.set_id(innerID.getRawString());
+                auto const result = getSHAMapNodeID(ledgerNode, *innerNode);
+                BEAST_EXPECT(result == innerID);
+            }
+
+            // Invalid: new `depth` field should not be used for inner nodes.
+            {
+                protocol::TMLedgerNode ledgerNode;
+                ledgerNode.set_nodedata(innerData);
+                ledgerNode.set_depth(10);
+                auto const result = getSHAMapNodeID(ledgerNode, *innerNode);
+                BEAST_EXPECT(!result);
+            }
+
+            // Invalid: both legacy `nodeid` and new `id` fields set for an inner node.
+            {
+                auto const innerDepth = 9;
+                auto const innerID = SHAMapNodeID::createID(innerDepth, uint256{});
+
+                protocol::TMLedgerNode ledgerNode;
+                ledgerNode.set_nodedata(innerData);
+                ledgerNode.set_nodeid(innerID.getRawString());
+                ledgerNode.set_id(innerID.getRawString());
+                auto const result = getSHAMapNodeID(ledgerNode, *innerNode);
+                BEAST_EXPECT(!result);
+            }
+        }
+
+        {
+            // Tests using leaf nodes at various depths.
+            auto const leafItem = makeTestItem(12345);
+            auto const leafNode = intr_ptr::makeShared<SHAMapAccountStateLeafNode>(leafItem, 1);
+            auto const leafData = serializeNode(leafNode);
+            auto const leafKey = leafItem->key();
+
+            // Valid: legacy `nodeid` field at arbitrary depth.
+            {
+                auto const kLeafDepth = 5;
+                auto const leafID = SHAMapNodeID::createID(kLeafDepth, leafKey);
+
+                protocol::TMLedgerNode ledgerNode;
+                ledgerNode.set_nodedata(leafData);
+                ledgerNode.set_nodeid(leafID.getRawString());
+                auto const result = getSHAMapNodeID(ledgerNode, *leafNode);
+                BEAST_EXPECT(result == leafID);
+            }
+
+            // Invalid: new `id` field should not be used for leaf nodes.
+            {
+                auto const kLeafDepth = 5;
+                auto const leafID = SHAMapNodeID::createID(kLeafDepth, leafKey);
+
+                protocol::TMLedgerNode ledgerNode;
+                ledgerNode.set_nodedata(leafData);
+                ledgerNode.set_id(leafID.getRawString());
+                auto const result = getSHAMapNodeID(ledgerNode, *leafNode);
+                BEAST_EXPECT(!result);
+            }
+
+            // Valid: new `depth` field at minimum depth.
+            {
+                auto const kLeafDepth = 0;
+                auto const leafID = SHAMapNodeID::createID(kLeafDepth, leafKey);
+
+                protocol::TMLedgerNode ledgerNode;
+                ledgerNode.set_nodedata(leafData);
+                ledgerNode.set_depth(kLeafDepth);
+                auto const result = getSHAMapNodeID(ledgerNode, *leafNode);
+                BEAST_EXPECT(result == leafID);
+            }
+
+            // Valid: new `depth` field at arbitrary depth between minimum and maximum.
+            {
+                auto const kLeafDepth = 10;
+                auto const leafID = SHAMapNodeID::createID(kLeafDepth, leafKey);
+
+                protocol::TMLedgerNode ledgerNode;
+                ledgerNode.set_nodedata(leafData);
+                ledgerNode.set_depth(kLeafDepth);
+                auto const result = getSHAMapNodeID(ledgerNode, *leafNode);
+                BEAST_EXPECT(result == leafID);
+            }
+
+            // Valid: new `depth` field at maximum depth.
+            // Note that we do not test a depth greater than the maximum depth, because the proto
+            // message is assumed to have been validated by the time the getSHAMapNodeID function is
+            // called.
+            {
+                auto const kLeafDepth = SHAMap::kLeafDepth;
+                auto const leafID = SHAMapNodeID::createID(kLeafDepth, leafKey);
+
+                protocol::TMLedgerNode ledgerNode;
+                ledgerNode.set_nodedata(leafData);
+                ledgerNode.set_depth(kLeafDepth);
+                auto const result = getSHAMapNodeID(ledgerNode, *leafNode);
+                BEAST_EXPECT(result == leafID);
+            }
+
+            // Invalid: legacy `nodeid` field where the node ID is inconsistent with the key.
+            {
+                auto const otherItem = makeTestItem(54321);
+                auto const otherNode =
+                    intr_ptr::makeShared<SHAMapAccountStateLeafNode>(otherItem, 1);
+                auto const otherData = serializeNode(otherNode);
+                auto const otherKey = otherItem->key();
+                auto const otherDepth = 1;
+                auto const otherID = SHAMapNodeID::createID(otherDepth, otherKey);
+
+                protocol::TMLedgerNode ledgerNode;
+                ledgerNode.set_nodedata(otherData);
+                ledgerNode.set_nodeid(otherID.getRawString());
+                auto const result = getSHAMapNodeID(ledgerNode, *leafNode);
+                BEAST_EXPECT(!result);
+            }
+        }
+
+        // Invalid: no field set.
+        {
+            auto const innerNode = intr_ptr::makeShared<SHAMapInnerNode>(1);
+            protocol::TMLedgerNode ledgerNode;
+            ledgerNode.set_nodedata("test_data");
+            auto const result = getSHAMapNodeID(ledgerNode, *innerNode);
+            BEAST_EXPECT(!result);
+        }
+    }
+
+public:
+    void
+    run() override
+    {
+        testGetTreeNode();
+        testGetSHAMapNodeID();
+    }
+};
+
+BEAST_DEFINE_TESTSUITE(LedgerNodeHelpers, app, xrpl);
+
+}  // namespace xrpl::tests

src/test/beast/SemanticVersion_test.cpp

@@ -1,266 +0,0 @@
-#include <xrpl/beast/core/SemanticVersion.h>
-#include <xrpl/beast/unit_test/suite.h>
-
-#include <string>
-
-namespace beast {
-
-class SemanticVersion_test : public unit_test::Suite
-{
-    using identifier_list = SemanticVersion::identifier_list;
-
-public:
-    void
-    checkPass(std::string const& input, bool shouldPass = true)
-    {
-        SemanticVersion v;
-
-        if (shouldPass)
-        {
-            BEAST_EXPECT(v.parse(input));
-            BEAST_EXPECT(v.print() == input);
-        }
-        else
-        {
-            BEAST_EXPECT(!v.parse(input));
-        }
-    }
-
-    void
-    checkFail(std::string const& input)
-    {
-        checkPass(input, false);
-    }
-
-    // check input and input with appended metadata
-    void
-    checkMeta(std::string const& input, bool shouldPass)
-    {
-        checkPass(input, shouldPass);
-
-        checkPass(input + "+a", shouldPass);
-        checkPass(input + "+1", shouldPass);
-        checkPass(input + "+a.b", shouldPass);
-        checkPass(input + "+ab.cd", shouldPass);
-
-        checkFail(input + "!");
-        checkFail(input + "+");
-        checkFail(input + "++");
-        checkFail(input + "+!");
-        checkFail(input + "+.");
-        checkFail(input + "+a.!");
-    }
-
-    void
-    checkMetaFail(std::string const& input)
-    {
-        checkMeta(input, false);
-    }
-
-    // check input, input with appended release data,
-    // input with appended metadata, and input with both
-    // appended release data and appended metadata
-    //
-    void
-    checkRelease(std::string const& input, bool shouldPass = true)
-    {
-        checkMeta(input, shouldPass);
-
-        checkMeta(input + "-1", shouldPass);
-        checkMeta(input + "-a", shouldPass);
-        checkMeta(input + "-a1", shouldPass);
-        checkMeta(input + "-a1.b1", shouldPass);
-        checkMeta(input + "-ab.cd", shouldPass);
-        checkMeta(input + "--", shouldPass);
-
-        checkMetaFail(input + "+");
-        checkMetaFail(input + "!");
-        checkMetaFail(input + "-");
-        checkMetaFail(input + "-!");
-        checkMetaFail(input + "-.");
-        checkMetaFail(input + "-a.!");
-        checkMetaFail(input + "-0.a");
-    }
-
-    // Checks the major.minor.version string alone and with all
-    // possible combinations of release identifiers and metadata.
-    //
-    void
-    check(std::string const& input, bool shouldPass = true)
-    {
-        checkRelease(input, shouldPass);
-    }
-
-    void
-    negcheck(std::string const& input)
-    {
-        check(input, false);
-    }
-
-    void
-    testParse()
-    {
-        testcase("parsing");
-
-        check("0.0.0");
-        check("1.2.3");
-        check("2147483647.2147483647.2147483647");  // max int
-
-        // negative values
-        negcheck("-1.2.3");
-        negcheck("1.-2.3");
-        negcheck("1.2.-3");
-
-        // missing parts
-        negcheck("");
-        negcheck("1");
-        negcheck("1.");
-        negcheck("1.2");
-        negcheck("1.2.");
-        negcheck(".2.3");
-
-        // whitespace
-        negcheck(" 1.2.3");
-        negcheck("1 .2.3");
-        negcheck("1.2 .3");
-        negcheck("1.2.3 ");
-
-        // leading zeroes
-        negcheck("01.2.3");
-        negcheck("1.02.3");
-        negcheck("1.2.03");
-    }
-
-    static identifier_list
-    ids()
-    {
-        return identifier_list();
-    }
-
-    static identifier_list
-    ids(std::string const& s1)
-    {
-        identifier_list v;
-        v.push_back(s1);
-        return v;
-    }
-
-    static identifier_list
-    ids(std::string const& s1, std::string const& s2)
-    {
-        identifier_list v;
-        v.push_back(s1);
-        v.push_back(s2);
-        return v;
-    }
-
-    static identifier_list
-    ids(std::string const& s1, std::string const& s2, std::string const& s3)
-    {
-        identifier_list v;
-        v.push_back(s1);
-        v.push_back(s2);
-        v.push_back(s3);
-        return v;
-    }
-
-    // Checks the decomposition of the input into appropriate values
-    void
-    checkValues(
-        std::string const& input,
-        int majorVersion,
-        int minorVersion,
-        int patchVersion,
-        identifier_list const& preReleaseIdentifiers = identifier_list(),
-        identifier_list const& metaData = identifier_list())
-    {
-        SemanticVersion v;
-
-        BEAST_EXPECT(v.parse(input));
-
-        BEAST_EXPECT(v.majorVersion == majorVersion);
-        BEAST_EXPECT(v.minorVersion == minorVersion);
-        BEAST_EXPECT(v.patchVersion == patchVersion);
-
-        BEAST_EXPECT(v.preReleaseIdentifiers == preReleaseIdentifiers);
-        BEAST_EXPECT(v.metaData == metaData);
-    }
-
-    void
-    testValues()
-    {
-        testcase("values");
-
-        checkValues("0.1.2", 0, 1, 2);
-        checkValues("1.2.3", 1, 2, 3);
-        checkValues("1.2.3-rc1", 1, 2, 3, ids("rc1"));
-        checkValues("1.2.3-rc1.debug", 1, 2, 3, ids("rc1", "debug"));
-        checkValues("1.2.3-rc1.debug.asm", 1, 2, 3, ids("rc1", "debug", "asm"));
-        checkValues("1.2.3+full", 1, 2, 3, ids(), ids("full"));
-        checkValues("1.2.3+full.prod", 1, 2, 3, ids(), ids("full", "prod"));
-        checkValues("1.2.3+full.prod.x86", 1, 2, 3, ids(), ids("full", "prod", "x86"));
-        checkValues(
-            "1.2.3-rc1.debug.asm+full.prod.x86",
-            1,
-            2,
-            3,
-            ids("rc1", "debug", "asm"),
-            ids("full", "prod", "x86"));
-    }
-
-    // makes sure the left version is less than the right
-    void
-    checkLessInternal(std::string const& lhs, std::string const& rhs)
-    {
-        SemanticVersion left;
-        SemanticVersion right;
-
-        BEAST_EXPECT(left.parse(lhs));
-        BEAST_EXPECT(right.parse(rhs));
-
-        BEAST_EXPECT(compare(left, left) == 0);
-        BEAST_EXPECT(compare(right, right) == 0);
-        BEAST_EXPECT(compare(left, right) < 0);
-        BEAST_EXPECT(compare(right, left) > 0);
-
-        BEAST_EXPECT(left < right);
-        BEAST_EXPECT(right > left);
-        BEAST_EXPECT(left == left);
-        BEAST_EXPECT(right == right);
-    }
-
-    void
-    checkLess(std::string const& lhs, std::string const& rhs)
-    {
-        checkLessInternal(lhs, rhs);
-        checkLessInternal(lhs + "+meta", rhs);
-        checkLessInternal(lhs, rhs + "+meta");
-        checkLessInternal(lhs + "+meta", rhs + "+meta");
-    }
-
-    void
-    testCompare()
-    {
-        testcase("comparisons");
-
-        checkLess("1.0.0-alpha", "1.0.0-alpha.1");
-        checkLess("1.0.0-alpha.1", "1.0.0-alpha.beta");
-        checkLess("1.0.0-alpha.beta", "1.0.0-beta");
-        checkLess("1.0.0-beta", "1.0.0-beta.2");
-        checkLess("1.0.0-beta.2", "1.0.0-beta.11");
-        checkLess("1.0.0-beta.11", "1.0.0-rc.1");
-        checkLess("1.0.0-rc.1", "1.0.0");
-        checkLess("0.9.9", "1.0.0");
-    }
-
-    void
-    run() override
-    {
-        testParse();
-        testValues();
-        testCompare();
-    }
-};
-
-BEAST_DEFINE_TESTSUITE(SemanticVersion, beast, beast);
-}  // namespace beast

src/test/overlay/ProtocolVersion_test.cpp

@@ -63,8 +63,8 @@ public:
                 negotiateProtocolVersion("RTXP/1.2, XRPL/2.0, XRPL/2.1") == makeProtocol(2, 1));
             BEAST_EXPECT(negotiateProtocolVersion("XRPL/2.2") == makeProtocol(2, 2));
             BEAST_EXPECT(
-                negotiateProtocolVersion("RTXP/1.2, XRPL/2.2, XRPL/2.3, XRPL/999.999") ==
-                makeProtocol(2, 2));
+                negotiateProtocolVersion("RTXP/1.2, XRPL/2.3, XRPL/2.4, XRPL/999.999") ==
+                makeProtocol(2, 3));
             BEAST_EXPECT(negotiateProtocolVersion("XRPL/999.999, WebSocket/1.0") == std::nullopt);
             BEAST_EXPECT(negotiateProtocolVersion("") == std::nullopt);
         }

src/tests/libxrpl/beast/SemanticVersion.cpp

@@ -0,0 +1,333 @@
+#include <xrpl/beast/core/SemanticVersion.h>
+
+#include <gtest/gtest.h>
+
+#include <algorithm>
+#include <array>
+#include <cctype>
+#include <locale>
+#include <string>
+#include <string_view>
+#include <vector>
+
+namespace beast {
+namespace {
+
+using IdentifierList = SemanticVersion::IdentifierList;
+
+// Version strings are not valid C++ identifiers, so squash their punctuation to
+// turn one into a gtest parameter name.
+std::string
+identifierFor(std::string_view version)
+{
+    std::string name{version};
+    std::ranges::replace_if(
+        name, [](char c) { return !std::isalnum(c, std::locale::classic()); }, '_');
+    if (!name.empty() && std::isdigit(name.front(), std::locale::classic()))
+        name.insert(0, "v_");
+    return name;
+}
+
+// Pre-release and metadata suffixes, each applied to a "major.minor.patch" base.
+// The valid ones leave a well-formed base well-formed; the invalid ones make any
+// base malformed.
+constexpr auto kValidPreRelease =
+    std::to_array<std::string_view>({"", "-1", "-a", "-a1", "-a1.b1", "-ab.cd", "--"});
+constexpr auto kInvalidPreRelease =
+    std::to_array<std::string_view>({"+", "!", "-", "-!", "-.", "-a.!", "-0.a"});
+constexpr auto kValidMetaData = std::to_array<std::string_view>({"", "+a", "+1", "+a.b", "+ab.cd"});
+constexpr auto kInvalidMetaData =
+    std::to_array<std::string_view>({"!", "+", "++", "+!", "+.", "+a.!"});
+
+// Assembles base + preRelease + metaData and checks whether it parses. A version
+// we accept must also round-trip through print().
+void
+expectParse(
+    std::string_view base,
+    std::string_view preRelease,
+    std::string_view metaData,
+    bool shouldPass)
+{
+    auto const input = std::string{base}.append(preRelease).append(metaData);
+    SCOPED_TRACE(::testing::Message() << '"' << input << '"');
+
+    SemanticVersion v;
+
+    if (shouldPass)
+    {
+        EXPECT_TRUE(v.parse(input));
+        EXPECT_EQ(v.print(), input);
+    }
+    else
+    {
+        EXPECT_FALSE(v.parse(input));
+    }
+}
+
+struct ParseCase
+{
+    std::string_view testName;
+    std::string_view base;
+    bool shouldPass;
+};
+
+std::string
+parseCaseName(::testing::TestParamInfo<ParseCase> const& info)
+{
+    return std::string{info.param.testName};
+}
+
+constexpr auto kParseCases = std::to_array<ParseCase>({
+    {.testName = "zeroes", .base = "0.0.0", .shouldPass = true},
+    {.testName = "simple", .base = "1.2.3", .shouldPass = true},
+    {.testName = "max_int", .base = "2147483647.2147483647.2147483647", .shouldPass = true},
+
+    // negative values
+    {.testName = "negative_major", .base = "-1.2.3", .shouldPass = false},
+    {.testName = "negative_minor", .base = "1.-2.3", .shouldPass = false},
+    {.testName = "negative_patch", .base = "1.2.-3", .shouldPass = false},
+
+    // missing parts
+    {.testName = "empty", .base = "", .shouldPass = false},
+    {.testName = "major_only", .base = "1", .shouldPass = false},
+    {.testName = "major_then_dot", .base = "1.", .shouldPass = false},
+    {.testName = "major_and_minor", .base = "1.2", .shouldPass = false},
+    {.testName = "major_minor_then_dot", .base = "1.2.", .shouldPass = false},
+    {.testName = "missing_major", .base = ".2.3", .shouldPass = false},
+
+    // whitespace
+    {.testName = "leading_space", .base = " 1.2.3", .shouldPass = false},
+    {.testName = "space_after_major", .base = "1 .2.3", .shouldPass = false},
+    {.testName = "space_after_minor", .base = "1.2 .3", .shouldPass = false},
+    {.testName = "trailing_space", .base = "1.2.3 ", .shouldPass = false},
+
+    // leading zeroes
+    {.testName = "leading_zero_in_major", .base = "01.2.3", .shouldPass = false},
+    {.testName = "leading_zero_in_minor", .base = "1.02.3", .shouldPass = false},
+    {.testName = "leading_zero_in_patch", .base = "1.2.03", .shouldPass = false},
+});
+
+struct ValuesCase
+{
+    std::string_view testName;
+    std::string_view input;
+    int majorVersion;
+    int minorVersion;
+    int patchVersion;
+    IdentifierList preReleaseIdentifiers{};  // NOLINT(readability-redundant-member-init)
+    IdentifierList metaData{};               // NOLINT(readability-redundant-member-init)
+};
+
+std::string
+valuesCaseName(::testing::TestParamInfo<ValuesCase> const& info)
+{
+    return std::string{info.param.testName};
+}
+
+std::vector<ValuesCase> const kValuesCases{
+    {
+        .testName = "zero_major",
+        .input = "0.1.2",
+        .majorVersion = 0,
+        .minorVersion = 1,
+        .patchVersion = 2,
+    },
+    {
+        .testName = "simple",
+        .input = "1.2.3",
+        .majorVersion = 1,
+        .minorVersion = 2,
+        .patchVersion = 3,
+    },
+    {
+        .testName = "one_pre_release_identifier",
+        .input = "1.2.3-rc1",
+        .majorVersion = 1,
+        .minorVersion = 2,
+        .patchVersion = 3,
+        .preReleaseIdentifiers = {"rc1"},
+    },
+    {
+        .testName = "two_pre_release_identifiers",
+        .input = "1.2.3-rc1.debug",
+        .majorVersion = 1,
+        .minorVersion = 2,
+        .patchVersion = 3,
+        .preReleaseIdentifiers = {"rc1", "debug"},
+    },
+    {
+        .testName = "three_pre_release_identifiers",
+        .input = "1.2.3-rc1.debug.asm",
+        .majorVersion = 1,
+        .minorVersion = 2,
+        .patchVersion = 3,
+        .preReleaseIdentifiers = {"rc1", "debug", "asm"},
+    },
+    {
+        .testName = "one_metadata_identifier",
+        .input = "1.2.3+full",
+        .majorVersion = 1,
+        .minorVersion = 2,
+        .patchVersion = 3,
+        .metaData = {"full"},
+    },
+    {
+        .testName = "two_metadata_identifiers",
+        .input = "1.2.3+full.prod",
+        .majorVersion = 1,
+        .minorVersion = 2,
+        .patchVersion = 3,
+        .metaData = {"full", "prod"},
+    },
+    {
+        .testName = "three_metadata_identifiers",
+        .input = "1.2.3+full.prod.x86",
+        .majorVersion = 1,
+        .minorVersion = 2,
+        .patchVersion = 3,
+        .metaData = {"full", "prod", "x86"},
+    },
+    {
+        .testName = "pre_release_and_metadata",
+        .input = "1.2.3-rc1.debug.asm+full.prod.x86",
+        .majorVersion = 1,
+        .minorVersion = 2,
+        .patchVersion = 3,
+        .preReleaseIdentifiers = {"rc1", "debug", "asm"},
+        .metaData = {"full", "prod", "x86"},
+    },
+};
+
+struct OrderCase
+{
+    std::string_view lesser;
+    std::string_view greater;
+};
+
+std::string
+orderCaseName(::testing::TestParamInfo<OrderCase> const& info)
+{
+    return identifierFor(info.param.lesser) + "_below_" + identifierFor(info.param.greater);
+}
+
+constexpr auto kOrderCases = std::to_array<OrderCase>({
+    {.lesser = "1.0.0-alpha", .greater = "1.0.0-alpha.1"},
+    {.lesser = "1.0.0-alpha.1", .greater = "1.0.0-alpha.beta"},
+    {.lesser = "1.0.0-alpha.beta", .greater = "1.0.0-beta"},
+    {.lesser = "1.0.0-beta", .greater = "1.0.0-beta.2"},
+    {.lesser = "1.0.0-beta.2", .greater = "1.0.0-beta.11"},
+    {.lesser = "1.0.0-beta.11", .greater = "1.0.0-rc.1"},
+    {.lesser = "1.0.0-rc.1", .greater = "1.0.0"},
+    {.lesser = "0.9.9", .greater = "1.0.0"},
+});
+
+}  // namespace
+
+class SemanticVersionParse : public ::testing::TestWithParam<ParseCase>
+{
+};
+
+// Exercises the base string on its own and with every combination of appended
+// pre-release identifiers and metadata.
+TEST_P(SemanticVersionParse, pre_release_and_metadata_combinations)
+{
+    auto const& [testName, base, shouldPass] = GetParam();
+
+    for (auto const preRelease : kValidPreRelease)
+    {
+        for (auto const metaData : kValidMetaData)
+            expectParse(base, preRelease, metaData, shouldPass);
+
+        for (auto const metaData : kInvalidMetaData)
+            expectParse(base, preRelease, metaData, false);
+    }
+
+    // A malformed pre-release section poisons the whole string, whatever
+    // metadata follows it.
+    for (auto const preRelease : kInvalidPreRelease)
+    {
+        for (auto const metaData : kValidMetaData)
+            expectParse(base, preRelease, metaData, false);
+
+        for (auto const metaData : kInvalidMetaData)
+            expectParse(base, preRelease, metaData, false);
+    }
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    Inputs,
+    SemanticVersionParse,
+    ::testing::ValuesIn(kParseCases),
+    parseCaseName);
+
+class SemanticVersionValues : public ::testing::TestWithParam<ValuesCase>
+{
+};
+
+TEST_P(SemanticVersionValues, decomposes_into_components)
+{
+    auto const& expected = GetParam();
+
+    SemanticVersion v;
+    EXPECT_TRUE(v.parse(expected.input));
+
+    EXPECT_EQ(v.majorVersion, expected.majorVersion);
+    EXPECT_EQ(v.minorVersion, expected.minorVersion);
+    EXPECT_EQ(v.patchVersion, expected.patchVersion);
+
+    EXPECT_EQ(v.preReleaseIdentifiers, expected.preReleaseIdentifiers);
+    EXPECT_EQ(v.metaData, expected.metaData);
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    Inputs,
+    SemanticVersionValues,
+    ::testing::ValuesIn(kValuesCases),
+    valuesCaseName);
+
+class SemanticVersionOrder : public ::testing::TestWithParam<OrderCase>
+{
+};
+
+TEST_P(SemanticVersionOrder, lesser_precedes_greater)
+{
+    auto const& [lesser, greater] = GetParam();
+
+    // Metadata takes no part in precedence, so attaching it to either side must
+    // leave the ordering untouched.
+    static constexpr auto kMetaData = std::to_array<std::string_view>({"", "+meta"});
+
+    for (auto const lesserMetaData : kMetaData)
+    {
+        for (auto const greaterMetaData : kMetaData)
+        {
+            auto const lesserInput = std::string{lesser}.append(lesserMetaData);
+            auto const greaterInput = std::string{greater}.append(greaterMetaData);
+            SCOPED_TRACE(
+                ::testing::Message() << '"' << lesserInput << "\" < \"" << greaterInput << '"');
+
+            SemanticVersion lesserVersion;
+            SemanticVersion greaterVersion;
+            EXPECT_TRUE(lesserVersion.parse(lesserInput));
+            EXPECT_TRUE(greaterVersion.parse(greaterInput));
+
+            EXPECT_EQ(compare(lesserVersion, lesserVersion), 0);
+            EXPECT_EQ(compare(greaterVersion, greaterVersion), 0);
+            EXPECT_LT(compare(lesserVersion, greaterVersion), 0);
+            EXPECT_GT(compare(greaterVersion, lesserVersion), 0);
+
+            EXPECT_LT(lesserVersion, greaterVersion);
+            EXPECT_GT(greaterVersion, lesserVersion);
+            EXPECT_EQ(lesserVersion, lesserVersion);
+            EXPECT_EQ(greaterVersion, greaterVersion);
+        }
+    }
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    Pairs,
+    SemanticVersionOrder,
+    ::testing::ValuesIn(kOrderCases),
+    orderCaseName);
+
+}  // namespace beast

src/tests/libxrpl/shamap/SHAMapSync.cpp

@@ -1,4 +1,3 @@
-#include <xrpl/basics/Blob.h>
 #include <xrpl/basics/SHAMapHash.h>
 #include <xrpl/basics/Slice.h>
 #include <xrpl/basics/base_uint.h>
@@ -112,14 +111,16 @@ TEST_F(SHAMapSyncTest, sync)
     destination.setSynching();
 
     {
-        std::vector<std::pair<SHAMapNodeID, Blob>> a;
+        std::vector<SHAMapNodeData> a;
 
         ASSERT_TRUE(source.getNodeFat(SHAMapNodeID(), a, randBool(eng_), randInt(eng_, 2)));
 
         ASSERT_FALSE(a.empty()) << "NodeSize";
 
-        ASSERT_TRUE(
-            destination.addRootNode(source.getHash(), makeSlice(a[0].second), nullptr).isGood());
+        auto node = SHAMapTreeNode::makeFromWire(makeSlice(a[0].data));
+        if (!node)
+            FAIL() << "Could not create node";
+        ASSERT_TRUE(destination.addRootNode(source.getHash(), std::move(node), nullptr).isGood());
     }
 
     do
@@ -133,7 +134,7 @@ TEST_F(SHAMapSyncTest, sync)
             break;
 
         // get as many nodes as possible based on this information
-        std::vector<std::pair<SHAMapNodeID, Blob>> b;
+        std::vector<SHAMapNodeData> b;
 
         for (auto& it : nodesMissing)
         {
@@ -155,7 +156,12 @@ TEST_F(SHAMapSyncTest, sync)
             // Keep failures fatal here because this loop is data-dependent.
             // non-deterministic number of times and the number of tests run
             // should be deterministic
-            if (!destination.addKnownNode(i.first, makeSlice(i.second), nullptr).isUseful())
+            auto node = SHAMapTreeNode::makeFromWire(makeSlice(i.data));
+            if (!node)
+                FAIL() << "Could not create node";
+            if (i.isLeaf != node->isLeaf())
+                FAIL() << "Node is not a leaf";
+            if (!destination.addKnownNode(i.nodeID, std::move(node), nullptr).isUseful())
                 FAIL() << "Known node was not useful";
         }
     } while (true);

src/xrpld/app/ledger/InboundLedger.h

@@ -6,7 +6,6 @@
 #include <xrpld/overlay/PeerSet.h>
 
 #include <xrpl/basics/CountedObject.h>
-#include <xrpl/basics/Slice.h>
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/beast/clock/abstract_clock.h>
 #include <xrpl/json/json_value.h>
@@ -24,7 +23,7 @@
 #include <memory>
 #include <mutex>
 #include <set>
-#include <string>
+#include <string_view>
 #include <utility>
 #include <vector>
 
@@ -154,16 +153,19 @@ private:
     processData(std::shared_ptr<Peer> peer, protocol::TMLedgerData const& data);
 
     bool
-    takeHeader(std::string const& data);
+    takeHeader(std::string_view data);
 
     void
-    receiveNode(protocol::TMLedgerData const& packet, SHAMapAddNode&);
+    receiveNode(
+        std::shared_ptr<Peer> const& peer,
+        protocol::TMLedgerData const& packet,
+        SHAMapAddNode& san);
 
     bool
-    takeTxRootNode(Slice const& data, SHAMapAddNode&);
+    takeTxRootNode(std::string_view data, SHAMapAddNode& san);
 
     bool
-    takeAsRootNode(Slice const& data, SHAMapAddNode&);
+    takeAsRootNode(std::string_view data, SHAMapAddNode& san);
 
     std::vector<uint256>
     neededTxHashes(int max, SHAMapSyncFilter const* filter) const;

src/xrpld/app/ledger/LedgerNodeHelpers.h

@@ -0,0 +1,52 @@
+#pragma once
+
+#include <xrpl/shamap/SHAMapNodeID.h>
+#include <xrpl/shamap/SHAMapTreeNode.h>
+
+#include <optional>
+#include <string_view>
+
+namespace protocol {
+class TMLedgerNode;
+}  // namespace protocol
+
+namespace xrpl {
+
+/**
+ * @brief Deserializes a SHAMapTreeNode from wire format data.
+ *
+ * This function attempts to create a SHAMapTreeNode from the provided data string. If the data is
+ * malformed or deserialization fails, the function returns a nullptr instead of throwing an
+ * exception.
+ *
+ * @param data The serialized node data in wire format.
+ * @return The deserialized tree node if successful, or a nullptr if deserialization fails.
+ */
+[[nodiscard]] SHAMapTreeNodePtr
+getTreeNode(std::string_view data);
+
+/**
+ * @brief Extracts or reconstructs the SHAMapNodeID from a ledger node proto message.
+ *
+ * This function retrieves the SHAMapNodeID for a tree node, with behavior that depends on which
+ * field is set and the node type (inner vs. leaf).
+ *
+ * When the legacy `nodeid` field is set in the message:
+ * - For all nodes: Deserializes the node ID from the field.
+ * - For leaf nodes: Validates that the node ID is consistent with the leaf's key.
+ *
+ * When the new `id` or `depth` field is set in the message:
+ * - For inner nodes: Deserializes the node ID from the `id` field.
+ * - For leaf nodes: Reconstructs the node ID using both the depth from the `depth` field and the
+ *   key from the leaf node's item.
+ * Note that root nodes may be inner nodes or leaf nodes.
+ *
+ * @param ledgerNode The validated protocol message containing the ledger node data.
+ * @param treeNode The deserialized tree node (inner or leaf node).
+ * @return An optional containing the node ID if extraction/reconstruction succeeds, or std::nullopt
+ *         if the required fields are missing or validation fails.
+ */
+[[nodiscard]] std::optional<SHAMapNodeID>
+getSHAMapNodeID(protocol::TMLedgerNode const& ledgerNode, SHAMapTreeNode const& treeNode);
+
+}  // namespace xrpl

src/xrpld/app/ledger/detail/InboundLedger.cpp

@@ -3,6 +3,7 @@
 #include <xrpld/app/ledger/AccountStateSF.h>
 #include <xrpld/app/ledger/InboundLedgers.h>
 #include <xrpld/app/ledger/LedgerMaster.h>
+#include <xrpld/app/ledger/LedgerNodeHelpers.h>
 #include <xrpld/app/ledger/TransactionStateSF.h>
 #include <xrpld/app/ledger/detail/TimeoutCounter.h>
 #include <xrpld/app/main/Application.h>
@@ -44,8 +45,8 @@
 #include <mutex>
 #include <random>
 #include <sstream>
-#include <stdexcept>
 #include <string>
+#include <string_view>
 #include <tuple>
 #include <unordered_map>
 #include <utility>
@@ -779,7 +780,7 @@ InboundLedger::filterNodes(
  */
 // data must not have hash prefix
 bool
-InboundLedger::takeHeader(std::string const& data)
+InboundLedger::takeHeader(std::string_view data)
 {
     // Return value: true=normal, false=bad data
     JLOG(journal_.trace()) << "got header acquiring ledger " << hash_;
@@ -825,7 +826,10 @@ InboundLedger::takeHeader(std::string const& data)
  * Call with a lock
  */
 void
-InboundLedger::receiveNode(protocol::TMLedgerData const& packet, SHAMapAddNode& san)
+InboundLedger::receiveNode(
+    std::shared_ptr<Peer> const& peer,
+    protocol::TMLedgerData const& packet,
+    SHAMapAddNode& san)
 {
     if (!haveHeader_)
     {
@@ -868,32 +872,47 @@ InboundLedger::receiveNode(protocol::TMLedgerData const& packet, SHAMapAddNode&
     {
         auto const f = filter.get();
 
-        for (auto const& node : packet.nodes())
+        for (auto const& ledgerNode : packet.nodes())
         {
-            auto const nodeID = deserializeSHAMapNodeID(node.nodeid());
+            auto treeNode = getTreeNode(ledgerNode.nodedata());
+            if (!treeNode)
+            {
+                JLOG(journal_.warn())
+                    << "Got invalid node data for ledger " << hash_ << " from peer " << peer->id();
+                peer->charge(Resource::kFeeInvalidData, "ledger_node.node_data invalid");
+                san.incInvalid();
+                return;
+            }
 
+            auto const nodeID = getSHAMapNodeID(ledgerNode, *treeNode);
             if (!nodeID)
-                throw std::runtime_error("data does not properly deserialize");
-
-            if (nodeID->isRoot())
             {
-                san += map.addRootNode(rootHash, makeSlice(node.nodedata()), f);
-            }
-            else
-            {
-                san += map.addKnownNode(*nodeID, makeSlice(node.nodedata()), f);
+                JLOG(journal_.warn())
+                    << "Got invalid node id for ledger " << hash_ << " from peer " << peer->id();
+                peer->charge(Resource::kFeeInvalidData, "ledger_node.node_id invalid");
+                san.incInvalid();
+                return;
             }
 
-            if (!san.isGood())
+            auto const result = nodeID->isRoot()
+                ? map.addRootNode(rootHash, std::move(treeNode), f)
+                : map.addKnownNode(*nodeID, std::move(treeNode), f);
+            san += result;
+
+            if (result.isInvalid())
             {
-                JLOG(journal_.warn()) << "Received bad node data";
+                JLOG(journal_.warn()) << "Got invalid node " << *nodeID << " for ledger " << hash_
+                                      << " from peer " << peer->id();
+                peer->charge(Resource::kFeeInvalidData, "ledger_node invalid");
                 return;
             }
         }
     }
     catch (std::exception const& e)
     {
-        JLOG(journal_.error()) << "Received bad node data: " << e.what();
+        // If we get here it is not necessarily because the node was bad, so don't charge the peer.
+        JLOG(journal_.error()) << "Could not process node for ledger " << hash_ << " from peer "
+                               << peer->id() << ": " << e.what();
         san.incInvalid();
         return;
     }
@@ -922,7 +941,7 @@ InboundLedger::receiveNode(protocol::TMLedgerData const& packet, SHAMapAddNode&
  * Call with a lock
  */
 bool
-InboundLedger::takeAsRootNode(Slice const& data, SHAMapAddNode& san)
+InboundLedger::takeAsRootNode(std::string_view data, SHAMapAddNode& san)
 {
     if (failed_ || haveState_)
     {
@@ -938,10 +957,19 @@ InboundLedger::takeAsRootNode(Slice const& data, SHAMapAddNode& san)
         // LCOV_EXCL_STOP
     }
 
+    auto treeNode = getTreeNode(data);
+    if (!treeNode)
+    {
+        JLOG(journal_.warn()) << "Got invalid AS root node data for ledger " << hash_;
+        san.incInvalid();
+        return false;
+    }
+
     AccountStateSF filter(ledger_->stateMap().family().db(), app_.getLedgerMaster());
-    san +=
-        ledger_->stateMap().addRootNode(SHAMapHash{ledger_->header().accountHash}, data, &filter);
-    return san.isGood();
+    auto const result = ledger_->stateMap().addRootNode(
+        SHAMapHash{ledger_->header().accountHash}, std::move(treeNode), &filter);
+    san += result;
+    return !result.isInvalid();
 }
 
 /**
@@ -949,7 +977,7 @@ InboundLedger::takeAsRootNode(Slice const& data, SHAMapAddNode& san)
  * Call with a lock
  */
 bool
-InboundLedger::takeTxRootNode(Slice const& data, SHAMapAddNode& san)
+InboundLedger::takeTxRootNode(std::string_view data, SHAMapAddNode& san)
 {
     if (failed_ || haveTransactions_)
     {
@@ -965,9 +993,19 @@ InboundLedger::takeTxRootNode(Slice const& data, SHAMapAddNode& san)
         // LCOV_EXCL_STOP
     }
 
+    auto treeNode = getTreeNode(data);
+    if (!treeNode)
+    {
+        JLOG(journal_.warn()) << "Got invalid TX root node data for ledger " << hash_;
+        san.incInvalid();
+        return false;
+    }
+
     TransactionStateSF filter(ledger_->txMap().family().db(), app_.getLedgerMaster());
-    san += ledger_->txMap().addRootNode(SHAMapHash{ledger_->header().txHash}, data, &filter);
-    return san.isGood();
+    auto const result = ledger_->txMap().addRootNode(
+        SHAMapHash{ledger_->header().txHash}, std::move(treeNode), &filter);
+    san += result;
+    return !result.isInvalid();
 }
 
 std::vector<InboundLedger::neededHash_t>
@@ -1065,20 +1103,33 @@ InboundLedger::processData(std::shared_ptr<Peer> peer, protocol::TMLedgerData co
             }
 
             if (!haveState_ && (packet.nodes().size() > 1) &&
-                !takeAsRootNode(makeSlice(packet.nodes(1).nodedata()), san))
+                !takeAsRootNode(packet.nodes(1).nodedata(), san))
             {
-                JLOG(journal_.warn()) << "Included AS root invalid";
+                JLOG(journal_.warn()) << "Included AS root invalid for ledger " << hash_
+                                      << " from peer " << peer->id();
+                if (san.isInvalid())
+                {
+                    peer->charge(Resource::kFeeInvalidData, "ledger_data invalid AS root");
+                    return -1;
+                }
             }
 
             if (!haveTransactions_ && (packet.nodes().size() > 2) &&
-                !takeTxRootNode(makeSlice(packet.nodes(2).nodedata()), san))
+                !takeTxRootNode(packet.nodes(2).nodedata(), san))
             {
-                JLOG(journal_.warn()) << "Included TX root invalid";
+                JLOG(journal_.warn()) << "Included TX root invalid for ledger " << hash_
+                                      << " from peer " << peer->id();
+                if (san.isInvalid())
+                {
+                    peer->charge(Resource::kFeeInvalidData, "ledger_data invalid TX root");
+                    return -1;
+                }
             }
         }
         catch (std::exception const& ex)
         {
-            JLOG(journal_.warn()) << "Included AS/TX root invalid: " << ex.what();
+            JLOG(journal_.warn()) << "Included AS/TX root invalid for ledger " << hash_
+                                  << " from peer " << peer->id() << ": " << ex.what();
             using namespace std::string_literals;
             peer->charge(Resource::kFeeInvalidData, "ledger_data "s + ex.what());
             return -1;
@@ -1102,24 +1153,18 @@ InboundLedger::processData(std::shared_ptr<Peer> peer, protocol::TMLedgerData co
 
         ScopedLockType const sl(mtx_);
 
-        // Verify node IDs and data are complete
-        for (auto const& node : packet.nodes())
-        {
-            if (!node.has_nodeid() || !node.has_nodedata())
-            {
-                JLOG(journal_.warn()) << "Got bad node";
-                peer->charge(Resource::kFeeMalformedRequest, "ledger_data bad node");
-                return -1;
-            }
-        }
-
         SHAMapAddNode san;
-        receiveNode(packet, san);
+        receiveNode(peer, packet, san);
 
         JLOG(journal_.debug()) << "Ledger "
                                << ((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.isUseful())
             progress_ = true;
 

src/xrpld/app/ledger/detail/InboundLedgers.cpp

@@ -2,13 +2,13 @@
 
 #include <xrpld/app/ledger/InboundLedger.h>
 #include <xrpld/app/ledger/LedgerMaster.h>
+#include <xrpld/app/ledger/LedgerNodeHelpers.h>
 #include <xrpld/app/main/Application.h>
 #include <xrpld/overlay/PeerSet.h>
 
 #include <xrpl/basics/Blob.h>
 #include <xrpl/basics/DecayingSample.h>
 #include <xrpl/basics/Log.h>
-#include <xrpl/basics/Slice.h>
 #include <xrpl/basics/UnorderedContainers.h>
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/basics/scope.h>
@@ -252,23 +252,17 @@ public:
         Serializer s;
         try
         {
-            for (int i = 0; i < packetPtr->nodes().size(); ++i)
+            for (auto const& ledgerNode : packetPtr->nodes())
             {
-                auto const& node = packetPtr->nodes(i);
-
-                if (!node.has_nodeid() || !node.has_nodedata())
-                    return;
-
-                auto newNode = SHAMapTreeNode::makeFromWire(makeSlice(node.nodedata()));
-
-                if (!newNode)
+                auto const treeNode = getTreeNode(ledgerNode.nodedata());
+                if (!treeNode)
                     return;
 
                 s.erase();
-                newNode->serializeWithPrefix(s);
+                treeNode->serializeWithPrefix(s);
 
                 app_.getLedgerMaster().addFetchPack(
-                    newNode->getHash().asUInt256(), std::make_shared<Blob>(s.begin(), s.end()));
+                    treeNode->getHash().asUInt256(), std::make_shared<Blob>(s.begin(), s.end()));
             }
         }
         catch (std::exception const&)  // NOLINT(bugprone-empty-catch)

src/xrpld/app/ledger/detail/InboundTransactions.cpp

@@ -1,11 +1,11 @@
 #include <xrpld/app/ledger/InboundTransactions.h>
 
+#include <xrpld/app/ledger/LedgerNodeHelpers.h>
 #include <xrpld/app/ledger/detail/TransactionAcquire.h>
 #include <xrpld/app/main/Application.h>
 #include <xrpld/overlay/PeerSet.h>
 
 #include <xrpl/basics/Log.h>
-#include <xrpl/basics/Slice.h>
 #include <xrpl/basics/UnorderedContainers.h>
 #include <xrpl/beast/insight/Collector.h>
 #include <xrpl/protocol/RippleLedgerHash.h>
@@ -14,6 +14,7 @@
 #include <xrpl/shamap/SHAMap.h>
 #include <xrpl/shamap/SHAMapMissingNode.h>
 #include <xrpl/shamap/SHAMapNodeID.h>
+#include <xrpl/shamap/SHAMapTreeNode.h>
 
 #include <xrpl.pb.h>
 
@@ -137,34 +138,45 @@ public:
 
         if (ta == nullptr)
         {
-            peer->charge(Resource::kFeeUselessData, "ledger_data");
+            peer->charge(Resource::kFeeUselessData, "ledger_data useless");
             return;
         }
 
-        std::vector<std::pair<SHAMapNodeID, Slice>> data;
+        std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>> data;
         data.reserve(packet.nodes().size());
 
-        for (auto const& node : packet.nodes())
+        for (auto const& ledgerNode : packet.nodes())
         {
-            if (!node.has_nodeid() || !node.has_nodedata())
+            auto treeNode = getTreeNode(ledgerNode.nodedata());
+            if (!treeNode)
             {
-                peer->charge(Resource::kFeeMalformedRequest, "ledger_data");
+                JLOG(j_.warn()) << "Got invalid node data for TX set " << hash << " from peer "
+                                << peer->id();
+                peer->charge(Resource::kFeeInvalidData, "ledger_node.node_data invalid");
                 return;
             }
 
-            auto const id = deserializeSHAMapNodeID(node.nodeid());
-
-            if (!id)
+            auto const nodeID = getSHAMapNodeID(ledgerNode, *treeNode);
+            if (!nodeID)
             {
-                peer->charge(Resource::kFeeInvalidData, "ledger_data");
+                JLOG(j_.warn()) << "Got invalid node id for TX set " << hash << " from peer "
+                                << peer->id();
+                peer->charge(Resource::kFeeInvalidData, "ledger_node.node_id invalid");
                 return;
             }
 
-            data.emplace_back(*id, makeSlice(node.nodedata()));
+            data.emplace_back(*nodeID, std::move(treeNode));
         }
 
-        if (!ta->takeNodes(data, peer).isUseful())
-            peer->charge(Resource::kFeeUselessData, "ledger_data not useful");
+        auto const san = ta->takeNodes(std::move(data), peer);
+        if (san.isInvalid())
+        {
+            peer->charge(Resource::kFeeInvalidData, "ledger_data invalid");
+        }
+        else if (!san.isUseful())
+        {
+            peer->charge(Resource::kFeeUselessData, "ledger_data useless");
+        }
     }
 
     void

src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp

@@ -0,0 +1,81 @@
+#include <xrpld/app/ledger/LedgerNodeHelpers.h>
+
+#include <xrpl/basics/Slice.h>
+#include <xrpl/beast/utility/instrumentation.h>
+#include <xrpl/shamap/SHAMap.h>
+#include <xrpl/shamap/SHAMapLeafNode.h>
+#include <xrpl/shamap/SHAMapNodeID.h>
+#include <xrpl/shamap/SHAMapTreeNode.h>
+
+#include <xrpl.pb.h>
+
+#include <exception>
+#include <optional>
+#include <string_view>
+
+namespace xrpl {
+
+SHAMapTreeNodePtr
+getTreeNode(std::string_view data)
+{
+    auto const slice = makeSlice(data);
+    try
+    {
+        return SHAMapTreeNode::makeFromWire(slice);
+    }
+    catch (std::exception const&)
+    {
+        return {};
+    }
+}
+
+std::optional<SHAMapNodeID>
+getSHAMapNodeID(protocol::TMLedgerNode const& ledgerNode, SHAMapTreeNode const& treeNode)
+{
+    if (ledgerNode.has_id() || ledgerNode.has_depth())
+    {
+        // Reject ambiguous messages that mix the legacy and new reference fields.
+        if (ledgerNode.has_nodeid())
+            return std::nullopt;
+
+        if (treeNode.isInner())
+        {
+            if (!ledgerNode.has_id())
+                return std::nullopt;
+
+            return deserializeSHAMapNodeID(ledgerNode.id());
+        }
+
+        if (treeNode.isLeaf())
+        {
+            if (!ledgerNode.has_depth() || ledgerNode.depth() > SHAMap::kLeafDepth)
+                return std::nullopt;
+
+            auto const key = leafKey(treeNode);
+            return SHAMapNodeID::createID(ledgerNode.depth(), key);
+        }
+        // LCOV_EXCL_START
+        UNREACHABLE("xrpl::getSHAMapNodeID : tree node is neither inner nor leaf");
+        return std::nullopt;
+        // LCOV_EXCL_STOP
+    }
+
+    if (!ledgerNode.has_nodeid())
+        return std::nullopt;
+
+    auto nodeID = deserializeSHAMapNodeID(ledgerNode.nodeid());
+    if (!nodeID.has_value())
+        return std::nullopt;
+
+    if (treeNode.isLeaf())
+    {
+        auto const key = leafKey(treeNode);
+        auto const expectedID = SHAMapNodeID::createID(static_cast<int>(nodeID->getDepth()), key);
+        if (nodeID->getNodeID() != expectedID.getNodeID())
+            return std::nullopt;
+    }
+
+    return nodeID;
+}
+
+}  // namespace xrpl

src/xrpld/app/ledger/detail/TransactionAcquire.cpp

@@ -7,13 +7,13 @@
 #include <xrpld/overlay/PeerSet.h>
 
 #include <xrpl/basics/Log.h>
-#include <xrpl/basics/Slice.h>
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/core/Job.h>
 #include <xrpl/server/NetworkOPs.h>
 #include <xrpl/shamap/SHAMap.h>
 #include <xrpl/shamap/SHAMapAddNode.h>
 #include <xrpl/shamap/SHAMapMissingNode.h>
+#include <xrpl/shamap/SHAMapTreeNode.h>
 
 #include <xrpl.pb.h>
 
@@ -171,7 +171,7 @@ TransactionAcquire::trigger(std::shared_ptr<Peer> const& peer)
 
 SHAMapAddNode
 TransactionAcquire::takeNodes(
-    std::vector<std::pair<SHAMapNodeID, Slice>> const& data,
+    std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>> data,
     std::shared_ptr<Peer> const& peer)
 {
     ScopedLockType const sl(mtx_);
@@ -195,7 +195,7 @@ TransactionAcquire::takeNodes(
 
         ConsensusTransSetSF sf(app_, app_.getTempNodeCache());
 
-        for (auto const& d : data)
+        for (auto& d : data)
         {
             if (d.first.isRoot())
             {
@@ -203,18 +203,22 @@ TransactionAcquire::takeNodes(
                 {
                     JLOG(journal_.debug()) << "Got root TXS node, already have it";
                 }
-                else if (!map_->addRootNode(SHAMapHash{hash_}, d.second, nullptr).isGood())
+                else if (!map_->addRootNode(SHAMapHash{hash_}, std::move(d.second), nullptr)
+                              .isGood())
                 {
-                    JLOG(journal_.warn()) << "TX acquire got bad root node";
+                    JLOG(journal_.warn()) << "TX acquire got bad root node for TX set " << hash_
+                                          << " from peer " << peer->id();
+                    return SHAMapAddNode::invalid();
                 }
                 else
                 {
                     haveRoot_ = true;
                 }
             }
-            else if (!map_->addKnownNode(d.first, d.second, &sf).isGood())
+            else if (!map_->addKnownNode(d.first, std::move(d.second), &sf).isGood())
             {
-                JLOG(journal_.warn()) << "TX acquire got bad non-root node";
+                JLOG(journal_.warn()) << "TX acquire got bad non-root node " << d.first
+                                      << " for TX set " << hash_ << " from peer " << peer->id();
                 return SHAMapAddNode::invalid();
             }
         }

src/xrpld/app/ledger/detail/TransactionAcquire.h

@@ -6,10 +6,10 @@
 #include <xrpld/overlay/PeerSet.h>
 
 #include <xrpl/basics/CountedObject.h>
-#include <xrpl/basics/Slice.h>
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/shamap/SHAMap.h>
 #include <xrpl/shamap/SHAMapAddNode.h>
+#include <xrpl/shamap/SHAMapTreeNode.h>
 
 #include <cstddef>
 #include <memory>
@@ -32,8 +32,8 @@ public:
 
     SHAMapAddNode
     takeNodes(
-        std::vector<std::pair<SHAMapNodeID, Slice>> const& data,
-        std::shared_ptr<Peer> const&);
+        std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>> data,
+        std::shared_ptr<Peer> const& peer);
 
     void
     init(int startPeers);

src/xrpld/overlay/Peer.h

@@ -23,6 +23,7 @@ enum class ProtocolFeature {
     ValidatorListPropagation,
     ValidatorList2Propagation,
     LedgerReplay,
+    LedgerNodeDepth,
 };
 
 /**

src/xrpld/overlay/detail/PeerImp.cpp

@@ -5,6 +5,7 @@
 #include <xrpld/app/ledger/InboundLedgers.h>
 #include <xrpld/app/ledger/InboundTransactions.h>
 #include <xrpld/app/ledger/LedgerMaster.h>
+#include <xrpld/app/ledger/LedgerNodeHelpers.h>
 #include <xrpld/app/ledger/TransactionMaster.h>
 #include <xrpld/app/misc/Transaction.h>
 #include <xrpld/app/misc/ValidatorList.h>
@@ -61,6 +62,7 @@
 #include <xrpl/resource/Gossip.h>
 #include <xrpl/server/LoadFeeTrack.h>
 #include <xrpl/server/NetworkOPs.h>
+#include <xrpl/shamap/SHAMap.h>
 #include <xrpl/shamap/SHAMapNodeID.h>
 #include <xrpl/tx/apply.h>
 
@@ -543,6 +545,8 @@ PeerImp::supportsFeature(ProtocolFeature f) const
             return protocol_ >= makeProtocol(2, 1);
         case ProtocolFeature::ValidatorList2Propagation:
             return protocol_ >= makeProtocol(2, 2);
+        case ProtocolFeature::LedgerNodeDepth:
+            return protocol_ >= makeProtocol(2, 3);
         case ProtocolFeature::LedgerReplay:
             return ledgerReplayEnabled_;
     }
@@ -1477,23 +1481,12 @@ PeerImp::onMessage(std::shared_ptr<protocol::TMGetLedger> const& m)
         }
     }
 
-    // Verify ledger node IDs
-    if (itype != protocol::liBASE)
+    // Verify ledger node counts. Full parsing of the node IDs is deferred to the job, so the I/O
+    // thread is not burdened with SHAMapNodeID deserialization for every TMGetLedger message.
+    if (itype != protocol::liBASE && m->nodeids_size() <= 0)
     {
-        if (m->nodeids_size() <= 0)
-        {
-            badData("Invalid ledger node IDs");
-            return;
-        }
-
-        for (auto const& nodeId : m->nodeids())
-        {
-            if (deserializeSHAMapNodeID(nodeId) == std::nullopt)
-            {
-                badData("Invalid SHAMap node ID");
-                return;
-            }
-        }
+        badData("Invalid ledger node IDs");
+        return;
     }
 
     // Verify query type
@@ -1513,11 +1506,57 @@ PeerImp::onMessage(std::shared_ptr<protocol::TMGetLedger> const& m)
         }
     }
 
-    // Queue a job to process the request
+    // Queue a job to process the request.
     std::weak_ptr<PeerImp> const weak = shared_from_this();
-    app_.getJobQueue().addJob(JtLedgerReq, "RcvGetLedger", [weak, m]() {
-        if (auto peer = weak.lock())
-            peer->processLedgerRequest(m);
+    app_.getJobQueue().addJob(JtLedgerReq, "RcvGetLedger", [weak, m, itype]() {
+        auto peer = weak.lock();
+        if (!peer)
+            return;
+
+        std::vector<SHAMapNodeID> nodeIDs;
+        bool tooManyNodeIds = false;
+        if (itype != protocol::liBASE)
+        {
+            nodeIDs.reserve(std::min(m->nodeids_size(), Tuning::kSoftMaxReplyNodes));
+            for (auto const& nodeId : m->nodeids())
+            {
+                if (nodeIDs.size() >= Tuning::kSoftMaxReplyNodes)
+                {
+                    // The peer requested too many node IDs. Continue processing the received node
+                    // IDs up to the limit. If the request is legitimate then at least they will get
+                    // a response and won't have to resend these nodes in their next request.
+                    tooManyNodeIds = true;
+                    break;
+                }
+                auto parsed = deserializeSHAMapNodeID(nodeId);
+                if (!parsed)
+                {
+                    peer->charge(Resource::kFeeInvalidData, "TMGetLedger: Invalid node ID");
+                    return;
+                }
+                nodeIDs.push_back(std::move(*parsed));
+            }
+        }
+
+        // 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()));
+        }
+        if (!m->has_requestcookie())
+        {
+            peer->charge(Resource::kFeeModerateBurdenPeer, "TMGetLedger: get ledger request");
+        }
+
+        peer->processLedgerRequest(m, std::move(nodeIDs));
     });
 }
 
@@ -1682,12 +1721,102 @@ PeerImp::onMessage(std::shared_ptr<protocol::TMLedgerData> const& m)
         return;
     }
 
-    // If there is a request cookie, attempt to relay the message
+    // If there is a request cookie, attempt to relay the message.
     if (m->has_requestcookie())
     {
         if (auto peer = overlay_.findPeerByShortID(m->requestcookie()))
         {
             m->clear_requestcookie();
+
+            // If the original requester doesn't support the new depth-based format, rewrite any
+            // nodes that use it back to the legacy nodeid format before relaying. Once all nodes
+            // have upgraded, the old protocol version and this code can be removed. Make sure that
+            // the format of the nodes is consistent - either all use the legacy format or the new
+            // format, unless it is liBASE data in which case none of these fields should be set.
+            auto const peerSupportsNodeDepth =
+                peer->supportsFeature(ProtocolFeature::LedgerNodeDepth);
+            enum class MessageType { Unknown, Base, Legacy, Depth };
+            MessageType messageType = MessageType::Unknown;
+            for (int i = 0; i < m->nodes_size(); ++i)
+            {
+                auto* ledgerNode = m->mutable_nodes(i);
+
+                MessageType msgType = MessageType::Unknown;
+                if (m->type() == protocol::liBASE)
+                {
+                    if (ledgerNode->has_nodeid() || ledgerNode->has_id() || ledgerNode->has_depth())
+                    {
+                        badData(
+                            "Received liBASE message with node reference while relaying ledger "
+                            "data for " +
+                            to_string(uint256::fromRaw(m->ledgerhash())) + " to peer " +
+                            std::to_string(peer->id()));
+                        return;
+                    }
+                    msgType = MessageType::Base;
+                }
+                else
+                {
+                    msgType = ledgerNode->has_nodeid() ? MessageType::Legacy : MessageType::Depth;
+                }
+                if (messageType != MessageType::Unknown && messageType != msgType)
+                {
+                    badData(
+                        "Received mixed mode message while relaying ledger data for " +
+                        to_string(uint256::fromRaw(m->ledgerhash())) + " to peer " +
+                        std::to_string(peer->id()));
+                    return;
+                }
+                messageType = msgType;
+
+                if (peerSupportsNodeDepth || msgType != MessageType::Depth)
+                    continue;
+
+                switch (ledgerNode->reference_case())
+                {
+                    case protocol::TMLedgerNode::kId: {
+                        // We can directly copy the `id` field, because it uses the same wire format
+                        // as the legacy `nodeid` field.
+                        ledgerNode->set_nodeid(ledgerNode->id());
+                        ledgerNode->clear_id();
+                        break;
+                    }
+                    case protocol::TMLedgerNode::kDepth: {
+                        // We need to regenerate the node ID from the node data and depth.
+                        auto treeNode = getTreeNode(ledgerNode->nodedata());
+                        if (!treeNode)
+                        {
+                            badData(
+                                "Unable to get tree node while relaying ledger data for " +
+                                to_string(uint256::fromRaw(m->ledgerhash())) + " to peer " +
+                                std::to_string(peer->id()));
+                            return;
+                        }
+
+                        auto const nodeID = getSHAMapNodeID(*ledgerNode, *treeNode);
+                        if (!nodeID)
+                        {
+                            badData(
+                                "Unable to get node ID while relaying ledger data for " +
+                                to_string(uint256::fromRaw(m->ledgerhash())) + " to peer " +
+                                std::to_string(peer->id()));
+                            return;
+                        }
+
+                        ledgerNode->set_nodeid(nodeID->getRawString());
+                        ledgerNode->clear_depth();
+                        break;
+                    }
+                    default: {
+                        badData(
+                            "Empty node reference while relaying ledger data for " +
+                            to_string(uint256::fromRaw(m->ledgerhash())) + " to peer " +
+                            std::to_string(peer->id()));
+                        return;
+                    }
+                }
+            }
+
             peer->send(std::make_shared<Message>(*m, protocol::mtLEDGER_DATA));
         }
         else
@@ -3287,12 +3416,10 @@ PeerImp::getTxSet(std::shared_ptr<protocol::TMGetLedger> const& m) const
 }
 
 void
-PeerImp::processLedgerRequest(std::shared_ptr<protocol::TMGetLedger> const& m)
+PeerImp::processLedgerRequest(
+    std::shared_ptr<protocol::TMGetLedger> const& m,
+    std::vector<SHAMapNodeID> nodeIDs)
 {
-    // Do not resource charge a peer responding to a relay
-    if (!m->has_requestcookie())
-        charge(Resource::kFeeModerateBurdenPeer, "received a get ledger request");
-
     std::shared_ptr<Ledger const> ledger;
     std::shared_ptr<SHAMap const> sharedMap;
     SHAMap const* map{nullptr};
@@ -3372,26 +3499,25 @@ PeerImp::processLedgerRequest(std::shared_ptr<protocol::TMGetLedger> const& m)
     }
 
     // Add requested node data to reply
-    if (m->nodeids_size() > 0)
+    if (!nodeIDs.empty())
     {
         std::uint32_t const defaultDepth = isHighLatency() ? 2 : 1;
         auto const queryDepth{m->has_querydepth() ? m->querydepth() : defaultDepth};
 
-        std::vector<std::pair<SHAMapNodeID, Blob>> data;
+        std::vector<SHAMapNodeData> data;
+        data.reserve(Tuning::kSoftMaxReplyNodes);
+        auto const useLedgerNodeDepth = supportsFeature(ProtocolFeature::LedgerNodeDepth);
 
-        for (int i = 0;
-             i < m->nodeids_size() && ledgerData.nodes_size() < Tuning::kSoftMaxReplyNodes;
-             ++i)
+        for (auto const& nodeID : nodeIDs)
         {
-            auto const shaMapNodeId{deserializeSHAMapNodeID(m->nodeids(i))};
+            if (ledgerData.nodes_size() >= Tuning::kSoftMaxReplyNodes)
+                break;
 
             data.clear();
-            data.reserve(Tuning::kSoftMaxReplyNodes);
 
             try
             {
-                // NOLINTNEXTLINE(bugprone-unchecked-optional-access) nodeids checked in onGetLedger
-                if (map->getNodeFat(*shaMapNodeId, data, fatLeaves, queryDepth))
+                if (map->getNodeFat(nodeID, data, fatLeaves, queryDepth))
                 {
                     JLOG(pJournal_.trace())
                         << "processLedgerRequest: getNodeFat got " << data.size() << " nodes";
@@ -3400,9 +3526,25 @@ PeerImp::processLedgerRequest(std::shared_ptr<protocol::TMGetLedger> const& m)
                     {
                         if (ledgerData.nodes_size() >= Tuning::kHardMaxReplyNodes)
                             break;
+
                         protocol::TMLedgerNode* node{ledgerData.add_nodes()};
-                        node->set_nodeid(d.first.getRawString());
-                        node->set_nodedata(d.second.data(), d.second.size());
+                        node->set_nodedata(d.data.data(), d.data.size());
+
+                        // When the LedgerNodeDepth protocol feature is not supported by the peer,
+                        // we always set the `nodeid` field. However, when it is supported then we
+                        // set the `id` field for inner nodes and the `depth` field for leaf nodes.
+                        if (!useLedgerNodeDepth)
+                        {
+                            node->set_nodeid(d.nodeID.getRawString());
+                        }
+                        else if (d.isLeaf)
+                        {
+                            node->set_depth(d.nodeID.getDepth());
+                        }
+                        else
+                        {
+                            node->set_id(d.nodeID.getRawString());
+                        }
                     }
                 }
                 else
@@ -3441,13 +3583,13 @@ PeerImp::processLedgerRequest(std::shared_ptr<protocol::TMGetLedger> const& m)
                     info += ", no hash specified";
 
                 JLOG(pJournal_.warn())
-                    << "processLedgerRequest: getNodeFat with nodeId " << *shaMapNodeId
+                    << "processLedgerRequest: getNodeFat with nodeId " << nodeID
                     << " and ledger info type " << info << " throws exception: " << e.what();
             }
         }
 
         JLOG(pJournal_.info()) << "processLedgerRequest: Got request for " << m->nodeids_size()
-                               << " nodes at depth " << queryDepth << ", return "
+                               << " node IDs at depth " << queryDepth << ", return "
                                << ledgerData.nodes_size() << " nodes";
     }
 

src/xrpld/overlay/detail/PeerImp.h

@@ -32,6 +32,7 @@
 #include <xrpl/resource/Consumer.h>
 #include <xrpl/resource/Fees.h>
 #include <xrpl/server/Handoff.h>
+#include <xrpl/shamap/SHAMapNodeID.h>
 
 #include <boost/circular_buffer.hpp>
 #include <boost/endian/conversion.hpp>
@@ -679,7 +680,9 @@ private:
     getTxSet(std::shared_ptr<protocol::TMGetLedger> const& m) const;
 
     void
-    processLedgerRequest(std::shared_ptr<protocol::TMGetLedger> const& m);
+    processLedgerRequest(
+        std::shared_ptr<protocol::TMGetLedger> const& m,
+        std::vector<SHAMapNodeID> nodeIDs);
 
 protected:
     // Kept `protected` so test subclasses (see

src/xrpld/overlay/detail/ProtocolVersion.cpp

@@ -29,6 +29,7 @@ namespace xrpl {
 constexpr ProtocolVersion const kSupportedProtocolList[]{
     {2, 1},
     {2, 2},
+    {2, 3},
 };
 
 // This ugly construct ensures that supportedProtocolList is sorted in strictly