fix: Count a duplicate as an answer when timing out a TX-set acquire

[bthomee]

Part 17/17 of a stack. Base: part 16 (bthomee/shamap-invalid-16-late-reply-survives-giveset).

High Level Overview of Change

TransactionAcquire::takeNodes() now counts a reply of nothing but duplicates as an answer, so a
peer that loses a fan-out race no longer looks like a stall, and InboundTransactions::gotData()
asks whether a reply is wanted before it deserializes the node list rather than after.

Context of Change

takeNodes() recorded progress only for a batch that hooked a node in, so a batch of nodes we
already held recorded none. trigger() sends to every peer it is given, so on a fan-out whichever
peer loses the race sends exactly such a batch, and onTimer() then treats a set that is plainly
being answered as stalled - spending retry budget on an acquisition that is making progress. A
duplicate now counts alongside a node hooked in, bounded two ways: only from a peer already in
requestedPeers_, whose reply says something about the peers being waited on, and only while the
acquisition was still running, since a settled one has no timer left to postpone. Both bounds are
read before the inner call, which enrolls the sender on some paths and settles the acquisition on
others.

gotData() also deserialized a whole node list, at a hash apiece, before takeNodes() could tell
it the set was already settled and the result would be discarded. wantsReplyFrom() moves that
decision ahead of the parse, and the late-reply allowance moves with it into chargeLateReply(),
shared with takeNodesLocked() so a reply is charged exactly once whichever site sees it. The
allowance itself is unchanged: one free reply per peer asked, keyed by identity.

TransactionAcquire_test covers the fan-out case, both bounds, and the parse order, which is
observable through the fee tier - unparseable node data charges kFeeInvalidData when parsed, and
nothing or kFeeUselessData when turned away first. One existing assertion in
testPartialBatchIsCounted is inverted, since it pinned the behavior this changes.

API Impact

None of the checkboxes below apply: this is an internal xrpld correctness fix with no libxrpl,
public-API, or peer-protocol surface.

[xrplf-ai-reviewer]

This is a well-scoped, carefully documented change that (1) makes TransactionAcquire::takeNodes() count a duplicate-only reply from an already-requested peer as progress so a fan-out loser isn't mistaken for a stalled acquisition, and (2) moves the late-reply/allowance check (wantsReplyFrom) ahead of node deserialization in InboundTransactions::gotData() so a settled acquisition doesn't pay parsing cost. The bounds (wasSettled/wasAsked) are deliberately snapshotted before the inner call since that call can itself enroll the peer or settle the acquisition, and chargeLateReply() is shared between wantsReplyFrom() and takeNodesLocked() so a given reply is charged exactly once even with the lock being dropped and reacquired between the two call sites. The inverted assertion in testPartialBatchIsCounted correctly reflects the new behavior, and the new tests (fan-out duplicate counts as progress, unasked duplicates don't, late reply turned away before parsing) match the described semantics. I traced the locking, the requestedPeers_/lateReplyGranted_ bookkeeping, and the race window between wantsReplyFrom() and takeNodes() (explicitly acknowledged in the header doc) and did not find a correctness, security, or double-charge bug introduced by this diff.

[Copilot]

Pull request overview

This PR improves TransactionAcquire progress tracking and late-reply handling so that duplicate-only TX-set replies (common in fan-out races) are treated as “answered” for timeout/progress purposes, and so that obviously-late replies are rejected (and optionally charged) before incurring per-node deserialization/hashing cost in InboundTransactions::gotData().

Changes:

• Count duplicate-only replies from requested peers as “answered” to avoid consuming retry budget due to fan-out race losers.
• Add TransactionAcquire::wantsReplyFrom() (and shared chargeLateReply()) so InboundTransactions::gotData() can avoid deserializing node lists for settled acquisitions.
• Extend unit tests to cover fan-out duplicate behavior, bounds (asked + running), and the “reject before parse” ordering (observable via fee tier differences).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
src/xrpld/app/ledger/detail/TransactionAcquire.h	Adds wantsReplyFrom() API and documents shared late-reply charging helper.
src/xrpld/app/ledger/detail/TransactionAcquire.cpp	Implements duplicate-only-as-answer progress logic; factors late-reply charging into chargeLateReply() and adds wantsReplyFrom().
src/xrpld/app/ledger/detail/InboundTransactions.cpp	Calls wantsReplyFrom() before deserializing nodes to avoid wasted work on settled acquisitions.
src/test/app/TransactionAcquire_test.cpp	Adds/updates tests for fan-out duplicates counting as progress and for skipping parse on late replies.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!