Fix invariant error in fee-sized `VaultWithdraw` #5876

Bronek · 2025-10-10T10:04:19Z

High Level Overview of Change

This fixes an invariant error where the amount withdrawn is equal to the transaction fee.

Context of Change

The invariants for VaultWithdraw enforce that the account receiving the funds must have their balance changed (increased). This won't happen when the amount withdrawn is exactly same as fee. The submitter account balance won't change, because the deduced transaction fee is exactly refilled with the withdrawn amount.

Type of Change

Bug fix (non-breaking change which fixes an issue)

codecov · 2025-10-10T12:21:12Z

Codecov Report

❌ Patch coverage is 88.46154% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.5%. Comparing base (e192ffe) to head (9fa9680).

Files with missing lines	Patch %	Lines
src/xrpld/app/tx/detail/InvariantCheck.cpp	88.5%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #5876   +/-   ##
=======================================
  Coverage     78.5%   78.5%           
=======================================
  Files          817     817           
  Lines        69017   69015    -2     
  Branches      8276    8273    -3     
=======================================
+ Hits         54197   54203    +6     
+ Misses       14820   14812    -8

Files with missing lines	Coverage Δ
src/xrpld/app/tx/detail/InvariantCheck.cpp	`92.8% <88.5%> (+0.1%)`	⬆️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

gregtatcam · 2025-10-15T17:15:34Z

src/xrpld/app/tx/detail/InvariantCheck.cpp


    uint256 const key = (before ? before->key() : after->key());
-    if (sign && balance != zero)
+    if (sign != 0)


Why is the balance check removed?

Because zero balance change, as long as it came from an object actually being updated, contains valid and useful information which the Vault invariants rely on.

The bug here is that the withdrawal amount is exactly equal to the fee, so the transaction does not change its TX submitter XRP balance. If we keep balance != zero condition, we lose the information that the balance was updated but the new XRP amount happened to be the same as the old amount. That information is in turn required by the invariant, and if not provided we have invariant violation withdrawal must change one destination balance

The bug here is that the withdrawal amount is exactly equal to the fee, so the transaction does not change its TX submitter XRP balance.

What happens if the fee is larger than the withdraw amount? The tx submitter's balance would decrease. That's still valid.

I was looking again at accountDeltaAssets (in a couple of places), and noticed that the lambda adds in the amount of the fee paid for tx[sfAccount]. That is going to give the wrong result for a delegated transaction, since the delegate pays the fee.

Fortunately, you only need to check for the presence of sfDelegate to determine who paid the fee. You may also want to factor those two lambdas into one that can be called from the two locations so you don't have to worry about drift.

Moved to single location in f36dd22 however I am not sure whether to keep this check

// Delegated transaction; no need to compensate for fees if (auto const delegate = tx[~sfDelegate]; delegate.has_value() && *delegate != tx[sfAccount]) return ret;

... since this is currently dead code, with no way to test.

... actually I was wrong, this can be tested - see c3b0562 . Although I am not sure how faithfully such a test represents an actual delegated transaction.

gregtatcam · 2025-10-15T17:16:52Z

src/test/app/Vault_test.cpp

