diff --git a/src/test/app/LedgerNodeHelpers_test.cpp b/src/test/app/LedgerNodeHelpers_test.cpp index 8a9b32d474..d23b1d233f 100644 --- a/src/test/app/LedgerNodeHelpers_test.cpp +++ b/src/test/app/LedgerNodeHelpers_test.cpp @@ -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(1); + auto const innerNode = intr_ptr::make_shared(1); auto const childNode = intr_ptr::make_shared(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(1); - auto const childNode = intr_ptr::make_shared(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(1); auto const childNode = intr_ptr::make_shared(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(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; diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp index 818ce92177..a46fded372 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp @@ -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(); } diff --git a/src/xrpld/app/ledger/detail/InboundTransactions.cpp b/src/xrpld/app/ledger/detail/InboundTransactions.cpp index f8c5239644..be5e50e671 100644 --- a/src/xrpld/app/ledger/detail/InboundTransactions.cpp +++ b/src/xrpld/app/ledger/detail/InboundTransactions.cpp @@ -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()) diff --git a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp index fdb21e0a38..394f8dadd2 100644 --- a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp +++ b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp @@ -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> diff --git a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h index 1e61d62adf..205b02f54a 100644 --- a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h +++ b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h @@ -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.