Skip to content

Fix removeDeletedTrustLines#6627

Open
phamdat721101 wants to merge 1 commit intoXRPLF:developfrom
phamdat721101:develop
Open

Fix removeDeletedTrustLines#6627
phamdat721101 wants to merge 1 commit intoXRPLF:developfrom
phamdat721101:develop

Conversation

@phamdat721101
Copy link
Copy Markdown

High Level Overview of Change

Replace the all-or-nothing size guard in removeDeletedTrustLines with an
iterate-up-to-limit loop, so each AMMDelete transaction cleans up as many
AMM trust lines as the per-transaction cap allows instead of abandoning
cleanup entirely when that cap is exceeded.

Fixes #6614


Context of Change

removeDeletedTrustLines is called at the end of Transactor::operator()
when a transaction returns tecINCOMPLETE. Its job is to finalize deletion
of AMM trust lines that were marked for removal during transaction processing.

The previous implementation contained this guard:

if (trustLines.size() > maxDeletableAMMTrustLines)
{
    JLOG(viewJ.error()) << "removeDeletedTrustLines: deleted trustlines exceed max "
                        << trustLines.size();
    return;
}

When an AMM account held more than 512 trust lines (maxDeletableAMMTrustLines),
this guard caused the function to return immediately without deleting any of
them. The stale trust line objects then accumulated on the ledger indefinitely
with no path to cleanup, because each subsequent AMMDelete attempt hit the
same guard and bailed out the same way.

The fix removes the guard and replaces the plain loop with a counter-based
loop that stops after maxDeletableAMMTrustLines attempts — identical in
structure to removeUnfundedOffers and removeExpiredNFTokenOffers in the
same file. Each AMMDelete transaction now deletes up to 512 trust lines;
when more remain the transaction returns tecINCOMPLETE and the next
invocation continues where the previous one left off, eventually completing
the full cleanup.

API Impact

- Public API: New feature (new methods and/or new fields)
- Public API: Breaking change
- libxrpl change
- Peer protocol change

No API impact. The change is entirely internal to Transactor.cpp. No new
fields, methods, configuration values, or protocol messages are introduced.
Transaction semantics (tecINCOMPLETE for oversized cleanup, eventual
success) remain unchanged — only the number of trust lines processed per
invocation improves.

---
Before / After

Before — if trustLines.size() > 512:
- Function logs an error and returns immediately
- Zero trust lines are deleted
- Stale AMM trust line objects accumulate on the ledger with no cleanup path

After — regardless of trustLines.size():
- Function deletes up to 512 trust lines per invocation
- Returns after the limit is reached, leaving the rest for the next transaction
- AMM accounts with any number of trust lines can be fully cleaned up across
one or more AMMDelete transactions

---
Test Plan

Existing tests in src/test/app/AMM_test.cpp — testAutoDelete() — cover
both boundary cases:

1. Slightly over limit (maxDeletableAMMTrustLines + 10 = 522 trust lines):
first AMMDelete returns tecINCOMPLETE, second call completes the AMM
deletion successfully.
2. Well over limit (maxDeletableAMMTrustLines * 2 + 10 = 1034 trust lines):
two AMMDelete transactions are required to finish cleanup; both complete
without error.

Run from the build directory:

./xrpld --unittest=AMM

All tests must pass.

Copy link
Copy Markdown
Collaborator

@mvadari mvadari left a comment

Choose a reason for hiding this comment

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

This needs an amendment to gate the change, as it would break consensus as-is. It also needs proper tests to a) ensure that 500 trustlines are deleted with this and b) that behavior remains the same before the amendment is enabled.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 24, 2026

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 81.4%. Comparing base (dcfcdab) to head (8095e81).

Files with missing lines Patch % Lines
src/libxrpl/tx/Transactor.cpp 66.7% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #6627     +/-   ##
=========================================
- Coverage     81.5%   81.4%   -0.0%     
=========================================
  Files          997     997             
  Lines        74415   74414      -1     
  Branches      7579    7562     -17     
=========================================
- Hits         60615   60597     -18     
- Misses       13800   13817     +17     
Files with missing lines Coverage Δ
src/libxrpl/tx/Transactor.cpp 93.6% <66.7%> (+0.4%) ⬆️

... and 9 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.

@phamdat721101
Copy link
Copy Markdown
Author

This needs an amendment to gate the change, as it would break consensus as-is. It also needs proper tests to a) ensure that 500 trustlines are deleted with this and b) that behavior remains the same before the amendment is enabled.

thanks for feedbacks, let me to check to solve it

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

removeDeletedTrustLines silently skips ALL cleanup when trust line count exceeds limit

2 participants