Remove depth, do not include node ID for leaf nodes

.github/scripts/levelization/results/loops.txt

@@ -5,7 +5,7 @@ Loop: test.jtx test.unit_test
   test.unit_test == test.jtx
 
 Loop: xrpld.app xrpld.overlay
-  xrpld.overlay ~= xrpld.app
+  xrpld.overlay == xrpld.app
 
 Loop: xrpld.app xrpld.peerfinder
   xrpld.peerfinder == xrpld.app

include/xrpl/protocol/detail/features.macro

@@ -15,6 +15,7 @@
 
 // Add new amendments to the top of this list.
 // Keep it sorted in reverse chronological order.
+XRPL_FIX    (LedgerNodeID,               Supported::yes, VoteBehavior::DefaultYes)
 XRPL_FIX   (PermissionedDomainInvariant, Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FIX    (ExpiredNFTokenOfferRemoval, Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FIX    (BatchInnerSigs,             Supported::yes, VoteBehavior::DefaultNo)

src/libxrpl/shamap/SHAMapNodeID.cpp

@@ -113,7 +113,7 @@ 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::leafDepth, "xrpl::SHAMapNodeID::createID : valid branch input");
     return SHAMapNodeID(depth, key & depthMask(depth));
 }
 

src/libxrpl/shamap/SHAMapSync.cpp

