diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h index 0a04ca554b..72e29472bf 100644 --- a/include/xrpl/shamap/SHAMap.h +++ b/include/xrpl/shamap/SHAMap.h @@ -81,8 +81,8 @@ enum class SHAMapState { struct SHAMapNodeData { SHAMapNodeID nodeID; - Blob data; bool isLeaf; + Blob data; // Placed last, so `isLeaf` can fit into the alignment padding of `nodeID`. }; class SHAMap @@ -320,8 +320,9 @@ public: * @param filter Optional sync filter to track received nodes. * @return Status indicating whether the node was useful, duplicate, or invalid. * - * @note This function expects that the caller has already validated that the nodeID is - * consistent with the node's content. + * @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( diff --git a/src/libxrpl/shamap/SHAMapNodeID.cpp b/src/libxrpl/shamap/SHAMapNodeID.cpp index 016747d76e..a511fc038c 100644 --- a/src/libxrpl/shamap/SHAMapNodeID.cpp +++ b/src/libxrpl/shamap/SHAMapNodeID.cpp @@ -130,8 +130,7 @@ SHAMapNodeID SHAMapNodeID::createID(int depth, uint256 const& key) { XRPL_ASSERT( - depth >= 0 && depth <= SHAMap::kLeafDepth, - "xrpl::SHAMapNodeID::createID : valid branch input"); + depth >= 0 && depth <= SHAMap::kLeafDepth, "xrpl::SHAMapNodeID::createID : valid depth"); return SHAMapNodeID(depth, key & depthMask(depth)); } diff --git a/src/libxrpl/shamap/SHAMapSync.cpp b/src/libxrpl/shamap/SHAMapSync.cpp index a76db3adda..cad5e0eb60 100644 --- a/src/libxrpl/shamap/SHAMapSync.cpp +++ b/src/libxrpl/shamap/SHAMapSync.cpp @@ -449,17 +449,16 @@ SHAMap::getNodeFat( std::stack> stack; stack.emplace(node, nodeID, depth); - Serializer s(8192); - while (!stack.empty()) { std::tie(node, nodeID, depth) = stack.top(); stack.pop(); - // Add this node to the reply - s.erase(); + // Use a fresh Serializer per node and move its buffer into `data` rather than copying it + // via Serializer::getData(): the move is O(1) whereas the copy was O(node size). + Serializer s(256); node->serializeForWire(s); - data.emplace_back(nodeID, s.getData(), node->isLeaf()); + data.emplace_back(nodeID, node->isLeaf(), std::move(s.modData())); if (node->isInner()) { @@ -487,9 +486,10 @@ SHAMap::getNodeFat( else if (childNode->isInner() || fatLeaves) { // Just include this node - s.erase(); - childNode->serializeForWire(s); - data.emplace_back(childID, s.getData(), childNode->isLeaf()); + Serializer cs(256); + childNode->serializeForWire(cs); + data.emplace_back( + childID, childNode->isLeaf(), std::move(cs.modData())); } } } @@ -518,8 +518,8 @@ SHAMap::addRootNode( // 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(); } @@ -554,8 +554,15 @@ SHAMap::addKnownNode( SHAMapTreeNodePtr treeNode, SHAMapSyncFilter const* filter) { - XRPL_ASSERT(!nodeID.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(), + safeDowncast(treeNode.get())->peekItem()->key()) + .getNodeID() == nodeID.getNodeID(), + "xrpl::SHAMap::addKnownNode : leaf position consistent with node ID"); if (!isSynching()) { diff --git a/src/test/app/LedgerNodeHelpers_test.cpp b/src/test/app/LedgerNodeHelpers_test.cpp index 6e4331ee42..4255a1b156 100644 --- a/src/test/app/LedgerNodeHelpers_test.cpp +++ b/src/test/app/LedgerNodeHelpers_test.cpp @@ -1,4 +1,4 @@ -#include +#include #include #include @@ -41,131 +41,6 @@ class LedgerNodeHelpers_test : public beast::unit_test::Suite return std::string(std::bit_cast(slice.data()), slice.size()); } - void - testValidateLedgerNode() - { - testcase("validateLedgerNode"); - - auto const validID = SHAMapNodeID::createID(3, uint256{}).getRawString(); - - // Invalid: missing all fields. - { - protocol::TMLedgerNode const node; - BEAST_EXPECT(!validateLedgerNode(node)); - } - - // Invalid: missing `nodedata` field. - { - protocol::TMLedgerNode node; - node.set_nodeid(validID); - BEAST_EXPECT(!validateLedgerNode(node)); - } - - // Invalid: missing `nodedata` field. - { - protocol::TMLedgerNode node; - node.set_id(validID); - BEAST_EXPECT(!validateLedgerNode(node)); - } - - // Invalid: missing `nodedata` field. - { - protocol::TMLedgerNode node; - node.set_depth(1); - BEAST_EXPECT(!validateLedgerNode(node)); - } - - // Valid: legacy `nodeid` field. - { - protocol::TMLedgerNode node; - node.set_nodedata("test_data"); - node.set_nodeid(validID); - BEAST_EXPECT(validateLedgerNode(node)); - } - - // Invalid: legacy `nodeid` field with garbage content. - { - protocol::TMLedgerNode node; - node.set_nodedata("test_data"); - node.set_nodeid("garbage"); - BEAST_EXPECT(!validateLedgerNode(node)); - } - - // Invalid: has both legacy `nodeid` and new `id` fields. - { - protocol::TMLedgerNode node; - node.set_nodedata("test_data"); - node.set_nodeid(validID); - node.set_id(validID); - BEAST_EXPECT(!validateLedgerNode(node)); - } - - // Invalid: has both legacy `nodeid` and new `depth` fields. - { - protocol::TMLedgerNode node; - node.set_nodedata("test_data"); - node.set_nodeid(validID); - node.set_depth(5); - BEAST_EXPECT(!validateLedgerNode(node)); - } - - // Valid: new `id` field. - { - protocol::TMLedgerNode node; - node.set_nodedata("test_data"); - node.set_id(validID); - BEAST_EXPECT(validateLedgerNode(node)); - } - - // Invalid: new `id` field with garbage content. - { - protocol::TMLedgerNode node; - node.set_nodedata("test_data"); - node.set_id("garbage"); - BEAST_EXPECT(!validateLedgerNode(node)); - } - - // Valid: new `depth` field. - { - protocol::TMLedgerNode node; - node.set_nodedata("test_data"); - node.set_depth(5); - BEAST_EXPECT(validateLedgerNode(node)); - } - - // Valid: `depth` at minimum depth. - { - protocol::TMLedgerNode node; - node.set_nodedata("test_data"); - node.set_depth(0); - BEAST_EXPECT(validateLedgerNode(node)); - } - - // Valid: `depth` at arbitrary depth between minimum and maximum. - { - protocol::TMLedgerNode node; - node.set_nodedata("test_data"); - node.set_depth(10); - BEAST_EXPECT(validateLedgerNode(node)); - } - - // Valid: `depth` at maximum depth. - { - protocol::TMLedgerNode node; - node.set_nodedata("test_data"); - node.set_depth(SHAMap::kLeafDepth); - BEAST_EXPECT(validateLedgerNode(node)); - } - - // Invalid: `depth` is greater than maximum depth. - { - protocol::TMLedgerNode node; - node.set_nodedata("test_data"); - node.set_depth(SHAMap::kLeafDepth + 1); - BEAST_EXPECT(!validateLedgerNode(node)); - } - } - void testGetTreeNode() { @@ -232,10 +107,10 @@ class LedgerNodeHelpers_test : public beast::unit_test::Suite auto const innerDepth = 3; auto const innerID = SHAMapNodeID::createID(innerDepth, uint256{}); - protocol::TMLedgerNode node; - node.set_nodedata(innerData); - node.set_nodeid(innerID.getRawString()); - auto const result = getSHAMapNodeID(node, *innerNode); + protocol::TMLedgerNode ledgerNode; + ledgerNode.set_nodedata(innerData); + ledgerNode.set_nodeid(innerID.getRawString()); + auto const result = getSHAMapNodeID(ledgerNode, *innerNode); BEAST_EXPECT(result == innerID); } @@ -244,19 +119,32 @@ class LedgerNodeHelpers_test : public beast::unit_test::Suite auto const innerDepth = 0; auto const innerID = SHAMapNodeID::createID(innerDepth, uint256{}); - protocol::TMLedgerNode node; - node.set_nodedata(innerData); - node.set_id(innerID.getRawString()); - auto const result = getSHAMapNodeID(node, *innerNode); + 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 node; - node.set_nodedata(innerData); - node.set_depth(10); - auto const result = getSHAMapNodeID(node, *innerNode); + 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); } } @@ -297,10 +185,10 @@ class LedgerNodeHelpers_test : public beast::unit_test::Suite auto const kLeafDepth = 0; auto const leafID = SHAMapNodeID::createID(kLeafDepth, leafKey); - protocol::TMLedgerNode node; - node.set_nodedata(leafData); - node.set_depth(kLeafDepth); - auto const result = getSHAMapNodeID(node, *leafNode); + protocol::TMLedgerNode ledgerNode; + ledgerNode.set_nodedata(leafData); + ledgerNode.set_depth(kLeafDepth); + auto const result = getSHAMapNodeID(ledgerNode, *leafNode); BEAST_EXPECT(result == leafID); } @@ -324,10 +212,10 @@ class LedgerNodeHelpers_test : public beast::unit_test::Suite auto const kLeafDepth = SHAMap::kLeafDepth; auto const leafID = SHAMapNodeID::createID(kLeafDepth, leafKey); - protocol::TMLedgerNode node; - node.set_nodedata(leafData); - node.set_depth(kLeafDepth); - auto const result = getSHAMapNodeID(node, *leafNode); + protocol::TMLedgerNode ledgerNode; + ledgerNode.set_nodedata(leafData); + ledgerNode.set_depth(kLeafDepth); + auto const result = getSHAMapNodeID(ledgerNode, *leafNode); BEAST_EXPECT(result == leafID); } @@ -348,13 +236,21 @@ class LedgerNodeHelpers_test : public beast::unit_test::Suite BEAST_EXPECT(!result); } } + + // Invalid: no field set. + { + auto const innerNode = intr_ptr::makeShared(1); + protocol::TMLedgerNode ledgerNode; + ledgerNode.set_nodedata("test_data"); + auto const result = getSHAMapNodeID(ledgerNode, *innerNode); + BEAST_EXPECT(!result); + } } public: void run() override { - testValidateLedgerNode(); testGetTreeNode(); testGetSHAMapNodeID(); } diff --git a/src/test/shamap/SHAMapSync_test.cpp b/src/test/shamap/SHAMapSync_test.cpp index d3d8e682c8..75952af0ce 100644 --- a/src/test/shamap/SHAMapSync_test.cpp +++ b/src/test/shamap/SHAMapSync_test.cpp @@ -124,6 +124,7 @@ public: auto node = SHAMapTreeNode::makeFromWire(makeSlice(a[0].data)); if (!node) fail("", __FILE__, __LINE__); + BEAST_EXPECT(a[0].isLeaf == node->isLeaf()); BEAST_EXPECT( destination.addRootNode(source.getHash(), std::move(node), nullptr).isGood()); } @@ -164,6 +165,8 @@ public: auto node = SHAMapTreeNode::makeFromWire(makeSlice(b[i].data)); if (!node) fail("", __FILE__, __LINE__); + if (b[i].isLeaf != node->isLeaf()) + fail("", __FILE__, __LINE__); if (!destination.addKnownNode(b[i].nodeID, std::move(node), nullptr).isUseful()) fail("", __FILE__, __LINE__); } diff --git a/src/xrpld/app/ledger/InboundLedger.h b/src/xrpld/app/ledger/InboundLedger.h index 7ad331b8b3..9a793f13d5 100644 --- a/src/xrpld/app/ledger/InboundLedger.h +++ b/src/xrpld/app/ledger/InboundLedger.h @@ -135,7 +135,10 @@ private: takeHeader(std::string_view data); void - receiveNode(protocol::TMLedgerData& packet, SHAMapAddNode& san); + receiveNode( + std::shared_ptr const& peer, + protocol::TMLedgerData& packet, + SHAMapAddNode& san); bool takeTxRootNode(std::string_view data, SHAMapAddNode& san); diff --git a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h b/src/xrpld/app/ledger/LedgerNodeHelpers.h similarity index 64% rename from src/xrpld/app/ledger/detail/LedgerNodeHelpers.h rename to src/xrpld/app/ledger/LedgerNodeHelpers.h index 7973ab2beb..2d80cca140 100644 --- a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h +++ b/src/xrpld/app/ledger/LedgerNodeHelpers.h @@ -13,24 +13,6 @@ class TMLedgerNode; namespace xrpl { -/** - * @brief Validates a ledger node proto message. - * - * This function checks whether a ledger node has the expected fields (for non-ledger base data): - * - The node must have `nodedata`. - * - If the legacy `nodeid` field is present then the new `id` and `depth` fields must not be - * present, and `nodeid` must be a valid serialized SHAMapNodeID. - * - If the new `id` or `depth` fields are present (it is a oneof field, so only one of the two can - * be set) then the legacy `nodeid` must not be present. - * - If the `id` field is present then it must be a valid serialized SHAMapNodeID. - * - If the `depth` field is present then it must be between 0 and SHAMap::kLeafDepth (inclusive). - * - * @param ledgerNode The ledger node to validate. - * @return true if the ledger node has the expected fields, false otherwise. - */ -[[nodiscard]] bool -validateLedgerNode(protocol::TMLedgerNode const& ledgerNode); - /** * @brief Deserializes a SHAMapTreeNode from wire format data. * @@ -64,8 +46,6 @@ getTreeNode(std::string_view 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. - * @note This function expects that the caller has already validated the ledger node by calling the - * `validateLedgerNode` function and obtained a valid tree node by calling `getTreeNode`. */ [[nodiscard]] std::optional getSHAMapNodeID(protocol::TMLedgerNode const& ledgerNode, SHAMapTreeNode const& treeNode); diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp index d84f0d370f..586ac8f58e 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp @@ -3,8 +3,8 @@ #include #include #include +#include #include -#include #include #include #include @@ -821,7 +821,10 @@ InboundLedger::takeHeader(std::string_view data) Call with a lock */ void -InboundLedger::receiveNode(protocol::TMLedgerData& packet, SHAMapAddNode& san) +InboundLedger::receiveNode( + std::shared_ptr const& peer, + protocol::TMLedgerData& packet, + SHAMapAddNode& san) { if (!haveHeader_) { @@ -870,6 +873,7 @@ InboundLedger::receiveNode(protocol::TMLedgerData& packet, SHAMapAddNode& san) if (!treeNode) { JLOG(journal_.warn()) << "Got invalid node data"; + peer->charge(Resource::kFeeInvalidData, "ledger_node.node_data invalid"); san.incInvalid(); return; } @@ -878,6 +882,7 @@ InboundLedger::receiveNode(protocol::TMLedgerData& packet, SHAMapAddNode& san) if (!nodeID) { JLOG(journal_.warn()) << "Got invalid node id"; + peer->charge(Resource::kFeeInvalidData, "ledger_node.node_id invalid"); san.incInvalid(); return; } @@ -893,14 +898,16 @@ InboundLedger::receiveNode(protocol::TMLedgerData& packet, SHAMapAddNode& san) if (!san.isGood()) { - JLOG(journal_.warn()) << "Received bad node data"; + JLOG(journal_.warn()) << "Got invalid node"; + 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: " << e.what(); san.incInvalid(); return; } @@ -1132,24 +1139,18 @@ InboundLedger::processData(std::shared_ptr peer, protocol::TMLedgerData& p ScopedLockType const sl(mtx_); - // Verify nodes are complete - for (auto const& ledgerNode : packet.nodes()) - { - if (!validateLedgerNode(ledgerNode)) - { - JLOG(journal_.warn()) << "Got malformed ledger node"; - peer->charge(Resource::kFeeMalformedRequest, "ledger_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(); + // Note: Peer charges for invalid/malformed data are issued from within receiveNode at the + // exact failure site, so the peer is only charged for problems they are responsible for. + if (san.isInvalid()) + return -1; + if (san.isUseful()) progress_ = true; diff --git a/src/xrpld/app/ledger/detail/InboundLedgers.cpp b/src/xrpld/app/ledger/detail/InboundLedgers.cpp index 2e710ac0bc..2bdaa104e9 100644 --- a/src/xrpld/app/ledger/detail/InboundLedgers.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedgers.cpp @@ -2,7 +2,7 @@ #include #include -#include +#include #include #include @@ -250,9 +250,6 @@ public: { for (auto const& ledgerNode : packetPtr->nodes()) { - if (!validateLedgerNode(ledgerNode)) - return; - auto const treeNode = getTreeNode(ledgerNode.nodedata()); if (!treeNode) return; diff --git a/src/xrpld/app/ledger/detail/InboundTransactions.cpp b/src/xrpld/app/ledger/detail/InboundTransactions.cpp index 91ceaf6eb0..f594a588fa 100644 --- a/src/xrpld/app/ledger/detail/InboundTransactions.cpp +++ b/src/xrpld/app/ledger/detail/InboundTransactions.cpp @@ -1,6 +1,6 @@ #include -#include +#include #include #include #include @@ -137,7 +137,7 @@ public: if (ta == nullptr) { - peer->charge(Resource::kFeeUselessData, "ledger_data"); + peer->charge(Resource::kFeeUselessData, "ledger_data useless"); return; } @@ -146,18 +146,11 @@ public: for (auto const& ledgerNode : packet.nodes()) { - if (!validateLedgerNode(ledgerNode)) - { - JLOG(j_.warn()) << "Got malformed ledger node"; - peer->charge(Resource::kFeeMalformedRequest, "ledger_node"); - return; - } - auto treeNode = getTreeNode(ledgerNode.nodedata()); if (!treeNode) { JLOG(j_.warn()) << "Got invalid node data"; - peer->charge(Resource::kFeeInvalidData, "node_data"); + peer->charge(Resource::kFeeInvalidData, "ledger_node.node_data invalid"); return; } @@ -165,15 +158,22 @@ public: if (!nodeID) { JLOG(j_.warn()) << "Got invalid node id"; - peer->charge(Resource::kFeeInvalidData, "node_id"); + peer->charge(Resource::kFeeInvalidData, "ledger_node.node_id invalid"); return; } data.emplace_back(*nodeID, std::move(treeNode)); } - if (!ta->takeNodes(std::move(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 diff --git a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp index 4bd1e871c7..62726d9088 100644 --- a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp +++ b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp @@ -1,4 +1,4 @@ -#include +#include #include #include @@ -16,24 +16,6 @@ namespace xrpl { -bool -validateLedgerNode(protocol::TMLedgerNode const& ledgerNode) -{ - if (!ledgerNode.has_nodedata()) - return false; - - if (ledgerNode.has_nodeid()) - { - return ledgerNode.reference_case() == ledgerNode.REFERENCE_NOT_SET && - deserializeSHAMapNodeID(ledgerNode.nodeid()).has_value(); - } - - if (ledgerNode.has_id()) - return deserializeSHAMapNodeID(ledgerNode.id()).has_value(); - - return ledgerNode.has_depth() && ledgerNode.depth() <= SHAMap::kLeafDepth; -} - SHAMapTreeNodePtr getTreeNode(std::string_view data) { @@ -53,6 +35,10 @@ getSHAMapNodeID(protocol::TMLedgerNode const& ledgerNode, SHAMapTreeNode const& { 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()) @@ -63,7 +49,7 @@ getSHAMapNodeID(protocol::TMLedgerNode const& ledgerNode, SHAMapTreeNode const& if (treeNode.isLeaf()) { - if (!ledgerNode.has_depth()) + if (!ledgerNode.has_depth() || ledgerNode.depth() > SHAMap::kLeafDepth) return std::nullopt; auto const key = safeDowncast(&treeNode)->peekItem()->key(); diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index ccf3603163..36991a65aa 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -1496,27 +1497,12 @@ PeerImp::onMessage(std::shared_ptr const& m) } } - // Verify and parse ledger node IDs - std::vector nodeIDs; - if (itype != protocol::liBASE) + // Cheap structural checks on node IDs; full parsing is deferred to the job + // so the IO thread is not burdened with SHAMapNodeID deserialization. + if (itype != protocol::liBASE && m->nodeids_size() <= 0) { - if (m->nodeids_size() <= 0) - { - badData("Invalid ledger node IDs"); - return; - } - - nodeIDs.reserve(m->nodeids_size()); - for (auto const& nodeId : m->nodeids()) - { - auto parsed = deserializeSHAMapNodeID(nodeId); - if (!parsed) - { - badData("Invalid SHAMap node ID"); - return; - } - nodeIDs.push_back(std::move(*parsed)); - } + badData("Invalid ledger node IDs"); + return; } // Verify query type @@ -1536,13 +1522,33 @@ PeerImp::onMessage(std::shared_ptr const& m) } } - // Queue a job to process the request + // Queue a job to process the request. Full parsing of the node IDs is + // performed inside the job so the IO thread is not burdened with + // SHAMapNodeID deserialization for every TMGetLedger. std::weak_ptr const weak = shared_from_this(); - app_.getJobQueue().addJob( - JtLedgerReq, "RcvGetLedger", [weak, m, nodeIDs = std::move(nodeIDs)]() mutable { - if (auto peer = weak.lock()) - peer->processLedgerRequest(m, std::move(nodeIDs)); - }); + app_.getJobQueue().addJob(JtLedgerReq, "RcvGetLedger", [weak, m, itype]() { + auto peer = weak.lock(); + if (!peer) + return; + + std::vector nodeIDs; + if (itype != protocol::liBASE) + { + nodeIDs.reserve(m->nodeids_size()); + for (auto const& nodeId : m->nodeids()) + { + auto parsed = deserializeSHAMapNodeID(nodeId); + if (!parsed) + { + peer->charge(Resource::kFeeInvalidData, "get_ledger invalid node ID"); + return; + } + nodeIDs.push_back(std::move(*parsed)); + } + } + + peer->processLedgerRequest(m, std::move(nodeIDs)); + }); } void @@ -1706,12 +1712,44 @@ PeerImp::onMessage(std::shared_ptr 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. + if (!peer->supportsFeature(ProtocolFeature::LedgerNodeDepth)) + { + for (int i = 0; i < m->nodes_size(); ++i) + { + auto* ledgerNode = m->mutable_nodes(i); + if (ledgerNode->reference_case() != ledgerNode->REFERENCE_NOT_SET) + { + auto treeNode = getTreeNode(ledgerNode->nodedata()); + if (!treeNode) + { + JLOG(pJournal_.warn()) << "Unable to get tree node"; + return; + } + + auto const nodeID = getSHAMapNodeID(*ledgerNode, *treeNode); + if (!nodeID) + { + JLOG(pJournal_.warn()) << "Unable to get node ID"; + return; + } + + ledgerNode->set_nodeid(nodeID->getRawString()); + ledgerNode->clear_id(); + ledgerNode->clear_depth(); + } + } + } + peer->send(std::make_shared(*m, protocol::mtLEDGER_DATA)); } else @@ -3343,6 +3381,7 @@ PeerImp::processLedgerRequest( auto const queryDepth{m->has_querydepth() ? m->querydepth() : defaultDepth}; std::vector data; + data.reserve(Tuning::kSoftMaxReplyNodes); auto const useLedgerNodeDepth = supportsFeature(ProtocolFeature::LedgerNodeDepth); for (auto const& nodeID : nodeIDs) @@ -3351,7 +3390,6 @@ PeerImp::processLedgerRequest( break; data.clear(); - data.reserve(Tuning::kSoftMaxReplyNodes); try {