diff --git a/src/test/app/InboundLedger_test.cpp b/src/test/app/InboundLedger_test.cpp index 06d57269c2..a8e564610d 100644 --- a/src/test/app/InboundLedger_test.cpp +++ b/src/test/app/InboundLedger_test.cpp @@ -522,6 +522,74 @@ struct InboundLedger_test : public beast::unit_test::Suite BEAST_EXPECT(waitFor([&] { return env.app().getInboundLedgers().isFailure(header.hash); })); } + /** + * A packet for an abandoned map must fail rather than be read as the map + * finishing. + * + * addKnownNode() reports every node a duplicate rather than invalid once + * the map is no longer Synching (see SHAMap::addKnownNode), so + * receiveNode() must read isValid() rather than isSynching() alone. + * + * The tx hash never resolves, so haveTransactions_ stays false and + * nothing but the check under test can make this packet report success. + * + * @param env The environment to run in. + */ + void + testInvalidatedMapFailsReceiveNode(jtx::Env& env) + { + testcase("A packet for an abandoned map fails rather than completes"); + + // The fabricated chain, so feeding it to the state map invalidates the map. + DeepChain const chain{nextSeed()}; + + auto const header = makeHeader(UInt256{777}, chain.rootHash.asUInt256()); + storeHeader(env, header); + + auto acquire = std::make_shared( + env.app(), + header.hash, + header.seq, + InboundLedger::Reason::GENERIC, + stopwatch(), + std::make_unique()); + + BEAST_EXPECT(!acquire->checkLocal()); + BEAST_EXPECT(!acquire->isFailed()); + BEAST_EXPECT(!acquire->getJson(0)[jss::have_state].asBool()); + + // Invalidate the state map directly, standing in for a concurrent trigger() walk that + // reached the same verdict with mtx_ released. + auto const ledger = mutableLedger(*acquire); + BEAST_EXPECT(ledger != nullptr); + if (!ledger) + return; + + auto& stateMap = ledger->stateMap(); + BEAST_EXPECT(stateMap.addRootNode(chain.rootHash, chain.nodeAt(0), nullptr).isGood()); + for (auto const& [nodeID, node] : chain.nodesBelowRoot()) + stateMap.addKnownNode(nodeID, node, nullptr); + BEAST_EXPECT(!stateMap.isValid()); + + // An ordinary state-node packet, arriving as if it were already in flight when the + // invalidation above happened. addKnownNode()'s "while not synching" branch reports it a + // duplicate, whatever it contains, since the map is no longer Synching. + auto const latePeer = std::make_shared(); + BEAST_EXPECT(acquire->gotData( + latePeer, stateNodePacket(header, chain, {{chain.idAt(1), chain.nodeAt(1)}}))); + acquire->runData(); + + // The verdict belongs to the concurrent walk, so the sender keeps its fee and the + // acquisition fails, leaving the duplicate verdict unread as a finished map. + BEAST_EXPECT(latePeer->charges().empty()); + BEAST_EXPECT(acquire->isFailed()); + BEAST_EXPECT(!acquire->isComplete()); + + // This assertion pins the isValid() check, which is what fails the acquisition while the + // map behind it is broken. + BEAST_EXPECT(!acquire->getJson(0)[jss::have_state].asBool()); + } + /** * An acquisition that fails on local data must still signal. * @@ -629,10 +697,7 @@ struct InboundLedger_test : public beast::unit_test::Suite BEAST_EXPECT(!acquire->isFailed()); // The root, then the rest of the chain ending in the inner node at kLeafDepth. - std::vector> data; - data.emplace_back(SHAMapNodeID{}, chain.nodeAt(0)); - for (auto const& node : chain.nodesBelowRoot()) - data.push_back(node); + auto const data = chain.allNodes(); // The header counted as progress, so clear it to see what the packet below records. acquire->clearProgress(); @@ -1099,26 +1164,99 @@ struct InboundLedger_test : public beast::unit_test::Suite BEAST_EXPECT(!acquire->isComplete()); BEAST_EXPECT(peerSetPtr->requests() > requestsFromInit); - // done() remembered the hash. This case reaches getLedger() with no ledger ever built, so - // both arms of the failed_ gate answer null here. The local-chain case above is what - // covers the gate itself: tryDB() builds a partial ledger and then fails, so the gate is - // the only reason the answer is null. + // No ledger was ever built here, so both arms of the failed_ gate answer null. The + // local-chain case above covers the gate itself. BEAST_EXPECT(acquire->getLedger() == nullptr); BEAST_EXPECT( waitFor([&] { return env.app().getInboundLedgers().isFailure(kUnknownLedger); })); } + /** + * A packet that leaves nothing to fetch must settle the ledger itself, + * rather than count on a later walk to notice. + * + * A map holding one item has that item's leaf for its root, so rooting it + * stops it synching on the spot. The packet here offers that root to a map + * that already holds it, which counts no good nodes, so runData() takes the + * sender for one that returned nothing and asks it nothing further. That + * leaves receiveNode() as the only thing that can settle this ledger. + * + * @param env The environment to run in. + */ + void + testCompletesWhenNothingIsLeftToFetch(jtx::Env& env) + { + testcase("A packet that leaves nothing to fetch settles the ledger"); + + // A leaf at the root, so the state map holds exactly one item. No transactions, which + // leaves the state map the only part the acquisition needs. + auto const chain = DeepChain::toLeaf(0, nextSeed()); + auto const header = makeHeader(chain); + + // Only the header is local, so the state map starts with no root of its own. + storeHeader(env, header); + + auto peerSet = std::make_unique(); + auto* const peerSetPtr = peerSet.get(); + + auto acquire = std::make_shared( + env.app(), + header.hash, + header.seq, + InboundLedger::Reason::GENERIC, + stopwatch(), + std::move(peerSet)); + + BEAST_EXPECT(!acquire->checkLocal()); + BEAST_EXPECT(!acquire->isComplete()); + BEAST_EXPECT(!acquire->isFailed()); + BEAST_EXPECT(!acquire->getJson(0)[jss::have_state].asBool()); + + // Root the map directly, standing in for a trigger() walk that got that far. The flag is + // the acquisition's own, so it stays clear while the map is already whole. + auto const ledger = mutableLedger(*acquire); + BEAST_EXPECT(ledger != nullptr); + if (!ledger) + return; + + auto& stateMap = ledger->stateMap(); + BEAST_EXPECT(stateMap.addRootNode(chain.rootHash, chain.nodeAt(0), nullptr).isUseful()); + BEAST_EXPECT(!stateMap.isSynching()); + BEAST_EXPECT(!acquire->getJson(0)[jss::have_state].asBool()); + + // The same root again. A leaf carries its own key, so a depth of zero names the root and + // the node goes through addRootNode(), which reports a node already held. + auto const sender = std::make_shared(); + BEAST_EXPECT(acquire->gotData(sender, stateNodePacket(header, chain, chain.allNodes()))); + acquire->runData(); + + // Nothing was asked of the sender afterwards, so no walk ran to reach the same verdict. + BEAST_EXPECT(!acquire->madeProgress()); + BEAST_EXPECT(peerSetPtr->requests() == 0); + + // The sender offered nothing bad, so it keeps its fee, and the map is now reported held. + BEAST_EXPECT(sender->charges().empty()); + BEAST_EXPECT(acquire->getJson(0)[jss::have_state].asBool()); + + // Settled, reported complete and handed on, all from this one packet. + BEAST_EXPECT(acquire->isComplete()); + BEAST_EXPECT(!acquire->isFailed()); + BEAST_EXPECT(ledger->isImmutable()); + + BEAST_EXPECT(env.app().getLedgerMaster().getLedgerByHash(header.hash) != nullptr); + BEAST_EXPECT(!env.app().getInboundLedgers().isFailure(header.hash)); + } + void run() override { - // One Env for the suite, since building one costs far more than any case here. Safe - // because every chain is seeded through nextSeed(): the node store, the fetch packs and - // the remembered failures are all shared, and all three are keyed by hash. + // One Env for the suite, shared safely because every chain is seeded through nextSeed(). jtx::Env env{*this}; testLocalLedgerCompletesAcquire(env); testWalkSettlesBeforeReportingComplete(env); testInvalidatedLedgerFailsInDone(env); + testInvalidatedMapFailsReceiveNode(env); testLocalFailureSignalsDone(env); testPeerZeroAccountHashFails(env); testPeerHeaderWithoutTransactionsCompletes(env); @@ -1127,6 +1265,7 @@ struct InboundLedger_test : public beast::unit_test::Suite testLocalChainFailsAcquire(env); testAggressiveRetryJudgesLocalMap(env); testWalkJudgesMapOnOrdinaryTrigger(env); + testCompletesWhenNothingIsLeftToFetch(env); // Last: the only case that waits out a whole timeout chain. testTimerRetriesThenGivesUp(env); diff --git a/src/test/app/TransactionAcquire_test.cpp b/src/test/app/TransactionAcquire_test.cpp index 5e99f75984..fbde9c32ad 100644 --- a/src/test/app/TransactionAcquire_test.cpp +++ b/src/test/app/TransactionAcquire_test.cpp @@ -538,20 +538,18 @@ struct TransactionAcquire_test : public beast::unit_test::Suite BEAST_EXPECT(peerSetPtr->addedPeers() == std::set{candidate->id()}); // The whole chain in one batch, from a different peer, which settles the set. - auto data = chain.nodesBelowRoot(); - data.emplace(data.begin(), SHAMapNodeID{}, chain.nodeAt(0)); + auto data = chain.allNodes(); auto const supplier = std::make_shared(); BEAST_EXPECT(acquire->takeNodes(std::move(data), supplier).isUseful()); BEAST_EXPECT(supplier->charges().empty()); - // A peer nobody asked is charged immediately: the allowance belongs to candidate, not - // to whichever late reply happens to arrive first. + // A peer nobody asked is charged immediately: the allowance belongs to candidate. auto const stranger = std::make_shared(); BEAST_EXPECT(wasIgnored(acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, stranger))); BEAST_EXPECT(stranger->charges() == std::vector{resource::kFeeUselessData}); - // candidate's own late reply is the one that was genuinely in flight, and is free. + // candidate's own late reply was genuinely in flight, and is free. BEAST_EXPECT( wasIgnored(acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, candidate))); BEAST_EXPECT(candidate->charges().empty()); @@ -625,35 +623,33 @@ struct TransactionAcquire_test : public beast::unit_test::Suite auto const chain = DeepChain::toLeaf(1, nextSeed()); // Two peers asked, so each earns its own pass. - auto const spammer = std::make_shared(); - auto const honest = std::make_shared(); + auto const replayer = std::make_shared(); + auto const otherPeer = std::make_shared(); auto peerSet = std::make_unique( - std::vector>{spammer, honest}); + std::vector>{replayer, otherPeer}); auto const acquire = std::make_shared( env.app(), chain.rootHash.asUInt256(), std::move(peerSet), kFastRetry); acquire->init(2); - // A third peer supplies the whole chain, so both spammer's and honest's replies below + // A third peer supplies the whole chain, so both replayer's and otherPeer's replies below // are late. - auto data = chain.nodesBelowRoot(); - data.emplace(data.begin(), SHAMapNodeID{}, chain.nodeAt(0)); + auto data = chain.allNodes(); auto const supplier = std::make_shared(); BEAST_EXPECT(acquire->takeNodes(std::move(data), supplier).isUseful()); - // spammer's first late reply is free - its own pass - but every one after that is its - // own replay, not anyone else's slot to spend. - BEAST_EXPECT(wasIgnored(acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, spammer))); - BEAST_EXPECT(spammer->charges().empty()); + // replayer's first late reply is its own pass, and every one after that is its own replay. + BEAST_EXPECT(wasIgnored(acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, replayer))); + BEAST_EXPECT(replayer->charges().empty()); for (int i = 0; i < 5; ++i) - acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, spammer); - BEAST_EXPECT(spammer->charges().size() == 5); + static_cast(acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, replayer)); + BEAST_EXPECT(replayer->charges().size() == 5); - // honest's own, single late reply is still free, because the allowance is keyed by peer - // identity: spammer's five replays above spend only spammer's own slot. - BEAST_EXPECT(wasIgnored(acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, honest))); - BEAST_EXPECT(honest->charges().empty()); + // otherPeer's own late reply is still free: the replays above spent only replayer's slot. + BEAST_EXPECT( + wasIgnored(acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, otherPeer))); + BEAST_EXPECT(otherPeer->charges().empty()); acquire->cancel(); } diff --git a/src/tests/libxrpl/shamap/DeepChain.h b/src/tests/libxrpl/shamap/DeepChain.h index fcc4457437..a906941a4a 100644 --- a/src/tests/libxrpl/shamap/DeepChain.h +++ b/src/tests/libxrpl/shamap/DeepChain.h @@ -155,16 +155,26 @@ struct DeepChain return data; } + /** + * Every node in the chain, root first. + * + * @return The nodes, each with its claimed position. + */ + [[nodiscard]] std::vector> + allNodes() const + { + return nodesBelowRoot(0); + } + /** * Fill a synching map, stopping one level short of the deepest node so the * caller offers that one itself. * - * Reports rather than asserts: the two binaries sharing this header use - * different test frameworks. + * Returns rather than asserts, since two test frameworks share this header. * * @param map The map to fill. * @return Whether the root and every node above the deepest one was - * accepted, which is a property of the chain. + * accepted. */ [[nodiscard]] bool fill(SHAMap& map) const diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp index c9d03b20cd..554cc31c39 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp @@ -1045,6 +1045,19 @@ InboundLedger::receiveNode( return; } + // The verdict belongs to whichever walk reached it rather than to this packet, so the + // acquisition fails here and the sender keeps its fee. The validity check runs ahead of + // isSynching() below, which reads an invalid map the same way as a finished one. + if (!map.isValid()) + { + failed_ = true; + done(); + + // The whole packet is discarded, since its nodes belong to the abandoned map. + san = SHAMapAddNode::invalid(); + return; + } + if (!map.isSynching()) { if (packet.type() == protocol::liTX_NODE)