fix: Keep a settled acquisition registered so late replies stay bounded

giveSet() resets the map entry's acquire pointer unconditionally once a
set arrives, so any reply for that hash arriving after completion takes
gotData()'s ta == nullptr branch - charged outright, since
takeNodesLocked() is never reached to apply the late-reply allowance. The
tests for that allowance call acquire->takeNodes() directly, bypassing
gotData()/giveSet() entirely, so none of them exercise this: the real
production entry point for peer replies never reaches the allowance at
all.

Only reset the entry's acquire pointer when something other than the
acquisition itself supplied the set: a set arriving some other way still
cancels an acquisition genuinely in flight, but the acquisition completing
on its own is not that. Keeping it alive until newRound() sweeps the entry
lets a late reply for this hash still reach getAcquire() and, through it,
takeNodesLocked()'s allowance.

Addresses Copilot review feedback on PR #8093, split out into its own
branch: unlike the late-reply allowance bound in branch 14, this gap is
unchanged pre-existing behavior, not something this stack makes worse, so
there is no urgency tying it to that PR's release.
This commit is contained in:
Bart
2026-08-24 09:42:31 -04:00
parent 7fe694a814
commit 117a896a05
2 changed files with 62 additions and 1 deletions

View File

@@ -263,6 +263,59 @@ struct TransactionAcquire_test : public beast::unit_test::Suite
BEAST_EXPECT(delivered->getHash() == chain.rootHash);
}
/**
* The late-reply allowance must survive giveSet(), through the real
* dispatch rather than by calling takeNodes() directly.
*
* giveSet() only resets the acquisition when something else supplied
* the set (see the fromAcquire guard), so a reply arriving after
* completion still reaches getAcquire() and takeNodesLocked()'s
* allowance instead of gotData()'s unconditional ta == nullptr charge.
* Driven end to end through InboundTransactions::gotData(), including
* the async job done() hands off to, since every other late-reply test
* in this suite calls takeNodes() directly and so never exercises that
* path.
*
* @param env The environment to run in.
*/
void
testLateReplyAllowanceSurvivesGiveSet(jtx::Env& env)
{
testcase("The late-reply allowance survives giveSet()");
auto const chain = DeepChain::toLeaf(3, nextSeed());
auto& inbound = env.app().getInboundTransactions();
uint256 const setHash = chain.rootHash.asUInt256();
BEAST_EXPECT(inbound.getSet(setHash, true) == nullptr);
// One peer supplies the whole chain, so it alone earns the one targeted follow-up
// request the root reply buys, and so the allowance's one slot.
auto const rootPeer = std::make_shared<ChargeRecordingPeer>();
inbound.gotData(setHash, rootPeer, packetFor(chain, {{SHAMapNodeID{}, chain.nodeAt(0)}}));
BEAST_EXPECT(rootPeer->charges().empty());
inbound.gotData(setHash, rootPeer, packetFor(chain, chain.nodesBelowRoot()));
BEAST_EXPECT(rootPeer->charges().empty());
// Proves the acquisition completed and handed off through the real done()/giveSet()
// path, rather than this case racing takeNodes() directly the way the rest of the
// suite does.
auto const delivered = waitForDeliveredSet(env, setHash);
BEAST_EXPECT(delivered != nullptr);
// Without keeping the acquisition registered past giveSet(), gotData() would take the
// ta == nullptr branch here and charge this outright - the allowance in
// takeNodesLocked() would never get a chance to run at all. rootPeer's own late reply
// is the one whose slot this is, and is free.
inbound.gotData(setHash, rootPeer, packetFor(chain, {{SHAMapNodeID{}, chain.nodeAt(0)}}));
BEAST_EXPECT(rootPeer->charges().empty());
// A second reply from rootPeer has already spent that slot: this one is a replay.
inbound.gotData(setHash, rootPeer, packetFor(chain, {{SHAMapNodeID{}, chain.nodeAt(0)}}));
BEAST_EXPECT(rootPeer->charges() == std::vector{resource::kFeeUselessData});
}
/**
* A chain reaching kLeafDepth must end the acquisition outright.
*
@@ -1104,6 +1157,7 @@ struct TransactionAcquire_test : public beast::unit_test::Suite
testHappyPathCompletesAcquisition(env);
testTwoPeersEachSupplyPartOfTheSet(env);
testLateReplyAllowanceSurvivesGiveSet(env);
testFabricatedChainFailsAcquire(env);
testWrongNodeKeepsAcquireAlive(env);
testBadRootKeepsAcquireAlive(env);

View File

@@ -197,7 +197,14 @@ public:
inboundSet.set = set;
}
inboundSet.acquire.reset();
// Only reset when something other than the acquisition itself gave us the set:
// dropping it here cancels an acquisition still in flight, which is correct when a
// set arrived some other way, but not when this call IS that acquisition completing.
// Keeping it alive until newRound() sweeps the entry is what lets a late reply for
// this hash still reach getAcquire() above, and so takeNodesLocked()'s "one free per
// peer asked" allowance, rather than the ta == nullptr branch charging it outright.
if (!fromAcquire)
inboundSet.acquire.reset();
}
if (isNew)