From efadaf6aa7c9d0e1e01ec99ac9928e82bb1130bc Mon Sep 17 00:00:00 2001 From: JCW Date: Wed, 26 Aug 2026 15:58:15 +0100 Subject: [PATCH] Self review --- include/xrpl/tx/invariants/LoanInvariant.h | 18 +++- include/xrpl/tx/invariants/VaultInvariant.h | 5 +- src/libxrpl/tx/invariants/InvariantCheck.cpp | 24 +++-- .../tx/invariants/LoanBrokerInvariant.cpp | 2 +- src/libxrpl/tx/invariants/LoanInvariant.cpp | 19 ++-- src/libxrpl/tx/invariants/VaultInvariant.cpp | 2 +- .../app/invariants/InvariantsMisc_test.cpp | 99 +++++++++++++++++++ .../app/invariants/InvariantsVault_test.cpp | 14 +-- 8 files changed, 151 insertions(+), 32 deletions(-) diff --git a/include/xrpl/tx/invariants/LoanInvariant.h b/include/xrpl/tx/invariants/LoanInvariant.h index 9ee36ed814..a5e0e029b4 100644 --- a/include/xrpl/tx/invariants/LoanInvariant.h +++ b/include/xrpl/tx/invariants/LoanInvariant.h @@ -15,9 +15,25 @@ namespace xrpl { /** * @brief Invariants: Loans are internally consistent * - * 1. If `Loan.PaymentRemaining = 0` then `Loan.PrincipalOutstanding = 0` + * 1. If `Loan.PaymentRemaining = 0` then `Loan.PrincipalOutstanding = 0`. * 2. A newly-created Loan against a closed-ended vault must satisfy * `StartDate + PaymentInterval * PaymentRemaining < Vault.RedemptionDate`. + * 3. An `ltLOAN` may only be created by a `ttLOAN_SET` transaction. + * 4. Prior to `featureLendingProtocolV1_1`, the `lsfLoanOverpayment` flag on a + * Loan must not change. From `featureLendingProtocolV1_1` onward this check + * is enforced in `InvariantChecks.cpp`. + * 5. Under `featureLendingProtocolV1_1`: + * a. An `ltLOAN` may only be deleted by a `ttLOAN_DELETE` transaction. + * b. If `Loan.PaymentRemaining = 0` then `Loan.NextPaymentDueDate = 0`. + * c. The `lsfLoanImpaired` flag may only change through a `ttLOAN_MANAGE` + * or `ttLOAN_PAY` transaction. + * d. The `lsfLoanDefault` flag may only change through a `ttLOAN_MANAGE` + * transaction. + * e. Interest due, computed as `TotalValueOutstanding - + * PrincipalOutstanding - ManagementFeeOutstanding`, must not be + * negative. + * f. A Loan must reference a live `ltLOAN_BROKER`, and that broker must + * reference a live `ltVAULT`. * */ class ValidLoan diff --git a/include/xrpl/tx/invariants/VaultInvariant.h b/include/xrpl/tx/invariants/VaultInvariant.h index 2ba42f0ab4..efeec7fda6 100644 --- a/include/xrpl/tx/invariants/VaultInvariant.h +++ b/include/xrpl/tx/invariants/VaultInvariant.h @@ -48,7 +48,10 @@ namespace xrpl { * vault phase is Investment * * Immutability of VaultKind, SubscriptionDate and RedemptionDate is enforced - * by NoModifiedUnmodifiableFields (see InvariantCheck.cpp). + * by NoModifiedUnmodifiableFields (see InvariantCheck.cpp). From + * featureLendingProtocolV1_1 onwards, immutability of the vault's Asset, + * pseudo-account and ShareMPTID is likewise enforced by + * NoModifiedUnmodifiableFields; prior to that amendment it is checked here. */ class ValidVault { diff --git a/src/libxrpl/tx/invariants/InvariantCheck.cpp b/src/libxrpl/tx/invariants/InvariantCheck.cpp index c4d4bab7c8..872bd81aff 100644 --- a/src/libxrpl/tx/invariants/InvariantCheck.cpp +++ b/src/libxrpl/tx/invariants/InvariantCheck.cpp @@ -1217,23 +1217,29 @@ NoModifiedUnmodifiableFields::finalize( } break; case ltVAULT: - if (view.rules().enabled(fixCleanup3_4_0)) + /* + * Immutability of sfAccount, sfAsset and sfShareMPTID used to be enforced by + * VaultInvariant, but is now checked here since InvariantCheck.cpp is where + * immutability checks live. The additional fields below are introduced by + * featureLendingProtocolV1_1 and only exist on V1_1 vaults. + */ + if (view.rules().enabled(featureLendingProtocolV1_1)) { - bad = bad || kFieldChanged(before, after, sfSequence) || + bad = bad || kFieldChanged(before, after, sfVaultKind) || + kFieldChanged(before, after, sfSubscriptionDate) || + kFieldChanged(before, after, sfRedemptionDate) || + kFieldChanged(before, after, sfSequence) || kFieldChanged(before, after, sfOwnerNode) || kFieldChanged(before, after, sfOwner) || kFieldChanged(before, after, sfWithdrawalPolicy) || - kFieldChanged(before, after, sfScale); + kFieldChanged(before, after, sfScale) || + kFieldChanged(before, after, sfLEVersion); } - if (view.rules().enabled(featureLendingProtocolV1_1)) + if (view.rules().enabled(fixCleanup3_4_0)) { bad = bad || kFieldChanged(before, after, sfAsset) || kFieldChanged(before, after, sfAccount) || - kFieldChanged(before, after, sfShareMPTID) || - kFieldChanged(before, after, sfVaultKind) || - kFieldChanged(before, after, sfSubscriptionDate) || - kFieldChanged(before, after, sfRedemptionDate) || - kFieldChanged(before, after, sfLEVersion); + kFieldChanged(before, after, sfShareMPTID); } break; default: diff --git a/src/libxrpl/tx/invariants/LoanBrokerInvariant.cpp b/src/libxrpl/tx/invariants/LoanBrokerInvariant.cpp index 2db617fe0e..84e0c9364b 100644 --- a/src/libxrpl/tx/invariants/LoanBrokerInvariant.cpp +++ b/src/libxrpl/tx/invariants/LoanBrokerInvariant.cpp @@ -52,7 +52,7 @@ ValidLoanBroker::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref mpts_.emplace_back(before); break; default: - return; + break; } } if (after) diff --git a/src/libxrpl/tx/invariants/LoanInvariant.cpp b/src/libxrpl/tx/invariants/LoanInvariant.cpp index 738d728503..aeb8461f8f 100644 --- a/src/libxrpl/tx/invariants/LoanInvariant.cpp +++ b/src/libxrpl/tx/invariants/LoanInvariant.cpp @@ -14,7 +14,6 @@ #include // IWYU pragma: keep #include #include -#include #include #include @@ -54,15 +53,6 @@ ValidLoan::finalize( // Ledger entry validation checks. for (auto const& [before, after] : loans_) { - // Only LoanSet may create a loan. This is an object-existence rule, not - // a transaction post-condition, so it applies even when apply failed. - if (!before && txType != ttLOAN_SET) - { - JLOG(j.fatal()) << "Invariant failed: Loan created by a transaction " - "other than LoanSet"; - return false; - } - // A closed-ended vault must not accept a loan whose final scheduled payment falls on or // after the vault's RedemptionDate. This mirrors the LoanSet::preclaim gate and only fires // on loan creation; once the loan exists, its StartDate / PaymentInterval are immutable and @@ -150,6 +140,15 @@ ValidLoan::finalize( } if (v1Enabled) { + // Only LoanSet may create a loan. This is an object-existence rule, not + // a transaction post-condition, so it applies even when apply failed. + if (!before && txType != ttLOAN_SET) + { + JLOG(j.fatal()) << "Invariant failed: Loan created by a transaction " + "other than LoanSet"; + return false; + } + if (after->at(sfPaymentRemaining) == 0 && after->at(~sfNextPaymentDueDate).value_or(0) != 0) { diff --git a/src/libxrpl/tx/invariants/VaultInvariant.cpp b/src/libxrpl/tx/invariants/VaultInvariant.cpp index a8ef0d3157..72f95ad874 100644 --- a/src/libxrpl/tx/invariants/VaultInvariant.cpp +++ b/src/libxrpl/tx/invariants/VaultInvariant.cpp @@ -516,7 +516,7 @@ ValidVault::finalize( // Universal transaction checks // From LendingProtocolV1_1 onwards, vault immutability check is moved to InvariantCheck.cpp - if (!beforeVault_.empty() && !view.rules().enabled(featureLendingProtocolV1_1)) + if (!beforeVault_.empty() && !view.rules().enabled(fixCleanup3_4_0)) { auto const& beforeVault = beforeVault_[0]; if (afterVault.asset != beforeVault.asset || afterVault.pseudoId != beforeVault.pseudoId || diff --git a/src/test/app/invariants/InvariantsMisc_test.cpp b/src/test/app/invariants/InvariantsMisc_test.cpp index 3c9760bac3..29050b87f5 100644 --- a/src/test/app/invariants/InvariantsMisc_test.cpp +++ b/src/test/app/invariants/InvariantsMisc_test.cpp @@ -884,6 +884,105 @@ class InvariantsMisc_test : public InvariantsBase } } + // ValidLoan::finalize enforces (under featureLendingProtocolV1_1) that + // interest due - TotalValueOutstanding minus PrincipalOutstanding minus + // ManagementFeeOutstanding - is never negative. Any rounding path in + // LoanPay / LoanManage that rounds Principal or ManagementFee up while + // rounding TotalValue down (or vice-versa) by a single ULP flips this + // negative and would halt the ledger. Exercise each of the three + // components at a one-ULP overshoot to cover the boundary explicitly, + // plus the exact-zero case to confirm the boundary itself is + // accepted. + { + struct Case + { + Number totalValue; + Number principal; + Number managementFee; + bool expectFire; + }; + // Baseline: Principal=100, TotalValue=100, MgmtFee=0 + // (interest due = 0, exactly at the boundary). Each firing case + // perturbs one component by -1 or +1 so interest_due = -1. + auto const cases = std::to_array({ + {.totalValue = Number(100), + .principal = Number(100), + .managementFee = Number(0), + .expectFire = false}, + {.totalValue = Number(99), + .principal = Number(100), + .managementFee = Number(0), + .expectFire = true}, + {.totalValue = Number(100), + .principal = Number(101), + .managementFee = Number(0), + .expectFire = true}, + {.totalValue = Number(100), + .principal = Number(100), + .managementFee = Number(1), + .expectFire = true}, + }); + + for (auto const& c : cases) + { + Env env{*this, all_}; + Account const a1{"A1"}; + env.fund(XRP(1000), a1); + env.close(); + + OpenView ov{*env.current()}; + + auto const brokerKeylet = + keylet::loanBroker(a1.id(), SeqProxy::rawSequence(ov.seq())); + auto const loanKeylet = keylet::loan(brokerKeylet.key, SeqProxy::rawSequence(1)); + // Seed a loan whose interest due sits at the boundary + // (100 - 100 - 0 = 0). The apply-view update below moves it. + { + auto sleLoan = makeLoanSle(brokerKeylet.key, 1, a1.id()); + sleLoan->at(sfPrincipalOutstanding) = Number(100); + sleLoan->at(sfTotalValueOutstanding) = Number(100); + sleLoan->at(sfManagementFeeOutstanding) = Number(0); + sleLoan->setFieldU32(sfPaymentRemaining, 1); + ov.rawInsert(sleLoan); + } + + STTx const tx{ttACCOUNT_SET, [](STObject&) {}}; + test::StreamSink sink{beast::Severity::Warning}; + beast::Journal const jlog{sink}; + ApplyContext ac{ + env.app(), ov, tx, tesSUCCESS, env.current()->fees().base, TapNone, jlog}; + CurrentTransactionRulesGuard const rulesGuard(ov.rules()); + + auto sleLoan = ac.view().peek(loanKeylet); + if (!BEAST_EXPECT(sleLoan)) + continue; + sleLoan->at(sfTotalValueOutstanding) = c.totalValue; + sleLoan->at(sfPrincipalOutstanding) = c.principal; + sleLoan->at(sfManagementFeeOutstanding) = c.managementFee; + ac.view().update(sleLoan); + + auto transactor = makeTransactor(ac); + if (!BEAST_EXPECT(transactor)) + continue; + TER const result = transactor->checkInvariants( + tesSUCCESS, XRPAmount{}, Transactor::InvariantScope::Full); + auto const messages = sink.messages().str(); + if (c.expectFire) + { + BEAST_EXPECT(result == tecINVARIANT_FAILED); + BEAST_EXPECT(messages.contains("Loan interest due is negative")); + } + else + { + // The boundary case (interest due == 0) must not trip the + // interest-due check. Other invariants may still fire + // (e.g. the broker-existence check on this raw-inserted + // loan), so only assert the specific message is absent. + BEAST_EXPECT(!messages.contains("Loan interest due is negative")); + } + } + } + // VaultKind, SubscriptionDate and RedemptionDate are immutable once set at creation. // Enforced by NoModifiedUnmodifiableFields on ltVAULT via kFieldChanged. Keylet closedEndedVaultKeylet = keylet::amendments(); diff --git a/src/test/app/invariants/InvariantsVault_test.cpp b/src/test/app/invariants/InvariantsVault_test.cpp index 7493ba357f..35240b18b5 100644 --- a/src/test/app/invariants/InvariantsVault_test.cpp +++ b/src/test/app/invariants/InvariantsVault_test.cpp @@ -6,7 +6,6 @@ #include #include #include -#include #include #include #include @@ -705,14 +704,14 @@ class InvariantsVault_test : public InvariantsBase {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, precloseXrp); - // Pre-featureLendingProtocolV1_1 the immutability of sfAsset, sfAccount + // Pre-fixCleanup3_4_0 the immutability of sfAsset, sfAccount // and sfShareMPTID is enforced by ValidVault directly, which reports // "violation of vault immutable data" on the first pass. ValidVault // returns early on the second pass (result already tec), so the check - // does not escalate to tef. Once V1_1 activates, the same fields are + // does not escalate to tef. Once fixCleanup3_4_0 activates, the same fields are // covered by NoModifiedUnmodifiableFields (see the three cases above); // the two paths are mutually exclusive so both need coverage. - auto const preLendingV11Amendments = all_ - featureLendingProtocolV1_1; + auto const preLendingV11Amendments = all_ - fixCleanup3_4_0; doInvariantCheck( makeEnv(preLendingV11Amendments), {"violation of vault immutable data"}, @@ -1554,12 +1553,9 @@ class InvariantsVault_test : public InvariantsBase {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, precloseXrp); - // The pre-V1_1 vault fields must be enforced without waiting for - // featureLendingProtocolV1_1. Run the withdrawal-policy mutation with - // V1_1 removed but fixCleanup3_4_0 (part of testableAmendments) still enabled: the - // invariant must still fire. + // fixCleanup3_4_0 moves the vault immutability checks from VaultInvariant to InvariantCheck. doInvariantCheck( - makeEnv(all_ - featureLendingProtocolV1_1), + makeEnv(all_ - fixCleanup3_4_0), {"changed an unchangeable field"}, [&](Account const& a1, Account const& a2, ApplyContext& ac) { auto const keylet = keylet::vault(a1.id(), SeqProxy::rawSequence(ac.view().seq()));