Skip to content

DRAFT: Track the last rotation of online delete more accurately#5542

Draft
ximinez wants to merge 86 commits into
ximinez/online-delete-gapsfrom
ximinez/online-delete-lastrotated
Draft

DRAFT: Track the last rotation of online delete more accurately#5542
ximinez wants to merge 86 commits into
ximinez/online-delete-gapsfrom
ximinez/online-delete-lastrotated

Conversation

@ximinez

@ximinez ximinez commented Jul 9, 2025

Copy link
Copy Markdown
Contributor

WIP. Follow-up to #5531.

High Level Overview of Change

Context of Change

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Performance (increase or change in throughput and/or latency)
  • Tests (you added tests for code that already exists, or your new feature included in this PR)
  • Documentation update
  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • Release

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)

This PR may resolve #4883 and #3912.

@ximinez ximinez added the DraftRunCI Normally CI does not run on draft PRs. This opts in. label Jul 9, 2025
@ximinez
ximinez force-pushed the ximinez/online-delete-lastrotated branch 2 times, most recently from 3f9ebce to a6f8d4e Compare July 9, 2025 01:11
@codecov

codecov Bot commented Jul 9, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (ximinez/online-delete-gaps@f456a85). Learn more about missing BASE report.

Additional details and impacted files

Impacted file tree graph

@@                     Coverage Diff                      @@
##             ximinez/online-delete-gaps   #5542   +/-   ##
============================================================
  Coverage                              ?   78.9%           
============================================================
  Files                                 ?     816           
  Lines                                 ?   72109           
  Branches                              ?    8404           
============================================================
  Hits                                  ?   56909           
  Misses                                ?   15200           
  Partials                              ?       0           
Files with missing lines Coverage Δ
src/xrpld/app/misc/SHAMapStoreImp.cpp 80.7% <100.0%> (ø)

Impacted file tree graph

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

@ximinez
ximinez force-pushed the ximinez/online-delete-lastrotated branch 4 times, most recently from 325bad2 to 97289e6 Compare July 10, 2025 02:27
@ximinez

ximinez commented Jul 10, 2025

Copy link
Copy Markdown
Contributor Author

TODO:

  1. Add a function to DatabaseRotating that is like fetchNodeData, but also includes the "depth" that the node is found at.
  2. At the end of the SHAMapStoreImp::run() rotation process, start with the current validated ledger index, then use the existing cond_ to check after every following validated ledger index
  3. Look up the hash using LedgerMaster.
  4. Find the ledger hash in the node store. It should always be found, but when it's found in the writable DB, then we're done. Details TBD, especially since I'm not sure if the header is written first or last.
  5. Once it's found an determined that all the modified nodes for that ledger are written to the writable DB, set lastRotated to that value.

Edit It's better to have lastRotated set too high and have extra data in the DB than it would be to have lastRotated be set too low and have data from that ledger missing from the DB.

Edit 2026-07-08 Another option to consider: After copying nodes from the archive to the writable (visitNodes) before updating lastRotated, If the validated ledger index is not the same as it was when the rotation started (validatedSeq), update validatedSeq, and try again. Probably implement as another loop inside if (readyToRotate). This causes rotation to chase a moving target. The first attempt will copy over the bulk of the needed nodes, but will take a long time. Thus it's likely that the validated ledger will have moved on. The second attempt will only copy the nodes changed between the first attempt and the current validated ledger, a much smaller set. This has a chance of finishing before the validated ledger moves on. If not, try again. The change set will be even smaller. And so on. The biggest risk is that the rotation never finishes in time. That can be handled with some variation of the process described above - specifically, after some number of attempts, only perform healthWait checks after some number of nodes are copied, instead of read. See also: #7763 for a description of the issue with transient objects causing inner nodes to go back to an old state.

@ximinez
ximinez force-pushed the ximinez/online-delete-lastrotated branch 5 times, most recently from a74b9a1 to 4494768 Compare July 16, 2025 16:57
@ximinez
ximinez force-pushed the ximinez/online-delete-lastrotated branch 5 times, most recently from 9dabe53 to b30c980 Compare July 24, 2025 22:24
@ximinez
ximinez force-pushed the ximinez/online-delete-lastrotated branch 4 times, most recently from 4ad11cf to 93385e3 Compare August 4, 2025 17:07
@ximinez
ximinez force-pushed the ximinez/online-delete-lastrotated branch 3 times, most recently from dfd54f7 to 467170e Compare August 8, 2025 22:27
@ximinez
ximinez force-pushed the ximinez/online-delete-lastrotated branch 3 times, most recently from 8f34db8 to a521ddc Compare August 21, 2025 15:59
@github-actions