+                testcase(prefix + " deposit/withdrawal same as fee");
+                auto const amount = env.current()->fees().base;
+
+                auto tx = vault.deposit(


Shouldn't per PR description all tx fail if the tx amount is the same as the fee?

no, it should not - we want this transaction to succeed because there's nothing inherently invalid about withdrawing an amount which matches exactly the fee.

Got it! I misread the description.

gregtatcam

LGTM 👍

src/test/app/Invariants_test.cpp

ximinez · 2025-10-17T21:48:59Z

src/xrpld/app/tx/detail/InvariantCheck.cpp


    uint256 const key = (before ? before->key() : after->key());
-    if (sign && balance != zero)
+    if (sign != 0)


I was looking again at accountDeltaAssets (in a couple of places), and noticed that the lambda adds in the amount of the fee paid for tx[sfAccount]. That is going to give the wrong result for a delegated transaction, since the delegate pays the fee.

Fortunately, you only need to check for the presence of sfDelegate to determine who paid the fee. You may also want to factor those two lambdas into one that can be called from the two locations so you don't have to worry about drift.

ximinez

Just a couple of small suggestions

src/test/app/Vault_test.cpp

src/xrpld/app/tx/detail/InvariantCheck.cpp

ximinez · 2025-10-23T17:14:25Z

src/test/app/Vault_test.cpp

+        // Special "error" value to recognize in tests if things fail.
+        // Accidentally it is also 2^31 - 1 and a Mersenne prime.
+        static constexpr long fail = 2'147'483'647;


I'm not usually a fan of "magic numbers", but since this is test code, I'm ok with it. If you want to rewrite the lambda to return a std::optional<long>, I wouldn't complain about it, but I know you're eager to get this merged.

ximinez · 2025-10-23T17:17:45Z

src/xrpld/app/tx/detail/InvariantCheck.cpp

+    // Number balanceDelta will capture the difference (delta) between "before"
    // state (zero if created) and "after" state (zero if destroyed), so the
    // invariants can validate that the change in account balances matches the
    // change in vault balances, stored to deltas_ at the end of this function.
-    Number balance{};
+    // This is done even if balanceDelta is zero, but an object was updated.
+    Number balanceDelta{};

-    // By default do not add anything to deltas
+    // Append to deltas if sign is non-zero, i.e. an object of an interesting
+    // type has been updated - even if the update did not change the balance.
    std::int8_t sign = 0;


These comments are contradictory, and the code behavior is to only append if the sign is not zero, so the first one is wrong. If the update did not change the balance, the sign will be zero, and this the value will not be appended.

If the update did not change the balance, the sign will be zero

I think that's incorrect. SAV transactors (as all others) will call view().update(sle) unconditionally, whether or not the balance has changed, and we actually rely on this behaviour. The specifics in this particular PR: withdraw increases the submitter balance by the exact same amount of XRP as the transaction fee, hence submitters sfBalance will not change but the account root object will be still updated. We also handle even more extreme case, when the submitter withdraws less XRP than the transaction fee, hence their balance will decrease (I guess I need to add test for this one).

In the context of this block of code, sign will be set to -1 in case ltACCOUNT_ROOT: but since both before->getFieldAmount(sfBalance) and after->getFieldAmount(sfBalance) yield the same number, balanceDelta will be zero.

I guess I need to change the comment to make it clear that updating an object happens whether or not its balance has changed, and that includes account root objects.

hope the comment in 26b57ef makes this clear.

Bronek added 2 commits October 10, 2025 10:59

Add failing test for fee-sized VaultWithdraw

eb7d446

Fix handling of zero-sized balance change

1e21187

Bronek marked this pull request as ready for review October 10, 2025 11:46

More unit tests

4da7141

Bronek requested review from gregtatcam and ximinez October 10, 2025 12:05

Bronek added 2 commits October 10, 2025 15:19

Merge branch 'develop' into Bronek/Vault_invariants_fix

ea2581c

Merge branch 'develop' into Bronek/Vault_invariants_fix

e3b2233

gregtatcam reviewed Oct 15, 2025

View reviewed changes

gregtatcam approved these changes Oct 15, 2025

View reviewed changes

Bronek requested a review from shawnxie999 October 17, 2025 09:33

a1q123456 self-requested a review October 17, 2025 13:46

ximinez requested changes Oct 17, 2025

View reviewed changes

Bronek added 2 commits October 20, 2025 16:50

Review feedback

f36dd22

Merge branch 'develop' into Bronek/Vault_invariants_fix

91fb3bb

Bronek requested a review from ximinez October 20, 2025 15:53

Bronek added 4 commits October 21, 2025 12:49

Fix typos

6301431

Merge branch 'develop' into Bronek/Vault_invariants_fix

6b98a60

Add unit tests

c3b0562

Add unit tests for delegated vault operations

5549c6b

a1q123456 approved these changes Oct 22, 2025

View reviewed changes

Merge branch 'develop' into Bronek/Vault_invariants_fix

eebd551

ximinez requested changes Oct 22, 2025

View reviewed changes

src/test/app/Vault_test.cpp Outdated Show resolved Hide resolved

src/xrpld/app/tx/detail/InvariantCheck.cpp Show resolved Hide resolved

Bronek added 2 commits October 23, 2025 10:08

Review feedback

328369b

Merge branch 'develop' into Bronek/Vault_invariants_fix

e6d864b

Bronek requested a review from ximinez October 23, 2025 09:19

ximinez requested changes Oct 23, 2025

View reviewed changes

shawnxie999 removed their request for review October 23, 2025 18:14

Review feedback, extend unit test

26b57ef

Bronek requested a review from ximinez October 24, 2025 12:23

Bronek added 2 commits October 24, 2025 13:25

Merge branch 'develop' into Bronek/Vault_invariants_fix

cad0305

Merge branch 'develop' into Bronek/Vault_invariants_fix

9fa9680

Uh oh!

Fix invariant error in fee-sized VaultWithdraw #5876

Are you sure you want to change the base?

Fix invariant error in fee-sized VaultWithdraw #5876

Conversation

Bronek commented Oct 10, 2025

High Level Overview of Change

Context of Change

Type of Change

Uh oh!

codecov bot commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregtatcam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ximinez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bronek Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fix invariant error in fee-sized `VaultWithdraw` #5876

Fix invariant error in fee-sized `VaultWithdraw` #5876

codecov bot commented Oct 10, 2025 •

edited

Loading

Bronek Oct 24, 2025 •

edited

Loading