Sponsored Content
Skip to content

fix: Skip deleted book directories and non-root modifications in ValidBookDirectory invariant - #7312

Merged
bthomee merged 7 commits into
XRPLF:developfrom
PeterChen13579:skip_deleted_book_directory
May 24, 2026
Merged

fix: Skip deleted book directories and non-root modifications in ValidBookDirectory invariant#7312
bthomee merged 7 commits into
XRPLF:developfrom
PeterChen13579:skip_deleted_book_directory

Conversation

@PeterChen13579

@PeterChen13579 PeterChen13579 commented May 21, 2026

Copy link
Copy Markdown
Contributor

High Level Overview of Change

Update ValidBookDirectory to validate newly-created directory nodes and modified
directory nodes only when sfRootIndex changes. This keeps legacy bad
exchange-rate metadata from causing false positives during ordinary
modifications, while still checking root-link changes.

Context of Change

API Impact

  • Public API: New feature (new methods and/or new fields)
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

@PeterChen13579 PeterChen13579 changed the title Skip deleted book directory fix: Skip deleted book directory May 21, 2026
@PeterChen13579
PeterChen13579 requested review from ximinez and yinyiqian1 and removed request for yinyiqian1 May 21, 2026 18:50
@codecov

codecov Bot commented May 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.3%. Comparing base (30de556) to head (da9d9c4).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #7312   +/-   ##
=======================================
  Coverage     82.3%   82.3%           
=======================================
  Files         1011    1011           
  Lines        76303   76305    +2     
  Branches      7314    7312    -2     
=======================================
+ Hits         62827   62836    +9     
+ Misses       13476   13469    -7     
Files with missing lines Coverage Δ
src/libxrpl/tx/invariants/DirectoryInvariant.cpp 100.0% <100.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.

@PeterChen13579 PeterChen13579 added this to the 3.2.0 milestone May 21, 2026
Comment thread src/test/app/Invariants_test.cpp Outdated

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

This PR updates the ValidBookDirectory invariant to ignore deleted directory entries, preventing legacy/bad book-root directories that are being removed from being misclassified as newly-created bad state. It also adds a regression test covering the deletion callback scenario where the deleted SLE image may still be provided to the invariant.

Changes:

  • Update ValidBookDirectory::visitEntry to early-return when isDelete is true.
  • Add a unit/regression test ensuring deleting a legacy bad root directory does not trip the invariant.

Reviewed changes

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

File Description
src/libxrpl/tx/invariants/DirectoryInvariant.cpp Skips invariant checks for deleted entries via the isDelete marker to avoid false positives.
src/test/app/Invariants_test.cpp Adds regression coverage for deletion of a legacy bad root book directory where the deleted SLE image is still passed to visitEntry.

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

@ximinez ximinez 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.

🐒 🔧

Comment thread src/libxrpl/tx/invariants/DirectoryInvariant.cpp Outdated
Comment thread src/test/app/Invariants_test.cpp Outdated

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.

Comment thread src/test/app/Invariants_test.cpp Outdated
@PeterChen13579

Copy link
Copy Markdown
Contributor Author

/ai-review

@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

Comment thread src/libxrpl/tx/invariants/DirectoryInvariant.cpp Outdated
Comment thread src/test/app/Invariants_test.cpp Outdated
Comment thread src/libxrpl/tx/invariants/DirectoryInvariant.cpp

@ximinez ximinez 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.

My issues are addressed.

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 no new comments.

@PeterChen13579 PeterChen13579 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 23, 2026
@bthomee bthomee changed the title fix: Skip deleted book directory fix: Skip deleted book directories and non-root modifications in ValidBookDirectory invariant May 24, 2026
@bthomee
bthomee added this pull request to the merge queue May 24, 2026
Merged via the queue into XRPLF:develop with commit e34c266 May 24, 2026
3 checks passed
Kassaking7 pushed a commit to Kassaking7/rippled that referenced this pull request Jun 2, 2026
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.

5 participants