Support leaf nodes at depth 0, use std::move, simplify tests

This commit is contained in:
Bart
2026-03-01 16:44:58 -05:00
parent ebf336f472
commit 6374f4886d
5 changed files with 28 additions and 73 deletions

View File

@@ -115,19 +115,11 @@ class LedgerNodeHelpers_test : public beast::unit_test::suite
BEAST_EXPECT(validateLedgerNode(node));
}
// Invalid: `depth` is less than minimum depth.
{
protocol::TMLedgerNode node;
node.set_nodedata("test_data");
node.set_depth(0);
BEAST_EXPECT(!validateLedgerNode(node));
}
// Valid: `depth` at minimum depth.
{
protocol::TMLedgerNode node;
node.set_nodedata("test_data");
node.set_depth(1);
node.set_depth(0);
BEAST_EXPECT(validateLedgerNode(node));
}
@@ -161,13 +153,13 @@ class LedgerNodeHelpers_test : public beast::unit_test::suite
{
testcase("getTreeNode");
// Valid: root (=inner) node. It must have at least one child.
// Valid: inner node. It must have at least one child for `serializeNode` to work.
{
auto const rootNode = intr_ptr::make_shared<SHAMapInnerNode>(1);
auto const innerNode = intr_ptr::make_shared<SHAMapInnerNode>(1);
auto const childNode = intr_ptr::make_shared<SHAMapInnerNode>(1);
rootNode->setChild(0, childNode);
auto const rootData = serializeNode(rootNode);
auto const result = getTreeNode(rootData);
innerNode->setChild(0, childNode);
auto const innerData = serializeNode(innerNode);
auto const result = getTreeNode(innerData);
BEAST_EXPECT(result.has_value());
BEAST_EXPECT((*result)->isInner());
}
@@ -215,84 +207,50 @@ class LedgerNodeHelpers_test : public beast::unit_test::suite
testcase("getSHAMapNodeID");
{
// Tests using a root node (=inner node at depth 0).
auto const rootNode = intr_ptr::make_shared<SHAMapInnerNode>(1);
auto const childNode = intr_ptr::make_shared<SHAMapInnerNode>(1);
rootNode->setChild(0, childNode);
auto const rootData = serializeNode(rootNode);
auto const rootNodeID = SHAMapNodeID{};
// Valid: legacy `nodeid` field.
{
protocol::TMLedgerNode node;
node.set_nodedata(rootData);
node.set_nodeid(rootNodeID.getRawString());
auto const result = getSHAMapNodeID(node, rootNode);
BEAST_EXPECT(result.has_value());
BEAST_EXPECT(*result == rootNodeID);
}
// Valid: new `id` field.
{
protocol::TMLedgerNode node;
node.set_nodedata(rootData);
node.set_id(rootNodeID.getRawString());
auto const result = getSHAMapNodeID(node, rootNode);
BEAST_EXPECT(result.has_value());
BEAST_EXPECT(*result == rootNodeID);
}
// Invalid: new `depth` field is present but should not be used for inner nodes.
{
protocol::TMLedgerNode node;
node.set_nodedata(rootData);
node.set_depth(0);
auto const result = getSHAMapNodeID(node, rootNode);
BEAST_EXPECT(!result.has_value());
}
}
{
// Tests using an inner node at arbitrary depth.
// Tests using inner nodes at various depths.
auto const innerNode = intr_ptr::make_shared<SHAMapInnerNode>(1);
auto const childNode = intr_ptr::make_shared<SHAMapInnerNode>(1);
innerNode->setChild(0, childNode);
auto const innerData = serializeNode(innerNode);
auto const innerDepth = 3;
auto const innerNodeID = SHAMapNodeID::createID(innerDepth, uint256{});
// Valid: legacy `nodeid` field.
// Valid: legacy `nodeid` field at arbitrary depth.
{
auto const innerDepth = 3;
auto const innerID = SHAMapNodeID::createID(innerDepth, uint256{});
protocol::TMLedgerNode node;
node.set_nodedata(innerData);
node.set_nodeid(innerNodeID.getRawString());
node.set_nodeid(innerID.getRawString());
auto const result = getSHAMapNodeID(node, innerNode);
BEAST_EXPECT(result.has_value());
BEAST_EXPECT(*result == innerNodeID);
BEAST_EXPECT(*result == innerID);
}
// Valid: new `id` field.
// Valid: new `id` field at minimum depth.
{
auto const innerDepth = 0;
auto const innerID = SHAMapNodeID::createID(innerDepth, uint256{});
protocol::TMLedgerNode node;
node.set_nodedata(innerData);
node.set_id(innerNodeID.getRawString());
node.set_id(innerID.getRawString());
auto const result = getSHAMapNodeID(node, innerNode);
BEAST_EXPECT(result.has_value());
BEAST_EXPECT(*result == innerNodeID);
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(innerDepth);
node.set_depth(10);
auto const result = getSHAMapNodeID(node, innerNode);
BEAST_EXPECT(!result.has_value());
}
}
{
// Tests using leaf nodes at various depths, but with the same key.
// Tests using leaf nodes at various depths.
auto const leafItem = makeTestItem(12345);
auto const leafNode = intr_ptr::make_shared<SHAMapAccountStateLeafNode>(leafItem, 1);
auto const leafData = serializeNode(leafNode);
@@ -324,11 +282,8 @@ class LedgerNodeHelpers_test : public beast::unit_test::suite
}
// Valid: new `depth` field at minimum depth.
// Note that we do not test a depth less than the minimum depth, because the proto
// message is assumed to have been validated by the time the getSHAMapNodeID function is
// called.
{
auto const leafDepth = 1;
auto const leafDepth = 0;
auto const leafID = SHAMapNodeID::createID(leafDepth, leafKey);
protocol::TMLedgerNode node;

View File

@@ -962,7 +962,8 @@ InboundLedger::takeTxRootNode(std::string const& data, SHAMapAddNode& san)
}
TransactionStateSF filter(mLedger->txMap().family().db(), app_.getLedgerMaster());
san += mLedger->txMap().addRootNode(SHAMapHash{mLedger->header().txHash}, *treeNode, &filter);
san += mLedger->txMap().addRootNode(
SHAMapHash{mLedger->header().txHash}, std::move(*treeNode), &filter);
return san.isGood();
}

View File

@@ -145,7 +145,7 @@ public:
return;
}
auto const treeNode = getTreeNode(ledger_node.nodedata());
auto treeNode = getTreeNode(ledger_node.nodedata());
if (!treeNode)
{
JLOG(j_.warn()) << "Got invalid node data";
@@ -161,7 +161,7 @@ public:
return;
}
data.emplace_back(std::make_pair(*nodeID, *treeNode));
data.emplace_back(std::make_pair(*nodeID, std::move(*treeNode)));
}
if (!ta->takeNodes(data, peer).isUseful())

View File

@@ -24,8 +24,7 @@ validateLedgerNode(protocol::TMLedgerNode const& ledger_node)
return !ledger_node.has_id() && !ledger_node.has_depth();
return ledger_node.has_id() ||
(ledger_node.has_depth() && ledger_node.depth() >= 1 &&
ledger_node.depth() <= SHAMap::leafDepth);
(ledger_node.has_depth() && ledger_node.depth() <= SHAMap::leafDepth);
}
std::optional<intr_ptr::SharedPtr<SHAMapTreeNode>>

View File

@@ -21,7 +21,7 @@ namespace xrpl {
* present.
* - 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 `depth` field is present then it must be between 1 and SHAMap::leafDepth (inclusive).
* - If the `depth` field is present then it must be between 0 and SHAMap::leafDepth (inclusive).
*
* @param ledger_node The ledger node to validate.
* @return true if the ledger node has the expected fields, false otherwise.