From fe8cc02bfae7e5d5031165fecd5ec976780239ee Mon Sep 17 00:00:00 2001 From: Bart <11445373+bthomee@users.noreply.github.com> Date: Wed, 18 Feb 2026 07:54:33 -0500 Subject: [PATCH] Refine --- include/xrpl/protocol/detail/features.macro | 2 +- include/xrpl/shamap/SHAMap.h | 8 +- src/libxrpl/shamap/SHAMapSync.cpp | 44 +-- src/test/app/LedgerNodeHelpers_test.cpp | 352 ++++++++++++++++++ src/test/shamap/SHAMapSync_test.cpp | 10 +- src/xrpld/app/ledger/InboundLedger.h | 4 +- src/xrpld/app/ledger/detail/InboundLedger.cpp | 42 +-- .../app/ledger/detail/InboundLedgers.cpp | 6 +- .../app/ledger/detail/InboundTransactions.cpp | 10 +- .../app/ledger/detail/LedgerNodeHelpers.cpp | 165 ++++++++ .../app/ledger/detail/LedgerNodeHelpers.h | 147 ++++---- .../app/ledger/detail/TransactionAcquire.cpp | 4 +- .../app/ledger/detail/TransactionAcquire.h | 2 +- src/xrpld/overlay/detail/PeerImp.cpp | 24 +- 14 files changed, 658 insertions(+), 162 deletions(-) create mode 100644 src/test/app/LedgerNodeHelpers_test.cpp create mode 100644 src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 543bb6562b..f83a4b442d 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -15,7 +15,7 @@ // Add new amendments to the top of this list. // Keep it sorted in reverse chronological order. -XRPL_FIX (LedgerNodeID, Supported::yes, VoteBehavior::DefaultNo) +XRPL_FIX (LedgerNodeID, Supported::yes, VoteBehavior::DefaultYes) XRPL_FIX (PermissionedDomainInvariant, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (ExpiredNFTokenOfferRemoval, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (BatchInnerSigs, Supported::yes, VoteBehavior::DefaultNo) diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h index 731acabf16..74249b22a8 100644 --- a/include/xrpl/shamap/SHAMap.h +++ b/include/xrpl/shamap/SHAMap.h @@ -252,7 +252,7 @@ public: bool getNodeFat( SHAMapNodeID const& wanted, - std::vector>& data, + std::vector>& data, bool fatLeaves, std::uint32_t depth) const; @@ -280,11 +280,11 @@ public: serializeRoot(Serializer& s) const; SHAMapAddNode - addRootNode(SHAMapHash const& hash, intr_ptr::SharedPtr root_node, SHAMapSyncFilter const* filter); + addRootNode(SHAMapHash const& hash, intr_ptr::SharedPtr rootNode, SHAMapSyncFilter const* filter); SHAMapAddNode addKnownNode( - SHAMapNodeID const& node_id, - intr_ptr::SharedPtr tree_node, + SHAMapNodeID const& nodeID, + intr_ptr::SharedPtr treeNode, SHAMapSyncFilter const* filter); // status functions diff --git a/src/libxrpl/shamap/SHAMapSync.cpp b/src/libxrpl/shamap/SHAMapSync.cpp index 27deb76144..108ac12a5e 100644 --- a/src/libxrpl/shamap/SHAMapSync.cpp +++ b/src/libxrpl/shamap/SHAMapSync.cpp @@ -374,7 +374,7 @@ SHAMap::getMissingNodes(int max, SHAMapSyncFilter* filter) bool SHAMap::getNodeFat( SHAMapNodeID const& wanted, - std::vector>& data, + std::vector>& data, bool fatLeaves, std::uint32_t depth) const { @@ -419,7 +419,7 @@ SHAMap::getNodeFat( // Add this node to the reply s.erase(); node->serializeForWire(s); - data.emplace_back(std::make_pair(nodeID, s.getData())); + data.emplace_back(std::make_tuple(nodeID, s.getData(), node->isLeaf())); if (node->isInner()) { @@ -449,7 +449,7 @@ SHAMap::getNodeFat( // Just include this node s.erase(); childNode->serializeForWire(s); - data.emplace_back(std::make_pair(childID, s.getData())); + data.emplace_back(std::make_tuple(childID, s.getData(), childNode->isLeaf())); } } } @@ -469,7 +469,7 @@ SHAMap::serializeRoot(Serializer& s) const SHAMapAddNode SHAMap::addRootNode( SHAMapHash const& hash, - intr_ptr::SharedPtr root_node, + intr_ptr::SharedPtr rootNode, SHAMapSyncFilter const* filter) { // we already have a root_ node @@ -481,13 +481,13 @@ SHAMap::addRootNode( } XRPL_ASSERT(cowid_ >= 1, "xrpl::SHAMap::addRootNode : valid cowid"); - if (root_node->getHash() != hash) + if (rootNode->getHash() != hash) return SHAMapAddNode::invalid(); if (backed_) - canonicalize(hash, root_node); + canonicalize(hash, rootNode); - root_ = root_node; + root_ = rootNode; if (root_->isLeaf()) clearSynching(); @@ -504,11 +504,11 @@ SHAMap::addRootNode( SHAMapAddNode SHAMap::addKnownNode( - SHAMapNodeID const& node_id, - intr_ptr::SharedPtr tree_node, + SHAMapNodeID const& nodeID, + intr_ptr::SharedPtr treeNode, SHAMapSyncFilter const* filter) { - XRPL_ASSERT(!node_id.isRoot(), "xrpl::SHAMap::addKnownNode : valid node input"); + XRPL_ASSERT(!nodeID.isRoot(), "xrpl::SHAMap::addKnownNode : valid node input"); if (!isSynching()) { @@ -521,14 +521,14 @@ SHAMap::addKnownNode( auto currNode = root_.get(); while (currNode->isInner() && !static_cast(currNode)->isFullBelow(generation) && - (currNodeID.getDepth() < node_id.getDepth())) + (currNodeID.getDepth() < nodeID.getDepth())) { - int const branch = selectBranch(currNodeID, node_id.getNodeID()); + int const branch = selectBranch(currNodeID, nodeID.getNodeID()); XRPL_ASSERT(branch >= 0, "xrpl::SHAMap::addKnownNode : valid branch"); auto inner = static_cast(currNode); if (inner->isEmptyBranch(branch)) { - JLOG(journal_.warn()) << "Add known node for empty branch" << node_id; + JLOG(journal_.warn()) << "Add known node for empty branch" << nodeID; return SHAMapAddNode::invalid(); } @@ -544,7 +544,7 @@ SHAMap::addKnownNode( if (currNode != nullptr) continue; - if (childHash != tree_node->getHash()) + if (childHash != treeNode->getHash()) { JLOG(journal_.warn()) << "Corrupt node received"; return SHAMapAddNode::invalid(); @@ -553,32 +553,32 @@ SHAMap::addKnownNode( // 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() > leafDepth) || (tree_node->isInner() && currNodeID.getDepth() == leafDepth)) + if ((currNodeID.getDepth() > leafDepth) || (treeNode->isInner() && currNodeID.getDepth() == leafDepth)) { // Map is provably invalid state_ = SHAMapState::Invalid; return SHAMapAddNode::useful(); } - if (currNodeID != node_id) + if (currNodeID != nodeID) { // Either this node is broken or we didn't request it (yet) - JLOG(journal_.warn()) << "unable to hook node " << node_id; + JLOG(journal_.warn()) << "unable to hook node " << nodeID; JLOG(journal_.info()) << " stuck at " << currNodeID; - JLOG(journal_.info()) << "got depth=" << node_id.getDepth() << ", walked to= " << currNodeID.getDepth(); + JLOG(journal_.info()) << "got depth=" << nodeID.getDepth() << ", walked to= " << currNodeID.getDepth(); return SHAMapAddNode::useful(); } if (backed_) - canonicalize(childHash, tree_node); + canonicalize(childHash, treeNode); - tree_node = prevNode->canonicalizeChild(branch, std::move(tree_node)); + treeNode = prevNode->canonicalizeChild(branch, std::move(treeNode)); if (filter) { Serializer s; - tree_node->serializeWithPrefix(s); - filter->gotNode(false, childHash, ledgerSeq_, std::move(s.modData()), tree_node->getType()); + treeNode->serializeWithPrefix(s); + filter->gotNode(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..540e48395b --- /dev/null +++ b/src/test/app/LedgerNodeHelpers_test.cpp @@ -0,0 +1,352 @@ +#include +#include +#include +#include + +#include + +#include +#include +#include +#include +#include +#include + +namespace xrpl { +namespace tests { + +class LedgerNodeHelpers_test : public beast::unit_test::suite +{ + // Helper to create a simple SHAMapItem for testing + boost::intrusive_ptr + makeTestItem(std::uint32_t seed) + { + Serializer s; + s.add32(seed); + s.add32(seed + 1); + s.add32(seed + 2); + return make_shamapitem(s.getSHA512Half(), s.slice()); + } + + // Helper to serialize a tree node to wire format + std::string + serializeNode(intr_ptr::SharedPtr const& node) + { + Serializer s; + node->serializeWithPrefix(s); + auto const slice = s.slice(); + return std::string(reinterpret_cast(slice.data()), slice.size()); + } + + void + testValidateLedgerNode() + { + testcase("validateLedgerNode"); + + using namespace test::jtx; + + /*// Test with amendment disabled. + { + Env env{*this, testable_amendments() - fixLedgerNodeID}; + auto& app = env.app(); + + // Valid node with nodeid. + protocol::TMLedgerNode node1; + node1.set_nodedata("test_data"); + node1.set_nodeid("test_nodeid"); + BEAST_EXPECT(validateLedgerNode(app, node1)); + + // Invalid: missing nodedata. + protocol::TMLedgerNode node2; + node2.set_nodeid("test_nodeid"); + BEAST_EXPECT(!validateLedgerNode(app, node2)); + + // Invalid: has new field (id). + protocol::TMLedgerNode node3; + node3.set_nodedata("test_data"); + node3.set_nodeid("test_nodeid"); + node3.set_id("test_id"); + BEAST_EXPECT(!validateLedgerNode(app, node3)); + + // Invalid: has new field (depth). + protocol::TMLedgerNode node4; + node4.set_nodedata("test_data"); + node4.set_nodeid("test_nodeid"); + node4.set_depth(5); + BEAST_EXPECT(!validateLedgerNode(app, node4)); + } + + // Test with amendment enabled. + { + Env env{*this, testable_amendments() | fixLedgerNodeID}; + auto& app = env.app(); + + // Valid inner node with id. + protocol::TMLedgerNode node1; + node1.set_nodedata("test_data"); + node1.set_id("test_id"); + BEAST_EXPECT(validateLedgerNode(app, node1)); + + // Valid leaf node with depth. + protocol::TMLedgerNode node2; + node2.set_nodedata("test_data"); + node2.set_depth(5); + BEAST_EXPECT(validateLedgerNode(app, node2)); + + // Valid leaf node at max depth. + protocol::TMLedgerNode node3; + node3.set_nodedata("test_data"); + node3.set_depth(SHAMap::leafDepth); + BEAST_EXPECT(validateLedgerNode(app, node3)); + + // Invalid: depth exceeds max depth. + protocol::TMLedgerNode node4; + node4.set_nodedata("test_data"); + node4.set_depth(SHAMap::leafDepth + 1); + BEAST_EXPECT(!validateLedgerNode(app, node4)); + + // Invalid: has legacy field (nodeid). + protocol::TMLedgerNode node5; + node5.set_nodedata("test_data"); + node5.set_nodeid("test_nodeid"); + BEAST_EXPECT(!validateLedgerNode(app, node5)); + + // Invalid: missing both id and depth. + protocol::TMLedgerNode node6; + node6.set_nodedata("test_data"); + BEAST_EXPECT(!validateLedgerNode(app, node6)); + + // Invalid: missing nodedata. + protocol::TMLedgerNode node7; + node7.set_id("test_id"); + BEAST_EXPECT(!validateLedgerNode(app, node7)); + }*/ + } + + /*void + testGetTreeNode() + { + testcase("getTreeNode"); + + using namespace test::jtx; + test::SuiteJournal journal("LedgerNodeHelpers_test", *this); + TestNodeFamily f(journal); + + // Test with valid inner node + { + auto innerNode = std::make_shared(1); + auto serialized = serializeNode(innerNode); + auto result = getTreeNode(serialized); + BEAST_EXPECT(result.has_value()); + BEAST_EXPECT((*result)->isInner()); + } + + // Test with valid leaf node + { + auto item = makeTestItem(12345); + auto leafNode = + std::make_shared(item, SHAMapNodeType::tnACCOUNT_STATE); + auto serialized = serializeNode(leafNode); + auto result = getTreeNode(serialized); + BEAST_EXPECT(result.has_value()); + BEAST_EXPECT((*result)->isLeaf()); + } + + // Test with invalid data - empty string + { + auto result = getTreeNode(""); + BEAST_EXPECT(!result.has_value()); + } + + // Test with invalid data - garbage data + { + std::string garbage = "This is not a valid serialized node!"; + auto result = getTreeNode(garbage); + BEAST_EXPECT(!result.has_value()); + } + + // Test with malformed data - truncated + { + auto item = makeTestItem(54321); + auto leafNode = + std::make_shared(item, SHAMapNodeType::tnACCOUNT_STATE); + auto serialized = serializeNode(leafNode); + // Truncate the data + serialized = serialized.substr(0, 5); + auto result = getTreeNode(serialized); + BEAST_EXPECT(!result.has_value()); + } + } + + void + testGetSHAMapNodeID() + { + testcase("getSHAMapNodeID"); + + using namespace test::jtx; + test::SuiteJournal journal("LedgerNodeHelpers_test", *this); + TestNodeFamily f(journal); + + auto item = makeTestItem(99999); + auto leafNode = + std::make_shared(item, SHAMapNodeType::tnACCOUNT_STATE); + auto innerNode = std::make_shared(1); + + // Test with amendment disabled + { + Env env{*this}; + auto& app = env.app(); + + // Test with leaf node - valid nodeid + { + auto nodeID = SHAMapNodeID::createID(5, item->key()); + protocol::TMLedgerNode ledgerNode; + ledgerNode.set_nodedata("test_data"); + ledgerNode.set_nodeid(nodeID.getRawString()); + + auto result = getSHAMapNodeID(app, ledgerNode, leafNode); + BEAST_EXPECT(result.has_value()); + BEAST_EXPECT(*result == nodeID); + } + + // Test with leaf node - invalid nodeid (wrong depth) + { + auto nodeID = SHAMapNodeID::createID(7, item->key()); + protocol::TMLedgerNode ledgerNode; + ledgerNode.set_nodedata("test_data"); + ledgerNode.set_nodeid(nodeID.getRawString()); + + auto wrongNodeID = SHAMapNodeID::createID(5, item->key()); + ledgerNode.set_nodeid(wrongNodeID.getRawString()); + + auto result = getSHAMapNodeID(app, ledgerNode, leafNode); + // Should fail because nodeid doesn't match the leaf's key at the + // given depth + BEAST_EXPECT(!result.has_value()); + } + + // Test with inner node + { + auto nodeID = SHAMapNodeID::createID(3, uint256{}); + protocol::TMLedgerNode ledgerNode; + ledgerNode.set_nodedata("test_data"); + ledgerNode.set_nodeid(nodeID.getRawString()); + + auto result = getSHAMapNodeID(app, ledgerNode, innerNode); + BEAST_EXPECT(result.has_value()); + BEAST_EXPECT(*result == nodeID); + } + + // Test missing nodeid + { + protocol::TMLedgerNode ledgerNode; + ledgerNode.set_nodedata("test_data"); + + auto result = getSHAMapNodeID(app, ledgerNode, leafNode); + BEAST_EXPECT(!result.has_value()); + } + } + + // Test with amendment enabled + { + Env env{*this, testable_amendments() | fixLedgerNodeID}; + auto& app = env.app(); + + // Test with leaf node - valid depth + { + std::uint32_t depth = 5; + protocol::TMLedgerNode ledgerNode; + ledgerNode.set_nodedata("test_data"); + ledgerNode.set_depth(depth); + + auto result = getSHAMapNodeID(app, ledgerNode, leafNode); + BEAST_EXPECT(result.has_value()); + auto expectedID = SHAMapNodeID::createID(depth, item->key()); + BEAST_EXPECT(*result == expectedID); + } + + // Test with leaf node - missing depth + { + protocol::TMLedgerNode ledgerNode; + ledgerNode.set_nodedata("test_data"); + + auto result = getSHAMapNodeID(app, ledgerNode, leafNode); + BEAST_EXPECT(!result.has_value()); + } + + // Test with inner node - valid id + { + auto nodeID = SHAMapNodeID::createID(3, uint256{}); + protocol::TMLedgerNode ledgerNode; + ledgerNode.set_nodedata("test_data"); + ledgerNode.set_id(nodeID.getRawString()); + + auto result = getSHAMapNodeID(app, ledgerNode, innerNode); + BEAST_EXPECT(result.has_value()); + BEAST_EXPECT(*result == nodeID); + } + + // Test with inner node - missing id + { + protocol::TMLedgerNode ledgerNode; + ledgerNode.set_nodedata("test_data"); + + auto result = getSHAMapNodeID(app, ledgerNode, innerNode); + BEAST_EXPECT(!result.has_value()); + } + + // Test that nodeid is rejected when amendment is enabled + { + auto nodeID = SHAMapNodeID::createID(5, item->key()); + protocol::TMLedgerNode ledgerNode; + ledgerNode.set_nodedata("test_data"); + ledgerNode.set_nodeid(nodeID.getRawString()); + + // Should fail validation before getSHAMapNodeID is called, + // but let's verify getSHAMapNodeID behavior + // Note: validateLedgerNode should be called first in practice + BEAST_EXPECT(!validateLedgerNode(app, ledgerNode)); + } + + // Test with root node (inner node at depth 0) + { + auto rootNode = std::make_shared(1); + auto nodeID = SHAMapNodeID{}; // root node ID + protocol::TMLedgerNode ledgerNode; + ledgerNode.set_nodedata("test_data"); + ledgerNode.set_id(nodeID.getRawString()); + + auto result = getSHAMapNodeID(app, ledgerNode, rootNode); + BEAST_EXPECT(result.has_value()); + BEAST_EXPECT(*result == nodeID); + } + + // Test with leaf at maximum depth + { + std::uint32_t depth = SHAMap::leafDepth; + protocol::TMLedgerNode ledgerNode; + ledgerNode.set_nodedata("test_data"); + ledgerNode.set_depth(depth); + + auto result = getSHAMapNodeID(app, ledgerNode, leafNode); + BEAST_EXPECT(result.has_value()); + auto expectedID = SHAMapNodeID::createID(depth, item->key()); + BEAST_EXPECT(*result == expectedID); + } + } + }*/ + +public: + void + run() override + { + testValidateLedgerNode(); + // testGetTreeNode(); + // testGetSHAMapNodeID(); + } +}; + +BEAST_DEFINE_TESTSUITE(LedgerNodeHelpers, app, xrpl); + +} // namespace tests +} // namespace xrpl diff --git a/src/test/shamap/SHAMapSync_test.cpp b/src/test/shamap/SHAMapSync_test.cpp index c5aaf2d305..e3e235b161 100644 --- a/src/test/shamap/SHAMapSync_test.cpp +++ b/src/test/shamap/SHAMapSync_test.cpp @@ -103,13 +103,13 @@ public: destination.setSynching(); { - std::vector> a; + std::vector> a; BEAST_EXPECT(source.getNodeFat(SHAMapNodeID(), a, rand_bool(eng_), rand_int(eng_, 2))); unexpected(a.size() < 1, "NodeSize"); - auto node = SHAMapTreeNode::makeFromWire(makeSlice(a[0].second)); + auto node = SHAMapTreeNode::makeFromWire(makeSlice(std::get<1>(a[0]))); BEAST_EXPECT(destination.addRootNode(source.getHash(), std::move(node), nullptr).isGood()); } @@ -124,7 +124,7 @@ public: break; // get as many nodes as possible based on this information - std::vector> b; + std::vector> b; for (auto& it : nodesMissing) { @@ -146,8 +146,8 @@ public: // Don't use BEAST_EXPECT here b/c it will be called a // non-deterministic number of times and the number of tests run // should be deterministic - auto node = SHAMapTreeNode::makeFromWire(makeSlice(b[i].second)); - if (!destination.addKnownNode(b[i].first, std::move(node), nullptr).isUseful()) + auto node = SHAMapTreeNode::makeFromWire(makeSlice(std::get<1>(b[i]))); + if (!destination.addKnownNode(std::get<0>(b[i]), std::move(node), nullptr).isUseful()) fail("", __FILE__, __LINE__); } } while (true); diff --git a/src/xrpld/app/ledger/InboundLedger.h b/src/xrpld/app/ledger/InboundLedger.h index 46a8cf19b5..c3da38d29c 100644 --- a/src/xrpld/app/ledger/InboundLedger.h +++ b/src/xrpld/app/ledger/InboundLedger.h @@ -137,10 +137,10 @@ private: receiveNode(protocol::TMLedgerData& packet, SHAMapAddNode&); bool - takeTxRootNode(Slice const& data, SHAMapAddNode&); + takeTxRootNode(std::string const& data, SHAMapAddNode&); bool - takeAsRootNode(Slice const& data, SHAMapAddNode&); + takeAsRootNode(std::string const& data, SHAMapAddNode&); std::vector neededTxHashes(int max, SHAMapSyncFilter* filter) const; diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp index 10ddc1aea8..102dd821f4 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp @@ -827,26 +827,26 @@ InboundLedger::receiveNode(protocol::TMLedgerData& packet, SHAMapAddNode& san) return; } - auto tree_node = getTreeNode(ledger_node.nodedata()); - if (!tree_node) + auto treeNode = getTreeNode(ledger_node.nodedata()); + if (!treeNode) { JLOG(journal_.warn()) << "Got invalid node data"; san.incInvalid(); return; } - auto const node_id = getSHAMapNodeID(app_, ledger_node, *tree_node); - if (!node_id) + auto const nodeID = getSHAMapNodeID(app_, ledger_node, *treeNode); + if (!nodeID) { JLOG(journal_.warn()) << "Got invalid node id"; san.incInvalid(); return; } - if (node_id->isRoot()) - san += map.addRootNode(rootHash, std::move(*tree_node), f); + if (nodeID->isRoot()) + san += map.addRootNode(rootHash, std::move(*treeNode), f); else - san += map.addKnownNode(*node_id, std::move(*tree_node), f); + san += map.addKnownNode(*nodeID, std::move(*treeNode), f); if (!san.isGood()) { @@ -874,7 +874,7 @@ InboundLedger::receiveNode(protocol::TMLedgerData& packet, SHAMapAddNode& san) Call with a lock */ bool -InboundLedger::takeAsRootNode(Slice const& data, SHAMapAddNode& san) +InboundLedger::takeAsRootNode(std::string const& data, SHAMapAddNode& san) { if (failed_ || mHaveState) { @@ -890,15 +890,16 @@ InboundLedger::takeAsRootNode(Slice const& data, SHAMapAddNode& san) // LCOV_EXCL_STOP } - AccountStateSF filter(mLedger->stateMap().family().db(), app_.getLedgerMaster()); - auto node = SHAMapTreeNode::makeFromWire(data); - if (!node) + auto treeNode = getTreeNode(data); + if (!treeNode) { JLOG(journal_.warn()) << "Got invalid node data"; san.incInvalid(); return false; } - san += mLedger->stateMap().addRootNode(SHAMapHash{mLedger->header().accountHash}, node, &filter); + + AccountStateSF filter(mLedger->stateMap().family().db(), app_.getLedgerMaster()); + san += mLedger->stateMap().addRootNode(SHAMapHash{mLedger->header().accountHash}, *treeNode, &filter); return san.isGood(); } @@ -906,7 +907,7 @@ InboundLedger::takeAsRootNode(Slice const& data, SHAMapAddNode& san) Call with a lock */ bool -InboundLedger::takeTxRootNode(Slice const& data, SHAMapAddNode& san) +InboundLedger::takeTxRootNode(std::string const& data, SHAMapAddNode& san) { if (failed_ || mHaveTransactions) { @@ -922,15 +923,16 @@ InboundLedger::takeTxRootNode(Slice const& data, SHAMapAddNode& san) // LCOV_EXCL_STOP } - TransactionStateSF filter(mLedger->txMap().family().db(), app_.getLedgerMaster()); - auto node = SHAMapTreeNode::makeFromWire(data); - if (!node) + auto treeNode = getTreeNode(data); + if (!treeNode) { JLOG(journal_.warn()) << "Got invalid node data"; san.incInvalid(); return false; } - san += mLedger->txMap().addRootNode(SHAMapHash{mLedger->header().txHash}, node, &filter); + + TransactionStateSF filter(mLedger->txMap().family().db(), app_.getLedgerMaster()); + san += mLedger->txMap().addRootNode(SHAMapHash{mLedger->header().txHash}, *treeNode, &filter); return san.isGood(); } @@ -1024,14 +1026,12 @@ InboundLedger::processData(std::shared_ptr peer, protocol::TMLedgerData& p san.incUseful(); } - if (!mHaveState && (packet.nodes().size() > 1) && - !takeAsRootNode(makeSlice(packet.nodes(1).nodedata()), san)) + if (!mHaveState && (packet.nodes().size() > 1) && !takeAsRootNode(packet.nodes(1).nodedata(), san)) { JLOG(journal_.warn()) << "Included AS root invalid"; } - if (!mHaveTransactions && (packet.nodes().size() > 2) && - !takeTxRootNode(makeSlice(packet.nodes(2).nodedata()), san)) + if (!mHaveTransactions && (packet.nodes().size() > 2) && !takeTxRootNode(packet.nodes(2).nodedata(), san)) { JLOG(journal_.warn()) << "Included TX root invalid"; } diff --git a/src/xrpld/app/ledger/detail/InboundLedgers.cpp b/src/xrpld/app/ledger/detail/InboundLedgers.cpp index 9924244d96..3a1d467a51 100644 --- a/src/xrpld/app/ledger/detail/InboundLedgers.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedgers.cpp @@ -219,10 +219,10 @@ public: if (!validateLedgerNode(app_, ledger_node)) return; - auto const tree_node = getTreeNode(ledger_node.nodedata()); - if (!tree_node) + auto const treeNode = getTreeNode(ledger_node.nodedata()); + if (!treeNode) return; - auto const tn = *tree_node; + auto const tn = *treeNode; s.erase(); tn->serializeWithPrefix(s); diff --git a/src/xrpld/app/ledger/detail/InboundTransactions.cpp b/src/xrpld/app/ledger/detail/InboundTransactions.cpp index ffe4ac3d5b..07bff4dda3 100644 --- a/src/xrpld/app/ledger/detail/InboundTransactions.cpp +++ b/src/xrpld/app/ledger/detail/InboundTransactions.cpp @@ -140,23 +140,23 @@ public: return; } - auto const tree_node = getTreeNode(ledger_node.nodedata()); - if (!tree_node) + auto const treeNode = getTreeNode(ledger_node.nodedata()); + if (!treeNode) { JLOG(j_.warn()) << "Got invalid node data"; peer->charge(Resource::feeInvalidData, "node_data"); return; } - auto const node_id = getSHAMapNodeID(app_, ledger_node, *tree_node); - if (!node_id) + auto const nodeID = getSHAMapNodeID(app_, ledger_node, *treeNode); + if (!nodeID) { JLOG(j_.warn()) << "Got invalid node id"; peer->charge(Resource::feeInvalidData, "node_id"); return; } - data.emplace_back(std::make_pair(*node_id, *tree_node)); + data.emplace_back(std::make_pair(*nodeID, *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 new file mode 100644 index 0000000000..66774b66f3 --- /dev/null +++ b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp @@ -0,0 +1,165 @@ +#pragma once + +#include +#include +#include + +#include +#include +#include +#include +#include + +namespace xrpl { + +/** + * @brief Validates a ledger node based on the fixLedgerNodeID amendment status. + * + * This function checks whether a ledger node has the expected fields based on + * whether the fixLedgerNodeID amendment is enabled. The validation rules differ + * depending on the amendment state: + * + * When the amendment is enabled: + * - The node must have `nodedata`. + * - The legacy `nodeid` field must NOT be present. + * - For inner nodes: the `id` field must be present. + * - For leaf nodes: the `depth` field must be present and <= SHAMap::leafDepth. + * + * When the amendment is disabled: + * - The node must have `nodedata`. + * - The `nodeid` field must be present. + * - The new `id` and `depth` fields must NOT be present. + * + * @param app The application instance used to check amendment status. + * @param ledger_node The ledger node to validate. + * @return true if the ledger node has the expected fields, false otherwise. + */ +bool +validateLedgerNode(Application& app, protocol::TMLedgerNode const& ledger_node) +{ + if (!ledger_node.has_nodedata()) + return false; + + if (app.getAmendmentTable().isEnabled(fixLedgerNodeID)) + { + // Note that we cannot confirm here whether the node is actually an + // inner or leaf node, and will need to perform additional checks + // separately. + if (ledger_node.has_nodeid()) + return false; + if (ledger_node.has_id()) + return true; + return ledger_node.has_depth() && ledger_node.depth() <= SHAMap::leafDepth; + } + + if (ledger_node.has_id() || ledger_node.has_depth()) + return false; + return ledger_node.has_nodeid(); +} + +/** + * @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 std::nullopt instead of throwing an exception. + * + * @param data The serialized node data in wire format. + * @return An optional containing the deserialized tree node if successful, or + * std::nullopt if deserialization fails. + */ +std::optional> +getTreeNode(std::string const& data) +{ + auto const slice = makeSlice(data); + try + { + return SHAMapTreeNode::makeFromWire(slice); + } + catch (std::exception const&) + { + return std::nullopt; + } +} + +/** + * @brief Extracts or reconstructs the SHAMapNodeID from a ledger node. + * + * This function retrieves the SHAMapNodeID for a tree node, with behavior that + * depends on the fixLedgerNodeID amendment status and the node type (inner vs. + * leaf). + * + * When the fixLedgerNodeID amendment is enabled: + * - For inner nodes: Deserializes the node ID from the `ledger_node.id` field. + * Note that root nodes are also inner nodes. + * - For leaf nodes: Reconstructs the node ID using both the depth from the + * `ledger_node.depth` field and the key from the leaf node's item. + * + * When the amendment is disabled: + * - For all nodes: Deserializes the node ID from the `ledger_node.nodeid` + * field. + * - For leaf nodes: Validates that the node ID is consistent with the leaf's + * key. + * + * @param app The application instance used to check amendment status. + * @param ledger_node The 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. + */ +std::optional +getSHAMapNodeID( + Application& app, + protocol::TMLedgerNode const& ledger_node, + intr_ptr::SharedPtr const& treeNode) +{ + // When the amendment is enabled and a node depth is present, we can calculate the node ID. + if (app.getAmendmentTable().isEnabled(fixLedgerNodeID)) + { + if (treeNode->isInner()) + { + XRPL_ASSERT(ledger_node.has_id(), "xrpl::getSHAMapNodeID : node ID is present"); + if (!ledger_node.has_id()) + return std::nullopt; + + return deserializeSHAMapNodeID(ledger_node.id()); + } + + if (treeNode->isLeaf()) + { + XRPL_ASSERT(ledger_node.has_depth(), "xrpl::getSHAMapNodeID : node depth is present"); + if (!ledger_node.has_depth()) + return std::nullopt; + + auto const key = static_cast(treeNode.get())->peekItem()->key(); + return SHAMapNodeID::createID(ledger_node.depth(), key); + } + + UNREACHABLE("xrpl::getSHAMapNodeID : tree node is neither inner nor leaf"); + return std::nullopt; + } + + // When the amendment is disabled, we expect the node ID to always be present. For leaf nodes + // we perform an extra check to ensure the node's position in the tree is consistent with its + // content. + XRPL_ASSERT(ledger_node.has_nodeid(), "xrpl::getSHAMapNodeID : node ID is present"); + if (!ledger_node.has_nodeid()) + return std::nullopt; + + auto const nodeID = deserializeSHAMapNodeID(ledger_node.nodeid()); + if (!nodeID) + return std::nullopt; + + if (treeNode->isLeaf()) + { + auto const key = static_cast(treeNode.get())->peekItem()->key(); + auto const expected_id = SHAMapNodeID::createID(static_cast(nodeID->getDepth()), key); + if (nodeID->getNodeID() != expected_id.getNodeID()) + return std::nullopt; + } + + return nodeID; +} + +} // namespace xrpl diff --git a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h index 5892c8ee13..7dd0dd3511 100644 --- a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h +++ b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.h @@ -1,100 +1,83 @@ #pragma once #include -#include #include -#include #include #include #include namespace xrpl { -inline bool -validateLedgerNode(Application& app, protocol::TMLedgerNode const& ledger_node) -{ - if (!ledger_node.has_nodedata()) - return false; +/** + * @brief Validates a ledger node based on the fixLedgerNodeID amendment status. + * + * This function checks whether a ledger node has the expected fields based on + * whether the fixLedgerNodeID amendment is enabled. The validation rules differ + * depending on the amendment state: + * + * When the amendment is enabled: + * - The node must have `nodedata`. + * - The legacy `nodeid` field must NOT be present. + * - For inner nodes: the `id` field must be present. + * - For leaf nodes: the `depth` field must be present and <= SHAMap::leafDepth. + * + * When the amendment is disabled: + * - The node must have `nodedata`. + * - The `nodeid` field must be present. + * - The new `id` and `depth` fields must NOT be present. + * + * @param app The application instance used to check amendment status. + * @param ledger_node The ledger node to validate. + * @return true if the ledger node has the expected fields, false otherwise. + */ +bool +validateLedgerNode(Application& app, protocol::TMLedgerNode const& ledger_node); - // When the amendment is enabled, we expect the node ID to be present in the ledger node for - // inner nodes, and the node depth to be present for leaf nodes. As we cannot confirm whether - // the node is actually an inner or leaf node here, we will need to perform additional checks - // separately. - if (app.getAmendmentTable().isEnabled(fixLedgerNodeID)) - return ledger_node.has_id() || ledger_node.has_depth(); +/** + * @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 std::nullopt instead of throwing an exception. + * + * @param data The serialized node data in wire format. + * @return An optional containing the deserialized tree node if successful, or + * std::nullopt if deserialization fails. + */ +std::optional> +getTreeNode(std::string const& data); - // When the amendment is disabled, we expect the node ID to always be present. - return ledger_node.has_nodeid(); -} - -inline std::optional> -getTreeNode(std::string const& data) -{ - auto const slice = makeSlice(data); - try - { - return SHAMapTreeNode::makeFromWire(slice); - } - catch (std::exception const&) - { - // We can use expected instead once we support C++23. - return std::nullopt; - } -} - -inline std::optional +/** + * @brief Extracts or reconstructs the SHAMapNodeID from a ledger node. + * + * This function retrieves the SHAMapNodeID for a tree node, with behavior that + * depends on the fixLedgerNodeID amendment status and the node type (inner vs. + * leaf). + * + * When the fixLedgerNodeID amendment is enabled: + * - For inner nodes: Deserializes the node ID from the `ledger_node.id` field. + * Note that root nodes are also inner nodes. + * - For leaf nodes: Reconstructs the node ID using both the depth from the + * `ledger_node.depth` field and the key from the leaf node's item. + * + * When the amendment is disabled: + * - For all nodes: Deserializes the node ID from the `ledger_node.nodeid` + * field. + * - For leaf nodes: Validates that the node ID is consistent with the leaf's + * key. + * + * @param app The application instance used to check amendment status. + * @param ledger_node The 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. + */ +std::optional getSHAMapNodeID( Application& app, protocol::TMLedgerNode const& ledger_node, - intr_ptr::SharedPtr const& tree_node) -{ - // When the amendment is enabled and a node depth is present, we can calculate the node ID. - if (app.getAmendmentTable().isEnabled(fixLedgerNodeID)) - { - if (tree_node->isInner()) - { - XRPL_ASSERT(ledger_node.has_id(), "xrpl::getSHAMapNodeID : node ID is present"); - if (!ledger_node.has_id()) - return std::nullopt; - - return deserializeSHAMapNodeID(ledger_node.id()); - } - - if (tree_node->isLeaf()) - { - XRPL_ASSERT(ledger_node.has_depth(), "xrpl::getSHAMapNodeID : node depth is present"); - if (!ledger_node.has_depth()) - return std::nullopt; - - auto const key = static_cast(tree_node.get())->peekItem()->key(); - return SHAMapNodeID::createID(ledger_node.depth(), key); - } - - UNREACHABLE("xrpl::getSHAMapNodeID : tree node is neither inner nor leaf"); - return std::nullopt; - } - - // When the amendment is disabled, we expect the node ID to always be present. For leaf nodes - // we perform an extra check to ensure the node's position in the tree is consistent with its - // content. - XRPL_ASSERT(ledger_node.has_nodeid(), "xrpl::getSHAMapNodeID : node ID is present"); - if (!ledger_node.has_nodeid()) - return std::nullopt; - - auto const node_id = deserializeSHAMapNodeID(ledger_node.nodeid()); - if (!node_id) - return std::nullopt; - - if (tree_node->isLeaf()) - { - auto const key = static_cast(tree_node.get())->peekItem()->key(); - auto const expected_id = SHAMapNodeID::createID(static_cast(node_id->getDepth()), key); - if (node_id->getNodeID() != expected_id.getNodeID()) - return std::nullopt; - } - - return node_id; -} + intr_ptr::SharedPtr const& treeNode); } // namespace xrpl diff --git a/src/xrpld/app/ledger/detail/TransactionAcquire.cpp b/src/xrpld/app/ledger/detail/TransactionAcquire.cpp index e747792f7f..0eb8d975fc 100644 --- a/src/xrpld/app/ledger/detail/TransactionAcquire.cpp +++ b/src/xrpld/app/ledger/detail/TransactionAcquire.cpp @@ -144,7 +144,7 @@ TransactionAcquire::trigger(std::shared_ptr const& peer) SHAMapAddNode TransactionAcquire::takeNodes( - std::vector>> const& data, + std::vector>>& data, std::shared_ptr const& peer) { ScopedLockType sl(mtx_); @@ -168,7 +168,7 @@ TransactionAcquire::takeNodes( ConsensusTransSetSF sf(app_, app_.getTempNodeCache()); - for (auto const& d : data) + for (auto& d : data) { if (d.first.isRoot() && mHaveRoot) { diff --git a/src/xrpld/app/ledger/detail/TransactionAcquire.h b/src/xrpld/app/ledger/detail/TransactionAcquire.h index eb9eb4179a..071ca5bce5 100644 --- a/src/xrpld/app/ledger/detail/TransactionAcquire.h +++ b/src/xrpld/app/ledger/detail/TransactionAcquire.h @@ -20,7 +20,7 @@ public: SHAMapAddNode takeNodes( - std::vector>> const& data, + std::vector>>& data, std::shared_ptr const&); void diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index da19a7c5ca..0426127441 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -3133,7 +3133,7 @@ PeerImp::processLedgerRequest(std::shared_ptr const& m) { auto const queryDepth{m->has_querydepth() ? m->querydepth() : (isHighLatency() ? 2 : 1)}; - std::vector> data; + std::vector> data; for (int i = 0; i < m->nodeids_size() && ledgerData.nodes_size() < Tuning::softMaxReplyNodes; ++i) { @@ -3154,23 +3154,19 @@ PeerImp::processLedgerRequest(std::shared_ptr const& m) break; protocol::TMLedgerNode* node{ledgerData.add_nodes()}; - node->set_nodedata(d.second.data(), d.second.size()); + + auto const& node_data = std::get<1>(d); + node->set_nodedata(node_data.data(), node_data.size()); // When the amendment is disabled, we always set the node ID. However, when the amendment is // enabled, we only set it for inner nodes, while for leaf nodes we set the node depth instead. + auto const& nodeID = std::get<0>(d); if (!app_.getAmendmentTable().isEnabled(fixLedgerNodeID)) - { - node->set_nodeid(d.first.getRawString()); - continue; - } - - // TODO: Can we determine whether the node is an inner or leaf node without calling the - // makeFromWire function? - auto const node_slice = makeSlice(node->nodedata()); - if (auto const tree_node = SHAMapTreeNode::makeFromWire(node_slice); tree_node->isInner()) - node->set_id(d.first.getRawString()); - else if (tree_node->isLeaf()) - node->set_depth(d.first.getDepth()); + node->set_nodeid(nodeID.getRawString()); + else if (std::get<2>(d)) + node->set_depth(nodeID.getDepth()); + else + node->set_id(nodeID.getRawString()); } } else