From c45ff5536635c45b0344c095b01370bc48e2f179 Mon Sep 17 00:00:00 2001 From: Bart <11445373+bthomee@users.noreply.github.com> Date: Mon, 24 Aug 2026 09:40:50 -0400 Subject: [PATCH] fix: Fail fast on an invalid map, and bound late replies by peer asked A node that leaves a map invalid proves the hash being chased cannot belong to any valid tree (see SHAMap::addKnownNode), so no peer can ever complete it. TransactionAcquire::takeNodesLocked() and InboundLedger::receiveNode() now fail the acquisition there instead of retrying until the timeout chain runs out, discarding the whole batch: the nodes hooked in ahead of the bad one belong to a tree that cannot exist. stillNeed() and InboundTransactions::getSet() refuse to revive or refresh such an acquisition, so a dead entry is swept rather than held open. Charging happens under the same lock that reaches the verdict: kFeeMalformedData for a node that invalidates the map, kFeeInvalidData for data that is merely wrong. A reply arriving after the set is settled is free once per peer the acquisition actually asked - trigger() can send a targeted request to an unsolicited sender directly, not only to peers addPeers() selected, so requestedPeers_ tracks every peer sent a request either way. The allowance is keyed by peer identity (lateReplyGranted_), not a shared count, so one peer replaying its own already-accepted reply cannot exhaust the pass a different, honest peer is still owed. Past the allowance, a reply is a replay and costs kFeeUselessData. --- src/test/app/InboundLedger_test.cpp | 181 +++++++ src/test/app/TransactionAcquire_test.cpp | 451 +++++++++++++++++- src/xrpld/app/ledger/detail/InboundLedger.cpp | 25 +- .../app/ledger/detail/InboundTransactions.cpp | 28 +- .../app/ledger/detail/TransactionAcquire.cpp | 86 +++- .../app/ledger/detail/TransactionAcquire.h | 74 ++- 6 files changed, 791 insertions(+), 54 deletions(-) diff --git a/src/test/app/InboundLedger_test.cpp b/src/test/app/InboundLedger_test.cpp index 8f1b37d378..8aecd81d6d 100644 --- a/src/test/app/InboundLedger_test.cpp +++ b/src/test/app/InboundLedger_test.cpp @@ -15,6 +15,11 @@ #include #include #include +#include +#include +#include + +#include #include #include @@ -90,6 +95,19 @@ struct TestableInboundLedger final : InboundLedger progress_ = false; } + /** + * Whether a packet has advanced the acquisition since the flag was last + * cleared. + * + * @return Whether progress has been recorded. + */ + [[nodiscard]] bool + madeProgress() const + { + ScopedLockType const sl(mtx_); + return progress_; + } + /** * Record that nothing is left to fetch. */ @@ -237,6 +255,46 @@ struct InboundLedger_test : public beast::unit_test::Suite } } + /** + * The header as a liBASE reply, which is how an acquisition learns what it + * is chasing. + * + * @param header The header to serialize. + * @return The reply packet. + */ + static std::shared_ptr + headerPacket(LedgerHeader const& header) + { + auto packet = std::make_shared(); + packet->set_ledgerhash(header.hash.data(), uint256::size()); + packet->set_ledgerseq(header.seq); + packet->set_type(protocol::liBASE); + + Serializer s; + addRaw(header, s); + + auto* const node = packet->add_nodes(); + node->set_nodedata(s.peekData().data(), s.peekData().size()); + return packet; + } + + /** + * The chain's state-map nodes as a liAS_NODE reply for the given header. + * + * @param header The header whose hash and sequence the reply names. + * @param chain The chain supplying the nodes. + * @param data The nodes to include, each with its claimed position. + * @return The reply packet. + */ + static std::shared_ptr + stateNodePacket( + LedgerHeader const& header, + DeepChain const& chain, + std::vector> const& data) + { + return packetFor(chain, data, protocol::liAS_NODE, header.hash, header.seq); + } + /** * A ledger whose maps all resolve locally finishes on the spot, and the * finished ledger is immutable and handed on. @@ -485,6 +543,127 @@ struct InboundLedger_test : public beast::unit_test::Suite waitFor([&] { return env.app().getInboundLedgers().isFailure(otherHeader.hash); })); } + /** + * An acquisition whose state root names a shape no valid tree can have must + * fail, and must cost the sender the harsher tier. + * + * @param env The environment to run in. + */ + void + testFabricatedChainFailsAcquire(jtx::Env& env) + { + testcase("A state-map chain reaching kLeafDepth fails the acquire"); + + DeepChain const chain{nextSeed()}; + auto const header = makeHeader(chain); + + auto acquire = std::make_shared( + env.app(), + header.hash, + header.seq, + InboundLedger::Reason::GENERIC, + stopwatch(), + std::make_unique()); + + // The header is accepted on its own terms, so the acquisition now chases this hash. + auto const headerPeer = std::make_shared(); + BEAST_EXPECT(acquire->gotData(headerPeer, headerPacket(header))); + acquire->runData(); + BEAST_EXPECT(headerPeer->charges().empty()); + BEAST_EXPECT(!acquire->isFailed()); + + // The root, then the rest of the chain ending in the inner node at kLeafDepth. + std::vector> data; + data.emplace_back(SHAMapNodeID{}, chain.nodeAt(0)); + for (auto const& node : chain.nodesBelowRoot()) + data.push_back(node); + + // The header counted as progress, so clear it to see what the packet below records. + acquire->clearProgress(); + + auto const chainPeer = std::make_shared(); + BEAST_EXPECT(acquire->gotData(chainPeer, stateNodePacket(header, chain, data))); + acquire->runData(); + + // The acquisition is over, and stays over: no peer can satisfy this hash. + BEAST_EXPECT(acquire->isFailed()); + BEAST_EXPECT(!acquire->isComplete()); + + // The nodes ahead of the bad one belong to a tree that cannot exist, so the packet counts + // nothing at all. Nothing else observes that tally, so without this the discarding could be + // dropped and the suite would stay green. + BEAST_EXPECT(!acquire->madeProgress()); + + // A failed acquisition must not hand back the partial ledger it built. + BEAST_EXPECT(acquire->getLedger() == nullptr); + + // Charged as data no honest peer sends by accident, not as merely-wrong data. + BEAST_EXPECT(chainPeer->charges() == std::vector{resource::kFeeMalformedData}); + + // getJson() walks the same maps to report what is still needed, and is reachable over RPC + // for as long as sweep() keeps the failed entry. It must report the failure and come back + // with nothing needed rather than descending the abandoned map. + auto const report = acquire->getJson(0); + BEAST_EXPECT(report[jss::failed].asBool()); + BEAST_EXPECT(report[jss::have_header].asBool()); + BEAST_EXPECT(!report[jss::have_state].asBool()); + BEAST_EXPECT(report[jss::needed_state_hashes].size() == 0); + } + + /** + * A merely-wrong state node must cost the recoverable tier and leave the + * acquisition alive. + * + * The counterpart to testFabricatedChainFailsAcquire() on this path: + * the fee split in receiveNode() turns on whether the map survived, so + * a node that cannot be hooked but leaves the map sound must be + * charged the lower tier. + * + * @param env The environment to run in. + */ + void + testWrongStateNodeKeepsAcquireAlive(jtx::Env& env) + { + testcase("A merely-wrong state node leaves the acquire recoverable"); + + DeepChain const chain{nextSeed()}; + auto const header = makeHeader(chain); + + auto acquire = std::make_shared( + env.app(), + header.hash, + header.seq, + InboundLedger::Reason::GENERIC, + stopwatch(), + std::make_unique()); + + auto const headerPeer = std::make_shared(); + BEAST_EXPECT(acquire->gotData(headerPeer, headerPacket(header))); + acquire->runData(); + BEAST_EXPECT(!acquire->isFailed()); + + auto const rootPeer = std::make_shared(); + BEAST_EXPECT(acquire->gotData( + rootPeer, stateNodePacket(header, chain, {{SHAMapNodeID{}, chain.nodeAt(0)}}))); + acquire->runData(); + BEAST_EXPECT(rootPeer->charges().empty()); + + // nodeAt(1) is the node the root is missing and its hash matches, but we label it as + // living at depth 2, so it cannot be hooked anywhere. + auto const wrongPeer = std::make_shared(); + BEAST_EXPECT(acquire->gotData( + wrongPeer, + stateNodePacket(header, chain, {{SHAMapNodeID{2, uint256{}}, chain.nodeAt(1)}}))); + acquire->runData(); + + BEAST_EXPECT(wrongPeer->charges() == std::vector{resource::kFeeInvalidData}); + + // The map is sound, so the acquisition is still going and still holds its ledger. + BEAST_EXPECT(!acquire->isFailed()); + BEAST_EXPECT(!acquire->isComplete()); + BEAST_EXPECT(acquire->getLedger() != nullptr); + } + /** * A ledger assembled from local data must be judged even when only * one map is settled. @@ -726,6 +905,8 @@ struct InboundLedger_test : public beast::unit_test::Suite testWalkSettlesBeforeReportingComplete(env); testInvalidatedLedgerFailsInDone(env); testLocalFailureSignalsDone(env); + testFabricatedChainFailsAcquire(env); + testWrongStateNodeKeepsAcquireAlive(env); testLocalChainFailsAcquire(env); testAggressiveRetryJudgesLocalMap(env); diff --git a/src/test/app/TransactionAcquire_test.cpp b/src/test/app/TransactionAcquire_test.cpp index 869f270547..e1eb84bb04 100644 --- a/src/test/app/TransactionAcquire_test.cpp +++ b/src/test/app/TransactionAcquire_test.cpp @@ -103,8 +103,10 @@ struct TransactionAcquire_test : public beast::unit_test::Suite /** * 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. + * TimeoutCounter::complete_ and failed_ are both protected, so this + * stands in for either. A done acquisition returns a bare duplicate, + * which is also what "we already have this root" reports, so the + * cases below pair this with a request count to tell the two apart. * * @param san The verdict a takeNodes() call returned. * @return Whether that verdict shows the data was never looked at. @@ -112,7 +114,24 @@ struct TransactionAcquire_test : public beast::unit_test::Suite static bool wasIgnored(SHAMapAddNode const& san) { - return tallyIs(san, 0, 0, 0); + return tallyIs(san, 0, 0, 1); + } + + /** + * Whether takeNodes() declined the data and held the sender responsible. + * + * The counterpart to wasIgnored(): a set whose map cannot be + * satisfied rejects further replies outright rather than reporting + * them as merely unwanted. Both cost the sender the same, so this is + * about the verdict rather than the fee. + * + * @param san The verdict a takeNodes() call returned. + * @return Whether that verdict shows the sender was held responsible. + */ + static bool + wasRejected(SHAMapAddNode const& san) + { + return tallyIs(san, 0, 1, 0); } /** @@ -244,9 +263,93 @@ struct TransactionAcquire_test : public beast::unit_test::Suite BEAST_EXPECT(delivered->getHash() == chain.rootHash); } + /** + * A chain reaching kLeafDepth must end the acquisition outright. + * + * @param env The environment to run in. + */ + void + testFabricatedChainFailsAcquire(jtx::Env& env) + { + testcase("A chain reaching kLeafDepth fails the acquire"); + + DeepChain const chain{nextSeed()}; + + auto peerSet = std::make_unique(); + auto* const peerSetPtr = peerSet.get(); + + auto const acquire = std::make_shared( + env.app(), chain.rootHash.asUInt256(), std::move(peerSet)); + auto const peer = std::make_shared(); + + // The root hashes to the set we asked for, so it is accepted. + auto const rootResult = acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, peer); + BEAST_EXPECTS(tallyIs(rootResult, 1, 0, 0), rootResult.get()); + BEAST_EXPECT(acquire->isMapValid()); + + // Accepting the root asks the peer for more, which is what the failure below has to stop. + int const requestsWhileAlive = peerSetPtr->requests(); + BEAST_EXPECT(requestsWhileAlive > 0); + + // Now the rest of the chain, ending in the inner node at kLeafDepth that no valid tree + // can hold. + auto const result = acquire->takeNodes(chain.nodesBelowRoot(), peer); + + BEAST_EXPECTS(tallyIs(result, 0, 1, 0), result.get()); + BEAST_EXPECT(!acquire->isMapValid()); + + // The acquisition is now dead: further data is not examined and no further requests go + // out, so the failure is recorded where the map is found invalid rather than left to a + // later trigger(). Rejected rather than merely ignored, since a later reply for a hash with + // no valid tree is worthless to everyone. + auto const afterFailure = acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, peer); + BEAST_EXPECT(wasRejected(afterFailure)); + BEAST_EXPECT(peerSetPtr->requests() == requestsWhileAlive); + + // stillNeed() revives a timed-out acquire, but must not revive this one: no valid tree + // exists for the hash, so re-arming it would only re-fail on the next packet. It says so, + // which is what stops InboundTransactions holding the entry out of newRound()'s reach. + BEAST_EXPECT(!acquire->stillNeed()); + BEAST_EXPECT(wasRejected(acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, peer))); + BEAST_EXPECT(peerSetPtr->requests() == requestsWhileAlive); + } + + /** + * A node that is merely wrong must leave the acquisition alive, so another + * peer can still complete it. + * + * @param env The environment to run in. + */ + void + testWrongNodeKeepsAcquireAlive(jtx::Env& env) + { + testcase("A merely-wrong node leaves the acquire recoverable"); + + DeepChain const chain{nextSeed()}; + + auto const acquire = std::make_shared( + env.app(), chain.rootHash.asUInt256(), std::make_unique()); + auto const peer = std::make_shared(); + + auto const rootResult = acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, peer); + BEAST_EXPECTS(tallyIs(rootResult, 1, 0, 0), rootResult.get()); + + // nodeAt(1) is exactly the node the root is missing and its hash matches, but we label it + // as living at depth 2. It cannot be hooked anywhere, yet the map stays sound. + auto const result = + acquire->takeNodes({{SHAMapNodeID{2, uint256{}}, chain.nodeAt(1)}}, peer); + + BEAST_EXPECTS(tallyIs(result, 0, 1, 0), result.get()); + BEAST_EXPECT(acquire->isMapValid()); + + // Still alive: the next packet is examined rather than ignored. + BEAST_EXPECT( + !wasIgnored(acquire->takeNodes({{SHAMapNodeID{2, uint256{}}, chain.nodeAt(1)}}, peer))); + } + /** * 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. + * not a structural impossibility. * * @param env The environment to run in. */ @@ -269,16 +372,108 @@ struct TransactionAcquire_test : public beast::unit_test::Suite // is untouched. BEAST_EXPECT(acquire->isMapValid()); + // Charged at the recoverable tier, not the fabrication one: a mismatched root proves + // nothing about the tree behind the hash we asked for. + BEAST_EXPECT(peer->charges() == std::vector{resource::kFeeInvalidData}); + // Still alive: the next packet is examined rather than waved through. BEAST_EXPECT(!wasIgnored(acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, peer))); } + /** + * A reply carrying no nodes at all is charged for. + * + * PeerImp rejects an empty node list before dispatch, so this is + * defensive, but it is still the one rejection whose charge is + * decided before any data is looked at. An empty reply says nothing + * about the map, so the acquisition stays alive. + * + * @param env The environment to run in. + */ + void + testEmptyReplyIsCharged(jtx::Env& env) + { + testcase("A reply carrying no nodes is charged as invalid data"); + + DeepChain const chain{nextSeed()}; + + auto const acquire = std::make_shared( + env.app(), chain.rootHash.asUInt256(), std::make_unique()); + auto const peer = std::make_shared(); + + auto const result = acquire->takeNodes({}, peer); + + BEAST_EXPECTS(tallyIs(result, 0, 1, 0), result.get()); + BEAST_EXPECT(peer->charges() == std::vector{resource::kFeeInvalidData}); + + // Nothing was touched, so another peer can still complete the set. + BEAST_EXPECT(acquire->isMapValid()); + BEAST_EXPECT(!wasIgnored(acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, peer))); + } + + /** + * The fee tier split, driven through InboundTransactions::gotData(). + * + * Goes through the real dispatch rather than reproducing its branch, so + * charging the wrong tier - or dropping the distinction - fails here. + * + * @param env The environment to run in. + */ + void + testFeeTierDistinguishesFabrication(jtx::Env& env) + { + testcase("Map-invalidating data is charged more harshly than wrong data"); + + // Guard the premise: if the tiers were equal, the distinction would be cosmetic. + BEAST_EXPECT(resource::kFeeMalformedData.cost() > resource::kFeeInvalidData.cost()); + + DeepChain const chain{nextSeed()}; + auto& inbound = env.app().getInboundTransactions(); + + // getSet() with acquire=true registers the TransactionAcquire that gotData() looks up. + uint256 const setHash = chain.rootHash.asUInt256(); + BEAST_EXPECT(inbound.getSet(setHash, true) == nullptr); + + // Feed the root first, so the acquire has somewhere to hook the rest. + auto const rootPeer = std::make_shared(); + inbound.gotData(setHash, rootPeer, packetFor(chain, {{SHAMapNodeID{}, chain.nodeAt(0)}})); + BEAST_EXPECT(rootPeer->charges().empty()); + + // A real node labeled with the wrong position: wrong, but the map stays sound, so this is + // the generic tier. + auto const wrongPeer = std::make_shared(); + inbound.gotData( + setHash, wrongPeer, packetFor(chain, {{SHAMapNodeID{2, uint256{}}, chain.nodeAt(1)}})); + BEAST_EXPECT(wrongPeer->charges() == std::vector{resource::kFeeInvalidData}); + + // The chain, ending in an inner node at kLeafDepth. This invalidates the map, so it costs + // the harsher tier. + auto const fabricatingPeer = std::make_shared(); + inbound.gotData(setHash, fabricatingPeer, packetFor(chain, chain.nodesBelowRoot())); + BEAST_EXPECT(fabricatingPeer->charges() == std::vector{resource::kFeeMalformedData}); + + // Data arriving after the set is over costs the useless tier, not the harsher one: the + // sender of this packet is not the one that broke the set. rootPeer's accepted root + // earned the one targeted follow-up request that is this test's only source of an + // allowance to spend - there are no other real peers here. Its own late reply is free; + // a second one from it has already spent that pass and is a replay. See + // testLateReplyIsFreeOncePerPeerAsked() for the peer-identity cases (a stranger nobody + // asked, and a second reply from the same peer) in isolation. + inbound.gotData(setHash, rootPeer, packetFor(chain, {{SHAMapNodeID{}, chain.nodeAt(0)}})); + BEAST_EXPECT(rootPeer->charges().empty()); + + inbound.gotData(setHash, rootPeer, packetFor(chain, {{SHAMapNodeID{}, chain.nodeAt(0)}})); + BEAST_EXPECT(rootPeer->charges() == std::vector{resource::kFeeUselessData}); + } + /** * 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 + * 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. + * takeNodes() therefore tests isGood() rather than isUseful(): an + * all-duplicate batch is good but not useful, and charging it would * penalize peers for answering. * * Covers the root specifically, which takeNodes() short-circuits on @@ -355,6 +550,183 @@ struct TransactionAcquire_test : public beast::unit_test::Suite BEAST_EXPECT(secondPeer->charges().empty()); } + /** + * A reply arriving after the set is settled is free once for the peer + * we asked, and charged after that. + * + * trigger() sends to every peer it was given, so when one of them + * settles the set the others' replies are already in flight and none + * of those senders could have known - including when what settled + * the set was a failure another peer caused. Charging them taxes + * honest peers for someone else's doing. The pass belongs to the + * specific peer requestedPeers_ says was asked, not to whichever late + * reply happens to arrive first: a peer nobody asked gets no benefit + * from it, and a peer that already spent it is replaying. What must + * not be free at all is a replay: InboundTransactions::gotData() + * deserializes and hashes the whole node list before takeNodes() ever + * sees it, so an unbounded number of them would otherwise cost a + * sender nothing but the per-message fee. + * + * @param env The environment to run in. + */ + void + testLateReplyIsFreeOncePerPeerAsked(jtx::Env& env) + { + testcase("A late reply is free once per peer we asked"); + + // A chain ending in a leaf, so the set settles rather than failing. + auto const chain = DeepChain::toLeaf(1, nextSeed()); + + // One peer asked, so it is the only one whose late reply can legitimately be free. + auto const candidate = std::make_shared(); + auto peerSet = + std::make_unique(std::vector>{candidate}); + auto* const peerSetPtr = peerSet.get(); + + auto const acquire = std::make_shared( + env.app(), chain.rootHash.asUInt256(), std::move(peerSet), kFastRetry); + + acquire->init(1); + BEAST_EXPECT(peerSetPtr->addedPeers() == std::set{candidate->id()}); + + // The whole chain in one batch, from a different peer, which settles the set. + auto data = chain.nodesBelowRoot(); + data.emplace(data.begin(), SHAMapNodeID{}, chain.nodeAt(0)); + + auto const supplier = std::make_shared(); + BEAST_EXPECT(acquire->takeNodes(std::move(data), supplier).isUseful()); + BEAST_EXPECT(supplier->charges().empty()); + + // A peer nobody asked is charged immediately: the allowance belongs to candidate, not + // to whichever late reply happens to arrive first. + auto const stranger = std::make_shared(); + BEAST_EXPECT(wasIgnored(acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, stranger))); + BEAST_EXPECT(stranger->charges() == std::vector{resource::kFeeUselessData}); + + // candidate's own late reply is the one that was genuinely in flight, and is free. + BEAST_EXPECT( + wasIgnored(acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, candidate))); + BEAST_EXPECT(candidate->charges().empty()); + + // A second reply from candidate has already spent its pass: this one is a replay. + BEAST_EXPECT( + wasIgnored(acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, candidate))); + BEAST_EXPECT(candidate->charges() == std::vector{resource::kFeeUselessData}); + + acquire->cancel(); + } + + /** + * One peer replaying its own late reply must not spend a different, + * genuinely honest peer's allowance. + * + * The allowance is tracked by which specific peers in requestedPeers_ + * have redeemed it (lateReplyGranted_), not by a shared count compared + * against requestedPeers_.size(). A count cannot tell whose slot a + * reply is spending, so one peer resending its own already-accepted + * reply several times would exhaust the whole allowance and leave a + * second, honestly-asked peer's first and only late reply charged for + * someone else's replaying. + * + * @param env The environment to run in. + */ + void + testOnePeersReplaysDoNotStarveAnother(jtx::Env& env) + { + testcase("One peer's replays do not spend a different peer's allowance"); + + auto const chain = DeepChain::toLeaf(1, nextSeed()); + + // Two peers asked, so each earns its own pass. + auto const spammer = std::make_shared(); + auto const honest = std::make_shared(); + auto peerSet = std::make_unique( + std::vector>{spammer, honest}); + + auto const acquire = std::make_shared( + env.app(), chain.rootHash.asUInt256(), std::move(peerSet), kFastRetry); + + acquire->init(2); + + // A third peer supplies the whole chain, so both spammer's and honest's replies below + // are late. + auto data = chain.nodesBelowRoot(); + data.emplace(data.begin(), SHAMapNodeID{}, chain.nodeAt(0)); + auto const supplier = std::make_shared(); + BEAST_EXPECT(acquire->takeNodes(std::move(data), supplier).isUseful()); + + // spammer's first late reply is free - its own pass - but every one after that is its + // own replay, not anyone else's slot to spend. + BEAST_EXPECT(wasIgnored(acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, spammer))); + BEAST_EXPECT(spammer->charges().empty()); + for (int i = 0; i < 5; ++i) + acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, spammer); + BEAST_EXPECT(spammer->charges().size() == 5); + + // honest's own, single late reply is still free. A shared count would have let + // spammer's five replays above exhaust the allowance before honest ever got here. + BEAST_EXPECT(wasIgnored(acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, honest))); + BEAST_EXPECT(honest->charges().empty()); + + acquire->cancel(); + } + + /** + * A revived acquisition's late-reply allowance starts over, rather than + * carrying over the round that just failed. + * + * The allowance is one pass per peer asked, for the round that settled - + * that peer's reply can still be in flight when it does. stillNeed() + * reviving a timed-out acquisition starts a fresh round; a peer that + * already spent its pass in the round before must not have that carry + * over and mischarge the fresh pass the new round owes it. Both rounds + * here are settled with cancel() alone, so no real data has to flow to + * demonstrate it: only lateReplyGranted_'s behavior across the revival is + * under test. + * + * @param env The environment to run in. + */ + void + testLateReplyAllowanceResetsOnRevival(jtx::Env& env) + { + testcase("A revived acquisition's late-reply allowance is not carried over"); + + DeepChain const chain{nextSeed()}; + + // One peer asked, so it is the one whose pass is under test in both rounds. + auto const candidate = std::make_shared(); + auto peerSet = + std::make_unique(std::vector>{candidate}); + + auto const acquire = std::make_shared( + env.app(), chain.rootHash.asUInt256(), std::move(peerSet), kFastRetry); + + acquire->init(1); + + // The first round fails, and candidate's one allowed late reply arrives and is free, + // spending its pass for this round. + acquire->cancel(); + BEAST_EXPECT( + wasIgnored(acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, candidate))); + BEAST_EXPECT(candidate->charges().empty()); + + // Revived: the map is still valid, so stillNeed() clears the failure, restarts the + // timer, and hands out a fresh pass for the round that follows. + BEAST_EXPECT(acquire->stillNeed()); + + // The second round fails too - cancel() alone settles it, so no data has to flow. + acquire->cancel(); + + // candidate's late reply here is the second round's own pass, not a reuse of the first + // round's already-spent one. Without clearing lateReplyGranted_ on revival, this would + // be charged as a repeat instead. + BEAST_EXPECT( + wasIgnored(acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, candidate))); + BEAST_EXPECT(candidate->charges().empty()); + + acquire->cancel(); + } + /** * A reply whose node data cannot be deserialized is charged for. * @@ -394,6 +766,48 @@ struct TransactionAcquire_test : public beast::unit_test::Suite BEAST_EXPECT(peer->charges() == std::vector{resource::kFeeInvalidData}); } + /** + * Each peer is charged for its own packet, not for whatever the map happens + * to look like afterwards. + * + * takeNodes() classifies and charges under one lock hold. Reading the + * tier back after it returns would let a second packet arriving in + * between - up to five run concurrently under JtTxnData - invalidate + * the map and make the first peer pay the fabrication tier for data + * it did not send. + * + * @param env The environment to run in. + */ + void + testChargeIsNotDecidedAfterTheLock(jtx::Env& env) + { + testcase("A peer is charged for its own packet only"); + + DeepChain const chain{nextSeed()}; + + auto const acquire = std::make_shared( + env.app(), chain.rootHash.asUInt256(), std::make_unique()); + + auto const rootPeer = std::make_shared(); + acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, rootPeer); + BEAST_EXPECT(rootPeer->charges().empty()); + + // A misplaced node: wrong, but the map survives, so the generic tier. + auto const wrongPeer = std::make_shared(); + acquire->takeNodes({{SHAMapNodeID{2, uint256{}}, chain.nodeAt(1)}}, wrongPeer); + BEAST_EXPECT(wrongPeer->charges() == std::vector{resource::kFeeInvalidData}); + + // Now a second peer invalidates the map, which must leave the earlier verdicts alone. + auto const fabricatingPeer = std::make_shared(); + acquire->takeNodes(chain.nodesBelowRoot(), fabricatingPeer); + BEAST_EXPECT(fabricatingPeer->charges() == std::vector{resource::kFeeMalformedData}); + BEAST_EXPECT(!acquire->isMapValid()); + + // The earlier peers' charges must be untouched by that. + BEAST_EXPECT(wrongPeer->charges() == std::vector{resource::kFeeInvalidData}); + BEAST_EXPECT(rootPeer->charges().empty()); + } + /** * A batch that ends on a bad node still counts the good nodes ahead of it, * and a batch that achieved nothing records no progress. @@ -442,6 +856,9 @@ struct TransactionAcquire_test : public beast::unit_test::Suite BEAST_EXPECT(acquire->madeProgress()); BEAST_EXPECT(acquire->isMapValid()); + // The bad node is still charged for, at the recoverable tier. + BEAST_EXPECT(peer->charges() == std::vector{resource::kFeeInvalidData}); + // A batch of nothing but the root we already have: counted as a duplicate rather than // reaching the clean exit with nothing counted, so it is neither reported as useful nor // allowed to postpone the timeout. @@ -465,8 +882,8 @@ struct TransactionAcquire_test : public beast::unit_test::Suite BEAST_EXPECT(withDuplicateRoot.isUseful()); BEAST_EXPECT(acquire->madeProgress()); - // takeNodes() charges nobody: InboundTransactions::gotData() reads the verdict and decides. - BEAST_EXPECT(peer->charges().empty()); + // None of that cost the sender anything beyond the one bad node above. + BEAST_EXPECT(peer->charges() == std::vector{resource::kFeeInvalidData}); } /** @@ -565,14 +982,16 @@ struct TransactionAcquire_test : public beast::unit_test::Suite // kNormTimeouts rather than to zero, so the very next tick broadcasts to every peer // already tracked - the only way a peer already selected once is asked again, and so // what shows the timer chain was restarted rather than just the failed flag cleared. - acquire->stillNeed(); + BEAST_EXPECT(acquire->stillNeed()); BEAST_EXPECT(waitFor([&] { return peerSetPtr->requests() > requestsBeforeRevival; })); - // Data is examined again too: the real root is accepted, and asks for the next level. + // Data is examined again too: the real root is accepted and asks for the next level, and + // the sender is not charged for data we are asking for once more. auto const peer = std::make_shared(); auto const revived = acquire->takeNodes({{SHAMapNodeID{}, chain.nodeAt(0)}}, peer); BEAST_EXPECT(!wasIgnored(revived)); BEAST_EXPECT(revived.isUseful()); + BEAST_EXPECT(peer->charges().empty()); // Stop the retry loop, which would otherwise keep asking for as long as this case runs. acquire->cancel(); @@ -617,7 +1036,7 @@ struct TransactionAcquire_test : public beast::unit_test::Suite // Ask again far faster than the interval, the way a short consensus round would. Every ask // clamps the timeout count, so the acquisition cannot give up while this runs. auto const askAgainRepeatedly = [&] { - acquire->stillNeed(); + static_cast(acquire->stillNeed()); std::this_thread::sleep_for(std::chrono::milliseconds{20}); return peerSetPtr->requests() > requestsFromInit; }; @@ -685,10 +1104,18 @@ struct TransactionAcquire_test : public beast::unit_test::Suite testHappyPathCompletesAcquisition(env); testTwoPeersEachSupplyPartOfTheSet(env); + testFabricatedChainFailsAcquire(env); + testWrongNodeKeepsAcquireAlive(env); testBadRootKeepsAcquireAlive(env); + testEmptyReplyIsCharged(env); + testFeeTierDistinguishesFabrication(env); testDuplicateRootReplyIsFree(env); testDuplicateNonRootReplyIsFree(env); + testLateReplyIsFreeOncePerPeerAsked(env); + testOnePeersReplaysDoNotStarveAnother(env); + testLateReplyAllowanceResetsOnRevival(env); testUndeserializableNodeIsCharged(env); + testChargeIsNotDecidedAfterTheLock(env); testPartialBatchIsCounted(env); testInitAsksOnlyPeersWithTheSet(env); testStillNeedLeavesARunningAcquireAlone(env); diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp index be5695e7c3..7f39899742 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp @@ -990,7 +990,26 @@ InboundLedger::receiveNode( { JLOG(journal_.warn()) << "Got invalid node " << *nodeID << " for ledger " << hash_ << " from peer " << peer->id(); - peer->charge(resource::kFeeInvalidData, "ledger_node invalid"); + if (!map.isValid()) + { + // Only a node that leaves the map invalid gets here, which no honest peer + // produces by accident, so charge it more harshly. No other peer can repair the + // map either (see SHAMap::addKnownNode), so fail now rather than time out. The + // charge is a deterrent rather than a control: such a node can reach a map by + // paths with no peer to charge, so nothing may rely on the sender having paid. + peer->charge(resource::kFeeMalformedData, "ledger_node makes map invalid"); + failed_ = true; + done(); + + // Nothing in this packet is worth counting: the nodes ahead of the bad one + // belong to a tree that cannot exist. Matches + // TransactionAcquire::takeNodesLocked(). + san = SHAMapAddNode::invalid(); + } + else + { + peer->charge(resource::kFeeInvalidData, "ledger_node invalid"); + } return; } } @@ -1248,7 +1267,9 @@ InboundLedger::processData(std::shared_ptr peer, protocol::TMLedgerData co // `san` accumulates across the whole packet, so `isInvalid()` (bad_ > 0) does not mean the // packet had no useful nodes: credit whatever good/useful nodes were sent rather than - // discarding everything because one node in an otherwise-good packet was bad. + // discarding everything because one node in an otherwise-good packet was bad. The one + // exception is a node that leaves the map invalid, which receiveNode() does discard + // everything for, since the nodes ahead of it belong to a tree that cannot exist. // Note: Peer charges for invalid/malformed data are issued from within receiveNode at the // exact failure site, so the peer is only charged for problems they are responsible for. if (san.isUseful()) diff --git a/src/xrpld/app/ledger/detail/InboundTransactions.cpp b/src/xrpld/app/ledger/detail/InboundTransactions.cpp index 9936417956..a84f7cd996 100644 --- a/src/xrpld/app/ledger/detail/InboundTransactions.cpp +++ b/src/xrpld/app/ledger/detail/InboundTransactions.cpp @@ -96,11 +96,12 @@ public: { if (acquire) { - it->second.seq = seq_; - if (it->second.acquire) - { - it->second.acquire->stillNeed(); - } + // Refreshed only while there is still something to wait for. An acquisition + // that failed on an invalid map can never be revived, so refreshing it would + // hold a dead entry - a set that stays null forever - out of newRound()'s reach + // for as long as anything keeps asking. + if (!it->second.acquire || it->second.acquire->stillNeed()) + it->second.seq = seq_; } return it->second.set; } @@ -168,18 +169,11 @@ public: data.emplace_back(*nodeID, std::move(treeNode)); } - auto const san = ta->takeNodes(std::move(data), peer); - if (san.isInvalid()) - { - peer->charge(resource::kFeeInvalidData, "ledger_data invalid"); - } - else if (!san.isGood()) - { - // Good rather than useful: the verdict now tells a batch of nodes we already hold from - // one that was never examined, and a duplicate is what an honest second responder to - // trigger()'s fan-out sends. - peer->charge(resource::kFeeUselessData, "ledger_data useless"); - } + // takeNodes() charges peers itself, at the failure site and under the lock that classified + // it: the tier (or no charge, within the late-reply allowance) depends on whether the map + // survived and whether we were still asking for the set. What it accepts is what we asked + // for. + ta->takeNodes(std::move(data), peer); } void diff --git a/src/xrpld/app/ledger/detail/TransactionAcquire.cpp b/src/xrpld/app/ledger/detail/TransactionAcquire.cpp index ee9e9bc968..6dbbc5ceae 100644 --- a/src/xrpld/app/ledger/detail/TransactionAcquire.cpp +++ b/src/xrpld/app/ledger/detail/TransactionAcquire.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -145,6 +146,8 @@ TransactionAcquire::trigger(std::shared_ptr const& peer) tmGL.set_querytype(protocol::qtINDIRECT); *(tmGL.add_nodeids()) = SHAMapNodeID().getRawString(); + if (peer) + requestedPeers_.insert(peer->id()); peerSet_->sendRequest(tmGL, peer); } else if (!map_->isValid()) @@ -183,6 +186,8 @@ TransactionAcquire::trigger(std::shared_ptr const& peer) { *tmGL.add_nodeids() = node.first.getRawString(); } + if (peer) + requestedPeers_.insert(peer->id()); peerSet_->sendRequest(tmGL, peer); } } @@ -215,16 +220,33 @@ TransactionAcquire::takeNodesLocked( std::shared_ptr const& peer, ScopedLockType&) { - if (complete_) + // A reply that arrives after the set is settled - by completing it, or by a different packet + // failing it. trigger() sends to every peer it was given, so any of their replies, including + // another packet from the same peer whose data failed the set, can already be in flight and + // could not have known the outcome. Those are solicited, and free: one per peer we asked, + // which is what bounds the honest case. + // + // Past that bound, further data for this hash is a replay - a resend of data already + // accepted or now known worthless, not a first-time reply - and serving it is not free work. + // InboundTransactions::gotData() deserializes and hashes the whole node list before this + // point, up to kHardMaxReplyNodes of them, and the object lingers until newRound() sweeps it, + // so an unbounded number of replays would otherwise cost only the trivial per-message fee. + if (isDone()) { - JLOG(journal_.trace()) << "TX set complete"; - return SHAMapAddNode(); - } + JLOG(journal_.trace()) << (complete_ ? "TX set complete" : "TX set failed"); - if (failed_) - { - JLOG(journal_.trace()) << "TX set failed"; - return SHAMapAddNode(); + // Free only the first time this specific peer shows up here, not merely the first + // reply of however many arrive: a peer outside requestedPeers_ was never asked at all, + // and one already in lateReplyGranted_ has already spent the one pass requestedPeers_ + // earned it. Keyed by identity rather than counted, so one peer resending its own + // already-accepted reply cannot exhaust the pass a different, genuinely honest peer in + // requestedPeers_ is still owed. + if (!requestedPeers_.contains(peer->id()) || !lateReplyGranted_.insert(peer->id()).second) + peer->charge(resource::kFeeUselessData, "tx_set data after the set was settled"); + + // Reported as a duplicate rather than as bad, unless the map itself is why the set failed: + // no reply for such a hash can ever be useful to anyone. + return map_->isValid() ? SHAMapAddNode::duplicate() : SHAMapAddNode::invalid(); } // Accumulated across the batch, so a packet ending in one bad node still counts the nodes @@ -234,7 +256,11 @@ TransactionAcquire::takeNodesLocked( try { if (data.empty()) + { + // Defensive: PeerImp rejects an empty node list before dispatch. + peer->charge(resource::kFeeInvalidData, "tx_set empty"); return SHAMapAddNode::invalid(); + } ConsensusTransSetSF sf(app_, app_.getTempNodeCache()); @@ -257,6 +283,9 @@ TransactionAcquire::takeNodesLocked( { JLOG(journal_.warn()) << "TX acquire got bad root node for TX set " << hash_ << " from peer " << peer->id(); + // addRootNode only rejects a hash mismatch, which never invalidates the map, + // so there is nothing to fail here: the timer will retry with another peer. + peer->charge(resource::kFeeInvalidData, "tx_set root hash mismatch"); return san; } @@ -271,6 +300,27 @@ TransactionAcquire::takeNodesLocked( { JLOG(journal_.warn()) << "TX acquire got bad non-root node " << d.first << " for TX set " << hash_ << " from peer " << peer->id(); + if (!map_->isValid()) + { + // No peer can complete this hash (see SHAMap::addKnownNode), so fail the + // acquisition rather than retrying; stillNeed() will not revive it either. + // Charged more harshly than data that is merely wrong, and charged here, under + // the lock that reached the verdict, so a concurrent packet cannot decide this + // peer's fee. A deterrent rather than a control even so: such a node can reach + // a map by paths with no peer to charge (see SHAMap::addKnownNode), so nothing + // may rely on the sender having paid. + peer->charge(resource::kFeeMalformedData, "tx_set node makes map invalid"); + failed_ = true; + done(); + + // Nothing in this batch is worth counting: the nodes ahead of the bad one + // belong to a tree that cannot exist, and the acquisition is over. + return SHAMapAddNode::invalid(); + } + + // Any other bad node leaves the map sound, so leave that retry to the timer + // rather than re-requesting from the peer that just sent us bad data. + peer->charge(resource::kFeeInvalidData, "tx_set node invalid"); return san; } } @@ -282,6 +332,7 @@ TransactionAcquire::takeNodesLocked( { JLOG(journal_.error()) << "Peer " << peer->id() << " sent us junky transaction node data: " << ex.what(); + peer->charge(resource::kFeeInvalidData, "tx_set junky node data"); san.incInvalid(); return san; } @@ -306,7 +357,7 @@ TransactionAcquire::init(int numPeers) setTimer(sl); } -void +bool TransactionAcquire::stillNeed() { ScopedLockType sl(mtx_); @@ -316,13 +367,28 @@ TransactionAcquire::stillNeed() // Nothing to revive: leave a running acquisition on the wait it has, rather than restarting it // for every consensus round that asks for the set again. if (!failed_) - return; + return true; + + // An invalid map is not a timeout: no peer can complete such a hash (see + // SHAMap::addKnownNode), so this one stays failed. Reported, so the caller stops holding its + // retention window open for a set nothing can ever finish. + if (!map_->isValid()) + return false; failed_ = false; + // The free allowance in takeNodesLocked() is "one per peer asked", which belongs to the round + // that just failed. Reviving starts a new round of asks, so a stale record of who already + // spent their pass must not carry over and eat into it, mischarging a reply the new round is + // still owed. requestedPeers_ is not reset alongside it: a peer already asked stays one we + // asked, in whichever round its reply arrives, just as peerSet_->getPeerIds() was never reset + // here either. + lateReplyGranted_.clear(); + // Restarting the timer is what resumes the acquisition. expires_after() cancels any pending // wait, so this cannot leave two timer chains running. setTimer(sl); + return true; } } // namespace xrpl diff --git a/src/xrpld/app/ledger/detail/TransactionAcquire.h b/src/xrpld/app/ledger/detail/TransactionAcquire.h index 8e7d879c16..2ff1336480 100644 --- a/src/xrpld/app/ledger/detail/TransactionAcquire.h +++ b/src/xrpld/app/ledger/detail/TransactionAcquire.h @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -54,11 +55,20 @@ public: /** * Add nodes a peer sent us to the set we are acquiring. * + * Charges the peer for data it declines, since the fee depends on + * whether the map stayed sound and on whether we were still asking for + * the set, and only this function holds the lock that decides either. + * A node that leaves the map invalid also fails the acquisition; see + * SHAMap::addKnownNode for why that verdict is final. A reply arriving + * after the set was settled is free once per peer we asked, since that + * many can be in flight; beyond that the sender is replaying. + * * @param data The nodes to add, each with its claimed position. - * @param peer The peer that sent them. - * @return The tally of useful, unwanted, and bad nodes in the batch. Useful and - * bad can both be nonzero, since only the node the batch stops on is - * bad. + * @param peer The peer that sent them, charged here if the data is + * declined. + * @return The tally of useful, unwanted, and bad nodes in the batch. + * Useful and bad can both be nonzero, since only the node the + * batch stops on is bad. */ SHAMapAddNode takeNodes( @@ -69,13 +79,19 @@ public: init(int startPeers); /** - * Resume a timed-out acquisition, or leave a running one alone. + * Resume a timed-out acquisition, or leave it alone. * - * Always clamps the timeout count. An acquisition that failed has its timer - * chain stopped, so this also clears the failed flag and restarts the timer; - * one that is still running already has a timer pending. + * Always clamps the timeout count. An acquisition that failed with + * its map still valid has its timer chain stopped, so this also + * clears the failed flag and restarts the timer. One that failed + * because its map went invalid cannot be satisfied by any peer (see + * SHAMap::addKnownNode), so it stays failed. + * + * @return Whether the set is still worth keeping. False only for one that + * cannot be revived, so the caller stops refreshing the window that + * decides when it is swept. */ - void + [[nodiscard]] bool stillNeed(); protected: @@ -85,17 +101,49 @@ protected: private: bool haveRoot_{false}; + + /** + * The peers a targeted request has actually been sent to. + * + * Broader than peerSet_->getPeerIds(): that only tracks peers addPeers() + * selected itself, but takeNodesLocked() can also trigger() an + * unsolicited sender directly, which sends a targeted request without + * ever going through addPeers(). Recording every peer a request went to, + * however it was chosen, is what bounds the free allowance to peers who + * could actually have a reply in flight. Not reset by stillNeed(): a + * peer already asked stays one we asked, whichever round its reply + * arrives in. + */ + std::set requestedPeers_; + + /** + * Peers in requestedPeers_ that have already spent this round's free + * late reply. + * + * Membership, not a count: a shared counter compared against + * requestedPeers_.size() would let one peer's replayed replies exhaust + * the allowance a different, honest peer in requestedPeers_ is still + * owed, since a count does not know whose slot it is spending. + * Charging is refused only the first time a peer already in + * requestedPeers_ shows up in takeNodesLocked()'s isDone() branch, so + * each peer's pass is its own. Reset by stillNeed() on revival, since a + * fresh round re-broadcasts to every peer in requestedPeers_ and so + * owes each of them a fresh pass. + */ + std::set lateReplyGranted_; + std::unique_ptr peerSet_; /** * Add nodes a peer sent us, on the lock takeNodes() holds. * - * Split out so recording what the batch achieved happens on one exit rather - * than on each of the several this has, including the ones that stop the batch - * early. + * Split out so recording what the batch achieved happens on one exit + * rather than on each of the several this has, including the ones + * that stop the batch early. * * @param data The nodes to add, each with its claimed position. - * @param peer The peer that sent them. + * @param peer The peer that sent them, charged here if the data is + * declined. * @return The tally of useful, unwanted, and bad nodes in the batch. */ SHAMapAddNode