diff --git a/include/xrpl/basics/Slice.h b/include/xrpl/basics/Slice.h index 36e7615c3a..75c9b8c7bd 100644 --- a/include/xrpl/basics/Slice.h +++ b/include/xrpl/basics/Slice.h @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -251,4 +252,11 @@ makeSlice(std::basic_string const& s) return Slice(s.data(), s.size()); } +template +Slice +makeSlice(std::basic_string_view s) +{ + return Slice(s.data(), s.size()); +} + } // namespace xrpl diff --git a/include/xrpl/proto/xrpl.proto b/include/xrpl/proto/xrpl.proto index d49920201e..bef5ec1d76 100644 --- a/include/xrpl/proto/xrpl.proto +++ b/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 { diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h index a1194ccfd3..e198c472fa 100644 --- a/include/xrpl/shamap/SHAMap.h +++ b/include/xrpl/shamap/SHAMap.h @@ -3,7 +3,6 @@ #include #include #include -#include #include #include #include @@ -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> getMissingNodes(int maxNodes, SHAMapSyncFilter const* filter); - bool + [[nodiscard]] bool getNodeFat( SHAMapNodeID const& wanted, - std::vector>& data, + std::vector& 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 diff --git a/include/xrpl/shamap/SHAMapLeafNode.h b/include/xrpl/shamap/SHAMapLeafNode.h index 26cfde9fe8..ab5bd574ed 100644 --- a/include/xrpl/shamap/SHAMapLeafNode.h +++ b/include/xrpl/shamap/SHAMapLeafNode.h @@ -1,6 +1,9 @@ #pragma once #include +#include +#include +#include #include #include #include @@ -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(node).peekItem()->key(); +} + } // namespace xrpl diff --git a/src/libxrpl/shamap/SHAMapNodeID.cpp b/src/libxrpl/shamap/SHAMapNodeID.cpp index 16aaafe709..a511fc038c 100644 --- a/src/libxrpl/shamap/SHAMapNodeID.cpp +++ b/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)); } diff --git a/src/libxrpl/shamap/SHAMapSync.cpp b/src/libxrpl/shamap/SHAMapSync.cpp index cc30426f9d..cbed6885c9 100644 --- a/src/libxrpl/shamap/SHAMapSync.cpp +++ b/src/libxrpl/shamap/SHAMapSync.cpp @@ -107,7 +107,7 @@ SHAMap::visitNodes(std::function const& function) const void SHAMap::visitDifferences( - SHAMap const* have, + SHAMap const* map, std::function const& function) const { // Visit every node in this SHAMap that is not present @@ -118,13 +118,13 @@ SHAMap::visitDifferences( if (root_->getHash().isZero()) return; - if ((have != nullptr) && (root_->getHash() == have->root_->getHash())) + if ((map != nullptr) && (root_->getHash() == map->root_->getHash())) return; if (root_->isLeaf()) { auto leaf = intr_ptr::staticPointerCast(root_); - if ((have == nullptr) || !have->hasLeafNode(leaf->peekItem()->key(), leaf->getHash())) + if ((map == nullptr) || !map->hasLeafNode(leaf->peekItem()->key(), leaf->getHash())) function(*root_); return; } @@ -149,18 +149,15 @@ SHAMap::visitDifferences( if (!node->isEmptyBranch(i)) { auto const& childHash = node->getChildHash(i); - SHAMapNodeID const childID = nodeID.getChildNodeID(i); + auto const childID = nodeID.getChildNodeID(i); auto next = descendThrow(node, i); if (next->isInner()) { - if ((have == nullptr) || !have->hasInnerNode(childID, childHash)) + if ((map == nullptr) || !map->hasInnerNode(childID, childHash)) stack.emplace(safeDowncast(next), childID); } - else if ( - (have == nullptr) || - !have->hasLeafNode( - safeDowncast(next)->peekItem()->key(), childHash)) + else if ((map == nullptr) || !map->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>& data, + std::vector& 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(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(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(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(); diff --git a/src/test/app/LedgerNodeHelpers_test.cpp b/src/test/app/LedgerNodeHelpers_test.cpp new file mode 100644 index 0000000000..a9e4e3ebfc --- /dev/null +++ b/src/test/app/LedgerNodeHelpers_test.cpp @@ -0,0 +1,260 @@ +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#include + +#include +#include + +namespace xrpl::tests { + +class LedgerNodeHelpers_test : public beast::unit_test::Suite +{ + static boost::intrusive_ptr + 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(1); + auto const childNode = intr_ptr::makeShared(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(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(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(1); + auto const childNode = intr_ptr::makeShared(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(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(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(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 diff --git a/src/test/overlay/ProtocolVersion_test.cpp b/src/test/overlay/ProtocolVersion_test.cpp index 2fc8e4447d..e31a574502 100644 --- a/src/test/overlay/ProtocolVersion_test.cpp +++ b/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); } diff --git a/src/tests/libxrpl/shamap/SHAMapSync.cpp b/src/tests/libxrpl/shamap/SHAMapSync.cpp index 5cefbae8a1..400509d217 100644 --- a/src/tests/libxrpl/shamap/SHAMapSync.cpp +++ b/src/tests/libxrpl/shamap/SHAMapSync.cpp @@ -1,4 +1,3 @@ -#include #include #include #include @@ -112,14 +111,16 @@ TEST_F(SHAMapSyncTest, sync) destination.setSynching(); { - std::vector> a; + std::vector 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> b; + std::vector 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); diff --git a/src/xrpld/app/ledger/InboundLedger.h b/src/xrpld/app/ledger/InboundLedger.h index 31ca4169ce..9a7ee510f6 100644 --- a/src/xrpld/app/ledger/InboundLedger.h +++ b/src/xrpld/app/ledger/InboundLedger.h @@ -6,7 +6,6 @@ #include #include -#include #include #include #include @@ -24,7 +23,7 @@ #include #include #include -#include +#include #include #include @@ -154,16 +153,19 @@ private: processData(std::shared_ptr 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 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 neededTxHashes(int max, SHAMapSyncFilter const* filter) const; diff --git a/src/xrpld/app/ledger/LedgerNodeHelpers.h b/src/xrpld/app/ledger/LedgerNodeHelpers.h new file mode 100644 index 0000000000..9df9ab06c7 --- /dev/null +++ b/src/xrpld/app/ledger/LedgerNodeHelpers.h @@ -0,0 +1,52 @@ +#pragma once + +#include +#include + +#include +#include + +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 +getSHAMapNodeID(protocol::TMLedgerNode const& ledgerNode, SHAMapTreeNode const& treeNode); + +} // namespace xrpl diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp index 55a2a9d283..b3dafcf5e6 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -44,8 +45,8 @@ #include #include #include -#include #include +#include #include #include #include @@ -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 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 @@ -1065,20 +1103,33 @@ InboundLedger::processData(std::shared_ptr 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, 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; diff --git a/src/xrpld/app/ledger/detail/InboundLedgers.cpp b/src/xrpld/app/ledger/detail/InboundLedgers.cpp index dc361694cf..4d565ca674 100644 --- a/src/xrpld/app/ledger/detail/InboundLedgers.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedgers.cpp @@ -2,13 +2,13 @@ #include #include +#include #include #include #include #include #include -#include #include #include #include @@ -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(s.begin(), s.end())); + treeNode->getHash().asUInt256(), std::make_shared(s.begin(), s.end())); } } catch (std::exception const&) // NOLINT(bugprone-empty-catch) diff --git a/src/xrpld/app/ledger/detail/InboundTransactions.cpp b/src/xrpld/app/ledger/detail/InboundTransactions.cpp index 9b50a1584f..d735a97d28 100644 --- a/src/xrpld/app/ledger/detail/InboundTransactions.cpp +++ b/src/xrpld/app/ledger/detail/InboundTransactions.cpp @@ -1,11 +1,11 @@ #include +#include #include #include #include #include -#include #include #include #include @@ -14,6 +14,7 @@ #include #include #include +#include #include @@ -137,34 +138,45 @@ public: if (ta == nullptr) { - peer->charge(Resource::kFeeUselessData, "ledger_data"); + peer->charge(Resource::kFeeUselessData, "ledger_data useless"); return; } - std::vector> data; + 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()) + 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 diff --git a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp new file mode 100644 index 0000000000..531dba59f9 --- /dev/null +++ b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp @@ -0,0 +1,89 @@ +#include + +#include +#include +#include +#include +#include +#include + +#include + +#include +#include +#include + +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 +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; + + REACHABLE("xrpl::getSHAMapNodeID : inner node ID from id field"); + return deserializeSHAMapNodeID(ledgerNode.id()); + } + + if (treeNode.isLeaf()) + { + SOMETIMES( + ledgerNode.has_depth() && ledgerNode.depth() > SHAMap::kLeafDepth, + "xrpl::getSHAMapNodeID : leaf depth exceeds max"); + if (!ledgerNode.has_depth() || ledgerNode.depth() > SHAMap::kLeafDepth) + return std::nullopt; + + auto const key = leafKey(treeNode); + REACHABLE("xrpl::getSHAMapNodeID : leaf node ID reconstructed from depth"); + 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(nodeID->getDepth()), key); + SOMETIMES( + nodeID->getNodeID() != expectedID.getNodeID(), + "xrpl::getSHAMapNodeID : legacy leaf ID inconsistent with key"); + if (nodeID->getNodeID() != expectedID.getNodeID()) + return std::nullopt; + } + + return nodeID; +} + +} // namespace xrpl diff --git a/src/xrpld/app/ledger/detail/TransactionAcquire.cpp b/src/xrpld/app/ledger/detail/TransactionAcquire.cpp index 62312b04d2..db99299fd6 100644 --- a/src/xrpld/app/ledger/detail/TransactionAcquire.cpp +++ b/src/xrpld/app/ledger/detail/TransactionAcquire.cpp @@ -7,13 +7,13 @@ #include #include -#include #include #include #include #include #include #include +#include #include @@ -171,7 +171,7 @@ TransactionAcquire::trigger(std::shared_ptr const& peer) SHAMapAddNode TransactionAcquire::takeNodes( - std::vector> const& data, + std::vector> data, std::shared_ptr 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(); } } diff --git a/src/xrpld/app/ledger/detail/TransactionAcquire.h b/src/xrpld/app/ledger/detail/TransactionAcquire.h index 5b33066390..2faf74b557 100644 --- a/src/xrpld/app/ledger/detail/TransactionAcquire.h +++ b/src/xrpld/app/ledger/detail/TransactionAcquire.h @@ -6,10 +6,10 @@ #include #include -#include #include #include #include +#include #include #include @@ -32,8 +32,8 @@ public: SHAMapAddNode takeNodes( - std::vector> const& data, - std::shared_ptr const&); + std::vector> data, + std::shared_ptr const& peer); void init(int startPeers); diff --git a/src/xrpld/overlay/Peer.h b/src/xrpld/overlay/Peer.h index 23a45dc512..20a8730cf1 100644 --- a/src/xrpld/overlay/Peer.h +++ b/src/xrpld/overlay/Peer.h @@ -23,6 +23,7 @@ enum class ProtocolFeature { ValidatorListPropagation, ValidatorList2Propagation, LedgerReplay, + LedgerNodeDepth, }; /** diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index d7a9a9e449..688d0ac314 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 @@ -61,6 +62,7 @@ #include #include #include +#include #include #include @@ -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 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 const& m) } } - // Queue a job to process the request + // Queue a job to process the request. std::weak_ptr 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 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(nodeIDs.size()), + m->nodeids_size() - static_cast(nodeIDs.size())); + } + if (!m->has_requestcookie()) + { + peer->charge(Resource::kFeeModerateBurdenPeer, "TMGetLedger: get ledger request"); + } + + peer->processLedgerRequest(m, std::move(nodeIDs)); }); } @@ -1682,12 +1721,119 @@ 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. 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); + + // All nodes should have non-empty data. The field is required so we don't need to + // check for presence first. + if (ledgerNode->nodedata().empty()) + { + badData( + "Received node with empty data while relaying ledger data for " + + to_string(uint256::fromRaw(m->ledgerhash())) + " to peer " + + std::to_string(peer->id())); + return; + } + + 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; + + SOMETIMES( + !peerSupportsNodeDepth, + "xrpl::PeerImp : relaying depth-format ledger data to pre-2.3 peer"); + 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. + REACHABLE("xrpl::PeerImp : relay downgrade id to nodeid"); + 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; + } + + REACHABLE("xrpl::PeerImp : relay downgrade depth to nodeid"); + ledgerNode->set_nodeid(nodeID->getRawString()); + ledgerNode->clear_depth(); + break; + } + default: { + SOMETIMES(true, "xrpl::PeerImp : relay node has empty reference"); + 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(*m, protocol::mtLEDGER_DATA)); } else @@ -3287,12 +3433,10 @@ PeerImp::getTxSet(std::shared_ptr const& m) const } void -PeerImp::processLedgerRequest(std::shared_ptr const& m) +PeerImp::processLedgerRequest( + std::shared_ptr const& m, + std::vector 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; std::shared_ptr sharedMap; SHAMap const* map{nullptr}; @@ -3372,26 +3516,25 @@ PeerImp::processLedgerRequest(std::shared_ptr 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> data; + std::vector 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 +3543,27 @@ PeerImp::processLedgerRequest(std::shared_ptr 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) + { + REACHABLE("xrpl::PeerImp : emit leaf depth in reply"); + node->set_depth(d.nodeID.getDepth()); + } + else + { + REACHABLE("xrpl::PeerImp : emit inner id in reply"); + node->set_id(d.nodeID.getRawString()); + } } } else @@ -3441,13 +3602,13 @@ PeerImp::processLedgerRequest(std::shared_ptr 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"; } diff --git a/src/xrpld/overlay/detail/PeerImp.h b/src/xrpld/overlay/detail/PeerImp.h index 90f8a917f4..de90e60955 100644 --- a/src/xrpld/overlay/detail/PeerImp.h +++ b/src/xrpld/overlay/detail/PeerImp.h @@ -32,6 +32,7 @@ #include #include #include +#include #include #include @@ -679,7 +680,9 @@ private: getTxSet(std::shared_ptr const& m) const; void - processLedgerRequest(std::shared_ptr const& m); + processLedgerRequest( + std::shared_ptr const& m, + std::vector nodeIDs); protected: // Kept `protected` so test subclasses (see diff --git a/src/xrpld/overlay/detail/ProtocolVersion.cpp b/src/xrpld/overlay/detail/ProtocolVersion.cpp index 347e59accb..2d5d0a56f7 100644 --- a/src/xrpld/overlay/detail/ProtocolVersion.cpp +++ b/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