diff --git a/src/libxrpl/tx/transactors/lending/LoanAccept.cpp b/src/libxrpl/tx/transactors/lending/LoanAccept.cpp index 74298c12c7..81fdfd6bc1 100644 --- a/src/libxrpl/tx/transactors/lending/LoanAccept.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanAccept.cpp @@ -80,8 +80,10 @@ LoanAccept::preclaim(PreclaimContext const& ctx) auto const brokerSle = ctx.view.read(keylet::loanBroker(loanSle->at(sfLoanBrokerID))); if (!brokerSle) { + // LCOV_EXCL_START JLOG(ctx.j.fatal()) << "LoanAccept: LoanBroker does not exist."; - return tefBAD_LEDGER; // LCOV_EXCL_LINE + return tefBAD_LEDGER; + // LCOV_EXCL_STOP } auto const brokerOwner = brokerSle->at(sfOwner); auto const brokerPseudo = brokerSle->at(sfAccount); @@ -89,8 +91,10 @@ LoanAccept::preclaim(PreclaimContext const& ctx) auto const vaultSle = ctx.view.read(keylet::vault(brokerSle->at(sfVaultID))); if (!vaultSle) { + // LCOV_EXCL_START JLOG(ctx.j.fatal()) << "LoanAccept: Vault does not exist."; - return tefBAD_LEDGER; // LCOV_EXCL_LINE + return tefBAD_LEDGER; + // LCOV_EXCL_STOP } Asset const asset = vaultSle->at(sfAsset); auto const vaultPseudo = vaultSle->at(sfAccount); @@ -210,7 +214,7 @@ LoanAccept::doApply() // 3.9.4.8 Make the borrower the owner of the loan. if (auto const ter = dirLink(view, borrower, loanSle, sfOwnerNode)) - return ter; + return ter; // LCOV_EXCL_LINE view.update(loanSle); associateAsset(*loanSle, vaultAsset); diff --git a/src/libxrpl/tx/transactors/lending/LoanSet.cpp b/src/libxrpl/tx/transactors/lending/LoanSet.cpp index 14a72fb843..a0399a118a 100644 --- a/src/libxrpl/tx/transactors/lending/LoanSet.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanSet.cpp @@ -477,7 +477,7 @@ applyPendingLoan( // Link the loan into the broker's directory. The borrower directory link is // deferred to LoanAccept for the two-step (pending) flow. if (auto const ter = dirLink(view, brokerPseudo, loan, sfLoanBrokerNode)) - return ter; + return ter; // LCOV_EXCL_LINE associateAsset(*vaultSle, vaultAsset); associateAsset(*brokerSle, vaultAsset); @@ -583,10 +583,10 @@ applyImmediateLoan( // Link the loan into the broker's directory, then make the borrower the // owner of the loan by linking it into the borrower's directory. if (auto const ter = dirLink(view, brokerPseudo, loan, sfLoanBrokerNode)) - return ter; + return ter; // LCOV_EXCL_LINE if (auto const ter = dirLink(view, plan.borrower, loan, sfOwnerNode)) - return ter; + return ter; // LCOV_EXCL_LINE associateAsset(*vaultSle, vaultAsset); associateAsset(*brokerSle, vaultAsset); diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index 70eaadbe17..575512ecef 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -2927,6 +2927,7 @@ class Invariants_test : public beast::unit_test::Suite std::optional assetsAvailable = std::nullopt; std::optional lossUnrealized = std::nullopt; std::optional assetsMaximum = std::nullopt; + std::optional assetsReserved = std::nullopt; std::optional sharesTotal = std::nullopt; std::optional vaultAssets = std::nullopt; std::optional accountAssets = std::nullopt; @@ -2948,6 +2949,8 @@ class Invariants_test : public beast::unit_test::Suite (*sleVault)[sfLossUnrealized] = *args.lossUnrealized; if (args.assetsMaximum) (*sleVault)[sfAssetsMaximum] = *args.assetsMaximum; + if (args.assetsReserved) + (*sleVault)[sfAssetsReserved] = *args.assetsReserved; // Remaining fields are adjusted in terms of difference if (args.assetsTotal) @@ -3585,6 +3588,20 @@ class Invariants_test : public beast::unit_test::Suite precloseXrp, TxAccount::A2); + doInvariantCheck( + {"assets reserved must be positive or zero"}, + [&](Account const& a1, Account const& a2, ApplyContext& ac) { + auto const keylet = keylet::vault(a1.id(), SeqProxy::rawSequence(ac.view().seq())); + return kAdjust(ac.view(), keylet, kArgs(a2.id(), 0, [&](Adjustments& sample) { + sample.assetsReserved = -1; + })); + }, + XRPAmount{}, + STTx{ttVAULT_SET, [](STObject& tx) {}}, + {tecINVARIANT_FAILED, tecINVARIANT_FAILED}, + precloseXrp, + TxAccount::A2); + doInvariantCheck( {"set must not change shares outstanding", "updated zero sized vault must have no assets outstanding", diff --git a/src/test/app/lending/LoanRounding_test.cpp b/src/test/app/lending/LoanRounding_test.cpp index 5e69c9f79e..dcb450a651 100644 --- a/src/test/app/lending/LoanRounding_test.cpp +++ b/src/test/app/lending/LoanRounding_test.cpp @@ -15,8 +15,12 @@ #include #include +#include #include #include +#include +#include +#include #include #include #include @@ -959,6 +963,129 @@ private: env.close(); } + // LoanDelete::deleteActiveLoan clears any sub-scale residual left on + // LoanBroker.DebtTotal when the last active loan is removed. In production + // the residual comes from cross-loan rounding when multiple loans on the + // same broker operate at significantly different scales (see the comment + // above the adjustImpreciseNumber call in LoanPay.cpp's doApply). Building + // that accumulation deterministically from real txs is fragile, so this + // test installs a sub-drop residual directly on the broker SLE via + // OpenLedger::modify — the same lower-layer edit LoanTwoStep_test's + // makeVaultAccrual uses to force VaultVersion::Legacy — and then submits + // the LoanDelete against the mutated open view. LoanBrokerInvariant only + // forbids negative DebtTotal, so a positive sub-scale value is + // invariant-safe; the residual (5e-8 drops) rounds toward zero to 0 drops + // so the XRPL_ASSERT_PARTS guarding the branch also holds. + void + testDeleteLastLoanClearsDebtDust() + { + testcase("coverage: LoanDelete clears sub-scale DebtTotal dust on last loan"); + + using namespace jtx; + using namespace loan; + + Account const issuer{"issuer"}; + Account const lender{"lender"}; + Account const borrower{"borrower"}; + + Env env(*this, all_); + env.fund(XRP(1'000'000), issuer, lender, borrower); + env.close(); + + // scale = 1 keeps xrpAsset(N) at N drops so the tiny residual + // installed below is unambiguously sub-drop. + PrettyAsset const xrpAsset{xrpIssue(), 1}; + + // 0% interest so origination and payoff cancel to exactly zero on + // DebtTotal; the residual we test is installed by hand below. + BrokerParameters const brokerParams{ + .vaultDeposit = 100'000, + .debtMax = 10'000, + .coverRateMin = TenthBips32{0}, + .coverDeposit = 0, + .managementFeeRate = TenthBips16{0}, + .coverRateLiquidation = TenthBips32{0}}; + BrokerInfo const broker{createVaultAndBroker(env, xrpAsset, lender, brokerParams)}; + + auto const sleBroker0 = env.le(broker.brokerKeylet()); + if (!BEAST_EXPECT(sleBroker0)) + return; + auto const loanKeylet = + keylet::loan(broker.brokerID, SeqProxy::rawSequence(sleBroker0->at(sfLoanSequence))); + + // Active loan (immediate flow), single payment, 0% interest. + env(set(borrower, broker.brokerID, xrpAsset(100).value()), + Sig(sfCounterpartySignature, lender), + kInterestRate(TenthBips32{0}), + kPaymentTotal(1), + kPaymentInterval(3600), + Fee(env.current()->fees().base * 2)); + env.close(); + + // Fully pay off; 0% interest means the actual debit is exactly the + // principal, so DebtTotal returns cleanly to zero. + env(pay(borrower, loanKeylet.key, xrpAsset(200).value(), tfLoanFullPayment)); + env.close(); + + // Baseline: DebtTotal is exactly zero, the broker still owns the + // (now fully-paid) loan, and PaymentRemaining is zero so LoanDelete + // will not trip tecHAS_OBLIGATIONS. + if (auto const b = env.le(broker.brokerKeylet()); BEAST_EXPECT(b)) + { + BEAST_EXPECT(b->at(sfDebtTotal) == beast::kZero); + BEAST_EXPECT(b->at(sfOwnerCount) == 1); + } + if (auto const l = env.le(loanKeylet); BEAST_EXPECT(l)) + BEAST_EXPECT(l->at(sfPaymentRemaining) == 0); + + // Install a sub-drop residual on the broker's DebtTotal directly on + // the open ledger. Not closing after: OpenLedger::accept rebuilds + // the open view from the last-closed ledger and re-applies pending + // txs, discarding raw mutations, so every post-condition below is + // read from the open view. + Number const kResidual{5, -8}; + auto const mutated = + env.app().getOpenLedger().modify([&](OpenView& view, beast::Journal) -> bool { + Sandbox sb(&view, TapNone); + auto b = sb.peek(broker.brokerKeylet()); + if (!b) + return false; + b->at(sfDebtTotal) = kResidual; + sb.update(b); + sb.apply(view); + return true; + }); + if (!BEAST_EXPECT(mutated)) + return; + + // Sanity: the residual is visible on the open view and rounds to + // zero at the vault's asset scale (which is what the branch's + // XRPL_ASSERT_PARTS requires). + if (auto const b = env.le(broker.brokerKeylet()); BEAST_EXPECT(b)) + BEAST_EXPECT(b->at(sfDebtTotal) == kResidual); + if (auto const v = env.le(broker.vaultKeylet()); BEAST_EXPECT(v)) + { + BEAST_EXPECT( + roundToAsset( + v->at(sfAsset), + Number{kResidual}, + getAssetsTotalScale(v), + Number::RoundingMode::TowardsZero) == beast::kZero); + } + + // Delete against the mutated open view. The last-loan branch of + // deleteActiveLoan fires: DebtTotal is zeroed, OwnerCount goes to + // zero, and the loan SLE is erased. + env(del(lender, loanKeylet.key)); + + if (auto const b = env.le(broker.brokerKeylet()); BEAST_EXPECT(b)) + { + BEAST_EXPECT(b->at(sfDebtTotal) == beast::kZero); + BEAST_EXPECT(b->at(sfOwnerCount) == 0); + } + BEAST_EXPECT(!env.le(loanKeylet)); + } + void runAmendmentIndependent() { @@ -967,6 +1094,7 @@ private: testBugOverpaymentPrincipalChange(); testBugOverpayUnroundedAmount(); testBugInterestDueDeltaCrash(); + testDeleteLastLoanClearsDebtDust(); } // Tests run under each entry in amendmentCombinations(). diff --git a/src/test/app/lending/LoanTwoStep_test.cpp b/src/test/app/lending/LoanTwoStep_test.cpp index cc30ecee20..559e717403 100644 --- a/src/test/app/lending/LoanTwoStep_test.cpp +++ b/src/test/app/lending/LoanTwoStep_test.cpp @@ -1857,6 +1857,89 @@ private: expectStillPending(env, loanKeylet); } + // The preceding test advances the ledger clock past both StartDate + // and RedemptionDate, so LoanAccept::preclaim's StartDate expiry + // check fires first and the vault-phase branch itself is never + // exercised. The next two cases isolate the phase gate by rewriting + // the vault's SubscriptionDate / RedemptionDate on the open ledger + // (bypassing the normally-immutable-field invariant the same way + // makeVaultAccrual does for sfLEVersion) while leaving the loan's + // StartDate comfortably in the future. + for (auto const scenario : {VaultPhase::Subscription, VaultPhase::Redemption}) + { + char const* const phaseName = + scenario == VaultPhase::Subscription ? "Subscription" : "Redemption"; + TER const expected = + scenario == VaultPhase::Subscription ? TER{tecTOO_SOON} : TER{tecEXPIRED}; + testcase << "Two-step: LoanAccept rejected during " << phaseName + << " (StartDate not yet expired)"; + + Env env(*this, features); + env.fund(XRP(100'000'000), noripple(lender)); + env.fund(XRP(1'000'000), borrower); + env.close(); + + BrokerParameters params{}; + params.vaultKind = VaultKind::ClosedEnded; + params.subscriptionOffset = 60; + // Generous so LoanSet's finalPayment < RedemptionDate guard passes. + params.redemptionOffset = 10u * 365u * 24u * 60u * 60u; + auto const asset = createAsset(env, AssetType::XRP, params, issuer, lender, borrower); + auto const broker = createVaultAndBroker(env, asset, lender, params); + + // Propose while the vault is in Investment. StartDate is 1h out + // so the StartDate expiry check does not fire before the phase + // check, no matter which phase the mutation forces below. + auto const loanKeylet = nextLoanKeylet(env, broker); + std::uint32_t const startDate = (env.now() + 1h).time_since_epoch().count(); + propose(env, broker, lender, borrower, startDate); + env.close(); + expectStillPending(env, loanKeylet); + + // Force the vault into the target phase by rewriting the + // relevant date on the open ledger. Not closing between the + // mutation and the LoanAccept: OpenLedger::accept rebuilds the + // open view from the last-closed ledger and re-applies pending + // txs, discarding raw mutations. + std::uint32_t const parentClose = + env.current()->parentCloseTime().time_since_epoch().count(); + auto const changed = + env.app().getOpenLedger().modify([&](OpenView& view, beast::Journal) -> bool { + Sandbox sb(&view, TapNone); + auto v = sb.peek(broker.vaultKeylet()); + if (!v) + return false; + if (scenario == VaultPhase::Subscription) + { + // parentClose < SubscriptionDate → Subscription. + // Sit strictly below StartDate so nothing else shifts. + v->setFieldU32(sfSubscriptionDate, parentClose + 600); + } + else + { + // RedemptionDate < parentClose → Redemption. + // SubscriptionDate is already <= parentClose from + // createVaultAndBroker's phase advance. + v->setFieldU32(sfRedemptionDate, parentClose - 1); + } + sb.update(v); + sb.apply(view); + return true; + }); + if (!BEAST_EXPECT(changed)) + continue; + + // Sanity: the open-ledger view now reports the intended phase, + // and StartDate is still in the future so the phase gate — not + // the StartDate expiry check — is what will trip. + if (auto const v = env.le(broker.vaultKeylet()); BEAST_EXPECT(v)) + BEAST_EXPECT(getVaultPhase(*env.current(), v) == scenario); + BEAST_EXPECT(parentClose < startDate); + + env(accept(borrower, loanKeylet.key), Ter(expected)); + expectStillPending(env, loanKeylet); + } + { testcase("Two-step: pending loan bounds cover clawback, LoanAccept still succeeds"); diff --git a/src/test/app/vault/VaultValidation_test.cpp b/src/test/app/vault/VaultValidation_test.cpp index 4219ce4661..6e56627ff8 100644 --- a/src/test/app/vault/VaultValidation_test.cpp +++ b/src/test/app/vault/VaultValidation_test.cpp @@ -17,9 +17,13 @@ #include #include #include +#include #include #include #include +#include +#include +#include #include #include #include @@ -1068,6 +1072,86 @@ private: } } + // Covers the third obligation gate in VaultDelete::preclaim + // (sfAssetsReserved != 0). The first two guards (sfAssetsAvailable and + // sfAssetsTotal) short-circuit on any real-world path that inflates + // sfAssetsReserved — the only production writer is the two-step LoanSet + // pending-loan bookkeeping, which simultaneously moves the same amount + // out of sfAssetsAvailable, so the first check always fires first. + // Reproducing the (Available == 0, Total == 0, Reserved != 0) + // combination from real txs is not possible, so this test installs the + // residual directly on the vault SLE via OpenLedger::modify (the same + // lower-layer edit VaultShares_test uses to tamper with token fields) + // and confirms preclaim rejects the delete with tecHAS_OBLIGATIONS. + void + testVaultDeleteAssetsReservedBlocks() + { + testcase("VaultDelete rejected when only AssetsReserved is non-zero"); + + using namespace test::jtx; + + Env env{*this}; + Account const owner{"owner"}; + env.fund(XRP(1'000'000), owner); + env.close(); + + Vault const vault{env}; + PrettyAsset const xrpAsset = xrpIssue(); + auto const [tx, keylet] = vault.create({.owner = owner, .asset = xrpAsset}); + env(tx, Ter(tesSUCCESS)); + env.close(); + + // Baseline: a freshly-created empty vault has all three buckets at + // zero, so without the mutation below VaultDelete would succeed. + if (auto const v = env.le(keylet); BEAST_EXPECT(v)) + { + BEAST_EXPECT(v->at(sfAssetsAvailable) == beast::kZero); + BEAST_EXPECT(v->at(sfAssetsTotal) == beast::kZero); + BEAST_EXPECT(v->at(sfAssetsReserved) == beast::kZero); + } + + // Install a non-zero sfAssetsReserved directly on the vault SLE. + // ValidVault only inspects vault accounting when a tx mutates the + // vault; the raw edit happens outside the tx machinery so no + // invariant fires. VaultDelete below rejects at preclaim, so it + // never modifies the vault and invariants stay silent for the tx + // too. + Number const kReserved{1'000}; + auto const mutated = + env.app().getOpenLedger().modify([&](OpenView& view, beast::Journal) -> bool { + Sandbox sb(&view, TapNone); + auto v = sb.peek(keylet); + if (!v) + return false; + v->at(sfAssetsReserved) = kReserved; + sb.update(v); + sb.apply(view); + return true; + }); + if (!BEAST_EXPECT(mutated)) + return; + + // Sanity: the residual is visible and the two preceding guards + // (Available, Total) still resolve to zero, so preclaim's third + // check is the one that fires. + if (auto const v = env.le(keylet); BEAST_EXPECT(v)) + { + BEAST_EXPECT(v->at(sfAssetsAvailable) == beast::kZero); + BEAST_EXPECT(v->at(sfAssetsTotal) == beast::kZero); + BEAST_EXPECT(v->at(sfAssetsReserved) == kReserved); + } + + // Delete against the mutated open view. Not closing after: on + // close, OpenLedger::accept rebuilds the open view from the + // last-closed ledger and re-applies pending txs, discarding raw + // mutations, so the post-condition is read from the open view. + env(vault.del({.owner = owner, .id = keylet.key}), Ter(tecHAS_OBLIGATIONS)); + + // Preclaim rejected the delete, so the fee was charged but the + // vault SLE is untouched. + BEAST_EXPECT(env.le(keylet) != nullptr); + } + public: void run() override @@ -1077,6 +1161,7 @@ public: testCreateFailIOU(); testCreateFailMPT(); testVaultDeleteMemoData(); + testVaultDeleteAssetsReservedBlocks(); testVaultCreateLEVersion(); } };