From 2d78d41f7bfc6fc0077a11334744cc34d63b8533 Mon Sep 17 00:00:00 2001 From: Bart <11445373+bthomee@users.noreply.github.com> Date: Wed, 11 Feb 2026 15:55:16 -0500 Subject: [PATCH] perf: Replace node ID by depth in TMLedgerNode --- include/xrpl/proto/xrpl.proto | 3 +- include/xrpl/protocol/detail/features.macro | 1 + src/libxrpl/shamap/SHAMapSync.cpp | 19 ------ src/xrpld/app/ledger/detail/InboundLedger.cpp | 63 ++++++++++++++----- .../app/ledger/detail/InboundLedgers.cpp | 16 ++--- .../app/ledger/detail/InboundTransactions.cpp | 49 ++++++++++++--- src/xrpld/overlay/detail/PeerImp.cpp | 6 +- 7 files changed, 108 insertions(+), 49 deletions(-) diff --git a/include/xrpl/proto/xrpl.proto b/include/xrpl/proto/xrpl.proto index 0af7deb35d..6a807a4a54 100644 --- a/include/xrpl/proto/xrpl.proto +++ b/include/xrpl/proto/xrpl.proto @@ -244,7 +244,8 @@ message TMGetObjectByHash { message TMLedgerNode { required bytes nodedata = 1; - optional bytes nodeid = 2; // missing for ledger base data + optional bytes nodeid = 2 [deprecated = true]; + optional uint32 nodedepth = 3; // missing for ledger base data } enum TMLedgerInfoType { diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 961bc6e44c..ef8257df43 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/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 (LedgerNodeDepth, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (PermissionedDomainInvariant, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (ExpiredNFTokenOfferRemoval, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (BatchInnerSigs, Supported::yes, VoteBehavior::DefaultNo) diff --git a/src/libxrpl/shamap/SHAMapSync.cpp b/src/libxrpl/shamap/SHAMapSync.cpp index 503505b80c..35ff799287 100644 --- a/src/libxrpl/shamap/SHAMapSync.cpp +++ b/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(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: diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp index 3c8f61a915..5fa38faf0c 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include @@ -819,22 +820,54 @@ InboundLedger::receiveNode(protocol::TMLedgerData& packet, SHAMapAddNode& san) { auto const f = filter.get(); - for (auto const& node : packet.nodes()) + for (auto const& ledgerNode : packet.nodes()) { - auto const nodeID = deserializeSHAMapNodeID(node.nodeid()); - - if (!nodeID) - throw std::runtime_error("data does not properly deserialize"); - - if (nodeID->isRoot()) + auto nodeSlice = makeSlice(ledgerNode.nodedata()); + auto treeNode = SHAMapTreeNode::makeFromWire(nodeSlice); + if (!treeNode) { - san += map.addRootNode(rootHash, makeSlice(node.nodedata()), f); + JLOG(journal_.warn()) << "Got invalid node data"; + san.incInvalid(); + return; + } + auto const nodeKey = static_cast(treeNode.get())->peekItem()->key(); + + SHAMapNodeID nodeID; + if (app_.getAmendmentTable().isSupported(fixLedgerNodeDepth)) + { + nodeID = SHAMapNodeID::createID(ledgerNode.nodedepth(), nodeKey); } else { - san += map.addKnownNode(*nodeID, makeSlice(node.nodedata()), f); + auto const nid = deserializeSHAMapNodeID(ledgerNode.nodeid()); + if (!nid) + { + JLOG(journal_.warn()) << "Got invalid node id"; + san.incInvalid(); + return; + } + nodeID = *nid; + + // For leaf nodes, verify that the passed-in node ID is actually + // the same as what the node ID should be, given the position of + // the node in the SHAMap. + if (treeNode->isLeaf()) + { + auto const expectedID = SHAMapNodeID::createID(nodeID.getDepth(), nodeKey); + if (nodeID.getNodeID() != expectedID.getNodeID()) + { + JLOG(journal_.warn()) << "Got invalid node id"; + san.incInvalid(); + return; + } + } } + if (nodeID.isRoot()) + san += map.addRootNode(rootHash, nodeSlice, f); + else + san += map.addKnownNode(nodeID, nodeSlice, f); + if (!san.isGood()) { JLOG(journal_.warn()) << "Received bad node data"; @@ -1044,13 +1077,15 @@ InboundLedger::processData(std::shared_ptr 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& ledgerNode : packet.nodes()) { - if (!node.has_nodeid() || !node.has_nodedata()) + if (!ledgerNode.has_nodedata() || + (app_.getAmendmentTable().isSupported(fixLedgerNodeDepth) && !ledgerNode.has_nodedepth()) || + (!app_.getAmendmentTable().isSupported(fixLedgerNodeDepth) && !ledgerNode.has_nodeid())) { - 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; } } diff --git a/src/xrpld/app/ledger/detail/InboundLedgers.cpp b/src/xrpld/app/ledger/detail/InboundLedgers.cpp index 626f58b686..f19ef5db5d 100644 --- a/src/xrpld/app/ledger/detail/InboundLedgers.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedgers.cpp @@ -1,6 +1,7 @@ #include #include #include +#include #include #include @@ -217,21 +218,22 @@ public: { for (int i = 0; i < packet_ptr->nodes().size(); ++i) { - auto const& node = packet_ptr->nodes(i); + auto const& ledgerNode = packet_ptr->nodes(i); - if (!node.has_nodeid() || !node.has_nodedata()) + if (!ledgerNode.has_nodedata() || + (app_.getAmendmentTable().isSupported(fixLedgerNodeDepth) && !ledgerNode.has_nodedepth()) || + (!app_.getAmendmentTable().isSupported(fixLedgerNodeDepth) && !ledgerNode.has_nodeid())) return; - auto newNode = SHAMapTreeNode::makeFromWire(makeSlice(node.nodedata())); - - if (!newNode) + auto treeNode = SHAMapTreeNode::makeFromWire(makeSlice(ledgerNode.nodedata())); + if (!treeNode) return; s.erase(); - newNode->serializeWithPrefix(s); + treeNode->serializeWithPrefix(s); app_.getLedgerMaster().addFetchPack( - newNode->getHash().as_uint256(), std::make_shared(s.begin(), s.end())); + treeNode->getHash().as_uint256(), std::make_shared(s.begin(), s.end())); } } catch (std::exception const&) diff --git a/src/xrpld/app/ledger/detail/InboundTransactions.cpp b/src/xrpld/app/ledger/detail/InboundTransactions.cpp index 36ebb7bd9e..6be03e7ab4 100644 --- a/src/xrpld/app/ledger/detail/InboundTransactions.cpp +++ b/src/xrpld/app/ledger/detail/InboundTransactions.cpp @@ -2,6 +2,7 @@ #include #include #include +#include #include #include @@ -130,23 +131,57 @@ public: std::vector> 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()) + if (!ledgerNode.has_nodedata() || + (app_.getAmendmentTable().isSupported(fixLedgerNodeDepth) && !ledgerNode.has_nodedepth()) || + (!app_.getAmendmentTable().isSupported(fixLedgerNodeDepth) && !ledgerNode.has_nodeid())) { - 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 nodeSlice = makeSlice(ledgerNode.nodedata()); + auto treeNode = SHAMapTreeNode::makeFromWire(nodeSlice); + if (!treeNode) { + JLOG(j_.warn()) << "Got invalid node data"; peer->charge(Resource::feeInvalidData, "ledger_data"); return; } + auto const nodeKey = static_cast(treeNode.get())->peekItem()->key(); - data.emplace_back(std::make_pair(*id, makeSlice(node.nodedata()))); + SHAMapNodeID nodeID; + if (app_.getAmendmentTable().isSupported(fixLedgerNodeDepth)) + { + nodeID = SHAMapNodeID::createID(ledgerNode.nodedepth(), nodeKey); + } + else + { + auto const nid = deserializeSHAMapNodeID(ledgerNode.nodeid()); + if (!nid) + { + peer->charge(Resource::feeInvalidData, "ledger_id"); + return; + } + nodeID = *nid; + + // For leaf nodes, verify that the passed-in node ID is actually + // the same as what the node ID should be, given the position of + // the node in the SHAMap. + if (treeNode->isLeaf()) + { + auto const expectedID = SHAMapNodeID::createID(nodeID.getDepth(), nodeKey); + if (nodeID.getNodeID() != expectedID.getNodeID()) + { + peer->charge(Resource::feeInvalidData, "ledger_id"); + return; + } + } + } + + data.emplace_back(std::make_pair(nodeID, nodeSlice)); } if (!ta->takeNodes(data, peer).isUseful()) diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 0664f64256..ba26b45fa4 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -3152,8 +3153,11 @@ PeerImp::processLedgerRequest(std::shared_ptr 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()); + if (app_.getAmendmentTable().isSupported(fixLedgerNodeDepth)) + node->set_nodedepth(d.first.getDepth()); + else + node->set_nodeid(d.first.getRawString()); } } else