diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index 9f70538778d..1e81959ae65 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -20,6 +20,9 @@ #include +#include +#include + namespace xrpl { namespace test { diff --git a/src/test/app/LendingHelpers_test.cpp b/src/test/app/LendingHelpers_test.cpp index 55fffad6b0e..53659c65262 100644 --- a/src/test/app/LendingHelpers_test.cpp +++ b/src/test/app/LendingHelpers_test.cpp @@ -3,16 +3,11 @@ #include #include #include -#include #include -#include #include #include -#include -#include - #include #include diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index e9780211de3..34407c4b94a 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -7641,6 +7641,149 @@ class Loan_test : public beast::unit_test::suite 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 @@ class Loan_test : public beast::unit_test::suite testLoanPayLateFullPaymentBypassesPenalties(); testLoanCoverMinimumRoundingExploit(); #endif + testWithdrawReflectsUnrealizedLoss(); testInvalidLoanSet(); testCoverDepositWithdrawNonTransferableMPT(); diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp index 2e0b3cbfab1..6fb832a05ea 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.cpp +++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp @@ -19,10 +19,11 @@ #include #include #include -#include #include -#include +#include +#include +#include #include 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( - after->getFieldU64(sfOutstandingAmount))); + balanceDelta -= Number( + static_cast( + 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 { 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; - } - - if (beforeVault.assetsAvailable + *vaultDeltaAssets != - afterVault.assetsAvailable) + 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; + } + + 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 diff --git a/src/xrpld/app/tx/detail/InvariantCheck.h b/src/xrpld/app/tx/detail/InvariantCheck.h index 87a1afb623c..83152f74859 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.h +++ b/src/xrpld/app/tx/detail/InvariantCheck.h @@ -9,7 +9,6 @@ #include #include -#include #include #include