ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-09-27 15:28:03 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Fail fast on an invalid map, and bound late replies by peer asked

Browse Source 
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.
This commit is contained in:
Bart
2026-08-24 09:40:50 -04:00
parent 530929e0e3
commit c45ff55366
6 changed files with 791 additions and 54 deletions
Download Patch File Download Diff File Expand all files Collapse all files

181
src/test/app/InboundLedger_test.cpp
View File

@@ -15,6 +15,11 @@
#include <xrpl/protocol/LedgerHeader.h>
#include <xrpl/protocol/Serializer.h>
#include <xrpl/protocol/jss.h>
#include <xrpl/resource/Fees.h>
#include <xrpl/shamap/SHAMapNodeID.h>
#include <xrpl/shamap/SHAMapTreeNode.h>
#include <xrpl.pb.h>
#include <chrono>
#include <memory>
@@ -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<protocol::TMLedgerData>
headerPacket(LedgerHeader const& header)
{
auto packet = std::make_shared<protocol::TMLedgerData>();
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<protocol::TMLedgerData>
stateNodePacket(
LedgerHeader const& header,
DeepChain const& chain,
std::vector<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>> 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<TestableInboundLedger>(
env.app(),
header.hash,
header.seq,
InboundLedger::Reason::GENERIC,
stopwatch(),
std::make_unique<RequestCountingPeerSet>());
// The header is accepted on its own terms, so the acquisition now chases this hash.
auto const headerPeer = std::make_shared<ChargeRecordingPeer>();
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<std::pair<SHAMapNodeID, SHAMapTreeNodePtr>> 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<ChargeRecordingPeer>();
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<InboundLedger>(
env.app(),
header.hash,
header.seq,
InboundLedger::Reason::GENERIC,
stopwatch(),
std::make_unique<RequestCountingPeerSet>());
auto const headerPeer = std::make_shared<ChargeRecordingPeer>();
BEAST_EXPECT(acquire->gotData(headerPeer, headerPacket(header)));
acquire->runData();
BEAST_EXPECT(!acquire->isFailed());
auto const rootPeer = std::make_shared<ChargeRecordingPeer>();
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<ChargeRecordingPeer>();
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);

451
src/test/app/TransactionAcquire_test.cpp
View File

@@ -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<RequestCountingPeerSet>();
auto* const peerSetPtr = peerSet.get();
auto const acquire = std::make_shared<TestableTransactionAcquire>(
env.app(), chain.rootHash.asUInt256(), std::move(peerSet));
auto const peer = std::make_shared<ChargeRecordingPeer>();
// 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<TestableTransactionAcquire>(
env.app(), chain.rootHash.asUInt256(), std::make_unique<RequestCountingPeerSet>());
auto const peer = std::make_shared<ChargeRecordingPeer>();
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<TestableTransactionAcquire>(
env.app(), chain.rootHash.asUInt256(), std::make_unique<RequestCountingPeerSet>());
auto const peer = std::make_shared<ChargeRecordingPeer>();
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<ChargeRecordingPeer>();
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<ChargeRecordingPeer>();
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<ChargeRecordingPeer>();
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<ChargeRecordingPeer>();
auto peerSet =
std::make_unique<RequestCountingPeerSet>(std::vector<std::shared_ptr<Peer>>{candidate});
auto* const peerSetPtr = peerSet.get();
auto const acquire = std::make_shared<TransactionAcquire>(
env.app(), chain.rootHash.asUInt256(), std::move(peerSet), kFastRetry);
acquire->init(1);
BEAST_EXPECT(peerSetPtr->addedPeers() == std::set<Peer::id_t>{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<ChargeRecordingPeer>();
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<ChargeRecordingPeer>();
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<ChargeRecordingPeer>();
auto const honest = std::make_shared<ChargeRecordingPeer>();
auto peerSet = std::make_unique<RequestCountingPeerSet>(
std::vector<std::shared_ptr<Peer>>{spammer, honest});
auto const acquire = std::make_shared<TransactionAcquire>(
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<ChargeRecordingPeer>();
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<ChargeRecordingPeer>();
auto peerSet =
std::make_unique<RequestCountingPeerSet>(std::vector<std::shared_ptr<Peer>>{candidate});
auto const acquire = std::make_shared<TransactionAcquire>(
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<TestableTransactionAcquire>(
env.app(), chain.rootHash.asUInt256(), std::make_unique<RequestCountingPeerSet>());
auto const rootPeer = std::make_shared<ChargeRecordingPeer>();
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<ChargeRecordingPeer>();
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<ChargeRecordingPeer>();
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<ChargeRecordingPeer>();
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<void>(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);

25
src/xrpld/app/ledger/detail/InboundLedger.cpp
View File

@@ -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> 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())

28
src/xrpld/app/ledger/detail/InboundTransactions.cpp
View File

@@ -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

86
src/xrpld/app/ledger/detail/TransactionAcquire.cpp
View File

@@ -10,6 +10,7 @@
#include <xrpl/basics/base_uint.h>
#include <xrpl/beast/utility/instrumentation.h>
#include <xrpl/core/Job.h>
#include <xrpl/resource/Fees.h>
#include <xrpl/server/NetworkOPs.h>
#include <xrpl/shamap/SHAMap.h>
#include <xrpl/shamap/SHAMapAddNode.h>
@@ -145,6 +146,8 @@ TransactionAcquire::trigger(std::shared_ptr<Peer> 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<Peer> 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<Peer> 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

74
src/xrpld/app/ledger/detail/TransactionAcquire.h
View File

@@ -14,6 +14,7 @@
#include <chrono>
#include <cstddef>
#include <memory>
#include <set>
#include <utility>
#include <vector>
@@ -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<Peer::id_t> 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<Peer::id_t> lateReplyGranted_;
std::unique_ptr<PeerSet> 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