diff --git a/.cspell.config.yaml b/.cspell.config.yaml index 7b4a280c65..da0ec848f0 100644 --- a/.cspell.config.yaml +++ b/.cspell.config.yaml @@ -102,6 +102,7 @@ words: - dearmor - decryptor - dedented + - dedup - deleteme - demultiplexer - deserializaton @@ -341,6 +342,7 @@ words: - unambiguity - unauthorizes - unauthorizing + - undeserializable - unergonomic - unfetched - unfindable diff --git a/.github/scripts/levelization/results/ordering.txt b/.github/scripts/levelization/results/ordering.txt index 5577c363fd..ab84993d04 100644 --- a/.github/scripts/levelization/results/ordering.txt +++ b/.github/scripts/levelization/results/ordering.txt @@ -62,6 +62,7 @@ libxrpl.tx > xrpl.protocol libxrpl.tx > xrpl.server libxrpl.tx > xrpl.tx test.app > test.jtx +test.app > tests.libxrpl test.app > test.unit_test test.app > xrpl.basics test.app > xrpl.config diff --git a/src/test/app/AcquireTestHelpers.h b/src/test/app/AcquireTestHelpers.h new file mode 100644 index 0000000000..20dcf31171 --- /dev/null +++ b/src/test/app/AcquireTestHelpers.h @@ -0,0 +1,464 @@ +#pragma once + +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace xrpl::test { + +// The chain builder needs only libxrpl, so it is shared with the gtest suites; see the header for +// why the protobuf reply builder below cannot be. +using tests::DeepChain; + +// A smallest-possible leaf plus its 4-byte HashPrefix lands one byte short of the floor +// ConsensusTransSetSF::gotNode() parses at, so a chain's leaf is never taken for a transaction and +// resubmitted. +static_assert( + sizeof(std::uint32_t) + DeepChain::kLeafItemBytes < ConsensusTransSetSF::kMinTxNodeBytesToParse, + "a smallest-possible leaf must stay below the resubmission floor"); + +/** + * A peer that records what it was charged, and is otherwise inert. + * + * One instance per packet keeps charges() unambiguous about which packet was + * charged what. + * + * charges_ is unguarded: nothing on the retry timer's path charges a peer, so + * every charge lands on the thread that fed the packet in. + */ +class ChargeRecordingPeer : public Peer +{ +public: + /** + * @param hasTxSet What hasTxSet() reports, which is how an acquisition + * decides whether this peer is worth asking. Defaults to true, so a + * peer handed straight to takeNodes() needs no argument. + */ + explicit ChargeRecordingPeer(bool hasTxSet = true) : id_(nextId()), hasTxSet_(hasTxSet) + { + } + + void + charge(resource::Charge const& fee, std::string const& context = {}) override + { + charges_.push_back(fee); + } + + [[nodiscard]] std::vector const& + charges() const + { + return charges_; + } + + [[nodiscard]] id_t + id() const override + { + return id_; + } + + [[nodiscard]] bool + hasTxSet(uint256 const&) const override + { + return hasTxSet_; + } + + // Nothing below is consulted by the paths under test. + + void + send(std::shared_ptr const&) override + { + } + [[nodiscard]] beast::ip::Endpoint + getRemoteAddress() const override + { + return {}; + } + [[nodiscard]] bool + cluster() const override + { + return false; + } + [[nodiscard]] bool + isHighLatency() const override + { + return false; + } + [[nodiscard]] int + getScore(bool) const override + { + return 0; + } + [[nodiscard]] PublicKey const& + getNodePublic() const override + { + // Shared across instances: nothing tells these peers apart by key, and deriving one + // per instance runs a real Ed25519 keygen for every peer a case builds. + static PublicKey const kNodePublicKey = + derivePublicKey(KeyType::Ed25519, randomSecretKey()); + return kNodePublicKey; + } + json::Value + json() override + { + return {}; + } + [[nodiscard]] bool + supportsFeature(ProtocolFeature) const override + { + return false; + } + [[nodiscard]] std::optional + publisherListSequence(PublicKey const&) const override + { + return {}; + } + void + setPublisherListSequence(PublicKey const&, std::size_t const) override + { + } + [[nodiscard]] uint256 + getClosedLedgerHash() const override + { + static uint256 const kHash{}; + return kHash; + } + [[nodiscard]] bool + hasLedger(uint256 const&, std::uint32_t) const override + { + return true; + } + void + ledgerRange(std::uint32_t&, std::uint32_t&) const override + { + } + void + cycleStatus() override + { + } + bool + hasRange(std::uint32_t, std::uint32_t) override + { + return false; + } + [[nodiscard]] bool + compressionEnabled() const override + { + return false; + } + void + sendTxQueue() override + { + } + void + addTxQueue(uint256 const&) override + { + } + void + removeTxQueue(uint256 const&) override + { + } + [[nodiscard]] bool + txReduceRelayEnabled() const override + { + return false; + } + [[nodiscard]] std::string const& + fingerprint() const override + { + static std::string const kFingerprint; + return kFingerprint; + } + +private: + /** + * The next id to hand out, distinct per instance so a test with + * several peers can tell from a recorded id which one an acquisition + * picked. + * + * @return The id. + */ + [[nodiscard]] static id_t + nextId() + { + static std::atomic next{1}; + return next++; + } + + std::vector charges_; + id_t id_; + bool hasTxSet_; +}; + +/** + * A peer set that counts the requests sent through it, which is what shows + * whether an acquisition is still asking for nodes, and that offers peers to a + * hasItem/onPeerAdded callback pair like the real one: hard-filtered by hasItem + * (which only scores in the real peer set) and deduped by tracked id, same as + * the real peer set. + * + * The retry timer drives addPeers() and sendRequest() from a job thread while + * the test reads the results, so everything recorded here is guarded. + */ +class RequestCountingPeerSet : public PeerSet +{ +public: + /** + * @param candidates The peers addPeers() may offer, in the order they are + * considered. Fixed at construction, so nothing can change them + * while an acquisition is running. Empty for a case that never lets + * addPeers() find anyone. + */ + explicit RequestCountingPeerSet(std::vector> candidates = {}) + : candidates_(std::move(candidates)) + { + } + + /** + * Offer the candidates to the caller, the way the real peer set offers the + * peers the overlay is tracking. + * + * @param limit The most peers to add, recorded so a test can check what was + * asked for. + * @param hasItem Hard-filters the candidates worth asking. Selects rather than merely + * scores, unlike the real peer set's use of the same parameter. + * @param onPeerAdded Called for each selected candidate. + */ + void + addPeers( + std::size_t limit, + std::function const&)> hasItem, + std::function const&)> onPeerAdded) override + { + std::vector> selected; + { + std::scoped_lock const lock(mutex_); + + if (!firstLimit_) + firstLimit_ = limit; + + for (auto const& candidate : candidates_) + { + if (selected.size() >= limit) + break; + // Dedup by tracked id, like the real peer set: a candidate already selected by an + // earlier call does not get offered - or its onPeerAdded rerun - again. + if (hasItem(candidate) && addedPeers_.insert(candidate->id()).second) + selected.push_back(candidate); + } + } + + // Outside the lock: onPeerAdded() calls back into the acquisition, which sends a + // request straight back through this object. + for (auto const& peer : selected) + onPeerAdded(peer); + } + + void + sendRequest( + ::google::protobuf::Message const&, + protocol::MessageType, + std::shared_ptr const& peer) override + { + std::scoped_lock const lock(mutex_); + ++requests_; + } + + /** + * The ids of every peer addPeers() has selected, which is what an + * acquisition takes for the peers it is tracking. + * + * Unguarded, like the real peer set's: every caller of this and of + * addPeers() is an acquisition holding its own mtx_, so nothing can be + * added while a caller iterates. The by-value accessor below is what the + * test thread reads instead. + * + * A caveat for a case that wants a peer count rather than a set of ids: + * InboundLedger::getPeerCount() resolves each id through + * Overlay::findPeerByShortID(), which only knows peers that really + * connected, so it still reports zero however many ids are returned here. + * + * @return The ids. + */ + [[nodiscard]] std::set const& + getPeerIds() const override + { + return addedPeers_; + } + + [[nodiscard]] int + requests() const + { + std::scoped_lock const lock(mutex_); + return requests_; + } + + /** + * The limit the first addPeers() call asked for. + * + * The first rather than the last, because onTimer() keeps calling + * addPeers(1) for as long as an acquisition runs, which would overwrite + * what init() asked for. + */ + [[nodiscard]] std::optional + firstLimit() const + { + std::scoped_lock const lock(mutex_); + return firstLimit_; + } + + /** + * The ids of every peer addPeers() selected, which is a set because + * onTimer() keeps re-offering the same candidates. + * + * @return The ids of every peer addPeers() has selected so far. + */ + [[nodiscard]] std::set + addedPeers() const + { + std::scoped_lock const lock(mutex_); + return addedPeers_; + } + +private: + std::vector> const candidates_; + + mutable std::mutex mutex_; + int requests_{0}; + std::optional firstLimit_; + std::set addedPeers_; +}; + +/** + * The given nodes of a chain as a TMLedgerData, so a test can go through the + * real dispatch rather than calling an acquisition directly. + * + * Not part of DeepChain itself: that header is shared with the gtest suites, + * whose binary has neither the protobuf types nor anything to send them to. + * + * @param chain The chain the nodes came from, which names the reply by default. + * @param data The nodes to include, each with its claimed position. + * @param type The reply type, which selects which map the receiver applies it + * to. + * @param ledgerHash The hash the reply claims to be about, defaulting to the + * chain root for a TX set. A ledger acquisition wants its header hash + * here instead, since the chain root is only that ledger's account hash. + * @param ledgerSeq The sequence to name in the reply. + * @return The reply packet. + */ +[[nodiscard]] inline std::shared_ptr +packetFor( + DeepChain const& chain, + std::vector> const& data, + protocol::TMLedgerInfoType type = protocol::liTS_CANDIDATE, + std::optional const& ledgerHash = std::nullopt, + std::uint32_t ledgerSeq = 0) +{ + auto packet = std::make_shared(); + auto const hash = ledgerHash.value_or(chain.rootHash.asUInt256()); + packet->set_ledgerhash(hash.data(), uint256::size()); + packet->set_ledgerseq(ledgerSeq); + packet->set_type(type); + + for (auto const& [nodeID, node] : data) + { + Serializer s; + node->serializeForWire(s); + + auto* const ledgerNode = packet->add_nodes(); + ledgerNode->set_nodedata(s.peekData().data(), s.peekData().size()); + + // A leaf carries its own key, so the receiver rebuilds its position from + // that plus a depth; an inner node has no key and needs the full ID. The two + // fields are a oneof, so sending the wrong one is rejected outright. + if (node->isLeaf()) + { + ledgerNode->set_depth(nodeID.getDepth()); + } + else + { + ledgerNode->set_id(nodeID.getRawString()); + } + } + + return packet; +} + +/** + * Poll until the condition holds, or give up. + * + * An acquisition's own timer and the jobs it hands finished work to both + * run on other threads, so a case cannot simply look once. The deadline + * is generous, so a loaded machine does not turn a pass into a failure. + * + * @param condition What to wait for. + * @param deadline The longest to wait. + * @return Whether the condition held before the deadline. + */ +[[nodiscard]] inline bool +waitFor( + std::function const& condition, + std::chrono::steady_clock::duration deadline = std::chrono::seconds{10}) +{ + auto const giveUp = std::chrono::steady_clock::now() + deadline; + while (std::chrono::steady_clock::now() < giveUp) + { + if (condition()) + return true; + std::this_thread::sleep_for(std::chrono::milliseconds{10}); + } + return condition(); +} + +/** + * Whether a batch verdict carries exactly the given counts. + * + * The counts rather than get(): that string is a log format, not an API. It is + * pinned once, in the SHAMapAddNode tests, and is what to pass BEAST_EXPECTS() + * as the reason a check here failed. Same name and meaning as the gtest suites' + * tallyIs(), which returns an AssertionResult instead. + * + * @param san The verdict to check. + * @param good How many nodes the batch should have hooked in. + * @param bad How many it should have rejected. + * @param duplicate How many it should have already held. + * @return Whether the verdict matches. + */ +[[nodiscard]] inline bool +tallyIs(SHAMapAddNode const& san, int good, int bad, int duplicate) +{ + return san.getGood() == good && san.getBad() == bad && san.getDuplicate() == duplicate; +} + +} // namespace xrpl::test diff --git a/src/test/app/InboundLedger_test.cpp b/src/test/app/InboundLedger_test.cpp new file mode 100644 index 0000000000..92673c2518 --- /dev/null +++ b/src/test/app/InboundLedger_test.cpp @@ -0,0 +1,312 @@ +#include +#include + +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include + +namespace xrpl::test { + +/** + * An acquisition that exposes the entry points its bases keep protected, so a + * case can reach them without the daemon's API growing. + */ +struct TestableInboundLedger final : InboundLedger +{ + using InboundLedger::InboundLedger; + + /** + * Look for the ledger locally, ask the peers being tracked for the + * rest, and arm the timer, as InboundLedgers::acquire() does. + * + * That caller holds its collection lock across init(), which releases + * it, so this stands in with a lock of its own. Declared before the + * lock, so it outlives it. + */ + void + startAcquire() + { + std::recursive_mutex collectionMutex; + ScopedLockType collectionLock(collectionMutex); + init(collectionLock); + } +}; + +struct InboundLedger_test : public beast::unit_test::Suite +{ + /** + * A retry interval short enough that a whole timeout chain costs a fraction + * of a second. TimeoutCounter refuses anything at or below 10ms. + */ + static constexpr auto kFastRetry = std::chrono::milliseconds{20}; + + /** + * A seed no other chain in this suite has used. + * + * The Env below is shared, and its node store, fetch packs and remembered + * failures are all keyed by hash, so two cases building identically seeded + * chains would let one resolve or judge the other's. Handing out a fresh + * seed per chain makes that impossible rather than merely unlikely. + * + * @return The seed. + */ + [[nodiscard]] unsigned int + nextSeed() + { + return ++seed_; + } + + /** + * A ledger header naming the given map roots. + * + * The hash is derived from the fields, so an acquisition accepts the + * header as its own however the roots are chosen. + * + * @param txHash The transaction map root; zero means no transactions. + * @param accountHash The state map root; zero is a ledger no + * acquisition can finish. + * @return The header, with its hash filled in. + */ + static LedgerHeader + makeHeader(uint256 const& txHash, uint256 const& accountHash) + { + LedgerHeader header; + header.seq = 2; + header.parentCloseTime = NetClock::time_point{}; + header.closeTime = NetClock::time_point{}; + header.closeTimeResolution = NetClock::duration{10}; + header.closeFlags = 0; + header.txHash = txHash; + header.accountHash = accountHash; + header.hash = calculateLedgerHash(header); + return header; + } + + /** + * The common shape: no transactions, so only the state map is in play. + * + * @param chain The chain whose root to name as the state hash. + * @return The header, with its hash filled in. + */ + static LedgerHeader + makeHeader(DeepChain const& chain) + { + return makeHeader(uint256{}, chain.rootHash.asUInt256()); + } + + /** + * Put the header in the local store, which is the first place tryDB() + * looks. + * + * Unlike a fetch pack, which hands each entry out once, the store + * keeps it, so more than one acquisition of the same ledger can find + * it. + * + * @param env The environment whose node store to seed. + * @param header The header to store, keyed by its own hash. + */ + static void + storeHeader(jtx::Env& env, LedgerHeader const& header) + { + Serializer s; + s.add32(HashPrefix::LedgerMaster); + addRaw(header, s); + + env.app().getNodeFamily().db().store( + NodeObjectType::Ledger, std::move(s.modData()), header.hash, header.seq); + } + + /** + * Put every node of a chain in the local store, so a state-map walk + * resolves the whole map without a peer. + * + * @param env The environment whose node store to seed. + * @param header The header whose sequence the nodes are stored under. + * @param chain The chain supplying the nodes. + * @param maxDepth The deepest node to store, so a caller can leave a walk + * something to ask for. + */ + static void + storeStateNodes( + jtx::Env& env, + LedgerHeader const& header, + DeepChain const& chain, + unsigned int maxDepth) + { + auto& db = env.app().getNodeFamily().db(); + + for (auto depth = 0u; depth <= maxDepth; ++depth) + { + db.store( + NodeObjectType::AccountNode, + chain.prefixedNodeAt(depth), + chain.nodeAt(depth)->getHash().asUInt256(), + header.seq); + } + } + + /** + * A ledger whose maps all resolve locally finishes on the spot, and the + * finished ledger is immutable and handed on. + * + * The case where tryDB() alone completes the acquisition, so it covers + * tryDB() reporting a ledger it found and done() taking its success arm + * on that path. Both entry points are driven: InboundLedgers::acquire() + * is the only caller of init(), and hands back the finished ledger + * itself, while checkLocal() is the route that reaches done(). + * + * @param env The environment to run in. + */ + void + testLocalLedgerCompletesAcquire(jtx::Env& env) + { + testcase("A ledger found locally completes the acquire"); + + // A chain ending in a real leaf, so the state map is genuinely complete rather than merely + // rooted. No transactions, so only the state map is in play. + auto const chain = DeepChain::toLeaf(2, nextSeed()); + auto const header = makeHeader(chain); + + storeHeader(env, header); + storeStateNodes(env, header, chain, chain.deepestDepth); + + // acquire() runs init() under its own collection lock and returns the ledger only once the + // acquisition is complete and unfailed, so a non-null result is what shows tryDB() found it + // without a peer ever being asked. + auto const acquired = env.app().getInboundLedgers().acquire( + header.hash, header.seq, InboundLedger::Reason::GENERIC); + + BEAST_EXPECT(acquired != nullptr); + if (acquired) + { + BEAST_EXPECT(acquired->isImmutable()); + BEAST_EXPECT(acquired->header().hash == header.hash); + } + + // init() hands a ledger it completed to LedgerMaster itself, which is what makes it + // available to everything else. + BEAST_EXPECT(env.app().getLedgerMaster().getLedgerByHash(header.hash) != nullptr); + + // Nothing was logged as a failure, which is the other arm of done(). + BEAST_EXPECT(!env.app().getInboundLedgers().isFailure(header.hash)); + + // The same ledger through checkLocal(), which unlike init() reaches done(). Everything it + // needs is still in the store, since the first acquisition read rather than consumed it. + auto again = std::make_shared( + env.app(), + header.hash, + header.seq, + InboundLedger::Reason::GENERIC, + stopwatch(), + std::make_unique()); + + // True because the acquisition ended, which here means it succeeded, and it reports that + // only after done() has run. + BEAST_EXPECT(again->checkLocal()); + BEAST_EXPECT(again->isComplete()); + BEAST_EXPECT(!again->isFailed()); + + auto const settled = again->getLedger(); + BEAST_EXPECT(settled != nullptr); + if (settled) + BEAST_EXPECT(settled->isImmutable()); + + BEAST_EXPECT(!env.app().getInboundLedgers().isFailure(header.hash)); + } + + /** + * The retry timer re-asks, then gives up and signals. + * + * The only case that drives onTimer() rather than trigger() directly, + * which is what covers the give-up: past kLedgerTimeoutRetriesMax the + * acquisition fails itself and done() records that, so the same + * doomed ledger is not asked for again on the next round. It is also + * what the retry interval is a constructor parameter for, since the + * chain runs past kLedgerTimeoutRetriesMax ticks of three seconds + * apiece in production. + * + * Nothing is local and no data ever arrives, so no tick can record progress + * and the count only climbs. A hash of its own, so no other case can have + * remembered it as a failure already. + * + * @param env The environment to run in. + */ + void + testTimerRetriesThenGivesUp(jtx::Env& env) + { + testcase("The retry timer re-asks, then gives up"); + + uint256 const kUnknownLedger{8}; + + // One candidate, which onTimer() re-offers on every tick. + auto const candidate = std::make_shared(); + auto peerSet = + std::make_unique(std::vector>{candidate}); + auto* const peerSetPtr = peerSet.get(); + + auto acquire = std::make_shared( + env.app(), + kUnknownLedger, + 0, + InboundLedger::Reason::GENERIC, + stopwatch(), + std::move(peerSet), + kFastRetry); + + BEAST_EXPECT(!env.app().getInboundLedgers().isFailure(kUnknownLedger)); + + // init() finds nothing locally, so it asks the candidate and queues the first check-in, + // which is what arms the retry timer for every cycle after. Those first requests are not + // the ones under test, so count from here. + acquire->startAcquire(); + BEAST_EXPECT(!acquire->isFailed()); + int const requestsFromInit = peerSetPtr->requests(); + BEAST_EXPECT(requestsFromInit > 0); + BEAST_EXPECT(peerSetPtr->addedPeers() == std::set{candidate->id()}); + + // Every tick asks again, and past kLedgerTimeoutRetriesMax (6) the chain gives up. + BEAST_EXPECT(waitFor([&] { return acquire->isFailed(); })); + BEAST_EXPECT(!acquire->isComplete()); + BEAST_EXPECT(peerSetPtr->requests() > requestsFromInit); + + // done() remembered the hash, which is what stops the next round asking again. + BEAST_EXPECT( + waitFor([&] { return env.app().getInboundLedgers().isFailure(kUnknownLedger); })); + } + + 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. + jtx::Env env{*this}; + + testLocalLedgerCompletesAcquire(env); + + // Last: the only case that waits out a whole timeout chain. + testTimerRetriesThenGivesUp(env); + } + +private: + unsigned int seed_{0}; +}; + +BEAST_DEFINE_TESTSUITE(InboundLedger, app, xrpl); + +} // namespace xrpl::test diff --git a/src/test/app/TransactionAcquire_test.cpp b/src/test/app/TransactionAcquire_test.cpp new file mode 100644 index 0000000000..0c3a7b1ffa --- /dev/null +++ b/src/test/app/TransactionAcquire_test.cpp @@ -0,0 +1,512 @@ +#include +#include + +#include +#include +#include + +#include +#include +#include +#include +#include +#include + +#include + +#include +#include +#include +#include +#include + +namespace xrpl::test { + +/** + * An acquisition that exposes the state its bases keep protected, so a case can + * reach it without the daemon's API growing. + */ +struct TestableTransactionAcquire final : TransactionAcquire +{ + using TransactionAcquire::TransactionAcquire; + + /** + * Whether the set being acquired is still structurally coherent. + * + * @return Whether the map is still valid. + */ + [[nodiscard]] bool + isMapValid() const + { + // Under the lock: a batch on another thread can reach the verdict. + ScopedLockType const sl(mtx_); + return map_->isValid(); + } + + /** + * Whether a batch has advanced the set since the flag was last cleared. + * + * @return Whether progress has been recorded. + */ + [[nodiscard]] bool + madeProgress() const + { + // Under the lock: a timer tick clears the flag on a job thread. + ScopedLockType const sl(mtx_); + return progress_; + } + + /** + * Forget any recorded progress. + */ + void + clearProgress() + { + ScopedLockType const sl(mtx_); + progress_ = false; + } +}; + +struct TransactionAcquire_test : public beast::unit_test::Suite +{ + /** + * A retry interval short enough that a whole timeout chain costs a fraction + * of a second. + * + * TimeoutCounter refuses anything at or below 10ms. At this interval the + * window between the first retry (four timeouts in) and giving up (twenty) + * is still a third of a second, which is what the one case that watches + * both needs. + */ + static constexpr auto kFastRetry = std::chrono::milliseconds{20}; + + /** + * A seed no other chain in this suite has used. + * + * The Env below is shared, and ConsensusTransSetSF::gotNode() puts + * every node it accepts into the application-wide NodeCache while + * InboundTransactions keys its acquisitions by set hash, so two cases + * building identically seeded chains would let one resolve or revive + * the other's. Handing out a fresh seed per chain makes that + * impossible rather than merely unlikely. + * + * @return The seed. + */ + [[nodiscard]] unsigned int + nextSeed() + { + return ++seed_; + } + + /** + * Whether takeNodes() declined to look at the data at all. + * + * TimeoutCounter::complete_ and failed_ are both protected, so this stands in + * for either: a done acquisition returns a verdict accounting for nothing. + * + * @param san The verdict a takeNodes() call returned. + * @return Whether that verdict shows the data was never looked at. + */ + static bool + wasIgnored(SHAMapAddNode const& san) + { + return tallyIs(san, 0, 0, 0); + } + + /** + * Wait for a finished set to reach InboundTransactions. + * + * done() hands the map over through a job, so this is what shows an + * acquisition completed rather than merely stopped. + * + * @param env The environment whose InboundTransactions to watch. + * @param setHash The set to wait for. + * @return The delivered map, or nullptr if none arrived. + */ + [[nodiscard]] static std::shared_ptr + waitForDeliveredSet(jtx::Env& env, uint256 const& setHash) + { + auto& inbound = env.app().getInboundTransactions(); + + // acquire=false: asking to acquire would spin up a second, unrelated acquisition for + // this hash (or, if one is already registered, needlessly poke stillNeed() on it). + // + // A shorter deadline than the polling default: the job is queued before takeNodes() + // returns, so anything beyond a few seconds means it is never coming. + std::shared_ptr delivered; + if (!waitFor( + [&] { return (delivered = inbound.getSet(setHash, false)) != nullptr; }, + std::chrono::seconds{5})) + return nullptr; + return delivered; + } + + /** + * A chain ending in a real leaf completes the acquisition, and later + * replies for it are then left alone. + * + * Also pins the two things that follow from finishing: nothing more + * is asked for, and done() hands the map to InboundTransactions, + * which is what consensus is waiting on. + * + * @param env The environment to run in. + */ + void + testHappyPathCompletesAcquisition(jtx::Env& env) + { + testcase("A chain ending in a leaf completes the acquire"); + + auto const chain = DeepChain::toLeaf(3, nextSeed()); + + auto peerSet = std::make_unique(); + auto* const peerSetPtr = peerSet.get(); + + uint256 const setHash = chain.rootHash.asUInt256(); + auto const acquire = + std::make_shared(env.app(), setHash, std::move(peerSet)); + auto const peer = std::make_shared(); + + // The root alone leaves the set incomplete, so accepting it asks for the level + // below. + auto const rootResult = acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, peer); + BEAST_EXPECT(rootResult.isUseful()); + int const requestsWhileIncomplete = peerSetPtr->requests(); + BEAST_EXPECT(requestsWhileIncomplete > 0); + + // The rest of the chain, ending in the leaf. + auto const result = acquire->takeNodes(chain.nodesBelowRoot(), peer); + BEAST_EXPECT(result.isUseful()); + BEAST_EXPECT(!result.isInvalid()); + + // Nothing more went out, which rules out the acquisition still asking for nodes. + // trigger() also sends nothing when it gives up, so the delivered set below is what + // shows it finished. + BEAST_EXPECT(peerSetPtr->requests() == requestsWhileIncomplete); + + // done() hands the map over only when it has not failed, so this is what separates + // completion from failure. + auto const delivered = waitForDeliveredSet(env, setHash); + BEAST_EXPECT(delivered != nullptr); + if (delivered) + { + BEAST_EXPECT(delivered->getHash() == chain.rootHash); + BEAST_EXPECT(delivered->isValid()); + } + + // A reply arriving after the set is finished is not examined, and does not restart the + // asking - the ordinary fate of every responder to trigger()'s broadcast but the one that + // completed the set. init() was never called, so no timer can have failed the acquisition + // since the set above was delivered. + BEAST_EXPECT(wasIgnored(acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, peer))); + BEAST_EXPECT(peerSetPtr->requests() == requestsWhileIncomplete); + } + + /** + * Two peers each answering with a different missing piece are both accepted + * without penalty, and the set completes from their combined replies. + * + * Driven through InboundTransactions::gotData() so the leaf goes over the + * real dispatch, which rebuilds a leaf's position from its own key rather + * than trusting the sender's label. + * + * @param env The environment to run in. + */ + void + testTwoPeersEachSupplyPartOfTheSet(jtx::Env& env) + { + testcase("Two peers each supplying part of a set are both accepted without penalty"); + + auto const chain = DeepChain::toLeaf(3, nextSeed()); + auto& inbound = env.app().getInboundTransactions(); + + // getSet() with acquire=true registers the TransactionAcquire that gotData() then + // looks up by hash. + uint256 const setHash = chain.rootHash.asUInt256(); + BEAST_EXPECT(inbound.getSet(setHash, true) == nullptr); + + // The first peer answers with the root only. + auto const peerA = std::make_shared(); + inbound.gotData(setHash, peerA, packetFor(chain, {{SHAMapNodeID{}, chain.nodeAt(0)}})); + BEAST_EXPECT(peerA->charges().empty()); + + // The second answers with everything the first left out, and finishes the set. + auto const peerB = std::make_shared(); + inbound.gotData(setHash, peerB, packetFor(chain, chain.nodesBelowRoot())); + BEAST_EXPECT(peerB->charges().empty()); + + // Completion does not depend on which peer sent which piece: the set finishes just + // as it does when one peer supplies the lot. + auto const delivered = waitForDeliveredSet(env, setHash); + BEAST_EXPECT(delivered != nullptr); + if (delivered) + BEAST_EXPECT(delivered->getHash() == chain.rootHash); + } + + /** + * A root that does not hash to the set we asked for is a plain mismatch, + * and has to leave the acquisition able to try another peer. + * + * @param env The environment to run in. + */ + void + testBadRootKeepsAcquireAlive(jtx::Env& env) + { + testcase("A mismatched root leaves the acquire recoverable"); + + DeepChain const chain{nextSeed()}; + + // Acquire an unrelated hash, so the chain's root cannot match it. + auto const acquire = std::make_shared( + env.app(), uint256{42}, std::make_unique()); + auto const peer = std::make_shared(); + + auto const result = acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, peer); + BEAST_EXPECTS(tallyIs(result, 0, 1, 0), result.get()); + + // A mismatched root says nothing about the tree behind the hash we asked for, so the map + // is untouched. + BEAST_EXPECT(acquire->isMapValid()); + + // Still alive: the next packet is examined rather than waved through. + BEAST_EXPECT(!wasIgnored(acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, peer))); + } + + /** + * A second reply carrying a root we already have must stay free. + * + * This is what an honest second responder to the initial fan-out sends: + * trigger() broadcasts to every tracked peer, so several answer the same + * request and all but the first carry nothing new. Charging for that would + * penalize peers for answering. + * + * Covers the root specifically, which takeNodes() short-circuits on + * haveRoot_ without consulting the map. A repeated non-root node takes the + * other route, through addKnownNode() - see + * testDuplicateNonRootReplyIsFree(). + * + * @param env The environment to run in. + */ + void + testDuplicateRootReplyIsFree(jtx::Env& env) + { + testcase("A reply of a root we already have is free"); + + DeepChain const chain{nextSeed()}; + auto& inbound = env.app().getInboundTransactions(); + + // getSet() with acquire=true registers the TransactionAcquire that gotData() then looks up + // by hash. + uint256 const setHash = chain.rootHash.asUInt256(); + BEAST_EXPECT(inbound.getSet(setHash, true) == nullptr); + + auto const rootPacket = packetFor(chain, {{SHAMapNodeID{}, chain.nodeAt(0)}}); + + // The first responder supplies the root, which is genuinely useful. + auto const firstPeer = std::make_shared(); + inbound.gotData(setHash, firstPeer, rootPacket); + BEAST_EXPECT(firstPeer->charges().empty()); + + // The second sends the same root. Nothing is added to the map, but the peer did what we + // asked, so it must not be charged. + auto const secondPeer = std::make_shared(); + inbound.gotData(setHash, secondPeer, rootPacket); + BEAST_EXPECT(secondPeer->charges().empty()); + } + + /** + * A repeated non-root node must stay free too. + * + * The counterpart to testDuplicateRootReplyIsFree(), covering the + * route that does consult the map: addKnownNode() reports a node it + * already holds as a duplicate, and takeNodes() tests isGood(), which + * counts a duplicate as success. Testing isUseful() there instead + * would turn every honest second responder into a peer we charge for + * invalid data. + * + * @param env The environment to run in. + */ + void + testDuplicateNonRootReplyIsFree(jtx::Env& env) + { + testcase("A repeated non-root node is free"); + + auto const chain = DeepChain::toLeaf(3, nextSeed()); + auto& inbound = env.app().getInboundTransactions(); + + uint256 const setHash = chain.rootHash.asUInt256(); + BEAST_EXPECT(inbound.getSet(setHash, true) == nullptr); + + auto const rootPeer = std::make_shared(); + inbound.gotData(setHash, rootPeer, packetFor(chain, {{SHAMapNodeID{}, chain.nodeAt(0)}})); + BEAST_EXPECT(rootPeer->charges().empty()); + + // Depth 1 alone, so the set stays incomplete and the acquisition keeps examining data + // rather than waving the second copy through as a late reply. + auto const level1 = packetFor(chain, {{chain.idAt(1), chain.nodeAt(1)}}); + + auto const firstPeer = std::make_shared(); + inbound.gotData(setHash, firstPeer, level1); + BEAST_EXPECT(firstPeer->charges().empty()); + + auto const secondPeer = std::make_shared(); + inbound.gotData(setHash, secondPeer, level1); + BEAST_EXPECT(secondPeer->charges().empty()); + } + + /** + * A reply whose node data cannot be deserialized is charged for. + * + * gotData() rejects the packet before the acquisition is handed + * anything, so this pins the charge on the dispatch layer rather than + * on takeNodes(). It also gives this suite's "was not charged" + * assertions their teeth: a harness that recorded no charge at all + * would satisfy all of them and fail only here. + * + * @param env The environment to run in. + */ + void + testUndeserializableNodeIsCharged(jtx::Env& env) + { + testcase("A reply with undeserializable node data is charged"); + + DeepChain const chain{nextSeed()}; + auto& inbound = env.app().getInboundTransactions(); + + uint256 const setHash = chain.rootHash.asUInt256(); + BEAST_EXPECT(inbound.getSet(setHash, true) == nullptr); + + // A single byte naming a wire type that does not exist, so getTreeNode() rejects it + // before the acquisition is handed anything. + auto packet = std::make_shared(); + packet->set_ledgerhash(setHash.data(), uint256::size()); + packet->set_ledgerseq(0); + packet->set_type(protocol::liTS_CANDIDATE); + + auto* const node = packet->add_nodes(); + node->set_nodedata("\xff", 1); + node->set_id(SHAMapNodeID{}.getRawString()); + + auto const peer = std::make_shared(); + inbound.gotData(setHash, peer, packet); + + BEAST_EXPECT(peer->charges() == std::vector{resource::kFeeInvalidData}); + } + + /** + * init() asks only the peers that claim to have the set. + * + * addPeers() passes hasTxSet(hash_) as its filter and trigger() as its + * callback, so a peer that says it has the set is asked and one that says + * it does not is left alone. Getting this wrong wastes a request on every + * peer in the overlay for every set. + * + * @param env The environment to run in. + */ + void + testInitAsksOnlyPeersWithTheSet(jtx::Env& env) + { + testcase("init() asks only the peers that have the set"); + + DeepChain const chain{nextSeed()}; + + // Ordered with the useless peer first, so a filter that is ignored altogether + // shows up as the wrong peer being asked rather than as one extra request. + auto const withoutSet = std::make_shared(false); + auto const withSet = std::make_shared(true); + + auto peerSet = std::make_unique( + std::vector>{withoutSet, withSet}); + auto* const peerSetPtr = peerSet.get(); + + auto const acquire = std::make_shared( + env.app(), chain.rootHash.asUInt256(), std::move(peerSet)); + + static constexpr int kStartPeers = 2; + acquire->init(kStartPeers); + + // Stop the retry loop, which would otherwise keep offering the same candidates + // for as long as this case runs. + acquire->cancel(); + + BEAST_EXPECT(peerSetPtr->firstLimit() == kStartPeers); + BEAST_EXPECT(peerSetPtr->addedPeers() == std::set{withSet->id()}); + + // The peer that was added is also asked, rather than merely tracked. + BEAST_EXPECT(peerSetPtr->requests() >= 1); + } + + /** + * The retry timer re-asks with no peer of its own, then gives up on + * its own. + * + * The only case that reaches onTimer(). Pins the two behaviors, not the + * thresholds they trip at: bounding those means asserting on wall clock. + * Both are read in one poll, so a fast interval cannot let the give-up land + * between the two readings. + * + * @param env The environment to run in. + */ + void + testTimerRetriesThenGivesUp(jtx::Env& env) + { + testcase("The retry timer re-asks, then gives up"); + + DeepChain const chain{nextSeed()}; + + auto peerSet = std::make_unique(); + auto* const peerSetPtr = peerSet.get(); + + // An unrelated hash, so the probe below can never be accepted. See probe(). + auto const acquire = std::make_shared( + env.app(), uint256{42}, std::move(peerSet), kFastRetry); + + // No candidates, so nothing goes out until onTimer() decides to broadcast. + acquire->init(1); + BEAST_EXPECT(peerSetPtr->requests() == 0); + + // A root that cannot hash to this acquisition's set is rejected without recording + // progress, so polling with it does not postpone the timeout being waited for. A fresh + // peer each time keeps the rejections from piling up on one. + auto const probe = [&] { + return wasIgnored(acquire->takeNodes( + {{SHAMapNodeID{}, chain.nodeAt(0)}}, std::make_shared())); + }; + + // kNormTimeouts (4) intervals in, onTimer() starts asking again with no peer of its own to + // ask, and the acquisition is still examining data at that point. + BEAST_EXPECT(waitFor([&] { return peerSetPtr->requests() > 0 && !probe(); })); + + // Past kMaxTimeouts (20) it fails itself, and stops examining data. + BEAST_EXPECT(waitFor(probe)); + + // Whichever poll saw the retry, the count it left behind is what records that it happened. + BEAST_EXPECT(peerSetPtr->requests() > 0); + } + + 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(): gotNode() puts every node it accepts + // into the application-wide NodeCache, so cases sharing an Env must not share a hash. + jtx::Env env{*this}; + + testHappyPathCompletesAcquisition(env); + testTwoPeersEachSupplyPartOfTheSet(env); + testBadRootKeepsAcquireAlive(env); + testDuplicateRootReplyIsFree(env); + testDuplicateNonRootReplyIsFree(env); + testUndeserializableNodeIsCharged(env); + testInitAsksOnlyPeersWithTheSet(env); + + // Last: the only case that waits out a whole timeout chain. + testTimerRetriesThenGivesUp(env); + } + +private: + unsigned int seed_{0}; +}; + +BEAST_DEFINE_TESTSUITE(TransactionAcquire, app, xrpl); + +} // namespace xrpl::test diff --git a/src/tests/libxrpl/shamap/DeepChain.h b/src/tests/libxrpl/shamap/DeepChain.h new file mode 100644 index 0000000000..57675b9781 --- /dev/null +++ b/src/tests/libxrpl/shamap/DeepChain.h @@ -0,0 +1,304 @@ +#pragma once + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +namespace xrpl::tests { + +/** + * A chain of inner nodes, each with one real child (and, when built with a decoy, an + * additional unresolvable second child), from the root down to one deepest node, in + * wire form. + * + * Built bottom-up, so the root hash commits to the whole shape and every node + * hashes correctly, which lets a test supply one level at a time and have each + * accepted on its own merits. Every node sits on the branch pathKey selects at + * its depth, so a receiver descending towards that key walks the whole chain. + * + * Two shapes, separating a chain a peer could honestly send from one it could + * not: + * + * - the constructors run inner nodes all the way to SHAMap::kLeafDepth, + * where no valid tree can hold one (see the badDepth check in + * SHAMap::addKnownNode), so feeding it invalidates the map; + * - toLeaf() stops at a real transaction leaf, so feeding it completes an + * acquisition. + * + * Shared by the gtest suites and the daemon's own, which is why nothing here + * reaches outside libxrpl: xrpl_tests links only xrpl.libxrpl, while Peer, + * PeerSet and the protobuf reply builder are all xrpld. The xrpld half of this + * helper is test::packetFor() in src/test/app/AcquireTestHelpers.h. + */ +struct DeepChain +{ + // nodes[d] is the deserialized node for depth d. + std::vector nodes; + SHAMapHash rootHash; + + // The key whose path through the tree this chain spells out. Zero for a + // fabricated chain, which therefore sits on branch 0 at every depth. + uint256 pathKey; + + // The depth of the deepest node, which is the last one nodesBelowRoot() hands out. + unsigned int deepestDepth{SHAMap::kLeafDepth}; + + /** + * The payload size of the leaf toLeaf() builds, which is the smallest a + * SHAMap item may be. Published so a caller can relate it to a threshold of + * its own, as AcquireTestHelpers.h does. + */ + static constexpr std::size_t kLeafItemBytes = kMinShaMapItemBytes; + + /** + * A chain of inner nodes reaching SHAMap::kLeafDepth, which no valid tree + * can hold. + * + * @param seed Varies the whole chain, so two chains can coexist without one + * resolving the other's nodes. Caches and fetch packs are keyed by + * hash, so identically-seeded chains are the same chain. + */ + explicit DeepChain(unsigned int seed = 1) : DeepChain(std::nullopt, seed, Decoy::No) + { + } + + /** + * A chain ending in a real transaction leaf, which completes an + * acquisition. + * + * @param depth Where the leaf sits, at most SHAMap::kLeafDepth. Zero puts + * the leaf at the root. Deeper is sparser than a one-transaction set + * would really be, but the sync path judges nodes by their hashes + * rather than by how sparse they are. + * @param seed Varies the leaf's contents, and so the whole chain. See the + * constructor. + * @return The chain. + */ + [[nodiscard]] static DeepChain + toLeaf(unsigned int depth, unsigned int seed = 1) + { + return DeepChain{std::optional{depth}, seed, Decoy::No}; + } + + /** + * The node the chain holds at the given depth, root first. + * + * @param depth The depth of the node to return, at most deepestDepth. + * @return The node. + */ + [[nodiscard]] SHAMapTreeNodePtr + nodeAt(unsigned int depth) const + { + return nodes[depth]; + } + + /** + * Where the node at the given depth claims to belong, which is on the + * path to pathKey. + * + * @param depth The depth of the node to locate. + * @return The node's claimed position. + */ + [[nodiscard]] SHAMapNodeID + idAt(unsigned int depth) const + { + return SHAMapNodeID::createID(depth, pathKey); + } + + /** + * The same node in the prefixed form used for storage and fetch packs, + * which is what hashes to the node's own hash. + * + * @param depth The depth of the node to serialize. + * @return The node's prefixed serialized form. + */ + [[nodiscard]] Blob + prefixedNodeAt(unsigned int depth) const + { + Serializer s; + nodeAt(depth)->serializeWithPrefix(s); + return s.modData(); + } + + /** + * Every node below the root, down to and including the deepest one. + * + * @param firstDepth The shallowest node to include, so a caller can feed + * the chain in more than one batch. + * @return The nodes, each with its claimed position. + */ + [[nodiscard]] std::vector> + nodesBelowRoot(unsigned int firstDepth = 1) const + { + std::vector> data; + for (auto depth = firstDepth; depth <= deepestDepth; ++depth) + data.emplace_back(idAt(depth), nodeAt(depth)); + return data; + } + + /** + * Fill a synching map, stopping one level short of the deepest node so the + * caller offers that one itself. + * + * Reports rather than asserts, since this header is shared with a binary + * that has no test framework of its own to assert through. + * + * @param map The map to fill. + * @return Whether the root and every node above the deepest one was + * accepted - a property of the chain, not of the map. + */ + [[nodiscard]] bool + fill(SHAMap& map) const + { + if (!map.addRootNode(rootHash, nodeAt(0), nullptr).isGood()) + return false; + + for (auto depth = 1u; depth < deepestDepth; ++depth) + { + if (!map.addKnownNode(idAt(depth), nodeAt(depth), nullptr).isUseful()) + return false; + } + + return true; + } + + /** + * Offer the deepest node, which for a fabricated chain is the inner node at + * SHAMap::kLeafDepth that no valid tree can hold. + * + * @param map The map to offer the node to, filled by fill() first. + * @return The verdict addKnownNode() reached. + */ + [[nodiscard]] SHAMapAddNode + addOffendingNode(SHAMap& map) const + { + return map.addKnownNode(idAt(deepestDepth), nodeAt(deepestDepth), nullptr); + } + +private: + // Whether each level carries a second child that is never stored anywhere. + enum class Decoy { No, Yes }; + + /** + * Build any of the shapes. + * + * @param leafDepth Where a real transaction leaf sits, or nullopt to run + * inner nodes all the way to SHAMap::kLeafDepth instead. + * @param seed Varies the chain's contents. See the public entry points. + * @param decoy Whether every level carries an unresolvable second child. + */ + DeepChain(std::optional leafDepth, unsigned int seed, Decoy decoy) + : nodes(leafDepth.value_or(SHAMap::kLeafDepth) + 1) + { + if (!leafDepth) + { + // No leaf, so the deepest inner node points at a child that is never fetched. + buildInnersDownTo(SHAMap::kLeafDepth, SHAMapHash{uint256{seed}}, decoy); + return; + } + + // Exactly kLeafItemBytes of payload, the smallest a leaf item may be, which keeps the leaf + // below the size at which a receiver tries to parse one as a transaction. Checked rather + // than assumed, since a caller relates that constant to a threshold of its own. + Serializer payload; + payload.add32(seed); + payload.add32(0); + payload.add32(0); + if (payload.size() != kLeafItemBytes) + Throw("DeepChain: unexpected leaf payload size"); + + Serializer wire; + wire.addRaw(payload.peekData()); + wire.add8(kWireTypeTransaction); + + auto const leaf = SHAMapTreeNode::makeFromWire(makeSlice(wire.peekData())); + + // A transaction leaf's key is the hash of its own contents, so the chain above it + // has no say in where it sits: it has to follow this key's nibbles. + pathKey = leafKey(*leaf); + deepestDepth = *leafDepth; + nodes[*leafDepth] = leaf; + + if (*leafDepth == 0) + { + // The leaf is the root, so there is nothing above it to build. + rootHash = leaf->getHash(); + return; + } + + buildInnersDownTo(*leafDepth - 1, leaf->getHash(), decoy); + } + + /** + * Fill in inner nodes, each with one real child (and, under Decoy::Yes, an + * additional unresolvable second child), from the root down to the given depth, + * and record the root hash. + * + * Bottom-up, since each node's hash covers the child hash below it. + * + * @param deepest The depth of the deepest inner node to build. May be + * SHAMap::kLeafDepth, which is the fabricated chain's whole point. + * @param childHash What that deepest inner node points at. + * @param decoy Whether to add an unresolvable second child at every level. + */ + void + buildInnersDownTo(unsigned int deepest, SHAMapHash childHash, Decoy decoy) + { + for (auto depth = deepest + 1; depth-- > 0;) + { + // A key has only 64 nibbles, so selectBranch() at SHAMap::kLeafDepth would index one + // byte past the end of the 32-byte key. A fabricated chain's pathKey is zero, so + // branch 0 is the position such a node claims anyway. + auto const branch = + depth == SHAMap::kLeafDepth ? 0u : selectBranch(idAt(depth), pathKey); + + Serializer s; + s.addBitString(childHash.asUInt256()); + s.add8(static_cast(branch)); + + if (decoy == Decoy::Yes) + { + // The decoy sits at branch 1, which only stays free of the real child because + // every caller that passes Decoy::Yes leaves pathKey at its default of zero. If a + // caller ever combined a non-zero pathKey with a decoy, the compressed-inner-node + // parser would silently let the decoy overwrite the real child's hash instead of + // rejecting the duplicate branch, so guard the assumption rather than rely on it. + if (branch == 1) + Throw("DeepChain: decoy branch collides with real child"); + + // Derived from the depth so it differs per level - each posts its own read - and + // is deterministic and cannot collide with a real node hash. + uint256 decoyHash; + decoyHash.begin()[0] = 0xDE; + decoyHash.begin()[1] = 0xC0; + decoyHash.begin()[2] = static_cast(depth); + s.addBitString(decoyHash); + s.add8(1); // the unresolvable decoy sits at branch 1 + } + + s.add8(kWireTypeCompressedInner); + + auto node = SHAMapTreeNode::makeFromWire(makeSlice(s.peekData())); + childHash = node->getHash(); + nodes[depth] = std::move(node); + } + + rootHash = childHash; + } +}; + +} // namespace xrpl::tests diff --git a/src/xrpld/app/ledger/ConsensusTransSetSF.cpp b/src/xrpld/app/ledger/ConsensusTransSetSF.cpp index a8ce70d978..4dd45b06e1 100644 --- a/src/xrpld/app/ledger/ConsensusTransSetSF.cpp +++ b/src/xrpld/app/ledger/ConsensusTransSetSF.cpp @@ -42,7 +42,7 @@ ConsensusTransSetSF::gotNode( nodeCache_.insert(nodeHash, nodeData); - if ((type == SHAMapNodeType::TnTransactionNm) && (nodeData.size() > 16)) + if ((type == SHAMapNodeType::TnTransactionNm) && (nodeData.size() >= kMinTxNodeBytesToParse)) { // this is a transaction, and we didn't have it JLOG(j_.debug()) << "Node on our acquiring TX set is TXN we may not have"; diff --git a/src/xrpld/app/ledger/ConsensusTransSetSF.h b/src/xrpld/app/ledger/ConsensusTransSetSF.h index 59894add7f..c38aee5755 100644 --- a/src/xrpld/app/ledger/ConsensusTransSetSF.h +++ b/src/xrpld/app/ledger/ConsensusTransSetSF.h @@ -9,6 +9,7 @@ #include #include +#include #include #include @@ -24,6 +25,18 @@ class ConsensusTransSetSF : public SHAMapSyncFilter public: using NodeCache = TaggedCache; + /** + * The size a node's hash-prefixed wire data must reach before gotNode() + * tries to parse and resubmit it as a transaction. + * + * A threshold rather than a derived bound: the smallest a hash-prefixed + * SHAMap leaf can be is the 4-byte HashPrefix plus kMinShaMapItemBytes, and + * nothing that size is a signed transaction either. The extra byte is the + * long-standing threshold this check has always used, kept as it was. + */ + static constexpr std::size_t kMinTxNodeBytesToParse = + sizeof(std::uint32_t) + kMinShaMapItemBytes + 1; + ConsensusTransSetSF(Application& app, NodeCache& nodeCache); // Note that the nodeData is overwritten by this call diff --git a/src/xrpld/app/ledger/InboundLedger.h b/src/xrpld/app/ledger/InboundLedger.h index 9a7ee510f6..5add2afc98 100644 --- a/src/xrpld/app/ledger/InboundLedger.h +++ b/src/xrpld/app/ledger/InboundLedger.h @@ -30,9 +30,9 @@ namespace xrpl { // A ledger we are trying to acquire -class InboundLedger final : public TimeoutCounter, - public std::enable_shared_from_this, - public CountedObject +class InboundLedger : public TimeoutCounter, + public std::enable_shared_from_this, + public CountedObject { public: using clock_type = beast::AbstractClock; @@ -44,13 +44,33 @@ public: CONSENSUS // We believe the consensus round requires this ledger }; + /** + * How long to wait between retries, and so how long each timeout counted + * against the acquisition takes. Long, since a ledger is worth chasing for + * far longer than a consensus round. + */ + static constexpr std::chrono::milliseconds kRetryInterval{3000}; + + /** + * @param app The application to run in. + * @param hash The ledger to acquire. + * @param seq Its sequence, or zero if not known yet. + * @param reason Why it is being acquired. + * @param clock The clock touch() records against. + * @param peerSet Which peers to ask, and how to reach them. + * @param retryInterval How long to wait between retries. Defaulted in + * production; InboundLedger_test passes a short one so a whole + * timeout chain runs in a fraction of the time. TimeoutCounter + * requires more than 10ms. + */ InboundLedger( Application& app, uint256 const& hash, std::uint32_t seq, Reason reason, - clock_type&, - std::unique_ptr peerSet); + clock_type& clock, + std::unique_ptr peerSet, + std::chrono::milliseconds retryInterval = kRetryInterval); ~InboundLedger() override; @@ -119,15 +139,35 @@ public: return lastAction_; } -private: +protected: + // Kept protected, with the two entry points naming it, so a test subclass (see + // InboundLedger_test) can drive an acquisition the way the timer chain does, without routing + // through the JobQueue. Production callers reach an acquisition through InboundLedgers. + + // Why trigger() is being run, which decides how deep a request goes and whether the + // aggressive-retry branch is eligible. enum class TriggerReason { Added, Reply, Timeout }; + /** + * Ask for more nodes, or judge what has been collected. + * + * @param peer The peer to ask, or nullptr to ask everyone being tracked. + * @param reason Why the acquisition is being triggered. + */ + void + trigger(std::shared_ptr const& peer, TriggerReason reason); + + /** + * Settle the acquisition and signal whatever is waiting on it. Runs at most + * once. Call under mtx_, which the flags written here require. + */ + void + done(); + +private: void filterNodes(std::vector>& nodes, TriggerReason reason); - void - trigger(std::shared_ptr const&, TriggerReason); - std::vector getNeededHashes(); @@ -138,10 +178,7 @@ private: tryDB(node_store::Database& srcDB); void - done(); - - void - onTimer(bool progress, ScopedLockType& peerSetLock) override; + onTimer(bool progress, ScopedLockType& sl) override; std::size_t getPeerCount() const; diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp index 246c7d567b..4d3d3780a3 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp @@ -54,8 +54,6 @@ namespace xrpl { -using namespace std::chrono_literals; - static constexpr auto kPeerCountStart = 5; // Number of peers to start with static constexpr auto kPeerCountAdd = 3; // Number of peers to add on a timeout static constexpr auto kLedgerTimeoutRetriesMax = 6; // how many timeouts before we give up @@ -65,20 +63,18 @@ static constexpr auto kMissingNodesFind = 256; // Number of nodes to find initi static constexpr auto kReqNodesReply = 128; // Number of nodes to request for a reply static constexpr auto kReqNodes = 12; // Number of nodes to request blindly -// millisecond for each ledger timeout -constexpr auto kLedgerAcquireTimeout = 3000ms; - InboundLedger::InboundLedger( Application& app, uint256 const& hash, std::uint32_t seq, Reason reason, clock_type& clock, - std::unique_ptr peerSet) + std::unique_ptr peerSet, + std::chrono::milliseconds retryInterval) : TimeoutCounter( app, hash, - kLedgerAcquireTimeout, + retryInterval, {.jobType = JtLedgerData, .jobName = "InboundLedger", .jobLimit = 5}, app.getJournal("InboundLedger")) , clock_(clock) diff --git a/src/xrpld/app/ledger/detail/TimeoutCounter.h b/src/xrpld/app/ledger/detail/TimeoutCounter.h index 682abf1537..6e6e732981 100644 --- a/src/xrpld/app/ledger/detail/TimeoutCounter.h +++ b/src/xrpld/app/ledger/detail/TimeoutCounter.h @@ -98,9 +98,16 @@ protected: /** * Hook called from invokeOnTimer(). + * + * @param progress Whether the subtype recorded progress since the + * last call. + * @param sl Proof mtx_ is held, and held for the whole call. It is + * this object's own mutex rather than anything belonging to a + * PeerSet, and it is recursive, so a nested lock taken inside + * this call releases nothing when it goes out of scope. */ virtual void - onTimer(bool progress, ScopedLockType&) = 0; + onTimer(bool progress, ScopedLockType& sl) = 0; /** * Return a weak pointer to this. diff --git a/src/xrpld/app/ledger/detail/TransactionAcquire.cpp b/src/xrpld/app/ledger/detail/TransactionAcquire.cpp index db99299fd6..5dd1b1907d 100644 --- a/src/xrpld/app/ledger/detail/TransactionAcquire.cpp +++ b/src/xrpld/app/ledger/detail/TransactionAcquire.cpp @@ -18,6 +18,7 @@ #include #include +#include #include #include #include @@ -26,22 +27,18 @@ namespace xrpl { -using namespace std::chrono_literals; - -// Timeout interval in milliseconds -constexpr auto kTxAcquireTimeout = 250ms; - static constexpr auto kNormTimeouts = 4; static constexpr auto kMaxTimeouts = 20; TransactionAcquire::TransactionAcquire( Application& app, uint256 const& hash, - std::unique_ptr peerSet) + std::unique_ptr peerSet, + std::chrono::milliseconds retryInterval) : TimeoutCounter( app, hash, - kTxAcquireTimeout, + retryInterval, {.jobType = JtTxnData, .jobName = "TxAcq", .jobLimit = {}}, app.getJournal("TransactionAcquire")) , peerSet_(std::move(peerSet)) @@ -79,7 +76,7 @@ TransactionAcquire::done() } void -TransactionAcquire::onTimer(bool progress, ScopedLockType& psl) +TransactionAcquire::onTimer(bool progress, ScopedLockType&) { if (timeouts_ > kMaxTimeouts) { diff --git a/src/xrpld/app/ledger/detail/TransactionAcquire.h b/src/xrpld/app/ledger/detail/TransactionAcquire.h index 2faf74b557..87eb632b1f 100644 --- a/src/xrpld/app/ledger/detail/TransactionAcquire.h +++ b/src/xrpld/app/ledger/detail/TransactionAcquire.h @@ -11,6 +11,7 @@ #include #include +#include #include #include #include @@ -20,14 +21,34 @@ namespace xrpl { // VFALCO TODO rename to PeerTxRequest // A transaction set we are trying to acquire -class TransactionAcquire final : public TimeoutCounter, - public std::enable_shared_from_this, - public CountedObject +class TransactionAcquire : public TimeoutCounter, + public std::enable_shared_from_this, + public CountedObject { public: using pointer = std::shared_ptr; - TransactionAcquire(Application& app, uint256 const& hash, std::unique_ptr peerSet); + /** + * How long to wait between retries, and so how long each timeout counted + * against the acquisition takes. Short, since a set is wanted for the + * consensus round that asked for it or not at all. + */ + static constexpr std::chrono::milliseconds kRetryInterval{250}; + + /** + * @param app The application to run in. + * @param hash The set to acquire. + * @param peerSet Which peers to ask, and how to reach them. + * @param retryInterval How long to wait between retries. Defaulted in + * production; TransactionAcquire_test passes a short one so a whole + * timeout chain runs in a fraction of the time. TimeoutCounter + * requires more than 10ms. + */ + TransactionAcquire( + Application& app, + uint256 const& hash, + std::unique_ptr peerSet, + std::chrono::milliseconds retryInterval = kRetryInterval); ~TransactionAcquire() override = default; SHAMapAddNode @@ -41,14 +62,25 @@ public: void stillNeed(); -private: +protected: + // Kept protected so a test subclass (see TransactionAcquire_test) can read the map's state, + // which nothing else publishes. Production callers reach a set through InboundTransactions. std::shared_ptr map_; + +private: bool haveRoot_{false}; std::unique_ptr peerSet_; void - onTimer(bool progress, ScopedLockType& peerSetLock) override; + onTimer(bool progress, ScopedLockType& sl) override; + /** + * Settle the acquired set and hand it on, or report the failure. Call under + * mtx_. + * + * Runs at most once per outcome rather than once in total, since + * stillNeed() can revive a timed-out acquisition that then finishes. + */ void done();