From 44590a7008e65188264027abd3eb0b89daa8e3cc Mon Sep 17 00:00:00 2001 From: Bart <11445373+bthomee@users.noreply.github.com> Date: Thu, 23 Apr 2026 18:15:01 -0700 Subject: [PATCH] Review feedback --- include/xrpl/shamap/SHAMap.h | 2 +- src/libxrpl/shamap/SHAMap.cpp | 8 ++-- src/test/app/LedgerNodeHelpers_test.cpp | 36 ++++++---------- src/xrpld/app/ledger/detail/InboundLedger.cpp | 10 ++--- .../app/ledger/detail/InboundLedgers.cpp | 5 +-- .../app/ledger/detail/InboundTransactions.cpp | 6 +-- .../app/ledger/detail/LedgerNodeHelpers.cpp | 9 ++-- .../app/ledger/detail/LedgerNodeHelpers.h | 2 +- src/xrpld/overlay/detail/PeerImp.cpp | 43 +++++++++---------- src/xrpld/overlay/detail/PeerImp.h | 5 ++- 10 files changed, 56 insertions(+), 70 deletions(-) diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h index 5aafa435e0..4cbf4fe951 100644 --- a/include/xrpl/shamap/SHAMap.h +++ b/include/xrpl/shamap/SHAMap.h @@ -422,7 +422,7 @@ private: // returns the first item at or below this node SHAMapLeafNode* - firstBelow(SHAMapTreeNodePtr, SharedPtrNodeStack& stack, int branch = 0) const; + firstBelow(SHAMapTreeNodePtr node, SharedPtrNodeStack& stack, int branch = 0) const; // returns the last item at or below this node SHAMapLeafNode* diff --git a/src/libxrpl/shamap/SHAMap.cpp b/src/libxrpl/shamap/SHAMap.cpp index 05407a35c5..f0d03f4771 100644 --- a/src/libxrpl/shamap/SHAMap.cpp +++ b/src/libxrpl/shamap/SHAMap.cpp @@ -694,10 +694,8 @@ SHAMap::delItem(uint256 const& id) SHAMapNodeType const type = leaf->getType(); - using TreeNodeType = SHAMapTreeNodePtr; - // What gets attached to the end of the chain (For now, nothing, since we deleted the leaf) - TreeNodeType prevNode; + SHAMapTreeNodePtr prevNode; while (!stack.empty()) { @@ -723,7 +721,7 @@ SHAMap::delItem(uint256 const& id) // no children below this branch // // Note: This is unnecessary due to the std::move above but left here for safety - prevNode = TreeNodeType{}; + prevNode = SHAMapTreeNodePtr{}; } else if (bc == 1) { @@ -736,7 +734,7 @@ SHAMap::delItem(uint256 const& id) { if (!node->isEmptyBranch(i)) { - node->setChild(i, TreeNodeType{}); + node->setChild(i, SHAMapTreeNodePtr{}); break; } } diff --git a/src/test/app/LedgerNodeHelpers_test.cpp b/src/test/app/LedgerNodeHelpers_test.cpp index 4636fcf325..e5b692d4fd 100644 --- a/src/test/app/LedgerNodeHelpers_test.cpp +++ b/src/test/app/LedgerNodeHelpers_test.cpp @@ -163,8 +163,7 @@ class LedgerNodeHelpers_test : public beast::unit_test::suite innerNode->setChild(0, childNode); auto const innerData = serializeNode(innerNode); auto const result = getTreeNode(innerData); - BEAST_EXPECT(result.has_value()); - BEAST_EXPECT((*result)->isInner()); + BEAST_EXPECT(result->isInner()); } // Valid: leaf node. @@ -173,20 +172,19 @@ class LedgerNodeHelpers_test : public beast::unit_test::suite auto const leafNode = intr_ptr::make_shared(leafItem, 1); auto const leafData = serializeNode(leafNode); auto result = getTreeNode(leafData); - BEAST_EXPECT(result.has_value()); - BEAST_EXPECT((*result)->isLeaf()); + BEAST_EXPECT(result->isLeaf()); } // Invalid: empty data. { auto const result = getTreeNode(""); - BEAST_EXPECT(!result.has_value()); + BEAST_EXPECT(!result); } // Invalid: garbage data. { auto const result = getTreeNode("invalid"); - BEAST_EXPECT(!result.has_value()); + BEAST_EXPECT(!result); } // Invalid: truncated data. @@ -198,7 +196,7 @@ class LedgerNodeHelpers_test : public beast::unit_test::suite uint256 const tag; auto const leafData = serializeNode(leafNode).substr(0, tag.bytes - 1); auto const result = getTreeNode(leafData); - BEAST_EXPECT(!result.has_value()); + BEAST_EXPECT(!result); } } @@ -223,8 +221,7 @@ class LedgerNodeHelpers_test : public beast::unit_test::suite node.set_nodedata(innerData); node.set_nodeid(innerID.getRawString()); auto const result = getSHAMapNodeID(node, innerNode); - BEAST_EXPECT(result.has_value()); - BEAST_EXPECT(*result == innerID); + BEAST_EXPECT(result == innerID); } // Valid: new `id` field at minimum depth. @@ -236,8 +233,7 @@ class LedgerNodeHelpers_test : public beast::unit_test::suite node.set_nodedata(innerData); node.set_id(innerID.getRawString()); auto const result = getSHAMapNodeID(node, innerNode); - BEAST_EXPECT(result.has_value()); - BEAST_EXPECT(*result == innerID); + BEAST_EXPECT(result == innerID); } // Invalid: new `depth` field should not be used for inner nodes. @@ -246,7 +242,7 @@ class LedgerNodeHelpers_test : public beast::unit_test::suite node.set_nodedata(innerData); node.set_depth(10); auto const result = getSHAMapNodeID(node, innerNode); - BEAST_EXPECT(!result.has_value()); + BEAST_EXPECT(!result); } } @@ -266,8 +262,7 @@ class LedgerNodeHelpers_test : public beast::unit_test::suite ledgerNode.set_nodedata(leafData); ledgerNode.set_nodeid(leafID.getRawString()); auto const result = getSHAMapNodeID(ledgerNode, leafNode); - BEAST_EXPECT(result.has_value()); - BEAST_EXPECT(*result == leafID); + BEAST_EXPECT(result == leafID); } // Invalid: new `id` field should not be used for leaf nodes. @@ -279,7 +274,7 @@ class LedgerNodeHelpers_test : public beast::unit_test::suite ledgerNode.set_nodedata(leafData); ledgerNode.set_id(leafID.getRawString()); auto const result = getSHAMapNodeID(ledgerNode, leafNode); - BEAST_EXPECT(!result.has_value()); + BEAST_EXPECT(!result); } // Valid: new `depth` field at minimum depth. @@ -291,8 +286,7 @@ class LedgerNodeHelpers_test : public beast::unit_test::suite node.set_nodedata(leafData); node.set_depth(leafDepth); auto const result = getSHAMapNodeID(node, leafNode); - BEAST_EXPECT(result.has_value()); - BEAST_EXPECT(*result == leafID); + BEAST_EXPECT(result == leafID); } // Valid: new `depth` field at arbitrary depth between minimum and maximum. @@ -304,8 +298,7 @@ class LedgerNodeHelpers_test : public beast::unit_test::suite ledgerNode.set_nodedata(leafData); ledgerNode.set_depth(leafDepth); auto const result = getSHAMapNodeID(ledgerNode, leafNode); - BEAST_EXPECT(result.has_value()); - BEAST_EXPECT(*result == leafID); + BEAST_EXPECT(result == leafID); } // Valid: new `depth` field at maximum depth. @@ -320,8 +313,7 @@ class LedgerNodeHelpers_test : public beast::unit_test::suite node.set_nodedata(leafData); node.set_depth(leafDepth); auto const result = getSHAMapNodeID(node, leafNode); - BEAST_EXPECT(result.has_value()); - BEAST_EXPECT(*result == leafID); + BEAST_EXPECT(result == leafID); } // Invalid: legacy `nodeid` field where the node ID is inconsistent with the key. @@ -338,7 +330,7 @@ class LedgerNodeHelpers_test : public beast::unit_test::suite ledgerNode.set_nodedata(otherData); ledgerNode.set_nodeid(otherID.getRawString()); auto const result = getSHAMapNodeID(ledgerNode, leafNode); - BEAST_EXPECT(!result.has_value()); + BEAST_EXPECT(!result); } } } diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp index be786a2d9d..f1e002c5b7 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp @@ -892,7 +892,7 @@ InboundLedger::receiveNode(protocol::TMLedgerData& packet, SHAMapAddNode& san) return; } - auto const nodeID = getSHAMapNodeID(ledgerNode, *treeNode); + auto const nodeID = getSHAMapNodeID(ledgerNode, treeNode); if (!nodeID) { JLOG(journal_.warn()) << "Got invalid node id"; @@ -902,11 +902,11 @@ InboundLedger::receiveNode(protocol::TMLedgerData& packet, SHAMapAddNode& san) if (nodeID->isRoot()) { - san += map.addRootNode(rootHash, std::move(*treeNode), f); + san += map.addRootNode(rootHash, std::move(treeNode), f); } else { - san += map.addKnownNode(*nodeID, std::move(*treeNode), f); + san += map.addKnownNode(*nodeID, std::move(treeNode), f); } if (!san.isGood()) @@ -972,7 +972,7 @@ InboundLedger::takeAsRootNode(std::string_view data, SHAMapAddNode& san) AccountStateSF filter(mLedger->stateMap().family().db(), app_.getLedgerMaster()); san += mLedger->stateMap().addRootNode( - SHAMapHash{mLedger->header().accountHash}, std::move(*treeNode), &filter); + SHAMapHash{mLedger->header().accountHash}, std::move(treeNode), &filter); return san.isGood(); } @@ -1006,7 +1006,7 @@ InboundLedger::takeTxRootNode(std::string_view data, SHAMapAddNode& san) TransactionStateSF filter(mLedger->txMap().family().db(), app_.getLedgerMaster()); san += mLedger->txMap().addRootNode( - SHAMapHash{mLedger->header().txHash}, std::move(*treeNode), &filter); + SHAMapHash{mLedger->header().txHash}, std::move(treeNode), &filter); return san.isGood(); } diff --git a/src/xrpld/app/ledger/detail/InboundLedgers.cpp b/src/xrpld/app/ledger/detail/InboundLedgers.cpp index c2bdfb2f3f..0a4220f6dc 100644 --- a/src/xrpld/app/ledger/detail/InboundLedgers.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedgers.cpp @@ -256,13 +256,12 @@ public: auto const treeNode = getTreeNode(ledgerNode.nodedata()); if (!treeNode) return; - auto const tn = *treeNode; s.erase(); - tn->serializeWithPrefix(s); + treeNode->serializeWithPrefix(s); app_.getLedgerMaster().addFetchPack( - tn->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&) // NOLINT(bugprone-empty-catch) diff --git a/src/xrpld/app/ledger/detail/InboundTransactions.cpp b/src/xrpld/app/ledger/detail/InboundTransactions.cpp index c0577100f7..f9f1ee6312 100644 --- a/src/xrpld/app/ledger/detail/InboundTransactions.cpp +++ b/src/xrpld/app/ledger/detail/InboundTransactions.cpp @@ -165,7 +165,7 @@ public: return; } - auto const nodeID = getSHAMapNodeID(ledgerNode, *treeNode); + auto const nodeID = getSHAMapNodeID(ledgerNode, treeNode); if (!nodeID) { JLOG(j_.warn()) << "Got invalid node id"; @@ -173,10 +173,10 @@ public: return; } - data.emplace_back(*nodeID, std::move(*treeNode)); + data.emplace_back(*nodeID, std::move(treeNode)); } - if (!ta->takeNodes(data, peer).isUseful()) + if (!ta->takeNodes(std::move(data), peer).isUseful()) peer->charge(Resource::feeUselessData, "ledger_data not useful"); } diff --git a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp index ec35d62416..b15b3f383c 100644 --- a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp +++ b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp @@ -29,20 +29,17 @@ validateLedgerNode(protocol::TMLedgerNode const& ledgerNode) (ledgerNode.has_depth() && ledgerNode.depth() <= SHAMap::leafDepth); } -std::optional +SHAMapTreeNodePtr getTreeNode(std::string_view data) { auto const slice = makeSlice(data); try { - auto treeNode = SHAMapTreeNode::makeFromWire(slice); - if (!treeNode) - return std::nullopt; - return treeNode; + return SHAMapTreeNode::makeFromWire(slice); } catch (std::exception const&) { - return std::nullopt; + return {}; } } diff --git a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h index 442610bb30..7c7ac222d4 100644 --- a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h +++ b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h @@ -41,7 +41,7 @@ validateLedgerNode(protocol::TMLedgerNode const& ledgerNode); * @return An optional containing the deserialized tree node if successful, or std::nullopt if * deserialization fails. */ -[[nodiscard]] std::optional +[[nodiscard]] SHAMapTreeNodePtr getTreeNode(std::string_view data); /** diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 7cc3416d76..5b6c9bee25 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -1614,7 +1614,8 @@ PeerImp::onMessage(std::shared_ptr const& m) } } - // Verify ledger node IDs + // Verify and parse ledger node IDs + std::vector nodeIDs; if (itype != protocol::liBASE) { if (m->nodeids_size() <= 0) @@ -1623,13 +1624,16 @@ PeerImp::onMessage(std::shared_ptr const& m) return; } + nodeIDs.reserve(m->nodeids_size()); for (auto const& nodeId : m->nodeids()) { - if (deserializeSHAMapNodeID(nodeId) == std::nullopt) + auto parsed = deserializeSHAMapNodeID(nodeId); + if (!parsed) { badData("Invalid SHAMap node ID"); return; } + nodeIDs.push_back(std::move(*parsed)); } } @@ -1652,10 +1656,11 @@ PeerImp::onMessage(std::shared_ptr const& m) // Queue a job to process the request std::weak_ptr const weak = shared_from_this(); - app_.getJobQueue().addJob(jtLEDGER_REQ, "RcvGetLedger", [weak, m]() { - if (auto peer = weak.lock()) - peer->processLedgerRequest(m); - }); + app_.getJobQueue().addJob( + jtLEDGER_REQ, "RcvGetLedger", [weak, m, nodeIDs = std::move(nodeIDs)]() mutable { + if (auto peer = weak.lock()) + peer->processLedgerRequest(m, std::move(nodeIDs)); + }); } void @@ -3364,7 +3369,9 @@ 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()) @@ -3449,7 +3456,7 @@ 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}; @@ -3457,21 +3464,17 @@ PeerImp::processLedgerRequest(std::shared_ptr const& m) std::vector data; auto const useLedgerNodeDepth = supportsFeature(ProtocolFeature::LedgerNodeDepth); - for (int i = 0; - i < m->nodeids_size() && ledgerData.nodes_size() < Tuning::softMaxReplyNodes; - ++i) + for (auto const& nodeID : nodeIDs) { - auto const shaMapNodeId{deserializeSHAMapNodeID(m->nodeids(i))}; - XRPL_ASSERT( - shaMapNodeId.has_value(), "xrpl::PeerImp::processLedgerRequest : valid node ID"); + if (ledgerData.nodes_size() >= Tuning::softMaxReplyNodes) + break; data.clear(); data.reserve(Tuning::softMaxReplyNodes); 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(p_journal_.trace()) << "processLedgerRequest: getNodeFat got " << data.size() << " nodes"; @@ -3489,17 +3492,11 @@ PeerImp::processLedgerRequest(std::shared_ptr const& m) // we always set the `nodeid` field. However, when it is supported then we // set the `id` field for inner nodes and the `depth` field for leaf nodes. if (!useLedgerNodeDepth) - { node->set_nodeid(d.nodeID.getRawString()); - } else if (d.isLeaf) - { node->set_depth(d.nodeID.getDepth()); - } else - { node->set_id(d.nodeID.getRawString()); - } } } else @@ -3538,7 +3535,7 @@ PeerImp::processLedgerRequest(std::shared_ptr const& m) info += ", no hash specified"; JLOG(p_journal_.warn()) - << "processLedgerRequest: getNodeFat with nodeId " << *shaMapNodeId + << "processLedgerRequest: getNodeFat with nodeId " << nodeID << " and ledger info type " << info << " throws exception: " << e.what(); } } diff --git a/src/xrpld/overlay/detail/PeerImp.h b/src/xrpld/overlay/detail/PeerImp.h index f2cca1407a..aa64f1df9b 100644 --- a/src/xrpld/overlay/detail/PeerImp.h +++ b/src/xrpld/overlay/detail/PeerImp.h @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -792,7 +793,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); }; //------------------------------------------------------------------------------