debugging, bug fixes

src/ripple/app/tx/impl/Escrow.cpp

@@ -235,12 +235,24 @@ EscrowCreate::doApply()
         return tecUNFUNDED;
     else
     {
+        // preflight will prevent this ever firing, included
+        // defensively for completeness
         if (!ctx_.view().rules().enabled(featurePaychanAndEscrowForTokens))
             return tefINTERNAL;
 
-        sleLine = ctx_.view().peek(keylet::line(account, amount.getIssuer(), amount.getCurrency()));
+        // check if the escrow is capable of being
+        // finished before we allow it to be created
+        if (TER result = 
+            trustXferAllowed(
+                ctx_.view(),
+                {account, ctx_.tx[sfDestination]},
+                amount.issue());
+            result != tesSUCCESS)
+            return result;
 
-        // perform the lock as a dry run first
+        // perform the lock as a dry run before
+        // we modify anything on-ledger
+        sleLine = ctx_.view().peek(keylet::line(account, amount.getIssuer(), amount.getCurrency()));
         if (TER result = trustAdjustLockedBalance(ctx_.view(), sleLine, amount, true);
                 result != tesSUCCESS)
             return result;
@@ -301,16 +313,17 @@ EscrowCreate::doApply()
     // Deduct owner's balance, increment owner count
     if (isXRP(amount))
         (*sle)[sfBalance] = (*sle)[sfBalance] - ctx_.tx[sfAmount];
-    else if (ctx_.view().rules().enabled(featurePaychanAndEscrowForTokens) && sleLine)
+    else 
     {
+        if (!ctx_.view().rules().enabled(featurePaychanAndEscrowForTokens) || !sleLine)
+            return tefINTERNAL;
+
         // do the lock-up for real now
         TER result =
-            trustAdjustLockedBalance(ctx_.view(), sleLine, amount, true);
+            trustAdjustLockedBalance(ctx_.view(), sleLine, amount, false);
         if (result != tesSUCCESS)
             return result;
     }
-    else
-        return tecINTERNAL;  // should never happen
 
     adjustOwnerCount(ctx_.view(), sle, 1, ctx_.journal);
     ctx_.view().update(sle);
@@ -540,8 +553,10 @@ EscrowFinish::doApply()
 
     if (isXRP(amount))
         (*sled)[sfBalance] = (*sled)[sfBalance] + (*slep)[sfAmount];
-    else if (ctx_.view().rules().enabled(featurePaychanAndEscrowForTokens))
+    else 
     {
+        if (!ctx_.view().rules().enabled(featurePaychanAndEscrowForTokens))
+            return tefINTERNAL;
 
         // all the significant complexity of checking the validity of this
         // transfer and ensuring the lines exist etc is hidden away in this
@@ -558,8 +573,6 @@ EscrowFinish::doApply()
         if (result != tesSUCCESS)
             return result;
     }
-    else
-        return tecINTERNAL; // should never happen
 
     ctx_.view().update(sled);
 
@@ -670,8 +683,11 @@ EscrowCancel::doApply()
     // Transfer amount back to the owner (or unlock it in TL case)
     if (isXRP(amount))
         (*sle)[sfBalance] = (*sle)[sfBalance] + (*slep)[sfAmount];
-    else if (ctx_.view().rules().enabled(featurePaychanAndEscrowForTokens))
+    else
     {
+        if (!ctx_.view().rules().enabled(featurePaychanAndEscrowForTokens))
+            return tefINTERNAL;
+
         // unlock previously locked tokens from source line
         TER result = trustAdjustLockedBalance(
                 ctx_.view(),
@@ -681,8 +697,6 @@ EscrowCancel::doApply()
         if (result != tesSUCCESS)
             return result;
     }
-    else
-        return tecINTERNAL;
 
     // Decrement owner count
     adjustOwnerCount(ctx_.view(), sle, -1, ctx_.journal);

src/ripple/ledger/View.h

@@ -33,6 +33,7 @@
 #include <ripple/protocol/STTx.h>
 #include <ripple/protocol/Serializer.h>
 #include <ripple/protocol/TER.h>
+#include <ripple/protocol/Feature.h>
 #include <functional>
 #include <map>
 #include <memory>
@@ -303,12 +304,6 @@ bool isTrustDefault(
     std::shared_ptr<SLE> const& acc,
     std::shared_ptr<SLE> const& line);
 
-[[nodiscard]] TER
-trustXferAllowed(
-    ReadView const& view,
-    std::vector<AccountID> const& parties,
-    Issue const& issue);
-
 template<class V, class S>
 [[nodiscard]] TER
 trustAdjustLockedBalance(
@@ -322,13 +317,10 @@ trustAdjustLockedBalance(
         (std::is_same<V, ReadView const>::value && std::is_same<S, std::shared_ptr<SLE const>>::value) ||
         (std::is_same<V, ApplyView>::value && std::is_same<S, std::shared_ptr<SLE>>::value));
 
-    constexpr bool bReadView = std::is_same<V, ReadView const>::value;
-
     // dry runs are explicit in code, but really the view type determines
     // what occurs here, so this combination is invalid.
 
-    if (bReadView && !dryRun)
+    assert(!(std::is_same<V, ReadView const>::value && !dryRun));
-            return tefINTERNAL;
 
     auto const currency = deltaAmt.getCurrency();
     auto const issuer   = deltaAmt.getIssuer();
@@ -350,7 +342,10 @@ trustAdjustLockedBalance(
             parties,
             deltaAmt.issue());
         result != tesSUCCESS)
+    {
+        printf("trustXferAllowed failed on trustAdjustLockedBalance\n");
         return result;
+    }
 
     // pull the TL balance from the account's perspective
     STAmount balance =
@@ -395,6 +390,156 @@ trustAdjustLockedBalance(
     return tesSUCCESS;
 }
 
+
+template<class V>
+ReadView const&
+forceReadView(V& view)
+{
+    static_assert(
+        std::is_same<V, std::shared_ptr<ReadView const>>::value ||
+        std::is_same<V, std::shared_ptr<ApplyView>>::value ||
+        std::is_same<V, ApplyView>::value ||
+        std::is_same<V, ReadView const>::value);
+
+    ReadView const* rv = NULL;
+
+    if constexpr (
+        std::is_same<V, std::shared_ptr<ReadView const>>::value ||
+        std::is_same<V, std::shared_ptr<ApplyView>>::value)
+        rv = dynamic_cast<ReadView const*>(&(*view));
+    else if constexpr(
+        std::is_same<V, ApplyView>::value ||
+        std::is_same<V, ReadView const>::value)
+        rv = dynamic_cast<ReadView const*>(&view);
+    
+    return *rv;
+}
+
+// Check if movement of a particular token between 1 or more accounts
+// (including unlocking) is forbidden by any flag or condition.
+// If parties contains 1 entry then noRipple is not a bar to xfer.
+// Part of featurePaychanAndEscrowForTokens, but can be callled without guard
+
+template<class V>
+[[nodiscard]]TER
+trustXferAllowed(
+    V& view_,
+    std::vector<AccountID> const& parties,
+    Issue const& issue)
+{
+    
+    ReadView const& view = forceReadView(view_);
+
+    if (isFakeXRP(issue.currency))
+        return tecNO_PERMISSION;
+
+    auto const sleIssuerAcc = view.read(keylet::account(issue.account));
+
+    bool lockedBalanceAllowed =
+        view.rules().enabled(featurePaychanAndEscrowForTokens);
+
+    // missing issuer is always a bar to xfer
+    if (!sleIssuerAcc)
+        return tecNO_ISSUER;
+
+    // issuer global freeze is always a bar to xfer
+    if (isGlobalFrozen(view, issue.account))
+        return tecFROZEN;
+
+    uint32_t issuerFlags = sleIssuerAcc->getFieldU32(sfFlags);
+
+    bool requireAuth = issuerFlags & lsfRequireAuth;
+
+    for (AccountID const& p: parties)
+    {
+        auto line = view.read(keylet::line(p, issue.account, issue.currency));
+        if (!line)
+        {
+            if (requireAuth)
+            {
+                // the line doesn't exist, i.e. it is in default state
+                // default state means the line has not been authed
+                // therefore if auth is required by issuer then
+                // this is now a bar to xfer
+                return tecNO_AUTH;
+            }
+
+            // missing line is a line in default state, this is not
+            // a general bar to xfer, however additional conditions
+            // do attach to completing an xfer into a default line
+            // but these are checked in trustXferLockedBalance at
+            // the point of transfer.
+            continue;
+        }
+
+        // sanity check the line, insane lines are a bar to xfer
+        {
+            // these "strange" old lines, if they even exist anymore are
+            // always a bar to xfer
+            if (line->getFieldAmount(sfLowLimit).getIssuer() ==
+                    line->getFieldAmount(sfHighLimit).getIssuer())
+                return tecINTERNAL;
+
+            if (line->isFieldPresent(sfLockedBalance))
+            {
+                if (!lockedBalanceAllowed)
+                {
+                    printf("lockedBalanceAllowed was false\n");
+                    return tecINTERNAL;
+                }
+
+                STAmount lockedBalance = line->getFieldAmount(sfLockedBalance);
+                STAmount balance = line->getFieldAmount(sfBalance);
+
+                if (lockedBalance.getCurrency() != balance.getCurrency())
+                {
+                    printf("lockedBalance issuer/currency did not match balance issuer/currency\n");
+                    return tecINTERNAL;
+                }
+            }
+        }
+
+        // check the bars to xfer ... these are:
+        // any TL in the set has noRipple on the issuer's side
+        // any TL in the set has a freeze on the issuer's side
+        // any TL in the set has RequireAuth and the TL lacks lsf*Auth
+        {
+            bool pHigh = p > issue.account;
+
+            auto const flagIssuerNoRipple { pHigh ? lsfLowNoRipple : lsfHighNoRipple };
+            auto const flagIssuerFreeze   { pHigh ? lsfLowFreeze   : lsfHighFreeze   };
+            auto const flagIssuerAuth     { pHigh ? lsfLowAuth     : lsfHighAuth     };
+
+            uint32_t flags = line->getFieldU32(sfFlags);
+
+            if (flags & flagIssuerFreeze)
+            {
+                printf("trustXferAllowed: issuerFreeze\n");
+                return tecFROZEN;
+            }
+
+            // if called with more than one party then any party
+            // that has a noripple on the issuer side of their tl
+            // blocks any possible xfer
+            if (parties.size() > 1 && (flags & flagIssuerNoRipple))
+            {
+                printf("trustXferAllowed: issuerNoRipple\n");
+                return tecPATH_DRY;
+            }
+
+            // every party involved must be on an authed trustline if
+            // the issuer has specified lsfRequireAuth
+            if (requireAuth && !(flags & flagIssuerAuth))
+            {
+                printf("trustXferAllowed: issuerRequireAuth\n");
+                return tecNO_AUTH;
+            }
+        }
+    }
+
+    return tesSUCCESS;
+}
+
 [[nodiscard]] TER
 trustXferLockedBalance(
     ApplyView& view,

src/ripple/ledger/impl/View.cpp

@@ -961,114 +961,6 @@ bool isTrustDefault(
     return true;
 }   
 
-
-// Check if movement of a particular token between 1 or more accounts
-// (including unlocking) is forbidden by any flag or condition.
-// If parties contains 1 entry then noRipple is not a bar to xfer.
-// Part of featurePaychanAndEscrowForTokens, but can be callled without guard
-TER
-trustXferAllowed(
-    ReadView const& view,
-    std::vector<AccountID> const& parties,
-    Issue const& issue)
-{
-
-    if (isFakeXRP(issue.currency))
-        return tecNO_PERMISSION;
-
-    auto const sleIssuerAcc = view.read(keylet::account(issue.account));
-
-    bool lockedBalanceAllowed =
-        view.rules().enabled(featurePaychanAndEscrowForTokens);
-
-    // missing issuer is always a bar to xfer
-    if (!sleIssuerAcc)
-        return tecNO_ISSUER;
-
-    // issuer global freeze is always a bar to xfer
-    if (isGlobalFrozen(view, issue.account))
-        return tecFROZEN;
-
-    uint32_t issuerFlags = sleIssuerAcc->getFieldU32(sfFlags);
-
-    bool requireAuth = issuerFlags & lsfRequireAuth;
-
-    for (AccountID const& p: parties)
-    {
-        auto line = view.read(keylet::line(p, issue.account, issue.currency));
-        if (!line)
-        {
-            if (requireAuth)
-            {
-                // the line doesn't exist, i.e. it is in default state
-                // default state means the line has not been authed
-                // therefore if auth is required by issuer then
-                // this is now a bar to xfer
-                return tecNO_AUTH;
-            }
-
-            // missing line is a line in default state, this is not
-            // a general bar to xfer, however additional conditions
-            // do attach to completing an xfer into a default line
-            // but these are checked in trustXferLockedBalance at
-            // the point of transfer.
-            continue;
-        }
-
-        // sanity check the line, insane lines are a bar to xfer
-        {
-            // these "strange" old lines, if they even exist anymore are
-            // always a bar to xfer
-            if (line->getFieldAmount(sfLowLimit).getIssuer() ==
-                    line->getFieldAmount(sfHighLimit).getIssuer())
-                return tecINTERNAL;
-
-            if (line->isFieldPresent(sfLockedBalance))
-            {
-                if (!lockedBalanceAllowed)
-                    return tecINTERNAL;
-
-                STAmount lockedBalance = line->getFieldAmount(sfLockedBalance);
-                STAmount balance = line->getFieldAmount(sfBalance);
-
-                if (lockedBalance.getIssuer() != balance.getIssuer() ||
-                        lockedBalance.getCurrency() != balance.getCurrency())
-                    return tecINTERNAL;
-            }
-        }
-
-        // check the bars to xfer ... these are:
-        // any TL in the set has noRipple on the issuer's side
-        // any TL in the set has a freeze on the issuer's side
-        // any TL in the set has RequireAuth and the TL lacks lsf*Auth
-        {
-            bool pHigh = p > issue.account;
-
-            auto const flagIssuerNoRipple { pHigh ? lsfLowNoRipple : lsfHighNoRipple };
-            auto const flagIssuerFreeze   { pHigh ? lsfLowFreeze   : lsfHighFreeze   };
-            auto const flagIssuerAuth     { pHigh ? lsfLowAuth     : lsfHighAuth     };
-
-            uint32_t flags = line->getFieldU32(sfFlags);
-
-            if (flags & flagIssuerFreeze)
-                return tecFROZEN;
-
-            // if called with more than one party then any party
-            // that has a noripple on the issuer side of their tl
-            // blocks any possible xfer
-            if (parties.size() > 1 && (flags & flagIssuerNoRipple))
-                return tecPATH_DRY;
-
-            // every party involved must be on an authed trustline if
-            // the issuer has specified lsfRequireAuth
-            if (requireAuth && !(flags & flagIssuerAuth))
-                return tecNO_AUTH;
-        }
-    }
-
-    return tesSUCCESS;
-}
-
 TER
 trustXferLockedBalance(
     ApplyView& view,