diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h index 0baea78931..228f517797 100644 --- a/include/xrpl/shamap/SHAMap.h +++ b/include/xrpl/shamap/SHAMap.h @@ -261,12 +261,17 @@ public: lowerBound(uint256 const& id) const; /** - * Visit every node in this SHAMap + * Visit every node in this SHAMap. * - * @param function called with every node visited. - * If function returns false, visitNodes exits. + * The walk ends at the first node it cannot read, so a caller that needs a + * complete walk to be correct must check the return value. + * + * @param function Called with every node visited. Returning false from it + * ends the walk, which on its own keeps the result true. + * @return True when the walk read every node it reached. False when a node + * could not be read. */ - void + bool visitNodes(std::function const& function) const; /** @@ -280,11 +285,12 @@ public: visitDifferences(SHAMap const* have, std::function const&) const; /** - * Visit every leaf node in this SHAMap + * Visit every leaf node in this SHAMap. * - * @param function called with every non inner node visited. + * @param function Called with every non-inner node visited. + * @return What visitNodes reports about the walk. */ - void + bool visitLeaves(std::function const&)> const&) const; // comparison/sync functions diff --git a/src/libxrpl/shamap/SHAMap.cpp b/src/libxrpl/shamap/SHAMap.cpp index 0e28c0222a..797d6e8712 100644 --- a/src/libxrpl/shamap/SHAMap.cpp +++ b/src/libxrpl/shamap/SHAMap.cpp @@ -326,9 +326,6 @@ SHAMap::descend(SHAMapInnerNode& parent, unsigned int branch) const return node; node = fetchNode(parent.getChildHash(branch)); - if (!node) - return {}; - node = parent.canonicalizeChild(branch, std::move(node)); return node; } @@ -340,7 +337,7 @@ SHAMap::descendNoStore(SHAMapInnerNode& parent, unsigned int branch) const { SHAMapTreeNodePtr ret = parent.getChild(branch); if (!ret && backed_) - ret = fetchNode(parent.getChildHash(branch)); + ret = fetchNodeNT(parent.getChildHash(branch)); return ret; } diff --git a/src/libxrpl/shamap/SHAMapDelta.cpp b/src/libxrpl/shamap/SHAMapDelta.cpp index 1306fe6990..d20901755c 100644 --- a/src/libxrpl/shamap/SHAMapDelta.cpp +++ b/src/libxrpl/shamap/SHAMapDelta.cpp @@ -13,9 +13,11 @@ #include #include +#include #include #include #include +#include #include #include #include @@ -283,21 +285,36 @@ bool SHAMap::walkMapParallel(std::vector& missingNodes, int maxMissing) const { if (!root_->isInner()) // root_ is only node, and we have it - return false; + return true; + + // Only the nodes this call records count towards the result, so remember what the + // caller already had. + auto const initialMissing = missingNodes.size(); using StackEntry = intr_ptr::SharedPtr; std::array topChildren; { + // This loop runs before the workers start, so it needs no lock. auto const& innerRoot = intr_ptr::staticPointerCast(root_); for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { - if (!innerRoot->isEmptyBranch(i)) - topChildren[i] = descendNoStore(*innerRoot, i); + if (innerRoot->isEmptyBranch(i)) + continue; + + topChildren[i] = descendNoStore(*innerRoot, i); + if (!topChildren[i]) + { + // A root child that cannot be read hides its whole subtree. Record it here, + // because the loop below skips a null child without visiting it. + missingNodes.emplace_back(type_, innerRoot->getChildHash(i)); + if (--maxMissing <= 0) + return false; + } } } std::vector workers; workers.reserve(SHAMapInnerNode::kBranchFactor); - std::vector exceptions; + std::vector exceptions; exceptions.reserve(SHAMapInnerNode::kBranchFactor); std::array>, SHAMapInnerNode::kBranchFactor> @@ -346,6 +363,14 @@ SHAMap::walkMapParallel(std::vector& missingNodes, int maxMis else { std::scoped_lock const l{m}; + + // Another worker may have spent the budget while this one + // was walking, so test it before adding to the list and not + // only after. Testing only after lets every worker still + // running add one more node than the caller asked for. + if (maxMissing <= 0) + return; + missingNodes.emplace_back(type_, node->getChildHash(i)); if (--maxMissing <= 0) return; @@ -353,10 +378,14 @@ SHAMap::walkMapParallel(std::vector& missingNodes, int maxMis } } } - catch (SHAMapMissingNode const& e) + catch (std::exception const& e) { + // LCOV_EXCL_START + // A worker must not let an exception leave its thread, so record it + // and let the join below report it. std::scoped_lock const l(m); - exceptions.push_back(e); + exceptions.emplace_back(e.what()); + // LCOV_EXCL_STOP } }, std::move(nodeStacks[rootChildIndex])); @@ -366,14 +395,29 @@ SHAMap::walkMapParallel(std::vector& missingNodes, int maxMis worker.join(); std::scoped_lock const l(m); - if (exceptions.empty()) - return true; - std::stringstream ss; - ss << "Exception(s) in ledger load: "; - for (auto const& e : exceptions) - ss << e.what() << ", "; - JLOG(journal_.error()) << ss.str(); - return false; + if (!exceptions.empty()) + { + // LCOV_EXCL_START + std::stringstream ss; + ss << "Exception(s) in ledger load: "; + for (auto const& e : exceptions) + ss << e << ", "; + JLOG(journal_.error()) << ss.str(); + return false; + // LCOV_EXCL_STOP + } + + // A node the workers could not read is recorded in `missingNodes` rather than thrown, + // so the result has to consult it too. + auto const found = missingNodes.size() - initialMissing; + if (found != 0) + { + JLOG(journal_.error()) << "Missing node(s) in ledger load: " << found + << ", first: " << missingNodes[initialMissing].what(); + return false; + } + + return true; } } // namespace xrpl diff --git a/src/libxrpl/shamap/SHAMapSync.cpp b/src/libxrpl/shamap/SHAMapSync.cpp index 602d8e629c..db2102f586 100644 --- a/src/libxrpl/shamap/SHAMapSync.cpp +++ b/src/libxrpl/shamap/SHAMapSync.cpp @@ -31,28 +31,36 @@ namespace xrpl { -void +bool SHAMap::visitLeaves( std::function const& item)> const& leafFunction) const { - visitNodes([&leafFunction](SHAMapTreeNode& node) { + return visitNodes([&leafFunction](SHAMapTreeNode& node) { if (!node.isInner()) leafFunction(safeDowncast(node).peekItem()); return true; }); } -void +bool SHAMap::visitNodes(std::function const& function) const { if (!root_) - return; + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::visitNodes : non-null root"); + return true; + // LCOV_EXCL_STOP + } - function(*root_); + // The root is readable by definition, so a stop here still reports a complete walk. + if (!function(*root_)) + return true; + // A map whose root is a leaf holds one node, and this walk just visited it. if (!root_->isInner()) - return; + return true; using StackEntry = std::pair>; std::stack> stack; @@ -67,8 +75,17 @@ SHAMap::visitNodes(std::function const& function) const if (!node->isEmptyBranch(pos)) { SHAMapTreeNodePtr const child = descendNoStore(*node, pos); + if (!child) + { + // No caller can use a partial walk, so end it here rather than pay + // for the rest of the map. The callers that act on the result log + // it at a level an operator reads, so keep this one at debug. + JLOG(journal_.debug()) + << "visitNodes: unreadable child node " << node->getChildHash(pos); + return false; + } if (!function(*child)) - return; + return true; if (child->isLeaf()) { @@ -103,6 +120,8 @@ SHAMap::visitNodes(std::function const& function) const std::tie(pos, node) = stack.top(); stack.pop(); } + + return true; } void diff --git a/src/test/app/LedgerReplay_test.cpp b/src/test/app/LedgerReplay_test.cpp index bb0c790e37..351914e965 100644 --- a/src/test/app/LedgerReplay_test.cpp +++ b/src/test/app/LedgerReplay_test.cpp @@ -28,17 +28,28 @@ #include #include +#include #include #include #include +#include #include #include +#include +#include +#include #include +#include #include +#include #include #include #include +#include +#include #include +#include // SHAMapType +#include #include @@ -64,6 +75,7 @@ #include #include #include +#include #include namespace xrpl::test { @@ -1090,6 +1102,111 @@ struct LedgerReplayer_test : public beast::unit_test::Suite } } + /** + * A ledger whose transaction map cannot be read in full must draw an error reply + * rather than a short transaction list. + * + * The two maps are built in memory and only part of them is written to the store: the + * whole state map, so the ledger loads, and the transaction map's root alone. The + * nodes below that root are never written, so the ledger the handler reads back from + * the store cannot reach them. + */ + void + testReplayDeltaIncompleteTxMap() + { + static constexpr auto kItems = 200; + static constexpr auto kSeed = 0xdeadbeefU; + + // Far enough ahead of the server's own history that the missing-node handler finds + // no hash to acquire and returns at once. + static constexpr auto kSeqAhead = 1000; + + testcase("ReplayDelta incomplete tx map"); + + LedgerServer server(*this, {.initLedgers = 1}); + auto& family = server.app.getNodeFamily(); + + // The items only have to be well formed, because the walk stops before the handler + // reads any of them. + beast::xor_shift_engine engine{kSeed}; + auto const makeItem = [&engine]() { + static constexpr auto kWordsPerItem = 3; + + Serializer s; + for (auto word = 0; word < kWordsPerItem; ++word) + s.add32(randInt(engine)); + return makeShamapitem(s.getSHA512Half(), s.slice()); + }; + + SHAMap stateMap{SHAMapType::STATE, family}; + SHAMap txMap{SHAMapType::TRANSACTION, family}; + for (auto i = 0; i < kItems; ++i) + { + stateMap.addItem(SHAMapNodeType::TnAccountState, makeItem()); + txMap.addItem(SHAMapNodeType::TnTransactionNm, makeItem()); + } + stateMap.setImmutable(); + txMap.setImmutable(); + + // Read both hashes before storing anything. getHash() is what computes the node + // hashes, through unshare(), so storing first would write every node under a stale + // hash and nothing would be findable afterwards. + LedgerHeader header = server.ledgerMaster.getClosedLedger()->header(); + header.seq += kSeqAhead; + header.accountHash = stateMap.getHash().asUInt256(); + header.txHash = txMap.getHash().asUInt256(); + header.hash = calculateLedgerHash(header); + + // Write a map's nodes into the store. visitNodes reports the root first, so + // rootOnly lets that one call through and stops at the next. + auto const storeMap = [&family](SHAMap const& map, bool rootOnly) { + auto stored = 0; + map.visitNodes([&family, rootOnly, &stored](SHAMapTreeNode& node) { + if (rootOnly && stored > 0) + return false; + + Serializer s; + node.serializeWithPrefix(s); + family.db().store( + NodeObjectType::AccountNode, + std::move(s.modData()), + node.getHash().asUInt256(), + 0); + ++stored; + return true; + }); + return stored; + }; + + BEAST_EXPECT(storeMap(stateMap, false) > 1); + BEAST_EXPECT(storeMap(txMap, true) == 1); + + bool loaded = false; + auto const holed = std::make_shared( + header, + loaded, + false, + server.ledgerMaster.getClosedLedger()->rules(), + server.ledgerMaster.getClosedLedger()->fees(), + family, + server.app.getLogs().journal("Ledger")); + + // Both roots are readable, so the ledger loads and the handler accepts it. + BEAST_EXPECT(loaded); + BEAST_EXPECT(holed->isImmutable()); + server.ledgerMaster.storeLedger(holed); + + auto request = std::make_shared(); + request->set_ledgerhash(header.hash.data(), header.hash.size()); + auto const reply = server.msgHandler.processReplayDeltaRequest(request); + + // The reply names the missing nodes and carries no partial payload. + BEAST_EXPECT(reply.has_error()); + BEAST_EXPECT(reply.error() == protocol::TMReplyError::reNO_NODE); + BEAST_EXPECT(reply.transaction_size() == 0); + BEAST_EXPECT(!reply.has_ledgerheader()); + } + void testTaskParameter() { @@ -1492,6 +1609,7 @@ struct LedgerReplayer_test : public beast::unit_test::Suite { testProofPath(); testReplayDelta(); + testReplayDeltaIncompleteTxMap(); testTruncatedHeader(); testTaskParameter(); testConfig(); diff --git a/src/tests/libxrpl/shamap/SHAMapMissingNode.cpp b/src/tests/libxrpl/shamap/SHAMapMissingNode.cpp new file mode 100644 index 0000000000..9e7993c01e --- /dev/null +++ b/src/tests/libxrpl/shamap/SHAMapMissingNode.cpp @@ -0,0 +1,501 @@ +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include + +#include +#include +#include +#include + +namespace xrpl::tests { + +namespace { + +// Fixed seed, so a failure reproduces when the test runs on its own. +constexpr std::uint32_t kSeed = 0xdeadbeefU; + +// Largest number of nodes getMissingNodes may report per round. +constexpr int kMaxNodesPerRequest = 2048; + +/** + * Copy part of a SHAMap into another SHAMap. + * + * The function answers at most nodeLimit of the requests that + * dest.getMissingNodes() makes. The remaining subtrees stay absent from the + * destination family's database, which is the state this test needs. + * + * @param source The complete map to copy from. + * @param dest The map to copy into. It must be in the synching state. + * @param nodeLimit The largest number of missing-node requests to answer. + * @return Nothing. The function reports a fatal gtest failure on error, so + * wrap the call in ASSERT_NO_FATAL_FAILURE. + */ +void +copyPartialMap(SHAMap const& source, SHAMap& dest, std::size_t nodeLimit) +{ + std::vector rootData; + if (!source.getNodeFat(SHAMapNodeID{}, rootData, false, 0)) + FAIL() << "Could not get root node"; + + auto rootNode = SHAMapTreeNode::makeFromWire(makeSlice(rootData[0].data)); + if (!rootNode) + FAIL() << "Could not deserialize root node"; + if (!dest.addRootNode(source.getHash(), std::move(rootNode), nullptr).isGood()) + FAIL() << "Could not add root node"; + + std::size_t answered = 0; + while (answered < nodeLimit) + { + auto const missing = dest.getMissingNodes(kMaxNodesPerRequest, nullptr); + if (missing.empty()) + break; + + for (auto const& request : missing) + { + if (answered >= nodeLimit) + return; + ++answered; + + std::vector nodeData; + if (!source.getNodeFat(request.first, nodeData, false, 0)) + continue; + + for (auto const& entry : nodeData) + { + auto node = SHAMapTreeNode::makeFromWire(makeSlice(entry.data)); + if (!node) + FAIL() << "Could not deserialize node " << entry.nodeID; + if (!dest.addKnownNode(entry.nodeID, std::move(node), nullptr).isGood()) + FAIL() << "Could not add known node " << entry.nodeID; + } + } + } +} + +/** + * Lift a leaf out of a one-item map in the wire format a peer would send it in. + * + * A locally built map always has an inner node for its root, so this is how a + * test reaches the one-node shape that addRootNode accepts. + * + * @param map A map holding exactly one item. + * @param wire Set to the leaf's wire form. + * @param hash Set to the leaf's hash. + * @return Nothing. The function reports a fatal gtest failure on error, so + * wrap the call in ASSERT_NO_FATAL_FAILURE. + */ +void +extractSoleLeaf(SHAMap const& map, Blob& wire, SHAMapHash& hash) +{ + auto leaves = 0; + map.visitNodes([&](SHAMapTreeNode& node) { + if (!node.isInner()) + { + ++leaves; + Serializer s; + node.serializeForWire(s); + wire = s.modData(); + hash = node.getHash(); + } + return true; + }); + if (leaves != 1) + FAIL() << "Expected one leaf, found " << leaves; +} + +struct NodeCounts +{ + bool complete; + int inner; + int leaves; +}; + +/** + * Count the inner and leaf nodes that visitNodes reaches. + * + * @param map The map to walk. + * @return The counts, plus what visitNodes reported about the walk. + */ +NodeCounts +countNodes(SHAMap const& map) +{ + int inner = 0; + int leaves = 0; + bool const complete = map.visitNodes([&inner, &leaves](SHAMapTreeNode& node) { + if (node.isInner()) + { + ++inner; + } + else + { + ++leaves; + } + return true; + }); + return {.complete = complete, .inner = inner, .leaves = leaves}; +} + +} // namespace + +// visitLeaves on a map with unreadable child nodes must not throw. It must end the walk +// at the first one and report that the walk was incomplete. +TEST(SHAMapMissingNode, visit_leaves_stops_at_an_unreadable_node) +{ + static constexpr auto kItems = 200; + static constexpr auto kNodesToCopy = 3uz; + + beast::Journal const j{TestSink::instance()}; + beast::xor_shift_engine engine{kSeed}; + TestNodeFamily sourceFamily{j}; + TestNodeFamily destFamily{j}; + + SHAMap source{SHAMapType::FREE, sourceFamily}; + for (auto i = 0; i < kItems; ++i) + source.addItem(SHAMapNodeType::TnAccountState, makeRandomAccountStateItem(engine)); + source.setImmutable(); + + // The source map is complete, so it holds one leaf per item added. + auto beforeCount = 0; + EXPECT_TRUE(source.visitLeaves([&beforeCount](auto const&) { ++beforeCount; })); + EXPECT_EQ(beforeCount, kItems); + + // Copy only a few nodes, so most subtrees stay absent from destFamily's + // database. This is the state a node reaches when a child is evicted after + // a database rotation, or when a sync is still incomplete. + SHAMap dest{SHAMapType::FREE, source.getHash().asUInt256(), destFamily}; + dest.setSynching(); + ASSERT_NO_FATAL_FAILURE(copyPartialMap(source, dest, kNodesToCopy)); + + // The walk must return, must report that it was incomplete, and must reach fewer + // leaves than the source holds. + auto afterCount = 0; + auto complete = true; + EXPECT_NO_THROW(complete = dest.visitLeaves([&afterCount](auto const&) { ++afterCount; })); + EXPECT_FALSE(complete); + EXPECT_LT(afterCount, beforeCount); +} + +// walkMap must report the nodes it cannot read rather than throwing, so that +// Ledger::walkLedger can return false and let its caller re-acquire the ledger. +TEST(SHAMapMissingNode, walk_map_reports_missing_nodes) +{ + static constexpr auto kItems = 200; + static constexpr auto kNodesToCopy = 3uz; + static constexpr auto kMaxMissing = 32; + + beast::Journal const j{TestSink::instance()}; + beast::xor_shift_engine engine{kSeed}; + TestNodeFamily sourceFamily{j}; + TestNodeFamily destFamily{j}; + + SHAMap source{SHAMapType::FREE, sourceFamily}; + for (auto i = 0; i < kItems; ++i) + source.addItem(SHAMapNodeType::TnAccountState, makeRandomAccountStateItem(engine)); + source.setImmutable(); + + SHAMap dest{SHAMapType::FREE, source.getHash().asUInt256(), destFamily}; + dest.setSynching(); + ASSERT_NO_FATAL_FAILURE(copyPartialMap(source, dest, kNodesToCopy)); + + std::vector missing; + EXPECT_NO_THROW(dest.walkMap(missing, kMaxMissing)); + EXPECT_FALSE(missing.empty()); + + // A complete map reports nothing missing. + std::vector none; + EXPECT_NO_THROW(source.walkMap(none, kMaxMissing)); + EXPECT_TRUE(none.empty()); +} + +// walkMapParallel must report a partial map as incomplete. Its workers record an +// unreadable child instead of throwing, so the result has to consult that list and not +// only the exceptions the workers caught. +TEST(SHAMapMissingNode, walk_map_parallel_reports_missing_nodes) +{ + static constexpr auto kItems = 200; + static constexpr auto kNodesToCopy = 3uz; + static constexpr auto kMaxMissing = 32; + + beast::Journal const j{TestSink::instance()}; + beast::xor_shift_engine engine{kSeed}; + TestNodeFamily sourceFamily{j}; + TestNodeFamily destFamily{j}; + + SHAMap source{SHAMapType::FREE, sourceFamily}; + for (auto i = 0; i < kItems; ++i) + source.addItem(SHAMapNodeType::TnAccountState, makeRandomAccountStateItem(engine)); + source.setImmutable(); + + SHAMap dest{SHAMapType::FREE, source.getHash().asUInt256(), destFamily}; + dest.setSynching(); + ASSERT_NO_FATAL_FAILURE(copyPartialMap(source, dest, kNodesToCopy)); + + std::vector missing; + EXPECT_FALSE(dest.walkMapParallel(missing, kMaxMissing)); + EXPECT_FALSE(missing.empty()); + + // A complete map reports nothing missing. + std::vector none; + EXPECT_TRUE(source.walkMapParallel(none, kMaxMissing)); + EXPECT_TRUE(none.empty()); +} + +// An empty branch of the root holds nothing, so the parallel walk must skip it rather +// than report it missing. Three items cannot fill sixteen branches, whereas the larger +// maps in this file leave none of them empty. +TEST(SHAMapMissingNode, walk_map_parallel_skips_empty_root_branches) +{ + static constexpr auto kItems = 3; + static constexpr auto kMaxMissing = 32; + + beast::Journal const j{TestSink::instance()}; + beast::xor_shift_engine engine{kSeed}; + TestNodeFamily family{j}; + + SHAMap map{SHAMapType::FREE, family}; + for (auto i = 0; i < kItems; ++i) + map.addItem(SHAMapNodeType::TnAccountState, makeRandomAccountStateItem(engine)); + map.setImmutable(); + + // The root has to be inner, or the walk returns before it reads a single branch. + auto const [complete, inner, leaves] = countNodes(map); + EXPECT_TRUE(complete); + EXPECT_EQ(leaves, kItems); + EXPECT_GT(inner, 0); + + std::vector missing; + EXPECT_TRUE(map.walkMapParallel(missing, kMaxMissing)); + EXPECT_TRUE(missing.empty()); +} + +// walkMapParallel reads the root's children before it starts any worker, and skips a null +// one. That pass has to record the miss itself, or a map holding nothing but its root +// reports as complete. +TEST(SHAMapMissingNode, walk_map_parallel_reports_missing_root_children) +{ + static constexpr auto kItems = 200; + static constexpr auto kMaxMissing = 32; + + beast::Journal const j{TestSink::instance()}; + beast::xor_shift_engine engine{kSeed}; + TestNodeFamily sourceFamily{j}; + TestNodeFamily destFamily{j}; + + SHAMap source{SHAMapType::FREE, sourceFamily}; + for (auto i = 0; i < kItems; ++i) + source.addItem(SHAMapNodeType::TnAccountState, makeRandomAccountStateItem(engine)); + source.setImmutable(); + + // Copy the root and nothing else, so every one of its children is unreadable and no + // worker ever runs. + SHAMap dest{SHAMapType::FREE, source.getHash().asUInt256(), destFamily}; + dest.setSynching(); + ASSERT_NO_FATAL_FAILURE(copyPartialMap(source, dest, 0)); + + std::vector missing; + EXPECT_FALSE(dest.walkMapParallel(missing, kMaxMissing)); + EXPECT_FALSE(missing.empty()); +} + +// Stopping the walk through the visitor is not the same thing as an unreadable node, so it +// must not make the result false. A stop at the root counts too; the root's answer used to +// be discarded. +TEST(SHAMapMissingNode, early_stop_on_a_complete_map_reports_complete) +{ + static constexpr auto kItems = 200; + static constexpr auto kVisitsBeforeStop = 3; + + beast::Journal const j{TestSink::instance()}; + beast::xor_shift_engine engine{kSeed}; + TestNodeFamily family{j}; + + SHAMap map{SHAMapType::FREE, family}; + for (auto i = 0; i < kItems; ++i) + map.addItem(SHAMapNodeType::TnAccountState, makeRandomAccountStateItem(engine)); + map.setImmutable(); + + auto visits = 0; + EXPECT_TRUE(map.visitNodes([&visits](SHAMapTreeNode&) { + ++visits; + return visits < kVisitsBeforeStop; + })); + EXPECT_EQ(visits, kVisitsBeforeStop); + + // Stopping at the root leaves the rest of the map unvisited. + auto rootVisits = 0; + EXPECT_TRUE(map.visitNodes([&rootVisits](SHAMapTreeNode&) { + ++rootVisits; + return false; + })); + EXPECT_EQ(rootVisits, 1); +} + +// A map holding nothing but its root cannot read any branch, so the walk visits the root +// alone and reports itself incomplete. +TEST(SHAMapMissingNode, a_root_only_map_reports_incomplete) +{ + static constexpr auto kItems = 200; + + beast::Journal const j{TestSink::instance()}; + beast::xor_shift_engine engine{kSeed}; + TestNodeFamily sourceFamily{j}; + TestNodeFamily destFamily{j}; + + SHAMap source{SHAMapType::FREE, sourceFamily}; + for (auto i = 0; i < kItems; ++i) + source.addItem(SHAMapNodeType::TnAccountState, makeRandomAccountStateItem(engine)); + source.setImmutable(); + + SHAMap dest{SHAMapType::FREE, source.getHash().asUInt256(), destFamily}; + dest.setSynching(); + ASSERT_NO_FATAL_FAILURE(copyPartialMap(source, dest, 0)); + + auto visits = 0; + EXPECT_FALSE(dest.visitNodes([&visits](SHAMapTreeNode&) { + ++visits; + return true; + })); + EXPECT_EQ(visits, 1); +} + +// visitNodes on a complete map must reach every node. +TEST(SHAMapMissingNode, visit_nodes_complete_map) +{ + static constexpr auto kItems = 50; + + beast::Journal const j{TestSink::instance()}; + beast::xor_shift_engine engine{kSeed}; + TestNodeFamily family{j}; + + SHAMap map{SHAMapType::FREE, family}; + for (auto i = 0; i < kItems; ++i) + map.addItem(SHAMapNodeType::TnAccountState, makeRandomAccountStateItem(engine)); + map.setImmutable(); + + // The walk must report itself complete, and the map must hold one leaf per item + // added, plus the inner nodes that hold those leaves. + auto const [complete, inner, leaves] = countNodes(map); + EXPECT_TRUE(complete); + EXPECT_EQ(leaves, kItems); + EXPECT_GT(inner, 0); +} + +// The budget caps what a walk records. Once it is spent the walk stops, and the parallel +// walk spends it on the root's children before it starts a worker. +TEST(SHAMapMissingNode, a_spent_missing_node_budget_stops_the_walk) +{ + static constexpr auto kItems = 200; + static constexpr auto kMaxMissing = 1; + + beast::Journal const j{TestSink::instance()}; + beast::xor_shift_engine engine{kSeed}; + TestNodeFamily sourceFamily{j}; + TestNodeFamily destFamily{j}; + + SHAMap source{SHAMapType::FREE, sourceFamily}; + for (auto i = 0; i < kItems; ++i) + source.addItem(SHAMapNodeType::TnAccountState, makeRandomAccountStateItem(engine)); + source.setImmutable(); + + // Copy the root and nothing else, so more than one child is unreadable and the + // budget of one runs out. + SHAMap dest{SHAMapType::FREE, source.getHash().asUInt256(), destFamily}; + dest.setSynching(); + ASSERT_NO_FATAL_FAILURE(copyPartialMap(source, dest, 0)); + + std::vector serial; + EXPECT_NO_THROW(dest.walkMap(serial, kMaxMissing)); + EXPECT_EQ(serial.size(), 1u); + + std::vector parallel; + EXPECT_FALSE(dest.walkMapParallel(parallel, kMaxMissing)); + EXPECT_EQ(parallel.size(), 1u); +} + +// A map whose root is a leaf holds one node and has it, so every walk reports it +// complete. addRootNode is the only way into that shape. +TEST(SHAMapMissingNode, a_leaf_rooted_map_reports_complete) +{ + static constexpr auto kMaxMissing = 32; + + beast::Journal const j{TestSink::instance()}; + beast::xor_shift_engine engine{kSeed}; + TestNodeFamily sourceFamily{j}; + TestNodeFamily destFamily{j}; + + SHAMap source{SHAMapType::FREE, sourceFamily}; + source.addItem(SHAMapNodeType::TnAccountState, makeRandomAccountStateItem(engine)); + source.setImmutable(); + + Blob wire; + SHAMapHash hash; + ASSERT_NO_FATAL_FAILURE(extractSoleLeaf(source, wire, hash)); + + SHAMap dest{SHAMapType::FREE, hash.asUInt256(), destFamily}; + dest.setSynching(); + auto leaf = SHAMapTreeNode::makeFromWire(makeSlice(wire)); + ASSERT_TRUE(leaf); + ASSERT_TRUE(dest.addRootNode(hash, std::move(leaf), nullptr).isGood()); + + auto visits = 0; + EXPECT_TRUE(dest.visitNodes([&visits](SHAMapTreeNode&) { + ++visits; + return true; + })); + EXPECT_EQ(visits, 1); + + std::vector serial; + EXPECT_NO_THROW(dest.walkMap(serial, kMaxMissing)); + EXPECT_TRUE(serial.empty()); + + std::vector parallel; + EXPECT_TRUE(dest.walkMapParallel(parallel, kMaxMissing)); + EXPECT_TRUE(parallel.empty()); +} + +// The missing-node budget belongs to the whole walk, not to each worker. Every worker that +// is still running shares one counter, so it has to be tested before a node is added to the +// list. Testing it only afterwards let a 16-worker walk record 17 nodes against a cap of 2. +TEST(SHAMapMissingNode, a_missing_node_budget_is_shared_by_every_worker) +{ + static constexpr auto kItems = 200; + static constexpr auto kMaxMissing = 2; + + // Enough to make all sixteen of the root's children readable inner nodes, so sixteen + // workers start and each one finds unreadable children below its own. + static constexpr auto kNodesToCopy = 16uz; + + beast::Journal const j{TestSink::instance()}; + beast::xor_shift_engine engine{kSeed}; + TestNodeFamily sourceFamily{j}; + TestNodeFamily destFamily{j}; + + SHAMap source{SHAMapType::FREE, sourceFamily}; + for (auto i = 0; i < kItems; ++i) + source.addItem(SHAMapNodeType::TnAccountState, makeRandomAccountStateItem(engine)); + source.setImmutable(); + + SHAMap dest{SHAMapType::FREE, source.getHash().asUInt256(), destFamily}; + dest.setSynching(); + ASSERT_NO_FATAL_FAILURE(copyPartialMap(source, dest, kNodesToCopy)); + + std::vector missing; + EXPECT_FALSE(dest.walkMapParallel(missing, kMaxMissing)); + EXPECT_FALSE(missing.empty()); + EXPECT_LE(missing.size(), static_cast(kMaxMissing)); +} + +} // namespace xrpl::tests diff --git a/src/tests/libxrpl/shamap/SHAMapSync.cpp b/src/tests/libxrpl/shamap/SHAMapSync.cpp index e4bcbd8970..5403eb4a2d 100644 --- a/src/tests/libxrpl/shamap/SHAMapSync.cpp +++ b/src/tests/libxrpl/shamap/SHAMapSync.cpp @@ -4,7 +4,6 @@ #include #include #include -#include #include #include #include @@ -18,7 +17,6 @@ #include #include -#include #include #include #include @@ -34,13 +32,7 @@ protected: boost::intrusive_ptr makeRandomAS() { - static constexpr auto kWordsPerState = 3uz; - - Serializer s; - - for (auto word = 0uz; word < kWordsPerState; ++word) - s.add32(randInt(eng_)); - return makeShamapitem(s.getSHA512Half(), s.slice()); + return makeRandomAccountStateItem(eng_); } bool diff --git a/src/tests/libxrpl/shamap/common.h b/src/tests/libxrpl/shamap/common.h index 91401d2973..09f92bb6dc 100644 --- a/src/tests/libxrpl/shamap/common.h +++ b/src/tests/libxrpl/shamap/common.h @@ -4,16 +4,21 @@ #include #include #include +#include #include #include #include #include #include #include +#include #include #include +#include #include +#include + #include #include #include @@ -21,6 +26,25 @@ namespace xrpl::tests { +/** + * Build a SHAMapItem holding random account-state data. + * + * @param engine The random engine to draw the data from. Pass an engine with a + * fixed seed to make a test reproducible on its own. + * @return The new item. + */ +template +boost::intrusive_ptr +makeRandomAccountStateItem(Engine& engine) +{ + static constexpr auto kWordsPerState = 3uz; + + Serializer s; + for (auto word = 0uz; word < kWordsPerState; ++word) + s.add32(randInt(engine)); + return makeShamapitem(s.getSHA512Half(), s.slice()); +} + class TestNodeFamily : public Family { private: diff --git a/src/xrpld/app/consensus/RCLConsensus.cpp b/src/xrpld/app/consensus/RCLConsensus.cpp index 42270a91f1..b842fc4248 100644 --- a/src/xrpld/app/consensus/RCLConsensus.cpp +++ b/src/xrpld/app/consensus/RCLConsensus.cpp @@ -391,6 +391,8 @@ RCLConsensus::Adaptor::onClose( LedgerIndex const seq = prevLedger->header().seq + 1; CensorshipDetector::TxIDSeqVec proposed; + // initialSet is unbacked, so the walk has no node store to fail to read and + // cannot report an incomplete result. initialSet->visitLeaves( [&proposed, seq](boost::intrusive_ptr const& item) { proposed.emplace_back(item->key(), seq); @@ -539,6 +541,8 @@ RCLConsensus::Adaptor::doAccept( { std::vector accepted; + // The consensus transaction set is unbacked, so the walk has no node store to + // fail to read and cannot report an incomplete result. result.txns.map->visitLeaves( [&accepted](boost::intrusive_ptr const& item) { accepted.push_back(item->key()); diff --git a/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp b/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp index 6ed4a296ac..b72627cc17 100644 --- a/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp +++ b/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp @@ -222,9 +222,18 @@ LedgerReplayMsgHandler::processReplayDeltaRequest( reply.set_ledgerheader(nData.getDataPtr(), nData.getLength()); // pack transactions auto const& txMap = ledger->txMap(); - txMap.visitLeaves([&](boost::intrusive_ptr const& txNode) { - reply.add_transaction(txNode->data(), txNode->size()); - }); + if (!txMap.visitLeaves([&](boost::intrusive_ptr const& txNode) { + reply.add_transaction(txNode->data(), txNode->size()); + })) + { + // The transaction list above is incomplete, so name the missing nodes instead. + // The ledger itself is present, and an error reply carries no partial payload. + JLOG(journal_.debug()) << "getReplayDelta: Incomplete tx map for ledger " << ledgerHash; + reply.clear_transaction(); + reply.clear_ledgerheader(); + reply.set_error(protocol::TMReplyError::reNO_NODE); + return reply; + } JLOG(journal_.debug()) << "getReplayDelta for ledger " << ledgerHash << " txMap hash " << txMap.getHash().asUInt256(); diff --git a/src/xrpld/app/misc/SHAMapStoreImp.cpp b/src/xrpld/app/misc/SHAMapStoreImp.cpp index e19df597a2..33d82b4779 100644 --- a/src/xrpld/app/misc/SHAMapStoreImp.cpp +++ b/src/xrpld/app/misc/SHAMapStoreImp.cpp @@ -25,7 +25,6 @@ #include #include #include -#include #include #include @@ -408,17 +407,17 @@ SHAMapStoreImp::run() JLOG(journal_.debug()) << "copying ledger " << validatedSeq; std::uint64_t nodeCount = 0; - try - { - validatedLedger->stateMap().snapShot(false)->visitNodes( + // A partial copy must not be followed by a rotation: clearPrior() above has + // already advanced minimumOnline_, and rotate() below deletes the archive, so + // a node the walk did not reach would have no remaining copy. Abandon this + // cycle and retry on a later ledger instead. + if (!validatedLedger->stateMap().snapShot(false)->visitNodes( [this, &nodeCount](SHAMapTreeNode const& node) { return copyNode(nodeCount, node); - }); - } - catch (SHAMapMissingNode const& e) + })) { - JLOG(journal_.error()) - << "Missing node while copying ledger before rotate: " << e.what(); + JLOG(journal_.error()) << "Missing node while copying ledger " << validatedSeq + << " before rotate; abandoning this rotation"; continue; }