fix: Fail fast on an invalid map, and bound late replies by peer asked by bthomee · Pull Request #8093 · XRPLF/rippled

bthomee · 2026-08-23T22:10:19Z

Part 14/16 of a stack. Base: part 13 (bthomee/shamap-invalid-13-no-ledger-on-failure).

High Level Overview of Change

A node that leaves a map invalid proves the hash being chased cannot belong to any valid tree, so
TransactionAcquire and InboundLedger now fail such an acquisition immediately instead of retrying
until the timeout chain runs out. A reply arriving after the set is settled is free once per peer the
acquisition actually asked, tracked by peer identity rather than by a shared count, so replaying an
already-accepted reply cannot exhaust the allowance a different, honest peer is still owed.

Context of Change

SHAMap::addKnownNode() reports invalid() for a node that leaves the map invalid (see part 5), which
proves the root hash being chased commits to a shape no valid tree can have — no peer can ever complete
it. TransactionAcquire::takeNodesLocked() and InboundLedger::receiveNode() now fail the acquisition
at that point rather than retrying, discarding the whole batch: the nodes hooked in ahead of the bad one
belong to a tree that cannot exist. TransactionAcquire::stillNeed() refuses to revive such an
acquisition, and InboundTransactions::getSet() refreshes its retention window only while it is still
worth keeping, 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 directly to an unsolicited sender, not only to peers addPeers()
selected, so requestedPeers_ tracks every peer sent a request either way — bounding by
peerSet_->getPeerIds() alone would let an honest peer that only ever received a targeted request be
charged for a reply we ourselves asked for. The allowance itself is keyed by peer identity
(lateReplyGranted_), not a shared count: a plain counter compared against the number of peers asked
would let one peer's replayed replies exhaust the allowance a different, genuinely honest peer is still
owed, since a count alone cannot tell whose slot it is spending. Past the allowance, a reply is a replay
and costs kFeeUselessData — InboundTransactions::gotData() deserializes and hashes the whole node
list before this point, so an unbounded number of replays would otherwise cost only the trivial
per-message fee.

Covered by the TransactionAcquire and InboundLedger gtest/boost suites: a chain reaching
kLeafDepth fails the acquisition outright, is not revived by stillNeed(), and rejects later replies;
a merely-wrong node or root leaves the acquisition able to try another peer; the fee-tier split is driven
through the real InboundTransactions::gotData() dispatch; a late reply is free once for the peer that
earned it and charged on a second attempt from the same peer or from a peer never asked at all; one
peer's replayed replies are shown not to spend a different, honestly-asked peer's own allowance; and a
revived acquisition's allowance is shown not to carry over a spent pass from the round that just failed.

API Impact

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

Copilot

Pull request overview

This PR stops transaction and ledger acquisitions from retrying structurally invalid maps, refines peer charging, and adds regression coverage.

Changes:

Terminates unrecoverable acquisitions.
Adds differentiated fee handling and late-reply allowances.
Expands transaction and ledger acquisition tests.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Review summary
`src/xrpld/app/ledger/detail/TransactionAcquire.h`	Updates acquisition state and documentation.
`src/xrpld/app/ledger/detail/TransactionAcquire.cpp`	Handles invalid maps and charging; targeted solicited peers may be excluded from late-reply allowances.
`src/xrpld/app/ledger/detail/InboundTransactions.cpp`	Adjusts dispatch and retention; post-completion replies can bypass the late-reply allowance.
`src/xrpld/app/ledger/detail/InboundLedger.cpp`	Handles invalid state-map failures; independently detect maps already invalidated by another walk.
`src/test/app/TransactionAcquire_test.cpp`	Adds transaction acquisition regression coverage.
`src/test/app/InboundLedger_test.cpp`	Adds ledger acquisition regression coverage.

Suppressed comments (1)

src/xrpld/app/ledger/detail/TransactionAcquire.cpp:233