github-actions Bot commented May 5, 2026

Copy link
Copy Markdown

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

ximinez added 2 commits May 5, 2026 19:17
…ximinez/online-delete-lastrotated

* XRPLF/ximinez/online-delete-gaps:
  fix: Fix regressions in `server_definitions` (7008)
  chore: Do not duplicate sanitizer flags (7058)
  ci: Run pre-commit on diff in clang-tidy workflow (7078)
  ci: Use XRPLF/create-issue (7076)
  ci: Rewrite clang-tidy workflow(s) in a reusable manner (7062)
  chore: Ignore identifier-naming update in git blame (7066)
  refactor: Enable clang-tidy `readability-identifier-naming` check (6571)
@github-actions

github-actions Bot commented May 5, 2026

Copy link
Copy Markdown

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

@github-actions

Copy link
Copy Markdown

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

…ximinez/online-delete-lastrotated

* XRPLF/ximinez/online-delete-gaps:
  ci: Only run reusable package in public repos (7293)
  fix: Set default peering port to 2459 (6848)
  fix: Use account ledger entry when canceling token escrows (6171)
  refactor: Rename `account_` to `accountID_` (7284)
  ci: Add Linux package builds (DEB + RPM) to CI (6639)
  refactor: Clean up comments post-clang-tidy changes (7283)
  release: Set version to 3.3.0-b0 (7280)
  refactor: Rename static constants (7120)
  refactor: Use `isFlag` where possible instead of bitwise math (7278)
  ci: Update XRPLF/actions (7281)
@github-actions

Copy link
Copy Markdown

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

@github-actions

Copy link
Copy Markdown

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

…ximinez/online-delete-lastrotated

* XRPLF/ximinez/online-delete-gaps: (135 commits)
  refactor: Use dispatch instead of post (7438)
  Correct a couple of comments
  refactor: Clean up tec object deletion logic (6588)
  fix: Move AMMInvariant weakInvariantCheck logic into the transaction (7032)
  fix: Improve ValidAMM invariant (7295)
  feat: Remove clear mutable flags for DynamicMPT XLS-94 (7439)
  ci: Build and push docker images in forks too (7588)
  ci: [DEPENDABOT] bump actions/checkout from 6.0.3 to 7.0.0 (7585)
  fix: Ensure xrpld service directories exist at startup (7565)
  fix: Use template for granular delegation permissions (6613)
  docs: Fix some comments to improve readability (7405)
  ci: Disable assertions on Release builds (7443)
  build: Add graphviz to Nix images (7566)
  refactor: Rerevert "Explicitly trim the heap after cache sweeps (6022)"
  release: Bump version to 3.3.0-b0
  ci: Make clang-tidy workflow adjustments to stay in sync with Clio (7563)
  build: Add git-lfs to Nix images (7561)
  clang-tidy: nodiscard rendezvous, headers, const correctness, {}s
  build: Add zip to Nix images (7551)
  docs: Rewrite build environment docs (7533)
  ...
@github-actions

Copy link
Copy Markdown

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

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

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

…ximinez/online-delete-lastrotated

* XRPLF/ximinez/online-delete-gaps: (48 commits)
  AI review feedback: variable initialization, comments, headers
  Apply suggestions from code review: fix typos
  chore: Enable modernize-use-auto (7707)
  build: Add protobuf dependencies to Nix (7706)
  feat: Enable ConfidentialTransfer and BatchV1_1 (7698)
  chore: Enable modernize-unary-static-assert (7705)
  chore: Make clang-tidy happy on macOS (7701)
  chore: Improve pre-commit hooks (7702)
  refactor: Retire DisallowIncomingV1 fix (7364)
  build: Add verify-headers target to cleanup headers (7670)
  Reorganize healthWait to avoid deadlock and duplicated code
  refactor: Retire InnerObjTemplate fix (7368)
  fix: Disable AMM creation with Vault shares (7666)
  test: Add tests for TMProofPathResponse and TMReplayDeltaResponse invalid hash/key sizes (7593)
  ci: [DEPENDABOT] bump actions/setup-python from 6.2.0 to 6.3.0 (7657)
  build: Don't reuse binaries between different C++ versions (7681)
  chore: Update pre-commit hooks && actions (7686)
  feat: Add an invariant to ensure object deletion also deletes its pseudo-account (7445)
  feat: Add Batch (XLS-56) V1_1 (6446)
  feat: Introduce lending 1.1 amendment and add `MemoData` field to `VaultDelete` transaction (6324)
  ...
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

JiraRefresh Manually trigger Jira automation to run for this item.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant