fix: Report no ledger from a failed acquisition - #8092
Open
bthomee wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR prevents partially-built ledgers from being observable through InboundLedger::getLedger() after an acquisition has failed, ensuring callers can’t accidentally use an unsound ledger even if they forget to check isFailed().
Changes:
- Update
InboundLedger::getLedger()to returnnullptrwhen the acquisition is marked failed. - Add/extend unit test assertions that failed acquisitions do not hand back a ledger (including cases involving a locally-resolvable tx-chain via fetch pack and a full timeout chain failure).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/xrpld/app/ledger/InboundLedger.h | Guard getLedger() so failed acquisitions never return the (potentially partial) ledger pointer. |
| src/test/app/InboundLedger_test.cpp | Add assertions ensuring getLedger() returns nullptr when acquisitions fail in the covered scenarios. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bthomee
force-pushed
the
bthomee/shamap-invalid-13-no-ledger-on-failure
branch
from
August 23, 2026 23:45
ad4cebe to
4b10ea8
Compare
bthomee
force-pushed
the
bthomee/shamap-invalid-13-no-ledger-on-failure
branch
from
August 24, 2026 14:32
4b10ea8 to
cf629e4
Compare
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
bthomee
force-pushed
the
bthomee/shamap-invalid-13-no-ledger-on-failure
branch
from
August 24, 2026 15:00
cf629e4 to
cf3531e
Compare
bthomee
force-pushed
the
bthomee/shamap-invalid-13-no-ledger-on-failure
branch
from
August 24, 2026 15:24
cf3531e to
0f0ff0d
Compare
bthomee
force-pushed
the
bthomee/shamap-invalid-13-no-ledger-on-failure
branch
2 times, most recently
from
August 25, 2026 19:36
b4a90b0 to
59f8bfd
Compare
bthomee
force-pushed
the
bthomee/shamap-invalid-13-no-ledger-on-failure
branch
from
August 25, 2026 19:48
59f8bfd to
1395a13
Compare
bthomee
force-pushed
the
bthomee/shamap-invalid-13-no-ledger-on-failure
branch
from
August 25, 2026 21:08
1395a13 to
a69203b
Compare
InboundLedger::getLedger() now reports nothing once the acquisition has failed, since a failed acquisition can still hold a partially built ledger that must never be used. The guard sits at the one accessor rather than relying on every caller to check isFailed() first; the pointer itself is kept, since getJson() still reports on the partial maps of a failed acquire.
bthomee
force-pushed
the
bthomee/shamap-invalid-13-no-ledger-on-failure
branch
from
August 25, 2026 21:58
a69203b to
cd79b1b
Compare
bthomee
marked this pull request as ready for review
August 26, 2026 00:21
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 13/16 of a stack. Base: part 12 (
bthomee/shamap-invalid-12-batch-progress).High Level Overview of Change
InboundLedger::getLedger()now returns nothing once the acquisition has failed, so a partially-built(and potentially unsound) ledger can never leak out through the one accessor, without requiring every
caller to check
isFailed()first.Context of Change
InboundLedger::getLedger()reports nothing once the acquisition has failed. A failed acquisition canstill hold a partially built ledger, and that ledger must never be used, so the guard sits at the one
accessor rather than resting on every caller checking
isFailed()first. The pointer itself is kept,since
getJson()reports on the partial maps of a failed acquire, which is what an operator inspectingone needs.
Two existing cases gain the assertion: the one whose transaction map is a chain found in a fetch pack,
and the one that waits out a whole timeout chain. Both already checked the failure itself; what's new is
that nothing is handed back with it.
API Impact
None of the checkboxes below apply: this is an internal
xrpldcorrectness fix with nolibxrpl,public-API, or peer-protocol surface.