fix: Treat an existing IOU line as a no-op in addEmptyHolding

include/xrpl/ledger/helpers/RippleStateHelpers.h

@@ -239,8 +239,13 @@ canTransfer(ReadView const& view, Issue const& issue, AccountID const& from, Acc
 //------------------------------------------------------------------------------
 
 /**
- * Any transactors that call addEmptyHolding() in doApply must call
- * canAddHolding() in preflight with the same View and Asset
+ * XRP and the issuer itself are always tesSUCCESS. Otherwise, after
+ * fixCleanup3_4_0, an existing trust line returns tecDUPLICATE without
+ * consulting issuer freeze or DefaultRipple; both still apply on the create
+ * path (DefaultRipple off is terNO_RIPPLE). canAddHolding() ignores existing
+ * holdings, so transactors that may create a holding in doApply should gate
+ * their preclaim call on it: after the amendment only when no holding
+ * exists, before it always.
  */
 [[nodiscard]] TER
 addEmptyHolding(

include/xrpl/ledger/helpers/TokenHelpers.h

@@ -319,6 +319,12 @@ transferRate(ReadView const& view, STAmount const& amount);
 [[nodiscard]] TER
 canAddHolding(ReadView const& view, Asset const& asset);
 
+/**
+ * True if the account already holds this asset (or is the issuer / XRP).
+ */
+[[nodiscard]] bool
+holdingExists(ReadView const& view, AccountID const& account, Asset const& asset);
+
 [[nodiscard]] TER
 addEmptyHolding(
     ApplyViewContext ctx,

src/libxrpl/ledger/helpers/MPTokenHelpers.cpp

@@ -184,6 +184,8 @@ addEmptyHolding(
     auto const mpt = ctx.view.peek(keylet::mptokenIssuance(mptID));
     if (!mpt)
         return tefINTERNAL;  // LCOV_EXCL_LINE
+    // Unlike IOU addEmptyHolding (post-fixCleanup3_4_0), a locked issuance is
+    // still rejected before the "MPToken already exists" short circuit.
     if (mpt->isFlag(lsfMPTLocked))
         return tefINTERNAL;  // LCOV_EXCL_LINE
     if (ctx.view.peek(keylet::mptoken(mptID, accountID)))

src/libxrpl/ledger/helpers/RippleStateHelpers.cpp

@@ -652,21 +652,32 @@ addEmptyHolding(
 
     auto const& issuerId = issue.getIssuer();
     auto const& currency = issue.currency;
-    if (isGlobalFrozen(ctx.view, issuerId))
-        return tecFROZEN;  // LCOV_EXCL_LINE
-
     auto const& srcId = issuerId;
     auto const& dstId = accountID;
     auto const high = srcId > dstId;
     auto const index = keylet::trustLine(srcId, dstId, currency);
+    // Post-fixCleanup3_4_0: an existing line is a no-op. Issuer freeze and
+    // DefaultRipple only matter when this function has to create a line.
+    bool const fix340Enabled = ctx.view.rules().enabled(fixCleanup3_4_0);
+    if (fix340Enabled && ctx.view.exists(index))
+        return tecDUPLICATE;
+
+    if (isGlobalFrozen(ctx.view, issuerId))
+        return tecFROZEN;  // LCOV_EXCL_LINE
+
     auto const sleSrc = ctx.view.peek(keylet::account(srcId));
     auto const sleDst = ctx.view.peek(keylet::account(dstId));
     if (!sleDst || !sleSrc)
         return tefINTERNAL;  // LCOV_EXCL_LINE
+    // Create path: DefaultRipple is still required. terNO_RIPPLE is
+    // intentional so VaultWithdraw / CoverWithdraw fail in preclaim via
+    // canAddHolding (retryable, no fee) rather than claiming a tec* fee
+    // in doApply. Transactor::operator() will not apply and will not
+    // convert it to tefINTERNAL.
     if (!sleSrc->isFlag(lsfDefaultRipple))
-        return tecINTERNAL;  // LCOV_EXCL_LINE
+        return fix340Enabled ? TER{terNO_RIPPLE} : tecINTERNAL;
     // If the line already exists, don't create it again.
-    if (ctx.view.read(index))
+    if (!fix340Enabled && ctx.view.exists(index))
         return tecDUPLICATE;
 
     // A reserve sponsor only covers tx.Account's own objects.

src/libxrpl/ledger/helpers/TokenHelpers.cpp

@@ -583,6 +583,32 @@ canAddHolding(ReadView const& view, Asset const& asset)
         asset.value());
 }
 
+[[nodiscard]] bool
+holdingExists(ReadView const& view, AccountID const& account, Issue const& issue)
+{
+    if (issue.native() || account == issue.getIssuer())
+        return true;
+    return view.exists(keylet::trustLine(account, issue));
+}
+
+[[nodiscard]] bool
+holdingExists(ReadView const& view, AccountID const& account, MPTIssue const& mptIssue)
+{
+    if (account == mptIssue.getIssuer())
+        return true;
+    return view.exists(keylet::mptoken(mptIssue.getMptID(), account));
+}
+
+[[nodiscard]] bool
+holdingExists(ReadView const& view, AccountID const& account, Asset const& asset)
+{
+    return std::visit(
+        [&]<ValidIssueType TIss>(TIss const& issue) -> bool {
+            return holdingExists(view, account, issue);
+        },
+        asset.value());
+}
+
 TER
 addEmptyHolding(
     ApplyViewContext ctx,

src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp

@@ -65,6 +65,7 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx)
 {
     auto const fix320Enabled = ctx.view.rules().enabled(fixCleanup3_2_0);
     auto const fix330Enabled = ctx.view.rules().enabled(fixCleanup3_3_0);
+    auto const fix340Enabled = ctx.view.rules().enabled(fixCleanup3_4_0);
     auto const& tx = ctx.tx;
 
     auto const account = tx[sfAccount];
@@ -140,6 +141,12 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx)
     if (auto const ter = requireAuth(ctx.view, vaultAsset, dstAcct, authType))
         return ter;
 
+    if (fix340Enabled && account == dstAcct && !holdingExists(ctx.view, dstAcct, vaultAsset))
+    {
+        if (auto const ter = canAddHolding(ctx.view, vaultAsset); !isTesSuccess(ter))
+            return ter;
+    }
+
     if (fix330Enabled)
     {
         if (auto const ret =

src/libxrpl/tx/transactors/lending/LoanSet.cpp

@@ -372,8 +372,24 @@ LoanSet::preclaim(PreclaimContext const& ctx)
         }
     }
 
-    if (auto const ter = canAddHolding(ctx.view, asset))
-        return ter;
+    // canAddHolding is an issuer-level check (DefaultRipple for IOU,
+    // lsfMPTCanTransfer for MPT); neither overload looks at the
+    // destination, so the holdingExists() clauses only decide whether a
+    // create path is reachable at all. It always runs before
+    // fixCleanup3_4_0: IOU addEmptyHolding checks DefaultRipple ahead of
+    // the existing-line case, so only preclaim can turn an existing line
+    // under a cleared DefaultRipple into terNO_RIPPLE rather than
+    // tecINTERNAL. After the amendment an existing line short-circuits to
+    // tecDUPLICATE, which doApply ignores, so run the check only when the
+    // borrower lacks a holding, or the origination fee is nonzero and the
+    // broker owner lacks one.
+    auto const originationFee = tx[~sfLoanOriginationFee].value_or(Number{});
+    if (!ctx.view.rules().enabled(fixCleanup3_4_0) || !holdingExists(ctx.view, borrower, asset) ||
+        (originationFee != beast::kZero && !holdingExists(ctx.view, brokerOwner, asset)))
+    {
+        if (auto const ter = canAddHolding(ctx.view, asset))
+            return ter;
+    }
 
     // vaultPseudo is going to send funds, so it can't be frozen.
     if (auto const ret = checkFrozen(ctx.view, vaultPseudo, asset))

src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp

@@ -205,6 +205,15 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx)
     if (auto const ter = requireAuth(ctx.view, vaultAsset, dstAcct, authType); !isTesSuccess(ter))
         return ter;
 
+    // Fail early when self-destination would have to create a holding.
+    // Skip when a holding already exists: canAddHolding does not look at that,
+    // and would block a no-op create (the DefaultRipple-cleared self-withdraw).
+    if (fix340Enabled && account == dstAcct && !holdingExists(ctx.view, dstAcct, vaultAsset))
+    {
+        if (auto const ter = canAddHolding(ctx.view, vaultAsset); !isTesSuccess(ter))
+            return ter;
+    }
+
     // The checks above only establish that an account may hold the asset. A
     // private vault additionally restricts who may take part in it, so paying
     // its asset out to a third party requires both ends of that payout to be

src/test/app/lending/LoanPay_test.cpp

@@ -37,6 +37,7 @@
 #include <cstdint>
 #include <limits>
 #include <memory>
+#include <string>
 #include <type_traits>
 
 namespace xrpl::test {
@@ -1419,6 +1420,69 @@ class LoanPay_test : public LoanTestBase
         BEAST_EXPECT(stateAfter.nextPaymentDate == exactDueDate);
     }
 
+    // LoanPay does not call canAddHolding. addEmptyHolding recreates the
+    // broker-owner holding when the borrower is also the broker owner. After
+    // fixCleanup3_4_0 an existing line is a no-op even if DefaultRipple is
+    // off; pre-fix that path dies with tecINTERNAL.
+    void
+    testLoanPaySelfBrokerExistingLineDefaultRipple()
+    {
+        using namespace jtx;
+        using namespace loan;
+
+        auto run = [this](FeatureBitset features, TER expected) {
+            testcase(
+                std::string(
+                    "LoanPay broker-owner borrower existing line after "
+                    "issuer clears asfDefaultRipple (") +
+                (features[fixCleanup3_4_0] ? "post" : "pre") + "-fixCleanup3_4_0)");
+
+            Env env(*this, features);
+            Account const issuer{"issuer"};
+            Account const alice{"alice"};
+
+            env.fund(XRP(10'000), issuer, alice);
+            env.close();
+            env(fset(issuer, asfDefaultRipple));
+            env.close();
+
+            PrettyAsset const usd{issuer["USD"]};
+            env(trust(alice, usd(10'000'000)));
+            env.close();
+            env(pay(issuer, alice, usd(2'000'000)));
+            env.close();
+
+            auto const broker = createVaultAndBroker(env, usd, alice);
+            auto const brokerSle = env.le(keylet::loanBroker(broker.brokerID));
+            if (!BEAST_EXPECT(brokerSle))
+                return;
+            auto const loanKeylet =
+                keylet::loan(broker.brokerID, SeqProxy::rawSequence(brokerSle->at(sfLoanSequence)));
+
+            Number const serviceFee = usd(2).value();
+            env(set(alice, broker.brokerID, usd(1'000).value()),
+                Sig(sfCounterpartySignature, alice),
+                kLoanServiceFee(serviceFee),
+                Fee(env.current()->fees().base * 2));
+            env.close();
+
+            env(fclear(issuer, asfDefaultRipple));
+            env.close();
+            BEAST_EXPECT(env.le(keylet::trustLine(alice.id(), usd.raw().get<Issue>())));
+
+            auto const state = getCurrentState(env, broker, loanKeylet);
+            STAmount const payment{
+                usd,
+                roundPeriodicPayment(usd, state.periodicPayment + serviceFee, state.loanScale)};
+
+            env(pay(alice, loanKeylet.key, payment), Ter(expected));
+            env.close();
+        };
+
+        run(all_ - fixCleanup3_4_0, tecINTERNAL);
+        run(all_, tesSUCCESS);
+    }
+
     void
     runAmendmentIndependent()
     {
@@ -1429,6 +1493,7 @@ class LoanPay_test : public LoanTestBase
         testLoanPayCatchUpFeeAtExactDueDatePostAmendment();
         testLoanPayCatchUpFeeAtExactDueDatePreAmendment();
         testRepayIntoUnauthorizedVault();
+        testLoanPaySelfBrokerExistingLineDefaultRipple();
     }
 
     // Tests run under each entry in amendmentCombinations().

src/test/app/lending/LoanSet_test.cpp

@@ -31,6 +31,7 @@
 #include <array>
 #include <cstdint>
 #include <functional>
+#include <string>
 #include <utility>
 #include <vector>
 
@@ -764,6 +765,69 @@ class LoanSet_test : public LoanTestBase
         });
     }
 
+    // LoanSet used to call canAddHolding unconditionally, so an existing
+    // borrower line still failed with terNO_RIPPLE after the issuer cleared
+    // DefaultRipple. After fixCleanup3_4_0, skip that gate when the holding
+    // already exists.
+    void
+    testLoanSetExistingLineAfterIssuerClearsDefaultRipple()
+    {
+        using namespace jtx;
+        using namespace loan;
+
+        auto run = [this](FeatureBitset features, TER expected) {
+            testcase(
+                std::string(
+                    "LoanSet existing borrower line after issuer "
+                    "clears asfDefaultRipple (") +
+                (features[fixCleanup3_4_0] ? "post" : "pre") + "-fixCleanup3_4_0)");
+
+            Env env(*this, features);
+            Account const issuer{"issuer"};
+            Account const lender{"lender"};
+            Account const borrower{"borrower"};
+
+            env.fund(XRP(10'000), issuer, lender, borrower);
+            env.close();
+            env(fset(issuer, asfDefaultRipple));
+            env.close();
+
+            PrettyAsset const usd{issuer["USD"]};
+            env(trust(lender, usd(10'000'000)));
+            env(trust(borrower, usd(10'000'000)));
+            env.close();
+            env(pay(issuer, lender, usd(2'000'000)));
+            env(pay(issuer, borrower, usd(1'000)));
+            env.close();
+            BEAST_EXPECT(env.le(keylet::trustLine(borrower.id(), usd.raw().get<Issue>())));
+
+            auto const broker = createVaultAndBroker(env, usd, lender);
+
+            env(fclear(issuer, asfDefaultRipple));
+            env.close();
+
+            Number const destBefore = env.balance(borrower, usd.raw()).number();
+            env(set(borrower, broker.brokerID, usd(100).value()),
+                Sig(sfCounterpartySignature, lender),
+                Fee(env.current()->fees().base * 2),
+                Ter(expected));
+            env.close();
+
+            Number const destAfter = env.balance(borrower, usd.raw()).number();
+            if (isTesSuccess(expected))
+            {
+                BEAST_EXPECT(destAfter == destBefore + Number{100});
+            }
+            else
+            {
+                BEAST_EXPECT(destAfter == destBefore);
+            }
+        };
+
+        run(all_ - fixCleanup3_4_0, terNO_RIPPLE);
+        run(all_, tesSUCCESS);
+    }
+
 public:
     void
     run() override
@@ -773,6 +837,7 @@ class LoanSet_test : public LoanTestBase
             testLoanSet(features);
 
         testLoanSetClosedEnded();
+        testLoanSetExistingLineAfterIssuerClearsDefaultRipple();
     }
 };