Merge branch 'develop' into tapanito/lending-vault-invariant

src/test/app/Invariants_test.cpp

@@ -20,6 +20,9 @@
 
 #include <boost/algorithm/string/predicate.hpp>
 
+#include <initializer_list>
+#include <string>
+
 namespace xrpl {
 namespace test {
 

src/test/app/LendingHelpers_test.cpp

@@ -3,16 +3,11 @@
 #include <test/jtx.h>
 #include <test/jtx/Account.h>
 #include <test/jtx/amount.h>
-#include <test/jtx/mpt.h>
 
 #include <xrpld/app/misc/LendingHelpers.h>
-#include <xrpld/app/misc/LoadFeeTrack.h>
 #include <xrpld/app/tx/detail/Batch.h>
 #include <xrpld/app/tx/detail/LoanSet.h>
 
-#include <xrpl/beast/xor_shift_engine.h>
-#include <xrpl/protocol/SField.h>
-
 #include <string>
 #include <vector>
 

src/test/app/Loan_test.cpp

@@ -7641,6 +7641,149 @@ protected:
         BEAST_EXPECT(afterSecondCoverAvailable == 0);
     }
 
+    // Tests that vault withdrawals work correctly when the vault has unrealized
+    // loss from an impaired loan, ensuring the invariant check properly
+    // accounts for the loss.
+    void
+    testWithdrawReflectsUnrealizedLoss()
+    {
+        using namespace jtx;
+        using namespace loan;
+        using namespace std::chrono_literals;
+
+        testcase("Vault withdraw reflects sfLossUnrealized");
+
+        // Test constants
+        static constexpr std::int64_t INITIAL_FUNDING = 1'000'000;
+        static constexpr std::int64_t LENDER_INITIAL_IOU = 5'000'000;
+        static constexpr std::int64_t DEPOSITOR_INITIAL_IOU = 1'000'000;
+        static constexpr std::int64_t BORROWER_INITIAL_IOU = 100'000;
+        static constexpr std::int64_t DEPOSIT_AMOUNT = 5'000;
+        static constexpr std::int64_t PRINCIPAL_AMOUNT = 99;
+        static constexpr std::uint64_t EXPECTED_SHARES_PER_DEPOSITOR =
+            5'000'000'000;
+        static constexpr std::uint32_t PAYMENT_INTERVAL = 600;
+        static constexpr std::uint32_t PAYMENT_TOTAL = 2;
+
+        Env env(*this, all);
+
+        // Setup accounts
+        Account const issuer{"issuer"};
+        Account const lender{"lender"};
+        Account const depositorA{"lpA"};
+        Account const depositorB{"lpB"};
+        Account const borrower{"borrowerA"};
+
+        env.fund(
+            XRP(INITIAL_FUNDING),
+            issuer,
+            lender,
+            depositorA,
+            depositorB,
+            borrower);
+        env.close();
+
+        // Setup trust lines
+        PrettyAsset const iouAsset = issuer[iouCurrency];
+        env(trust(lender, iouAsset(10'000'000)));
+        env(trust(depositorA, iouAsset(10'000'000)));
+        env(trust(depositorB, iouAsset(10'000'000)));
+        env(trust(borrower, iouAsset(10'000'000)));
+        env.close();
+
+        // Fund accounts with IOUs
+        env(pay(issuer, lender, iouAsset(LENDER_INITIAL_IOU)));
+        env(pay(issuer, depositorA, iouAsset(DEPOSITOR_INITIAL_IOU)));
+        env(pay(issuer, depositorB, iouAsset(DEPOSITOR_INITIAL_IOU)));
+        env(pay(issuer, borrower, iouAsset(BORROWER_INITIAL_IOU)));
+        env.close();
+
+        // Create vault and broker, then add deposits from two depositors
+        auto const broker = createVaultAndBroker(env, iouAsset, lender);
+        Vault v{env};
+
+        env(v.deposit({
+                .depositor = depositorA,
+                .id = broker.vaultKeylet().key,
+                .amount = iouAsset(DEPOSIT_AMOUNT),
+            }),
+            ter(tesSUCCESS));
+        env(v.deposit({
+                .depositor = depositorB,
+                .id = broker.vaultKeylet().key,
+                .amount = iouAsset(DEPOSIT_AMOUNT),
+            }),
+            ter(tesSUCCESS));
+        env.close();
+
+        // Create a loan
+        auto const sleBroker = env.le(keylet::loanbroker(broker.brokerID));
+        if (!BEAST_EXPECT(sleBroker))
+            return;
+
+        auto const loanKeylet =
+            keylet::loan(broker.brokerID, sleBroker->at(sfLoanSequence));
+
+        env(set(borrower, broker.brokerID, PRINCIPAL_AMOUNT),
+            sig(sfCounterpartySignature, lender),
+            paymentTotal(PAYMENT_TOTAL),
+            paymentInterval(PAYMENT_INTERVAL),
+            fee(env.current()->fees().base * 2),
+            ter(tesSUCCESS));
+        env.close();
+
+        // Impair the loan to create unrealized loss
+        env(manage(lender, loanKeylet.key, tfLoanImpair), ter(tesSUCCESS));
+        env.close();
+
+        // Verify unrealized loss is recorded in the vault
+        auto const vaultAfterImpair = env.le(broker.vaultKeylet());
+        if (!BEAST_EXPECT(vaultAfterImpair))
+            return;
+
+        BEAST_EXPECT(
+            vaultAfterImpair->at(sfLossUnrealized) ==
+            broker.asset(PRINCIPAL_AMOUNT).value());
+
+        // Helper to get share balance for a depositor
+        auto const shareAsset = vaultAfterImpair->at(sfShareMPTID);
+        auto const getShareBalance =
+            [&](Account const& depositor) -> std::uint64_t {
+            auto const token =
+                env.le(keylet::mptoken(shareAsset, depositor.id()));
+            return token ? token->getFieldU64(sfMPTAmount) : 0;
+        };
+
+        // Verify both depositors have equal shares
+        auto const sharesLpA = getShareBalance(depositorA);
+        auto const sharesLpB = getShareBalance(depositorB);
+        BEAST_EXPECT(sharesLpA == EXPECTED_SHARES_PER_DEPOSITOR);
+        BEAST_EXPECT(sharesLpB == EXPECTED_SHARES_PER_DEPOSITOR);
+        BEAST_EXPECT(sharesLpA == sharesLpB);
+
+        // Helper to attempt withdrawal
+        auto const attemptWithdrawShares = [&](Account const& depositor,
+                                               std::uint64_t shareAmount,
+                                               TER expected) {
+            STAmount const shareAmt{MPTIssue{shareAsset}, Number(shareAmount)};
+            env(v.withdraw(
+                    {.depositor = depositor,
+                     .id = broker.vaultKeylet().key,
+                     .amount = shareAmt}),
+                ter(expected));
+            env.close();
+        };
+
+        // Regression test: Both depositors should successfully withdraw despite
+        // unrealized loss. Previously failed with invariant violation:
+        // "withdrawal must change vault and destination balance by equal
+        // amount". This was caused by sharesToAssetsWithdraw rounding down,
+        // creating a mismatch where vaultDeltaAssets * -1 != destinationDelta
+        // when unrealized loss exists.
+        attemptWithdrawShares(depositorA, sharesLpA, tesSUCCESS);
+        attemptWithdrawShares(depositorB, sharesLpB, tesSUCCESS);
+    }
+
 public:
     void
     run() override
@@ -7649,6 +7792,7 @@ public:
         testLoanPayLateFullPaymentBypassesPenalties();
         testLoanCoverMinimumRoundingExploit();
 #endif
+        testWithdrawReflectsUnrealizedLoss();
         testInvalidLoanSet();
 
         testCoverDepositWithdrawNonTransferableMPT();

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

@@ -19,10 +19,11 @@
 #include <xrpl/protocol/SystemParameters.h>
 #include <xrpl/protocol/TER.h>
 #include <xrpl/protocol/TxFormats.h>
-#include <xrpl/protocol/Units.h>
 #include <xrpl/protocol/nftPageMask.h>
 
-#include <cstdint>
+#include <algorithm>
+#include <cstddef>
+#include <initializer_list>
 #include <optional>
 
 namespace xrpl {
@@ -2707,8 +2708,9 @@ ValidVault::visitEntry(
                 // At this moment we have no way of telling if this object holds
                 // vault shares or something else. Save it for finalize.
                 afterMPTs_.push_back(Shares::make(*after));
-                balanceDelta -= Number(static_cast<std::int64_t>(
-                    after->getFieldU64(sfOutstandingAmount)));
+                balanceDelta -= Number(
+                    static_cast<std::int64_t>(
+                        after->getFieldU64(sfOutstandingAmount)));
                 sign = 1;
                 break;
             case ltMPTOKEN:
@@ -3072,6 +3074,15 @@ ValidVault::finalize(
         return vault.assetsAvailable == 0 && vault.assetsTotal == 0;
     };
 
+    auto const withinDistance = [&](Number const& num1,
+                                    Number const& num2) -> bool {
+        if (vaultAsset.integral())
+            return num1 == num2;
+        // The exponent was set experimentally, based on whether
+        // Loan_test::testWithdrawReflectsUnrealizedLoss unit test fail
+        return withinRelativeDistance(num1, num2, Number{1, -13});
+    };
+
     // Technically this does not need to be a lambda, but it's more
     // convenient thanks to early "return false"; the not-so-nice
     // alternatives are several layers of nested if/else or more complex
@@ -3196,16 +3207,17 @@ ValidVault::finalize(
                     "xrpl::ValidVault::finalize : deposit updated a vault");
                 auto const& beforeVault = beforeVault_[0];
 
-                auto const vaultDeltaAssets = deltaAssets(afterVault.pseudoId);
-
-                if (!vaultDeltaAssets)
+                auto const maybeVaultDeltaAssets =
+                    deltaAssets(afterVault.pseudoId);
+                if (!maybeVaultDeltaAssets)
                 {
                     JLOG(j.fatal()) <<  //
                         "Invariant failed: deposit must change vault balance";
                     return false;  // That's all we can do
                 }
 
-                if (*vaultDeltaAssets > tx[sfAmount])
+                auto const vaultDeltaAssets = *maybeVaultDeltaAssets;
+                if (vaultDeltaAssets > tx[sfAmount])
                 {
                     JLOG(j.fatal()) <<  //
                         "Invariant failed: deposit must not change vault "
@@ -3213,7 +3225,7 @@ ValidVault::finalize(
                     result = false;
                 }
 
-                if (*vaultDeltaAssets <= zero)
+                if (vaultDeltaAssets <= zero)
                 {
                     JLOG(j.fatal()) <<  //
                         "Invariant failed: deposit must increase vault balance";
@@ -3230,16 +3242,17 @@ ValidVault::finalize(
 
                 if (!issuerDeposit)
                 {
-                    auto const accountDeltaAssets = deltaAssetsTxAccount();
-                    if (!accountDeltaAssets)
+                    auto const maybeAccDeltaAssets = deltaAssetsTxAccount();
+                    if (!maybeAccDeltaAssets)
                     {
                         JLOG(j.fatal()) <<  //
                             "Invariant failed: deposit must change depositor "
                             "balance";
                         return false;
                     }
+                    auto const accountDeltaAssets = *maybeAccDeltaAssets;
 
-                    if (*accountDeltaAssets >= zero)
+                    if (accountDeltaAssets >= zero)
                     {
                         JLOG(j.fatal()) <<  //
                             "Invariant failed: deposit must decrease depositor "
@@ -3247,7 +3260,8 @@ ValidVault::finalize(
                         result = false;
                     }
 
-                    if (*accountDeltaAssets * -1 != *vaultDeltaAssets)
+                    if (!withinDistance(
+                            vaultDeltaAssets * -1, accountDeltaAssets))
                     {
                         JLOG(j.fatal()) <<  //
                             "Invariant failed: deposit must change vault and "
@@ -3265,16 +3279,17 @@ ValidVault::finalize(
                     result = false;
                 }
 
-                auto const accountDeltaShares = deltaShares(tx[sfAccount]);
-                if (!accountDeltaShares)
+                auto const maybeAccDeltaShares = deltaShares(tx[sfAccount]);
+                if (!maybeAccDeltaShares)
                 {
                     JLOG(j.fatal()) <<  //
                         "Invariant failed: deposit must change depositor "
                         "shares";
                     return false;  // That's all we can do
                 }
-
-                if (*accountDeltaShares <= zero)
+                // We don't need to round shares, they are integral MPT
+                auto const& accountDeltaShares = *maybeAccDeltaShares;
+                if (accountDeltaShares <= zero)
                 {
                     JLOG(j.fatal()) <<  //
                         "Invariant failed: deposit must increase depositor "
@@ -3282,15 +3297,18 @@ ValidVault::finalize(
                     result = false;
                 }
 
-                auto const vaultDeltaShares = deltaShares(afterVault.pseudoId);
-                if (!vaultDeltaShares || *vaultDeltaShares == zero)
+                auto const maybeVaultDeltaShares =
+                    deltaShares(afterVault.pseudoId);
+                if (!maybeVaultDeltaShares || *maybeVaultDeltaShares == zero)
                 {
                     JLOG(j.fatal()) <<  //
                         "Invariant failed: deposit must change vault shares";
                     return false;  // That's all we can do
                 }
 
-                if (*vaultDeltaShares * -1 != *accountDeltaShares)
+                // We don't need to round shares, they are integral MPT
+                auto const& vaultDeltaShares = *maybeVaultDeltaShares;
+                if (vaultDeltaShares * -1 != accountDeltaShares)
                 {
                     JLOG(j.fatal()) <<  //
                         "Invariant failed: deposit must change depositor and "
@@ -3298,15 +3316,18 @@ ValidVault::finalize(
                     result = false;
                 }
 
-                if (beforeVault.assetsTotal + *vaultDeltaAssets !=
-                    afterVault.assetsTotal)
+                auto const assetTotalDelta =
+                    afterVault.assetsTotal - beforeVault.assetsTotal;
+                if (!withinDistance(assetTotalDelta, vaultDeltaAssets))
                 {
                     JLOG(j.fatal()) << "Invariant failed: deposit and assets "
                                        "outstanding must add up";
                     result = false;
                 }
-                if (beforeVault.assetsAvailable + *vaultDeltaAssets !=
-                    afterVault.assetsAvailable)
+
+                auto const assetAvailableDelta =
+                    afterVault.assetsAvailable - beforeVault.assetsAvailable;
+                if (!withinDistance(assetAvailableDelta, vaultDeltaAssets))
                 {
                     JLOG(j.fatal()) << "Invariant failed: deposit and assets "
                                        "available must add up";
@@ -3324,22 +3345,24 @@ ValidVault::finalize(
                     "vault");
                 auto const& beforeVault = beforeVault_[0];
 
-                auto const vaultDeltaAssets = deltaAssets(afterVault.pseudoId);
+                auto const maybeVaultDeltaAssets =
+                    deltaAssets(afterVault.pseudoId);
 
-                if (!vaultDeltaAssets)
+                if (!maybeVaultDeltaAssets)
                 {
                     JLOG(j.fatal()) << "Invariant failed: withdrawal must "
                                        "change vault balance";
                     return false;  // That's all we can do
                 }
 
-                if (*vaultDeltaAssets >= zero)
+                auto const vaultPseudoDeltaAssets = *maybeVaultDeltaAssets;
+
+                if (vaultPseudoDeltaAssets >= zero)
                 {
                     JLOG(j.fatal()) << "Invariant failed: withdrawal must "
                                        "decrease vault balance";
                     result = false;
                 }
-
                 // Any payments (including withdrawal) going to the issuer
                 // do not change their balance, but destroy funds instead.
                 bool const issuerWithdrawal = [&]() -> bool {
@@ -3352,8 +3375,8 @@ ValidVault::finalize(
 
                 if (!issuerWithdrawal)
                 {
-                    auto const accountDeltaAssets = deltaAssetsTxAccount();
-                    auto const otherAccountDelta =
+                    auto const maybeAccDelta = deltaAssetsTxAccount();
+                    auto const maybeOtherAccDelta =
                         [&]() -> std::optional<Number> {
                         if (auto const destination = tx[~sfDestination];
                             destination && *destination != tx[sfAccount])
@@ -3361,8 +3384,8 @@ ValidVault::finalize(
                         return std::nullopt;
                     }();
 
-                    if (accountDeltaAssets.has_value() ==
-                        otherAccountDelta.has_value())
+                    if (maybeAccDelta.has_value() ==
+                        maybeOtherAccDelta.has_value())
                     {
                         JLOG(j.fatal()) <<  //
                             "Invariant failed: withdrawal must change one "
@@ -3371,8 +3394,7 @@ ValidVault::finalize(
                     }
 
                     auto const destinationDelta =  //
-                        accountDeltaAssets ? *accountDeltaAssets
-                                           : *otherAccountDelta;
+                        maybeAccDelta ? *maybeAccDelta : *maybeOtherAccDelta;
 
                     if (destinationDelta <= zero)
                     {
@@ -3382,7 +3404,8 @@ ValidVault::finalize(
                         result = false;
                     }
 
-                    if (*vaultDeltaAssets * -1 != destinationDelta)
+                    if (!withinDistance(
+                            vaultPseudoDeltaAssets * -1, destinationDelta))
                     {
                         JLOG(j.fatal()) <<  //
                             "Invariant failed: withdrawal must change vault "
@@ -3390,7 +3413,7 @@ ValidVault::finalize(
                         result = false;
                     }
                 }
-
+                // We don't need to round shares, they are integral MPT
                 auto const accountDeltaShares = deltaShares(tx[sfAccount]);
                 if (!accountDeltaShares)
                 {
@@ -3407,7 +3430,7 @@ ValidVault::finalize(
                         "shares";
                     result = false;
                 }
-
+                // We don't need to round shares, they are integral MPT
                 auto const vaultDeltaShares = deltaShares(afterVault.pseudoId);
                 if (!vaultDeltaShares || *vaultDeltaShares == zero)
                 {
@@ -3424,17 +3447,20 @@ ValidVault::finalize(
                     result = false;
                 }
 
+                auto const assetTotalDelta =
+                    afterVault.assetsTotal - beforeVault.assetsTotal;
                 // Note, vaultBalance is negative (see check above)
-                if (beforeVault.assetsTotal + *vaultDeltaAssets !=
-                    afterVault.assetsTotal)
+                if (!withinDistance(assetTotalDelta, vaultPseudoDeltaAssets))
                 {
                     JLOG(j.fatal()) << "Invariant failed: withdrawal and "
                                        "assets outstanding must add up";
                     result = false;
                 }
 
-                if (beforeVault.assetsAvailable + *vaultDeltaAssets !=
-                    afterVault.assetsAvailable)
+                auto const assetAvailableDelta =
+                    afterVault.assetsAvailable - beforeVault.assetsAvailable;
+                if (!withinDistance(
+                        assetAvailableDelta, vaultPseudoDeltaAssets))
                 {
                     JLOG(j.fatal()) << "Invariant failed: withdrawal and "
                                        "assets available must add up";
@@ -3468,10 +3494,12 @@ ValidVault::finalize(
                     }
                 }
 
-                auto const vaultDeltaAssets = deltaAssets(afterVault.pseudoId);
-                if (vaultDeltaAssets)
+                auto const maybeVaultDeltaAssets =
+                    deltaAssets(afterVault.pseudoId);
+                if (maybeVaultDeltaAssets)
                 {
-                    if (*vaultDeltaAssets >= zero)
+                    auto const vaultDeltaAssets = *maybeVaultDeltaAssets;
+                    if (vaultDeltaAssets >= zero)
                     {
                         JLOG(j.fatal()) <<  //
                             "Invariant failed: clawback must decrease vault "
@@ -3479,17 +3507,22 @@ ValidVault::finalize(
                         result = false;
                     }
 
-                    if (beforeVault.assetsTotal + *vaultDeltaAssets !=
-                        afterVault.assetsTotal)
-                    {
-                        JLOG(j.fatal()) <<  //
-                            "Invariant failed: clawback and assets outstanding "
-                            "must add up";
-                        result = false;
-                    }
+                    auto const assetsTotalDelta =
+                        afterVault.assetsTotal - beforeVault.assetsTotal;
+                    if (assetsTotalDelta != vaultDeltaAssets)
+                        if (!withinDistance(assetsTotalDelta, vaultDeltaAssets))
+                        {
+                            JLOG(j.fatal()) <<  //
+                                "Invariant failed: clawback and assets "
+                                "outstanding "
+                                "must add up";
+                            result = false;
+                        }
 
-                    if (beforeVault.assetsAvailable + *vaultDeltaAssets !=
-                        afterVault.assetsAvailable)
+                    auto const assetAvailableDelta =
+                        afterVault.assetsAvailable -
+                        beforeVault.assetsAvailable;
+                    if (!withinDistance(assetAvailableDelta, vaultDeltaAssets))
                     {
                         JLOG(j.fatal()) <<  //
                             "Invariant failed: clawback and assets available "
@@ -3504,15 +3537,15 @@ ValidVault::finalize(
                     return false;  // That's all we can do
                 }
 
-                auto const accountDeltaShares = deltaShares(tx[sfHolder]);
-                if (!accountDeltaShares)
+                // We don't need to round shares, they are integral MPT
+                auto const maybeAccountDeltaShares = deltaShares(tx[sfHolder]);
+                if (!maybeAccountDeltaShares)
                 {
                     JLOG(j.fatal()) <<  //
                         "Invariant failed: clawback must change holder shares";
                     return false;  // That's all we can do
                 }
-
-                if (*accountDeltaShares >= zero)
+                if (*maybeAccountDeltaShares >= zero)
                 {
                     JLOG(j.fatal()) <<  //
                         "Invariant failed: clawback must decrease holder "
@@ -3520,6 +3553,7 @@ ValidVault::finalize(
                     result = false;
                 }
 
+                // We don't need to round shares, they are integral MPT
                 auto const vaultDeltaShares = deltaShares(afterVault.pseudoId);
                 if (!vaultDeltaShares || *vaultDeltaShares == zero)
                 {
@@ -3528,7 +3562,7 @@ ValidVault::finalize(
                     return false;  // That's all we can do
                 }
 
-                if (*vaultDeltaShares * -1 != *accountDeltaShares)
+                if (*vaultDeltaShares * -1 != *maybeAccountDeltaShares)
                 {
                     JLOG(j.fatal()) <<  //
                         "Invariant failed: clawback must change holder and "
@@ -3565,5 +3599,4 @@ ValidVault::finalize(
 
     return true;
 }
-
 }  // namespace xrpl

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

@@ -9,7 +9,6 @@
 #include <xrpl/protocol/STTx.h>
 #include <xrpl/protocol/TER.h>
 
-#include <cstdint>
 #include <tuple>
 #include <unordered_set>