Sponsored Content
Skip to content

refactor: Revert certain Throws by LogicErrors - #7036

Merged
bthomee merged 4 commits into
developfrom
bthomee/logic
May 1, 2026
Merged

refactor: Revert certain Throws by LogicErrors#7036
bthomee merged 4 commits into
developfrom
bthomee/logic

Conversation

@bthomee

@bthomee bthomee commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

High Level Overview of Change

This change reverts a few Throw instances to LogicError.

Context of Change

In #6540 a number of LogicError, which are [[noreturn]] noexcept that call std::abort(), were replaced with Throw<std::logic_error>(). While this works as expected in transactor code paths (protected by try-catch), the same replacement was applied to Ledger::rawErase, Ledger::rawInsert, Ledger::rawReplace, and Ledger::rawTxInsert, which are called from unprotected code paths.

The above notwithstanding, this change is purely cosmetic, for the purpose of making the code "better" even though functionally there is no difference. Using LogicError guarantees a clean std::abort() with no partial state; although using Throw would leave a local ledger in a partially modified state, since the local ledger is never written to disk it would not corrupt the ledger state.

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues.

Review by Claude Opus 4.6 · Prompt: V15

@bthomee
bthomee requested a review from vvysokikh1 April 28, 2026 00:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Reverts several low-level Ledger mutation invariants from Throw<std::logic_error> back to LogicError to ensure these failure paths abort cleanly (instead of throwing) when invoked from unprotected call stacks.

Changes:

  • Replace Throw<std::logic_error> with LogicError in Ledger::rawErase (both overloads).
  • Replace Throw<std::logic_error> with LogicError in Ledger::rawInsert, Ledger::rawReplace, and Ledger::rawTxInsert.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/libxrpl/ledger/Ledger.cpp
@codecov

codecov Bot commented Apr 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.1%. Comparing base (46b997b) to head (04249ae).
⚠️ Report is 10 commits behind head on develop.

Files with missing lines Patch % Lines
src/libxrpl/ledger/Ledger.cpp 0.0% 5 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #7036   +/-   ##
=======================================
  Coverage     82.1%   82.1%           
=======================================
  Files         1010    1010           
  Lines        75961   75951   -10     
  Branches      7392    7378   -14     
=======================================
- Hits         62330   62322    -8     
+ Misses       13631   13629    -2     
Files with missing lines Coverage Δ
include/xrpl/ledger/Ledger.h 100.0% <ø> (ø)
src/libxrpl/ledger/Ledger.cpp 84.1% <0.0%> (+2.0%) ⬆️

... and 5 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues.

Review by Claude Opus 4.6 · Prompt: V15

Copilot AI review requested due to automatic review settings April 28, 2026 17:39

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues.

Review by Claude Opus 4.6 · Prompt: V15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread include/xrpl/ledger/Ledger.h

@vvysokikh1 vvysokikh1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@bthomee bthomee added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label May 1, 2026
@bthomee
bthomee added this pull request to the merge queue May 1, 2026
Merged via the queue into develop with commit 182d844 May 1, 2026
2 of 3 checks passed
@bthomee
bthomee deleted the bthomee/logic branch May 1, 2026 10:21
marek-foss-neti pushed a commit to marek-foss-neti/rippled that referenced this pull request May 5, 2026
Co-authored-by: Bart <11445373+bthomee@users.noreply.github.com>
@mvadari mvadari added this to the 3.2.0 milestone May 20, 2026
beartec-jpg pushed a commit to beartec-jpg/FalconLedger that referenced this pull request Jun 1, 2026
Co-authored-by: Bart <11445373+bthomee@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants