ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-07-25 16:10:57 +00:00
Code Issues Packages Projects Releases Wiki Activity

Review feedback

Browse Source
This commit is contained in:
Bart
2026-04-23 18:15:01 -07:00
parent 1934c316b2
commit 44590a7008
10 changed files with 56 additions and 70 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
include/xrpl/shamap/SHAMap.h
View File

@@ -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*

8
src/libxrpl/shamap/SHAMap.cpp
View File

@@ -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;
}
}

36
src/test/app/LedgerNodeHelpers_test.cpp
View File

@@ -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<SHAMapAccountStateLeafNode>(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);
}
}
}

10
src/xrpld/app/ledger/detail/InboundLedger.cpp
View File

@@ -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();
}

5
src/xrpld/app/ledger/detail/InboundLedgers.cpp
View File

@@ -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<Blob>(s.begin(), s.end()));
treeNode->getHash().as_uint256(), std::make_shared<Blob>(s.begin(), s.end()));
}
}
catch (std::exception const&) // NOLINT(bugprone-empty-catch)

6
src/xrpld/app/ledger/detail/InboundTransactions.cpp
View File

@@ -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");
}

9
src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp
View File

@@ -29,20 +29,17 @@ validateLedgerNode(protocol::TMLedgerNode const& ledgerNode)
(ledgerNode.has_depth() && ledgerNode.depth() <= SHAMap::leafDepth);
}
std::optional<SHAMapTreeNodePtr>
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 {};
}
}

2
src/xrpld/app/ledger/detail/LedgerNodeHelpers.h
View File

@@ -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<SHAMapTreeNodePtr>
[[nodiscard]] SHAMapTreeNodePtr
getTreeNode(std::string_view data);
/**

43
src/xrpld/overlay/detail/PeerImp.cpp
View File

@@ -1614,7 +1614,8 @@ PeerImp::onMessage(std::shared_ptr<protocol::TMGetLedger> const& m)
}
}
// Verify ledger node IDs
// Verify and parse ledger node IDs
std::vector<SHAMapNodeID> nodeIDs;
if (itype != protocol::liBASE)
{
if (m->nodeids_size() <= 0)
@@ -1623,13 +1624,16 @@ PeerImp::onMessage(std::shared_ptr<protocol::TMGetLedger> 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<protocol::TMGetLedger> const& m)
// Queue a job to process the request
std::weak_ptr<PeerImp> 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<protocol::TMGetLedger> const& m) const
}
void
PeerImp::processLedgerRequest(std::shared_ptr<protocol::TMGetLedger> const& m)
PeerImp::processLedgerRequest(
std::shared_ptr<protocol::TMGetLedger> const& m,
std::vector<SHAMapNodeID> nodeIDs)
{
// Do not resource charge a peer responding to a relay
if (!m->has_requestcookie())
@@ -3449,7 +3456,7 @@ PeerImp::processLedgerRequest(std::shared_ptr<protocol::TMGetLedger> 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<protocol::TMGetLedger> const& m)
std::vector<SHAMapNodeData> 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<protocol::TMGetLedger> 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<protocol::TMGetLedger> 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();
}
}

5
src/xrpld/overlay/detail/PeerImp.h
View File

@@ -14,6 +14,7 @@
#include <xrpl/protocol/STTx.h>
#include <xrpl/protocol/STValidation.h>
#include <xrpl/resource/Fees.h>
#include <xrpl/shamap/SHAMapNodeID.h>
#include <boost/circular_buffer.hpp>
#include <boost/endian/conversion.hpp>
@@ -792,7 +793,9 @@ private:
getTxSet(std::shared_ptr<protocol::TMGetLedger> const& m) const;
void
processLedgerRequest(std::shared_ptr<protocol::TMGetLedger> const& m);
processLedgerRequest(
std::shared_ptr<protocol::TMGetLedger> const& m,
std::vector<SHAMapNodeID> nodeIDs);
};
//------------------------------------------------------------------------------