From d8754775b5f1667a0bdd4f524cdd0e3747e79c81 Mon Sep 17 00:00:00 2001 From: Bart <11445373+bthomee@users.noreply.github.com> Date: Mon, 24 Aug 2026 09:35:10 -0400 Subject: [PATCH] refactor: Add a reusable peer harness for acquisition tests DeepChain (src/tests/libxrpl/shamap/DeepChain.h) builds node chains for both acquisition suites: fabricated chains that run to SHAMap::kLeafDepth, which no valid tree can hold, and toLeaf() chains that complete an acquisition. AcquireTestHelpers.h adds ChargeRecordingPeer, RequestCountingPeerSet (deduping by tracked id like the real PeerSetImpl), packetFor(), waitFor(), and tallyIs(), so both suites can drive an acquisition through its real gotData() dispatch instead of reproducing it. TransactionAcquire and InboundLedger drop final and take a defaulted retryInterval, so tests can run a whole timeout chain in a fraction of a second; nothing in production passes one. AcquireTestHelpers.h is the first src/test file to include one from src/tests, so levelization records a new test.app > tests.libxrpl edge in ordering.txt. No loop is introduced: nothing under src/tests includes src/test. Addresses Copilot review feedback on PR #8081. --- .cspell.config.yaml | 2 + .../scripts/levelization/results/ordering.txt | 1 + src/test/app/AcquireTestHelpers.h | 464 ++++++++++++++++ src/test/app/InboundLedger_test.cpp | 312 +++++++++++ src/test/app/TransactionAcquire_test.cpp | 512 ++++++++++++++++++ src/tests/libxrpl/shamap/DeepChain.h | 304 +++++++++++ src/xrpld/app/ledger/ConsensusTransSetSF.cpp | 2 +- src/xrpld/app/ledger/ConsensusTransSetSF.h | 13 + src/xrpld/app/ledger/InboundLedger.h | 63 ++- src/xrpld/app/ledger/detail/InboundLedger.cpp | 10 +- src/xrpld/app/ledger/detail/TimeoutCounter.h | 9 +- .../app/ledger/detail/TransactionAcquire.cpp | 13 +- .../app/ledger/detail/TransactionAcquire.h | 44 +- 13 files changed, 1713 insertions(+), 36 deletions(-) create mode 100644 src/test/app/AcquireTestHelpers.h create mode 100644 src/test/app/InboundLedger_test.cpp create mode 100644 src/test/app/TransactionAcquire_test.cpp create mode 100644 src/tests/libxrpl/shamap/DeepChain.h diff --git a/.cspell.config.yaml b/.cspell.config.yaml index 8929973e8a..a46a9f2bac 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();