Stop tx processing if failed to delete expired credentials - #6715
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## ripple/staging-313 #6715 +/- ##
====================================================
- Coverage 79.5% 79.5% -0.0%
====================================================
Files 840 840
Lines 71890 71902 +12
Branches 8200 8205 +5
====================================================
+ Hits 57128 57135 +7
- Misses 14762 14767 +5
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
628230a to
f85b917
Compare
mvadari
left a comment
mvadari
left a comment
There was a problem hiding this comment.
Some extra stuff included in this diff in the workflows - can we remove those?
ximinez
left a comment
ximinez
left a comment
There was a problem hiding this comment.
One small thing, then I think it's done!
There was a problem hiding this comment.
Pull request overview
This PR ports the develop-branch fix to ensure that failures while deleting expired credentials can be surfaced as transaction errors (instead of silently continuing), and updates the credential-expiration helper API accordingly.
Changes:
- Refactors
credentials::checkExpiredto takeSLE const&and updates call sites to dereferenceSLEpointers before checking expiration. - Updates expired-credential cleanup in
CredentialHelpers.cppto propagatedeleteSLEfailures (behindfixSecurity3_1_3) via anExpected<bool, TER>result. - Adds a unit test that simulates ledger corruption and verifies behavior before/after
fixSecurity3_1_3.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/xrpld/rpc/handlers/DepositAuthorized.cpp | Adjusts expiration checks to match the new checkExpired(SLE const&) signature. |
| src/xrpld/app/tx/detail/Transactor.cpp | Adds logging when reapply-time credential deletion fails (but currently doesn’t propagate failure). |
| src/xrpld/app/tx/detail/Credentials.cpp | Updates expiration checks to dereference SLE and adds a defensive null check in doApply(). |
| src/xrpld/app/misc/PermissionedDEXHelpers.cpp | Updates expiration checks to match the new checkExpired signature. |
| src/test/app/Credentials_test.cpp | Adds regression coverage for delete-failure handling under simulated corruption and fixSecurity3_1_3. |
| src/libxrpl/ledger/CredentialHelpers.cpp | Implements error propagation from expired-credential deletion when fixSecurity3_1_3 is enabled. |
| include/xrpl/ledger/CredentialHelpers.h | Updates public helper signatures (checkExpired) and marks deleteSLE as [[nodiscard]]. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| STVector256 credHashes; | ||
| credHashes.push_back(credKeylet.key); |
There was a problem hiding this comment.
STVector256 credHashes is created and populated but never used, which will trigger -Wunused-variable/-Wunused-but-set-variable warnings in many builds (and may fail under -Werror). Remove it or use it in the subsequent calls.
| STVector256 credHashes; | |
| credHashes.push_back(credKeylet.key); |
|
|
||
| // Check if credential sfExpiration field has passed ledger's parentCloseTime | ||
| bool | ||
| checkExpired( | ||
| std::shared_ptr<SLE const> const& sleCredential, | ||
| NetClock::time_point const& closed); | ||
|
|
||
| // Return true if any expired credential was found in arr (and deleted) | ||
| bool | ||
| removeExpired(ApplyView& view, STVector256 const& arr, beast::Journal const j); | ||
| checkExpired(SLE const& sleCredential, NetClock::time_point const& closed); | ||
|
|
||
| // Actually remove a credentials object from the ledger | ||
| TER | ||
| [[nodiscard]] TER | ||
| deleteSLE( |
There was a problem hiding this comment.
This change removes credentials::removeExpired from a public header and changes checkExpired's parameter from shared_ptr<SLE const> to SLE const&. Both are API/ABI breaking for any external consumers of the installed include/xrpl/* headers; if downstream compatibility matters, consider keeping a deprecated overload/wrapper (or leaving removeExpired declared and forwarding internally) rather than removing/changing the public signatures outright.
| { | ||
| if (auto const ter = credentials::deleteSLE(view, sle, viewJ); | ||
| !isTesSuccess(ter)) | ||
| { | ||
| JLOG(viewJ.error()) << "removeExpiredCredentials: failed to " | ||
| "delete expired credential. Err: " | ||
| << transToken(ter); | ||
| } |
There was a problem hiding this comment.
removeExpiredCredentials logs when credentials::deleteSLE fails but then continues, so the transaction can still complete with tecEXPIRED while leaving the expired credential undeleted. Since deleteSLE failures indicate ledger corruption (e.g. missing owner account / dirRemove failure), this should be treated as a hard failure: propagate the TER back to Transactor::operator() (and discard/stop processing) instead of only logging (optionally gated behind fixSecurity3_1_3).
Co-authored-by: Ed Hennis <ed@ripple.com>
…F#6715) (XRPLF#6962) Co-authored-by: Ed Hennis <ed@ripple.com> Co-authored-by: Ayaz Salikhov <mathbunnyru@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…F#6715) (XRPLF#6962) Co-authored-by: Ed Hennis <ed@ripple.com> Co-authored-by: Ayaz Salikhov <mathbunnyru@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
High Level Overview of Change
Return error code if deleting expired credentials failed
Port to develop PR #6962