@@ -547,25 +547,6 @@ SHAMap::addKnownNode(SHAMapNodeID const& node, Slice const& rawNode, SHAMapSyncF
             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 = static_cast<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:

src/test/app/TxQ_test.cpp

@@ -1010,7 +1010,7 @@ public:
         // Charlie - queue a transaction, with a higher fee
         // than default
         env(noop(charlie), fee(15), queued);
-        checkMetrics(*this, env, 6, initQueueMax, 4, 3, 257);
+        checkMetrics(*this, env, 6, initQueueMax, 4, 3, 256);
 
         BEAST_EXPECT(env.seq(alice) == aliceSeq);
         BEAST_EXPECT(env.seq(bob) == bobSeq);

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

@@ -3,6 +3,7 @@
 #include <xrpld/app/ledger/InboundLedgers.h>
 #include <xrpld/app/ledger/LedgerMaster.h>
 #include <xrpld/app/ledger/TransactionStateSF.h>
+#include <xrpld/app/ledger/detail/LedgerNodeHelpers.h>
 #include <xrpld/app/main/Application.h>
 #include <xrpld/overlay/Overlay.h>
 
@@ -819,21 +820,36 @@ InboundLedger::receiveNode(protocol::TMLedgerData& packet, SHAMapAddNode& san)
     {
         auto const f = filter.get();
 
-        for (auto const& node : packet.nodes())
+        for (auto const& ledger_node : packet.nodes())
         {
-            auto const nodeID = deserializeSHAMapNodeID(node.nodeid());
-
-            if (!nodeID)
-                throw std::runtime_error("data does not properly deserialize");
-
-            if (nodeID->isRoot())
+            if (!validateLedgerNode(app_, ledger_node))
             {
-                san += map.addRootNode(rootHash, makeSlice(node.nodedata()), f);
+                JLOG(journal_.warn()) << "Got malformed ledger node";
+                san.incInvalid();
+                return;
             }
+
+            auto const node_slice = makeSlice(ledger_node.nodedata());
+            auto const tree_node = getTreeNode(node_slice);
+            if (!tree_node)
+            {
+                JLOG(journal_.warn()) << "Got invalid node data";
+                san.incInvalid();
+                return;
+            }
+
+            auto const node_id = getSHAMapNodeID(app_, ledger_node, *tree_node);
+            if (!node_id)
+            {
+                JLOG(journal_.warn()) << "Got invalid node id";
+                san.incInvalid();
+                return;
+            }
+
+            if (node_id->isRoot())
+                san += map.addRootNode(rootHash, node_slice, f);
             else
-            {
-                san += map.addKnownNode(*nodeID, makeSlice(node.nodedata()), f);
-            }
+                san += map.addKnownNode(*node_id, node_slice, f);
 
             if (!san.isGood())
             {
@@ -1044,13 +1060,13 @@ InboundLedger::processData(std::shared_ptr<Peer> peer, protocol::TMLedgerData& p
 
         ScopedLockType sl(mtx_);
 
-        // Verify node IDs and data are complete
-        for (auto const& node : packet.nodes())
+        // Verify nodes are complete
+        for (auto const& ledger_node : packet.nodes())
         {
-            if (!node.has_nodeid() || !node.has_nodedata())
+            if (!validateLedgerNode(app_, ledger_node))
             {
-                JLOG(journal_.warn()) << "Got bad node";
-                peer->charge(Resource::feeMalformedRequest, "ledger_data bad node");
+                JLOG(journal_.warn()) << "Got malformed ledger node";
+                peer->charge(Resource::feeMalformedRequest, "ledger_node");
                 return -1;
             }
         }

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

@@ -1,5 +1,6 @@
 #include <xrpld/app/ledger/InboundLedgers.h>
 #include <xrpld/app/ledger/LedgerMaster.h>
+#include <xrpld/app/ledger/detail/LedgerNodeHelpers.h>
 #include <xrpld/app/main/Application.h>
 
 #include <xrpl/basics/DecayingSample.h>
@@ -215,23 +216,22 @@ public:
         Serializer s;
         try
         {
-            for (int i = 0; i < packet_ptr->nodes().size(); ++i)
+            for (auto const& ledger_node : packet_ptr->nodes())
             {
-                auto const& node = packet_ptr->nodes(i);
-
-                if (!node.has_nodeid() || !node.has_nodedata())
+                if (!validateLedgerNode(app_, ledger_node))
                     return;
 
-                auto newNode = SHAMapTreeNode::makeFromWire(makeSlice(node.nodedata()));
-
-                if (!newNode)
+                auto const node_slice = makeSlice(ledger_node.nodedata());
+                auto const tree_node = getTreeNode(node_slice);
+                if (!tree_node)
                     return;
+                auto const& tn = *tree_node;
 
                 s.erase();
-                newNode->serializeWithPrefix(s);
+                tn->serializeWithPrefix(s);
 
                 app_.getLedgerMaster().addFetchPack(
-                    newNode->getHash().as_uint256(), std::make_shared<Blob>(s.begin(), s.end()));
+                    tn->getHash().as_uint256(), std::make_shared<Blob>(s.begin(), s.end()));
             }
         }
         catch (std::exception const&)

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

@@ -1,5 +1,6 @@
 #include <xrpld/app/ledger/InboundLedgers.h>
 #include <xrpld/app/ledger/InboundTransactions.h>
+#include <xrpld/app/ledger/detail/LedgerNodeHelpers.h>
 #include <xrpld/app/ledger/detail/TransactionAcquire.h>
 #include <xrpld/app/main/Application.h>
 
@@ -130,23 +131,33 @@ public:
         std::vector<std::pair<SHAMapNodeID, Slice>> data;
         data.reserve(packet.nodes().size());
 
-        for (auto const& node : packet.nodes())
+        for (auto const& ledger_node : packet.nodes())
         {
-            if (!node.has_nodeid() || !node.has_nodedata())
+            if (!validateLedgerNode(app_, ledger_node))
             {
-                peer->charge(Resource::feeMalformedRequest, "ledger_data");
+                JLOG(j_.warn()) << "Got malformed ledger node";
+                peer->charge(Resource::feeMalformedRequest, "ledger_node");
                 return;
             }
 
-            auto const id = deserializeSHAMapNodeID(node.nodeid());
-
-            if (!id)
+            auto const node_slice = makeSlice(ledger_node.nodedata());
+            auto const tree_node = getTreeNode(node_slice);
+            if (!tree_node)
             {
-                peer->charge(Resource::feeInvalidData, "ledger_data");
+                JLOG(j_.warn()) << "Got invalid node data";
+                peer->charge(Resource::feeInvalidData, "node_data");
                 return;
             }
 
-            data.emplace_back(std::make_pair(*id, makeSlice(node.nodedata())));
+            auto const node_id = getSHAMapNodeID(app_, ledger_node, *tree_node);
+            if (!node_id)
+            {
+                JLOG(j_.warn()) << "Got invalid node id";
+                peer->charge(Resource::feeInvalidData, "node_id");
+                return;
+            }
+
+            data.emplace_back(std::make_pair(*node_id, node_slice));
         }
 
         if (!ta->takeNodes(data, peer).isUseful())

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

@@ -0,0 +1,98 @@
+#pragma once
+
+#include <xrpld/app/main/Application.h>
+#include <xrpld/app/misc/AmendmentTable.h>
+
+#include <xrpl/basics/IntrusivePointer.h>
+#include <xrpl/beast/utility/instrumentation.h>
+#include <xrpl/shamap/SHAMapLeafNode.h>
+#include <xrpl/shamap/SHAMapNodeID.h>
+#include <xrpl/shamap/SHAMapTreeNode.h>
+
+namespace xrpl {
+
+inline bool
+validateLedgerNode(Application& app, protocol::TMLedgerNode const& ledger_node)
+{
+    if (!ledger_node.has_nodedata())
+        return false;
+
+    // When the amendment is enabled, we expect the node ID to be present in the ledger node for
+    // inner nodes, while we expect it to not be present in the ledger node for leaf nodes. However,
+    // at this point we don't yet know whether the node is an inner node or a leaf node, so we
+    // allow both cases.
+    if (app.getAmendmentTable().isEnabled(fixLedgerNodeID))
+        return true;
+
+    // When the amendment is disabled, we expect the node ID to always be present.
+    return ledger_node.has_nodeid();
+}
+
+inline std::optional<intr_ptr::SharedPtr<SHAMapTreeNode>>
+getTreeNode(Slice const& node_slice)
+{
+    try
+    {
+        return SHAMapTreeNode::makeFromWire(node_slice);
+    }
+    catch (...)
+    {
+        return std::nullopt;
+    }
+}
+
+inline std::optional<SHAMapNodeID>
+getSHAMapNodeID(
+    Application& app,
+    protocol::TMLedgerNode const& ledger_node,
+    intr_ptr::SharedPtr<SHAMapTreeNode> const& tree_node)
+{
+    // When the amendment is enabled, we can get the node ID directly from the ledger node for inner
+    // nodes, while we compute it for leaf nodes.
+    if (app.getAmendmentTable().isEnabled(fixLedgerNodeID))
+    {
+        if (tree_node->isInner())
+        {
+            XRPL_ASSERT(ledger_node.has_nodeid(), "xrpl::getSHAMapNodeID : node ID is present");
+            if (!ledger_node.has_nodeid())
+                return std::nullopt;
+
+            return deserializeSHAMapNodeID(ledger_node.nodeid());
+        }
+
+        if (tree_node->isLeaf())
+        {
+            XRPL_ASSERT(!ledger_node.has_nodeid(), "xrpl::getSHAMapNodeID : node ID is not present");
+            if (ledger_node.has_nodeid())
+                return std::nullopt;
+
+            auto const key = static_cast<SHAMapLeafNode const*>(tree_node.get())->peekItem()->key();
+            return SHAMapNodeID::createID(SHAMap::leafDepth, key);
+        }
+
+        return std::nullopt;
+    }
+
+    // When the amendment is disabled, we expect the node ID to always be present. For leaf nodes
+    // we perform an extra check to ensure the node's position in the tree is consistent with its
+    // content.
+    XRPL_ASSERT(ledger_node.has_nodeid(), "xrpl::getSHAMapNodeID : node ID is present");
+    if (!ledger_node.has_nodeid())
+        return std::nullopt;
+
+    auto const node_id = deserializeSHAMapNodeID(ledger_node.nodeid());
+    if (!node_id)
+        return std::nullopt;
+
+    if (tree_node->isLeaf())
+    {
+        auto const key = static_cast<SHAMapLeafNode const*>(tree_node.get())->peekItem()->key();
+        auto const expected_id = SHAMapNodeID::createID(static_cast<int>(node_id->getDepth()), key);
+        if (node_id->getNodeID() != expected_id.getNodeID())
+            return std::nullopt;
+    }
+
+    return node_id;
+}
+
+}  // namespace xrpl

src/xrpld/overlay/detail/PeerImp.cpp

@@ -3,6 +3,7 @@
 #include <xrpld/app/ledger/InboundTransactions.h>
 #include <xrpld/app/ledger/LedgerMaster.h>
 #include <xrpld/app/ledger/TransactionMaster.h>
+#include <xrpld/app/misc/AmendmentTable.h>
 #include <xrpld/app/misc/LoadFeeTrack.h>
 #include <xrpld/app/misc/Transaction.h>
 #include <xrpld/app/misc/ValidatorList.h>
@@ -3152,8 +3153,14 @@ PeerImp::processLedgerRequest(std::shared_ptr<protocol::TMGetLedger> const& m)
                         if (ledgerData.nodes_size() >= Tuning::hardMaxReplyNodes)
                             break;
                         protocol::TMLedgerNode* node{ledgerData.add_nodes()};
-                        node->set_nodeid(d.first.getRawString());
                         node->set_nodedata(d.second.data(), d.second.size());
+                        // When the amendment is enabled, we only include the node ID in the ledger node for inner
+                        // nodes, while we do not include it for leaf nodes as it can be calculated from the data. When
+                        // the amendment is disabled, we always need to include the node ID in the ledger node.
+                        if ((app_.getAmendmentTable().isEnabled(fixLedgerNodeID) &&
+                             d.first.getDepth() != SHAMap::leafDepth) ||
+                            !app_.getAmendmentTable().isEnabled(fixLedgerNodeID))
+                            node->set_nodeid(d.first.getRawString());
                     }
                 }
                 else