For normal network traffic, this late-reply branch is bypassed after a successful acquisition is handed off: done() posts giveSet(), which clears InboundTransactionSet::acquire; a TMLedgerData that arrives after that is charged by InboundTransactions::gotData() as useless before it can reach takeNodes(). Thus the advertised free allowance is only effective until the asynchronous handoff, and an honest fan-out reply that arrives just afterward is still charged. Preserve the allowance in the container while late replies can arrive, or apply it before clearing the acquisition.

    if (isDone())
    {
        JLOG(journal_.trace()) << (complete_ ? "TX set complete" : "TX set failed");

        if (++lateReplies_ > peerSet_->getPeerIds().size())
            peer->charge(resource::kFeeUselessData, "tx_set data after the set was settled");

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

Copilot

Pull request overview

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

trigger() walks a ledger's state map with mtx_ released (the AS-node getMissingNodes() call), so it can invalidate the map while a different packet is still in flight in receiveNode(). SHAMap::addKnownNode() then reports every node in that packet a duplicate rather than invalid, so isSynching() alone cannot tell "this packet just finished the map" apart from "some other packet already broke it" - the acquisition can end up reporting itself satisfied on a map that is actually invalid, retrying a hash no peer can ever complete instead of failing fast. Check map.isValid() independently right after the locked receive, before isSynching() can misread the verdict, and fail without charging the packet that lost the race - it did nothing wrong. Addresses Copilot review feedback on PR #8093, split out into its own branch: this bug predates the stack and is not made worse by it, so there is no urgency tying it to that PR's release.

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.

trigger() walks a ledger's state map with mtx_ released (the AS-node getMissingNodes() call), so it can invalidate the map while a different packet is still in flight in receiveNode(). SHAMap::addKnownNode() then reports every node in that packet a duplicate rather than invalid, so isSynching() alone cannot tell "this packet just finished the map" apart from "some other packet already broke it" - the acquisition can end up reporting itself satisfied on a map that is actually invalid, retrying a hash no peer can ever complete instead of failing fast. Check map.isValid() independently right after the locked receive, before isSynching() can misread the verdict, and fail without charging the packet that lost the race - it did nothing wrong. Addresses Copilot review feedback on PR #8093, split out into its own branch: this bug predates the stack and is not made worse by it, so there is no urgency tying it to that PR's release.

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.

trigger() walks a ledger's state map with mtx_ released (the AS-node getMissingNodes() call), so it can invalidate the map while a different packet is still in flight in receiveNode(). SHAMap::addKnownNode() then reports every node in that packet a duplicate rather than invalid, so isSynching() alone cannot tell "this packet just finished the map" apart from "some other packet already broke it" - the acquisition can end up reporting itself satisfied on a map that is actually invalid, retrying a hash no peer can ever complete instead of failing fast. Check map.isValid() independently right after the locked receive, before isSynching() can misread the verdict, and fail without charging the packet that lost the race - it did nothing wrong. Addresses Copilot review feedback on PR #8093, split out into its own branch: this bug predates the stack and is not made worse by it, so there is no urgency tying it to that PR's release.

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.

trigger() walks a ledger's state map with mtx_ released (the AS-node getMissingNodes() call), so it can invalidate the map while a different packet is still in flight in receiveNode(). SHAMap::addKnownNode() then reports every node in that packet a duplicate rather than invalid, so isSynching() alone cannot tell "this packet just finished the map" apart from "some other packet already broke it" - the acquisition can end up reporting itself satisfied on a map that is actually invalid, retrying a hash no peer can ever complete instead of failing fast. Check map.isValid() independently right after the locked receive, before isSynching() can misread the verdict, and fail without charging the packet that lost the race - it did nothing wrong. Addresses Copilot review feedback on PR #8093, split out into its own branch: this bug predates the stack and is not made worse by it, so there is no urgency tying it to that PR's release.

github-actions · 2026-08-25T19:36:35Z

This PR has conflicts, please resolve them in order for the PR to be reviewed.

github-actions · 2026-08-25T19:36:36Z

This PR has conflicts, please resolve them in order for the PR to be reviewed.

github-actions · 2026-08-25T19:36:38Z

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

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.

trigger() walks a ledger's state map with mtx_ released (the AS-node getMissingNodes() call), so it can invalidate the map while a different packet is still in flight in receiveNode(). SHAMap::addKnownNode() then reports every node in that packet a duplicate rather than invalid, so isSynching() alone cannot tell "this packet just finished the map" apart from "some other packet already broke it" - the acquisition can end up reporting itself satisfied on a map that is actually invalid, retrying a hash no peer can ever complete instead of failing fast. Check map.isValid() independently right after the locked receive, before isSynching() can misread the verdict, and fail without charging the packet that lost the race - it did nothing wrong. Addresses Copilot review feedback on PR #8093, split out into its own branch: this bug predates the stack and is not made worse by it, so there is no urgency tying it to that PR's release.

github-actions · 2026-08-25T19:48:34Z

This PR has conflicts, please resolve them in order for the PR to be reviewed.

github-actions · 2026-08-25T19:48:35Z

This PR has conflicts, please resolve them in order for the PR to be reviewed.

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.

github-actions · 2026-08-25T19:48:41Z

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

trigger() walks a ledger's state map with mtx_ released (the AS-node getMissingNodes() call), so it can invalidate the map while a different packet is still in flight in receiveNode(). SHAMap::addKnownNode() then reports every node in that packet a duplicate rather than invalid, so isSynching() alone cannot tell "this packet just finished the map" apart from "some other packet already broke it" - the acquisition can end up reporting itself satisfied on a map that is actually invalid, retrying a hash no peer can ever complete instead of failing fast. Check map.isValid() independently right after the locked receive, before isSynching() can misread the verdict, and fail without charging the packet that lost the race - it did nothing wrong. Addresses Copilot review feedback on PR #8093, split out into its own branch: this bug predates the stack and is not made worse by it, so there is no urgency tying it to that PR's release.

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.

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.

trigger() walks a ledger's state map with mtx_ released (the AS-node getMissingNodes() call), so it can invalidate the map while a different packet is still in flight in receiveNode(). SHAMap::addKnownNode() then reports every node in that packet a duplicate rather than invalid, so isSynching() alone cannot tell "this packet just finished the map" apart from "some other packet already broke it" - the acquisition can end up reporting itself satisfied on a map that is actually invalid, retrying a hash no peer can ever complete instead of failing fast. Check map.isValid() independently right after the locked receive, before isSynching() can misread the verdict, and fail without charging the packet that lost the race - it did nothing wrong. Addresses Copilot review feedback on PR #8093, split out into its own branch: this bug predates the stack and is not made worse by it, so there is no urgency tying it to that PR's release.

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.

codecov · 2026-08-26T00:23:13Z

Codecov Report

❌ Patch coverage is 90.62500% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...rc/xrpld/app/ledger/detail/InboundTransactions.cpp	33.3%	2 Missing ⚠️
src/xrpld/app/ledger/detail/TransactionAcquire.cpp	95.7%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

bthomee requested a lite review from Copilot August 23, 2026 22:11

Copilot started reviewing on behalf of bthomee August 23, 2026 22:11 View session

Copilot AI reviewed Aug 23, 2026

View reviewed changes

Comment thread src/xrpld/app/ledger/detail/InboundLedger.cpp

Comment thread src/xrpld/app/ledger/detail/InboundTransactions.cpp

Comment thread src/xrpld/app/ledger/detail/TransactionAcquire.cpp

bthomee force-pushed the bthomee/shamap-invalid-14-fail-unsatisfiable branch from bf26872 to 0ec627b Compare August 23, 2026 23:45

bthomee force-pushed the bthomee/shamap-invalid-14-fail-unsatisfiable branch from 0ec627b to 2aa13f9 Compare August 24, 2026 14:32

bthomee changed the title ~~fix: Stop retrying an acquisition whose hash cannot be satisfied~~ fix: Fail fast on an invalid map, and bound late replies by peer asked Aug 24, 2026

bthomee requested a lite review from Copilot August 24, 2026 14:40

Copilot started reviewing on behalf of bthomee August 24, 2026 14:41 View session

Copilot AI reviewed Aug 24, 2026

View reviewed changes

bthomee force-pushed the bthomee/shamap-invalid-14-fail-unsatisfiable branch from 2aa13f9 to 7fae7a9 Compare August 24, 2026 15:00

bthomee force-pushed the bthomee/shamap-invalid-14-fail-unsatisfiable branch from 7fae7a9 to edeb11f Compare August 24, 2026 15:24

bthomee force-pushed the bthomee/shamap-invalid-14-fail-unsatisfiable branch from edeb11f to f64a56b Compare August 25, 2026 15:02

bthomee force-pushed the bthomee/shamap-invalid-14-fail-unsatisfiable branch from f64a56b to 693f0bc Compare August 25, 2026 19:36

github-actions Bot added the PR: has conflicts label Aug 25, 2026

github-actions Bot removed the PR: has conflicts label Aug 25, 2026

github-actions Bot added PR: has conflicts labels Aug 25, 2026

bthomee force-pushed the bthomee/shamap-invalid-14-fail-unsatisfiable branch from 693f0bc to 9a11f79 Compare August 25, 2026 19:48

github-actions Bot removed the PR: has conflicts label Aug 25, 2026

bthomee force-pushed the bthomee/shamap-invalid-14-fail-unsatisfiable branch from 9a11f79 to ac13a7b Compare August 25, 2026 21:08

bthomee force-pushed the bthomee/shamap-invalid-14-fail-unsatisfiable branch from ac13a7b to 242c6e2 Compare August 25, 2026 21:58

bthomee marked this pull request as ready for review August 26, 2026 00:21

Navigation Menu

Uh oh!

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

fix: Fail fast on an invalid map, and bound late replies by peer asked#8093
bthomee wants to merge 1 commit into
bthomee/shamap-invalid-13-no-ledger-on-failurefrom
bthomee/shamap-invalid-14-fail-unsatisfiable

bthomee commented Aug 23, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

github-actions Bot commented Aug 25, 2026

Uh oh!

github-actions Bot commented Aug 25, 2026

Uh oh!

github-actions Bot commented Aug 25, 2026

Uh oh!

github-actions Bot commented Aug 25, 2026

Uh oh!

github-actions Bot commented Aug 25, 2026

Uh oh!

github-actions Bot commented Aug 25, 2026

Uh oh!

codecov Bot commented Aug 26, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

bthomee commented Aug 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High Level Overview of Change

Context of Change

API Impact

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

github-actions Bot commented Aug 25, 2026

Uh oh!

github-actions Bot commented Aug 25, 2026

Uh oh!

github-actions Bot commented Aug 25, 2026

Uh oh!

github-actions Bot commented Aug 25, 2026

Uh oh!

github-actions Bot commented Aug 25, 2026

Uh oh!

github-actions Bot commented Aug 25, 2026

Uh oh!

codecov Bot commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bthomee commented Aug 23, 2026 •

edited

Loading

codecov Bot commented Aug 26, 2026 •

edited

Loading